fix: correct session cookie chunking and reassembly - #5022
Open
anuanas2007 wants to merge 2 commits into
Open
Conversation
Two bugs in the client-side (cookie) session backend. Chunk reassembly used a plain `sorted()` over the cookie names, which orders them lexicographically: `session-0, session-1, session-10, session-11, session-2, ...`. Below ten chunks that matches the numeric order, so it went unnoticed; at ten or more the base64 payload is concatenated out of order and the session is destroyed. Usually it decodes to an empty session and the user is silently logged out, but a misordered payload can also yield malformed JSON in the associated-data segment, raising `SerializationException` from `decode_json`, which `load_from_connection` does not suppress - a 500. `get_cookie_keys` now sorts by the numeric chunk index. The index is read from the suffix left after the configured key rather than by splitting the name on `-`, so keys that themselves contain hyphens (`my-session`) stay correct. Chunk size used a fixed 64 byte allowance for the cookie name and attributes. The real overhead is at least 66 bytes with the default key and `secure=True`, and grows with the key length, `domain` and `path` - up to 289 bytes past the RFC 6265 limit of 4096 for the longest permitted key. Oversized cookies are silently dropped by some proxies, along with every other `Set-Cookie` header on the same response. The budget is now measured from a zero-length-value cookie built with the configured parameters. An empty value is itself emitted quoted (`session=""`), so the two quote bytes that Litestar adds to base64 values - which contain `+`, `/` and `=` - are already accounted for. The two interact: smaller chunks mean more chunks, so fixing the size alone would push more deployments past the ten-chunk threshold and into the reassembly bug. Fixes litestar-org#5021
If the configured `key`, `path` and `domain` are long enough to fill the whole 4096 byte cookie allowance on their own, the measured chunk size drops to zero and no session cookie is emitted at all. The response still carries a normal status code, so every user is silently logged out with nothing in the response or the logs to point at the cause - the same class of silent failure this change set exists to remove. Raise `ImproperlyConfiguredException` naming the offending key instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5022 +/- ##
==========================================
+ Coverage 67.19% 67.23% +0.03%
==========================================
Files 293 293
Lines 15363 15381 +18
Branches 1745 1748 +3
==========================================
+ Hits 10323 10341 +18
Misses 4890 4890
Partials 150 150 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Author
|
The failing Test server integration check is unrelated to this change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes two bugs in the client-side session backend. They interact, so both are fixed here.
Fixes #5021
1. Chunks are reassembled in the wrong order (not previously reported)
I ran into this while verifying #5021. Happy to file it as a separate issue if you would rather track it on its own.
A large session is split across
session-0,session-1, ... and reassembled with a plainsorted(), which orders the names alphabetically. Alphabeticallysession-10comes beforesession-2, so once a session needs ten or more cookies the payload is concatenated out of order and the session is lost. Below ten chunks the two orders agree, which is why this has gone unnoticed.Usually the session decodes to empty and the user is silently logged out. Sometimes the misordered payload yields malformed JSON in the associated-data segment, raising
SerializationException, whichload_from_connectiondoes not suppress, so the request returns a 500.2. Chunks exceed the 4096-byte cookie limit (#5021)
CHUNK_SIZE = 4096 - 64reserves a fixed 64 bytes for the cookie name and attributes. The real overhead is 66 with the default key andsecure=True, and grows with the key,domainandpath, so every full chunk goes over the limit. Some proxies then silently drop everySet-Cookieheader on the response.Fixing #5021 produces smaller, and therefore more, chunks, which makes the ordering bug easier to hit. Hence one PR.
The fix
get_cookie_keyssorts by the numeric chunk index instead of the cookie name. The index is read from the suffix after the configured key rather than by splitting on-, so keys that contain a hyphen (my-session) stay correct.session=""), so the quoting that base64 values incur is already accounted for.key,pathanddomainfill the 4096 bytes on their own, that measurement goes negative and no cookie was emitted at all, a silent logout with a 200 response. It now raisesImproperlyConfiguredException.Verification
The MCVE from #5021, run unchanged. The "before" column matches the output posted in the issue byte-for-byte:
Sessions of 40,000 B (14 chunks), 200,000 B (67) and 900,000 B (298) were all lost before and all round-trip now.
Across 4 key lengths x 3 attribute combinations x 5 session sizes:
Tests
Four tests added to
tests/unit/test_middleware/test_session/test_client_side_backend.py:test_get_cookie_keys_are_ordered_by_chunk_indexasserts the ordering directly.test_large_session_round_tripschecks that a session needing more than ten cookies survives a round trip, over bothsessionandmy-session.test_session_cookies_do_not_exceed_max_cookie_sizechecks that every emittedSet-Cookieis within 4096 bytes, over key lengths 7, 60 and 250 and three attribute combinations.test_dump_data_raises_when_cookie_attributes_leave_no_roomcovers the new guard.test_dump_and_load_datapreviously assertedlen(value) <= CHUNK_SIZE. It now checks the emitted header againstMAX_COOKIE_SIZE, since the fixed constant no longer determines the limit.