Reject unmasked client WebSocket frames (RFC 6455 §5.1) - #232
Conversation
RFC 6455 §5.1 requires a server to close the connection upon receiving an unmasked frame. HTTPConnection now enforces both §5.1 masking rules for every WSHandler: inbound frames without a mask fail the connection with a single 1002 (protocol error) close, inbound masks are stripped before frames reach handlers, and outbound frames are always sent unmasked. WSFrameEncoder preserves the decoded mask so the boundary can detect the violation. Closes TVT-1061 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #232 +/- ##
==========================================
+ Coverage 92.83% 92.96% +0.12%
==========================================
Files 71 71
Lines 3743 3795 +52
==========================================
+ Hits 3475 3528 +53
+ Misses 268 267 -1 ☔ View full report in Codecov by Harness. |
The SocketError.disconnected path in HTTPConnection's client frame stream was the one line codecov flagged unmasked-frame enforcement left untested. A peer closing TCP without a Close frame must end the session cleanly — not throw, and not register as a §5.1 violation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // pulled past the upgrade request remain available to the WS framer. | ||
| let client = AsyncThrowingStream.decodingFrames(from: bytes) | ||
| let bytes = self.bytes | ||
| let client = AsyncThrowingStream<WSFrame, any Swift.Error> { |
There was a problem hiding this comment.
I'm wondering whether HTTPConnection.swift is the best place for this validation.
Presently, decoding errors from WSFrameEncoder.decodeFrame(from:) and protocol validation errors from incoming client frames are surfaced through the AsyncThrowingStream received by WSHandler. The handler is responsible for terminating its outgoing server-frame stream and may optionally send a Close frame first.
It seems simpler to perform the incoming mask validation in WSFrameEncoder, perhaps by adding a server-specific decoding method:
static func decodeClientFrame(
from bytes: some AsyncBufferedSequence<UInt8>
) async throws -> WSFrame {
let frame = try await decodeFrame(from: bytes)
guard frame.mask != nil else {
throw Error("Incoming client frames must be masked")
}
return frame
}This requires decodeFrame(from:) to preserve the decoded mask, as you have in this PR. The client-frame stream could then use something like decodeClientFrame(from:), and the WSHandler will receive the validation error through its existing input stream.
MessageFrameWSHandler, which is the higher-level handler I expect most users to use, should already catch this error, emit a protocol-error Close frame, and terminate its outgoing frame stream. This appears to avoid the violation side-channel and task-group coordination currently being added to HTTPConnection.
There was a problem hiding this comment.
Hmmmmmmmmmm..... let me give some thought and check on any related TODOs.
I'll take another pass.
Problem
The server currently processes unmasked client-to-server WebSocket frames as if they were valid. RFC 6455 §5.1:
The frame decoder read the mask and unmasked the payload, but discarded the mask — so no downstream code could tell a masked frame from an unmasked one.
Change
Masking rules are enforced at the transport boundary in
HTTPConnection.switchToWebSocket, so they hold for everyWSHandler(the built-inMessageFrameWSHandlerand custom implementations alike):WSFrameEncoder.decodeFramenow preserves the decoded mask on the returned frame (payload remains stored unmasked) so the boundary can detect the violation.No public API changes; handlers continue to observe frames with
mask == nilexactly as before. The close frame is written only after the server-output task has been cancelled and drained, so it can never interleave with an in-flight write (ThrowingTaskGroup.waitForAll()stores the first error without cancelling siblings, so the coordination waits onnext(), cancels, drains, then writes).Tests
Four integration tests in
HTTPConnectionTestsdrive a real socket pair:Plus a decode round-trip assertion pinning mask preservation. Full suite passes (456 tests).
🤖 Generated with Claude Code