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 <carenas@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

2 files changed, 40 insertions, 14 deletions

diff --git a/credential.c b/credential.c
index d1bb71b..39b7cb0 100644
--- a/credential.c
+++ b/credential.c
@@ -88,6 +88,11 @@ static int proto_is_http(const char *s)
 
 static void credential_apply_config(struct credential *c)
 {
+	if (!c->host)
+		die(_("refusing to work with credential missing host field"));
+	if (!c->protocol)
+		die(_("refusing to work with credential missing protocol field"));
+
 	if (c->configured)
 		return;
 	git_config(credential_config_callback, c);
@@ -190,8 +195,11 @@ int credential_read(struct credential *c, FILE *fp)
 	return 0;
 }
 
-static void credential_write_item(FILE *fp, const char *key, const char *value)
+static void credential_write_item(FILE *fp, const char *key, const char *value,
+				  int required)
 {
+	if (!value && required)
+		BUG("credential value for %s is missing", key);
 	if (!value)
 		return;
 	if (strchr(value, '\n'))
@@ -201,11 +209,11 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)
 
 void credential_write(const struct credential *c, FILE *fp)
 {
-	credential_write_item(fp, "protocol", c->protocol);
-	credential_write_item(fp, "host", c->host);
-	credential_write_item(fp, "path", c->path);
-	credential_write_item(fp, "username", c->username);
-	credential_write_item(fp, "password", c->password);
+	credential_write_item(fp, "protocol", c->protocol, 1);
+	credential_write_item(fp, "host", c->host, 1);
+	credential_write_item(fp, "path", c->path, 0);
+	credential_write_item(fp, "username", c->username, 0);
+	credential_write_item(fp, "password", c->password, 0);
 }
 
 static int run_credential_helper(struct credential *c,
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 1c1010b..646f845 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