Repository navigation
feat(security): encryption.ts failclosed on crypto failure (audit F3) - #549
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 · |
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Note
|
Implements Option C+A (startup canary + runtime throw) per plans/encryption-failclosed-spec.md. The encrypt() fallback to plaintext (line 144-148) is the highest-risk governance-debt finding from the recent audit. When STORAGE_ENCRYPTION_KEY is set but crypto fails (bad key material, OOM, native binding broken), tokens silently store as plaintext. This is undetectable from operator view. Changes: 1. New EncryptionRuntimeError class — thrown when crypto pipeline fails after key was successfully derived 2. validateEncryptionAtStartup() — runs a known-plaintext round-trip to detect broken encryption config before serving traffic 3. src/instrumentation-node.ts — calls the startup canary before ensureSecrets() and any DB init; refuses to start if encryption is broken (process.exit(1)) 4. encryptConnectionFields() return type: T -> T | null to signal caller when encryption failed; providers.ts:348,544 callers updated to throw a clear error and skip the DB write 5. commandCodeAuth.ts:142 wraps encrypt() in try/catch; returns null on EncryptionRuntimeError so the session is not poisoned with plaintext apiKey 6. src/lib/db/encryptionStartup.ts (new) — async wrapper runEncryptionStartupCanary() + process-exiting runEncryptionStartupCheck() helpers 7. .env.example documents the new behaviour (EncryptionRuntimeError and StartupEncryptionError error names, remediation hint) State A preserved exactly: when STORAGE_ENCRYPTION_KEY is unset, passthrough returns plaintext (no breaking change for dev/test setups). Out of scope (per spec): - Migration of legacy ciphertext - Key rotation - Hardware-backed keys Refs: PR #507 F3 follow-up. Closes State B from audit. Verification: - node:test encryption suite: 23/23 pass (db-encryption, encryption-error-handling, encryption-strict, encryption-connection-fields-failclosed, db-command-code-auth) - vitest failclosed+startup suite: 14/14 pass - providers-batch-update: 12/12 pass (no regression) - TSC: 1 unrelated pre-existing error in apiKeys.ts (no new errors) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
L17 Latency Budget ReportChecked against: budgets/rest-endpoints.yaml. |
ac158c6 to
fb93243
Compare
codeant-ai
Bot
added
the
size:XL
L17 Latency Regression ReportThreshold: 10% p99 regression. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| let encryptedApiKey: string | null; | ||
| try { | ||
| encryptedApiKey = encrypt(input.apiKey) ?? null; | ||
| } catch (err: unknown) { | ||
| if (err instanceof EncryptionRuntimeError) { | ||
| // FAIL-CLOSED: encryption layer is broken (key configured but crypto | ||
| // pipeline threw). Refuse to write apiKey plaintext; surface the | ||
| // failure to the caller as "session not found". | ||
| log.error( | ||
| { err: err.message, op: "markCommandCodeAuthSessionReceived", stateHash: input.stateHash }, | ||
| `[commandCodeAuth] Refusing to store apiKey — encryption layer failed.` | ||
| ); | ||
| return null; |
There was a problem hiding this comment.
Suggestion: The encryption failure is collapsed into null, but the callback route treats every null result as an invalid or expired state and returns HTTP 400. A real server-side crypto outage is therefore indistinguishable from bad client input, causing clients to retry authentication and preventing monitoring or callers from recognizing that the server cannot persist the API key. Preserve the fail-closed write behavior, but propagate a distinct error/result so the route can return an appropriate server-error response. [api mismatch]
Severity Level: Major ⚠️
- ❌ Command Code callback reports crypto outages as HTTP 400.
- ⚠️ Valid authentication attempts appear to have expired state.
- ⚠️ Clients may retry instead of surfacing server failure.
- ⚠️ Monitoring cannot distinguish bad state from encryption failure.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/db/commandCodeAuth.ts
**Line:** 145:157
**Comment:**
*Api Mismatch: The encryption failure is collapsed into `null`, but the callback route treats every `null` result as an invalid or expired state and returns HTTP 400. A real server-side crypto outage is therefore indistinguishable from bad client input, causing clients to retry authentication and preventing monitoring or callers from recognizing that the server cannot persist the API key. Preserve the fail-closed write behavior, but propagate a distinct error/result so the route can return an appropriate server-error response.
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![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: ac158c6e76
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (conn.apiKey) conn.apiKey = encrypt(conn.apiKey) ?? conn.apiKey; | ||
| if (conn.accessToken) conn.accessToken = encrypt(conn.accessToken) ?? conn.accessToken; | ||
| if (conn.refreshToken) conn.refreshToken = encrypt(conn.refreshToken) ?? conn.refreshToken; | ||
| if (conn.idToken) conn.idToken = encrypt(conn.idToken) ?? conn.idToken; |
There was a problem hiding this comment.
When STORAGE_ENCRYPTION_KEY is present but getStaticKey() returns null from its derivation-failure path, encrypt() still returns the original secret instead of throwing, and these assignments treat that plaintext as a successful encryption result. In any entry point that does not run the new startup canary before provider writes, createProviderConnection/updateProviderConnection will pass the new null check and store API keys or OAuth tokens in plaintext even though encryption was configured; distinguish “no key configured” from “configured key failed” or verify encrypted fields get the enc:v1: prefix before returning success.
AGENTS.md reference: src/lib/db/AGENTS.md:L45-L50
Useful? React with 👍 / 👎.
|
❌ The last analysis has failed. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| `[Encryption] STORAGE_ENCRYPTION_KEY is set but encrypt() failed. ` + | ||
| `Refusing to write plaintext. Regenerate with: openssl rand -base64 32` | ||
| ); | ||
| throw new EncryptionRuntimeError( |
There was a problem hiding this comment.
[WARNING]: encrypt() now throws EncryptionRuntimeError, but 9+ external callers are not updated to handle it
encrypt() was changed to throw instead of returning plaintext on crypto failure. These callers still rely on the old fallback behavior and will throw unhandled exceptions at runtime:
src/lib/services/apiKey.ts:32—encrypt(key) ?? keysrc/lib/webhookDispatcher.ts:25—encrypt(JSON.stringify(meta)) ?? JSON.stringify(meta)src/lib/db/obsidian.ts:31—encrypt(token) ?? token(outer try/catch swallows error, token not persisted)src/lib/db/obsidian.ts:190—encrypt(password) ?? passwordsrc/lib/cloudAgent/credentials.ts:71—const encrypted = encrypt(apiKey);src/app/api/settings/proxy/cloudflare-deploy/route.ts:155—const encryptedRelayAuth = encrypt(relayAuth);src/app/api/settings/proxy/vercel-deploy/route.ts:236— same patternsrc/app/api/settings/proxy/deno-deploy/route.ts:347— same patternsrc/app/api/services/9router/rotate-key/route.ts:12—encrypt(newKey) ?? newKey
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (conn.idToken) conn.idToken = encrypt(conn.idToken); | ||
| return conn; | ||
| try { | ||
| if (conn.apiKey) conn.apiKey = encrypt(conn.apiKey) ?? conn.apiKey; |
There was a problem hiding this comment.
[SUGGESTION]: Dead ?? conn.apiKey fallback — encrypt() now throws on failure
Since encrypt() throws EncryptionRuntimeError instead of returning plaintext on failure, the ?? conn.apiKey fallback is unreachable and contradicts the fail-closed intent. Lines 489-491 have the same pattern.
| if (conn.apiKey) conn.apiKey = encrypt(conn.apiKey) ?? conn.apiKey; | |
| if (conn.apiKey) conn.apiKey = encrypt(conn.apiKey); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| let encrypted: string; | ||
| try { | ||
| encrypted = encrypt(STARTUP_CANARY_PLAINTEXT) ?? ""; |
There was a problem hiding this comment.
[SUGGESTION]: Dead ?? "" fallback — encrypt() now throws on failure
Since encrypt() throws EncryptionRuntimeError instead of returning plaintext, the ?? "" fallback is unreachable.
| encrypted = encrypt(STARTUP_CANARY_PLAINTEXT) ?? ""; | |
| encrypted = encrypt(STARTUP_CANARY_PLAINTEXT); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const encryptedApiKey = encrypt(input.apiKey); | ||
| let encryptedApiKey: string | null; | ||
| try { | ||
| encryptedApiKey = encrypt(input.apiKey) ?? null; |
There was a problem hiding this comment.
[SUGGESTION]: Dead ?? null fallback — encrypt() now throws on failure
Since encrypt() throws EncryptionRuntimeError instead of returning plaintext, the ?? null fallback is unreachable.
| encryptedApiKey = encrypt(input.apiKey) ?? null; | |
| encryptedApiKey = encrypt(input.apiKey); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * Strategy: we test the contract via the exported error class shape (no | ||
| * fault injection needed) and via the real encrypt() with the real crypto | ||
| * module (no fault injection needed for State A / happy-path). We cover the | ||
| * catch-block behaviour of `encryptConnectionFields` and the canary through |
There was a problem hiding this comment.
[SUGGESTION]: Test comment claims catch-block coverage that doesn't exist in the PR diff
The comment says "We cover the catch-block behaviour of encryptConnectionFields and the canary through a parallel test file that uses vi.spyOn on the real crypto module", but no such fault-injection test exists in the PR diff. Either add the missing test or update the comment to match actual coverage.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 173.1K · Output: 28K · Cached: 2.6M |
User description
Closes audit finding F3 from PR #507 (the highest-priority deferred item). When STORAGE_ENCRYPTION_KEY is set but the crypto pipeline fails, tokens silently store as plaintext — this is the State-B bug.
What this PR does
Implements Option C+A per plans/encryption-failclosed-spec.md:
Tests (all passing)
Test status
Breaking surface
The T | null return type of encryptConnectionFields is a TS-breaking change at compile time only — runtime behavior is unchanged for the happy path. External consumers pinned to the old signature will see a type error and must handle null. Container.ts re-export surface is also affected (TypeScript will surface this in the type checker).
Out of scope (per spec)
Refs: PR #507 F3 follow-up. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
CodeAnt-AI Description
Prevent plaintext credential storage when encryption fails
What Changed
STORAGE_ENCRYPTION_KEYcontinue to use the existing passthrough behavior.Impact
✅ No plaintext credentials written after encryption failures✅ Faster detection of broken encryption keys at startup✅ Clearer encryption failure logs and remediation guidance💡 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.