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
