Repository navigation
fix(security): log 3 crypto-relevant silent catches - #508
KooshaPari wants to merge 2 commits into
Conversation
…extras test Continuation of the governance-debt cleanup started in PRs #505 and #506. Six independent fixes, each addressing a class of silent-failure pattern that the audits surfaced. Changes: 1. **`src/lib/resilience/anomalyHook.ts:12`** — Broken import path. `isFeatureFlagEnabled` was imported from `@/lib/featureFlags`, but the module lives at `@/shared/utils/featureFlags`. This bug was masked because `tsconfig.typecheck-core.json` does not include `src/lib/resilience/` and no test loads the module successfully. After the fix, the module loads and exports the expected surface. 2. **`src/server-init.ts:128`** — Same broken import path. The surrounding try/catch at lines 130-139 silently swallowed the import failure. Fix: correct the path AND upgrade the catch log from `warn` to `error` (matches PR #506 pattern). 3. **`src/lib/versionManager/processManager.ts:155`** — `getProcessInfo` catch returned `{pid, alive: true}` after `ps`/readFile failures, which lies when the process is actually gone. Fix: catch now logs the error and returns `{pid, alive: false}` (honest about not being able to read process state). 4. **`src/server/ws/liveServer.ts:479`** — `loadAuthModule().catch(() => {})` silently swallowed initial auth module load failures, allowing the WS server to come up without auth configured. Fix: catch now logs `log.error`; fallback behavior preserved per the existing comment ("handler retries the import lazily"). 5. **`src/lib/machineToken.ts:1-10`** — Crypto-relevant: empty catch around `require("node-machine-id")` silently fell back to `() => ""`, which collapses HMAC inputs to a constant. Fix: catch now logs `log.error` with security-context message, gated by a `fallbackLogged` flag so the log fires only once per process (avoiding log spam from any downstream reload). 6. **`tests/unit/quota/keyvQuotaStoreExtras.test.ts`** (new) — Closes spec §8.2 reachability test gap. 5 sanity tests for the `recordPlanUsage` / `upsertProviderPlan` / `listProviderPlans` / `setPools` / `getPool` surface on KeyvQuotaStore. Note: this branch is based on `origin/agent/migration-version-collision-fix` (pre-PR-#505), so the methods live directly on KeyvQuotaStore. When PR #505 lands, update the test to import from `keyvQuotaStoreExtras.ts` instead. Verification: - TSC error count: 8 unchanged (all pre-existing quota keystore errors that PR #505 fixes; this PR is independent of #505) - Vitest extras test: 5/5 pass - Node:test combined (processManager + machineToken + WS): 47/47 pass - All success-path behavior preserved; only catch/fallback paths now log Out of scope (separate PRs): - Promote Keyv to "embedded default" driver (spec §10) - Pre-existing e2e failure at `tests/e2e/quota-store.e2e.ts:117` (poolUsageWithDimensions shape mismatch — fixed in PR #505) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous audit (PR #507) only fixed the module-load catch in machineToken.ts; the runtime catches in getMachineTokenSync() and getLegacyCliTokenSync() still silently returned empty strings. The encryption.ts getLegacyDynamicKey() catch was also left unfixed. In all three cases, the empty-string return collapses HMAC/HMAC-SHA256 inputs into a constant-key value, so security-relevant operations on the fallback path produce identical tokens regardless of input. Fixes: 1. src/lib/machineToken.ts:44 - getMachineTokenSync catch: log.error + return "" (matches PR #507 module-load catch pattern) 2. src/lib/machineToken.ts:62 - getLegacyCliTokenSync catch: log.error + return "" (same pattern) 3. src/lib/db/encryption.ts:87-95 - getLegacyDynamicKey catch: added pino logger (createLogger("db:encryption")) + log.error instead of returning null silently Success-path behavior preserved in all three. Only the catch path is changed (now logs where it previously swallowed). Out of scope (separate work): - encryption.ts:144-148 (encrypt() catch falls back to plaintext when crypto fails - needs design discussion before changing) - src/lib/db/*.ts console.* migration to pino (large, separate PR) Verification: - TSC: 8 unchanged (pre-existing quota keystore errors, PR #505 territory) - Targeted machineToken + encryption unit tests pass - All 3 success-path behaviors preserved
🤖 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 · |
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
codeant-ai
Bot
added
the
size:L
|
Tick the box to add this pull request to the merge queue (same as
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| */ | ||
|
|
||
| import { isFeatureFlagEnabled } from "@/lib/featureFlags"; | ||
| import { isFeatureFlagEnabled } from "@/shared/utils/featureFlags"; |
There was a problem hiding this comment.
Suggestion: Importing isFeatureFlagEnabled routes every health sample through resolveFeatureFlag, which performs a synchronous database query for the override before checking the environment or default. Since this hook runs once per provider request, the new dependency adds blocking database I/O to every request's telemetry path even when self-healing is disabled. Cache the effective flag value or use a non-blocking/configuration-level flag check. [performance]
Severity Level: Major ⚠️
- ⚠️ Provider telemetry performs synchronous database work.
- ⚠️ Disabled self-healing still adds request-path overhead.
- ⚠️ High request volume can increase event-loop latency.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/resilience/anomalyHook.ts
**Line:** 12:12
**Comment:**
*Performance: Importing `isFeatureFlagEnabled` routes every health sample through `resolveFeatureFlag`, which performs a synchronous database query for the override before checking the environment or default. Since this hook runs once per provider request, the new dependency adds blocking database I/O to every request's telemetry path even when self-healing is disabled. Cache the effective flag value or use a non-blocking/configuration-level flag check.
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| await loadAuthModule().catch((err) => | ||
| log.error( | ||
| { err }, | ||
| "liveServer: failed to warm auth module — clients may experience cold-import latency" | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Suggestion: The rejected dynamic-import promise is memoized in authModulePromise, so when the startup warm-up fails, every later API-key connection reuses the same rejected promise. This contradicts the message that the handler will retry lazily and permanently makes API-key WebSocket authentication unavailable for the process. Clear authModulePromise when the import rejects, or avoid caching rejected promises. [stale reference]
Severity Level: Major ⚠️
- ❌ API-key WebSocket authentication remains unavailable.
- ⚠️ Live dashboard clients receive Auth system unavailable.
- ⚠️ Recovery requires restarting the WebSocket process.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/server/ws/liveServer.ts
**Line:** 482:487
**Comment:**
*Stale Reference: The rejected dynamic-import promise is memoized in `authModulePromise`, so when the startup warm-up fails, every later API-key connection reuses the same rejected promise. This contradicts the message that the handler will retry lazily and permanently makes API-key WebSocket authentication unavailable for the process. Clear `authModulePromise` when the import rejects, or avoid caching rejected promises.
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
|
Closing: this PR's branch was accidentally forked off #507's branch (fix/governance-cleanup-20260806), so the diff vs canonical base included all of #507's 6 governance changes plus the 3 new crypto catches — not surgical. Replaced with #509 from fix/crypto-catches-clean-20260806 (rebased onto canonical base), which has the surgical 2-file diff (src/lib/machineToken.ts + src/lib/db/encryption.ts). |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Reviewed by step-3.7-flash · Input: 81K · Output: 17.1K · Cached: 676.5K |
|
User description
The previous audit (PR #507) only fixed the module-load catch in machineToken.ts; the runtime catches in getMachineTokenSync() and getLegacyCliTokenSync() still silently returned empty strings. The encryption.ts getLegacyDynamicKey() catch was also left unfixed.
In all three cases, the empty-string return collapses HMAC/HMAC-SHA256 inputs into a constant-key value, so security-relevant operations on the fallback path produce identical tokens regardless of input.
Fixes
log.error+ return "" (matches PR fix(governance): broken featureFlags imports + 3 silent fail-opens + extras test #507 module-load catch pattern)log.error+ return "" (same pattern)createLogger("db:encryption")) +log.errorinstead of returning null silentlySuccess-path behavior preserved in all three. Only the catch path is changed (now logs where it previously swallowed).
Out of scope (separate work)
encrypt()catch falls back to plaintext when crypto fails - needs design discussion before changing)Verification
CodeAnt-AI Description
Fix silent startup, process, and cryptographic failures
What Changed
Impact
✅ Correct process status after monitoring failures✅ Visible diagnostics for crypto and authentication failures✅ Self-healing telemetry starts from the intended feature flag💡 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.