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

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

On Mon, Apr 27, 2020 at 05:10:13PM -0400, Jeff King wrote:
> On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:
> > >> These files are not supposed to be viewed or edited without the help
> > >> of the credential helpers.  Do these blank lines and comments even
> > >> survive when a new credential is approved, or do we just overwrite
> > >> and lose them?
> > >
> > > That's a good question. In the older code we do save them, because
> > > credential-store passes through lines which don't match the credential
> > > we're operating on.
> > >
> > > But in Carlo's patch, the immediate "continue" means we wouldn't ever
> > > call "other_cb", which is what does that pass-through.
> > 
> > So, does that mean the patch that started this thread will still help
> > users who wrote custom comments and blank lines in their credential
> > store by letting git-credential-store start up, but leaves a ticking
> > bomb for them to lose these precious comments and blanks once they
> > add a new site, change password, etc., at which point the user realizes
> > that comments and blanks are not supported after all?
> 
> The more I look into it, the more negative I am on adding such a comment
> feature.

FWIW I am not interested on implementing a comment feature (regardless of
what the subject says, and indeed was going to use quotes around it in my
original patch), but it was already too long ;)

my objective was to allow people that were not upgrading because of this
"regression" a path forward, but after all this discussion I am convinced
too that my approach is misguided.

if users are already manually editing this file, they should be able to
fix the corrupted lines themselves but the error shown needs to be improved.
and the failure mode corrected as well so they are not blocked.

this also means that my "sneaky" way to fix formatting issues is gone
and the passthrough is called so nothing will be lost, but as you also
said the location is not warranted to be kept so this comment "feature"
is as useless as intended.

would something like the following (wouldn't pass the last test yet as
it will need special handling to be able to handle the full path of the
broken file as shown in the warning sent to stderr) on top of Junio's
make more sense?

it still "silently" fixes the formatting issues where a leading tab (test
added) or spaces were added to easy the transition, but probably should
also error out as Junio suggested originally.

had also updated the commit message and what is left of the documentation.

Carlo
-- >8 --
Subject: [RFC PATCH v4] git-credential-store: skip empty lines and comments
 from store

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.

using the store file in this manner wasn't intended by the original
code and it had latent issues which the new code dutifully prevented
but since the strings used wouldn't had been valid credentials anyway
we could instead detect them and skip the matching logic and therefore
prevent a fatal failure.

trim all lines as they are being read from the store file and skip the
ones that will be otherwise empty or that start with "#" (therefore
assuming them to be comments) but warn users so they will be make
aware of the issues introduced by their manual editing.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 Documentation/git-credential-store.txt |  4 ++++
 credential-store.c                     |  7 ++++++
 t/t0302-credential-store.sh            | 31 ++++++++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 693dd9d9d7..e9a1993021 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -101,6 +101,10 @@ username (if we already have one) match, then the password is returned
 to Git. See the discussion of configuration in linkgit:gitcredentials[7]
 for more information.
 
+The parser will ignore any lines that are empty or starting with '#' character
+during the processing of credentials for read, but warn the user so they can
+be corrected.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..f1f016fec1 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -24,6 +24,13 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
+		strbuf_trim(&line);
+		if (line.len == 0 || *line.buf == '#') {
+			warning("ignoring corrupted credential entry \"%s\" in %s", line.buf, fn);
+			if (other_cb)
+				other_cb(&line);
+			continue;
+		}
 		credential_from_url(&entry, line.buf);
 		if (entry.username && entry.password &&
 		    credential_match(c, &entry)) {
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..12a158c136 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,35 @@ 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: attempt to workaround formatting isssues (ex: tab)' '
+	q_to_tab >"$HOME/.git-credentials" <<-\EOF &&
+	Qhttps://user:pass@example.com
+	EOF
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
+test_expect_success 'get: skip empty lines or "comments" in store file' '
+	test_write_lines "#comment" " " "" \
+		 https://user:pass@example.com >"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
 test_done
-- 
2.26.2.569.g1d74ac4d14