Fix CORS header duplication in proxy chains

[bk-simon]

Summary

Fixes issue #2853 - CORS middleware was duplicating headers when multiple Echo services with CORS middleware were chained (e.g., Service A proxies to Service B, both with middleware.CORS enabled).

Changes

• Added check to detect existing Access-Control-Allow-Origin headers in responses (indicating the request was proxied from an upstream service that already applied CORS)
• When CORS headers are already present, the middleware now skips re-applying them to prevent duplication
• Updated Vary header handling to check if values already exist before adding them, preventing duplicate Vary entries
• Added comprehensive test cases for proxy chain scenarios (both regular and preflight requests)

Test Plan

• All existing CORS tests pass
• Added TestCORSProxyChain to verify headers are not duplicated in proxy scenarios
• Added TestCORSProxyChainPreflight to verify preflight requests in proxy chains
• Verified that the fix prevents duplicate Access-Control-Allow-Origin and Vary headers

Reproduces Issue

This fix addresses the exact scenario described in #2853 where multiple proxy layers each independently apply CORS headers, causing accumulation.

Before: Multiple CORS middlewares in a chain would each add headers, resulting in duplicates
After: Middleware detects existing CORS headers and skips processing, preventing duplication

[epaes90]

Argus Code Review

Score: 100/100

Reviewed 2 files with no issues found. Highlights: Test coverage is present; Changes are focused and well-scoped; No security, performance, or quality issues detected. Score: 100/100.

No inline findings to report.

[epaes90]

Argus Code Review

Score: 100/100

Reviewed 2 files with no issues found. Highlights: Test coverage is present; Changes are focused and well-scoped; No security, performance, or quality issues detected. Score: 100/100.

No inline findings to report.

[epaes90]

Argus Code Review

Score: 100/100

Reviewed 2 files with no issues found. Highlights: Test coverage is present; Changes are focused and well-scoped; No security, performance, or quality issues detected. 3 minor observations noted. Score: 100/100.

No inline findings to report.

[epaes90]

Argus Code Review

Score: 95/100

Reviewed 2 files. Found 1 issues. Score: 95/100.

---

Additional Findings

These findings are on lines outside the diff and cannot be shown inline.

• [MEDIUM] Incorrect handling of negative MaxAge values

The code sets maxAge to "0" only when config.MaxAge > 0, but the comment on line 108 states that a negative value should send "0" to instruct browsers not to cache. However, negative values are not handled; they fall through and keep maxAge as "0" only because it's initialized to "0". This is misleading and inconsistent with the documented behavior.

Justification: The comment on line 108 says 'negative value sends "0" which instructs browsers not to cache that response.' but the code only checks if config.MaxAge > 0. If config.MaxAge is negative, the condition is false and maxAge remains "0" due to initialization, but this is not explicit and relies on the initial value. This could lead to confusion about the intended behavior.

Validation: Lines 160-163: maxAge is initialized to "0" on line 160. The condition on line 161 only updates maxAge when config.MaxAge > 0. Negative values are not explicitly handled, so they keep the initial "0" value, but the comment suggests they should be explicitly set to "0". This inconsistency between comment and code could cause maintenance issues.

Suggestion: Explicitly handle negative MaxAge values by setting maxAge to "0" when config.MaxAge < 0, or update the documentation to clarify that negative values are treated as zero. (middleware/cors.go:160)

[epaes90]

Argus Code Review

Score: 85/100

Reviewed 2 files. Found 2 issues. 1 high. Score: 85/100.

[epaes90]

[MEDIUM] Vary header deduplication uses substring match instead of token comparison

The code checks if 'Origin' is present in the Vary header using substring matching (strings.Contains). This can incorrectly match headers like 'X-Origin-Custom' or 'Original', causing the code to skip adding 'Origin' when it should be added, leading to incorrect caching behavior.

Justification: The substring check on line 208 (!strings.Contains(varyHeader, echo.HeaderOrigin)) is vulnerable to false positives. If the Vary header contains 'X-Origin-Custom', the substring 'Origin' is found, causing the code to skip adding 'Origin' even though the exact token isn't present. This violates the HTTP specification which requires exact token matching for the Vary header.

Validation: Line 208: if !strings.Contains(varyHeader, echo.HeaderOrigin) { — uses substring search instead of token parsing. This will incorrectly match 'X-Origin-Custom' as containing 'Origin', preventing the proper addition of the 'Origin' token to the Vary header.

Suggestion: Split the Vary header on ', ' and check for exact token matches. Use a helper function like containsToken(varyHeader, echo.HeaderOrigin) that properly parses the header.

[epaes90]

[HIGH] CORS middleware bypass via pre-existing Access-Control-Allow-Origin header

The middleware checks if the Access-Control-Allow-Origin header is already set in the response and skips all CORS processing if it is. An attacker or a misconfigured upstream proxy can inject this header, causing the middleware to skip origin validation, method checks, and credential settings, effectively bypassing CORS security.

Justification: The code at lines 196-204 introduces a new check: if res.Header().Get(echo.HeaderAccessControlAllowOrigin) != "", it skips processing and either returns a 204 for preflight or calls next(c). This allows any upstream component (or an attacker via header injection in a request that proxies through) to set the CORS header and bypass the security validation.

Validation: Lines 198-204: if res.Header().Get(echo.HeaderAccessControlAllowOrigin) != "" { ... // CORS headers already present, likely from upstream - skip processing ... }. This conditional skip occurs before any origin validation (allowOriginFunc is never called), allowing the request to proceed without CORS checks.

Suggestion: Remove the early exit based on pre-existing CORS headers. Always validate the origin and apply CORS logic regardless of existing headers, or at least verify the existing header matches the validated origin before skipping.