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

["brian m. carlson" <sandals@crustytoothpaste.net>]

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