Skip to content

fix(envoy-client): remove native websocket input limits#5396

Merged
NathanFlurry merged 2 commits into
mainfrom
fix-ws-size
Jul 11, 2026
Merged

fix(envoy-client): remove native websocket input limits#5396
NathanFlurry merged 2 commits into
mainfrom
fix-ws-size

Conversation

@NathanFlurry

@NathanFlurry NathanFlurry commented Jul 10, 2026

Copy link
Copy Markdown
Member
  • Remove Tungstenite's native envoy frame and aggregate message input limits so incident-sized engine frames are accepted.
  • Cover the production WebSocket configuration and a binary frame larger than 16 MiB with focused envoy-client tests.
  • Add a Guard-to-native-envoy boundary test that sends 16 MiB minus 64 KiB and then 16 MiB plus 64 KiB, verifies both complete echoes, and asserts the envoy stays connected.
  • Update the shared normal runner-config fixture for the current optional API fields so the standalone integration target compiles.
  • Reproduce the original failure without the fix by proving the below-limit control succeeds before the above-limit payload triggers MessageTooLong, then verify both pass with the unlimited native configuration restored.
  • Verify with cargo test -p rivet-envoy-client, cargo check -p rivetkit-core, cargo check -p rivetkit-napi, and cargo test -p rivet-engine --test envoy_large_payload.

@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5396 July 10, 2026 13:35 Destroyed
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review

Main concern: unbounded WebSocket message/frame size at an untrusted boundary

websocket_config() in engine/sdks/rust/envoy-client/src/connection/native.rs sets both max_message_size and max_frame_size to None, i.e. fully unbounded, for the WebSocket connection that rivetkit-core (the actor runtime, "envoy") uses to talk to pegboard-envoy. Per this repo's Trust Boundaries (CLAUDE.md), the envoy/pegboard-envoy hop is explicitly untrusted, and this is the only size check on that hop; nothing else in envoy-client or pegboard-envoy enforces a cap on incoming frame/message size.

By contrast, guard-core's proxy_service.rs (the client-facing WebSocket boundary) solves the same "reject legitimate large frames" problem by using a large but bounded, configurable limit (DEFAULT_WEBSOCKET_MAX_MESSAGE_SIZE / DEFAULT_WEBSOCKET_MAX_FRAME_SIZE, both set to 32 MiB, overridable via config) rather than disabling the check entirely. Since Guard already caps at 32 MiB, raising the tungstenite default here to something similarly bounded (large enough to cover legitimate payloads, ideally sourced from config) would fix the original incident, frames just over the old 16 MiB default being rejected, without removing the only backstop against a misbehaving or compromised peer on either side of this boundary sending an arbitrarily large single message and exhausting memory. Worth mirroring Guard's pattern here instead of None.

Test placement: new file should live under tests/envoy/

engine/packages/engine/tests/envoy_large_payload.rs is added as a new top-level integration test binary. engine/CLAUDE.md says: "Write new actor-hosting engine tests under engine/packages/engine/tests/envoy/; do not add new legacy runner tests under engine/packages/engine/tests/runner/." This test sets up an envoy plus actor via common::setup_envoy/common::create_actor, so per that convention it should be engine/packages/engine/tests/envoy/envoy_large_payload.rs with a pub mod envoy_large_payload; entry added to tests/envoy/mod.rs, rather than a new standalone top-level file that duplicates the #[path = "common/mod.rs"] boilerplate other top-level files use.

Minor: new inline #[cfg(test)] mod tests in native.rs

CLAUDE.md's testing rule keeps Rust tests out of inline #[cfg(test)] mod tests in src/, with an exception for testing private internals unreachable from integration tests. websocket_config() is private, so the first unit test (websocket_config_has_no_input_size_limits) is a reasonable exception. The second test (receives_frame_larger_than_default_limit) doesn't need private access beyond calling that same helper; it could be expressed as an integration test under engine/sdks/rust/envoy-client/tests/ (which already exists, e.g. command_dedup.rs) by constructing the same WebSocketConfig inline with public tungstenite APIs. Not a blocker, just a nit given the crate already has a tests/ directory for this kind of coverage.

Nits

  • test_helpers.rs changes look like an unrelated fixture fixup (new optional RunnerConfigKind::Normal fields) needed just to keep the crate compiling. Fine as a drive-by fix, but worth calling out explicitly in the PR description since it's otherwise unrelated to the size-limit fix.

Positives

  • Good regression coverage: the new tests explicitly reproduce the original bug, a payload just below the old default limit succeeding, one just above it failing, before showing both pass with the fix, which is exactly the right pattern for this kind of fix.
  • The Guard-to-native-envoy end-to-end test is a nice belt-and-suspenders check that the envoy connection stays alive across the boundary.

@railway-app

railway-app Bot commented Jul 10, 2026

Copy link
Copy Markdown

🚅 Deployed to the rivet-pr-5396 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink ✅ Success (View Logs) Web Jul 11, 2026 at 10:14 pm
website ✅ Success (View Logs) Web Jul 11, 2026 at 10:12 pm
frontend-cloud 😴 Sleeping (View Logs) Web Jul 11, 2026 at 8:48 pm
frontend-inspector 😴 Sleeping (View Logs) Web Jul 11, 2026 at 6:52 pm
ladle ✅ Success (View Logs) Web Jul 10, 2026 at 1:37 pm
mcp-hub ✅ Success (View Logs) Web Jul 10, 2026 at 1:35 pm

