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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 23, 2020, 13:28 UTC
Message-ID: <nycvar.QRO.7.76.6.2004231433060.18039@tvgsbejvaqbjf.bet>
In-Reply-To: <CAPUEspgJvN6f6Wjo-yjYj-x+bYtC3vdSvwUtrF=MbJDjwYUTdA@mail.gmail.com>

Hi Carlo,

On Wed, 22 Apr 2020, Carlo Arenas wrote:

Show 16 quoted lines

> 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