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

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, Johannes.Schindelin@gmx.de
Subject: Re: [PATCH v10] credential-store: ignore bogus lines from store file
Date: Sat, 02 May 2020 13:47:09 -0700	[thread overview]
Message-ID: <xmqqo8r6jc8i.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200502181643.38203-1-carenas@gmail.com> ("Carlo Marcelo Arenas =?utf-8?Q?Bel=C3=B3n=22's?= message of "Sat, 2 May 2020 11:16:43 -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.
>
> 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.  

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

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

> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index d6b54e8c65..9fd0aca55e 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> -
>  test_expect_success 'erase: erase matching credentials from both xdg and home files' '
>  	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
>  	mkdir -p "$HOME/.config/git" &&
> @@ -120,4 +119,63 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> +invalid_credential_test() {
> +	test_expect_success 'get: ignore credentials without $1 as invalid' '
> +		echo "$2" >"$HOME/.git-credentials" &&
> +		check fill store <<-\EOF
> +		protocol=https
> +		host=example.com
> +		--
> +		protocol=https
> +		host=example.com
> +		username=askpass-username
> +		password=askpass-password
> +		--
> +		askpass: Username for '\''https://example.com'\'':
> +		askpass: Password for '\''https://askpass-username@example.com'\'':
> +		--
> +		EOF
> +	'
> +}
> +
> +invalid_credential_test "scheme" ://user:pass@example.com
> +invalid_credential_test "valid host/path" https://user:pass@
> +invalid_credential_test "username/password" https://pass@example.com

These are quite clear to see.  Nicely done.

> +test_expect_success 'get: credentials with DOS line endings are invalid' '
> +	printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" &&
> +	check fill store <<-\EOF
> +	protocol=https
> +	host=example.com
> +	--
> +	protocol=https
> +	host=example.com
> +	username=askpass-username
> +	password=askpass-password
> +	--
> +	askpass: Username for '\''https://example.com'\'':
> +	askpass: Password for '\''https://askpass-username@example.com'\'':
> +	--
> +	EOF
> +'

What I am curious is if anything breaks around this test if you lost
the extra "!strchr() &&" check.  I suspect that this test will pass.

> +test_expect_success 'get: store file can contain empty/bogus lines' '
> +	echo "" >"$HOME/.git-credentials" &&
> +	q_to_tab <<-\CREDENTIAL >>"$HOME/.git-credentials" &&
> +	#comment
> +	Q
> +	https://user:pass@example.com
> +	CREDENTIAL
> +	check fill store <<-\EOF
> +	protocol=https
> +	host=example.com
> +	--
> +	protocol=https
> +	host=example.com
> +	username=user
> +	password=pass
> +	--
> +	EOF
> +'
> +
>  test_done
>
> base-commit: 49458fb91d61461938881559ce23abbb1a2f8c35