[codex] harden client id validation#25
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR introduces X-Client-Id header validation in the risk-gate application and expands test coverage from 90 specs across 10 files to 112 specs across 13 files. Documentation is updated across multiple files to reflect the new test count and mark Gensyn AXL as integrated. ChangesX-Client-Id Validation & Documentation Updates
Sequence Diagram(s)sequenceDiagram
participant Client
participant Fastify Handler
participant clientIdOf
participant policyService
participant Error Handler
Client->>Fastify Handler: PUT /policies/:id with X-Client-Id
Fastify Handler->>clientIdOf: Validate X-Client-Id header
alt Valid header present
clientIdOf-->>Fastify Handler: Return trimmed valid ID
Fastify Handler->>policyService: Update with validated cid
policyService-->>Fastify Handler: Success
Fastify Handler-->>Client: 200 OK
else Header missing
clientIdOf-->>Fastify Handler: Return undefined
Fastify Handler->>policyService: Update with undefined
policyService-->>Fastify Handler: Success
Fastify Handler-->>Client: 200 OK
else Invalid header (blank, oversized, malformed, etc.)
clientIdOf->>clientIdOf: Throw InvalidClientIdError
clientIdOf-->>Error Handler: InvalidClientIdError
Error Handler-->>Client: 400 InvalidClientId
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 44 minutes and 20 seconds.Comment |
Deploying chainshield with
|
| Latest commit: |
b995dd1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0c9596f8.chainshield.pages.dev |
| Branch Preview URL: | https://feature-review-fix-hardening.chainshield.pages.dev |
|
@codex please deeply review this PR for correctness, security/isolation regressions, API semantics, test coverage, docs accuracy, and merge risk; prioritize blocker/P1 findings with file/line references. |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/risk-gate/app.ts (1)
146-156: ⚡ Quick win
catchinPUT /policies/:idstill swallows all non-NotFounderrors as404.The PR intentionally moved
clientIdOfoutside thetrysoInvalidClientIdErrorreachessetErrorHandler. However, thecatchstill absorbs any other error frompolicyService.update(e.g. a ZeroGStore upload failure) and misreports it as404 NotFound. Narrowing the catch to re-throw unexpected errors avoids this mask.Proposed fix
} catch (err) { + if (!(err instanceof Error && err.message.toLowerCase().includes("not found"))) { + throw err; + } reply.status(404); return { error: "NotFound", message: (err as Error).message }; }Alternatively, introduce a typed
PolicyNotFoundErrorinpolicyService.tsand narrow on that, which avoids the string-matching fragility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/risk-gate/app.ts` around lines 146 - 156, The PUT /policies/:id handler currently catches all errors and converts them to 404; change the catch to only handle the policy-not-found case and re-throw any other errors so they propagate to setErrorHandler. Concretely, in the app.put handler (the function using clientIdOf and calling policyService.update) replace the broad catch with logic that detects the specific not-found error (preferably by checking a typed PolicyNotFoundError from policyService.update or, if not yet available, by matching a unique error.name or code) and return the 404 payload only for that case; for any other err re-throw it. If needed, add a PolicyNotFoundError type in policyService and throw it from update to make the check robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/risk-gate/app.ts`:
- Around line 146-156: The PUT /policies/:id handler currently catches all
errors and converts them to 404; change the catch to only handle the
policy-not-found case and re-throw any other errors so they propagate to
setErrorHandler. Concretely, in the app.put handler (the function using
clientIdOf and calling policyService.update) replace the broad catch with logic
that detects the specific not-found error (preferably by checking a typed
PolicyNotFoundError from policyService.update or, if not yet available, by
matching a unique error.name or code) and return the 404 payload only for that
case; for any other err re-throw it. If needed, add a PolicyNotFoundError type
in policyService and throw it from update to make the check robust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbc38d58-b422-41b3-b282-17fde0db617e
📒 Files selected for processing (9)
AGENTS.mdCLAUDE.mdREADME.mddocs/architecture.mddocs/submission.mdsrc/README.mdsrc/risk-gate/app.tstests/README.mdtests/clientIsolation.test.ts
|
Addressed the CodeRabbit catch-path feedback: |
|
@codex review the changes |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
What changed
X-Client-Idhandling so present-but-invalid headers return400 InvalidClientIdinstead of silently falling back to unscoped admin/global access.PUT /policies/:idclient-id validation outside the policy updatecatchso validation errors do not get misreported as404 NotFound.Why
The previous boundary behavior treated invalid
X-Client-Idvalues as absent. Since absence intentionally means global/admin view, malformed browser traffic could bypass per-browser isolation semantics.Validation
bun test-> 112 pass, 0 fail, 337 assertionsbun run test:coverage-> 112 pass, 0 failbun run typecheck-> server + Astro cleanbun run build-> server bundle + Astro static build clean/healthOK and malformedX-Client-Idreturns400 InvalidClientIdgit diff --checkcleanReview note
Do not merge until explicitly approved. This PR is intended for review first.
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
Release Notes
New Features
Documentation