76 lines
3.0 KiB
Diff
76 lines
3.0 KiB
Diff
From fe29a9b7b0236d3d45c254965580d6aff7fa8504 Mon Sep 17 00:00:00 2001
|
|
From: Jeff King <peff@peff.net>
|
|
Date: Sat, 18 Apr 2020 20:53:09 -0700
|
|
Subject: [PATCH] credential: die() when parsing invalid urls
|
|
|
|
When we try to initialize credential loading by URL and find that the
|
|
URL is invalid, we set all fields to NULL in order to avoid acting on
|
|
malicious input. Later when we request credentials, we diagonse the
|
|
erroneous input:
|
|
|
|
fatal: refusing to work with credential missing host field
|
|
|
|
This is problematic in two ways:
|
|
|
|
- The message doesn't tell the user *why* we are missing the host
|
|
field, so they can't tell from this message alone how to recover.
|
|
There can be intervening messages after the original warning of
|
|
bad input, so the user may not have the context to put two and two
|
|
together.
|
|
|
|
- The error only occurs when we actually need to get a credential. If
|
|
the URL permits anonymous access, the only encouragement the user gets
|
|
to correct their bogus URL is a quiet warning.
|
|
|
|
This is inconsistent with the check we perform in fsck, where any use
|
|
of such a URL as a submodule is an error.
|
|
|
|
When we see such a bogus URL, let's not try to be nice and continue
|
|
without helpers. Instead, die() immediately. This is simpler and
|
|
obviously safe. And there's very little chance of disrupting a normal
|
|
workflow.
|
|
|
|
It's _possible_ that somebody has a legitimate URL with a raw newline in
|
|
it. It already wouldn't work with credential helpers, so this patch
|
|
steps that up from an inconvenience to "we will refuse to work with it
|
|
at all". If such a case does exist, we should figure out a way to work
|
|
with it (especially if the newline is only in the path component, which
|
|
we normally don't even pass to helpers). But until we see a real report,
|
|
we're better off being defensive.
|
|
|
|
Reported-by: Carlo Arenas <carenas@gmail.com>
|
|
Signed-off-by: Jeff King <peff@peff.net>
|
|
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
|
---
|
|
credential.c | 6 ++----
|
|
t/t0300-credentials.sh | 3 +--
|
|
2 files changed, 3 insertions(+), 6 deletions(-)
|
|
|
|
--- a/credential.c
|
|
+++ b/credential.c
|
|
@@ -408,8 +408,6 @@ int credential_from_url_gently(struct cr
|
|
|
|
void credential_from_url(struct credential *c, const char *url)
|
|
{
|
|
- if (credential_from_url_gently(c, url, 0) < 0) {
|
|
- warning(_("skipping credential lookup for url: %s"), url);
|
|
- credential_clear(c);
|
|
- }
|
|
+ if (credential_from_url_gently(c, url, 0) < 0)
|
|
+ die(_("credential url cannot be parsed: %s"), url);
|
|
}
|
|
--- a/t/t0300-credentials.sh
|
|
+++ b/t/t0300-credentials.sh
|
|
@@ -405,8 +405,7 @@ test_expect_success 'url parser rejects
|
|
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/
|
|
- fatal: refusing to work with credential missing host field
|
|
+ fatal: credential url cannot be parsed: https://one.example.com?%0ahost=two.example.com/
|
|
EOF
|
|
test_i18ncmp expect stderr
|
|
'
|
|
--
|
|
1.8.3.1
|