fix!: cookie string parser #3034
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.
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.
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.
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.
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.
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.
PR is pointed at v3.0 branch now, can we merge it there?
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 |
provinzkraut
force-pushed
the
3023-fix-cookie-string-parser
branch
from
649dce6
to
f6c8bdb
Compare
provinzkraut
force-pushed
the
v3.0
branch
3 times, most recently
from
c517c0b
to
2536957
Compare
provinzkraut
force-pushed
the
v3.0
branch
2 times, most recently
from
81081dd
to
a4d7ea1
Compare
provinzkraut
force-pushed
the
v3.0
branch
2 times, most recently
from
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
github-actions
bot
added
area/private-api
|
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 |
provinzkraut
force-pushed
the
v3.0
branch
3 times, most recently
from
8df1b17
to
33ded66
Compare
provinzkraut
force-pushed
the
v3.0
branch
4 times, most recently
from
466de2a
to
2292b5d
Compare
provinzkraut
force-pushed
the
v3.0
branch
3 times, most recently
from
dd4215a
to
126a504
Compare
Fixes #3023