From 2832419dfbd3679cc68499b3c51d5ab265a5144d Mon Sep 17 00:00:00 2001 From: mpettibone <94031664+mpettibone@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:18:06 -0600 Subject: [PATCH] Apply max_fragmented_message_size to the first fragment of a message `max_fragmented_message_size` isn't checked against the first fragment of a message. A client can open a fragmented message with a frame far larger than the limit and Bandit will hold it, unchecked, until the client decides to send the next frame. Only then is the limit applied and the connection closed. The way it works today: `dispatch_frame/3` checks the limit in both `Frame.Continuation` clauses, adding the new frame to the running `fragment_size` and closing with 1009 if the total is over. The two clauses for a text or binary frame with FIN clear, the ones that start a fragmented message, store the frame and record its size without a check. So with `max_fragmented_message_size: 1024` and the default 8 MB `max_frame_size`, a 7 MB opening fragment is accepted and sits in `fragment_frame` for as long as the client cares to leave it there. The first continuation frame, even a one byte one, triggers the 1009. Why fix it. The limit's job is to bound the memory a fragmented message can occupy while it's being reassembled, and a frame with FIN clear is by definition the start of such a message, so "already over the limit" is known the moment the frame arrives. Deferring the check until the next frame means the one party who decides when the check happens is the client, and a client that never sends the next frame is never checked at all. There's also a plain correctness point: the option is documented as the maximum size of a message delivered across multiple frames, and a message of 7 MB and counting is over a 1 KB maximum whether or not the second frame has shown up yet. I don't think it rises to the security process. The opening fragment is still capped by `max_frame_size`, which the extractor enforces before the frame reaches this code, and an operator who allows a frame of that size allows it unfragmented too, so the memory a client can park per connection is a bound the operator already chose. What's wrong is the timing and the fact that one configured limit silently doesn't apply to the first frame. That's a bug fix, so it's a PR. The change is small. The two FIN-clear clauses now go through a `start_fragmented_message/3` that runs the existing `oversize_message?/2` on the first fragment. Over the limit it closes with 1009 and the same "Received oversize fragmented message" error the continuation path uses; otherwise it stores the fragment exactly as before. The continuation clauses are untouched, and so is the non-fragmented path. Tests: two new ones alongside the existing `max_fragmented_message_size` tests, one text and one binary, each sending a 1,010 byte opening fragment against a 1,000 byte limit and expecting the 1009 straight away, with the WebSock's terminate seeing the same reason as for an oversize continuation. Both fail on `main`, where nothing happens until a continuation arrives. Suite, format, credo and dialyzer are clean. --- lib/bandit/websocket/connection.ex | 19 ++++++++++++---- test/bandit/websocket/protocol_test.exs | 30 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/lib/bandit/websocket/connection.ex b/lib/bandit/websocket/connection.ex index cbfa563ed..f6c78cd9a 100644 --- a/lib/bandit/websocket/connection.ex +++ b/lib/bandit/websocket/connection.ex @@ -110,8 +110,7 @@ defmodule Bandit.WebSocket.Connection do end %Frame.Text{fin: false} = frame -> - {:continue, - %{connection | fragment_frame: frame, fragment_size: IO.iodata_length(frame.data)}} + start_fragmented_message(frame, socket, connection) %Frame.Binary{fin: true, compressed: true} = frame -> do_inflate(frame, socket, connection) @@ -121,8 +120,7 @@ defmodule Bandit.WebSocket.Connection do |> handle_continuation(socket, connection) %Frame.Binary{fin: false} = frame -> - {:continue, - %{connection | fragment_frame: frame, fragment_size: IO.iodata_length(frame.data)}} + start_fragmented_message(frame, socket, connection) frame -> handle_control_frame(frame, socket, connection) @@ -186,6 +184,19 @@ defmodule Bandit.WebSocket.Connection do end end + # The first fragment of a message counts towards max_fragmented_message_size like the ones that + # follow it, so one larger than the limit is rejected as it arrives rather than being held until + # the next frame arrives and the limit is checked + defp start_fragmented_message(frame, socket, connection) do + fragment_size = IO.iodata_length(frame.data) + + if oversize_message?(fragment_size, connection.opts) do + do_error(1009, "Received oversize fragmented message", socket, connection) + else + {:continue, %{connection | fragment_frame: frame, fragment_size: fragment_size}} + end + end + defp maybe_coalesce_fragments(%{fragment_pending_count: count} = connection) when count >= @max_pending_fragments do chunk = IO.iodata_to_binary(connection.fragment_pending) diff --git a/test/bandit/websocket/protocol_test.exs b/test/bandit/websocket/protocol_test.exs index b9cfd6c51..0b93cb67a 100644 --- a/test/bandit/websocket/protocol_test.exs +++ b/test/bandit/websocket/protocol_test.exs @@ -354,6 +354,36 @@ defmodule WebSocketProtocolTest do assert output =~ "Received oversize fragmented message" end + + # The first fragment is subject to the limit as well. It used to be stored unchecked, and the + # limit only applied once a continuation frame arrived + for {opcode, send_frame} <- [text: :send_text_frame, binary: :send_binary_frame] do + test "max_fragmented_message_size enforced for an opening #{opcode} fragment", context do + output = + capture_log(fn -> + context = + http_server(context, websocket_options: [max_fragmented_message_size: 1_000]) + + client = SimpleWebSocketClient.tcp_client(context) + SimpleWebSocketClient.http1_handshake(client, TerminateWebSock) + + payload = String.duplicate("0123456789", 101) + apply(SimpleWebSocketClient, unquote(send_frame), [client, payload, 0x0]) + + # Get the error that terminate saw, to ensure we're closing for the expected reason + assert_receive {:error, "Received oversize fragmented message"}, 500 + + assert SimpleWebSocketClient.recv_connection_close_frame(client) == + {:ok, <<1009::16>>} + + # Verify that the server didn't send any extraneous frames + assert SimpleWebSocketClient.connection_closed_for_reading?(client) + Process.sleep(500) + end) + + assert output =~ "Received oversize fragmented message" + end + end end describe "compressed frames" do