Re: [PATCH 2/2] remote-curl: unquote incoming push-options

[Brandon Williams]

On 02/19, Jeff King wrote:
> The transport-helper protocol c-style quotes the value of
> any options passed to the helper via the "option  "
> directive. However, remote-curl doesn't actually unquote the
> push-option values, meaning that we will send the quoted
> version to the other side (whereas git-over-ssh would send
> the raw value).
> 
> The pack-protocol.txt documentation defines the push-options
> as a series of VCHARs, which excludes most characters that
> would need quoting. But:
> 
>   1. You can still see the bug with a valid push-option that
>  starts with a double-quote (since that triggers
>  quoting).
> 
>   2. We do currently handle any non-NUL characters correctly
>  in git-over-ssh. So even though the spec does not say
>  that we need to handle most quoted characters, it's
>  nice if our behavior is consistent between protocols.
> 
> There are two new tests: the "direct" one shows that this
> already works in the non-http case, and the http one covers
> this bugfix.

This seems like a fairly obvious fix.  If the value is quoted, unquote
it and send the unquoted value as a push-option, otherwise just send the
already unquoted value as a push-option.

Thanks for finding and fixing this :)

> 
> Reported-by: Jon Simons 
> Signed-off-by: Jeff King 
> ---
>  remote-curl.c   | 11 ++-
>  t/t5545-push-options.sh | 18 ++
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/remote-curl.c b/remote-curl.c
> index 6ec5352435..f5b3d22e26 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -13,6 +13,7 @@
>  #include "credential.h"
>  #include "sha1-array.h"
>  #include "send-pack.h"
> +#include "quote.h"
>  
>  static struct remote *remote;
>  /* always ends with a trailing slash */
> @@ -145,7 +146,15 @@ static int set_option(const char *name, const char 
> *value)
>   return -1;
>   return 0;
>   } else if (!strcmp(name, "push-option")) {
> - string_list_append(_options, value);
> + if (*value != '"')
> + string_list_append(_options, value);
> + else {
> + struct strbuf unquoted = STRBUF_INIT;
> + if (unquote_c_style(, value, NULL) < 0)
> + die("invalid quoting in push-option value");
> + string_list_append_nodup(_options,
> +  strbuf_detach(, 
> NULL));
> + }
>   return 0;
>  
>  #if LIBCURL_VERSION_NUM >= 0x070a08
> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
> index c64dee2127..b47a95871c 100755
> --- a/t/t5545-push-options.sh
> +++ b/t/t5545-push-options.sh
> @@ -217,6 +217,15 @@ test_expect_success 'invalid push option in config' '
>   test_refs master HEAD@{1}
>  '
>  
> +test_expect_success 'push options keep quoted characters intact (direct)' '
> + mk_repo_pair &&
> + git -C upstream config receive.advertisePushOptions true &&
> + test_commit -C workbench one &&
> + git -C workbench push --push-option="\"embedded quotes\"" up master &&
> + echo "\"embedded quotes\"" >expect &&
> + test_cmp expect upstream/.git/hooks/pre-receive.push_options
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd
>  
> @@ -260,6 +269,15 @@ test_expect_success 'push options work properly across 
> http' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'push options keep quoted characters intact (http)' '
> + mk_http_pair true &&
> +
> + test_commit -C test_http_clone one &&
> + git -C test_http_clone push --push-option="\"embedded quotes\"" origin 
> master &&
> + echo "\"embedded quotes\"" >expect &&
> + test_cmp expect 
> "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options
> +'
> +
>  stop_httpd
>  
>  test_done
> -- 
> 2.16.2.552.gea2a3cf654

-- 
Brandon Williams

[PATCH 2/2] remote-curl: unquote incoming push-options

[Jeff King]

The transport-helper protocol c-style quotes the value of
any options passed to the helper via the "option  "
directive. However, remote-curl doesn't actually unquote the
push-option values, meaning that we will send the quoted
version to the other side (whereas git-over-ssh would send
the raw value).

The pack-protocol.txt documentation defines the push-options
as a series of VCHARs, which excludes most characters that
would need quoting. But:

  1. You can still see the bug with a valid push-option that
 starts with a double-quote (since that triggers
 quoting).

  2. We do currently handle any non-NUL characters correctly
 in git-over-ssh. So even though the spec does not say
 that we need to handle most quoted characters, it's
 nice if our behavior is consistent between protocols.

There are two new tests: the "direct" one shows that this
already works in the non-http case, and the http one covers
this bugfix.

Reported-by: Jon Simons 
Signed-off-by: Jeff King 
---
 remote-curl.c   | 11 ++-
 t/t5545-push-options.sh | 18 ++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 6ec5352435..f5b3d22e26 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -13,6 +13,7 @@
 #include "credential.h"
 #include "sha1-array.h"
 #include "send-pack.h"
+#include "quote.h"
 
 static struct remote *remote;
 /* always ends with a trailing slash */
@@ -145,7 +146,15 @@ static int set_option(const char *name, const char *value)
return -1;
return 0;
} else if (!strcmp(name, "push-option")) {
-   string_list_append(_options, value);
+   if (*value != '"')
+   string_list_append(_options, value);
+   else {
+   struct strbuf unquoted = STRBUF_INIT;
+   if (unquote_c_style(, value, NULL) < 0)
+   die("invalid quoting in push-option value");
+   string_list_append_nodup(_options,
+strbuf_detach(, 
NULL));
+   }
return 0;
 
 #if LIBCURL_VERSION_NUM >= 0x070a08
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index c64dee2127..b47a95871c 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -217,6 +217,15 @@ test_expect_success 'invalid push option in config' '
test_refs master HEAD@{1}
 '
 
+test_expect_success 'push options keep quoted characters intact (direct)' '
+   mk_repo_pair &&
+   git -C upstream config receive.advertisePushOptions true &&
+   test_commit -C workbench one &&
+   git -C workbench push --push-option="\"embedded quotes\"" up master &&
+   echo "\"embedded quotes\"" >expect &&
+   test_cmp expect upstream/.git/hooks/pre-receive.push_options
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
@@ -260,6 +269,15 @@ test_expect_success 'push options work properly across 
http' '
test_cmp expect actual
 '
 
+test_expect_success 'push options keep quoted characters intact (http)' '
+   mk_http_pair true &&
+
+   test_commit -C test_http_clone one &&
+   git -C test_http_clone push --push-option="\"embedded quotes\"" origin 
master &&
+   echo "\"embedded quotes\"" >expect &&
+   test_cmp expect 
"$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options
+'
+
 stop_httpd
 
 test_done
-- 
2.16.2.552.gea2a3cf654