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

["Carlo Marcelo Arenas Belón" <carenas@gmail.com>]

On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote:
> 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. 

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