Skip to content

Fix websocket.Conn.Read dropping the tail of messages larger than the read buffer - #1741

Open
pkit wants to merge 2 commits into
cloudflare:masterfrom
pkit:fix/websocket-read-remainder
Open

pkit wants to merge 2 commits into
cloudflare:masterfrom
pkit:fix/websocket-read-remainder

Conversation

@pkit

@pkit pkit commented Sep 14, 2026

Copy link
Copy Markdown

Summary

For tcp://, ssh://, rdp://, smb://, bastion and socks-over-WebSocket ingress services, cloudflared unwraps the client's WebSocket frames itself via the server-side websocket.Conn. Its Read (websocket/connection.go) called wsutil.ReadClientBinary, which returns one complete WebSocket message, and then returned copy(reader, data). Any bytes of the message past len(reader) were silently discarded.

stream.Pipe drives that Read through cfio.Copy, which uses a pooled 16 KiB buffer (cfio/copy.go). So every client message larger than 16 KiB reached the origin truncated to its first 16 KiB, while the origin-to-client direction was unaffected. The origin then saw a corrupted byte stream (in our case a broken TLS record) and reset the connection, which shows up in debug logs as downstream->upstream copy: stream error: stream ID N; NO_ERROR.

The code has been this way since e226208 (TUN-3617, 2020). cloudflared access tcp never triggers it because it also copies through cfio.Copy and therefore never emits messages larger than 16 KiB, but any other WebSocket client that writes larger binary messages does.

Reproduction

  • Ingress rule tcp://<host>:443 behind Cloudflare Access.
  • A client that dials the wss:// endpoint and forwards a TCP stream as one binary message per Write (for example io.Copy from a *net.TCPConn, which produces 32 KiB messages).
  • Client to origin: of each message exactly the first 16 KiB arrives; a 4 MiB stream sent as 32 KiB messages arrives as every other 16 KiB chunk (first difference at byte 16385). With 64 KiB messages still only 16 KiB per message survives. With messages of 16 KiB or less everything arrives intact.
  • Same result with 2025.5.0 on http2 and 2026.8.2 on QUIC, so it does not depend on the tunnel transport.

Fix

Keep the unread remainder of a message in a bytes.Buffer on Conn and drain it on subsequent Read calls before reading the next message. This mirrors what GorillaConn.Read in the same file already does. Write, ping/pong handling, control-frame handling and error behaviour are unchanged.

Tests

websocket/connection_test.go adds:

  • TestConnReadMessageLargerThanBuffer: a single 100 KiB client binary message read through 16 KiB Reads arrives byte for byte.
  • TestConnReadMixedMessageSizes: messages of 1, 16383, 16384, 16385, 40000, 49152, 102400, 7 and 16384 bytes sent back to back arrive concatenated in order.

Both tests fail on the previous code (the reader times out waiting for the dropped tail) and pass with the fix. go test ./websocket/ and go vet ./websocket/ are clean. The change applies unmodified on 2025.5.0, 2026.8.2 and master.

C✳

… buffer

Conn.Read returned copy(reader, data) after reading a whole WebSocket
message, dropping every byte past len(reader). stream.Pipe feeds it a
16 KiB buffer via cfio.Copy, so client messages larger than 16 KiB reached
tcp://, ssh://, rdp://, smb://, bastion and socks ingress origins with only
their first 16 KiB.

Keep the unread remainder in a bytes.Buffer and drain it on the next Read,
as GorillaConn.Read already does. Add tests for a 100 KiB message and for
a mix of sizes around the 16 KiB boundary.
@pkit pkit closed this Sep 14, 2026
@pkit pkit reopened this Sep 14, 2026
@jcsf
jcsf requested review from jcsf and a balanced review from Copilot September 16, 2026 10:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The retained buffer allocation can unnecessarily increase memory usage for long-lived connections.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes truncation of oversized WebSocket messages when proxying stream-based ingress traffic.

Changes:

  • Buffers unread message tails across successive Read calls.
  • Adds regression tests for large and mixed-size messages.
File summaries
File Description
websocket/connection.go Preserves unread WebSocket message data.
websocket/connection_test.go Tests message integrity across buffer boundaries.
Review details

Suppressed comments (1)

websocket/connection_test.go:77

  • This test is isolated and parallel-safe, so it should call t.Parallel() as required by the repository's testing guidelines.
func TestConnReadMixedMessageSizes(t *testing.T) {
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread websocket/connection.go
Comment thread websocket/connection_test.go
Comment thread websocket/connection.go Outdated
Comment thread websocket/connection.go Outdated
Comment thread websocket/connection.go Outdated
Holding the remainder as a subslice of the message wsutil returned avoids a
second copy and releases the memory once the tail is drained, instead of a
bytes.Buffer pinning its peak allocation for the life of the connection.
The two new tests are parallel-safe, so they call t.Parallel.
@pkit

pkit commented Sep 16, 2026

Copy link
Copy Markdown
Author

@jcsf it looks like the reader accepts messages of any length (with or without this patch)
Which probably needs a follow up to fix.

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.

3 participants