🩹 fix: Implement RFC 6265 Cookie Value Sanitization #3379
Conversation
c00kie17
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 new method Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Ctx as DefaultCtx (Cookies)
participant Parser as parseCookieValue
participant Validator as validCookieValueByte
Client->>Ctx: Send request with Cookie header
Ctx->>Parser: Call parseCookieValue(rawValue)
loop For each character in rawValue
Parser->>Validator: Validate character
Validator-->>Parser: Return validity
end
Parser-->>Ctx: Return parsedValue
Ctx-->>Client: Return cookie with parsedValue
sequenceDiagram
participant Client as Client
participant Redirect as Redirect
participant Flash as Flash Messages
Client->>Redirect: Request with Flash cookie
Redirect->>Flash: Retrieve cookie value
Flash->>Redirect: Check if cookie value is empty
alt Not empty
Flash->>Redirect: Decode base64 cookie value
Redirect->>Flash: Unmarshal decoded value
end
Redirect-->>Client: Return flash messages
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements cookie value sanitization in accordance with RFC 6265 standards for cookie values by removing invalid characters (double quotes, semicolons, and backslashes) while allowing commas.
- Implements a new helper function in ctx.go for sanitizing cookie values.
- Updates test cases in both ctx_test.go and middleware/keyauth/keyauth_test.go to verify correct sanitization behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/keyauth/keyauth_test.go | Updates the expected cookie key to ensure it no longer contains disallowed chars. |
| ctx_test.go | Adds test cases for sanitization of cookies with quotes, semicolons, and backslashes. |
| ctx.go | Introduces cookie value sanitization and logs warnings on encountering invalid bytes. |
|
@c00kie17 csrf middleware may need to be updated? |
gaby
changed the title
|
Im not sure how you want to handle the raw binary on headers, the current tests are failing due to that, looks like there is a We can convert them to base64 before storing so they are RFC 6265 compliant |
|
@c00kie17 Where are we with this PR? Thanks! |
|
sorry for the delay, Ive updated the flash messages to be base64 encoded and decoded so that the cookie parser does not reject the the flash message as byte values |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3379 +/- ##
==========================================
+ Coverage 83.87% 84.01% +0.13%
==========================================
Files 119 119
Lines 11892 11921 +29
==========================================
+ Hits 9974 10015 +41
+ Misses 1488 1479 -9
+ Partials 430 427 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@c00kie17 sorry for the late message |
I was wondering that too, this should go in |
I agree. This RFC check belongs to http server side, not framework. I think this PR should be opened in fasthttp repository for convenience |
| cookieValue := c.Cookies(FlashCookieName) | ||
| require.NotEmpty(t, cookieValue) | ||
|
|
||
| decodedValue, err := base64.StdEncoding.DecodeString(cookieValue) |
There was a problem hiding this comment.
I think hex is faster for this stuff as flash messages are mostly small.
Description
Added cookie value sanitization according to RFC 6265 standards. The implementation validates and sanitizes cookie values by removing invalid characters (
"",;,\), while intentionally allowing commas to maintain compatibility with Go's net/http package (see https://golang.org/issue/7243). Test cases have been updated accordingly to use RFC-compliant values.Fixes #3176
Changes introduced
Breaking Change Notice
This change enforces RFC 6265 compliance for cookie values. If an existing application currently stores or relies on cookie values containing these characters, they will be silently removed when processed by Fiber.
Type of change
Checklist