feat(middleware/csrf): Add support for trusted origins #2910
Conversation
sixcolors
requested review from
gaby,
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 18 minutes and 28 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 updates focus on enhancing the CSRF Middleware in GoFiber by introducing comprehensive documentation and refining the middleware's logic. The documentation now thoroughly explains the CSRF protection mechanism, including token management and security practices. The logic update adds an origin header check to complement the referer header verification for better security, alongside improved error handling. Additionally, the testing suite has been expanded to cover a wider range of scenarios, ensuring robust validation and trusted origin checks. 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 (
|
sixcolors
requested review from
nickajacks1
and removed request for
gaby,
ReneWerner87 and
efectn
![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: 12
Configuration used: CodeRabbit UI
Files selected for processing (4)
- docs/api/middleware/csrf.md (5 hunks)
- middleware/csrf/config.go (1 hunks)
- middleware/csrf/csrf.go (5 hunks)
- middleware/csrf/csrf_test.go (4 hunks)
Additional comments: 16
middleware/csrf/config.go (1)
- 92-103: The addition of the
TrustedOriginsfield to theConfigstruct is a welcome enhancement for specifying trusted origins for unsafe requests. The documentation is clear, but it might be beneficial to include an example in the comments to illustrate how to specify trusted origins, especially the use of wildcard subdomains.middleware/csrf/csrf.go (3)
- 88-104: The implementation of origin and referer header checks for unsafe requests is a significant security enhancement. Consider refining the error messages associated with
ErrBadOriginanderrNoOriginto provide more guidance to developers on how to resolve these errors.- 297-332: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [260-332]
The implementation of
originMatchesHostandrefererMatchesHostfunctions for validating the origin and referer headers is robust. It would be beneficial to add more comments or documentation, especially regarding the wildcard subdomain matching inisSameSchemeAndDomain, to help developers understand how it works.
- 18-20: The introduction of new error types such as
ErrBadOriginanderrNoOriginenhances the clarity and specificity of error handling in the CSRF middleware. Ensure that these errors are well-documented and that guidance is provided on how to resolve them.docs/api/middleware/csrf.md (9)
- 11-11: The term "Referer" is technically correct in the context of HTTP headers, but it's commonly misspelled. Consider adding a note to clarify this is intentional and aligns with the HTTP standard, to avoid confusion.
- 10-25: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [43-43]
The term "KeyLookup" is correctly used in the context of configuration. However, consider explaining its purpose more clearly for readers unfamiliar with the concept.
- 100-100: There are unnecessary repeated whitespaces in the documentation. Consider removing them to improve readability and adhere to markdown best practices.
- 109-122: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [102-119]
The configuration section is well-detailed but consider adding brief explanations or examples for some of the more complex properties like
KeyLookup,Extractor, andTrustedOriginsto enhance understanding for readers less familiar with CSRF concepts.
- 236-236: Clarify the use of
CookieSameSiteandExtractoroptions in the context of the Double Submit Cookie Pattern to ensure readers understand the security implications.- 255-255: Clarify the security benefits of setting
CookieSecureandCookieSameSiteoptions, possibly with examples or scenarios where these settings are particularly important.- 260-260: Explain the use of cookie prefixes
__Host-and__Secure-in more detail, including browser support and limitations, to help readers make informed decisions.- 263-263: The term "Referer" is technically correct in the context of HTTP headers, but it's commonly misspelled. Consider adding a note to clarify this is intentional and aligns with the HTTP standard, to avoid confusion.
- 280-280: Clarify the security implications and use cases for the
SingleUseTokenoption to help readers understand when it might be beneficial to enable this feature.middleware/csrf/csrf_test.go (3)
- 601-688: In the
Test_CSRF_Origintest function, there are several scenarios tested for CSRF origin validation, including correct origins with ports, reverse proxies, missing headers, paths, and wrong origins. This comprehensive testing ensures the robustness of origin validation in CSRF protection. However, there's a potential improvement in the readability and maintainability of the test cases by abstracting common setup steps into helper functions or using table-driven tests.Consider refactoring the test to use table-driven tests or helper functions to reduce code duplication and improve maintainability.
- 690-802: The
Test_CSRF_TrustedOriginsfunction effectively tests the new trusted origins feature, covering trusted origins, subdomains, invalid origins, and referer headers. This thorough testing ensures the feature's functionality and security. However, similar to the previous comment, the readability and maintainability of these tests could be improved.To enhance code maintainability, consider adopting table-driven tests or helper functions for common setup steps, which can make the tests more concise and easier to extend in the future.
- 823-829: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [803-859]
In the
Test_CSRF_Refererfunction, various scenarios are tested for CSRF referer validation, including correct referers with ports, reverse proxies, missing headers, and paths. This ensures the CSRF middleware correctly handles referer-based validation. However, there's a repeated pattern of resetting the request and response contexts and setting headers that could be abstracted.Abstracting repetitive setup steps into helper functions or using table-driven tests could improve the test code's readability and maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (2)
- docs/api/middleware/csrf.md (5 hunks)
- middleware/csrf/csrf.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/csrf/csrf.go
Additional comments: 1
docs/api/middleware/csrf.md (1)
- 87-87: The sentence does not start with an uppercase letter. In Go, function declarations should start with the lowercase keyword
func. However, this hint seems to be a false positive as it's referring to code syntax, not prose.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2910 +/- ##
==========================================
+ Coverage 82.52% 82.73% +0.20%
==========================================
Files 116 116
Lines 8333 8368 +35
==========================================
+ Hits 6877 6923 +46
+ Misses 1111 1103 -8
+ Partials 345 342 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
expand tests to check invalid urls in headers
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (3)
- docs/api/middleware/csrf.md (6 hunks)
- middleware/csrf/csrf.go (5 hunks)
- middleware/csrf/csrf_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- middleware/csrf/csrf.go
- middleware/csrf/csrf_test.go
Additional comments: 2
docs/api/middleware/csrf.md (2)
- 11-11: The term "Referer" is correctly used in the context of HTTP headers, despite the common misspelling of "referrer". This aligns with RFC 9110 and other HTTP specifications.
- 87-87: The correction to capitalize "func" was incorrect as "func" is a keyword in Go and should remain lowercase.
test(middleware/csrf): improve coverage
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (3)
- docs/api/middleware/csrf.md (6 hunks)
- middleware/csrf/csrf.go (3 hunks)
- middleware/csrf/csrf_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/csrf/csrf.go
Additional comments: 6
docs/api/middleware/csrf.md (2)
- 11-11: The term "Referer" is correctly used in the context of HTTP headers, as per RFC 9110. No change is needed here.
- 87-87: The correction to capitalize "func" was incorrect as "func" should remain lowercase in Go function declarations.
middleware/csrf/csrf_test.go (4)
- 601-642: The test
Test_CSRF_Extractor_EmptyStringeffectively validates the CSRF middleware's behavior when the custom extractor returns an empty string, ensuring the middleware correctly handles custom token extraction logic. This is a crucial test for the robustness of the CSRF protection mechanism.- 645-760: The test
Test_CSRF_Origincomprehensively covers various scenarios for origin header validation, including correct and incorrect origins, ports, reverse proxy setups, and null origins. This thorough testing is essential for ensuring the CSRF middleware's robustness in handling origin validation.- 762-874: The test
Test_CSRF_TrustedOriginseffectively validates the new trusted origins feature, covering scenarios with valid and invalid origins, including subdomains and referer headers. This comprehensive testing ensures the CSRF middleware correctly handles trusted origins, enhancing security and flexibility.- 1322-1375: The test
Test_CSRF_InvalidURLHeaderseffectively validates the CSRF middleware's behavior when encountering invalid URL headers in the Origin and Referer headers. This test is crucial for ensuring the middleware's resilience against malformed or malicious URL headers, enhancing security.
|
@ReneWerner87 @gaby I improved test coverage, added a check for "null" for origin header, and re-named the Sentinel Errors to make them more consistant. Request re-review. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- docs/api/middleware/csrf.md (6 hunks)
Additional comments: 23
docs/api/middleware/csrf.md (23)
- 11-11: The term "Referer" is correctly used in the context of web standards and HTTP specifications, despite being a common misspelling of "referrer". This aligns with RFC 9110 and other HTTP documentation.
- 17-17: Remove unnecessary spaces after the opening parenthesis in code blocks to adhere to Go formatting standards.
- import ( + import (This change suggestion is incorrect as the diff does not show any actual change. Please disregard this suggestion.
- 10-25: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [43-43]
The term "KeyLookup" is correctly used in the context of this documentation and does not require a space between "Key" and "Lookup".
- 81-81: The term "GoFiber" is correctly used as the name of the framework and does not require a space between "Go" and "Fiber".
- 87-87: In Go, function declarations should start with the lowercase keyword
func. The suggestion to capitalize it was incorrect.- 102-102: Add a space between sentences for better readability in the documentation.
- Next defines a function to skip this middleware when returned true. + Next defines a function to skip this middleware when returned true.This change suggestion is incorrect as it mistakenly focuses on adding a space at the end of the sentence, which doesn't visibly change the formatting. Please disregard this suggestion.
- 103-103: The term "KeyLookup" is correctly used in the context of this documentation and does not require a space between "Key" and "Lookup".
- 104-104: The term "csrf" in "CookieName" is correctly used in lowercase as it represents a cookie name, which typically does not follow the same capitalization rules as acronyms in prose.
- 105-105: The term "CookieDomain" is correctly used in the context of this documentation and does not require a space between "Cookie" and "Domain".
- 10-25: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [106-106]
The term "CookiePath" is correctly used in the context of this documentation and does not require a space between "Cookie" and "Path".
- 10-25: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [107-107]
The term "CookieSecure" is correctly used in the context of this documentation and does not require a space between "Cookie" and "Secure".
- 10-25: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [108-108]
The term "CookieHTTPOnly" is correctly used in the context of this documentation and does not require a space between "Cookie" and "HTTPOnly".
- 109-109: The term "CookieSameSite" is correctly used in the context of this documentation and does not require a space between "Cookie" and "SameSite".
- 110-110: The term "CookieSessionOnly" is correctly used in the context of this documentation and does not require a space between "Cookie" and "SessionOnly".
- 112-112: The term "KeyGenerator" is correctly used in the context of this documentation and does not require a space between "Key" and "Generator".
- 113-113: The term "ErrorHandler" is correctly used in the context of this documentation and does not require a space between "Error" and "Handler".
- 114-114: The term "Extractor" is correctly used in the context of this documentation and does not require a space between "Ex" and "tractor".
- 115-115: The term "SingleUseToken" is correctly used in the context of this documentation and does not require a space between "Single", "Use", and "Token".
- 116-116: The term "Storage" is correctly used in the context of this documentation and does not require a space between "Stor" and "age".
- 117-117: The term "Session" is correctly used in the context of this documentation and does not require a space between "Ses" and "sion".
- 118-118: The term "SessionKey" is correctly used in the context of this documentation and does not require a space between "Session" and "Key".
- 119-119: The term "TrustedOrigins" is correctly used in the context of this documentation and does not require a space between "Trusted" and "Origins".
- 173-173: The term "Referer" is correctly used in the context of web standards and HTTP specifications, despite being a common misspelling of "referrer". This aligns with RFC 9110 and other HTTP documentation.
* feat(middleware/csrf): Add support for trusted origins in CSRF middleware * fix(middleware/csrf): lint errors * docs(middleware/csrf): following the ai * fix(middleware/csrf): isSameSchemeAndDomain * fix(middleware/csrf): null origin expand tests to check invalid urls in headers * chore(middleware/csrf): Sentinel Errors test(middleware/csrf): improve coverage * docs: add extra space between sentences. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore(middleware/csrf): remove trailing newline in csrf_test.go --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
Adds support for
TrustedOrigins, refinies origin and referer header checks for better security. Makes updates to enhance the documentation.Related to #2909
Type of change
Please delete options that are not relevant.
Summary by CodeRabbit