KTOR-8343 Fix: Store client header cookies with RAW encoding to preve… #4873
Conversation
WalkthroughThe changes update how cookies parsed from the "Cookie" request header are instantiated in the HTTP client core, ensuring they are created with explicit raw encoding. Additionally, a new test is added to verify that cookies captured from headers retain their original raw format and that the header remains unchanged after processing. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 ensures that cookies captured from the request header are stored using RAW encoding to preserve their original values, addressing the risk of unintended value alteration.
- Updates HttpCookies.kt to map header cookies with CookieEncoding.RAW.
- Adds a new test in CookiesTest.kt to verify that the original cookie header values are preserved.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ktor-client/ktor-client-core/common/test/CookiesTest.kt | Adds a test case to ensure that header cookies remain unchanged when stored with RAW encoding. |
| ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cookies/HttpCookies.kt | Updates the cookie parsing logic to force RAW encoding for header cookies. |
Comments suppressed due to low confidence (1)
ktor-client/ktor-client-core/common/test/CookiesTest.kt:115
- Consider expanding the test to also assert that each Cookie object added to the storage has its 'encoding' property set to CookieEncoding.RAW for added verification of the desired behavior.
assertEquals(cookies, builder.headers[HttpHeaders.Cookie])
| @@ -53,7 +53,9 @@ public class HttpCookies internal constructor( | |||
| val url = builder.url.clone().build() | |||
| val cookies = headers[HttpHeaders.Cookie]?.let { cookieHeader -> | |||
| LOGGER.trace("Saving cookie $cookieHeader for ${builder.url}") | |||
| parseClientCookiesHeader(cookieHeader).map { (name, encodedValue) -> Cookie(name, encodedValue) } | |||
| parseClientCookiesHeader(cookieHeader).map { (name, encodedValue) -> | |||
| Cookie(name, encodedValue, encoding = CookieEncoding.RAW) | |||
There was a problem hiding this comment.
Consider adding a short inline comment explaining why the cookies captured from the header are explicitly set to RAW encoding. This will help maintain the clarity of this change for future maintainers.
Copilot uses AI. Check for mistakes.
…nt value alteration
|
Thank you for the review and merge! 🤩 |
…nt value alteration
Subsystem
Client, HttpCookies
Motivation
KTOR-8343 HttpCookies: Encoding of request cookies is not preserved in CookiesStorage
Currently, when
HttpCookiescaptures cookies from theCookierequest header, it doesn't explicitly specify an encoding for storage. This can lead to unintended alteration of cookie values if theCookiesStoragelater retrieves them assuming a different encoding (e.g.,URI_ENCODINGby default).This is problematic because the original cookie value, as sent by the client, might not be preserved, potentially leading to unexpected behavior in applications that rely on exact cookie values.
Solution
This PR modifies
HttpCookiesto explicitly store cookies captured from theCookierequest header withCookieEncoding.RAW.By doing so, we ensure that the cookie value is preserved in its original form as received in the header, regardless of any default encoding assumptions by the
CookiesStorage.A new test case,
testCapturedHeaderCookiesStoredAsRawPreserveOriginalHeader, has been added toCookiesTest.ktto verify that cookies captured from the header are indeed stored and retrieved with their original values intact, even when they contain characters that might be altered by other encodings.