Skip to content

enforce close reason byte limit for websockets#6167

Merged
anonrig merged 1 commit intomainfrom
yagiz/throw-syntaxerror-long-websocket
Feb 25, 2026
Merged

enforce close reason byte limit for websockets#6167
anonrig merged 1 commit intomainfrom
yagiz/throw-syntaxerror-long-websocket

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Feb 24, 2026

No description provided.

@anonrig anonrig requested a review from jasnell February 24, 2026 23:50
@anonrig anonrig requested review from a team as code owners February 24, 2026 23:50
@ask-bonk
Copy link
Contributor

ask-bonk bot commented Feb 24, 2026

Everything looks correct. Let me review the complete diff one more time carefully:

  1. compatibility-date.capnp: New flag websocketCloseReasonByteLimit @164 with enable date 2026-03-03. Ordinal is sequential after @163. Has enable/disable flag names. Documentation is clear. All good.

  2. web-socket.c++: The check is correctly gated behind the compat flag. Uses r.size() <= 123 which is correct for UTF-8 byte counting since kj::String stores UTF-8. Throws DOMSyntaxError which matches the WHATWG spec. Placement is correct — after code validation but before the "reason without code" check.

  3. Test file: Good coverage — tests 124 ASCII bytes (should throw), exactly 123 bytes (should not throw), and multibyte characters (62 × 2 = 124 bytes, should throw). Tests properly clean up by calling ws.close() after the throwing case.

  4. .wd-test: Correctly adds the compat flag to enable the new behavior for tests.

  5. WPT test: Removes the expectedFailures entry for Close-Reason-124Bytes.any.js since the test should now pass. Adds a cleanup handler to close the WebSocket properly. This is well-reasoned — after close(1000, longReason) throws, the WebSocket remains in CONNECTING state and needs to be closed.

This is a clean, well-structured change. The compat flag is correctly used to gate the behavioral change. The implementation matches the spec. The tests are thorough. No issues found.

LGTM

github run

@anonrig anonrig enabled auto-merge February 24, 2026 23:57
@anonrig anonrig merged commit 853aea6 into main Feb 25, 2026
25 of 27 checks passed
@anonrig anonrig deleted the yagiz/throw-syntaxerror-long-websocket branch February 25, 2026 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants