Fix CORS header duplication in chained proxies#2990
Conversation
|
I do not think this is a good idea. I do not think this is sustainable for middlewares to start adding extra logic so they work potentially with some other middleware. and Are we lacking mechnisms (callbacks/handler funcs etc) at the moment in middlewares to allow for you to plug-in these work-a-rounds at your app/repo? |
There was a problem hiding this comment.
Pull request overview
This PR fixes duplicated CORS-related response headers (notably Access-Control-Allow-Origin and Vary) when Echo’s CORS middleware is used in chained proxy scenarios where upstream headers are copied with Header.Add.
Changes:
- Moves simple-request CORS header writing into
Response.Before(...)hooks so the middleware canSet/dedupe headers after an upstream/proxy handler hasAdded them. - Adds an
addVaryHeaderhelper to merge and deduplicateVaryvalues case-insensitively. - Adds a unit test that simulates reverse-proxy-style header copying to ensure CORS headers are not duplicated.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| middleware/cors.go | Defers simple-request CORS header writes via Response.Before and introduces addVaryHeader for deduping Vary. |
| middleware/cors_test.go | Adds a regression test simulating proxy header copying to verify no duplication of CORS/Vary headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func addVaryHeader(h http.Header, value string) { | ||
| if h.Get(echo.HeaderVary) == "" { | ||
| h.Set(echo.HeaderVary, value) | ||
| return | ||
| } | ||
| for _, v := range h.Values(echo.HeaderVary) { | ||
| for _, part := range strings.Split(v, ",") { | ||
| if strings.EqualFold(strings.TrimSpace(part), value) { | ||
| return | ||
| } | ||
| } | ||
| } | ||
| h.Add(echo.HeaderVary, value) | ||
| } |
| // no CORS middleware should block non-preflight requests; | ||
| // such requests should be let through. One reason is that not all requests that | ||
| // carry an Origin header participate in the CORS protocol. | ||
| res.Before(func() { | ||
| addVaryHeader(res.Header(), echo.HeaderOrigin) | ||
| }) | ||
| return next(c) |
|
As I said I am not in favor of this PR, especially because of just for information - there is similar PR agains |
Fixes #2853.
When Echo CORS middleware is run in a chained proxy setup (or in front of any handler copying upstream headers using Add), headers like Access-Control-Allow-Origin and Vary get duplicated in the response.
Changes: