* [PATCH 0/2] quoting bug sending push-options over http @ 2018-02-19 19:47 Jeff King 2018-02-19 19:48 ` [PATCH 1/2] t5545: factor out http repository setup Jeff King 2018-02-19 19:50 ` [PATCH 2/2] remote-curl: unquote incoming push-options Jeff King 0 siblings, 2 replies; 4+ messages in thread From: Jeff King @ 2018-02-19 19:47 UTC (permalink / raw) To: git; +Cc: Brandon Williams, Jon Simons 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] t5545: factor out http repository setup 2018-02-19 19:47 [PATCH 0/2] quoting bug sending push-options over http Jeff King @ 2018-02-19 19:48 ` Jeff King 2018-02-19 19:50 ` [PATCH 2/2] remote-curl: unquote incoming push-options Jeff King 1 sibling, 0 replies; 4+ messages in thread From: Jeff King @ 2018-02-19 19:48 UTC (permalink / raw) To: git; +Cc: Brandon Williams, Jon Simons 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(-) 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] remote-curl: unquote incoming push-options 2018-02-19 19:47 [PATCH 0/2] quoting bug sending push-options over http Jeff King 2018-02-19 19:48 ` [PATCH 1/2] t5545: factor out http repository setup Jeff King @ 2018-02-19 19:50 ` Jeff King 2018-02-20 19:05 ` Brandon Williams 1 sibling, 1 reply; 4+ messages in thread From: Jeff King @ 2018-02-19 19:50 UTC (permalink / raw) To: git; +Cc: Brandon Williams, Jon Simons 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(-) 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] remote-curl: unquote incoming push-options 2018-02-19 19:50 ` [PATCH 2/2] remote-curl: unquote incoming push-options Jeff King @ 2018-02-20 19:05 ` Brandon Williams 0 siblings, 0 replies; 4+ messages in thread From: Brandon Williams @ 2018-02-20 19:05 UTC (permalink / raw) To: Jeff King; +Cc: git, Jon Simons 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 <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 :) > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-20 19:05 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-19 19:47 [PATCH 0/2] quoting bug sending push-options over http Jeff King 2018-02-19 19:48 ` [PATCH 1/2] t5545: factor out http repository setup Jeff King 2018-02-19 19:50 ` [PATCH 2/2] remote-curl: unquote incoming push-options Jeff King 2018-02-20 19:05 ` Brandon Williams
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).