[v3,2/2] credential: erase all matching credentials

Commit Message

M Hickford June 15, 2023, 6:03 a.m. UTC

From: M Hickford <[e-mail]>

`credential reject` sends the erase action to each helper, but the
exact behaviour of erase isn't specified in documentation or tests.
Some helpers (such as credential-libsecret) delete all matching
credentials, others (such as credential-cache and credential-store)
delete at most one matching credential.

Test that helpers erase all matching credentials. This behaviour is
easiest to reason about. Users expect that `echo
"url=https://example.com" | git credential reject` or `echo
"url=https://example.com\nusername=tim" | git credential reject` erase
all matching credentials.

Fix credential-cache and credential-store.

Signed-off-by: M Hickford <[e-mail]>
---
 Documentation/git-credential.txt   |  2 +-
 Documentation/gitcredentials.txt   |  2 +-
 builtin/credential-cache--daemon.c | 15 ++++++++------
 builtin/credential-store.c         |  1 -
 t/lib-credential.sh                | 33 ++++++++++++++++++++++++++++++
 5 files changed, 44 insertions(+), 9 deletions(-)

Comments

Jeff King June 15, 2023, 7:09 a.m. UTC | #1

On Thu, Jun 15, 2023 at 06:03:24AM +0000, M Hickford via GitGitGadget wrote:

> From: M Hickford <[e-mail]>
> 
> `credential reject` sends the erase action to each helper, but the
> exact behaviour of erase isn't specified in documentation or tests.
> Some helpers (such as credential-libsecret) delete all matching
> credentials, others (such as credential-cache and credential-store)
> delete at most one matching credential.
> 
> Test that helpers erase all matching credentials. This behaviour is
> easiest to reason about. Users expect that `echo
> "url=https://example.com" | git credential reject` or `echo
> "url=https://example.com\nusername=tim" | git credential reject` erase
> all matching credentials.
> 
> Fix credential-cache and credential-store.

This last sentence is out of date now, right? We are only fixing
credential-cache.

> diff --git a/builtin/credential-store.c b/builtin/credential-store.c
> index f80ff59f18a..06bbfa4dd03 100644
> --- a/builtin/credential-store.c
> +++ b/builtin/credential-store.c
> @@ -35,7 +35,6 @@ static int parse_credential_file(const char *fn,
>  			found_credential = 1;
>  			if (match_cb) {
>  				match_cb(&entry);
> -				break;
>  			}
>  		}
>  		else if (other_cb)

This hunk is wrong, isn't it? We should not be touching credential-store
at all (and losing the "break" means that we'd accidentally print
multiple matching entries for "get").

-Peff

Patch

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index 0e6d9e85ec7..a220afed4f3 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -39,7 +39,7 @@  for later use.
 
 If the action is `reject`, git-credential will send the description to
 any configured credential helpers, which may erase any stored
-credential matching the description.
+credentials matching the description.
 
 If the action is `approve` or `reject`, no output should be emitted.
 
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index 100f045bb1a..65d652dc40e 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -260,7 +260,7 @@  appended to its command line, which is one of:
 
 `erase`::
 
-	Remove a matching credential, if any, from the helper's storage.
+	Remove matching credentials, if any, from the helper's storage.
 
 The details of the credential will be provided on the helper's stdin
 stream. The exact format is the same as the input/output format of the
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index f64dd21d335..dc1cf2d25fc 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -33,12 +33,12 @@  static void cache_credential(struct credential *c, int timeout)
 	e->expiration = time(NULL) + timeout;
 }
 
-static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)
+static struct credential_cache_entry *lookup_credential(const struct credential *c)
 {
 	int i;
 	for (i = 0; i < entries_nr; i++) {
 		struct credential *e = &entries[i].item;
-		if (credential_match(c, e, match_password))
+		if (credential_match(c, e, 0))
 			return &entries[i];
 	}
 	return NULL;
@@ -48,9 +48,12 @@  static void remove_credential(const struct credential *c, int match_password)
 {
 	struct credential_cache_entry *e;
 
-	e = lookup_credential(c, match_password);
-	if (e)
-		e->expiration = 0;
+	int i;
+	for (i = 0; i < entries_nr; i++) {
+		e = &entries[i];
+		if (credential_match(c, &e->item, match_password))
+			e->expiration = 0;
+	}
 }
 
 static timestamp_t check_expirations(void)
@@ -127,7 +130,7 @@  static void serve_one_client(FILE *in, FILE *out)
 	if (read_request(in, &c, &action, &timeout) < 0)
 		/* ignore error */ ;
 	else if (!strcmp(action.buf, "get")) {
-		struct credential_cache_entry *e = lookup_credential(&c, 0);
+		struct credential_cache_entry *e = lookup_credential(&c);
 		if (e) {
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index f80ff59f18a..06bbfa4dd03 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -35,7 +35,6 @@  static int parse_credential_file(const char *fn,
 			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
-				break;
 			}
 		}
 		else if (other_cb)
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 7b4fdd011d7..3f21ba66929 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -45,6 +45,8 @@  helper_test_clean() {
 	reject $1 https example.com user2
 	reject $1 https example.com user4
 	reject $1 https example.com user5
+	reject $1 https example.com user6
+	reject $1 https example.com user7
 	reject $1 https example.com user8
 	reject $1 http path.tld user
 	reject $1 https timeout.tld user
@@ -342,6 +344,37 @@  helper_test() {
 		EOF
 	'
 
+	test_expect_success "helper ($HELPER) erases all matching credentials" '
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user6
+		password=pass1
+		EOF
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user7
+		password=pass1
+		EOF
+		check reject $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		EOF
+		check fill $HELPER <<-\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://[e-mail]'\'':
+		EOF
+	'
+
 	: ${GIT_TEST_LONG_CRED_BUFFER:=1024}
 	# 23 bytes accounts for "wwwauth[]=basic realm=" plus NUL
 	LONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))