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@xxxxxxxxx> > --- > 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@xxxxxxxxxxx > ------------------------------ > > +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
