feat(middleware/cors): Add support for Access-Control-Allow-Private-Network #2908
Conversation
Behzad-Khokher
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Warning Rate Limit Exceeded@sixcolors has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 19 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe update introduces a new configuration option, Changes
Related 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 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 (4)
- docs/api/middleware/cors.md (2 hunks)
- helpers.go (1 hunks)
- middleware/cors/cors.go (2 hunks)
- middleware/cors/cors_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- helpers.go
Additional comments: 5
docs/api/middleware/cors.md (2)
- 83-83: The addition of the
AllowPrivateNetworkAccessconfiguration option is well-documented, providing clear guidance on its purpose and usage. This aligns with the PR's objective to enhance CORS middleware by supporting theAccess-Control-Allow-Private-Networkheader.- 105-105: The documentation correctly reflects the default setting for
AllowPrivateNetworkAccessin theConfigDefaultstruct. This ensures developers are aware that the feature is disabled by default, promoting conscious activation of the feature.middleware/cors/cors.go (2)
- 66-71: The addition of the
AllowPrivateNetworkAccessfield to theConfigstruct is correctly implemented. The comment clearly explains the purpose of the field and its default behavior, which is essential for understanding how to use this new feature.- 236-240: The logic to conditionally set the
Access-Control-Allow-Private-Networkheader based on theAllowPrivateNetworkAccessconfiguration is correctly implemented. This ensures that the header is only added when explicitly enabled, adhering to the principle of secure defaults.middleware/cors/cors_test.go (1)
- 692-727: The implementation of
Test_CORS_AllowPrivateNetworkAccesscorrectly tests the newAllowPrivateNetworkAccessfeature by verifying the presence or absence of theAccess-Control-Allow-Private-Networkheader based on the configuration. The test is well-structured, with clear separation between the scenario whereAllowPrivateNetworkAccessis enabled and the default scenario where it is disabled. This ensures comprehensive coverage of the new feature's behavior.However, there are a few areas for potential improvement:
- Test Naming Convention: The test name is descriptive but could be more specific about the scenarios it covers. Consider splitting it into two separate tests or renaming it to reflect both scenarios being tested (enabled and disabled states).
- Code Duplication: There's some repetition in setting up the test contexts (
fasthttp.RequestCtx). Extracting this setup into a helper function could reduce duplication and improve maintainability.- Asserting Absence of Header: The test asserts the absence of the
Access-Control-Allow-Private-Networkheader by checking for an empty string. While this is effective, using a more explicit assertion to check for the header's absence could improve readability and intent clarity.Overall, the test function effectively validates the new feature, ensuring that the
Access-Control-Allow-Private-Networkheader is correctly handled based on the middleware configuration. These suggestions aim to refine the test for better maintainability and clarity.Consider refactoring the test to reduce code duplication and improve clarity in asserting the header's absence. Splitting the test or renaming it for specificity could also enhance readability.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2908 +/- ##
==========================================
+ Coverage 82.65% 82.68% +0.02%
==========================================
Files 116 116
Lines 8385 8396 +11
==========================================
+ Hits 6931 6942 +11
Misses 1112 1112
Partials 342 342
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
thx |
There was a problem hiding this comment.
This is still a draft. It’s at the proposal stage and has not been accepted as a standard. Recommend we do not accept at this time.
https://wicg.github.io/private-network-access/
Status of this document
This specification was published by the Web Platform Incubator Community Group. It is not a W3C Standard nor is it on the W3C Standards Track. Please note that under the W3C Community Contributor License Agreement (CLA) there is a limited opt-out and other conditions apply. Learn more about W3C Community and Business Groups.
sixcolors
changed the title
|
@gofiber/maintainers do we want to close the pull request and open it again later or how do we want to proceed here? |
It seems Google has already roll-out this feature since Chrome 104. Several projects have already merged support for it Examples:
I vote for merging it |
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 (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- middleware/cors/cors.go
- middleware/cors/cors_test.go
Okay thanks, will wrap this up today. |
Don't worry, I already did them. :-) Just pending review from @gofiber/maintainers |
Awsome! Thanks :) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- docs/api/middleware/cors.md (2 hunks)
- helpers.go (1 hunks)
- middleware/cors/cors.go (3 hunks)
- middleware/cors/cors_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- helpers.go
- middleware/cors/cors.go
- middleware/cors/cors_test.go
Additional comments: 1
docs/api/middleware/cors.md (1)
- 121-129: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [38-38]
Consider adding a space after the period to improve readability in the documentation.
There was a problem hiding this comment.
Preflight requires the method OPTIONS along with Access-Control-Request-Method. Tests made incorrect assumptions.
fiber.HeaderAccessControlRequestPrivateNetwork should be defined as Access-Control-Request-Private-Network.
Preflight should Vary Access-Control-Request-Private-Network.
Tests will pass and CORS will function correctly after you commit the suggestions below:
|
CORS.md ## Config
| Property | Type | Description | Default |
|:--------------------|:---------------------------|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-----------------------------------|
| Next | `func(fiber.Ctx) bool` | Next defines a function to skip this middleware when returned true. | `nil` |
| AllowOriginsFunc | `func(origin string) bool` | `AllowOriginsFunc` is a function that dynamically determines whether to allow a request based on its origin. If this function returns `true`, the 'Access-Control-Allow-Origin' response header will be set to the request's 'origin' header. This function is only used if the request's origin doesn't match any origin in `AllowOrigins`. | `nil` |
| AllowOrigins | `string` | AllowOrigins defines a comma-separated list of origins that may access the resource. This supports subdomain matching, so you can use a value like "https://*.example.com" to allow any subdomain of example.com to submit requests. | `"*"` |
| AllowMethods | `string` | AllowMethods defines a list of methods allowed when accessing the resource. This is used in response to a preflight request. | `"GET,POST,HEAD,PUT,DELETE,PATCH"` |
| AllowHeaders | `string` | AllowHeaders defines a list of request headers that can be used when making the actual request. This is in response to a preflight request. | `""` |
| AllowCredentials | `bool` | AllowCredentials indicates whether or not the response to the request can be exposed when the credentials flag is true. When used as part of a response to a preflight request, this indicates whether or not the actual request can be made using credentials. Note: If true, AllowOrigins cannot be set to a wildcard (`"*"`) to prevent security vulnerabilities. | `false` |
| ExposeHeaders | `string` | ExposeHeaders defines whitelist headers that clients are allowed to access. | `""` |
| MaxAge | `int` | MaxAge indicates how long (in seconds) the results of a preflight request can be cached. If you pass MaxAge 0, the Access-Control-Max-Age header will not be added and the browser will use 5 seconds by default. To disable caching completely, pass MaxAge value negative. It will set the Access-Control-Max-Age header to 0. | `0` |
| AllowPrivateNetwork | `bool` | Indicates whether the `Access-Control-Allow-Private-Network` response header should be set to `true`, allowing requests from private networks. This aligns with modern security practices for web applications interacting with private networks. | `false` |I can't seem to make a committable suggestion since the alignment changes lines which are not in your PR. |
|
Wouldn’t hurt to order the table alphabetically either. ## Config
| Property | Type | Description | Default |
|:---------------------|:----------------------------|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-----------------------------------|
| AllowCredentials | `bool` | AllowCredentials indicates whether or not the response to the request can be exposed when the credentials flag is true. When used as part of a response to a preflight request, this indicates whether or not the actual request can be made using credentials. Note: If true, AllowOrigins cannot be set to a wildcard (`"*"`) to prevent security vulnerabilities. | `false` |
| AllowHeaders | `string` | AllowHeaders defines a list of request headers that can be used when making the actual request. This is in response to a preflight request. | `""` |
| AllowMethods | `string` | AllowMethods defines a list of methods allowed when accessing the resource. This is used in response to a preflight request. | `"GET,POST,HEAD,PUT,DELETE,PATCH"` |
| AllowOrigins | `string` | AllowOrigins defines a comma-separated list of origins that may access the resource. This supports subdomain matching, so you can use a value like "https://*.example.com" to allow any subdomain of example.com to submit requests. | `"*"` |
| AllowOriginsFunc | `func(origin string) bool` | `AllowOriginsFunc` is a function that dynamically determines whether to allow a request based on its origin. If this function returns `true`, the 'Access-Control-Allow-Origin' response header will be set to the request's 'origin' header. This function is only used if the request's origin doesn't match any origin in `AllowOrigins`. | `nil` |
| AllowPrivateNetwork | `bool` | Indicates whether the `Access-Control-Allow-Private-Network` response header should be set to `true`, allowing requests from private networks. This aligns with modern security practices for web applications interacting with private networks. | `false` |
| ExposeHeaders | `string` | ExposeHeaders defines whitelist headers that clients are allowed to access. | `""` |
| MaxAge | `int` | MaxAge indicates how long (in seconds) the results of a preflight request can be cached. If you pass MaxAge 0, the Access-Control-Max-Age header will not be added and the browser will use 5 seconds by default. To disable caching completely, pass MaxAge value negative. It will set the Access-Control-Max-Age header to 0. | `0` |
| Next | `func(fiber.Ctx) bool` | Next defines a function to skip this middleware when returned true. | `nil` | |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- middleware/cors/cors.go (3 hunks)
Additional comments: 1
middleware/cors/cors.go (1)
- 66-71: The addition of
AllowPrivateNetworkto theConfigstruct is well-implemented, with a safe default value offalse. This aligns with the existing configuration structure and is a necessary addition for the new feature.
|
@ReneWerner87 @efectn This is ready for review. @sixcolors Are you backporting this to v2 after merge? Given the other changes to CORS in that branch. |
I can do that. |
|
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
This pull request introduces support for the Access-Control-Allow-Private-Network header in the GoFiber CORS middleware. This addition allows Fiber applications to explicitly grant web applications access to resources on a user's private network as requested by @GalvinGao .
Related to #2804
Changes introduced
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Summary by CodeRabbit
New Features
AllowPrivateNetworkfor CORS middleware, enabling support for theAccess-Control-Allow-Private-Networkheader in requests from private networks.Documentation
AllowPrivateNetworkconfiguration option.Tests
AllowPrivateNetworkfunctionality in the CORS middleware.