fix: strip upstream CORS headers in proxy and tunnel - #454
Conversation
When the upstream application handles CORS itself, its headers were forwarded alongside the ones the proxy's own CORS middleware sets, so the response carried two of each. A duplicated Access-Control-Allow-Origin is a hard failure per the Fetch spec, and a duplicated Access-Control-Allow-Credentials no longer reads as exactly "true", so credentials are dropped. The browser rejected a response that both the upstream and the proxy had answered correctly, and the developer could not fix it from their own application. The proxy terminates CORS: the browser talks to the proxy, so the proxy's configuration is what must apply. The upstream computed its headers for a different client — the proxy — so they are not meaningful at this boundary and are now removed. Access-Control-Expose-Headers is deliberately left alone, because browsers merge duplicates of it into a single list rather than failing; stripping it would silently discard headers the upstream meant to expose. The response middleware is extracted into respMiddleware so it can be tested directly, mirroring reqMiddleware. Closes #344 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYzGVwAKQ4ormxRHZg1eDu
Stripping the upstream's Access-Control-Allow-Origin regressed cross-origin HEAD requests. HEAD is CORS-safelisted, so browsers send it without a preflight, but corsx.CORSDefaultAllowedMethods omits it — and for a method it does not allow, rs/cors returns from handleActualRequest before adding any header. The response therefore reached the browser with no CORS headers at all, where before this branch the upstream's copy made it work. HEAD is now allowed, and the CORS configuration is extracted into corsOptions so the strip list and the headers that replace it are derived from one place. The regression test builds its handler from that same helper instead of restating the options, which had already drifted from the real configuration. Also replaces filepath.Join with path.Join when matching the welcome page URL. filepath.Join joins with a backslash on Windows, so the redirect rewrite never fired there and --default-redirect-url was silently ignored. And folds the nested append chains into slices.Concat, which removes the risk of writing into a package-level slice's backing array. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYzGVwAKQ4ormxRHZg1eDu
|
Warning Review limit reached
Next review available in: 59 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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
Closes #344
Problem
When the upstream application handles CORS itself, its headers were forwarded alongside the ones the proxy's own CORS middleware sets, so the response carried two of each:
A duplicated
Access-Control-Allow-Originis a hard failure per the Fetch spec, and a duplicatedAccess-Control-Allow-Credentialsno longer reads as exactlytrue, which fails a credentialed request outright. The browser rejects a response that both the upstream and the proxy answered correctly, and the developer cannot fix it from their own application — which is why the reporter had to fork and patch the CLI.Fix
The proxy terminates CORS: the browser talks to the proxy, so the proxy's configuration is what must apply. The upstream computed its headers for a different client — the proxy — so they are not meaningful at this boundary and are now stripped.
Access-Control-Expose-Headersis deliberately not stripped. Browsers merge duplicates of it into a single list rather than failing, so removing it would silently discard headers the upstream meant to expose. (The patch in the issue would have dropped it; the rest of that patch also hardcodeslocalhost:3000/4000origins and forces the origin check to always pass, which would defeat--allowed-cors-origins.)Review caught a regression this introduced: cross-origin
HEADrequests lost CORS entirely. HEAD is CORS-safelisted so browsers send it without a preflight, butcorsx.CORSDefaultAllowedMethodsomits it, and for a method it does not allowrs/corsreturns fromhandleActualRequestbefore adding any header. With the upstream's copy stripped, such a response reached the browser with no CORS headers at all — worse than the bug being fixed. HEAD is now allowed.Two unrelated defects fixed in passing, both in the function this PR extracts:
filepath.Joinwas used to match the welcome-page URL. On Windows it joins with a backslash, so the comparison never matched and--default-redirect-urlwas silently ignored. Nowpath.Join.append(a, append(b, c...)...)chains could write into a package-level slice's backing array. Nowslices.Concat.Worth your judgement
When
--allowed-cors-originsis set and the request origin is not on the list, the proxy adds no CORS headers of its own, so stripping the upstream's leaves the response with none — where previously the upstream's header made the request succeed.I think that is correct: if you explicitly restrict origins, letting the upstream silently override that restriction defeats the flag. But it is a behaviour change with no diagnostic, so it is your call. If you would rather it degraded, the cleaner shape is to dedupe at the
ResponseWriter(keep the proxy's value, fall back to the upstream's when the proxy set none) instead of deleting unconditionally.Also noted but not changed, as it predates this PR: the welcome-page rewrite is not gated on a 3xx status, so any response carrying a
Locationheader is rewritten.Tests
TestRespMiddlewarecovers stripping, keepingExpose-Headers, and the welcome-page rewrite.TestCORSHeadersAreNotDuplicatedis the regression test, run over both GET and HEAD. It builds its handler from the samecorsOptionshelperrunReverseProxyuses rather than restating the options — the earlier hand-copied literal had already drifted from the real configuration. Verified it fails without the fix (count=2), and that the HEAD case fails without theAllowedMethodschange ([]).🤖 Generated with Claude Code
https://claude.ai/code/session_01JYzGVwAKQ4ormxRHZg1eDu