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
