🩹Fix: Adaptor middleware duplicates cookies #3151
Conversation
sigmundxia
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe changes introduce a modification to the Changes
Assessment against linked 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- middleware/adaptor/adaptor.go (1 hunks)
- middleware/adaptor/adaptor_test.go (2 hunks)
🔇 Additional comments not posted (1)
middleware/adaptor/adaptor.go (1)
104-105: Approve the change with suggestions for improvementThe addition of
c.Request().Header.DelAllCookies()effectively addresses the issue of cookie duplication described in the PR objectives. This change aligns with the fix proposed in the linked issue #3089.Consider adding a more detailed comment explaining why this step is necessary. For example:
- // Remove all cookies before setting, see https://github.com/valyala/fasthttp/pull/1864 + // Remove all cookies before setting new ones to prevent duplication. + // This addresses an issue where the adaptor middleware was causing cookie duplication. + // For more details, see: https://github.com/gofiber/fiber/issues/3089 + // and https://github.com/valyala/fasthttp/pull/1864To ensure this change doesn't have unintended side effects, please verify that other parts of the application aren't relying on the presence of the original cookies at this point in the middleware chain. Run the following script to check for any other references to cookies in the codebase:
This will help identify any potential areas that might be affected by this change.
✅ Verification successful
Change Verified Successfully
The addition of
c.Request().Header.DelAllCookies()in theHTTPMiddlewarefunction effectively addresses the issue of cookie duplication as described in the PR objectives. Thorough searches across the codebase indicate that this change does not negatively impact other parts of the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other references to cookies in the codebase echo "Searching for other references to cookies in the codebase:" rg --type go -i 'cookie' echo "\nSearching for uses of the HTTPMiddleware function:" rg --type go 'HTTPMiddleware'Length of output: 80700
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3151 +/- ##
=======================================
Coverage 80.09% 80.09%
=======================================
Files 117 117
Lines 9043 9044 +1
=======================================
+ Hits 7243 7244 +1
Misses 1365 1365
Partials 435 435
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- middleware/adaptor/adaptor_test.go (2 hunks)
🔇 Additional comments not posted (5)
middleware/adaptor/adaptor_test.go (5)
204-232: LGTM: Constants and middleware setup look good.The constant definitions and middleware setup are well-structured and clear. The use of constants for cookie names and values improves readability and maintainability.
234-255: LGTM: Test case for POST request with cookies is well-implemented.The test case correctly sets up the request with cookies, sends it to the app, and verifies the response. The assertions are comprehensive, checking both the number of cookies and their individual values.
257-265: LGTM: Test case for GET request is correctly implemented.This test case verifies that the middleware correctly returns a 405 Method Not Allowed status for GET requests, which aligns with the middleware's implementation.
267-276: LGTM: Test case for POST request without cookies is well-implemented.This test case correctly verifies the behavior when no cookies are present in the request. It checks for a 200 OK status and ensures that no cookies are present in the response.
204-277: Overall, the new test function effectively addresses the PR objectives.The
Test_HTTPMiddlewareWithCookiesfunction successfully tests the behavior of the HTTP middleware with respect to cookie handling. It covers the main scenarios:
- POST requests with cookies
- GET requests (which should be rejected)
- POST requests without cookies
These test cases align well with the PR objective of fixing the issue where cookies were being duplicated after passing through the
HTTPMiddleware. The implementation ensures that cookies are correctly processed and returned in the response without duplication.A minor suggestion for improvement would be to add error handling when setting cookies in the response, as noted in a previous comment. This would further enhance the robustness of the implementation.
Great job on addressing the issue and providing comprehensive test coverage!
|
@ReneWerner87 This is good to merge. |
|
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
In
adapter.go, add a step to clear existing cookies before introducing new ones in theHTTPMiddlewaresection, so that cookies are not duplicated after passing through the middleware.Fixes #3089
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md