From 62a2d7e519603d0c06b70ab38880274c48852259 Mon Sep 17 00:00:00 2001 From: Luis Villa Date: Tue, 14 Jul 2026 08:45:32 -0700 Subject: [PATCH] fix(server): reject empty patrol batch and survive coordination lag Two independent robustness fixes on the localhost server: - A patrol request with an explicit empty batch_rev_ids ([]) reached rev_ids[0] on an empty Vec and panicked the handler (the None fallback only fires on a missing batch, and validation never rejected an empty one). validate_action_request now rejects a present-but-empty batch with 400, and the executor uses split_first so the index panic is unreachable even if validation is bypassed. - The coordination WebSocket fan-out task treated broadcast RecvError::Lagged as loop termination, so a slow operator panel that fell more than the room's 128-message buffer behind silently stopped receiving all further updates on an otherwise-open socket. It now logs the skip and resyncs to live, only exiting on Closed. Co-Authored-By: Claude Fable 5 --- crates/sp42-server/src/action_routes.rs | 37 +++++++++++++++++++------ crates/sp42-server/src/main.rs | 21 +++++++++++++- crates/sp42-server/src/tests.rs | 35 +++++++++++++++++++++++ 3 files changed, 83 insertions(+), 10 deletions(-) diff --git a/crates/sp42-server/src/action_routes.rs b/crates/sp42-server/src/action_routes.rs index d4d35c8b..85cb6f10 100644 --- a/crates/sp42-server/src/action_routes.rs +++ b/crates/sp42-server/src/action_routes.rs @@ -273,17 +273,21 @@ async fn execute_session_action( let rev_ids = payload .batch_rev_ids .clone() + .filter(|ids| !ids.is_empty()) .unwrap_or_else(|| vec![payload.rev_id]); + let (first, rest) = rev_ids + .split_first() + .expect("rev_ids is non-empty: the empty fallback yields [payload.rev_id]"); let mut response = execute_patrol( client, config, &PatrolRequest { - rev_id: rev_ids[0], + rev_id: *first, token: token.clone(), }, ) .await?; - for rid in rev_ids.iter().skip(1) { + for rid in rest { response = execute_patrol( client, config, @@ -981,6 +985,27 @@ fn action_shell_feedback( feedback } +/// Validate a Patrol request: reject a present-but-empty batch (which would +/// otherwise index `rev_ids[0]` on an empty Vec and panic in +/// `execute_session_action`) and require the patrol capability. A missing +/// `batch_rev_ids` is fine — it falls back to the single `rev_id`. +fn validate_patrol_request( + payload: &SessionActionExecutionRequest, + capabilities: &DevAuthCapabilityReport, +) -> Result<(), (StatusCode, Json)> { + if payload.batch_rev_ids.as_ref().is_some_and(Vec::is_empty) { + return Err(invalid_payload( + "batch_rev_ids must be non-empty when present", + )); + } + if !capabilities.capabilities.moderation.can_patrol { + return Err(forbidden_error( + "The authenticated session does not currently have patrol capability on this wiki.", + )); + } + Ok(()) +} + pub(crate) fn validate_action_request( payload: &SessionActionExecutionRequest, capabilities: &DevAuthCapabilityReport, @@ -1006,13 +1031,7 @@ pub(crate) fn validate_action_request( )); } } - SessionActionKind::Patrol => { - if !capabilities.capabilities.moderation.can_patrol { - return Err(forbidden_error( - "The authenticated session does not currently have patrol capability on this wiki.", - )); - } - } + SessionActionKind::Patrol => validate_patrol_request(payload, capabilities)?, SessionActionKind::Undo => { if payload.title.as_deref().is_none_or(str::is_empty) { return Err(invalid_payload("title is required for undo")); diff --git a/crates/sp42-server/src/main.rs b/crates/sp42-server/src/main.rs index c0f7dafb..bd6d16a8 100644 --- a/crates/sp42-server/src/main.rs +++ b/crates/sp42-server/src/main.rs @@ -408,8 +408,27 @@ async fn handle_socket(socket: WebSocket, wiki_id: String, actor: Option let mut subscriber = state.coordination.subscribe(&wiki_id).await; let (mut sender, mut receiver) = socket.split(); + let log_wiki_id = wiki_id.clone(); let send_task = tokio::spawn(async move { - while let Ok(envelope) = subscriber.recv().await { + loop { + let envelope = match subscriber.recv().await { + Ok(envelope) => envelope, + // A slow client that fell more than the room's buffer behind + // dropped `skipped` messages. Resync and keep serving live + // updates rather than killing the fan-out task (which would + // silently stop delivering to an otherwise-open socket). + Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped)) => { + warn!( + wiki_id = %log_wiki_id, + client_id, + skipped, + "coordination subscriber lagged; resyncing to live" + ); + continue; + } + // Sender dropped (room evicted): nothing more will arrive. + Err(tokio::sync::broadcast::error::RecvError::Closed) => break, + }; if envelope.sender_id == client_id { continue; } diff --git a/crates/sp42-server/src/tests.rs b/crates/sp42-server/src/tests.rs index 53768ace..7b3f6222 100644 --- a/crates/sp42-server/src/tests.rs +++ b/crates/sp42-server/src/tests.rs @@ -3222,6 +3222,41 @@ fn validate_accepts_inline_edit_with_node_locator() { assert!(crate::action_routes::validate_action_request(&payload, &report).is_ok()); } +#[test] +fn validate_rejects_patrol_with_an_empty_batch() { + // A present-but-empty batch_rev_ids would index rev_ids[0] on an empty + // Vec and panic in execute_session_action; validation must reject it. + let report = sp42_core::DevAuthCapabilityReport { + checked: true, + wiki_id: "frwiki".to_string(), + capabilities: sp42_core::DevAuthDerivedCapabilities { + moderation: sp42_core::DevAuthModerationCapabilities { + can_patrol: true, + ..Default::default() + }, + ..Default::default() + }, + ..Default::default() + }; + let mut payload = inline_edit_request(None, None, None); + payload.kind = sp42_core::SessionActionKind::Patrol; + payload.batch_rev_ids = Some(vec![]); + + let (status, body) = crate::action_routes::validate_action_request(&payload, &report) + .expect_err("an empty patrol batch must be rejected"); + assert_eq!(status, axum::http::StatusCode::BAD_REQUEST); + assert!( + body.0["error"] + .as_str() + .expect("error body should carry a message") + .contains("batch_rev_ids") + ); + + // A missing batch (None) still falls back to the single rev_id. + payload.batch_rev_ids = None; + assert!(crate::action_routes::validate_action_request(&payload, &report).is_ok()); +} + #[test] fn validate_rejects_inline_edit_without_selected_text_or_locator() { let payload = inline_edit_request(None, None, Some("x".to_string()));