Re: [PATCH v2] credential-cache: respect request capabilities
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Jan 6, 2025, 22:32 UTC
- Message-ID
- <Z3xaE_v45G447HQe@tapette.crustytoothpaste.net>
- In-Reply-To
- <pull.1842.v2.git.1736193131798.gitgitgadget@gmail.com>
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.
Show 15 quoted lines
> 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.
Show 9 quoted lines
> 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.
Show 33 quoted lines
> 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