Repository navigation
Enforce RFC 6455 frame rules in WebSocket writer - #14233
rinopatrick wants to merge 4 commits into
Conversation
Automation signalsActivity patterns show signs of automation. Evidence
Last 5 PRs: This is an automated analysis by AgentScan |
pyba-chronographer
Bot
added
the
bot:chronographer:provided
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14233 +/- ##
==========================================
- Coverage 99.11% 99.11% -0.01%
==========================================
Files 135 135
Lines 53338 53499 +161
Branches 2717 2732 +15
==========================================
+ Hits 52865 53023 +158
- Misses 359 360 +1
- Partials 114 116 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
 [Medium impact] The change affects outbound WebSocket traffic on both aiohttp client and server connections. Not ready to merge: rejected closes can still end a usable connection, and the required changelog link is missing. Reviews (2) · Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| allowed = code in _ALLOWED_CLOSE_CODES or ( | ||
| WS_CLOSE_CODE_MIN_APPLICATION <= code <= WS_CLOSE_CODE_MAX_APPLICATION | ||
| ) | ||
| if not allowed: | ||
| raise ValueError(f"Invalid close code: {code}") | ||
| # RFC 6455 §5.5.1: the close reason must be valid UTF-8. | ||
| try: | ||
| message.decode("utf-8") | ||
| except UnicodeDecodeError as exc: | ||
| raise ValueError("Close message must be valid UTF-8") from exc |
There was a problem hiding this comment.
The new close-argument errors do not reach callers of either public close() method. Both methods set _closed before calling the writer, then catch these ValueErrors and close the transport. For example, await ws.close(code=1005) returns True instead of raising, and a corrected retry returns False.
Check arguments before the public methods change state, and let input errors reach the caller. The size check also needs to precede the writer's closing state change: writer.close(1000, b"x" * 124) currently sets _closing = True in finally despite sending nothing.
| Started enforcing :rfc:`6455` frame rules on the WebSocket writer, mirroring | ||
| what the reader already enforces: control frames (ping/pong/close) with more | ||
| than 125 bytes of payload, close frames with a code that must not appear on | ||
| the wire (1005, 1006, 1015, 1016-2999, above 4999), and close frames with a | ||
| non-UTF-8 reason are now rejected with :exc:`ValueError` instead of emitting | ||
| an invalid frame -- by :user:`rinopatrick`. |
There was a problem hiding this comment.
AGENTS.md requires a docs/ update for user-visible API changes and a PR-numbered symlink when a changelog uses an issue number. This PR adds new limits and errors without updating docs/client_reference.rst or docs/web_reference.rst, and CHANGES/14233.bugfix.rst is missing.
Before merging, update both reference pages and add CHANGES/14233.bugfix.rst as a symlink to 14167.bugfix.rst to satisfy these repository requirements.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| async def test_ping_payload_too_large(writer: WebSocketWriter) -> None: | ||
| # RFC 6455 §5.5: control frames must have ≤125 bytes of payload | ||
| with pytest.raises(ValueError, match="125"): | ||
| await writer.send_frame(b"x" * 126, WSMsgType.PING) |
There was a problem hiding this comment.
The three new rejection tests only check that ValueError is raised. A writer that sends the invalid frame and then raises would still pass them. Assert that transport.write was not called after each rejected input, before the valid close calls in test_close_invalid_code.
Merging this PR will not alter performance1 warning
Comparing 
Footnotes
|
Address review: raw close frames via send_frame() carried unchecked codes/reasons, and close() argument errors fired after _set_closed() tore the connection down. Validate up front in both wrappers and the writer; document ValueError in client/web references; changelog links PR. Closes aio-libs#14167
|
Addressed all four review points: (1) raw CLOSE frames via send_frame() are now validated (code + UTF-8 reason + payload shape), (2) close()/wrapper validate before _set_closed() so invalid arguments surface to the caller without tearing the connection down, (3) client/web reference docs document the new ValueError cases, changelog has the PR-numbered symlink, (4) rejection tests assert the invalid frame never hits the wire. |
for more information, see https://pre-commit.ci
Closes #14167.
The writer accepted frames the reader (conforming peer) must reject: control frames over 125 bytes, close codes 1005/1006/1015/1016-2999/above 4999, and non-UTF-8 close reasons. Validation now happens in WebSocketWriter.send_frame (covers ping/pong/close plus direct send_frame callers) and close(), raising ValueError before anything hits the wire — mirroring the reader-side checks.