feat(middleware/csrf): TrustedOrigins using https://*.example.com style subdomains #2925
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
sixcolors
requested review from
gaby,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe CSRF middleware has been updated to enhance security and flexibility by introducing wildcard support for subdomain matching in the 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2925 +/- ##
==========================================
+ Coverage 82.75% 82.76% +0.01%
==========================================
Files 116 116
Lines 8396 8415 +19
==========================================
+ Hits 6948 6965 +17
- Misses 1108 1110 +2
Partials 340 340
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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/csrf.md (3 hunks)
- middleware/csrf/csrf.go (9 hunks)
- middleware/csrf/csrf_test.go (5 hunks)
- middleware/csrf/helpers.go (2 hunks)
- middleware/csrf/helpers_test.go (1 hunks)
Additional comments: 8
middleware/csrf/helpers.go (1)
- 17-48: Consider adding more detailed comments within the
normalizeOriginfunction to explain the rationale behind each validation check. This will improve maintainability and understanding for future contributors.middleware/csrf/helpers_test.go (1)
- 115-130: The benchmark test for
subdomain.matchis well-implemented and provides valuable insights into the method's performance. It's a good practice to assess the performance impact of changes, such as making the matching case-insensitive.middleware/csrf/csrf.go (2)
- 28-28: Changing the
configfield in theHandlerstruct from a pointer to a value type is a good practice, as it ensures the configuration's immutability once the handler is created. This prevents accidental modifications and enhances safety.- 60-81: The updates to the initialization of
trustedOriginsandtrustedSubOriginsto support wildcard subdomain matching are well-implemented. This enhancement significantly improves the flexibility and security of the CSRF middleware by allowing more granular control over trusted origins.docs/api/middleware/csrf.md (3)
- 119-119: The documentation update to specify the use of wildcard characters (e.g.,
https://*.example.com) for subdomain matching in theTrustedOriginsconfiguration is clear and correctly reflects the new functionality. This change allows for a more flexible and convenient way to specify trusted origins, especially for applications that operate across multiple subdomains.- 159-159: The explanation of the
TrustedOriginsoption and its support for matching subdomains at any level is well-documented. The inclusion of an example using"https://*.example.com"to allow any subdomain ofexample.comto submit requests enhances the clarity of this feature. This section effectively communicates the new capability and its potential use cases.- 182-182: The caution note regarding the use of
TrustedOriginswith subdomain matching is an important inclusion. It rightly emphasizes the security considerations that users must be aware of when configuring trusted origins with wildcard subdomains. This helps ensure that users understand the implications and use the feature securely.middleware/csrf/csrf_test.go (1)
- 999-1010: The logic for deleting the CSRF token and removing the cookie in
Test_CSRF_DeleteTokenis a valuable addition. It's important to ensure that the CSRF protection mechanism can handle token deletion securely and correctly. This test case helps verify that behavior. However, consider adding a comment explaining the purpose of deleting all cookies in line 1005 and its relevance to CSRF protection, as it might not be immediately clear to future maintainers.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…iber into csrf-subdomain-style
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (2)
- middleware/csrf/csrf.go (8 hunks)
- middleware/csrf/csrf_test.go (5 hunks)
Additional comments: 2
middleware/csrf/csrf.go (2)
- 27-27: The change from a pointer to a value type for the
configfield in theHandlerstruct is a significant alteration. This change ensures that theconfigis not nil, potentially improving the safety and reliability of the middleware. However, it's important to consider the impact on memory usage, especially if theConfigstruct is large or contains many fields. In most cases, the benefits of avoiding nil pointer dereferences outweigh the slight increase in memory usage.- 117-122: The implementation of origin and referer matching functions to handle subdomains is a key enhancement. It correctly enforces origin checks for unsafe requests and falls back to referer checks for HTTPS connections when the origin is not available. This approach aligns with security best practices by prioritizing the Origin header over the Referer header due to the latter's less reliable nature. The logic appears sound and well-integrated with the existing CSRF protection mechanisms.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- docs/api/middleware/csrf.md (3 hunks)
- middleware/csrf/csrf_test.go (7 hunks)
- middleware/csrf/helpers_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- middleware/csrf/csrf_test.go
- middleware/csrf/helpers_test.go
Additional comments: 3
docs/api/middleware/csrf.md (3)
- 119-119: The update to the
TrustedOriginsconfiguration description to include wildcard subdomain matching (https://*.example.com) is clear and accurately reflects the new functionality. This change enhances the flexibility and usability of the CSRF middleware by allowing developers to specify a range of trusted subdomains in a concise manner.- 159-161: The explanation of the
TrustedOriginsoption and its ability to match subdomains at any level is well-detailed and informative. It correctly emphasizes the utility of the feature in allowing requests from any subdomain of a specified domain, enhancing the middleware's flexibility for applications with multiple subdomains.- 183-185: The caution note regarding the use of
TrustedOriginswith subdomain matching is crucial for security. It rightly warns users to ensure control and trust over all subdomains, including all subdomain levels, to prevent potential misuse by attackers. This is an important security consideration that helps guide users in safely utilizing the new feature.
ReneWerner87
approved these changes
Mar 24, 2024
I'd like to note that the benchmark Benchmark_Middleware_CSRF_Check is testing a best-case scenario for strings.ToLower(), where the Referer header is already in lowercase and does not contain any non-ASCII characters, which is typically the general case for web browsers as clients. Also, I found a bug in the CSRF benchmark, and I'll push a fix shortly. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- middleware/csrf/csrf_test.go (11 hunks)
Additional comments: 7
middleware/csrf/csrf_test.go (7)
- 758-759: The addition of wildcard domain support in
Test_CSRF_TrustedOriginsis a significant enhancement. It's crucial to ensure that the implementation correctly handles various subdomain patterns and edge cases. Consider adding more comprehensive tests to cover scenarios such as deeply nested subdomains (e.g.,https://a.b.c.domain-1.com) and subdomains with special characters or hyphens to ensure robustness.- 892-892: The
Test_CSRF_TrustedOrigins_InvalidOriginstest function correctly identifies invalid origin formats. However, it's recommended to callt.Parallel()at the beginning of each subtest to allow parallel execution and improve test suite performance. This change can significantly reduce the overall test execution time, especially as the number of tests grows.- 1030-1041: Adding logic for deleting the CSRF token and removing the cookie in
Test_CSRF_DeleteTokenis a good practice to ensure the middleware behaves correctly in scenarios where the token needs to be invalidated. This addition enhances the test coverage for token management within the CSRF middleware.- 1343-1369: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1333-1366]
The benchmark test
Benchmark_Middleware_CSRF_Checkis well-structured and provides a good measure of the middleware's performance under load. However, ensure that the CSRF token generation and validation logic are optimized to minimize performance overhead, especially in high-traffic applications.
- 1387-1391: The benchmark test
Benchmark_Middleware_CSRF_GenerateTokenfocuses on measuring the performance of CSRF token generation. It's important to ensure that the token generation process is both secure and efficient, as it directly impacts the user experience and the application's security posture.- 1392-1392: The test
Test_CSRF_InvalidURLHeadersis crucial for ensuring that the CSRF middleware correctly handles invalid Origin and Referer headers, preventing potential security vulnerabilities. It's good practice to include such tests to cover edge cases and malformed inputs.- 1393-1393: The tests
Test_CSRF_TokenFromContext,Test_CSRF_FromContextMethods, andTest_CSRF_FromContextMethods_Invalidare valuable for verifying the utility functions related to CSRF token management within the application context. These tests ensure that the middleware's API is reliable and behaves as expected in various scenarios.
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.
PR Description:
This PR updates the
TrustedOriginssubdomain matching style to match the changes made in the CORS middleware. Here's a summary of the changes:Changes Made:
docs/api/middleware/csrf.mdto reflect the new subdomain matching style forTrustedOrigins.TrustedOriginssection to include examples and cautionary notes regarding subdomain matching.csrf.gofile to parse and handleTrustedOriginswith subdomain matching.helpers.gofile containing helper functions for normalizing origins and matching subdomains.csrf_test.goand added new tests inhelpers_test.goto ensure the correct behavior of subdomain matching.Notes for Review:
docs/api/middleware/csrf.mdto ensure clarity and correctness.csrf.gofile to confirm that the handling ofTrustedOriginswith subdomain matching aligns with our requirements.csrf_test.gocover various scenarios related to subdomain matching and ensure they provide sufficient coverage.helpers.gofile and ensure it's acceptable.Kindly review and provide feedback. Thank you!
Summary by CodeRabbit
TrustedOriginsconfiguration in CSRF middleware to support subdomain matching with wildcard characters. Added examples and security guidance in the documentation.