Decline permessage-deflate when the compressor window would be 8 bits - #1757
Decline permessage-deflate when the compressor window would be 8 bits#1757dylanpulver wants to merge 1 commit into
Conversation
zlib cannot build a raw deflate compressor with an 8 bit window:
zlib.compressobj(wbits=-8) raises ValueError("Invalid initialization
option"), while zlib.decompressobj(wbits=-8) is fine. permessage-deflate
uses raw deflate, so 8 is usable for the window we decompress with and
unusable for the window we compress with.
_MAX_WINDOW_BITS_VALUES accepts "8" from the wire, and both factories pass
it straight through to PerMessageDeflate as local_max_window_bits, so a
peer asking for a window of 8 bits on the side that compresses turned a
handshake into a ValueError. On a default server that is one request
header: Sec-WebSocket-Extensions: permessage-deflate;
server_max_window_bits=8 returned 500 where 9 returned 101. With
server_no_context_takeover the encoder is built lazily, so the handshake
succeeded and the first encode() raised instead.
RFC 7692 7.1.2.1 makes 8 a legal value in an offer and says a server
declines an offer it cannot support, so both factories now raise
NegotiationError for the compressor side only. The server declines the
extension and the connection proceeds uncompressed; the client fails the
connection with a clear error instead of a ValueError. 8 remains accepted
for the decompressor side, which works.
This matches docs/topics/compression.rst, which already documents the
usable range as 9 to 15.
|
Hello, I found it difficult to understand what's the problem in the AI-written novella describing only the solution, but I think I know the issue you're describing. This is madler/zlib#171. It hasn't been a problem in the past decade because there isn't any reasonable situation (that I'm aware of) where:
That's why I just mentioned it in the docs (which the AI found) for the sake of completeness and stopped there (docs added in this change). Would you please give me a one-sentence version of: 1/ where is this happening in real life If this is only a theoretical issue found by AI, but that no one has hit in 10 years, despite this lib having enough users to have encountered a lot of "creative" usage, I'll want a much shorter fix and no changelog — no one cares. |
zlibcannot build a raw deflate compressor with an 8 bit window:(zlib 1.2.12;
-9and-15are the controls.)permessage-deflate uses raw deflate, so 8 is fine for the window we decompress with and unusable for the window we compress with.
_MAX_WINDOW_BITS_VALUES(permessage_deflate.py:30) accepts"8"from the wire, and both factories pass it through unchanged intoPerMessageDeflateaslocal_max_window_bits, wherepermessage_deflate.py:71-75callszlib.compressobj(wbits=-8).Measured on a default
websockets.sync.server.serve()— nocompression=argument, soserver_max_window_bits=12— one request header changed between rows:8 vs 9 is the whole difference. Adding
server_no_context_takeovermoves it past the handshake, because no encoder is built in__init__then — before this change the handshake returns 101, the server echoesserver_max_window_bits=8, and the firstencode()raises the sameValueError:RFC 7692 §7.1.2.1 makes 8 a legal thing for a peer to ask for, and states the remedy:
So both factories now raise
NegotiationErrorfor the compressor side only. The server declines the extension and the connection proceeds uncompressed; the client fails the connection with a message instead of aValueError. 8 stays accepted for the decompressor side. This also matchesdocs/topics/compression.rst:82-84, which already documents the usable range as 9 to 15 — "Setting it to 8 is possible but rejected by some versions of zlib and not very useful." The code said 8, the docs said 9, and nothing cross-checked them.Why the tests did not catch it
The window-bits values the negotiation tests use:
7, 16 and 42 are all covered; 8 — the only value zlib rejects — is not. It appears twice in the file, in
test_init/test_init_error, which construct the factory and never aPerMessageDeflate. The four cases added here put 8 through both directions of both factories.Mutants
ValueError: Invalid initialization option, i.e. the bug."8"from_MAX_WINDOW_BITS_VALUESand add no guard: the two decompressor-side cases fail withInvalidParameterValue. So the added cases pin that 8 must stay legal on the wire, not just that the crash goes away.What I ran
python -m unittest(per the Makefile, withPYTHONPATH=src): 2584 tests.ruff format --checkandruff checkovercompliance docs src testsclean;mypy --strict srcclean.One honest note on the suite:
tests.sync.test_client.ClientTests.test_reconnectfails intermittently for me — but it does so on pristinemainas well (observed on both arms, and passing 5/5 when run alone), which matches whatdocs/project/contributing.rstsays about the suite being tuned for speed "with a tolerable level of flakiness". Not attributable to this change.Not tested: no Windows or free-threaded run, no
toxmatrix, and no run of the Autobahn compliance suite. I also did not check whether a real-world client sendsserver_max_window_bits=8in practice.Behaviour change worth flagging:
ServerPerMessageDeflateFactory(server_max_window_bits=8)(and the client'sclient_max_window_bits=8) currently raisesValueErrorat connection time; it now declines the extension instead. That is the local-configuration footgun from #941. Making the factory constructors reject 8 outright, so a bad local config fails loudly at construction rather than silently disabling compression, seemed like a separate decision — happy to add it here if you want it.Provenance
Not from a bug report. I was auditing extension-parameter negotiation in websocket and HTTP libraries against the RFCs, spotted that the code's accepted range (8-15) and the project's own docs (9-15) disagreed, and worked out which side zlib agreed with.
AI assistance: this change was written with an AI coding assistant.