feat(inputs): server-side input-validation seam (ValidateInputs)

[mmannerm]

Summary

A minimal, ECS-capable seam for game-side input validation on the server, per the design discussed in #1531. A normal Bevy system, with full SystemParam access, runs between message receipt and input buffering and may mutate or drop received InputMessages in place.

MessageSystems::Receive  →  InputSystems::ValidateInputs (user systems)  →  InputSystems::ReceiveInputs (buffer apply)

No new resource and no rewrite of receive_input_message — systems intercept the existing MessageReceiver<InputMessage<S>> (a Vec of received messages). No behavior change by default: the seam is empty until a game registers a validator.

This PR is an API seam, not a spoofed-target security fix. It does not enforce target authorization by default — ControlledBy stays an optional, non-authoritative helper (per the maintainer on #1531). A game that wants target authorization implements it on the seam (see the test below). #1526 remains unresolved — the decision there is whether lightyear ships default protection, an opt-in helper, or docs/example only. An interim revision of this PR had a compulsory AuthorizeInputs pass; it was removed (it made ControlledBy required and broke existing input tests).

What's here

• MessageReceiver::retain_messages(|&mut M| -> bool) — mutate and/or drop buffered messages in place, before they're consumed. In-place preserves per-message metadata (remote_tick, channel_kind, message_id) — unlike drain-then-re-push.
• MessageReceiver::retain_received_messages(|MessageMetadata, &mut M| -> bool) — same, but the predicate also gets the per-message metadata (remote_tick / channel_kind / message_id) read-only (only the data is &mut), for rate-limit / tick-window / replay / per-channel validators.
• InputSystems::ValidateInputs — empty-by-default set, ordered after MessageSystems::Receive and before InputSystems::ReceiveInputs.
• app.add_input_validator(system) — schedules a plain system into ValidateInputs (full ECS access).

fn reject_inputs(
    reject: Res<RejectInputs>,                        // arbitrary game state
    mut receivers: Query<&mut MessageReceiver<InputMessage<MySequence>>>,
) {
    if !reject.0 { return; }
    for mut r in &mut receivers {
        r.retain_messages(|_msg| false);              // clamp/inspect instead, normally
    }
}
app.add_input_validator(reject_inputs);

Tests

• test_input_validator_system_can_drop_messages — a validator with ECS access (Res) drops messages via retain_messages; the input never reaches the server ActionState.
• test_user_validator_can_authorize_targets — a game-supplied validator implements ControlledBy-based target authorization (drops InputTarget::Entity the sender doesn't control). Client 0 forges a target on an uncontrolled entity: the authorized input still lands (non-overblocking) and the spoofed one is dropped. Demonstrates the maintainer's "users can implement authorization themselves in ValidateInputs" point.
• test_validator_can_read_message_metadata — a validator reads a message's remote_tick via retain_received_messages (read-only MessageMetadata) and drops it; asserts the metadata was observable and the drop took effect.

Notes

• receive_input_message is unchanged, so the rebroadcast and rollback-mismatch machinery is untouched.
• fix(inputs): bound InputMessage end_tick lookahead (DoS) #1525 (end_tick bound) is merged and stays inline. feat(inputs): opt-in authorize_controlled_targets helper (spoofed-target defense) #1526 (target authorization) remains the place to decide whether lightyear enforces target ownership by default.
• Supersedes the closed feat(inputs): pluggable validation hook for received input messages #1529 (in-system Box<dyn> chain) — this gives full ECS access.

Discussion / rationale: #1531.

🤖 Generated with Claude Code
Co-authored with Claude Opus 4.8

[mmannerm]

Co-authored critical pass from me + OpenAI Codex, refreshed at #1535 08ee8e04 and #1526 c3a9cdcf.

Current read: the PR pair now matches the design we were converging on. #1535 is a policy-free validation seam; #1526 is the opt-in spoofed-target helper built on that seam. I do not see a remaining PR-level architectural blocker.

What changed since the previous pass

• feat(inputs): opt-in authorize_controlled_targets helper (spoofed-target defense) #1526 PR body now correctly says emptied input messages are kept as keepalives, not dropped.
• feat(inputs): opt-in authorize_controlled_targets helper (spoofed-target defense) #1526 now has coverage for the host-client is_local exemption, PreSpawned pass-through, and custom validators ordered after authorize_controlled_targets.
• feat(inputs): server-side input-validation seam (ValidateInputs) #1535 PR body now lists the metadata-read test.
• feat(inputs): server-side input-validation seam (ValidateInputs) #1535 documents that validators in ValidateInputs are unordered unless the caller supplies explicit ordering.

Remaining findings

[P3] #1531's design-record text is now stale in a couple of places.
The issue body still says retain_received_messages(|&mut ReceivedMessage<M>| -> bool) in the resolved metadata note, but the actual API is now retain_received_messages(|MessageMetadata, &mut M| -> bool). It also still lists auth-helper telemetry as an open question even though #1526 now emits a trace! when it strips targets. This is not a code blocker, but the RFC should be refreshed so future readers do not reconstruct the wrong API contract.

[P3] The #1526 audit/status comment appears stale relative to the current head.
The audit comment says the green run was at ff8bcd6d, while the current #1526 head is c3a9cdcf after the validator-ordering docs/test commit. Since that last change includes a new test, I would re-run/update the audit marker before merge. I did not independently re-run the full test suite in this refresh.

Verdict

#1535 looks ready from an API-design standpoint: retain_messages covers simple mutation/drop, retain_received_messages covers metadata-aware validation without allowing metadata mutation, and ValidateInputs gives games/anti-cheat systems full ECS access without forcing Lightyear policy.

#1526 also looks right after the latest fixes: it highlights the spoofed-target security issue, stays opt-in, strips only unauthorized entity targets, keeps emptied keepalive messages, documents ordering for follow-up validators, and now covers the important branch behavior. Merge order should remain #1535 first, then #1526 rebased down to the helper/tests.

Co-authored-by: OpenAI Codex codex@openai.com

[mmannerm]

Audit: server-side input-validation seam (#1535)

Verdict: [APPROVE] — re-run at 08ee8e04. History: BLOCK (default-on authorization) → decoupled (a716a7e5); retain_received_messages added (e4ba6144) → refined to a read-only MessageMetadata view (a4237230); validator-ordering docs (08ee8e04, docs-only). The full bug/security/simplicity/test-quality lenses were run at e4ba6144/a4237230; the delta since is documentation only, so they still hold. Merge is the maintainer's call; remaining items are non-blocking.

Build / test / lint (re-run at 08ee8e04)

• build: ✓ · fmt: ✓ · clippy (-D warnings, --all-targets): ✓
• test (cargo test -p lightyear_tests --lib client_server::input, serial): ✓ 20 passed, 0 failed.
• Intent: PR body matches the diff — lists all three seam tests (drop / user-authorization / metadata-read).

Resolution

The compulsory AuthorizeInputs set + authorize_input_targets system were removed. Per the maintainer (#1531, 16:01), ControlledBy is an optional helper, not a required input component — and enabling authorization by default broke 8 existing tests that don't set it. #1535 now ships the seam only (retain_messages + ValidateInputs + add_input_validator), a no-behavior-change addition. Authorization is demonstrated as a user-space validator in test_user_validator_can_authorize_targets, which also asserts the authorized input still lands (closing the non-overblocking gap) and exercises the retain_messages per-target mutation path.

Findings (all non-blocking)

Original BLOCK (resolved) + the lenses

• [resolved] Test gate / intent: default-on AuthorizeInputs made ControlledBy compulsory and broke 8 tests; undisclosed breaking change. Fixed by decoupling.
• Code review: no ≥80%-confidence logic bugs (retain_messages metadata, ordering, host-client bypass, entity-map edge all sound). Corroborated by @mmannerm + Codex's review on this PR.
• Security (invariants): lightyear trust boundary — the seam itself adds no default enforcement (intended); no rollback-determinism / server-authority / asset-load issue; no new vulnerability.

Simplicity (open): add_input_validator is thin sugar over add_systems(...).in_set(InputSystems::ValidateInputs) — keep for discoverability or drop for a smaller surface (maintainer's call).

Codex's review: metadata exposure → addressed (retain_received_messages with the read-only MessageMetadata view). The mutability-contract and add_input_validator naming points are resolved/maintainer's-call.

These are design-discussion points already raised on this PR / #1531; I have not auto-filed them as issues on the upstream repo.

---

Run by /audit-task. Re-runs edit this comment in place.

[mmannerm]

Ready for review/merge. This is the policy-free ValidateInputs seam — retain_messages, retain_received_messages (read-only MessageMetadata), and add_input_validator — with no default behavior change. Both your co-authored Codex review and the /audit-task pass are at APPROVE with no open findings.

Suggested merge order: this first, then #1526 rebases down to just the authorize_controlled_targets helper + tests.

CI: Doc + Lint green; the full Test job is running (the suite passes locally — cargo test -p lightyear_tests --lib client_server::input, 20/20).

🤖 Co-authored with Claude Code (Claude Opus 4.8)