From e5a7e26a1008227ff6a0b9430a6df2470475bec1 Mon Sep 17 00:00:00 2001 From: Wes Date: Tue, 1 Sep 2026 13:43:01 -0600 Subject: [PATCH] fix(desktop): preserve keyring identity during recovery (#7203) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pinky is opening this PR on Wes's behalf. ## Summary Carry forward the fix from #7103 by **Taksh / @Chessing234**, with the original author and DCO sign-off preserved in commit `99dd19191c`. A separate Pinky-authored commit fixes and strengthens the regression tests. `recover_from_keyring` previously deleted an unreadable keyring identity before checking for a valid leftover `identity.key`. A migrated installation normally has a marker but no file, so that deletion could destroy the only remaining key material. - Attempt valid-file recovery before any explicit keyring deletion. - If a migration marker exists but no valid file can be recovered, enter `Lost` without deleting the unreadable keyring value. - Retain the existing no-marker clear/generate policy. - Correct the stale deletion expectation in `corrupt_keyring_recovers_valid_file_without_rotating`, retaining its identity and persistence checks. - Add an explicit no-delete assertion to `corrupt_keyring_with_valid_file_recovers_before_delete`. Production recovery behavior matches the contributor's fix; follow-up production-file edits only clarify comments and restore a doc comment. The existing migration durability order remains **store → uncached verification → marker → file removal**. Lost-state signing/startup gates and explicit user re-import remain intact. ### Related issue Fixes #6218. Linked continuation/replacement of #7103, not a competing implementation. The original PR remains open for maintainer disposition. Searched existing keyring PRs and identity/keyring issues; #7103 is the direct duplicate and source contribution. ### Testing Validated head: `b6a5512ba85f9e76a039f69714ed04a98eabc710`, based on `4365883151698cd30e31cf4091629543b61c2478`. - **Focused baseline:** `cd desktop/src-tauri && cargo test --lib app_state::` — 52 passed, 0 failed. - **Mutation check:** in an isolated copy, restore the original delete-before-file-probe ordering without changing the test file. `corrupt_keyring_with_valid_file_recovers_before_delete` fails at the new `store.deleted.borrow().is_empty()` assertion (0 passed, 1 failed; expected exit 101). The candidate working tree was never mutated. - **Actual pre-push hooks passed, without bypass:** `push-head-scope`, `branch-skew`, `file-size-check`, and `desktop-tauri-checks`. - `cargo clippy --manifest-path desktop/src-tauri/Cargo.toml --workspace --all-targets -- -D warnings` - `cd desktop/src-tauri && cargo test --workspace` — 3,156 passed, 0 failed, 20 ignored across test binaries. - Pre-commit Tauri formatting and DCO hooks passed. - Independent read-only review by Brain found no further defects in the final range and verified the baseline/mutation artifacts and contributor attribution. This is not a GitHub approval. Local validation used the repository's sidecar stubs for compilation. Full-repository `just ci`, a native-app recovery workflow, and live-keyring fault injection were not run. GitHub CI and an actual current-range Codex security review are still pending; local hooks do not substitute for those results. No UI markup changed, so screenshots are not applicable. ### Limits This prevents destructive cleanup on the marker-backed recovery path; it does not identify what originally made the keyring value unreadable or guarantee recovery of already-corrupt material. Markerless last-copy preservation remains a pre-existing limitation outside this change. Only the two app-state Rust files change; no new dependency, workflow, or secret-export surface is introduced. --------- Signed-off-by: Taksh Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> Co-authored-by: Taksh Co-authored-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> --- desktop/src-tauri/src/app_state.rs | 32 ++++++------- desktop/src-tauri/src/app_state_tests.rs | 58 ++++++++++++------------ 2 files changed, 45 insertions(+), 45 deletions(-) diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index 11b0728c91..08028ddbe9 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -634,23 +634,20 @@ fn resolve_identity_with_store( }) } -/// Recover from a corrupt nsec in the keyring (parse failed). Clear the bad -/// keyring value, then migrate a valid leftover `identity.key` if one exists. -/// If the migration marker is present but no valid file exists, the prior -/// identity is unrecoverable — return `Lost` recovery rather than silently -/// generating a new identity. Generating fresh is only correct when no prior -/// identity ever existed (no marker). The keyring delete is best-effort: a -/// delete failure logs and continues — it must never block startup. +/// Recover from an unparseable keyring nsec, preferring a valid `identity.key`. +/// If a migration marker exists without a valid file, retain the keyring value +/// and return `Lost`. Without a marker, preserve the existing generate-fresh policy. fn recover_from_keyring( store: &impl IdentityKeyStore, legacy_path: &std::path::Path, data_dir: &std::path::Path, error: &str, ) -> Result { - eprintln!("buzz-desktop: corrupt nsec in keyring ({error}), clearing and recovering from file"); - if let Err(e) = store.delete(IDENTITY_KEY_NAME) { - eprintln!("buzz-desktop: failed to clear corrupt keyring value: {e}"); - } + eprintln!( + "buzz-desktop: corrupt nsec in keyring ({error}), looking for a recovery path before clearing" + ); + // Marker-only installs have no file fallback. Keep unreadable keyring + // material until a replacement exists rather than destroying the only copy. if legacy_path.exists() { if let Some(keys) = migrate_identity_file(store, legacy_path, data_dir)? { return Ok(ResolvedIdentity { @@ -661,13 +658,13 @@ fn recover_from_keyring( } } // No valid file to recover from. If the migration marker exists, a prior - // identity was stored in the keyring and is now corrupt AND gone — the key - // is unrecoverable. Enter Lost recovery instead of silently rotating. + // identity was stored in the keyring — keep the corrupt entry for support / + // manual export and enter Lost rather than silently rotating. if migration_marker_path(data_dir).exists() { let ephemeral = Keys::generate(); eprintln!( - "buzz-desktop: identity lost — keyring had corrupt data and no valid identity.key \ - backup; prior identity (migration marker present) is unrecoverable; \ + "buzz-desktop: identity lost — keyring value failed to parse and no valid identity.key \ + backup exists; leaving the keyring entry in place; \ using ephemeral key {}, awaiting user re-import", ephemeral.public_key().to_hex() ); @@ -677,7 +674,10 @@ fn recover_from_keyring( storage: IdentityStorage::Ephemeral, }); } - // No marker: genuine first launch with a corrupt keyring. Generate fresh. + // No marker: preserve the existing clear-and-generate first-launch policy. + if let Err(e) = store.delete(IDENTITY_KEY_NAME) { + eprintln!("buzz-desktop: failed to clear corrupt keyring value: {e}"); + } let (keys, storage) = generate_and_persist(store, legacy_path, data_dir)?; Ok(ResolvedIdentity { keys, diff --git a/desktop/src-tauri/src/app_state_tests.rs b/desktop/src-tauri/src/app_state_tests.rs index 751bcf22e5..ceef4d3f93 100644 --- a/desktop/src-tauri/src/app_state_tests.rs +++ b/desktop/src-tauri/src/app_state_tests.rs @@ -326,8 +326,8 @@ fn corrupt_keyring_recovers_valid_file_without_rotating() { // nsec (Present) AND a valid `identity.key` is on disk (leftover from a // failed prior migration), recovery must RECOVER THE FILE'S identity — // not quarantine the file and rotate to a fresh key (the original - // hazard). The corrupt keyring value must be cleared and replaced by the - // file's key (migrated in). + // hazard). Recovery must overwrite the corrupt keyring value with the + // file's key without deleting the keyring entry first. let dir = tempfile::tempdir().unwrap(); let legacy_path = dir.path().join("identity.key"); let file_keys = Keys::generate(); @@ -338,8 +338,8 @@ fn corrupt_keyring_recovers_valid_file_without_rotating() { // The FILE's identity is recovered — NOT a freshly generated one. assert_key_eq(&file_keys, &resolved.keys); - // The corrupt keyring value was cleared. - assert_eq!(store.deleted.borrow().as_slice(), [IDENTITY_KEY_NAME]); + // Recovery overwrites the corrupt value without deleting first. + assert!(store.deleted.borrow().is_empty()); // The keyring now holds the file's key (migrated in, read-back verified). let file_nsec = file_keys.secret_key().to_bech32().unwrap(); assert_eq!( @@ -1363,13 +1363,9 @@ fn verify_fails_store_does_not_write_marker_or_delete_file() { ); } -// ── I2: corrupt keyring + marker = Lost recovery ────────────────────────── - #[test] fn corrupt_keyring_marker_present_no_file_is_lost() { - // I2: Present(corrupt) + migration marker + no identity.key → the prior - // identity was migrated into the keyring and is now unrecoverable (corrupt - // AND no file backup). Must enter Lost recovery, NOT generate a fresh key. + // I2: corrupt keyring + marker + no file → Lost (do not mint a fresh key). let dir = tempfile::tempdir().unwrap(); let legacy_path = dir.path().join("identity.key"); write_migration_marker(&migration_marker_path(dir.path())).unwrap(); @@ -1378,22 +1374,33 @@ fn corrupt_keyring_marker_present_no_file_is_lost() { let store = FakeIdentityStore::present_with("not-a-valid-nsec"); let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); - // Must enter Lost recovery — a prior identity existed and is now unrecoverable. - assert_eq!( - resolved.recovery, - RecoveryState::Lost, - "corrupt keyring + marker + no file must return Lost recovery, not a fresh key" - ); - - // No identity.key written — the ephemeral key is in-memory only. + assert_eq!(resolved.recovery, RecoveryState::Lost); + // Lost must keep the corrupt keyring entry for support/export. + assert!(!store + .deleted + .borrow() + .contains(&IDENTITY_KEY_NAME.to_string())); + assert!(store.slot.borrow().contains_key(IDENTITY_KEY_NAME)); assert!(!legacy_path.exists()); } +#[test] +fn corrupt_keyring_with_valid_file_recovers_before_delete() { + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + let file_keys = Keys::generate(); + save_key_file(&legacy_path, &file_keys).unwrap(); + write_migration_marker(&migration_marker_path(dir.path())).unwrap(); + let store = FakeIdentityStore::present_with("not-a-valid-nsec"); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + assert_eq!(resolved.recovery, RecoveryState::None); + assert_key_eq(&file_keys, &resolved.keys); + assert!(store.deleted.borrow().is_empty()); +} + #[test] fn corrupt_keyring_no_marker_no_file_generates_fresh() { - // I2 (counter-case): Present(corrupt) + NO marker + no identity.key → - // genuine first launch with a corrupt keyring, no prior identity to - // protect. generate_and_persist is still the correct last resort. + // I2 counter-case: corrupt keyring, no marker, no file → generate fresh. let dir = tempfile::tempdir().unwrap(); let legacy_path = dir.path().join("identity.key"); assert!(!legacy_path.exists()); @@ -1402,16 +1409,9 @@ fn corrupt_keyring_no_marker_no_file_generates_fresh() { let store = FakeIdentityStore::present_with("not-a-valid-nsec"); let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); - // No lost recovery — this is a fresh machine with no prior identity. - assert_eq!( - resolved.recovery, - RecoveryState::None, - "corrupt keyring + no marker + no file must generate a fresh key (no prior identity)" - ); - - // A fresh, valid key was stored (keyring or file). + assert_eq!(resolved.recovery, RecoveryState::None); assert!( store.slot.borrow().contains_key(IDENTITY_KEY_NAME) || legacy_path.exists(), - "a fresh key must be stored in the keyring or the file after generate_and_persist" + "fresh key must be stored after generate_and_persist" ); }