Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[Ilya Tretyakov]

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

Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[Jeff King]

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

Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1665 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[Jonathan Nieder]

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

Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[Jeff King]

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

Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[Jeff King]

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

Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[Johannes Schindelin]

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
>
>

[PATCH] credential: fix matching URLs with multiple levels in path

[brian m. carlson]

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(-)

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

Re: [PATCH] credential: fix matching URLs with multiple levels in path

[Jeff King]

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

Re: [PATCH] credential: fix matching URLs with multiple levels in path

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3125 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v2] credential: fix matching URLs with multiple levels in path

[brian m. carlson]

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 <it@it3xl.ru>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
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

Re: [PATCH v2] credential: fix matching URLs with multiple levels in path

[Jeff King]

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

Re: [PATCH v2] credential: fix matching URLs with multiple levels in path

[Carlo Marcelo Arenas Belón]

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

Re: [PATCH v2] credential: fix matching URLs with multiple levels in path

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> 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?

[PATCH v3] redential: fix matching URLs with multiple levels in path

[brian m. carlson]

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 <it@it3xl.ru>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
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

Re: [PATCH v3] redential: fix matching URLs with multiple levels in path

[Eric Sunshine]

On Sat, Apr 25, 2020 at 5:34 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> redential: fix matching URLs with multiple levels in path

What's a "redential"?

[PATCH v4] credential: fix matching URLs with multiple levels in path

[brian m. carlson]

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 <it@it3xl.ru>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
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

[PATCH v5] credential: fix matching URLs with multiple levels in path

[brian m. carlson]

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 <it@it3xl.ru>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
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

Re: [PATCH v5] credential: fix matching URLs with multiple levels in path

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> 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.