credential: improvements to erase in helpers

From: M Hickford <[email protected]>

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.

Fixes for credential-cache and credential-store.

Signed-off-by M Hickford <[email protected]>

---
 Documentation/git-credential.txt   |  4 ++--
 Documentation/gitcredentials.txt   |  2 +-
 builtin/credential-cache--daemon.c | 15 ++++++++------
 builtin/credential-store.c         |  3 ++-
 t/lib-credential.sh                | 33 ++++++++++++++++++++++++++++++
 5 files changed, 47 insertions(+), 10 deletions(-)

Documentation/git-credential.txt +2 -2

index 0e6d9e85ec7..04bfb918de6 100644
@@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential
 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.
+any configured credential helpers, which may erase stored credentials
+matching the description.
 
 If the action is `approve` or `reject`, no output should be emitted.
 

Documentation/gitcredentials.txt +1 -1

index 100f045bb1a..65d652dc40e 100644
@@ -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

builtin/credential-cache--daemon.c +9 -6

index 82f376d3351..5e3a766e42d 100644
@@ -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)
 {
 	struct credential_cache_entry *e;
 
-	e = lookup_credential(c, c->password != NULL);
-	if (e)
-		e->expiration = 0;
+	int i;
+	for (i = 0; i < entries_nr; i++) {
+		e = &entries[i];
+		if (credential_match(c, &e->item, c->password != NULL))
+			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);

builtin/credential-store.c +2 -1

index e0ae028b1c3..85b147e460f 100644
@@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn,
 			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
-				break;
+				if (strcmp(op, "erase"))
+					break;
 			}
 		}
 		else if (other_cb)

t/lib-credential.sh +33 -0

index f7e4e29c5e1..3f4100b6ce2 100644
@@ -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 http path.tld user
 	reject $1 https timeout.tld user
 	reject $1 https sso.tld

@@ -298,6 +300,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://[email protected]'\'':
+		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))

Quoted 2 lines · M Hickford via GitGitGadget

From: M Hickford <[email protected]>

Quoted 4 lines

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.

Perhaps that is a sensible expectation. It is unclear from the above description what happens instead without the "fix", though.

By the way, I do not think your

cc. Jeff King [email protected] cc: Matthew John Cheetham [email protected]

in 0/2 is doing anything; I've manually added them to Cc: of this message and left everything in the patch below, even though I am not commenting on anything there myself, to give them easier reference.

Thanks.

Quoted 148 lines

Fixes for credential-cache and credential-store.

Signed-off-by M Hickford <[email protected]>

---
 Documentation/git-credential.txt   |  4 ++--
 Documentation/gitcredentials.txt   |  2 +-
 builtin/credential-cache--daemon.c | 15 ++++++++------
 builtin/credential-store.c         |  3 ++-
 t/lib-credential.sh                | 33 ++++++++++++++++++++++++++++++
 5 files changed, 47 insertions(+), 10 deletions(-)

Documentation/git-credential.txt +2 -2

index 0e6d9e85ec7..04bfb918de6 100644
@@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential
 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.
+any configured credential helpers, which may erase stored credentials
+matching the description.
 
 If the action is `approve` or `reject`, no output should be emitted.
 

Documentation/gitcredentials.txt +1 -1

index 100f045bb1a..65d652dc40e 100644
@@ -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

builtin/credential-cache--daemon.c +9 -6

index 82f376d3351..5e3a766e42d 100644
@@ -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)
 {
      struct credential_cache_entry *e;
 
-     e = lookup_credential(c, c->password != NULL);
-     if (e)
-             e->expiration = 0;
+     int i;
+     for (i = 0; i < entries_nr; i++) {
+             e = &entries[i];
+             if (credential_match(c, &e->item, c->password != NULL))
+                     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);

builtin/credential-store.c +2 -1

index e0ae028b1c3..85b147e460f 100644
@@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn,
                      found_credential = 1;
                      if (match_cb) {
                              match_cb(&entry);
-                             break;
+                             if (strcmp(op, "erase"))
+                                     break;
                      }
              }
              else if (other_cb)

