✨ feat: Add support for CookieConfig in Redirect #3076
Conversation
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe updates streamline the redirect functionality in Fiber, introduce improved cookie configuration, and refine documentation for ease of understanding. Major modifications include adding a Changes
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3076 +/- ##
==========================================
- Coverage 83.10% 83.08% -0.03%
==========================================
Files 115 115
Lines 8321 8334 +13
==========================================
+ Hits 6915 6924 +9
- Misses 1076 1079 +3
- Partials 330 331 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
gaby
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
Makefile (1)
32-32: Install and configuremarkdownlint-cli2The
Makefileincludes a target that usesmarkdownlint-cli2, but there is no evidence of its installation or configuration in the repository. To avoid build failures, please ensure thatmarkdownlint-cli2is installed and configured correctly.
- Add
markdownlint-cli2installation steps, for example, in theMakefileor a setup script.- Create a
.markdownlint.jsonconfiguration file if needed.Analysis chain
Verify the installation and configuration of
markdownlint-cli2.Ensure that
markdownlint-cli2is installed and configured correctly in the development environment to avoid build failures.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if markdownlint-cli2 is installed and configured correctly. # Test: Search for markdownlint-cli2 installation and configuration. which markdownlint-cli2 cat .markdownlint.jsonLength of output: 174
Script:
#!/bin/bash # Description: Check if markdownlint-cli2 is listed as a dependency in package.json and search for markdownlint configuration files. # Search for markdownlint-cli2 in package.json rg "markdownlint-cli2" package.json # Search for markdownlint configuration files fd --type f --name "*.markdownlint*"Length of output: 378
Script:
#!/bin/bash # Description: Search for markdownlint configuration files and check for markdownlint-cli2 in Makefile or shell script files. # Corrected search for markdownlint configuration files fd --type f ".markdownlint" # Search for markdownlint-cli2 in Makefile and shell script files rg "markdownlint-cli2" Makefile fd --type f -e sh | xargs rg "markdownlint-cli2"Length of output: 195
Script:
#!/bin/bash # Description: Search for any scripts or methods that install markdownlint-cli2. # Search for installation commands in Makefile and shell script files rg "install" Makefile fd --type f -e sh | xargs rg "install"Length of output: 523
docs/api/redirect.md (1)
15-15: Documentation updates are clear and align with code changes.The updates in the documentation are consistent with the new functionalities introduced in the code, such as the
CookieConfigstruct and its usage.Consider simplifying the phrase 'in conjunction with'.
The phrase "in conjunction with" could be simplified to "with" to enhance readability.
- This method is used in conjunction with [**To**](#to), [**Route**](#route) and [**Back**](#back) methods. + This method is used with [**To**](#to), [**Route**](#route) and [**Back**](#back) methods.Also applies to: 18-18, 51-51, 54-54, 92-92, 95-95, 127-127, 143-143, 154-154, 158-158, 160-160, 162-162, 164-164, 166-166, 167-167, 168-168, 186-186, 190-190, 198-198, 205-205, 213-213, 220-220, 228-228, 235-235, 243-243, 250-250, 258-258, 262-262, 269-269, 278-278, 282-282
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- Makefile (1 hunks)
- docs/api/redirect.md (7 hunks)
- docs/whats_new.md (8 hunks)
- redirect.go (9 hunks)
- redirect_test.go (4 hunks)
- router.go (1 hunks)
Files skipped from review due to trivial changes (1)
- router.go
Additional context used
Additional comments not posted (6)
Makefile (1)
32-32: Change of target fromformattomarkdownis appropriate.The change aligns with the PR's objectives to enhance the documentation and markdown linting capabilities.
redirect.go (1)
27-33: Structural and functional changes inredirect.goare well-implemented.The additions and modifications in the
RedirectandCookieConfigstructs are consistent with the PR's objectives to enhance cookie handling in redirects.Ensure secure cookie settings when
SameSite=None.The implementation correctly sets the
Secureflag whenSameSiteis set toNone. This is crucial for preventing security issues related to cookies.Also applies to: 44-48, 53-58, 60-65, 75-75, 208-208, 214-216, 218-220, 222-224, 226-231, 233-235, 283-283, 296-296, 328-333
docs/whats_new.md (1)
245-257: Enhancements to the Redirect feature are well-documented.The documentation clearly outlines the new capabilities related to flash messages, old input data, and customizable HTTP status codes, providing a comprehensive overview of the improvements.
Verify the backend implementation for cookie storage.
Ensure that the backend implementation correctly handles cookie storage as described, particularly the use of
CookieConfigin theRedirectConfigstruct.Also applies to: 259-259
Verification successful
Backend implementation for cookie storage is correctly handled.
The
redirect.gofile contains the necessary implementations forCookieConfigwithin theRedirectConfigstruct, confirming that the backend handles cookie storage as described in the documentation.
redirect.go: Multiple references toCookieConfigincluding default configuration, struct definition, and various assignments and checks.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the backend implementation for cookie storage. # Test: Search for the implementation of CookieConfig in RedirectConfig. rg --type go 'CookieConfig' 'redirect.go'Length of output: 895
redirect_test.go (3)
186-186: Duplicate issue: Ensure consistent cookie string representation in assertions.This is a repeat of the previous issue where the test checks for two possible string representations of the cookie. As mentioned earlier, consider a robust method for parsing and asserting cookie properties to avoid flakiness.
229-229: Duplicate issue: Ensure consistent cookie string representation in assertions.Again, this test faces the same issue with potential flakiness due to checking two possible string representations of the cookie. It's crucial to address this across all tests to ensure reliability.
245-302: Review comprehensive test cases for differentCookieConfigsettings.The test suite here is well-structured and covers a variety of scenarios for the
CookieConfig, which is crucial for ensuring that all configurations behave as expected. However, ensure that the test names and expected values accurately reflect the intended cookie settings, especially regarding security attributes likeHttpOnlyandSecure.
| @@ -45,7 +45,7 @@ func Test_Redirect_To_WithFlashMessages(t *testing.T) { | |||
| require.Equal(t, 302, c.Response().StatusCode()) | |||
| require.Equal(t, "http://example.com", string(c.Response().Header.Peek(HeaderLocation))) | |||
|
|
|||
| equal := c.GetRespHeader(HeaderSetCookie) == "fiber_flash=success:1,message:test; path=/; SameSite=Lax" || c.GetRespHeader(HeaderSetCookie) == "fiber_flash=message:test,success:1; path=/; SameSite=Lax" | |||
| equal := c.GetRespHeader(HeaderSetCookie) == "fiber_flash=success:1,message:test; path=/; HttpOnly; SameSite=Lax" || c.GetRespHeader(HeaderSetCookie) == "fiber_flash=message:test,success:1; path=/; HttpOnly; SameSite=Lax" | |||
There was a problem hiding this comment.
Ensure consistent cookie string representation in assertions.
The test checks for two possible string representations of the cookie, which could lead to flakiness if the order of the cookie attributes isn't guaranteed by the implementation. Consider using a more robust method to parse and assert the properties of the cookie.
| cfg := RedirectConfig{CookieConfig: CookieConfigDefault} | ||
|
|
||
| if len(config) > 0 { | ||
| cfg = config[0] | ||
| } | ||
|
|
||
| if cfg.CookieConfig == (CookieConfig{}) { | ||
| cfg.CookieConfig = CookieConfigDefault | ||
| } | ||
|
|
||
| if cfg.CookieConfig.Name == "" { | ||
| cfg.CookieConfig.Name = CookieConfigDefault.Name | ||
| } | ||
|
|
||
| if cfg.CookieConfig.SameSite == "" { | ||
| cfg.CookieConfig.SameSite = CookieConfigDefault.SameSite | ||
| } | ||
|
|
||
| // RFC6265: If SameSite=None, the Secure attribute must be set | ||
| // https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-14#name-storage-model | ||
| // Section 5.6.19 | ||
| if utils.ToLower(cfg.CookieConfig.SameSite) == "none" { | ||
| cfg.CookieConfig.Secure = true // Ensure Secure is true if SameSite=None | ||
| } | ||
|
|
||
| // Set the cookie configuration in the Redirect struct | ||
| r.cookieConfig = cfg.CookieConfig |
There was a problem hiding this comment.
since the part with the configurations has become larger, we can move this to a private function for creating the configuration, so that it becomes more usable again
Description
CookieConfigin theRedirectConfigstruct. This allows the user to fully configure the Cookies used by theRedirectfeature.CookieConfigRedirectsection to the "What's new" document.Redirectdocumentation.Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.