Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store

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

On Tue, Apr 28, 2020 at 09:36:00PM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> > +static char *redact_credential(const struct strbuf *line)
> > +{
> > +	struct strbuf redacted_line = STRBUF_INIT;
> > +	char *at = strchr(line->buf, '@');
> > +	char *colon;
> > +	int redacted = 0;
> > +
> > +	if (at) {
> > +		strbuf_addf(&redacted_line, "%.*s",
> > +			(int)(at - line->buf), line->buf);
> > +		colon = strrchr(redacted_line.buf, ':');
> 
> Just showing my ignorance, but ...
> 
>  - Is the above strrchr() that forbids a colon in the password
>    intended, or should it be strchr() that only forbids a colon in
>    the username instead?

neither; that ":" is the separator we put in there between username
and password (unless it was edited out), as any original ":" in both
is already urlencoded.

strrchr() was used in purpose to back only as much as it is needed
from the string to hopefully find the beginning of the password.

indeed, most of the uglyness of the code comes from the fact it tries
really hard to only redact the password so the rest of the credential
is still useful for context and do so without changing the original
line.

>  - Would it hurt to redact both username and password as sensitive?
>    If not, it would certainly make it simpler to unconditionally:
> 
>                 int i;
>                 for (i = 0; i < redacted_line.len; i++) {
>                         if (redacted_line.buf[i] != ':')
>                                 redacted_line.buf[i] = 'x';
>                 }

that would leak the length of the password which might also be considered
sensitive

IMHO if we are redacting both might as well not print anything and let
the user find the offending credential by line number instead ;) after
all I got a feeling most of those entries are empty lines or "comments".

for example with the current code merged in pu we get :

$ cat secretfile

://u:p@example.com
http://user:some%3apass@example.com

$ git credential-store --file secretfile get
protocol=http

warning: secretfile:1 ignoring invalid credential:
warning: secretfile:2 ignoring invalid credential: ://u:<redacted>@example.com
username=user
password=some:pass

Carlo