Re: [PATCH 2/2] remote-curl: unquote incoming push-options
- From
- Brandon Williams <bmwill@google.com>
- Date
- Feb 20, 2018, 19:05 UTC
- Message-ID
- <20180220190535.GB201604@google.com>
- In-Reply-To
- <20180219195014.GB988@sigill.intra.peff.net>
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