| Message ID | 20200422012344.2051103-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | credential: fix matching URLs with multiple levels in path | expand |
On Wed, Apr 22, 2020 at 01:23:44AM +0000, brian m. carlson wrote: > 46fd7b3900 ("credential: allow wildcard patterns when matching config", > 2020-02-20) introduced support for matching credential helpers using > urlmatch. In doing so, it introduced code to percent-encode the paths > we get from the credential helper so that they could be effectively > matched by the urlmatch code. > > Unfortunately, that code had a bug: it percent-encoded the slashes in > the path, resulting in any URL path that contained multiple levels > (i.e., a directory component) not matching. > > We are currently the only caller of the percent-encoding code and could > simply change it not to encode slashes. However, this would be > surprising to other potential users who might want to use it and might > result in unwanted slashes appearing in the encoded value. > > So instead, let's add a flag to skip encoding slashes, which is the > behavior we want here, and use it when calling the code in this case. > Add a test for credential helper URLs using multiple slashes in the > path, which our test suite previously lacked. Thanks for the quick turnaround. The explanation makes sense. The patch leaves me with one question, though... > diff --git a/credential.c b/credential.c > index 108d9e183a..f0e55a27ac 100644 > --- a/credential.c > +++ b/credential.c > @@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out) > return; > strbuf_addf(out, "%s://", c->protocol); > if (c->username && *c->username) { > - strbuf_add_percentencode(out, c->username); > + strbuf_add_percentencode(out, c->username, STRBUF_PERCENTENCODE_PATH); > strbuf_addch(out, '@'); Wouldn't we want to keep encoding slashes in the username? > if (c->path) { > strbuf_addch(out, '/'); > - strbuf_add_percentencode(out, c->path); > + strbuf_add_percentencode(out, c->path, STRBUF_PERCENTENCODE_PATH); > } This hunk is the one I expected. > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh > index 5555a1524f..15eeef1dfd 100755 > --- a/t/t0300-credentials.sh > +++ b/t/t0300-credentials.sh > @@ -510,6 +510,24 @@ test_expect_success 'helpers can abort the process' ' > test_i18ncmp expect stderr > ' > > +test_expect_success 'helpers can fetch with multiple path components' ' > + test_unconfig credential.helper && > + test_config credential.https://example.com/foo/repo.git.helper "verbatim foo bar" && OK, you can't just use an argument to "check" because you want to set a specific config option, not just credential.helper. Would this test make sense a little higher in the file, below "match percent-encoded values" perhaps? > + echo url=https://example.com/foo/repo.git | git credential fill && What's this line doing? It will just do the same "check fill" as below, but without the stdout checking. Is it leftover debugging cruft? > + check fill <<-\EOF > + url=https://example.com/foo/repo.git > + -- > + protocol=https > + host=example.com > + username=foo > + password=bar > + -- > + verbatim: get > + verbatim: protocol=https > + verbatim: host=example.com And here we confirm that we got values from the "verbatim" helper. Good. -Peff
On 2020-04-22 at 04:16:14, Jeff King wrote: > On Wed, Apr 22, 2020 at 01:23:44AM +0000, brian m. carlson wrote: > > > 46fd7b3900 ("credential: allow wildcard patterns when matching config", > > 2020-02-20) introduced support for matching credential helpers using > > urlmatch. In doing so, it introduced code to percent-encode the paths > > we get from the credential helper so that they could be effectively > > matched by the urlmatch code. > > > > Unfortunately, that code had a bug: it percent-encoded the slashes in > > the path, resulting in any URL path that contained multiple levels > > (i.e., a directory component) not matching. > > > > We are currently the only caller of the percent-encoding code and could > > simply change it not to encode slashes. However, this would be > > surprising to other potential users who might want to use it and might > > result in unwanted slashes appearing in the encoded value. > > > > So instead, let's add a flag to skip encoding slashes, which is the > > behavior we want here, and use it when calling the code in this case. > > Add a test for credential helper URLs using multiple slashes in the > > path, which our test suite previously lacked. > > Thanks for the quick turnaround. The explanation makes sense. > > The patch leaves me with one question, though... > > > diff --git a/credential.c b/credential.c > > index 108d9e183a..f0e55a27ac 100644 > > --- a/credential.c > > +++ b/credential.c > > @@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out) > > return; > > strbuf_addf(out, "%s://", c->protocol); > > if (c->username && *c->username) { > > - strbuf_add_percentencode(out, c->username); > > + strbuf_add_percentencode(out, c->username, STRBUF_PERCENTENCODE_PATH); > > strbuf_addch(out, '@'); > > Wouldn't we want to keep encoding slashes in the username? Oh, you're right, we would. That makes my commit message shorter, even. > > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh > > index 5555a1524f..15eeef1dfd 100755 > > --- a/t/t0300-credentials.sh > > +++ b/t/t0300-credentials.sh > > @@ -510,6 +510,24 @@ test_expect_success 'helpers can abort the process' ' > > test_i18ncmp expect stderr > > ' > > > > +test_expect_success 'helpers can fetch with multiple path components' ' > > + test_unconfig credential.helper && > > + test_config credential.https://example.com/foo/repo.git.helper "verbatim foo bar" && > > OK, you can't just use an argument to "check" because you want to set a > specific config option, not just credential.helper. Would this test make > sense a little higher in the file, below "match percent-encoded values" > perhaps? I can hoist it, sure. > > + echo url=https://example.com/foo/repo.git | git credential fill && > > What's this line doing? It will just do the same "check fill" as > below, but without the stdout checking. Is it leftover debugging cruft? Yeah, it is. I'll rip it out in v2.
diff --git a/credential.c b/credential.c index 108d9e183a..f0e55a27ac 100644 --- a/credential.c +++ b/credential.c @@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out) return; strbuf_addf(out, "%s://", c->protocol); if (c->username && *c->username) { - strbuf_add_percentencode(out, c->username); + strbuf_add_percentencode(out, c->username, STRBUF_PERCENTENCODE_PATH); strbuf_addch(out, '@'); } if (c->host) strbuf_addstr(out, c->host); if (c->path) { strbuf_addch(out, '/'); - strbuf_add_percentencode(out, c->path); + strbuf_add_percentencode(out, c->path, STRBUF_PERCENTENCODE_PATH); } } diff --git a/strbuf.c b/strbuf.c index bb0065ccaf..da5a1c9ca4 100644 --- a/strbuf.c +++ b/strbuf.c @@ -479,15 +479,17 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src) } } -#define URL_UNSAFE_CHARS " <>\"%{}|\\^`:/?#[]@!$&'()*+,;=" +#define URL_UNSAFE_CHARS " <>\"%{}|\\^`:?#[]@!$&'()*+,;=" -void strbuf_add_percentencode(struct strbuf *dst, const char *src) +void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags) { size_t i, len = strlen(src); for (i = 0; i < len; i++) { unsigned char ch = src[i]; - if (ch <= 0x1F || ch >= 0x7F || strchr(URL_UNSAFE_CHARS, ch)) + if (ch <= 0x1F || ch >= 0x7F || + (ch == '/' && !(flags & STRBUF_PERCENTENCODE_PATH)) || + strchr(URL_UNSAFE_CHARS, ch)) strbuf_addf(dst, "%%%02X", (unsigned char)ch); else strbuf_addch(dst, ch); diff --git a/strbuf.h b/strbuf.h index ce8e49c0b2..651aedff57 100644 --- a/strbuf.h +++ b/strbuf.h @@ -378,11 +378,15 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, */ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src); +#define STRBUF_PERCENTENCODE_PATH 1 + /** * Append the contents of a string to a strbuf, percent-encoding any characters * that are needed to be encoded for a URL. + * + * If STRBUF_PERCENTENCODE_PATH is set in flags, don't percent-encode slashes. */ -void strbuf_add_percentencode(struct strbuf *dst, const char *src); +void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags); /** * Append the given byte size as a human-readable string (i.e. 12.23 KiB, diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 5555a1524f..15eeef1dfd 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -510,6 +510,24 @@ test_expect_success 'helpers can abort the process' ' test_i18ncmp expect stderr ' +test_expect_success 'helpers can fetch with multiple path components' ' + test_unconfig credential.helper && + test_config credential.https://example.com/foo/repo.git.helper "verbatim foo bar" && + echo url=https://example.com/foo/repo.git | git credential fill && + check fill <<-\EOF + url=https://example.com/foo/repo.git + -- + protocol=https + host=example.com + username=foo + password=bar + -- + verbatim: get + verbatim: protocol=https + verbatim: host=example.com + EOF +' + test_expect_success 'empty helper spec resets helper list' ' test_config credential.helper "verbatim file file" && check fill "" "verbatim cmdline cmdline" <<-\EOF
46fd7b3900 ("credential: allow wildcard patterns when matching config", 2020-02-20) introduced support for matching credential helpers using urlmatch. In doing so, it introduced code to percent-encode the paths we get from the credential helper so that they could be effectively matched by the urlmatch code. Unfortunately, that code had a bug: it percent-encoded the slashes in the path, resulting in any URL path that contained multiple levels (i.e., a directory component) not matching. We are currently the only caller of the percent-encoding code and could simply change it not to encode slashes. However, this would be surprising to other potential users who might want to use it and might result in unwanted slashes appearing in the encoded value. So instead, let's add a flag to skip encoding slashes, which is the behavior we want here, and use it when calling the code in this case. Add a test for credential helper URLs using multiple slashes in the path, which our test suite previously lacked. Reported-by: Ilya Tretyakov <it@it3xl.ru> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- credential.c | 4 ++-- strbuf.c | 8 +++++--- strbuf.h | 6 +++++- t/t0300-credentials.sh | 18 ++++++++++++++++++ 4 files changed, 30 insertions(+), 6 deletions(-)