Package: git / 1:2.20.1-2+deb10u3

0053-credential-die-when-parsing-invalid-urls.diff Patch series | download
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
From 2062d1fb629c87e2b2f5921c8276026b33978799 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Sat, 18 Apr 2020 20:53:09 -0700
Subject: 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>
(cherry picked from commit fe29a9b7b0236d3d45c254965580d6aff7fa8504)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 credential.c           | 6 ++----
 t/t0300-credentials.sh | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/credential.c b/credential.c
index e08ed84ff48..22649d559f1 100644
--- a/credential.c
+++ b/credential.c
@@ -408,8 +408,6 @@ int credential_from_url_gently(struct credential *c, const char *url,
 
 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);
 }
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index acb2b243208..6d44e7e5ccd 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -405,8 +405,7 @@ test_expect_success 'url parser rejects embedded newlines' '
 	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
 '
-- 
2.26.1.301.g55bc3eb7cb9