🧹 chore: Improve CORS middleware response headers #3505
Conversation
WalkthroughThe CORS middleware was updated to preserve the original casing of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FiberApp
participant CORS_Middleware
Client->>FiberApp: Send request with Origin header
FiberApp->>CORS_Middleware: Pass request
CORS_Middleware->>CORS_Middleware: Lowercase Origin for comparison
CORS_Middleware->>CORS_Middleware: Preserve original Origin for response
alt Preflight (OPTIONS)
CORS_Middleware->>CORS_Middleware: setPreflightHeaders()
CORS_Middleware->>Client: Respond with Access-Control headers (original Origin case)
else Simple request
CORS_Middleware->>CORS_Middleware: setSimpleHeaders()
CORS_Middleware->>Client: Respond with Access-Control headers (original Origin case)
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gaby
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3505 +/- ##
==========================================
+ Coverage 83.88% 83.91% +0.03%
==========================================
Files 120 120
Lines 12278 12284 +6
==========================================
+ Hits 10299 10308 +9
+ Misses 1555 1553 -2
+ Partials 424 423 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refines the CORS middleware to align more closely with the CORS RFC by preserving Origin header casing, separating simple vs. preflight header logic, and omitting the Access-Control-Max-Age header on simple requests. It also introduces regression tests for these behaviors.
- Refactor
cors.goto usesetSimpleHeadersandsetPreflightHeaders, and keep the original Origin casing - Remove Max-Age header on non-OPTIONS (simple) requests
- Add tests for Max-Age omission and origin-case preservation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/cors/cors.go | Split header helpers, rename and preserve raw origin header, remove slices import |
| middleware/cors/cors_test.go | Add tests for simple-request MaxAge omission and origin-case preservation |
Comments suppressed due to low confidence (4)
middleware/cors/cors.go:86
- [nitpick] The name
originHeaderRawis descriptive but a bit verbose; you might rename it torawOriginorrequestOriginfor brevity and clarity.
originHeaderRaw := c.Get(fiber.HeaderOrigin)
middleware/cors/cors.go:189
- Add a GoDoc comment above
setSimpleHeadersto explain its parameters and behavior, which will improve maintainability and aid new contributors.
func setSimpleHeaders(c fiber.Ctx, allowOrigin string, cfg Config) {
middleware/cors/cors_test.go:100
- There are no tests verifying that
ExposeHeadersare correctly applied in both simple and preflight flows—consider adding tests for theAccess-Control-Expose-Headersheader.
func testDefaultOrEmptyConfig(t *testing.T, app *fiber.App) {
middleware/cors/cors.go:118
- Consider converting
allowOriginsinto a map or a set at initialization to avoid linear scans on every request, improving lookup performance for large origin lists.
for _, origin := range allowOrigins {
|
Neither issue are Non-RFC compliant. Origin header values are case-insensitive and max-age is not required in a non-preflight, but having it also isn’t explicitly not allowed. The fixes are however improving the middleware. I just changed the title. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/cors/cors.go(6 hunks)middleware/cors/cors_test.go(1 hunks)
🔇 Additional comments (7)
middleware/cors/cors.go (5)
85-87: Excellent RFC compliance improvement!Preserving the original case of the
Originheader while performing case-insensitive comparisons is the correct approach per CORS RFC standards. The dual-variable approach is clean and efficient.
118-123: Well-implemented case preservation logic.The switch from
slices.Containsto an explicit loop is necessary to preserve the original case in the response while maintaining case-insensitive comparison. The logic correctly compares normalized origins with the lowercase request origin and returns the raw origin header when a match is found.
189-189: Good RFC compliance improvements.Removing the unused
maxAgeparameter and addingExposeHeadersto simple requests aligns better with CORS specifications. TheAccess-Control-Expose-Headersshould indeed be set for both simple and preflight requests when configured.Also applies to: 204-208
210-220: Clean separation of header-setting logic.The new
setPreflightHeadersfunction provides a clear separation between simple and preflight request headers. The implementation correctly callssetSimpleHeadersfirst and then adds preflight-specific headers likeAccess-Control-Max-Age. This refactoring improves code maintainability and RFC compliance.
150-150: Correct implementation of the refactored architecture.The function calls correctly route simple requests to
setSimpleHeaders(without maxAge) and preflight requests tosetPreflightHeaders. This ensures thatAccess-Control-Max-Ageis only set for preflight requests, improving RFC compliance.Also applies to: 168-168
middleware/cors/cors_test.go (2)
69-81: Comprehensive test for MaxAge behavior on simple requests.This test correctly verifies that the
Access-Control-Max-Ageheader is not set on simple requests even when the MaxAge configuration is specified. This validates the RFC compliance improvement where MaxAge should only be sent with preflight responses.
83-98: Excellent test coverage for origin case preservation.This test thoroughly validates that the middleware preserves the exact casing of the
Originheader in the response. The test design is particularly good - using lowercase in the configuration and uppercase in the request clearly demonstrates that case-insensitive matching works while preserving the original case in the response.
sixcolors
changed the title
Summary
Originheader when respondingAccess-Control-Max-Ageon simple requests