fix(KeyringStorage): don't wipe stored secrets when saving before the initial fetch - #3616
Open
PHONE1X wants to merge 1 commit into
Open
fix(KeyringStorage): don't wipe stored secrets when saving before the initial fetch#3616PHONE1X wants to merge 1 commit into
PHONE1X wants to merge 1 commit into
Conversation
… initial fetch
saveKeyringData() serialises the whole keyringData object and `secret-tool
store` replaces the stored value outright, so there is no merge on the storage
side. keyringData defaults to {} and is only populated by the async
try_lookup.sh fetch, which nothing triggers at startup.
Any setNestedField() call that lands before that fetch resolves therefore
persists a blob containing only the field just written, destroying every other
secret in the keyring. Both call sites are reachable that way: Ai.setApiKey()
(the /key command) and GoogleCloud.setKeyJson().
Queue writes that arrive before the data is loaded and replay them once the
real contents are in memory, rather than saving over them. Also emit
dataChanged() when the initial fetch populates the data, and make
fetchKeyringData() re-entrancy safe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
KeyringStorage.saveKeyringData()serialises the entirekeyringDataobject and pipes it to
secret-tool store, which replaces the stored valueoutright — there is no merge on the storage side.
keyringDatadefaults to{}and is only populated by the asynchronoustry_lookup.shfetch. Nothing triggers that fetch at startup, so anysetNestedField()call that lands before it resolves writes a blob containingonly the field just written, destroying every other secret already stored.
Both call sites are reachable in that state:
services/Ai.qml:546—setApiKey(), behind the/key <apikey>command.The nearest protection is
Ai.qml:600, which fires a fire-and-forgetfetchKeyringData()in a different function and never waits for it.services/GoogleCloud.qml:46—setKeyJson(), when pasting a serviceaccount key.
loadKeyIfPossible()checksKeyringStorage.loadedbeforereading, but
setKeyJson()does not check it before writing.So: open the shell, paste an API key before the keyring fetch has completed,
and the other keys are silently gone. There is no error and no obvious cause —
the panel simply shows blank fields next time.
Fix
Queue writes that arrive before the data is loaded, and replay them once the
real contents are in memory:
setNestedField()returns early and queues when!loaded, kicking off afetch instead of saving.
applyNestedField(), so queuedwrites reuse exactly the same logic.
flushPendingWrites()replays the queue afterloadedbecomes true andsaves once.
exitCode === 1(no entry yet) branch no longer writes an empty blobwhen writes are already queued, so a fresh install does one save, not two.
dataChanged()is emitted when the initial fetch populates the data.GoogleCloud.qml:107already listens for it; previously it only ever firedafter a save.
fetchKeyringData()is now re-entrancy safe, since the queue path can callit while a fetch is already in flight.
I deliberately did not add an eager fetch at startup. That would fix the
race too, but it would spawn
secret-toolon every launch and can trigger akeyring unlock prompt at login, which is a behaviour change. Queueing keeps
the existing lazy-fetch model intact.
If the keyring is locked (
exitCode === 2),loadedstays false and writesremain queued rather than being lost.
LockScreen.qmlalready re-fetches onunlock, which drains the queue.
Testing
I extracted the state machine and ran it against the scenarios that matter.
All pass, including a regression check confirming current
maindoes destroythe other keys:
Also running on my own install (Hyprland 0.56.2, CachyOS) with several keys in
the keyring; setting a new key no longer disturbs the others.
For context, I hit this as actual data loss on my own machine before tracking
down the cause.