🩹 Fix: Middleware/CORS Remove Scheme Restriction #3163
Conversation
zingi
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe changes primarily involve modifications to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 #3163 +/- ##
==========================================
- Coverage 82.46% 82.38% -0.08%
==========================================
Files 113 113
Lines 8468 8466 -2
==========================================
- Hits 6983 6975 -8
- Misses 1084 1088 +4
- Partials 401 403 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
gaby
changed the title
gaby
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: 0
🧹 Outside diff range and nitpick comments (1)
middleware/cors/utils_test.go (1)
Line range hint
1-186: Overall impact: Positive, with room for enhancement.The addition of the "app" scheme test case is a good start in addressing the issue with non-standard schemes. However, to ensure robust coverage, consider the following suggestions:
- Add more test cases for other potential non-standard schemes (e.g., "custom://", "ftp://").
- Include edge cases, such as schemes with special characters or unusual formats.
- Consider adding negative test cases to ensure that invalid schemes are still properly rejected.
These additions would further strengthen the test suite and provide more comprehensive coverage for the CORS middleware changes.
Would you like assistance in generating additional test cases to cover these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- middleware/cors/utils.go (0 hunks)
- middleware/cors/utils_test.go (1 hunks)
💤 Files with no reviewable changes (1)
- middleware/cors/utils.go
🧰 Additional context used
🔇 Additional comments (1)
middleware/cors/utils_test.go (1)
20-20: LGTM: New test case aligns with PR objectives.The added test case for the "app" scheme correctly validates the new behavior of the
normalizeOriginfunction. This change directly addresses the issue described in #3160, allowing non-standard schemes like "app://" to be processed without causing panics.To ensure comprehensive coverage, let's verify if similar test cases exist for other non-standard schemes:
This will help us confirm if the change is consistently applied across all relevant test cases.
✅ Verification successful
Verification Successful: Comprehensive test coverage confirmed.
The test suite includes the new "app" scheme and appropriately handles existing standard and non-standard origins. No additional test cases for other non-standard schemes are required at this time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases with non-standard schemes in the CORS middleware tests rg --type go 'origin:.*?://' middleware/corsLength of output: 5578
|
Hi @zingi can you backport this PR to v2 as well? |
Yes, we need a separate PR to address this in the |
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
The Fiber CORS middleware unnecessarily restricts the scheme for the origins that can be configured. As already stated here, the CORS spec does not define any restrictions on the scheme. However, the Fiber CORS middleware restricts the scheme to
httporhttps.Fixes #3160
Changes introduced
Removes the scheme restriction of the CORS middleware configuration.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md