Re: [PATCH] credential/libsecret: load secrets explicitly

["Daniel Martí" <mvdan@mvdan.cc>]

Gentle nudge on this, anything I can do to assist in getting it reviewed?

I still run into the error a few times per week :)

On 8/4/26 11:40 PM, Daniel Martí via GitGitGadget wrote:
> From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
>
> secret_service_search_sync() can return an item whose secret is not
> loaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search
> silently discards secret-loading failures, and the GNOME keyring
> daemon silently omits from its GetSecrets reply any item that is
> locked or that was deleted after the search matched it, e.g. by a
> concurrent "credential erase" from another git process.
>
> secret_item_get_secret() then returns NULL, which we pass unchecked
> to secret_value_get_text() and secret_value_unref(), producing
>
>      secret_value_get_text: assertion 'value' failed
>      secret_value_unref: assertion 'value != NULL' failed
>
> and losing the password even when the secret is still retrievable.
>
> Drop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the
> one item we use with secret_item_load_secret_sync(), which does
> report errors. A secret the search would have silently dropped is
> now retrieved normally, and a genuinely inaccessible item produces
> a useful message instead of assertion spew, with git falling back
> to prompting either way. Merely guarding against NULL would avoid
> the assertions, but would forfeit a secret that is still available.
> The cost is unchanged: the search no longer batch-fetches the
> secrets of all matching items, and the explicit load fetches the
> one we use.
>
> Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
> ---
>      credential/libsecret: load secrets explicitly
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v1
> Pull-Request: https://github.com/git/git/pull/2372
>
>   .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
> index 941b2afd5e..6bbdf2bd45 100644
> --- a/contrib/credential/libsecret/git-credential-libsecret.c
> +++ b/contrib/credential/libsecret/git-credential-libsecret.c
> @@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)
>   	items = secret_service_search_sync(service,
>   					   &schema,
>   					   attributes,
> -					   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
> +					   SECRET_SEARCH_UNLOCK,
>   					   NULL,
>   					   &error);
>   	g_hash_table_unref(attributes);
> @@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)
>   		gchar **parts;
>   
>   		item = items->data;
> +
> +		/*
> +		 * Load the secret explicitly rather than via
> +		 * SECRET_SEARCH_LOAD_SECRETS, which silently discards load
> +		 * failures and returns items whose secret is NULL.
> +		 */
> +		if (!secret_item_load_secret_sync(item, NULL, &error)) {
> +			g_critical("could not load secret: %s", error->message);
> +			g_error_free(error);
> +			g_list_free_full(items, g_object_unref);
> +			return EXIT_FAILURE;
> +		}
>   		secret = secret_item_get_secret(item);
>   		attributes = secret_item_get_attributes(item);
>   
>
> base-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a