Skip to content

Desktop deletes the keyring identity on a parse failure before checking whether any fallback exists #6218

Description

@obsidianaddict

Describe the bug

recover_from_keyring in desktop/src-tauri/src/app_state.rs deletes the stored identity from the
OS keyring as its first action when Keys::parse fails, and only afterwards looks for a legacy
identity.key to recover from:

eprintln!("buzz-desktop: corrupt nsec in keyring ({error}), clearing and recovering from file");
if let Err(e) = store.delete(IDENTITY_KEY_NAME) {   // <- destroyed here
    eprintln!("buzz-desktop: failed to clear corrupt keyring value: {e}");
}
if legacy_path.exists() {                            // <- only now is a replacement looked for
    ...
}

If no valid file exists, the identity is gone — the delete has already happened, and it is
irreversible.

This is worse than it first looks, because on a healthy install there is deliberately no fallback.
Once the keyring is authoritative a matching identity.key is removed (ensure_marker_then_cleanup
/ cleanup_leftover_identity_file), and the tests assert that removal — in app_state_tests.rs, 22
of the 51 test functions assert a deletion (either identity.key's absence or the keyring delete
call). So in the normal steady state this code deletes the only copy of the user's identity in order
to attempt a recovery that then cannot succeed. The user lands in RecoveryState::Lost and sees the
first-run create-or-import screen, with no error shown.

Steps to reproduce

  1. On Windows, sign in so the identity is stored in Credential Manager, and confirm the normal
    steady state: identity.migrated exists in the app data directory and identity.key does not.
  2. Replace the stored identity value inside that credential with a string that is not a valid
    nsec. Any unparseable value reproduces it.
  3. Launch the app. The stored value is deleted, no identity.key is found, and onboarding appears.
  4. Relaunch. The identity cannot be recovered, because nothing is left to read.

The same ordering is reachable through the existing test seam without touching a real keyring:
resolve_identity_with_store with a store returning Present and an unparseable value shows the
delete happening before any fallback is looked for.

Expected behavior

Attempt recovery first, and delete the corrupt value only once a replacement has been stored and
read-back verified. Where no replacement exists, preserve the corrupt value and surface Lost.
Preserving it costs nothing and keeps the material available for manual recovery, whereas deleting
it cannot be undone. Ideally the user is also told the stored identity was unreadable, rather than
being shown a screen that implies they never had one.

Version and platform

  • Buzz version: 0.5.8, built from source at commit 119a848. This is a fork, but the file in
    question is untouched by our changes — the code quoted above is exactly as it stands at that
    commit.
  • OS: Windows 11

Logs / additional context

No logs, and that is part of the problem: these are eprintln! calls, so in a windowed Windows
build the output goes nowhere. After the fact there was no corrupt nsec in keyring line to read,
which is why we can report the ordering defect but not what corrupted the value.

What we observed on the affected machine: the stored value became unreadable, the app showed
create-or-import, identity.migrated was present and identity.key absent — the documented Lost
precondition. The account was only recovered because the user happened to have exported their nsec
separately. Notably the user did not see the "identity is safe but the keyring is unreachable"
screen, so the keyring was readable and the value was genuinely gone from it.

We could not reproduce how the stored value became unparseable in the first place.

One cause we did rule out, in case it saves someone the same detour: this was not the 2560-byte
Windows credential overflow. For one identity plus three agents the compact JSON blob is roughly
320–500 bytes, well under the cap.

Adjacent observation, possibly a separate issue: KeyringProbe collapses every read error to
Unreachable ("corrupt blob — fail closed"), so at the call site a transient backend error is
indistinguishable from a corrupt blob. Those deserve different handling, since one is safe to retry
and the other is not.

Nearest existing issues, both of which look different to this: #4472 is a label problem where the
identity is intact and only the button text is wrong, and #5837 is the shared keychain item
surfacing as "keyring unreachable".

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions