[RFC,v6,1/2] credential-store: warn instead of fatal for bogus lines from store
diff mbox series

Message ID 20200429203546.56753-2-carenas@gmail.com
State New
Headers show
Series
  • credential-store: prevent fatal errors
Related show

Commit Message

Carlo Marcelo Arenas Belón April 29, 2020, 8:35 p.m. UTC
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
warn the user so any invalid entries could be corrected.

[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>
---
 Documentation/git-credential-store.txt | 12 ++++++--
 credential-store.c                     | 22 +++++++++-----
 t/t0302-credential-store.sh            | 42 ++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 10 deletions(-)

Comments

Junio C Hamano April 29, 2020, 9:05 p.m. UTC | #1
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
> warn the user so any invalid entries could be corrected.

Yeah, this version looks be best so far.  No need to worry about
redacting anything; we are dealing with folks who have edited
the file, and they shouldn't have any trouble going to the line with
a problem given an exact line number.

> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 76b0798856..30d498fe54 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -95,8 +95,16 @@ 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 edit the file with editors as it could compromise the validity
> +of your credentials by sometimes subtle formatting issues (like spaces)

I do not think dropping "view or" is warranted.  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.

> +In cases where those formatting issues are detected during parsing a
> +warning indicating the line found will be printed to stderr so it can
> +be corrected at your earliest convenience, but the remaining valid
> +credentials will be used to try to find a match as described below.

I am often just as guilty, but the above four lines in a single
sentence is hard to read, especially without minimum number of
comma.  With a comma after "during parsing", and another after "to
stderr", might make it more readable, but at that point, it would
become far more readable if we split them into two sentences.

Perhaps

	An unparseable line is ignored, and a warning message points
	out the line number the problematic line appears in (you may
	want to delete them with an editor).

>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> +		lineno++;
> +		if (!credential_from_url_gently(&entry, line.buf, 1)) {
> +			if (entry.username && entry.password &&
> +				credential_match(c, &entry)) {
> +				found_credential = 1;
> +				if (match_cb) {
> +					match_cb(&entry);
> +					break;
> +				}
>  			}
>  		}
> +		else
> +			warning(_("ignoring invalid credential in %s:%d"),
> +				fn, lineno);
> +		if (!found_credential && other_cb)
>  			other_cb(&line);
>  	}

The above is harder to follow than necessary.

	while (... != EOF) {
		lineno++;
		if (credential is not well formed) {
			warning(_("ignoring..."));
		} else if (the entry matches) {
			found_credential = 1;
			if (match_cb) {
				match_cb(&entry);
				break; /* stop at the first match */
			}
			continue;
		}

                if (other_cb)
			other_cb(&line);
	}

may make the intention a bit clearer, with the loud "continue" inside.

 (1) we give warning for malformed one; and
 (2) we do not let other_cb touch a matching entry.
Junio C Hamano April 29, 2020, 9:17 p.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

>>  	while (strbuf_getline_lf(&line, fh) != EOF) {
>> +		lineno++;
>> +		if (!credential_from_url_gently(&entry, line.buf, 1)) {
>> +			if (entry.username && entry.password &&
>> +				credential_match(c, &entry)) {
>> +				found_credential = 1;
>> +				if (match_cb) {
>> +					match_cb(&entry);
>> +					break;
>> +				}
>>  			}
>>  		}
>> +		else
>> +			warning(_("ignoring invalid credential in %s:%d"),
>> +				fn, lineno);
>> +		if (!found_credential && other_cb)
>>  			other_cb(&line);
>>  	}
>
> The above is harder to follow than necessary.
>
> 	while (... != EOF) {
> 		lineno++;
> 		if (credential is not well formed) {

After reading your [2/2], I think this part deserves a bit of
explanation.  By "is not well formed", what I mean is not just
credential_from_url_gently() returns non-zero, but also user/pass
are specified.  And your step [2/2] may make the condition for a
line to be "well formed" even stricter by requiring at least one of
host or path, and that would also belong to this test.

> 			warning(_("ignoring..."));
> 		} else if (the entry matches) {

And the "entry matches" test here would only be "what does
credential_match() say?"

> 			found_credential = 1;
> 			if (match_cb) {
> 				match_cb(&entry);
> 				break; /* stop at the first match */
> 			}
> 			continue;
> 		}
>
>                 if (other_cb)
> 			other_cb(&line);
> 	}
>
> may make the intention a bit clearer, with the loud "continue" inside.
>
>  (1) we give warning for malformed one; and
>  (2) we do not let other_cb touch a matching entry.

Patch
diff mbox series

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..30d498fe54 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -95,8 +95,16 @@  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 edit the file with editors as it could compromise the validity
+of your credentials by sometimes subtle formatting issues (like spaces)
+
+In cases where those formatting issues are detected during parsing a
+warning indicating the line found will be printed to stderr so it can
+be corrected at your earliest convenience, but the remaining valid
+credentials will be used to try to find a match as described below.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..1cc5ca081a 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -15,6 +15,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 +25,21 @@  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 &&
-		    credential_match(c, &entry)) {
-			found_credential = 1;
-			if (match_cb) {
-				match_cb(&entry);
-				break;
+		lineno++;
+		if (!credential_from_url_gently(&entry, line.buf, 1)) {
+			if (entry.username && entry.password &&
+				credential_match(c, &entry)) {
+				found_credential = 1;
+				if (match_cb) {
+					match_cb(&entry);
+					break;
+				}
 			}
 		}
-		else if (other_cb)
+		else
+			warning(_("ignoring invalid credential in %s:%d"),
+				fn, lineno);
+		if (!found_credential && other_cb)
 			other_cb(&line);
 	}
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..801c1eb200 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,46 @@  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 &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" > "$HOME/.git-credentials" &&
+	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	STDOUT
+	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
+'
+
 test_done