[PATCH] credential-cache: respect request capabilities

[PATCH] credential-cache: respect request capabilities

[M Hickford via GitGitGadget]

From: M Hickford <mirth.hickford@gmail.com>

Previously, credential-cache responded with capability[]=authtype
regardless of request.

The capabilities in a credential helper response should be a subset of
the capabilities in the request.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential-cache: respect request capabilities

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1842

 builtin/credential-cache--daemon.c | 11 +++++------
 t/lib-credential.sh                | 15 +++++++++++++++
 t/t0303-credential-external.sh     |  1 +
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index bc22f5c6d24..692216cf83c 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)
 	else if (!strcmp(action.buf, "get")) {
 		struct credential_cache_entry *e = lookup_credential(&c);
 		if (e) {
-			e->item.capa_authtype.request_initial = 1;
-			e->item.capa_authtype.request_helper = 1;
-
-			fprintf(out, "capability[]=authtype\n");
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
+				fprintf(out, "capability[]=authtype\n");
+			}
 			if (e->item.username)
 				fprintf(out, "username=%s\n", e->item.username);
 			if (e->item.password)
 				fprintf(out, "password=%s\n", e->item.password);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
 				fprintf(out, "authtype=%s\n", e->item.authtype);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
 				fprintf(out, "credential=%s\n", e->item.credential);
 			if (e->item.password_expiry_utc != TIME_MAX)
 				fprintf(out, "password_expiry_utc=%"PRItime"\n",
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 58b9c740605..fe170b513fd 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -566,6 +566,21 @@ helper_test_authtype() {
 		EOF
 	'
 
+	test_expect_success "helper ($HELPER) does not get authtype and credential without authtype capability" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=git.example.com
+		--
+		protocol=https
+		host=git.example.com
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''https://git.example.com'\'':
+		askpass: Password for '\''https://askpass-username@git.example.com'\'':
+		EOF
+	'
+
 	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
 		check approve $HELPER <<-\EOF
 		capability[]=authtype
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index 8aadbe86c45..437eae5002a 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -63,6 +63,7 @@ helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
 helper_test "$GIT_TEST_CREDENTIAL_HELPER"
 helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
 helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
+helper_test_authtype "$GIT_TEST_CREDENTIAL_HELPER"
 
 if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
 	say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"

base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
-- 
gitgitgadget

[PATCH v2] credential-cache: respect request capabilities

[M Hickford via GitGitGadget]

From: M Hickford <mirth.hickford@gmail.com>

Previously, credential-cache responded with capability[]=authtype
regardless of request.

The capabilities in a credential helper response should be a subset of
the capabilities in the request.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential-cache: respect request capabilities

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1842

Range-diff vs v1:

 1:  9197941029f ! 1:  696780d4782 credential-cache: respect request capabilities
     @@ t/lib-credential.sh: helper_test_authtype() {
       		EOF
       	'
       
     -+	test_expect_success "helper ($HELPER) does not get authtype and credential without authtype capability" '
     ++	test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
      +		check fill $HELPER <<-\EOF
      +		protocol=https
      +		host=git.example.com
     @@ t/lib-credential.sh: helper_test_authtype() {
       	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
       		check approve $HELPER <<-\EOF
       		capability[]=authtype
     -
     - ## t/t0303-credential-external.sh ##
     -@@ t/t0303-credential-external.sh: helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
     - helper_test "$GIT_TEST_CREDENTIAL_HELPER"
     - helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
     - helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
     -+helper_test_authtype "$GIT_TEST_CREDENTIAL_HELPER"
     - 
     - if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
     - 	say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"


 builtin/credential-cache--daemon.c | 11 +++++------
 t/lib-credential.sh                | 15 +++++++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index bc22f5c6d24..692216cf83c 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)
 	else if (!strcmp(action.buf, "get")) {
 		struct credential_cache_entry *e = lookup_credential(&c);
 		if (e) {
-			e->item.capa_authtype.request_initial = 1;
-			e->item.capa_authtype.request_helper = 1;
-
-			fprintf(out, "capability[]=authtype\n");
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
+				fprintf(out, "capability[]=authtype\n");
+			}
 			if (e->item.username)
 				fprintf(out, "username=%s\n", e->item.username);
 			if (e->item.password)
 				fprintf(out, "password=%s\n", e->item.password);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
 				fprintf(out, "authtype=%s\n", e->item.authtype);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
 				fprintf(out, "credential=%s\n", e->item.credential);
 			if (e->item.password_expiry_utc != TIME_MAX)
 				fprintf(out, "password_expiry_utc=%"PRItime"\n",
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 58b9c740605..324ecc792d5 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -566,6 +566,21 @@ helper_test_authtype() {
 		EOF
 	'
 
+	test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=git.example.com
+		--
+		protocol=https
+		host=git.example.com
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''https://git.example.com'\'':
+		askpass: Password for '\''https://askpass-username@git.example.com'\'':
+		EOF
+	'
+
 	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
 		check approve $HELPER <<-\EOF
 		capability[]=authtype

base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
-- 
gitgitgadget

Re: [PATCH v2] credential-cache: respect request capabilities

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3144 bytes --]

On 2025-01-06 at 19:52:11, M Hickford via GitGitGadget wrote:
> From: M Hickford <mirth.hickford@gmail.com>
> 
> Previously, credential-cache responded with capability[]=authtype
> regardless of request.

That's the correct behaviour.

> The capabilities in a credential helper response should be a subset of
> the capabilities in the request.

No, it should not.  Otherwise, it's impossible for Git to know whether
the helper does or does not support the capability.  We rely on that
information to correctly pass data back when saving data.

> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index bc22f5c6d24..692216cf83c 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)
>  	else if (!strcmp(action.buf, "get")) {
>  		struct credential_cache_entry *e = lookup_credential(&c);
>  		if (e) {
> -			e->item.capa_authtype.request_initial = 1;
> -			e->item.capa_authtype.request_helper = 1;
> -
> -			fprintf(out, "capability[]=authtype\n");
> +			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
> +				fprintf(out, "capability[]=authtype\n");
> +			}

This part is not correct.

>  			if (e->item.username)
>  				fprintf(out, "username=%s\n", e->item.username);
>  			if (e->item.password)
>  				fprintf(out, "password=%s\n", e->item.password);
> -			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
> +			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
>  				fprintf(out, "authtype=%s\n", e->item.authtype);
> -			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
> +			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)

This part may very well be correct.

>  				fprintf(out, "credential=%s\n", e->item.credential);
>  			if (e->item.password_expiry_utc != TIME_MAX)
>  				fprintf(out, "password_expiry_utc=%"PRItime"\n",
> diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> index 58b9c740605..324ecc792d5 100644
> --- a/t/lib-credential.sh
> +++ b/t/lib-credential.sh
> @@ -566,6 +566,21 @@ helper_test_authtype() {
>  		EOF
>  	'
>  
> +	test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
> +		check fill $HELPER <<-\EOF
> +		protocol=https
> +		host=git.example.com
> +		--
> +		protocol=https
> +		host=git.example.com
> +		username=askpass-username
> +		password=askpass-password
> +		--
> +		askpass: Username for '\''https://git.example.com'\'':
> +		askpass: Password for '\''https://askpass-username@git.example.com'\'':
> +		EOF
> +	'
> +
>  	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
>  		check approve $HELPER <<-\EOF
>  		capability[]=authtype
> 
> base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
> -- 
> gitgitgadget

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2] credential-cache: respect request capabilities

[M Hickford]

On Mon, 6 Jan 2025 at 22:32, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2025-01-06 at 19:52:11, M Hickford via GitGitGadget wrote:
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > Previously, credential-cache responded with capability[]=authtype
> > regardless of request.
>
> That's the correct behaviour.
>
> > The capabilities in a credential helper response should be a subset of
> > the capabilities in the request.
>
> No, it should not.  Otherwise, it's impossible for Git to know whether
> the helper does or does not support the capability.  We rely on that
> information to correctly pass data back when saving data.
>
> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > index bc22f5c6d24..692216cf83c 100644
> > --- a/builtin/credential-cache--daemon.c
> > +++ b/builtin/credential-cache--daemon.c
> > @@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)
> >       else if (!strcmp(action.buf, "get")) {
> >               struct credential_cache_entry *e = lookup_credential(&c);
> >               if (e) {
> > -                     e->item.capa_authtype.request_initial = 1;
> > -                     e->item.capa_authtype.request_helper = 1;
> > -
> > -                     fprintf(out, "capability[]=authtype\n");
> > +                     if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
> > +                             fprintf(out, "capability[]=authtype\n");
> > +                     }
>
> This part is not correct.

Thanks for the review. I'll revert this part and amend the commit message.

>
> >                       if (e->item.username)
> >                               fprintf(out, "username=%s\n", e->item.username);
> >                       if (e->item.password)
> >                               fprintf(out, "password=%s\n", e->item.password);
> > -                     if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
> > +                     if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
> >                               fprintf(out, "authtype=%s\n", e->item.authtype);
> > -                     if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
> > +                     if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
>
> This part may very well be correct.
>
> >                               fprintf(out, "credential=%s\n", e->item.credential);
> >                       if (e->item.password_expiry_utc != TIME_MAX)
> >                               fprintf(out, "password_expiry_utc=%"PRItime"\n",
> > diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> > index 58b9c740605..324ecc792d5 100644
> > --- a/t/lib-credential.sh
> > +++ b/t/lib-credential.sh
> > @@ -566,6 +566,21 @@ helper_test_authtype() {
> >               EOF
> >       '
> >
> > +     test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
> > +             check fill $HELPER <<-\EOF
> > +             protocol=https
> > +             host=git.example.com
> > +             --
> > +             protocol=https
> > +             host=git.example.com
> > +             username=askpass-username
> > +             password=askpass-password
> > +             --
> > +             askpass: Username for '\''https://git.example.com'\'':
> > +             askpass: Password for '\''https://askpass-username@git.example.com'\'':
> > +             EOF
> > +     '
> > +
> >       test_expect_success "helper ($HELPER) stores authtype and credential with username" '
> >               check approve $HELPER <<-\EOF
> >               capability[]=authtype
> >
> > base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
> > --
> > gitgitgadget
>
> --
> brian m. carlson (they/them or he/him)
> Toronto, Ontario, CA

Re: [PATCH v2] credential-cache: respect request capabilities

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

On 2025-01-06 at 22:57:06, M Hickford wrote:
> On Mon, 6 Jan 2025 at 22:32, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> >
> > On 2025-01-06 at 19:52:11, M Hickford via GitGitGadget wrote:
> > > From: M Hickford <mirth.hickford@gmail.com>
> > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > > index bc22f5c6d24..692216cf83c 100644
> > > --- a/builtin/credential-cache--daemon.c
> > > +++ b/builtin/credential-cache--daemon.c
> > > @@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)
> > >       else if (!strcmp(action.buf, "get")) {
> > >               struct credential_cache_entry *e = lookup_credential(&c);
> > >               if (e) {
> > > -                     e->item.capa_authtype.request_initial = 1;
> > > -                     e->item.capa_authtype.request_helper = 1;
> > > -
> > > -                     fprintf(out, "capability[]=authtype\n");
> > > +                     if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
> > > +                             fprintf(out, "capability[]=authtype\n");
> > > +                     }
> >
> > This part is not correct.
> 
> Thanks for the review. I'll revert this part and amend the commit message.

I applied this without that change and it does still pass the test,
which I think is good and shows that can be omitted.  If I have some
time, I may send a follow-up patch to add some additional tests.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v3] credential-cache: respect request capabilities

[M Hickford via GitGitGadget]

From: M Hickford <mirth.hickford@gmail.com>

Previously, credential-cache populated authtype regardless of request.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential-cache: respect request capabilities

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1842

Range-diff vs v2:

 1:  696780d4782 ! 1:  e9851c5c4ac credential-cache: respect request capabilities
     @@ Metadata
       ## Commit message ##
          credential-cache: respect request capabilities
      
     -    Previously, credential-cache responded with capability[]=authtype
     -    regardless of request.
     -
     -    The capabilities in a credential helper response should be a subset of
     -    the capabilities in the request.
     +    Previously, credential-cache populated authtype regardless of request.
      
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      
       ## builtin/credential-cache--daemon.c ##
      @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE *out)
     - 	else if (!strcmp(action.buf, "get")) {
     - 		struct credential_cache_entry *e = lookup_credential(&c);
     - 		if (e) {
     --			e->item.capa_authtype.request_initial = 1;
     --			e->item.capa_authtype.request_helper = 1;
     --
     --			fprintf(out, "capability[]=authtype\n");
     -+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
     -+				fprintf(out, "capability[]=authtype\n");
     -+			}
     - 			if (e->item.username)
       				fprintf(out, "username=%s\n", e->item.username);
       			if (e->item.password)
       				fprintf(out, "password=%s\n", e->item.password);
     @@ t/lib-credential.sh: helper_test_authtype() {
       		EOF
       	'
       
     -+	test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
     ++	test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" '
      +		check fill $HELPER <<-\EOF
      +		protocol=https
      +		host=git.example.com
      +		--
     ++		capability[]=authtype
      +		protocol=https
      +		host=git.example.com
      +		username=askpass-username


 builtin/credential-cache--daemon.c |  4 ++--
 t/lib-credential.sh                | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index bc22f5c6d24..e707618e743 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)
 				fprintf(out, "username=%s\n", e->item.username);
 			if (e->item.password)
 				fprintf(out, "password=%s\n", e->item.password);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
 				fprintf(out, "authtype=%s\n", e->item.authtype);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
 				fprintf(out, "credential=%s\n", e->item.credential);
 			if (e->item.password_expiry_utc != TIME_MAX)
 				fprintf(out, "password_expiry_utc=%"PRItime"\n",
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 58b9c740605..8da0afe9395 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -566,6 +566,22 @@ helper_test_authtype() {
 		EOF
 	'
 
+	test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=git.example.com
+		--
+		capability[]=authtype
+		protocol=https
+		host=git.example.com
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''https://git.example.com'\'':
+		askpass: Password for '\''https://askpass-username@git.example.com'\'':
+		EOF
+	'
+
 	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
 		check approve $HELPER <<-\EOF
 		capability[]=authtype

base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
-- 
gitgitgadget

[PATCH v4] credential-cache: respect request capabilities

[M Hickford via GitGitGadget]

From: M Hickford <mirth.hickford@gmail.com>

Previously, credential-cache populated authtype regardless of request.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential-cache: respect request capabilities
    
    CC: sandals@crustytoothpaste.net
    
    Patch v4 fixes test

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1842

Range-diff vs v3:

 1:  e9851c5c4ac ! 1:  23942f9fa47 credential-cache: respect request capabilities
     @@ t/lib-credential.sh: helper_test_authtype() {
      +		protocol=https
      +		host=git.example.com
      +		--
     -+		capability[]=authtype
      +		protocol=https
      +		host=git.example.com
      +		username=askpass-username


 builtin/credential-cache--daemon.c |  4 ++--
 t/lib-credential.sh                | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index bc22f5c6d24..e707618e743 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)
 				fprintf(out, "username=%s\n", e->item.username);
 			if (e->item.password)
 				fprintf(out, "password=%s\n", e->item.password);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
 				fprintf(out, "authtype=%s\n", e->item.authtype);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
 				fprintf(out, "credential=%s\n", e->item.credential);
 			if (e->item.password_expiry_utc != TIME_MAX)
 				fprintf(out, "password_expiry_utc=%"PRItime"\n",
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 58b9c740605..cc6bf9aa5f3 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -566,6 +566,21 @@ helper_test_authtype() {
 		EOF
 	'
 
+	test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=git.example.com
+		--
+		protocol=https
+		host=git.example.com
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''https://git.example.com'\'':
+		askpass: Password for '\''https://askpass-username@git.example.com'\'':
+		EOF
+	'
+
 	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
 		check approve $HELPER <<-\EOF
 		capability[]=authtype

base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
-- 
gitgitgadget

Re: [PATCH v4] credential-cache: respect request capabilities

[Junio C Hamano]

"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: M Hickford <mirth.hickford@gmail.com>
>
> Previously, credential-cache populated authtype regardless of request.

OK, that may be a correct statement of the fact, but it does not
tell readers any of the following:

 - If it is a bad thing to populate authtype regardless of request,
   and if so why?

 - What is the (negative) consequence of doing so, if any?  What
   breaks because it populates authtype regardless of request?

 - What is the remedy?  Instead of unconditionally populating the
   authtype, how does the new code decide when to populate it and
   with what value?

 - Can there be downsides of fixing this?  Are there use cases where
   this unconditional population of authtype was relied upon?

 - Where did the bug come from and what is its fix?  We used to look
   at OP_HELPER to decide when to emit authtype, but the updated
   code checks OP_RESPONSE, which readers can see in the patch.

   It would be nice if the proposed log message helped them by
   briefly explaining their differences, for example.

which would help future "git log" readers what this fix was about.

Will queue for now, but the log message would want to be a bit more
helpful to the readers.

Thanks.

> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
>     credential-cache: respect request capabilities
>     
>     CC: sandals@crustytoothpaste.net
>     
>     Patch v4 fixes test
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1842
>
> Range-diff vs v3:
>
>  1:  e9851c5c4ac ! 1:  23942f9fa47 credential-cache: respect request capabilities
>      @@ t/lib-credential.sh: helper_test_authtype() {
>       +		protocol=https
>       +		host=git.example.com
>       +		--
>      -+		capability[]=authtype
>       +		protocol=https
>       +		host=git.example.com
>       +		username=askpass-username
>
>
>  builtin/credential-cache--daemon.c |  4 ++--
>  t/lib-credential.sh                | 15 +++++++++++++++
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index bc22f5c6d24..e707618e743 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)
>  				fprintf(out, "username=%s\n", e->item.username);
>  			if (e->item.password)
>  				fprintf(out, "password=%s\n", e->item.password);
> -			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
> +			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
>  				fprintf(out, "authtype=%s\n", e->item.authtype);
> -			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
> +			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
>  				fprintf(out, "credential=%s\n", e->item.credential);
>  			if (e->item.password_expiry_utc != TIME_MAX)
>  				fprintf(out, "password_expiry_utc=%"PRItime"\n",
> diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> index 58b9c740605..cc6bf9aa5f3 100644
> --- a/t/lib-credential.sh
> +++ b/t/lib-credential.sh
> @@ -566,6 +566,21 @@ helper_test_authtype() {
>  		EOF
>  	'
>  
> +	test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" '
> +		check fill $HELPER <<-\EOF
> +		protocol=https
> +		host=git.example.com
> +		--
> +		protocol=https
> +		host=git.example.com
> +		username=askpass-username
> +		password=askpass-password
> +		--
> +		askpass: Username for '\''https://git.example.com'\'':
> +		askpass: Password for '\''https://askpass-username@git.example.com'\'':
> +		EOF
> +	'
> +
>  	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
>  		check approve $HELPER <<-\EOF
>  		capability[]=authtype
>
> base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f

[PATCH v5] credential-cache: respect authtype capability

[M Hickford via GitGitGadget]

From: M Hickford <mirth.hickford@gmail.com>

Previously, credential-cache populated authtype regardless whether
"get" request had authtype capability. As documented in
git-credential.txt, authtype "should not be sent unless the appropriate
capability ... is provided".

Add test. Without this change, the test failed because "credential fill"
printed an incomplete credential with only protocol and host attributes
(the unexpected authtype attribute was discarded by credential.c).

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential-cache: respect request capabilities
    
    CC: sandals@crustytoothpaste.net CC: gitster@pobox.com
    
    Patch v5 adds details to the commit message

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1842

Range-diff vs v4:

 1:  23942f9fa47 ! 1:  db575d9d116 credential-cache: respect request capabilities
     @@ Metadata
      Author: M Hickford <mirth.hickford@gmail.com>
      
       ## Commit message ##
     -    credential-cache: respect request capabilities
     +    credential-cache: respect authtype capability
      
     -    Previously, credential-cache populated authtype regardless of request.
     +    Previously, credential-cache populated authtype regardless whether
     +    "get" request had authtype capability. As documented in
     +    git-credential.txt, authtype "should not be sent unless the appropriate
     +    capability ... is provided".
     +
     +    Add test. Without this change, the test failed because "credential fill"
     +    printed an incomplete credential with only protocol and host attributes
     +    (the unexpected authtype attribute was discarded by credential.c).
      
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      


 builtin/credential-cache--daemon.c |  4 ++--
 t/lib-credential.sh                | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index bc22f5c6d24..e707618e743 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)
 				fprintf(out, "username=%s\n", e->item.username);
 			if (e->item.password)
 				fprintf(out, "password=%s\n", e->item.password);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
 				fprintf(out, "authtype=%s\n", e->item.authtype);
-			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
+			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
 				fprintf(out, "credential=%s\n", e->item.credential);
 			if (e->item.password_expiry_utc != TIME_MAX)
 				fprintf(out, "password_expiry_utc=%"PRItime"\n",
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 58b9c740605..cc6bf9aa5f3 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -566,6 +566,21 @@ helper_test_authtype() {
 		EOF
 	'
 
+	test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=git.example.com
+		--
+		protocol=https
+		host=git.example.com
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''https://git.example.com'\'':
+		askpass: Password for '\''https://askpass-username@git.example.com'\'':
+		EOF
+	'
+
 	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
 		check approve $HELPER <<-\EOF
 		capability[]=authtype

base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
-- 
gitgitgadget

Re: [PATCH v5] credential-cache: respect authtype capability

[M Hickford]

On 2025-01-09 22:45, M Hickford via GitGitGadget wrote:
> From: M Hickford <mirth.hickford@gmail.com>
> 
> Previously, credential-cache populated authtype regardless whether
> "get" request had authtype capability. As documented in
> git-credential.txt, authtype "should not be sent unless the appropriate
> capability ... is provided".
> 
> Add test. Without this change, the test failed because "credential fill"
> printed an incomplete credential with only protocol and host attributes
> (the unexpected authtype attribute was discarded by credential.c).
> 
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
>      credential-cache: respect request capabilities
>      
>      CC: sandals@crustytoothpaste.net CC: gitster@pobox.com
>      
>      Patch v5 adds details to the commit message
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/1842
> 
> Range-diff vs v4:
> 
>   1:  23942f9fa47 ! 1:  db575d9d116 credential-cache: respect request capabilities
>       @@ Metadata
>        Author: M Hickford <mirth.hickford@gmail.com>
>        
>         ## Commit message ##
>       -    credential-cache: respect request capabilities
>       +    credential-cache: respect authtype capability
>        
>       -    Previously, credential-cache populated authtype regardless of request.
>       +    Previously, credential-cache populated authtype regardless whether
>       +    "get" request had authtype capability. As documented in
>       +    git-credential.txt, authtype "should not be sent unless the appropriate
>       +    capability ... is provided".
>       +
>       +    Add test. Without this change, the test failed because "credential fill"
>       +    printed an incomplete credential with only protocol and host attributes
>       +    (the unexpected authtype attribute was discarded by credential.c).
>        
>            Signed-off-by: M Hickford <mirth.hickford@gmail.com>
>        
> 
> 
>   builtin/credential-cache--daemon.c |  4 ++--
>   t/lib-credential.sh                | 15 +++++++++++++++
>   2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index bc22f5c6d24..e707618e743 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)
>   				fprintf(out, "username=%s\n", e->item.username);
>   			if (e->item.password)
>   				fprintf(out, "password=%s\n", e->item.password);
> -			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
> +			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
>   				fprintf(out, "authtype=%s\n", e->item.authtype);
> -			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
> +			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
>   				fprintf(out, "credential=%s\n", e->item.credential);
>   			if (e->item.password_expiry_utc != TIME_MAX)
>   				fprintf(out, "password_expiry_utc=%"PRItime"\n",
> diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> index 58b9c740605..cc6bf9aa5f3 100644
> --- a/t/lib-credential.sh
> +++ b/t/lib-credential.sh
> @@ -566,6 +566,21 @@ helper_test_authtype() {
>   		EOF
>   	'
>   
> +	test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" '
> +		check fill $HELPER <<-\EOF
> +		protocol=https
> +		host=git.example.com
> +		--
> +		protocol=https
> +		host=git.example.com
> +		username=askpass-username
> +		password=askpass-password
> +		--
> +		askpass: Username for '\''https://git.example.com'\'':
> +		askpass: Password for '\''https://askpass-username@git.example.com'\'':
> +		EOF
> +	'
> +
>   	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
>   		check approve $HELPER <<-\EOF
>   		capability[]=authtype
> 
> base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f

Hi Brian. Any further comments on patch v5? This addresses your comments 
on v2 and expands the commit message as encouraged by Junio. (Thank you 
both for the review so far.)

https://lore.kernel.org/git/Z3xhqCf7Gr74BHO4@tapette.crustytoothpaste.net/
https://lore.kernel.org/git/xmqqttaaoyaz.fsf@gitster.g/

Re: [PATCH v5] credential-cache: respect authtype capability

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 324 bytes --]

On 2025-01-18 at 20:09:50, M Hickford wrote:
> Hi Brian. Any further comments on patch v5? This addresses your comments on
> v2 and expands the commit message as encouraged by Junio. (Thank you both
> for the review so far.)

I think this looks fine.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]