Skip to content

feat(desktop): retire keytar → safeStorage secret storage (dual-read migration)#772

Merged
h4yfans merged 2 commits into
mainfrom
keytar-safestorage-migration
Jul 17, 2026
Merged

feat(desktop): retire keytar → safeStorage secret storage (dual-read migration)#772
h4yfans merged 2 commits into
mainfrom
keytar-safestorage-migration

Conversation

@h4yfans

@h4yfans h4yfans commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

What

Retires the unmaintained keytar native module as the primary secret store. All five secret call sites (vault master key + sync keys, Google Calendar tokens, voice transcription API key, local agent provider API key, capture pairing token) now store secrets as Electron safeStorage ciphertext in an atomically written secure-secrets.json under userData, keyed by the exact account strings the OS keychain used.

Closes #766

Why

keytar is archived/unmaintained and is a native-module liability on every Electron/ABI bump. safeStorage ships with Electron, needs no native rebuild, and uses the same OS-level protection (Keychain on macOS, DPAPI on Windows, keyring-backed encryption on Linux).

Migration safety design

This is a live-beta production app — a botched migration means permanent vault-key loss. The design is defensive throughout:

  • Dual-read at all 5 sites: safeStorage store first, then keytar.getPassword fallback with the identical account string. Every existing account-suffix scheme is preserved (MEMRY_DEVICE per device, Google per-accountId + token kind, voice, pairing-token unsuffixed).
  • Verify-before-delete: per-key migrate-on-read does encrypt → persist (atomic temp-write + rename) → decrypt round-trip verify byte-identical against the source → only then keytar.deletePassword. Any verification failure keeps keytar authoritative, removes the bad ciphertext, and logs.
  • Master key gets an extra gate: its keytar delete is deferred until the retrieved key has passed the vault verifier check (confirmMasterKeyMigrated runs in getOrInitializeLocalVaultKey after bindOrVerifyVaultKey succeeds).
  • Availability gating: every write gates on safeStorage.isEncryptionAvailable() evaluated only after app ready. On Linux, the plaintext basic_text backend refuses migration entirely (logged) — no silent security downgrade.
  • Idempotent + crash-resumable: a crash between persist and keytar-delete leaves both copies identical; the next run serves the safeStorage copy and lazily deletes the keytar copy only while the two still match (fire-and-forget so a hung OS keychain can never block reads).
  • Atomic store file: temp file + rename in the same directory; a corrupt store file is moved aside (.corrupt-<ts>) instead of clobbered, with the keytar fallback still serving reads.
  • Plain-dev worktrees keep keytar authoritative: they share one dev master key machine-globally while userData is per-worktree; migrating would strand the shared key for other worktrees (would regress fix(crypto): stabilize dev keychain master-key account across worktrees #758).
  • keytar stays as a dependency, electron-rebuild target, and read-only fallback in this PR. Removal is staged behind a later release gated on field telemetry. The e2e keychain-hang test timeout now bounds the whole dual-read, so the keytar fallback path stays covered.

Test plan

42 new unit tests (src/main/secrets/), all with electron + keytar fully mocked (never touching the real OS keychain):

  • happy-path migration per entry type (master key, signing key, Google access/refresh, voice, local provider, pairing)
  • decrypt-verify failure → keytar stays authoritative, no delete, no bad ciphertext left shadowing
  • isEncryptionAvailable() === false → no migration, keytar reads/writes still work
  • Linux basic_text backend → refusal
  • dual-read fallback order (store wins over keytar)
  • idempotent re-run (keytar deleted exactly once)
  • crash-resume (store + keytar both present → clean lazy delete; mismatched copies → never deleted)
  • master-key deferral: keytar copy kept until confirmMasterKeyMigrated; vault-verifier reject → keytar copy survives
  • account-string suffix preservation for all 5 sites (including dev-<hash>dev normalization and the unsuffixed pairing token)
  • atomic write behavior (temp + rename; crash mid-rename leaves prior content intact, no temp litter)

Full desktop suite: 11010 passed. pnpm lint, pnpm typecheck, pnpm check:architecture, pnpm check:contracts, pnpm docs:build all green. No contracts/preload changes, so no IPC regeneration needed.

Rollout note

Merge should wait until #765 (Electron 43 upgrade) is confirmed stable in the field — this change deliberately rides on the post-upgrade safeStorage behavior and should not compound risk in the same release window.

h4yfans added 2 commits July 17, 2026 16:08
…allback

Retires the unmaintained keytar native module as the primary secret store.
Secrets now live as safeStorage ciphertext in an atomically written
secure-secrets.json under userData, keyed by the exact per-site account
strings (device suffixes, Google per-account token kinds, voice, pairing).

Migration safety:
- Dual-read at all five call sites: safeStorage store first, keytar
  fallback with the identical account string.
- Per-key migrate-on-read: encrypt, persist, decrypt round-trip verify
  byte-identical, only then delete the keytar copy. Any verify failure
  keeps keytar authoritative.
- The vault master key defers its keytar delete until the retrieved key
  has passed the vault verifier check (confirmMasterKeyMigrated).
- Writes gate on safeStorage.isEncryptionAvailable() evaluated only after
  app ready; Linux basic_text (plaintext) backend refuses migration.
- Idempotent and crash-resumable: a crash between persist and delete is
  cleaned up lazily on the next run, only while both copies match.
- Plain-dev worktrees keep keytar authoritative: they share one master
  key through the machine-global OS keychain while userData is
  per-worktree, so migrating would strand the other worktrees.

keytar stays a dependency, electron-rebuild target, and read-only
fallback for at least one release; removal is gated on field telemetry.
@github-actions github-actions Bot added documentation Improvements or additions to documentation enhancement New feature or request test labels Jul 17, 2026
@github-actions

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit 07898c0.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.43062% with 20 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
apps/desktop/src/main/secrets/secret-storage.ts 89.01% 19 Missing ⚠️
apps/desktop/src/main/capture/pairing.ts 75.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@h4yfans
h4yfans marked this pull request as ready for review July 17, 2026 20:43
@h4yfans
h4yfans merged commit e4b43a1 into main Jul 17, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Electron follow-up A: retire keytar → safeStorage (sync API) migration

1 participant