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

From: Junio C Hamano <gitster@pobox.com>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org, dirk@ed4u.de, sunshine@sunshineco.com,
	peff@peff.net, jrnieder@gmail.com
Subject: Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store
Date: Wed, 29 Apr 2020 16:47:31 -0700	[thread overview]
Message-ID: <xmqqlfmdua5o.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200429232322.68038-1-carenas@gmail.com> ("Carlo Marcelo Arenas =?utf-8?Q?Bel=C3=B3n=22's?= message of "Wed, 29 Apr 2020 16:23:22 -0700")

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.

with -> With

> instead of doing a hard check for credentials, do a soft one and
> warn the user so any invalid entries could be corrected.

instead -> instead

> make sure that the credential to use is complete before calling
> credential_match by confirming it has all fields set as expected
> by the updated rules.

make -> Make

> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 76b0798856..122e238eb2 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -95,8 +95,15 @@ https://user:pass@example.com
>  ------------------------------
>  
>  No other kinds of lines (e.g. empty lines or comment lines) are
> -allowed in the file, even though some may be silently ignored. Do
> -not view or edit the file with editors.
> +allowed in the file, even though historically the parser was very
> +lenient and some might had been silently ignored.
> +
> +Do not view or edit the file with editors as it could compromise the
> +validity of your credentials by sometimes subtle formatting issues,
> +like spaces, line wrapping or text encoding.

I do not think dropping "view or" is justifiable.  There is no need
to invite the risk of opening with the intention to only view and
then end up saving a modification.  In other words, do not encourage
use of an *editor* in any way.

> +
> +An unparseable or otherwise invalid line is ignored, and a warning
> +message points out the problematic line number and file it appears in.

OK.  You didn't want to tell them they can remove the problematic
line as a whole with their editor?

> diff --git a/credential-store.c b/credential-store.c
> index c010497cb2..4d3c9e8000 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -6,6 +6,15 @@
>  
>  static struct lock_file credential_lock;
>  
> +/*
> + * entry->protocol comes validated from credential_from_url_gently
> + */

Sure, but wouldn't it be simpler to do without such a comment and
check it also here, just as a belt-and-suspender safety?

> +static int valid_credential(struct credential *entry)
> +{
> +	return (entry->username && entry->password &&
> +		((entry->host && *entry->host) || entry->path));
> +}

> @@ -15,6 +24,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) {
> @@ -24,16 +34,24 @@ static int parse_credential_file(const char *fn,
>  	}
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> +		lineno++;
> +
> +		if (credential_from_url_gently(&entry, line.buf, 1) ||
> +			!valid_credential(&entry)) {
> +			warning(_("%s:%d: ignoring invalid credential"),
> +				fn, lineno);
> +		}
> +		else if (credential_match(c, &entry))
> +		{

Style:  "} else if (...) {" on a single line.

>  			found_credential = 1;
>  			if (match_cb) {
>  				match_cb(&entry);
>  				break;
>  			}
> +			continue;
>  		}
> +
> +		if (other_cb)
>  			other_cb(&line);
>  	}

This got vastly easier to read, thanks to the additional helper.
Looking really good.

> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index d6b54e8c65..3150f304cb 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -120,4 +120,84 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> +test_expect_success 'get: credentials without scheme are invalid' '
> +	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
> +	cat >expect-stdout <<-\STDOUT &&
> +	protocol=https
> +	host=example.com
> +	username=askpass-username
> +	password=askpass-password
> +	STDOUT
> +	test_config credential.helper store &&
> +	git credential fill <<-\EOF >stdout 2>stderr &&
> +	protocol=https
> +	host=example.com
> +	EOF
> +	test_cmp expect-stdout stdout &&
> +	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
> +	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&

These messages are passed from credential.c::credential_ask_one() to
git_prompt() without any i18n, so use of a bare "grep" is good.

> +	test_i18ngrep "ignoring invalid credential" stderr

And this is new message inside _(), so i18ngrep is good.

Nice to see such an attention to the details.

> +	test_config credential.helper store &&
> +	git credential fill <<-\EOF >stdout 2>stderr &&
> +	protocol=https
> +	host=example.com
> +	EOF
> +	test_cmp expect-stdout stdout &&
> +	test_i18ngrep "ignoring invalid credential" stderr &&
> +	test_line_count = 3 stderr

The "ignoring invalid credential" message could be translated into
two or more lines, but I think that is worrying too much about
theoretical possibility, so checking line count would probably be
sufficient.

Thanks.