✨ feat: Add support for CookieConfig in Redirect #3076
Conversation
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 40 files out of 97 files are above the max files limit of 50. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the 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 Configuration 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
* Improve performance of adaptor middleware by over 50% * Update whats_new documentation * Remove fasthttp.Request pool * Update whats_new.md
* added startup default probe endpoint * added test case * updated docs * updated test order * added test case * fixed go fmt and md lint * fixed go fmt and md lint * updated doc as per coderabbitai suggestions * changed healhtcheck route register to use default const instead of string for test cases * updated whats new with healthcheck content * updated whats new doc with coderabbitai sugg * updated migration guide
* feat: add rebuild tree method * docs: add newline at the end of app.md * docs: add an example of dynamic defined routes * docs: remove tabs from example code on app.md * Update docs/api/app.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update app.md * docs: add RebuildTree to what's new documentation * fix: markdown errors in documentation Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * refactor: add mutex lock to the addRoute function * refactor: remove mutex lock from addRoute * refactor: fix mutex deadlock in addRoute --------- Co-authored-by: Juan Calderon-Perez <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* test(ctx_test): Fix race condition * chore: Release ctx resource after sending file * refactor: sendFileBodyReader function Refactor the `sendFileBodyReader` function to remove the unnecessary `app` parameter. This simplifies the function signature and improves code readability.
* Refactor benchmarks workflow * Use full semver tag * Add fetch depth
We should only fail during a PR, not during a merge
…ger Middleware (#3066) * logger: Use Content-Length header for BytesReceived and BytesSent tags * Use strconv.AppendInt instead of fasthttp.AppendUint
… length 0 (#3075) Resolves #3072 Signed-off-by: brunodmartins <[email protected]>
…duce Memory Usage (#3079) * Use composites for internal structures. Fix alignment of structures across Fiber * Update struct alignment in test files * Enable alignment check with govet * Fix ctx autoformat unit-test * Revert app Config struct. Add betteralign to Makefile * Disable comment on alert since it wont work for forks * Update benchmark.yml * Update benchmark.yml * Remove warning from using positional fields * Update router.go
* Improve performance of helper functions * Fix issue with PR comments from forks
* feat: add max calculator to limiter middleware * docs: update docs including the new parameter * refactor: add new line before go code in docs * fix: use crypto/rand instead of math/rand on tests * test: add new test with zero set as limit * fix: repeated tests failing when generating random limits * fix: wrong type of MaxCalculator in docs * feat: include max calculator in limiter_sliding * refactor: rename MaxCalculator to MaxFunc * docs: update docs with MaxFunc parameter * tests: rename tests and add test for limiter sliding
Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Use utils Trim functions instead of the strings/bytes functions * rename Test and Benchmark functions with same name
|
Will finish this during the weekend. |
what is the state here ? |
|
Blocked by #3099 both PR's are doing massive changes to the same file. |
|
@gaby can you update the PR |
|
@gaby friendly ping |
|
I'm going to create a new branch/PR. The merge conflicts are impossible to deal with. |
Do not forget |
|
@ReneWerner87 It's not, the changes for adding msgpack a few PR's ago were in the same lines that this one was modifying. |
|
ok |
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.