| Message ID | 20200429232322.68038-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] credential-store: warn instead of fatal for bogus lines from store | 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. 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.
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).
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
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
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(-) base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0