Re: [PATCH v10] credential-store: ignore bogus lines from store file

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date: May 2, 2020, 21:23 UTC
Message-ID: <20200502212335.GB41113@Carlos-MBP>
In-Reply-To: <xmqqo8r6jc8i.fsf@gitster.c.googlers.com>

On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> 
> > With the added checks for invalid URLs in credentials, any locally
> > modified store files which might have empty lines or even comments
> > were reported[1] failing to parse as valid credentials.
> >
> > Instead of doing a hard check for credentials, do a soft one and
> > therefore avoid the reported fatal error.
> >
> > As a special case, flag files with CRLF endings as invalid early
> > to prevent current problems in credential_from_url_gently() with
> > handling of '\r' in the host.
> 
> I do not think it hurts to silently ignore a line that ends with CR,
> but only because I do not think credential_from_url_gently() would
> not match such a line when asked to match something without
> complaining.

for a credential like the one in the testcase (meaning no url), it will append \r to the hostname, which would cause havoc if that credential is printed (meaning you will end up without a host line) and be back in the die() in credential_apply()

> In other words, isn't the new "!strchr() &&" in the condition a
> no-op?

you are correct that it will be unlikely (but not imposible) to get an embedded CR from the other side to match, which is what I want to address in the next patchset.

IMHO adding the proposed early check gives us space to fix the other issues at our own leasure and it is meant to be gone eventually.

Show 21 quoted lines

> > diff --git a/credential-store.c b/credential-store.c
> > index c010497cb2..fdfb81e632 100644
> > --- a/credential-store.c
> > +++ b/credential-store.c
> > @@ -24,8 +24,9 @@ static int parse_credential_file(const char *fn,
> >  	}
> >  
> >  	while (strbuf_getline_lf(&line, fh) != EOF) {
> > -		credential_from_url(&entry, line.buf);
> > -		if (entry.username && entry.password &&
> > +		if (strchr(line.buf, '\r') == NULL &&
> > +		    !credential_from_url_gently(&entry, line.buf, 1) &&
> > +		    entry.username && entry.password &&
> >  		    credential_match(c, &entry)) {
> >  			found_credential = 1;
> >  			if (match_cb) {
> 
> In any case, among the ones we discussed, this probably has the
> least chance of unintended regression, I would think (with or
> without the added "!strchr() &&" check), so let's queue it and
> quickly merge it down thru 'next' to 'master'.

considering the only line that I wrote was the strchr and the other one was written by Jonathan and reviewed by Peff I definitly agree.

don't forget this is also a good candidate for maint (most likely all the way to maint-2.17)

Carlo