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

[Junio C Hamano <gitster@pobox.com>]

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.
>
> Warn the user indicating the filename and line number so any invalid
> entries could be corrected but continue parsing until a match is
> found or all valid credentials are processed.
>
> Make sure that the credential that we will use to match is complete by
> confirming it has all fields set as expected by the updated rules.
>
> [1] https://stackoverflow.com/a/61420852/5005936
>
> Reported-by: Dirk <dirk@ed4u.de>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---

Looks good.

> diff --git a/credential-store.c b/credential-store.c
> index c010497cb2..b1f5b2dea6 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -4,10 +4,20 @@
>  #include "string-list.h"
>  #include "parse-options.h"
>  
> +#define PARSE_VERBOSE 0x01
> +
>  static struct lock_file credential_lock;
>  
> +static int valid_credential(struct credential *entry)
> +{
> +	return (entry->username && entry->password &&
> +		entry->protocol && *entry->protocol &&
> +		((entry->host && *entry->host) || entry->path));
> +}

OK.

>  static int parse_credential_file(const char *fn,
>  				  struct credential *c,
> +				  int flags,
>  				  void (*match_cb)(struct credential *),
>  				  void (*other_cb)(struct strbuf *))
>  {
> @@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,
>  	struct strbuf line = STRBUF_INIT;
>  	struct credential entry = CREDENTIAL_INIT;
>  	int found_credential = 0;
> +	int lineno = 0;
>  
>  	fh = fopen(fn, "r");
>  	if (!fh) {
> @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn,
>  		return found_credential;
>  	}
>  
> -	while (strbuf_getline_lf(&line, fh) != EOF) {
> -		credential_from_url(&entry, line.buf);
> -		if (entry.username && entry.password &&
> -		    credential_match(c, &entry)) {
> +	while (strbuf_getline(&line, fh) != EOF) {

Hmph, I probably have missed some discussion that happened in the
last few days, but from the use of _lf() in the original, I sense
that it is very much deliberate that the file format is designed to
be LF delimited records, *not* a text file in which each line is
terminated with some end-of-line marker that is platform dependent.
IOW, an explicit use of _lf() shouts, at least to me, that we want a
sequence of LF terminated records even on Windows and Macs.

So, I am not sure why this change is desirable.

> +		lineno++;
> +
> +		if (credential_from_url_gently(&entry, line.buf, 1) ||
> +			!valid_credential(&entry)) {

OK, so we read a line, fed it to the parser, and if we had trouble
parsing or the line did not have enough credential material, we
discard and warn (when told to).

> +			if (flags & PARSE_VERBOSE)
> +				warning(_("%s:%d: ignoring invalid credential"),
> +					fn, lineno);
> +		} else if (credential_match(c, &entry)) {
>  			found_credential = 1;
>  			if (match_cb) {
>  				match_cb(&entry);
>  				break;
>  			}
> +			continue;

And if the credential material on a good line matches, we remember
we saw a match.  If there is match callback, we call it and leave
the loop to return from the function.  Otherwise we go to the next
line.
>  		}
> +
> +		if (other_cb)
>  			other_cb(&line);

A malformed/underspecified line and an unmatched line is fed to the
other callback.

>  	}
>  
> @@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
>  		die_errno("unable to get credential storage lock");
>  	if (extra)
>  		print_line(extra);
> -	parse_credential_file(fn, c, NULL, print_line);
> +	parse_credential_file(fn, c, 0, NULL, print_line);

This helper function is called from two places.

 - In store_credential_file, we write the credential we were asked
   to store to a strbuf, and then call this function.  The function
   first writes that new credential and then calls the parse helper
   without PARSE_VERBOSE.  We do not give any match callback, and
   other callback is to just print the line read from the credential
   file.  So, the final output would be the new credential, followed
   by the current contents of the credential file except for the
   records that match the one we are storing.  We do this without
   warning about malformed entries at all.

 - In remove_credential, we go over multiple files and copy the
   lines in each of them, except the ones that match the credential
   we are given.  We also do this without warning about malformed
   entries at all.

>  	if (commit_lock_file(&credential_lock) < 0)
>  		die_errno("unable to write credential store");
>  }
> @@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *
>  	struct string_list_item *fn;
>  
>  	for_each_string_list_item(fn, fns)
> -		if (parse_credential_file(fn->string, c, print_entry, NULL))
> +		if (parse_credential_file(fn->string, c, PARSE_VERBOSE,
> +						 print_entry, NULL))
>  			return; /* Found credential */
>  }

This is triggered by the "get" operation.  We read until we hit the
entry we are looking for, at that point the match-callback will
print out "username=... / password=..." lines and we ignore the
remainder of the file.  While we look for the first matching
credential, we do warn about malformed or underspecified lines that
we are ignoring, but because we leave the loop once a matching line
is found, bad lines that come after it won't be even looked at to be
warned.

Validating and warning about bad entries is *not* the main purpose
of the "credential-store" program, so I fully agree with the design
of the "get" part.  I am not so sure about the other two operations
(i.e. "store" and "erase") that do scan all the entries and has
chance to warn about bad ones, though (note: I am not saying that we
should parse verbosely---it is just that I do not know why you chose
not to and I am not convinced that it is a good idea not to warn).

The tests looked good.

Thanks.