credential: handle partial URLs in config settings again #615
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dscho
force-pushed
the
credential-config-partial-url-maint-2.17
branch
2 times, most recently
from
5d45559
to
66823c7
Compare
|
/submit |
|
Submitted as pull.615.git.1587588665.gitgitgadget@gmail.com |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
| @@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value, | |||
| char *url = xmemdupz(key, dot - key); | |||
There was a problem hiding this comment.
On the Git mailing list, Jonathan Nieder wrote (reply to this):
Johannes Schindelin wrote:
> In the patches for CVE-2020-11008, the ability to specify credential
> settings in the config for partial URLs got lost. For example, it used
> to be possible to specify a credential helper for a specific protocol:
>
> [credential "https://"]
> helper = my-https-helper
>
> Likewise, it used to be possible to configure settings for a specific
> host, e.g.:
>
> [credential "dev.azure.com"]
> useHTTPPath = true
Ah, I see.
[...]
> --- a/credential.c
> +++ b/credential.c
> @@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value,
> char *url = xmemdupz(key, dot - key);
> int matched;
>
> - credential_from_url(&want, url);
> + if (credential_from_url_gently(&want, url, 0, 0) < 0) {
> + warning(_("skipping credential lookup for url: %s"), url);
> + credential_clear(c);
Hm, the error message doesn't seem right here, since `url` is a config
key instead of URL whose credential's are being looked up.
Should the error message include the entirety of `var` to make
debugging easier?
[...]
> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' '
> test_i18ncmp expect stderr
> '
>
> +test_expect_success 'credential config accepts partial URLs' '
> + echo url=https://example.com |
> + git -c credential.example.com.username=boo \
> + credential fill >actual &&
Can the tests also check the behavior with bad URLs (that we are
appropriately tolerating and warning about them?
Stepping back: one thing I like about the code in "master" that uses
urlmatch_config_entry is that it is not treating these config keys
in the same way as URLs that Git would fetch from. For example, if
one of the config keys contains %0a, then that's perfectly fine ---
we are not going to write them to a credential helper or to libcurl.
The only problem is that the pattern matching syntax doesn't match
the behavior that users historically expected. (Keeping in mind
that we never actually provided the behavior that users expected.
`credential.example.com.helper` settings applied to all hosts.)
Would it make sense for parsing these config url patterns to share
*less* code with credential_from_url? Ramifications:
- unless we add specific support for it, we'd lose support for
patterns that are specific to a user (e.g.
"credential.https://user@example.com.helper").
- likewise, we'd lose support for
"credential.https://user:pass@example.com.helper".
- we could control what "credential.https:///repo.git.helper"
means, for example by rejecting it. When we reject it, the
error message could be specific to this use case instead of
shared with other URL handling.
- we wouldn't suport "credential.example.com/repo.git.helper"
by mistake.
- to sum up, we could specifically define exactly what cases we want
to support:
[credential "example.com"]
# settings for the host
...
[credential "user@example.com"] # maybe
# settings for the host/user combination
...
[credential "https://"]
# settings for the scheme
...
[credential "https://example.com"]
# settings for the host/scheme combination
...
[credential "https://example.com/"]
# likewise
...
[credential "https://user@example.com"] # maybe
# settings for the host/scheme/user combination
...
[credential "https://user@example.com/"] # maybe
# likewise
...
[credential "https://example.com/repo.git"]
# settings for the host/scheme/path combination
...
[credential "https://user@example.com/repo.git"] # maybe
# settings for the host/scheme/user/path combination
...
without accidentally promising support for anything else
What do you think?
Thanks,
Jonathan
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Jonathan,
On Wed, 22 Apr 2020, Jonathan Nieder wrote:
> Johannes Schindelin wrote:
>
> [...]
> > --- a/credential.c
> > +++ b/credential.c
> > @@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value,
> > char *url = xmemdupz(key, dot - key);
> > int matched;
> >
> > - credential_from_url(&want, url);
> > + if (credential_from_url_gently(&want, url, 0, 0) < 0) {
> > + warning(_("skipping credential lookup for url: %s"), url);
> > + credential_clear(c);
>
> Hm, the error message doesn't seem right here, since `url` is a config
> key instead of URL whose credential's are being looked up.
>
> Should the error message include the entirety of `var` to make
> debugging easier?
I suppose you're right.
BTW I just realized a much worse issue: `credential_clear(c);`. This
clears the wrong struct. It should clear `want`, not `c`.
> [...]
> > --- a/t/t0300-credentials.sh
> > +++ b/t/t0300-credentials.sh
> > @@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' '
> > test_i18ncmp expect stderr
> > '
> >
> > +test_expect_success 'credential config accepts partial URLs' '
> > + echo url=https://example.com |
> > + git -c credential.example.com.username=boo \
> > + credential fill >actual &&
>
> Can the tests also check the behavior with bad URLs (that we are
> appropriately tolerating and warning about them?
Yes, my bad! The next iteration will have a test for that.
> Stepping back: one thing I like about the code in "master" that uses
> urlmatch_config_entry is that it is not treating these config keys
> in the same way as URLs that Git would fetch from. For example, if
> one of the config keys contains %0a, then that's perfectly fine ---
> we are not going to write them to a credential helper or to libcurl.
That is actually not what the code does, at least not in `maint-2.17`. It
very much warns about `%0a` in config keys. The test I am adding to verify
that the warning above is exercised correctly looks like this:
git -c credential.$partial.helper=yep \
-c credential.with%0anewline.username=uh-oh \
credential fill >output 2>error &&
test_i18ngrep "skipping credential lookup for url" error
That is literally the only way I get `credential_from_url_gently()` to
return `-1` in the lenient mode.
> The only problem is that the pattern matching syntax doesn't match
> the behavior that users historically expected. (Keeping in mind
> that we never actually provided the behavior that users expected.
> `credential.example.com.helper` settings applied to all hosts.)
Yes, I fix that, too. It is a bad usability bug, in my eyes, and I think
it is better to fix it while I'm in the space anyway.
> Would it make sense for parsing these config url patterns to share
> *less* code with credential_from_url? Ramifications:
>
> - unless we add specific support for it, we'd lose support for
> patterns that are specific to a user (e.g.
> "credential.https://user@example.com.helper").
>
> - likewise, we'd lose support for
> "credential.https://user:pass@example.com.helper".
>
> - we could control what "credential.https:///repo.git.helper"
> means, for example by rejecting it. When we reject it, the
> error message could be specific to this use case instead of
> shared with other URL handling.
>
> - we wouldn't suport "credential.example.com/repo.git.helper"
> by mistake.
I think we can have _all_ of this _without_ violating the DRY principle.
Remember: my main motivation for keeping 2/3 apart from 3/3 was so that it
is really easy to verify that 2/3 does not break the callers that _need_
the strict mode.
And since that is the case, we can then enjoy the benefit of the shared
code for the one caller that wants to match also partial URLs.
> - to sum up, we could specifically define exactly what cases we want
> to support:
>
> [credential "example.com"]
> # settings for the host
> ...
>
> [credential "user@example.com"] # maybe
> # settings for the host/user combination
> ...
>
> [credential "https://"]
> # settings for the scheme
> ...
>
> [credential "https://example.com"]
> # settings for the host/scheme combination
> ...
>
> [credential "https://example.com/"]
> # likewise
> ...
>
> [credential "https://user@example.com"] # maybe
> # settings for the host/scheme/user combination
> ...
>
> [credential "https://user@example.com/"] # maybe
> # likewise
> ...
>
> [credential "https://example.com/repo.git"]
> # settings for the host/scheme/path combination
> ...
>
> [credential "https://user@example.com/repo.git"] # maybe
> # settings for the host/scheme/user/path combination
> ...
>
> without accidentally promising support for anything else
>
> What do you think?
I added tests for all of those. They all work as a naive user (like me)
would expect them to.
I threw in another test, too:
[credential "/repo.git"]
...
And I also threw in negative tests, to verify that non-matching protocol
or host name or path mean that the config setting is ignored.
Thank you for your thorough review, it really helps me,
Dscho
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
There was a lot going on behind the scenes when the vulnerability and possible solutions were discussed. Grammar was not a primary focus, that's why this slipped in. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
credential-config-partial-url-maint-2.17
branch
from
66823c7
to
daedaff
Compare
|
/submit |
|
Submitted as pull.615.v2.git.1587685397.gitgitgadget@gmail.com |
|
On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this): |
…ly() Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we required from a URL in order to parse it into a `struct credential`. That led to serious vulnerabilities. There was one call site, though, that really needed that leniency: when parsing config settings a la `credential.dev.azure.com.useHTTPPath`. Settings like this might be desired when users want to use, say, a given user name on a given host, regardless of the protocol to be used. In preparation for fixing that bug, let's refactor the code to optionally allow for partial URLs. For the moment, this functionality is only exposed via the now-renamed function `credential_from_url_1()`, but it is not used. The intention is to make it easier to verify that this commit does not change the existing behavior unless explicitly allowing for partial URLs. Please note that this patch does more than just reinstating a way to imitate the behavior before those CVE-2020-11008 fixes: Before that, we would simply ignore URLs without a protocol. In other words, misleadingly, the following setting would be applied to _all_ URLs: [credential "example.com"] username = that-me The obvious intention is to match the host name only. With this patch, we allow precisely that: when parsing the URL with non-zero `allow_partial_url`, we do not simply return success if there was no protocol, but we simply leave the protocol unset and continue parsing the URL. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
credential-config-partial-url-maint-2.17
branch
from
daedaff
to
0535908
Compare
|
/submit |
|
Submitted as pull.615.v3.git.1587728992.gitgitgadget@gmail.com |
| @@ -35,6 +35,10 @@ int credential_match(const struct credential *want, | |||
| #undef CHECK | |||
There was a problem hiding this comment.
On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this):
On Fri, Apr 24, 2020 at 11:49:52AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/credential.c b/credential.c
> index 7dbbf26f174..c1a9ca4e485 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -35,6 +35,10 @@ int credential_match(const struct credential *want,
> #undef CHECK
> }
>
> +
> +static int credential_from_potentially_partial_url(struct credential *c,
> + const char *url);
> +
> static int credential_config_callback(const char *var, const char *value,
> void *data)
> {
something like credential_from_url_partial might had been more grep friendly
but this would work as well.
> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> index efed3ea2955..f796bbfd48b 100755
> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' '
> test_i18ncmp expect stderr
> '
>
> +test_expect_success 'credential config with partial URLs' '
> + echo "echo password=yep" | write_script git-credential-yep &&
> + test_write_lines url=https://user@example.com/repo.git >stdin &&
> + for partial in \
> + example.com \
> + user@example.com \
even if it works, configurations with a username might not be worth the
trouble to support IMHO
maybe better not to include them in the tests then either?
other than that, like the previous version (which is functionally equivalent)
should be IMHO good to go.
Carlo
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--8323328-952375376-1587811402=:18039
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Hi Carlo,
On Fri, 24 Apr 2020, Carlo Marcelo Arenas Bel=C3=B3n wrote:
> On Fri, Apr 24, 2020 at 11:49:52AM +0000, Johannes Schindelin via GitGit=
Gadget wrote:
> > diff --git a/credential.c b/credential.c
> > index 7dbbf26f174..c1a9ca4e485 100644
> > --- a/credential.c
> > +++ b/credential.c
> > @@ -35,6 +35,10 @@ int credential_match(const struct credential *want,
> > #undef CHECK
> > }
> >
> > +
> > +static int credential_from_potentially_partial_url(struct credential =
*c,
> > + const char *url);
> > +
> > static int credential_config_callback(const char *var, const char *va=
lue,
> > void *data)
> > {
>
> something like credential_from_url_partial might had been more grep frie=
ndly
> but this would work as well.
It might be more grep'able, but it sounds really awkward to me, that's why
I did not use that name (it was my first candidate).
> > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> > index efed3ea2955..f796bbfd48b 100755
> > --- a/t/t0300-credentials.sh
> > +++ b/t/t0300-credentials.sh
> > @@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to=
work with missing protocol' '
> > test_i18ncmp expect stderr
> > '
> >
> > +test_expect_success 'credential config with partial URLs' '
> > + echo "echo password=3Dyep" | write_script git-credential-yep &&
> > + test_write_lines url=3Dhttps://user@example.com/repo.git >stdin &&
> > + for partial in \
> > + example.com \
> > + user@example.com \
>
> even if it works, configurations with a username might not be worth the
> trouble to support IMHO
>
> maybe better not to include them in the tests then either?
Let me counter this:
- It would take extra code _to prevent_ the username from being used, and
- There is precedent where the user name _does_ matter: it is relatively
normal to access different orgs' repositories at
https://dev.azure.com/<org>/<repo>/_git via different accounts.
Together, those points convince me that special-casing the username _and
explicitly ignoring it_ would not make sense.
> other than that, like the previous version (which is functionally equiva=
lent)
> should be IMHO good to go.
Thank you for reviewing it!
Ciao,
Dscho
--8323328-952375376-1587811402=:18039--
| @@ -35,6 +35,10 @@ int credential_match(const struct credential *want, | |||
| #undef CHECK | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,
> char *url = xmemdupz(key, dot - key);
> int matched;
>
> - credential_from_url(&want, url);
> + if (credential_from_potentially_partial_url(&want, url) < 0) {
> + warning(_("skipping credential lookup for key: %s"),
> + var);
> + credential_clear(&want);
> + free(url);
> + return 0;
> + }
> matched = credential_match(&want, c);
Unfortunately, the whole section of this code that is being patched
here goes away with 46fd7b39 (credential: allow wildcard patterns
when matching config, 2020-02-20), that delegates large part of the
logic to urlmatch.
Dscho and Brian, how do we want to proceed? As the conflicting
change has already been in 'next' for more than a month and a half,
we'd need this "partial URL" logic build to work well with it.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Fri, 24 Apr 2020, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,
> > char *url = xmemdupz(key, dot - key);
> > int matched;
> >
> > - credential_from_url(&want, url);
> > + if (credential_from_potentially_partial_url(&want, url) < 0) {
> > + warning(_("skipping credential lookup for key: %s"),
> > + var);
> > + credential_clear(&want);
> > + free(url);
> > + return 0;
> > + }
> > matched = credential_match(&want, c);
>
> Unfortunately, the whole section of this code that is being patched
> here goes away with 46fd7b39 (credential: allow wildcard patterns
> when matching config, 2020-02-20), that delegates large part of the
> logic to urlmatch.
>
> Dscho and Brian, how do we want to proceed? As the conflicting
> change has already been in 'next' for more than a month and a half,
> we'd need this "partial URL" logic build to work well with it.
I prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not
necessarily feel 100% comfortable with that approach yet, but at least it
lets the new test case of t0300 pass.
Ciao,
Dscho
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Fri, 24 Apr 2020, Junio C Hamano wrote:
>
>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>>
>> > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,
>> > char *url = xmemdupz(key, dot - key);
>> > int matched;
>> >
>> > - credential_from_url(&want, url);
>> > + if (credential_from_potentially_partial_url(&want, url) < 0) {
>> > + warning(_("skipping credential lookup for key: %s"),
>> > + var);
>> > + credential_clear(&want);
>> > + free(url);
>> > + return 0;
>> > + }
>> > matched = credential_match(&want, c);
>>
>> Unfortunately, the whole section of this code that is being patched
>> here goes away with 46fd7b39 (credential: allow wildcard patterns
>> when matching config, 2020-02-20), that delegates large part of the
>> logic to urlmatch.
>>
>> Dscho and Brian, how do we want to proceed? As the conflicting
>> change has already been in 'next' for more than a month and a half,
>> we'd need this "partial URL" logic build to work well with it.
>
> I prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not
> necessarily feel 100% comfortable with that approach yet, but at least it
> lets the new test case of t0300 pass.
The -2.17 patch series (your [v3 3/3]) ends t0300 like so
+ done &&
+
+ git -c credential.$partial.helper=yep \
+ -c credential.with%0anewline.username=uh-oh \
+ credential fill <stdin >stdout 2>stderr &&
+ test_i18ngrep "skipping credential lookup for key" stderr
+
+'
+
test_done
while the other branch lacks the extra blank line just before the
single quote is closed. I queued 850ef627 (SQUASH??? lose excess
blank line to match the other side of the eventual merge,
2020-04-24) on top so that the "-s ours" merge would be without
unnecessary evil-merge noise. You probably would want to squash it
in.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Tue, 28 Apr 2020, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Hi Junio,
> >
> > On Fri, 24 Apr 2020, Junio C Hamano wrote:
> >
> >> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> >> writes:
> >>
> >> > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,
> >> > char *url = xmemdupz(key, dot - key);
> >> > int matched;
> >> >
> >> > - credential_from_url(&want, url);
> >> > + if (credential_from_potentially_partial_url(&want, url) < 0) {
> >> > + warning(_("skipping credential lookup for key: %s"),
> >> > + var);
> >> > + credential_clear(&want);
> >> > + free(url);
> >> > + return 0;
> >> > + }
> >> > matched = credential_match(&want, c);
> >>
> >> Unfortunately, the whole section of this code that is being patched
> >> here goes away with 46fd7b39 (credential: allow wildcard patterns
> >> when matching config, 2020-02-20), that delegates large part of the
> >> logic to urlmatch.
> >>
> >> Dscho and Brian, how do we want to proceed? As the conflicting
> >> change has already been in 'next' for more than a month and a half,
> >> we'd need this "partial URL" logic build to work well with it.
> >
> > I prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not
> > necessarily feel 100% comfortable with that approach yet, but at least it
> > lets the new test case of t0300 pass.
>
> The -2.17 patch series (your [v3 3/3]) ends t0300 like so
>
> + done &&
> +
> + git -c credential.$partial.helper=yep \
> + -c credential.with%0anewline.username=uh-oh \
> + credential fill <stdin >stdout 2>stderr &&
> + test_i18ngrep "skipping credential lookup for key" stderr
> +
> +'
> +
> test_done
>
> while the other branch lacks the extra blank line just before the
> single quote is closed. I queued 850ef627 (SQUASH??? lose excess
> blank line to match the other side of the eventual merge,
> 2020-04-24) on top so that the "-s ours" merge would be without
> unnecessary evil-merge noise. You probably would want to squash it
> in.
Thank you for pointing that out. I already did that:
https://github.com/gitgitgadget/git/compare/0535908dd7ea4487b342c0f86182579279c57b34..d59738ecf741a5fddd70f133eaaf69768e245bcc
Do you want me to send another iteration, for completeness' sake?
Ciao,
Dscho
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> while the other branch lacks the extra blank line just before the
>> single quote is closed. I queued 850ef627 (SQUASH??? ...
>
> Thank you for pointing that out. I already did that: ...
> Do you want me to send another iteration, for completeness' sake?
Then if I squash 850ef627 on my end, we'd be in sync, so unless
there is any other change, no need to resend it. Have we got enough
eyeballs that looked at the patches already?
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Wed, 29 Apr 2020, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> while the other branch lacks the extra blank line just before the
> >> single quote is closed. I queued 850ef627 (SQUASH??? ...
> >
> > Thank you for pointing that out. I already did that: ...
> > Do you want me to send another iteration, for completeness' sake?
>
> Then if I squash 850ef627 on my end, we'd be in sync, so unless
> there is any other change, no need to resend it.
Perfect, thanks!
> Have we got enough eyeballs that looked at the patches already?
I would obviously prefer more reviews.
Having said that, GitHub Desktop rolled out a new release this past Monday
with the MinGit backport I prepared with these here patches, and the only
additional report at https://github.com/desktop/desktop/issues/9597 talks
about macOS (and the MinGit backport is Windows-only). Which I take as
good news for the robustness and correctness of the fixes.
Ciao,
Dscho
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I would obviously prefer more reviews.
>
> Having said that, GitHub Desktop rolled out a new release this past Monday
> with the MinGit backport I prepared with these here patches, and the only
> additional report at https://github.com/desktop/desktop/issues/9597 talks
> about macOS (and the MinGit backport is Windows-only). Which I take as
> good news for the robustness and correctness of the fixes.
Good. My preference actually is to fast-track this down to 'master'
without the usual 'at least about a week in next' gestation period.
|
This patch series was integrated into pu via git@81a6e6c. |
|
This patch series was integrated into pu via git@f3a547b. |
In the patches for CVE-2020-11008, the ability to specify credential settings in the config for partial URLs got lost. For example, it used to be possible to specify a credential helper for a specific protocol: [credential "https://"] helper = my-https-helper Likewise, it used to be possible to configure settings for a specific host, e.g.: [credential "dev.azure.com"] useHTTPPath = true Let's reinstate this behavior. While at it, increase the test coverage to document and verify the behavior with a couple other categories of partial URLs. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
credential-config-partial-url-maint-2.17
branch
from
0535908
to
d59738e
Compare
|
This patch series was integrated into pu via git@2243047. |
|
This patch series was integrated into pu via git@4eb5cf6. |
|
This patch series was integrated into pu via git@27ce083. |
|
This branch is now known as |
|
This patch series was integrated into pu via git@f72abd7. |
|
This patch series was integrated into pu via git@17b9b5b. |
|
This patch series was integrated into pu via git@456c6d8. |
|
This patch series was integrated into pu via git@4d69ab5. |
|
This patch series was integrated into next via git@7c69571. |
|
This patch series was integrated into pu via git@da05cac. |
|
This patch series was integrated into next via git@da05cac. |
|
This patch series was integrated into master via git@da05cac. |
|
Closed via da05cac. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the problem illustrated by Peff's example, in
maint-2.17:The fix is necessarily different than what was proposed by brian because that fix targets v2.26.x which has 46fd7b3 (credential: allow wildcard patterns when matching config, 2020-02-20).
This patch series targets
maint-2.17instead (and might actually not be able to fixmaintdue to that wildcard pattern patch; I haven't had the time to check yet).Please note that Git v2.17.4 will not do what we would expect here: if any host name (without protocol) is specified, e.g.
-c credential.golli.wog.username = boo, it will actually ignore the host name. That is, this will populate the username:Obviously, this is unexpected, as a Git config like this would leave the last specified user name as "winner":
This patch series fixes this. The quoted part of
[credential "<value>"]will be interpreted as a partial URL:://, but does not have to.://is followed immediately by yet another slash, then it will be set to the empty string).Changes since v3:
Changes since v2:
credential_from_potentially_partial_url()function so that existing callers (except for the one we want to change) stay untouched.stdin/stdout/stderroft/lib-credential.sh.Changes since v1:
c->hostfield is set was made more intuitive.strictand renamed it toallow_partial_urls, to clarify its role.https://is now correctly interpreted as having only the protocol set, but not host name nor path.credential.h.struct credentialis cleared (i.e. the just-parsed one, not the one against which we wanted to match the just-parsed one).Cc: Jeff King peff@peff.net, "brian m. carlson" sandals@crustytoothpaste.net, Jonathan Nieder jrnieder@gmail.com, Ilya Tretyakov it@it3xl.ru, Junio C Hamano gitster@pobox.com