t/lib-credential.sh +33 -0

index f7e4e29c5e1..3f4100b6ce2 100644
@@ -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 http path.tld user
      reject $1 https timeout.tld user
      reject $1 https sso.tld

@@ -298,6 +300,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://[email protected]'\'':
+             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))

M Hickford (2):
credential: avoid erasing distinct password
credential: erase all matching credentials

 Documentation/git-credential.txt   |  4 +-
 Documentation/gitcredentials.txt   |  2 +-
 builtin/credential-cache--daemon.c | 11 ++++--
 builtin/credential-store.c         | 21 ++++++-----
 credential.c                       | 11 +++---
 credential.h                       |  2 +-
 t/lib-credential.sh                | 59 ++++++++++++++++++++++++++++++
 7 files changed, 88 insertions(+), 22 deletions(-)

base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v2
Pull-Request: https://github.com/git/git/pull/1525

Range-diff vs v1:

 1:  35ee1795bcd = 1:  35ee1795bcd credential: avoid erasing distinct password
 2:  fcdb579263f ! 2:  9b12f17dc7e credential: erase all matching credentials

@@ Metadata
## Commit message ##
credential: erase all matching credentials

- 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.
+ 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.

- Fixes for credential-cache and credential-store.
+ 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 <[email protected]>

From: M Hickford <[email protected]>

Test that credential helpers do not erase a password distinct from the input. Such calls can happen when multiple credential helpers are configured.

Fixes for credential-cache and credential-store.

builtin/credential-cache--daemon.c +2 -2

