From 9bdc1633a0b11ec5badcf3a742cdcfe0f97e6e6b Mon Sep 17 00:00:00 2001 From: funman300 Date: Mon, 24 Aug 2026 19:27:44 +0000 Subject: [PATCH] fix(core): refuse a squad replacement that would empty a populated squad /squad/replace is a full replacement: it deletes every assignment and reinserts the supplied slots. Nothing validated that the supplied list was non-empty, so a caller sending no slots silently wiped the squad and got 200/ok back. This happened for real. A FIFA 17 client whose in-memory squad had been destroyed by a bad parse wrote its emptiness back twice; WAL forensics on the staging DB pin the damage to commit frame 465, squad_players 18 rows -> 0, logged as route=squad-replace status=200 outcome=ok. The squad is the authority's state, so mirroring a broken client's model is unrecoverable. No product flow empties a squad: a full-replacement client sends its complete slot array, and no caller or test in the tree builds an empty slot list. So an empty list means the caller's model is broken, and the write is refused with BadRequest. The check runs inside the transaction, so a concurrent write cannot slip between the count and the delete, and a newly created squad counts zero and is unaffected. The regression test asserts both halves: the empty replacement is rejected, and the existing assignments survive it. With the guard removed the test fails with 200 and slots_written 0 - the exact production symptom. --- src/services/squad.rs | 26 +++++++++++++ tests/integration_test.rs | 82 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/src/services/squad.rs b/src/services/squad.rs index e095af5..3d16a1f 100644 --- a/src/services/squad.rs +++ b/src/services/squad.rs @@ -345,6 +345,32 @@ async fn replace_squad_inner( } }; + // A replacement carrying no slots would DELETE every assignment below and + // insert nothing, silently emptying the squad. No product flow does that: + // a full-replacement client sends its COMPLETE slot array, so an empty list + // means the caller's own model was destroyed, not that the user emptied + // their squad. Mirroring that damage into the authority is unrecoverable, + // so refuse it. + // + // Observed for real: a FIFA 17 client whose in-memory squad had been + // destroyed by a bad parse wrote its emptiness back twice, taking + // `squad_players` from 18 rows to 0 while the request logged 200/ok. + // + // Checked inside the transaction so a concurrent write cannot slip between + // the count and the delete. A newly created squad counts 0 and is unaffected. + if replacement.slots.is_empty() { + let existing = + sqlx::query_scalar::<_, i64>("SELECT COUNT(*) FROM squad_players WHERE squad_id = ?") + .bind(&squad_id) + .fetch_one(&mut *tx) + .await?; + if existing > 0 { + return Err(AppError::BadRequest(format!( + "refusing to empty a populated squad: replacement carried no slots, but squad '{squad_id}' holds {existing} assignments" + ))); + } + } + sqlx::query("DELETE FROM squad_players WHERE squad_id = ?") .bind(&squad_id) .execute(&mut *tx) diff --git a/tests/integration_test.rs b/tests/integration_test.rs index fe14159..ca098b3 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -3016,6 +3016,88 @@ async fn test_squad_ext_replace_read_roundtrip_and_idempotency() { assert_eq!(other["extension"]["state"], "missing"); } +/// A full replacement that carries no slots MUST NOT empty a populated squad. +/// +/// Regression: a FIFA 17 client whose in-memory squad had been destroyed by a +/// bad parse wrote that emptiness back through `/squad/replace`, taking the +/// canonical squad from 18 assignments to 0 while the request logged 200/ok. +/// The squad is the authority's state, so mirroring a broken client's model is +/// unrecoverable data loss. +#[tokio::test] +async fn test_squad_replace_refuses_to_empty_a_populated_squad() { + let app = build_test_app().await; + auth(&app, "SquadWipeGuardUser").await; + + let (_, packs) = json_get(&app, "/packs").await; + let pack_id = packs["packs"][0]["pack_id"].as_str().unwrap().to_string(); + json_post( + &app, + &format!("/packs/open/{pack_id}"), + serde_json::json!({}), + ) + .await; + let (_, coll) = json_get(&app, "/collection").await; + let ids: Vec = coll["collection"] + .as_array() + .unwrap() + .iter() + .take(2) + .map(|c| c["owned_card_id"].as_str().unwrap().to_string()) + .collect(); + + let ext_write = serde_json::json!({ + "namespace": "fifa17.squad", "schema_version": 1, "payload": "{\"custom\":\"[1]\"}" + }); + let client_reported = serde_json::json!({ + "client_reported_chemistry": 52, + "client_reported_rating": 90, + "client_reported_star_rating": 90 + }); + let populate = serde_json::json!({ + "name": "OpenFUT", + "formation": "f442", + "slots": [ + {"owned_card_id": ids[0], "slot": 0, "is_captain": true, "is_on_bench": false}, + {"owned_card_id": ids[1], "slot": 1, "is_captain": false, "is_on_bench": false}, + ], + "client_reported": client_reported, + "extension": ext_write, + }); + let (s, put) = json_put(&app, "/squad/replace", populate).await; + assert_eq!(s, StatusCode::OK, "{put}"); + assert_eq!(put["slots_written"], 2); + + // The destructive write: a well-formed replacement that simply carries no + // slots. It must be REFUSED, not applied — this is the exact shape that + // emptied a real squad. + let (s, err) = json_put( + &app, + "/squad/replace", + serde_json::json!({ + "name": "OpenFUT", + "formation": "f442", + "slots": [], + "client_reported": client_reported, + "extension": ext_write, + }), + ) + .await; + assert_eq!( + s, + StatusCode::BAD_REQUEST, + "an empty replacement must be refused, not applied: {err}" + ); + + // The squad is untouched — the refusal rolled back, it did not half-apply. + let (s, ext) = json_get(&app, "/squad/ext?namespace=fifa17.squad").await; + assert_eq!(s, StatusCode::OK); + assert_eq!( + ext["players"].as_array().unwrap().len(), + 2, + "both assignments survive the refused replacement" + ); +} + // ─────────────────────────── economy HTTP boundary ────────────────────────── #[tokio::test]