security(desktop): fix API-key encryption using a real secret - #355
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 GuideDesktop API-key storage is moved off an insecure derived-passphrase scheme and onto the existing real passphrase-based storageEncryptionService primitives, with plaintext used when no passphrase is configured, locked-session behavior aligned with IDB, obsolete formats discarded, and crypto helper/tests updated accordingly. Sequence diagram for desktop API-key protection using real passphrase-based encryptionsequenceDiagram
actor User
participant FsSettingsStore
participant StorageEncryptionService as storageEncryptionService
participant FileSystem as writeTextFileAtomic/readTextFile
User->>FsSettingsStore: saveApiKey(provider, apiKey)
FsSettingsStore->>StorageEncryptionService: resolveProtectedWriteKey()
alt passphrase configured and unlocked (key returned)
FsSettingsStore->>StorageEncryptionService: idbEncryptWithKey(key, apiKey)
FsSettingsStore->>FileSystem: writeTextFileAtomic(payload scheme=protected-v1, data=bytesToBase64(ciphertext))
else no passphrase configured (key is null)
FsSettingsStore->>FileSystem: writeTextFileAtomic(payload scheme=plaintext-v1, value=apiKey)
else IdbStorageLockedError
FsSettingsStore-->>User: saveApiKey rejects with IdbStorageLockedError
end
User->>FsSettingsStore: getApiKey(provider)
FsSettingsStore->>FileSystem: readTextFile(<provider>_key.enc.json)
FsSettingsStore->>FsSettingsStore: JSON.parse(content)
alt scheme=plaintext-v1
FsSettingsStore-->>User: return value
else scheme=protected-v1
FsSettingsStore->>FsSettingsStore: readProtectedApiKey(data)
FsSettingsStore->>StorageEncryptionService: resolveProtectedWriteKey()
alt key returned
FsSettingsStore->>StorageEncryptionService: idbDecryptWithKey(key, base64ToBytes(data))
FsSettingsStore-->>User: return apiKey
else key is null
FsSettingsStore-->>User: throw Error("at-rest encryption is no longer configured")
end
else obsolete/unsupported payload
FsSettingsStore->>FileSystem: remove(<provider>_key.enc.json)
FsSettingsStore-->>User: return null
end
opt IdbStorageLockedError during protected read
FsSettingsStore-->>User: return null (file retained)
end
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: PASSEDQuality Gate Details
|
![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 left some high level feedback:
- In
readProtectedApiKey/getApiKey, using a genericErrorfor the "passphrase no longer configured" case causes it to be treated like a legacy/invalid format and the file is deleted; consider a distinct error type or branch so this scenario can surface a clearer UX and avoid conflation with obsolete payloads. - The scheme identifiers (
plaintext-v1/protected-v1) are currently duplicated as string literals in both the implementation and tests; centralizing them in a shared constant would reduce the risk of drift if these formats change in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `readProtectedApiKey`/`getApiKey`, using a generic `Error` for the "passphrase no longer configured" case causes it to be treated like a legacy/invalid format and the file is deleted; consider a distinct error type or branch so this scenario can surface a clearer UX and avoid conflation with obsolete payloads.
- The scheme identifiers (`plaintext-v1` / `protected-v1`) are currently duplicated as string literals in both the implementation and tests; centralizing them in a shared constant would reduce the risk of drift if these formats change in the future.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR Summary by Qodosecurity(desktop): protect filesystem API keys with real passphrase key
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![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: be80d0ea23
ℹ️ 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".
| // passphrase — there is nothing left to decrypt with; treat the same as an unreadable file. | ||
| throw new Error('Protected API key exists but at-rest encryption is no longer configured'); | ||
| } | ||
| return idbDecryptWithKey<string>(key, base64ToBytes(base64Data)); |
There was a problem hiding this comment.
On Tauri, when an API key has been saved as protected-v1 and the user changes their at-rest-encryption passphrase, rotateIdbPassphrase() migrates only the registered IndexedDB adapters before replacing the active key. This filesystem payload remains encrypted under the old key, so the next call here decrypts with the new key, falls into the cleanup path, deletes the key file, and forces the user to re-enter every provider key. Include filesystem API-key files in the rotation or otherwise rewrite them before activating the new key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct finding, partially addressed so far. aa6ec88 fixes the immediate danger — a rotation no longer permanently deletes the still-valid key file (see the resolved thread above), it's preserved as an unreadable protected-v1 file the user can simply re-enter. Full auto-re-encryption during rotation (so the user never even sees a gap) requires a proper migration bridge that includes filesystem-backed stores in the disable/rotate flow — this is genuinely the same class of finding as the sibling #356 PR's disable-migration issue, and I'm building a shared bridge for both (API keys here + project data there) as a follow-up. Leaving this thread open until that lands rather than claiming it's fully resolved.
There was a problem hiding this comment.
Confirmed. Fixed via a new migration bridge (services/fs/fsEncryptionMigration.ts, commit 8106b7a on the downstream PR #356 branch fix/desktop-project-data-encryption, which is stacked on this PR) rather than in this PR's own diff, since it needs to cover both API keys (this PR's domain) and project/settings/snapshot/codex/RAG data (#356's domain) together with one shared bridge. useSettingsView.ts's handlePassphraseConfirm now calls migrateAllProtectedFsData(targetKey) — which re-keys every fs-backed protected file, including provider API keys via FsSettingsStore.reprotectApiKeyFile() — BEFORE rotateIdbPassphrase() swaps the active session key, so the old key is never lost before fs data is converted. Any file that fails to decrypt aborts the whole rotate operation rather than silently stranding it. Leaving this thread open (not resolving) since the actual fix lives on a different PR in the stack — will resolve once #356 lands and this is verified end-to-end.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
be80d0e to
bedd50d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bedd50d068
ℹ️ 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 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'); |
There was a problem hiding this comment.
When a desktop user saves an API key while at-rest encryption is active and then disables encryption in Settings → Privacy, clearIdbPassphrase() migrates only the registered IDB adapters before deleting the sentinel and active key. The next read of this protected-v1 file therefore reaches this no-key branch, and the generic catch removes the file, permanently discarding the provider credential. Include filesystem API-key files in the disable migration and rewrite them to the intended post-disable format before clearing the key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same underlying issue as the rotation-deletion bug above, for the disable direction specifically — correctly identified. Immediate danger already fixed in aa6ec88: clearIdbPassphrase() deleting the sentinel/key means resolveProtectedWriteKey() now returns null on the next read, readProtectedApiKey() throws its own distinct error, and that error is NOT IdbStorageLockedError so it now falls into the preserve-not-discard branch — the file is no longer deleted. Full auto-conversion to plaintext during disable (so the user never even sees an unreadable file) needs the same migration-bridge work referenced in the other open thread on this PR — building a shared bridge that also covers #356's project-data stores. Leaving this open until that lands.
There was a problem hiding this comment.
Same root cause and same fix as the sibling rotation finding above — services/fs/fsEncryptionMigration.ts's migrateAllProtectedFsData(null) now converts every fs-backed protected file (including API keys) to plaintext BEFORE clearIdbPassphrase() destroys the sentinel, fixed in commit 8106b7a on the downstream #356 branch. Leaving open for the same reason — the fix lives on a different PR in the stack.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa6ec88be5
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfeb22fb59
ℹ️ 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".
F-05/F-06 (2026-07-29, AUDIT.md/CHANGELOG.md) was credited as fixed for
upgrading desktop API-key encryption from unsalted SHA-256 to PBKDF2 +
random salt. That hardened the KDF against rainbow-table/multi-target
attacks but never addressed the actual finding: the PBKDF2 passphrase
input, `${appDataPath}|${provider}|WorldScriptStudio|v1`, is built
entirely from public/discoverable values (a standard OS app-data path,
a public provider enum, hardcoded literals). Anyone with read access to
the encrypted `<provider>_key.enc.json` file - the exact threat model
this exists to defend against - can reconstruct the identical string and
decrypt in one PBKDF2 call. No brute force needed, so the iteration
count defended against an attack that isn't the real one.
Retire the derived-passphrase scheme entirely (encryptText/decryptText/
deriveFileSystemCryptoKey removed from fsCore.ts) rather than patch it
again. API keys now reuse services/storage/storageEncryptionService.ts's
crypto primitives directly - the same real, user-chosen passphrase-
derived key that already protects IDB at-rest data, via its already-
exported resolveProtectedWriteKey/idbEncryptWithKey/idbDecryptWithKey.
No new module, no changes to that already-audited service, no migration-
journal integration - this is pure reuse of existing, tested machinery.
Behavior:
- Passphrase configured + unlocked: real AES-256-GCM protection under
the actual user secret.
- No passphrase configured: honest plaintext (removes the false-
confidence derivation instead of leaving a fake "encrypted" fallback -
matches the existing opt-in model where actual encryption only
activates once a passphrase is set).
- Passphrase configured but locked: fails closed on both save and read
(IdbStorageLockedError propagates) rather than silently downgrading to
plaintext, matching the existing IDB protected-write policy. A locked
read does NOT discard the file - the key is still there, just
temporarily unreadable.
- Either obsolete pre-2026-08-13 format (unsalted, or salted-but-public-
passphrase) is discarded and the user re-prompted on next read - same
"locked decision, no migration" precedent as the original F-05/F-06
fix.
bytesToBase64/base64ToBytes stay in fsCore.ts (now exported) as the
JSON-safe codec for the new protected envelope's ciphertext.
Stacked on fix/desktop-atomic-writes (writeTextFileAtomic) since the
API-key file write needed it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes two confirmed bugs found during the review correction loop, both
independently flagged by multiple reviewers:
- Passphrase rotation permanently deleted valid keys (Critical). Any
decrypt failure that wasn't IdbStorageLockedError fell into the
generic "obsolete format, discard + notify" path - including a
perfectly valid protected-v1 file that just doesn't decrypt under the
CURRENT key because the passphrase was rotated since it was saved.
getApiKey() now only discards payloads it can positively identify as
one of the two actually-obsolete pre-2026-08-13 formats; any other
decrypt failure on a recognized protected-v1 envelope (rotation,
transient sentinel-lookup error, corruption) preserves the file and
returns null instead. Restructured getApiKey() to separate file
read/parse from scheme interpretation from the (now much narrower)
discard path.
- No provider-identity binding on the ciphertext (P2 hardening). Every
provider's key used the same session CryptoKey with no binding to
which provider it belongs to, so a process able to modify the Tauri
app-data directory could swap two protected-v1 files and both would
still decrypt successfully under the wrong provider. Now encrypts
{provider, apiKey} together instead of the bare key string; a
provider mismatch on decrypt is rejected (and the file preserved, not
discarded) rather than silently handing one provider's key to another.
Also condensed the remaining QNBS-v3 multi-line comments introduced by
this PR onto one physical line.
Not changed (see PR discussion): the "API keys must be persisted only
via encrypted IndexedDB storage, never plaintext" compliance claim on
one thread doesn't correspond to any actual rule in this repo's
AGENTS.md/CLAUDE.md - checked directly, no match. This is the Tauri
filesystem-backed store, not IndexedDB; honest plaintext when no
passphrase is configured is the deliberate, discussed design.
Rebased onto the updated fix/desktop-atomic-writes to pick up its own
review-loop fixes (temp-file-leak, concurrent-write race, QNBS-v3).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…cheme docs/SECURITY-THREAT-MODEL.md still described the retired per-file PBKDF2-of-public-material scheme as the current mitigation and pointed to the now-deleted deriveFileSystemCryptoKey(). Updated the desktop API-key row and the local-file-read attack tree to describe the actual replacement (real passphrase-derived key shared with the IDB path, provider-bound envelope, honest plaintext fallback when unconfigured). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cfeb22f to
ac33b61
Compare
…fresh keys on unlock Review-loop follow-up on PR #355: - getApiKey() indexed the parsed JSON payload without checking it was actually an object first — a file containing valid-but-non-object JSON (e.g. literal `null`) threw an uncaught TypeError instead of being treated as unreadable. - The "obsolete format, discard" fallback deleted any payload that didn't match a currently-recognized scheme, without verifying it matched one of the two legacy shapes — an unexpected future format or a merely corrupted current-format file would have been permanently destroyed. Now only positively-identified legacy {iv, data} envelopes (no scheme field) are discarded. - AiProviderCard and OpenRouterSection read API keys in a mount-only effect; a key read while the session was still locked stayed shown as missing even after unlock, since nothing re-triggered the fetch. Both now re-fetch when encryptionReady (threaded from SettingsViewContext) changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64268cbae2
ℹ️ 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".
Review-loop follow-up on PR #355: two comments introduced by the null-payload guard and legacy-discard fixes were each wrapped across multiple // lines. Condensed both to one physical line each. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c16ea10d64
ℹ️ 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: 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. |
There was a problem hiding this comment.
Condense this explanation into a single physical // QNBS-v3: line. Fresh evidence after the earlier claimed fix is that this newly added final-tree comment still spans three // lines, contrary to the repository's explicit hard rule.
AGENTS.md reference: AGENTS.md:L248-L248
Useful? React with 👍 / 👎.
| cancelled = true; | ||
| }; | ||
| }, []); | ||
| }, [encryptionReady]); |
There was a problem hiding this comment.
When Settings remains mounted and the user selects Lock Session, useSettingsView changes encryptionReady to false, causing this effect and AiProviderCard's equivalent to read the locked keys as null; however, a successful global unlock invokes only setIdbUnlockOpen(false) in App.tsx:895 and never changes this hook's encryptionReady state back to true. Fresh evidence after the prior fix is therefore that this dependency never changes on that unlock path, leaving all saved provider keys displayed as missing until Settings remounts; publish the global unlock to the Settings state or subscribe to a shared readiness signal.
Useful? React with 👍 / 👎.
| .then((k) => setAnthropicKey(k ?? '')) | ||
| .catch(() => {}); | ||
| }, []); | ||
| }, [encryptionReady]); |
There was a problem hiding this comment.
When encryptionReady changes more than once while these asynchronous reads are pending—for example, a quick lock followed by unlock—the effect now starts overlapping key loads without cancellation or a sequence guard. If the older locked-session request resolves after the newer unlocked request, its null result clears the successfully reloaded OpenAI, Grok, or Anthropic key and leaves the UI showing it as missing; cancel the prior effect or apply results only from the latest readiness generation.
Useful? React with 👍 / 👎.
1 participant
User description
Stacked on #354 (PR G) — needs
writeTextFileAtomic. Base will auto-retarget tomainonce #354 merges.Summary
F-05/F-06 (2026-07-29,
AUDIT.md/CHANGELOG.md) was credited as fixed for upgrading desktop API-key encryption from unsalted SHA-256 to PBKDF2 + random salt. That hardened the KDF against rainbow-table/multi-target attacks but never addressed the actual finding: the PBKDF2 passphrase input,`${appDataPath}|${provider}|WorldScriptStudio|v1`, is built entirely from public/discoverable values (a standard OS app-data path, a public provider enum, hardcoded literals). Anyone with read access to the encrypted<provider>_key.enc.jsonfile — the exact threat model this exists to defend against — can reconstruct the identical string and decrypt in one PBKDF2 call. No brute force needed, so the iteration count defended against an attack that isn't the real one.Fix
Retires the derived-passphrase scheme entirely (
encryptText/decryptText/deriveFileSystemCryptoKeyremoved fromfsCore.ts) rather than patching it again. API keys now reuseservices/storage/storageEncryptionService.ts's crypto primitives directly — the same real, user-chosen passphrase-derived key that already protects IDB at-rest data — via its already-exportedresolveProtectedWriteKey/idbEncryptWithKey/idbDecryptWithKey. No new module, no changes to that already-audited service, no migration-journal integration — this is pure reuse of existing, tested machinery.Behavior:
IdbStorageLockedErrorpropagates) rather than silently downgrading to plaintext, matching the existing IDB protected-write policy. A locked read does not discard the file — the key is still there, just temporarily unreadable.bytesToBase64/base64ToBytesstay infsCore.ts(now exported) as the JSON-safe codec for the new protected envelope's ciphertext.Tests
fsStores.test.tsgets a controllable fake forstorageEncryptionService's IDB-backed sentinel/session state (onlyhasPassphraseSentinel/resolveProtectedWriteKeyare faked —idbEncryptWithKey/idbDecryptWithKey/IdbStorageLockedErrorare the real implementation viaimportOriginal, since they're pure Web Crypto with no IDB access, so this test file never needs to mock or initialize real IndexedDB):saveApiKeyrejects (fails closed) when configured-but-lockedgetApiKeyreturnsnullwithout discarding the file when configured-but-lockedfsCore.test.ts: removed the retiredencryptText/decryptTexttests, added round-trip coverage for the now-exportedbytesToBase64/base64ToBytes.Test plan
pnpm exec vitest run tests/unit/services/fs/fsCore.test.ts tests/unit/services/fs/fsStores.test.ts— 52/52 passingnpx tsgo --project tsconfig.tsgo.json --noEmit --checkers 4— 0 errorspnpm run lint— cleanFollow-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 docs(security): correct false desktop encryption claims #352) will need a follow-up pass once this and docs(security): correct false desktop encryption claims #352 both land, to reflect the real fix — they're on independent branches so I didn't touch them here to avoid merge conflicts.🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by Sourcery
Replace desktop API-key filesystem encryption with reuse of the existing user-passphrase-derived storageEncryptionService key, and retire the insecure derived-passphrase scheme.
Bug Fixes:
Enhancements:
Documentation:
Tests:
CodeAnt-AI Description
Protect desktop API keys with the user's actual encryption passphrase
What Changed
Impact
✅ Real confidentiality for API keys✅ No plaintext fallback while encryption is locked✅ Clear re-entry prompt for obsolete API keys💡 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.