[PATCH 0/3] credential: handle partial URLs in config settings again

[PATCH 0/3] credential: handle partial URLs in config settings again

[Johannes Schindelin via GitGitGadget]

This fixes the problem illustrated by Peff's example
[https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]
, in maint-2.17:

  $ echo url=https://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

The fix is necessarily different than what was proposed by brian
[https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/] 
because that fix targets v2.26.x which has 46fd7b390034 (credential: allow
wildcard patterns when matching config, 2020-02-20).

This patch series targets maint-2.17 instead (and might actually not be able
to fix maint due 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:

  $ echo url=https://example.com |
    git -c credential.totally.bog.us.username=foo credential fill

Obviously, this is unexpected, as a Git config like this would leave the
last specified user name as "winner":

[credential "first.com"]
    username = Whos On
[credential "second.com"]
    username = Who

This patch series fixes this. The quoted part of [credential "<value>"] will
be interpreted as a partial URL:

 * It can start with a protocol followed by ://, but does not have to.
 * If it starts with a protocol, the host name will always be set (if the 
   :// is followed immediately by yet another slash, then it will be set to
   the empty string).
 * If it starts without a protocol, it is treated as a path if the value
   starts with a slash (and the host will be left unset).
 * If it starts without a protocol and the first character is not a slash,
   it will be treated as a host name, optionally followed by a slash and the
   path.

Johannes Schindelin (3):
  credential: fix grammar
  credential: teach `credential_from_url()` a non-strict mode
  credential: handle `credential.<partial-URL>.<key>` again

 credential.c           | 21 ++++++++++++++-------
 credential.h           |  8 ++++++--
 fsck.c                 |  2 +-
 t/t0300-credentials.sh | 13 +++++++++++++
 4 files changed, 34 insertions(+), 10 deletions(-)


base-commit: df5be6dc3fd18c294ec93a9af0321334e3f92c9c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-615%2Fdscho%2Fcredential-config-partial-url-maint-2.17-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-615/dscho/credential-config-partial-url-maint-2.17-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/615
-- 
gitgitgadget

[PATCH 1/3] credential: fix grammar

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 credential.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential.h b/credential.h
index 122a23cd2f1..5a86502d95c 100644
--- a/credential.h
+++ b/credential.h
@@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *);
 /*
  * Parse a url into a credential struct, replacing any existing contents.
  *
- * Ifthe url can't be parsed (e.g., a missing "proto://" component), the
+ * If the url can't be parsed (e.g., a missing "proto://" component), the
  * resulting credential will be empty but we'll still return success from the
  * "gently" form.
  *
-- 
gitgitgadget

[PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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`.

In preparation for fixing that regression, let's add a parameter called
`strict` to the `credential_from_url()` function and convert the
existing callers to enforce that strict mode.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 credential.c | 14 ++++++++------
 credential.h |  6 +++++-
 fsck.c       |  2 +-
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/credential.c b/credential.c
index 64a841eddca..c73260ac40f 100644
--- a/credential.c
+++ b/credential.c
@@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,
 }
 
 int credential_from_url_gently(struct credential *c, const char *url,
-			       int quiet)
+			       int strict, int quiet)
 {
 	const char *at, *colon, *cp, *slash, *host, *proto_end;
 
@@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,
 	 *   (3) proto://<user>:<pass>@<host>/...
 	 */
 	proto_end = strstr(url, "://");
-	if (!proto_end || proto_end == url) {
+	if (strict && (!proto_end || proto_end == url)) {
 		if (!quiet)
 			warning(_("url has no scheme: %s"), url);
 		return -1;
 	}
-	cp = proto_end + 3;
+	cp = proto_end ? proto_end + 3 : url;
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');
@@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
 		host = at + 1;
 	}
 
-	c->protocol = xmemdupz(url, proto_end - url);
-	c->host = url_decode_mem(host, slash - host);
+	if (proto_end && proto_end - url > 0)
+		c->protocol = xmemdupz(url, proto_end - url);
+	if (slash - url > 0)
+		c->host = url_decode_mem(host, slash - host);
 	/* Trim leading and trailing slashes from path */
 	while (*slash == '/')
 		slash++;
@@ -407,6 +409,6 @@ int credential_from_url_gently(struct credential *c, const char *url,
 
 void credential_from_url(struct credential *c, const char *url)
 {
-	if (credential_from_url_gently(c, url, 0) < 0)
+	if (credential_from_url_gently(c, url, 1, 0) < 0)
 		die(_("credential url cannot be parsed: %s"), url);
 }
diff --git a/credential.h b/credential.h
index 5a86502d95c..823ec2caf35 100644
--- a/credential.h
+++ b/credential.h
@@ -41,9 +41,13 @@ void credential_write(const struct credential *, FILE *);
  * an error but leave the broken state in the credential object for further
  * examination.  The non-gentle form will issue a warning to stderr and return
  * an empty credential.
+ *
+ * In strict mode, an empty protocol or an empty host name are not allowed.
+ * The credential_from_url() function enforces strict mode.
  */
 void credential_from_url(struct credential *, const char *url);
-int credential_from_url_gently(struct credential *, const char *url, int quiet);
+int credential_from_url_gently(struct credential *, const char *url,
+			       int strict, int quiet);
 
 int credential_match(const struct credential *have,
 		     const struct credential *want);
diff --git a/fsck.c b/fsck.c
index 31b5be05f54..aa66dc1e742 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1076,7 +1076,7 @@ static int check_submodule_url(const char *url)
 	else if (url_to_curl_url(url, &curl_url)) {
 		struct credential c = CREDENTIAL_INIT;
 		int ret = 0;
-		if (credential_from_url_gently(&c, curl_url, 1) ||
+		if (credential_from_url_gently(&c, curl_url, 1, 1) ||
 		    !*c.host)
 			ret = -1;
 		credential_clear(&c);
-- 
gitgitgadget

[PATCH 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Original-test-case-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 credential.c           |  7 ++++++-
 t/t0300-credentials.sh | 13 +++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/credential.c b/credential.c
index c73260ac40f..b9e8daa5406 100644
--- 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);
+			free(url);
+			return 0;
+		}
 		matched = credential_match(&want, c);
 
 		credential_clear(&want);
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index efed3ea2955..9dcba6a7ad9 100755
--- 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 &&
+	cat >expect <<-EOF &&
+	protocol=https
+	host=example.com
+	username=boo
+	password=askpass-password
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 0/3] credential: handle partial URLs in config settings again

[Jeff King]

On Wed, Apr 22, 2020 at 08:51:02PM +0000, Johannes Schindelin via GitGitGadget wrote:

> This fixes the problem illustrated by Peff's example
> [https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]
> , in maint-2.17:
> 
>   $ echo url=https://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
> 
> The fix is necessarily different than what was proposed by brian
> [https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/] 
> because that fix targets v2.26.x which has 46fd7b390034 (credential: allow
> wildcard patterns when matching config, 2020-02-20).
> 
> This patch series targets maint-2.17 instead (and might actually not be able
> to fix maint due to that wildcard pattern patch; I haven't had the time to
> check yet).

Yes, this will remove the die() in all versions, but in v2.26.0 and up,
that config would be silently ignored (and your new test will fail).

> 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:
> 
>   $ echo url=https://example.com |
>     git -c credential.totally.bog.us.username=foo credential fill

That seems scary. What if it is not .username, but:

  [credential "example.com"]
  username = foo
  helper = "!echo password=bar"

? (Or you can imagine a helper that is pulling from a read-only store,
like "pass"). That would send the credential to the wrong host.

I think any fix we do here needs to make sure we are not any
reintroducing wrong-host problems (though I do admit that the usage like
my example above is probably way less common than vanilla helpers that
do the host-selection themselves).

-Peff

Re: [PATCH 0/3] credential: handle partial URLs in config settings again

[Johannes Schindelin]

Hi Peff,

On Wed, 22 Apr 2020, Jeff King wrote:

> On Wed, Apr 22, 2020 at 08:51:02PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > This fixes the problem illustrated by Peff's example
> > [https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]
> > , in maint-2.17:
> >
> >   $ echo url=https://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
> >
> > The fix is necessarily different than what was proposed by brian
> > [https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/]
> > because that fix targets v2.26.x which has 46fd7b390034 (credential: allow
> > wildcard patterns when matching config, 2020-02-20).
> >
> > This patch series targets maint-2.17 instead (and might actually not be able
> > to fix maint due to that wildcard pattern patch; I haven't had the time to
> > check yet).
>
> Yes, this will remove the die() in all versions, but in v2.26.0 and up,
> that config would be silently ignored (and your new test will fail).

Thanks for testing!

> > 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:
> >
> >   $ echo url=https://example.com |
> >     git -c credential.totally.bog.us.username=foo credential fill
>
> That seems scary. What if it is not .username, but:
>
>   [credential "example.com"]
>   username = foo
>   helper = "!echo password=bar"
>
> ? (Or you can imagine a helper that is pulling from a read-only store,
> like "pass"). That would send the credential to the wrong host.

It would. But I am not aware of any implications regarding `.gitmodules`
(for some reason I now expect every bug to open an attack vector via
submodules, I wonder why that is), so that's at least good.

There do seem to be a few projects that set a
`credential.<hostname>.useHTTPPath` as part of their build, but I doubt
that that could be exploited.

> I think any fix we do here needs to make sure we are not any
> reintroducing wrong-host problems (though I do admit that the usage like
> my example above is probably way less common than vanilla helpers that
> do the host-selection themselves).

Yes. For the record, I tried to be very careful here. The _only_ code path
that is affected by this change is the config reading. Reading
`.gitmodules` (also in `fsck.c`) should behave identically to the way
v2.17.5 behaves: they should not allow empty protocol or hostname. If you
spot any difference in behavior in this regard, please do let me know:
that would be a bug.

And I do not think that these patches could re-introduce the problems you
fixed: the `credential_match()` tries to work against a `struct
credential` that has been sent to `credential_apply_config()` and as per
your fixes, this function now verifies that neither protocol nor hostname
are unset.

But again, I would love another pair of eyes on this, to confirm my
analysis.

Thanks,
Dscho

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> -	if (!proto_end || proto_end == url) {
> +	if (strict && (!proto_end || proto_end == url)) {
>  		if (!quiet)
>  			warning(_("url has no scheme: %s"), url);
>  		return -1;
>  	}
> -	cp = proto_end + 3;
> +	cp = proto_end ? proto_end + 3 : url;
>  	at = strchr(cp, '@');
>  	colon = strchr(cp, ':');
>  	slash = strchrnul(cp, '/');
> @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
>  		host = at + 1;
>  	}
>  
> -	c->protocol = xmemdupz(url, proto_end - url);
> -	c->host = url_decode_mem(host, slash - host);
> +	if (proto_end && proto_end - url > 0)
> +		c->protocol = xmemdupz(url, proto_end - url);

Missing "proto://" under non-strict mode would leave c->protocol
NULL (not "") here, as described in [0/3].

Here, slash would be pointing at one of "/?#" at the end of the host
and url would be pointing at...?  E.g. for "http:///path", URL
points at 'h' at the beginning, proto_end points at ':', cp points
at the last '/' before "path" and slash is the same as cp.  host
points at cp as there is no '@' at sign.

> +	if (slash - url > 0)
> +		c->host = url_decode_mem(host, slash - host);

This wants to make c->host NULL when host is missing, as described
in [0/3].

Shouldn't the condition based on "slash - host", though?

Other than that, it looks sensible.

Re: [PATCH 0/3] credential: handle partial URLs in config settings again

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:
> On Wed, 22 Apr 2020, Jeff King wrote:
>> On Wed, Apr 22, 2020 at 08:51:02PM +0000, Johannes Schindelin via GitGitGadget wrote:

>>> 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:
>>>
>>>   $ echo url=https://example.com |
>>>     git -c credential.totally.bog.us.username=foo credential fill
>>
>> That seems scary. What if it is not .username, but:
>>
>>   [credential "example.com"]
>>   username = foo
>>   helper = "!echo password=bar"
>>
>> ? (Or you can imagine a helper that is pulling from a read-only store,
>> like "pass"). That would send the credential to the wrong host.
>
> It would. But I am not aware of any implications regarding `.gitmodules`
> (for some reason I now expect every bug to open an attack vector via
> submodules, I wonder why that is), so that's at least good.

Submodules are only one of many ways that people end up cloning from
an attacker-controlled URL.  In submodules we're careful not to use
--recurse-submodules by default at clone time.  So I'll mentally
subsitute "attacker-controlled URLs" where you say "submodules". ;-)

