{"thread":{"id":"47875","subject":"[PATCH 0/2] quoting bug sending push-options over http","startedAt":"2018-02-19T19:47:17Z","lastAt":"2018-02-20T19:05:43Z","messageCount":4,"participants":["Jeff King","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"339636","messageId":"20180219194709.GA837@sigill.intra.peff.net","threadId":"47875","inReplyTo":null,"subject":"[PATCH 0/2] quoting bug sending push-options over http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-19T19:47:09Z","receivedAt":"2018-02-19T19:47:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes a small quoting problem in 511155db51 (remote-curl:\nallow push options, 2017-03-22). The interesting one is the second\npatch.\n\n  [1/2]: t5545: factor out http repository setup\n  [2/2]: remote-curl: unquote incoming push-options\n\n remote-curl.c           | 11 ++++++++++-\n t/t5545-push-options.sh | 40 +++++++++++++++++++++++++++++-----------\n 2 files changed, 39 insertions(+), 12 deletions(-)\n\n-Peff\n"},{"id":"339637","messageId":"20180219194844.GA988@sigill.intra.peff.net","threadId":"47875","inReplyTo":"20180219194709.GA837@sigill.intra.peff.net","subject":"[PATCH 1/2] t5545: factor out http repository setup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-19T19:48:44Z","receivedAt":"2018-02-19T19:48:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We repeat many lines of setup code in the two http tests,\nand further tests would need to repeat it again.  Let's\nfactor this out into a function.\n\nIncidentally, this also fixes an unlikely bug: if the httpd\nroot path contains a double-quote, our test_when_finished\nwould barf due to improper quoting (we escape the embedded\nquotes, but not the $, meaning we expand the variable before\nthe eval).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nArguably this setup could be done once and then reused by several tests,\nwhich would be a bit more efficient.  But the whole script is written in\nthis \"remake repos fresh\" style, so I didn't look into switching it.\n\n t/t5545-push-options.sh | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex 463783789c..c64dee2127 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -220,14 +220,20 @@ test_expect_success 'invalid push option in config' '\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-test_expect_success 'push option denied properly by http server' '\n+# set up http repository for fetching/pushing, with push options config\n+# bool set to $1\n+mk_http_pair () {\n \ttest_when_finished \"rm -rf test_http_clone\" &&\n-\ttest_when_finished \"rm -rf \\\"$HTTPD_DOCUMENT_ROOT_PATH\\\"/upstream.git\" &&\n+\ttest_when_finished 'rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git' &&\n \tmk_repo_pair &&\n-\tgit -C upstream config receive.advertisePushOptions false &&\n+\tgit -C upstream config receive.advertisePushOptions \"$1\" &&\n \tgit -C upstream config http.receivepack true &&\n \tcp -R upstream/.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git &&\n-\tgit clone \"$HTTPD_URL\"/smart/upstream test_http_clone &&\n+\tgit clone \"$HTTPD_URL\"/smart/upstream test_http_clone\n+}\n+\n+test_expect_success 'push option denied properly by http server' '\n+\tmk_http_pair false &&\n \ttest_commit -C test_http_clone one &&\n \ttest_must_fail git -C test_http_clone push --push-option=asdf origin master 2>actual &&\n \ttest_i18ngrep \"the receiving end does not support push options\" actual &&\n@@ -235,13 +241,7 @@ test_expect_success 'push option denied properly by http server' '\n '\n \n test_expect_success 'push options work properly across http' '\n-\ttest_when_finished \"rm -rf test_http_clone\" &&\n-\ttest_when_finished \"rm -rf \\\"$HTTPD_DOCUMENT_ROOT_PATH\\\"/upstream.git\" &&\n-\tmk_repo_pair &&\n-\tgit -C upstream config receive.advertisePushOptions true &&\n-\tgit -C upstream config http.receivepack true &&\n-\tcp -R upstream/.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git &&\n-\tgit clone \"$HTTPD_URL\"/smart/upstream test_http_clone &&\n+\tmk_http_pair true &&\n \n \ttest_commit -C test_http_clone one &&\n \tgit -C test_http_clone push origin master &&\n-- \n2.16.2.552.gea2a3cf654\n\n"},{"id":"339638","messageId":"20180219195014.GB988@sigill.intra.peff.net","threadId":"47875","inReplyTo":"20180219194709.GA837@sigill.intra.peff.net","subject":"[PATCH 2/2] remote-curl: unquote incoming push-options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-19T19:50:14Z","receivedAt":"2018-02-19T19:50:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The transport-helper protocol c-style quotes the value of\nany options passed to the helper via the \"option <key> <value>\"\ndirective. However, remote-curl doesn't actually unquote the\npush-option values, meaning that we will send the quoted\nversion to the other side (whereas git-over-ssh would send\nthe raw value).\n\nThe pack-protocol.txt documentation defines the push-options\nas a series of VCHARs, which excludes most characters that\nwould need quoting. But:\n\n  1. You can still see the bug with a valid push-option that\n     starts with a double-quote (since that triggers\n     quoting).\n\n  2. We do currently handle any non-NUL characters correctly\n     in git-over-ssh. So even though the spec does not say\n     that we need to handle most quoted characters, it's\n     nice if our behavior is consistent between protocols.\n\nThere are two new tests: the \"direct\" one shows that this\nalready works in the non-http case, and the http one covers\nthis bugfix.\n\nReported-by: Jon Simons <jon@jonsimons.org>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote-curl.c           | 11 ++++++++++-\n t/t5545-push-options.sh | 18 ++++++++++++++++++\n 2 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 6ec5352435..f5b3d22e26 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -13,6 +13,7 @@\n #include \"credential.h\"\n #include \"sha1-array.h\"\n #include \"send-pack.h\"\n+#include \"quote.h\"\n \n static struct remote *remote;\n /* always ends with a trailing slash */\n@@ -145,7 +146,15 @@ static int set_option(const char *name, const char *value)\n \t\t\treturn -1;\n \t\treturn 0;\n \t} else if (!strcmp(name, \"push-option\")) {\n-\t\tstring_list_append(&options.push_options, value);\n+\t\tif (*value != '\"')\n+\t\t\tstring_list_append(&options.push_options, value);\n+\t\telse {\n+\t\t\tstruct strbuf unquoted = STRBUF_INIT;\n+\t\t\tif (unquote_c_style(&unquoted, value, NULL) < 0)\n+\t\t\t\tdie(\"invalid quoting in push-option value\");\n+\t\t\tstring_list_append_nodup(&options.push_options,\n+\t\t\t\t\t\t strbuf_detach(&unquoted, NULL));\n+\t\t}\n \t\treturn 0;\n \n #if LIBCURL_VERSION_NUM >= 0x070a08\ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex c64dee2127..b47a95871c 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -217,6 +217,15 @@ test_expect_success 'invalid push option in config' '\n \ttest_refs master HEAD@{1}\n '\n \n+test_expect_success 'push options keep quoted characters intact (direct)' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\ttest_commit -C workbench one &&\n+\tgit -C workbench push --push-option=\"\\\"embedded quotes\\\"\" up master &&\n+\techo \"\\\"embedded quotes\\\"\" >expect &&\n+\ttest_cmp expect upstream/.git/hooks/pre-receive.push_options\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n@@ -260,6 +269,15 @@ test_expect_success 'push options work properly across http' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'push options keep quoted characters intact (http)' '\n+\tmk_http_pair true &&\n+\n+\ttest_commit -C test_http_clone one &&\n+\tgit -C test_http_clone push --push-option=\"\\\"embedded quotes\\\"\" origin master &&\n+\techo \"\\\"embedded quotes\\\"\" >expect &&\n+\ttest_cmp expect \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git/hooks/pre-receive.push_options\n+'\n+\n stop_httpd\n \n test_done\n-- \n2.16.2.552.gea2a3cf654\n"},{"id":"339726","messageId":"20180220190535.GB201604@google.com","threadId":"47875","inReplyTo":"20180219195014.GB988@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] remote-curl: unquote incoming push-options","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-02-20T19:05:35Z","receivedAt":"2018-02-20T19:05:43Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/19, Jeff King wrote:\n> The transport-helper protocol c-style quotes the value of\n> any options passed to the helper via the \"option <key> <value>\"\n> directive. However, remote-curl doesn't actually unquote the\n> push-option values, meaning that we will send the quoted\n> version to the other side (whereas git-over-ssh would send\n> the raw value).\n> \n> The pack-protocol.txt documentation defines the push-options\n> as a series of VCHARs, which excludes most characters that\n> would need quoting. But:\n> \n>   1. You can still see the bug with a valid push-option that\n>      starts with a double-quote (since that triggers\n>      quoting).\n> \n>   2. We do currently handle any non-NUL characters correctly\n>      in git-over-ssh. So even though the spec does not say\n>      that we need to handle most quoted characters, it's\n>      nice if our behavior is consistent between protocols.\n> \n> There are two new tests: the \"direct\" one shows that this\n> already works in the non-http case, and the http one covers\n> this bugfix.\n\nThis seems like a fairly obvious fix.  If the value is quoted, unquote\nit and send the unquoted value as a push-option, otherwise just send the\nalready unquoted value as a push-option.\n\nThanks for finding and fixing this :)\n\n> \n> Reported-by: Jon Simons <jon@jonsimons.org>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  remote-curl.c           | 11 ++++++++++-\n>  t/t5545-push-options.sh | 18 ++++++++++++++++++\n>  2 files changed, 28 insertions(+), 1 deletion(-)\n> \n> diff --git a/remote-curl.c b/remote-curl.c\n> index 6ec5352435..f5b3d22e26 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -13,6 +13,7 @@\n>  #include \"credential.h\"\n>  #include \"sha1-array.h\"\n>  #include \"send-pack.h\"\n> +#include \"quote.h\"\n>  \n>  static struct remote *remote;\n>  /* always ends with a trailing slash */\n> @@ -145,7 +146,15 @@ static int set_option(const char *name, const char *value)\n>  \t\t\treturn -1;\n>  \t\treturn 0;\n>  \t} else if (!strcmp(name, \"push-option\")) {\n> -\t\tstring_list_append(&options.push_options, value);\n> +\t\tif (*value != '\"')\n> +\t\t\tstring_list_append(&options.push_options, value);\n> +\t\telse {\n> +\t\t\tstruct strbuf unquoted = STRBUF_INIT;\n> +\t\t\tif (unquote_c_style(&unquoted, value, NULL) < 0)\n> +\t\t\t\tdie(\"invalid quoting in push-option value\");\n> +\t\t\tstring_list_append_nodup(&options.push_options,\n> +\t\t\t\t\t\t strbuf_detach(&unquoted, NULL));\n> +\t\t}\n>  \t\treturn 0;\n>  \n>  #if LIBCURL_VERSION_NUM >= 0x070a08\n> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\n> index c64dee2127..b47a95871c 100755\n> --- a/t/t5545-push-options.sh\n> +++ b/t/t5545-push-options.sh\n> @@ -217,6 +217,15 @@ test_expect_success 'invalid push option in config' '\n>  \ttest_refs master HEAD@{1}\n>  '\n>  \n> +test_expect_success 'push options keep quoted characters intact (direct)' '\n> +\tmk_repo_pair &&\n> +\tgit -C upstream config receive.advertisePushOptions true &&\n> +\ttest_commit -C workbench one &&\n> +\tgit -C workbench push --push-option=\"\\\"embedded quotes\\\"\" up master &&\n> +\techo \"\\\"embedded quotes\\\"\" >expect &&\n> +\ttest_cmp expect upstream/.git/hooks/pre-receive.push_options\n> +'\n> +\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n>  start_httpd\n>  \n> @@ -260,6 +269,15 @@ test_expect_success 'push options work properly across http' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'push options keep quoted characters intact (http)' '\n> +\tmk_http_pair true &&\n> +\n> +\ttest_commit -C test_http_clone one &&\n> +\tgit -C test_http_clone push --push-option=\"\\\"embedded quotes\\\"\" origin master &&\n> +\techo \"\\\"embedded quotes\\\"\" >expect &&\n> +\ttest_cmp expect \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git/hooks/pre-receive.push_options\n> +'\n> +\n>  stop_httpd\n>  \n>  test_done\n> -- \n> 2.16.2.552.gea2a3cf654\n\n-- \nBrandon Williams\n"}]}