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

[Jeff King <peff@peff.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