I agree with Peff's concern about the unpatched state: since there are
people who use `[credential "host.example.com"] helper` and there are
credential helpers that ignore the host passed in, the combination can
hurt people.  (Fortunately, there aren't many credential helpers in
that category that are commonly used.)

[...]
> Yes. For the record, I tried to be very careful here. The _only_ code path
> that is affected by this change is the config reading.
[...]
> But again, I would love another pair of eyes on this, to confirm my
> analysis.

As mentioned above, the config reading is sensitive, too.  That said,
I suspect you got it to do basically the right thing.

Reading through the patches.  Thank you.

Jonathan

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Apr 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > -	if (!proto_end || proto_end == url) {
> > +	if (strict && (!proto_end || proto_end == url)) {
> >  		if (!quiet)
> >  			warning(_("url has no scheme: %s"), url);
> >  		return -1;
> >  	}
> > -	cp = proto_end + 3;
> > +	cp = proto_end ? proto_end + 3 : url;
> >  	at = strchr(cp, '@');
> >  	colon = strchr(cp, ':');
> >  	slash = strchrnul(cp, '/');
> > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
> >  		host = at + 1;
> >  	}
> >
> > -	c->protocol = xmemdupz(url, proto_end - url);
> > -	c->host = url_decode_mem(host, slash - host);
> > +	if (proto_end && proto_end - url > 0)
> > +		c->protocol = xmemdupz(url, proto_end - url);
>
> Missing "proto://" under non-strict mode would leave c->protocol
> NULL (not "") here, as described in [0/3].

Right. I debated whether to set it to `NULL` explicitly, or whether to
leave it alone. At the end, I settled with the version in v2.17.4.

> Here, slash would be pointing at one of "/?#" at the end of the host
> and url would be pointing at...?  E.g. for "http:///path", URL
> points at 'h' at the beginning, proto_end points at ':', cp points
> at the last '/' before "path" and slash is the same as cp.  host
> points at cp as there is no '@' at sign.

It is not obvious from the diff: `url` is never changed. It would still
point at the first `h`, as you said.

> > +	if (slash - url > 0)
> > +		c->host = url_decode_mem(host, slash - host);
>
> This wants to make c->host NULL when host is missing, as described
> in [0/3].

I did not describe well enough in 0/3, my apologies.

Again, I tried to go with the version from v2.17.4 here, i.e. to set
`c->host` if we get a value, but not set it to `NULL` otherwise (instead
leave as-is).

> Shouldn't the condition based on "slash - host", though?

That would have been my preferred solution, too. But there is some
subtlety at work: for a `url` like `/this/is/my/path`, we want to leave
`c->host` as-is, but for `cert:///...` we want to set it to "" (t0300.23
and t7416.12 test for this explicitly).

I guess a better condition would be `if (proto_end || slash - host > 0)`.
What do you think?

> Other than that, it looks sensible.

Thanks,
Dscho

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It is not obvious from the diff: `url` is never changed. It would still
> point at the first `h`, as you said.
>
>> > +	if (slash - url > 0)
>> > +		c->host = url_decode_mem(host, slash - host);
> ...
> I guess a better condition would be `if (proto_end || slash - host > 0)`.

Yeah, that would probably be more intuitive to read.  What triggered
my reaction was the mismatch between "slash - url" in the condition
and "slash - host" that specifies the length of the memory region.

Re: [PATCH 1/3] credential: fix grammar

[Jonathan Nieder]

Johannes Schindelin wrote:

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  credential.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Jonathan Nieder]

Johannes Schindelin wrote:

> There was one call site, though, that really needed that leniency: when
> parsing config settings a la `credential.dev.azure.com.useHTTPPath`.

Thanks for tackling this.

Can the commit message say a little more about the semantics and when
someone would use this?

Is it a shortcut for

	[credential "http://dev.azure.com"]
		useHttpPath = true

	[credential "https://dev.azure.com"]
		useHttpPath = true

?

> In preparation for fixing that regression, let's add a parameter called
> `strict` to the `credential_from_url()` function and convert the
> existing callers to enforce that strict mode.

I suspect this would be easier to read squashed with patch 3.  That
would also mean that the functionality and test coverage come at the
same time.

[...]
> diff --git a/credential.c b/credential.c
> index 64a841eddca..c73260ac40f 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,
>  }
>  
>  int credential_from_url_gently(struct credential *c, const char *url,
> -			       int quiet)
> +			       int strict, int quiet)

The collection of flags makes me wonder whether it's time to use a
single "flags" parameter with flags that are |ed together.  That way,
call sites are easier to read without requiring cross-reference
assistance to see which option each boolean parameter represents.

Alternatively, could the non-strict form be a separate public function
that uses the same static helper that takes two boolean args?  That is,
something like

	int credential_from_url_gently(struct credential *c, const char *url,
				       int quiet)
	{
		return parse_credential_url(c, url, 1, quiet);
	}

	int credential_from_url_nonstrict(struct credential *c, const char *url,
					  int quiet)
	{
		return parse_credential_url(c, url, 0, quiet);
	}

[...]
> @@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,
>  	 *   (3) proto://<user>:<pass>@<host>/...
>  	 */
>  	proto_end = strstr(url, "://");
> -	if (!proto_end || proto_end == url) {
> +	if (strict && (!proto_end || proto_end == url)) {
>  		if (!quiet)
>  			warning(_("url has no scheme: %s"), url);
>  		return -1;
>  	}

When !strict, this means we are not requiring a protocol.  No other
difference appears to be intended.

[...]
> @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
>  		host = at + 1;
>  	}
>  
> -	c->protocol = xmemdupz(url, proto_end - url);
> -	c->host = url_decode_mem(host, slash - host);
> +	if (proto_end && proto_end - url > 0)
> +		c->protocol = xmemdupz(url, proto_end - url);

What should happen when the protocol isn't present?  Does this mean
callers will need to be audited to make sure they handle NULL?

> +	if (slash - url > 0)
> +		c->host = url_decode_mem(host, slash - host);

What should happen the URL starts with a slash?

Thanks,
Jonathan

Re: [PATCH 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Jonathan Nieder]

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

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Carlo Arenas]

On Wed, Apr 22, 2020 at 4:41 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
> Johannes Schindelin wrote:
> > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
> >               host = at + 1;
> >       }
> >
> > -     c->protocol = xmemdupz(url, proto_end - url);
> > -     c->host = url_decode_mem(host, slash - host);
> > +     if (proto_end && proto_end - url > 0)
> > +             c->protocol = xmemdupz(url, proto_end - url);
>
> What should happen when the protocol isn't present?  Does this mean
> callers will need to be audited to make sure they handle NULL?

the previous code was ensuring protocol was always at least "" (albeit it
might had been easier to understand with a comment)

removing the proto_end - url check would have the same effect, but then
we will need to also add a explicit xmemdupz("") for the nonstrict part
or audit (and with certainty add) checks to prevent a NULL protocol to
introduce regressions

Carlo

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Johannes Schindelin]

Hi Carlo,

On Wed, 22 Apr 2020, Carlo Arenas wrote:

