| Message ID | 20200427084235.60798-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] git-credential-store: skip empty lines and comments from store | expand |
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
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
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/
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.
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
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
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(+)