-
-
Notifications
You must be signed in to change notification settings - Fork 374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix!: cookie string parser #3034
base: v3.0
Are you sure you want to change the base?
Conversation
litestar/_parsers.py
Outdated
@@ -56,7 +56,7 @@ def parse_cookie_string(cookie_string: str) -> dict[str, str]: | |||
Returns: | |||
A string keyed dictionary of values | |||
""" | |||
cookies = [cookie.split("=", 1) if "=" in cookie else ("", cookie) for cookie in cookie_string.split(";")] | |||
cookies = [cookie.split("=", 1) if "=" in cookie else (cookie, cookie) for cookie in cookie_string.split(";")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be
cookies = [cookie.split("=", 1) if "=" in cookie else (cookie, cookie) for cookie in cookie_string.split(";")] | |
cookies = [cookie.split("=", 1) if "=" in cookie else (cookie, "") for cookie in cookie_string.split(";")] |
I've checked out other implementations and found that:
- Stdlib refuses to parse this
- Wekzeug defaults to the cookie name and an empty value
- Starlette behaves the same we do
In addition, Starlette also specifically references this: https://bugzilla.mozilla.org/show_bug.cgi?id=169091.
Personally, I think defaulting to an empty value makes more sense than defaulting to the name as a value, as that's rather arbitrary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that an empty string makes way more sense as a value.
The only reason I have went with (cookie, cookie)
was to not change the output of the data extractors in a way that would potentially break user code (or log DB queries, etc.) in case someone is currently looking for these optional attributes among the cookie values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this is something we'd either have to hide behind a feature flag or wait for 3.0 to release due to the breaking nature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure how a feature flag for this would work, my initial idea was to use "experimental features" but the extractor has no access to the app.
Adding an additional parameter such as future_cookie_string_parsing: bool = False
to ResponseDataExtractor
to control the behavior of parse_cookie_string
seems to be the easiest solution. (but would be slightly problematic when it disappears in 3.0)
However after I added this future_cookie_string_parsing
parameter I noticed another issue; #3040, due to this one I think it might be better to just wait until 3.0 with both of them, if so, should the target branch be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to just wait until 3.0 with both of them
Yes, I agree.
if so, should the target branch be changed?
You can leave them as-is for now. We'll update the branch once the 3.0 work officially has been started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is pointed at v3.0 branch now, can we merge it there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm except for mentioned comments above; if we want to hold these until 3.0 i think pointing it at develop
for now might make sense?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.0 #3034 +/- ##
==========================================
- Coverage 98.26% 98.24% -0.03%
==========================================
Files 322 323 +1
Lines 14733 14504 -229
Branches 2343 2299 -44
==========================================
- Hits 14477 14249 -228
- Misses 117 118 +1
+ Partials 139 137 -2 ☔ View full report in Codecov by Sentry. |
@floxay @provinzkraut can we merge this now |
Co-authored-by: Janek Nouvertné <provinzkraut@posteo.de>
649dce6
to
f6c8bdb
Compare
c517c0b
to
2536957
Compare
81081dd
to
a4d7ea1
Compare
4dc78a8
to
81a323f
Compare
…ors (litestar-org#3529) Remove deprecated test
ignore type error
…deprecated params of internal `ExceptionHandlerMiddleware` (litestar-org#3435) * refactor!: Remove deprecated `app` param of `Response.to_asgi_response` (litestar-org#3393) * Remove 'app' parameter from `.to_asgi_response` * Remove debug param * Remove exception_handlers param * Remove litestar.middleware.exceptions
Port parse_values_from_connection_kwargs changes
v3 branch is available with changes so this could be merged into that if ci passes |
Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3034 |
8df1b17
to
33ded66
Compare
466de2a
to
2292b5d
Compare
dd4215a
to
126a504
Compare
Fixes #3023