Skip to content

Reject unmasked client WebSocket frames (RFC 6455 §5.1) - #237

Merged
swhitty merged 1 commit into
swhitty:mainfrom
ianegordon:ian/tvt-1164-rework-unmasked-client-frame-rejection-per-pr-232-review
Aug 10, 2026
Merged

Reject unmasked client WebSocket frames (RFC 6455 §5.1)#237
swhitty merged 1 commit into
swhitty:mainfrom
ianegordon:ian/tvt-1164-rework-unmasked-client-frame-rejection-per-pr-232-review

Conversation

@ianegordon

Copy link
Copy Markdown
Contributor

Problem

The server processes unmasked client-to-server WebSocket frames as if they were valid. RFC 6455 §5.1:

"a client MUST mask all frames that it sends to the server. ... The server MUST close the connection upon receiving a frame that is not masked. In this case, a server MAY send a Close frame with a status code of 1002 (protocol error)"

Change

This reworks closed PR #232 following the architecture proposed in this review comment:

  • WSFrameEncoder.decodeFrame(from:) now preserves the decoded mask (payload remains stored unmasked).
  • New server-specific WSFrameEncoder.decodeClientFrame(from:) throws when a frame has no mask.
  • HTTPConnection.switchToWebSocket decodes the client stream via a new decodingClientFrames(from:) adapter — the error surfaces through the AsyncThrowingStream the WSHandler already receives. The violation side-channel and task-group coordination from Reject unmasked client WebSocket frames (RFC 6455 §5.1) #232 are gone; HTTPConnection is a two-line diff.
  • HTTPClient and the shared decoder remain permissive — servers legitimately send unmasked frames.

The review's expectation that MessageFrameWSHandler "should already catch this error, emit a protocol-error Close frame, and terminate" was verified rather than assumed: the input-stream error reaches framesOut.finish(throwing:), and the AsyncStream.protocolFrames wrapper converts it into a Close frame carrying WSCloseCode.protocolError (1002) before ending the stream.

One amendment to the review's sketch: decodeClientFrame clears the mask after validating it is present. MessageFrameWSHandler echoes frames verbatim (ping → pong, and the client's Close frame), so a preserved mask would be re-encoded on the way out — violating §5.1 "A server MUST NOT mask any frames that it sends to the client." Clearing it also means handlers continue to observe mask == nil exactly as before; no handler-visible behavior changes.

Caveats

  • Enforcement flows through the established handler contract: the handler owns terminating its output stream when the input stream fails. MessageFrameWSHandler does this; a custom WSHandler that never consumes its input or swallows the error can keep the connection open — the same as for every other decode error today.
  • A custom handler that deliberately yields masked frames will still have them encoded masked on the wire. Outbound enforcement is intentionally out of scope here.
  • Client-side, readFrame now surfaces a mask set by a non-conforming server instead of silently stripping it.

Tests

  • decodeFrame mask preservation (unit + stream round-trip).
  • decodeClientFrame: unmasked → throws; masked → decoded payload, mask cleared.
  • decodingClientFrames: masked frames delivered unmasked; unmasked frame propagates the error (not normalized away like a disconnect); clean disconnect ends the stream without error.
  • HTTPConnection integration over socket pairs: unmasked frame → unmasked 1002 Close on the wire and the response loop terminates; masked frames reach a handler with mask == nil and payload decoded; abrupt client disconnect completes cleanly.

Full suite passes (468 tests).

🤖 Generated with Claude Code

Validate in WSFrameEncoder.decodeClientFrame so the error surfaces
through the handler's existing input stream; MessageFrameWSHandler
responds with a 1002 close. decodeFrame now preserves the decoded
mask; decodeClientFrame clears it so handlers can safely echo frames.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ianegordon
ianegordon marked this pull request as ready for review August 10, 2026 01:17
@ianegordon

Copy link
Copy Markdown
Contributor Author

I believe the test failures are false positives, but I'll keep an eye out for failures.

This is a followup PR on a resolution based on feedback in #232.

LMK if there are any requested changes.

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.04%. Comparing base (71dcb81) to head (cc5aea8).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   93.00%   93.04%   +0.03%     
==========================================
  Files          72       72              
  Lines        3748     3769      +21     
==========================================
+ Hits         3486     3507      +21     
  Misses        262      262              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@swhitty swhitty left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you,
This looks great

@swhitty
swhitty merged commit 88b4729 into swhitty:main Aug 10, 2026
21 of 26 checks passed
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