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

[Jonathan Nieder <jrnieder@gmail.com>]

Junio C Hamano wrote:

> -- >8 --
> Subject: credential-store: document the file format a bit more
>
> Reading a malformed credential URL line and silently ignoring it
> does not mean that we promise to torelate and/or keep empty lines
> and "# commented" lines forever.
>
> Some people seem to take anything that is not explicitly forbidden
> as allowed, but the world does not work that way.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-credential-store.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 693dd9d9d7..76b0798856 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -94,6 +94,10 @@ stored on its own line as a URL like:
>  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.
> +
>  When Git needs authentication for a particular URL context,

I like this.

I do suspect this is easy to run into accidentally.  In $DAYJOB (in
the context of [1]) there was a service that accidentally wrote a \n\n
at the end of a line that was used by git-credential-store.  Once the
cause was tracked down, it was straightforward to fix, but I don't
like the idea that others in a similar position may end up tempted to
just not upgrade Git.

Independently, there is the thread we are replying to.

Independently, in Debian's bug tracking system, Stefan (cc-ed)
reports[2]:

| the vulnerability in CVE-2020-11008 is related to the handling
| of credential helpers in git. In Buster this has been fixed in
| 1:2.20.1-2+deb10u3. This broke my existing configuration where
| repositories have credential.helper=store set. This is
| documented in /usr/share/man/man1/git-credential-store.1.gz
| and other files from git, git-doc etc.
| I am unsure how to proceed... is this helper now unsupported?

(Stefan, do you have more details?  Did you manually populate your
credential store?  What error message do you get?)

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

And then we can tighten the handling of unrecognized lines to first
warn and then error out, as a controlled change that doesn't lead
people to regret updating git.

Thoughts?

Thanks,
Jonathan

[1] https://cs.opensource.google/copybara/copybara/+/master:java/com/google/copybara/git/GitOptions.java;drc=bc79a0b1ffe18f79dea0b75ba3a24b641a50a9fc;l=46
[2] https://bugs.debian.org/958929