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@xxxxxxx> Helped-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> --- 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@xxxxxxxxxxx + 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@xxxxxxxxxxx >"$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
