From 3a9bd9908b4e90a847fcd38d5d13671aee0c0441 Mon Sep 17 00:00:00 2001 From: Duncan Date: Thu, 13 Aug 2026 05:47:01 -0400 Subject: [PATCH] fix(relay): make admin cancel atomic + ownership-fenced (cross-report 409) cancel_action fenced only on id+state='failed'+step_marker IS NULL, with no report/community constraint, and discarded the report-reopen row count. POST /reports/A/cancel {actionId:B} cancelled B's action, stranded B as processing with a terminal action, and returned a fabricated {status:"open"} for A. Make cancellation one atomic, ownership-fenced transition mirroring finalize_success: the action UPDATE now also fences report_id + report_community_id, the report UPDATE now also fences status='processing', and both updates must each affect exactly one row or the whole transaction rolls back to false -> 409 with zero state change. This is what makes the handler's hard-coded "status":"open" legitimate. Adds an HTTP->DB regression: two processing reports sharing a community, each with a distinct failed action; /reports/A/cancel {actionId:B} must 409 and leave both reports and both actions byte-for-byte unchanged. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-db/src/relay_admin_actions.rs | 48 +++++++-- crates/buzz-relay/src/api/admin/mod.rs | 125 ++++++++++++++++++++++ 2 files changed, 164 insertions(+), 9 deletions(-) diff --git a/crates/buzz-db/src/relay_admin_actions.rs b/crates/buzz-db/src/relay_admin_actions.rs index 673308e57..f03bfb80a 100644 --- a/crates/buzz-db/src/relay_admin_actions.rs +++ b/crates/buzz-db/src/relay_admin_actions.rs @@ -963,9 +963,19 @@ pub async fn record_failure(pool: &PgPool, action_id: Uuid, error: &str) -> Resu Ok(()) } -/// Cancel a failed action (pre-mutation only). Clears the claim so the report -/// returns to 'open'. Returns false if the action was not in 'failed' state -/// or had a step_marker (post-mutation cancel is forbidden). +/// Cancel a failed action (pre-mutation only) and return its report to 'open'. +/// +/// This is one atomic, ownership-fenced transition: the action is cancelled +/// only if it is `failed`/pre-mutation AND belongs to the path `report_id` + +/// `community_id`, and the report is reopened only if it is still `processing` +/// and still points at this exact action. Both updates must each affect +/// exactly one row; any mismatch rolls the whole transaction back and returns +/// `false`. +/// +/// Returns `false` (→ 409 at the HTTP layer, no state change) when the action +/// is not `failed`, has a `step_marker` (post-mutation cancel is forbidden), +/// does not belong to the path report/community (cross-report cancel), or the +/// report moved underneath the cancel. pub async fn cancel_action( pool: &PgPool, action_id: Uuid, @@ -974,29 +984,44 @@ pub async fn cancel_action( ) -> Result { let mut tx = pool.begin().await?; - // Only cancel from pre-mutation failed state. + // Cancel only a pre-mutation `failed` action that BELONGS to the path + // report and community. Fencing on report_id + report_community_id is what + // blocks cross-report cancellation: `/reports/A/cancel {actionId:B}` matches + // zero rows because B's report_id is not A. let updated = sqlx::query( r#" UPDATE relay_admin_actions SET state = 'cancelled', updated_at = now() - WHERE id = $1 AND state = 'failed' AND step_marker IS NULL + WHERE id = $1 + AND report_id = $2 + AND report_community_id = $3 + AND state = 'failed' + AND step_marker IS NULL "#, ) .bind(action_id) + .bind(report_id) + .bind(community_id.as_uuid()) .execute(&mut *tx) .await?; - if updated.rows_affected() == 0 { + if updated.rows_affected() != 1 { tx.rollback().await?; return Ok(false); } - // Clear the claim on the report → back to open. - sqlx::query( + // Return the report to `open`, fenced on it still being `processing` and + // still pointing at this exact action. Must affect exactly one row — any + // mismatch means the report moved underneath us, so roll back the action + // cancel too. This is what makes the handler's `"status":"open"` legitimate. + let reopened = sqlx::query( r#" UPDATE moderation_reports SET status = 'open', active_action_id = NULL - WHERE community_id = $1 AND id = $2 AND active_action_id = $3 + WHERE community_id = $1 + AND id = $2 + AND status = 'processing' + AND active_action_id = $3 "#, ) .bind(community_id.as_uuid()) @@ -1005,6 +1030,11 @@ pub async fn cancel_action( .execute(&mut *tx) .await?; + if reopened.rows_affected() != 1 { + tx.rollback().await?; + return Ok(false); + } + tx.commit().await?; Ok(true) } diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index 70fdb9d90..9dee74faf 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -3377,6 +3377,131 @@ mod tests { cleanup_admin_host_report(&pool, report_id).await; } + #[tokio::test] + #[ignore = "requires Postgres — cross-report cancel is rejected without side effects"] + async fn cancel_route_rejects_cross_report_action_id_with_409_and_no_side_effects() { + // Ownership fence: POST /reports/A/cancel {actionId: B's action} must be + // rejected (409) and leave BOTH reports and BOTH actions untouched. The + // two reports share a community, so only the report_id fence — not the + // community fence — can block this: it is the sharper negative case. + let keys = nostr::Keys::generate(); + let state = nip98_state(vec![keys.public_key().to_hex()]).await; + let pool = sqlx::PgPool::connect( + &std::env::var("BUZZ_TEST_DATABASE_URL") + .unwrap_or_else(|_| "postgres://buzz:buzz_dev@localhost:5432/buzz".to_string()), + ) + .await + .expect("connect to test DB"); + + // Two reports on the same admin.example community, each driven to + // `processing` with its own distinct pre-mutation `failed` action. + let report_a = seed_admin_host_report(&pool, "open").await; + let report_b = seed_admin_host_report(&pool, "open").await; + let community_id: Uuid = + sqlx::query_scalar("SELECT community_id FROM moderation_reports WHERE id = $1") + .bind(report_a) + .fetch_one(&pool) + .await + .expect("community id"); + let cid = buzz_core::CommunityId::from_uuid(community_id); + + let seed_failed_action = |report_id: Uuid| { + let pool = pool.clone(); + async move { + let action_id = match buzz_db::relay_admin_actions::claim_report( + &pool, + cid, + report_id, + Uuid::new_v4(), + &[2u8; 32], + "operator", + "ban", + None, + None, + "resolve:ban", + "relay_operator", + Some(&[1u8; 32]), + None, + None, + ) + .await + .expect("claim") + { + buzz_db::relay_admin_actions::ClaimResult::Claimed(a) => a.id, + other => panic!("expected Claimed, got {other:?}"), + }; + buzz_db::relay_admin_actions::begin_enforcing(&pool, action_id) + .await + .expect("begin_enforcing"); + buzz_db::relay_admin_actions::record_failure(&pool, action_id, "boom") + .await + .expect("record_failure"); + action_id + } + }; + let action_a = seed_failed_action(report_a).await; + let action_b = seed_failed_action(report_b).await; + + // Cross-report cancel: cancel report A citing report B's action id. + let body = serde_json::json!({ "actionId": action_b }).to_string(); + let path = format!("/reports/{report_a}/cancel"); + let auth = make_nostr_auth_post(&keys, &path, body.as_bytes()); + let response = status_for( + state, + Request::builder() + .method("POST") + .uri(&path) + .header(header::HOST, "admin.example") + .header(header::AUTHORIZATION, auth) + .header(header::CONTENT_TYPE, "application/json") + .body(Body::from(body)) + .expect("request"), + ) + .await; + assert_eq!( + response.status(), + StatusCode::CONFLICT, + "cross-report cancel must be 409" + ); + + // No side effects: both reports still `processing` pointing at their own + // action, and both actions still `failed`. + let read_state = |report_id: Uuid, action_id: Uuid| { + let pool = pool.clone(); + async move { + let (r_status, r_active): (String, Option) = sqlx::query_as( + "SELECT status, active_action_id FROM moderation_reports WHERE id = $1", + ) + .bind(report_id) + .fetch_one(&pool) + .await + .expect("read report"); + let a_state: String = + sqlx::query_scalar("SELECT state FROM relay_admin_actions WHERE id = $1") + .bind(action_id) + .fetch_one(&pool) + .await + .expect("read action"); + (r_status, r_active, a_state) + } + }; + let (a_status, a_active, a_action_state) = read_state(report_a, action_a).await; + let (b_status, b_active, b_action_state) = read_state(report_b, action_b).await; + assert_eq!( + (a_status.as_str(), a_active, a_action_state.as_str()), + ("processing", Some(action_a), "failed"), + "report A and its action must be unchanged" + ); + assert_eq!( + (b_status.as_str(), b_active, b_action_state.as_str()), + ("processing", Some(action_b), "failed"), + "report B and its action must be unchanged — B is the cancel victim guarded against" + ); + + cleanup_admin_host_report(&pool, report_a).await; + cleanup_admin_host_report(&pool, report_b).await; + } + async fn cleanup_admin_host_report(pool: &sqlx::PgPool, report_id: Uuid) { sqlx::query("DELETE FROM relay_admin_actions WHERE report_id = $1") .bind(report_id)