# Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version 20 messages from 2020-04-21 to 2020-04-27. Participants: Ilya Tretyakov, Jeff King, brian m. carlson, Jonathan Nieder, Johannes Schindelin, Carlo Marcelo Arenas Belón, Junio C Hamano, Eric Sunshine. Thread: https://gitlist.dev/t/53281 ## Ilya Tretyakov, 2020-04-21 22:31 Subject: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version Message-ID: URL: https://gitlist.dev/e/CAOrRacVviJP3w98-%3DQpFKYp630cN3gZQYnvAWZXeKqZRk2UDXg%40mail.gmail.com ``` Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. For example, if we have a "/my-proj/" part in the repository URL. The following configuration doesn't invoke a credential helper script in 2.26.1 version of Git but invokes in 2.24.1.2. [credential "https://git.exaple.com/my-proj/my-repo.git"] helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh' provide repo_b A workaround here is to cut the URL to a root view. For example, we can use https://git.exaple.com/ instead of https://git.exaple.com/my-proj/ or https://git.exaple.com/my-proj/my-repo.git as below. [credential "https://git.exaple.com/"] helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh' provide repo_b BTW, the following configuration still works in all Git versions. [credential "https://foo.bar/my-repo.git"] helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh' provide repo_a If this would be possible, please let me know if this could be fixed in future Git versions. Or that I have to adopt my tools, described below, to this new configuration limitation. JFYI, I have a tool to synchronize remote Git repositories. This is when two remote repositories on different servers behave as a single repository. - https://github.com/it3xl/git-repo-sync I often use this tool with my own bash credential helper on different automation servers. - https://github.com/it3xl/bash-git-credential-helper And now, my Git-credential helper isn't invoked for repositories that have additional parts in their URLs. Of course, I've found a workaround but some people may want to configure more specific credentials for specific URL-s on the same domain. I am aware of the latest vulnerability discovered in the Git-credential helper functionality. I hope this was the cause of the trouble I described here. )) Kind Regards, Ilya it3xl.ru ``` ## Jeff King, 2020-04-21 22:58 Subject: Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version Message-ID: <20200421225837.GB3515235@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200421225837.GB3515235%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Apr 22, 2020 at 01:31:46AM +0300, Ilya Tretyakov wrote: > Credential helpers are no longer invoked in case of having sub-folder > parts in a repository URL. > > For example, if we have a "/my-proj/" part in the repository URL. > The following configuration doesn't invoke a credential helper script > in 2.26.1 version of Git but invokes in 2.24.1.2. > > [credential "https://git.exaple.com/my-proj/my-repo.git"] > helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh' > provide repo_b This is unrelated to the recent helper fixes in v2.26.x. Here's a simple reproduction: url=https://git.example.com/my-proj/my-repo.git echo url=$url | GIT_TERMINAL_PROMPT=0 \ ./git \ -c credential.helper= \ -c credential.$url.helper='!echo username=foo; echo password=bar;:' \ credential fill which should print a filled credential (with "foo/bar"), but will fail with recent versions. It bisects to brian's 46fd7b3900 (credential: allow wildcard patterns when matching config, 2020-02-20). -Peff ``` ## brian m. carlson, 2020-04-22 01:09 Subject: Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version Message-ID: <20200422010943.GD6465@camp.crustytoothpaste.net> URL: https://gitlist.dev/e/20200422010943.GD6465%40camp.crustytoothpaste.net In-Reply-To: <20200421225837.GB3515235@coredump.intra.peff.net> ``` On 2020-04-21 at 22:58:37, Jeff King wrote: > On Wed, Apr 22, 2020 at 01:31:46AM +0300, Ilya Tretyakov wrote: > > > Credential helpers are no longer invoked in case of having sub-folder > > parts in a repository URL. > > > > For example, if we have a "/my-proj/" part in the repository URL. > > The following configuration doesn't invoke a credential helper script > > in 2.26.1 version of Git but invokes in 2.24.1.2. > > > > [credential "https://git.exaple.com/my-proj/my-repo.git"] > > helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh' > > provide repo_b > > This is unrelated to the recent helper fixes in v2.26.x. Here's a simple > reproduction: > > url=https://git.example.com/my-proj/my-repo.git > echo url=$url | > GIT_TERMINAL_PROMPT=0 \ > ./git \ > -c credential.helper= \ > -c credential.$url.helper='!echo username=foo; echo password=bar;:' \ > credential fill > > which should print a filled credential (with "foo/bar"), but will fail > with recent versions. It bisects to brian's 46fd7b3900 (credential: > allow wildcard patterns when matching config, 2020-02-20). Yeah, I can reproduce this. It looks like what's happening is that we're percent-encoding the slash in the paths as %2f, which of course isn't going to match in the urlmatch code. We probably need to tell the percent encoding function not to encode slashes in this case. I'm testing a patch now and hope to have it on the list a little later this evening. Thanks for reporting and bisecting, and sorry for the breakage. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 ``` ## brian m. carlson, 2020-04-22 01:23 Subject: [PATCH] credential: fix matching URLs with multiple levels in path Message-ID: <20200422012344.2051103-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20200422012344.2051103-1-sandals%40crustytoothpaste.net In-Reply-To: <20200421225837.GB3515235@coredump.intra.peff.net> ``` 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 Signed-off-by: brian m. carlson --- credential.c | 4 ++-- strbuf.c | 8 +++++--- strbuf.h | 6 +++++- t/t0300-credentials.sh | 18 ++++++++++++++++++ 4 files changed, 30 insertions(+), 6 deletions(-) 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 ``` ## Jonathan Nieder, 2020-04-22 01:28 Subject: Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version Message-ID: <20200422012817.GD103469@google.com> URL: https://gitlist.dev/e/20200422012817.GD103469%40google.com In-Reply-To: <20200422010943.GD6465@camp.crustytoothpaste.net> ``` brian m. carlson wrote: > On 2020-04-21 at 22:58:37, Jeff King wrote: >> This is unrelated to the recent helper fixes in v2.26.x. Here's a simple >> reproduction: >> >> url=https://git.example.com/my-proj/my-repo.git >> echo url=$url | >> GIT_TERMINAL_PROMPT=0 \ >> ./git \ >> -c credential.helper= \ >> -c credential.$url.helper='!echo username=foo; echo password=bar;:' \ >> credential fill >> >> which should print a filled credential (with "foo/bar"), but will fail >> with recent versions. It bisects to brian's 46fd7b3900 (credential: >> allow wildcard patterns when matching config, 2020-02-20). > > Yeah, I can reproduce this. It looks like what's happening is that > we're percent-encoding the slash in the paths as %2f, which of course > isn't going to match in the urlmatch code. We probably need to tell the > percent encoding function not to encode slashes in this case. > > I'm testing a patch now and hope to have it on the list a little later > this evening. Thanks for reporting and bisecting, and sorry for the > breakage. Thanks. Here's another (though I haven't tried bisecting yet): echo url='https://github.com/git/git' | GIT_TERMINAL_PROMPT=0 \ git -c credential.helper= \ -c credential.github.com.helper='!echo username=foo; echo password=bar;:' \ credential fill produces fatal: could not read Username for 'https://github.com': terminal prompts disabled Jonathan ``` ## Jeff King, 2020-04-22 01:36 Subject: Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version Message-ID: <20200422013629.GA3559880@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200422013629.GA3559880%40coredump.intra.peff.net In-Reply-To: <20200422012817.GD103469@google.com> ``` On Tue, Apr 21, 2020 at 06:28:17PM -0700, Jonathan Nieder wrote: > Thanks. Here's another (though I haven't tried bisecting yet): > > echo url='https://github.com/git/git' | > GIT_TERMINAL_PROMPT=0 \ > git -c credential.helper= \ > -c credential.github.com.helper='!echo username=foo; echo password=bar;:' \ > credential fill > > produces > > fatal: could not read Username for 'https://github.com': terminal prompts disabled It's almost certainly the same commit. The credential_match() function would have said "aha, there is no protocol in the pattern, so match any protocol". But now we are trying to match full URLs. TBH, I'm not sure if the original was actually sane (especially in light of all of the recent confusion around missing protocols). -Peff ``` ## brian m. carlson, 2020-04-22 02:20 Subject: Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version Message-ID: <20200422022020.GF6465@camp.crustytoothpaste.net> URL: https://gitlist.dev/e/20200422022020.GF6465%40camp.crustytoothpaste.net In-Reply-To: <20200422012817.GD103469@google.com> ``` On 2020-04-22 at 01:28:17, Jonathan Nieder wrote: > brian m. carlson wrote: > > On 2020-04-21 at 22:58:37, Jeff King wrote: > > >> This is unrelated to the recent helper fixes in v2.26.x. Here's a simple > >> reproduction: > >> > >> url=https://git.example.com/my-proj/my-repo.git > >> echo url=$url | > >> GIT_TERMINAL_PROMPT=0 \ > >> ./git \ > >> -c credential.helper= \ > >> -c credential.$url.helper='!echo username=foo; echo password=bar;:' \ > >> credential fill > >> > >> which should print a filled credential (with "foo/bar"), but will fail > >> with recent versions. It bisects to brian's 46fd7b3900 (credential: > >> allow wildcard patterns when matching config, 2020-02-20). > > > > Yeah, I can reproduce this. It looks like what's happening is that > > we're percent-encoding the slash in the paths as %2f, which of course > > isn't going to match in the urlmatch code. We probably need to tell the > > percent encoding function not to encode slashes in this case. > > > > I'm testing a patch now and hope to have it on the list a little later > > this evening. Thanks for reporting and bisecting, and sorry for the > > breakage. > > Thanks. Here's another (though I haven't tried bisecting yet): > > echo url='https://github.com/git/git' | > GIT_TERMINAL_PROMPT=0 \ > git -c credential.helper= \ > -c credential.github.com.helper='!echo username=foo; echo password=bar;:' \ > credential fill gitcredentials(7) says the following: Git considers each credential to have a context defined by a URL. This context is used to look up context-specific configuration, and is passed to any helpers, which may use it as an index into secure storage. I'm not sure a hostname qualifies as a URL in this case. So while my patch did break this, I don't believe it's ever been documented to actually work and was an artifact of our implementation (along with "credential./git/git.helper" and "credential.https://.helper"). I've also never seen this syntax used in the wild, but maybe I'm not looking in the right places. I don't think we can shoehorn it into urlmatch, since that would break compatibility with the `http.*` config options, so I think we'd have to revert the entire feature if we want to preserve it. I think I'd prefer to leave things as it is since it seems uncommon and there are easy alternatives, but if folks prefer, I can send a patch to revert the urlmatch feature. I will likely not be online tomorrow (Wednesday), so if folks decide they want a revert, I can send that out Thursday afternoon (GMT-05:00). -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 ``` ## Jeff King, 2020-04-22 04:06 Subject: Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version Message-ID: <20200422040644.GC3559880@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200422040644.GC3559880%40coredump.intra.peff.net In-Reply-To: <20200422022020.GF6465@camp.crustytoothpaste.net> ``` On Wed, Apr 22, 2020 at 02:20:20AM +0000, brian m. carlson wrote: > > Thanks. Here's another (though I haven't tried bisecting yet): > > > > echo url='https://github.com/git/git' | > > GIT_TERMINAL_PROMPT=0 \ > > git -c credential.helper= \ > > -c credential.github.com.helper='!echo username=foo; echo password=bar;:' \ > > credential fill > > gitcredentials(7) says the following: > > Git considers each credential to have a context defined by a URL. > This context is used to look up context-specific configuration, and is > passed to any helpers, which may use it as an index into secure > storage. > > I'm not sure a hostname qualifies as a URL in this case. So while my > patch did break this, I don't believe it's ever been documented to > actually work and was an artifact of our implementation (along with > "credential./git/git.helper" and "credential.https://.helper"). I've > also never seen this syntax used in the wild, but maybe I'm not looking > in the right places. I'm pretty sure it was an intended use case, though it is a natural outcome of the credential_match() strategy of "unspecified things match anything". I'd suspect that anybody relying on it is doing so unintentionally, and just forgot to put the protocol field in. Though I suppose doing so would let you cover http/https in a single block. At any rate, even in versions _without_ your patch, that became a hard error in this week's release. In v2.24.3, for example: $ echo url=https://anyhost.example.com | git -c credential.example.com.username=foo credential fill warning: url has no scheme: example.com fatal: credential url cannot be parsed: example.com because we're relying there on credential_from_url() to parse the config credentials, too. After your patch, we use the http-config machinery, which simply doesn't match. > I don't think we can shoehorn it into urlmatch, since that would break > compatibility with the `http.*` config options, so I think we'd have to > revert the entire feature if we want to preserve it. I think I'd prefer > to leave things as it is since it seems uncommon and there are easy > alternatives, but if folks prefer, I can send a patch to revert the > urlmatch feature. I agree that we should leave it. Aside from the dual http/https thing (which _hopefully_ is rare these days as https become more of a standard), I don't think it has a legitimate use case. And I think we should be pushing users to be a bit more careful with their url config. -Peff ``` ## Jeff King, 2020-04-22 04:16 Subject: Re: [PATCH] credential: fix matching URLs with multiple levels in path Message-ID: <20200422041614.GD3559880@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200422041614.GD3559880%40coredump.intra.peff.net In-Reply-To: <20200422012344.2051103-1-sandals@crustytoothpaste.net> ``` 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 ``` ## brian m. carlson, 2020-04-22 18:45 Subject: Re: [PATCH] credential: fix matching URLs with multiple levels in path Message-ID: <20200422184549.GH6465@camp.crustytoothpaste.net> URL: https://gitlist.dev/e/20200422184549.GH6465%40camp.crustytoothpaste.net In-Reply-To: <20200422041614.GD3559880@coredump.intra.peff.net> ``` 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. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 ``` ## Johannes Schindelin, 2020-04-22 19:20 Subject: Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.2004222117400.18039%40tvgsbejvaqbjf.bet In-Reply-To: <20200422040644.GC3559880@coredump.intra.peff.net> ``` Hi, On Wed, 22 Apr 2020, Jeff King wrote: > On Wed, Apr 22, 2020 at 02:20:20AM +0000, brian m. carlson wrote: > > > > Thanks. Here's another (though I haven't tried bisecting yet): > > > > > > echo url='https://github.com/git/git' | > > > GIT_TERMINAL_PROMPT=0 \ > > > git -c credential.helper= \ > > > -c credential.github.com.helper='!echo username=foo; echo password=bar;:' \ > > > credential fill > > > > gitcredentials(7) says the following: > > > > Git considers each credential to have a context defined by a URL. > > This context is used to look up context-specific configuration, and is > > passed to any helpers, which may use it as an index into secure > > storage. > > > > I'm not sure a hostname qualifies as a URL in this case. So while my > > patch did break this, I don't believe it's ever been documented to > > actually work and was an artifact of our implementation (along with > > "credential./git/git.helper" and "credential.https://.helper"). I've > > also never seen this syntax used in the wild, but maybe I'm not looking > > in the right places. > > I'm pretty sure it was an intended use case, though it is a natural > outcome of the credential_match() strategy of "unspecified things match > anything". I'd suspect that anybody relying on it is doing so > unintentionally, and just forgot to put the protocol field in. Though I > suppose doing so would let you cover http/https in a single block. > > At any rate, even in versions _without_ your patch, that became a hard > error in this week's release. In v2.24.3, for example: > > $ echo url=https://anyhost.example.com | > git -c credential.example.com.username=foo credential fill > warning: url has no scheme: example.com > fatal: credential url cannot be parsed: example.com > > because we're relying there on credential_from_url() to parse the config > credentials, too. After your patch, we use the http-config machinery, > which simply doesn't match. This affects also pre-v2.26.* versions. One fallout is that some GitHub Desktop users reported that they cannot fetch anymore: https://github.com/desktop/desktop/issues/9597 I _think_ I have a good fix for this, and am only waiting for the PR builds at https://github.com/gitgitgadget/git/pull/615 to finish before I will submit that patch series for review. Ideally, I will integrate these patches into some MinGit backports tonight, still, so that GitHub Desktop can roll out another release to avoid many more reports. Therefore, I hope for quick reviews ;-) Ciao, Dscho > > > I don't think we can shoehorn it into urlmatch, since that would break > > compatibility with the `http.*` config options, so I think we'd have to > > revert the entire feature if we want to preserve it. I think I'd prefer > > to leave things as it is since it seems uncommon and there are easy > > alternatives, but if folks prefer, I can send a patch to revert the > > urlmatch feature. > > I agree that we should leave it. Aside from the dual http/https thing > (which _hopefully_ is rare these days as https become more of a > standard), I don't think it has a legitimate use case. And I think we > should be pushing users to be a bit more careful with their url config. > > -Peff > > ``` ## brian m. carlson, 2020-04-22 19:51 Subject: [PATCH v2] credential: fix matching URLs with multiple levels in path Message-ID: <20200422195109.2224035-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20200422195109.2224035-1-sandals%40crustytoothpaste.net In-Reply-To: <20200421225837.GB3515235@coredump.intra.peff.net> ``` 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, we still want to encode slashes in the username component, so we need to have both behaviors available. 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, as well as one ensuring that we handle usernames with slashes gracefully. Reported-by: Ilya Tretyakov Signed-off-by: brian m. carlson --- Changes from v1: * Continue to encode slashes in usernames. * Add a test for encoding slashes in usernames. * Hoist existing tests near the other percent-encoding tests. * Update commit message. * Remove debugging information. credential.c | 4 ++-- strbuf.c | 8 +++++--- strbuf.h | 6 +++++- t/t0300-credentials.sh | 30 ++++++++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/credential.c b/credential.c index 108d9e183a..6658cd0860 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, 0); 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..87267a7aac 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -366,6 +366,36 @@ test_expect_success 'match percent-encoded values' ' EOF ' +test_expect_success 'match percent-encoded values in username' ' + test_config credential.https://user%2fname@example.com/foo/bar.git.helper "$HELPER" && + check fill <<-\EOF + url=https://user%2fname@example.com/foo/bar.git + -- + protocol=https + host=example.com + username=foo + password=bar + -- + EOF +' + +test_expect_success 'fetch with multiple path components' ' + test_unconfig credential.helper && + test_config credential.https://example.com/foo/repo.git.helper "verbatim foo bar" && + 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 'pull username from config' ' test_config credential.https://example.com.username foo && check fill <<-\EOF ``` ## Jeff King, 2020-04-22 20:04 Subject: Re: [PATCH v2] credential: fix matching URLs with multiple levels in path Message-ID: <20200422200424.GF558336@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200422200424.GF558336%40coredump.intra.peff.net In-Reply-To: <20200422195109.2224035-1-sandals@crustytoothpaste.net> ``` On Wed, Apr 22, 2020 at 07:51:09PM +0000, brian m. carlson wrote: > Changes from v1: > * Continue to encode slashes in usernames. > * Add a test for encoding slashes in usernames. > * Hoist existing tests near the other percent-encoding tests. > * Update commit message. > * Remove debugging information. Thank for an easy-to-read explanation and patch (as usual). This version looks good to me. I think there's still an open question on: [credential "no-scheme.example.com"] config sections. I feel like that's something we never really intended to support and should discourage, but it seems as though it may be in wide use. But it's definitely a separate patch, and it sounds like Dscho is working on it. -Peff ``` ## Carlo Marcelo Arenas Belón, 2020-04-24 04:50 Subject: Re: [PATCH v2] credential: fix matching URLs with multiple levels in path Message-ID: <20200424045030.GA44448@Carlos-MBP> URL: https://gitlist.dev/e/20200424045030.GA44448%40Carlos-MBP In-Reply-To: <20200422195109.2224035-1-sandals@crustytoothpaste.net> ``` On Wed, Apr 22, 2020 at 07:51:09PM +0000, brian m. carlson wrote: > --- 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. > */ wouldn't it be better to call this STRBUF_PERCENTENCODE_SLASH instead?, since it is actually not applied to path? Carlo ``` ## Junio C Hamano, 2020-04-24 20:20 Subject: Re: [PATCH v2] credential: fix matching URLs with multiple levels in path Message-ID: URL: https://gitlist.dev/e/xmqqeescd4a2.fsf%40gitster.c.googlers.com In-Reply-To: <20200424045030.GA44448@Carlos-MBP> ``` Carlo Marcelo Arenas Belón writes: > On Wed, Apr 22, 2020 at 07:51:09PM +0000, brian m. carlson wrote: >> --- 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. >> */ > > wouldn't it be better to call this STRBUF_PERCENTENCODE_SLASH instead?, since > it is actually not applied to path? I was about to say the same thing. Can we phrase it a bit shorter than PERCENTENCODE, by the way? ``` ## brian m. carlson, 2020-04-25 21:32 Subject: [PATCH v3] redential: fix matching URLs with multiple levels in path Message-ID: <20200425213215.71674-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20200425213215.71674-1-sandals%40crustytoothpaste.net In-Reply-To: <20200421225837.GB3515235@coredump.intra.peff.net> ``` 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, we still want to encode slashes in the username component, so we need to have both behaviors available. So instead, let's add a flag to control 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, as well as one ensuring that we handle usernames with slashes gracefully. Reported-by: Ilya Tretyakov Signed-off-by: brian m. carlson --- Changes from v2: * Invert the sense of the flag. * Shorten the flag name. Changes from v1: * Continue to encode slashes in usernames. * Add a test for encoding slashes in usernames. * Hoist existing tests near the other percent-encoding tests. * Update commit message. * Remove debugging information. credential.c | 4 ++-- strbuf.c | 8 +++++--- strbuf.h | 7 ++++++- t/t0300-credentials.sh | 30 ++++++++++++++++++++++++++++++ 4 files changed, 43 insertions(+), 6 deletions(-) diff --git a/credential.c b/credential.c index 108d9e183a..f5327d3d9e 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_ENCODE_SLASH); 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, 0); } } diff --git a/strbuf.c b/strbuf.c index bb0065ccaf..51c83aae2d 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_ENCODE_SLASH)) || + 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..e524c52e01 100644 --- a/strbuf.h +++ b/strbuf.h @@ -378,11 +378,16 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, */ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src); +#define STRBUF_ENCODE_SLASH 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_ENCODE_SLASH is set in flags, percent-encode slashes. Otherwise, + * slashes are not percent-encoded. */ -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..87267a7aac 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -366,6 +366,36 @@ test_expect_success 'match percent-encoded values' ' EOF ' +test_expect_success 'match percent-encoded values in username' ' + test_config credential.https://user%2fname@example.com/foo/bar.git.helper "$HELPER" && + check fill <<-\EOF + url=https://user%2fname@example.com/foo/bar.git + -- + protocol=https + host=example.com + username=foo + password=bar + -- + EOF +' + +test_expect_success 'fetch with multiple path components' ' + test_unconfig credential.helper && + test_config credential.https://example.com/foo/repo.git.helper "verbatim foo bar" && + 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 'pull username from config' ' test_config credential.https://example.com.username foo && check fill <<-\EOF ``` ## Eric Sunshine, 2020-04-26 01:51 Subject: Re: [PATCH v3] redential: fix matching URLs with multiple levels in path Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQW1qLRgQg%3DyR2Uv5T9vQG-8zWyfBLzTh6%3DYU943Ayb%2BQ%40mail.gmail.com In-Reply-To: <20200425213215.71674-1-sandals@crustytoothpaste.net> ``` On Sat, Apr 25, 2020 at 5:34 PM brian m. carlson wrote: > redential: fix matching URLs with multiple levels in path What's a "redential"? ``` ## brian m. carlson, 2020-04-26 17:26 Subject: [PATCH v4] credential: fix matching URLs with multiple levels in path Message-ID: <20200426172659.118503-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20200426172659.118503-1-sandals%40crustytoothpaste.net In-Reply-To: <20200421225837.GB3515235@coredump.intra.peff.net> ``` 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, we still want to encode slashes in the username component, so we need to have both behaviors available. So instead, let's add a flag to control 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, as well as one ensuring that we handle usernames with slashes gracefully. Reported-by: Ilya Tretyakov Signed-off-by: brian m. carlson --- Changes from v3: * Don't delete the first character of the summary. Changes from v2: * Invert the sense of the flag. * Shorten the flag name. Changes from v1: * Continue to encode slashes in usernames. * Add a test for encoding slashes in usernames. * Hoist existing tests near the other percent-encoding tests. * Update commit message. * Remove debugging information. credential.c | 4 ++-- strbuf.c | 8 +++++--- strbuf.h | 7 ++++++- t/t0300-credentials.sh | 30 ++++++++++++++++++++++++++++++ 4 files changed, 43 insertions(+), 6 deletions(-) diff --git a/credential.c b/credential.c index 108d9e183a..f5327d3d9e 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_ENCODE_SLASH); 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, 0); } } diff --git a/strbuf.c b/strbuf.c index bb0065ccaf..51c83aae2d 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_ENCODE_SLASH)) || + 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..e524c52e01 100644 --- a/strbuf.h +++ b/strbuf.h @@ -378,11 +378,16 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, */ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src); +#define STRBUF_ENCODE_SLASH 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_ENCODE_SLASH is set in flags, percent-encode slashes. Otherwise, + * slashes are not percent-encoded. */ -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..87267a7aac 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -366,6 +366,36 @@ test_expect_success 'match percent-encoded values' ' EOF ' +test_expect_success 'match percent-encoded values in username' ' + test_config credential.https://user%2fname@example.com/foo/bar.git.helper "$HELPER" && + check fill <<-\EOF + url=https://user%2fname@example.com/foo/bar.git + -- + protocol=https + host=example.com + username=foo + password=bar + -- + EOF +' + +test_expect_success 'fetch with multiple path components' ' + test_unconfig credential.helper && + test_config credential.https://example.com/foo/repo.git.helper "verbatim foo bar" && + 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 'pull username from config' ' test_config credential.https://example.com.username foo && check fill <<-\EOF ``` ## brian m. carlson, 2020-04-27 01:18 Subject: [PATCH v5] credential: fix matching URLs with multiple levels in path Message-ID: <20200427011809.2494377-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20200427011809.2494377-1-sandals%40crustytoothpaste.net In-Reply-To: <20200421225837.GB3515235@coredump.intra.peff.net> ``` 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, we still want to encode slashes in the username component, so we need to have both behaviors available. So instead, let's add a flag to control 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, as well as one ensuring that we handle usernames with slashes gracefully. Since we're testing other percent-encoding handling, let's add one for non-ASCII UTF-8 characters as well. Reported-by: Ilya Tretyakov Signed-off-by: Carlo Marcelo Arenas Belón Signed-off-by: brian m. carlson --- I talked to Carlo off-list because he was concerned about the potential security implications of the patch, and as part of that discussion he suggested an additional test for our UTF-8 handling. Since adding some additional tests for that case seemed like a good idea, I squashed in a patch of his to handle that case. That's the only change between v4 and v5. credential.c | 4 ++-- strbuf.c | 8 +++++--- strbuf.h | 7 ++++++- t/t0300-credentials.sh | 45 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 6 deletions(-) diff --git a/credential.c b/credential.c index 064e25e5d5..d72e2ed0d8 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_ENCODE_SLASH); 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, 0); } } diff --git a/strbuf.c b/strbuf.c index bb0065ccaf..51c83aae2d 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_ENCODE_SLASH)) || + 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..e524c52e01 100644 --- a/strbuf.h +++ b/strbuf.h @@ -378,11 +378,16 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, */ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src); +#define STRBUF_ENCODE_SLASH 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_ENCODE_SLASH is set in flags, percent-encode slashes. Otherwise, + * slashes are not percent-encoded. */ -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 48484cbcf6..91a007fbfa 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -366,6 +366,51 @@ test_expect_success 'match percent-encoded values' ' EOF ' +test_expect_success 'match percent-encoded UTF-8 values in path' ' + test_config credential.https://example.com.useHttpPath true && + test_config credential.https://example.com/perú.git.helper "$HELPER" && + check fill <<-\EOF + url=https://example.com/per%C3%BA.git + -- + protocol=https + host=example.com + path=perú.git + username=foo + password=bar + -- + EOF +' + +test_expect_success 'match percent-encoded values in username' ' + test_config credential.https://user%2fname@example.com/foo/bar.git.helper "$HELPER" && + check fill <<-\EOF + url=https://user%2fname@example.com/foo/bar.git + -- + protocol=https + host=example.com + username=foo + password=bar + -- + EOF +' + +test_expect_success 'fetch with multiple path components' ' + test_unconfig credential.helper && + test_config credential.https://example.com/foo/repo.git.helper "verbatim foo bar" && + 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 'pull username from config' ' test_config credential.https://example.com.username foo && check fill <<-\EOF ``` ## Junio C Hamano, 2020-04-27 18:44 Subject: Re: [PATCH v5] credential: fix matching URLs with multiple levels in path Message-ID: URL: https://gitlist.dev/e/xmqqblnc93bd.fsf%40gitster.c.googlers.com In-Reply-To: <20200427011809.2494377-1-sandals@crustytoothpaste.net> ``` "brian m. carlson" writes: > I talked to Carlo off-list because he was concerned about the potential > security implications of the patch, and as part of that discussion he > suggested an additional test for our UTF-8 handling. Since adding some > additional tests for that case seemed like a good idea, I squashed in a > patch of his to handle that case. > > That's the only change between v4 and v5. With the polarity flip of the option done in v4, the resulting code is quite easy to follow. Thanks, will queue. ```