| Message ID | 20200429203546.56753-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | credential-store: prevent fatal errors | 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 > 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.
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.
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
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(-)