Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Wed, 22 Apr 2020, Jeff King wrote:

> On Wed, Apr 22, 2020 at 02:20:20AM +0000, brian m. carlson wrote:
>
> > > Thanks.  Here's another (though I haven't tried bisecting yet):
> > >
> > > 	echo url='https://github.com/git/git' |
> > > 	GIT_TERMINAL_PROMPT=0 \
> > > 	git -c credential.helper= \
> > > 		-c credential.github.com.helper='!echo username=foo; echo password=bar;:' \
> > > 		credential fill
> >
> > gitcredentials(7) says the following:
> >
> >   Git considers each credential to have a context defined by a URL.
> >   This context is used to look up context-specific configuration, and is
> >   passed to any helpers, which may use it as an index into secure
> >   storage.
> >
> > I'm not sure a hostname qualifies as a URL in this case.  So while my
> > patch did break this, I don't believe it's ever been documented to
> > actually work and was an artifact of our implementation (along with
> > "credential./git/git.helper" and "credential.https://.helper").  I've
> > also never seen this syntax used in the wild, but maybe I'm not looking
> > in the right places.
>
> I'm pretty sure it was an intended use case, though it is a natural
> outcome of the credential_match() strategy of "unspecified things match
> anything". I'd suspect that anybody relying on it is doing so
> unintentionally, and just forgot to put the protocol field in. Though I
> suppose doing so would let you cover http/https in a single block.
>
> At any rate, even in versions _without_ your patch, that became a hard
> error in this week's release. In v2.24.3, for example:
>
>   $ echo url=https://anyhost.example.com |
>     git -c credential.example.com.username=foo credential fill
>   warning: url has no scheme: example.com
>   fatal: credential url cannot be parsed: example.com
>
> because we're relying there on credential_from_url() to parse the config
> credentials, too. After your patch, we use the http-config machinery,
> which simply doesn't match.

This affects also pre-v2.26.* versions. One fallout is that some GitHub
Desktop users reported that they cannot fetch anymore:
https://github.com/desktop/desktop/issues/9597

I _think_ I have a good fix for this, and am only waiting for the PR
builds at https://github.com/gitgitgadget/git/pull/615 to finish before I
will submit that patch series for review.

Ideally, I will integrate these patches into some MinGit backports
tonight, still, so that GitHub Desktop can roll out another release to
avoid many more reports. Therefore, I hope for quick reviews ;-)

Ciao,
Dscho

>
> > I don't think we can shoehorn it into urlmatch, since that would break
> > compatibility with the `http.*` config options, so I think we'd have to
> > revert the entire feature if we want to preserve it.  I think I'd prefer
> > to leave things as it is since it seems uncommon and there are easy
> > alternatives, but if folks prefer, I can send a patch to revert the
> > urlmatch feature.
>
> I agree that we should leave it. Aside from the dual http/https thing
> (which _hopefully_ is rare these days as https become more of a
> standard), I don't think it has a legitimate use case. And I think we
> should be pushing users to be a bit more careful with their url config.
>
> -Peff
>
>