From 064d51900f79c82a81767b9bc419201e8ff979f8 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 2 Aug 2026 22:40:10 -0700 Subject: [PATCH 1/3] fix(server): accept Interaction submissions over WebSocket (#6941) `ClientMessage` had no `Interaction` variant, so the server rejected every interaction submission at serde deserialization -- before any game logic ran -- with "unknown variant `Interaction`". PR #6778 wired the engine, WASM, and client halves of the attachment interaction fan but shipped no server half (`git show --stat a8766823cd -- crates/server-core` is empty). Live since v0.42.0: every interaction submission in a WebSocket multiplayer game failed. The client envelope was already correct -- `ClientMessage` is `#[serde(tag = "type", content = "data")]`, matching what `ws-adapter.ts` sends -- so no client file changes. - Add `ClientMessage::Interaction { submission }` and declare its wire policy in all three exhaustive `ClientMessage` matches. - Add `SessionManager::handle_interaction`, deriving the acting seat from the authenticated session token, never from the payload. `submit_interaction` then re-authorizes against the interaction slot inside the engine, so a forged id belonging to another seat is rejected twice. - Extract the `ClientMessage::Action` handler body into `handle_full_game_submission` so both wire variants share one authenticated, applied, and broadcast path rather than forking eight fan-out behaviours. - Answer interaction bounds failures on `ServerMessage::ActionRejected`, not `ServerMessage::Error`. The native client disposes its adapter on any `Error`, and a free-form `Text` response exceeding the 256-byte bound is reachable by an ordinary paste -- routing it to `Error` would end the match. `wire_rejection_message` makes that channel a per-variant wire policy. - Promote the engine's existing response bounds to a public `bound_interaction_submission` so the wire invokes the engine's own limits instead of restating them. Server-hosted draft matches remain blocked on a separate identity-binding defect (the draft adapter drops `DraftMatchStart.player_token`), which needs a client change and is filed separately. --- crates/engine/src/game/interaction.rs | 18 +- .../tests/integration/interaction_contract.rs | 84 ++ crates/phase-server/src/main.rs | 1315 ++++++++++++----- .../src/client_message_wire_guard.rs | 146 +- .../src/interaction_payload_guard.rs | 150 ++ crates/server-core/src/lib.rs | 3 +- crates/server-core/src/protocol.rs | 14 + crates/server-core/src/session.rs | 289 +++- 8 files changed, 1611 insertions(+), 408 deletions(-) create mode 100644 crates/server-core/src/interaction_payload_guard.rs diff --git a/crates/engine/src/game/interaction.rs b/crates/engine/src/game/interaction.rs index 0d769c6c2f..1c563d6d97 100644 --- a/crates/engine/src/game/interaction.rs +++ b/crates/engine/src/game/interaction.rs @@ -7924,6 +7924,21 @@ fn validate_response_bounds(response: &InteractionResponse) -> Result<(), Intera } } +/// Wire-boundary bounds for one inbound submission, evaluated without touching +/// game state. +/// +/// Transports call this at the wire so a rejection is answered before the +/// dispatcher does identity work; [`submit_interaction`] re-runs it via +/// [`resolve_interaction_response`], so no caller can skip it and no transport +/// can drift from these bounds by restating them. +pub fn bound_interaction_submission( + submission: &InteractionSubmission, +) -> Result<(), InteractionSubmitError> { + bound_string(submission.interaction_id.as_str())?; + validate_response_bounds(&submission.response)?; + Ok(()) +} + fn slot_for_submission<'a>( state: &'a GameState, actor: PlayerId, @@ -9321,8 +9336,7 @@ pub fn resolve_interaction_response( actor: PlayerId, submission: &InteractionSubmission, ) -> Result { - bound_string(submission.interaction_id.as_str())?; - validate_response_bounds(&submission.response)?; + bound_interaction_submission(submission)?; slot_for_submission(state, actor, &submission.interaction_id)?; let filtered = visibility::filter_state_for_viewer(state, actor); let (action, _) = materialize_response( diff --git a/crates/engine/tests/integration/interaction_contract.rs b/crates/engine/tests/integration/interaction_contract.rs index 30e8179b51..54211f8425 100644 --- a/crates/engine/tests/integration/interaction_contract.rs +++ b/crates/engine/tests/integration/interaction_contract.rs @@ -3021,3 +3021,87 @@ fn deck_partition_schema_publishes_an_interval_not_an_exact_deck_size() { 14 ); } + +/// The interaction contract omits a debug-capability gate at the transport +/// (`SessionManager::handle_interaction`) on the grounds that candidate +/// enumeration never produces one. This converts that "cannot happen" into +/// something that fails the day it starts happening. +/// +/// It asserts on the **client-visible** publication — `derive_viewer_interaction` +/// -> `opportunity_for_slot` -> `actor_candidates` -> `ai_support`'s validated +/// candidate set — rather than on an internal helper, so it covers what a +/// remote seat could actually submit. +/// +/// The sandbox capability is armed *fully* and deliberately: the claim is not +/// that debug actions are unreachable because sandbox mode is off, it is that +/// enumeration ignores the flag even when it is on. All three of +/// `allow_debug_actions`, `debug_mode`, and `debug_permitted` are set because +/// `apply`'s own gate requires the latter two together — arming only one would +/// leave the capability half-granted and the test could pass for the wrong +/// reason. +#[test] +fn published_interaction_choices_never_offer_a_debug_action_in_a_sandbox_game() { + let mut state = GameState::new_two_player(42); + state.format_config.allow_debug_actions = true; + state.debug_mode = true; + state.debug_permitted.insert(P0); + bind(&mut state, "sandbox-debug-enumeration"); + + let view = priority_view(&state); + + // Reach guard (1): a `ViewerInteraction` with `can_submit: false`, or a + // terminal `waiting_for`, publishes no opportunities at all and would + // satisfy the negative below vacuously. + assert!( + !view.opportunities.is_empty(), + "the fixture must publish something for the negative assertion to bite" + ); + + // Reach guard (3): the capability is genuinely in force at assertion time. + assert!( + state.format_config.allow_debug_actions + && state.debug_mode + && state.debug_permitted.contains(&P0), + "the sandbox capability must be armed, or this asserts nothing" + ); + + // Reach guard (2): `WaitingFor::Priority` maps to + // `HumanResponseModel::ExactCandidates`, which is the `actor_candidates` + // branch — the enumerator whose output this test is about. + assert!( + matches!(state.waiting_for, WaitingFor::Priority { .. }), + "the enumerating branch is selected by the waiting_for shape, got {:?}", + state.waiting_for + ); + + let mut saw_choices = false; + for opportunity in &view.opportunities { + let InteractionOpportunityResponse::ExactChoices { choices } = &opportunity.response else { + continue; + }; + saw_choices |= !choices.is_empty(); + for choice in choices { + for surface in &choice.surfaces { + if let InteractionPresentationSurface::Action { code, .. } = surface { + assert!( + !matches!( + code, + InteractionActionCode::Debug + | InteractionActionCode::GrantDebugPermission + | InteractionActionCode::RevokeDebugPermission + ), + "candidate enumeration published a debug action ({code:?}); \ + `SessionManager::handle_interaction`'s missing debug gate is \ + no longer safe" + ); + } + } + } + } + + assert!( + saw_choices, + "an ExactChoices opportunity with real choices is what proves the \ + actor_candidates path ran" + ); +} diff --git a/crates/phase-server/src/main.rs b/crates/phase-server/src/main.rs index 017a49221f..9eb250dee8 100644 --- a/crates/phase-server/src/main.rs +++ b/crates/phase-server/src/main.rs @@ -28,8 +28,10 @@ use engine::database::CardDatabase; use engine::game::derived_views::derive_filtered_views; use engine::game::interaction::{derive_viewer_interaction, object_action_payloads}; use engine::game::validate_name_deck_for_format_full; +use engine::types::actions::GameAction; use engine::types::events::GameEvent; use engine::types::game_state::GameState; +use engine::types::interaction::InteractionSubmission; use engine::types::player::PlayerId; use engine::types::GameLogEntry; use http::{HeaderMap, HeaderValue}; @@ -42,7 +44,7 @@ use seat_reducer::types::{DeckChoice, DeckResolver, ReducerCtx}; use server_core::ai_seats_wire_guard::{guard_create_ai_seats, MAX_FULL_GAME_PLAYER_COUNT}; use server_core::client_hello_guard::guard_client_hello; use server_core::client_message_wire_guard::{ - guard_broker_projection_inbound, guard_client_message_before_dispatch, + guard_broker_projection_inbound, guard_client_message_before_dispatch, wire_rejection_message, }; use server_core::draft_action_payload_guard::guard_draft_action_payload; use server_core::draft_session::{draft_seats_needing_auto_pick, DraftSessionManager}; @@ -56,6 +58,7 @@ use server_core::game_reconnect_guard::guard_game_reconnect; use server_core::game_state_snapshot_wire_guard::{ guard_game_state_for_broadcast, guard_state_snapshot_broadcast, StateSnapshotParts, }; +use server_core::interaction_payload_guard::guard_interaction_submission_payload; use server_core::legacy_deck_guard::guard_legacy_deck; use server_core::legacy_join_guard::guard_legacy_join_game; use server_core::lobby::RegisterGameRequest; @@ -865,6 +868,7 @@ fn reject_if_disabled(msg: &ClientMessage, mode: ServerMode) -> Option<&'static ClientMessage::CreateGame { .. } | ClientMessage::JoinGame { .. } | ClientMessage::Action { .. } + | ClientMessage::Interaction { .. } | ClientMessage::PreviewManaPayment { .. } | ClientMessage::Reconnect { .. } | ClientMessage::AbandonGame @@ -3535,6 +3539,452 @@ async fn broadcast_takeback_approved( } } +/// What a Full-mode game socket submitted: a client-materialized `GameAction`, +/// or an opaque engine-authored interaction response. Both are authenticated, +/// applied, and broadcast identically, so they share one handler. A bool or a +/// pair of `Option`s here would hide that the two are alternatives — and would +/// not give the payload-guard match below a total, wildcard-free form. +#[derive(Debug)] +enum GameSubmission { + Action(GameAction), + Interaction(InteractionSubmission), +} + +impl GameSubmission { + /// Wire-bounds this submission and, on failure, names the channel the + /// rejection is answered on. + /// + /// Defense in depth: `guard_client_message_before_dispatch` already ran + /// these exact bounds before dispatch, so in production this returns `Ok` + /// for every frame that reaches the handler — the same standing as the + /// `Action` guard it replaces. It is kept because the handler must not + /// assume its caller ran them. + /// + /// The channels here MUST agree with + /// `client_message_wire_guard::wire_rejection_message`, which answers the + /// same failure at the wire. An oversized `GameAction` is a malformed + /// frame — a client materializes actions from engine-published legal + /// actions and cannot produce one by accident. An oversized interaction + /// response is a rejected decision: `TextChoiceProjection::allow_arbitrary` + /// accepts free-form text and the bound is 256 bytes, so an ordinary paste + /// trips it and `ServerMessage::Error` would tear the session down. + /// `handler_payload_channels_agree_with_the_wire` pins that agreement + /// directly, by comparing this function's answer with + /// `wire_rejection_message`'s for the same payload. + /// + /// The `Err` is boxed because `ServerMessage` is ~13 KiB (it carries a + /// `GameState`), matching how this file already passes the type around + /// (`game_started_msg: Box`). + fn payload_rejection(&self) -> Result<(), Box> { + match self { + GameSubmission::Action(action) => guard_game_action_payload(action) + .map_err(|reason| Box::new(ServerMessage::error(reason))), + GameSubmission::Interaction(submission) => { + guard_interaction_submission_payload(submission) + .map_err(|reason| Box::new(ServerMessage::ActionRejected { reason })) + } + } + } +} + +/// Apply one authenticated game submission from a Full-mode game socket, then +/// broadcast the resulting state to every participant and spectator. +/// +/// Extracted verbatim from the `ClientMessage::Action` arm of +/// [`handle_client_message`] so that `ClientMessage::Interaction` can reuse the +/// identical authorization, application, and fan-out path instead of growing a +/// second ~400-line copy that would drift. +#[allow(clippy::too_many_arguments)] +async fn handle_full_game_submission( + submission: GameSubmission, + socket: &mut WebSocket, + state: &SharedState, + draft_state: &SharedDraftState, + connections: &SharedConnections, + game_db: &SharedGameDb, + game_spectators: &SharedGameSpectators, + // Read-only: this handler reads `game_code`, `player_token`, and `player_id` + // and mutates nothing. `&SocketIdentity` is deliberate, not an oversight -- + // `require_host`, `is_joining_current_game`, and their neighbours already + // take it by shared reference; only `dispatch_broker` and + // `handle_client_message` need `&mut`. Do not "fix" this to `&mut`. + identity: &SocketIdentity, +) { + let game_code = match &identity.game_code { + Some(c) => c.clone(), + None => { + warn!("Action received but not in a game"); + let msg = ServerMessage::error("Not in a game".to_string()); + if let Ok(json) = serde_json::to_string(&msg) { + let _ = socket.send(Message::text(json)).await; + } + return; + } + }; + let player_token = match &identity.player_token { + Some(t) => t.clone(), + None => { + let msg = ServerMessage::error("No player token".to_string()); + if let Ok(json) = serde_json::to_string(&msg) { + let _ = socket.send(Message::text(json)).await; + } + return; + } + }; + + debug!(game = %game_code, player = ?identity.player_id, submission = ?submission, "game submission"); + + // Bound client-supplied payload sizes before the clone-heavy engine + // reducers process them (mirrors guard_draft_action_payload for draft + // actions). The channel each variant answers on is declared by + // `GameSubmission::payload_rejection`. + if let Err(msg) = submission.payload_rejection() { + if let Ok(json) = serde_json::to_string(&msg) { + let _ = socket.send(Message::text(json)).await; + } + return; + } + + // Apply human action and collect AI follow-up results while holding the lock. + // Filtering is deferred until after the lock is dropped to reduce contention. + let action_result = { + let lock_start = std::time::Instant::now(); + let mut mgr = state.lock().await; + let applied = match submission { + GameSubmission::Action(action) => mgr.handle_action(&game_code, &player_token, action), + GameSubmission::Interaction(submission) => { + mgr.handle_interaction(&game_code, &player_token, submission) + } + }; + match applied { + Ok(human_result) => { + let human_revision = mgr + .sessions + .get_mut(&game_code) + .expect("handled action must retain its session") + .advance_state_revision(); + // Run AI follow-up actions (still inside lock — needs &mut state) + let ai_results = match mgr.sessions.get_mut(&game_code) { + Some(session) => session.run_ai(), + None => vec![], + }; + let session = mgr.sessions.get(&game_code).unwrap(); + let eliminated = session.state.eliminated_players.clone(); + let player_count = session.player_count; + let game_over_winner = match &session.state.waiting_for { + engine::types::game_state::WaitingFor::GameOver { winner } => Some(*winner), + _ => None, + }; + let terminal = if let Some(winner) = game_over_winner { + info!(game = %game_code, winner = ?winner, reason = "game_rules", "game over"); + let ranked_result = ranked_duel_players(session).and_then(|players| { + ranked_result_for_duel(game_db, &game_code, &players, winner) + }); + terminal_artifact(session, winner, "Game ended".to_string(), ranked_result) + .map(Some) + } else { + persist_full_session_async(game_db, session); + Ok(None) + }; + + let lock_ms = lock_start.elapsed().as_millis(); + info!( + game = %game_code, + lock_ms, + ai_actions = ai_results.len(), + "action processed (lock held)" + ); + + terminal.map(|terminal| { + ( + human_revision, + human_result, + ai_results, + eliminated, + player_count, + game_over_winner, + terminal, + ) + }) + } + Err(e) => Err(e), + } + }; // lock dropped — filtering happens below without blocking other games + + match action_result { + Ok(( + human_revision, + ( + raw_state, + events, + legal_actions, + log_entries, + _auto_pass_rec, + spell_costs, + legal_actions_by_object, + ), + ai_results, + eliminated, + player_count, + game_over_winner, + terminal, + )) => { + if let Err(reason) = guard_state_snapshot_broadcast(StateSnapshotParts { + state: &raw_state, + events: &events, + log_entries: &log_entries, + legal_actions: &legal_actions, + legal_actions_by_object: &legal_actions_by_object, + spell_costs: &spell_costs, + }) { + warn!(game = %game_code, %reason, "action snapshot too large to broadcast"); + let msg = ServerMessage::error(reason); + if let Ok(json) = serde_json::to_string(&msg) { + let _ = socket.send(Message::text(json)).await; + } + return; + } + + let terminal_deliveries = match terminal { + Some(artifact) => match prepare_full_terminal(game_db, artifact).await { + Ok(deliveries) => deliveries, + Err(error) => { + error!(game = %game_code, %error, "terminal preparation failed"); + let msg = ServerMessage::error(error); + if let Ok(json) = serde_json::to_string(&msg) { + let _ = socket.send(Message::text(json)).await; + } + return; + } + }, + None => Vec::new(), + }; + + // Filter state per-player outside the lock + let filtered_states: Vec<(PlayerId, GameState)> = (0..player_count) + .map(|i| { + let pid = PlayerId(i); + (pid, server_core::filter_state_for_player(&raw_state, pid)) + }) + .collect(); + + // Broadcast human action result + { + let conns = connections.lock().await; + if let Some(players) = conns.get(&game_code) { + for (pid, pstate) in &filtered_states { + if let Some(s) = players.get(pid) { + let is_actor = server_core::is_acting(&raw_state, *pid); + let player_legals = if ai_results.is_empty() && is_actor { + legal_actions.clone() + } else { + // AI will act next — don't send legal actions yet + vec![] + }; + let p_auto_pass = if ai_results.is_empty() { + engine_auto_pass_for_viewer(&raw_state, *pid, &legal_actions) + } else { + false + }; + let p_end_continuous_effect_offers = + engine_end_continuous_effect_offers(&player_legals); + let p_mana_payment_shortcut_actions = + if ai_results.is_empty() && is_actor { + engine_mana_payment_shortcut_actions( + &raw_state, + &legal_actions_by_object, + ) + } else { + Vec::new() + }; + let p_spell_costs = if ai_results.is_empty() && is_actor { + spell_costs.clone() + } else { + HashMap::new() + }; + let p_by_object = if ai_results.is_empty() && is_actor { + legal_actions_by_object.clone() + } else { + HashMap::new() + }; + let _ = s.send(ServerMessage::StateUpdate { + state_revision: human_revision, + state: pstate.clone(), + events: server_core::filter_events_for_player( + &events, &raw_state, *pid, + ), + legal_actions: player_legals, + auto_pass_recommended: p_auto_pass, + end_continuous_effect_offers: p_end_continuous_effect_offers, + mana_payment_shortcut_actions: p_mana_payment_shortcut_actions, + eliminated_players: eliminated.clone(), + log_entries: log_entries.clone(), + spell_costs: p_spell_costs, + legal_actions_by_object: object_action_payloads(&p_by_object), + derived: derive_transport_views(&raw_state, pstate, Some(*pid)), + viewer_interaction: derive_viewer_interaction( + &raw_state, pstate, *pid, + ), + }); + } + } + } + } + if let Ok(spectator_msg) = build_spectator_state_update_message( + &raw_state, + &events, + &log_entries, + human_revision, + ) { + let mut specs = game_spectators.lock().await; + if let Some(spectators) = specs.get_mut(&game_code) { + spectators.retain(|sender| sender.send(spectator_msg.clone()).is_ok()); + if spectators.is_empty() { + specs.remove(&game_code); + } + } + } + + // Broadcast AI follow-up results with delays + for (i, (ai_revision, result)) in ai_results.iter().enumerate() { + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + let ( + ai_raw_state, + ai_events, + ai_legal, + ai_log_entries, + _ai_auto_pass, + ai_spell_costs, + ai_by_object, + ) = result; + if guard_state_snapshot_broadcast(StateSnapshotParts { + state: ai_raw_state, + events: ai_events, + log_entries: ai_log_entries, + legal_actions: ai_legal, + legal_actions_by_object: ai_by_object, + spell_costs: ai_spell_costs, + }) + .is_err() + { + continue; + } + let is_last = i == ai_results.len() - 1; + + // Filter AI state per-player outside the lock + let ai_filtered: Vec<(PlayerId, GameState)> = (0..player_count) + .map(|j| { + let pid = PlayerId(j); + (pid, server_core::filter_state_for_player(ai_raw_state, pid)) + }) + .collect(); + + let conns = connections.lock().await; + if let Some(players) = conns.get(&game_code) { + for (pid, pstate) in &ai_filtered { + if let Some(s) = players.get(pid) { + let is_actor = server_core::is_acting(ai_raw_state, *pid); + let player_legals = if is_last && is_actor { + ai_legal.clone() + } else { + vec![] + }; + let p_auto_pass = if is_last { + engine_auto_pass_for_viewer(ai_raw_state, *pid, ai_legal) + } else { + false + }; + let p_end_continuous_effect_offers = + engine_end_continuous_effect_offers(&player_legals); + let p_mana_payment_shortcut_actions = if is_last && is_actor { + engine_mana_payment_shortcut_actions(ai_raw_state, ai_by_object) + } else { + Vec::new() + }; + let p_spell_costs = if is_last && is_actor { + ai_spell_costs.clone() + } else { + HashMap::new() + }; + let p_by_object = if is_last && is_actor { + ai_by_object.clone() + } else { + HashMap::new() + }; + let _ = s.send(ServerMessage::StateUpdate { + state_revision: *ai_revision, + state: pstate.clone(), + events: server_core::filter_events_for_player( + ai_events, + ai_raw_state, + *pid, + ), + legal_actions: player_legals, + auto_pass_recommended: p_auto_pass, + end_continuous_effect_offers: p_end_continuous_effect_offers, + mana_payment_shortcut_actions: p_mana_payment_shortcut_actions, + eliminated_players: eliminated.clone(), + log_entries: ai_log_entries.clone(), + spell_costs: p_spell_costs, + legal_actions_by_object: object_action_payloads(&p_by_object), + derived: derive_transport_views(ai_raw_state, pstate, Some(*pid)), + viewer_interaction: derive_viewer_interaction( + ai_raw_state, + pstate, + *pid, + ), + }); + } + } + } + let (ai_raw_state, ai_events, _, ai_log_entries, _, _, _) = result; + if let Ok(spectator_msg) = build_spectator_state_update_message( + ai_raw_state, + ai_events, + ai_log_entries, + *ai_revision, + ) { + let mut specs = game_spectators.lock().await; + if let Some(spectators) = specs.get_mut(&game_code) { + spectators.retain(|sender| sender.send(spectator_msg.clone()).is_ok()); + if spectators.is_empty() { + specs.remove(&game_code); + } + } + } + } + + if !terminal_deliveries.is_empty() { + let conns = connections.lock().await; + if let Some(players) = conns.get(&game_code) { + for (player, delivery) in &terminal_deliveries { + if let Some(sender) = players.get(player) { + let _ = sender.send(ServerMessage::TerminalResult { + delivery: Some(delivery.clone()), + }); + } + } + } + drop(conns); + report_draft_game_over( + draft_state, + connections, + &game_code, + game_over_winner.flatten(), + ) + .await; + state.lock().await.remove_game(&game_code); + } + } + Err(e) => { + let msg = ServerMessage::ActionRejected { reason: e }; + if let Ok(json) = serde_json::to_string(&msg) { + let _ = socket.send(Message::text(json)).await; + } + } + } +} + #[allow(clippy::too_many_arguments)] async fn handle_client_message( client_msg: ClientMessage, @@ -3632,7 +4082,10 @@ async fn handle_client_message( } if let Err(reason) = guard_client_message_before_dispatch(&client_msg, mode) { - let msg = ServerMessage::error(reason); + // The answer channel is wire policy, declared per variant alongside the + // bounds themselves. Every variant except `Interaction` keeps today's + // `ServerMessage::error`. + let msg = wire_rejection_message(&client_msg, reason); if let Ok(json) = serde_json::to_string(&msg) { let _ = socket.send(Message::text(json)).await; } @@ -3895,404 +4348,31 @@ async fn handle_client_message( } ClientMessage::Action { action } => { - let game_code = match &identity.game_code { - Some(c) => c.clone(), - None => { - warn!("Action received but not in a game"); - let msg = ServerMessage::error("Not in a game".to_string()); - if let Ok(json) = serde_json::to_string(&msg) { - let _ = socket.send(Message::text(json)).await; - } - return; - } - }; - let player_token = match &identity.player_token { - Some(t) => t.clone(), - None => { - let msg = ServerMessage::error("No player token".to_string()); - if let Ok(json) = serde_json::to_string(&msg) { - let _ = socket.send(Message::text(json)).await; - } - return; - } - }; - - debug!(game = %game_code, player = ?identity.player_id, action = ?action, "Action"); - - // Bound client-supplied action payload sizes before the clone-heavy - // engine reducers process them (mirrors guard_draft_action_payload - // for draft actions). - if let Err(reason) = guard_game_action_payload(&action) { - let msg = ServerMessage::error(reason); - if let Ok(json) = serde_json::to_string(&msg) { - let _ = socket.send(Message::text(json)).await; - } - return; - } - - // Apply human action and collect AI follow-up results while holding the lock. - // Filtering is deferred until after the lock is dropped to reduce contention. - let action_result = { - let lock_start = std::time::Instant::now(); - let mut mgr = state.lock().await; - match mgr.handle_action(&game_code, &player_token, action) { - Ok(human_result) => { - let human_revision = mgr - .sessions - .get_mut(&game_code) - .expect("handled action must retain its session") - .advance_state_revision(); - // Run AI follow-up actions (still inside lock — needs &mut state) - let ai_results = match mgr.sessions.get_mut(&game_code) { - Some(session) => session.run_ai(), - None => vec![], - }; - let session = mgr.sessions.get(&game_code).unwrap(); - let eliminated = session.state.eliminated_players.clone(); - let player_count = session.player_count; - let game_over_winner = match &session.state.waiting_for { - engine::types::game_state::WaitingFor::GameOver { winner } => { - Some(*winner) - } - _ => None, - }; - let terminal = if let Some(winner) = game_over_winner { - info!(game = %game_code, winner = ?winner, reason = "game_rules", "game over"); - let ranked_result = ranked_duel_players(session).and_then(|players| { - ranked_result_for_duel(game_db, &game_code, &players, winner) - }); - terminal_artifact( - session, - winner, - "Game ended".to_string(), - ranked_result, - ) - .map(Some) - } else { - persist_full_session_async(game_db, session); - Ok(None) - }; - - let lock_ms = lock_start.elapsed().as_millis(); - info!( - game = %game_code, - lock_ms, - ai_actions = ai_results.len(), - "action processed (lock held)" - ); - - terminal.map(|terminal| { - ( - human_revision, - human_result, - ai_results, - eliminated, - player_count, - game_over_winner, - terminal, - ) - }) - } - Err(e) => Err(e), - } - }; // lock dropped — filtering happens below without blocking other games - - match action_result { - Ok(( - human_revision, - ( - raw_state, - events, - legal_actions, - log_entries, - _auto_pass_rec, - spell_costs, - legal_actions_by_object, - ), - ai_results, - eliminated, - player_count, - game_over_winner, - terminal, - )) => { - if let Err(reason) = guard_state_snapshot_broadcast(StateSnapshotParts { - state: &raw_state, - events: &events, - log_entries: &log_entries, - legal_actions: &legal_actions, - legal_actions_by_object: &legal_actions_by_object, - spell_costs: &spell_costs, - }) { - warn!(game = %game_code, %reason, "action snapshot too large to broadcast"); - let msg = ServerMessage::error(reason); - if let Ok(json) = serde_json::to_string(&msg) { - let _ = socket.send(Message::text(json)).await; - } - return; - } - - let terminal_deliveries = match terminal { - Some(artifact) => match prepare_full_terminal(game_db, artifact).await { - Ok(deliveries) => deliveries, - Err(error) => { - error!(game = %game_code, %error, "terminal preparation failed"); - let msg = ServerMessage::error(error); - if let Ok(json) = serde_json::to_string(&msg) { - let _ = socket.send(Message::text(json)).await; - } - return; - } - }, - None => Vec::new(), - }; - - // Filter state per-player outside the lock - let filtered_states: Vec<(PlayerId, GameState)> = (0..player_count) - .map(|i| { - let pid = PlayerId(i); - (pid, server_core::filter_state_for_player(&raw_state, pid)) - }) - .collect(); - - // Broadcast human action result - { - let conns = connections.lock().await; - if let Some(players) = conns.get(&game_code) { - for (pid, pstate) in &filtered_states { - if let Some(s) = players.get(pid) { - let is_actor = server_core::is_acting(&raw_state, *pid); - let player_legals = if ai_results.is_empty() && is_actor { - legal_actions.clone() - } else { - // AI will act next — don't send legal actions yet - vec![] - }; - let p_auto_pass = if ai_results.is_empty() { - engine_auto_pass_for_viewer( - &raw_state, - *pid, - &legal_actions, - ) - } else { - false - }; - let p_end_continuous_effect_offers = - engine_end_continuous_effect_offers(&player_legals); - let p_mana_payment_shortcut_actions = - if ai_results.is_empty() && is_actor { - engine_mana_payment_shortcut_actions( - &raw_state, - &legal_actions_by_object, - ) - } else { - Vec::new() - }; - let p_spell_costs = if ai_results.is_empty() && is_actor { - spell_costs.clone() - } else { - HashMap::new() - }; - let p_by_object = if ai_results.is_empty() && is_actor { - legal_actions_by_object.clone() - } else { - HashMap::new() - }; - let _ = s.send(ServerMessage::StateUpdate { - state_revision: human_revision, - state: pstate.clone(), - events: server_core::filter_events_for_player( - &events, &raw_state, *pid, - ), - legal_actions: player_legals, - auto_pass_recommended: p_auto_pass, - end_continuous_effect_offers: - p_end_continuous_effect_offers, - mana_payment_shortcut_actions: - p_mana_payment_shortcut_actions, - eliminated_players: eliminated.clone(), - log_entries: log_entries.clone(), - spell_costs: p_spell_costs, - legal_actions_by_object: object_action_payloads( - &p_by_object, - ), - derived: derive_transport_views( - &raw_state, - pstate, - Some(*pid), - ), - viewer_interaction: derive_viewer_interaction( - &raw_state, pstate, *pid, - ), - }); - } - } - } - } - if let Ok(spectator_msg) = build_spectator_state_update_message( - &raw_state, - &events, - &log_entries, - human_revision, - ) { - let mut specs = game_spectators.lock().await; - if let Some(spectators) = specs.get_mut(&game_code) { - spectators.retain(|sender| sender.send(spectator_msg.clone()).is_ok()); - if spectators.is_empty() { - specs.remove(&game_code); - } - } - } - - // Broadcast AI follow-up results with delays - for (i, (ai_revision, result)) in ai_results.iter().enumerate() { - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - let ( - ai_raw_state, - ai_events, - ai_legal, - ai_log_entries, - _ai_auto_pass, - ai_spell_costs, - ai_by_object, - ) = result; - if guard_state_snapshot_broadcast(StateSnapshotParts { - state: ai_raw_state, - events: ai_events, - log_entries: ai_log_entries, - legal_actions: ai_legal, - legal_actions_by_object: ai_by_object, - spell_costs: ai_spell_costs, - }) - .is_err() - { - continue; - } - let is_last = i == ai_results.len() - 1; - - // Filter AI state per-player outside the lock - let ai_filtered: Vec<(PlayerId, GameState)> = (0..player_count) - .map(|j| { - let pid = PlayerId(j); - (pid, server_core::filter_state_for_player(ai_raw_state, pid)) - }) - .collect(); - - let conns = connections.lock().await; - if let Some(players) = conns.get(&game_code) { - for (pid, pstate) in &ai_filtered { - if let Some(s) = players.get(pid) { - let is_actor = server_core::is_acting(ai_raw_state, *pid); - let player_legals = if is_last && is_actor { - ai_legal.clone() - } else { - vec![] - }; - let p_auto_pass = if is_last { - engine_auto_pass_for_viewer(ai_raw_state, *pid, ai_legal) - } else { - false - }; - let p_end_continuous_effect_offers = - engine_end_continuous_effect_offers(&player_legals); - let p_mana_payment_shortcut_actions = if is_last && is_actor { - engine_mana_payment_shortcut_actions( - ai_raw_state, - ai_by_object, - ) - } else { - Vec::new() - }; - let p_spell_costs = if is_last && is_actor { - ai_spell_costs.clone() - } else { - HashMap::new() - }; - let p_by_object = if is_last && is_actor { - ai_by_object.clone() - } else { - HashMap::new() - }; - let _ = s.send(ServerMessage::StateUpdate { - state_revision: *ai_revision, - state: pstate.clone(), - events: server_core::filter_events_for_player( - ai_events, - ai_raw_state, - *pid, - ), - legal_actions: player_legals, - auto_pass_recommended: p_auto_pass, - end_continuous_effect_offers: - p_end_continuous_effect_offers, - mana_payment_shortcut_actions: - p_mana_payment_shortcut_actions, - eliminated_players: eliminated.clone(), - log_entries: ai_log_entries.clone(), - spell_costs: p_spell_costs, - legal_actions_by_object: object_action_payloads( - &p_by_object, - ), - derived: derive_transport_views( - ai_raw_state, - pstate, - Some(*pid), - ), - viewer_interaction: derive_viewer_interaction( - ai_raw_state, - pstate, - *pid, - ), - }); - } - } - } - let (ai_raw_state, ai_events, _, ai_log_entries, _, _, _) = result; - if let Ok(spectator_msg) = build_spectator_state_update_message( - ai_raw_state, - ai_events, - ai_log_entries, - *ai_revision, - ) { - let mut specs = game_spectators.lock().await; - if let Some(spectators) = specs.get_mut(&game_code) { - spectators - .retain(|sender| sender.send(spectator_msg.clone()).is_ok()); - if spectators.is_empty() { - specs.remove(&game_code); - } - } - } - } + handle_full_game_submission( + GameSubmission::Action(action), + socket, + state, + draft_state, + connections, + game_db, + game_spectators, + identity, + ) + .await; + } - if !terminal_deliveries.is_empty() { - let conns = connections.lock().await; - if let Some(players) = conns.get(&game_code) { - for (player, delivery) in &terminal_deliveries { - if let Some(sender) = players.get(player) { - let _ = sender.send(ServerMessage::TerminalResult { - delivery: Some(delivery.clone()), - }); - } - } - } - drop(conns); - report_draft_game_over( - draft_state, - connections, - &game_code, - game_over_winner.flatten(), - ) - .await; - state.lock().await.remove_game(&game_code); - } - } - Err(e) => { - let msg = ServerMessage::ActionRejected { reason: e }; - if let Ok(json) = serde_json::to_string(&msg) { - let _ = socket.send(Message::text(json)).await; - } - } - } + ClientMessage::Interaction { submission } => { + handle_full_game_submission( + GameSubmission::Interaction(submission), + socket, + state, + draft_state, + connections, + game_db, + game_spectators, + identity, + ) + .await; } ClientMessage::Reconnect { @@ -7653,7 +7733,8 @@ mod issue_4548_full_create_tests { } } - async fn spawn_full_mode_server() -> (String, tokio::task::JoinHandle<()>, tempfile::TempDir) { + pub(super) async fn spawn_full_mode_server( + ) -> (String, tokio::task::JoinHandle<()>, tempfile::TempDir) { let temp_dir = tempfile::tempdir().expect("temp dir"); let game_db = Arc::new( persistence::GameDb::open( @@ -7691,7 +7772,7 @@ mod issue_4548_full_create_tests { (format!("ws://{addr}/ws"), handle, temp_dir) } - async fn recv_server_message(socket: &mut WebSocketStream) -> ServerMessage + pub(super) async fn recv_server_message(socket: &mut WebSocketStream) -> ServerMessage where S: AsyncRead + AsyncWrite + Unpin, { @@ -7926,6 +8007,404 @@ mod issue_4548_full_create_tests { } } +/// End-to-end coverage for the shared game-submission handler +/// ([`handle_full_game_submission`]) over a real socket. +/// +/// Before this module existed, `ClientMessage::Action` had **no** end-to-end +/// test through `handle_client_message` at all: every `ClientMessage::Action` +/// occurrence in this file's test modules exercised `reject_if_disabled` or +/// `classify_hello_gate` as pure functions. These tests are what gate the +/// extraction of that arm into a shared handler. +#[cfg(test)] +mod game_submission_tests { + use super::issue_4548_full_create_tests::{recv_server_message, spawn_full_mode_server}; + use super::*; + use engine::game::interaction::MAX_INTERACTION_STRING_LEN; + use engine::types::interaction::{InteractionChoiceId, InteractionId, InteractionResponse}; + use futures_util::SinkExt; + use server_core::game_action_payload_guard::MAX_ACTION_LIST_LEN; + use server_core::protocol::DeckData; + use tokio_tungstenite::tungstenite::Message as WsMessage; + use tokio_tungstenite::MaybeTlsStream; + use tokio_tungstenite::WebSocketStream; + + /// Connect, handshake, and create a two-seat game so the socket carries an + /// authenticated `SocketIdentity` with both a `game_code` and a + /// `player_token`. + /// + /// Seat 1 never joins, so the game never *starts* — which is exactly the + /// reachable surface these tests need: everything up to and including + /// `SessionManager`'s verdict and the `Err(e) => ActionRejected` arm. The + /// `Ok(..)` broadcast fan-out is not reachable over this harness, because + /// `spawn_full_mode_server` builds `AppState` with an empty + /// `CardDatabase::default()`. + async fn create_authenticated_game_socket( + url: String, + ) -> WebSocketStream> { + let (mut socket, _) = tokio_tungstenite::connect_async(url) + .await + .expect("connect"); + + assert!(matches!( + recv_server_message(&mut socket).await, + ServerMessage::ServerHello { .. } + )); + + let hello = ClientMessage::ClientHello { + client_version: env!("CARGO_PKG_VERSION").to_string(), + build_commit: build_commit().to_string(), + protocol_version: PROTOCOL_VERSION, + }; + socket + .send(WsMessage::Text( + serde_json::to_string(&hello).expect("hello json").into(), + )) + .await + .expect("send hello"); + + let create = ClientMessage::CreateGameWithSettings { + deck: DeckData::default(), + display_name: "Alice".to_string(), + public: false, + password: None, + timer_seconds: None, + player_count: 2, + match_config: Default::default(), + ai_seats: Vec::new(), + format_config: None, + room_name: None, + host_peer_id: None, + draft_metadata: None, + start_when_full: true, + ranked: false, + }; + socket + .send(WsMessage::Text( + serde_json::to_string(&create).expect("create json").into(), + )) + .await + .expect("send create"); + + let mut saw_created = false; + let mut saw_slots = false; + while !saw_created || !saw_slots { + match recv_server_message(&mut socket).await { + ServerMessage::GameCreated { .. } => saw_created = true, + ServerMessage::PlayerSlotsUpdate { .. } => saw_slots = true, + _ => {} + } + } + + socket + } + + /// Read frames until one is an `ActionRejected` or an `Error`, ignoring the + /// unrelated broadcasts the session emits. The enclosing + /// `tokio::time::timeout` is the failure mode, as in every sibling test. + async fn recv_submission_answer(socket: &mut WebSocketStream) -> ServerMessage + where + S: tokio::io::AsyncRead + tokio::io::AsyncWrite + Unpin, + { + loop { + let msg = recv_server_message(socket).await; + if matches!( + msg, + ServerMessage::ActionRejected { .. } | ServerMessage::Error { .. } + ) { + return msg; + } + } + } + + /// Gates the extraction of the `ClientMessage::Action` arm body into + /// [`handle_full_game_submission`]: an extraction that broke identity + /// extraction, the payload guard, the lock, `player_for_token`, or the + /// `Err(e) => ActionRejected` arm fails here. + /// + /// `GrantDebugPermission` is chosen over `PassPriority` deliberately. + /// `GameState::new` initializes `waiting_for: WaitingFor::Priority` with the + /// host holding priority, so a `PassPriority` from this socket may well be + /// *accepted* — which would make the assertion pass for the wrong reason. + /// The sandbox refusal, by contrast, is decidable without running the + /// engine: the action is Full-mode allowed, is a payloadless no-op in + /// `guard_game_action_payload`, and hits `handle_action`'s Grant/Revoke gate + /// *first* — before the seat check, before `debug_permitted`, and before + /// `apply` — where `format_config.allow_debug_actions` is `false` because + /// the wire sent `format_config: None` and `FormatConfig::standard()` sets + /// it to `false`. `handle_action`'s `Err(String)` is then answered verbatim, + /// with no `"Engine error: "` prefix. + #[tokio::test] + async fn action_frame_reaches_the_shared_submission_handler() { + let (url, server, _temp_dir) = spawn_full_mode_server().await; + let answer = tokio::time::timeout(Duration::from_secs(5), async { + let mut socket = create_authenticated_game_socket(url).await; + + let action = ClientMessage::Action { + action: GameAction::GrantDebugPermission { + player_id: PlayerId(1), + }, + }; + socket + .send(WsMessage::Text( + serde_json::to_string(&action).expect("action json").into(), + )) + .await + .expect("send action"); + + recv_submission_answer(&mut socket).await + }) + .await; + server.abort(); + + let answer = answer.expect("action frame was never answered"); + match answer { + ServerMessage::ActionRejected { reason } => { + assert_eq!(reason, "Sandbox mode is not enabled for this game"); + } + other => panic!("expected ActionRejected from the shared handler, got {other:?}"), + } + } + + fn submission(response: InteractionResponse) -> InteractionSubmission { + InteractionSubmission { + interaction_id: InteractionId("interaction-1".to_string()), + response, + } + } + + fn oversized_submission() -> InteractionSubmission { + submission(InteractionResponse::Text { + value: "x".repeat(MAX_INTERACTION_STRING_LEN + 1), + }) + } + + /// The real cross-check of the two independent channel declarations. + /// + /// Both `GameSubmission::payload_rejection` and `wire_rejection_message` + /// are pure functions, so they can be compared directly. An end-to-end + /// socket test structurally cannot do this: the wire guard returns before + /// dispatch, so no frame ever reaches both layers. + #[test] + fn handler_payload_channels_agree_with_the_wire() { + let oversized = oversized_submission(); + let oversized_action = GameAction::ReorderHand { + order: vec![engine::types::identifiers::ObjectId(1); MAX_ACTION_LIST_LEN + 1], + }; + + // (i) an oversized interaction answers on the benign channel. + let handler_reason = match *GameSubmission::Interaction(oversized.clone()) + .payload_rejection() + .expect_err("an oversized interaction is refused") + { + ServerMessage::ActionRejected { reason } => reason, + ref other => panic!("an oversized paste must not tear the session down: {other:?}"), + }; + + // (ii) an oversized action stays a malformed frame. + let action_reason = match *GameSubmission::Action(oversized_action.clone()) + .payload_rejection() + .expect_err("an oversized action is refused") + { + ServerMessage::Error { message, .. } => message, + ref other => panic!("an oversized action is a malformed frame, got {other:?}"), + }; + + // (iii) both layers agree, in variant *and* in reason string. + let wire_interaction = ClientMessage::Interaction { + submission: oversized, + }; + let wire_reason = + guard_client_message_before_dispatch(&wire_interaction, ServerMode::Full).unwrap_err(); + match wire_rejection_message(&wire_interaction, wire_reason) { + ServerMessage::ActionRejected { reason } => assert_eq!(reason, handler_reason), + other => panic!("wire and handler disagree on the interaction channel: {other:?}"), + } + + let wire_action = ClientMessage::Action { + action: oversized_action, + }; + let wire_action_reason = + guard_client_message_before_dispatch(&wire_action, ServerMode::Full).unwrap_err(); + match wire_rejection_message(&wire_action, wire_action_reason) { + ServerMessage::Error { message, .. } => assert_eq!(message, action_reason), + other => panic!("wire and handler disagree on the action channel: {other:?}"), + } + + // Reach guard: without it, a `payload_rejection` that always errored + // would satisfy (i) and (ii). + assert!( + GameSubmission::Interaction(submission(InteractionResponse::Choose { + choice_id: InteractionChoiceId("a".to_string()), + })) + .payload_rejection() + .is_ok() + ); + assert!(GameSubmission::Action(GameAction::PassPriority) + .payload_rejection() + .is_ok()); + } + + /// The revert-failing assertion for #6941. + /// + /// Before the fix, the frame never reaches a handler at all: serde answers + /// `ServerMessage::Error { message: "Invalid message: unknown variant + /// `Interaction` ..." }` — a different variant *and* a different string. + /// + /// Asserting the exact reason is the reach guard: only a frame that + /// traversed serde, `reject_if_disabled`, the wire guard, both identity + /// checks, `payload_rejection`, `state.lock()`, `player_for_token`, + /// `submit_interaction`, and `slot_for_submission`'s + /// `.ok_or(StaleInteraction)` can produce it. + #[tokio::test] + async fn interaction_frame_is_accepted_by_the_wire_schema() { + let (url, server, _temp_dir) = spawn_full_mode_server().await; + let answer = tokio::time::timeout(Duration::from_secs(5), async { + let mut socket = create_authenticated_game_socket(url).await; + + let frame = ClientMessage::Interaction { + submission: submission(InteractionResponse::Choose { + choice_id: InteractionChoiceId("no-such-choice".to_string()), + }), + }; + socket + .send(WsMessage::Text( + serde_json::to_string(&frame) + .expect("interaction json") + .into(), + )) + .await + .expect("send interaction"); + + recv_submission_answer(&mut socket).await + }) + .await; + server.abort(); + + let answer = answer.expect("interaction frame was never answered"); + match answer { + ServerMessage::ActionRejected { reason } => { + assert_eq!(reason, "Engine error: StaleInteraction"); + } + other => panic!("the wire schema must accept an Interaction frame, got {other:?}"), + } + } + + /// Scope note: this exercises the **wire** layer only — the guard returns + /// before dispatch, so the handler's `payload_rejection` is never reached + /// by this frame. It makes no claim about the handler layer; that agreement + /// is pinned by `handler_payload_channels_agree_with_the_wire`. + #[tokio::test] + async fn an_oversized_interaction_is_answered_on_the_benign_channel() { + let (url, server, _temp_dir) = spawn_full_mode_server().await; + let answers = tokio::time::timeout(Duration::from_secs(5), async { + let mut socket = create_authenticated_game_socket(url).await; + + let frame = ClientMessage::Interaction { + submission: oversized_submission(), + }; + socket + .send(WsMessage::Text( + serde_json::to_string(&frame) + .expect("oversized json") + .into(), + )) + .await + .expect("send oversized interaction"); + + let first = recv_submission_answer(&mut socket).await; + + // Liveness probe rather than a vacuous `!matches!`: a socket that + // had been answered with `ServerMessage::Error` would have been + // torn down client-side, so a second answer on the same socket is + // what proves the benign channel was used. + let bounded = ClientMessage::Interaction { + submission: submission(InteractionResponse::Choose { + choice_id: InteractionChoiceId("no-such-choice".to_string()), + }), + }; + socket + .send(WsMessage::Text( + serde_json::to_string(&bounded) + .expect("bounded json") + .into(), + )) + .await + .expect("send bounded interaction"); + + (first, recv_submission_answer(&mut socket).await) + }) + .await; + server.abort(); + + let (first, second) = answers.expect("oversized interaction was never answered"); + match first { + ServerMessage::ActionRejected { reason } => { + assert_eq!(reason, "Engine error: PayloadTooLarge"); + } + other => panic!("an oversized paste must not end the match, got {other:?}"), + } + assert!( + matches!(second, ServerMessage::ActionRejected { .. }), + "the socket must still be live after a bounds rejection, got {second:?}" + ); + } + + /// Pins the pre-session rows of the channel table, and is the discriminator + /// for `interaction_frame_is_accepted_by_the_wire_schema`: it proves that + /// test's `ActionRejected` came from an engine verdict rather than being + /// this handler's blanket answer. + #[tokio::test] + async fn a_game_submission_without_a_session_is_answered_on_the_error_channel() { + let (url, server, _temp_dir) = spawn_full_mode_server().await; + let answer = tokio::time::timeout(Duration::from_secs(5), async { + let (mut socket, _) = tokio_tungstenite::connect_async(url) + .await + .expect("connect"); + + assert!(matches!( + recv_server_message(&mut socket).await, + ServerMessage::ServerHello { .. } + )); + + let hello = ClientMessage::ClientHello { + client_version: env!("CARGO_PKG_VERSION").to_string(), + build_commit: build_commit().to_string(), + protocol_version: PROTOCOL_VERSION, + }; + socket + .send(WsMessage::Text( + serde_json::to_string(&hello).expect("hello json").into(), + )) + .await + .expect("send hello"); + + let frame = ClientMessage::Interaction { + submission: submission(InteractionResponse::Choose { + choice_id: InteractionChoiceId("a".to_string()), + }), + }; + socket + .send(WsMessage::Text( + serde_json::to_string(&frame) + .expect("interaction json") + .into(), + )) + .await + .expect("send interaction"); + + recv_submission_answer(&mut socket).await + }) + .await; + server.abort(); + + let answer = answer.expect("sessionless interaction was never answered"); + match answer { + ServerMessage::Error { message, .. } => assert_eq!(message, "Not in a game"), + other => panic!("a pre-session condition is not an engine verdict: {other:?}"), + } + } +} + #[cfg(test)] mod mode_gate_tests { use super::*; @@ -8093,6 +8572,40 @@ mod mode_gate_tests { assert!(reject_if_disabled(&m, ServerMode::Full).is_none()); } } + + fn interaction_frame() -> ClientMessage { + ClientMessage::Interaction { + submission: InteractionSubmission { + interaction_id: engine::types::interaction::InteractionId("i-1".to_string()), + response: engine::types::interaction::InteractionResponse::Choose { + choice_id: engine::types::interaction::InteractionChoiceId("a".to_string()), + }, + }, + } + } + + /// Both halves are required: either alone is satisfiable by a wrong + /// grouping. A LobbyOnly broker runs no engine and holds no + /// `SessionManager`, so it publishes no interactions and no client can hold + /// a live `interaction_id` against it — identical to `Action`. + #[test] + fn interaction_is_full_only() { + assert!(reject_if_disabled(&interaction_frame(), ServerMode::Full).is_none()); + assert!(reject_if_disabled(&interaction_frame(), ServerMode::LobbyOnly).is_some()); + } + + /// Supplies the other half of `server-core`'s + /// `broker_projection_accepts_an_interaction_without_bounding_it`: leaving + /// the projection guard unbounded for this variant is safe only because + /// nothing is ever cloned into the broker. + /// + /// The paired positive is required in the same test so a wholesale-`None` + /// regression in `to_lobby_client_message` cannot satisfy it. + #[test] + fn interaction_is_never_projected_into_the_lobby_broker() { + assert!(to_lobby_client_message(&interaction_frame()).is_none()); + assert!(to_lobby_client_message(&ClientMessage::SubscribeLobby).is_some()); + } } #[cfg(test)] diff --git a/crates/server-core/src/client_message_wire_guard.rs b/crates/server-core/src/client_message_wire_guard.rs index 26f497c7e4..b86c079438 100644 --- a/crates/server-core/src/client_message_wire_guard.rs +++ b/crates/server-core/src/client_message_wire_guard.rs @@ -25,9 +25,10 @@ use crate::draft_wire_guard::{ use crate::emote_guard::guard_emote; use crate::game_action_payload_guard::guard_game_action_payload; use crate::game_reconnect_guard::guard_game_reconnect; +use crate::interaction_payload_guard::guard_interaction_submission_payload; use crate::legacy_deck_guard::guard_legacy_deck; use crate::legacy_join_guard::guard_legacy_join_game; -use crate::protocol::{ClientMessage, ServerMode}; +use crate::protocol::{ClientMessage, ServerMessage, ServerMode}; use crate::seat_mutation_wire_guard::guard_seat_mutation; use crate::spectator_wire_guard::{guard_spectate_draft, guard_spectator_join}; @@ -50,6 +51,9 @@ pub fn guard_client_message_before_dispatch( ClientMessage::Action { action } | ClientMessage::PreviewManaPayment { action, .. } => { guard_game_action_payload(action) } + ClientMessage::Interaction { submission } => { + guard_interaction_submission_payload(submission) + } ClientMessage::Reconnect { game_code, player_token, @@ -193,6 +197,59 @@ pub fn guard_client_message_before_dispatch( } } +/// Answer a frame that [`guard_client_message_before_dispatch`] rejected, on +/// the channel that frame's variant declares. +/// +/// Exhaustive by design, like the two sibling matches in this module: a new +/// variant must declare not only *which* bounds apply at the wire, but *how a +/// rejection is answered*. The native client disposes its adapter on ANY +/// `ServerMessage::Error`, so any variant whose wire bounds a routine, +/// non-hostile client can trip MUST answer on `ActionRejected`. +pub fn wire_rejection_message(msg: &ClientMessage, reason: String) -> ServerMessage { + match msg { + // An oversized interaction response is reachable without hostility: + // `TextChoiceProjection::allow_arbitrary` accepts free-form text and + // `MAX_INTERACTION_STRING_LEN` is 256, so a long paste is a rejected + // decision, not a malformed frame. `ServerMessage::error` here would + // end the match on a paste. + ClientMessage::Interaction { .. } => ServerMessage::ActionRejected { reason }, + + // Every other variant keeps today's behavior exactly: a bounds failure + // on these is a malformed frame, not a rejected decision. + ClientMessage::ClientHello { .. } + | ClientMessage::CreateGame { .. } + | ClientMessage::JoinGame { .. } + | ClientMessage::Action { .. } + | ClientMessage::PreviewManaPayment { .. } + | ClientMessage::Reconnect { .. } + | ClientMessage::AbandonGame + | ClientMessage::SubscribeLobby + | ClientMessage::UnsubscribeLobby + | ClientMessage::CreateGameWithSettings { .. } + | ClientMessage::JoinGameWithPassword { .. } + | ClientMessage::LookupJoinTarget { .. } + | ClientMessage::Concede + | ClientMessage::ConcedeMatch + | ClientMessage::BootstrapTerminalDelivery { .. } + | ClientMessage::ReadTerminalResult { .. } + | ClientMessage::AckTerminalDelivery { .. } + | ClientMessage::Emote { .. } + | ClientMessage::SpectatorJoin { .. } + | ClientMessage::Ping { .. } + | ClientMessage::UpdateLobbyMetadata { .. } + | ClientMessage::SeatMutate { .. } + | ClientMessage::UnregisterLobby { .. } + | ClientMessage::CreateDraftWithSettings { .. } + | ClientMessage::JoinDraftWithPassword { .. } + | ClientMessage::DraftAction { .. } + | ClientMessage::ReconnectDraft { .. } + | ClientMessage::SpectateDraft { .. } + | ClientMessage::RequestTakeback + | ClientMessage::RespondTakeback { .. } + | ClientMessage::CancelTakeback => ServerMessage::error(reason), + } +} + /// Validate broker-projected lobby frames without constructing `LobbyClientMessage`. /// /// Used by `dispatch_broker` before `to_lobby_client_message` clones strings and @@ -269,6 +326,7 @@ pub fn guard_broker_projection_inbound(msg: &ClientMessage) -> Result<(), String ClientMessage::CreateGame { .. } | ClientMessage::JoinGame { .. } | ClientMessage::Action { .. } + | ClientMessage::Interaction { .. } | ClientMessage::PreviewManaPayment { .. } | ClientMessage::Reconnect { .. } | ClientMessage::AbandonGame @@ -298,6 +356,10 @@ mod tests { use engine::types::ability::{TriggerBaseSetInstanceRef, TriggerDefinitionOccurrenceRef}; use engine::types::game_state::ProductionOverride; use engine::types::identifiers::ObjectIncarnationRef; + use engine::types::interaction::{ + InteractionChoiceId, InteractionId, InteractionResponse, InteractionSubmission, + MAX_INTERACTION_LIST_LEN, + }; use engine::types::mana::{ ManaRestriction, ManaSourcePenalty, ManaSourceSelection, ManaType, TapsForManaSelection, }; @@ -427,4 +489,86 @@ mod tests { let err = guard_client_message_before_dispatch(&msg, ServerMode::Full).unwrap_err(); assert!(err.contains("game_code")); } + + fn interaction_frame(response: InteractionResponse) -> ClientMessage { + ClientMessage::Interaction { + submission: InteractionSubmission { + interaction_id: InteractionId("interaction-1".to_string()), + response, + }, + } + } + + fn oversized_interaction_frame() -> ClientMessage { + interaction_frame(InteractionResponse::Select { + choice_ids: vec![InteractionChoiceId("a".to_string()); MAX_INTERACTION_LIST_LEN + 1], + }) + } + + /// Direct sibling of `dispatch_guard_rejects_oversized_game_action_before_handler_work`. + #[test] + fn dispatch_guard_rejects_oversized_interaction_before_handler_work() { + let err = + guard_client_message_before_dispatch(&oversized_interaction_frame(), ServerMode::Full) + .unwrap_err(); + + assert!(err.contains("PayloadTooLarge"), "unexpected reason: {err}"); + } + + /// Non-vacuity guard for the test above, and a statement that the dispatch + /// guard does not double as the mode gate — that is `reject_if_disabled`'s + /// job. + #[test] + fn dispatch_guard_accepts_a_bounded_interaction() { + let msg = interaction_frame(InteractionResponse::Choose { + choice_id: InteractionChoiceId("a".to_string()), + }); + + assert!(guard_client_message_before_dispatch(&msg, ServerMode::Full).is_ok()); + assert!(guard_client_message_before_dispatch(&msg, ServerMode::LobbyOnly).is_ok()); + } + + /// Declared wire policy for the projection boundary: an interaction is a + /// game frame, so `to_lobby_client_message` returns `None` for it and + /// nothing is ever cloned into the broker. Unbounded is safe here *only* + /// because nothing is cloned — which is why this test is meaningless + /// without its pair, `interaction_is_never_projected_into_the_lobby_broker` + /// in `phase-server`. + #[test] + fn broker_projection_accepts_an_interaction_without_bounding_it() { + assert!(guard_broker_projection_inbound(&oversized_interaction_frame()).is_ok()); + } + + /// Both halves are required: the `Interaction` half alone would pass for a + /// function that answered `ActionRejected` for everything, which would + /// change `Action`'s behavior. + #[test] + fn interaction_wire_rejection_answers_on_the_benign_channel() { + let interaction = oversized_interaction_frame(); + let reason = + guard_client_message_before_dispatch(&interaction, ServerMode::Full).unwrap_err(); + + match wire_rejection_message(&interaction, reason.clone()) { + ServerMessage::ActionRejected { reason: answered } => { + assert_eq!(answered, reason); + } + other => panic!("an interaction rejection must not tear the session down: {other:?}"), + } + + let action = ClientMessage::Action { + action: GameAction::ReorderHand { + order: vec![ObjectId(1); MAX_ACTION_LIST_LEN + 1], + }, + }; + let action_reason = + guard_client_message_before_dispatch(&action, ServerMode::Full).unwrap_err(); + + assert!( + matches!( + wire_rejection_message(&action, action_reason), + ServerMessage::Error { .. } + ), + "an oversized action stays a malformed frame" + ); + } } diff --git a/crates/server-core/src/interaction_payload_guard.rs b/crates/server-core/src/interaction_payload_guard.rs new file mode 100644 index 0000000000..b1212828c8 --- /dev/null +++ b/crates/server-core/src/interaction_payload_guard.rs @@ -0,0 +1,150 @@ +//! Wire-payload bounds for inbound `InteractionSubmission` bodies on the native +//! WebSocket path. +//! +//! The engine owns these bounds +//! (`engine::game::interaction::bound_interaction_submission`) and re-runs them +//! inside `submit_interaction`. This module exists so +//! `guard_client_message_before_dispatch` can *declare* the variant's wire +//! policy in its exhaustive match, the way every other arm declares one, rather +//! than declaring `Ok(())` — "unbounded at the wire" — for a variant that +//! carries a client-controlled payload. +//! +//! It deliberately restates no limit of its own: the engine owns the bounds, and +//! a second copy here would drift the first time a response variant changes. +//! +//! The rejection string is byte-identical to `engine-wasm`'s +//! (`format!("Engine error: {:?}", code)`) and to +//! `SessionManager::handle_interaction`'s, so the same engine reason code reads +//! the same on the WASM and WebSocket transports and at every server layer that +//! reports one. + +use engine::game::interaction::bound_interaction_submission; +use engine::types::interaction::InteractionSubmission; + +/// Validate a client-supplied interaction submission before session dispatch. +pub fn guard_interaction_submission_payload( + submission: &InteractionSubmission, +) -> Result<(), String> { + bound_interaction_submission(submission) + .map_err(|error| format!("Engine error: {:?}", error.code)) +} + +#[cfg(test)] +mod tests { + use super::*; + use engine::game::interaction::MAX_INTERACTION_STRING_LEN; + use engine::types::interaction::{ + InteractionChoiceId, InteractionId, InteractionReasonCode, InteractionResponse, + InteractionShortcutDecision, InteractionShortcutPin, MAX_INTERACTION_LIST_LEN, + }; + + fn choice(id: &str) -> InteractionChoiceId { + InteractionChoiceId(id.to_string()) + } + + fn submission(response: InteractionResponse) -> InteractionSubmission { + InteractionSubmission { + interaction_id: InteractionId("interaction-1".to_string()), + response, + } + } + + /// Reach guard for every negative below: the guard is not rejecting + /// everything it is handed. + #[test] + fn accepts_a_realistic_submission() { + let msg = submission(InteractionResponse::Select { + choice_ids: vec![choice("a"), choice("b")], + }); + + assert_eq!(guard_interaction_submission_payload(&msg), Ok(())); + } + + #[test] + fn rejects_an_oversized_select_list() { + let msg = submission(InteractionResponse::Select { + choice_ids: vec![choice("a"); MAX_INTERACTION_LIST_LEN + 1], + }); + + assert!(guard_interaction_submission_payload(&msg).is_err()); + + // Boundary sibling: the bound sits at the constant, not at "any large + // list". + let at_limit = submission(InteractionResponse::Select { + choice_ids: vec![choice("a"); MAX_INTERACTION_LIST_LEN], + }); + assert_eq!(guard_interaction_submission_payload(&at_limit), Ok(())); + } + + /// The routine-reachable case: a `TextChoiceProjection` with + /// `allow_arbitrary` accepts free-form text, so an ordinary paste can + /// exceed the bound. This is why the rejection must not travel on + /// `ServerMessage::Error`. + #[test] + fn rejects_an_oversized_text_value() { + let msg = submission(InteractionResponse::Text { + value: "x".repeat(MAX_INTERACTION_STRING_LEN + 1), + }); + + assert!(guard_interaction_submission_payload(&msg).is_err()); + + let at_limit = submission(InteractionResponse::Text { + value: "x".repeat(MAX_INTERACTION_STRING_LEN), + }); + assert_eq!(guard_interaction_submission_payload(&at_limit), Ok(())); + } + + #[test] + fn rejects_an_oversized_interaction_id() { + let msg = InteractionSubmission { + interaction_id: InteractionId("x".repeat(MAX_INTERACTION_STRING_LEN + 1)), + response: InteractionResponse::Choose { + choice_id: choice("a"), + }, + }; + + assert!(guard_interaction_submission_payload(&msg).is_err()); + } + + /// The `OutboundBudget` cumulative path — the only nested branch in the + /// engine validator, and precisely what a naive per-field guard would miss: + /// no single pin is oversized, only their sum. + #[test] + fn rejects_an_oversized_nested_shortcut_pin_budget() { + let per_pin = MAX_INTERACTION_LIST_LEN / 2; + let pins: Vec = (0..3) + .map(|group| InteractionShortcutPin { + group, + choice_ids: vec![choice("a"); per_pin], + }) + .collect(); + + for pin in &pins { + assert!(pin.choice_ids.len() <= MAX_INTERACTION_LIST_LEN); + } + + let msg = submission(InteractionResponse::Shortcut { + decision: InteractionShortcutDecision::AcceptSuggested, + pins, + }); + + assert!(guard_interaction_submission_payload(&msg).is_err()); + } + + /// Pins the byte-identity claim at this layer so it cannot silently drift + /// from `engine-wasm/src/lib.rs`'s `format!("Engine error: {:?}", code)`. + #[test] + fn rejection_string_matches_the_wasm_transport() { + let msg = submission(InteractionResponse::Select { + choice_ids: vec![choice("a"); MAX_INTERACTION_LIST_LEN + 1], + }); + + assert_eq!( + guard_interaction_submission_payload(&msg), + Err(format!( + "Engine error: {:?}", + InteractionReasonCode::PayloadTooLarge + )) + ); + } +} diff --git a/crates/server-core/src/lib.rs b/crates/server-core/src/lib.rs index 83c2f1098b..96deb36a6c 100644 --- a/crates/server-core/src/lib.rs +++ b/crates/server-core/src/lib.rs @@ -12,6 +12,7 @@ pub mod game_reconnect_guard; pub mod game_state_snapshot_wire_guard; #[cfg(test)] mod harness; +pub mod interaction_payload_guard; pub mod legacy_deck_guard; pub mod legacy_join_guard; pub mod lobby; @@ -29,7 +30,7 @@ pub mod takeback; pub use ai_seats_wire_guard::guard_create_ai_seats; pub use client_hello_guard::guard_client_hello; pub use client_message_wire_guard::{ - guard_broker_projection_inbound, guard_client_message_before_dispatch, + guard_broker_projection_inbound, guard_client_message_before_dispatch, wire_rejection_message, }; pub use deck_resolve::resolve_deck; pub use draft_action_payload_guard::guard_draft_action_payload; diff --git a/crates/server-core/src/protocol.rs b/crates/server-core/src/protocol.rs index f8d96e3bf6..e435a1856d 100644 --- a/crates/server-core/src/protocol.rs +++ b/crates/server-core/src/protocol.rs @@ -6,6 +6,7 @@ use engine::types::events::GameEvent; use engine::types::format::FormatConfig; use engine::types::game_state::GameState; use engine::types::identifiers::ObjectId; +use engine::types::interaction::InteractionSubmission; use engine::types::log::GameLogEntry; use engine::types::mana::ManaCost; use engine::types::match_config::MatchConfig; @@ -157,6 +158,19 @@ pub enum ClientMessage { request_id: u64, action: GameAction, }, + /// One opaque, engine-authored interaction response. The client echoes the + /// submission the engine published in `ViewerInteraction`; it never derives + /// a `GameAction` from the opportunity schema. Like `Action`, the + /// authenticated session — not the payload — determines the acting seat. + /// + /// Unlike `Action`, a bounds rejection on this variant is answered on + /// `ServerMessage::ActionRejected`, not `ServerMessage::Error`: a free-form + /// `Text` response can exceed `MAX_INTERACTION_STRING_LEN` by an ordinary + /// paste, and the native client tears the session down on any `Error`. + /// See `client_message_wire_guard::wire_rejection_message`. + Interaction { + submission: InteractionSubmission, + }, Reconnect { game_code: String, player_token: String, diff --git a/crates/server-core/src/session.rs b/crates/server-core/src/session.rs index 8b33977ad0..2a92c374d3 100644 --- a/crates/server-core/src/session.rs +++ b/crates/server-core/src/session.rs @@ -8,7 +8,7 @@ use engine::database::CardDatabase; use engine::game::deck_loading::{DeckPayload, PlayerDeckPayload}; use engine::game::engine::{apply, start_game}; use engine::game::finalize_public_state; -use engine::game::interaction::bind_interaction_authority; +use engine::game::interaction::{bind_interaction_authority, submit_interaction}; use engine::game::match_flow::apply_trusted_match_forfeit; use engine::game::preview::preview_auto_payment_sources; use engine::game::{load_and_hydrate_decks, rehydrate_game_from_card_db}; @@ -17,7 +17,7 @@ use engine::types::events::GameEvent; use engine::types::format::FormatConfig; use engine::types::game_state::{GameState, PersistedGameState}; use engine::types::identifiers::ObjectId; -use engine::types::interaction::InteractionSessionId; +use engine::types::interaction::{InteractionSessionId, InteractionSubmission}; use engine::types::log::GameLogEntry; use engine::types::mana::ManaCost; use engine::types::match_config::MatchConfig; @@ -1400,6 +1400,116 @@ impl SessionManager { )) } + /// Apply one engine-authored interaction submission from a player. + /// + /// Deliberately shaped as the exact sibling of [`Self::handle_action`]: it + /// returns the same [`ActionResult`], so the transport's broadcast path is + /// shared rather than duplicated. + /// + /// The acting `PlayerId` is resolved from the join-token-authenticated + /// session, never from the payload — the wire frame carries no actor field + /// at all (`protocol.rs`: "The authenticated session, rather than the + /// client, determines the actor"). `submit_interaction` then re-authorizes + /// the actor against the interaction slot inside the engine, so a forged + /// `interaction_id` belonging to another seat is rejected twice. + /// + /// There is deliberately no debug-capability gate here, unlike + /// `handle_action`. `materialize_response` dispatches on + /// `human_response_model`, and its `Choose` fallthrough sources actions from + /// `actor_candidates` -> + /// `ai_support::validated_candidate_actions_for_semantic_owner`. An + /// engine-wide grep shows no production constructor of + /// `GameAction::Debug`, `GrantDebugPermission`, or `RevokeDebugPermission` + /// anywhere in that chain — only classification and ordering matches — so + /// guarding here would be validation for a case that cannot occur. The + /// engine test + /// `published_interaction_choices_never_offer_a_debug_action_in_a_sandbox_game` + /// (`crates/engine/tests/integration/interaction_contract.rs`) fails the day + /// that stops being true. + pub fn handle_interaction( + &mut self, + game_code: &str, + player_token: &str, + submission: InteractionSubmission, + ) -> Result { + let session = self + .sessions + .get_mut(game_code) + .ok_or_else(|| format!("Game not found: {game_code}"))?; + + let player = session + .player_for_token(player_token) + .ok_or_else(|| "Invalid player token".to_string())?; + + // GH #1507: the authoritative state must not move while the table is + // voting on a rollback. Same interlock, same reason, as `handle_action`. + if session.pending_takeback.is_some() { + return Err( + "A takeback request is pending — resolve it before taking further actions" + .to_string(), + ); + } + + // Snapshot BEFORE `log_player_names` is written, matching + // `handle_action`'s order exactly. The two handlers are siblings; the + // ordering is part of that. + // + // The clone is unconditional here where `handle_action`'s is + // conditional on `!action.is_actor_scoped_preference()`, because the + // action is not known until `submit_interaction` returns. That is + // equivalent, not a regression: the seven actions that predicate + // matches are UI preferences that no candidate enumerator or + // `materialize_*` function ever produces, so the predicate is always + // false on this path. The guard below is kept anyway so the two + // handlers stay structurally identical if that ever changes. + let pre_action_state = session.state.clone(); + + // Set player names for log resolution. + session.state.log_player_names = session.display_names.clone(); + + let applied = + submit_interaction(&mut session.state, player, submission).map_err(|error| { + warn!( + game = %game_code, + player = ?player, + code = ?error.code, + reason = "interaction_rejected", + "interaction rejected" + ); + // Byte-identical to the WASM transport + // (`engine-wasm/src/lib.rs`) and to + // `interaction_payload_guard`, so every layer that reports an + // engine reason code, on both transports, classifies the same + // rejection identically. + format!("Engine error: {:?}", error.code) + })?; + + if !applied.action.is_actor_scoped_preference() { + session.push_takeback_state(player, pre_action_state); + } + + info!( + game = %game_code, + player = ?player, + action_type = applied.action.variant_name(), + event_count = applied.result.events.len(), + "interaction applied" + ); + + let (new_legal_actions, spell_costs, by_object) = engine_legal_actions_full(&session.state); + let auto_pass = auto_pass_recommended(&session.state, &new_legal_actions); + + Ok(( + session.state.clone(), + applied.result.events, + new_legal_actions, + applied.result.log_entries, + auto_pass, + spell_costs, + by_object, + )) + } + /// Applies the payload-free match-concede intent after binding its /// requester to an authenticated player token. The closed cause is chosen /// here, never by the wire payload or a game action. @@ -1596,7 +1706,10 @@ mod tests { use engine::types::card::CardFace; use engine::types::card_type::CardType; use engine::types::game_state::{CastPaymentMode, PersistedGameState, WaitingFor}; - use engine::types::interaction::{InteractionAvailability, InteractionReasonCode}; + use engine::types::interaction::{ + InteractionAvailability, InteractionChoiceId, InteractionReasonCode, InteractionResponse, + MAX_INTERACTION_LIST_LEN, + }; use engine::types::mana::ManaCost; use engine::types::phase::{Phase, PhaseStop, PhaseStopScope}; use engine::types::zones::Zone; @@ -4345,4 +4458,174 @@ mod tests { && attacker.attack_target == AttackTarget::Player(PlayerId(2)) })); } + + /// A live, engine-published submission for whichever seat currently holds a + /// complete progress witness, paired with that seat's authenticated token. + /// + /// Deriving the witness from `derive_viewer_interaction` rather than + /// hand-building one is the point: these tests must exercise a capability + /// the engine actually minted, or a `StaleInteraction` would be + /// indistinguishable from a fabricated id. + fn live_witness<'a>( + mgr: &SessionManager, + code: &str, + token0: &'a str, + token1: &'a str, + ) -> (PlayerId, &'a str, &'a str, InteractionSubmission) { + let state = &mgr.sessions.get(code).expect("session").state; + for (player, token, other) in [(PlayerId(0), token0, token1), (PlayerId(1), token1, token0)] + { + let filtered = filter_state_for_player(state, player); + let view = derive_viewer_interaction(state, &filtered, player); + if let InteractionAvailability::ProgressAvailable { witness } = view.availability { + return (player, token, other, witness); + } + } + panic!("a started session must publish a progress witness for some seat"); + } + + fn started_two_seat_game() -> (SessionManager, String, String, String) { + let mut mgr = SessionManager::new(); + let (code, token0) = mgr.create_game(make_deck()); + let (token1, _) = mgr + .join_game(&code, make_deck()) + .expect("second seat joins"); + (mgr, code, token0, token1) + } + + /// The multi-authority hostile fixture: two seats, both legitimately + /// authenticated, one capability. Seat B holding a *valid* token for the + /// same game must not be able to spend seat A's `interaction_id`. + #[test] + fn handle_interaction_binds_the_actor_to_the_authenticated_token() { + let (mut mgr, code, token0, token1) = started_two_seat_game(); + let (_acting, acting_token, other_token, witness) = + live_witness(&mgr, &code, &token0, &token1); + + let before = mgr.sessions[&code].state.active_interaction_slots.clone(); + + let forged = mgr.handle_interaction(&code, other_token, witness.clone()); + let error = forged.expect_err("a valid token for another seat must not spend this slot"); + assert!( + error.contains("NotAuthorized"), + "unexpected reason: {error}" + ); + assert_eq!( + mgr.sessions[&code].state.active_interaction_slots, before, + "a refused submission must not consume the capability" + ); + + // The success half is the reach guard: without it the `Err` above could + // have come from staleness rather than from authorization. + mgr.handle_interaction(&code, acting_token, witness) + .expect("the authenticated owner of the slot may spend it"); + assert_ne!( + mgr.sessions[&code].state.active_interaction_slots, before, + "an accepted submission re-mints the interaction slots" + ); + } + + #[test] + fn handle_interaction_rejects_an_unknown_token() { + let (mut mgr, code, token0, token1) = started_two_seat_game(); + let (_acting, _acting_token, _other, witness) = live_witness(&mgr, &code, &token0, &token1); + + let before_waiting = mgr.sessions[&code].state.waiting_for.clone(); + let before_slots = mgr.sessions[&code].state.active_interaction_slots.clone(); + + let error = mgr + .handle_interaction(&code, "not-a-real-token", witness) + .expect_err("an unknown token is not a seat"); + assert_eq!(error, "Invalid player token"); + + assert_eq!(mgr.sessions[&code].state.waiting_for, before_waiting); + assert_eq!( + mgr.sessions[&code].state.active_interaction_slots, + before_slots + ); + } + + /// Capability consumption. This is the routine condition the benign + /// rejection channel exists for: a double-click produces exactly this. + #[test] + fn handle_interaction_rejects_a_stale_submission_benignly() { + let (mut mgr, code, token0, token1) = started_two_seat_game(); + let (_acting, acting_token, _other, witness) = live_witness(&mgr, &code, &token0, &token1); + + mgr.handle_interaction(&code, acting_token, witness.clone()) + .expect("the first submission spends a live capability"); + + let error = mgr + .handle_interaction(&code, acting_token, witness) + .expect_err("an interaction id is single-use"); + assert!(error.contains("StaleInteraction"), "unexpected: {error}"); + } + + #[test] + fn handle_interaction_rejects_an_oversized_response_before_the_reducer() { + let (mut mgr, code, token0, token1) = started_two_seat_game(); + let (_acting, acting_token, _other, witness) = live_witness(&mgr, &code, &token0, &token1); + + let choice = InteractionChoiceId("a".to_string()); + let oversized = InteractionSubmission { + interaction_id: witness.interaction_id.clone(), + response: InteractionResponse::Select { + choice_ids: vec![choice.clone(); MAX_INTERACTION_LIST_LEN + 1], + }, + }; + + let error = mgr + .handle_interaction(&code, acting_token, oversized) + .expect_err("an oversized response is refused"); + assert_eq!(error, "Engine error: PayloadTooLarge"); + + // Negative sibling: the boundary sits at the constant, not at "any + // large list". The same shape at exactly the limit is still refused, + // but for a different reason and further downstream. + let at_limit = InteractionSubmission { + interaction_id: witness.interaction_id.clone(), + response: InteractionResponse::Select { + choice_ids: vec![choice; MAX_INTERACTION_LIST_LEN], + }, + }; + let error = mgr + .handle_interaction(&code, acting_token, at_limit) + .expect_err("unknown choices are still refused"); + assert!( + !error.contains("PayloadTooLarge"), + "the bound must be the constant, not list size in general: {error}" + ); + } + + #[test] + fn handle_interaction_defers_to_a_pending_takeback() { + let (mut mgr, code, token0, token1) = started_two_seat_game(); + let (acting, acting_token, _other, witness) = live_witness(&mgr, &code, &token0, &token1); + + let target_state = mgr.sessions[&code].state.clone(); + mgr.sessions.get_mut(&code).unwrap().pending_takeback = Some(PendingTakeback { + requested_by: acting, + target_state, + approvals: HashSet::new(), + }); + + let before_slots = mgr.sessions[&code].state.active_interaction_slots.clone(); + let error = mgr + .handle_interaction(&code, acting_token, witness.clone()) + .expect_err("the table is voting; the state must not move"); + assert!( + error.contains("takeback request is pending"), + "unexpected: {error}" + ); + assert_eq!( + mgr.sessions[&code].state.active_interaction_slots, + before_slots + ); + + // Reach guard: the very same submission succeeds once the interlock is + // cleared, so the refusal above is attributable to the takeback. + mgr.sessions.get_mut(&code).unwrap().pending_takeback = None; + mgr.handle_interaction(&code, acting_token, witness) + .expect("with no pending takeback the same submission applies"); + } } From 88ed143999816b2e6d6e5ed10999e56722231910 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Mon, 3 Aug 2026 01:37:58 -0700 Subject: [PATCH 2/3] fix(server): label shared submission diagnostics by kind `handle_full_game_submission` serves both `GameSubmission::Action` and `GameSubmission::Interaction`, but two of its events still named only the action path -- so an operator triaging an interaction-submission report would grep for "interaction" and match neither the "not in a game" warning nor the "processed (lock held)" info event. Derive a `kind` label from the submission variant and attach it to both, renaming the messages to name the shared concept. `GameSubmission::kind` keeps the two call sites from restating the variant set. Raised by CodeRabbit on #6949. --- crates/phase-server/src/main.rs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/crates/phase-server/src/main.rs b/crates/phase-server/src/main.rs index 9eb250dee8..4b914fdd46 100644 --- a/crates/phase-server/src/main.rs +++ b/crates/phase-server/src/main.rs @@ -3575,6 +3575,19 @@ impl GameSubmission { /// The `Err` is boxed because `ServerMessage` is ~13 KiB (it carries a /// `GameState`), matching how this file already passes the type around /// (`game_started_msg: Box`). + /// Stable `kind` label for the diagnostics this handler emits. + /// + /// Both submission variants share one handler, so an unlabelled event is + /// unattributable: an operator triaging an interaction report greps for + /// "interaction" and matches nothing. Deriving the label here keeps the two + /// call sites from restating the variant set. + fn kind(&self) -> &'static str { + match self { + GameSubmission::Action(_) => "action", + GameSubmission::Interaction(_) => "interaction", + } + } + fn payload_rejection(&self) -> Result<(), Box> { match self { GameSubmission::Action(action) => guard_game_action_payload(action) @@ -3610,10 +3623,11 @@ async fn handle_full_game_submission( // `handle_client_message` need `&mut`. Do not "fix" this to `&mut`. identity: &SocketIdentity, ) { + let kind = submission.kind(); let game_code = match &identity.game_code { Some(c) => c.clone(), None => { - warn!("Action received but not in a game"); + warn!(kind, "game submission received but not in a game"); let msg = ServerMessage::error("Not in a game".to_string()); if let Ok(json) = serde_json::to_string(&msg) { let _ = socket.send(Message::text(json)).await; @@ -3690,9 +3704,10 @@ async fn handle_full_game_submission( let lock_ms = lock_start.elapsed().as_millis(); info!( game = %game_code, + kind, lock_ms, ai_actions = ai_results.len(), - "action processed (lock held)" + "game submission processed (lock held)" ); terminal.map(|terminal| { From 32db11f6ce045a85ec4c40a7eced4afa12fff913 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Mon, 3 Aug 2026 02:11:20 -0700 Subject: [PATCH 3/3] docs(server-core): correct the wire-guard module's match count The module header claimed this layer "guarantees a single exhaustive match". It has had two for some time and now has three, and the doc on `wire_rejection_message` already referred to "the two sibling matches in this module" -- contradicting the header it sits under. State the guarantee in terms of what it actually enforces (every wire policy is declared in an exhaustive, wildcard-free match, so a new variant cannot compile until it states one) and name the three policy axes by function rather than by count, so the header does not go stale again the next time one is added. Raised by review on #6949. Doc-only; no behavior change. --- crates/server-core/src/client_message_wire_guard.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/server-core/src/client_message_wire_guard.rs b/crates/server-core/src/client_message_wire_guard.rs index b86c079438..6be4a9a5b6 100644 --- a/crates/server-core/src/client_message_wire_guard.rs +++ b/crates/server-core/src/client_message_wire_guard.rs @@ -2,8 +2,15 @@ //! variant before handler dispatch and broker projection clones. //! //! Individual handlers still run their guards for defense in depth; this layer -//! guarantees a single exhaustive match so new variants must declare wire policy +//! guarantees that every wire policy is declared in an exhaustive, wildcard-free +//! match, so a new `ClientMessage` variant cannot compile until it states one, //! and broker-projected frames are bounded before `to_lobby_client_message` clones. +//! +//! Three such matches live here, one per policy axis: +//! [`guard_client_message_before_dispatch`] (payload bounding), +//! [`wire_rejection_message`] (which channel a rejection is answered on), and +//! [`guard_broker_projection_inbound`] (broker projection). A new variant must +//! declare a policy in all three. use lobby_broker::inbound_guard::{ guard_create_game_settings_inbound, guard_join_game_with_password_inbound,