fix(middleware): avoid duplicate CORS headers behind proxies#2983
fix(middleware): avoid duplicate CORS headers behind proxies#2983leno23 wants to merge 2 commits into
Conversation
Set CORS response headers after the handler runs for simple requests and skip them when an upstream handler already set Access-Control-Allow-Origin. This prevents duplicated headers when CORS middleware is used together with reverse proxies in chained services. Fixes labstack#2853 Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent duplicated CORS response headers (e.g., Access-Control-Allow-Origin, Vary: Origin) when Echo apps are chained behind reverse proxies and each layer uses middleware.CORS() (issue #2853).
Changes:
- Adjust CORS middleware flow for simple (non-preflight) requests to avoid adding CORS headers when an upstream handler already set them.
- Move/condition
Vary: Originheader additions so they’re not always added unconditionally. - Add a regression test covering a proxy layer + upstream both using CORS middleware.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| middleware/cors.go | Updates when/if CORS headers are applied to avoid duplicates in chained proxy setups. |
| middleware/cors_test.go | Adds a regression test for “proxy + upstream both set CORS” header-duplication behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
why this is against |
|
Addressed the Copilot review feedback in e6dc496: simple-request CORS headers are now registered with Response.Before so they are applied before the response is committed, while still skipping them when an upstream/proxy handler already set Access-Control-Allow-Origin. The regression test now asserts against rec.Result().Header and closes the response body.\n\nValidation: go test ./middleware -run 'TestCORS|TestCORSNoDuplicateHeadersFromUpstream'; go test ./...\n\nOn the base branch question: I targeted v4 because this PR is against the v4 middleware/API. I checked retargeting to master, but master has already moved to the v5 API/package shape, so rebasing this PR there is not a trivial base change and would need a separate v5 adaptation if that is preferred. |
|
As said in #2990 (comment) the My question is - what upstream proxy it is that adds does CORS? and why it does? CORS is not a network security mechanism - it's a browser access policy. The server/application that owns the API ultimately defines which origins are allowed to access it. At the moment we are trying to change application CORS middleware to some nische use-case - to Proxy level, conditional, CORS middleware. In microservices setup as #2853 (comment) refers to - why is CORS done in both places - should it not be either "gateway" for all microservices or in every microservice itself (right place). Note: in case of Echo proxy - you can always configure it to add headers conditionally - in case downstream server (app) did not add the header - the proxy will add it. There is I am not favor of these PRs. |
Summary
Access-Control-Allow-Originand related headers when an upstream handler (e.g. reverse proxy) already set themTest plan
go test ./middleware/...TestCORSNoDuplicateHeadersFromUpstream— proxy layer + upstream both use CORS middleware, response has singleAccess-Control-Allow-OriginandVaryFixes #2853