Re: [PATCH] credential: fix matching URLs with multiple levels in path
- From
Jeff King <peff@peff.net>
- Date
- Apr 22, 2020, 04:16 UTC
- Message-ID
- <20200422041614.GD3559880@coredump.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:
Show 19 quoted lines
> 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...
Show 11 quoted lines
> 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?
Show 5 quoted lines
> 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.
Show 11 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?
Show 11 quoted lines
> + 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