index 756c5f02aef..82f376d3351 100644
@@ -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)
+static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)
 {
 	int i;
 	for (i = 0; i < entries_nr; i++) {
 		struct credential *e = &entries[i].item;
-		if (credential_match(c, e))
+		if (credential_match(c, e, match_password))
 			return &entries[i];
 	}
 	return NULL;
[...]

builtin/credential-store.c +3 -1

index 30c6ccf56c0..e0ae028b1c3 100644
@@ -13,12 +13,14 @@ static struct lock_file credential_lock;
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
 				  void (*match_cb)(struct credential *),
-				  void (*other_cb)(struct strbuf *))
+				  void (*other_cb)(struct strbuf *),
+				  const char* op)
 {
 	FILE *fh;
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
+	const int match_password = !strcmp(op, "erase") && c->password != NULL;
 
 	fh = fopen(fn, "r");
 	if (!fh) {

builtin/credential-store.c +1 -1

@@ -60,7 +62,7 @@ static void print_line(struct strbuf *buf)
 }
 
 static void rewrite_credential_file(const char *fn, struct credential *c,
-				    struct strbuf *extra)
+				    struct strbuf *extra, const char *op)
 {
 	int timeout_ms = 1000;
 

builtin/credential-store.c +14 -0

@@ -221,6 +222,31 @@ helper_test() {
 		EOF
 	'
 
+	test_expect_success "helper ($HELPER) does not erase a password distinct from input" '
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user5
+		password=pass1
+		EOF
+		check reject $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user5
+		password=pass2
+		EOF
+		check fill $HELPER <<-\EOF

From: M Hickford <[email protected]>

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.

builtin/credential-cache--daemon.c +0 -0

index 82f376d3351..5e3a766e42d 100644
[...]

@@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c)
 {
 	struct credential_cache_entry *e;
 

builtin/credential-store.c +0 -0

index e0ae028b1c3..85b147e460f 100644

builtin/credential-store.c +20 -0

@@ -298,6 +300,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

Quoted 3 lines · M Hickford via GitGitGadget

M Hickford (2):
credential: avoid erasing distinct password
credential: erase all matching credentials

Will queue. Comments by folks with more experience (than me) who have worked on the credential subsystem are very much welcomed.

Quoted 7 lines · M Hickford via GitGitGadget

From: M Hickford <[email protected]>

Test that credential helpers do not erase a password distinct from the input. Such calls can happen when multiple credential helpers are configured.

Fixes for credential-cache and credential-store.

I think this goal makes sense. I missed the "multiple helpers" part of your message at first, and wondered how you would even get such duplicate entries stored in a helper. But the point is that you might have two helpers with two separate passwords.

Which is...weird, I'd think, because we will only ever use one of them. But from past discussions, I guess you're in a situation where you could use some kind of renewable token _or_ possibly a more stable password, via two different helpers. And when the token expires, you want to be able to remove it from a caching helper without asking for the stable password to be dropped.

I don't think it's strictly necessary to spell out the intended use in the commit message. The behavior you're proposing seems like the least-surprising thing to me in general (and it is only because the situation is rare that nobody complained about the existing behavior in the last decade). But it might not hurt to outline the case that we'd expect this to help.

The other thing I'd watch out for is how this behavior would affect non-erase modes. Let's see...

Quoted 21 lines

builtin/credential-cache--daemon.c +2 -2

index 756c5f02aef..82f376d3351 100644
@@ -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)
+static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)
 {
      int i;
      for (i = 0; i < entries_nr; i++) {
              struct credential *e = &entries[i].item;
-             if (credential_match(c, e))
+             if (credential_match(c, e, match_password))
                      return &entries[i];
      }
      return NULL;
[...]

builtin/credential-cache--daemon.c static void serve_one_client(FILE *in, FILE *out) · line 133

Quoted 8 lines

builtin/credential-cache--daemon.c +1 -1

@@ -127,7 +127,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);
+             struct credential_cache_entry *e = lookup_credential(&c, 0);
              if (e) {
                      fprintf(out, "username=%s\n", e->item.username);
                      fprintf(out, "password=%s\n", e->item.password);

OK, for a cache "get" we'll continue not to match the password. This shouldn't trigger in practice, as Git will stop asking helpers once it has a username and password, but it is good to retain the existing behavior here.

builtin/credential-cache--daemon.c static void remove_credential(const struct credential *c) · line 54

Quoted 9 lines

builtin/credential-cache--daemon.c +1 -1

@@ -48,7 +48,7 @@ static void remove_credential(const struct credential *c)
 {
      struct credential_cache_entry *e;
 
-     e = lookup_credential(c);
+     e = lookup_credential(c, c->password != NULL);
      if (e)
              e->expiration = 0;
 }

And this is erase, where we do want to respect it. I actually think passing an unconditional "1" here instead of the NULL check would be OK, as the CHECK() macro in credential_match() treats NULL as "always match". But I am also OK with making that logic explicit here.

This made me wonder how "store" works, since it's not touched here. It's implemented as remove_credential(), plus an unconditional allocation via cache_credential().

It seems like overwriting would be broken, then, wouldn't it? If I

store:

https://user:[email protected]

and then try to store:

https://user:[email protected]

then the call to remove_credential() will see a "struct credential" with the password set to "pass2". And because it always passes match_password, it will fail to remove it, and we'll end up with two.

We should be able to test that pretty easily, like this:

echo url=https://user:[email protected] | git credential-cache store echo url=https://user:[email protected] | git credential-cache store echo url=https://example.com | git credential-cache get

Before your patch that will return "pass1". After, it returns "pass2", because the old credential is left in place. I think you'd want something like:

builtin/credential-cache--daemon.c +4 -4

index 82f376d335..f64dd21d33 100644
@@ -44,11 +44,11 @@ static struct credential_cache_entry *lookup_credential(const struct credential
        return NULL;
 }
 
-static void remove_credential(const struct credential *c)
+static void remove_credential(const struct credential *c, int match_password)
 {
        struct credential_cache_entry *e;
 
-       e = lookup_credential(c, c->password != NULL);
+       e = lookup_credential(c, match_password);
        if (e)
                e->expiration = 0;
 }

@@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)
                exit(0);
        }
        else if (!strcmp(action.buf, "erase"))
-               remove_credential(&c);
+               remove_credential(&c, 1);
        else if (!strcmp(action.buf, "store")) {
                if (timeout < 0)
                        warning("cache client didn't specify a timeout");
                else if (!c.username || !c.password)
                        warning("cache client gave us a partial credential");
                else {
-                       remove_credential(&c);
+                       remove_credential(&c, 0);
                        cache_credential(&c, timeout);
                }
        }

