security(desktop): encrypt project data at rest (text stores) - #356
security(desktop): encrypt project data at rest (text stores)#356qnbs wants to merge 12 commits into
Conversation
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideImplements opportunistic at-rest encryption for desktop filesystem text stores by introducing shared protected-text helpers in fsCore and wiring them into all relevant Fs*Stores, plus comprehensive unit tests and changelog documentation. Sequence diagram for opportunistic encrypted project save and loadsequenceDiagram
actor User
participant FsProjectStore
participant fsCore
participant storageEncryptionService
participant TauriApis as TauriApis
User->>FsProjectStore: saveProject(projectId, project)
FsProjectStore->>FsProjectStore: compressData(flat)
FsProjectStore->>fsCore: writeProtectedTextFileAtomic(apis, path, plaintext)
fsCore->>storageEncryptionService: resolveProtectedWriteKey()
alt key available
fsCore->>storageEncryptionService: idbEncryptWithKey(key, plaintext)
fsCore->>fsCore: bytesToBase64(ciphertext)
fsCore->>TauriApis: writeTextFile(path, protectedEnvelope)
else no key
fsCore->>TauriApis: writeTextFile(path, plaintext)
end
User->>FsProjectStore: loadProject(projectId)
FsProjectStore->>fsCore: readProtectedTextFile(apis, path)
fsCore->>TauriApis: readTextFile(path)
fsCore->>fsCore: parseProtectedTextEnvelope(raw)
alt protected envelope
fsCore->>storageEncryptionService: resolveProtectedWriteKey()
fsCore->>storageEncryptionService: idbDecryptWithKey(key, bytes)
fsCore-->>FsProjectStore: plaintext
else legacy/plaintext
fsCore-->>FsProjectStore: raw
end
FsProjectStore->>FsProjectStore: decompressData(plaintext)
FsProjectStore-->>User: StoryProject
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
codeant-ai
Bot
added
the
size:L
🏁 CodeAnt Quality Gate ResultsCommit: ❌ Overall Status: FAILEDQuality Gate Details
View Failure Result🐛 Bugs — 1 issues
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
unprotectTextValuepath throws a genericErrorwith a hardcoded message when encryption is no longer configured; consider using a dedicated error type or reusing existing domain errors so callers can reliably distinguish misconfiguration from other failures without relying on string matching. - The StorageEncryptionService mocking pattern with
cryptoStateand hoisted setup is duplicated betweenfsCore.test.tsandfsStores.test.ts; factoring this into a shared test helper would reduce repetition and make it easier to keep the simulated sentinel/session state behavior consistent across suites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `unprotectTextValue` path throws a generic `Error` with a hardcoded message when encryption is no longer configured; consider using a dedicated error type or reusing existing domain errors so callers can reliably distinguish misconfiguration from other failures without relying on string matching.
- The StorageEncryptionService mocking pattern with `cryptoState` and hoisted setup is duplicated between `fsCore.test.ts` and `fsStores.test.ts`; factoring this into a shared test helper would reduce repetition and make it easier to keep the simulated sentinel/session state behavior consistent across suites.
## Individual Comments
### Comment 1
<location path="services/fs/assetFsStore.ts" line_range="53-54" />
<code_context>
}
- const base64Data = await retryFs(() => apis.readTextFile(imageFile));
+ const stored = await retryFs(() => apis.readTextFile(imageFile));
+ const base64Data = await unprotectTextValue(stored);
return `data:image/png;base64,${base64Data}`;
} catch (error) {
</code_context>
<issue_to_address>
**suggestion:** Consider reusing readProtectedTextFile for images to avoid duplicating the read+unprotect pattern.
The new image path reimplements `readProtectedTextFile` by calling `retryFs(readTextFile)` and then `unprotectTextValue`. Using `const base64Data = await readProtectedTextFile(apis, imageFile);` here would keep protected file reads consistent across stores and avoid duplicating this logic, and should behave identically since images are already stored as raw text files.
Suggested implementation:
```typescript
const base64Data = await readProtectedTextFile(apis, imageFile);
```
1. Ensure `readProtectedTextFile` is imported at the top of `services/fs/assetFsStore.ts` from the same module where `protectTextValue`/`unprotectTextValue` and `retryFs` are defined.
2. If `readProtectedTextFile` currently has a different signature, add or adjust an overload so it accepts `(apis, imageFile)` in the same way other callers do.
</issue_to_address>
### Comment 2
<location path="tests/unit/services/fs/fsStores.test.ts" line_range="371-380" />
<code_context>
+ it('protects only the data field when at-rest encryption is configured, keeping name/date/wordCount plaintext and listable', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a snapshot test for the case where encryption is disabled after snapshots have been saved protected
You already cover locked-session behaviour for snapshots. Another important scenario is when snapshots were created with at-rest encryption enabled and the user later disables encryption (sentinel cleared). In that case, `unprotectTextValue` will throw a "no longer configured" error and `getSnapshotData` will follow its error path. Please add a test that creates an encrypted snapshot, then clears `cryptoState.activeKey` and `cryptoState.sentinelConfigured` before calling `getSnapshotData(id)`, to confirm the failure mode and returned value (likely `null`) are intentional.
Suggested implementation:
```typescript
// QNBS-v3: value-level protection, not file-level — only the `data` field is protected so
// listSnapshots() (name/date/wordCount) never needs to decrypt just to render a list.
it('protects only the data field when at-rest encryption is configured, keeping name/date/wordCount plaintext and listable', async () => {
await enableTestPassphrase();
const id = await store.saveSnapshot('My Snapshot', {
manuscript: [{ content: 'secret prose' }],
});
const onDiskFile = [...fake.text.keys()].find((k) => k.endsWith(`${id}.json`)) as string;
const onDisk = JSON.parse(fake.text.get(onDiskFile) as string);
expect(onDisk.name).toBe('My Snapshot'); // metadata stays plaintext
expect(onDisk.data).not.toContain('secret prose'); // content is protected
expect(JSON.parse(onDisk.data).scheme).toBe('protected-v1');
});
it('returns a failure value when encryption is disabled after creating a protected snapshot', async () => {
// Enable at-rest encryption and create a protected snapshot
await enableTestPassphrase();
const id = await store.saveSnapshot('Encrypted Snapshot', {
manuscript: [{ content: 'secret prose' }],
});
// Simulate the user later disabling encryption (sentinel cleared)
// This should cause unprotectTextValue to fail with "no longer configured"
// and getSnapshotData to follow its error path.
cryptoState.activeKey = null;
cryptoState.sentinelConfigured = false;
const result = await store.getSnapshotData(id);
// Snapshot data should follow the failure path (intentionally non-decryptable);
// the current contract is that callers receive a null value.
expect(result).toBeNull();
```
1. Ensure `cryptoState` is imported into `fsStores.test.ts` from the same module that `fsStores` uses for at-rest encryption (the module that defines `activeKey` and `sentinelConfigured`). For example:
`import { cryptoState } from 'src/services/fs/cryptoState';`
2. If your store exposes a higher-level helper to disable passphrase / encryption (e.g. `disableTestPassphrase` or similar), you may prefer to use that instead of mutating `cryptoState` directly. In that case, replace the direct assignments with the appropriate helper call.
3. Confirm the expected failure value from `getSnapshotData` when decryption fails. If the implementation uses something other than `null` (e.g. throws, or returns `{ error: ... }`), update `expect(result).toBeNull();` to assert the actual, intentional contract.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR Summary by QodoEncrypt Tauri desktop project text stores at rest using existing passphrase key
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| export async function protectTextValue(plaintext: string): Promise<string> { | ||
| const key = await resolveProtectedWriteKey(); | ||
| if (!key) return plaintext; | ||
| return JSON.stringify({ | ||
| scheme: PROTECTED_TEXT_SCHEME, | ||
| data: bytesToBase64(await idbEncryptWithKey(key, plaintext)), | ||
| }); |
There was a problem hiding this comment.
Suggestion: 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. [race condition]
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 fixThere 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdc59e16a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Enabling Settings -> Privacy -> "Encrypt project data at rest" only ever protected the browser/PWA build's IndexedDB path. On the Tauri desktop build, services/fs/*Store.ts wrote project.json, settings.json, snapshots, Codex, RAG vectors, and images as plaintext regardless of the setting - the same passphrase unlock screen appeared on desktop, but it gated nothing on the filesystem side. Desktop now reuses services/storage/storageEncryptionService.ts's real passphrase-derived key directly, mirroring the API-key fix in the sibling fix/desktop-api-key-encryption branch: AES-256-GCM protection when a passphrase is configured and unlocked, honest plaintext otherwise. No new module, no changes to that already-audited service. Migration is lazy/opportunistic by design (per discussion): 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 "encrypt everything now" step, no data-loss risk, no new failure mode beyond what saves already have. New shared primitives in fsCore.ts: - protectTextValue/unprotectTextValue - value-level protection. Used directly by snapshotFsStore.ts so only the snapshot's `data` field is protected, keeping name/date/wordCount metadata plaintext - listing snapshots never needs to decrypt just to render names and dates. - writeProtectedTextFileAtomic/readProtectedTextFile - whole-file wrappers around the above + the existing atomic-write primitive, for stores with no metadata/content split (project.json, settings.json, codex.snap, vectors.snap, images). Wired into projectFsStore (project.json), settingsFsStore (settings.json only - API keys are the sibling PR's concern), codexFsStore (codex + RAG vectors), snapshotFsStore (data field only), assetFsStore (images). Not covered by this PR: binder-asset binary blobs (assetFsStore.ts's .bin files) - they need a byte-native encrypt path (StorageEncryptionService's encryptBytes/decryptBytes operating on Uint8Array directly) rather than the JSON-serializing idbEncryptWithKey/idbDecryptWithKey used here, which would be wasteful for potentially-large binary blobs. Tracked as a separate follow-up rather than forcing it into this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cdc59e1 to
8717e94
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8717e94036
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return JSON.stringify({ | ||
| scheme: PROTECTED_TEXT_SCHEME, | ||
| data: bytesToBase64(await idbEncryptWithKey(key, plaintext)), |
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 project.json, snapshot data field, image, or Codex/RAG payload can therefore be moved to another record and still decrypt successfully; the corresponding getters trust the decrypted payload without checking it belongs to the requested project, snapshot, or image. Pass a stable logical context as AES-GCM additional authenticated data, or encrypt and validate the record identity with the payload, so cross-file substitution fails authentication.
Useful? React with 👍 / 👎.
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.
…tate Disabling or rotating the at-rest passphrase destroyed/swapped the shared salt/session key with no awareness that desktop's services/fs/* data (project.json, settings, API keys, snapshots, Codex, RAG vectors, images) depends on the same key material, permanently stranding it. New services/fs/fsEncryptionMigration.ts converts every fs-backed protected file to plaintext (disable) or re-encrypts it under an independently-derived target key (rotate) BEFORE the sentinel/session key is touched; a file that fails to decrypt aborts the whole operation rather than silently stranding it. Wired into useSettingsView's handlePassphraseConfirm, gated on isTauriRuntime(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8106b7a987
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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 clearIdbPassphrase() deletes the key (the rotation branch has the same window before swapping it), leaving fresh ciphertext under the obsolete key. Hold an exclusive filesystem-write admission lock, or otherwise pause and drain saves, across both the filesystem migration and key commit.
Useful? React with 👍 / 👎.
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.
…interrupted migrations Review-loop follow-up on PR #356, addressing an external assessment of the fs-data migration bridge: - First-time "Encrypt project data at rest" setup only protected future writes — every already-existing file (project.json, settings, API keys, snapshots, Codex, RAG, images) stayed plaintext until its next incidental save, despite the UI reporting success immediately. migrateAllProtectedFsData now runs on 'set' too, encrypting every existing file with the newly-active key. Its per-file helpers were generalized to converge a file to whatever state targetKey implies (encrypt-if-plaintext, re-key-if-protected, or decrypt-to-plaintext), rather than only handling the decrypt/re-key direction disable/rotate needed. - 'set' runs in non-strict mode: a file that can't be read (e.g. a stray leftover from a previously-disabled, unrelated encryption session) is logged and skipped rather than aborting the whole setup — nothing valuable is at risk by turning encryption on, unlike disable/rotate where a decrypt failure means an about-to-be-replaced key could be lost. - The bridge has no persistent per-file journal/checkpoint (unlike the IDB migration path), so a process kill mid-rotate could leave a mixed-key filesystem state with no way to detect it. Added a durable marker written before migration starts and cleared only on full success; App.tsx now checks for it once at startup and surfaces a notification if a previous migration didn't finish. This detects, but does not yet resolve, an interrupted migration — full resumable recovery is tracked in issue #359. - Documented that binder-asset `.meta.json` (not just `.bin`) also remains plaintext, including `originalFileName` — filenames can themselves carry sensitive project information. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dd151ca7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…a row to threat model Third review-loop wave on PR #352: - Tightened attacker-capability wording in both the Mitigation Mapping row and the attack tree: recovering the key needs the ciphertext AND the public derivation inputs, not the ciphertext alone. - Added the Gemini-exception note (already in README.md/ IDB-ENCRYPTION.md) to the threat model's own API-key row and attack tree, so all three documents agree. - Added a new Mitigation Mapping row for desktop project/settings/ snapshot/Codex/RAG/image data — the authoritative threat model previously didn't mention this exposure at all, so a reader could conclude manuscript disclosure/tampering was mitigated on desktop when it wasn't. Points at PR #356 for the real fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n propagation, migration safety, and nuclear-reset fs cleanup - Re-throw IdbStorageLockedError (instead of swallowing to null) from all 6 fs-store read methods (project, settings, codex, RAG vectors, snapshot, image) so a locked session surfaces the existing web-build unlock-modal-and-retry flow instead of silently hydrating as a brand-new user. - Verify the current passphrase against the durable sentinel BEFORE the fs migration bridge re-keys any file during rotate, closing a mixed-key bug where a mistyped current passphrase could re-key fs data to a key the IDB side never activates. - Replace exists-then-catch-swallow reads in reprotectWholeFile/reprotectSnapshotFile with exists-first + strict-respecting reads, so strict-mode (disable/rotate) no longer silently skips a genuinely-unreadable file before destroying the only key that could decrypt it. - Wrap reprotectApiKeyFile's body in try/catch so non-strict (set) mode also catches synchronous JSON.parse failures on malformed pre-existing key files, instead of leaving setupIdbEncryption() stranded mid-flow. - Reject (instead of laundering) an API-key ciphertext whose decrypted provider doesn't match its filename during migration, mirroring the existing readProtectedApiKey guard. - Add deleteAllFsData() and call it before IDB/localStorage deletion in both resetAllDatabases() and wipeAllAppData() — neither nuclear-reset flow previously touched Tauri filesystem data despite both destroying the KDF salt, which permanently orphaned any already-encrypted desktop file. - Surface an interrupted fs-migration marker as a startup notification via checkForInterruptedFsMigration(). - Condense all QNBS-v3 comments (including pre-existing ones in App.tsx and useSettingsView.ts) to single physical lines per the project's hard-wrap rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aea6d690e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e-ordering race, set-mode rollback, docs accuracy
- Fix a write-ordering race: writeProtectedTextFileAtomic() used to await protectTextValue() BEFORE entering the per-path write queue, so two overlapping saves could have the OLDER save's slower encryption land in the queue after the newer one and silently overwrite it. Encryption now happens inside the same queue slot that serializes the atomic write (fsCore.ts's new enqueueTextFileWrite), closing the same gap in assetFsStore.ts's saveImage() by routing it through the shared helper instead of duplicating the pattern.
- Harden migrateAllProtectedFsData's non-strict ('set') mode to also catch write failures (reprotectWholeFile/reprotectSnapshotFile) and readDir failures (listDirEntries) — previously only read/decrypt failures were caught, so a routine per-file I/O error during first-time setup could still crash the whole migration after setupIdbEncryption() had already activated the sentinel.
- Add a rollback in useSettingsView.ts's 'set' branch: if the fs migration still fails after the hardening above (e.g. the migration marker itself can't be written, before any file is touched), the just-created sentinel is undone via clearIdbPassphrase() instead of being left active with the feature flag off.
- Correct stale desktop-encryption claims in docs/IDB-ENCRYPTION.md (still described Tauri as sharing the IDB migration path wholesale) and locales/en/help.json (still claimed passphrase rotation/disable were unavailable, predating the B-1 passphrase UX).
- Add a snapshot test for reading a protected snapshot after encryption has been disabled entirely (sourcery-ai suggestion), and regression tests for the write-ordering fix and the set-mode rollback.
Opened #360 (fs reads/writes don't participate in the migration admission lock) and #361 (fs ciphertext isn't bound to its record identity) to track the two remaining architectural gaps external review surfaced that need their own design work rather than a inline fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c93a92f0a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| targetKey: CryptoKey | null, | ||
| operation: FsMigrationMarker['operation'], | ||
| ): Promise<void> { | ||
| const opts: MigrationOptions = { targetKey, strict: operation !== 'set' }; |
There was a problem hiding this comment.
On Tauri first-time enable, operation === 'set' makes the migration non-strict, so any per-file read or write failure is merely logged and skipped; the marker is then cleared and the hook enables the flag and reports success. An existing snapshot, image, project, or API-key file that is not subsequently saved can therefore remain plaintext indefinitely while the UI says encryption is active. Treat an incomplete setup migration as a recoverable failure rather than successful encryption.
AGENTS.md reference: AGENTS.md:L371-L371
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| // QNBS-v3: migrateAllProtectedFsData('set') is non-strict and already skips per-file failures — reaching here means a pre-flight failure (e.g. the migration marker itself couldn't be written) before any file was touched, so undoing the sentinel just-created above is safe, not just cosmetic. | ||
| await clearIdbPassphrase(); | ||
| setEncryptionReady(false); |
There was a problem hiding this comment.
Fresh evidence after the earlier setup-rollback comment is that listDirEntries() still awaits apis.exists(dir) outside its non-strict catch. If setup has already encrypted files in config and that check then rejects for snapshots, images, or a later directory, this catch calls clearIdbPassphrase(), deleting the sentinel and KDF salt while the already-migrated filesystem ciphertext remains under the new key. Only roll back after proving no filesystem write occurred, or retain the key and resume/undo the partial migration.
Useful? React with 👍 / 👎.
| useEffect(() => { | ||
| if (!isTauriRuntime()) return; | ||
| void (async () => { | ||
| const marker = await checkForInterruptedFsMigration(); |
There was a problem hiding this comment.
After a Tauri rotation is interrupted midway, some files may require the new key while the sentinel still authenticates the old key, but this marker is checked only in an App effect after bootApp() has already called loadPersistedRootState(). Once the user unlocks with the old passphrase, AES-GCM failures for already-rekeyed settings or project files are converted to null by their store readers, so the app can hydrate defaults and permit normal edits/autosaves before merely showing this notification. Check the marker before persisted-state hydration and block normal startup until the mixed-key state is recovered or explicitly handled.
Useful? React with 👍 / 👎.
| ); | ||
|
|
||
| const projectIds = await fileSystemService.listProjects(); | ||
| await Promise.all( |
There was a problem hiding this comment.
During a strict disable or rotation, each Promise.all rejects as soon as one file fails, but its other per-file migrations are not cancelled and can continue writing after migrateAllProtectedFsData() has rejected and the settings flow has reported failure. For example, one corrupt project can reject quickly while a slower sibling subsequently finishes re-encrypting a healthy project under the uncommitted target key; the active session remains on the old key, making that healthy project unreadable and causing retries to fail. Ensure all started work is drained without further mutation after the first failure, or migrate through a recoverable transaction before returning rejection.
Useful? React with 👍 / 👎.
…g, project-enumeration bypass, malformed-envelope classification - Reject a protected-v1 envelope whose data field is missing or the wrong type instead of returning the envelope shell as plaintext — a truncated/corrupted write that still parses as JSON was previously deserialized by the caller as if it were real domain data, silently corrupting in-memory state instead of surfacing the corruption. - Replace migrateAllProtectedFsData's use of the best-effort listProjects() (which swallows every readDir failure to []) with the same failure-propagating listDirEntries() helper every other directory scan in the bridge already uses — a transient permission/I/O error enumerating projects/ could otherwise skip every project/Codex/vector file while the migration still reported success. - Stop clearing the fs migration marker inside migrateAllProtectedFsData itself. For disable/rotate, an IDB-side commit (clearIdbPassphrase()/rotateIdbPassphrase()) still runs after the bridge succeeds — clearing the marker before that commit erased the only "mid-flight" signal a crash in that remaining window would leave behind. New clearFsMigrationMarker() is now called by useSettingsView.ts only once the whole operation, including that later IDB commit, has succeeded. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbd38209cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (error instanceof IdbStorageLockedError) throw error; | ||
| logger.error('Failed to load settings:', error); | ||
| return null; |
There was a problem hiding this comment.
When a structurally valid protected settings envelope has corrupted base64/ciphertext or a bad GCM tag, readProtectedTextFile() rejects with a generic error, but this catch converts it to null; startup then hydrates default settings and a later settings autosave can overwrite the only damaged file instead of surfacing a recovery error. Fresh evidence beyond the earlier malformed-envelope fix is that it validates only the envelope shape, so bit-corrupted but structurally valid ciphertext still follows this path; the analogous project, snapshot, Codex, vector, and image readers have the same locked-error-only exception.
Useful? React with 👍 / 👎.
| const apis = await loadTauriApis(); | ||
| const appDataPath = await apis.appDataDir(); | ||
| const markerPath = await apis.join(appDataPath, 'config', MIGRATION_MARKER_FILENAME); | ||
| await apis.remove(markerPath).catch(() => {}); |
There was a problem hiding this comment.
If Tauri cannot remove the marker after a successful setup, disable, or rotation—for example because of a transient I/O or permission error—this unconditional catch reports success to the caller while leaving the marker on disk. Every subsequent launch then warns that the library may be in a mixed-key state even though the migration committed successfully; ignore only a confirmed missing marker and propagate other removal failures so the operation can be retried.
Useful? React with 👍 / 👎.
| - Gated behind `featureFlags.enableIdbAtRestEncryption`. When a library is configured but locked, protected reads and writes fail closed rather than falling back to plaintext. | ||
| - Disable and passphrase rotation are temporarily unavailable until a journaled, cross-store migration protocol can prove recovery after interruption. | ||
| - **Web/PWA build only.** The unlock screen (`IdbUnlockModal`) and session-scoped in-memory key protect the IndexedDB-backed storage path used by the browser/PWA build. On the **Tauri desktop build**, primary project, settings, snapshot, image, Codex, RAG, and binder-asset data are written by the filesystem-backed store (`services/fs/*`), which is plaintext (LZ-string compressed, not encrypted) regardless of this setting — enabling it on desktop still shows the same unlock screen (the passphrase sentinel lives in the WebView's IndexedDB) but does not encrypt the actual manuscript files on disk. No `tauri-plugin-stronghold` or equivalent OS-keychain integration ships today — see the API-key encryption note below for the desktop-specific mechanism that does exist. | ||
| - **Tauri desktop build.** The unlock screen (`IdbUnlockModal`) and session-scoped in-memory key are shared with the browser/PWA build, and now genuinely protect the filesystem-backed store (`services/fs/*`) too — project, settings, snapshot, Codex, RAG, and image data reuse the same passphrase-derived key. Binder-asset files (`.bin` binary blob and `.meta.json` metadata sidecar) are the one exception and remain plaintext — see the encryption-mechanism table below. No `tauri-plugin-stronghold` or equivalent OS-keychain integration ships today — see the API-key encryption note below for the desktop-specific mechanism that does exist. |
There was a problem hiding this comment.
Fresh evidence after the earlier documentation fix is that the immediately preceding README bullet still says disable and passphrase rotation are unavailable, while this new desktop paragraph and the implemented Settings flow say they are supported; locales/en/help.json's help.settingsGuide.flags.content also still says desktop filesystem storage is not covered. Users consulting these sections receive contradictory security and recovery instructions, so update all remaining canonical/help entries together.
Useful? React with 👍 / 👎.
| // QNBS-v3: both resetAllDatabases() (storage-init-failure recovery) and wipeAllAppData() (factory | ||
| // reset) previously deleted only IDB + localStorage (including the KDF salt) without touching this |
There was a problem hiding this comment.
Fresh evidence after the earlier comment-format fix is that this newly added QNBS-v3 comment still continues across four physical // lines. Condense the tagged reason to one physical line as required by the repository's recurring hard rule.
AGENTS.md reference: AGENTS.md:L248-L248
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54ec14daad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| return; // legacy raw-project-data snapshot format predates the envelope — never protected | ||
| } | ||
| if (typeof envelope.data !== 'string') return; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier non-strict setup-failure comment is that valid pre-envelope snapshots are unconditionally skipped here: getSnapshotData() still explicitly supports these files as raw project objects, which normally have no string data field and therefore return at this check without being rewritten. Manual snapshots are never resaved or auto-pruned, so desktop can report encryption active while complete historical manuscripts remain plaintext indefinitely and bypass unprotectTextValue()'s locked-session gate; migrate the legacy object into an encrypted envelope or protect the whole file.
AGENTS.md reference: AGENTS.md:L371-L371
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cf8c5e219
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const obj = parsed as Record<string, unknown>; | ||
| if (obj['scheme'] !== PROTECTED_TEXT_SCHEME) return null; // plain JSON content, not a protected envelope | ||
| if (typeof obj['data'] !== 'string') { | ||
| throw new Error('Malformed protected-v1 envelope: missing or invalid "data" field'); |
There was a problem hiding this comment.
Fresh evidence after the prior malformed-envelope fix is that this branch throws a generic Error, while filesystem store readers only propagate errors recognized by isStorageAccessError. A truncated but valid JSON envelope such as {"scheme":"protected-v1"} is therefore still swallowed by loadSettings, loadProject, and the snapshot/Codex/image readers as missing data, allowing default hydration and a later autosave to overwrite the damaged file. Throw SecureRecordCorruptError here so malformed envelopes follow the same fail-closed path as an invalid GCM tag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b1c7fd21a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isTauriRuntime()) { | ||
| await deleteAllFsData().catch((error) => { |
There was a problem hiding this comment.
When factory reset runs in the mounted Tauri app, a previously scheduled project/settings autosave can wake after deleteAllFsData() but before reload—especially during the explicit 300 ms delay—because this reset never acquires withMigrationAdmission. The save can recreate projects/ or config/, causing plaintext data to survive the reset or encrypted ciphertext to be stranded after the KDF salt is cleared. Hold exclusive admission across filesystem deletion, metadata clearing, and reload.
AGENTS.md reference: AGENTS.md:L385-L385
Useful? React with 👍 / 👎.
| export async function readProtectedTextFile(apis: TauriApis, path: string): Promise<string> { | ||
| return unprotectTextValue(await retryFs(() => apis.readTextFile(path))); |
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 unprotectTextValue() acquires shared admission. The migration can therefore acquire exclusive admission in between, rewrite the file, and commit the new or absent key; the resumed read then decrypts stale ciphertext with the wrong key. In the disable case, snapshot reads swallow the resulting generic error as null, and restoreSnapshotThunk.fulfilled assigns that null payload to project state. Keep the filesystem read and decryption under one shared admission.
Useful? React with 👍 / 👎.
1 participant
User description
Stacked on #354 (PR G) — needs
writeTextFileAtomic. Sibling of #355 (PR H1, API keys) — both branch from #354 independently. Base will auto-retarget tomainonce #354 merges.Summary
Enabling Settings → Privacy → "Encrypt project data at rest" only ever protected the browser/PWA build's IndexedDB path. On the Tauri desktop build,
services/fs/*Store.tswroteproject.json,settings.json, snapshots, Codex, RAG vectors, and images as plaintext regardless of the setting — the same passphrase unlock screen appeared on desktop, but it gated nothing on the filesystem side.Fix
Desktop now reuses
services/storage/storageEncryptionService.ts's real passphrase-derived key directly, mirroring #355's API-key fix: AES-256-GCM protection when a passphrase is configured and unlocked, honest plaintext otherwise. No new module, no changes to that already-audited service.Migration strategy (per discussion before starting this PR): lazy/opportunistic. 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 "encrypt everything now" step, no data-loss risk, no new failure mode beyond what saves already have. Full journal-backed migration (mirroring the IDB path's resumable migration) was considered and explicitly deferred as much larger scope.
New shared primitives in
fsCore.ts:protectTextValue/unprotectTextValue— value-level protection. Used directly bysnapshotFsStore.tsso only the snapshot'sdatafield is protected, keepingname/date/wordCountmetadata plaintext — listing snapshots never needs to decrypt just to render names and dates, even while the session is locked.writeProtectedTextFileAtomic/readProtectedTextFile— whole-file wrappers around the above + the existing atomic-write primitive, for stores with no metadata/content split (project.json,settings.json,codex.snap,vectors.snap, images).Wired into:
projectFsStore(project.json),settingsFsStore(settings.jsononly — API keys are #355's concern),codexFsStore(codex + RAG vectors),snapshotFsStore(datafield only),assetFsStore(images).Not covered by this PR
Binder-asset binary blobs (
assetFsStore.ts's.binfiles) — they need a byte-native encrypt path (StorageEncryptionService'sencryptBytes/decryptBytesoperating onUint8Arraydirectly) rather than the JSON-serializingidbEncryptWithKey/idbDecryptWithKeyused here, which would be wasteful for potentially-large binary blobs. Tracked as a separate follow-up rather than forcing it into this PR.Tests
fsCore.test.ts: dedicated coverage forprotectTextValue/unprotectTextValue/writeProtectedTextFileAtomic/readProtectedTextFile— plaintext passthrough when unconfigured, real encrypt/decrypt round-trip when configured+unlocked, LZ-compressed plaintext correctly not misidentified as a protected envelope, throws when configured-then-disabled, fails closed (propagates locked error) when configured-but-locked.fsStores.test.ts: per-store integration tests asserting the on-disk content is actually encrypted (scheme: 'protected-v1', doesn't contain the plaintext) and still round-trips through the real store methods; the snapshot test specifically proveslistSnapshots()works while the session is locked (metadata untouched) and content decryption works once unlocked again.Test plan
pnpm exec vitest run tests/unit/services/fs/fsCore.test.ts tests/unit/services/fs/fsStores.test.ts— 63/63 passingnpx tsgo --project tsconfig.tsgo.json --noEmit --checkers 4— 0 errorspnpm run lint— cleanKnown merge-order note
This PR and #355 (sibling, same base) independently add the same
vi.mock('.../storageEncryptionService', ...)scaffolding totests/unit/services/fs/fsStores.test.ts(flagged in both PRs' comments). Whichever merges second will hit a trivial conflict there — same mock block twice, keep one copy.Follow-up not in this PR
docs/IDB-ENCRYPTION.md/docs/SECURITY-THREAT-MODEL.md/README.md's encryption tables (corrected to say "not yet fixed" in #352) will need a follow-up pass once #352/#354/#355/this PR all land, to reflect the real (partial — text stores only) fix.🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by Sourcery
Honor the desktop at-rest encryption setting for filesystem-backed project data by introducing shared text-protection helpers and wiring all relevant desktop stores to use them, with lazy migration and comprehensive tests.
New Features:
Enhancements:
Tests:
CodeAnt-AI Description
Honor desktop project-data encryption and protect saved content at rest
What Changed
Impact
✅ Encrypted desktop project data✅ Protected snapshots while keeping lists usable when locked✅ Safe readback for existing plaintext files💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.