* [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
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).