diff --git a/CHANGELOG.md b/CHANGELOG.md index 160abc94..8bf13e7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,42 @@ 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. **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. **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 - **Atomic writes for desktop filesystem storage.** Every `services/fs/*Store.ts` writer 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/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`) 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..b04d046a 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,46 +96,117 @@ 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 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, + // 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() }; + 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(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'); + } + 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: unknown; 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 payload = JSON.parse(content) as { iv: string; salt?: string; data: string }; - return await decryptText(payload, `${appDataPath}|${provider}|WorldScriptStudio|v1`); + parsed = JSON.parse(content); } 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 (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; + } + + // 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).`, + ); + return null; + } + const record = parsed as Record; + + if (record['scheme'] === PLAINTEXT_SCHEME && typeof record['value'] === 'string') { + return record['value']; + } + + if (record['scheme'] === PROTECTED_SCHEME && typeof record['data'] === 'string') { + try { + 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. + 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; + } + + // 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' && + !('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; + } + + // 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/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..5e17e890 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,99 @@ 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: 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); + }); + + // 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'); @@ -239,10 +346,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 +377,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. 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', () => ({