Repository navigation
security(desktop): encrypt project data at rest (text stores) #356
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,17 +18,20 @@ import { settingsActions } from '../features/settings/settingsSlice'; | |
| import { statusActions } from '../features/status/statusSlice'; | ||
| import { useTranslation } from '../hooks/useTranslation'; | ||
| import { wipeAllAppData } from '../services/factoryResetService'; | ||
| import { migrateAllProtectedFsData } from '../services/fs/fsEncryptionMigration'; | ||
| import { logger } from '../services/logger'; | ||
| import type { ProtectedStoreMigrationProgress } from '../services/storage/protectedStoreMigration'; | ||
| import { | ||
| clearIdbEncryptionKey, | ||
| clearIdbPassphrase, | ||
| deriveRotationTargetKey, | ||
| isIdbEncryptionReady, | ||
| rotateIdbPassphrase, | ||
| setupIdbEncryption, | ||
| verifyAndInitIdbEncryption, | ||
| } from '../services/storage/storageEncryptionService'; | ||
| import { storageService } from '../services/storageService'; | ||
| import { isTauriRuntime } from '../services/tauriRuntime'; | ||
| import type { | ||
| AccessibilitySettings, | ||
| AdvancedAiSettings, | ||
|
|
@@ -398,6 +401,8 @@ export const useSettingsView = () => { | |
| // QNBS-v3: clearIdbPassphrase() requires an already-unlocked session key — no passphrase re-entry. | ||
| setMigrationProgress(null); | ||
| try { | ||
| // QNBS-v3: must convert fs-backed desktop data to plaintext BEFORE the sentinel below is destroyed — clearIdbPassphrase() has no awareness of services/fs/*, so ordering here is load-bearing, not cosmetic. | ||
| if (isTauriRuntime()) await migrateAllProtectedFsData(null); | ||
| await clearIdbPassphrase((progress) => setMigrationProgress(progress)); | ||
|
There was a problem hiding this comment.
During desktop disable or rotation, normal project/settings autosaves remain active while this bridge and the subsequent IDB migration run. Because filesystem writes resolve the still-active old key without consulting the IDB migration journal, a pending save can overwrite a file after the bridge processed it but before Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Valid finding, deliberately not fixed inline — same root cause as #360 (no admission lock held across the fs migration and the IDB key commit), tracked there. |
||
| } finally { | ||
| setMigrationProgress(null); | ||
|
|
@@ -408,6 +413,11 @@ export const useSettingsView = () => { | |
| } else if (passphraseModal === 'rotate') { | ||
| setMigrationProgress(null); | ||
| try { | ||
| // QNBS-v3: derives the SAME target key rotateIdbPassphrase() will activate (same salt/passphrase) and re-keys fs-backed desktop data under it BEFORE the active session key is swapped below — otherwise fs data stays under the old, soon-unrecoverable key. | ||
| if (isTauriRuntime()) { | ||
| const targetKey = await deriveRotationTargetKey(newPassphrase); | ||
| await migrateAllProtectedFsData(targetKey); | ||
|
qnbs marked this conversation as resolved.
Outdated
|
||
| } | ||
| await rotateIdbPassphrase(_current, newPassphrase, (progress) => | ||
| setMigrationProgress(progress), | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,11 @@ | |
|
|
||
| import LZString from 'lz-string'; | ||
| import { logger } from '../logger'; | ||
| import { | ||
| idbDecryptWithKey, | ||
| idbEncryptWithKey, | ||
| resolveProtectedWriteKey, | ||
| } from '../storage/storageEncryptionService'; | ||
|
|
||
| // Dynamic imports for Tauri v2 plugin APIs — fail gracefully in browser | ||
| export type TauriApis = { | ||
|
|
@@ -149,6 +154,91 @@ export function writeFileAtomic(apis: TauriApis, path: string, data: Uint8Array) | |
| }); | ||
| } | ||
|
|
||
| // --- Protected text files (opportunistic at-rest encryption) --- | ||
| // QNBS-v3 (2026-08-13): desktop project/settings/snapshot/Codex/RAG-vector data was previously | ||
| // always plaintext, regardless of the "Encrypt project data at rest" setting — enabling it only | ||
| // gated the IndexedDB path (web build); this fs-backed store ignored it entirely. Reuses | ||
| // services/storage/storageEncryptionService.ts's real user-passphrase-derived key directly (same | ||
|
qodo-code-review[bot] marked this conversation as resolved.
|
||
| // pattern as settingsFsStore.ts's API-key fix) rather than a second parallel crypto/migration | ||
|
qnbs marked this conversation as resolved.
|
||
| // system. Lazy/opportunistic migration by design: a save encrypts if a key is currently available; | ||
| // a read transparently handles either format. Existing plaintext files stay plaintext until their | ||
| // next save (autosave already runs on a short interval) — no explicit "migrate everything now" | ||
| // step, no data-loss risk, and no new failure mode beyond what saves already have. | ||
|
|
||
| const PROTECTED_TEXT_SCHEME = 'protected-v1'; | ||
|
|
||
| interface ProtectedTextEnvelope { | ||
| scheme: typeof PROTECTED_TEXT_SCHEME; | ||
| data: string; | ||
| } | ||
|
|
||
| function parseProtectedTextEnvelope(raw: string): ProtectedTextEnvelope | null { | ||
| // QNBS-v3: plaintext content here is compressData()'s output — either plain JSON or an | ||
| // LZ-compressed string carrying its own \x00lz1\x00 sentinel, never valid envelope JSON. A | ||
| // JSON.parse failure (the LZ-compressed case) or a shape mismatch both simply mean "not | ||
| // protected" rather than an error — this is deliberately a probe, not a strict parser. | ||
| try { | ||
| const parsed: unknown = JSON.parse(raw); | ||
| if ( | ||
| parsed !== null && | ||
| typeof parsed === 'object' && | ||
| (parsed as Record<string, unknown>)['scheme'] === PROTECTED_TEXT_SCHEME && | ||
| typeof (parsed as Record<string, unknown>)['data'] === 'string' | ||
| ) { | ||
| return parsed as ProtectedTextEnvelope; | ||
|
qnbs marked this conversation as resolved.
Outdated
|
||
| } | ||
| } catch { | ||
| /* not JSON at all — definitely plaintext (or LZ-compressed plaintext) */ | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Protect a single text value (e.g. compressData()'s output), encrypted under the real at-rest key | ||
| * when one is available. Value-level, not file-level — lets a caller (snapshotFsStore.ts) embed | ||
| * the result as one field inside an otherwise-plaintext JSON envelope, so listing/metadata reads | ||
| * never need to decrypt (mirrors the IDB path's own "encryption applied at the value level" design). | ||
| */ | ||
| export async function protectTextValue(plaintext: string): Promise<string> { | ||
| const key = await resolveProtectedWriteKey(); | ||
| if (!key) return plaintext; | ||
|
qodo-code-review[bot] marked this conversation as resolved.
|
||
| return JSON.stringify({ | ||
| scheme: PROTECTED_TEXT_SCHEME, | ||
| data: bytesToBase64(await idbEncryptWithKey(key, plaintext)), | ||
|
qnbs marked this conversation as resolved.
qnbs marked this conversation as resolved.
Comment on lines
+228
to
+230
There was a problem hiding this comment.
When multiple filesystem records use the same encryption key, these envelopes are transferable because neither the path nor a logical store/record identity is authenticated. A valid encrypted Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Valid finding, deliberately not fixed inline — tracked in #361 (opened this round). Binding ciphertext to its record identity (AAD or an embedded identity check, mirroring what the API-key path already does for {provider, apiKey}) is a design decision with its own migration story, scoped separately from this PR. |
||
| }); | ||
|
Comment on lines
+221
to
+231
There was a problem hiding this comment. Suggestion: Filesystem writes resolve and capture the current key without participating in Severity Level: Critical 🚨- ❌ Desktop project saves can become unreadable after passphrase rotation.
- ❌ `FsProjectStore.loadProject()` returns null after decryption failure.
- ⚠️ Filesystem data is outside the IndexedDB migration adapter set.
Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** services/fs/fsCore.ts
**Line:** 170:176
**Comment:**
*Race Condition: Filesystem writes resolve and capture the current key without participating in `withProtectedWriteAdmission` or checking `assertNoActiveEncryptionMigration`. During passphrase rotation, a save can encrypt a file with the old key while the IDB migration completes and switches the active key to the new generation; subsequent reads then cannot decrypt that filesystem file. Hold the protected-write admission across key resolution, encryption, and the filesystem commit, or otherwise make filesystem writes part of the migration protocol.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Owner
Author
There was a problem hiding this comment. Valid finding, deliberately not fixed inline — tracked in #360 (opened this round), which covers this together with the related read-side and set-time-write races other reviewers flagged on this same PR. Closing it properly means giving the fs path the same Web Locks admission control the IDB path already has, which is a real architectural addition best scoped on its own rather than folded into an already-large PR. |
||
| } | ||
|
|
||
| /** | ||
| * Reverse of protectTextValue — transparently handles either a protected envelope or plain | ||
| * (legacy/unprotected) text. Propagates IdbStorageLockedError when the value is protected but the | ||
| * session is locked — callers should let that fail closed like any other protected read, not fall | ||
| * back to treating it as absent. | ||
| */ | ||
| export async function unprotectTextValue(stored: string): Promise<string> { | ||
| const envelope = parseProtectedTextEnvelope(stored); | ||
| if (!envelope) return stored; | ||
| const key = await resolveProtectedWriteKey(); | ||
|
qodo-code-review[bot] marked this conversation as resolved.
Outdated
|
||
| if (!key) { | ||
| // Sentinel was cleared/disabled since this value was saved — nothing left to decrypt with. | ||
| throw new Error('Protected value exists but at-rest encryption is no longer configured'); | ||
| } | ||
| return idbDecryptWithKey<string>(key, base64ToBytes(envelope.data)); | ||
|
qnbs marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /** Whole-file variant of protectTextValue, for stores with no separate plaintext metadata to preserve. */ | ||
| export async function writeProtectedTextFileAtomic( | ||
| apis: TauriApis, | ||
| path: string, | ||
| plaintext: string, | ||
| ): Promise<void> { | ||
| await writeTextFileAtomic(apis, path, await protectTextValue(plaintext)); | ||
|
qnbs marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /** Whole-file variant of unprotectTextValue, for stores with no separate plaintext metadata to preserve. */ | ||
| export async function readProtectedTextFile(apis: TauriApis, path: string): Promise<string> { | ||
| return unprotectTextValue(await retryFs(() => apis.readTextFile(path))); | ||
|
Comment on lines
+324
to
+325
There was a problem hiding this comment.
When a disable or rotation overlaps a filesystem read that has already started, this fetches the old bytes before Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| // --- LZ-String compression (mirrors dbService threshold and prefix) --- | ||
|
|
||
| const COMPRESS_THRESHOLD = 10_240; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.