fix(middleware/cors): CORS handling #2937
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes involve refining the CORS middleware's request handling by adjusting header conditions in the Changes
Related issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- middleware/cors/cors.go (1 hunks)
- middleware/cors/cors_test.go (13 hunks)
Additional comments: 2
middleware/cors/cors.go (1)
- 165-171: The logic for determining if a request is outside the scope of CORS has been refined by checking the presence of the
Originheader for all requests and theAccess-Control-Request-Methodheader specifically for preflight requests. This is a crucial change that directly addresses the issue described in the PR objectives. However, it's important to ensure that this logic accurately captures all scenarios where a request should be considered outside the scope of CORS. Additionally, consider adding comments to explain the rationale behind these checks for future maintainability.middleware/cors/cors_test.go (1)
- 476-481: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [466-496]
The test cases have been updated to reflect the changes in CORS request handling logic. Specifically, the tests now cover scenarios without the
Originheader, preflight requests with bothOriginandAccess-Control-Request-Methodheaders, and non-preflight requests with theOriginheader. These updates are essential for validating the new CORS logic. It's important to ensure comprehensive coverage, including edge cases that might not be immediately obvious. Consider adding more test cases if any scenarios are not yet covered.
3 tasks
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- middleware/cors/cors.go (3 hunks)
- middleware/cors/cors_test.go (15 hunks)
Files skipped from review as they are similar to previous changes (2)
- middleware/cors/cors.go
- middleware/cors/cors_test.go
ReneWerner87
approved these changes
Mar 26, 2024
ReneWerner87
added a commit
that referenced
this pull request
Mar 28, 2024
* Update pull_request_template.md * Update v3-changes.md * Update CONTRIBUTING.md (#2752) Grammar correction. * chore(encryptcookie)!: update default config (#2753) * chore(encryptcookie)!: update default config docs(encryptcookie): enhance documentation and examples BREAKING CHANGE: removed the hardcoded "csrf_" from the Except. * docs(encryptcookie): reads or modifies cookies * chore(encryptcookie): csrf config example * docs(encryptcookie): md table spacing * build(deps): bump actions/setup-go from 4 to 5 (#2754) Bumps [actions/setup-go](https://github.com/actions/setup-go) from 4 to 5. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v4...v5) --- updated-dependencies: - dependency-name: actions/setup-go dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * 🩹 middleware/logger/: log client IP address by default (#2755) * middleware/logger: Log client IP address by default. * Update doc. * fix: don't constrain middlewares' context-keys to strings 🐛 (#2751) * Revert "Revert ":bug: requestid.Config.ContextKey is interface{} (#2369)" (#2742)" This reverts commit 28be17f. * fix: request ContextKey default value condition Should check for `nil` since it is `any`. * fix: don't constrain middlewares' context-keys to strings `context` recommends using "unexported type" as context keys to avoid collisions https://pkg.go.dev/github.com/gofiber/fiber/v2#Ctx.Locals. The official go blog also recommends this https://go.dev/blog/context. `fiber.Ctx.Locals(key any, value any)` correctly allows consumers to use unexported types or e.g. strings. But some fiber middlewares constrain their context-keys to `string` in their "default config structs", making it impossible to use unexported types. This PR removes the `string` _constraint_ from all middlewares, allowing to now use unexported types as per the official guidelines. However the default value is still a string, so it's not a breaking change, and anyone still using strings as context keys is not affected. * 📚 Update app.md for indentation (#2761) Update app.md for indentation * build(deps): bump github.com/google/uuid from 1.4.0 to 1.5.0 (#2762) Bumps [github.com/google/uuid](https://github.com/google/uuid) from 1.4.0 to 1.5.0. - [Release notes](https://github.com/google/uuid/releases) - [Changelog](https://github.com/google/uuid/blob/master/CHANGELOG.md) - [Commits](google/uuid@v1.4.0...v1.5.0) --- updated-dependencies: - dependency-name: github.com/google/uuid dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github/codeql-action from 2 to 3 (#2763) Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2 to 3. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@v2...v3) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Changing default log output (#2730) changing default log output Closes #2729 * Update hooks.md fix wrong hooks signature * 🩹 Fix: CORS middleware should use the defined AllowedOriginsFunc config when AllowedOrigins is empty (#2771) * 🐛 [Bug]: Adaptator + otelfiber issue #2641 (#2772) * 🩹🚨 - fix for redirect with query params (#2748) * redirect with query params did not work, fix it and add test for it * redirect middleware - fix test typo * ♻️ logger/middleware colorize logger error message #2593 (#2773) * ✨ feat: add liveness and readiness checks (#2509) * ✨ feat: add liveness and readiness checkers * 📝 docs: add docs for liveness and readiness * ✨ feat: add options method for probe checkers * ✅ tests: add tests for liveness and readiness * ♻️ refactor: change default endpoint values * ♻️ refactor: change default value for liveness endpoint * 📝 docs: add return status for liveness and readiness probes * ♻️ refactor: change probechecker to middleware * 📝 docs: move docs to middleware session * ♻️ refactor: apply gofumpt formatting * ♻️ refactor: remove unused parameter * split config and apply a review * apply reviews and add testcases * add benchmark * cleanup * rename middleware * fix linter * Update docs and config values * Revert change to IsReady * Updates based on code review * Update docs to match other middlewares --------- Co-authored-by: Muhammed Efe Cetin <efectn@protonmail.com> Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com> Co-authored-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com> * prepare release v2.52.0 - add more Parser tests * fix healthcheck.md * configure workflows for V2 branch * configure workflows for V2 branch * Fix default value to false in docs of QueryBool (#2811) fix default value to false in docs of QueryBool * update queryParser config * Update ctx.md * Update routing.md * 📚 Doc: Fix code snippet indentation in /docs/api/middleware/keyauth.md Removes an an extra level of indentation in line 51 of `keyauth.md` [here](https://github.com/gofiber/fiber/blob/v2/docs/api/middleware/keyauth.md?plain=1#L51) * fix: healthcheck middleware not working with route group (#2863) * fix: healthcheck middleware not working with route group * perf: change verification method to improve perf * Update healthcheck_test.go * test: add not matching route test for strict routing * add more test cases * correct tests * correct test helpers * correct tests * correct tests --------- Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com> Co-authored-by: René Werner <rene@gofiber.io> * Merge pull request from GHSA-fmg4-x8pw-hjhg * Enforce Wildcard Origins with AllowCredentials check * Expand unit-tests, fix issues with subdomains logic, update docs * Update cors.md * Added test using localhost, ipv4, and ipv6 address * improve documentation markdown --------- Co-authored-by: René Werner <rene@gofiber.io> * Update app.go prepare release v2.52.1 * fix cors domain normalize * fix sync-docs workflow * fix sync-docs workflow * fix(middleware/cors): Validation of multiple Origins (#2883) * fix: allow origins check Refactor CORS origin validation and normalization to trim leading or trailing whitespace in the cfg.AllowOrigins string [list]. URLs with whitespace inside the URL are invalid, so the normalizeOrigin will return false because url.Parse will fail, and the middleware will panic. fixes #2882 * test: AllowOrigins with whitespace * test(middleware/cors): add benchmarks * chore: fix linter errors * test(middleware/cors): use h() instead of app.Test() * test(middleware/cors): add miltiple origins in Test_CORS_AllowOriginScheme * chore: refactor validate and normalize * test(cors/middleware): add more benchmarks * prepare release v2.52.2 * refactor(docs): deactivate docs sync for v2 * refactor(docs): deactivate docs sync for v2 * fix(middleware/cors): Handling and wildcard subdomain matching (#2915) * fix: allow origins check Refactor CORS origin validation and normalization to trim leading or trailing whitespace in the cfg.AllowOrigins string [list]. URLs with whitespace inside the URL are invalid, so the normalizeOrigin will return false because url.Parse will fail, and the middleware will panic. fixes #2882 * test: AllowOrigins with whitespace * test(middleware/cors): add benchmarks * chore: fix linter errors * test(middleware/cors): use h() instead of app.Test() * test(middleware/cors): add miltiple origins in Test_CORS_AllowOriginScheme * chore: refactor validate and normalize * test(cors/middleware): add more benchmarks * fix(middleware/cors): handling and wildcard subdomain matching docs(middleware/cors): add How it works and Security Considerations * chore: grammar * Apply suggestions from code review Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: fix misspelling * test(middleware/cors): combine Invalid_Origins tests * refactor(middleware/cors): headers handling * docs(middleware/cors): Update AllowOrigins description * chore: merge * perf(middleware/cors): optimize handler * perf(middleware/cors): optimize handler * chore(middleware/cors): ipdate origin handling logic * chore(middleware/cors): fix header capitalization * docs(middleware/cors): improve sercuity notes * docs(middleware/cors): Improve security notes * docs(middleware/cors): improve CORS overview * docs(middleware/cors): fix ordering of how it works * docs(middleware/cors): add additional info to How to works * docs(middleware/cors): rm space * docs(middleware/cors): add validation for AllowOrigins origins to overview * docs(middleware/cors): update ExposeHeaders and MaxAge descriptions * docs(middleware/cors): Add dynamic origin validation example * docs(middleware/cors): Improve security notes and fix header capitalization * docs(middleware/cors): configuration examples * docs(middleware/cors): `"*"` --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(middleware/cors): Categorize requests correctly (#2921) * fix(middleware/cors): categorise requests correctly * test(middleware/cors): improve test coverage for request types * test(middleware/cors): Add subdomain matching tests * test(middleware/cors): parallel tests for CORS headers based on request type * test(middleware/cors): Add benchmark for CORS subdomain matching * test(middleware/cors): cover additiona test cases * refactor(middleware/cors): origin validation and normalization * test(middleware/csrf): Fix Benchmark Tests (#2932) * test(middleware/csrf): fix Benchmark_Middleware_CSRF_* * fix(middleware/csrf): update refererMatchesHost() * Prepare release v2.52.3 * fix(middleware/cors): CORS handling (#2937) * fix(middleware/cors): CORS handling * fix(middleware/cors): Vary header handling * test(middleware/cors): Ensure Vary Headers checked * fix(middleware/cors): Vary header handling non-cors OPTIONS requests (#2939) * fix(middleware/cors): Vary header handling non-cors OPTIONS requests * chore(middleware/cors): Add Vary header for non-CORS OPTIONS requests comment * prepare release v2.52.4 * merge v2 in main(v3) * merge v2 in main(v3) * merge v2 in main(v3) --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: tokelo-12 <113810058+tokelo-12@users.noreply.github.com> Co-authored-by: Jason McNeil <sixcolors@mac.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: iRedMail <2048991+iredmail@users.noreply.github.com> Co-authored-by: Benjamin Grosse <ste3ls@gmail.com> Co-authored-by: Mehmet Firat KOMURCU <mehmetfiratkomurcu@hotmail.com> Co-authored-by: Bruno <bdm2943@icloud.com> Co-authored-by: Muhammad Kholid B <muhammadkholidb@gmail.com> Co-authored-by: gilwo <gilwo@users.noreply.github.com> Co-authored-by: Lucas Lemos <lucashenriqueblemos@gmail.com> Co-authored-by: Muhammed Efe Cetin <efectn@protonmail.com> Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com> Co-authored-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com> Co-authored-by: Jongmin Kim <kjongmin26@gmail.com> Co-authored-by: Giovanni Rivera <rivera.giovanni271@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
fsandel
pushed a commit
to stackitcloud/stackit-cert-manager-webhook
that referenced
this pull request
Apr 4, 2024
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/gofiber/fiber/v2](https://github.com/gofiber/fiber) | require | patch | `v2.52.2` -> `v2.52.4` | --- ### Release Notes <details> <summary>gofiber/fiber (github.com/gofiber/fiber/v2)</summary> ### [`v2.52.4`](https://github.com/gofiber/fiber/releases/tag/v2.52.4) [Compare Source](gofiber/fiber@v2.52.3...v2.52.4) ### 🐛 Fixes - Middleware/cors: CORS handling by [@​sixcolors](https://github.com/sixcolors) in gofiber/fiber#2937 - Middleware/cors: Vary header handling non-cors OPTIONS requests by [@​sixcolors](https://github.com/sixcolors) in gofiber/fiber#2939 **Full Changelog**: gofiber/fiber@v2.52.3...v2.52.4 ### [`v2.52.3`](https://github.com/gofiber/fiber/releases/tag/v2.52.3) [Compare Source](gofiber/fiber@v2.52.2...v2.52.3) #### 🐛 Fixes - Middleware/cors: Handling and wildcard subdomain matching by [@​sixcolors](https://github.com/sixcolors) in gofiber/fiber#2915 - Middleware/cors: Categorize requests correctly by [@​sixcolors](https://github.com/sixcolors) in gofiber/fiber#2921 - Middleware/csrf: Fix Benchmark Tests by [@​sixcolors](https://github.com/sixcolors) in gofiber/fiber#2932 **Full Changelog**: gofiber/fiber@v2.52.2...v2.52.3 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or rename PR to start with "rebase!". 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request addresses issue #2936, fixing a bug introduced in the Fiber v2.52.3 release. The bug caused incorrect checking of non-preflight requests using preflight conditions.
Fiber CORS middleware validation sample: https://github.com/sixcolors/fiber-cors-middleware-validation-sample
Summary by CodeRabbit
Summary by CodeRabbit
fiber.HeaderAccessControlRequestMethodin various test cases.fiber.HeaderAccessControlRequestMethodsettings.