Fix 1909 - #1916
Merged
Merged
Conversation
…crossbario#1909) A permessage-compressed WebSocket message is bounded by maxMessagePayloadSize against its COMPRESSED wire size (the check in onMessageFrameBegin runs on the declared frame length, before inflation), so a small compressed frame that inflates far beyond the configured limit is delivered to the application (security advisory GHSA-hxp9-w8x3-p566). Add a backend-neutral protocol-level test (drives _dataReceived with a crafted masked, compressed frame) asserting the message is rejected before delivery. The module is deliberately NOT gated on USE_TWISTED, so it runs under both the Twisted (trial) and asyncio (pytest) coverage phases, and it is parametrized over every available permessage-compress backend (deflate/bzip2/snappy/brotli; skipped when the optional dependency is absent) and both processing modes (whole-message and streaming). A control confirms under-limit messages are still delivered intact. The rejection cases fail on current code, proving the bypass. Note: This work was completed with AI assistance (Claude Code).
Contributor
Author
|
reviewed (by me), tested locally, and now TDD phase 1 is running, let's see it fail (what we do want): 
|
…bario#1909) The previous commit obtained the compressor class via the permessage-compress extension registry, whose value the ty static-typing gate infers as a union of all codec classes - so the codec-specific constructor calls tripped [missing-argument] and failed `just check-typing`, which aborted the job before the unit tests could run at all. Use concrete per-codec imports (each gated by ImportError for the optional snappy/brotli backends) so every constructor call site resolves to a single signature. No behavioural change: the enforcement tests still fail red on current code, now for the right reason (the missing enforcement), not the type checker. Verified locally: `ruff check`, the ANN/UP/TCH ruff pass, and `ty check` are all clean; the two enforcement tests fail under both the Twisted and asyncio phases. Note: This work was completed with AI assistance (Claude Code).
Contributor
Author
|
there was an issue with phase 1 - it did fail, but for the wrong reason;) after pointing that out, an ack, and another iteration: 
and after that 2nd commit, it now works - as in, it fails, but for the right reason: 
however, there is still a thing missing / to fix for a perfect TDD red-green cycle: 
so that's yet one more commit and run for the right red! and then we can move on for the right green (the actual fix). I want to do this TDD properly;) |
…o#1909) maxMessagePayloadSize was enforced only in onMessageFrameBegin(), against the declared (compressed) frame length, before permessage-compress inflation. A small compressed frame could therefore inflate far beyond the configured limit and be delivered to the application - the compressed-size check standing in for a decompressed-size bound (security advisory GHSA-hxp9-w8x3-p566; same class as CVE-2016-10544). Enforce the limit at the inflation site in onFrameData(): for a compressed message, add the inflation delta (uncompressedLen - compressedLen) to message_data_total_length - which onMessageFrameBegin() had seeded with the compressed length - so the running total reflects the UNCOMPRESSED reassembled message, and re-check against maxMessagePayloadSize. On exceed, fail the connection with MESSAGE_TOO_BIG before delivery. This is backend-agnostic and runs before frame data is dispatched, so it protects both whole-message and streaming consumers and every permessage-compress backend. Uncompressed messages are unaffected (delta is zero; the frame-begin check already bounds them), as is the maxFramePayloadSize per-frame wire guard. Makes the tests added in the previous commit pass across deflate/bzip2/brotli (snappy when installed), both backends, and both processing modes. Also cancel the opening-handshake timeout in the test harness after _connectionMade(): the tests drive protocol bytes directly and never run a real close handshake, so the scheduled DelayedCall would otherwise leak and Twisted's trial would report a dirty reactor once the enforcement fix makes the connection drop cleanly. Note: This work was completed with AI assistance (Claude Code).
The maxMessagePayloadSize test harness cancelled only the opening-handshake timeout that _connectionMade() schedules. That is sufficient today - the enforcement path fails the connection via dropConnection() (failByDrop defaults True), which never runs the closing handshake, so no close-handshake timer is scheduled - but it silently depends on failByDrop. Cancel both handshake timers defensively (mirroring _connectionLost), so a future variant that drives a real close cannot leak a DelayedCall and trip Twisted trial's dirty-reactor check. No behavior change: closeHandshakeTimeoutCall is None in all current test paths, so the added guard is a no-op there. Note: This work was completed with AI assistance (Claude Code).
Contributor
Author
|
very good! test results are now exactly as intended: https://github.com/crossbario/autobahn-python/actions/runs/29343192492/job/87121506560?pr=1916 here is in-depth results archive review: 
|
…rossbario#1908) Write down the wire-vs-application distinction that the maxMessagePayloadSize security fix establishes, so the semantics that caused the advisory are no longer ambiguous: - interfaces.py: clarify the setProtocolOptions() docstrings (server and client) so maxFramePayloadSize is the on-the-wire (compressed) per-frame size and maxMessagePayloadSize is the reassembled AND decompressed (uncompressed, application-level) message size, enforced against the inflated size, not the compressed wire size. - programming.rst: same clarification for the two options in the WebSocket programming guide. - changelog.rst: add a Security section for 26.7.1 with the crossbario#1909 fix (advisory GHSA-hxp9-w8x3-p566, including the behaviour change) and the previously undocumented crossbario#1908 deflate unconsumed_tail cap fix. Docs only; no code or behaviour change. Note: This work was completed with AI assistance (Claude Code).
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
1 participant
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.
fixes #1909 - this will be two-phase, proper "red green" TDD