> On Wed, Apr 22, 2020 at 4:41 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Johannes Schindelin wrote:
> > > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
> > >               host = at + 1;
> > >       }
> > >
> > > -     c->protocol = xmemdupz(url, proto_end - url);
> > > -     c->host = url_decode_mem(host, slash - host);
> > > +     if (proto_end && proto_end - url > 0)
> > > +             c->protocol = xmemdupz(url, proto_end - url);
> >
> > What should happen when the protocol isn't present?  Does this mean
> > callers will need to be audited to make sure they handle NULL?
>
> the previous code was ensuring protocol was always at least "" (albeit it
> might had been easier to understand with a comment)

If you mean v2.17.5 by "the previous code", then yes.

However, what I wanted to reinstate was the behavior of v2.17.4, at least
the behavior that was obviously intended by this code:

	int credential_match(const struct credential *want,
			     const struct credential *have)
	{
	#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
		return CHECK(protocol) &&
		       CHECK(host) &&
		       CHECK(path) &&
		       CHECK(username);
	#undef CHECK
	}

What speaks loudly to me is that _any_ of these can be `NULL` in `want`.
Any of them. Even protocol.

Remember, the call that I modified here to be more lenient populates that
`want`, not that `have`. The `have` still uses the strict mode, and that
is very much by design.

I also saw reports where users try to set `useHTTPPath` for hosts, not for
URLs, and it kind of makes sense. So I wanted to make that work, too.

Except that I uncovered the bug during my investigation that v2.17.4
handled `credential.<hostname>.useHTTPPath` as if no `<hostname>` was
provided at all!

So I wanted to fix that bug as well.

What I do _not_ want is to enforce `protocol` to be set (falling back to
an empty string if not specified). See below as to why.

> removing the proto_end - url check would have the same effect, but then
> we will need to also add a explicit xmemdupz("") for the nonstrict part
> or audit (and with certainty add) checks to prevent a NULL protocol to
> introduce regressions

I fear that my patches did not make it clear that the lenient mode is
_only_ used in the config parsing, in which case we very much do not want
to have the unspecified parts be interpreted as empty strings.

For example, if I specify

	[credential "http://"]
		helper = prevent

to catch _all_ http:// URLs, I definitely do not want that to be used only
for URLs with an empty host name!

Likewise, if I specify

	[credential "dev.azure.com"]
		useHTTPPath = true

I positively want to use the path part of the URL to specify what
credential to use _when accessing http://dev.azure.com/... _or_
https://dev.azure.com/... URLs, I do not expect that setting to be used
only for URLs with an empty protocol (which are now forbidden, anyway,
IIUC)!

So no, that `xmemdupz("")` would introduce very much undesirable behavior,
I believe.

Ciao,
Dscho

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Carlo Marcelo Arenas Belón]

On Thu, Apr 23, 2020 at 03:28:13PM +0200, Johannes Schindelin wrote:
> On Wed, 22 Apr 2020, Carlo Arenas wrote:
> > On Wed, Apr 22, 2020 at 4:41 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
> > > Johannes Schindelin wrote:
> > > > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
> > > >               host = at + 1;
> > > >       }
> > > >
> > > > -     c->protocol = xmemdupz(url, proto_end - url);
> > > > -     c->host = url_decode_mem(host, slash - host);
> > > > +     if (proto_end && proto_end - url > 0)
> > > > +             c->protocol = xmemdupz(url, proto_end - url);
> > >
> > > What should happen when the protocol isn't present?  Does this mean
> > > callers will need to be audited to make sure they handle NULL?
> >
> > the previous code was ensuring protocol was always at least "" (albeit it
> > might had been easier to understand with a comment)
> 
> I fear that my patches did not make it clear that the lenient mode is
> _only_ used in the config parsing, in which case we very much do not want
> to have the unspecified parts be interpreted as empty strings.

I think the concern raised was that since we are using the same function
in both cases there might be unintended consequences on changing the
semantics for the other case.

the argument made by Jonathan to use something else for configuration
(as is done in master) will help in that direction, and might be needed
anyway as your code it otherwise broken for current maint and master, but
if not possible (maybe later?) something like this could probably help :

diff --git a/credential.c b/credential.c
index 88612e583c..f972fcc895 100644
--- a/credential.c
+++ b/credential.c
@@ -389,8 +389,9 @@ int credential_from_url_gently(struct credential *c, const char *url,
 
 	if (proto_end && proto_end - url > 0)
 		c->protocol = xmemdupz(url, proto_end - url);
-	if (slash - url > 0)
+	if (strict || slash > url)
 		c->host = url_decode_mem(host, slash - host);
+
 	/* Trim leading and trailing slashes from path */
 	while (*slash == '/')
 		slash++;

changing the condition there as you suggested to Junio would be a plus IMHO
as well as getting some test that would excercise the new warning that was
introduced in credential.c:57

Carlo

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]

Hi Carlo,

On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:

> On Thu, Apr 23, 2020 at 03:28:13PM +0200, Johannes Schindelin wrote:
> > On Wed, 22 Apr 2020, Carlo Arenas wrote:
> > > On Wed, Apr 22, 2020 at 4:41 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
> > > > Johannes Schindelin wrote:
> > > > > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
> > > > >               host = at + 1;
> > > > >       }
> > > > >
> > > > > -     c->protocol = xmemdupz(url, proto_end - url);
> > > > > -     c->host = url_decode_mem(host, slash - host);
> > > > > +     if (proto_end && proto_end - url > 0)
> > > > > +             c->protocol = xmemdupz(url, proto_end - url);
> > > >
> > > > What should happen when the protocol isn't present?  Does this mean
> > > > callers will need to be audited to make sure they handle NULL?
> > >
> > > the previous code was ensuring protocol was always at least "" (albeit it
> > > might had been easier to understand with a comment)
> >
> > I fear that my patches did not make it clear that the lenient mode is
> > _only_ used in the config parsing, in which case we very much do not want
> > to have the unspecified parts be interpreted as empty strings.
>
> I think the concern raised was that since we are using the same function
> in both cases there might be unintended consequences on changing the
> semantics for the other case.

Indeed, I share that concern. That's why I wanted to be extra careful
there to make sure that introducing this lenient mode does _not_ change
the non-lenient mode in the least, i.e. it is the reason why I kept 2/3
separate from 3/3.

> the argument made by Jonathan to use something else for configuration
> (as is done in master) will help in that direction, and might be needed
> anyway as your code it otherwise broken for current maint and master,

I am in general not a fan of the idea to have two separate parsers for
essentially the same thing. In this instance, the difference between the
lenient mode and the non-lenient mode is so obvious that I'd rather reuse
the same code (think: Don't Repeat Yourself).

> but if not possible (maybe later?) something like this could probably
> help :
>
> diff --git a/credential.c b/credential.c
> index 88612e583c..f972fcc895 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -389,8 +389,9 @@ int credential_from_url_gently(struct credential *c, const char *url,
>
>  	if (proto_end && proto_end - url > 0)
>  		c->protocol = xmemdupz(url, proto_end - url);
> -	if (slash - url > 0)
> +	if (strict || slash > url)
>  		c->host = url_decode_mem(host, slash - host);
> +
>  	/* Trim leading and trailing slashes from path */
>  	while (*slash == '/')
>  		slash++;
>
> changing the condition there as you suggested to Junio would be a plus IMHO
> as well as getting some test that would excercise the new warning that was
> introduced in credential.c:57

Yes (modulo doing "greater than" comparison on pointers which is IIRC not
permitted in C in general).

Ciao,
Dscho

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Yes (modulo doing "greater than" comparison on pointers which is IIRC not
> permitted in C in general).

Of course, people write a loop like this

	char *cp, *ep = strchr(string, '\n');

	for (cp = string; cp < ep; cp++)
		...

all the time, and forbidding pointer comparison would make the
language impossible to use ;-)

I think you are confused with a different rule---what is not kosher
is to compare two pointers that do not point into elements of the
same array.  Whether the comparison is done in (ptr1 < ptr2) way, or
(ptr2 - ptr1 < 0) way, does not change the equation.

Re: [PATCH 0/3] credential: handle partial URLs in config settings again

[Johannes Schindelin]

Hi Jonathan,

On Wed, 22 Apr 2020, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> > On Wed, 22 Apr 2020, Jeff King wrote:
> >> On Wed, Apr 22, 2020 at 08:51:02PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> >>> 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:
> >>>
> >>>   $ echo url=https://example.com |
> >>>     git -c credential.totally.bog.us.username=foo credential fill
> >>
> >> That seems scary. What if it is not .username, but:
> >>
> >>   [credential "example.com"]
> >>   username = foo
> >>   helper = "!echo password=bar"
> >>
> >> ? (Or you can imagine a helper that is pulling from a read-only store,
> >> like "pass"). That would send the credential to the wrong host.
> >
> > It would. But I am not aware of any implications regarding `.gitmodules`
> > (for some reason I now expect every bug to open an attack vector via
> > submodules, I wonder why that is), so that's at least good.
>
> Submodules are only one of many ways that people end up cloning from
> an attacker-controlled URL.

Yet the vast majority of the vulnerabilities in Git that we fixed over the
last years has involved submodules in their attack vector.

> In submodules we're careful not to use --recurse-submodules by default
> at clone time.  So I'll mentally subsitute "attacker-controlled URLs"
> where you say "submodules". ;-)

Say what you will about this, but the practical reality seems to be that
most of the security bugs in Git affected submodule users.

Somebody even muttered the suggestion to me that submodules should be
deprecated already.

> I agree with Peff's concern about the unpatched state: since there are
> people who use `[credential "host.example.com"] helper` and there are
> credential helpers that ignore the host passed in, the combination can
> hurt people.  (Fortunately, there aren't many credential helpers in
> that category that are commonly used.)

Yes. That's why I want to make sure that the URL parser we use here is
strict by default, and lenient _only_ when parsing the config (because
then, it is more like a URL *pattern* used for matching, the parsed URL is
never used directly).

