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
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).
| // 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
|
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 One detail worth adding to the case: the gap is widest exactly in the configuration this crate encourages. Our server is 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 On the review feedback: separate options for frame size and message size is the right call — reusing 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 |
|
@greluc I'll publish a PR with the new options during the week |
|
Ok, thank you very much! :-) |
|
Supersed by #775 |
Problem
EngineIoConfig.max_payloadis only enforced for the polling transport (HTTP request bodies). Websocket connections are initialized with tungstenite's defaultWebSocketConfig, which allows ~64 MiB messages / 16 MiB frames — so the configured payload ceiling is silently unenforced for websocket clients. A server operator who setsmax_payloadto bound inbound payloads is only actually protected on polling.Fix
Apply the same ceiling to inbound websocket traffic in
ws::on_init:max_frame_sizebounds each frame at parse time andmax_message_sizebounds reassembled fragmented messages, giving both transports identical enforcement. An oversized message now closes the connection with a capacity error (surfaced asDisconnectReason::TransportError) instead of being delivered to the handler.The
max_payloaddoc comment is updated to mention websocket.Tests
New
crates/engineioxide/tests/ws_max_payload.rs(runs with--features __test_harnesslike the other integration tests):ws_message_within_max_payload_is_delivered— messages under the limit still reach the handler.ws_message_exceeding_max_payload_closes_the_connection— an oversized message disconnects withTransportErrorand never reaches the handler.The second test fails without the
ws.rschange 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.