threads / patch / 47875

patch, 2 partsquoting bug sending push-options over http

Subject: [PATCH 0/2] quoting bug sending push-options over http

## tl;dr

4 messages between Feb 19, 2018 and Feb 20, 2018. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jeff King· Feb 19, 2018, 19:47 UTC · lore

This series fixes a small quoting problem in 511155db51 (remote-curl: allow push options, 2017-03-22). The interesting one is the second patch.

  [1/2]: t5545: factor out http repository setup
  [2/2]: remote-curl: unquote incoming push-options

 remote-curl.c           | 11 ++++++++++-
 t/t5545-push-options.sh | 40 +++++++++++++++++++++++++++++-----------
 2 files changed, 39 insertions(+), 12 deletions(-)

-Peff

Jeff King· Feb 19, 2018, 19:48 UTC · re: Jeff King · lore

[PATCH 1/2] t5545: factor out http repository setup

We repeat many lines of setup code in the two http tests, and further tests would need to repeat it again. Let's factor this out into a function.

Incidentally, this also fixes an unlikely bug: if the httpd root path contains a double-quote, our test_when_finished would barf due to improper quoting (we escape the embedded quotes, but not the $, meaning we expand the variable before the eval).

Signed-off-by: Jeff King <peff@peff.net>
---
Arguably this setup could be done once and then reused by several tests,
which would be a bit more efficient.  But the whole script is written in
this "remake repos fresh" style, so I didn't look into switching it.

 t/t5545-push-options.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Show changes to t/t5545-push-options.sh +11 −11

diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 463783789c..c64dee2127 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -220,14 +220,20 @@ test_expect_success 'invalid push option in config' '
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-test_expect_success 'push option denied properly by http server' '
+# set up http repository for fetching/pushing, with push options config
+# bool set to $1
+mk_http_pair () {
 	test_when_finished "rm -rf test_http_clone" &&
-	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
+	test_when_finished 'rm -rf "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git' &&
 	mk_repo_pair &&
-	git -C upstream config receive.advertisePushOptions false &&
+	git -C upstream config receive.advertisePushOptions "$1" &&
 	git -C upstream config http.receivepack true &&
 	cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git &&
-	git clone "$HTTPD_URL"/smart/upstream test_http_clone &&
+	git clone "$HTTPD_URL"/smart/upstream test_http_clone
+}
+
+test_expect_success 'push option denied properly by http server' '
+	mk_http_pair false &&
 	test_commit -C test_http_clone one &&
 	test_must_fail git -C test_http_clone push --push-option=asdf origin master 2>actual &&
 	test_i18ngrep "the receiving end does not support push options" actual &&
@@ -235,13 +241,7 @@ test_expect_success 'push option denied properly by http server' '
 '
 
 test_expect_success 'push options work properly across http' '
-	test_when_finished "rm -rf test_http_clone" &&
-	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
-	mk_repo_pair &&
-	git -C upstream config receive.advertisePushOptions true &&
-	git -C upstream config http.receivepack true &&
-	cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git &&
-	git clone "$HTTPD_URL"/smart/upstream test_http_clone &&
+	mk_http_pair true &&
 
 	test_commit -C test_http_clone one &&
 	git -C test_http_clone push origin master &&

-- 
2.16.2.552.gea2a3cf654

Jeff King· Feb 19, 2018, 19:50 UTC · re: Jeff King · lore

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

The transport-helper protocol c-style quotes the value of any options passed to the helper via the "option <key> <value>" 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 <jon@jonsimons.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c           | 11 ++++++++++-
 t/t5545-push-options.sh | 18 ++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

Show changes to 2 files +28 −1

remote-curl.c, t/t5545-push-options.sh

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.push_options, value);
+		if (*value != '"')
+			string_list_append(&options.push_options, value);
+		else {
+			struct strbuf unquoted = STRBUF_INIT;
+			if (unquote_c_style(&unquoted, value, NULL) < 0)
+				die("invalid quoting in push-option value");
+			string_list_append_nodup(&options.push_options,
+						 strbuf_detach(&unquoted, 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· Feb 20, 2018, 19:05 UTC · re: Jeff King · lore

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

On 02/19, Jeff King wrote:

Show 23 quoted lines

> The transport-helper protocol c-style quotes the value of
> any options passed to the helper via the "option <key> <value>"
> 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 :)

Show 75 quoted lines

> 
> Reported-by: Jon Simons <jon@jonsimons.org>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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.push_options, value);
> +		if (*value != '"')
> +			string_list_append(&options.push_options, value);
> +		else {
> +			struct strbuf unquoted = STRBUF_INIT;
> +			if (unquote_c_style(&unquoted, value, NULL) < 0)
> +				die("invalid quoting in push-option value");
> +			string_list_append_nodup(&options.push_options,
> +						 strbuf_detach(&unquoted, 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