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

From: M Hickford <[email-masked]>
Date: Sat, 18 Jan 2025 20:09:50 +0000

On 2025-01-09 22:45, M Hickford via GitGitGadget wrote:
> From: M Hickford <[email-masked]>
> 
> 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 <[email-masked]>
> ---
>      credential-cache: respect request capabilities
>      
>      CC: [email-masked] CC: [email-masked]
>      
>      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 <[email-masked]>
>        
>         ## 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 <[email-masked]>
>        
> 
> 
>   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://[email-masked]'\'':
> +		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/[email-masked]/
https://lore.kernel.org/git/[email-masked]/