fix(cors): set single-valued CORS headers instead of appending#1466
Conversation
Applying CORS headers more than once (e.g. middleware + handler) could duplicate single-valued response headers, producing invalid values like `*, *` for `access-control-allow-origin`, `access-control-allow-methods`, `access-control-max-age`, `access-control-allow-credentials`, etc. Use `.set` for CORS response headers and keep `.append` only for the legitimately multi-valued `vary` header. Co-Authored-By: Claude Fable 5 <[email protected]>
pi0x
left a comment
pi0x
left a comment
There was a problem hiding this comment.
Review: LGTM (posting as comment — GitHub blocks self-approval on own PR)
The change is correct and well-targeted.
Correctness
- All single-valued CORS response headers now use
.set:access-control-allow-origin,access-control-allow-methods,access-control-allow-credentials,access-control-allow-headers,access-control-expose-headers,access-control-max-age. Each is produced by thecreate*helpers as a single fully-computed value (comma-joined lists built in one shot), so.setis the right semantics — including forallow-headers/expose-headers, where a repeated application should replace rather than concatenate. varycorrectly stays on.append(legitimately multi-valued; distinct code paths contribute different vary tokens).- Both
res.headersandres.errHeadersare handled identically, matching prior behavior. - Single-application output (headers emitted and their values) is unchanged — verified by the existing unit tests passing.
API/types: setCorsHeaders is a non-exported internal helper; no public API or type surface change.
Tests: The two regression tests apply CORS twice and assert single values, directly covering the *, * bug.
Verification: pnpm vitest run test/unit/cors.test.ts → 46 passed; pnpm lint clean.
Minor, non-blocking: Applying preflight twice still duplicates vary tokens (e.g. origin, origin). Harmless/valid per HTTP and out of scope for this fix; noting in case dedup is later desired.
🤖 Generated with Claude Code
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCORS header helpers now centralize response updates, replacing single-valued headers instead of appending duplicates while continuing to append ChangesCORS header idempotency
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
2 participants
Applying CORS headers more than once (e.g. middleware plus handler) duplicated single-valued response headers, producing invalid values like
*, *foraccess-control-allow-origin,access-control-allow-methods,access-control-max-age, andaccess-control-allow-credentials.This switches
appendCorsHeaders/appendCorsPreflightHeadersto use.setfor CORS response headers, keeping.appendonly for the legitimately multi-valuedvaryheader. Added regression tests that apply CORS twice and assert single values.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Varyheader values correctly for preflight and standard requests.Tests