> [...]
> > Yes. For the record, I tried to be very careful here. The _only_ code path
> > that is affected by this change is the config reading.
> [...]
> > But again, I would love another pair of eyes on this, to confirm my
> > analysis.
>
> As mentioned above, the config reading is sensitive, too.  That said,
> I suspect you got it to do basically the right thing.
>
> Reading through the patches.  Thank you.

Thank you very much!
Dscho

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Yes (modulo doing "greater than" comparison on pointers which is IIRC not
>> permitted in C in general).
>
> Of course, people write a loop like this
>
> 	char *cp, *ep = strchr(string, '\n');
>
> 	for (cp = string; cp < ep; cp++)
> 		...
>
> all the time, and forbidding pointer comparison would make the
> language impossible to use ;-)
>
> I think you are confused with a different rule---what is not kosher
> is to compare two pointers that do not point into elements of the
> same array.  Whether the comparison is done in (ptr1 < ptr2) way, or
> (ptr2 - ptr1 < 0) way, does not change the equation.

Having said that, between

1.	if (strict || slash - url > 0)
2.	if (strict || slash > url)
 		c->host = url_decode_mem(host, slash - host);

I think the former is moderately easier to read.  It still has the
same "Huh?" factor that a comparison between slash and URL guards
the size of the region being decoded, which is slash - host, and
makes the reader wonder how these two variables, URL and host,
relate to each other at this point in the code, though, either way
the comparison is spelled.

Thanks.

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Johannes Schindelin]

Hi Jonathan,

On Wed, 22 Apr 2020, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
>
> > There was one call site, though, that really needed that leniency: when
> > parsing config settings a la `credential.dev.azure.com.useHTTPPath`.
>
> Thanks for tackling this.
>
> Can the commit message say a little more about the semantics and when
> someone would use this?
>
> Is it a shortcut for
>
> 	[credential "http://dev.azure.com"]
> 		useHttpPath = true
>
> 	[credential "https://dev.azure.com"]
> 		useHttpPath = true
>
> ?

I don't really want to be too verbose about this in _this_ commit message.
But I do see your point. This is my current version of the commit message:

    credential: optionally allow partial URLs in credential_from_url_gently()

    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 add a parameter called
    `allow_partial_url` to the `credential_from_url_gently()` function and
    convert the existing callers to set that parameter to 0, i.e. do not
    change the existing behavior, just add the option to be more lenient.

    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.

As you see, I added substantially more information there.

> > In preparation for fixing that regression, let's add a parameter
> > called `strict` to the `credential_from_url()` function and convert
> > the existing callers to enforce that strict mode.
>
> I suspect this would be easier to read squashed with patch 3.  That
> would also mean that the functionality and test coverage come at the
> same time.

On the contrary, I _really_ want them to be separate, so that it is easier
to review. I know that _I_ had an easier time looking over the patch that
introduces the non-strict mode, to make sure that it does not change a
thing in strict mode (and yes, v1 made that harder than necessary by _not_
using `strict ||` in the condition that guards the `c->host` assignment).

In short: it is my intention to keep the two patches separate, with the
main goal to make sure that I don't introduce any of those regressions
Peff is worried about.

> [...]
> > diff --git a/credential.c b/credential.c
> > index 64a841eddca..c73260ac40f 100644
> > --- a/credential.c
> > +++ b/credential.c
> > @@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,
> >  }
> >
> >  int credential_from_url_gently(struct credential *c, const char *url,
> > -			       int quiet)
> > +			       int strict, int quiet)
>
> The collection of flags makes me wonder whether it's time to use a
> single "flags" parameter with flags that are |ed together.  That way,
> call sites are easier to read without requiring cross-reference
> assistance to see which option each boolean parameter represents.

I resisted the temptation because I don't want this to be a big patch
series, but a fast one. The reports keep coming in, and the current plan
seems to be for GitHub Desktop to release another version on Monday, for
which I need to release a MinGit backport probably tomorrow, otherwise it
won't work timewise.

Meaning: I don't really want to do "nice to have" work like this flag.

> Alternatively, could the non-strict form be a separate public function
> that uses the same static helper that takes two boolean args?  That is,
> something like
>
> 	int credential_from_url_gently(struct credential *c, const char *url,
> 				       int quiet)
> 	{
> 		return parse_credential_url(c, url, 1, quiet);
> 	}
>
> 	int credential_from_url_nonstrict(struct credential *c, const char *url,
> 					  int quiet)
> 	{
> 		return parse_credential_url(c, url, 0, quiet);
> 	}

Looks good, but there are only three callers (and I don't expect more).
And the only caller interested in the lenient mode (read: the only
now-complicated call) lives in the very same file as the called function.
So it strikes me as quite a bit of an overkill to introduce two new
functions.

> [...]
> > @@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,
> >  	 *   (3) proto://<user>:<pass>@<host>/...
> >  	 */
> >  	proto_end = strstr(url, "://");
> > -	if (!proto_end || proto_end == url) {
> > +	if (strict && (!proto_end || proto_end == url)) {
> >  		if (!quiet)
> >  			warning(_("url has no scheme: %s"), url);
> >  		return -1;
> >  	}
>
> When !strict, this means we are not requiring a protocol.  No other
> difference appears to be intended.

Almost. My intention was to handle missing host names, too: `/repo.git`
should also match `https://example.com/repo.git`.

The user name is optional already, anyway.

BTW I realized (while working on these patches) that it would probably be
a good idea to warn about passwords in these `credential.<URL>.<key>`
settings: the password will be ignored as far as `credential_match()` is
concerned, and they should probably not live in the config.

But for aforementioned reasons, I decided against including a patch that
makes that happen.

> [...]
> > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
> >  		host = at + 1;
> >  	}
> >
> > -	c->protocol = xmemdupz(url, proto_end - url);
> > -	c->host = url_decode_mem(host, slash - host);
> > +	if (proto_end && proto_end - url > 0)
> > +		c->protocol = xmemdupz(url, proto_end - url);
>
> What should happen when the protocol isn't present?  Does this mean
> callers will need to be audited to make sure they handle NULL?

Again, the sole caller of this lenient mode is the config parser which
then wants to match the (partial) URL against the actual URL, using this
function:

	int credential_match(const struct credential *want,
			     const struct credential *have)
	{
	#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
		return CHECK(protocol) &&
		       CHECK(host) &&
		       CHECK(path) &&
		       CHECK(username);
	#undef CHECK
	}

The lenient parser is supposed to populate the `want` of this function. In
other words, the code _expects_ `protocol` (or for that matter, _any_
attribute of `want`) to be optional.

> > +	if (slash - url > 0)
> > +		c->host = url_decode_mem(host, slash - host);
>
> What should happen the URL starts with a slash?

Oh... I thought it was obvious that a partial URL starting with a slash
would refer to the path part only... Does that not make sense?

Ciao,
Dscho

Re: [PATCH 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin]

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

Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

[Johannes Schindelin]

Hi Junio,

On Thu, 23 Apr 2020, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> Yes (modulo doing "greater than" comparison on pointers which is IIRC not
> >> permitted in C in general).
> >
> > Of course, people write a loop like this
> >
> > 	char *cp, *ep = strchr(string, '\n');
> >
> > 	for (cp = string; cp < ep; cp++)
> > 		...
> >
> > all the time, and forbidding pointer comparison would make the
> > language impossible to use ;-)
> >
> > I think you are confused with a different rule---what is not kosher
> > is to compare two pointers that do not point into elements of the
> > same array.  Whether the comparison is done in (ptr1 < ptr2) way, or
> > (ptr2 - ptr1 < 0) way, does not change the equation.

Yep, that's my confusion all right.

> Having said that, between
>
> 1.	if (strict || slash - url > 0)
> 2.	if (strict || slash > url)
>  		c->host = url_decode_mem(host, slash - host);
>
> I think the former is moderately easier to read.  It still has the
> same "Huh?" factor that a comparison between slash and URL guards
> the size of the region being decoded, which is slash - host, and
> makes the reader wonder how these two variables, URL and host,
> relate to each other at this point in the code, though, either way
> the comparison is spelled.

I fully agree! That's why I use `strict || slash - host > 0` in my next
iteration (actually, I decided to rename `strict`, but that's beside the
point).

Ciao,
Dscho

[PATCH v2 0/3] credential: handle partial URLs in config settings again

[Johannes Schindelin via GitGitGadget]

This fixes the problem illustrated by Peff's example
[https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]
, in maint-2.17:

  $ echo url=https://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

The fix is necessarily different than what was proposed by brian
[https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/] 
because that fix targets v2.26.x which has 46fd7b390034 (credential: allow
wildcard patterns when matching config, 2020-02-20).

