jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-10-27 04:30:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(desktop): preserve keyring identity during recovery (#7203)

Browse Source 
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 <takshkothari09@gmail.com>
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
Co-authored-by: Taksh <takshkothari09@gmail.com>
Co-authored-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
This commit is contained in:
Wes
2026-09-01 13:43:01 -06:00
committed by GitHub
parent 42b42447b0
commit e5a7e26a10
2 changed files with 45 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
desktop/src-tauri/src/app_state.rs
View File

@@ -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<ResolvedIdentity, String> {
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,

58
desktop/src-tauri/src/app_state_tests.rs
View File

@@ -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"
);
}