fix(csrf): merge CSRF token into effectiveInit for Request inputs (bot-fix for #1991)#1999
fix(csrf): merge CSRF token into effectiveInit for Request inputs (bot-fix for #1991)#1999hognek wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (2)
✨ 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. Comment |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
|
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
|
…t rebuild - Compute effective method from init?.method || input.method - Merge token into effectiveInit headers instead of rebuilding Request (avoids body stream consumption and respects init-provided headers/method) - Update test to inspect init.headers instead of Request.headers Addresses Kilo finding (Request inputs excluded from CSRF) and CodeRabbit finding (broken token injection when init overrides method/headers + body stream consumption). Refs: PR jaylfc#1991
d852c5c to
3a4505f
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const headers = new Headers(input.headers); | ||
| headers.set("X-CSRF-Token", token); | ||
| effectiveInput = new Request(input, { headers }); | ||
| const baseHeaders = init?.headers || input.headers; |
There was a problem hiding this comment.
WARNING: Headers set on the input Request object are silently dropped when init.headers is also provided.
baseHeaders is init?.headers || input.headers, so if the caller passes headers via init (e.g. fetch(new Request(url, { headers: { Authorization } }), { method: 'POST', headers: { ... } })), the Request object's own headers are discarded entirely. The previous new Request(input, { headers }) approach preserved input's headers. Consider merging both sources, e.g. new Headers(input.headers) then overlay init?.headers, before deciding whether to set the token.
| const baseHeaders = init?.headers || input.headers; | |
| const baseHeaders = new Headers(input.headers); | |
| if (init?.headers) new Headers(init.headers).forEach((v, k) => baseHeaders.set(k, v)); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Rebased onto origin/dev (a3ef7f7). 3 upstream commits auto-dropped (already in dev). Hogne's fix-for-Request-inputs commit re-applied cleanly. Desktop build + vitest pass. |
1 participant
Summary
Fixes the 2 inline bot findings on PR #1991 (Kilo + CodeRabbit) in
auth-guard.ts.Changes
auth-guard.ts — Request-object CSRF handling:
init?.method || input.method(was:input.methodonly)effectiveInitheaders instead of rebuilding the Request withnew Request(input, { headers })init.methodoverrides were ignored — a GET Request withinit: { method: 'POST' }would skip CSRFnew Request(input, ...)consumes the original body streamauth-guard.test.ts — Updated Request-object assertion:
spy.mock.calls[1](init arg) instead ofspy.mock.calls[0](Request arg) since the token lives in effectiveInitTests