Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

["Carlo Marcelo Arenas Belón" <carenas@gmail.com>]

On Tue, Apr 28, 2020 at 09:03:36AM -0700, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > I wonder if in addition to the above documentation change we may want
> > something guaranteed to catch all cases where people would have
> > experienced a regression, like
> >
> > diff --git i/credential-store.c w/credential-store.c
> > index c010497cb21..294e7716815 100644
> > --- i/credential-store.c
> > +++ w/credential-store.c
> > @@ -24,8 +24,8 @@ 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 (!credential_from_url_gently(&entry, line.buf, 1) &&
> > +		    entry.username && entry.password &&
> >  		    credential_match(c, &entry)) {
> >  			found_credential = 1;
> >  			if (match_cb) {
> 
> Hmph, so the idea is, instead of ignoring the potential error
> detected by credential_from_url() and using credential when it is
> available, we loudly attempt to parse and give warning on malformed
> entries when we discard a line?

the idea was to silently ignore the line (notice quiet = 1), which
is the "original behaviour" as Peff pointed out in his reply in
20200428054155.GB2376380@coredump.intra.peff.net[1]

> I think that is an excellent idea.
> 
> It would be nicer if we can somehow add where we found the offending
> line, e.g.
> 
>     /home/me/.gitcredential:396: url has no scheme: ://u:p@host/path
> 
> Do we feel it a bit disturbing that u:p is shown in the error
> message, without redacting, by the way?

we could do so with the following on top as a 5/4.

most of the test changes would go away in a reroll, and are similar
to what would have been needed for my original suggested patch[2] with
the exception that in this series we won't support silently fixing
credentials which is something you mentioned a preference on.

Carlo

[1] https://lore.kernel.org/git/20200428054155.GB2376380@coredump.intra.peff.net/ 
[2] https://lore.kernel.org/git/20200428013726.GD61348@Carlos-MBP/
-- >8 --
Subject: [RFC PATCH] credential-store: SQUASH!!

add warnings

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 credential-store.c          | 55 +++++++++++++++++++++++++++++++------
 t/t0302-credential-store.sh | 42 ++++++++++++++++------------
 2 files changed, 70 insertions(+), 27 deletions(-)

diff --git a/credential-store.c b/credential-store.c
index 294e771681..f76292df20 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -6,6 +6,32 @@
 
 static struct lock_file credential_lock;
 
+static char *redact_credential(const struct strbuf *line)
+{
+	struct strbuf redacted_line = STRBUF_INIT;
+	char *at = strchr(line->buf, '@');
+	char *colon;
+	int redacted = 0;
+
+	if (at) {
+		strbuf_addf(&redacted_line, "%.*s",
+			(int)(at - line->buf), line->buf);
+		colon = strrchr(redacted_line.buf, ':');
+		if (colon && *(colon + 1) != '/' && colon > redacted_line.buf) {
+			redacted_line.len = colon - redacted_line.buf + 1;
+			strbuf_addstr(&redacted_line, "<redacted>");
+			strbuf_addstr(&redacted_line, at);
+			redacted = 1;
+		}
+		else
+			strbuf_reset(&redacted_line);
+	}
+	if (!redacted)
+		strbuf_addbuf(&redacted_line, line);
+
+	return redacted_line.buf;
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
 				  void (*match_cb)(struct credential *),
@@ -15,6 +41,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,17 +51,27 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		if (!credential_from_url_gently(&entry, line.buf, 1) &&
-		    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)
+				other_cb(&line);
+		}
+		else {
+			if (other_cb)
+				other_cb(&line);
+			char *redacted = redact_credential(&line);
+			warning("%s:%d ignoring invalid credential: %s",
+				fn, lineno, redacted);
+			free(redacted);
 		}
-		else if (other_cb)
-			other_cb(&line);
 	}
 
 	credential_clear(&entry);
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 7c7a48303f..597a75903a 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,36 +120,42 @@ 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: store file can contain empty/bogus lines' '
-	test_write_lines "#comment" " " "" \
-		 https://user:pass@example.com >"$HOME/.git-credentials" &&
-	check fill store <<-\EOF
+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
-	username=user
-	password=pass
-	--
 	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: ignore credentials without scheme' '
-	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
-	check fill store <<-\EOF
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	test_write_lines "#comment" " " "" \
+		 https://user:pass@example.com >"$HOME/.git-credentials" &&
+	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
-	username=askpass-username
-	password=askpass-password
-	--
-	askpass: Username for '\''https://example.com'\'':
-	askpass: Password for '\''https://askpass-username@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