Enforce WebSocket max message size across fragments - #1085
Conversation
|
Docs preview: https://f63a058f-httpx2-docs.pydantic.workers.dev |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Kludex
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3937e2ca12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if partial_message_size > max_bytes: | ||
| self.close(CloseReason.MESSAGE_TOO_BIG, "Message too big") | ||
| self._events.put(WebSocketDisconnect(CloseReason.MESSAGE_TOO_BIG, "Message too big")) |
There was a problem hiding this comment.
With the default configuration, every message over 65,536 bytes now closes the connection, but docs/websockets.md:96-101 still defines max_message_size_bytes as the network read chunk size and explicitly promises that larger messages are reassembled. Applications following that documented contract will unexpectedly receive WebSocketDisconnect; update the public documentation alongside this behavioral change so users know that this value is now a hard message-size limit.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/httpx2/httpx2/websockets/_api.py">
<violation number="1" location="src/httpx2/httpx2/websockets/_api.py:524">
P3: The new byte-limit behavior is not covered for multibyte text or binary fragments, so the advertised byte-based limit can regress silently for payloads whose character count differs from their UTF-8 byte count. Adding boundary tests for a UTF-8 text message and fragmented binary payload would make the enforcement reliable.</violation>
<violation number="2" location="src/httpx2/httpx2/websockets/_api.py:525">
P2: The `max_bytes` parameter is now used both as the network read chunk size (passed to `self._read_stream`) and as the hard maximum allowed message size (checked here). Previously, `max_message_size_bytes` only controlled the read buffer and larger messages were transparently reassembled from fragments. This dual use silently changes the public contract—existing callers relying on reassembly of messages larger than the read buffer will now receive `WebSocketDisconnect`. Consider introducing a separate parameter (e.g., `max_message_size`) to decouple the size enforcement from the read-buffer size, or at minimum update the documentation in `docs/websockets.md` which still describes `max_message_size_bytes` as the read chunk size.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| self._should_close.set() | ||
| if isinstance(event, wsproto.events.Message): | ||
| partial_message_size += len(event.data.encode() if isinstance(event.data, str) else event.data) | ||
| if partial_message_size > max_bytes: |
There was a problem hiding this comment.
P2: The max_bytes parameter is now used both as the network read chunk size (passed to self._read_stream) and as the hard maximum allowed message size (checked here). Previously, max_message_size_bytes only controlled the read buffer and larger messages were transparently reassembled from fragments. This dual use silently changes the public contract—existing callers relying on reassembly of messages larger than the read buffer will now receive WebSocketDisconnect. Consider introducing a separate parameter (e.g., max_message_size) to decouple the size enforcement from the read-buffer size, or at minimum update the documentation in docs/websockets.md which still describes max_message_size_bytes as the read chunk size.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpx2/httpx2/websockets/_api.py, line 526:
<comment>The `max_bytes` parameter is now used both as the network read chunk size (passed to `self._read_stream`) and as the hard maximum allowed message size (checked here). Previously, `max_message_size_bytes` only controlled the read buffer and larger messages were transparently reassembled from fragments. This dual use silently changes the public contract—existing callers relying on reassembly of messages larger than the read buffer will now receive `WebSocketDisconnect`. Consider introducing a separate parameter (e.g., `max_message_size`) to decouple the size enforcement from the read-buffer size, or at minimum update the documentation in `docs/websockets.md` which still describes `max_message_size_bytes` as the read chunk size.</comment>
<file context>
@@ -520,6 +522,11 @@ def _background_receive(self, max_bytes: int) -> None:
self._should_close.set()
if isinstance(event, wsproto.events.Message):
+ partial_message_size += len(event.data.encode() if isinstance(event.data, str) else event.data)
+ if partial_message_size > max_bytes:
+ self.close(CloseReason.MESSAGE_TOO_BIG, "Message too big")
+ self._events.put(WebSocketDisconnect(CloseReason.MESSAGE_TOO_BIG, "Message too big"))
</file context>
| if isinstance(event, wsproto.events.CloseConnection): | ||
| self._should_close.set() | ||
| if isinstance(event, wsproto.events.Message): | ||
| partial_message_size += len(event.data.encode() if isinstance(event.data, str) else event.data) |
There was a problem hiding this comment.
P3: The new byte-limit behavior is not covered for multibyte text or binary fragments, so the advertised byte-based limit can regress silently for payloads whose character count differs from their UTF-8 byte count. Adding boundary tests for a UTF-8 text message and fragmented binary payload would make the enforcement reliable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpx2/httpx2/websockets/_api.py, line 525:
<comment>The new byte-limit behavior is not covered for multibyte text or binary fragments, so the advertised byte-based limit can regress silently for payloads whose character count differs from their UTF-8 byte count. Adding boundary tests for a UTF-8 text message and fragmented binary payload would make the enforcement reliable.</comment>
<file context>
@@ -520,6 +522,11 @@ def _background_receive(self, max_bytes: int) -> None:
if isinstance(event, wsproto.events.CloseConnection):
self._should_close.set()
if isinstance(event, wsproto.events.Message):
+ partial_message_size += len(event.data.encode() if isinstance(event.data, str) else event.data)
+ if partial_message_size > max_bytes:
+ self.close(CloseReason.MESSAGE_TOO_BIG, "Message too big")
</file context>
|
Addressed review feedback in
Checks:
|
1 participant
Summary
max_message_size_bytesagainst cumulative incoming WebSocket message sizeMESSAGE_TOO_BIGand raiseWebSocketDisconnectwhen the limit is exceededTests
uv run ruff check src/httpx2/httpx2/websockets/_api.py tests/httpx2/websockets/test_api.pyuv run ruff format --check src/httpx2/httpx2/websockets/_api.py tests/httpx2/websockets/test_api.pyuv run pytest tests/httpx2/websockets/ -q