Re: [PATCH] credential: fix matching URLs with multiple levels in path
On 2020-04-22 at 04:16:14, Jeff King wrote:
Show 39 quoted lines
> 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.
Show 16 quoted lines
> > 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?
Yeah, it is. I'll rip it out in v2.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
brian m. carlson <sandals@crustytoothpaste.net>