chore(middleware/cors): Merge changes from v2 #2922
Conversation
…er#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 gofiber#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>
sixcolors
requested review from
gaby,
ReneWerner87 and
efectn
and removed request for
a team
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2922 +/- ##
==========================================
- Coverage 82.75% 82.67% -0.09%
==========================================
Files 116 116
Lines 8374 8393 +19
==========================================
+ Hits 6930 6939 +9
- Misses 1101 1112 +11
+ Partials 343 342 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
WalkthroughThe Changes
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: 7
Configuration used: CodeRabbit UI
Files selected for processing (5)
- docs/api/middleware/cors.md (4 hunks)
- middleware/cors/cors.go (4 hunks)
- middleware/cors/cors_test.go (21 hunks)
- middleware/cors/utils.go (3 hunks)
- middleware/cors/utils_test.go (1 hunks)
Additional comments: 26
middleware/cors/utils.go (1)
- 45-50: The update to
normalizeOriginto disallow wildcards with a protocol is a significant security enhancement. It prevents potential misuse of wildcard origins in CORS configurations, which could lead to security vulnerabilities.middleware/cors/cors.go (1)
- 113-113: The panic in case of insecure setup (
AllowCredentialsis true andAllowOriginsis set to a wildcard) is a critical security measure. It prevents a common misconfiguration that could lead to security vulnerabilities. This strict approach ensures that developers are aware of potential security risks.docs/api/middleware/cors.md (9)
- 11-11: The term
AllowOriginsis correctly used in the context of CORS configuration and does not require a space between "Allow" and "Origins".- 13-13: The term
AllowOriginsis correctly used and does not require a space between "Allow" and "Origins".- 4-18: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [25-25]
The space after the opening parenthesis is correctly placed in the context of the code block formatting.
- 44-44: The space before the closing parenthesis is correctly placed in the context of the code block formatting.
- 67-67: The space after the opening parenthesis is correctly placed in the context of the code block formatting.
- 96-96: The term
AllowOriginsandAllowCredentialsare correctly used in the context of CORS configuration and do not require a space between the words.- 96-96: The space before the closing parenthesis is correctly placed in the context of the code block formatting.
- 124-124: The space before the closing parenthesis is correctly placed in the context of the code block formatting.
- 152-152: The space before the closing parenthesis is correctly placed in the context of the code block formatting.
middleware/cors/cors_test.go (15)
- 37-37: Setting the
Access-Control-Request-Methodheader toGETin theTest_CORS_Negative_MaxAgetest function is a good practice for simulating preflight requests accurately.- 49-56: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [52-64]
In the
testDefaultOrEmptyConfighelper function, the addition of setting theAccess-Control-Request-Methodheader for both GET and OPTIONS methods is a positive change. It ensures that the tests more accurately reflect real-world preflight request scenarios.
- 103-110: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [92-107]
The modifications in the
Test_CORS_Wildcardtest function, specifically setting theAccess-Control-Request-Methodheader, enhance the test's accuracy in simulating preflight requests. This is crucial for ensuring the middleware behaves as expected in real-world scenarios.
- 145-152: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [135-149]
Similar to previous comments, setting the
Access-Control-Request-Methodheader in theTest_CORS_Origin_AllowCredentialstest function is a commendable practice. It ensures the test accurately represents preflight requests, which is essential for validating the middleware's behavior.
- 184-216: The
Test_CORS_Invalid_Origins_Panictest function introduces a comprehensive set of invalid origins to verify that the middleware correctly panics in these scenarios. This is an excellent addition for ensuring robust error handling and validation within the middleware.- 244-266: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [235-263]
The
Test_CORS_Subdomaintest function's changes, including setting theAccess-Control-Request-Methodheader for various origin scenarios, are well thought out. These changes ensure that the middleware's subdomain handling is thoroughly tested, which is crucial for applications that require flexible CORS configurations.
- 340-352: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [333-346]
The addition of test cases in
Test_CORS_AllowOriginSchemeto cover various origin patterns and request origins is a valuable enhancement. It ensures that the middleware's behavior is consistent and predictable across different origin schemes and subdomain configurations.
- 378-378: Setting the
Access-Control-Request-Methodheader in theTest_CORS_AllowOriginSchemetest function for various origin and pattern scenarios is a good practice. It ensures the tests accurately simulate preflight requests, which is essential for validating the middleware's behavior in real-world scenarios.- 391-418: The
Test_CORS_AllowOriginHeader_NoMatchtest function correctly verifies that theAccess-Control-Allow-Originheader is not set when the origin does not match the allowed origins. This is an important security feature to test, ensuring that the middleware correctly enforces CORS policies.- 425-425: The
Test_CORS_Nextfunction tests the middleware's ability to correctly call the next handler in the chain when theNextfunction returnstrue. This is an important aspect of middleware behavior to test, ensuring that the middleware integrates well with other handlers in the application.- 436-529: The
Test_CORS_Headers_BasedOnRequestTypefunction introduces comprehensive tests for various request types and scenarios, including with and without origin headers, and preflight requests. This thorough testing is crucial for ensuring the middleware behaves correctly across a wide range of request types and CORS scenarios.- 547-553: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [531-578]
The
Test_CORS_AllowOriginsAndAllowOriginsFuncfunction's changes, including the addition of tests for theAllowOriginsFuncconfiguration, are a significant improvement. These tests ensure that dynamic origin validation works as expected, which is essential for applications that require more complex CORS configurations.
- 618-618: The addition of setting the
Access-Control-Request-Methodheader in theTest_CORS_AllowOriginsFuncfunction is a good practice. It ensures the tests accurately simulate preflight requests, which is essential for validating the middleware's behavior in real-world scenarios.- 766-766: In the
Test_CORS_AllowOriginsAndAllowOriginsFunc_AllUseCasesfunction, setting theAccess-Control-Request-Methodheader for various configurations and request origins is a positive change. It ensures the tests more accurately reflect real-world preflight request scenarios.- 857-857: The
Test_CORS_AllowCredentialsfunction's changes, including setting theAccess-Control-Request-Methodheader for various configurations and request origins, enhance the test's accuracy in simulating preflight requests. This is crucial for ensuring the middleware behaves as expected in real-world scenarios.
gaby
changed the title
|
@sixcolors pls check my 2 last suggestions |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- docs/api/middleware/cors.md (4 hunks)
- middleware/cors/cors.go (4 hunks)
- middleware/cors/cors_test.go (21 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/cors/cors_test.go
Additional comments: 9
middleware/cors/cors.go (6)
- 18-21: The introduction of
AllowOriginsFuncfor dynamic evaluation of allowed origins is a significant enhancement. It provides flexibility in handling CORS requests, especially for applications with complex origin validation requirements.- 113-113: The panic in case of insecure setup (
AllowCredentialsis true andAllowOriginsis set to a wildcard) is a critical security measure. It prevents potential misconfigurations that could lead to security vulnerabilities.- 122-132: The
processOriginfunction, which normalizes and checks the validity of origins, is a robust addition. However, panicking on invalid origin configurations might not be the best approach for all applications. Consider allowing for a configurable error handling strategy to provide more flexibility in how such errors are managed.- 173-179: The logic to determine if a request is outside the scope of CORS by checking the presence of
OriginandAccess-Control-Request-Methodheaders is correctly implemented. This is a crucial step in ensuring that non-CORS requests are handled appropriately.- 185-203: The logic for determining the
allowOriginbased onallowAllOrigins, the list of allowed origins, and the list of allowed subdomains is comprehensive and well-structured. It effectively handles different scenarios for validating theOriginheader against the configured allowed origins and subdomains.- 232-274: The
setCORSHeadersfunction is a key component of the middleware, correctly setting CORS headers based on the configuration. It handles various scenarios, including credentials support and default headers. Ensure comprehensive test coverage for this function, especially for the newly added logic, to maintain the middleware's reliability.docs/api/middleware/cors.md (3)
- 11-11: The term "AllowOrigins" is mentioned with a possible spelling mistake. However, in the context of this document, "AllowOrigins" and "AllowOriginsFunc" are specific configuration options for the CORS middleware and are correctly used. No action is needed here.
- 115-115: There's a minor typographical issue with an extra space before the description of the
Nextproperty. However, this is a minor formatting issue in the Markdown table and does not impact the readability or understanding of the content significantly.Consider removing the extra space for consistency in the table formatting.
- 120-120: The phrase "whether or not" can be shortened to "whether" to make the sentence more concise without changing its meaning.
Consider revising the sentence for conciseness.
middleware/cors/cors.go
Outdated
| for _, origin := range origins { | ||
| if i := strings.Index(origin, "://*."); i != -1 { | ||
| normalizedOrigin, isValid := processOrigin(origin[:i+3] + origin[i+4:]) | ||
| if !isValid { | ||
| continue | ||
| } | ||
| sd := subdomain{prefix: normalizedOrigin[:i+3], suffix: normalizedOrigin[i+3:]} | ||
| allowSOrigins = append(allowSOrigins, sd) | ||
| } else { | ||
| normalizedOrigin, isValid := processOrigin(origin) | ||
| if !isValid { | ||
| continue | ||
| } | ||
| allowOrigins = append(allowOrigins, normalizedOrigin) |
There was a problem hiding this comment.
The handling of allowed origins and subdomains through the processOrigin function and the construction of allowSOrigins and allowOrigins arrays is well-implemented. However, static analysis hints at potential lack of test coverage for some of the new lines of code, specifically lines 141 and 148. Ensure these functionalities are covered by unit tests to verify their correctness and prevent future regressions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- middleware/cors/utils_test.go (4 hunks)
Additional comments: 6
middleware/cors/utils_test.go (6)
- 6-6: Please ensure that the
assertpackage fromgithub.com/stretchr/testify/assertis used consistently across all tests for uniformity and readability.- 18-26: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [6-32]
The
Test_normalizeOriginfunction tests various scenarios for origin normalization. It's comprehensive and covers a wide range of cases, including scheme, port, path, query, fragment, and IPv6 addresses. However, it's important to ensure that these tests align with the updated logic innormalizeOriginfunction, especially regarding the handling of wildcards and file schemes.
- 2-9: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [33-64]
The
Test_matchSchemefunction tests the scheme matching logic. It's crucial to verify that the implementation ofmatchSchemeaccurately reflects the expectations set by these test cases, especially considering the importance of scheme matching in CORS policy enforcement.
- 80-85: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [65-98]
The
Test_normalizeDomainfunction tests the domain normalization logic. Given the removal ofvalidateDomainand introduction of new functionalities likesubdomain.match, it's essential to ensure thatnormalizeDomainis still relevant and correctly implemented. Consider adding or updating test cases if the domain normalization logic has changed.Verification successful
The
normalizeDomainfunction's logic, as revealed by the script output, aligns with the test cases provided in the review comment. The function is designed to remove schemes (http or https) and ports from the input string, which matches the expectations set by the test cases. The test cases cover various scenarios, including schemes, ports, paths, queries, fragments, and cases without schemes, which are all adequately addressed by the current implementation ofnormalizeDomain. Therefore, the review comment's concern about ensuringnormalizeDomainis still relevant and correctly implemented appears to be addressed by the existing function logic.* 114-159: The newly added `TestSubdomainMatch` function tests the `subdomain.match` method. It's well-structured and covers various scenarios, including valid subdomain matches, invalid prefixes, invalid suffixes, empty origins, and partial matches. Ensure that these test cases comprehensively cover all possible scenarios for subdomain matching to guarantee the robustness of the `subdomain.match` method. * 161-176: The `Benchmark_CORS_SubdomainMatch` function provides a benchmark for the `subdomain.match` method. It's a good practice to include benchmarks for critical performance paths. However, consider adding more varied scenarios to the benchmark to cover different cases, such as varying lengths of subdomains and origins, to get a more comprehensive understanding of the performance characteristics.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `normalizeDomain` function logic aligns with these test cases. ast-grep --lang go --pattern $'func normalizeDomain($_) $_ { $$$ }'Length of output: 792
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (3)
- middleware/cors/cors.go (4 hunks)
- middleware/cors/cors_test.go (23 hunks)
- middleware/cors/utils_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- middleware/cors/cors_test.go
- middleware/cors/utils_test.go
Additional comments: 3
middleware/cors/cors.go (3)
- 18-21: The introduction of
AllowOriginsFuncprovides a flexible way to dynamically evaluate allowed origins, enhancing the middleware's configurability. This is a positive change for scenarios where static origin lists are insufficient.- 163-169: The check for the presence of
OriginandAccess-Control-Request-Methodheaders to determine if a request falls within the scope of CORS is correctly implemented. This is a crucial step in ensuring that the middleware only processes relevant requests.- 175-193: The logic for determining the
allowOriginvalue based on static origins, subdomains, and dynamic evaluation viaAllowOriginsFuncis comprehensive and well-structured. It effectively covers different scenarios for allowed origins, ensuring flexibility and security in CORS policy enforcement.
| @@ -110,31 +110,38 @@ | |||
|
|
|||
| // Validate CORS credentials configuration | |||
| if cfg.AllowCredentials && cfg.AllowOrigins == "*" { | |||
| log.Panic("[CORS] Insecure setup, 'AllowCredentials' is set to true, and 'AllowOrigins' is set to a wildcard.") //nolint:revive // we want to exit the program | |||
| panic("[CORS] Insecure setup, 'AllowCredentials' is set to true, and 'AllowOrigins' is set to a wildcard.") | |||
There was a problem hiding this comment.
Raising a panic for insecure configurations (AllowCredentials set to true with AllowOrigins as a wildcard) is a critical security measure. This ensures that developers are aware of potential vulnerabilities in their CORS setup. However, consider providing a more detailed error message or documentation reference to help developers understand and resolve the issue.
- panic("[CORS] Insecure setup, 'AllowCredentials' is set to true, and 'AllowOrigins' is set to a wildcard.")
+ panic("[CORS] Insecure setup detected: 'AllowCredentials' is set to true while 'AllowOrigins' is set to a wildcard ('*'). This configuration is disallowed due to security vulnerabilities. Please refer to the CORS middleware documentation for more information on secure configuration practices.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic("[CORS] Insecure setup, 'AllowCredentials' is set to true, and 'AllowOrigins' is set to a wildcard.") | |
| panic("[CORS] Insecure setup detected: 'AllowCredentials' is set to true while 'AllowOrigins' is set to a wildcard ('*'). This configuration is disallowed due to security vulnerabilities. Please refer to the CORS middleware documentation for more information on secure configuration practices.") |
| for _, origin := range origins { | ||
| if i := strings.Index(origin, "://*."); i != -1 { | ||
| trimmedOrigin := strings.TrimSpace(origin[:i+3] + origin[i+4:]) | ||
| isValid, normalizedOrigin := normalizeOrigin(trimmedOrigin) | ||
| if !isValid { | ||
| panic("[CORS] Invalid origin format in configuration: " + trimmedOrigin) | ||
| } | ||
| sd := subdomain{prefix: normalizedOrigin[:i+3], suffix: normalizedOrigin[i+3:]} | ||
| allowSOrigins = append(allowSOrigins, sd) | ||
| } else { | ||
| trimmedOrigin := strings.TrimSpace(origin) | ||
| isValid, normalizedOrigin := normalizeOrigin(trimmedOrigin) | ||
| if !isValid { | ||
| panic("[CORS] Invalid origin format in configuration: " + trimmedOrigin) | ||
| } | ||
| allowOrigins = append(allowOrigins, normalizedOrigin) |
There was a problem hiding this comment.
The logic for handling allowed origins, including subdomain support, is well-implemented. It correctly identifies and normalizes origins, and panics on invalid formats, which is crucial for maintaining a secure CORS policy. However, consider encapsulating this logic into a separate function for better readability and maintainability.
// Suggested refactor into a separate function
func parseAndValidateOrigins(originsStr string) ([]string, []subdomain) {
var allowOrigins []string
var allowSOrigins []subdomain
origins := strings.Split(originsStr, ",")
for _, origin := range origins {
if i := strings.Index(origin, "://*."); i != -1 {
// Subdomain handling logic...
} else {
// Standard origin handling logic...
}
}
return allowOrigins, allowSOrigins
}| // Ommit allowMethods and allowHeaders, only used for pre-flight requests | ||
| if c.Method() != fiber.MethodOptions { | ||
| c.Vary(fiber.HeaderOrigin) | ||
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | ||
|
|
||
| if cfg.AllowCredentials { | ||
| c.Set(fiber.HeaderAccessControlAllowCredentials, "true") | ||
| } | ||
| if exposeHeaders != "" { | ||
| c.Set(fiber.HeaderAccessControlExposeHeaders, exposeHeaders) | ||
| } | ||
| setCORSHeaders(c, allowOrigin, "", "", exposeHeaders, maxAge, cfg) | ||
| return c.Next() | ||
| } | ||
|
|
||
| // Preflight request | ||
| c.Vary(fiber.HeaderOrigin) | ||
| c.Vary(fiber.HeaderAccessControlRequestMethod) | ||
| c.Vary(fiber.HeaderAccessControlRequestHeaders) | ||
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | ||
| c.Set(fiber.HeaderAccessControlAllowMethods, allowMethods) | ||
|
|
||
| if cfg.AllowCredentials { | ||
| // When AllowCredentials is true, set the Access-Control-Allow-Origin to the specific origin instead of '*' | ||
| if allowOrigin != "*" && allowOrigin != "" { | ||
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | ||
| c.Set(fiber.HeaderAccessControlAllowCredentials, "true") | ||
| } else if allowOrigin == "*" { | ||
| log.Warn("[CORS] 'AllowCredentials' is true. Ensure 'AllowOrigins' is not set to '*' in the configuration.") | ||
| } | ||
| } else { | ||
| // For non-credential requests, it's safe to set to '*' or specific origins | ||
| setCORSHeaders(c, allowOrigin, allowMethods, allowHeaders, exposeHeaders, maxAge, cfg) | ||
|
|
||
| // Send 204 No Content | ||
| return c.SendStatus(fiber.StatusNoContent) | ||
| } |
There was a problem hiding this comment.
The distinction between handling simple and preflight CORS requests is clear and aligns with the CORS specification. However, static analysis hints at potential lack of test coverage for the new lines of code related to setting CORS headers, specifically line 251. Enhancing test coverage for these parts would improve the reliability of the middleware.
Please add or extend unit tests to cover the new logic for setting CORS headers in both simple and preflight requests.
| if cfg.AllowCredentials { | ||
| // When AllowCredentials is true, set the Access-Control-Allow-Origin to the specific origin instead of '*' | ||
| if allowOrigin == "*" { | ||
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | ||
| log.Warn("[CORS] 'AllowCredentials' is true, but 'AllowOrigins' cannot be set to '*'.") | ||
| } else if allowOrigin != "" { | ||
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | ||
| c.Set(fiber.HeaderAccessControlAllowCredentials, "true") | ||
| } | ||
| } else if allowOrigin != "" { | ||
| // For non-credential requests, it's safe to set to '*' or specific origins | ||
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | ||
| } |
There was a problem hiding this comment.
When AllowCredentials is true, the logic to set Access-Control-Allow-Origin to the specific origin instead of '*' is correctly implemented. This is important for security, as it prevents credentials from being shared across origins indiscriminately. However, the warning log on line 230 might not be necessary since the condition is already enforced by a panic earlier in the code. Consider removing the redundant warning.
- log.Warn("[CORS] 'AllowCredentials' is true, but 'AllowOrigins' cannot be set to '*'.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if cfg.AllowCredentials { | |
| // When AllowCredentials is true, set the Access-Control-Allow-Origin to the specific origin instead of '*' | |
| if allowOrigin == "*" { | |
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | |
| log.Warn("[CORS] 'AllowCredentials' is true, but 'AllowOrigins' cannot be set to '*'.") | |
| } else if allowOrigin != "" { | |
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | |
| c.Set(fiber.HeaderAccessControlAllowCredentials, "true") | |
| } | |
| } else if allowOrigin != "" { | |
| // For non-credential requests, it's safe to set to '*' or specific origins | |
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | |
| } | |
| if cfg.AllowCredentials { | |
| // When AllowCredentials is true, set the Access-Control-Allow-Origin to the specific origin instead of '*' | |
| if allowOrigin == "*" { | |
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | |
| } else if allowOrigin != "" { | |
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | |
| c.Set(fiber.HeaderAccessControlAllowCredentials, "true") | |
| } | |
| } else if allowOrigin != "" { | |
| // For non-credential requests, it's safe to set to '*' or specific origins | |
| c.Set(fiber.HeaderAccessControlAllowOrigin, allowOrigin) | |
| } |
| } else { | ||
| h := c.Get(fiber.HeaderAccessControlRequestHeaders) | ||
| if h != "" { | ||
| c.Set(fiber.HeaderAccessControlAllowHeaders, h) |
There was a problem hiding this comment.
The added line #L251 was not covered by tests. This line sets the Access-Control-Allow-Headers based on the request headers if not explicitly defined. It's crucial to ensure this behavior is tested to verify correct CORS header handling.
Please ensure that test coverage is extended to include scenarios where Access-Control-Allow-Headers is dynamically set based on the request headers.
Merges CORS middleware changes from v2 branch to main
Pending merge of #2921
Summary by CodeRabbit
Summary by CodeRabbit