From 20b4964fdf43491494122fbd297b2da7d66663b2 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 28 Apr 2020 11:15:15 -0700 Subject: [PATCH 1/2] credential-store: document the file format a bit more Reading a malformed credential URL line and silently ignoring it does not mean that we support empty lines and/or "# commented" lines forever. We should document it to avoid confusion. Signed-off-by: Junio C Hamano --- Documentation/git-credential-store.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 25fb963f4b..24166ca8b9 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -94,6 +94,10 @@ stored on its own line as a URL like: 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. + When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against each entry in the credentials file. If the protocol, hostname, and From c03859a66525c6bdc5cd3c02b31f06cfdd80b595 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= Date: Sat, 2 May 2020 15:34:47 -0700 Subject: [PATCH 2/2] credential-store: ignore bogus lines from store file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. While at it add tests for all known corruptions that are currently ignored to keep track of them and avoid the risk of regressions. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk Helped-by: Eric Sunshine Helped-by: Junio C Hamano Based-on-patch-by: Jonathan Nieder Signed-off-by: Carlo Marcelo Arenas Belón Signed-off-by: Junio C Hamano --- credential-store.c | 4 +- t/t0302-credential-store.sh | 91 ++++++++++++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/credential-store.c b/credential-store.c index ac295420dd..68c02fb5d9 100644 --- a/credential-store.c +++ b/credential-store.c @@ -24,8 +24,8 @@ 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 (!credential_from_url_gently(&entry, line.buf, 1) && + entry.username && entry.password && credential_match(c, &entry)) { found_credential = 1; if (match_cb) { diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..716bf1af9f 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@example.com" >"$HOME/.git-credentials" && mkdir -p "$HOME/.config/git" && @@ -120,4 +119,94 @@ 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@example.com'\'': + -- + EOF + ' +} + +invalid_credential_test "scheme" ://user:pass@example.com +invalid_credential_test "valid host/path" https://user:pass@ +invalid_credential_test "username/password" https://pass@example.com + +test_expect_success 'get: credentials with DOS line endings are invalid' ' + printf "https://user:pass@example.com\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@example.com'\'': + -- + EOF +' + +test_expect_success 'get: credentials with path and DOS line endings are valid' ' + printf "https://user:pass@example.com/repo.git\r\n" >"$HOME/.git-credentials" && + check fill store <<-\EOF + url=https://example.com/repo.git + -- + protocol=https + host=example.com + username=user + password=pass + -- + EOF +' + +test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' ' + printf "https://user:pass@example.com/repo.git\r\n" >"$HOME/.git-credentials" && + test_config credential.useHttpPath true && + check fill store <<-\EOF + url=https://example.com/repo.git + -- + protocol=https + host=example.com + path=repo.git + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://example.com/repo.git'\'': + askpass: Password for '\''https://askpass-username@example.com/repo.git'\'': + -- + EOF +' + +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@example.com + CREDENTIAL + check fill store <<-\EOF + protocol=https + host=example.com + -- + protocol=https + host=example.com + username=user + password=pass + -- + EOF +' + test_done