| Message ID | 20200502181643.38203-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v10] credential-store: ignore bogus lines from store file | expand |
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
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
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
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
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.
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
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
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(-) base-commit: 49458fb91d61461938881559ce23abbb1a2f8c35