Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> 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@xxxxxxxxxxx > ------------------------------ > > 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@xxxxxxxxxxx" >"$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@xxxxxxxxxxx'\'':" 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.
