[RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store

From: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
To: git@vger.kernel.org
Cc: dirk@ed4u.de, sunshine@sunshineco.com, peff@peff.net,
	gitster@pobox.com, jrnieder@gmail.com,
	"Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Subject: [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store
Date: Wed, 29 Apr 2020 13:35:45 -0700	[thread overview]
Message-ID: <20200429203546.56753-2-carenas@gmail.com> (raw)
In-Reply-To: <20200429203546.56753-1-carenas@gmail.com>

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(-)

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
-- 
2.26.2.569.g1d74ac4d14