Unless your goal really is to store two entries that differ only in their passwords within a single cache. That seems weird, though. Again, it might help if your use case was fleshed out a bit more.

Quoted 21 lines

builtin/credential-store.c +3 -1

index 30c6ccf56c0..e0ae028b1c3 100644
@@ -13,12 +13,14 @@ static struct lock_file credential_lock;
 static int parse_credential_file(const char *fn,
                                struct credential *c,
                                void (*match_cb)(struct credential *),
-                               void (*other_cb)(struct strbuf *))
+                               void (*other_cb)(struct strbuf *),
+                               const char* op)
 {
      FILE *fh;
      struct strbuf line = STRBUF_INIT;
      struct credential entry = CREDENTIAL_INIT;
      int found_credential = 0;
+     const int match_password = !strcmp(op, "erase") && c->password != NULL;
 
      fh = fopen(fn, "r");
      if (!fh) {

builtin/credential-store.c static int parse_credential_file(const char *fn, · line 38

Quoted 10 lines

builtin/credential-store.c +2 -2

@@ -29,8 +31,8 @@ static int parse_credential_file(const char *fn,
 
      while (strbuf_getline_lf(&line, fh) != EOF) {
              if (!credential_from_url_gently(&entry, line.buf, 1) &&
-                 entry.username && entry.password &&
-                 credential_match(c, &entry)) {
+                     entry.username && entry.password &&
+                     credential_match(c, &entry, match_password)) {
                      found_credential = 1;
                      if (match_cb) {
                              match_cb(&entry);

OK, so for credential-store we only kick in the new behavior for "erase". So it would not have the same quirk that I mentioned above.

I do think your logic could likewise here be:

  const int match_password = !strcmp(op, "erase");

because credential_match() handles the NULL case already.

(It's also unusual for us to declare non-parameter variables as "const"; it's not wrong to do so, so it's really just a style thing, but there's not much benefit).

Quoted 9 lines

builtin/credential-store.c +1 -1

@@ -60,7 +62,7 @@ static void print_line(struct strbuf *buf)
 }
 
 static void rewrite_credential_file(const char *fn, struct credential *c,
-                                 struct strbuf *extra)
+                                 struct strbuf *extra, const char *op)
 {
      int timeout_ms = 1000;
 

builtin/credential-store.c static void rewrite_credential_file(const char *fn, struct credential *c, · line 77

Quoted 9 lines

builtin/credential-store.c +1 -1

@@ -69,7 +71,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
              die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms);
      if (extra)
              print_line(extra);
-     parse_credential_file(fn, c, NULL, print_line);
+     parse_credential_file(fn, c, NULL, print_line, op);
      if (commit_lock_file(&credential_lock) < 0)
              die_errno("unable to write credential store");
 }

If we have to plumb through this "op" string, I wonder if it would be more flexible and robust to just pass along a match_password flag. That's generally a bit more robust than string matching (where a misspelling in one string would not be caught by the compiler, unlike a boolean variable).

Quoted 19 lines

builtin/credential-store.c +14 -0

@@ -221,6 +222,31 @@ helper_test() {
              EOF
      '
 
+     test_expect_success "helper ($HELPER) does not erase a password distinct from input" '
+             check approve $HELPER <<-\EOF &&
+             protocol=https
+             host=example.com
+             username=user5
+             password=pass1
+             EOF
+             check reject $HELPER <<-\EOF &&
+             protocol=https
+             host=example.com
+             username=user5
+             password=pass2
+             EOF
+             check fill $HELPER <<-\EOF

builtin/credential-store.c helper_test() { · line 248

Quoted 10 lines

builtin/credential-store.c +10 -0

+             protocol=https
+             host=example.com
+             username=user5
+             --
+             protocol=https
+             host=example.com
+             username=user5
+             password=pass1
+             EOF
+     '

The test makes sense. We are not using multiple helpers, but the behavior can be checked using a single helper, since the point is that it should be independent of what any other helper is doing.

-Peff

Quoted 8 lines · M Hickford via GitGitGadget

From: M Hickford <[email protected]>

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.

Quoted 7 lines

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.

I think this was how it was intended to work all along, but I agree that credential-cache does not behave this way. I think credential-store already works, though.

Documentation/git-credential.txt to any configured credential helpers, which may store the credential · line 42

Quoted 12 lines

Documentation/git-credential.txt +2 -2

index 0e6d9e85ec7..04bfb918de6 100644
@@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential
 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.
+any configured credential helpers, which may erase stored credentials
+matching the description.

I read the existing documentation as claiming to delete all such matching credentials, but I guess it is ambiguous (and I did not look, but it is almost a certainty that I wrote the original ;) ).

Saying "credentials" in the plural makes it more clear we are getting all of them. But losing "any" makes it less clear to me. Maybe:

...which may erase any stored credentials matching the description.

is the best of both?

Documentation/gitcredentials.txt appended to its command line, which is one of: · line 263

Quoted 11 lines

Documentation/gitcredentials.txt +1 -1

index 100f045bb1a..65d652dc40e 100644
@@ -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.

This one seems like a strict improvement in ambiguity.

Quoted 9 lines

builtin/credential-cache--daemon.c +0 -0

index 82f376d3351..5e3a766e42d 100644
[...]

@@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c)
 {
      struct credential_cache_entry *e;
 

builtin/credential-cache--daemon.c static void remove_credential(const struct credential *c) · line 57

Quoted 10 lines

builtin/credential-cache--daemon.c +6 -3

-     e = lookup_credential(c, c->password != NULL);
-     if (e)
-             e->expiration = 0;
+     int i;
+     for (i = 0; i < entries_nr; i++) {
+             e = &entries[i];
+             if (credential_match(c, &e->item, c->password != NULL))
+                     e->expiration = 0;
+     }
 }

OK, so now instead of looking up a single credential, we loop and cover them all. That makes perfect sense. It is a little funny that we undo the extra parameter to lookup_credential() that was added in the previous commit. I wonder if reversing the order of the patches would make it simpler, but it is probably not a big deal either way.

Quoted 5 lines

builtin/credential-store.c +0 -0

index e0ae028b1c3..85b147e460f 100644

builtin/credential-store.c static int parse_credential_file(const char *fn, · line 43

Quoted 9 lines

builtin/credential-store.c +2 -1

@@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn,
                      found_credential = 1;
                      if (match_cb) {
                              match_cb(&entry);
-                             break;
+                             if (strcmp(op, "erase"))
+                                     break;
                      }
              }
              else if (other_cb)

I think this hunk does nothing. When the op is "erase", we do not pass a match_cb at all, so this break would never trigger (and indeed, it would be disastrous if it did, because we would stop copying entries from the old file to the new, losing everything after the matched entry).

And I think if you revert this, your new test still passes.

Quoted 25 lines

builtin/credential-store.c +20 -0

@@ -298,6 +300,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

builtin/credential-store.c helper_test() { · line 332

Quoted 10 lines

builtin/credential-store.c +10 -0

+             --
+             protocol=https
+             host=example.com
+             username=askpass-username
+             password=askpass-password
+             --
+             askpass: Username for '\''https://example.com'\'':
+             askpass: Password for '\''https://[email protected]'\'':
+             EOF
+     '

The test makes sense. We add two, delete them both, and then check that nothing is left to serve.

-Peff

builtin/credential-cache--daemon.c +1 -1

@@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)
 		exit(0);
 	}
 	else if (!strcmp(action.buf, "erase"))
-		remove_credential(&c);
+		remove_credential(&c, 1);
 	else if (!strcmp(action.buf, "store")) {

credential.c +1 -1

index 023b59d5711..9157ff0865e 100644
@@ -33,13 +33,14 @@ void credential_clear(struct credential *c)
 }
 
 int credential_match(const struct credential *want,
-		     const struct credential *have)
+		     const struct credential *have, int match_password)
 {
 #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))

From: M Hickford <[email protected]>

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.

M Hickford (2):
credential: avoid erasing distinct password
credential: erase all matching credentials

 Documentation/git-credential.txt   |   2 +-
 Documentation/gitcredentials.txt   |   2 +-
 builtin/credential-cache--daemon.c |  17 +++--
 builtin/credential-store.c         |  15 +++--
 credential.c                       |   7 +-
 credential.h                       |   2 +-
 t/lib-credential.sh                | 103 +++++++++++++++++++++++++++++
 7 files changed, 128 insertions(+), 20 deletions(-)

base-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v4
Pull-Request: https://github.com/git/git/pull/1525

Range-diff vs v3:

1: df3c8a15bf8 ! 1: 91d4b04b5e1 credential: avoid erasing distinct password

     @@ builtin/credential-store.c: static struct lock_file credential_lock;
       	FILE *fh;
       	struct strbuf line = STRBUF_INIT;
      @@ builtin/credential-store.c: static int parse_credential_file(const char *fn,
     - 
       	while (strbuf_getline_lf(&line, fh) != EOF) {
       		if (!credential_from_url_gently(&entry, line.buf, 1) &&
     --		    entry.username && entry.password &&
     + 		    entry.username && entry.password &&
      -		    credential_match(c, &entry)) {
     -+			entry.username && entry.password &&
     -+			credential_match(c, &entry, match_password)) {
     ++		    credential_match(c, &entry, match_password)) {
       			found_credential = 1;
       			if (match_cb) {
       				match_cb(&entry);
     @@ credential.c: void credential_clear(struct credential *c)
       {
       #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
       	return CHECK(protocol) &&
     --	       CHECK(host) &&
     --	       CHECK(path) &&
     + 	       CHECK(host) &&
     + 	       CHECK(path) &&
      -	       CHECK(username);
     -+		CHECK(host) &&
     -+		CHECK(path) &&
     -+		CHECK(username) &&
     -+		(!match_password || CHECK(password));
     ++	       CHECK(username) &&
     ++	       (!match_password || CHECK(password));
       #undef CHECK
       }
       
     @@ t/lib-credential.sh: helper_test_clean() {
       	reject $1 https example.com user1
       	reject $1 https example.com user2
       	reject $1 https example.com user4
     -+	reject $1 https example.com user5
     -+	reject $1 https example.com user8
     ++	reject $1 https example.com user-distinct-pass
     ++	reject $1 https example.com user-overwrite
       	reject $1 http path.tld user
       	reject $1 https timeout.tld user
       	reject $1 https sso.tld
     @@ t/lib-credential.sh: helper_test() {
      +		check approve $HELPER <<-\EOF &&
      +		protocol=https
      +		host=example.com
     -+		username=user8
     ++		username=user-overwrite
      +		password=pass1
      +		EOF
      +		check approve $HELPER <<-\EOF &&
      +		protocol=https
      +		host=example.com
     -+		username=user8
     ++		username=user-overwrite
      +		password=pass2
      +		EOF
      +		check fill $HELPER <<-\EOF &&
      +		protocol=https
      +		host=example.com
     -+		username=user8
     ++		username=user-overwrite
      +		--
      +		protocol=https
      +		host=example.com
     -+		username=user8
     ++		username=user-overwrite
      +		password=pass2
      +		EOF
      +		check reject $HELPER <<-\EOF &&
      +		protocol=https
      +		host=example.com
     -+		username=user8
     ++		username=user-overwrite
      +		password=pass2
      +		EOF
      +		check fill $HELPER <<-\EOF
      +		protocol=https
      +		host=example.com
     -+		username=user8
     ++		username=user-overwrite
      +		--
      +		protocol=https
      +		host=example.com
     -+		username=user8
     ++		username=user-overwrite
      +		password=askpass-password
      +		--
     -+		askpass: Password for '\''https://[email protected]'\'':
     ++		askpass: Password for '\''https://[email protected]'\'':
      +		EOF
      +	'
      +
     @@ t/lib-credential.sh: helper_test() {
      +		check approve $HELPER <<-\EOF &&
      +		protocol=https
      +		host=example.com
     -+		username=user5
     ++		username=user-distinct-pass
      +		password=pass1
      +		EOF
      +		check reject $HELPER <<-\EOF &&
      +		protocol=https
      +		host=example.com
     -+		username=user5
     ++		username=user-distinct-pass
      +		password=pass2
      +		EOF
      +		check fill $HELPER <<-\EOF
      +		protocol=https
      +		host=example.com
     -+		username=user5
     ++		username=user-distinct-pass
      +		--
      +		protocol=https
      +		host=example.com
     -+		username=user5
     ++		username=user-distinct-pass
      +		password=pass1
      +		EOF
      +	'

2: e06d80e99a0 ! 2: 42f41b28e6e credential: erase all matching credentials
@@ Commit message

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.
+ Some helpers (such as credential-store and credential-libsecret) delete
+ all matching credentials, others (such as credential-cache) delete at
+ most one matching credential.

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

- Fix credential-cache and credential-store.
+ Fix credential-cache.

Signed-off-by M Hickford <[email protected]>

@@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE
fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password);

- ## builtin/credential-store.c ##
-@@ builtin/credential-store.c: static int parse_credential_file(const char *fn,
- found_credential = 1;
- if (match_cb) {
- match_cb(&entry);
-- break;
- }
- }
- else if (other_cb)
-
## t/lib-credential.sh ##
@@ t/lib-credential.sh: 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 https example.com user-distinct-pass
+ reject $1 https example.com user-overwrite
++ reject $1 https example.com user-erase1
++ reject $1 https example.com user-erase2
reject $1 http path.tld user
reject $1 https timeout.tld user
+ reject $1 https sso.tld
@@ t/lib-credential.sh: helper_test() {
EOF
'
@@ t/lib-credential.sh: helper_test() {
+ check approve $HELPER <<-\EOF &&
+ protocol=https
+ host=example.com
-+ username=user6
++ username=user-erase1
+ password=pass1
+ EOF
+ check approve $HELPER <<-\EOF &&
+ protocol=https
+ host=example.com
-+ username=user7
++ username=user-erase2
+ password=pass1
+ EOF
+ check reject $HELPER <<-\EOF &&

Quoted 4 lines · M Hickford via GitGitGadget

M Hickford (2):
credential: avoid erasing distinct password
credential: erase all matching credentials

Quoted 8 lines

 Documentation/git-credential.txt   |   2 +-
 Documentation/gitcredentials.txt   |   2 +-
 builtin/credential-cache--daemon.c |  17 +++--
 builtin/credential-store.c         |  15 +++--
 credential.c                       |   7 +-
 credential.h                       |   2 +-
 t/lib-credential.sh                | 103 +++++++++++++++++++++++++++++
 7 files changed, 128 insertions(+), 20 deletions(-)

It is helpful to reviewers to describe/summarize, in your own words, what changed since the previous version, in the cover letter.

The range-diff generated for the versions can serve as a good supporting material, and it would help you while writing that summary, but not a substitute for the summary.

Quoted 18 lines · Junio C Hamano

Quoted 4 lines · M Hickford via GitGitGadget

M Hickford (2):
credential: avoid erasing distinct password
credential: erase all matching credentials

Quoted 8 lines

 Documentation/git-credential.txt   |   2 +-
 Documentation/gitcredentials.txt   |   2 +-
 builtin/credential-cache--daemon.c |  17 +++--
 builtin/credential-store.c         |  15 +++--
 credential.c                       |   7 +-
 credential.h                       |   2 +-
 t/lib-credential.sh                | 103 +++++++++++++++++++++++++++++
 7 files changed, 128 insertions(+), 20 deletions(-)

It is helpful to reviewers to describe/summarize, in your own words, what changed since the previous version, in the cover letter.

Quoted 3 lines

The range-diff generated for the versions can serve as a good supporting material, and it would help you while writing that summary, but not a substitute for the summary.

Yeah, I agree that would have made reviewing much easier. :)

That said, I just re-reviewed the patches themselves, and everything now looks good to me.

That said, I just re-reviewed the patches themselves, and everything now looks good to me.