fix(engineioxide): enforce max_payload on the websocket transport - #762
fix(engineioxide): enforce max_payload on the websocket transport#762schulzfel wants to merge 1 commit into
Conversation
Sorry, something went wrong.
max_payload was only enforced for the polling transport (http request bodies). Websocket connections rode tungstenite's defaults (~64 MiB message / 16 MiB frame), so the configured ceiling was silently unenforced for websocket clients. Map the same ceiling onto inbound websocket frames (max_frame_size) and reassembled messages (max_message_size), giving both transports identical payload enforcement. An oversized message now closes the connection with a transport error instead of being delivered.
Totodore
left a comment
There was a problem hiding this comment.
Thanks for the PR,
However I think we should provide two different options for those two params (frame_size and message_size). These don't have exactly the same implication than max_payload setting.
Also could you propagate this to the socketioxide builder (see https://docs.rs/socketioxide/latest/socketioxide/struct.SocketIoBuilder.html#method.ws_read_buffer_size).
Sorry, something went wrong.
| // Apply the configured `max_payload` ceiling to inbound websocket | ||
| // frames/messages, matching the polling transport. Without this, | ||
| // tungstenite's defaults (~64 MiB message / 16 MiB frame) apply and | ||
| // `max_payload` is silently unenforced for websocket clients. |
There was a problem hiding this comment.
| // Apply the configured `max_payload` ceiling to inbound websocket | |
| // frames/messages, matching the polling transport. Without this, | |
| // tungstenite's defaults (~64 MiB message / 16 MiB frame) apply and | |
| // `max_payload` is silently unenforced for websocket clients. |
Useless AI-comments
Sorry, something went wrong.
|
Independent confirmation from a second downstream, and an offer to finish the requested changes. We hit this while porting the signalling server of AnotherCrewLink (proximity voice chat for Among Us) to socketioxide 0.18.6 / engineioxide 0.17.6. We reached the same three lines independently before finding this PR, which is why I am commenting rather than opening a duplicate issue. One detail worth adding to the case: the gap is widest exactly in the configuration this crate encourages. Our server is transports([TransportType::Websocket]) — both of our shipping clients connect that way, and refusing polling removes an advisory surface — so max_payload ends up enforced on a transport we do not mount at all. We set 64 KiB; the effective inbound ceiling is tungstenite's 64 MiB. A factor of a thousand, and the configured number survives only as an advertisement in the OPEN packet that a hostile client ignores. Two things that make it unfixable outside the crate, in case they help justify the change:
That leaves a process memory limit as the only real backstop, which turns a hostile message into a restart instead of a refusal. We currently carry it as an accepted risk with MemoryMax=512M in our systemd unit, and documented for operators, because there is no configuration line that closes it. On the review feedback: separate options for frame size and message size is the right call — reusing max_payload conflates a per-frame parse bound with a reassembled-message bound, and only the second is what an operator setting a payload budget usually means. If it would help, I am happy to contribute that shape — ws_max_frame_size and ws_max_message_size on EngineIoConfig, each defaulting to max_payload so existing behaviour is unchanged for anyone who sets neither, plus the matching pair on SocketIoBuilder next to ws_read_buffer_size — either as a patch to @schulzfel's branch if they prefer, or as a follow-up PR crediting this one. Just say which you would rather have; I did not want to fork the work without asking. Disclosure: this comment was written by Claude, an AI assistant, working on the downstream project named above, and posted from my account. Every version number, file path and line number in it was checked against the vendored crate sources rather than recalled. — @greluc |
Sorry, something went wrong.
|
@greluc I'll publish a PR with the new options during the week |
Sorry, something went wrong.
|
Ok, thank you very much! :-) |
Sorry, something went wrong.
Problem
EngineIoConfig.max_payload is only enforced for the polling transport (HTTP request bodies). Websocket connections are initialized with tungstenite's default WebSocketConfig, which allows ~64 MiB messages / 16 MiB frames — so the configured payload ceiling is silently unenforced for websocket clients. A server operator who sets max_payload to bound inbound payloads is only actually protected on polling.
Fix
Apply the same ceiling to inbound websocket traffic in ws::on_init:
max_frame_size bounds each frame at parse time and max_message_size bounds reassembled fragmented messages, giving both transports identical enforcement. An oversized message now closes the connection with a capacity error (surfaced as DisconnectReason::TransportError) instead of being delivered to the handler.
The max_payload doc comment is updated to mention websocket.
Tests
New crates/engineioxide/tests/ws_max_payload.rs (runs with --features __test_harness like the other integration tests):
The second test fails without the ws.rs change and passes with it.
Notes
Downstream we currently carry this as a vendored two-line patch on 0.17.1 to safely enable the websocket transport behind a strict payload budget; happy to adjust anything (naming, error surface, backport) to get payload parity landed upstream.