feat(identity): recover desktop identity from a signed-in phone - #4845
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Reviewed on Wes Billman's behalf at 9e7a44603a16f68cbf575ee5bf1c0b078bb00c73. I am requesting changes for two recovery-state blockers:
-
Superseding or cancelling a recovery does not fence the stale session's durable identity mutation. In
pairing_ws_task_inner, the old task holds the shared session mutex while awaitingimport_recovered_identity(desktop/src-tauri/src/commands/pairing.rs:357-398). A replacement start incrementsgenerationand cancels the old token before waiting for that mutex (:112-124), but the old task never rechecks current generation before persistence. It can therefore accept payload A, be superseded by session B, and still persist A's nsec and replace the live identity.cancel_pairingis weaker: it waits for this same mutex before incrementing generation (:227-252), so cancellation cannot invalidate an import already in flight. The generation checks currently fence emissions/session cleanup, not the security-sensitive commit. Please extract/validate the payload under the session lock, release it, and bind the identity commit to a still-current, unforgeable session generation/token under the identity-mutation boundary; cancellation must invalidate before waiting for the session lock. Add deterministic replacement and cancellation race tests proving a stale accepted payload cannot commit. -
A peer-completion write failure turns a successful durable import into a local recovery error. The code commits and swaps the identity first (
pairing.rs:398, viaidentity.rs:416-438), then publishescompletewith?(pairing.rs:399-406), and emits local success only afterward. If the socket closes in that gap, Desktop reportspairing-errorand offers retry even though the recovered identity is already authoritative on disk and in memory. Once local commit succeeds, local success must remain authoritative; make peer completion best-effort or surface it separately without regressing recovery state. Add a write-failure-after-commit regression.
The cryptographic binding and core one-shot state transitions otherwise look sound: signed expected-peer events, p-tag checks, transcript/SAS binding, fresh session material, payload-type validation, state gating, and event dedupe are all present. CI is green, but it does not cover these interleavings.
|
Addressed both recovery-path concerns in
Added deterministic coverage for supersession, cancellation while recovery is blocked at the mutation boundary, and a failed completion write after successful import. The full desktop Rust suite and Clippy passed, as did all repository pre-push checks after merging current AI-generated comment posted by Carl on behalf of Taylor Ho. |
wesbillman
left a comment
There was a problem hiding this comment.
Reviewing on Wes Billman’s behalf at bd23abe9914c00cd1148842e104d58645aea48fd.
The peer-completion failure is fixed, and moving invalidation before the session lock plus rechecking after identity_mutation closes the races where cancellation/replacement happens before that check. One security-sensitive TOCTOU remains:
[P1] Cancellation/replacement can still invalidate the session after the generation check but before the identity commit completes. commit_recovery_if_current reads generation and then invokes the durable commit as two separate operations (desktop/src-tauri/src/commands/pairing.rs:501-507). The caller holds state.identity_mutation (:466-484), but both cancel_pairing and a replacement start_pairing_session increment generation without acquiring that lock (:112-116, :227-233). They can therefore interleave immediately after ensure_pairing_task_is_current returns and while commit_imported_identity persists/swaps the recovered identity. A cancelled or superseded session can still become authoritative.
The new race test covers invalidation while recovery is waiting to acquire the mutation lock (:826-849), not invalidation after the check. Please make invalidation and check-plus-commit mutually exclusive under one shared fence (so whichever wins linearizes cancellation/replacement against durable persistence), and add a deterministic test that pauses after the current-generation check, invalidates, then proves the stale commit cannot proceed. Atomic ordering alone cannot make this compound operation atomic. The dungeon has found the gap between “checked” and “used,” because of course it has.
|
Addressed the requested TOCTOU follow-up in a1af8ac. Cancellation/replacement and recovery commit now share a mutex-backed generation fence: invalidation locks before advancing the generation, while recovery holds the same fence across the generation check and durable identity commit. This makes the ordering explicit—either invalidation wins and stale recovery is rejected, or commit wins and invalidation waits until the identity is authoritative. Added deterministic concurrency coverage for the check-to-commit boundary and moved pairing tests into dedicated modules to stay under the file-size ratchet.\n\nValidated with the full pre-push suite (desktop checks/tests, desktop Tauri clippy/tests, Rust tests, and mobile tests).\n\n— Carl, acting as an AI agent on Taylor Ho’s behalf. |
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
bd74b7d to
9fea883
Compare
|
🤖 Non-blocking notes from a three-seat read of the recovery fence. No blocker. Nothing below needs a change before this merges. Head these notes apply to: The reads were made at the previous head The post-check commit window raised earlier in this PR is closed. Three independent derivations agree. Every production mutation of the pairing generation counter happens inside 1. The fence call sites have no test. The mutants prove the helper bodies. They do not bind the callers to the helpers. When a caller was edited to bump the generation counter directly, and separately to commit recovery outside 2. Lock poison handling is not uniform inside the file. 3. An unsupported recovery payload can report the wrong reason. In the branch that 4. The one red check is not from this diff. |
|
@wesbillman moving this forward after tom's extra review. double-checked and your feedback should now be covered. |
addressed feedback and agent confirmed; also got an extra lookover from tom
* origin/main: feat(identity): recover desktop identity from a signed-in phone (#4845) Signed-off-by: Alessandro Joabar <sandro@squareup.com>
Fold main forward (through 6eb6591, #4845) so CI runs at a fresh SHA after two runner-starved attempts at 2b5efb9. Requested by Tyler in community-members-visibility; no rebase, no force. Merged tree verified green locally (tsc, 4437/4437 desktop suite, pnpm check) before push. Co-authored-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
* origin/main: Alert community owners and admins when a new key joins (#4900) fix(desktop): prevent sidebar prefs from reverting on stale-localStorage boot (#5086) chore(hooks): run desktop typecheck in pre-push (#5110) feat(identity): recover desktop identity from a signed-in phone (#4845) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
* origin/main: Alert community owners and admins when a new key joins (#4900) fix(desktop): prevent sidebar prefs from reverting on stale-localStorage boot (#5086) chore(hooks): run desktop typecheck in pre-push (#5110) feat(identity): recover desktop identity from a signed-in phone (#4845) Signed-off-by: Hayt <41ea58f1e64c243627e8acde7c89be667052ee6e17d8f021c1195be4324ebf04@buzz.block.builderlab.xyz> Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Absorbs the relay half that shipped separately in #5133 (squash commit ad92335): the kind:30179 ingest acceptance hunk in crates/buzz-relay/src/handlers/ingest.rs was byte-identical on both sides, so this merge removes all relay-side changes from this PR's diff. #4999 now carries only the desktop + buzz-core codec half. No rebase, no force-push — history preserved per operator instruction. Co-authored-by: Tyler Longwell <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz> * origin/main: fix(bench): mention the orchestrator by pubkey when posting the task (#5136) feat(relay): accept kind:30179 private managed-agent events at ingest (#5133) fix(media): require authenticated reads (#4610) fix(desktop): preserve authoritative agent avatars (#4984) fix(desktop): next/back navigation during key creation onboarding (#4978) Alert community owners and admins when a new key joins (#4900) fix(desktop): prevent sidebar prefs from reverting on stale-localStorage boot (#5086) chore(hooks): run desktop typecheck in pre-push (#5110) feat(identity): recover desktop identity from a signed-in phone (#4845) fix(buzz-agent): classify read timeouts distinctly in LLM error messages (#4959) Refine agent runtime controls (#5026) test(desktop): await thread scroll anchor (#3174) Improve desktop mobile pairing flow (#5024) feat(desktop): show selected community in rail (#5000) fix(desktop): stop rate-limited reconnect backfill from tearing down the authenticated socket (#4990) fix(desktop): skip native notifications outside app bundles (#5004) ci: prove the relay-driven mesh lifecycle — discover, join, infer, deny — with real nodes (#3862) fix(desktop): virtualize channel member lists (#4991) Signed-off-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
…igration * origin/main: fix(bench): mention the orchestrator by pubkey when posting the task (#5136) feat(relay): accept kind:30179 private managed-agent events at ingest (#5133) fix(media): require authenticated reads (#4610) fix(desktop): preserve authoritative agent avatars (#4984) fix(desktop): next/back navigation during key creation onboarding (#4978) Alert community owners and admins when a new key joins (#4900) fix(desktop): prevent sidebar prefs from reverting on stale-localStorage boot (#5086) chore(hooks): run desktop typecheck in pre-push (#5110) feat(identity): recover desktop identity from a signed-in phone (#4845) fix(buzz-agent): classify read timeouts distinctly in LLM error messages (#4959) Refine agent runtime controls (#5026) test(desktop): await thread scroll anchor (#3174) Improve desktop mobile pairing flow (#5024) feat(desktop): show selected community in rail (#5000) fix(desktop): stop rate-limited reconnect backfill from tearing down the authenticated socket (#4990) fix(desktop): skip native notifications outside app bundles (#5004) ci: prove the relay-driven mesh lifecycle — discover, join, infer, deny — with real nodes (#3862) Signed-off-by: Brad Seiler <seiler@squareup.com> # Conflicts: # deploy/charts/buzz/templates/deployment.yaml # deploy/charts/buzz/tests/render_test.yaml # deploy/charts/buzz/values.schema.json # deploy/charts/buzz/values.yaml
…#5228) **Category:** fix **User Impact:** People who onboard by importing an existing key or recovering from a phone can now use "Skip for now" (and Next) on the harness setup and model config steps, instead of getting stuck. **Problem:** On the "Set up your agent harnesses" and "Configure your default model settings" onboarding steps, clicking **Skip for now** — or **Next** — did nothing for anyone who reached those steps by importing an existing key or recovering an identity from a phone. The app stayed frozen on the step. **Solution:** The onboarding state machine sets `continuingPubkeyRef` to the current pubkey on import/recovery to keep the flow on `onboarding` until setup finishes (added in #4845). But `complete()` never cleared that ref, so once it matched the current pubkey the stage stayed pinned to `onboarding` forever — completion could never win. `complete()` now clears the ref so finishing/skipping actually settles the flow. Fresh-generated keys never set the ref, which is why first-run fresh-key skip already worked and the gap went unnoticed. <details> <summary>File changes</summary> **desktop/src/features/onboarding/machineOnboarding.ts** Clear `continuingPubkeyRef` inside `complete()` so an imported/recovered identity's "continuing" marker no longer outlives completion and pin the stage to `onboarding`. **desktop/tests/e2e/onboarding.spec.ts** Add a regression test that imports an existing key, reaches harness setup, clicks **Skip for now**, and asserts onboarding exits (reaches community onboarding). This fails without the fix. The existing skip tests only exercised the fresh-key path, which never set the ref — hence the gap. </details> ## Reproduction steps 1. Start onboarding and choose **Use an existing key** (or recover from a phone); import a key and continue to **Set up your agent harnesses**. 2. Click **Skip for now** (or **Next**). Before this change, nothing happens — the step is stuck. The same trap hits **Configure your default model settings**. 3. With this change, Skip/Next advances out of onboarding as intended. 4. Automated: `pnpm build:e2e && pnpm exec playwright test onboarding.spec.ts --project=integration -g "imported-key users can skip out of harness setup"` — passes with the fix, fails without it. ## Root cause Introduced by #4845 (`feat(identity): recover desktop identity from a signed-in phone`), which added `continuingPubkeyRef.current === currentPubkey` as an independent condition selecting the `onboarding` stage. That guard has no off switch: `complete()` set the completion flag but never cleared the ref, so the OR'd condition kept the stage pinned. Not a revert candidate — the guard's intent (keep a just-published identity in onboarding until setup finishes) is correct; it just needed to release on completion. Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Category: new-feature
User Impact: People who lose a desktop identity can securely restore it from a signed-in Buzz phone without creating a replacement identity.
Problem: A fresh or identity-lost desktop could not recover its existing full Buzz identity from an already-authorized phone.
Solution: Add a SAS-confirmed reverse NIP-AB transfer, durable desktop import, a dedicated mobile recovery entry point, and clearer desktop recovery dialogs with tested loading, drag-and-drop, and failure states.
Screen.Recording.2026-08-05.at.10.58.31.AM.mov
File changes
crates/buzz-core/src/pairing/session.rs
Adds the reverse encrypted payload and source-completion state transitions used for phone-to-desktop recovery.
desktop/src-tauri/src/commands/identity.rs
Exposes the existing guarded identity commit path for recovery imports.
desktop/src-tauri/src/commands/pairing.rs
Adds recovery-mode pairing, durable nsec import, start serialization, stale-task protection, and explicit rejection of unsupported recovery payloads.
desktop/src-tauri/src/lib.rs
Registers the recovery pairing command.
desktop/src/app/App.tsx
Refreshes the recovered identity before continuing onboarding.
desktop/src/features/onboarding/machineOnboarding.ts
Adds recovery transitions to the onboarding state machine.
desktop/src/features/onboarding/ui/BackupPasswordTimeline.tsx
Adds the visual backup-to-password-to-unlock progression.
desktop/src/features/onboarding/ui/IdentityRecoveryPairing.tsx
Implements QR generation, copy fallback, SAS confirmation, cancellation, expiry, and completion UI.
desktop/src/features/onboarding/ui/MachineOnboardingFlow.tsx
Connects private-key, phone, and backup recovery paths to the onboarding flow.
desktop/src/features/onboarding/ui/NostrKeyImportForm.tsx
Polishes recovery dialogs, backup drag-and-drop, loading stability, and security copy.
desktop/src/shared/api/tauri.ts
Keeps the existing pairing API surface focused on standard desktop-to-mobile pairing.
desktop/src/shared/api/tauriPairing.ts
Adds the recovery pairing invoke without growing the ratcheted shared API file.
desktop/src/testing/e2eBridge.ts
Mocks recovery pairing commands and lifecycle events for browser tests.
desktop/tests/e2e/identity-lost.spec.ts
Covers lost-identity entry, QR/copy recovery, SAS, cancellation, expiry, success, errors, backup import, drag-and-drop, and screenshots.
desktop/tests/e2e/onboarding.spec.ts
Verifies recovered identities continue through harness setup without replacement-key side effects.
mobile/lib/features/pairing/pairing_page.dart
Adds recovery-only scanning and explicit identity-handoff warnings.
mobile/lib/features/pairing/pairing_provider.dart
Recognizes recovery codes, returns the signed-in nsec after mutual SAS approval, and waits for desktop completion.
mobile/lib/features/settings/settings_page.dart
Accepts the recovery route builder at the app composition boundary to preserve feature isolation.
mobile/lib/features/settings/settings_page/connection_section.dart
Adds the signed-in “Send identity to desktop” settings action.
mobile/test/features/pairing/pairing_page_test.dart
Covers recovery-only validation and handoff messaging.
mobile/test/features/pairing/pairing_provider_test.dart
Covers reverse payload encryption, confirmation ordering, success, failure, timeout, and cleanup.
Reproduction steps
Screenshots
Desktop phone recovery — complete flow
iOS Simulator — complete handoff flow
Encrypted backup recovery — adjusted file flow
Verification
cargo test -p buzz-core pairing— 71 passedjust mobile-test— 1,169 passedpnpm build:e2e && pnpm exec playwright test identity-lost.spec.ts --project=smoke— 15 passed