* [PATCH] git-credential-store: skip empty lines and comments from store @ 2020-04-26 23:47 Carlo Marcelo Arenas Belón 2020-04-27 0:19 ` Eric Sunshine 2020-04-27 8:42 ` [PATCH v2] " Carlo Marcelo Arenas Belón 0 siblings, 2 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-26 23:47 UTC (permalink / raw) To: git; +Cc: jrnieder, Carlo Marcelo Arenas Belón with the added checks for invalid URLs in credentials, any locally modified store files which might have empty lines or even comments were reported as failing[1] to parse as valid credentials. instead of passing every line to the matcher as read, trim them from spaces and skip the ones that will be otherwise empty or start with "#" (assumed to be comments) [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- credential-store.c | 3 +++ t/t0302-credential-store.sh | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/credential-store.c b/credential-store.c index c010497cb2..b2f160890d 100644 --- a/credential-store.c +++ b/credential-store.c @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn, } while (strbuf_getline_lf(&line, fh) != EOF) { + strbuf_trim(&line); + if (line.len == 0 || *line.buf == '#') + 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..7245d2f449 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,21 @@ 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: allow for empty lines or comments in store file' ' + echo "#this is a comment" >"$HOME/.git-credentials" && + echo "" >>"$HOME/.git-credentials" && + echo "https://user:pass@example.com" >>"$HOME/.git-credentials" && + echo " " >>"$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 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH] git-credential-store: skip empty lines and comments from store 2020-04-26 23:47 [PATCH] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón @ 2020-04-27 0:19 ` Eric Sunshine 2020-04-27 0:46 ` Carlo Marcelo Arenas Belón 2020-04-27 8:42 ` [PATCH v2] " Carlo Marcelo Arenas Belón 1 sibling, 1 reply; 79+ messages in thread From: Eric Sunshine @ 2020-04-27 0:19 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: Git List, Jonathan Nieder, Dirk [Cc:+Dirk -- so he knows his bug report[1] wasn't ignored] On Sun, Apr 26, 2020 at 7:48 PM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote: > with the added checks for invalid URLs in credentials, any locally > modified store files which might have empty lines or even comments > were reported as failing[1] to parse as valid credentials. > > instead of passing every line to the matcher as read, trim them > from spaces and skip the ones that will be otherwise empty or > start with "#" (assumed to be comments) > > Reported-by: Dirk <dirk@ed4u.de> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > @@ -120,4 +120,21 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi > +test_expect_success 'get: allow for empty lines or comments in store file' ' > + echo "#this is a comment" >"$HOME/.git-credentials" && > + echo "" >>"$HOME/.git-credentials" && > + echo "https://user:pass@example.com" >>"$HOME/.git-credentials" && > + echo " " >>"$HOME/.git-credentials" && Is there a reason you don't use a here-doc for the above (which would be less noisy)? For instance: q_to_tab >"$HOME/.git-credentials" <<-\EOF && #this is a comment https://user:pass@example.com Q EOF [1]: https://lore.kernel.org/git/ad80aa0d-3a35-6d7e-7958-b3520e16c855@ed4u.de/ ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH] git-credential-store: skip empty lines and comments from store 2020-04-27 0:19 ` Eric Sunshine @ 2020-04-27 0:46 ` Carlo Marcelo Arenas Belón 0 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-27 0:46 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Jonathan Nieder, Dirk On Sun, Apr 26, 2020 at 08:19:35PM -0400, Eric Sunshine wrote: > [Cc:+Dirk -- so he knows his bug report[1] wasn't ignored] > On Sun, Apr 26, 2020 at 7:48 PM Carlo Marcelo Arenas Belón > <carenas@gmail.com> wrote: > > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > > @@ -120,4 +120,21 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi > > +test_expect_success 'get: allow for empty lines or comments in store file' ' > > + echo "#this is a comment" >"$HOME/.git-credentials" && > > + echo "" >>"$HOME/.git-credentials" && > > + echo "https://user:pass@example.com" >>"$HOME/.git-credentials" && > > + echo " " >>"$HOME/.git-credentials" && > > Is there a reason you don't use a here-doc for the above (which would > be less noisy)? For instance: because I didn't knew it existed ;), thanks for your help and will include for next version. Carlo ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v2] git-credential-store: skip empty lines and comments from store 2020-04-26 23:47 [PATCH] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón 2020-04-27 0:19 ` Eric Sunshine @ 2020-04-27 8:42 ` Carlo Marcelo Arenas Belón 2020-04-27 11:52 ` Jeff King 2020-04-27 12:59 ` [PATCH v3] " Carlo Marcelo Arenas Belón 1 sibling, 2 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-27 8:42 UTC (permalink / raw) To: git; +Cc: dirk, sunshine, Carlo Marcelo Arenas Belón with the added checks for invalid URLs in credentials, any locally modified store files which might have empty lines or even comments were reported failing[1] to parse as valid credentials. instead of passing every line to the matcher for processing, trim them from spaces and skip the ones that will be otherwise empty or that start with "#" (assumed to be comments) [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> --- v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 7 +++++++ credential-store.c | 3 +++ t/t0302-credential-store.sh | 19 +++++++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 693dd9d9d7..7f7b53e4da 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -101,6 +101,13 @@ 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. +Note that the file used is not a configuration file and should be ideally +managed only through git, as any manually introduced typos will compromise +the validation of credentials. + +The parser will ignore any lines starting with the '#' character during +the processing of credentials, though. + GIT --- Part of the linkgit:git[1] suite diff --git a/credential-store.c b/credential-store.c index c010497cb2..b2f160890d 100644 --- a/credential-store.c +++ b/credential-store.c @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn, } while (strbuf_getline_lf(&line, fh) != EOF) { + strbuf_trim(&line); + if (line.len == 0 || *line.buf == '#') + 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..0d13318255 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,23 @@ 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: allow for empty lines or comments in store file' ' + q_to_cr >"$HOME/.git-credentials" <<-\EOF && + #this is a comment and the next line contains leading spaces + Q + https://user:pass@example.com + Q + EOF + 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 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v2] git-credential-store: skip empty lines and comments from store 2020-04-27 8:42 ` [PATCH v2] " Carlo Marcelo Arenas Belón @ 2020-04-27 11:52 ` Jeff King 2020-04-27 12:25 ` Carlo Marcelo Arenas Belón 2020-04-27 17:47 ` Junio C Hamano 2020-04-27 12:59 ` [PATCH v3] " Carlo Marcelo Arenas Belón 1 sibling, 2 replies; 79+ messages in thread From: Jeff King @ 2020-04-27 11:52 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote: > with the added checks for invalid URLs in credentials, any locally > modified store files which might have empty lines or even comments > were reported failing[1] to parse as valid credentials. Those were never supposed to work. I'm mildly surprised that they did. It looks like the username/password check here is what prevented us from matching an empty line against a very broad pattern: while (strbuf_getline_lf(&line, fh) != EOF) { credential_from_url(&entry, line.buf); if (entry.username && entry.password && credential_match(c, &entry)) { found_credential = 1; And there was no such thing as a comment character. E.g., a "commented-out" url like this: $ echo "#https://user:pass@example.com/" >creds would still be matched: $ echo host=example.com | git credential-store --file creds get host=example.com username=user password=pass I guess I'm not really opposed to adding this as a feature, but I think the justification should be "because it is somehow useful" and not because it's a bugfix. > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > index d6b54e8c65..0d13318255 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -120,4 +120,23 @@ 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: allow for empty lines or comments in store file' ' > + q_to_cr >"$HOME/.git-credentials" <<-\EOF && > + #this is a comment and the next line contains leading spaces > + Q > + https://user:pass@example.com > + Q > + EOF q_to_cr is a little weird here, as we wouldn't expect there to be CRs in the file. They do get removed by strbuf_trim(), even in non-comment lines. But perhaps "sed s/Q//" would accomplish the same thing (making the whitespace more visible) without making anyone wonder whether the CR is an important part of the test? -Peff ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v2] git-credential-store: skip empty lines and comments from store 2020-04-27 11:52 ` Jeff King @ 2020-04-27 12:25 ` Carlo Marcelo Arenas Belón 2020-04-27 14:43 ` Eric Sunshine 2020-04-27 17:47 ` Junio C Hamano 1 sibling, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-27 12:25 UTC (permalink / raw) To: Jeff King; +Cc: git, dirk, sunshine On Mon, Apr 27, 2020 at 07:52:23AM -0400, Jeff King wrote: > On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote: > > > with the added checks for invalid URLs in credentials, any locally > > modified store files which might have empty lines or even comments > > were reported failing[1] to parse as valid credentials. > > Those were never supposed to work. I'm mildly surprised that they did. agree on that, which is why I have to admit I couldn't find the right wording for the previous paragraph, and also why I didn't pinpoint a specific commit as the one that introduced a bug. it was instead just meant to describe a "current state of affairs" and I was hoping the documentation entry will help guide future users to be more careful with this file. > I guess I'm not really opposed to adding this as a feature, but I think > the justification should be "because it is somehow useful" and not > because it's a bugfix. I think that is a matter of semantics, because for some users it seems like a regression on the last bugfix and therefore might be used (albeit IMHO incorrectly) as an excuse not to upgrade, which will be even worst. so yes, I am implementint this "feature" to prevent them to have an excuse but it also means we would most likely need to fastrack this into maint. > > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > > index d6b54e8c65..0d13318255 100755 > > --- a/t/t0302-credential-store.sh > > +++ b/t/t0302-credential-store.sh > > @@ -120,4 +120,23 @@ 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: allow for empty lines or comments in store file' ' > > + q_to_cr >"$HOME/.git-credentials" <<-\EOF && > > + #this is a comment and the next line contains leading spaces > > + Q > > + https://user:pass@example.com > > + Q > > + EOF > > q_to_cr is a little weird here, as we wouldn't expect there to be CRs in > the file. They do get removed by strbuf_trim(), even in non-comment > lines. But perhaps "sed s/Q//" would accomplish the same thing (making > the whitespace more visible) without making anyone wonder whether the CR > is an important part of the test? I know, but commiting a line with 1 tab and 4 empty spaces instead of using Q seemed even worst and q_to_cr almost fits the bill and might even make this test "windows compatible" for once ;) will rewrite then using test_write_lines and I hope it is still as readable without having to resort to the originl ugly chain of echo Carlo ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v2] git-credential-store: skip empty lines and comments from store 2020-04-27 12:25 ` Carlo Marcelo Arenas Belón @ 2020-04-27 14:43 ` Eric Sunshine 0 siblings, 0 replies; 79+ messages in thread From: Eric Sunshine @ 2020-04-27 14:43 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: Jeff King, Git List, Dirk On Mon, Apr 27, 2020 at 8:25 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote: > On Mon, Apr 27, 2020 at 07:52:23AM -0400, Jeff King wrote: > > On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote: > > > +test_expect_success 'get: allow for empty lines or comments in store file' ' > > > + q_to_cr >"$HOME/.git-credentials" <<-\EOF && > > > + #this is a comment and the next line contains leading spaces > > > + Q > > > + https://user:pass@example.com > > > + Q > > > + EOF > > > > q_to_cr is a little weird here, as we wouldn't expect there to be CRs in > > the file. They do get removed by strbuf_trim(), even in non-comment > > lines. But perhaps "sed s/Q//" would accomplish the same thing (making > > the whitespace more visible) without making anyone wonder whether the CR > > is an important part of the test? > > I know, but commiting a line with 1 tab and 4 empty spaces instead of > using Q seemed even worst and q_to_cr almost fits the bill and might > even make this test "windows compatible" for once ;) Hmm? In the proposal[1], q_to_tab() was used, not q_to_cr(), and q_to_tab() seemed to fit quite well as a replacement for the original 'echo' chain[2] which emitted one whitespace-only line. Substituting that whitespace-only line with "Q" (which gets translated into a TAB) seems to fit much better than q_to_cr(). > will rewrite then using test_write_lines and I hope it is still as > readable without having to resort to the originl ugly chain of echo test_write_lines() doesn't do a very good job of representing -- at a glance -- what the "expected" output should be. The q_to_tab() block made it much more clear. (Having said that, it's probably not worth another re-roll just to change that.) [1]: https://lore.kernel.org/git/CAPig+cR8HKcbNxxw65ERz0iHvnO5aC6RXvF9NjvFTySXpcHCSQ@mail.gmail.com/ [2]: https://lore.kernel.org/git/20200426234750.40418-1-carenas@gmail.com/ ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v2] git-credential-store: skip empty lines and comments from store 2020-04-27 11:52 ` Jeff King 2020-04-27 12:25 ` Carlo Marcelo Arenas Belón @ 2020-04-27 17:47 ` Junio C Hamano 2020-04-27 19:09 ` Jeff King 1 sibling, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2020-04-27 17:47 UTC (permalink / raw) To: Jeff King; +Cc: Carlo Marcelo Arenas Belón, git, dirk, sunshine Jeff King <peff@peff.net> writes: > Those were never supposed to work. I'm mildly surprised that they did. I know that feeling X-<. It probably was a mistake to pretend that the file were editable with an editor, but it is too late to fix that. The next complainer may say that "; comment" no longer works. We probably should draw a line somewhere, but I am not sure where. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v2] git-credential-store: skip empty lines and comments from store 2020-04-27 17:47 ` Junio C Hamano @ 2020-04-27 19:09 ` Jeff King 0 siblings, 0 replies; 79+ messages in thread From: Jeff King @ 2020-04-27 19:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Carlo Marcelo Arenas Belón, git, dirk, sunshine On Mon, Apr 27, 2020 at 10:47:02AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Those were never supposed to work. I'm mildly surprised that they did. > > I know that feeling X-<. It probably was a mistake to pretend that > the file were editable with an editor, but it is too late to fix > that. I guess it is somewhat my fault for documenting the format at all git-credential-store.txt. I probably should have said "this is a black box and is subject to change". > The next complainer may say that "; comment" no longer works. We > probably should draw a line somewhere, but I am not sure where. If we get a bug report for "credential-store does not respect core.commentChar" I think my head would explode. :) -Peff ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 8:42 ` [PATCH v2] " Carlo Marcelo Arenas Belón 2020-04-27 11:52 ` Jeff King @ 2020-04-27 12:59 ` Carlo Marcelo Arenas Belón 2020-04-27 13:48 ` Philip Oakley ` (3 more replies) 1 sibling, 4 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-27 12:59 UTC (permalink / raw) To: git; +Cc: dirk, sunshine, peff, Carlo Marcelo Arenas Belón 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 formalize this "feature". 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) [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> --- v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 7 +++++++ credential-store.c | 3 +++ t/t0302-credential-store.sh | 15 +++++++++++++++ 3 files changed, 25 insertions(+) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 693dd9d9d7..48ab4b13e5 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -101,6 +101,13 @@ 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. +Note that the file used is not a configuration file and should be ideally +managed only through git, as any manually introduced typos will compromise +the validation of credentials. + +The parser will ignore any lines starting with the '#' character during +the processing of credentials for read, though. + GIT --- Part of the linkgit:git[1] suite diff --git a/credential-store.c b/credential-store.c index c010497cb2..b2f160890d 100644 --- a/credential-store.c +++ b/credential-store.c @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn, } while (strbuf_getline_lf(&line, fh) != EOF) { + strbuf_trim(&line); + if (line.len == 0 || *line.buf == '#') + 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..5e6ace3a06 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,19 @@ 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: allow for 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 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 12:59 ` [PATCH v3] " Carlo Marcelo Arenas Belón @ 2020-04-27 13:48 ` Philip Oakley 2020-04-28 1:49 ` Carlo Marcelo Arenas Belón 2020-04-27 15:39 ` Dirk ` (2 subsequent siblings) 3 siblings, 1 reply; 79+ messages in thread From: Philip Oakley @ 2020-04-27 13:48 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón, git; +Cc: dirk, sunshine, peff Hi Carlo, On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote: > 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 > formalize this "feature". > > trim all lines as they are being read from the store file and skip the Does trimming affect any credentials that may have spaces either end? I've not looked at the format. > ones that will be otherwise empty or that start with "#" (therefore > assuming them to be comments) > > [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> > --- > v3: > * avoid using q_to_cr as suggested by Peff > * a more verbose commit message and slightly more complete documentation > v2: > * use a here-doc for clarity as suggested by Eric > * improve commit message and include documentation > > Documentation/git-credential-store.txt | 7 +++++++ > credential-store.c | 3 +++ > t/t0302-credential-store.sh | 15 +++++++++++++++ > 3 files changed, 25 insertions(+) > > diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt > index 693dd9d9d7..48ab4b13e5 100644 > --- a/Documentation/git-credential-store.txt > +++ b/Documentation/git-credential-store.txt > @@ -101,6 +101,13 @@ 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. > > +Note that the file used is not a configuration file and should be ideally > +managed only through git, as any manually introduced typos will compromise > +the validation of credentials. > + > +The parser will ignore any lines starting with the '#' character during > +the processing of credentials for read, though. Should the trimming of leading/trailing spaces be mentioned? Also the git-credential page mentions that the credential must end with a blank line ("don't forget.."). Should that be mentioned here, or have I misunderstood? > + > GIT > --- > Part of the linkgit:git[1] suite > diff --git a/credential-store.c b/credential-store.c > index c010497cb2..b2f160890d 100644 > --- a/credential-store.c > +++ b/credential-store.c > @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn, > } > > while (strbuf_getline_lf(&line, fh) != EOF) { > + strbuf_trim(&line); > + if (line.len == 0 || *line.buf == '#') > + 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..5e6ace3a06 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -120,4 +120,19 @@ 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: allow for 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 Philip ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 13:48 ` Philip Oakley @ 2020-04-28 1:49 ` Carlo Marcelo Arenas Belón 2020-04-29 10:09 ` Philip Oakley 0 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 1:49 UTC (permalink / raw) To: Philip Oakley; +Cc: git, dirk, sunshine, peff On Mon, Apr 27, 2020 at 02:48:05PM +0100, Philip Oakley wrote: > On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote: > > 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 > > formalize this "feature". > > > > trim all lines as they are being read from the store file and skip the > > Does trimming affect any credentials that may have spaces either end? all credentials are url encoded so the only spaces that are affected are the ones that were added by careless editing of that file. as Eric pointed out, any tabs will be silently "cleaned" as well. > Should the trimming of leading/trailing spaces be mentioned? I wanted to keep that as an undocumented "feature" as it is just meant to help people to avoid a fatal error during the transition but didn't want to encourage people thinking it is a supported part or even worse encourage people to start editing their files to add tabs and spaces. Junio's suggested documentation fix[1] makes that clearer that I could > Also the git-credential page mentions that the credential must end with > a blank line ("don't forget.."). Should that be mentioned here, or have > I misunderstood? that is for the protocol part of it (which is used between git and the credential helper), the file format for this specific helper doesn't require that. Carlo [1] https://lore.kernel.org/git/xmqqv9lk7j7p.fsf@gitster.c.googlers.com/ ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-28 1:49 ` Carlo Marcelo Arenas Belón @ 2020-04-29 10:09 ` Philip Oakley 0 siblings, 0 replies; 79+ messages in thread From: Philip Oakley @ 2020-04-29 10:09 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff Hi Carlo, Thanks for the clarification. Philip On 28/04/2020 02:49, Carlo Marcelo Arenas Belón wrote: > On Mon, Apr 27, 2020 at 02:48:05PM +0100, Philip Oakley wrote: >> On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote: >>> 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 >>> formalize this "feature". >>> >>> trim all lines as they are being read from the store file and skip the >> Does trimming affect any credentials that may have spaces either end? > all credentials are url encoded so the only spaces that are affected are > the ones that were added by careless editing of that file. > > as Eric pointed out, any tabs will be silently "cleaned" as well. > >> Should the trimming of leading/trailing spaces be mentioned? > I wanted to keep that as an undocumented "feature" as it is just meant > to help people to avoid a fatal error during the transition but didn't > want to encourage people thinking it is a supported part or even worse > encourage people to start editing their files to add tabs and spaces. > > Junio's suggested documentation fix[1] makes that clearer that I could > >> Also the git-credential page mentions that the credential must end with >> a blank line ("don't forget.."). Should that be mentioned here, or have >> I misunderstood? > that is for the protocol part of it (which is used between git and the > credential helper), the file format for this specific helper doesn't > require that. > > Carlo > > [1] https://lore.kernel.org/git/xmqqv9lk7j7p.fsf@gitster.c.googlers.com/ ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 12:59 ` [PATCH v3] " Carlo Marcelo Arenas Belón 2020-04-27 13:48 ` Philip Oakley @ 2020-04-27 15:39 ` Dirk 2020-04-27 18:09 ` Junio C Hamano 2020-04-28 10:48 ` [PATCH v4 0/4] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón 3 siblings, 0 replies; 79+ messages in thread From: Dirk @ 2020-04-27 15:39 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón, git; +Cc: sunshine, peff Thank you very much. That's correct and ideal in my eyes. Regarding the comments, this is a new feature, of course. I think it's a worthless discussion about the question if the correct handling of empty lines are a bugfix or a new feature. In fact empty lines were handled correcty (ignored). So this might be considered a feature. But it wasn't documented, so it's a bugfix... Anyway. Thank you all. Dirk Am 27.04.20 um 14:59 schrieb Carlo Marcelo Arenas Belón: > 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 > formalize this "feature". > > 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) > > [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> > --- > v3: > * avoid using q_to_cr as suggested by Peff > * a more verbose commit message and slightly more complete documentation > v2: > * use a here-doc for clarity as suggested by Eric > * improve commit message and include documentation > > Documentation/git-credential-store.txt | 7 +++++++ > credential-store.c | 3 +++ > t/t0302-credential-store.sh | 15 +++++++++++++++ > 3 files changed, 25 insertions(+) > > diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt > index 693dd9d9d7..48ab4b13e5 100644 > --- a/Documentation/git-credential-store.txt > +++ b/Documentation/git-credential-store.txt > @@ -101,6 +101,13 @@ 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. > > +Note that the file used is not a configuration file and should be ideally > +managed only through git, as any manually introduced typos will compromise > +the validation of credentials. > + > +The parser will ignore any lines starting with the '#' character during > +the processing of credentials for read, though. > + > GIT > --- > Part of the linkgit:git[1] suite > diff --git a/credential-store.c b/credential-store.c > index c010497cb2..b2f160890d 100644 > --- a/credential-store.c > +++ b/credential-store.c > @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn, > } > > while (strbuf_getline_lf(&line, fh) != EOF) { > + strbuf_trim(&line); > + if (line.len == 0 || *line.buf == '#') > + 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..5e6ace3a06 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -120,4 +120,19 @@ 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: allow for 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 12:59 ` [PATCH v3] " Carlo Marcelo Arenas Belón 2020-04-27 13:48 ` Philip Oakley 2020-04-27 15:39 ` Dirk @ 2020-04-27 18:09 ` Junio C Hamano 2020-04-27 19:18 ` Jeff King 2020-04-28 10:48 ` [PATCH v4 0/4] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón 3 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2020-04-27 18:09 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > with the added checks for invalid URLs in credentials, any locally s/with/With/ > modified store files which might have empty lines or even comments > were reported[1] failing to parse as valid credentials. 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? I'd rather not to do either, if we did not have to, but if it were necessary for us to do something, I am OK to ignore empty lines. But I'd prefer not to mix the new "# comment" feature in, if we did not have to. Also, triming the lines that are not empty is unwarranted. IIUC, what the "store" action writes encodes whitespaces, so as soon as you see whitespace on either end, (or anywhere on the line for that matter), it is a hand-edited cruft in the file. If you ignore comments, you probably should ignore those lines, too. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 18:09 ` Junio C Hamano @ 2020-04-27 19:18 ` Jeff King 2020-04-27 20:43 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Jeff King @ 2020-04-27 19:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Carlo Marcelo Arenas Belón, git, dirk, sunshine On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote: > > modified store files which might have empty lines or even comments > > were reported[1] failing to parse as valid credentials. > > 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. > I'd rather not to do either, if we did not have to, but if it were > necessary for us to do something, I am OK to ignore empty lines. > But I'd prefer not to mix the new "# comment" feature in, if we did > not have to. > > Also, triming the lines that are not empty is unwarranted. IIUC, > what the "store" action writes encodes whitespaces, so as soon as > you see whitespace on either end, (or anywhere on the line for that > matter), it is a hand-edited cruft in the file. If you ignore > comments, you probably should ignore those lines, too. Yeah, all of that seems quite sensible. -Peff ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 19:18 ` Jeff King @ 2020-04-27 20:43 ` Junio C Hamano 2020-04-27 21:10 ` Jeff King ` (2 more replies) 0 siblings, 3 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-27 20:43 UTC (permalink / raw) To: Jeff King; +Cc: Carlo Marcelo Arenas Belón, git, dirk, sunshine Jeff King <peff@peff.net> writes: > On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote: > >> > modified store files which might have empty lines or even comments >> > were reported[1] failing to parse as valid credentials. >> >> 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? >> I'd rather not to do either, if we did not have to, but if it were >> necessary for us to do something, I am OK to ignore empty lines. >> But I'd prefer not to mix the new "# comment" feature in, if we did >> not have to. >> >> Also, triming the lines that are not empty is unwarranted. IIUC, >> what the "store" action writes encodes whitespaces, so as soon as >> you see whitespace on either end, (or anywhere on the line for that >> matter), it is a hand-edited cruft in the file. If you ignore >> comments, you probably should ignore those lines, too. > > Yeah, all of that seems quite sensible. I think the first patch we need is a (belated) documentation patch, that adds to the existing "STORAGE FORMAT". We already say "Each credential is stored on its own line as a URL", but we do not say anything about allowing other cruft in the file. We probably should. Adding a "comment" feature, if anybody feels like it, is OK and we can loosen the paragraph when that happens. -- >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, credential-store will consider that context a pattern to match against each entry in the credentials file. If the protocol, hostname, and ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 20:43 ` Junio C Hamano @ 2020-04-27 21:10 ` Jeff King 2020-04-28 1:37 ` Carlo Marcelo Arenas Belón 2020-04-27 23:49 ` Carlo Marcelo Arenas Belón 2020-04-28 5:25 ` Jonathan Nieder 2 siblings, 1 reply; 79+ messages in thread From: Jeff King @ 2020-04-27 21:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Carlo Marcelo Arenas Belón, git, dirk, sunshine 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? Yes, exactly. If I start with: cat >creds <<\EOF # this is a comment https://user:pass@example.com/ EOF then: echo url=https://other:pass@other.example.com | git credential-store --file=creds store with v2.26.0 results in: https://other:pass@other.example.com # this is a comment https://user:pass@example.com but applying the patch on top: https://other:pass@other.example.com https://user:pass@example.com/ That raises another issue about comments, too: we make no promises about where we write new entries. They could be inserted between a comment line and one it is meant to annotate (that line could even be moved if we reject and then re-approve a credential). > I think the first patch we need is a (belated) documentation patch, > that adds to the existing "STORAGE FORMAT". We already say "Each > credential is stored on its own line as a URL", but we do not say > anything about allowing other cruft in the file. We probably > should. Adding a "comment" feature, if anybody feels like it, is OK > and we can loosen the paragraph when that happens. The more I look into it, the more negative I am on adding such a comment feature. > -- >8 -- > Subject: credential-store: document the file format a bit more Yeah, this certainly clarifies things. I suppose one could ask why we'd bother documenting the format at all, then, though I do think it could be helpful for debugging. > 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. s/torelate/tolerate/ -Peff ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 21:10 ` Jeff King @ 2020-04-28 1:37 ` Carlo Marcelo Arenas Belón 0 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 1:37 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, dirk, sunshine 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 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 20:43 ` Junio C Hamano 2020-04-27 21:10 ` Jeff King @ 2020-04-27 23:49 ` Carlo Marcelo Arenas Belón 2020-04-28 5:25 ` Jonathan Nieder 2 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-27 23:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, dirk, sunshine On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote: > > > >> > modified store files which might have empty lines or even comments > >> > were reported[1] failing to parse as valid credentials. > >> > >> 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? yes, and it also helps users that might have added spaces or tabs around their credentials while editing them to still be able to use those instead of just failing to match them. IMHO the only "regression" I was fixing was the fact that the current code will get to throw a fatal error with an unhelpful message and prevent access to valid credentials as shown by : $ /git credential fill protocol=http host=example.com warning: url has no scheme: fatal: credential url cannot be parsed: Username for 'http://example.com': > >> I'd rather not to do either, if we did not have to, but if it were > >> necessary for us to do something, I am OK to ignore empty lines. > >> But I'd prefer not to mix the new "# comment" feature in, if we did > >> not have to. > >> > >> Also, triming the lines that are not empty is unwarranted. IIUC, > >> what the "store" action writes encodes whitespaces, so as soon as > >> you see whitespace on either end, (or anywhere on the line for that > >> matter), it is a hand-edited cruft in the file. If you ignore > >> comments, you probably should ignore those lines, too. > > > > Yeah, all of that seems quite sensible. > > I think the first patch we need is a (belated) documentation patch, > that adds to the existing "STORAGE FORMAT". We already say "Each > credential is stored on its own line as a URL", but we do not say > anything about allowing other cruft in the file. We probably > should. Adding a "comment" feature, if anybody feels like it, is OK > and we can loosen the paragraph when that happens. > > -- >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. view should be ok; mentioning that any typos or extraneous characters will compromise the validation of credentials like I mentioned in my proposed documentation update probably worth doing here instead, too. Carlo ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-27 20:43 ` Junio C Hamano 2020-04-27 21:10 ` Jeff King 2020-04-27 23:49 ` Carlo Marcelo Arenas Belón @ 2020-04-28 5:25 ` Jonathan Nieder 2020-04-28 5:41 ` Jeff King ` (2 more replies) 2 siblings, 3 replies; 79+ messages in thread From: Jonathan Nieder @ 2020-04-28 5:25 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Carlo Marcelo Arenas Belón, git, dirk, sunshine, Stefan Tauner 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 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-28 5:25 ` Jonathan Nieder @ 2020-04-28 5:41 ` Jeff King 2020-04-28 7:18 ` Carlo Marcelo Arenas Belón 2020-04-28 10:58 ` Stefan Tauner 2020-04-28 16:03 ` Junio C Hamano 2 siblings, 1 reply; 79+ messages in thread From: Jeff King @ 2020-04-28 5:41 UTC (permalink / raw) To: Jonathan Nieder Cc: Junio C Hamano, Carlo Marcelo Arenas Belón, git, dirk, sunshine, Stefan Tauner On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote: > 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. I like that solution, as it mostly brings us back to the original behavior, as weird or unexpected as it was. The only I think would be different is: ://user:pass@example.com which I think the old code would have treated as matching any protocol (and the new code will reject as a bogus URL). I wondered if you'd need to free the partially-parsed struct fields on failure, but the answer is no; we'd clear it the next time through the loop, or at the very end of the function. -Peff ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-28 5:41 ` Jeff King @ 2020-04-28 7:18 ` Carlo Marcelo Arenas Belón 2020-04-28 8:16 ` Jeff King 0 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 7:18 UTC (permalink / raw) To: Jeff King Cc: Jonathan Nieder, Junio C Hamano, git, dirk, sunshine, Stefan Tauner On Tue, Apr 28, 2020 at 01:41:55AM -0400, Jeff King wrote: > On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote: > > > 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. > > I like that solution, as it mostly brings us back to the original > behavior, as weird or unexpected as it was. I like this version better as well, and we could even reuse my test case. it wouldn't cover cases where there were leading spaces/tabs around the credential which I have to admit I liked just because it is more robust to bad input, and there is no sane way now to tell the user that there is invalid data anyway, but I am ok eitherway. Carlo ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-28 7:18 ` Carlo Marcelo Arenas Belón @ 2020-04-28 8:16 ` Jeff King 2020-04-28 11:25 ` Carlo Marcelo Arenas Belón 0 siblings, 1 reply; 79+ messages in thread From: Jeff King @ 2020-04-28 8:16 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: Jonathan Nieder, Junio C Hamano, git, dirk, sunshine, Stefan Tauner On Tue, Apr 28, 2020 at 12:18:02AM -0700, Carlo Marcelo Arenas Belón wrote: > > > 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. > > > > I like that solution, as it mostly brings us back to the original > > behavior, as weird or unexpected as it was. > > I like this version better as well, and we could even reuse my test > case. > > it wouldn't cover cases where there were leading spaces/tabs around > the credential which I have to admit I liked just because it is > more robust to bad input, and there is no sane way now to tell the > user that there is invalid data anyway, but I am ok eitherway. I think if we're discouraging people from hand-editing the file, then that feature would be going in the wrong direction anyway. Did you or Jonathan want to wrap it up with the test and commit message? -Peff ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-28 8:16 ` Jeff King @ 2020-04-28 11:25 ` Carlo Marcelo Arenas Belón 0 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 11:25 UTC (permalink / raw) To: Jeff King Cc: Jonathan Nieder, Junio C Hamano, git, dirk, sunshine, Stefan Tauner On Tue, Apr 28, 2020 at 04:16:27AM -0400, Jeff King wrote: > On Tue, Apr 28, 2020 at 12:18:02AM -0700, Carlo Marcelo Arenas Belón wrote: > > > > it wouldn't cover cases where there were leading spaces/tabs around > > the credential which I have to admit I liked just because it is > > more robust to bad input, and there is no sane way now to tell the > > user that there is invalid data anyway, but I am ok eitherway. > > I think if we're discouraging people from hand-editing the file, then > that feature would be going in the wrong direction anyway. I think it will actually encourage them to edit the file (and complain of a regression), since they can see the credential is correct, but somehow git fails to work with it unlike their previous version. remember there is no warning that could explain why that happens and with this pach there is no error either. > Did you or Jonathan want to wrap it up with the test and commit message? I was going to say Jonathan will have to do it, since he was the only one that didn't provide a patch (and therefore a sign off) but guess I can save everyone some time and practice my git-send-email skills (which obviously failed me) Jonathan, make sure your patch[1] is complete and correct and to resend it with a sign off, feel free to add me as Tested-by if you feel like, but I added myself as Helped-by anyway ;) Junio, didn't fix the typo that Peff suggested fixed[2], because I forgot and I wasn't sure if that also means I should SOB it, but if you could also consider my suggestion[3] then at you get my SOB warranted ;) Carlo PS. guess I should had send it as an RFC instead [1] https://lore.kernel.org/git/20200428105254.28658-3-carenas@gmail.com/ [2] https://lore.kernel.org/git/20200427211013.GB1732530@coredump.intra.peff.net/ [3] https://lore.kernel.org/git/20200427234909.GC61348@Carlos-MBP/ ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-28 5:25 ` Jonathan Nieder 2020-04-28 5:41 ` Jeff King @ 2020-04-28 10:58 ` Stefan Tauner 2020-04-28 16:03 ` Junio C Hamano 2 siblings, 0 replies; 79+ messages in thread From: Stefan Tauner @ 2020-04-28 10:58 UTC (permalink / raw) To: Jonathan Nieder Cc: Junio C Hamano, Jeff King, Carlo Marcelo Arenas Belón, git, dirk, sunshine On Mon, 27 Apr 2020 22:25:10 -0700 Jonathan Nieder <jrnieder@gmail.com> wrote: > 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 can't remember for sure - it's literally years ago - but I am pretty sure I did not *populate* it manually initially. However, I might have edited it to separate entries (https vs. smtp) and I might have added additional entries by c&p & editing. I have replaced the one instance of \n\n used for separation with a normal single line break between entries and how it works fine. Before that I got the following output (that's obviously not very helpful if you don't know the whole story :) > git pull > warning: url has no scheme: > fatal: credential url cannot be parsed: > Already up to date. > Current branch master is up to date. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-28 5:25 ` Jonathan Nieder 2020-04-28 5:41 ` Jeff King 2020-04-28 10:58 ` Stefan Tauner @ 2020-04-28 16:03 ` Junio C Hamano 2020-04-28 21:14 ` Carlo Marcelo Arenas Belón 2 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2020-04-28 16:03 UTC (permalink / raw) To: Jonathan Nieder Cc: Jeff King, Carlo Marcelo Arenas Belón, git, dirk, sunshine, Stefan Tauner 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? 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? ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-28 16:03 ` Junio C Hamano @ 2020-04-28 21:14 ` Carlo Marcelo Arenas Belón 2020-04-28 21:17 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 21:14 UTC (permalink / raw) To: Junio C Hamano Cc: Jonathan Nieder, Jeff King, git, dirk, sunshine, Stefan Tauner 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 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v3] git-credential-store: skip empty lines and comments from store 2020-04-28 21:14 ` Carlo Marcelo Arenas Belón @ 2020-04-28 21:17 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-28 21:17 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: Jonathan Nieder, Jeff King, git, dirk, sunshine, Stefan Tauner Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: >> > + 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 Ah, sorry, I apparently did not read the patch carefully enough. Thanks for a correction. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v4 0/4] credential-store: prevent fatal errors 2020-04-27 12:59 ` [PATCH v3] " Carlo Marcelo Arenas Belón ` (2 preceding siblings ...) 2020-04-27 18:09 ` Junio C Hamano @ 2020-04-28 10:48 ` Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 1/4] credential-store: document the file format a bit more Carlo Marcelo Arenas Belón 2020-04-29 0:33 ` [PATCH v5] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 3 siblings, 2 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 10:48 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón 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] as failing to parse as valid credentials. document better the format for the credential file and make sure any failures are still handled gently by the new code as a first step to allow people to upgrade without having first to fix their corrupted credential files. [1] https://stackoverflow.com/a/61420852/5005936 Carlo Marcelo Arenas Belón (2): git-credential-store: skip empty lines and comments from store credential-store: make sure there is no regression with missing scheme Jonathan Nieder (1): git-credential-store: fix (WIP) Junio C Hamano (1): credential-store: document the file format a bit more Documentation/git-credential-store.txt | 4 ++++ credential-store.c | 4 ++-- t/t0302-credential-store.sh | 32 ++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v4 1/4] credential-store: document the file format a bit more 2020-04-28 10:48 ` [PATCH v4 0/4] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón @ 2020-04-28 10:52 ` Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón ` (4 more replies) 2020-04-29 0:33 ` [PATCH v5] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 1 sibling, 5 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 10:52 UTC (permalink / raw) To: git; +Cc: dirk, sunshine, peff, gitster, jrnieder From: Junio C Hamano <gitster@pobox.com> 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, credential-store will consider that context a pattern to match against each entry in the credentials file. If the protocol, hostname, and -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store 2020-04-28 10:52 ` [PATCH v4 1/4] credential-store: document the file format a bit more Carlo Marcelo Arenas Belón @ 2020-04-28 10:52 ` Carlo Marcelo Arenas Belón 2020-04-28 16:09 ` Eric Sunshine 2020-04-28 10:52 ` [PATCH v4 3/4] git-credential-store: fix (WIP) Carlo Marcelo Arenas Belón ` (3 subsequent siblings) 4 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 10:52 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón 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. add corresponding failing cases [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> --- t/t0302-credential-store.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..94cdcb9e56 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi test_must_be_empty "$HOME/.config/git/credentials" ' +test_expect_failure 'get: store file can contain empty/bogus lines' ' + 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 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store 2020-04-28 10:52 ` [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón @ 2020-04-28 16:09 ` Eric Sunshine 2020-04-28 16:42 ` Carlo Marcelo Arenas Belón 0 siblings, 1 reply; 79+ messages in thread From: Eric Sunshine @ 2020-04-28 16:09 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: Git List, Dirk, Jeff King, Junio C Hamano, Jonathan Nieder On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote: > git-credential-store: skip empty lines and comments from store I don't see anything in this patch which makes it skip anything at all; it only introduces a new test. > 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. > > add corresponding failing cases > > [1] https://stackoverflow.com/a/61420852/5005936 > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store 2020-04-28 16:09 ` Eric Sunshine @ 2020-04-28 16:42 ` Carlo Marcelo Arenas Belón 0 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 16:42 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Dirk, Jeff King, Junio C Hamano, Jonathan Nieder On Tue, Apr 28, 2020 at 12:09:50PM -0400, Eric Sunshine wrote: > On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón > <carenas@gmail.com> wrote: > > git-credential-store: skip empty lines and comments from store > > I don't see anything in this patch which makes it skip anything at > all; it only introduces a new test. my bad, forgot to fix the subject when rerolling; does something like: credential-store: show "regression" when bogus credentials in file make sense for the reroll? Carlo ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v4 3/4] git-credential-store: fix (WIP) 2020-04-28 10:52 ` [PATCH v4 1/4] credential-store: document the file format a bit more Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón @ 2020-04-28 10:52 ` Carlo Marcelo Arenas Belón 2020-04-28 16:11 ` Eric Sunshine 2020-04-28 10:52 ` [PATCH v4 4/4] credential-store: make sure there is no regression with missing scheme Carlo Marcelo Arenas Belón ` (2 subsequent siblings) 4 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 10:52 UTC (permalink / raw) To: git; +Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belon From: Jonathan Nieder <jrnieder@gmail.com> Helped-by: Carlo Marcelo Arenas Belon <carenas@gmail.com> --- credential-store.c | 4 ++-- t/t0302-credential-store.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/credential-store.c b/credential-store.c index c010497cb2..294e771681 100644 --- a/credential-store.c +++ b/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) { diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index 94cdcb9e56..4e5a73cb99 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,7 +120,7 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi test_must_be_empty "$HOME/.config/git/credentials" ' -test_expect_failure 'get: store file can contain empty/bogus lines' ' +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 -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v4 3/4] git-credential-store: fix (WIP) 2020-04-28 10:52 ` [PATCH v4 3/4] git-credential-store: fix (WIP) Carlo Marcelo Arenas Belón @ 2020-04-28 16:11 ` Eric Sunshine 2020-04-28 17:14 ` Carlo Marcelo Arenas Belón 0 siblings, 1 reply; 79+ messages in thread From: Eric Sunshine @ 2020-04-28 16:11 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: Git List, Dirk, Jeff King, Junio C Hamano, Jonathan Nieder On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote: > From: Jonathan Nieder <jrnieder@gmail.com> > Subject: git-credential-store: fix (WIP) Um, what? Did you forget to squash this into the previous patch? > Helped-by: Carlo Marcelo Arenas Belon <carenas@gmail.com> No sign-off? (Jonathan's) ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 3/4] git-credential-store: fix (WIP) 2020-04-28 16:11 ` Eric Sunshine @ 2020-04-28 17:14 ` Carlo Marcelo Arenas Belón 0 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 17:14 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Dirk, Jeff King, Junio C Hamano, Jonathan Nieder On Tue, Apr 28, 2020 at 12:11:20PM -0400, Eric Sunshine wrote: > On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón > <carenas@gmail.com> wrote: > > From: Jonathan Nieder <jrnieder@gmail.com> > > Subject: git-credential-store: fix (WIP) > > Um, what? Did you forget to squash this into the previous patch? no; was documenting his proposal from 20200428052510.GA201501@google.com[1] with the hope (unsuccesfully) to save everyone time and easy testing by having a series ready to apply. > > Helped-by: Carlo Marcelo Arenas Belon <carenas@gmail.com> > > No sign-off? (Jonathan's) really my fault, but was expected as documented in: https://lore.kernel.org/git/20200428112518.GA15981@Carlos-MBP/ sorry for the confussion and thanks for your review Carlo [1] https://lore.kernel.org/git/20200428052510.GA201501@google.com/ ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v4 4/4] credential-store: make sure there is no regression with missing scheme 2020-04-28 10:52 ` [PATCH v4 1/4] credential-store: document the file format a bit more Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 3/4] git-credential-store: fix (WIP) Carlo Marcelo Arenas Belón @ 2020-04-28 10:52 ` Carlo Marcelo Arenas Belón 2020-04-28 16:06 ` [PATCH v4 1/4] credential-store: document the file format a bit more Eric Sunshine 2020-04-28 18:15 ` Junio C Hamano 4 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-28 10:52 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18) made scheme (AKA protocol) mandatory so make sure that still fails. Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- t/t0302-credential-store.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index 4e5a73cb99..7c7a48303f 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -135,4 +135,21 @@ test_expect_success 'get: store file can contain empty/bogus lines' ' EOF ' +test_expect_success 'get: ignore credentials without scheme' ' + echo "://user:pass@example.com" >"$HOME/.git-credentials" && + check fill store <<-\EOF + protocol=https + host=example.com + -- + 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_done -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v4 1/4] credential-store: document the file format a bit more 2020-04-28 10:52 ` [PATCH v4 1/4] credential-store: document the file format a bit more Carlo Marcelo Arenas Belón ` (2 preceding siblings ...) 2020-04-28 10:52 ` [PATCH v4 4/4] credential-store: make sure there is no regression with missing scheme Carlo Marcelo Arenas Belón @ 2020-04-28 16:06 ` Eric Sunshine 2020-04-28 18:18 ` Junio C Hamano 2020-04-28 18:15 ` Junio C Hamano 4 siblings, 1 reply; 79+ messages in thread From: Eric Sunshine @ 2020-04-28 16:06 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: Git List, Dirk, Jeff King, Junio C Hamano, Jonathan Nieder On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote: > 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> > --- > diff --git 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: > +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. I suggest dropping the "even though some may be silently ignored" bit since it's both mysterious (providing no concrete information) and unnecessarily confusing since it flat out contradicts the earlier part of the sentence. The fact that the implementation has accidentally been loose in its parsing doesn't warrant introducing such ambiguity into the (newly-added) documentation. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 1/4] credential-store: document the file format a bit more 2020-04-28 16:06 ` [PATCH v4 1/4] credential-store: document the file format a bit more Eric Sunshine @ 2020-04-28 18:18 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-28 18:18 UTC (permalink / raw) To: Eric Sunshine Cc: Carlo Marcelo Arenas Belón, Git List, Dirk, Jeff King, Jonathan Nieder Eric Sunshine <sunshine@sunshineco.com> writes: > On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón > <carenas@gmail.com> wrote: >> 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> >> --- >> diff --git 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: >> +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. > > I suggest dropping the "even though some may be silently ignored" bit > since it's both mysterious (providing no concrete information) and > unnecessarily confusing since it flat out contradicts the earlier part > of the sentence. The fact that the implementation has accidentally > been loose in its parsing doesn't warrant introducing such ambiguity > into the (newly-added) documentation. I do not think it is an ambiguity. The phrase is there just to remind the readers that they are not allowed to take a loose implementation in the past as an excuse to throw in crufts and expect the result to continue working. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 1/4] credential-store: document the file format a bit more 2020-04-28 10:52 ` [PATCH v4 1/4] credential-store: document the file format a bit more Carlo Marcelo Arenas Belón ` (3 preceding siblings ...) 2020-04-28 16:06 ` [PATCH v4 1/4] credential-store: document the file format a bit more Eric Sunshine @ 2020-04-28 18:15 ` Junio C Hamano 4 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-28 18:15 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff, jrnieder Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > From: Junio C Hamano <gitster@pobox.com> > > Reading a malformed credential URL line and silently ignoring it > does not mean ... I'd reuse the one I've already queued as 272281ef (credential-store: document the file format a bit more, 2020-04-27) for this step. Thanks. -- >8 -- Subject: [PATCH] credential-store: document the file format a bit more Reading a malformed credential URL line and silently ignoring it does not mean that we support empty lines and/or "# commented" lines forever. We should document it to avoid confusion. 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, credential-store will consider that context a pattern to match against each entry in the credentials file. If the protocol, hostname, and -- 2.26.2-266-ge870325ee8 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5] credential-store: warn instead of fatal for bogus lines from store 2020-04-28 10:48 ` [PATCH v4 0/4] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 1/4] credential-store: document the file format a bit more Carlo Marcelo Arenas Belón @ 2020-04-29 0:33 ` Carlo Marcelo Arenas Belón 2020-04-29 4:36 ` Junio C Hamano 2020-04-29 20:35 ` [RFC PATCH v6 0/2] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón 1 sibling, 2 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-29 0:33 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón 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. instead of doing a hard check for credentials, do a soft one and warn the user so any invalid entries could be corrected. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v5: * q_to_tab this round, with a single echo to make sure empty line is covered, as that seems to be a popular issue * rebase on top of jc/credential-store-file-format-doc * implement a redacter for credentials to use on errora to avoid leaking passwordss v4: * use credential_from_url_gently instead as shown by Jonathan * add documentation to clarify "comments" is not a supported feature v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 12 +++++- credential-store.c | 51 ++++++++++++++++++++++---- t/t0302-credential-store.sh | 43 ++++++++++++++++++++++ 3 files changed, 96 insertions(+), 10 deletions(-) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 76b0798856..30d498fe54 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -95,8 +95,16 @@ 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. +allowed in the file, even though historically the parser was very +lenient and some might had been silently ignored. + +Do not edit the file with editors as it could compromise the validity +of your credentials by sometimes subtle formatting issues (like spaces) + +In cases where those formatting issues are detected during parsing a +warning indicating the line found will be printed to stderr so it can +be corrected at your earliest convenience, but the remaining valid +credentials will be used to try to find a match as described below. When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against diff --git a/credential-store.c b/credential-store.c index c010497cb2..5324c56ce1 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) != '/') { + 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,16 +51,24 @@ 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 && - 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) + else { + char *redacted = redact_credential(&line); + warning("%s:%d %s: %s", fn, lineno, + _("ignoring invalid credential"), redacted); + free(redacted); + } + if (!found_credential && other_cb) other_cb(&line); } diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..68b59e6f98 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,47 @@ 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: credentials without scheme are invalid' ' + echo "://user:secret@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 + 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 && + ! grep secret stderr +' + +test_expect_success 'get: store file can contain empty/bogus lines' ' + echo "" > "$HOME/.git-credentials" && + q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" && + #comment + Q + https://user:pass@example.com + CONFIG + 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 + EOF + test_cmp expect-stdout stdout && + test_i18ngrep "ignoring invalid credential" stderr && + test_line_count = 3 stderr +' + test_done base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0 -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 0:33 ` [PATCH v5] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón @ 2020-04-29 4:36 ` Junio C Hamano 2020-04-29 7:31 ` Carlo Marcelo Arenas Belón 2020-04-29 20:35 ` [RFC PATCH v6 0/2] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón 1 sibling, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2020-04-29 4:36 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff, jrnieder Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > +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, ':'); Just showing my ignorance, but ... - Is the above strrchr() that forbids a colon in the password intended, or should it be strchr() that only forbids a colon in the username instead? - Would it hurt to redact both username and password as sensitive? If not, it would certainly make it simpler to unconditionally: int i; for (i = 0; i < redacted_line.len; i++) { if (redacted_line.buf[i] != ':') redacted_line.buf[i] = 'x'; } ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 4:36 ` Junio C Hamano @ 2020-04-29 7:31 ` Carlo Marcelo Arenas Belón 2020-04-29 16:46 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-29 7:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, dirk, sunshine, peff, jrnieder On Tue, Apr 28, 2020 at 09:36:00PM -0700, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > +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, ':'); > > Just showing my ignorance, but ... > > - Is the above strrchr() that forbids a colon in the password > intended, or should it be strchr() that only forbids a colon in > the username instead? neither; that ":" is the separator we put in there between username and password (unless it was edited out), as any original ":" in both is already urlencoded. strrchr() was used in purpose to back only as much as it is needed from the string to hopefully find the beginning of the password. indeed, most of the uglyness of the code comes from the fact it tries really hard to only redact the password so the rest of the credential is still useful for context and do so without changing the original line. > - Would it hurt to redact both username and password as sensitive? > If not, it would certainly make it simpler to unconditionally: > > int i; > for (i = 0; i < redacted_line.len; i++) { > if (redacted_line.buf[i] != ':') > redacted_line.buf[i] = 'x'; > } that would leak the length of the password which might also be considered sensitive IMHO if we are redacting both might as well not print anything and let the user find the offending credential by line number instead ;) after all I got a feeling most of those entries are empty lines or "comments". for example with the current code merged in pu we get : $ cat secretfile ://u:p@example.com http://user:some%3apass@example.com $ git credential-store --file secretfile get protocol=http warning: secretfile:1 ignoring invalid credential: warning: secretfile:2 ignoring invalid credential: ://u:<redacted>@example.com username=user password=some:pass Carlo ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 7:31 ` Carlo Marcelo Arenas Belón @ 2020-04-29 16:46 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-29 16:46 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff, jrnieder Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > IMHO if we are redacting both might as well not print anything and let > the user find the offending credential by line number instead ;) after > all I got a feeling most of those entries are empty lines or "comments". Very sensible. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [RFC PATCH v6 0/2] credential-store: prevent fatal errors 2020-04-29 0:33 ` [PATCH v5] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 2020-04-29 4:36 ` Junio C Hamano @ 2020-04-29 20:35 ` Carlo Marcelo Arenas Belón 2020-04-29 20:35 ` [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón ` (2 more replies) 1 sibling, 3 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-29 20:35 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón 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] as failing to parse as valid credentials. document better the format for the credential file and make sure any failures are still handled gently by the new code to avoid regressions but make sure that any invalid credentials are flagged as such instead of being silently ignored. ideally would like to add a test against the use of cert:// to make sure that is still well supported but don't know of any use case or if it is being used with store (AFAIK, it could and at least in theory it should still work fine with the added validations), hence sending this as an RFC. also, there is no real reason to have 2 patches but I am keeping them separated because the first one should be AFAIK otherwise almost ready. but considering that we are validating since v4 against a missing protocol and printing a warning since v5, it seems we should then fully avoid all those silently ignored issues as well (as shown in patch 2) which I hope we could squash and release together (if agreed) [1] https://stackoverflow.com/a/61420852/5005936 Carlo Marcelo Arenas Belón (2): credential-store: warn instead of fatal for bogus lines from store credential-store: warn for any incomplete credentials instead of using --- v6: * get rid of redacter and only use line number for context * make validation more strict to also catch incomplete credentials v5: * q_to_tab this round, with a single echo to make sure empty line is covered, as that seems to be a popular issue * rebase on top of jc/credential-store-file-format-doc * implement a redacter for credentials to use on errors to avoid leaking passwordss v4: * use credential_from_url_gently instead as shown by Jonathan * add documentation to clarify "comments" is not a supported feature v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 12 +++- credential-store.c | 23 +++++--- t/t0302-credential-store.sh | 80 ++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 10 deletions(-) base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0 -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply [flat|nested] 79+ messages in thread
* [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 20:35 ` [RFC PATCH v6 0/2] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón @ 2020-04-29 20:35 ` Carlo Marcelo Arenas Belón 2020-04-29 21:05 ` Junio C Hamano 2020-04-29 20:35 ` [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using Carlo Marcelo Arenas Belón 2020-04-29 23:23 ` [PATCH v6] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 2 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-29 20:35 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón 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. instead of doing a hard check for credentials, do a soft one and warn the user so any invalid entries could be corrected. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Documentation/git-credential-store.txt | 12 ++++++-- credential-store.c | 22 +++++++++----- t/t0302-credential-store.sh | 42 ++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 76b0798856..30d498fe54 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -95,8 +95,16 @@ 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. +allowed in the file, even though historically the parser was very +lenient and some might had been silently ignored. + +Do not edit the file with editors as it could compromise the validity +of your credentials by sometimes subtle formatting issues (like spaces) + +In cases where those formatting issues are detected during parsing a +warning indicating the line found will be printed to stderr so it can +be corrected at your earliest convenience, but the remaining valid +credentials will be used to try to find a match as described below. When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against diff --git a/credential-store.c b/credential-store.c index c010497cb2..1cc5ca081a 100644 --- a/credential-store.c +++ b/credential-store.c @@ -15,6 +15,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,16 +25,21 @@ 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 && - 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) + else + warning(_("ignoring invalid credential in %s:%d"), + fn, lineno); + if (!found_credential && other_cb) other_cb(&line); } diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..801c1eb200 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,46 @@ 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: 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 + 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: store file can contain empty/bogus lines' ' + echo "" > "$HOME/.git-credentials" && + q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" && + #comment + Q + https://user:pass@example.com + CONFIG + 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 + EOF + test_cmp expect-stdout stdout && + test_i18ngrep "ignoring invalid credential" stderr && + test_line_count = 3 stderr +' + test_done -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 20:35 ` [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón @ 2020-04-29 21:05 ` Junio C Hamano 2020-04-29 21:17 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2020-04-29 21:05 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff, jrnieder Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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. > > instead of doing a hard check for credentials, do a soft one and > warn the user so any invalid entries could be corrected. Yeah, this version looks be best so far. No need to worry about redacting anything; we are dealing with folks who have edited the file, and they shouldn't have any trouble going to the line with a problem given an exact line number. > diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt > index 76b0798856..30d498fe54 100644 > --- a/Documentation/git-credential-store.txt > +++ b/Documentation/git-credential-store.txt > @@ -95,8 +95,16 @@ 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. > +allowed in the file, even though historically the parser was very > +lenient and some might had been silently ignored. > + > +Do not edit the file with editors as it could compromise the validity > +of your credentials by sometimes subtle formatting issues (like spaces) I do not think dropping "view or" is warranted. There is no need to invite the risk of opening with the intention to only view and then end up saving a modification. In other words, do not encourage use of an *editor* in any way. > +In cases where those formatting issues are detected during parsing a > +warning indicating the line found will be printed to stderr so it can > +be corrected at your earliest convenience, but the remaining valid > +credentials will be used to try to find a match as described below. I am often just as guilty, but the above four lines in a single sentence is hard to read, especially without minimum number of comma. With a comma after "during parsing", and another after "to stderr", might make it more readable, but at that point, it would become far more readable if we split them into two sentences. Perhaps An unparseable line is ignored, and a warning message points out the line number the problematic line appears in (you may want to delete them with an editor). > while (strbuf_getline_lf(&line, fh) != EOF) { > + 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 > + warning(_("ignoring invalid credential in %s:%d"), > + fn, lineno); > + if (!found_credential && other_cb) > other_cb(&line); > } The above is harder to follow than necessary. while (... != EOF) { lineno++; if (credential is not well formed) { warning(_("ignoring...")); } else if (the entry matches) { found_credential = 1; if (match_cb) { match_cb(&entry); break; /* stop at the first match */ } continue; } if (other_cb) other_cb(&line); } may make the intention a bit clearer, with the loud "continue" inside. (1) we give warning for malformed one; and (2) we do not let other_cb touch a matching entry. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 21:05 ` Junio C Hamano @ 2020-04-29 21:17 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-29 21:17 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff, jrnieder Junio C Hamano <gitster@pobox.com> writes: >> while (strbuf_getline_lf(&line, fh) != EOF) { >> + 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 >> + warning(_("ignoring invalid credential in %s:%d"), >> + fn, lineno); >> + if (!found_credential && other_cb) >> other_cb(&line); >> } > > The above is harder to follow than necessary. > > while (... != EOF) { > lineno++; > if (credential is not well formed) { After reading your [2/2], I think this part deserves a bit of explanation. By "is not well formed", what I mean is not just credential_from_url_gently() returns non-zero, but also user/pass are specified. And your step [2/2] may make the condition for a line to be "well formed" even stricter by requiring at least one of host or path, and that would also belong to this test. > warning(_("ignoring...")); > } else if (the entry matches) { And the "entry matches" test here would only be "what does credential_match() say?" > found_credential = 1; > if (match_cb) { > match_cb(&entry); > break; /* stop at the first match */ > } > continue; > } > > if (other_cb) > other_cb(&line); > } > > may make the intention a bit clearer, with the loud "continue" inside. > > (1) we give warning for malformed one; and > (2) we do not let other_cb touch a matching entry. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using 2020-04-29 20:35 ` [RFC PATCH v6 0/2] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón 2020-04-29 20:35 ` [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón @ 2020-04-29 20:35 ` Carlo Marcelo Arenas Belón 2020-04-29 21:12 ` Junio C Hamano 2020-04-29 23:23 ` [PATCH v6] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 2 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-29 20:35 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón originally any credential found was tried for matching as far as it had a username and password, but that resulted in fatal errors as the rules were harden. now that we have a way to report malformed credentials, use it to notify the user when username/password was missing, instead of just silently skipping. do the same for credentials that are missing host (or had one that is empty) or that are missing a path (for supporting cert://) as well. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- credential-store.c | 7 ++++--- t/t0302-credential-store.sh | 38 +++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/credential-store.c b/credential-store.c index 1cc5ca081a..53f77ff6f5 100644 --- a/credential-store.c +++ b/credential-store.c @@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn, while (strbuf_getline_lf(&line, fh) != EOF) { lineno++; - if (!credential_from_url_gently(&entry, line.buf, 1)) { - if (entry.username && entry.password && - credential_match(c, &entry)) { + if (!credential_from_url_gently(&entry, line.buf, 1) && + ((entry.host && *entry.host) || entry.path) && + entry.username && entry.password) { + if (credential_match(c, &entry)) { found_credential = 1; if (match_cb) { match_cb(&entry); diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index 801c1eb200..3150f304cb 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -139,6 +139,44 @@ test_expect_success 'get: credentials without scheme are invalid' ' test_i18ngrep "ignoring invalid credential" stderr ' +test_expect_success 'get: credentials without valid host/path are invalid' ' + echo "https://user:pass@" >"$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 + 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: credentials without username/password are invalid' ' + echo "https://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 + 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: store file can contain empty/bogus lines' ' echo "" > "$HOME/.git-credentials" && q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" && -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using 2020-04-29 20:35 ` [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using Carlo Marcelo Arenas Belón @ 2020-04-29 21:12 ` Junio C Hamano 2020-04-29 21:49 ` [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy Carlo Marcelo Arenas Belón 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2020-04-29 21:12 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff, jrnieder Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > originally any credential found was tried for matching as far as it had > a username and password, but that resulted in fatal errors as the rules > were harden. harden -> hardened > now that we have a way to report malformed credentials, use it to notify > the user when username/password was missing, instead of just silently > skipping. Sorry, but isn't that what happend already in the previous step? What are you ordering the codebase (after applying the previous stpe) do further? It already is "using it to notify the user when username and/or password is missing". > do the same for credentials that are missing host (or had one that is > empty) or that are missing a path (for supporting cert://) as well. While the intention may be a good one ... > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > credential-store.c | 7 ++++--- > t/t0302-credential-store.sh | 38 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 42 insertions(+), 3 deletions(-) > > diff --git a/credential-store.c b/credential-store.c > index 1cc5ca081a..53f77ff6f5 100644 > --- a/credential-store.c > +++ b/credential-store.c > @@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn, > > while (strbuf_getline_lf(&line, fh) != EOF) { > lineno++; > - if (!credential_from_url_gently(&entry, line.buf, 1)) { > - if (entry.username && entry.password && > - credential_match(c, &entry)) { > + if (!credential_from_url_gently(&entry, line.buf, 1) && > + ((entry.host && *entry.host) || entry.path) && > + entry.username && entry.password) { > + if (credential_match(c, &entry)) { ... this makes the code even harder to follow than it already was after the previous step. In the previous step, at least it was clear that we'd catch *all* the well-formed/parseable input will come into the first if () {...} block, but with the extra logic, that is no longer true. Even a line that is well formed may be bypassed from matching and will be fed to "else" side. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy 2020-04-29 21:12 ` Junio C Hamano @ 2020-04-29 21:49 ` Carlo Marcelo Arenas Belón 2020-04-29 22:04 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-29 21:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, dirk, sunshine, peff, jrnieder On Wed, Apr 29, 2020 at 02:12:21PM -0700, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > originally any credential found was tried for matching as far as it had > > a username and password, but that resulted in fatal errors as the rules > > were harden. > > harden -> hardened > > > now that we have a way to report malformed credentials, use it to notify > > the user when username/password was missing, instead of just silently > > skipping. > > Sorry, but isn't that what happend already in the previous step? > What are you ordering the codebase (after applying the previous > stpe) do further? It already is "using it to notify the user when > username and/or password is missing". not sure I follow, but the current code (as well as the one after patch 1) just silently ignores any credential that was missing username and password since the _gently version of the call will only really fail for a missing protocol. so patch1 will notify ONLY(*) when the credential is so malformed that we can't figure out which protocol to use (like empty lines and comments) (*) it will also fail if we have a '\n' in one of the components but that is hopefully not relevant here. > > diff --git a/credential-store.c b/credential-store.c > > index 1cc5ca081a..53f77ff6f5 100644 > > --- a/credential-store.c > > +++ b/credential-store.c > > @@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn, > > > > while (strbuf_getline_lf(&line, fh) != EOF) { > > lineno++; > > - if (!credential_from_url_gently(&entry, line.buf, 1)) { > > - if (entry.username && entry.password && > > - credential_match(c, &entry)) { > > + if (!credential_from_url_gently(&entry, line.buf, 1) && > > + ((entry.host && *entry.host) || entry.path) && > > + entry.username && entry.password) { > > + if (credential_match(c, &entry)) { > > ... this makes the code even harder to follow than it already was > after the previous step. In the previous step, at least it was > clear that we'd catch *all* the well-formed/parseable input will > come into the first if () {...} block, but with the extra logic, > that is no longer true. Even a line that is well formed may be > bypassed from matching and will be fed to "else" side. I think it will be clearer with the reformatting you suggested and that I agree is needed, but wanted to make sure first that we agreed on the direction. FWIW the test cases show the format of the lines affected and while they are parseable enough that were processed they were obviously not valid, specially considering the updated rules. maybe we should make the _gently version less gently instead?, so that callers will be able to know they have an incomplete credential for matching after calling it without having to do their own check? if the later, I am affraid this will be a far bigger change, which is why I would rather duplicate the rule checking for at least the first iteration. Carlo ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy 2020-04-29 21:49 ` [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy Carlo Marcelo Arenas Belón @ 2020-04-29 22:04 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-29 22:04 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff, jrnieder Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: >> Sorry, but isn't that what happend already in the previous step? >> What are you ordering the codebase (after applying the previous >> stpe) do further? It already is "using it to notify the user when >> username and/or password is missing". > > not sure I follow, but the current code (as well as the one after patch 1) > just silently ignores any credential that was missing username and password > since the _gently version of the call will only really fail for a missing > protocol. Ah, I misread the intention of [1/1]. While I was rewriting the if/else structure inside the loop, somehow I thought that we want to warn when from_url_gently() fails or user/pass are missing, and in this step, we are adding to the "warning-worthy" set an entry that does not have either host or path. Sorry about the confusion. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v6] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 20:35 ` [RFC PATCH v6 0/2] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón 2020-04-29 20:35 ` [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 2020-04-29 20:35 ` [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using Carlo Marcelo Arenas Belón @ 2020-04-29 23:23 ` Carlo Marcelo Arenas Belón 2020-04-29 23:47 ` Junio C Hamano 2020-04-30 1:19 ` [PATCH v7] " Carlo Marcelo Arenas Belón 2 siblings, 2 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-29 23:23 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón 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. instead of doing a hard check for credentials, do a soft one and warn the user so any invalid entries could be corrected. make sure that the credential to use is complete before calling credential_match by confirming it has all fields set as expected by the updated rules. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v6: * get rid of redacter and only use line number for context in warning * make validation more strict to also catch incomplete credentials * reorder check as suggested by Junio v5: * q_to_tab this round, with a single echo to make sure empty line is covered, as that seems to be a popular issue * rebase on top of jc/credential-store-file-format-doc * implement a redacter for credentials to use on errors to avoid leaking passwords v4: * use credential_from_url_gently instead as shown by Jonathan * add documentation to clarify "comments" is not a supported feature v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 11 +++- credential-store.c | 26 +++++++-- t/t0302-credential-store.sh | 80 ++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 6 deletions(-) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 76b0798856..122e238eb2 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -95,8 +95,15 @@ 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. +allowed in the file, even though historically the parser was very +lenient and some might had been silently ignored. + +Do not view or edit the file with editors as it could compromise the +validity of your credentials by sometimes subtle formatting issues, +like spaces, line wrapping or text encoding. + +An unparseable or otherwise invalid line is ignored, and a warning +message points out the problematic line number and file it appears in. When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against diff --git a/credential-store.c b/credential-store.c index c010497cb2..4d3c9e8000 100644 --- a/credential-store.c +++ b/credential-store.c @@ -6,6 +6,15 @@ static struct lock_file credential_lock; +/* + * entry->protocol comes validated from credential_from_url_gently + */ +static int valid_credential(struct credential *entry) +{ + return (entry->username && entry->password && + ((entry->host && *entry->host) || entry->path)); +} + static int parse_credential_file(const char *fn, struct credential *c, void (*match_cb)(struct credential *), @@ -15,6 +24,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,16 +34,24 @@ 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 && - credential_match(c, &entry)) { + lineno++; + + if (credential_from_url_gently(&entry, line.buf, 1) || + !valid_credential(&entry)) { + warning(_("%s:%d: ignoring invalid credential"), + fn, lineno); + } + else if (credential_match(c, &entry)) + { found_credential = 1; if (match_cb) { match_cb(&entry); break; } + continue; } - else if (other_cb) + + if (other_cb) other_cb(&line); } diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..3150f304cb 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,84 @@ 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: 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 + 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: credentials without valid host/path are invalid' ' + echo "https://user:pass@" >"$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 + 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: credentials without username/password are invalid' ' + echo "https://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 + 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: store file can contain empty/bogus lines' ' + echo "" > "$HOME/.git-credentials" && + q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" && + #comment + Q + https://user:pass@example.com + CONFIG + 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 + EOF + test_cmp expect-stdout stdout && + test_i18ngrep "ignoring invalid credential" stderr && + test_line_count = 3 stderr +' + test_done base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0 -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 23:23 ` [PATCH v6] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón @ 2020-04-29 23:47 ` Junio C Hamano 2020-04-29 23:57 ` Junio C Hamano 2020-04-30 1:00 ` Carlo Marcelo Arenas Belón 2020-04-30 1:19 ` [PATCH v7] " Carlo Marcelo Arenas Belón 1 sibling, 2 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-29 23:47 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff, jrnieder Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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. with -> With > instead of doing a hard check for credentials, do a soft one and > warn the user so any invalid entries could be corrected. instead -> instead > make sure that the credential to use is complete before calling > credential_match by confirming it has all fields set as expected > by the updated rules. make -> Make > diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt > index 76b0798856..122e238eb2 100644 > --- a/Documentation/git-credential-store.txt > +++ b/Documentation/git-credential-store.txt > @@ -95,8 +95,15 @@ 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. > +allowed in the file, even though historically the parser was very > +lenient and some might had been silently ignored. > + > +Do not view or edit the file with editors as it could compromise the > +validity of your credentials by sometimes subtle formatting issues, > +like spaces, line wrapping or text encoding. I do not think dropping "view or" is justifiable. There is no need to invite the risk of opening with the intention to only view and then end up saving a modification. In other words, do not encourage use of an *editor* in any way. > + > +An unparseable or otherwise invalid line is ignored, and a warning > +message points out the problematic line number and file it appears in. OK. You didn't want to tell them they can remove the problematic line as a whole with their editor? > diff --git a/credential-store.c b/credential-store.c > index c010497cb2..4d3c9e8000 100644 > --- a/credential-store.c > +++ b/credential-store.c > @@ -6,6 +6,15 @@ > > static struct lock_file credential_lock; > > +/* > + * entry->protocol comes validated from credential_from_url_gently > + */ Sure, but wouldn't it be simpler to do without such a comment and check it also here, just as a belt-and-suspender safety? > +static int valid_credential(struct credential *entry) > +{ > + return (entry->username && entry->password && > + ((entry->host && *entry->host) || entry->path)); > +} > @@ -15,6 +24,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,16 +34,24 @@ static int parse_credential_file(const char *fn, > } > > while (strbuf_getline_lf(&line, fh) != EOF) { > + lineno++; > + > + if (credential_from_url_gently(&entry, line.buf, 1) || > + !valid_credential(&entry)) { > + warning(_("%s:%d: ignoring invalid credential"), > + fn, lineno); > + } > + else if (credential_match(c, &entry)) > + { Style: "} else if (...) {" on a single line. > found_credential = 1; > if (match_cb) { > match_cb(&entry); > break; > } > + continue; > } > + > + if (other_cb) > other_cb(&line); > } This got vastly easier to read, thanks to the additional helper. Looking really good. > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > index d6b54e8c65..3150f304cb 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -120,4 +120,84 @@ 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: 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 > + EOF > + test_cmp expect-stdout stdout && > + grep "askpass: Username for '\''https://example.com'\'':" stderr && > + grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr && These messages are passed from credential.c::credential_ask_one() to git_prompt() without any i18n, so use of a bare "grep" is good. > + test_i18ngrep "ignoring invalid credential" stderr And this is new message inside _(), so i18ngrep is good. Nice to see such an attention to the details. > + test_config credential.helper store && > + git credential fill <<-\EOF >stdout 2>stderr && > + protocol=https > + host=example.com > + EOF > + test_cmp expect-stdout stdout && > + test_i18ngrep "ignoring invalid credential" stderr && > + test_line_count = 3 stderr The "ignoring invalid credential" message could be translated into two or more lines, but I think that is worrying too much about theoretical possibility, so checking line count would probably be sufficient. Thanks. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 23:47 ` Junio C Hamano @ 2020-04-29 23:57 ` Junio C Hamano 2020-04-30 1:00 ` Carlo Marcelo Arenas Belón 1 sibling, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-29 23:57 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, dirk, sunshine, peff, jrnieder Junio C Hamano <gitster@pobox.com> writes: >> +Do not view or edit the file with editors as it could compromise the >> +validity of your credentials by sometimes subtle formatting issues, >> +like spaces, line wrapping or text encoding. > > I do not think dropping "view or" is justifiable.... Sorry, this comment is now stale. >> +An unparseable or otherwise invalid line is ignored, and a warning >> +message points out the problematic line number and file it appears in. > > OK. You didn't want to tell them they can remove the problematic > line as a whole with their editor? This one I do not care too deeply either way. Probably it is obvious to the readers that they can remove the lines (otherwise there is no good reason to give warnings in the first place). ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 23:47 ` Junio C Hamano 2020-04-29 23:57 ` Junio C Hamano @ 2020-04-30 1:00 ` Carlo Marcelo Arenas Belón 1 sibling, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-30 1:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, dirk, sunshine, peff, jrnieder On Wed, Apr 29, 2020 at 04:47:31PM -0700, Junio C Hamano wrote: > > instead of doing a hard check for credentials, do a soft one and > > warn the user so any invalid entries could be corrected. > > instead -> instead ;) > I do not think dropping "view or" is justifiable. There is no need > to invite the risk of opening with the intention to only view and > then end up saving a modification. In other words, do not encourage > use of an *editor* in any way. > > > +An unparseable or otherwise invalid line is ignored, and a warning > > +message points out the problematic line number and file it appears in. > > OK. You didn't want to tell them they can remove the problematic > line as a whole with their editor? someone said "do not encourage use of an *editor* in any way" just a few lines before ;) > > + test_config credential.helper store && > > + git credential fill <<-\EOF >stdout 2>stderr && > > + protocol=https > > + host=example.com > > + EOF > > + test_cmp expect-stdout stdout && > > + test_i18ngrep "ignoring invalid credential" stderr && > > + test_line_count = 3 stderr > > The "ignoring invalid credential" message could be translated into > two or more lines, but I think that is worrying too much about > theoretical possibility, so checking line count would probably be > sufficient. yes, and specially considering that if I even ended up adding a -c option to test_i18ngrep as I intended originally we will still the same issue. what is the preferred way to do something like this, or is it better to add a check for each line formatting issue this is handling? Carlo ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v7] credential-store: warn instead of fatal for bogus lines from store 2020-04-29 23:23 ` [PATCH v6] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 2020-04-29 23:47 ` Junio C Hamano @ 2020-04-30 1:19 ` Carlo Marcelo Arenas Belón 2020-04-30 9:29 ` [PATCH v8] " Carlo Marcelo Arenas Belón 2020-04-30 16:06 ` [PATCH v9] " Carlo Marcelo Arenas Belón 1 sibling, 2 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-30 1:19 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón 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. Instead of doing a hard check for credentials, do a soft one and warn the user so any invalid entries could be corrected. Make sure that the credential to use is complete before calling credential_match by confirming it has all fields set as expected by the updated rules. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Junio C Hamano <gitster@pobox.com> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v7: * check for protocol in helper as suggested by Junio v6: * get rid of redacter and only use line number for context in warning * make validation more strict to also catch incomplete credentials * reorder check as suggested by Junio v5: * q_to_tab this round, with a single echo to make sure empty line is covered, as that seems to be a popular issue * rebase on top of jc/credential-store-file-format-doc * implement a redacter for credentials to use on errors to avoid leaking passwords v4: * use credential_from_url_gently instead as shown by Jonathan * add documentation to clarify "comments" is not a supported feature v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 11 +++- credential-store.c | 22 +++++-- t/t0302-credential-store.sh | 80 ++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 6 deletions(-) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 76b0798856..122e238eb2 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -95,8 +95,15 @@ 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. +allowed in the file, even though historically the parser was very +lenient and some might had been silently ignored. + +Do not view or edit the file with editors as it could compromise the +validity of your credentials by sometimes subtle formatting issues, +like spaces, line wrapping or text encoding. + +An unparseable or otherwise invalid line is ignored, and a warning +message points out the problematic line number and file it appears in. When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against diff --git a/credential-store.c b/credential-store.c index c010497cb2..2c5f9736fc 100644 --- a/credential-store.c +++ b/credential-store.c @@ -6,6 +6,13 @@ static struct lock_file credential_lock; +static int valid_credential(struct credential *entry) +{ + return (entry->username && entry->password && + entry->protocol && *entry->protocol && + ((entry->host && *entry->host) || entry->path)); +} + static int parse_credential_file(const char *fn, struct credential *c, void (*match_cb)(struct credential *), @@ -15,6 +22,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,16 +32,22 @@ 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 && - credential_match(c, &entry)) { + lineno++; + + if (credential_from_url_gently(&entry, line.buf, 1) || + !valid_credential(&entry)) { + warning(_("%s:%d: ignoring invalid credential"), + fn, lineno); + } else if (credential_match(c, &entry)) { found_credential = 1; if (match_cb) { match_cb(&entry); break; } + continue; } - else if (other_cb) + + if (other_cb) other_cb(&line); } diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..3150f304cb 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,84 @@ 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: 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 + 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: credentials without valid host/path are invalid' ' + echo "https://user:pass@" >"$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 + 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: credentials without username/password are invalid' ' + echo "https://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 + 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: store file can contain empty/bogus lines' ' + echo "" > "$HOME/.git-credentials" && + q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" && + #comment + Q + https://user:pass@example.com + CONFIG + 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 + EOF + test_cmp expect-stdout stdout && + test_i18ngrep "ignoring invalid credential" stderr && + test_line_count = 3 stderr +' + test_done base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0 -- 2.26.2.569.g1d74ac4d14 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v8] credential-store: warn instead of fatal for bogus lines from store 2020-04-30 1:19 ` [PATCH v7] " Carlo Marcelo Arenas Belón @ 2020-04-30 9:29 ` Carlo Marcelo Arenas Belón 2020-04-30 16:06 ` [PATCH v9] " Carlo Marcelo Arenas Belón 1 sibling, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-30 9:29 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Carlo Marcelo Arenas Belón 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. Instead of doing a hard check for credentials, do a soft one and therefore avoid the reported fatal error. Warn the user indicating the filename and line number so any invalid entries could be corrected but continue parsing until a match is found or all valid credentials are processed. Make sure that the credential that we will use to match is complete by confirming it has all fields set as expected by the updated rules. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v8: * only warn during get operations as otherwise line number might be incorrect v7: * check for protocol in helper as suggested by Junio v6: * get rid of redacter and only use line number for context in warning * make validation more strict to also catch incomplete credentials * reorder check as suggested by Junio v5: * q_to_tab this round, with a single echo to make sure empty line is covered, as that seems to be a popular issue * rebase on top of jc/credential-store-file-format-doc * implement a redacter for credentials to use on errors to avoid leaking passwords v4: * use credential_from_url_gently instead as shown by Jonathan * add documentation to clarify "comments" is not a supported feature v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 11 +++- credential-store.c | 31 +++++++-- t/t0302-credential-store.sh | 89 ++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 8 deletions(-) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 76b0798856..d5841cffad 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -95,8 +95,15 @@ 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. +allowed in the file, even though historically the parser was very +lenient and some might had been silently ignored. + +Do not view or edit the file with editors as it could compromise the +validity of your credentials by sometimes subtle formatting issues, +like spaces, line wrapping or text encoding. + +An unparseable or otherwise invalid line is ignored, and a warning +message points out the problematic file and line number it appears in. When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against diff --git a/credential-store.c b/credential-store.c index c010497cb2..9bd8ec9b67 100644 --- a/credential-store.c +++ b/credential-store.c @@ -4,10 +4,20 @@ #include "string-list.h" #include "parse-options.h" +#define PARSE_VERBOSE 0x01 + static struct lock_file credential_lock; +static int valid_credential(struct credential *entry) +{ + return (entry->username && entry->password && + entry->protocol && *entry->protocol && + ((entry->host && *entry->host) || entry->path)); +} + static int parse_credential_file(const char *fn, struct credential *c, + int flags, void (*match_cb)(struct credential *), void (*other_cb)(struct strbuf *)) { @@ -15,6 +25,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,16 +35,23 @@ 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 && - credential_match(c, &entry)) { + lineno++; + + if (credential_from_url_gently(&entry, line.buf, 1) || + !valid_credential(&entry)) { + if (flags & PARSE_VERBOSE) + warning(_("%s:%d: ignoring invalid credential"), + fn, lineno); + } else if (credential_match(c, &entry)) { found_credential = 1; if (match_cb) { match_cb(&entry); break; } + continue; } - else if (other_cb) + + if (other_cb) other_cb(&line); } @@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c, die_errno("unable to get credential storage lock"); if (extra) print_line(extra); - parse_credential_file(fn, c, NULL, print_line); + parse_credential_file(fn, c, 0, NULL, print_line); if (commit_lock_file(&credential_lock) < 0) die_errno("unable to write credential store"); } @@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential * struct string_list_item *fn; for_each_string_list_item(fn, fns) - if (parse_credential_file(fn->string, c, print_entry, NULL)) + if (parse_credential_file(fn->string, c, PARSE_VERBOSE, + print_entry, NULL)) return; /* Found credential */ } diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..8bc89b7efb 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,93 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi test_must_be_empty "$HOME/.config/git/credentials" ' +test_expect_success 'store: store succeeds silently in corrupted file' ' + echo "#comment" >"$HOME/.git-credentials" && + check approve store <<-\EOF && + url=https://user:pass@example.com + EOF + grep "https://user:pass@example.com" "$HOME/.git-credentials" && + test_must_be_empty stderr +' + +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 + 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: credentials without valid host/path are invalid' ' + echo "https://user:pass@" >"$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 + 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: credentials without username/password are invalid' ' + echo "https://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 + 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: store file can contain empty/bogus lines' ' + echo "" > "$HOME/.git-credentials" && + q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" && + #comment + Q + https://user:pass@example.com + CONFIG + 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 + EOF + test_cmp expect-stdout stdout && + test_i18ngrep "ignoring invalid credential" stderr && + test_line_count = 3 stderr +' + test_done base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0 -- 2.26.2.2.g9c647e0d3b.dirty ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v9] credential-store: warn instead of fatal for bogus lines from store 2020-04-30 1:19 ` [PATCH v7] " Carlo Marcelo Arenas Belón 2020-04-30 9:29 ` [PATCH v8] " Carlo Marcelo Arenas Belón @ 2020-04-30 16:06 ` Carlo Marcelo Arenas Belón 2020-04-30 20:21 ` Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-04-30 16:06 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Johannes.Schindelin, Carlo Marcelo Arenas Belón 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. Instead of doing a hard check for credentials, do a soft one and therefore avoid the reported fatal error. Warn the user indicating the filename and line number so any invalid entries could be corrected but continue parsing until a match is found or all valid credentials are processed. Make sure that the credential that we will use to match is complete by confirming it has all fields set as expected by the updated rules. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v9: * use strbuf_getline() instead to better handle files with CRLF v8: * only warn during get operations as otherwise line number might be incorrect v7: * check for protocol in helper as suggested by Junio v6: * get rid of redacter and only use line number for context in warning * make validation more strict to also catch incomplete credentials * reorder check as suggested by Junio v5: * q_to_tab this round, with a single echo to make sure empty line is covered, as that seems to be a popular issue * rebase on top of jc/credential-store-file-format-doc * implement a redacter for credentials to use on errors to avoid leaking passwords v4: * use credential_from_url_gently instead as shown by Jonathan * add documentation to clarify "comments" is not a supported feature v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 11 +++- credential-store.c | 33 ++++++++-- t/t0302-credential-store.sh | 90 +++++++++++++++++++++++++- 3 files changed, 124 insertions(+), 10 deletions(-) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 76b0798856..d5841cffad 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -95,8 +95,15 @@ 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. +allowed in the file, even though historically the parser was very +lenient and some might had been silently ignored. + +Do not view or edit the file with editors as it could compromise the +validity of your credentials by sometimes subtle formatting issues, +like spaces, line wrapping or text encoding. + +An unparseable or otherwise invalid line is ignored, and a warning +message points out the problematic file and line number it appears in. When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against diff --git a/credential-store.c b/credential-store.c index c010497cb2..b1f5b2dea6 100644 --- a/credential-store.c +++ b/credential-store.c @@ -4,10 +4,20 @@ #include "string-list.h" #include "parse-options.h" +#define PARSE_VERBOSE 0x01 + static struct lock_file credential_lock; +static int valid_credential(struct credential *entry) +{ + return (entry->username && entry->password && + entry->protocol && *entry->protocol && + ((entry->host && *entry->host) || entry->path)); +} + static int parse_credential_file(const char *fn, struct credential *c, + int flags, void (*match_cb)(struct credential *), void (*other_cb)(struct strbuf *)) { @@ -15,6 +25,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) { @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn, return found_credential; } - while (strbuf_getline_lf(&line, fh) != EOF) { - credential_from_url(&entry, line.buf); - if (entry.username && entry.password && - credential_match(c, &entry)) { + while (strbuf_getline(&line, fh) != EOF) { + lineno++; + + if (credential_from_url_gently(&entry, line.buf, 1) || + !valid_credential(&entry)) { + if (flags & PARSE_VERBOSE) + warning(_("%s:%d: ignoring invalid credential"), + fn, lineno); + } else if (credential_match(c, &entry)) { found_credential = 1; if (match_cb) { match_cb(&entry); break; } + continue; } - else if (other_cb) + + if (other_cb) other_cb(&line); } @@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c, die_errno("unable to get credential storage lock"); if (extra) print_line(extra); - parse_credential_file(fn, c, NULL, print_line); + parse_credential_file(fn, c, 0, NULL, print_line); if (commit_lock_file(&credential_lock) < 0) die_errno("unable to write credential store"); } @@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential * struct string_list_item *fn; for_each_string_list_item(fn, fns) - if (parse_credential_file(fn->string, c, print_entry, NULL)) + if (parse_credential_file(fn->string, c, PARSE_VERBOSE, + print_entry, NULL)) return; /* Found credential */ } diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..05fd2785b9 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home test_must_be_empty "$HOME/.config/git/credentials" ' - test_expect_success 'erase: erase matching credentials from both xdg and home files' ' echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" && mkdir -p "$HOME/.config/git" && @@ -120,4 +119,93 @@ 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: 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 + 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: credentials without valid host/path are invalid' ' + echo "https://user:pass@" >"$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 + 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: credentials without username/password are invalid' ' + echo "https://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 + 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: store file can contain empty/bogus lines' ' + echo "" > "$HOME/.git-credentials" && + q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" && + #comment + Q + https://user:pass@example.com + CONFIG + 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 + EOF + test_cmp expect-stdout stdout && + test_i18ngrep "ignoring invalid credential" stderr && + test_line_count = 3 stderr +' + +test_expect_success 'store: store succeeds silently in corrupted file' ' + echo "#comment" >"$HOME/.git-credentials" && + check approve store <<-\EOF && + url=https://user:pass@example.com + EOF + grep "https://user:pass@example.com" "$HOME/.git-credentials" && + test_must_be_empty stderr +' + test_done base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0 -- 2.26.2.2.gf219a08795 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store 2020-04-30 16:06 ` [PATCH v9] " Carlo Marcelo Arenas Belón @ 2020-04-30 20:21 ` Junio C Hamano 2020-04-30 21:14 ` Junio C Hamano 2020-05-01 0:30 ` Carlo Marcelo Arenas Belón 2020-05-01 3:21 ` [RFC PATCH v10] credential-store: warn/ignore for bogus lines from store file Carlo Marcelo Arenas Belón 2020-05-02 18:16 ` [PATCH v10] credential-store: ignore bogus lines from store file Carlo Marcelo Arenas Belón 2 siblings, 2 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-30 20:21 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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. > > Instead of doing a hard check for credentials, do a soft one and > therefore avoid the reported fatal error. > > Warn the user indicating the filename and line number so any invalid > entries could be corrected but continue parsing until a match is > found or all valid credentials are processed. > > Make sure that the credential that we will use to match is complete by > confirming it has all fields set as expected by the updated rules. > > [1] https://stackoverflow.com/a/61420852/5005936 > > Reported-by: Dirk <dirk@ed4u.de> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- Looks good. > diff --git a/credential-store.c b/credential-store.c > index c010497cb2..b1f5b2dea6 100644 > --- a/credential-store.c > +++ b/credential-store.c > @@ -4,10 +4,20 @@ > #include "string-list.h" > #include "parse-options.h" > > +#define PARSE_VERBOSE 0x01 > + > static struct lock_file credential_lock; > > +static int valid_credential(struct credential *entry) > +{ > + return (entry->username && entry->password && > + entry->protocol && *entry->protocol && > + ((entry->host && *entry->host) || entry->path)); > +} OK. > static int parse_credential_file(const char *fn, > struct credential *c, > + int flags, > void (*match_cb)(struct credential *), > void (*other_cb)(struct strbuf *)) > { > @@ -15,6 +25,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) { > @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn, > return found_credential; > } > > - while (strbuf_getline_lf(&line, fh) != EOF) { > - credential_from_url(&entry, line.buf); > - if (entry.username && entry.password && > - credential_match(c, &entry)) { > + while (strbuf_getline(&line, fh) != EOF) { Hmph, I probably have missed some discussion that happened in the last few days, but from the use of _lf() in the original, I sense that it is very much deliberate that the file format is designed to be LF delimited records, *not* a text file in which each line is terminated with some end-of-line marker that is platform dependent. IOW, an explicit use of _lf() shouts, at least to me, that we want a sequence of LF terminated records even on Windows and Macs. So, I am not sure why this change is desirable. > + lineno++; > + > + if (credential_from_url_gently(&entry, line.buf, 1) || > + !valid_credential(&entry)) { OK, so we read a line, fed it to the parser, and if we had trouble parsing or the line did not have enough credential material, we discard and warn (when told to). > + if (flags & PARSE_VERBOSE) > + warning(_("%s:%d: ignoring invalid credential"), > + fn, lineno); > + } else if (credential_match(c, &entry)) { > found_credential = 1; > if (match_cb) { > match_cb(&entry); > break; > } > + continue; And if the credential material on a good line matches, we remember we saw a match. If there is match callback, we call it and leave the loop to return from the function. Otherwise we go to the next line. > } > + > + if (other_cb) > other_cb(&line); A malformed/underspecified line and an unmatched line is fed to the other callback. > } > > @@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c, > die_errno("unable to get credential storage lock"); > if (extra) > print_line(extra); > - parse_credential_file(fn, c, NULL, print_line); > + parse_credential_file(fn, c, 0, NULL, print_line); This helper function is called from two places. - In store_credential_file, we write the credential we were asked to store to a strbuf, and then call this function. The function first writes that new credential and then calls the parse helper without PARSE_VERBOSE. We do not give any match callback, and other callback is to just print the line read from the credential file. So, the final output would be the new credential, followed by the current contents of the credential file except for the records that match the one we are storing. We do this without warning about malformed entries at all. - In remove_credential, we go over multiple files and copy the lines in each of them, except the ones that match the credential we are given. We also do this without warning about malformed entries at all. > if (commit_lock_file(&credential_lock) < 0) > die_errno("unable to write credential store"); > } > @@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential * > struct string_list_item *fn; > > for_each_string_list_item(fn, fns) > - if (parse_credential_file(fn->string, c, print_entry, NULL)) > + if (parse_credential_file(fn->string, c, PARSE_VERBOSE, > + print_entry, NULL)) > return; /* Found credential */ > } This is triggered by the "get" operation. We read until we hit the entry we are looking for, at that point the match-callback will print out "username=... / password=..." lines and we ignore the remainder of the file. While we look for the first matching credential, we do warn about malformed or underspecified lines that we are ignoring, but because we leave the loop once a matching line is found, bad lines that come after it won't be even looked at to be warned. Validating and warning about bad entries is *not* the main purpose of the "credential-store" program, so I fully agree with the design of the "get" part. I am not so sure about the other two operations (i.e. "store" and "erase") that do scan all the entries and has chance to warn about bad ones, though (note: I am not saying that we should parse verbosely---it is just that I do not know why you chose not to and I am not convinced that it is a good idea not to warn). The tests looked good. Thanks. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store 2020-04-30 20:21 ` Junio C Hamano @ 2020-04-30 21:14 ` Junio C Hamano 2020-05-01 0:30 ` Carlo Marcelo Arenas Belón 1 sibling, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-04-30 21:14 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin Junio C Hamano <gitster@pobox.com> writes: [part of the commit log message] >> Warn the user indicating the filename and line number so any invalid >> entries could be corrected but continue parsing until a match is >> found or all valid credentials are processed. We should say "Do so only during the 'get' operation; give up giving any warning for 'erase' or 'store' operation, as keeping track of the line number of the input file, while having to issue a warning that has the line number of the output file, is too hard for us" or something like that here. I suspect that it might not be "too hard", but we'd need to rename other_cb to a more specific name first and limit the possibility of repurposing parse_credential_file(). For example, other_cb can become "copy_cb", we declare that the function works in two (and no other) modes, one is to look-up (which is read-only so we report the input line number in our warning messges) and the other is to copy-out-with-filtering (which will leave a file that is different from the input, so we report the output line number). To support the copy-out-wiht-filtering mode, we pass the starting line number into the function (i.e. 'store' may store a new line, so the first line copied out to the file may be the second line in the output), and count the output lines there: if (other_cb) { lines++; other_cb(&line); } and of course we won't increment lines++ when we saw a match in the copy-out-with-filtering mode. In look-up mode (i.e. copy_cb == NULL), we count the input lines just like your code. But I suspect that it may not be worth doing. But if we decide not to do so, we should document why we chose (not) to in the commit log message. > Validating and warning about bad entries is *not* the main purpose > of the "credential-store" program, so I fully agree with the design > of the "get" part. I am not so sure about the other two operations > (i.e. "store" and "erase") that do scan all the entries and has > chance to warn about bad ones, though (note: I am not saying that we > should parse verbosely---it is just that I do not know why you chose > not to and I am not convinced that it is a good idea not to warn). I re-read the incremental log for v8 and found your explanation. Thanks. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store 2020-04-30 20:21 ` Junio C Hamano 2020-04-30 21:14 ` Junio C Hamano @ 2020-05-01 0:30 ` Carlo Marcelo Arenas Belón 2020-05-01 1:40 ` Junio C Hamano 1 sibling, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-05-01 0:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin On Thu, Apr 30, 2020 at 01:21:06PM -0700, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn, > > return found_credential; > > } > > > > - while (strbuf_getline_lf(&line, fh) != EOF) { > > - credential_from_url(&entry, line.buf); > > - if (entry.username && entry.password && > > - credential_match(c, &entry)) { > > + while (strbuf_getline(&line, fh) != EOF) { > > Hmph, I probably have missed some discussion that happened in the > last few days, but from the use of _lf() in the original, I sense > that it is very much deliberate that the file format is designed to > be LF delimited records, *not* a text file in which each line is > terminated with some end-of-line marker that is platform dependent. > IOW, an explicit use of _lf() shouts, at least to me, that we want a > sequence of LF terminated records even on Windows and Macs. apologize for the confusion, the only discussion was the one I had with myself late at night when I realized that specific corruption was not being detected, and might be related to issues that seemed to be common[1] the problem is that practically speaking, if users in Windows and Macs edited the file they "might" had saved lines with the wrong line ending (*) (part of the reason I added a warning about "encoding" to the documentation), and those are difficult to "see" as invalid. using the non _lf() version ensures any trailing '\r' character would get removed from the credential and allow us to use them, instead of silently failing. originally I added a explicit check for it, which I assume you would prefer but .. > So, I am not sure why this change is desirable. to give users immediate relief from what seems to be a commonly seen issue. IMHO after we get this released out and they see the updated documentation explaining the risks, and some learn how to fix their credential files ( and hopefully use an editor that lets them see the problem); we could then add also a detection for this edge case and go back to _lf() I also had to admit I might had overreacted, as I was testing before v8 with a very corrupted file and was seeing warnings twice on each operation and somehow even got myself into the original fatal, which I had to admit I can't replicate now after some needed rest. Carlo [1] https://github.com/desktop/desktop/issues/9597 (*) textedit, which comes with macOS doesn't even write an EOF record when creating files, for example, and that behaviour seems to be common for most other native editors but a credential line WITHOUT line ending works ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store 2020-05-01 0:30 ` Carlo Marcelo Arenas Belón @ 2020-05-01 1:40 ` Junio C Hamano 2020-05-01 2:24 ` Carlo Arenas 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2020-05-01 1:40 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > the problem is that practically speaking, if users in Windows and Macs > edited the file they "might" had saved lines with the wrong line ending (*) > (part of the reason I added a warning about "encoding" to the documentation), > and those are difficult to "see" as invalid. > > using the non _lf() version ensures any trailing '\r' character would > get removed from the credential and allow us to use them, instead of > silently failing. You are forgetting why we are fixing credential-store, aren't you? It is primarily to help those who damaged their files by editing, and introducing cruft that cannot be parsed, from a stricter parsing introduced recently in 2.17.4 and above. Without the fix, they cannot operate with the store they already have. Now, think about those users who saved their file, after adding CR at the end of each line, but didn't do any other edit (like adding a blank line or "# comment"). It may have happened 3 years ago. What did they see from the system back then? It may have happened 3 minutes ago. What would they see with the stricter parser now? With or without the recent parser change, they would have seen that these lines no longer match the URLs they wanted to match, but the credential store does not die on them for malformed lines, no? In other words, the stricter parsing did nothing to these users. In fact, those users who added CR at the end of each line 3 years ago may have even depended on the disappearance of these entries for all these years. Maybe lines that record their ancient passwords for sites are still buried in the later parts of the file, with CR, but doing no harm because these lines do not match anything. These users may have changed their password since then and wrote new records with "credential store", and these new records are stored without CR at the end of the line, so they match the URLs. By using the non _lf() variant, you are suddenly resurrecting these old records that the users thought are already gone and have been causing no harm to them. Do we know that resurrecting these old records is a good thing to do? I don't. For example, once the user decides to "sort" the file (after all, we are talking about users who edit the file, so we cannot assume they won't do so), they would end up with duplicate records that record two passwords to the same site and they cannot tell which one is current, as you even lost the CR at the end of line that would have told you which ones are broken. In short, you wouldn't know what ramification it has by suddenly using non _lf() variant. And it has nothing to do with the fix we are trying to make. When fixing something, it is tempting to see if you can cover a bit more area with the same change. But you should make it a habit to stick to the absolute minimum first for safety. When contemplating to step out of the absolute minimum, you need to think twice to see if that extra "fix" really fixes, or if it potentially harms. And I am not convinced that turning CRLF into LF in this case is a good change. In any case, it certainly does not belong to the same commit as the one that fixes the fallout from stricter parser introduced in 2.17.4 and above. Thanks. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store 2020-05-01 1:40 ` Junio C Hamano @ 2020-05-01 2:24 ` Carlo Arenas 2020-05-01 5:27 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Carlo Arenas @ 2020-05-01 2:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin On Thu, Apr 30, 2020 at 6:40 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > the problem is that practically speaking, if users in Windows and Macs > > edited the file they "might" had saved lines with the wrong line ending (*) > > (part of the reason I added a warning about "encoding" to the documentation), > > and those are difficult to "see" as invalid. > > > > using the non _lf() version ensures any trailing '\r' character would > > get removed from the credential and allow us to use them, instead of > > silently failing. > > You are forgetting why we are fixing credential-store, aren't you? indeed, and thanks for the clarification. you are correct this was a bad idea and is better to warn them instead (even if only during get operations and for the lines that were processed) than my "solution". FWIW though, my change wasn't going to change the file on disk but only allow the line to be processed. gave up already on sneakily "fixing" corruption issues after Peff called me on it and yes, another version you will never see had a PARSER_TRIM flag added ;) Carlo PS. should we really do the warn even in store/erase operations as a followup or is the warning not useful as is, and probably then all operations should be quiet (as Jonathan suggested originally?) and we could do warn (and maybe fix) in a different change (maybe adding a fsck command of sorts)? > When fixing something, it is tempting to see if you can cover a bit > more area with the same change. But you should make it a habit to > stick to the absolute minimum first for safety. When contemplating > to step out of the absolute minimum, you need to think twice to see > if that extra "fix" really fixes, or if it potentially harms. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store 2020-05-01 2:24 ` Carlo Arenas @ 2020-05-01 5:27 ` Junio C Hamano 2020-05-01 13:57 ` Carlo Marcelo Arenas Belón 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2020-05-01 5:27 UTC (permalink / raw) To: Carlo Arenas; +Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin Carlo Arenas <carenas@gmail.com> writes: > PS. should we really do the warn even in store/erase operations as a > followup or is the warning not useful as is, and probably then all > operations should be quiet (as Jonathan suggested originally?) and we > could do warn (and maybe fix) in a different change (maybe adding a > fsck command of sorts)? Yeah, I think I like the "no warning, just ignore" much better. The implementation I suspect would become a lot simpler, right? Thanks for thinking one extra step. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store 2020-05-01 5:27 ` Junio C Hamano @ 2020-05-01 13:57 ` Carlo Marcelo Arenas Belón 2020-05-01 18:59 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-05-01 13:57 UTC (permalink / raw) To: Junio C Hamano, jrnieder; +Cc: git On Thu, Apr 30, 2020 at 10:27:24PM -0700, Junio C Hamano wrote: > Carlo Arenas <carenas@gmail.com> writes: > > > PS. should we really do the warn even in store/erase operations as a > > followup or is the warning not useful as is, and probably then all > > operations should be quiet (as Jonathan suggested originally?) and we > > could do warn (and maybe fix) in a different change (maybe adding a > > fsck command of sorts)? > > Yeah, I think I like the "no warning, just ignore" much better. The > implementation I suspect would become a lot simpler, right? sure, it will be basically the version that Jonathan suggested[1] on top of yours and that I included in v4 but that was missing a SOB. Jonathan, would you want to submit it formally?, IMHO better if with base (from pu): 272281efcc (credential-store: document the file format a bit more, 2020-04-27) it might be still worth including some of the test cases, so I could do it instead too; but let me know what would be your preference otherwise, as they will need to be wrapped, probably better than on what became my confusing attempt to save everyone's time with the v4 series[2], and that was made obsolete by Junio's creation of jc/credential-store-file-format-doc. Carlo [1] https://lore.kernel.org/git/20200428052510.GA201501@google.com/ [2] https://lore.kernel.org/git/20200428104858.28573-1-carenas@gmail.com/ CC: trim to avoid spam, probably long overdue ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store 2020-05-01 13:57 ` Carlo Marcelo Arenas Belón @ 2020-05-01 18:59 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-05-01 18:59 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: jrnieder, git Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > attempt to save everyone's time with the v4 series[2], and that was made > obsolete by Junio's creation of jc/credential-store-file-format-doc. I actually discarded that "file format only" patch after you included in your series. One fewer patch I have to keep track of is always better ;-) Thanks. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [RFC PATCH v10] credential-store: warn/ignore for bogus lines from store file 2020-04-30 16:06 ` [PATCH v9] " Carlo Marcelo Arenas Belón 2020-04-30 20:21 ` Junio C Hamano @ 2020-05-01 3:21 ` Carlo Marcelo Arenas Belón 2020-05-01 5:18 ` [RFC PATCH v10 2/1] credential-store: warn also for store and erase Carlo Marcelo Arenas Belón 2020-05-02 18:16 ` [PATCH v10] credential-store: ignore bogus lines from store file Carlo Marcelo Arenas Belón 2 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-05-01 3:21 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Johannes.Schindelin, Carlo Marcelo Arenas Belón 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. Instead of doing a hard check for credentials, do a soft one and therefore avoid the reported fatal error. Warn the user indicating the filename and line number during get, for all entries that were processed, but continue parsing until a match is found or all valid credentials are processed. Warning during store and erase operations has been suppressed for now by adding a PARSER_VERBOSE flag that is only enabled during get operations. Make sure that the credential that we will use to match is complete by confirming it has all fields set as expected by the updated rules. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v10: * insted of trying to use broken credentials in DOS format, make sure they are flagged as broken v9: * use strbuf_getline() instead to better handle files with CRLF v8: * only warn during get operations as otherwise line number might be incorrect v7: * check for protocol in helper as suggested by Junio v6: * get rid of redacter and only use line number for context in warning * make validation more strict to also catch incomplete credentials * reorder check as suggested by Junio v5: * q_to_tab this round, with a single echo to make sure empty line is covered, as that seems to be a popular issue * rebase on top of jc/credential-store-file-format-doc * implement a redacter for credentials to use on errors to avoid leaking passwords v4: * use credential_from_url_gently instead as shown by Jonathan * add documentation to clarify "comments" is not a supported feature v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 18 +++- credential-store.c | 32 ++++++-- t/t0302-credential-store.sh | 109 ++++++++++++++++++++++++- 3 files changed, 150 insertions(+), 9 deletions(-) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 76b0798856..18ba8b6984 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -95,8 +95,22 @@ 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. +allowed in the file, even though historically the parser was very +lenient and some might had been silently ignored. + +Do not view or edit the file with editors as it could compromise the +validity of your credentials by sometimes subtle formatting issues, +like spaces, line wrapping or text encoding. + +An unparseable or otherwise invalid line is ignored, and a warning +message points out the problematic file and line number it appears in. + +Note that because of deficiencies of the current implementation, these +warnings will be only generated for the entries that were processed +while reading the credential files during a get operation, and so they +are not comprehensive, and will require you to use an editor (or +better a sed/awk/perl one liner) to edit the credential file and remove +them. When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against diff --git a/credential-store.c b/credential-store.c index c010497cb2..c2778eff8a 100644 --- a/credential-store.c +++ b/credential-store.c @@ -4,10 +4,20 @@ #include "string-list.h" #include "parse-options.h" +#define PARSE_VERBOSE 0x01 + static struct lock_file credential_lock; +static int valid_credential(struct credential *entry) +{ + return (entry->username && entry->password && + entry->protocol && *entry->protocol && + ((entry->host && *entry->host) || entry->path)); +} + static int parse_credential_file(const char *fn, struct credential *c, + int flags, void (*match_cb)(struct credential *), void (*other_cb)(struct strbuf *)) { @@ -15,6 +25,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,16 +35,24 @@ 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 && - credential_match(c, &entry)) { + lineno++; + + if (strchr(line.buf, '\r') || + credential_from_url_gently(&entry, line.buf, 1) || + !valid_credential(&entry)) { + if (flags & PARSE_VERBOSE) + warning(_("%s:%d: ignoring invalid credential"), + fn, lineno); + } else if (credential_match(c, &entry)) { found_credential = 1; if (match_cb) { match_cb(&entry); break; } + continue; } - else if (other_cb) + + if (other_cb) other_cb(&line); } @@ -62,7 +81,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c, die_errno("unable to get credential storage lock"); if (extra) print_line(extra); - parse_credential_file(fn, c, NULL, print_line); + parse_credential_file(fn, c, 0, NULL, print_line); if (commit_lock_file(&credential_lock) < 0) die_errno("unable to write credential store"); } @@ -139,7 +158,8 @@ static void lookup_credential(const struct string_list *fns, struct credential * struct string_list_item *fn; for_each_string_list_item(fn, fns) - if (parse_credential_file(fn->string, c, print_entry, NULL)) + if (parse_credential_file(fn->string, c, PARSE_VERBOSE, + print_entry, NULL)) return; /* Found credential */ } diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..00608e365a 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home test_must_be_empty "$HOME/.config/git/credentials" ' - test_expect_success 'erase: erase matching credentials from both xdg and home files' ' echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" && mkdir -p "$HOME/.config/git" && @@ -120,4 +119,112 @@ 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: 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 + 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: credentials without valid host/path are invalid' ' + echo "https://user:pass@" >"$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 + 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: credentials without username/password are invalid' ' + echo "https://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 + 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: credentials with DOS line endings are invalid' ' + printf "https://user:pass@example.com\r\n" >"$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 + 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: store file can contain empty/bogus lines' ' + echo "" > "$HOME/.git-credentials" && + q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" && + #comment + Q + https://user:pass@example.com + CONFIG + 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 + EOF + test_cmp expect-stdout stdout && + test_i18ngrep "ignoring invalid credential" stderr && + test_line_count = 3 stderr +' + +test_expect_success 'store: store succeeds silently in corrupted file' ' + echo "#comment" >"$HOME/.git-credentials" && + check approve store <<-\EOF && + url=https://user:pass@example.com + EOF + grep "https://user:pass@example.com" "$HOME/.git-credentials" && + test_must_be_empty stderr +' + test_done base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0 -- 2.26.2.2.gf219a08795 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [RFC PATCH v10 2/1] credential-store: warn also for store and erase 2020-05-01 3:21 ` [RFC PATCH v10] credential-store: warn/ignore for bogus lines from store file Carlo Marcelo Arenas Belón @ 2020-05-01 5:18 ` Carlo Marcelo Arenas Belón 2020-05-01 5:35 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-05-01 5:18 UTC (permalink / raw) To: gitster; +Cc: git, Carlo Marcelo Arenas Belón done in a hacky way and only as a POC, but at least works, if we are still thinking that warning should be at least comprehensive for the first iteration. didn't change the callback as you suggested but I think this doesn't look that bad either. squash for final v10 or ..? --- Documentation/git-credential-store.txt | 7 ------- credential-store.c | 19 +++++++++---------- t/t0302-credential-store.sh | 20 +++++++++++++++++--- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 18ba8b6984..d5841cffad 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -105,13 +105,6 @@ like spaces, line wrapping or text encoding. An unparseable or otherwise invalid line is ignored, and a warning message points out the problematic file and line number it appears in. -Note that because of deficiencies of the current implementation, these -warnings will be only generated for the entries that were processed -while reading the credential files during a get operation, and so they -are not comprehensive, and will require you to use an editor (or -better a sed/awk/perl one liner) to edit the credential file and remove -them. - When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against each entry in the credentials file. If the protocol, hostname, and diff --git a/credential-store.c b/credential-store.c index c2778eff8a..236df8e7bf 100644 --- a/credential-store.c +++ b/credential-store.c @@ -4,8 +4,6 @@ #include "string-list.h" #include "parse-options.h" -#define PARSE_VERBOSE 0x01 - static struct lock_file credential_lock; static int valid_credential(struct credential *entry) @@ -17,7 +15,7 @@ static int valid_credential(struct credential *entry) static int parse_credential_file(const char *fn, struct credential *c, - int flags, + int initial_line, void (*match_cb)(struct credential *), void (*other_cb)(struct strbuf *)) { @@ -25,7 +23,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; + int lineno = initial_line; fh = fopen(fn, "r"); if (!fh) { @@ -40,8 +38,7 @@ static int parse_credential_file(const char *fn, if (strchr(line.buf, '\r') || credential_from_url_gently(&entry, line.buf, 1) || !valid_credential(&entry)) { - if (flags & PARSE_VERBOSE) - warning(_("%s:%d: ignoring invalid credential"), + warning(_("%s:%d: ignoring invalid credential"), fn, lineno); } else if (credential_match(c, &entry)) { found_credential = 1; @@ -77,11 +74,14 @@ static void print_line(struct strbuf *buf) static void rewrite_credential_file(const char *fn, struct credential *c, struct strbuf *extra) { + int initial_line = 0; if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0) die_errno("unable to get credential storage lock"); - if (extra) + if (extra) { print_line(extra); - parse_credential_file(fn, c, 0, NULL, print_line); + initial_line++; + } + parse_credential_file(fn, c, initial_line, NULL, print_line); if (commit_lock_file(&credential_lock) < 0) die_errno("unable to write credential store"); } @@ -158,8 +158,7 @@ static void lookup_credential(const struct string_list *fns, struct credential * struct string_list_item *fn; for_each_string_list_item(fn, fns) - if (parse_credential_file(fn->string, c, PARSE_VERBOSE, - print_entry, NULL)) + if (parse_credential_file(fn->string, c, 0, print_entry, NULL)) return; /* Found credential */ } diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index 00608e365a..78010590f4 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -218,13 +218,27 @@ test_expect_success 'get: store file can contain empty/bogus lines' ' test_line_count = 3 stderr ' -test_expect_success 'store: store succeeds silently in corrupted file' ' +test_expect_success 'store: succeeds in corrupted file but correctly warn' ' echo "#comment" >"$HOME/.git-credentials" && - check approve store <<-\EOF && + test_config credential.helper store && + git credential approve <<-\EOF && url=https://user:pass@example.com EOF grep "https://user:pass@example.com" "$HOME/.git-credentials" && - test_must_be_empty stderr + test_i18ngrep "git-credentials:2: ignoring invalid credential" stderr +' + +test_expect_success 'erase: succeeds in corrupted file but correctly warn' ' + cat <<-\CONFIG >"$HOME/.git-credentials" && + #comment + https://user:pass@example.com + CONFIG + test_config credential.helper store && + git credential reject <<-\EOF && + url=https://example.com + EOF + ! grep "https://user:pass@example.com" "$HOME/.git-credentials" && + test_i18ngrep "git-credentials:1: ignoring invalid credential" stderr ' test_done -- 2.26.2.3.g90bd2f7136 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [RFC PATCH v10 2/1] credential-store: warn also for store and erase 2020-05-01 5:18 ` [RFC PATCH v10 2/1] credential-store: warn also for store and erase Carlo Marcelo Arenas Belón @ 2020-05-01 5:35 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-05-01 5:35 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > done in a hacky way and only as a POC, but at least works, if we > are still thinking that warning should be at least comprehensive > for the first iteration. ... > @@ -25,7 +23,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; > + int lineno = initial_line; > > fh = fopen(fn, "r"); > if (!fh) { > @@ -40,8 +38,7 @@ static int parse_credential_file(const char *fn, > if (strchr(line.buf, '\r') || > credential_from_url_gently(&entry, line.buf, 1) || > !valid_credential(&entry)) { > - if (flags & PARSE_VERBOSE) > - warning(_("%s:%d: ignoring invalid credential"), > + warning(_("%s:%d: ignoring invalid credential"), > fn, lineno); > } else if (credential_match(c, &entry)) { > found_credential = 1; I am not sure how this would correctly count when "store" and "erase" is used. During "store", the initial_line may let us correctly count the new line, and as long as we keep copying, our output line number would stay to be correct, as we increment line by one every time we read in, but as soon as we see the older version of the credential record this "store" is updating, i.e. when the credential_match() reports a match, we will discard the line we read by hitting the "continue" and not calling other_cb(). We increment line, but we don't output a line when that happens, so any warning after that point will be out of sync, no? And "erase" is the same deal. It behaves the same way as "store" except that "erase" does not emit a replacement record at the beginning. As soon as the matching record from the input is dropped, our line number will be out of sync. In any case, I think it probably is a good idea to start with a version that ignores without warning, and I had an impression that you were on the same page from reading your response to v9 review, so... ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v10] credential-store: ignore bogus lines from store file 2020-04-30 16:06 ` [PATCH v9] " Carlo Marcelo Arenas Belón 2020-04-30 20:21 ` Junio C Hamano 2020-05-01 3:21 ` [RFC PATCH v10] credential-store: warn/ignore for bogus lines from store file Carlo Marcelo Arenas Belón @ 2020-05-02 18:16 ` Carlo Marcelo Arenas Belón 2020-05-02 20:47 ` Junio C Hamano ` (2 more replies) 2 siblings, 3 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-05-02 18:16 UTC (permalink / raw) To: git Cc: dirk, sunshine, peff, gitster, jrnieder, Johannes.Schindelin, Carlo Marcelo Arenas Belón 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. Instead of doing a hard check for credentials, do a soft one and therefore avoid the reported fatal error. As a special case, flag files with CRLF endings as invalid early to prevent current problems in credential_from_url_gently() with handling of '\r' in the host. While at it add tests for all known corruptions that are currently ignored to keep track of them and avoid the risk of regressions. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Junio C Hamano <gitster@pobox.com> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v10: * go back to v4 but with better testing and commit message * make sure broken CR characters are ignored early v9: * use strbuf_getline() instead to better handle files with CRLF v8: * only warn during get operations as otherwise line number might be incorrect v7: * check for protocol in helper as suggested by Junio v6: * get rid of redacter and only use line number for context in warning * make validation more strict to also catch incomplete credentials * reorder check as suggested by Junio v5: * q_to_tab this round, with a single echo to make sure empty line is covered, as that seems to be a popular issue * rebase on top of jc/credential-store-file-format-doc * implement a redacter for credentials to use on errors to avoid leaking passwords v4: * use credential_from_url_gently instead as shown by Jonathan * add documentation to clarify "comments" is not a supported feature v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation credential-store.c | 5 ++-- t/t0302-credential-store.sh | 60 ++++++++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/credential-store.c b/credential-store.c index c010497cb2..fdfb81e632 100644 --- a/credential-store.c +++ b/credential-store.c @@ -24,8 +24,9 @@ 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 (strchr(line.buf, '\r') == NULL && + !credential_from_url_gently(&entry, line.buf, 1) && + entry.username && entry.password && credential_match(c, &entry)) { found_credential = 1; if (match_cb) { diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..9fd0aca55e 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home test_must_be_empty "$HOME/.config/git/credentials" ' - test_expect_success 'erase: erase matching credentials from both xdg and home files' ' echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" && mkdir -p "$HOME/.config/git" && @@ -120,4 +119,63 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi test_must_be_empty "$HOME/.config/git/credentials" ' +invalid_credential_test() { + test_expect_success 'get: ignore credentials without $1 as invalid' ' + echo "$2" >"$HOME/.git-credentials" && + check fill store <<-\EOF + protocol=https + host=example.com + -- + 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 + ' +} + +invalid_credential_test "scheme" ://user:pass@example.com +invalid_credential_test "valid host/path" https://user:pass@ +invalid_credential_test "username/password" https://pass@example.com + +test_expect_success 'get: credentials with DOS line endings are invalid' ' + printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" && + check fill store <<-\EOF + protocol=https + host=example.com + -- + 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_expect_success 'get: store file can contain empty/bogus lines' ' + echo "" >"$HOME/.git-credentials" && + q_to_tab <<-\CREDENTIAL >>"$HOME/.git-credentials" && + #comment + Q + https://user:pass@example.com + CREDENTIAL + check fill store <<-\EOF + protocol=https + host=example.com + -- + protocol=https + host=example.com + username=user + password=pass + -- + EOF +' + test_done base-commit: 49458fb91d61461938881559ce23abbb1a2f8c35 -- 2.26.2.686.gfaf46a9ccd ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v10] credential-store: ignore bogus lines from store file 2020-05-02 18:16 ` [PATCH v10] credential-store: ignore bogus lines from store file Carlo Marcelo Arenas Belón @ 2020-05-02 20:47 ` Junio C Hamano 2020-05-02 21:23 ` Carlo Marcelo Arenas Belón ` (2 more replies) 2020-05-02 21:05 ` Carlo Marcelo Arenas Belón 2020-05-02 22:34 ` [PATCH v11] " Carlo Marcelo Arenas Belón 2 siblings, 3 replies; 79+ messages in thread From: Junio C Hamano @ 2020-05-02 20:47 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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. > > Instead of doing a hard check for credentials, do a soft one and > therefore avoid the reported fatal error. > > As a special case, flag files with CRLF endings as invalid early > to prevent current problems in credential_from_url_gently() with > handling of '\r' in the host. I do not think it hurts to silently ignore a line that ends with CR, but only because I do not think credential_from_url_gently() would not match such a line when asked to match something without complaining. In other words, isn't the new "!strchr() &&" in the condition a no-op? > diff --git a/credential-store.c b/credential-store.c > index c010497cb2..fdfb81e632 100644 > --- a/credential-store.c > +++ b/credential-store.c > @@ -24,8 +24,9 @@ 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 (strchr(line.buf, '\r') == NULL && > + !credential_from_url_gently(&entry, line.buf, 1) && > + entry.username && entry.password && > credential_match(c, &entry)) { > found_credential = 1; > if (match_cb) { In any case, among the ones we discussed, this probably has the least chance of unintended regression, I would think (with or without the added "!strchr() &&" check), so let's queue it and quickly merge it down thru 'next' to 'master'. > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > index d6b54e8c65..9fd0aca55e 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home > test_must_be_empty "$HOME/.config/git/credentials" > ' > > - > test_expect_success 'erase: erase matching credentials from both xdg and home files' ' > echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" && > mkdir -p "$HOME/.config/git" && > @@ -120,4 +119,63 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi > test_must_be_empty "$HOME/.config/git/credentials" > ' > > +invalid_credential_test() { > + test_expect_success 'get: ignore credentials without $1 as invalid' ' > + echo "$2" >"$HOME/.git-credentials" && > + check fill store <<-\EOF > + protocol=https > + host=example.com > + -- > + 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 > + ' > +} > + > +invalid_credential_test "scheme" ://user:pass@example.com > +invalid_credential_test "valid host/path" https://user:pass@ > +invalid_credential_test "username/password" https://pass@example.com These are quite clear to see. Nicely done. > +test_expect_success 'get: credentials with DOS line endings are invalid' ' > + printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" && > + check fill store <<-\EOF > + protocol=https > + host=example.com > + -- > + 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 > +' What I am curious is if anything breaks around this test if you lost the extra "!strchr() &&" check. I suspect that this test will pass. > +test_expect_success 'get: store file can contain empty/bogus lines' ' > + echo "" >"$HOME/.git-credentials" && > + q_to_tab <<-\CREDENTIAL >>"$HOME/.git-credentials" && > + #comment > + Q > + https://user:pass@example.com > + CREDENTIAL > + check fill store <<-\EOF > + protocol=https > + host=example.com > + -- > + protocol=https > + host=example.com > + username=user > + password=pass > + -- > + EOF > +' > + > test_done > > base-commit: 49458fb91d61461938881559ce23abbb1a2f8c35 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v10] credential-store: ignore bogus lines from store file 2020-05-02 20:47 ` Junio C Hamano @ 2020-05-02 21:23 ` Carlo Marcelo Arenas Belón 2020-05-02 21:53 ` Carlo Marcelo Arenas Belón 2020-05-03 10:06 ` Jeff King 2 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-05-02 21:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > 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. > > > > Instead of doing a hard check for credentials, do a soft one and > > therefore avoid the reported fatal error. > > > > As a special case, flag files with CRLF endings as invalid early > > to prevent current problems in credential_from_url_gently() with > > handling of '\r' in the host. > > I do not think it hurts to silently ignore a line that ends with CR, > but only because I do not think credential_from_url_gently() would > not match such a line when asked to match something without > complaining. for a credential like the one in the testcase (meaning no url), it will append \r to the hostname, which would cause havoc if that credential is printed (meaning you will end up without a host line) and be back in the die() in credential_apply() > In other words, isn't the new "!strchr() &&" in the condition a > no-op? you are correct that it will be unlikely (but not imposible) to get an embedded CR from the other side to match, which is what I want to address in the next patchset. IMHO adding the proposed early check gives us space to fix the other issues at our own leasure and it is meant to be gone eventually. > > diff --git a/credential-store.c b/credential-store.c > > index c010497cb2..fdfb81e632 100644 > > --- a/credential-store.c > > +++ b/credential-store.c > > @@ -24,8 +24,9 @@ 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 (strchr(line.buf, '\r') == NULL && > > + !credential_from_url_gently(&entry, line.buf, 1) && > > + entry.username && entry.password && > > credential_match(c, &entry)) { > > found_credential = 1; > > if (match_cb) { > > In any case, among the ones we discussed, this probably has the > least chance of unintended regression, I would think (with or > without the added "!strchr() &&" check), so let's queue it and > quickly merge it down thru 'next' to 'master'. considering the only line that I wrote was the strchr and the other one was written by Jonathan and reviewed by Peff I definitly agree. don't forget this is also a good candidate for maint (most likely all the way to maint-2.17) Carlo ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v10] credential-store: ignore bogus lines from store file 2020-05-02 20:47 ` Junio C Hamano 2020-05-02 21:23 ` Carlo Marcelo Arenas Belón @ 2020-05-02 21:53 ` Carlo Marcelo Arenas Belón 2020-05-03 0:44 ` Junio C Hamano 2020-05-03 10:06 ` Jeff King 2 siblings, 1 reply; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-05-02 21:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote: > > What I am curious is if anything breaks around this test if you lost > the extra "!strchr() &&" check. I suspect that this test will pass. correct and with the strchr I am introducing a regression (compared against 2.26.0) in cases where the '\r' gets appended to the path and therefore gets ignored for matching (unless useHttpPath is true, which is not the default) will add 2 more test cases to cover those, and guess we are going to 11 :( Carlo PS. thanks for all your patience and reviewing ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v10] credential-store: ignore bogus lines from store file 2020-05-02 21:53 ` Carlo Marcelo Arenas Belón @ 2020-05-03 0:44 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2020-05-03 0:44 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: git, dirk, sunshine, peff, jrnieder, Johannes.Schindelin Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > and with the strchr I am introducing a regression (compared against 2.26.0) > in cases where the '\r' gets appended to the path and therefore gets ignored > for matching (unless useHttpPath is true, which is not the default) > > will add 2 more test cases to cover those, and guess we are going to 11 :( > > Carlo > > PS. thanks for all your patience and reviewing Among those 11, I also am responsible for a few while we ere pursuing the approach to warn X-<. Thanks for _your_ patience. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v10] credential-store: ignore bogus lines from store file 2020-05-02 20:47 ` Junio C Hamano 2020-05-02 21:23 ` Carlo Marcelo Arenas Belón 2020-05-02 21:53 ` Carlo Marcelo Arenas Belón @ 2020-05-03 10:06 ` Jeff King 2 siblings, 0 replies; 79+ messages in thread From: Jeff King @ 2020-05-03 10:06 UTC (permalink / raw) To: Junio C Hamano Cc: Carlo Marcelo Arenas Belón, git, dirk, sunshine, jrnieder, Johannes.Schindelin On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote: > > As a special case, flag files with CRLF endings as invalid early > > to prevent current problems in credential_from_url_gently() with > > handling of '\r' in the host. > > I do not think it hurts to silently ignore a line that ends with CR, > but only because I do not think credential_from_url_gently() would > not match such a line when asked to match something without > complaining. I wondered if we might hit a case where the CR ends up in the path, like: $ printf 'https://user:pass@example.com/\r\n' >creds $ echo url=https://example.com/ | git credential-store --file=creds get username=user password=pass because the path is parsed as missing in the incoming pattern (and thus we match any path, even "\r"). But credential-store would never write such a path in the first place. Even with the trailing slash on an incoming URL, it will write: https://example.com without a slash at all (and thus any inserted CR would be part of the hostname). So somebody would have to have inserted it themselves, or have turned useHTTPPath on (in which case we _would_ complain on the matching side, too, because we'd try matching the path with a CR in it). I think it's reasonable to assume that any CR would have been a problem even in older versions. -Peff ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v10] credential-store: ignore bogus lines from store file 2020-05-02 18:16 ` [PATCH v10] credential-store: ignore bogus lines from store file Carlo Marcelo Arenas Belón 2020-05-02 20:47 ` Junio C Hamano @ 2020-05-02 21:05 ` Carlo Marcelo Arenas Belón 2020-05-02 22:34 ` [PATCH v11] " Carlo Marcelo Arenas Belón 2 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-05-02 21:05 UTC (permalink / raw) To: gitster, Johannes.Schindelin; +Cc: dirk, sunshine, peff, jrnieder, git Dscho, this was tested[1] in windows and was able to reproduce the bogus issues with credential store in macOS using t0302.50 [1] https://github.com/carenas/git/actions/runs/93858709 Junio, will followup with a more comprehensive fix for the problems with '\r' which will obiously need also changes in credential.c, but that IMHO might be more of a prerequisite for the next iteration (the one that warns) would be very helpful if you squash the following, otherwise Carlo -- >8 -- Subject: [PATCH] SQUASH!!! Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- t/t0302-credential-store.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index 9fd0aca55e..a05b64c8b3 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,7 +120,7 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi ' invalid_credential_test() { - test_expect_success 'get: ignore credentials without $1 as invalid' ' + test_expect_success "get: ignore credentials without $1 as invalid" ' echo "$2" >"$HOME/.git-credentials" && check fill store <<-\EOF protocol=https -- 2.26.2.686.gfaf46a9ccd ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v11] credential-store: ignore bogus lines from store file 2020-05-02 18:16 ` [PATCH v10] credential-store: ignore bogus lines from store file Carlo Marcelo Arenas Belón 2020-05-02 20:47 ` Junio C Hamano 2020-05-02 21:05 ` Carlo Marcelo Arenas Belón @ 2020-05-02 22:34 ` Carlo Marcelo Arenas Belón 2 siblings, 0 replies; 79+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-05-02 22:34 UTC (permalink / raw) To: git Cc: Carlo Marcelo Arenas Belón, Dirk, Eric Sunshine, Junio C Hamano, Jonathan Nieder 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. Instead of doing a hard check for credentials, do a soft one and therefore avoid the reported fatal error. While at it add tests for all known corruptions that are currently ignored to keep track of them and avoid the risk of regressions. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Junio C Hamano <gitster@pobox.com> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v11: * remove bogus early check for CR characters v10: * go back to v4 but with better testing and commit message * make sure broken CR characters are ignored early v9: * use strbuf_getline() instead to better handle files with CRLF v8: * only warn during get operations as otherwise line number might be incorrect v7: * check for protocol in helper as suggested by Junio v6: * get rid of redacter and only use line number for context in warning * make validation more strict to also catch incomplete credentials * reorder check as suggested by Junio v5: * q_to_tab this round, with a single echo to make sure empty line is covered, as that seems to be a popular issue * rebase on top of jc/credential-store-file-format-doc * implement a redacter for credentials to use on errors to avoid leaking passwords v4: * use credential_from_url_gently instead as shown by Jonathan * add documentation to clarify "comments" is not a supported feature v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation credential-store.c | 4 +- t/t0302-credential-store.sh | 91 ++++++++++++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/credential-store.c b/credential-store.c index c010497cb2..294e771681 100644 --- a/credential-store.c +++ b/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) { diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..716bf1af9f 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home test_must_be_empty "$HOME/.config/git/credentials" ' - test_expect_success 'erase: erase matching credentials from both xdg and home files' ' echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" && mkdir -p "$HOME/.config/git" && @@ -120,4 +119,94 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi test_must_be_empty "$HOME/.config/git/credentials" ' +invalid_credential_test() { + test_expect_success "get: ignore credentials without $1 as invalid" ' + echo "$2" >"$HOME/.git-credentials" && + check fill store <<-\EOF + protocol=https + host=example.com + -- + 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 + ' +} + +invalid_credential_test "scheme" ://user:pass@example.com +invalid_credential_test "valid host/path" https://user:pass@ +invalid_credential_test "username/password" https://pass@example.com + +test_expect_success 'get: credentials with DOS line endings are invalid' ' + printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" && + check fill store <<-\EOF + protocol=https + host=example.com + -- + 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_expect_success 'get: credentials with path and DOS line endings are valid' ' + printf "https://user:pass@example.com/repo.git\r\n" >"$HOME/.git-credentials" && + check fill store <<-\EOF + url=https://example.com/repo.git + -- + protocol=https + host=example.com + username=user + password=pass + -- + EOF +' + +test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' ' + printf "https://user:pass@example.com/repo.git\r\n" >"$HOME/.git-credentials" && + test_config credential.useHttpPath true && + check fill store <<-\EOF + url=https://example.com/repo.git + -- + protocol=https + host=example.com + path=repo.git + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://example.com/repo.git'\'': + askpass: Password for '\''https://askpass-username@example.com/repo.git'\'': + -- + EOF +' + +test_expect_success 'get: store file can contain empty/bogus lines' ' + echo "" >"$HOME/.git-credentials" && + q_to_tab <<-\CREDENTIAL >>"$HOME/.git-credentials" && + #comment + Q + https://user:pass@example.com + CREDENTIAL + check fill store <<-\EOF + protocol=https + host=example.com + -- + protocol=https + host=example.com + username=user + password=pass + -- + EOF +' + test_done base-commit: 49458fb91d61461938881559ce23abbb1a2f8c35 -- 2.26.2.686.gfaf46a9ccd ^ permalink raw reply related [flat|nested] 79+ messages in thread
end of thread, other threads:[~2020-05-03 10:06 UTC | newest] Thread overview: 79+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-26 23:47 [PATCH] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón 2020-04-27 0:19 ` Eric Sunshine 2020-04-27 0:46 ` Carlo Marcelo Arenas Belón 2020-04-27 8:42 ` [PATCH v2] " Carlo Marcelo Arenas Belón 2020-04-27 11:52 ` Jeff King 2020-04-27 12:25 ` Carlo Marcelo Arenas Belón 2020-04-27 14:43 ` Eric Sunshine 2020-04-27 17:47 ` Junio C Hamano 2020-04-27 19:09 ` Jeff King 2020-04-27 12:59 ` [PATCH v3] " Carlo Marcelo Arenas Belón 2020-04-27 13:48 ` Philip Oakley 2020-04-28 1:49 ` Carlo Marcelo Arenas Belón 2020-04-29 10:09 ` Philip Oakley 2020-04-27 15:39 ` Dirk 2020-04-27 18:09 ` Junio C Hamano 2020-04-27 19:18 ` Jeff King 2020-04-27 20:43 ` Junio C Hamano 2020-04-27 21:10 ` Jeff King 2020-04-28 1:37 ` Carlo Marcelo Arenas Belón 2020-04-27 23:49 ` Carlo Marcelo Arenas Belón 2020-04-28 5:25 ` Jonathan Nieder 2020-04-28 5:41 ` Jeff King 2020-04-28 7:18 ` Carlo Marcelo Arenas Belón 2020-04-28 8:16 ` Jeff King 2020-04-28 11:25 ` Carlo Marcelo Arenas Belón 2020-04-28 10:58 ` Stefan Tauner 2020-04-28 16:03 ` Junio C Hamano 2020-04-28 21:14 ` Carlo Marcelo Arenas Belón 2020-04-28 21:17 ` Junio C Hamano 2020-04-28 10:48 ` [PATCH v4 0/4] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 1/4] credential-store: document the file format a bit more Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón 2020-04-28 16:09 ` Eric Sunshine 2020-04-28 16:42 ` Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 3/4] git-credential-store: fix (WIP) Carlo Marcelo Arenas Belón 2020-04-28 16:11 ` Eric Sunshine 2020-04-28 17:14 ` Carlo Marcelo Arenas Belón 2020-04-28 10:52 ` [PATCH v4 4/4] credential-store: make sure there is no regression with missing scheme Carlo Marcelo Arenas Belón 2020-04-28 16:06 ` [PATCH v4 1/4] credential-store: document the file format a bit more Eric Sunshine 2020-04-28 18:18 ` Junio C Hamano 2020-04-28 18:15 ` Junio C Hamano 2020-04-29 0:33 ` [PATCH v5] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 2020-04-29 4:36 ` Junio C Hamano 2020-04-29 7:31 ` Carlo Marcelo Arenas Belón 2020-04-29 16:46 ` Junio C Hamano 2020-04-29 20:35 ` [RFC PATCH v6 0/2] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón 2020-04-29 20:35 ` [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 2020-04-29 21:05 ` Junio C Hamano 2020-04-29 21:17 ` Junio C Hamano 2020-04-29 20:35 ` [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using Carlo Marcelo Arenas Belón 2020-04-29 21:12 ` Junio C Hamano 2020-04-29 21:49 ` [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy Carlo Marcelo Arenas Belón 2020-04-29 22:04 ` Junio C Hamano 2020-04-29 23:23 ` [PATCH v6] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón 2020-04-29 23:47 ` Junio C Hamano 2020-04-29 23:57 ` Junio C Hamano 2020-04-30 1:00 ` Carlo Marcelo Arenas Belón 2020-04-30 1:19 ` [PATCH v7] " Carlo Marcelo Arenas Belón 2020-04-30 9:29 ` [PATCH v8] " Carlo Marcelo Arenas Belón 2020-04-30 16:06 ` [PATCH v9] " Carlo Marcelo Arenas Belón 2020-04-30 20:21 ` Junio C Hamano 2020-04-30 21:14 ` Junio C Hamano 2020-05-01 0:30 ` Carlo Marcelo Arenas Belón 2020-05-01 1:40 ` Junio C Hamano 2020-05-01 2:24 ` Carlo Arenas 2020-05-01 5:27 ` Junio C Hamano 2020-05-01 13:57 ` Carlo Marcelo Arenas Belón 2020-05-01 18:59 ` Junio C Hamano 2020-05-01 3:21 ` [RFC PATCH v10] credential-store: warn/ignore for bogus lines from store file Carlo Marcelo Arenas Belón 2020-05-01 5:18 ` [RFC PATCH v10 2/1] credential-store: warn also for store and erase Carlo Marcelo Arenas Belón 2020-05-01 5:35 ` Junio C Hamano 2020-05-02 18:16 ` [PATCH v10] credential-store: ignore bogus lines from store file Carlo Marcelo Arenas Belón 2020-05-02 20:47 ` Junio C Hamano 2020-05-02 21:23 ` Carlo Marcelo Arenas Belón 2020-05-02 21:53 ` Carlo Marcelo Arenas Belón 2020-05-03 0:44 ` Junio C Hamano 2020-05-03 10:06 ` Jeff King 2020-05-02 21:05 ` Carlo Marcelo Arenas Belón 2020-05-02 22:34 ` [PATCH v11] " Carlo Marcelo Arenas Belón
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).