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. > > 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@xxxxxxxxxxx" >"$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@xxxxxxxxxxx'\'': > + -- > + EOF > + ' > +} > + > +invalid_credential_test "scheme" ://user:pass@xxxxxxxxxxx > +invalid_credential_test "valid host/path" https://user:pass@ > +invalid_credential_test "username/password" https://pass@xxxxxxxxxxx These are quite clear to see. Nicely done. > +test_expect_success 'get: credentials with DOS line endings are invalid' ' > + printf "https://user:pass@xxxxxxxxxxx\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@xxxxxxxxxxx'\'': > + -- > + 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@xxxxxxxxxxx > + 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