From c90773968f4d60ab662e2b23be4311d9a7fec0fc Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:58:26 +0200 Subject: [PATCH 1/5] security(desktop): fix API-key encryption using a real secret F-05/F-06 (2026-07-29, AUDIT.md/CHANGELOG.md) was credited as fixed for upgrading desktop API-key encryption from unsalted SHA-256 to PBKDF2 + random salt. That hardened the KDF against rainbow-table/multi-target attacks but never addressed the actual finding: the PBKDF2 passphrase input, `${appDataPath}|${provider}|WorldScriptStudio|v1`, is built entirely from public/discoverable values (a standard OS app-data path, a public provider enum, hardcoded literals). Anyone with read access to the encrypted `_key.enc.json` file - the exact threat model this exists to defend against - can reconstruct the identical string and decrypt in one PBKDF2 call. No brute force needed, so the iteration count defended against an attack that isn't the real one. Retire the derived-passphrase scheme entirely (encryptText/decryptText/ deriveFileSystemCryptoKey removed from fsCore.ts) rather than patch it again. API keys now reuse services/storage/storageEncryptionService.ts's crypto primitives directly - the same real, user-chosen passphrase- derived key that already protects IDB at-rest data, via its already- exported resolveProtectedWriteKey/idbEncryptWithKey/idbDecryptWithKey. No new module, no changes to that already-audited service, no migration- journal integration - this is pure reuse of existing, tested machinery. Behavior: - Passphrase configured + unlocked: real AES-256-GCM protection under the actual user secret. - No passphrase configured: honest plaintext (removes the false- confidence derivation instead of leaving a fake "encrypted" fallback - matches the existing opt-in model where actual encryption only activates once a passphrase is set). - Passphrase configured but locked: fails closed on both save and read (IdbStorageLockedError propagates) rather than silently downgrading to plaintext, matching the existing IDB protected-write policy. A locked read does NOT discard the file - the key is still there, just temporarily unreadable. - Either obsolete pre-2026-08-13 format (unsalted, or salted-but-public- passphrase) is discarded and the user re-prompted on next read - same "locked decision, no migration" precedent as the original F-05/F-06 fix. bytesToBase64/base64ToBytes stay in fsCore.ts (now exported) as the JSON-safe codec for the new protected envelope's ciphertext. Stacked on fix/desktop-atomic-writes (writeTextFileAtomic) since the API-key file write needed it. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 19 ++++ services/fs/fsCore.ts | 71 +-------------- services/fs/settingsFsStore.ts | 82 ++++++++++++++--- tests/unit/services/fs/fsCore.test.ts | 43 +++------ tests/unit/services/fs/fsStores.test.ts | 111 ++++++++++++++++++++++-- 5 files changed, 213 insertions(+), 113 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 160abc94..7c084095 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- **Desktop API-key protection now uses a real secret.** The scheme credited as "fixed" for + F-05/F-06 (2026-07-29) upgraded the KDF (unsalted SHA-256 → PBKDF2 + random salt) but never + addressed the actual finding: its passphrase input — + `${appDataPath}|${provider}|WorldScriptStudio|v1` — was built entirely from public/discoverable + values, so anyone with read access to the encrypted `_key.enc.json` file could + reconstruct it and decrypt in one PBKDF2 call. API keys now reuse + `services/storage/storageEncryptionService.ts`'s real user-passphrase-derived key (the same one + protecting IDB at-rest data) whenever at-rest encryption is configured and unlocked; when no + passphrase is configured, keys are stored as honest plaintext rather than fake-encrypted. + Configured-but-locked reads/writes fail closed (no silent plaintext downgrade), matching the + existing IDB protected-write policy. The two obsolete pre-2026-08-13 formats (unsalted and + salted-but-public-passphrase) are discarded, not migrated, on next read — same "locked decision" + precedent as the original F-05/F-06 fix; the user is notified and re-prompted for the key. + `services/fs/fsCore.ts`'s derived-passphrase `encryptText`/`decryptText` were retired outright. + Project/settings/snapshot/Codex/RAG/binder-asset data on desktop remains plaintext — that gap + is tracked separately and not fixed by this entry. + ### Fixed - **Atomic writes for desktop filesystem storage.** Every `services/fs/*Store.ts` writer diff --git a/services/fs/fsCore.ts b/services/fs/fsCore.ts index 220408f6..6d221db7 100644 --- a/services/fs/fsCore.ts +++ b/services/fs/fsCore.ts @@ -168,13 +168,10 @@ export function decompressData(raw: string): T { return JSON.parse(raw) as T; } -// --- Crypto helpers --- -// QNBS-v3: PBKDF2-SHA-256 (600k iter, OWASP 2024 minimum) + random 32-byte salt, mirroring storageEncryptionService.ts#deriveKey, replacing a prior unsalted-SHA-256-of-public-material scheme (F-05/F-06); legacy (no `salt` field) payloads are treated as unreadable, not migrated — see decryptText below. +// --- Base64 codec helpers --- +// QNBS-v3: previously private to a now-removed derived-passphrase crypto scheme (F-05/F-06 fix, then found still-insecure — see settingsFsStore.ts header comment); API-key protection now reuses storageEncryptionService.ts's real passphrase-derived key directly, and these two helpers stay, exported, as the JSON-safe codec for its Uint8Array ciphertext. -const PBKDF2_ITERATIONS = 600_000; // OWASP 2024 minimum for PBKDF2-HMAC-SHA-256 -const SALT_BYTE_LENGTH = 32; - -function bytesToBase64(bytes: Uint8Array): string { +export function bytesToBase64(bytes: Uint8Array): string { let bin = ''; for (let i = 0; i < bytes.byteLength; i++) { bin += String.fromCharCode(bytes[i]!); @@ -183,7 +180,7 @@ function bytesToBase64(bytes: Uint8Array): string { } // QNBS-v3: explicit Uint8Array return type — a bare `Uint8Array` annotation widens to `Uint8Array` (includes SharedArrayBuffer), rejected by crypto.subtle as a BufferSource; same pattern as libraryBackupService.ts#copyToFixedBuffer. -function base64ToBytes(b64: string): Uint8Array { +export function base64ToBytes(b64: string): Uint8Array { const bin = atob(b64); const out = new Uint8Array(bin.length); for (let i = 0; i < bin.length; i++) { @@ -192,66 +189,6 @@ function base64ToBytes(b64: string): Uint8Array { return out; } -async function deriveFileSystemCryptoKey( - secretMaterial: string, - salt: Uint8Array, -): Promise { - const encoder = new TextEncoder(); - const keyMaterial = await crypto.subtle.importKey( - 'raw', - encoder.encode(secretMaterial), - { name: 'PBKDF2' }, - false, - ['deriveBits', 'deriveKey'], - ); - return crypto.subtle.deriveKey( - { name: 'PBKDF2', salt: new Uint8Array(salt), iterations: PBKDF2_ITERATIONS, hash: 'SHA-256' }, - keyMaterial, - { name: 'AES-GCM', length: 256 }, - // QNBS-v3: extractable: false — key cannot leave the WebCrypto context. - false, - ['encrypt', 'decrypt'], - ); -} - -export interface EncryptedFsPayload { - iv: string; - salt: string; - data: string; -} - -export async function encryptText( - value: string, - secretMaterial: string, -): Promise { - const salt = crypto.getRandomValues(new Uint8Array(SALT_BYTE_LENGTH)); - const key = await deriveFileSystemCryptoKey(secretMaterial, salt); - const iv = crypto.getRandomValues(new Uint8Array(12)); - const encoded = new TextEncoder().encode(value); - const encrypted = await crypto.subtle.encrypt({ name: 'AES-GCM', iv }, key, encoded); - return { - iv: bytesToBase64(iv), - salt: bytesToBase64(salt), - data: bytesToBase64(new Uint8Array(encrypted)), - }; -} - -export async function decryptText( - payload: { iv: string; salt?: string; data: string }, - secretMaterial: string, -): Promise { - if (!payload.salt) { - // QNBS-v3: pre-2026-07-29 payloads have no salt field (unsalted single-SHA-256 scheme, F-05) — not migrated by design (locked decision); the caller treats this as "no key available". - throw new Error('Legacy unsalted key payload is no longer supported; re-enter the API key.'); - } - const salt = base64ToBytes(payload.salt); - const key = await deriveFileSystemCryptoKey(secretMaterial, salt); - const iv = base64ToBytes(payload.iv); - const encrypted = base64ToBytes(payload.data); - const decrypted = await crypto.subtle.decrypt({ name: 'AES-GCM', iv }, key, encrypted); - return new TextDecoder().decode(decrypted); -} - // --- Path sanitization helpers --- const stripControlChars = (value: string): string => { diff --git a/services/fs/settingsFsStore.ts b/services/fs/settingsFsStore.ts index 2ea5fff0..e5334c70 100644 --- a/services/fs/settingsFsStore.ts +++ b/services/fs/settingsFsStore.ts @@ -1,6 +1,12 @@ /** - * FsSettingsStore — Settings persistence and AES-256-GCM API key encryption on the filesystem. - * ENCRYPTION: AES-256-GCM — API keys stored as `_key.enc.json` in config/. + * FsSettingsStore — Settings persistence and API key protection on the filesystem. + * ENCRYPTION: AES-256-GCM under the user's real at-rest-encryption passphrase (reused from + * services/storage/storageEncryptionService.ts) when one is configured and unlocked; honest + * plaintext otherwise. QNBS-v3 (F-05/F-06 follow-up, 2026-08-13): the prior scheme derived its + * PBKDF2 key from `${appDataPath}|${provider}|WorldScriptStudio|v1` — entirely public/ + * reconstructible by anyone who can read the encrypted file, so it provided no real + * confidentiality despite looking like AES-256-GCM+PBKDF2. Retired outright rather than patched; + * see getApiKey's legacy-format discard path. * QNBS-v3: Extracted from fileSystemService.ts. */ @@ -9,8 +15,27 @@ import { statusActions } from '../../features/status/statusSlice'; import type { Settings } from '../../types'; import { logger } from '../logger'; import { normalizePersistedSettings } from '../storage/idbProjectStore'; +import { + IdbStorageLockedError, + idbDecryptWithKey, + idbEncryptWithKey, + resolveProtectedWriteKey, +} from '../storage/storageEncryptionService'; import type { TauriApis } from './fsCore'; -import { decryptText, encryptText, FsCore, retryFs, writeTextFileAtomic } from './fsCore'; +import { base64ToBytes, bytesToBase64, FsCore, retryFs, writeTextFileAtomic } from './fsCore'; + +const PLAINTEXT_SCHEME = 'plaintext-v1'; +const PROTECTED_SCHEME = 'protected-v1'; + +interface PlaintextApiKeyPayload { + scheme: typeof PLAINTEXT_SCHEME; + value: string; +} + +interface ProtectedApiKeyPayload { + scheme: typeof PROTECTED_SCHEME; + data: string; +} export class FsSettingsStore extends FsCore { async saveSettings(settings: Settings): Promise { @@ -59,7 +84,8 @@ export class FsSettingsStore extends FsCore { return this.clearApiKey('gemini'); } - // Generic provider API key — stored encrypted in app data dir + // Generic provider API key — protected under the real at-rest-encryption passphrase when one is + // configured and unlocked; stored as honest plaintext otherwise (never a fake-secret derivation). async saveApiKey(provider: string, apiKey: string): Promise { if (!apiKey?.trim()) { throw new Error('API key cannot be empty'); @@ -70,12 +96,30 @@ export class FsSettingsStore extends FsCore { const configPath = await apis.join(appDataPath, 'config'); if (!(await apis.exists(configPath))) await apis.mkdir(configPath, { recursive: true }); - const encrypted = await encryptText( - apiKey.trim(), - `${appDataPath}|${provider}|WorldScriptStudio|v1`, - ); + // QNBS-v3: resolveProtectedWriteKey() throws IdbStorageLockedError when a passphrase is + // configured but the session is locked — propagated deliberately (fail closed) rather than + // silently falling back to plaintext while the user believes encryption is active, matching + // the existing IDB protected-write policy this reuses. + const key = await resolveProtectedWriteKey(); + const payload: ProtectedApiKeyPayload | PlaintextApiKeyPayload = key + ? { + scheme: PROTECTED_SCHEME, + data: bytesToBase64(await idbEncryptWithKey(key, apiKey.trim())), + } + : { scheme: PLAINTEXT_SCHEME, value: apiKey.trim() }; + const filePath = await apis.join(configPath, `${provider}_key.enc.json`); - await writeTextFileAtomic(apis, filePath, JSON.stringify(encrypted)); + await writeTextFileAtomic(apis, filePath, JSON.stringify(payload)); + } + + private async readProtectedApiKey(base64Data: string): Promise { + const key = await resolveProtectedWriteKey(); + if (!key) { + // Sentinel was cleared/disabled since this key was saved under the old, now-configured-off + // passphrase — there is nothing left to decrypt with; treat the same as an unreadable file. + throw new Error('Protected API key exists but at-rest encryption is no longer configured'); + } + return idbDecryptWithKey(key, base64ToBytes(base64Data)); } async getApiKey(provider: string): Promise { @@ -90,10 +134,24 @@ export class FsSettingsStore extends FsCore { keyFileForCleanup = keyFile; if (!(await apis.exists(keyFile))) return null; const content = await retryFs(() => apis.readTextFile(keyFile)); - const payload = JSON.parse(content) as { iv: string; salt?: string; data: string }; - return await decryptText(payload, `${appDataPath}|${provider}|WorldScriptStudio|v1`); + const parsed = JSON.parse(content) as Record; + + if (parsed['scheme'] === PLAINTEXT_SCHEME && typeof parsed['value'] === 'string') { + return parsed['value']; + } + if (parsed['scheme'] === PROTECTED_SCHEME && typeof parsed['data'] === 'string') { + return await this.readProtectedApiKey(parsed['data']); + } + // Anything else is one of the two obsolete pre-2026-08-13 formats (unsalted single-SHA-256, + // or salted-but-publicly-derivable-passphrase — F-05/F-06) — neither provides real + // confidentiality and neither is migrated by design; fall through to the discard+notify path. + throw new Error('Unrecognized or obsolete API key payload format'); } catch (error) { - // QNBS-v3: pre-2026-07-29 key files (unsalted single-SHA-256, F-05/F-06) are not migrated (locked decision — discard); removing the stale file avoids retrying every call, and a one-time notification beats a silent null indistinguishable from "no key ever set". + if (error instanceof IdbStorageLockedError) { + // Configured but locked — the key still exists, just temporarily unreadable. Fail closed without discarding the file; the caller re-prompts once the session is unlocked. + return null; + } + // QNBS-v3: pre-2026-08-13 key files (both obsolete crypto schemes) are not migrated (locked decision — discard); removing the stale file avoids retrying every call, and a one-time notification beats a silent null indistinguishable from "no key ever set". if (apisForCleanup && keyFileForCleanup) { try { await apisForCleanup.remove(keyFileForCleanup); diff --git a/tests/unit/services/fs/fsCore.test.ts b/tests/unit/services/fs/fsCore.test.ts index 06ea94f1..6bbedbff 100644 --- a/tests/unit/services/fs/fsCore.test.ts +++ b/tests/unit/services/fs/fsCore.test.ts @@ -7,12 +7,12 @@ import { describe, expect, it, vi } from 'vitest'; import type { TauriApis } from '../../../../services/fs/fsCore'; import { + base64ToBytes, + bytesToBase64, cleanupOrphanedTempFiles, compressData, countProjectWords, decompressData, - decryptText, - encryptText, retryFs, sanitizePathSegment, writeFileAtomic, @@ -308,35 +308,20 @@ describe('compressData / decompressData', () => { }); }); -describe('encryptText / decryptText', () => { - it('round-trips a value with the same secret', async () => { - const payload = await encryptText('top secret manuscript', 'passphrase-123'); - expect(payload.iv).toBeTruthy(); - expect(payload.salt).toBeTruthy(); - expect(payload.data).toBeTruthy(); - await expect(decryptText(payload, 'passphrase-123')).resolves.toBe('top secret manuscript'); +// QNBS-v3 (2026-08-13): the derived-passphrase encryptText/decryptText scheme these tests used to +// cover was retired entirely — its "secret" (`${appDataPath}|${provider}|WorldScriptStudio|v1`) +// was fully public/reconstructible by anyone who could read the encrypted file, providing no real +// confidentiality (see services/fs/settingsFsStore.ts's header comment and AUDIT.md's F-05/F-06 +// row). API-key protection now reuses services/storage/storageEncryptionService.ts's real +// passphrase-derived key directly; see tests/unit/services/fs/fsStores.test.ts for that coverage. +describe('bytesToBase64 / base64ToBytes', () => { + it('round-trips arbitrary byte sequences, including zero and high bytes', () => { + const bytes = new Uint8Array([0, 1, 2, 254, 255, 128, 42]); + expect(base64ToBytes(bytesToBase64(bytes))).toEqual(bytes); }); - it('fails to decrypt with the wrong secret', async () => { - const payload = await encryptText('top secret', 'right-key'); - await expect(decryptText(payload, 'wrong-key')).rejects.toBeDefined(); - }); - - // QNBS-v3 (F-05/F-06 fix, 2026-07-29): regression guard for the PBKDF2 + random-salt derivation replacing the prior unsalted single-SHA-256 scheme. - it('produces a different ciphertext, iv, and salt on every encryption of the same secret+plaintext', async () => { - const a = await encryptText('same plaintext', 'same-secret-material'); - const b = await encryptText('same plaintext', 'same-secret-material'); - expect(a.salt).not.toBe(b.salt); - expect(a.iv).not.toBe(b.iv); - expect(a.data).not.toBe(b.data); - // Both must still independently decrypt correctly with their own salt/iv. - await expect(decryptText(a, 'same-secret-material')).resolves.toBe('same plaintext'); - await expect(decryptText(b, 'same-secret-material')).resolves.toBe('same plaintext'); - }); - - it('rejects a legacy (pre-2026-07-29) payload with no salt field', async () => { - const legacyPayload = { iv: 'AAAAAAAAAAAAAAAA', data: 'AAAAAAAAAAAAAAAA' }; - await expect(decryptText(legacyPayload, 'any-secret')).rejects.toThrow(/legacy/i); + it('round-trips an empty byte sequence', () => { + expect(base64ToBytes(bytesToBase64(new Uint8Array([])))).toEqual(new Uint8Array([])); }); }); diff --git a/tests/unit/services/fs/fsStores.test.ts b/tests/unit/services/fs/fsStores.test.ts index 4afad36b..cdbb1990 100644 --- a/tests/unit/services/fs/fsStores.test.ts +++ b/tests/unit/services/fs/fsStores.test.ts @@ -11,6 +11,24 @@ import type { TauriApis } from '../../../../services/fs/fsCore'; // QNBS-v3: typed via `unknown` (not `any`) so the mock factories see a non-null TauriApis; the real value is set in beforeEach before any mock is invoked. const { fsHolder } = vi.hoisted(() => ({ fsHolder: { current: null as unknown as TauriApis } })); +// QNBS-v3: controllable fake for storageEncryptionService's IDB-backed sentinel/session state — activeKey/sentinelConfigured drive resolveProtectedWriteKey()'s tri-state (real key | never configured | locked); idbEncryptWithKey/idbDecryptWithKey/IdbStorageLockedError are the REAL implementation via importOriginal (pure Web Crypto, no IDB access), so this file never needs to mock or initialize real IndexedDB. +const { cryptoState } = vi.hoisted(() => ({ + cryptoState: { activeKey: null as CryptoKey | null, sentinelConfigured: false }, +})); +vi.mock('../../../../services/storage/storageEncryptionService', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + hasPassphraseSentinel: () => Promise.resolve(cryptoState.sentinelConfigured), + resolveProtectedWriteKey: () => { + if (cryptoState.activeKey) return Promise.resolve(cryptoState.activeKey); + if (cryptoState.sentinelConfigured) return Promise.reject(new actual.IdbStorageLockedError()); + return Promise.resolve(null); + }, + }; +}); + // QNBS-v3: mock the @tauri-apps plugin modules so the REAL loadTauriApis assembles a TauriApis whose methods delegate to the per-test in-memory fake FS (memoization-safe); exercises real store logic AND loadTauriApis itself. vi.mock('@tauri-apps/api/core', () => ({ invoke: (cmd: string, args?: Record) => fsHolder.current.invoke(cmd, args), @@ -42,6 +60,7 @@ vi.mock('../../../../services/logger', async (importOriginal) => { import { appStoreRef } from '../../../../app/storeRef'; import { FsProjectStore } from '../../../../services/fs/projectFsStore'; import { logger } from '../../../../services/logger'; +import { StorageEncryptionService } from '../../../../services/storage/storageEncryptionService'; interface FakeFs { apis: TauriApis; @@ -121,6 +140,8 @@ beforeEach(() => { fake = makeFakeFs(); fsHolder.current = fake.apis; store = new FsProjectStore(); + cryptoState.activeKey = null; + cryptoState.sentinelConfigured = false; }); afterEach(() => { vi.clearAllMocks(); @@ -219,13 +240,59 @@ describe('FsSettingsStore — settings + encrypted API keys', () => { expect(await store.loadSettings()).toBeNull(); }); - it('encrypts and decrypts an API key round-trip', async () => { + // QNBS-v3 (2026-08-13, F-05/F-06 follow-up): no passphrase configured — honest plaintext, + // not a fake-secret derivation. + it('round-trips an API key as plaintext when no at-rest passphrase is configured', async () => { await store.saveApiKey('openai', 'sk-secret-123'); + const stored = JSON.parse(fake.text.get('/app/config/openai_key.enc.json') as string); + expect(stored.scheme).toBe('plaintext-v1'); expect(await store.getApiKey('openai')).toBe('sk-secret-123'); await store.clearApiKey('openai'); expect(await store.getApiKey('openai')).toBeNull(); }); + // QNBS-v3: the actual fix under test — a real, non-public secret protects the key when the + // user has configured and unlocked at-rest encryption. + it('round-trips an API key under real AES-GCM protection when a passphrase is configured and unlocked', async () => { + cryptoState.activeKey = await new StorageEncryptionService().deriveKey( + 'test-passphrase', + new Uint8Array(32).fill(7), + ); + cryptoState.sentinelConfigured = true; + + await store.saveApiKey('openai', 'sk-secret-123'); + const stored = JSON.parse(fake.text.get('/app/config/openai_key.enc.json') as string); + expect(stored.scheme).toBe('protected-v1'); + expect(stored.data).not.toContain('sk-secret-123'); + + expect(await store.getApiKey('openai')).toBe('sk-secret-123'); + }); + + // QNBS-v3: fail-closed, matching the existing IDB protected-write policy — never silently + // downgrade to plaintext while the user believes at-rest encryption is protecting this key. + it('rejects saveApiKey when at-rest encryption is configured but the session is locked', async () => { + cryptoState.sentinelConfigured = true; // configured, but no activeKey — locked + + await expect(store.saveApiKey('openai', 'sk-secret-123')).rejects.toThrow(/storage is locked/i); + expect(fake.text.has('/app/config/openai_key.enc.json')).toBe(false); + }); + + // QNBS-v3: a locked-but-configured read must fail closed WITHOUT discarding the file — the key + // is still there, just temporarily unreadable until the user unlocks their session. + it('returns null without discarding the file when reading a protected key while locked', async () => { + cryptoState.activeKey = await new StorageEncryptionService().deriveKey( + 'test-passphrase', + new Uint8Array(32).fill(7), + ); + cryptoState.sentinelConfigured = true; + await store.saveApiKey('openai', 'sk-secret-123'); + + cryptoState.activeKey = null; // simulate session lock; sentinelConfigured stays true + + expect(await store.getApiKey('openai')).toBeNull(); + expect(fake.text.has('/app/config/openai_key.enc.json')).toBe(true); + }); + it('delegates the Gemini key helpers to provider storage', async () => { await store.saveGeminiApiKey('gem-key'); expect(await store.getGeminiApiKey()).toBe('gem-key'); @@ -239,10 +306,10 @@ describe('FsSettingsStore — settings + encrypted API keys', () => { expect(await store.getApiKey('anthropic')).toBeNull(); }); - // QNBS-v3 (F-05/F-06 fix, 2026-07-29): a pre-2026-07-29 key file (unsalted single-SHA-256 - // scheme) is discarded, not migrated (locked decision) — this asserts the discard path returns - // null without throwing, removes the stale file, and surfaces a one-time notification rather - // than failing silently. + // QNBS-v3 (F-05/F-06 fix, 2026-07-29; superseded 2026-08-13): a pre-2026-07-29 key file + // (unsalted single-SHA-256 scheme) is discarded, not migrated (locked decision) — this asserts + // the discard path returns null without throwing, removes the stale file, and surfaces a + // one-time notification rather than failing silently. it('discards a legacy unsalted key file, removes it, and notifies instead of throwing', async () => { const dispatch = vi.fn(); appStoreRef.current = { getState: vi.fn(), dispatch } as never; @@ -270,6 +337,40 @@ describe('FsSettingsStore — settings + encrypted API keys', () => { } }); + // QNBS-v3 (2026-08-13): the 2026-07-29 fix (salted PBKDF2) is now ALSO obsolete — its passphrase + // was `${appDataPath}|${provider}|WorldScriptStudio|v1`, entirely public — so a key file in that + // format must be discarded exactly like the older unsalted one, not trusted as "already secure". + it('discards a legacy salted-but-public-passphrase key file (2026-07-29 scheme), removes it, and notifies', async () => { + const dispatch = vi.fn(); + appStoreRef.current = { getState: vi.fn(), dispatch } as never; + try { + const legacyFile = '/app/config/legacyprovider3_key.enc.json'; + fake.text.set( + legacyFile, + JSON.stringify({ + iv: 'AAAAAAAAAAAAAAAA', + salt: 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=', + data: 'AAAAAAAAAAAAAAAA', + }), + ); + + const result = await store.getApiKey('legacyprovider3'); + + expect(result).toBeNull(); + expect(fake.text.has(legacyFile)).toBe(false); + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + payload: expect.objectContaining({ + type: 'info', + title: expect.stringContaining('API Key Reset'), + }), + }), + ); + } finally { + appStoreRef.current = null; + } + }); + // QNBS-v3 (Codecov-flagged missing line): the discard path's own cleanup can itself fail (e.g. // the file is locked or already gone) — asserts that failure is swallowed (logged, not thrown) // rather than surfacing as an unhandled rejection from getApiKey. From 33c7f39191d9f0188e9975762d1bc1b7187a7a54 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Thu, 13 Aug 2026 11:15:13 +0200 Subject: [PATCH 2/5] security(desktop): address review findings on API-key encryption PR Fixes two confirmed bugs found during the review correction loop, both independently flagged by multiple reviewers: - Passphrase rotation permanently deleted valid keys (Critical). Any decrypt failure that wasn't IdbStorageLockedError fell into the generic "obsolete format, discard + notify" path - including a perfectly valid protected-v1 file that just doesn't decrypt under the CURRENT key because the passphrase was rotated since it was saved. getApiKey() now only discards payloads it can positively identify as one of the two actually-obsolete pre-2026-08-13 formats; any other decrypt failure on a recognized protected-v1 envelope (rotation, transient sentinel-lookup error, corruption) preserves the file and returns null instead. Restructured getApiKey() to separate file read/parse from scheme interpretation from the (now much narrower) discard path. - No provider-identity binding on the ciphertext (P2 hardening). Every provider's key used the same session CryptoKey with no binding to which provider it belongs to, so a process able to modify the Tauri app-data directory could swap two protected-v1 files and both would still decrypt successfully under the wrong provider. Now encrypts {provider, apiKey} together instead of the bare key string; a provider mismatch on decrypt is rejected (and the file preserved, not discarded) rather than silently handing one provider's key to another. Also condensed the remaining QNBS-v3 multi-line comments introduced by this PR onto one physical line. Not changed (see PR discussion): the "API keys must be persisted only via encrypted IndexedDB storage, never plaintext" compliance claim on one thread doesn't correspond to any actual rule in this repo's AGENTS.md/CLAUDE.md - checked directly, no match. This is the Tauri filesystem-backed store, not IndexedDB; honest plaintext when no passphrase is configured is the deliberate, discussed design. Rebased onto the updated fix/desktop-atomic-writes to pick up its own review-loop fixes (temp-file-leak, concurrent-write race, QNBS-v3). Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 9 +- services/fs/settingsFsStore.ts | 114 ++++++++++++++---------- tests/unit/services/fs/fsStores.test.ts | 52 +++++++++-- 3 files changed, 122 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c084095..6dcf4347 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 precedent as the original F-05/F-06 fix; the user is notified and re-prompted for the key. `services/fs/fsCore.ts`'s derived-passphrase `encryptText`/`decryptText` were retired outright. Project/settings/snapshot/Codex/RAG/binder-asset data on desktop remains plaintext — that gap - is tracked separately and not fixed by this entry. + is tracked separately and not fixed by this entry. **Review-loop follow-up fixes to the same + change:** a `protected-v1` file that fails to decrypt under the *current* key (e.g. after a + passphrase rotation, or a transient error resolving the key) is no longer discarded — only + payloads positively identified as one of the two obsolete pre-2026-08-13 formats are; a rotation + no longer permanently destroys an otherwise-valid saved key. Encrypted payloads now bind + `{provider, apiKey}` together (not just the bare key string), so a ciphertext swapped between two + providers' files decrypts under the same key but fails the provider check, closing a cross-file + substitution gap. ### Fixed diff --git a/services/fs/settingsFsStore.ts b/services/fs/settingsFsStore.ts index e5334c70..2ef7d323 100644 --- a/services/fs/settingsFsStore.ts +++ b/services/fs/settingsFsStore.ts @@ -96,15 +96,13 @@ export class FsSettingsStore extends FsCore { const configPath = await apis.join(appDataPath, 'config'); if (!(await apis.exists(configPath))) await apis.mkdir(configPath, { recursive: true }); - // QNBS-v3: resolveProtectedWriteKey() throws IdbStorageLockedError when a passphrase is - // configured but the session is locked — propagated deliberately (fail closed) rather than - // silently falling back to plaintext while the user believes encryption is active, matching - // the existing IDB protected-write policy this reuses. + // QNBS-v3: resolveProtectedWriteKey() throws IdbStorageLockedError when configured-but-locked — propagated deliberately (fail closed) rather than silently falling back to plaintext, matching the existing IDB protected-write policy this reuses. const key = await resolveProtectedWriteKey(); const payload: ProtectedApiKeyPayload | PlaintextApiKeyPayload = key ? { scheme: PROTECTED_SCHEME, - data: bytesToBase64(await idbEncryptWithKey(key, apiKey.trim())), + // QNBS-v3: encrypts {provider, apiKey} together (not just the bare key) so a ciphertext swapped between two providers' files decrypts but fails the provider check below, instead of silently handing one provider's key to another. + data: bytesToBase64(await idbEncryptWithKey(key, { provider, apiKey: apiKey.trim() })), } : { scheme: PLAINTEXT_SCHEME, value: apiKey.trim() }; @@ -112,62 +110,86 @@ export class FsSettingsStore extends FsCore { await writeTextFileAtomic(apis, filePath, JSON.stringify(payload)); } - private async readProtectedApiKey(base64Data: string): Promise { + private async readProtectedApiKey(provider: string, base64Data: string): Promise { const key = await resolveProtectedWriteKey(); if (!key) { // Sentinel was cleared/disabled since this key was saved under the old, now-configured-off // passphrase — there is nothing left to decrypt with; treat the same as an unreadable file. throw new Error('Protected API key exists but at-rest encryption is no longer configured'); } - return idbDecryptWithKey(key, base64ToBytes(base64Data)); + const decrypted = await idbDecryptWithKey<{ provider: string; apiKey: string }>( + key, + base64ToBytes(base64Data), + ); + if (decrypted.provider !== provider) { + throw new Error( + `Decrypted payload belongs to provider "${decrypted.provider}", not "${provider}"`, + ); + } + return decrypted.apiKey; } async getApiKey(provider: string): Promise { - // QNBS-v3: separate outer refs (rather than narrowing `apis`/`keyFile` from the try block) — TS's control-flow narrowing doesn't survive into the retryFs() closure, and the catch block below still needs both for the legacy-payload cleanup path. - let apisForCleanup: TauriApis | undefined; - let keyFileForCleanup: string | undefined; + const apis = await this.getApis(); + const appDataPath = await this.ensureAppDataPath(); + const keyFile = await apis.join(appDataPath, 'config', `${provider}_key.enc.json`); + + let parsed: Record; try { - const apis = await this.getApis(); - apisForCleanup = apis; - const appDataPath = await this.ensureAppDataPath(); - const keyFile = await apis.join(appDataPath, 'config', `${provider}_key.enc.json`); - keyFileForCleanup = keyFile; if (!(await apis.exists(keyFile))) return null; const content = await retryFs(() => apis.readTextFile(keyFile)); - const parsed = JSON.parse(content) as Record; - - if (parsed['scheme'] === PLAINTEXT_SCHEME && typeof parsed['value'] === 'string') { - return parsed['value']; - } - if (parsed['scheme'] === PROTECTED_SCHEME && typeof parsed['data'] === 'string') { - return await this.readProtectedApiKey(parsed['data']); - } - // Anything else is one of the two obsolete pre-2026-08-13 formats (unsalted single-SHA-256, - // or salted-but-publicly-derivable-passphrase — F-05/F-06) — neither provides real - // confidentiality and neither is migrated by design; fall through to the discard+notify path. - throw new Error('Unrecognized or obsolete API key payload format'); + parsed = JSON.parse(content) as Record; } catch (error) { - if (error instanceof IdbStorageLockedError) { - // Configured but locked — the key still exists, just temporarily unreadable. Fail closed without discarding the file; the caller re-prompts once the session is unlocked. - return null; - } - // QNBS-v3: pre-2026-08-13 key files (both obsolete crypto schemes) are not migrated (locked decision — discard); removing the stale file avoids retrying every call, and a one-time notification beats a silent null indistinguishable from "no key ever set". - if (apisForCleanup && keyFileForCleanup) { - try { - await apisForCleanup.remove(keyFileForCleanup); - appStoreRef.current?.dispatch( - statusActions.addNotification({ - type: 'info', - title: 'API Key Reset Required', - description: `Your saved ${provider} API key was encrypted with a scheme that has since been hardened and could not be read. Please re-enter it in Settings → AI.`, - }), - ); - } catch (cleanupError) { - logger.warn(`Failed to remove stale key file for provider "${provider}":`, cleanupError); + // A transient read/parse failure is not evidence the format is obsolete — preserve the file. + logger.warn(`Failed to read API key file for provider "${provider}":`, error); + return null; + } + + if (parsed['scheme'] === PLAINTEXT_SCHEME && typeof parsed['value'] === 'string') { + return parsed['value']; + } + + if (parsed['scheme'] === PROTECTED_SCHEME && typeof parsed['data'] === 'string') { + try { + return await this.readProtectedApiKey(provider, parsed['data']); + } catch (error) { + if (error instanceof IdbStorageLockedError) { + // Configured but locked — the key still exists, just temporarily unreadable. Fail closed without discarding the file; the caller re-prompts once the session is unlocked. + return null; } + // QNBS-v3: a RECOGNIZED protected-v1 envelope that fails to decrypt (rotated passphrase, transient sentinel-lookup error) is NOT an obsolete format — never discard on an ambiguous decrypt failure; only genuinely unrecognized shapes are discarded below. + logger.warn( + `Failed to decrypt protected API key for provider "${provider}" (file preserved, not discarded):`, + error, + ); + return null; } - logger.warn(`Failed to decrypt API key for provider "${provider}":`, error); - return null; + } + + // Anything else is one of the two obsolete pre-2026-08-13 formats (unsalted single-SHA-256, or + // salted-but-publicly-derivable-passphrase — F-05/F-06) — neither provides real confidentiality + // and neither is migrated by design; this is the only path that discards the file. + await this.discardObsoleteApiKeyFile(provider, apis, keyFile); + return null; + } + + // QNBS-v3: pre-2026-08-13 key files (both obsolete crypto schemes) are not migrated (locked decision — discard); removing the stale file avoids retrying every call, and a one-time notification beats a silent null indistinguishable from "no key ever set". + private async discardObsoleteApiKeyFile( + provider: string, + apis: TauriApis, + keyFile: string, + ): Promise { + try { + await apis.remove(keyFile); + appStoreRef.current?.dispatch( + statusActions.addNotification({ + type: 'info', + title: 'API Key Reset Required', + description: `Your saved ${provider} API key was encrypted with a scheme that has since been hardened and could not be read. Please re-enter it in Settings → AI.`, + }), + ); + } catch (cleanupError) { + logger.warn(`Failed to remove stale key file for provider "${provider}":`, cleanupError); } } diff --git a/tests/unit/services/fs/fsStores.test.ts b/tests/unit/services/fs/fsStores.test.ts index cdbb1990..5e17e890 100644 --- a/tests/unit/services/fs/fsStores.test.ts +++ b/tests/unit/services/fs/fsStores.test.ts @@ -251,8 +251,7 @@ describe('FsSettingsStore — settings + encrypted API keys', () => { expect(await store.getApiKey('openai')).toBeNull(); }); - // QNBS-v3: the actual fix under test — a real, non-public secret protects the key when the - // user has configured and unlocked at-rest encryption. + // QNBS-v3: a real, non-public secret protects the key when the user has configured and unlocked at-rest encryption. it('round-trips an API key under real AES-GCM protection when a passphrase is configured and unlocked', async () => { cryptoState.activeKey = await new StorageEncryptionService().deriveKey( 'test-passphrase', @@ -268,8 +267,7 @@ describe('FsSettingsStore — settings + encrypted API keys', () => { expect(await store.getApiKey('openai')).toBe('sk-secret-123'); }); - // QNBS-v3: fail-closed, matching the existing IDB protected-write policy — never silently - // downgrade to plaintext while the user believes at-rest encryption is protecting this key. + // QNBS-v3: fail-closed, matching the existing IDB protected-write policy — never silently downgrade to plaintext while the user believes at-rest encryption is protecting this key. it('rejects saveApiKey when at-rest encryption is configured but the session is locked', async () => { cryptoState.sentinelConfigured = true; // configured, but no activeKey — locked @@ -277,8 +275,7 @@ describe('FsSettingsStore — settings + encrypted API keys', () => { expect(fake.text.has('/app/config/openai_key.enc.json')).toBe(false); }); - // QNBS-v3: a locked-but-configured read must fail closed WITHOUT discarding the file — the key - // is still there, just temporarily unreadable until the user unlocks their session. + // QNBS-v3: a locked-but-configured read must fail closed WITHOUT discarding the file — the key is still there, just temporarily unreadable until the user unlocks their session. it('returns null without discarding the file when reading a protected key while locked', async () => { cryptoState.activeKey = await new StorageEncryptionService().deriveKey( 'test-passphrase', @@ -293,6 +290,49 @@ describe('FsSettingsStore — settings + encrypted API keys', () => { expect(fake.text.has('/app/config/openai_key.enc.json')).toBe(true); }); + // QNBS-v3: critical fix — a passphrase rotation (active key no longer matching what a protected-v1 file was encrypted under) must NEVER discard the file; previously any non-locked decrypt failure fell into the generic discard path and would have permanently deleted a still-valid key. + it('preserves a protected-v1 file that fails to decrypt under a different (rotated) key, instead of discarding it', async () => { + cryptoState.activeKey = await new StorageEncryptionService().deriveKey( + 'old-passphrase', + new Uint8Array(32).fill(7), + ); + cryptoState.sentinelConfigured = true; + await store.saveApiKey('openai', 'sk-secret-123'); + + // Simulate a completed passphrase rotation: a different active key, same salt. + cryptoState.activeKey = await new StorageEncryptionService().deriveKey( + 'new-passphrase', + new Uint8Array(32).fill(7), + ); + + expect(await store.getApiKey('openai')).toBeNull(); + // The file must still be there — not silently deleted. + expect(fake.text.has('/app/config/openai_key.enc.json')).toBe(true); + }); + + // QNBS-v3: provider-identity binding — a ciphertext swapped between two providers' files decrypts under the same key but fails the provider check, so it's rejected (and preserved), not silently handed to the wrong provider. + it('rejects (without discarding) a protected-v1 file whose ciphertext belongs to a different provider', async () => { + cryptoState.activeKey = await new StorageEncryptionService().deriveKey( + 'test-passphrase', + new Uint8Array(32).fill(7), + ); + cryptoState.sentinelConfigured = true; + await store.saveApiKey('openai', 'sk-openai-secret'); + await store.saveApiKey('anthropic', 'sk-anthropic-secret'); + + // Swap the two providers' encrypted payloads on disk. + const openaiContent = fake.text.get('/app/config/openai_key.enc.json') as string; + const anthropicContent = fake.text.get('/app/config/anthropic_key.enc.json') as string; + fake.text.set('/app/config/openai_key.enc.json', anthropicContent); + fake.text.set('/app/config/anthropic_key.enc.json', openaiContent); + + expect(await store.getApiKey('openai')).toBeNull(); + expect(await store.getApiKey('anthropic')).toBeNull(); + // Neither file is discarded — the ciphertext is intact, just bound to the wrong provider. + expect(fake.text.has('/app/config/openai_key.enc.json')).toBe(true); + expect(fake.text.has('/app/config/anthropic_key.enc.json')).toBe(true); + }); + it('delegates the Gemini key helpers to provider storage', async () => { await store.saveGeminiApiKey('gem-key'); expect(await store.getGeminiApiKey()).toBe('gem-key'); From ac33b6143356c2ef4da376014875e58a5f8041fc Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Thu, 13 Aug 2026 11:21:37 +0200 Subject: [PATCH 3/5] docs(security): update threat model for the real API-key protection scheme docs/SECURITY-THREAT-MODEL.md still described the retired per-file PBKDF2-of-public-material scheme as the current mitigation and pointed to the now-deleted deriveFileSystemCryptoKey(). Updated the desktop API-key row and the local-file-read attack tree to describe the actual replacement (real passphrase-derived key shared with the IDB path, provider-bound envelope, honest plaintext fallback when unconfigured). Co-Authored-By: Claude Sonnet 5 --- docs/SECURITY-THREAT-MODEL.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/docs/SECURITY-THREAT-MODEL.md b/docs/SECURITY-THREAT-MODEL.md index 96e8b829..5bd001df 100644 --- a/docs/SECURITY-THREAT-MODEL.md +++ b/docs/SECURITY-THREAT-MODEL.md @@ -1,7 +1,7 @@ # Security Threat Model **Version:** 1.0.0 -**Date:** 2026-06-05 (baseline); desktop-crypto mitigation row updated 2026-07-29 (v1.24.2, F-05/F-06) +**Date:** 2026-06-05 (baseline); desktop-crypto mitigation row updated 2026-08-13 (F-05/F-06 superseded — see below) **Status:** v1.24.2 baseline This document provides a formal STRIDE threat analysis for WorldScript Studio, mapping threats to mitigations and code locations. @@ -39,7 +39,7 @@ This document provides a formal STRIDE threat analysis for WorldScript Studio, m | Threat | Mitigation | Code Location | |--------|------------|-------------| | API key leakage via logs | StructuredLogger sanitization; never log keys | `services/logger.ts:sanitizeLogContext()` | -| Desktop API key exposure via local file-read access | AES-256-GCM with a PBKDF2-derived key (600 000 iterations, SHA-256, random 32-byte salt per encryption) — fixed 2026-07-29; the prior scheme derived the key from a single unsalted SHA-256 digest of publicly-derivable material (own file's parent path + provider name from the filename + a hardcoded literal), so anyone with read access to `config/_key.enc.json` could reconstruct the key in one hash operation (F-05/F-06). No migration path for pre-fix files by design — a legacy (unsalted) payload is discarded and the user is prompted to re-enter the key. | `services/fs/fsCore.ts:deriveFileSystemCryptoKey()`, `services/fs/settingsFsStore.ts:getApiKey()` | +| Desktop API key exposure via local file-read access | **Fixed 2026-08-13 (supersedes F-05/F-06).** API keys are now protected by the real user at-rest-encryption passphrase (the same `CryptoKey` protecting IDB data — `services/storage/storageEncryptionService.ts`) whenever one is configured and unlocked, AEAD-encrypting `{provider, apiKey}` together so a ciphertext swapped between two providers' files fails the provider check on decrypt. **The two prior F-05/F-06 schemes (both retired, not migrated)**: (1) pre-2026-07-29, unsalted single-SHA-256 of publicly-derivable material; (2) 2026-07-29–2026-08-13, salted PBKDF2 — but of that *same* publicly-derivable material (`${appDataPath}\|${provider}\|WorldScriptStudio\|v1`), so it hardened against rainbow tables without addressing the actual finding (anyone with file-read access could reconstruct the identical string). Neither is migrated — the file is discarded and the user re-prompted for the key. **Honest residual gap**: with no at-rest passphrase configured (the default), API keys are stored as plaintext, not fake-encrypted — this is a disclosed, deliberate tradeoff, not an oversight. | `services/storage/storageEncryptionService.ts`, `services/fs/settingsFsStore.ts:getApiKey()`/`saveApiKey()` | | Manuscript data in IndexedDB | AES-256-GCM at-rest encryption | `services/storage/storageEncryptionService.ts` | | Voice audio to cloud | Web Speech API consent gate | `components/voice/VoicePrivacyConsentModal.tsx` | | DuckDB analytics unencrypted (SEC-6) | **Bounded by design, with one prose column now encrypted:** most persisted fields are local metadata only (titles, loglines, character names, word counts, embeddings) and **nothing leaves the device**. The one column that genuinely holds literal manuscript prose, `codex_mentions.excerpt`, is now cell-level encrypted (AES-256-GCM via `services/duckdb/duckdbEncryption.ts`, reusing the IDB at-rest encryption key) whenever `enableIdbAtRestEncryption` is active: `duckdbCodexWrite()` writes ciphertext into `excerpt_enc BLOB` and nulls the plaintext `excerpt` column; `services/duckdb/codexExcerptEncryptionMigration.ts` backfills any pre-existing plaintext rows once encryption is unlocked. Gated by `enableDuckDbAnalytics` **and** the Settings → Privacy "Analytics" opt-out (`isAnalyticsPersistenceAllowed` in `app/listenerMiddleware.ts`); turning the toggle off stops all DuckDB writes + inference telemetry. Full OPFS file-level encryption remains **infeasible** — DuckDB-WASM owns the OPFS file handle directly, so there is no app-level interception point; the other metadata columns stay intentionally plaintext (bounded-exposure design). | `app/listenerMiddleware.ts:isAnalyticsPersistenceAllowed`, `services/duckdb/duckdbAnalytics.ts:duckdbCodexWrite()`, `services/duckdb/duckdbEncryption.ts`, `services/duckdb/codexExcerptEncryptionMigration.ts` | @@ -124,9 +124,12 @@ Goal: Intercept/decrypt collaboration traffic ``` Goal: Recover a user's cloud-provider API key from the Tauri desktop install ├─ OR: Read config/_key.enc.json directly (local process / malware with user-level FS access) -│ └─ Mitigation: AES-256-GCM with PBKDF2-derived key (600k iter, random 32-byte salt per file) — -│ reading the ciphertext no longer reveals the key material; the pre-2026-07-29 scheme derived -│ the key from data an attacker with file-read access already had (F-05/F-06, fixed) +│ └─ Mitigation (when at-rest encryption is configured+unlocked): AES-256-GCM under the real, +│ user-chosen passphrase-derived key — not derivable from file contents or path. When no +│ passphrase is configured (default), the file is honestly plaintext, not fake-encrypted — a +│ disclosed tradeoff, not a bypass of this mitigation. Both pre-2026-08-13 schemes (F-05/F-06) +│ derived their key from data an attacker with file-read access already had; retired, not +│ migrated (superseded 2026-08-13) ├─ OR: Read the IDB-at-rest passphrase sentinel (enableIdbAtRestEncryption) │ └─ Mitigation: same PBKDF2 + non-extractable-key pattern; session-scoped in-memory key, never │ persisted to disk (`services/storage/storageEncryptionService.ts`) From 64268cbae2902f262872be0428af50fde85afa7a Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:45:02 +0200 Subject: [PATCH 4/5] fix(desktop): guard malformed key payloads, narrow legacy discard, refresh keys on unlock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-loop follow-up on PR #355: - getApiKey() indexed the parsed JSON payload without checking it was actually an object first — a file containing valid-but-non-object JSON (e.g. literal `null`) threw an uncaught TypeError instead of being treated as unreadable. - The "obsolete format, discard" fallback deleted any payload that didn't match a currently-recognized scheme, without verifying it matched one of the two legacy shapes — an unexpected future format or a merely corrupted current-format file would have been permanently destroyed. Now only positively-identified legacy {iv, data} envelopes (no scheme field) are discarded. - AiProviderCard and OpenRouterSection read API keys in a mount-only effect; a key read while the session was still locked stayed shown as missing even after unlock, since nothing re-triggered the fetch. Both now re-fetch when encryptionReady (threaded from SettingsViewContext) changes. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 12 +++++- components/settings/AiProviderCard.tsx | 8 +++- components/settings/AiSections.tsx | 3 +- components/settings/OpenRouterSection.tsx | 9 +++- services/fs/settingsFsStore.ts | 43 ++++++++++++++----- tests/unit/settings/AiSections.test.tsx | 1 + .../unit/settings/OpenRouterSection.test.tsx | 7 +++ tests/unit/settings/SettingsSections.test.tsx | 1 + 8 files changed, 69 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dcf4347..8bf13e7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 no longer permanently destroys an otherwise-valid saved key. Encrypted payloads now bind `{provider, apiKey}` together (not just the bare key string), so a ciphertext swapped between two providers' files decrypts under the same key but fails the provider check, closing a cross-file - substitution gap. + substitution gap. **Second round of review-loop follow-up fixes:** a key file containing valid + JSON that isn't an object (e.g. the literal `null`) previously threw an uncaught `TypeError` when + indexed instead of being treated as unreadable — now guarded explicitly. The "obsolete format, + discard" fallback previously deleted *any* payload that didn't match a currently-recognized + scheme, without checking it actually matched one of the two legacy shapes — an unexpected future + format (e.g. after a rollback) or a merely corrupted current-format file would have been + permanently destroyed instead of preserved; now only payloads positively identified as the legacy + `{iv, data}` shape (no `scheme` field) are discarded. `AiProviderCard` and `OpenRouterSection` + read API keys in a mount-only effect, so a key that read as locked (not "absent") while the + encrypted session was still unlocking stayed shown as missing even after the user unlocked — + both now re-fetch when `encryptionReady` (threaded from `SettingsViewContext`) changes. ### Fixed diff --git a/components/settings/AiProviderCard.tsx b/components/settings/AiProviderCard.tsx index d62e2c64..00d68a50 100644 --- a/components/settings/AiProviderCard.tsx +++ b/components/settings/AiProviderCard.tsx @@ -89,6 +89,10 @@ interface AiProviderCardProps { // instead of requiring desktop when the user has separately configured OLLAMA_ORIGINS. Default // false so callers that don't pass it (e.g. older tests) keep today's desktop-only behavior. browserOllamaEnabled?: boolean; + // QNBS-v3: at-rest-encrypted API keys read as null while the session is locked (fail-closed, not + // "no key saved") — this must be in the key-fetch effect's deps so a mount-time-locked read gets + // retried once the session unlocks, instead of leaving the input permanently blank until reload. + encryptionReady?: boolean; } interface LocalDiagnosticState { @@ -191,6 +195,7 @@ export const AiProviderCard: FC = ({ onProviderChange, onModelSelect, browserOllamaEnabled = false, + encryptionReady, }) => { const { t } = useTranslation(); const provider = advancedAi.provider; @@ -297,6 +302,7 @@ export const AiProviderCard: FC = ({ }, [provider, probeWebGpu]); useEffect(() => { + void encryptionReady; storageService .getApiKey('openai') .then((k) => setOpenaiKey(k ?? '')) @@ -309,7 +315,7 @@ export const AiProviderCard: FC = ({ .getApiKey('anthropic') .then((k) => setAnthropicKey(k ?? '')) .catch(() => {}); - }, []); + }, [encryptionReady]); // QNBS-v3: save/clear via storageService, matching every other provider's key persistence. const handleSaveGrokKey = useCallback(async () => { diff --git a/components/settings/AiSections.tsx b/components/settings/AiSections.tsx index 1b255bb4..992dd3d7 100644 --- a/components/settings/AiSections.tsx +++ b/components/settings/AiSections.tsx @@ -49,7 +49,7 @@ const isCustomOllamaModel = (model: string) => model.startsWith('ollama/') && !KNOWN_OLLAMA_MODELS.has(model); export const AiSection: FC = () => { - const { t, settings, handleSettingChange } = useSettingsViewContext(); + const { t, settings, handleSettingChange, encryptionReady } = useSettingsViewContext(); // QNBS-v3: Issue 10 — gate at the parent level so useAdaptiveAi hook + device profiling // never run when the feature flag is off (saves GPU queries + IDB reads on every settings open) const adaptiveAiEnabled = useAppSelector(selectEnableAdaptiveAiEngine); @@ -71,6 +71,7 @@ export const AiSection: FC = () => { handleSettingChange('advancedAi', { ...settings.advancedAi, ...patch }) } diff --git a/components/settings/OpenRouterSection.tsx b/components/settings/OpenRouterSection.tsx index 7677cccf..58f1b614 100644 --- a/components/settings/OpenRouterSection.tsx +++ b/components/settings/OpenRouterSection.tsx @@ -7,6 +7,7 @@ import type { FC } from 'react'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useAppDispatch, useAppSelector } from '../../app/hooks'; +import { useSettingsViewContext } from '../../contexts/SettingsViewContext'; import { settingsActions } from '../../features/settings/settingsSlice'; import { statusActions } from '../../features/status/statusSlice'; import { useTranslation } from '../../hooks/useTranslation'; @@ -110,6 +111,7 @@ const CircuitBreakerStatus: FC<{ t: ReturnType['t'] }> = export const OpenRouterSection: FC = () => { const { t } = useTranslation(); const dispatch = useAppDispatch(); + const { encryptionReady } = useSettingsViewContext(); const openRouterSettings = useAppSelector((s) => s.settings.openRouter); const aiMode = useAppSelector((s) => s.settings.aiMode); const privacy = useAppSelector((s) => s.settings.privacy); @@ -166,8 +168,11 @@ export const OpenRouterSection: FC = () => { // afterwards and overwrite the user's newer key state with a stale value. const keyOverriddenRef = useRef(false); - // Load stored key status on mount. + // QNBS-v3: re-runs when encryptionReady changes, not just on mount — a mount-time-locked read + // (fail-closed null, not "no key saved") must be retried once the session unlocks, instead of + // leaving the key permanently shown as missing until this component remounts/the page reloads. useEffect(() => { + void encryptionReady; let cancelled = false; storageService .getApiKey('openrouter') @@ -184,7 +189,7 @@ export const OpenRouterSection: FC = () => { return () => { cancelled = true; }; - }, []); + }, [encryptionReady]); // QNBS-v3: Monotonic guard so concurrent catalog fetches are last-wins — a slower or failing // response can never overwrite the result of a newer request, and a response that resolves after diff --git a/services/fs/settingsFsStore.ts b/services/fs/settingsFsStore.ts index 2ef7d323..52b28fa3 100644 --- a/services/fs/settingsFsStore.ts +++ b/services/fs/settingsFsStore.ts @@ -134,24 +134,35 @@ export class FsSettingsStore extends FsCore { const appDataPath = await this.ensureAppDataPath(); const keyFile = await apis.join(appDataPath, 'config', `${provider}_key.enc.json`); - let parsed: Record; + let parsed: unknown; try { if (!(await apis.exists(keyFile))) return null; const content = await retryFs(() => apis.readTextFile(keyFile)); - parsed = JSON.parse(content) as Record; + parsed = JSON.parse(content); } catch (error) { // A transient read/parse failure is not evidence the format is obsolete — preserve the file. logger.warn(`Failed to read API key file for provider "${provider}":`, error); return null; } - if (parsed['scheme'] === PLAINTEXT_SCHEME && typeof parsed['value'] === 'string') { - return parsed['value']; + // QNBS-v3: JSON.parse succeeds on non-object payloads too (e.g. literal `null`, a bare number) + // — indexing those below would throw a TypeError instead of returning null, so guard explicitly + // rather than asserting the shape via a cast. + if (typeof parsed !== 'object' || parsed === null) { + logger.warn( + `API key file for provider "${provider}" is not a recognizable object (preserved, not discarded).`, + ); + return null; + } + const record = parsed as Record; + + if (record['scheme'] === PLAINTEXT_SCHEME && typeof record['value'] === 'string') { + return record['value']; } - if (parsed['scheme'] === PROTECTED_SCHEME && typeof parsed['data'] === 'string') { + if (record['scheme'] === PROTECTED_SCHEME && typeof record['data'] === 'string') { try { - return await this.readProtectedApiKey(provider, parsed['data']); + return await this.readProtectedApiKey(provider, record['data']); } catch (error) { if (error instanceof IdbStorageLockedError) { // Configured but locked — the key still exists, just temporarily unreadable. Fail closed without discarding the file; the caller re-prompts once the session is unlocked. @@ -166,10 +177,22 @@ export class FsSettingsStore extends FsCore { } } - // Anything else is one of the two obsolete pre-2026-08-13 formats (unsalted single-SHA-256, or - // salted-but-publicly-derivable-passphrase — F-05/F-06) — neither provides real confidentiality - // and neither is migrated by design; this is the only path that discards the file. - await this.discardObsoleteApiKeyFile(provider, apis, keyFile); + // QNBS-v3: only positively-identified pre-2026-08-13 legacy envelopes (iv+data strings, no + // scheme field — the exact shape the retired encryptText()/decryptText() wrote) are discarded. + // An unrecognized-but-not-legacy shape (a future format after a rollback, or a merely corrupted + // current-format file) is preserved instead of guessed away — same "never destroy on ambiguous + // read" rule as the decrypt-failure branch above. + const looksLikeLegacyEnvelope = + typeof record['iv'] === 'string' && + typeof record['data'] === 'string' && + !('scheme' in record); + if (looksLikeLegacyEnvelope) { + await this.discardObsoleteApiKeyFile(provider, apis, keyFile); + } else { + logger.warn( + `API key file for provider "${provider}" has an unrecognized format (preserved, not discarded).`, + ); + } return null; } diff --git a/tests/unit/settings/AiSections.test.tsx b/tests/unit/settings/AiSections.test.tsx index 27fbf694..bd242f6f 100644 --- a/tests/unit/settings/AiSections.test.tsx +++ b/tests/unit/settings/AiSections.test.tsx @@ -36,6 +36,7 @@ vi.mock('../../../contexts/SettingsViewContext', () => ({ }, featureFlags: { enableDuckDbAnalytics: false }, handleSettingChange: mockHandleSettingChange, + encryptionReady: false, }), })); diff --git a/tests/unit/settings/OpenRouterSection.test.tsx b/tests/unit/settings/OpenRouterSection.test.tsx index ce6fca79..e76bb747 100644 --- a/tests/unit/settings/OpenRouterSection.test.tsx +++ b/tests/unit/settings/OpenRouterSection.test.tsx @@ -78,6 +78,13 @@ vi.mock('../../../services/storageService', () => ({ }, })); +// QNBS-v3: OpenRouterSection now reads encryptionReady from SettingsViewContext to re-fetch the +// key once a locked session unlocks — mock the context module rather than wrapping in the real +// provider tree, matching this repo's established pattern for use*ViewContext hooks. +vi.mock('../../../contexts/SettingsViewContext', () => ({ + useSettingsViewContext: () => ({ encryptionReady: true }), +})); + vi.mock('../../../services/logger', () => ({ logger: { debug: vi.fn(), diff --git a/tests/unit/settings/SettingsSections.test.tsx b/tests/unit/settings/SettingsSections.test.tsx index 97ca28e1..36fd262d 100644 --- a/tests/unit/settings/SettingsSections.test.tsx +++ b/tests/unit/settings/SettingsSections.test.tsx @@ -64,6 +64,7 @@ const baseContextValue = { handleDeleteSnapshot: vi.fn(), projectSize: '2.3 KB', currentWordCount: 0, + encryptionReady: false, }; vi.mock('../../../contexts/SettingsViewContext', () => ({ From c16ea10d640abd233c4c5a8d1040c59a3f53ece7 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Thu, 13 Aug 2026 14:44:31 +0200 Subject: [PATCH 5/5] fix(desktop): keep QNBS-v3 comments on one physical line Review-loop follow-up on PR #355: two comments introduced by the null-payload guard and legacy-discard fixes were each wrapped across multiple // lines. Condensed both to one physical line each. Co-Authored-By: Claude Sonnet 5 --- services/fs/settingsFsStore.ts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/services/fs/settingsFsStore.ts b/services/fs/settingsFsStore.ts index 52b28fa3..b04d046a 100644 --- a/services/fs/settingsFsStore.ts +++ b/services/fs/settingsFsStore.ts @@ -145,9 +145,7 @@ export class FsSettingsStore extends FsCore { return null; } - // QNBS-v3: JSON.parse succeeds on non-object payloads too (e.g. literal `null`, a bare number) - // — indexing those below would throw a TypeError instead of returning null, so guard explicitly - // rather than asserting the shape via a cast. + // QNBS-v3: JSON.parse succeeds on non-object payloads too (e.g. literal `null`) — indexing those below would throw instead of returning null, so guard explicitly rather than asserting the shape via a cast. if (typeof parsed !== 'object' || parsed === null) { logger.warn( `API key file for provider "${provider}" is not a recognizable object (preserved, not discarded).`, @@ -177,11 +175,7 @@ export class FsSettingsStore extends FsCore { } } - // QNBS-v3: only positively-identified pre-2026-08-13 legacy envelopes (iv+data strings, no - // scheme field — the exact shape the retired encryptText()/decryptText() wrote) are discarded. - // An unrecognized-but-not-legacy shape (a future format after a rollback, or a merely corrupted - // current-format file) is preserved instead of guessed away — same "never destroy on ambiguous - // read" rule as the decrypt-failure branch above. + // QNBS-v3: only positively-identified legacy envelopes (iv+data strings, no scheme field — the retired encryptText()/decryptText() shape) are discarded; anything else is preserved, same "never destroy on ambiguous read" rule as the decrypt-failure branch above. const looksLikeLegacyEnvelope = typeof record['iv'] === 'string' && typeof record['data'] === 'string' &&