{"thread":{"id":"66117","subject":"[PATCH] credential/libsecret: load secrets explicitly","startedAt":"2026-08-04T22:40:19Z","lastAt":"2026-09-28T07:37:12Z","messageCount":10,"participants":["Daniel Martí via GitGitGadget","Daniel Martí","Junio C Hamano","M Hickford"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"549630","messageId":"pull.2372.git.git.1785883217733.gitgitgadget@gmail.com","threadId":"66117","inReplyTo":null,"subject":"[PATCH] credential/libsecret: load secrets explicitly","fromName":"Daniel Martí via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-04T22:40:17Z","receivedAt":"2026-08-04T22:40:19Z","isPatch":true,"body":"From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>\n\nsecret_service_search_sync() can return an item whose secret is not\nloaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search\nsilently discards secret-loading failures, and the GNOME keyring\ndaemon silently omits from its GetSecrets reply any item that is\nlocked or that was deleted after the search matched it, e.g. by a\nconcurrent \"credential erase\" from another git process.\n\nsecret_item_get_secret() then returns NULL, which we pass unchecked\nto secret_value_get_text() and secret_value_unref(), producing\n\n    secret_value_get_text: assertion 'value' failed\n    secret_value_unref: assertion 'value != NULL' failed\n\nand losing the password even when the secret is still retrievable.\n\nDrop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the\none item we use with secret_item_load_secret_sync(), which does\nreport errors. A secret the search would have silently dropped is\nnow retrieved normally, and a genuinely inaccessible item produces\na useful message instead of assertion spew, with git falling back\nto prompting either way. Merely guarding against NULL would avoid\nthe assertions, but would forfeit a secret that is still available.\nThe cost is unchanged: the search no longer batch-fetches the\nsecrets of all matching items, and the explicit load fetches the\none we use.\n\nSigned-off-by: Daniel Martí <mvdan@mvdan.cc>\n---\n    credential/libsecret: load secrets explicitly\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v1\nPull-Request: https://github.com/git/git/pull/2372\n\n .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\nindex 941b2afd5e..6bbdf2bd45 100644\n--- a/contrib/credential/libsecret/git-credential-libsecret.c\n+++ b/contrib/credential/libsecret/git-credential-libsecret.c\n@@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)\n \titems = secret_service_search_sync(service,\n \t\t\t\t\t   &schema,\n \t\t\t\t\t   attributes,\n-\t\t\t\t\t   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,\n+\t\t\t\t\t   SECRET_SEARCH_UNLOCK,\n \t\t\t\t\t   NULL,\n \t\t\t\t\t   &error);\n \tg_hash_table_unref(attributes);\n@@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)\n \t\tgchar **parts;\n \n \t\titem = items->data;\n+\n+\t\t/*\n+\t\t * Load the secret explicitly rather than via\n+\t\t * SECRET_SEARCH_LOAD_SECRETS, which silently discards load\n+\t\t * failures and returns items whose secret is NULL.\n+\t\t */\n+\t\tif (!secret_item_load_secret_sync(item, NULL, &error)) {\n+\t\t\tg_critical(\"could not load secret: %s\", error->message);\n+\t\t\tg_error_free(error);\n+\t\t\tg_list_free_full(items, g_object_unref);\n+\t\t\treturn EXIT_FAILURE;\n+\t\t}\n \t\tsecret = secret_item_get_secret(item);\n \t\tattributes = secret_item_get_attributes(item);\n \n\nbase-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a\n-- \ngitgitgadget\n"},{"id":"550910","messageId":"331e94ec-c07a-4d56-938d-d12c9a0cc5c9@mvdan.cc","threadId":"66117","inReplyTo":"pull.2372.git.git.1785883217733.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential/libsecret: load secrets explicitly","fromName":"Daniel Martí","fromEmail":"mvdan@mvdan.cc","sentAt":"2026-08-20T15:00:12Z","receivedAt":"2026-08-20T15:00:17Z","isPatch":true,"body":"Gentle nudge on this, anything I can do to assist in getting it reviewed?\n\nI still run into the error a few times per week :)\n\nOn 8/4/26 11:40 PM, Daniel Martí via GitGitGadget wrote:\n> From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>\n>\n> secret_service_search_sync() can return an item whose secret is not\n> loaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search\n> silently discards secret-loading failures, and the GNOME keyring\n> daemon silently omits from its GetSecrets reply any item that is\n> locked or that was deleted after the search matched it, e.g. by a\n> concurrent \"credential erase\" from another git process.\n>\n> secret_item_get_secret() then returns NULL, which we pass unchecked\n> to secret_value_get_text() and secret_value_unref(), producing\n>\n>      secret_value_get_text: assertion 'value' failed\n>      secret_value_unref: assertion 'value != NULL' failed\n>\n> and losing the password even when the secret is still retrievable.\n>\n> Drop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the\n> one item we use with secret_item_load_secret_sync(), which does\n> report errors. A secret the search would have silently dropped is\n> now retrieved normally, and a genuinely inaccessible item produces\n> a useful message instead of assertion spew, with git falling back\n> to prompting either way. Merely guarding against NULL would avoid\n> the assertions, but would forfeit a secret that is still available.\n> The cost is unchanged: the search no longer batch-fetches the\n> secrets of all matching items, and the explicit load fetches the\n> one we use.\n>\n> Signed-off-by: Daniel Martí <mvdan@mvdan.cc>\n> ---\n>      credential/libsecret: load secrets explicitly\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v1\n> Pull-Request: https://github.com/git/git/pull/2372\n>\n>   .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-\n>   1 file changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\n> index 941b2afd5e..6bbdf2bd45 100644\n> --- a/contrib/credential/libsecret/git-credential-libsecret.c\n> +++ b/contrib/credential/libsecret/git-credential-libsecret.c\n> @@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)\n>   \titems = secret_service_search_sync(service,\n>   \t\t\t\t\t   &schema,\n>   \t\t\t\t\t   attributes,\n> -\t\t\t\t\t   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,\n> +\t\t\t\t\t   SECRET_SEARCH_UNLOCK,\n>   \t\t\t\t\t   NULL,\n>   \t\t\t\t\t   &error);\n>   \tg_hash_table_unref(attributes);\n> @@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)\n>   \t\tgchar **parts;\n>   \n>   \t\titem = items->data;\n> +\n> +\t\t/*\n> +\t\t * Load the secret explicitly rather than via\n> +\t\t * SECRET_SEARCH_LOAD_SECRETS, which silently discards load\n> +\t\t * failures and returns items whose secret is NULL.\n> +\t\t */\n> +\t\tif (!secret_item_load_secret_sync(item, NULL, &error)) {\n> +\t\t\tg_critical(\"could not load secret: %s\", error->message);\n> +\t\t\tg_error_free(error);\n> +\t\t\tg_list_free_full(items, g_object_unref);\n> +\t\t\treturn EXIT_FAILURE;\n> +\t\t}\n>   \t\tsecret = secret_item_get_secret(item);\n>   \t\tattributes = secret_item_get_attributes(item);\n>   \n>\n> base-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a\n"},{"id":"550938","messageId":"xmqq33w8sjcg.fsf@gitster.g","threadId":"66117","inReplyTo":"331e94ec-c07a-4d56-938d-d12c9a0cc5c9@mvdan.cc","subject":"Re: [PATCH] credential/libsecret: load secrets explicitly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-20T19:05:51Z","receivedAt":"2026-08-20T19:05:54Z","isPatch":true,"body":"Daniel Martí <mvdan@mvdan.cc> writes:\n\n> Gentle nudge on this, anything I can do to assist in getting it reviewed?\n>\n> I still run into the error a few times per week :)\n>\n> On 8/4/26 11:40 PM, Daniel Martí via GitGitGadget wrote:\n>> From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>\n>>\n>> secret_service_search_sync() can return an item whose secret is not\n>> loaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search\n>> silently discards secret-loading failures, and the GNOME keyring\n>> daemon silently omits from its GetSecrets reply any item that is\n>> locked or that was deleted after the search matched it, e.g. by a\n>> concurrent \"credential erase\" from another git process.\n>>\n>> secret_item_get_secret() then returns NULL, which we pass unchecked\n>> to secret_value_get_text() and secret_value_unref(), producing\n\nI do not program Gnome so I am not exactly qualified to review this,\nbut anyway.\n\nThe above makes it sound like we can just request with\nsecret_service_search_sync() exactly the same way as before (i.e.,\nwith LOAD_SECRETS), and then check with secret_item_get_secret()\nto see if it has secret value in it.  The problem with the current\ncode is that we do not validate what that secrete value is (iow, we\ndo not say \"ah, NULL, we should not assume we do have secret already\nobtained here\").\n\nSo does the first hunk to drop _LOAD_SECRETS really needed?  Rather,\nwould it be more straight-forward to do\n\n\titem = items->data;\n\n\tsecret = secret_item_get_secret(item);\n\tif (!secret &&\n            !secret_item_load_secret_sync(item, NULL, &error)) {\n\t\t... your error handling here ...\n\t\treturn EXIT_FAILURE;\n\t} \n        if (!secret)\n\t\tsecret = secret_item_get_secret(item);\n\ninstead?  I am assuming that it is rare (like, only a few times a\nweek) to race with other activities that unloads the secret and most\nof the time the first secret_service_search_sync() gets everything\nneeded in a single call.\n\nI am also assuming that this is a race condition that is not very\neasy to reliably reproduce in the test, so I wouldn't expect it to\ncome with a test to ensure that the fix will not regress in the\nfuture (in other words, lack of patch to t/ directory is fine).\n\nThanks.\n\n\n>>\n>>      secret_value_get_text: assertion 'value' failed\n>>      secret_value_unref: assertion 'value != NULL' failed\n>>\n>> and losing the password even when the secret is still retrievable.\n>>\n>> Drop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the\n>> one item we use with secret_item_load_secret_sync(), which does\n>> report errors. A secret the search would have silently dropped is\n>> now retrieved normally, and a genuinely inaccessible item produces\n>> a useful message instead of assertion spew, with git falling back\n>> to prompting either way. Merely guarding against NULL would avoid\n>> the assertions, but would forfeit a secret that is still available.\n>> The cost is unchanged: the search no longer batch-fetches the\n>> secrets of all matching items, and the explicit load fetches the\n>> one we use.\n>>\n>> Signed-off-by: Daniel Martí <mvdan@mvdan.cc>\n>> ---\n>>      credential/libsecret: load secrets explicitly\n>>\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v1\n>> Pull-Request: https://github.com/git/git/pull/2372\n>>\n>>   .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-\n>>   1 file changed, 13 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\n>> index 941b2afd5e..6bbdf2bd45 100644\n>> --- a/contrib/credential/libsecret/git-credential-libsecret.c\n>> +++ b/contrib/credential/libsecret/git-credential-libsecret.c\n>> @@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)\n>>   \titems = secret_service_search_sync(service,\n>>   \t\t\t\t\t   &schema,\n>>   \t\t\t\t\t   attributes,\n>> -\t\t\t\t\t   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,\n>> +\t\t\t\t\t   SECRET_SEARCH_UNLOCK,\n>>   \t\t\t\t\t   NULL,\n>>   \t\t\t\t\t   &error);\n>>   \tg_hash_table_unref(attributes);\n>> @@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)\n>>   \t\tgchar **parts;\n>>   \n>>   \t\titem = items->data;\n>> +\n>> +\t\t/*\n>> +\t\t * Load the secret explicitly rather than via\n>> +\t\t * SECRET_SEARCH_LOAD_SECRETS, which silently discards load\n>> +\t\t * failures and returns items whose secret is NULL.\n>> +\t\t */\n>> +\t\tif (!secret_item_load_secret_sync(item, NULL, &error)) {\n>> +\t\t\tg_critical(\"could not load secret: %s\", error->message);\n>> +\t\t\tg_error_free(error);\n>> +\t\t\tg_list_free_full(items, g_object_unref);\n>> +\t\t\treturn EXIT_FAILURE;\n>> +\t\t}\n>>   \t\tsecret = secret_item_get_secret(item);\n>>   \t\tattributes = secret_item_get_attributes(item);\n>>   \n>>\n>> base-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a\n"},{"id":"551057","messageId":"a60afe54-7059-4827-8c3b-dfe01a0cde9c@mvdan.cc","threadId":"66117","inReplyTo":"xmqq33w8sjcg.fsf@gitster.g","subject":"Re: [PATCH] credential/libsecret: load secrets explicitly","fromName":"Daniel Martí","fromEmail":"mvdan@mvdan.cc","sentAt":"2026-08-22T20:47:12Z","receivedAt":"2026-08-22T20:47:28Z","isPatch":true,"body":"Hi Junio, thanks very much for your review. I also do not program Gnome,\nso this is just based on my best effort research :)\n\nJunio C Hamano <gitster@pobox.com> writes:\n > The above makes it sound like we can just request with\n > secret_service_search_sync() exactly the same way as before (i.e.,\n > with LOAD_SECRETS), and then check with secret_item_get_secret()\n > to see if it has secret value in it.  The problem with the current\n > code is that we do not validate what that secrete value is (iow, we\n > do not say \"ah, NULL, we should not assume we do have secret already\n > obtained here\").\n >\n > So does the first hunk to drop _LOAD_SECRETS really needed?\n\nNot strictly; your version would avoid the assertion failures too.\n\nI dropped the flag because it isn't the single call it appears to be:\nlibsecret implements SECRET_SEARCH_LOAD_SECRETS as the SearchItems\nD-Bus call followed by a separate GetSecrets call covering the matched\nitems, with any error from that second step discarded. It also skips\nlocked items on the client side without even asking the daemon, so\nSECRET_SEARCH_UNLOCK having silently failed is one way to end up with\na NULL secret. The patch does SearchItems plus one GetSecret for the\none item we use, so the common case is the same two round trips either\nway; it is your fallback version that would add a third in the rare\ncase. That is what the \"cost is unchanged\" paragraph in the commit\nmessage is trying to say.\n\nKeeping the flag with a fallback load would also mean the explicit\nload only runs during the rare race, so that path would go essentially\nunexercised, and load errors on the common path would still be\nsilently discarded. With the unconditional load there is one code\npath, taken on every run, and any failure produces a real error\nmessage.\n\n > I am also assuming that this is a race condition that is not very\n > easy to reliably reproduce in the test, so I wouldn't expect it to\n > come with a test to ensure that the fix will not regress in the\n > future (in other words, lack of patch to t/ directory is fine).\n\nRight, it needs a concurrently-racing process (or a locked keyring)\nto trigger, so I don't see a way to test it reliably.\n\nThanks!\n\n"},{"id":"552954","messageId":"c3f51472-8dfa-42df-aa8b-b0f5bfd0aafe@mvdan.cc","threadId":"66117","inReplyTo":"a60afe54-7059-4827-8c3b-dfe01a0cde9c@mvdan.cc","subject":"Re: [PATCH] credential/libsecret: load secrets explicitly","fromName":"Daniel Martí","fromEmail":"mvdan@mvdan.cc","sentAt":"2026-09-21T21:54:15Z","receivedAt":"2026-09-21T21:54:31Z","isPatch":true,"body":"Friendly nudge :) Happy to look into any tweaks or questions.\n\nOn 8/22/26 9:47 PM, Daniel Martí wrote:\n> Hi Junio, thanks very much for your review. I also do not program Gnome,\n> so this is just based on my best effort research :)\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n> > The above makes it sound like we can just request with\n> > secret_service_search_sync() exactly the same way as before (i.e.,\n> > with LOAD_SECRETS), and then check with secret_item_get_secret()\n> > to see if it has secret value in it.  The problem with the current\n> > code is that we do not validate what that secrete value is (iow, we\n> > do not say \"ah, NULL, we should not assume we do have secret already\n> > obtained here\").\n> >\n> > So does the first hunk to drop _LOAD_SECRETS really needed?\n>\n> Not strictly; your version would avoid the assertion failures too.\n>\n> I dropped the flag because it isn't the single call it appears to be:\n> libsecret implements SECRET_SEARCH_LOAD_SECRETS as the SearchItems\n> D-Bus call followed by a separate GetSecrets call covering the matched\n> items, with any error from that second step discarded. It also skips\n> locked items on the client side without even asking the daemon, so\n> SECRET_SEARCH_UNLOCK having silently failed is one way to end up with\n> a NULL secret. The patch does SearchItems plus one GetSecret for the\n> one item we use, so the common case is the same two round trips either\n> way; it is your fallback version that would add a third in the rare\n> case. That is what the \"cost is unchanged\" paragraph in the commit\n> message is trying to say.\n>\n> Keeping the flag with a fallback load would also mean the explicit\n> load only runs during the rare race, so that path would go essentially\n> unexercised, and load errors on the common path would still be\n> silently discarded. With the unconditional load there is one code\n> path, taken on every run, and any failure produces a real error\n> message.\n>\n> > I am also assuming that this is a race condition that is not very\n> > easy to reliably reproduce in the test, so I wouldn't expect it to\n> > come with a test to ensure that the fix will not regress in the\n> > future (in other words, lack of patch to t/ directory is fine).\n>\n> Right, it needs a concurrently-racing process (or a locked keyring)\n> to trigger, so I don't see a way to test it reliably.\n>\n> Thanks!\n"},{"id":"552958","messageId":"xmqqv77yjjys.fsf@gitster.g","threadId":"66117","inReplyTo":"c3f51472-8dfa-42df-aa8b-b0f5bfd0aafe@mvdan.cc","subject":"Re: [PATCH] credential/libsecret: load secrets explicitly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-21T22:52:27Z","receivedAt":"2026-09-21T22:52:31Z","isPatch":true,"body":"Daniel Martí <mvdan@mvdan.cc> writes:\n\n> Friendly nudge :) Happy to look into any tweaks or questions.\n\nWe have had a review exchange.  A reviewer had to ask questions on\nthe points where the code and the proposed log message weren't\nclear.  The ball is now in the author's court to improve the patch\nto the point that the next reviewer who did not see the first\nexchange would not have to ask the same questions.\n\nThanks for working on this topic.\n"},{"id":"553145","messageId":"CAGJzqs=sUA7vGDwadL9h-dcuPAsQvhAjiirZhA5=_fyqH1QXuA@mail.gmail.com","threadId":"66117","inReplyTo":"pull.2372.git.git.1785883217733.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential/libsecret: load secrets explicitly","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2026-09-24T07:00:00Z","receivedAt":"2026-09-24T07:01:09Z","isPatch":true,"body":"Thanks for sharing\n\nOn Tue, 4 Aug 2026 at 23:40, Daniel Martí via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>\n>\n> secret_service_search_sync() can return an item whose secret is not\n> loaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search\n> silently discards secret-loading failures, and the GNOME keyring\n> daemon silently omits from its GetSecrets reply any item that is\n> locked or that was deleted after the search matched it, e.g. by a\n> concurrent \"credential erase\" from another git process.\n>\n> secret_item_get_secret() then returns NULL, which we pass unchecked\n> to secret_value_get_text() and secret_value_unref(), producing\n>\n>     secret_value_get_text: assertion 'value' failed\n>     secret_value_unref: assertion 'value != NULL' failed\n>\n> and losing the password even when the secret is still retrievable.\n>\n> Drop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the\n> one item we use with secret_item_load_secret_sync(), which does\n> report errors. A secret the search would have silently dropped is\n> now retrieved normally, and a genuinely inaccessible item produces\n> a useful message instead of assertion spew, with git falling back\n> to prompting either way. Merely guarding against NULL would avoid\n> the assertions, but would forfeit a secret that is still available.\n> The cost is unchanged: the search no longer batch-fetches the\n> secrets of all matching items, and the explicit load fetches the\n> one we use.\n>\n> Signed-off-by: Daniel Martí <mvdan@mvdan.cc>\n\nThanks for explaining the motivation.\n\nIs this an upstream bug in libsecret?\n\nThe libsecret docs for SECRET_SEARCH_LOAD_SECRETS  are unfortunately\ntruncated https://gnome.pages.gitlab.gnome.org/libsecret/method.Service.search_sync.html\n\n> If SECRET_SEARCH_LOAD_SECRETS is set in flags, then the items’ secret values will be loaded for any unlocked items. Loaded item secret values are available via secret_item_get_secret(). If the load of a secret values fail, then the [mystery consequence]\n\n\n> ---\n>     credential/libsecret: load secrets explicitly\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v1\n> Pull-Request: https://github.com/git/git/pull/2372\n>\n>  .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-\n>  1 file changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\n> index 941b2afd5e..6bbdf2bd45 100644\n> --- a/contrib/credential/libsecret/git-credential-libsecret.c\n> +++ b/contrib/credential/libsecret/git-credential-libsecret.c\n> @@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)\n>         items = secret_service_search_sync(service,\n>                                            &schema,\n>                                            attributes,\n> -                                          SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,\n> +                                          SECRET_SEARCH_UNLOCK,\n>                                            NULL,\n>                                            &error);\n>         g_hash_table_unref(attributes);\n> @@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)\n>                 gchar **parts;\n>\n>                 item = items->data;\n> +\n> +               /*\n> +                * Load the secret explicitly rather than via\n> +                * SECRET_SEARCH_LOAD_SECRETS, which silently discards load\n> +                * failures and returns items whose secret is NULL.\n> +                */\n> +               if (!secret_item_load_secret_sync(item, NULL, &error)) {\n> +                       g_critical(\"could not load secret: %s\", error->message);\n> +                       g_error_free(error);\n> +                       g_list_free_full(items, g_object_unref);\n> +                       return EXIT_FAILURE;\n> +               }\n>                 secret = secret_item_get_secret(item);\n>                 attributes = secret_item_get_attributes(item);\n>\n>\n> base-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a\n> --\n> gitgitgadget\n"},{"id":"553394","messageId":"3cae7bd6-33fa-4695-bf4e-9f473ac98042@mvdan.cc","threadId":"66117","inReplyTo":"CAGJzqs=sUA7vGDwadL9h-dcuPAsQvhAjiirZhA5=_fyqH1QXuA@mail.gmail.com","subject":"Re: [PATCH] credential/libsecret: load secrets explicitly","fromName":"Daniel Martí","fromEmail":"mvdan@mvdan.cc","sentAt":"2026-09-27T22:45:03Z","receivedAt":"2026-09-27T22:45:11Z","isPatch":true,"body":"On 9/24/26 8:00 AM, M Hickford wrote:\n> Is this an upstream bug in libsecret?\n>\n> The libsecret docs for SECRET_SEARCH_LOAD_SECRETS  are unfortunately\n> truncated https://gnome.pages.gitlab.gnome.org/libsecret/method.Service.search_sync.html\n\nPartly. The truncated sentence is a docs bug, which I've sent a fix for:\nhttps://gitlab.gnome.org/GNOME/libsecret/-/merge_requests/182\n\nThe behavior itself looks intentional, though. The search does not\nload secrets of locked items, and just like a failed unlock, a failed\nload does not fail the search; secret_item_get_secret() is documented\nto return NULL for a locked or unloaded item. The daemon side is\ndeliberate too: gnome-keyring's GetSecrets skips items which are\nlocked or no longer exist, whereas GetSecret on a single item returns\nan error.\n\nSo git needs to handle a NULL secret either way, including with every\nlibsecret release out there. libsecret's own secret-tool also loads\neach secret explicitly after searching, which is what this patch does.\n\nI'll send a v2 with a reworded commit message shortly.\n\nThanks!\n\n"},{"id":"553395","messageId":"pull.2372.v2.git.git.1790549181518.gitgitgadget@gmail.com","threadId":"66117","inReplyTo":"pull.2372.git.git.1785883217733.gitgitgadget@gmail.com","subject":"[PATCH v2] credential/libsecret: load secrets explicitly","fromName":"Daniel Martí via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-27T22:46:21Z","receivedAt":"2026-09-27T22:46:25Z","isPatch":true,"body":"From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>\n\nkeyring_get() searches with SECRET_SEARCH_LOAD_SECRETS, then passes\nsecret_item_get_secret() of the first match unchecked to\nsecret_value_get_text() and secret_value_unref(). As libsecret\ndocuments, that secret can be NULL: the search does not load secrets\nof locked items, such as when SECRET_SEARCH_UNLOCK fails to unlock\nthem, and it ignores errors from loading secrets. The GNOME keyring\ndaemon also silently leaves out of its reply any item which is locked\nor which was deleted after the search matched it, e.g. by a concurrent\n\"credential erase\" from another git process. We then print\n\n    secret_value_get_text: assertion 'value' failed\n    secret_value_unref: assertion 'value != NULL' failed\n\nbefore git falls back to prompting for the password.\n\nWe could keep the flag and load the secret explicitly only when it is\nNULL, but SECRET_SEARCH_LOAD_SECRETS is not part of the search call:\nlibsecret implements it as a separate GetSecrets D-Bus call after\nSearchItems. Drop the flag and instead always load the one secret we\nuse with secret_item_load_secret_sync(), which reports errors. This\ntakes as many D-Bus calls as before, and leaves a single code path\nthat runs every time, rather than a fallback that only runs in a rare\nrace. libsecret's own secret-tool also loads each secret explicitly\nafter searching.\n\nAn inaccessible item now produces a useful error message instead of\nthe assertion failures, and git still falls back to prompting. The\nrace needs a concurrent process or a locked keyring to trigger, so\nthere is no test.\n\nSigned-off-by: Daniel Martí <mvdan@mvdan.cc>\n---\n    credential/libsecret: load secrets explicitly\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v2\nPull-Request: https://github.com/git/git/pull/2372\n\nRange-diff vs v1:\n\n 1:  89d3eee91f ! 1:  b9ddf13fa1 credential/libsecret: load secrets explicitly\n     @@ Metadata\n       ## Commit message ##\n          credential/libsecret: load secrets explicitly\n      \n     -    secret_service_search_sync() can return an item whose secret is not\n     -    loaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search\n     -    silently discards secret-loading failures, and the GNOME keyring\n     -    daemon silently omits from its GetSecrets reply any item that is\n     -    locked or that was deleted after the search matched it, e.g. by a\n     -    concurrent \"credential erase\" from another git process.\n     -\n     -    secret_item_get_secret() then returns NULL, which we pass unchecked\n     -    to secret_value_get_text() and secret_value_unref(), producing\n     +    keyring_get() searches with SECRET_SEARCH_LOAD_SECRETS, then passes\n     +    secret_item_get_secret() of the first match unchecked to\n     +    secret_value_get_text() and secret_value_unref(). As libsecret\n     +    documents, that secret can be NULL: the search does not load secrets\n     +    of locked items, such as when SECRET_SEARCH_UNLOCK fails to unlock\n     +    them, and it ignores errors from loading secrets. The GNOME keyring\n     +    daemon also silently leaves out of its reply any item which is locked\n     +    or which was deleted after the search matched it, e.g. by a concurrent\n     +    \"credential erase\" from another git process. We then print\n      \n              secret_value_get_text: assertion 'value' failed\n              secret_value_unref: assertion 'value != NULL' failed\n      \n     -    and losing the password even when the secret is still retrievable.\n     +    before git falls back to prompting for the password.\n     +\n     +    We could keep the flag and load the secret explicitly only when it is\n     +    NULL, but SECRET_SEARCH_LOAD_SECRETS is not part of the search call:\n     +    libsecret implements it as a separate GetSecrets D-Bus call after\n     +    SearchItems. Drop the flag and instead always load the one secret we\n     +    use with secret_item_load_secret_sync(), which reports errors. This\n     +    takes as many D-Bus calls as before, and leaves a single code path\n     +    that runs every time, rather than a fallback that only runs in a rare\n     +    race. libsecret's own secret-tool also loads each secret explicitly\n     +    after searching.\n      \n     -    Drop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the\n     -    one item we use with secret_item_load_secret_sync(), which does\n     -    report errors. A secret the search would have silently dropped is\n     -    now retrieved normally, and a genuinely inaccessible item produces\n     -    a useful message instead of assertion spew, with git falling back\n     -    to prompting either way. Merely guarding against NULL would avoid\n     -    the assertions, but would forfeit a secret that is still available.\n     -    The cost is unchanged: the search no longer batch-fetches the\n     -    secrets of all matching items, and the explicit load fetches the\n     -    one we use.\n     +    An inaccessible item now produces a useful error message instead of\n     +    the assertion failures, and git still falls back to prompting. The\n     +    race needs a concurrent process or a locked keyring to trigger, so\n     +    there is no test.\n      \n          Signed-off-by: Daniel Martí <mvdan@mvdan.cc>\n      \n     @@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(\n      +\n      +\t\t/*\n      +\t\t * Load the secret explicitly rather than via\n     -+\t\t * SECRET_SEARCH_LOAD_SECRETS, which silently discards load\n     -+\t\t * failures and returns items whose secret is NULL.\n     ++\t\t * SECRET_SEARCH_LOAD_SECRETS, which skips locked items and\n     ++\t\t * ignores load failures, leaving the secret NULL.\n      +\t\t */\n      +\t\tif (!secret_item_load_secret_sync(item, NULL, &error)) {\n      +\t\t\tg_critical(\"could not load secret: %s\", error->message);\n\n\n .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\nindex 941b2afd5e..ad4f60e4d7 100644\n--- a/contrib/credential/libsecret/git-credential-libsecret.c\n+++ b/contrib/credential/libsecret/git-credential-libsecret.c\n@@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)\n \titems = secret_service_search_sync(service,\n \t\t\t\t\t   &schema,\n \t\t\t\t\t   attributes,\n-\t\t\t\t\t   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,\n+\t\t\t\t\t   SECRET_SEARCH_UNLOCK,\n \t\t\t\t\t   NULL,\n \t\t\t\t\t   &error);\n \tg_hash_table_unref(attributes);\n@@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)\n \t\tgchar **parts;\n \n \t\titem = items->data;\n+\n+\t\t/*\n+\t\t * Load the secret explicitly rather than via\n+\t\t * SECRET_SEARCH_LOAD_SECRETS, which skips locked items and\n+\t\t * ignores load failures, leaving the secret NULL.\n+\t\t */\n+\t\tif (!secret_item_load_secret_sync(item, NULL, &error)) {\n+\t\t\tg_critical(\"could not load secret: %s\", error->message);\n+\t\t\tg_error_free(error);\n+\t\t\tg_list_free_full(items, g_object_unref);\n+\t\t\treturn EXIT_FAILURE;\n+\t\t}\n \t\tsecret = secret_item_get_secret(item);\n \t\tattributes = secret_item_get_attributes(item);\n \n\nbase-commit: 34f06850c16c7f7ac822b1adc71354f11b0f2ca3\n-- \ngitgitgadget\n"},{"id":"553413","messageId":"f73a8b6c-37cd-4804-8587-3df4f0999f0a@mvdan.cc","threadId":"66117","inReplyTo":"pull.2372.v2.git.git.1790549181518.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] credential/libsecret: load secrets explicitly","fromName":"Daniel Martí","fromEmail":"mvdan@mvdan.cc","sentAt":"2026-09-28T07:37:05Z","receivedAt":"2026-09-28T07:37:12Z","isPatch":true,"body":"I didn't add the \"changes since v1\" text to the right section of the PR,\nso the bot didn't pick it up correctly. Adding it here now...\n\nChanges since v1:\n- Reworded the commit message to answer Junio's questions: why the \nexplicit load replaces SECRET_SEARCH_LOAD_SECRETS rather than serving as \na fallback, what it costs, and why there is no test.\n- Dropped the claim that a secret dropped by the search is now \nretrieved; a deleted or still-locked item fails the explicit load too, \nonly with a proper error message.\n- Updated the code comment to say what libsecret actually does.\n\nOn 9/27/26 11:46 PM, Daniel Martí via GitGitGadget wrote:\n> From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>\n>\n> keyring_get() searches with SECRET_SEARCH_LOAD_SECRETS, then passes\n> secret_item_get_secret() of the first match unchecked to\n> secret_value_get_text() and secret_value_unref(). As libsecret\n> documents, that secret can be NULL: the search does not load secrets\n> of locked items, such as when SECRET_SEARCH_UNLOCK fails to unlock\n> them, and it ignores errors from loading secrets. The GNOME keyring\n> daemon also silently leaves out of its reply any item which is locked\n> or which was deleted after the search matched it, e.g. by a concurrent\n> \"credential erase\" from another git process. We then print\n>\n>      secret_value_get_text: assertion 'value' failed\n>      secret_value_unref: assertion 'value != NULL' failed\n>\n> before git falls back to prompting for the password.\n>\n> We could keep the flag and load the secret explicitly only when it is\n> NULL, but SECRET_SEARCH_LOAD_SECRETS is not part of the search call:\n> libsecret implements it as a separate GetSecrets D-Bus call after\n> SearchItems. Drop the flag and instead always load the one secret we\n> use with secret_item_load_secret_sync(), which reports errors. This\n> takes as many D-Bus calls as before, and leaves a single code path\n> that runs every time, rather than a fallback that only runs in a rare\n> race. libsecret's own secret-tool also loads each secret explicitly\n> after searching.\n>\n> An inaccessible item now produces a useful error message instead of\n> the assertion failures, and git still falls back to prompting. The\n> race needs a concurrent process or a locked keyring to trigger, so\n> there is no test.\n>\n> Signed-off-by: Daniel Martí <mvdan@mvdan.cc>\n> ---\n>      credential/libsecret: load secrets explicitly\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v2\n> Pull-Request: https://github.com/git/git/pull/2372\n>\n> Range-diff vs v1:\n>\n>   1:  89d3eee91f ! 1:  b9ddf13fa1 credential/libsecret: load secrets explicitly\n>       @@ Metadata\n>         ## Commit message ##\n>            credential/libsecret: load secrets explicitly\n>        \n>       -    secret_service_search_sync() can return an item whose secret is not\n>       -    loaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search\n>       -    silently discards secret-loading failures, and the GNOME keyring\n>       -    daemon silently omits from its GetSecrets reply any item that is\n>       -    locked or that was deleted after the search matched it, e.g. by a\n>       -    concurrent \"credential erase\" from another git process.\n>       -\n>       -    secret_item_get_secret() then returns NULL, which we pass unchecked\n>       -    to secret_value_get_text() and secret_value_unref(), producing\n>       +    keyring_get() searches with SECRET_SEARCH_LOAD_SECRETS, then passes\n>       +    secret_item_get_secret() of the first match unchecked to\n>       +    secret_value_get_text() and secret_value_unref(). As libsecret\n>       +    documents, that secret can be NULL: the search does not load secrets\n>       +    of locked items, such as when SECRET_SEARCH_UNLOCK fails to unlock\n>       +    them, and it ignores errors from loading secrets. The GNOME keyring\n>       +    daemon also silently leaves out of its reply any item which is locked\n>       +    or which was deleted after the search matched it, e.g. by a concurrent\n>       +    \"credential erase\" from another git process. We then print\n>        \n>                secret_value_get_text: assertion 'value' failed\n>                secret_value_unref: assertion 'value != NULL' failed\n>        \n>       -    and losing the password even when the secret is still retrievable.\n>       +    before git falls back to prompting for the password.\n>       +\n>       +    We could keep the flag and load the secret explicitly only when it is\n>       +    NULL, but SECRET_SEARCH_LOAD_SECRETS is not part of the search call:\n>       +    libsecret implements it as a separate GetSecrets D-Bus call after\n>       +    SearchItems. Drop the flag and instead always load the one secret we\n>       +    use with secret_item_load_secret_sync(), which reports errors. This\n>       +    takes as many D-Bus calls as before, and leaves a single code path\n>       +    that runs every time, rather than a fallback that only runs in a rare\n>       +    race. libsecret's own secret-tool also loads each secret explicitly\n>       +    after searching.\n>        \n>       -    Drop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the\n>       -    one item we use with secret_item_load_secret_sync(), which does\n>       -    report errors. A secret the search would have silently dropped is\n>       -    now retrieved normally, and a genuinely inaccessible item produces\n>       -    a useful message instead of assertion spew, with git falling back\n>       -    to prompting either way. Merely guarding against NULL would avoid\n>       -    the assertions, but would forfeit a secret that is still available.\n>       -    The cost is unchanged: the search no longer batch-fetches the\n>       -    secrets of all matching items, and the explicit load fetches the\n>       -    one we use.\n>       +    An inaccessible item now produces a useful error message instead of\n>       +    the assertion failures, and git still falls back to prompting. The\n>       +    race needs a concurrent process or a locked keyring to trigger, so\n>       +    there is no test.\n>        \n>            Signed-off-by: Daniel Martí <mvdan@mvdan.cc>\n>        \n>       @@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(\n>        +\n>        +\t\t/*\n>        +\t\t * Load the secret explicitly rather than via\n>       -+\t\t * SECRET_SEARCH_LOAD_SECRETS, which silently discards load\n>       -+\t\t * failures and returns items whose secret is NULL.\n>       ++\t\t * SECRET_SEARCH_LOAD_SECRETS, which skips locked items and\n>       ++\t\t * ignores load failures, leaving the secret NULL.\n>        +\t\t */\n>        +\t\tif (!secret_item_load_secret_sync(item, NULL, &error)) {\n>        +\t\t\tg_critical(\"could not load secret: %s\", error->message);\n>\n>\n>   .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-\n>   1 file changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\n> index 941b2afd5e..ad4f60e4d7 100644\n> --- a/contrib/credential/libsecret/git-credential-libsecret.c\n> +++ b/contrib/credential/libsecret/git-credential-libsecret.c\n> @@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)\n>   \titems = secret_service_search_sync(service,\n>   \t\t\t\t\t   &schema,\n>   \t\t\t\t\t   attributes,\n> -\t\t\t\t\t   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,\n> +\t\t\t\t\t   SECRET_SEARCH_UNLOCK,\n>   \t\t\t\t\t   NULL,\n>   \t\t\t\t\t   &error);\n>   \tg_hash_table_unref(attributes);\n> @@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)\n>   \t\tgchar **parts;\n>   \n>   \t\titem = items->data;\n> +\n> +\t\t/*\n> +\t\t * Load the secret explicitly rather than via\n> +\t\t * SECRET_SEARCH_LOAD_SECRETS, which skips locked items and\n> +\t\t * ignores load failures, leaving the secret NULL.\n> +\t\t */\n> +\t\tif (!secret_item_load_secret_sync(item, NULL, &error)) {\n> +\t\t\tg_critical(\"could not load secret: %s\", error->message);\n> +\t\t\tg_error_free(error);\n> +\t\t\tg_list_free_full(items, g_object_unref);\n> +\t\t\treturn EXIT_FAILURE;\n> +\t\t}\n>   \t\tsecret = secret_item_get_secret(item);\n>   \t\tattributes = secret_item_get_attributes(item);\n>   \n>\n> base-commit: 34f06850c16c7f7ac822b1adc71354f11b0f2ca3\n"}]}