From 8ba8ed568e2a3b75ee84c49ddffb026fde1a0a91 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Apr 2020 20:50:48 -0700 Subject: credential: refuse to operate when missing host or protocol The credential helper protocol was designed to be very flexible: the fields it takes as input are treated as a pattern, and any missing fields are taken as wildcards. This allows unusual things like: echo protocol=https | git credential reject to delete all stored https credentials (assuming the helpers themselves treat the input that way). But when helpers are invoked automatically by Git, this flexibility works against us. If for whatever reason we don't have a "host" field, then we'd match _any_ host. When you're filling a credential to send to a remote server, this is almost certainly not what you want. Prevent this at the layer that writes to the credential helper. Add a check to the credential API that the host and protocol are always passed in, and add an assertion to the credential_write function that speaks credential helper protocol to be doubly sure. There are a few ways this can be triggered in practice: - the "git credential" command passes along arbitrary credential parameters it reads from stdin. - until the previous patch, when the host field of a URL is empty, we would leave it unset (rather than setting it to the empty string) - a URL like "example.com/foo.git" is treated by curl as if "http://" was present, but our parser sees it as a non-URL and leaves all fields unset - the recent fix for URLs with embedded newlines blanks the URL but otherwise continues. Rather than having the desired effect of looking up no credential at all, many helpers will return _any_ credential Our earlier test for an embedded newline didn't catch this because it only checked that the credential was cleared, but didn't configure an actual helper. Configuring the "verbatim" helper in the test would show that it is invoked (it's obviously a silly helper which doesn't look at its input, but the point is that it shouldn't be run at all). Since we're switching this case to die(), we don't need to bother with a helper. We can see the new behavior just by checking that the operation fails. We'll add new tests covering partial input as well (these can be triggered through various means with url-parsing, but it's simpler to just check them directly, as we know we are covered even if the url parser changes behavior in the future). [jn: changed to die() instead of logging and showing a manual username/password prompt] Reported-by: Carlo Arenas Signed-off-by: Jeff King Signed-off-by: Jonathan Nieder --- t/t0300-credentials.sh | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) (limited to 't') diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 1c1010bc54..646f8456ff 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -400,18 +400,16 @@ test_expect_success 'empty helper spec resets helper list' ' EOF ' -test_expect_success 'url parser ignores embedded newlines' ' - check fill <<-EOF +test_expect_success 'url parser rejects embedded newlines' ' + test_must_fail git credential fill 2>stderr <<-\EOF && url=https://one.example.com?%0ahost=two.example.com/ - -- - username=askpass-username - password=askpass-password - -- + EOF + cat >expect <<-\EOF && warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/ warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/ - askpass: Username: - askpass: Password: + fatal: refusing to work with credential missing host field EOF + test_i18ncmp expect stderr ' test_expect_success 'host-less URLs are parsed as empty host' ' @@ -431,4 +429,24 @@ test_expect_success 'host-less URLs are parsed as empty host' ' EOF ' +test_expect_success 'credential system refuses to work with missing host' ' + test_must_fail git credential fill 2>stderr <<-\EOF && + protocol=http + EOF + cat >expect <<-\EOF && + fatal: refusing to work with credential missing host field + EOF + test_i18ncmp expect stderr +' + +test_expect_success 'credential system refuses to work with missing protocol' ' + test_must_fail git credential fill 2>stderr <<-\EOF && + host=example.com + EOF + cat >expect <<-\EOF && + fatal: refusing to work with credential missing protocol field + EOF + test_i18ncmp expect stderr +' + test_done -- cgit v1.2.3