This patch series targets maint-2.17 instead (and might actually not be able
to fix maint due 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:

  $ echo url=https://example.com |
    git -c credential.totally.bog.us.username=foo credential fill

Obviously, this is unexpected, as a Git config like this would leave the
last specified user name as "winner":

[credential "first.com"]
    username = Whos On
[credential "second.com"]
    username = Who

This patch series fixes this. The quoted part of [credential "<value>"] will
be interpreted as a partial URL:

 * It can start with a protocol followed by ://, but does not have to.
 * If it starts with a protocol, the host name will always be set (if the 
   :// is followed immediately by yet another slash, then it will be set to
   the empty string).
 * If it starts without a protocol, it is treated as a path if the value
   starts with a slash (and the host will be left unset).
 * If it starts without a protocol and the first character is not a slash,
   it will be treated as a host name, optionally followed by a slash and the
   path.

Changes since v1:

 * The condition when the c->host field is set was made more intuitive.
 * The "fix grammar" commit now has Jonathan's "reviewed-by" footer.
 * Inverted the meaning of the parameter strict and renamed it to 
   allow_partial_urls, to clarify its role.
 * Enhanced the commit message of the second patch to illustrate the
   motivation for the lenient mode a bit better.
 * [Not a change] I did leave the second and the third patch separate from
   one another. This makes it a lot easier to follow the iteration and to
   keep the reviews straight: it separates the "how do we make URL parts
   optional?" from the "where do we need URL parts to be optional?"
 * A partial URL https:// is now correctly interpreted as having only the
   protocol set, but not host name nor path.
 * The lenient mode is now explained a lot more verbosely in the code
   comment in credential.h.
 * When skipping a config setting, we now show the config key, not just the
   URL (which might be incomplete, i.e. not actually a URL).
 * When skipping a config setting, the correct struct credential is cleared
   (i.e. the just-parsed one, not the one against which we wanted to match
   the just-parsed one).
 * Added a ton more partial URLs to the test, and the test now also verifies
   that the warning comes out all right.

Johannes Schindelin (3):
  credential: fix grammar
  credential: optionally allow partial URLs in
    credential_from_url_gently()
  credential: handle `credential.<partial-URL>.<key>` again

 credential.c           | 22 +++++++++++++++-------
 credential.h           | 28 ++++++++++++++++++++++++++--
 fsck.c                 |  2 +-
 t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 81 insertions(+), 10 deletions(-)


base-commit: df5be6dc3fd18c294ec93a9af0321334e3f92c9c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-615%2Fdscho%2Fcredential-config-partial-url-maint-2.17-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-615/dscho/credential-config-partial-url-maint-2.17-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/615

Range-diff vs v1:

 1:  2bf781081d9 ! 1:  2c1c0ae91eb credential: fix grammar
     @@ Commit message
          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>
      
       ## credential.h ##
 2:  1081841b16d ! 2:  fc772d21b74 credential: teach `credential_from_url()` a non-strict mode
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    credential: teach `credential_from_url()` a non-strict mode
     +    credential: optionally allow partial URLs in credential_from_url_gently()
      
          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`.
     @@ Commit message
      
          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 regression, let's add a parameter called
     -    `strict` to the `credential_from_url()` function and convert the
     -    existing callers to enforce that strict mode.
     +    In preparation for fixing that bug, let's add a parameter called
     +    `allow_partial_url` to the `credential_from_url_gently()` function and
     +    convert the existing callers to set that parameter to 0, i.e. do not
     +    change the existing behavior, just add the option to be more lenient.
     +
     +    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>
      
     @@ credential.c: static int check_url_component(const char *url, int quiet,
       
       int credential_from_url_gently(struct credential *c, const char *url,
      -			       int quiet)
     -+			       int strict, int quiet)
     ++			       int allow_partial_url, int quiet)
       {
       	const char *at, *colon, *cp, *slash, *host, *proto_end;
       
     @@ credential.c: int credential_from_url_gently(struct credential *c, const char *u
       	 */
       	proto_end = strstr(url, "://");
      -	if (!proto_end || proto_end == url) {
     -+	if (strict && (!proto_end || proto_end == url)) {
     ++	if (!allow_partial_url && (!proto_end || proto_end == url)) {
       		if (!quiet)
       			warning(_("url has no scheme: %s"), url);
       		return -1;
     @@ credential.c: int credential_from_url_gently(struct credential *c, const char *u
      -	c->host = url_decode_mem(host, slash - host);
      +	if (proto_end && proto_end - url > 0)
      +		c->protocol = xmemdupz(url, proto_end - url);
     -+	if (slash - url > 0)
     ++	if (!allow_partial_url || slash - host > 0)
      +		c->host = url_decode_mem(host, slash - host);
       	/* Trim leading and trailing slashes from path */
       	while (*slash == '/')
     @@ credential.c: int credential_from_url_gently(struct credential *c, const char *u
       void credential_from_url(struct credential *c, const char *url)
       {
      -	if (credential_from_url_gently(c, url, 0) < 0)
     -+	if (credential_from_url_gently(c, url, 1, 0) < 0)
     ++	if (credential_from_url_gently(c, url, 0, 0) < 0)
       		die(_("credential url cannot be parsed: %s"), url);
       }
      
     @@ credential.h: void credential_write(const struct credential *, FILE *);
        * examination.  The non-gentle form will issue a warning to stderr and return
        * an empty credential.
      + *
     -+ * In strict mode, an empty protocol or an empty host name are not allowed.
     -+ * The credential_from_url() function enforces strict mode.
     ++ * If allow_partial_url is non-zero, partial URLs are allowed, i.e. it can, but
     ++ * does not have to, contain
     ++ *
     ++ * - a protocol (or scheme) of the form "<protocol>://"
     ++ *
     ++ * - a host name (the part after the protocol and before the first slash after
     ++ *   that, if any)
     ++ *
     ++ * - a user name and potentially a password (as "<user>[:<password>]@" part of
     ++ *   the host name)
     ++ *
     ++ * - a path (the part after the host name, if any, starting with the slash)
     ++ *
     ++ * Missing parts will be left unset in `struct credential`. Thus, `https://`
     ++ * will have only the `protocol` set, `example.com` only the host name, and
     ++ * `/git` only the path.
     ++ *
     ++ * Note that an empty host name in an otherwise fully-qualified URL will be
     ++ * treated as unset when allow_partial_url is non-zero (and only then,
     ++ * otherwise it is treated as the empty string).
     ++ *
     ++ * The credential_from_url() function does not allow partial URLs.
        */
       void credential_from_url(struct credential *, const char *url);
      -int credential_from_url_gently(struct credential *, const char *url, int quiet);
      +int credential_from_url_gently(struct credential *, const char *url,
     -+			       int strict, int quiet);
     ++			       int allow_partial_url, int quiet);
       
       int credential_match(const struct credential *have,
       		     const struct credential *want);
     @@ fsck.c: static int check_submodule_url(const char *url)
       		struct credential c = CREDENTIAL_INIT;
       		int ret = 0;
      -		if (credential_from_url_gently(&c, curl_url, 1) ||
     -+		if (credential_from_url_gently(&c, curl_url, 1, 1) ||
     ++		if (credential_from_url_gently(&c, curl_url, 0, 1) ||
       		    !*c.host)
       			ret = -1;
       		credential_clear(&c);
 3:  66823c735b1 ! 3:  daedaffe960 credential: handle `credential.<partial-URL>.<key>` again
     @@ Commit message
      
          Let's reinstate this behavior.
      
     -    Original-test-case-by: Jeff King <peff@peff.net>
     +    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>
      
       ## credential.c ##
     @@ credential.c: static int credential_config_callback(const char *var, const char
       		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);
     ++		if (credential_from_url_gently(&want, url, 1, 0) < 0) {
     ++			warning(_("skipping credential lookup for key: %s"),
     ++				var);
     ++			credential_clear(&want);
      +			free(url);
      +			return 0;
      +		}
     @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w
       	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 &&
     -+	cat >expect <<-EOF &&
     -+	protocol=https
     -+	host=example.com
     -+	username=boo
     -+	password=askpass-password
     -+	EOF
     -+	test_cmp expect actual
     ++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 >input &&
     ++	for partial in \
     ++		example.com \
     ++		user@example.com \
     ++		https:// \
     ++		https://example.com \
     ++		https://example.com/ \
     ++		https://user@example.com \
     ++		https://user@example.com/ \
     ++		https://example.com/repo.git \
     ++		https://user@example.com/repo.git \
     ++		/repo.git
     ++	do
     ++		git -c credential.$partial.helper=yep \
     ++			credential fill <input >output &&
     ++		grep yep output ||
     ++		return 1
     ++	done &&
     ++
     ++	for partial in \
     ++		dont.use.this \
     ++		http:// \
     ++		/repo
     ++	do
     ++		git -c credential.$partial.helper=yep \
     ++			credential fill <input >output &&
     ++		! grep yep output ||
     ++		return 1
     ++	done &&
     ++
     ++	git -c credential.$partial.helper=yep \
     ++		-c credential.with%0anewline.username=uh-oh \
     ++		credential fill <input >output 2>error &&
     ++	test_i18ngrep "skipping credential lookup for key" error
     ++
      +'
      +
       test_done

-- 
gitgitgadget

[PATCH v2 1/3] credential: fix grammar

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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>
---
 credential.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential.h b/credential.h
index 122a23cd2f1..5a86502d95c 100644
--- a/credential.h
+++ b/credential.h
@@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *);
 /*
  * Parse a url into a credential struct, replacing any existing contents.
  *
- * Ifthe url can't be parsed (e.g., a missing "proto://" component), the
+ * If the url can't be parsed (e.g., a missing "proto://" component), the
  * resulting credential will be empty but we'll still return success from the
  * "gently" form.
  *
-- 
gitgitgadget

[PATCH v2 2/3] credential: optionally allow partial URLs in credential_from_url_gently()

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 add a parameter called
`allow_partial_url` to the `credential_from_url_gently()` function and
convert the existing callers to set that parameter to 0, i.e. do not
change the existing behavior, just add the option to be more lenient.

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>
---
 credential.c | 14 ++++++++------
 credential.h | 26 +++++++++++++++++++++++++-
 fsck.c       |  2 +-
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/credential.c b/credential.c
index 64a841eddca..52965a5122c 100644
--- a/credential.c
+++ b/credential.c
@@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,
 }
 
 int credential_from_url_gently(struct credential *c, const char *url,
-			       int quiet)
+			       int allow_partial_url, int quiet)
 {
 	const char *at, *colon, *cp, *slash, *host, *proto_end;
 
@@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,
 	 *   (3) proto://<user>:<pass>@<host>/...
 	 */
 	proto_end = strstr(url, "://");
-	if (!proto_end || proto_end == url) {
+	if (!allow_partial_url && (!proto_end || proto_end == url)) {
 		if (!quiet)
 			warning(_("url has no scheme: %s"), url);
 		return -1;
 	}
-	cp = proto_end + 3;
+	cp = proto_end ? proto_end + 3 : url;
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');
@@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
 		host = at + 1;
 	}
 
-	c->protocol = xmemdupz(url, proto_end - url);
-	c->host = url_decode_mem(host, slash - host);
+	if (proto_end && proto_end - url > 0)
+		c->protocol = xmemdupz(url, proto_end - url);
+	if (!allow_partial_url || slash - host > 0)
+		c->host = url_decode_mem(host, slash - host);
 	/* Trim leading and trailing slashes from path */
 	while (*slash == '/')
 		slash++;
@@ -407,6 +409,6 @@ int credential_from_url_gently(struct credential *c, const char *url,
 
 void credential_from_url(struct credential *c, const char *url)
 {
-	if (credential_from_url_gently(c, url, 0) < 0)
+	if (credential_from_url_gently(c, url, 0, 0) < 0)
 		die(_("credential url cannot be parsed: %s"), url);
 }
diff --git a/credential.h b/credential.h
index 5a86502d95c..5abc865b95d 100644
--- a/credential.h
+++ b/credential.h
@@ -41,9 +41,33 @@ void credential_write(const struct credential *, FILE *);
  * an error but leave the broken state in the credential object for further
  * examination.  The non-gentle form will issue a warning to stderr and return
  * an empty credential.
+ *
+ * If allow_partial_url is non-zero, partial URLs are allowed, i.e. it can, but
+ * does not have to, contain
+ *
+ * - a protocol (or scheme) of the form "<protocol>://"
+ *
+ * - a host name (the part after the protocol and before the first slash after
+ *   that, if any)
+ *
+ * - a user name and potentially a password (as "<user>[:<password>]@" part of
+ *   the host name)
+ *
+ * - a path (the part after the host name, if any, starting with the slash)
+ *
+ * Missing parts will be left unset in `struct credential`. Thus, `https://`
+ * will have only the `protocol` set, `example.com` only the host name, and
+ * `/git` only the path.
+ *
+ * Note that an empty host name in an otherwise fully-qualified URL will be
+ * treated as unset when allow_partial_url is non-zero (and only then,
+ * otherwise it is treated as the empty string).
+ *
+ * The credential_from_url() function does not allow partial URLs.
  */
 void credential_from_url(struct credential *, const char *url);
-int credential_from_url_gently(struct credential *, const char *url, int quiet);
+int credential_from_url_gently(struct credential *, const char *url,
+			       int allow_partial_url, int quiet);
 
 int credential_match(const struct credential *have,
 		     const struct credential *want);
diff --git a/fsck.c b/fsck.c
index 31b5be05f54..cec97b63901 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1076,7 +1076,7 @@ static int check_submodule_url(const char *url)
 	else if (url_to_curl_url(url, &curl_url)) {
 		struct credential c = CREDENTIAL_INIT;
 		int ret = 0;
-		if (credential_from_url_gently(&c, curl_url, 1) ||
+		if (credential_from_url_gently(&c, curl_url, 0, 1) ||
 		    !*c.host)
 			ret = -1;
 		credential_clear(&c);
-- 
gitgitgadget

[PATCH v2 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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>
---
 credential.c           |  8 +++++++-
 t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/credential.c b/credential.c
index 52965a5122c..3505f6356d8 100644
--- a/credential.c
+++ b/credential.c
@@ -53,7 +53,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_url_gently(&want, url, 1, 0) < 0) {
+			warning(_("skipping credential lookup for key: %s"),
+				var);
+			credential_clear(&want);
+			free(url);
+			return 0;
+		}
 		matched = credential_match(&want, c);
 
 		credential_clear(&want);
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index efed3ea2955..6fff76cb932 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 >input &&
+	for partial in \
+		example.com \
+		user@example.com \
+		https:// \
+		https://example.com \
+		https://example.com/ \
+		https://user@example.com \
+		https://user@example.com/ \
+		https://example.com/repo.git \
+		https://user@example.com/repo.git \
+		/repo.git
+	do
+		git -c credential.$partial.helper=yep \
+			credential fill <input >output &&
+		grep yep output ||
+		return 1
+	done &&
+
+	for partial in \
+		dont.use.this \
+		http:// \
+		/repo
+	do
+		git -c credential.$partial.helper=yep \
+			credential fill <input >output &&
+		! grep yep output ||
+		return 1
+	done &&
+
+	git -c credential.$partial.helper=yep \
+		-c credential.with%0anewline.username=uh-oh \
+		credential fill <input >output 2>error &&
+	test_i18ngrep "skipping credential lookup for key" error
+
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Carlo Marcelo Arenas Belón]

On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/credential.c b/credential.c
> index 52965a5122c..3505f6356d8 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -53,7 +53,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_url_gently(&want, url, 1, 0) < 0) {

definitely not worth a reroll, but just wondering if would make sense to call
credential_from_url_gently(!quiet) here, just for consistency?

other than that this series is looking great, under the assumption that there
is going to be some more followup with non essential changes.

will chip in with an test helper for that series so we can hopefully keep our
sanity next time someone touches that function again.

Carlo

Re: [PATCH v2 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

Hi Carlo,

On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:

> On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/credential.c b/credential.c
> > index 52965a5122c..3505f6356d8 100644
> > --- a/credential.c
> > +++ b/credential.c
> > @@ -53,7 +53,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_url_gently(&want, url, 1, 0) < 0) {
>
> definitely not worth a reroll, but just wondering if would make sense to call
> credential_from_url_gently(!quiet) here, just for consistency?

We don't have any `quiet` variable here.

> other than that this series is looking great, under the assumption that there
> is going to be some more followup with non essential changes.

I am sure I don't follow. What non-essential changes are you assuming will
follow up?

> will chip in with an test helper for that series so we can hopefully keep our
> sanity next time someone touches that function again.

Are the tests I added to t0300 not enough? Or do you think it will need a
native test helper that is included in `t/helper/test-tool` and exercised
in the test suite somehow?

Ciao,
Dscho

Re: [PATCH v2 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
>> diff --git a/credential.c b/credential.c
>> index 52965a5122c..3505f6356d8 100644
>> --- a/credential.c
>> +++ b/credential.c
>> @@ -53,7 +53,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_url_gently(&want, url, 1, 0) < 0) {
>
> definitely not worth a reroll, but just wondering if would make sense to call
> credential_from_url_gently(!quiet) here, just for consistency?

Speaking of which, it is not clear which one of "...url, 1, 0)" is
the "quiet" bit.  I somehow thought that somebody suggested to roll
these two into a flags word and give quiet and the other bit a name,
and after seeing this line, I tend to agree that would be great for
readability.

> other than that this series is looking great, under the assumption that there
> is going to be some more followup with non essential changes.
>
> will chip in with an test helper for that series so we can hopefully keep our
> sanity next time someone touches that function again.

Thanks, everybody, for help polishing this topic.

Re: [PATCH v2 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Carlo Marcelo Arenas Belón]

On Fri, Apr 24, 2020 at 02:16:45AM +0200, Johannes Schindelin wrote:
> On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:
> > On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > diff --git a/credential.c b/credential.c
> > > index 52965a5122c..3505f6356d8 100644
> > > --- a/credential.c
> > > +++ b/credential.c
> > > @@ -53,7 +53,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_url_gently(&want, url, 1, 0) < 0) {
> >
> > definitely not worth a reroll, but just wondering if would make sense to call
> > credential_from_url_gently(!quiet) here, just for consistency?
> 
> We don't have any `quiet` variable here.

my bad, should had been more explicit as it is confusing with all those
booleans at the end without a flags parameter.

I mean the call should be instead :

  if (credential_from_url_gently(&want, url, 1, 1) < 0) {

since we want to warn if the configuration is not supported just like is
done after this check.

> 
> > other than that this series is looking great, under the assumption that there
> > is going to be some more followup with non essential changes.
> 
> I am sure I don't follow. What non-essential changes are you assuming will
> follow up?

the ones that were discussed with Jonathan in a differen thread :

* using a flags parameter instead of two ints
* whatever will be needed so it also works in master and maint

> > will chip in with an test helper for that series so we can hopefully keep our
> > sanity next time someone touches that function again.
> 
> Are the tests I added to t0300 not enough? Or do you think it will need a
> native test helper that is included in `t/helper/test-tool` and exercised
> in the test suite somehow?

I think they are enough, it is only that is easier to quickly iterate with
possible bad input with a helper. which is what I was offering for the next
thread since its need is orthogonal to what you are trying to accomplish.

Carlo

Re: [PATCH v2 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
>
>> On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
>>> diff --git a/credential.c b/credential.c
>>> index 52965a5122c..3505f6356d8 100644
>>> --- a/credential.c
>>> +++ b/credential.c
>>> @@ -53,7 +53,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_url_gently(&want, url, 1, 0) < 0) {
>>
>> definitely not worth a reroll, but just wondering if would make sense to call
>> credential_from_url_gently(!quiet) here, just for consistency?
>
> Speaking of which, it is not clear which one of "...url, 1, 0)" is
> the "quiet" bit.  I somehow thought that somebody suggested to roll
> these two into a flags word and give quiet and the other bit a name,
> and after seeing this line, I tend to agree that would be great for
> readability.

Ah, I should have checked before opening my mouth.  It was this
message <20200422233854.GE140314@google.com> from Jonathan Nieder.

I also am OK with his "two thin wrappers around the underlying
helper that takes two separate arguments", if that makes the
resulting code easier to follow.  I have a feeling that the caller
knows (from the context, or the reason why it calls the
credential-from-url code) if it wants strict or nonstrict variant
and that is not something the caller is told from its caller.  And
if that is the case, "two thin wrappers" approach does make a lot of
sense.

Thanks.

Re: [PATCH v2 0/3] credential: handle partial URLs in config settings again

[Carlo Marcelo Arenas Belón]

Reviewed-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>

for the whole series

Re: [PATCH v2 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3098 bytes --]

Hi Carlo,

On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:

> On Fri, Apr 24, 2020 at 02:16:45AM +0200, Johannes Schindelin wrote:
> > On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:
> > > On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > > diff --git a/credential.c b/credential.c
> > > > index 52965a5122c..3505f6356d8 100644
> > > > --- a/credential.c
> > > > +++ b/credential.c
> > > > @@ -53,7 +53,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_url_gently(&want, url, 1, 0) < 0) {
> > >
> > > definitely not worth a reroll, but just wondering if would make sense to call
> > > credential_from_url_gently(!quiet) here, just for consistency?
> >
> > We don't have any `quiet` variable here.
>
> my bad, should had been more explicit as it is confusing with all those
> booleans at the end without a flags parameter.
>
> I mean the call should be instead :
>
>   if (credential_from_url_gently(&want, url, 1, 1) < 0) {
>
> since we want to warn if the configuration is not supported just like is
> done after this check.

Since Junio expressed support for Jonathan's idea of using separate
functions that wrap one helper function, I went with that instead.

> > > other than that this series is looking great, under the assumption that there
> > > is going to be some more followup with non essential changes.
> >
> > I am sure I don't follow. What non-essential changes are you assuming will
> > follow up?
>
> the ones that were discussed with Jonathan in a differen thread :
>
> * using a flags parameter instead of two ints
> * whatever will be needed so it also works in master and maint

Oy, I am still under water just trying to get the MinGit for Windows
backports updated so that users can upgrade instead of complaining on bug
trackers and on Twitter...

But yes, that will be the next step. Now that I have a comprehensive test
case, it should be relatively easy.

> > > will chip in with an test helper for that series so we can hopefully keep our
> > > sanity next time someone touches that function again.
> >
> > Are the tests I added to t0300 not enough? Or do you think it will need a
> > native test helper that is included in `t/helper/test-tool` and exercised
> > in the test suite somehow?
>
> I think they are enough, it is only that is easier to quickly iterate with
> possible bad input with a helper. which is what I was offering for the next
> thread since its need is orthogonal to what you are trying to accomplish.

Usually I would agree with you. It's quicker to iterate, and a ton faster
to run (especially on Windows).

In this instance, I am going for a regression test case rather than a unit
test, though, because I really need to ensure that what end users are
trying to do on their machines will work (and continue to work).

Ciao,
Dscho

Re: [PATCH v2 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1854 bytes --]

Hi Junio,

On Thu, 23 Apr 2020, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> >
> >> On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
> >>> diff --git a/credential.c b/credential.c
> >>> index 52965a5122c..3505f6356d8 100644
> >>> --- a/credential.c
> >>> +++ b/credential.c
> >>> @@ -53,7 +53,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_url_gently(&want, url, 1, 0) < 0) {
> >>
> >> definitely not worth a reroll, but just wondering if would make sense to call
> >> credential_from_url_gently(!quiet) here, just for consistency?
> >
> > Speaking of which, it is not clear which one of "...url, 1, 0)" is
> > the "quiet" bit.  I somehow thought that somebody suggested to roll
> > these two into a flags word and give quiet and the other bit a name,
> > and after seeing this line, I tend to agree that would be great for
> > readability.
>
> Ah, I should have checked before opening my mouth.  It was this
> message <20200422233854.GE140314@google.com> from Jonathan Nieder.
>
> I also am OK with his "two thin wrappers around the underlying
> helper that takes two separate arguments", if that makes the
> resulting code easier to follow.  I have a feeling that the caller
> knows (from the context, or the reason why it calls the
> credential-from-url code) if it wants strict or nonstrict variant
> and that is not something the caller is told from its caller.  And
> if that is the case, "two thin wrappers" approach does make a lot of
> sense.

All right, two wrapper functions it is.

Ciao,
Dscho

[PATCH v3 0/3] credential: handle partial URLs in config settings again

[Johannes Schindelin via GitGitGadget]

This fixes the problem illustrated by Peff's example
[https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]
, in maint-2.17:

  $ echo url=https://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

The fix is necessarily different than what was proposed by brian
[https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/] 
because that fix targets v2.26.x which has 46fd7b390034 (credential: allow
wildcard patterns when matching config, 2020-02-20).

This patch series targets maint-2.17 instead (and might actually not be able
to fix maint due 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:

  $ echo url=https://example.com |
    git -c credential.totally.bog.us.username=foo credential fill

Obviously, this is unexpected, as a Git config like this would leave the
last specified user name as "winner":

[credential "first.com"]
    username = Whos On
[credential "second.com"]
    username = Who

This patch series fixes this. The quoted part of [credential "<value>"] will
be interpreted as a partial URL:

 * It can start with a protocol followed by ://, but does not have to.
 * If it starts with a protocol, the host name will always be set (if the 
   :// is followed immediately by yet another slash, then it will be set to
   the empty string).
 * If it starts without a protocol, it is treated as a path if the value
   starts with a slash (and the host will be left unset).
 * If it starts without a protocol and the first character is not a slash,
   it will be treated as a host name, optionally followed by a slash and the
   path.

Changes since v2:

 * Refactored out the credential_from_potentially_partial_url() function so
   that existing callers (except for the one we want to change) stay
   untouched.
 * When encountering an invalid URL while parsing the config, we no longer
   suppress the parser's warning.
 * The test now uses the file name convention stdin/stdout/stderr of 
   t/lib-credential.sh.

Changes since v1:

 * The condition when the c->host field is set was made more intuitive.
 * The "fix grammar" commit now has Jonathan's "reviewed-by" footer.
 * Inverted the meaning of the parameter strict and renamed it to 
   allow_partial_urls, to clarify its role.
 * Enhanced the commit message of the second patch to illustrate the
   motivation for the lenient mode a bit better.
 * [Not a change] I did leave the second and the third patch separate from
   one another. This makes it a lot easier to follow the iteration and to
   keep the reviews straight: it separates the "how do we make URL parts
   optional?" from the "where do we need URL parts to be optional?"
 * A partial URL https:// is now correctly interpreted as having only the
   protocol set, but not host name nor path.
 * The lenient mode is now explained a lot more verbosely in the code
   comment in credential.h.
 * When skipping a config setting, we now show the config key, not just the
   URL (which might be incomplete, i.e. not actually a URL).
 * When skipping a config setting, the correct struct credential is cleared
   (i.e. the just-parsed one, not the one against which we wanted to match
   the just-parsed one).
 * Added a ton more partial URLs to the test, and the test now also verifies
   that the warning comes out all right.

Johannes Schindelin (3):
  credential: fix grammar
  credential: optionally allow partial URLs in
    credential_from_url_gently()
  credential: handle `credential.<partial-URL>.<key>` again

 credential.c           | 60 +++++++++++++++++++++++++++++++++++++-----
 credential.h           |  2 +-
 t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 8 deletions(-)


base-commit: df5be6dc3fd18c294ec93a9af0321334e3f92c9c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-615%2Fdscho%2Fcredential-config-partial-url-maint-2.17-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-615/dscho/credential-config-partial-url-maint-2.17-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/615

Range-diff vs v2:

 1:  2c1c0ae91eb = 1:  2c1c0ae91eb credential: fix grammar
 2:  fc772d21b74 ! 2:  e92f139dfc7 credential: optionally allow partial URLs in credential_from_url_gently()
     @@ Commit message
          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 add a parameter called
     -    `allow_partial_url` to the `credential_from_url_gently()` function and
     -    convert the existing callers to set that parameter to 0, i.e. do not
     -    change the existing behavior, just add the option to be more lenient.
     +    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
     @@ Commit message
      
       ## credential.c ##
      @@ credential.c: static int check_url_component(const char *url, int quiet,
     + 	return -1;
       }
       
     - int credential_from_url_gently(struct credential *c, const char *url,
     +-int credential_from_url_gently(struct credential *c, const char *url,
      -			       int quiet)
     -+			       int allow_partial_url, int quiet)
     ++/*
     ++ * Potentially-partial URLs can, but do not have to, contain
     ++ *
     ++ * - a protocol (or scheme) of the form "<protocol>://"
     ++ *
     ++ * - a host name (the part after the protocol and before the first slash after
     ++ *   that, if any)
     ++ *
     ++ * - a user name and potentially a password (as "<user>[:<password>]@" part of
     ++ *   the host name)
     ++ *
     ++ * - a path (the part after the host name, if any, starting with the slash)
     ++ *
     ++ * Missing parts will be left unset in `struct credential`. Thus, `https://`
     ++ * will have only the `protocol` set, `example.com` only the host name, and
     ++ * `/git` only the path.
     ++ *
     ++ * Note that an empty host name in an otherwise fully-qualified URL (e.g.
     ++ * `cert:///path/to/cert.pem`) will be treated as unset if we expect the URL to
     ++ * be potentially partial, and only then (otherwise, the empty string is used).
     ++ *
     ++ * The credential_from_url() function does not allow partial URLs.
     ++ */
     ++static int credential_from_url_1(struct credential *c, const char *url,
     ++				 int allow_partial_url, int quiet)
       {
       	const char *at, *colon, *cp, *slash, *host, *proto_end;
       
     @@ credential.c: int credential_from_url_gently(struct credential *c, const char *u
       	while (*slash == '/')
       		slash++;
      @@ credential.c: int credential_from_url_gently(struct credential *c, const char *url,
     + 	return 0;
     + }
       
     ++int credential_from_url_gently(struct credential *c, const char *url, int quiet)
     ++{
     ++	return credential_from_url_1(c, url, 0, quiet);
     ++}
     ++
       void credential_from_url(struct credential *c, const char *url)
       {
     --	if (credential_from_url_gently(c, url, 0) < 0)
     -+	if (credential_from_url_gently(c, url, 0, 0) < 0)
     - 		die(_("credential url cannot be parsed: %s"), url);
     - }
     -
     - ## credential.h ##
     -@@ credential.h: void credential_write(const struct credential *, FILE *);
     -  * an error but leave the broken state in the credential object for further
     -  * examination.  The non-gentle form will issue a warning to stderr and return
     -  * an empty credential.
     -+ *
     -+ * If allow_partial_url is non-zero, partial URLs are allowed, i.e. it can, but
     -+ * does not have to, contain
     -+ *
     -+ * - a protocol (or scheme) of the form "<protocol>://"
     -+ *
     -+ * - a host name (the part after the protocol and before the first slash after
     -+ *   that, if any)
     -+ *
     -+ * - a user name and potentially a password (as "<user>[:<password>]@" part of
     -+ *   the host name)
     -+ *
     -+ * - a path (the part after the host name, if any, starting with the slash)
     -+ *
     -+ * Missing parts will be left unset in `struct credential`. Thus, `https://`
     -+ * will have only the `protocol` set, `example.com` only the host name, and
     -+ * `/git` only the path.
     -+ *
     -+ * Note that an empty host name in an otherwise fully-qualified URL will be
     -+ * treated as unset when allow_partial_url is non-zero (and only then,
     -+ * otherwise it is treated as the empty string).
     -+ *
     -+ * The credential_from_url() function does not allow partial URLs.
     -  */
     - void credential_from_url(struct credential *, const char *url);
     --int credential_from_url_gently(struct credential *, const char *url, int quiet);
     -+int credential_from_url_gently(struct credential *, const char *url,
     -+			       int allow_partial_url, int quiet);
     - 
     - int credential_match(const struct credential *have,
     - 		     const struct credential *want);
     -
     - ## fsck.c ##
     -@@ fsck.c: static int check_submodule_url(const char *url)
     - 	else if (url_to_curl_url(url, &curl_url)) {
     - 		struct credential c = CREDENTIAL_INIT;
     - 		int ret = 0;
     --		if (credential_from_url_gently(&c, curl_url, 1) ||
     -+		if (credential_from_url_gently(&c, curl_url, 0, 1) ||
     - 		    !*c.host)
     - 			ret = -1;
     - 		credential_clear(&c);
     + 	if (credential_from_url_gently(c, url, 0) < 0)
 3:  daedaffe960 ! 3:  0535908dd7e credential: handle `credential.<partial-URL>.<key>` again
     @@ Commit message
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## credential.c ##
     +@@ credential.c: 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)
     + {
      @@ credential.c: 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, 1, 0) < 0) {
     ++		if (credential_from_potentially_partial_url(&want, url) < 0) {
      +			warning(_("skipping credential lookup for key: %s"),
      +				var);
      +			credential_clear(&want);
     @@ credential.c: static int credential_config_callback(const char *var, const char
       		matched = credential_match(&want, c);
       
       		credential_clear(&want);
     +@@ credential.c: static int credential_from_url_1(struct credential *c, const char *url,
     + 	return 0;
     + }
     + 
     ++static int credential_from_potentially_partial_url(struct credential *c,
     ++						   const char *url)
     ++{
     ++	return credential_from_url_1(c, url, 1, 0);
     ++}
     ++
     + int credential_from_url_gently(struct credential *c, const char *url, int quiet)
     + {
     + 	return credential_from_url_1(c, url, 0, quiet);
      
       ## t/t0300-credentials.sh ##
      @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work with missing protocol' '
     @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w
       
      +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 >input &&
     ++	test_write_lines url=https://user@example.com/repo.git >stdin &&
      +	for partial in \
      +		example.com \
      +		user@example.com \
     @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w
      +		/repo.git
      +	do
      +		git -c credential.$partial.helper=yep \
     -+			credential fill <input >output &&
     -+		grep yep output ||
     ++			credential fill <stdin >stdout &&
     ++		grep yep stdout ||
      +		return 1
      +	done &&
      +
     @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w
      +		/repo
      +	do
      +		git -c credential.$partial.helper=yep \
     -+			credential fill <input >output &&
     -+		! grep yep output ||
     ++			credential fill <stdin >stdout &&
     ++		! grep yep stdout ||
      +		return 1
      +	done &&
      +
      +	git -c credential.$partial.helper=yep \
      +		-c credential.with%0anewline.username=uh-oh \
     -+		credential fill <input >output 2>error &&
     -+	test_i18ngrep "skipping credential lookup for key" error
     ++		credential fill <stdin >stdout 2>stderr &&
     ++	test_i18ngrep "skipping credential lookup for key" stderr
      +
      +'
      +

-- 
gitgitgadget

[PATCH v3 1/3] credential: fix grammar

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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>
---
 credential.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential.h b/credential.h
index 122a23cd2f1..5a86502d95c 100644
--- a/credential.h
+++ b/credential.h
@@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *);
 /*
  * Parse a url into a credential struct, replacing any existing contents.
  *
- * Ifthe url can't be parsed (e.g., a missing "proto://" component), the
+ * If the url can't be parsed (e.g., a missing "proto://" component), the
  * resulting credential will be empty but we'll still return success from the
  * "gently" form.
  *
-- 
gitgitgadget

[PATCH v3 2/3] credential: optionally allow partial URLs in credential_from_url_gently()

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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>
---
 credential.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/credential.c b/credential.c
index 64a841eddca..7dbbf26f174 100644
--- a/credential.c
+++ b/credential.c
@@ -343,8 +343,31 @@ static int check_url_component(const char *url, int quiet,
 	return -1;
 }
 
-int credential_from_url_gently(struct credential *c, const char *url,
-			       int quiet)
+/*
+ * Potentially-partial URLs can, but do not have to, contain
+ *
+ * - a protocol (or scheme) of the form "<protocol>://"
+ *
+ * - a host name (the part after the protocol and before the first slash after
+ *   that, if any)
+ *
+ * - a user name and potentially a password (as "<user>[:<password>]@" part of
+ *   the host name)
+ *
+ * - a path (the part after the host name, if any, starting with the slash)
+ *
+ * Missing parts will be left unset in `struct credential`. Thus, `https://`
+ * will have only the `protocol` set, `example.com` only the host name, and
+ * `/git` only the path.
+ *
+ * Note that an empty host name in an otherwise fully-qualified URL (e.g.
+ * `cert:///path/to/cert.pem`) will be treated as unset if we expect the URL to
+ * be potentially partial, and only then (otherwise, the empty string is used).
+ *
+ * The credential_from_url() function does not allow partial URLs.
+ */
+static int credential_from_url_1(struct credential *c, const char *url,
+				 int allow_partial_url, int quiet)
 {
 	const char *at, *colon, *cp, *slash, *host, *proto_end;
 
@@ -357,12 +380,12 @@ int credential_from_url_gently(struct credential *c, const char *url,
 	 *   (3) proto://<user>:<pass>@<host>/...
 	 */
 	proto_end = strstr(url, "://");
-	if (!proto_end || proto_end == url) {
+	if (!allow_partial_url && (!proto_end || proto_end == url)) {
 		if (!quiet)
 			warning(_("url has no scheme: %s"), url);
 		return -1;
 	}
-	cp = proto_end + 3;
+	cp = proto_end ? proto_end + 3 : url;
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');
@@ -382,8 +405,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
 		host = at + 1;
 	}
 
-	c->protocol = xmemdupz(url, proto_end - url);
-	c->host = url_decode_mem(host, slash - host);
+	if (proto_end && proto_end - url > 0)
+		c->protocol = xmemdupz(url, proto_end - url);
+	if (!allow_partial_url || slash - host > 0)
+		c->host = url_decode_mem(host, slash - host);
 	/* Trim leading and trailing slashes from path */
 	while (*slash == '/')
 		slash++;
@@ -405,6 +430,11 @@ int credential_from_url_gently(struct credential *c, const char *url,
 	return 0;
 }
 
+int credential_from_url_gently(struct credential *c, const char *url, int quiet)
+{
+	return credential_from_url_1(c, url, 0, quiet);
+}
+
 void credential_from_url(struct credential *c, const char *url)
 {
 	if (credential_from_url_gently(c, url, 0) < 0)
-- 
gitgitgadget

[PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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>
---
 credential.c           | 18 +++++++++++++++++-
 t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

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)
 {
@@ -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);
 
 		credential_clear(&want);
@@ -430,6 +440,12 @@ static int credential_from_url_1(struct credential *c, const char *url,
 	return 0;
 }
 
+static int credential_from_potentially_partial_url(struct credential *c,
+						   const char *url)
+{
+	return credential_from_url_1(c, url, 1, 0);
+}
+
 int credential_from_url_gently(struct credential *c, const char *url, int quiet)
 {
 	return credential_from_url_1(c, url, 0, quiet);
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 \
+		https:// \
+		https://example.com \
+		https://example.com/ \
+		https://user@example.com \
+		https://user@example.com/ \
+		https://example.com/repo.git \
+		https://user@example.com/repo.git \
+		/repo.git
+	do
+		git -c credential.$partial.helper=yep \
+			credential fill <stdin >stdout &&
+		grep yep stdout ||
+		return 1
+	done &&
+
+	for partial in \
+		dont.use.this \
+		http:// \
+		/repo
+	do
+		git -c credential.$partial.helper=yep \
+			credential fill <stdin >stdout &&
+		! grep yep stdout ||
+		return 1
+	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
-- 
gitgitgadget

Re: [PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Carlo Marcelo Arenas Belón]

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

Re: [PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Junio C Hamano]

"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.

Re: [PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin]

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

Re: [PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

Hi Carlo,

On Fri, 24 Apr 2020, Carlo Marcelo Arenas Belón wrote:

> 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.

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=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?

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 equivalent)
> should be IMHO good to go.

Thank you for reviewing it!

Ciao,
Dscho

Re: [PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Junio C Hamano]

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.

Re: [PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin]

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

Re: [PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Junio C Hamano]

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.

Re: [PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin]

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

Re: [PATCH v3 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Junio C Hamano]

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.