Skip to content

feat(net): cap payload accepted before bridge hello - #111

Merged
AlxcBTW merged 2 commits into
anyplaynet:mainfrom
fogydes:feat/bridge-cap-payload-before-hello
Oct 27, 2026
Merged

AlxcBTW merged 2 commits into
anyplaynet:mainfrom
fogydes:feat/bridge-cap-payload-before-hello

Conversation

@fogydes

@fogydes fogydes commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

What and why

Caps the payload accepted before a bridge \hello\ frame in \crates/net/src/bridge.rs\ (\MAX_HELLO_PAYLOAD = 1024\ bytes).

  • A bridge \hello\ carries only a 2-byte version and a token (a 32-byte secret), so 1024 bytes is ample.
  • If a connection announces or streams an oversized payload before a valid \hello, or declares a payload length exceeding \MAX_HELLO_PAYLOAD, the connection is closed immediately without buffering up to \�nyplay_bridge_protocol::MAX_PAYLOAD\ (16 MiB).
  • Includes unit tests in \�ridge.rs\ covering: valid hello acceptance, refusal of oversized payload, and immediate refusal on oversized declared header length.

Checklist

  • \cargo fmt --all -- --check, \cargo clippy --workspace --all-targets -- -D warnings\ and \cargo test --workspace\ pass.
  • New behaviour has a hermetic test (own servers on loopback, no internet, no game).
  • A change to a specification or a decision comes with an ADR in \docs/adr/.
  • A change to the bridge protocol (\spec/bridge-protocol.md, \crates/bridge-protocol) states its wire effect: the version, and whether the AnyPS5 fork has to change with it.
  • It works for every game. No title ID in code; what is known per title is in specifications and tests.
  • It stays on the right side of ADR 0021, and no game data is attached: no dumps, keys, firmware, saves, tokens, or logs with game memory.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@AlxcBTW AlxcBTW left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks fago, this is the right idea and the tests check real behaviour (small hello passes, oversized payload and oversized declared length are closed). Three things before merge:

  1. The limit is visible on the wire, so it belongs in spec/bridge-protocol.md (Connection section, next to the hello description): hello payload is at most 1024 bytes, a larger first frame closes the connection. The bridge in AnyPS5 must match that file, and the token length is not fixed there today, so please state the limit explicitly.
  2. The hand-rolled header sniffing in serve (header_buf, header_read, the 12 + MAX_HELLO_PAYLOAD running total) duplicates the framing logic and is easy to get subtly wrong. Please move the cap into the decoder in crates/bridge-protocol (for example Decoder::with_max_payload(n), default MAX_PAYLOAD, returning Error::TooLarge from the header alone) and use it for the pre-hello phase, then keep the 16 MiB decoder after hello. That also covers the 'announced length' case without extra state, and gets a unit test next to the existing TooLarge tests.
  3. Please add the boundary test: a payload of exactly MAX_HELLO_PAYLOAD bytes is accepted (or reaches the parse step), so an off-by-one cannot slip in. Also a test with the hello split over several tiny writes would be nice.

Everything else (timeout kept, Closed still reported) looks fine.

@fogydes

fogydes commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Updated per review:

  1. Documented the 1024-byte hello payload limit in spec/bridge-protocol.md (in the Connection overview and handshake table).
  2. Moved the cap directly into Decoder (with_max_payload and set_max_payload), so crates/net/src/bridge.rs uses the decoder limit without hand-rolled sniffing. Added corresponding TooLarge and boundary tests in crates/bridge-protocol/src/tests.rs.
  3. Added tests for exact boundary acceptance (1024 bytes payload) and hello split across tiny chunked writes.

@AlxcBTW AlxcBTW left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks fago, this is exactly what I hoped for: the cap lives in the decoder, the spec documents the 1024-byte limit, and the boundary and split-write tests are there. Merging once CI is green.

@AlxcBTW
AlxcBTW merged commit 73ffcaf into anyplaynet:main Oct 27, 2026
23 of 24 checks passed
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.

2 participants