@NathanFlurry NathanFlurry force-pushed the fix-ws-size branch 2 times, most recently from b50153f to 7d55b4f Compare July 10, 2026 14:23
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5396 July 10, 2026 14:23 Destroyed
@NathanFlurry NathanFlurry changed the base branch from main to rivetkit-2.3.3-rc.1-pin July 10, 2026 14:35
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5396 July 10, 2026 14:38 Destroyed
@NathanFlurry NathanFlurry changed the base branch from rivetkit-2.3.3-rc.1-pin to main July 10, 2026 14:38
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5396 July 11, 2026 22:01 Destroyed
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5396 July 11, 2026 22:09 Destroyed
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5396 July 11, 2026 22:14 Destroyed
@NathanFlurry NathanFlurry marked this pull request as ready for review July 11, 2026 22:14
@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Review

Good fix for a real bug (Tungstenite's default 16 MiB message limit disconnecting the native envoy client), and nice to see both a unit test and a guard-to-envoy integration test added. A few things worth addressing before merge:

Correctness / repo conventions

  1. Commit message violates an explicit CLAUDE.md rule. Commit c4c500b2 is titled [SLOP(gpt-5)] test(envoy-client): reproduce oversized guard payload disconnect. CLAUDE.md is explicit: "Never indicate that a change was written by a coding agent: no model name, no agent name, no [SLOP(...)] prefix in the title, body, or PR text." This should be reworded to a plain conventional-commit title before merge.

  2. New integration test is misplaced. engine/packages/engine/tests/envoy_large_payload.rs is added directly under tests/, but engine/CLAUDE.md says: "Write new actor-hosting engine tests under engine/packages/engine/tests/envoy/; do not add new legacy runner tests under engine/packages/engine/tests/runner/." All the other guard/actor-hosting tests (actors_lifecycle.rs, api_actors_create.rs, etc.) live in tests/envoy/ and are wired in via tests/mod.rs's mod envoy;. As written, this new file compiles as its own separate top-level test binary instead of being aggregated with the rest of the envoy suite -- it should move to tests/envoy/ and be registered in tests/envoy/mod.rs.

  3. Inline #[cfg(test)] mod tests in src/connection/native.rs contradicts the testing convention. Top-level CLAUDE.md: "Rust tests live under tests/, not inline #[cfg(test)] mod tests in src/... Exceptions must be justified (e.g., testing a private internal that can't be reached from an integration test)" and separately: "When moving Rust inline tests out of src/, keep a tiny source-owned #[cfg(test)] #[path = ...] mod tests; shim so the moved file still has private module access." The crate already has engine/sdks/rust/envoy-client/tests/. Since websocket_config() is private, the fix should either use the shim pattern to move these two tests into tests/, or make websocket_config() pub(crate) so it's testable from the existing integration test directory, rather than adding a new inline module.

Security / defense-in-depth

  1. max_message_size(None) / max_frame_size(None) removes the bound entirely rather than raising it. The envoy-to-pegboard-envoy leg (which this connection traverses, via guard) is explicitly called out in CLAUDE.md as an untrusted boundary that must be validated. Guard itself never goes unbounded: engine/packages/config/src/config/guard.rs defines DEFAULT_WEBSOCKET_MAX_MESSAGE_SIZE/DEFAULT_WEBSOCKET_MAX_FRAME_SIZE = 32 MiB, configurable but always bounded, and guard-core/src/proxy_service.rs applies that bound on both the client-facing and guard-to-upstream legs. So today the practical exposure is limited since Guard is still capping at 32 MiB, but this client-side change removes the local backstop entirely instead of aligning it to a large-but-bounded value (e.g. matching or slightly exceeding Guard's configured max). If envoy-client is ever used against an endpoint that doesn't route through Guard, or Guard's config is loosened, there is now no local ceiling and a malicious/compromised peer could force unbounded in-memory buffering of a single WS message. Recommend picking a generous explicit bound instead of None, consistent with the "always bound, but generously" pattern already used elsewhere in the codebase.

  2. Minor/no-op note: the WASM transport (connection/wasm.rs) uses the raw browser WebSocket API, which has no equivalent size-limit knob, so no parity change is needed there -- just flagging that this was checked, not a gap.

Test coverage

  • The new integration test only exercises payloads up to 16 MiB + 64 KiB, comfortably under Guard's 32 MiB default, so it validates the specific regression being fixed but doesn't exercise behavior near/above Guard's own limit. Worth a follow-up (not blocking) to add a case that intentionally exceeds Guard's configured limit and asserts a clean rejection, to lock in the boundary once item 4 above is addressed.
  • rivetkit-rust/packages/client/src/remote_manager.rs (connect_async at lines 582/629) has plain, unconfigured Tungstenite defaults and looks susceptible to the same MessageTooLong bug class. Out of scope for this PR, but worth a tracked follow-up per the "Engine runner parity" expectations in CLAUDE.md if that client talks to the same endpoints.

Unrelated-but-necessary change

  • engine/packages/engine/tests/common/test_helpers.rs's RunnerConfigKind::Normal fields and drain_on_version_upgrade: Some(true) update looks like an unrelated API-schema compile fix bundled into this PR (needed to keep the standalone integration target compiling against current RunnerConfig). Fine to keep bundled given the small size, just noting it's not part of the actual bug fix.

Overall: the core fix direction is right, but please fix the commit message before merge, relocate/reshape the tests per the repo's stated conventions, and reconsider using an explicit bounded limit instead of fully unbounded for the untrusted envoy-to-engine leg.

@NathanFlurry NathanFlurry merged commit a1519da into main Jul 11, 2026
10 of 12 checks passed
@NathanFlurry NathanFlurry deleted the fix-ws-size branch July 11, 2026 22:17
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.

1 participant