From d07ce15cff7836ee36f34ccc7bdf1fca251f299d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 22 Aug 2026 16:38:00 -0700 Subject: [PATCH] refactor(host): route secret storage through a SecretStore port (#15916) --- config/scripts/vitest-secret-store-setup.ts | 29 ++++++++ config/vitest.config.ts | 5 +- .../bitbucket/credential-connection.test.ts | 14 ++-- src/main/bitbucket/credential-store.test.ts | 14 ++-- src/main/bitbucket/status-no-decrypt.test.ts | 14 ++-- src/main/host/electron-secret-store.test.ts | 61 +++++++++++++++++ src/main/host/electron-secret-store.ts | 31 +++++++++ src/main/index.ts | 7 ++ src/main/integration-credential-file.ts | 14 ++-- src/main/jira/client.test.ts | 12 ++-- src/main/jira/site-credential-store.ts | 8 +-- src/main/linear/client.test.ts | 14 ++-- src/main/linear/linear-token-store.ts | 8 +-- ...tence-protected-secret-fail-closed.test.ts | 52 ++++++++------- ...stence-protected-secret-write-race.test.ts | 14 ++-- .../persistence-proxy-secret-recovery.test.ts | 42 ++++++------ src/main/persistence-single-serialize.test.ts | 32 ++++----- src/main/persistence-ui-state.test.ts | 24 ++++--- src/main/protected-secret-persistence.test.ts | 29 +++++--- src/main/protected-secret-persistence.ts | 19 ++++-- src/main/speech/openai-api-key-store.test.ts | 8 ++- src/main/speech/openai-api-key-store.ts | 18 +++-- src/shared/secret-store.test.ts | 53 +++++++++++++++ src/shared/secret-store.ts | 66 +++++++++++++++++++ 24 files changed, 436 insertions(+), 152 deletions(-) create mode 100644 config/scripts/vitest-secret-store-setup.ts create mode 100644 src/main/host/electron-secret-store.test.ts create mode 100644 src/main/host/electron-secret-store.ts create mode 100644 src/shared/secret-store.test.ts create mode 100644 src/shared/secret-store.ts diff --git a/config/scripts/vitest-secret-store-setup.ts b/config/scripts/vitest-secret-store-setup.ts new file mode 100644 index 00000000000..6000e28c768 --- /dev/null +++ b/config/scripts/vitest-secret-store-setup.ts @@ -0,0 +1,29 @@ +import { beforeEach } from 'vitest' +import { setSecretStore } from '../../src/shared/secret-store' + +/** + * Why: `getSecretStore()` throws until an entrypoint installs a store, which is the + * right production behaviour but would fail ~67 suites that only ever cared that + * *some* store existed. Install a reversible in-memory one before every test so + * those suites stay unchanged; a suite that asserts on sealing behaviour calls + * `setSecretStore()` itself and wins, because this runs first. + * + * Deliberately not a plaintext passthrough: encryptString must return something a + * test can tell apart from the input, or a test that forgot to seal would pass. + */ +const SEAL_PREFIX = 'vitest-sealed:' + +beforeEach(() => { + setSecretStore({ + isEncryptionAvailable: () => true, + encryptString: (plainText) => Buffer.from(`${SEAL_PREFIX}${plainText}`), + decryptString: (cipher) => { + const text = cipher.toString() + if (!text.startsWith(SEAL_PREFIX)) { + throw new Error('vitest secret store: ciphertext was not produced by this store') + } + return text.slice(SEAL_PREFIX.length) + }, + describeUnavailable: () => null + }) +}) diff --git a/config/vitest.config.ts b/config/vitest.config.ts index d23bee128c9..745ff8c263d 100644 --- a/config/vitest.config.ts +++ b/config/vitest.config.ts @@ -19,7 +19,10 @@ export default defineConfig({ // Why --expose-gc: retention tests need a deterministic collection point to measure what a queue really holds. execArgv: ['--no-experimental-webstorage', '--expose-gc'], // Why: happy-dom drops MutationObserver callbacks on GC; keep them alive like a browser does. - setupFiles: [resolve('config/scripts/happy-dom-mutation-observer-retention.ts')], + setupFiles: [ + resolve('config/scripts/happy-dom-mutation-observer-retention.ts'), + resolve('config/scripts/vitest-secret-store-setup.ts') + ], include: [ 'src/**/*.test.ts', 'src/**/*.test.tsx', diff --git a/src/main/bitbucket/credential-connection.test.ts b/src/main/bitbucket/credential-connection.test.ts index 0026e6901cb..0d1a7e632bb 100644 --- a/src/main/bitbucket/credential-connection.test.ts +++ b/src/main/bitbucket/credential-connection.test.ts @@ -10,13 +10,13 @@ let tempHome = '' async function loadModule() { vi.resetModules() - vi.doMock('electron', () => ({ - safeStorage: { - isEncryptionAvailable: () => true, - encryptString: (value: string) => Buffer.from(value), - decryptString: (value: Buffer) => value.toString('utf-8') - } - })) + const { setSecretStore } = await import('../../shared/secret-store') + setSecretStore({ + isEncryptionAvailable: () => true, + encryptString: (value) => Buffer.from(value), + decryptString: (value) => value.toString('utf-8'), + describeUnavailable: () => null + }) vi.doMock('node:os', async () => { const actual = await vi.importActual('node:os') return { ...actual, homedir: () => tempHome } diff --git a/src/main/bitbucket/credential-store.test.ts b/src/main/bitbucket/credential-store.test.ts index 4edd11b0444..601831a5930 100644 --- a/src/main/bitbucket/credential-store.test.ts +++ b/src/main/bitbucket/credential-store.test.ts @@ -19,13 +19,13 @@ async function loadStore( // Why: doMock registrations outlive resetModules, so an injected failure from // one case would leak into every later one in this file. vi.doUnmock('node:fs') - vi.doMock('electron', () => ({ - safeStorage: { - isEncryptionAvailable: () => true, - encryptString: (value: string) => Buffer.from(value), - decryptString: decryptStringMock - } - })) + const { setSecretStore } = await import('../../shared/secret-store') + setSecretStore({ + isEncryptionAvailable: () => true, + encryptString: (value) => Buffer.from(value), + decryptString: decryptStringMock, + describeUnavailable: () => null + }) vi.doMock('node:os', async () => { const actual = await vi.importActual('node:os') return { ...actual, homedir: () => tempHome } diff --git a/src/main/bitbucket/status-no-decrypt.test.ts b/src/main/bitbucket/status-no-decrypt.test.ts index 946f1aea4aa..05543108ea6 100644 --- a/src/main/bitbucket/status-no-decrypt.test.ts +++ b/src/main/bitbucket/status-no-decrypt.test.ts @@ -19,13 +19,13 @@ vi.mock('../git/runner', () => ({ gitExecFileAsync: vi.fn() })) async function loadModules() { vi.resetModules() - vi.doMock('electron', () => ({ - safeStorage: { - isEncryptionAvailable: () => true, - encryptString: (value: string) => Buffer.from(value), - decryptString: decryptSpy - } - })) + const { setSecretStore } = await import('../../shared/secret-store') + setSecretStore({ + isEncryptionAvailable: () => true, + encryptString: (value) => Buffer.from(value), + decryptString: decryptSpy, + describeUnavailable: () => null + }) vi.doMock('node:os', async () => { const actual = await vi.importActual('node:os') return { ...actual, homedir: () => tempHome } diff --git a/src/main/host/electron-secret-store.test.ts b/src/main/host/electron-secret-store.test.ts new file mode 100644 index 00000000000..3c171d7d499 --- /dev/null +++ b/src/main/host/electron-secret-store.test.ts @@ -0,0 +1,61 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const safeStorageMock = vi.hoisted(() => ({ + isEncryptionAvailable: vi.fn(() => true), + encryptString: vi.fn((plainText: string) => Buffer.from(`os-sealed:${plainText}`)), + decryptString: vi.fn((cipher: Buffer) => cipher.toString().slice('os-sealed:'.length)) +})) + +vi.mock('electron', () => ({ safeStorage: safeStorageMock })) + +const { ElectronSecretStore } = await import('./electron-secret-store') + +describe('ElectronSecretStore', () => { + beforeEach(() => { + vi.clearAllMocks() + safeStorageMock.isEncryptionAvailable.mockReturnValue(true) + }) + + // Why this shape: the whole safety argument for the SecretStore refactor is that the + // desktop byte path did not change. That is only true if this adapter forwards + // verbatim — same argument, same return value, no re-encoding. + it('forwards encryptString to safeStorage and returns its buffer unchanged', () => { + const sealed = new ElectronSecretStore().encryptString('token') + expect(safeStorageMock.encryptString).toHaveBeenCalledExactlyOnceWith('token') + expect(sealed).toBe(safeStorageMock.encryptString.mock.results[0]!.value) + }) + + it('forwards decryptString to safeStorage and returns its string unchanged', () => { + const cipher = Buffer.from('os-sealed:token') + expect(new ElectronSecretStore().decryptString(cipher)).toBe('token') + expect(safeStorageMock.decryptString).toHaveBeenCalledExactlyOnceWith(cipher) + }) + + // Why the narrower claim: safeStorage is mocked here, so this proves the adapter + // pairs encrypt/decrypt without mangling the buffer — NOT that credentials sealed by + // a previous build still open. Real-ciphertext compatibility needs a captured fixture. + it('pairs encryptString and decryptString without altering the payload', () => { + const store = new ElectronSecretStore() + expect(store.decryptString(store.encryptString('linear-token'))).toBe('linear-token') + }) + + it('reports availability from safeStorage rather than caching it', () => { + const store = new ElectronSecretStore() + expect(store.isEncryptionAvailable()).toBe(true) + safeStorageMock.isEncryptionAvailable.mockReturnValue(false) + expect(store.isEncryptionAvailable()).toBe(false) + }) + + it('has no reason to give while sealing works', () => { + expect(new ElectronSecretStore().describeUnavailable()).toBeNull() + }) + + it('names the missing facility when sealing is unavailable, so the plaintext fallback is explainable', () => { + safeStorageMock.isEncryptionAvailable.mockReturnValue(false) + const reason = new ElectronSecretStore().describeUnavailable() + expect(reason).toContain('unencrypted') + if (process.platform === 'linux') { + expect(reason).toContain('keyring') + } + }) +}) diff --git a/src/main/host/electron-secret-store.ts b/src/main/host/electron-secret-store.ts new file mode 100644 index 00000000000..63d4bb78d9b --- /dev/null +++ b/src/main/host/electron-secret-store.ts @@ -0,0 +1,31 @@ +import { safeStorage } from 'electron' +import type { SecretStore } from '../../shared/secret-store' + +/** + * Electron-backed SecretStore for the desktop app: a pass-through to + * `electron.safeStorage`, which seals against the OS keychain. + */ +export class ElectronSecretStore implements SecretStore { + isEncryptionAvailable(): boolean { + return safeStorage.isEncryptionAvailable() + } + + encryptString(plainText: string): Buffer { + return safeStorage.encryptString(plainText) + } + + decryptString(cipher: Buffer): string { + return safeStorage.decryptString(cipher) + } + + describeUnavailable(): string | null { + if (safeStorage.isEncryptionAvailable()) { + return null + } + // Why platform-specific: the fix differs, and "encryption unavailable" alone + // sends users looking in the wrong place. + return process.platform === 'linux' + ? 'The OS keyring is unavailable, so secrets are stored unencrypted. Install and unlock gnome-keyring or kwallet to seal them.' + : 'The OS keychain is unavailable, so secrets are stored unencrypted.' + } +} diff --git a/src/main/index.ts b/src/main/index.ts index db9cc9506b6..729a613e4ff 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -11,6 +11,8 @@ import { getCanonicalUserDataPath, migrateMobilePairingDataToCanonicalUserDataPath } from './persistence' +import { setSecretStore } from '../shared/secret-store' +import { ElectronSecretStore } from './host/electron-secret-store' import { initSessionParseCachePersistence } from './ai-vault/session-parse-cache-persistence' import { ensureActiveOrcaProfile, initOrcaProfilePaths } from './orca-profiles/profile-index-store' import { getOrcaCloudAuthConfig } from './orca-profiles/profile-cloud-auth-config' @@ -850,6 +852,11 @@ if (!hasSingleInstanceLock) { // Why: when another process holds the lock we've already exited; skip file-writing side effects so this transient process never touches userData. if (hasSingleInstanceLock) { + // Why: every secret read resolves through this port, and getSecretStore() throws + // until it is installed — so install before any other bootstrap step can touch a + // credential. Constructing it does not call safeStorage, so this stays clear of the + // pre-ready Keychain service-name resolution the setName block below depends on. + setSecretStore(new ElectronSecretStore()) // Why: couple to dev-parent only for electron-vite desktop runs; `orca serve`'s parent (CLI shim/background shell) isn't the intended server lifetime. const shouldCoupleToDevParent = is.dev && !isServeMode installDevParentDisconnectQuit(shouldCoupleToDevParent) diff --git a/src/main/integration-credential-file.ts b/src/main/integration-credential-file.ts index 96823a77a65..01a85c28b51 100644 --- a/src/main/integration-credential-file.ts +++ b/src/main/integration-credential-file.ts @@ -8,7 +8,7 @@ import { unlinkSync, writeSync } from 'node:fs' -import { safeStorage } from 'electron' +import { getSecretStore } from '../shared/secret-store' import { credentialDecryptionMessage, type IntegrationCredentialService @@ -31,12 +31,12 @@ export function writeEncryptedCredential( path: string, value: string ): void { - if (safeStorage.isEncryptionAvailable()) { - writeCredentialFileAtomic(path, safeStorage.encryptString(value)) + if (getSecretStore().isEncryptionAvailable()) { + writeCredentialFileAtomic(path, getSecretStore().encryptString(value)) return } console.warn( - `[${service.toLowerCase()}] safeStorage encryption unavailable — storing credential in plaintext` + `[${service.toLowerCase()}] secret encryption unavailable — storing credential in plaintext` ) writeCredentialFileAtomic(path, Buffer.from(value, 'utf-8')) } @@ -111,9 +111,9 @@ export function readStoredCredentialToken( return null } - if (safeStorage.isEncryptionAvailable()) { + if (getSecretStore().isEncryptionAvailable()) { try { - return usableToken(safeStorage.decryptString(raw)) + return usableToken(getSecretStore().decryptString(raw)) } catch { return readPlaintextLegacyCredential(service, raw) } @@ -127,7 +127,7 @@ function readPlaintextLegacyCredential( raw: Buffer ): string | null { const plaintext = decodeUtf8(raw) - // Why: legacy plaintext tokens are printable UTF-8; safeStorage ciphertext + // Why: legacy plaintext tokens are printable UTF-8; sealed ciphertext // such as macOS v10 blobs must not be decoded into auth-header junk. if (plaintext === null || hasControlCharacter(plaintext)) { throw new CredentialDecryptionError(service) diff --git a/src/main/jira/client.test.ts b/src/main/jira/client.test.ts index faee0d2eaa4..4e77150e72b 100644 --- a/src/main/jira/client.test.ts +++ b/src/main/jira/client.test.ts @@ -93,11 +93,6 @@ async function loadClientModule(options: SafeStorageMockOptions = {}) { vi.resetModules() vi.doMock('electron', () => ({ net: { fetch: netFetchMock }, - safeStorage: { - isEncryptionAvailable: () => options.encryptionAvailable ?? false, - encryptString: (value: string) => Buffer.from(value), - decryptString: options.decryptString ?? ((value: Buffer) => value.toString('utf-8')) - }, session: { defaultSession: { closeAllConnections: closeAllConnectionsMock, @@ -106,6 +101,13 @@ async function loadClientModule(options: SafeStorageMockOptions = {}) { } } })) + const { setSecretStore } = await import('../../shared/secret-store') + setSecretStore({ + isEncryptionAvailable: () => options.encryptionAvailable ?? false, + encryptString: (value) => Buffer.from(value), + decryptString: options.decryptString ?? ((value) => value.toString('utf-8')), + describeUnavailable: () => null + }) vi.doMock('os', async () => { const actual = await vi.importActual('os') return { ...actual, homedir: () => tempHome } diff --git a/src/main/jira/site-credential-store.ts b/src/main/jira/site-credential-store.ts index 0f8933097d3..63c241ca62c 100644 --- a/src/main/jira/site-credential-store.ts +++ b/src/main/jira/site-credential-store.ts @@ -1,7 +1,7 @@ import { existsSync, mkdirSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs' import { homedir } from 'node:os' import { join } from 'node:path' -import { safeStorage } from 'electron' +import { getSecretStore } from '../../shared/secret-store' import { CredentialDecryptionError, credentialFileHasContent, @@ -157,11 +157,11 @@ export function writeSiteFile(file: JiraSiteFile): void { } function writeEncryptedToken(path: string, apiToken: string): void { - if (safeStorage.isEncryptionAvailable()) { - writeFileSync(path, safeStorage.encryptString(apiToken), { mode: 0o600 }) + if (getSecretStore().isEncryptionAvailable()) { + writeFileSync(path, getSecretStore().encryptString(apiToken), { mode: 0o600 }) return } - console.warn('[jira] safeStorage encryption unavailable — storing token in plaintext') + console.warn('[jira] secret encryption unavailable — storing token in plaintext') writeFileSync(path, apiToken, { encoding: 'utf-8', mode: 0o600 }) } diff --git a/src/main/linear/client.test.ts b/src/main/linear/client.test.ts index 7f518afe6e8..f2105a250de 100644 --- a/src/main/linear/client.test.ts +++ b/src/main/linear/client.test.ts @@ -90,13 +90,13 @@ async function loadClientModule(options: SafeStorageMockOptions = {}) { }) }) }) - vi.doMock('electron', () => ({ - safeStorage: { - isEncryptionAvailable: () => options.encryptionAvailable ?? false, - encryptString: (value: string) => Buffer.from(value), - decryptString: options.decryptString ?? ((value: Buffer) => value.toString('utf-8')) - } - })) + const { setSecretStore } = await import('../../shared/secret-store') + setSecretStore({ + isEncryptionAvailable: () => options.encryptionAvailable ?? false, + encryptString: (value) => Buffer.from(value), + decryptString: options.decryptString ?? ((value) => value.toString('utf-8')), + describeUnavailable: () => null + }) vi.doMock('os', async () => { const actual = await vi.importActual('os') return { ...actual, homedir: () => tempHome } diff --git a/src/main/linear/linear-token-store.ts b/src/main/linear/linear-token-store.ts index 4d724c32a00..7a2bf892213 100644 --- a/src/main/linear/linear-token-store.ts +++ b/src/main/linear/linear-token-store.ts @@ -1,4 +1,4 @@ -import { safeStorage } from 'electron' +import { getSecretStore } from '../../shared/secret-store' import { existsSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs' import { LEGACY_WORKSPACE_ID, @@ -33,13 +33,13 @@ import { import type { LinearWorkspace } from '../../shared/linear/workspace-types' function writeEncryptedToken(path: string, apiKey: string): void { - if (safeStorage.isEncryptionAvailable()) { - const encrypted = safeStorage.encryptString(apiKey) + if (getSecretStore().isEncryptionAvailable()) { + const encrypted = getSecretStore().encryptString(apiKey) writeFileSync(path, encrypted, { mode: 0o600 }) return } - console.warn('[linear] safeStorage encryption unavailable — storing token in plaintext') + console.warn('[linear] secret encryption unavailable — storing token in plaintext') writeFileSync(path, apiKey, { encoding: 'utf-8', mode: 0o600 }) } diff --git a/src/main/persistence-protected-secret-fail-closed.test.ts b/src/main/persistence-protected-secret-fail-closed.test.ts index 42d348af61c..9d407ab505b 100644 --- a/src/main/persistence-protected-secret-fail-closed.test.ts +++ b/src/main/persistence-protected-secret-fail-closed.test.ts @@ -19,31 +19,7 @@ vi.mock('./ssh/ssh-config-parser', () => ({ })) vi.mock('electron', () => ({ - app: { getPath: () => testState.dir }, - safeStorage: { - isEncryptionAvailable: () => { - if (cipherState.availability === 'throws') { - throw new Error('keychain access denied') - } - return cipherState.availability === 'available' - }, - encryptString: (plaintext: string) => { - if (cipherState.encryptionThrows) { - throw new Error('keychain encryption failed') - } - return Buffer.from(`enc:${randomUUID()}:${plaintext}`, 'utf-8') - }, - decryptString: (ciphertext: Buffer) => { - if (cipherState.decryptionThrows) { - throw new Error('keychain decryption failed') - } - const decoded = ciphertext.toString('utf-8') - if (!decoded.startsWith('enc:')) { - throw new Error('invalid ciphertext') - } - return decoded.slice('enc:'.length + 36 + 1) - } - } + app: { getPath: () => testState.dir } })) vi.mock('./telemetry/client', () => ({ track: vi.fn() })) @@ -53,6 +29,32 @@ vi.mock('./telemetry/cohort-classifier', () => ({ async function createStore() { vi.resetModules() + const { setSecretStore } = await import('../shared/secret-store') + setSecretStore({ + isEncryptionAvailable: () => { + if (cipherState.availability === 'throws') { + throw new Error('keychain access denied') + } + return cipherState.availability === 'available' + }, + encryptString: (plaintext) => { + if (cipherState.encryptionThrows) { + throw new Error('keychain encryption failed') + } + return Buffer.from(`enc:${randomUUID()}:${plaintext}`, 'utf-8') + }, + decryptString: (ciphertext) => { + if (cipherState.decryptionThrows) { + throw new Error('keychain decryption failed') + } + const decoded = ciphertext.toString('utf-8') + if (!decoded.startsWith('enc:')) { + throw new Error('invalid ciphertext') + } + return decoded.slice('enc:'.length + 36 + 1) + }, + describeUnavailable: () => null + }) const { Store, initDataPath } = await import('./persistence') initDataPath() return new Store() diff --git a/src/main/persistence-protected-secret-write-race.test.ts b/src/main/persistence-protected-secret-write-race.test.ts index 20bbe78d5cf..89849d22dde 100644 --- a/src/main/persistence-protected-secret-write-race.test.ts +++ b/src/main/persistence-protected-secret-write-race.test.ts @@ -40,16 +40,18 @@ vi.mock('./telemetry/cohort-classifier', () => ({ })) vi.mock('electron', () => ({ - app: { getPath: () => testState.dir }, - safeStorage: { - isEncryptionAvailable: () => cipherState.available, - encryptString: (plaintext: string) => Buffer.from(`enc:${plaintext}`, 'utf-8'), - decryptString: (ciphertext: Buffer) => ciphertext.toString('utf-8').slice('enc:'.length) - } + app: { getPath: () => testState.dir } })) async function createStore() { vi.resetModules() + const { setSecretStore } = await import('../shared/secret-store') + setSecretStore({ + isEncryptionAvailable: () => cipherState.available, + encryptString: (plaintext) => Buffer.from(`enc:${plaintext}`, 'utf-8'), + decryptString: (ciphertext) => ciphertext.toString('utf-8').slice('enc:'.length), + describeUnavailable: () => null + }) const { Store, initDataPath } = await import('./persistence') initDataPath() return new Store() diff --git a/src/main/persistence-proxy-secret-recovery.test.ts b/src/main/persistence-proxy-secret-recovery.test.ts index 769ebec69ba..faa6f65822e 100644 --- a/src/main/persistence-proxy-secret-recovery.test.ts +++ b/src/main/persistence-proxy-secret-recovery.test.ts @@ -28,26 +28,7 @@ vi.mock('electron', () => ({ app: { getPath: () => testState.dir }, - session: { defaultSession: undefined }, - safeStorage: { - isEncryptionAvailable: () => { - if (cipherState.availabilityThrows) { - throw new Error('safeStorage cannot be used before the app is ready') - } - return cipherState.encryptionAvailable - }, - encryptString: (plaintext: string) => Buffer.from(`enc:${randomUUID()}:${plaintext}`, 'utf-8'), - decryptString: (ciphertext: Buffer) => { - if (cipherState.decryptAlwaysThrows) { - throw new Error('keychain access denied') - } - const decoded = ciphertext.toString('utf-8') - if (!decoded.startsWith('enc:')) { - throw new Error('invalid ciphertext') - } - return decoded.slice('enc:'.length + 36 + 1) - } - } + session: { defaultSession: undefined } })) vi.mock('./telemetry/client', () => ({ @@ -60,6 +41,27 @@ vi.mock('./telemetry/cohort-classifier', () => ({ async function createStore() { vi.resetModules() + const { setSecretStore } = await import('../shared/secret-store') + setSecretStore({ + isEncryptionAvailable: () => { + if (cipherState.availabilityThrows) { + throw new Error('safeStorage cannot be used before the app is ready') + } + return cipherState.encryptionAvailable + }, + encryptString: (plaintext) => Buffer.from(`enc:${randomUUID()}:${plaintext}`, 'utf-8'), + decryptString: (ciphertext) => { + if (cipherState.decryptAlwaysThrows) { + throw new Error('keychain access denied') + } + const decoded = ciphertext.toString('utf-8') + if (!decoded.startsWith('enc:')) { + throw new Error('invalid ciphertext') + } + return decoded.slice('enc:'.length + 36 + 1) + }, + describeUnavailable: () => null + }) const { Store, initDataPath } = await import('./persistence') initDataPath() return new Store() diff --git a/src/main/persistence-single-serialize.test.ts b/src/main/persistence-single-serialize.test.ts index c638ac12224..06512f9a089 100644 --- a/src/main/persistence-single-serialize.test.ts +++ b/src/main/persistence-single-serialize.test.ts @@ -27,21 +27,6 @@ const DETERMINISTIC_IV = 'd'.repeat(36) vi.mock('electron', () => ({ app: { getPath: () => testState.dir - }, - safeStorage: { - isEncryptionAvailable: () => cipherState.encryptionAvailable, - encryptString: (plaintext: string) => - Buffer.from( - `enc:${cipherState.deterministic ? DETERMINISTIC_IV : randomUUID()}:${plaintext}`, - 'utf-8' - ), - decryptString: (ciphertext: Buffer) => { - const decoded = ciphertext.toString('utf-8') - if (!decoded.startsWith('enc:')) { - throw new Error('invalid ciphertext') - } - return decoded.slice('enc:'.length + 36 + 1) - } } })) @@ -55,6 +40,23 @@ vi.mock('./telemetry/cohort-classifier', () => ({ async function createStore() { vi.resetModules() + const { setSecretStore } = await import('../shared/secret-store') + setSecretStore({ + isEncryptionAvailable: () => cipherState.encryptionAvailable, + encryptString: (plaintext) => + Buffer.from( + `enc:${cipherState.deterministic ? DETERMINISTIC_IV : randomUUID()}:${plaintext}`, + 'utf-8' + ), + decryptString: (ciphertext) => { + const decoded = ciphertext.toString('utf-8') + if (!decoded.startsWith('enc:')) { + throw new Error('invalid ciphertext') + } + return decoded.slice('enc:'.length + 36 + 1) + }, + describeUnavailable: () => null + }) const { Store, initDataPath } = await import('./persistence') initDataPath() return new Store() diff --git a/src/main/persistence-ui-state.test.ts b/src/main/persistence-ui-state.test.ts index ccafa4e3fab..7bb676ffe34 100644 --- a/src/main/persistence-ui-state.test.ts +++ b/src/main/persistence-ui-state.test.ts @@ -7,7 +7,6 @@ import { getDefaultPersistedState } from '../shared/constants' import { createDefaultWorkspaceCleanupBrowseState } from '../shared/workspace-cleanup-browse-state' import { testState, - createStore, dataFile, writeDataFile, readDataFile, @@ -32,19 +31,28 @@ const { trackMock, getCohortAtEmitMock } = vi.hoisted(() => ({ vi.mock('electron', () => ({ app: { getPath: () => testState.dir - }, - safeStorage: { + } +})) + +async function createStore() { + vi.resetModules() + const { setSecretStore } = await import('../shared/secret-store') + setSecretStore({ isEncryptionAvailable: () => true, - encryptString: (plaintext: string) => Buffer.from(`encrypted:${plaintext}`, 'utf-8'), - decryptString: (ciphertext: Buffer) => { + encryptString: (plaintext) => Buffer.from(`encrypted:${plaintext}`, 'utf-8'), + decryptString: (ciphertext) => { const decoded = ciphertext.toString('utf-8') if (!decoded.startsWith('encrypted:')) { throw new Error('invalid ciphertext') } return decoded.slice('encrypted:'.length) - } - } -})) + }, + describeUnavailable: () => null + }) + const { Store, initDataPath } = await import('./persistence') + initDataPath() + return new Store() +} vi.mock('./telemetry/client', () => ({ track: trackMock diff --git a/src/main/protected-secret-persistence.test.ts b/src/main/protected-secret-persistence.test.ts index 815edc5d477..ff71a9e8c9d 100644 --- a/src/main/protected-secret-persistence.test.ts +++ b/src/main/protected-secret-persistence.test.ts @@ -1,18 +1,29 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it } from 'vitest' +import { _resetSecretStoreForTests, setSecretStore } from '../shared/secret-store' const cipherState = { available: true } -vi.mock('electron', () => ({ - safeStorage: { - isEncryptionAvailable: () => cipherState.available, - encryptString: (plaintext: string) => Buffer.from(`encrypted:${plaintext}`), - decryptString: (ciphertext: Buffer) => ciphertext.toString().slice('encrypted:'.length) - } -})) - describe('ProtectedSecretPersistence', () => { beforeEach(() => { cipherState.available = true + setSecretStore({ + isEncryptionAvailable: () => cipherState.available, + encryptString: (plaintext) => Buffer.from(`encrypted:${plaintext}`), + decryptString: (ciphertext) => ciphertext.toString().slice('encrypted:'.length), + describeUnavailable: () => null + }) + }) + + it('surfaces an uninstalled secret store instead of degrading silently', async () => { + // Why: a missing setSecretStore() is a startup bug. If the availability check + // swallows it, encrypt() hands back an empty blob and decryptWithStatus() reports + // 'unavailable' — a real secret silently not stored, which is the outcome the + // port throws to prevent. + const { ProtectedSecretPersistence } = await import('./protected-secret-persistence') + const secrets = new ProtectedSecretPersistence() + _resetSecretStoreForTests() + + expect(() => secrets.encrypt('slot', 'token')).toThrow(/SecretStore not initialized/) }) it('evicts dynamic slots across repeated SSH recovery lifecycles', async () => { diff --git a/src/main/protected-secret-persistence.ts b/src/main/protected-secret-persistence.ts index 85ab3153043..8445e67680f 100644 --- a/src/main/protected-secret-persistence.ts +++ b/src/main/protected-secret-persistence.ts @@ -1,4 +1,4 @@ -import { safeStorage } from 'electron' +import { getSecretStore } from '../shared/secret-store' export const PROTECTED_SECRET_SLOT = { opencodeSessionCookie: 'settings.opencodeSessionCookie', @@ -77,7 +77,7 @@ export class ProtectedSecretPersistence { } } try { - const blob = safeStorage.encryptString(plaintext).toString('base64') + const blob = getSecretStore().encryptString(plaintext).toString('base64') return { blob, degraded: false, @@ -109,7 +109,7 @@ export class ProtectedSecretPersistence { } try { const decrypted = { - plaintext: safeStorage.decryptString(Buffer.from(ciphertext, 'base64')), + plaintext: getSecretStore().decryptString(Buffer.from(ciphertext, 'base64')), status: 'decrypted' as const } this.sealedSlots.delete(slot) @@ -117,22 +117,27 @@ export class ProtectedSecretPersistence { } catch { if (isLegacyPlaintext?.(ciphertext)) { this.sealedSlots.delete(slot) - console.warn('[persistence] safeStorage decryption failed; accepting legacy plaintext.') + console.warn('[persistence] secret decryption failed; accepting legacy plaintext.') return { plaintext: ciphertext, status: 'failed' } } this.sealedSlots.add(slot) console.warn( - '[persistence] safeStorage decryption failed; retaining the protected value without exposing it.' + '[persistence] secret decryption failed; retaining the protected value without exposing it.' ) return { plaintext: '', status: 'failed' } } } private encryptionAvailable(): boolean { + // Why getSecretStore() sits outside the try: an uninstalled store is a startup bug, + // not a keyring failure. Swallowing it would degrade to an empty blob and report + // 'unavailable' — the silent-wrong-state outcome the port throws to prevent. Only + // the backend probe itself may fail softly. + const store = getSecretStore() try { - return safeStorage.isEncryptionAvailable() + return store.isEncryptionAvailable() } catch (err) { - console.warn('[persistence] safeStorage availability check failed:', err) + console.warn('[persistence] secret store availability check failed:', err) return false } } diff --git a/src/main/speech/openai-api-key-store.test.ts b/src/main/speech/openai-api-key-store.test.ts index 9475d5afa54..2153c9fe608 100644 --- a/src/main/speech/openai-api-key-store.test.ts +++ b/src/main/speech/openai-api-key-store.test.ts @@ -14,9 +14,11 @@ let tempHome = '' async function loadStoreModule() { vi.resetModules() - vi.doMock('electron', () => ({ - safeStorage: safeStorageMock - })) + const { setSecretStore } = await import('../../shared/secret-store') + setSecretStore({ + ...safeStorageMock, + describeUnavailable: () => null + }) vi.doMock('os', async () => { const actual = await vi.importActual('os') return { ...actual, homedir: () => tempHome } diff --git a/src/main/speech/openai-api-key-store.ts b/src/main/speech/openai-api-key-store.ts index dc7345f3037..b587c878f4d 100644 --- a/src/main/speech/openai-api-key-store.ts +++ b/src/main/speech/openai-api-key-store.ts @@ -1,4 +1,4 @@ -import { safeStorage } from 'electron' +import { getSecretStore } from '../../shared/secret-store' import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { homedir } from 'node:os' import { join } from 'node:path' @@ -43,7 +43,7 @@ function readLegacyJsonStoredOpenAiKey(): StoredOpenAiKey | null { export function hasOpenAiSpeechApiKey(): boolean { // Why: Settings and model-state refresh call this on startup; checking file - // existence avoids decrypting safeStorage and triggering macOS keychain prompts. + // existence avoids a decrypt that triggers macOS keychain prompts. return existsSync(getOpenAiKeyPath()) } @@ -53,15 +53,13 @@ export function saveOpenAiSpeechApiKey(apiKey: string): void { throw new Error('OpenAI API key is required') } ensureOrcaDir() - if (safeStorage.isEncryptionAvailable()) { - writeFileSync(getOpenAiKeyPath(), safeStorage.encryptString(trimmed), { mode: 0o600 }) + if (getSecretStore().isEncryptionAvailable()) { + writeFileSync(getOpenAiKeyPath(), getSecretStore().encryptString(trimmed), { mode: 0o600 }) cachedOpenAiSpeechApiKey = trimmed return } - console.warn( - '[speech] safeStorage encryption unavailable — storing OpenAI speech key in plaintext' - ) + console.warn('[speech] secret encryption unavailable — storing OpenAI speech key in plaintext') writeFileSync(getOpenAiKeyPath(), trimmed, { encoding: 'utf8', mode: 0o600 }) cachedOpenAiSpeechApiKey = trimmed } @@ -79,13 +77,13 @@ export function readOpenAiSpeechApiKey(): string { const raw = readFileSync(keyPath) const legacyJson = readLegacyJsonStoredOpenAiKey() if (legacyJson) { - cachedOpenAiSpeechApiKey = safeStorage.decryptString( + cachedOpenAiSpeechApiKey = getSecretStore().decryptString( Buffer.from(legacyJson.encryptedKeyBase64, 'base64') ) return cachedOpenAiSpeechApiKey } - cachedOpenAiSpeechApiKey = safeStorage.isEncryptionAvailable() - ? safeStorage.decryptString(raw) + cachedOpenAiSpeechApiKey = getSecretStore().isEncryptionAvailable() + ? getSecretStore().decryptString(raw) : raw.toString('utf8') return cachedOpenAiSpeechApiKey } catch { diff --git a/src/shared/secret-store.test.ts b/src/shared/secret-store.test.ts new file mode 100644 index 00000000000..c0478472cd5 --- /dev/null +++ b/src/shared/secret-store.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it, beforeEach } from 'vitest' +import { + getSecretStore, + hasSecretStore, + _resetSecretStoreForTests, + setSecretStore, + type SecretStore +} from './secret-store' + +function fakeStore(overrides: Partial = {}): SecretStore { + return { + isEncryptionAvailable: () => true, + encryptString: (plainText) => Buffer.from(`sealed:${plainText}`), + decryptString: (cipher) => cipher.toString().slice('sealed:'.length), + describeUnavailable: () => null, + ...overrides + } +} + +describe('SecretStore registry', () => { + beforeEach(() => { + _resetSecretStoreForTests() + }) + + it('throws until a store is installed, rather than defaulting to one that cannot seal', () => { + expect(hasSecretStore()).toBe(false) + expect(() => getSecretStore()).toThrow(/SecretStore not initialized/) + }) + + it('returns the installed store', () => { + const store = fakeStore() + setSecretStore(store) + expect(hasSecretStore()).toBe(true) + expect(getSecretStore()).toBe(store) + expect(getSecretStore().encryptString('token').toString()).toBe('sealed:token') + }) + + it('lets a later install replace an earlier one, so a test fake wins over the global default', () => { + setSecretStore(fakeStore()) + setSecretStore(fakeStore({ isEncryptionAvailable: () => false })) + expect(getSecretStore().isEncryptionAvailable()).toBe(false) + }) + + it('carries a reason when sealing is unavailable, so the degradation can be surfaced', () => { + setSecretStore( + fakeStore({ + isEncryptionAvailable: () => false, + describeUnavailable: () => 'The OS keyring is unavailable.' + }) + ) + expect(getSecretStore().describeUnavailable()).toBe('The OS keyring is unavailable.') + }) +}) diff --git a/src/shared/secret-store.ts b/src/shared/secret-store.ts new file mode 100644 index 00000000000..4f3d594e207 --- /dev/null +++ b/src/shared/secret-store.ts @@ -0,0 +1,66 @@ +/** + * SecretStore abstracts at-rest secret encryption that the desktop gets from + * Electron's `safeStorage` (OS keychain). A plain-Node host installs its own + * implementation so core modules never import `electron`. + * + * The contract mirrors safeStorage exactly, including the part that matters most: + * `isEncryptionAvailable()` may return false. A store that cannot seal must say so + * rather than throw; how a caller degrades is its own decision (persistence retains + * the prior sealed blob rather than writing plaintext). See `describeUnavailable()`, + * which exists so the reason reaches the user, not a console warning nobody reads. + */ + +export type SecretStore = { + isEncryptionAvailable(): boolean + encryptString(plainText: string): Buffer + decryptString(cipher: Buffer): string + /** + * Why: "encryption unavailable" is a security posture, not a detail. When + * `isEncryptionAvailable()` is false this returns a short, user-safe sentence + * explaining which host facility is missing, for the degradation surface. + * Returns null when encryption IS available. + */ + describeUnavailable(): string | null +} + +/** + * Why a global symbol and not a module-level `let`: `vi.resetModules()` gives the + * re-imported graph a fresh copy of this module, so a store installed before the reset + * would silently read back as uninstalled — and `getSecretStore()` throws on that. + * Anchoring to the realm keeps one instance per process however often the module + * registry is rebuilt. + */ +const SLOT = Symbol.for('orca.host.secretStore') + +type Slot = { [SLOT]?: SecretStore | null } + +function slot(): Slot { + return globalThis as unknown as Slot +} + +function read(): SecretStore | null { + return slot()[SLOT] ?? null +} + +export function setSecretStore(store: SecretStore): void { + slot()[SLOT] = store +} + +export function getSecretStore(): SecretStore { + const current = read() + if (!current) { + throw new Error( + 'SecretStore not initialized — call setSecretStore() during startup before reading or writing secrets' + ) + } + return current +} + +export function hasSecretStore(): boolean { + return read() !== null +} + +/** Test-only: drop the installed store so suites do not leak one across files. */ +export function _resetSecretStoreForTests(): void { + slot()[SLOT] = null +}