From 8819cc76a14c2da5986455c3704f6221ad29619f Mon Sep 17 00:00:00 2001 From: funman300 Date: Mon, 24 Aug 2026 19:58:54 +0000 Subject: [PATCH] fix(core): keep an absent manager field distinct from an explicit removal PUT /club/manager took `owned_card_id: Option`, so serde collapsed "field absent" and "field explicitly null" into the same None, and the route treated both as a clear. A caller that simply had nothing to say about the manager therefore DELETED the assignment. That is the second destructive squad-save path. WAL forensics on the staging DB pin it to commit frame 468, squad_managers 1 row -> 0, in a transaction touching only squad_managers and its indexes - disjoint from the player wipe at frame 465, which touched squads/squad_players/game_entity_ext. The two wipes came from two different writes, and only the first was guarded. The three states are now distinct: {} leave the manager exactly as it is {"owned_card_id": null} explicitly remove it (still supported) {"owned_card_id": ""} assign that owned card A deliberate removal is a legitimate operation and is preserved; only the "absent means delete" reading is gone. set_squad_manager_for_squad now runs its two existence checks and the insert in ONE transaction. Validating on the pool and then inserting left a window in which the squad or the card could disappear between check and write. Tests cover assign, reassign, idempotent re-assign, absent-is-a-no-op, explicit-null-still-removes, unowned-manager-refused, absent-against-no-manager not over-guarded, and that no manager write disturbs player assignments. A malformed body is asserted to be a parser rejection, distinguishable from the guard. With the fix reverted the absent-field test fails. --- src/routes/club.rs | 38 ++++++++-- src/services/club.rs | 12 ++- tests/integration_test.rs | 156 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 196 insertions(+), 10 deletions(-) diff --git a/src/routes/club.rs b/src/routes/club.rs index d014bf2..b9d91b6 100644 --- a/src/routes/club.rs +++ b/src/routes/club.rs @@ -139,15 +139,36 @@ pub async fn get_squad_manager( Ok(Json(json!({ "manager": manager }))) } +/// A manager write. The three states are DISTINCT and must stay that way: +/// +/// | body | meaning | +/// | --- | --- | +/// | `{}` — field absent | say nothing about the manager; leave it as it is | +/// | `{"owned_card_id": null}` | explicitly remove the current manager | +/// | `{"owned_card_id": ""}` | assign that owned card | +/// +/// A plain `Option` collapsed the first two into `None`, so a caller +/// that simply had nothing to say silently deleted the assignment. That is how a +/// FIFA 17 client with a destroyed squad model wiped a real manager row. The +/// double option keeps "absent" and "null" apart. #[derive(Deserialize)] pub struct SetManagerRequest { - /// The owned card to assign as manager, or `null`/absent to clear it. - pub owned_card_id: Option, + #[serde(default, deserialize_with = "deserialize_present_option")] + pub owned_card_id: Option>, } -/// Assign (or, with a null/absent `owned_card_id`, clear) the active squad's -/// manager. Fail-closed: the card must be owned by this club and the club must -/// have a squad. Returns the resulting assignment. +/// Deserialize a field that is present-but-null into `Some(None)`, leaving an +/// absent field as `None` (supplied by `#[serde(default)]`). +fn deserialize_present_option<'de, D>(d: D) -> Result>, D::Error> +where + D: serde::Deserializer<'de>, +{ + Option::::deserialize(d).map(Some) +} + +/// Assign, explicitly remove, or leave unchanged the active squad's manager. +/// Fail-closed: the card must be owned by this club and the club must have a +/// squad. Returns the resulting assignment. pub async fn put_squad_manager( State(state): State, game: GameId, @@ -156,10 +177,13 @@ pub async fn put_squad_manager( let profile = profile_svc::get_active_profile(&state.pool, game.as_str()).await?; let club = club_svc::get_club_by_profile(&state.pool, &profile.id).await?; match req.owned_card_id { - Some(owned_card_id) => { + Some(Some(owned_card_id)) => { club_svc::set_squad_manager(&state.pool, &club.id, &owned_card_id).await? } - None => club_svc::clear_squad_manager(&state.pool, &club.id).await?, + // Explicit null: a deliberate removal, which is a legitimate operation. + Some(None) => club_svc::clear_squad_manager(&state.pool, &club.id).await?, + // Absent: this request expresses no manager decision. Touch nothing. + None => {} } let manager = club_svc::get_squad_manager(&state.pool, &club.id).await?; Ok(Json(json!({ "manager": manager }))) diff --git a/src/services/club.rs b/src/services/club.rs index bc34f60..fe57e6c 100644 --- a/src/services/club.rs +++ b/src/services/club.rs @@ -195,11 +195,16 @@ pub async fn set_squad_manager_for_squad( squad_id: &str, owned_card_id: &str, ) -> AppResult<()> { + // One transaction: both existence checks and the write. Validating on the + // pool and then inserting left a window in which the squad or the card could + // be removed between the check and the write, persisting an assignment whose + // preconditions no longer held. + let mut tx = pool.begin().await?; let squad_ok = sqlx::query_scalar::<_, String>("SELECT id FROM squads WHERE id = ? AND club_id = ?") .bind(squad_id) .bind(club_id) - .fetch_optional(pool) + .fetch_optional(&mut *tx) .await?; if squad_ok.is_none() { return Err(AppError::NotFound(format!("squad '{squad_id}' not found"))); @@ -208,7 +213,7 @@ pub async fn set_squad_manager_for_squad( sqlx::query_scalar::<_, String>("SELECT id FROM owned_cards WHERE id = ? AND club_id = ?") .bind(owned_card_id) .bind(club_id) - .fetch_optional(pool) + .fetch_optional(&mut *tx) .await?; if card_ok.is_none() { return Err(AppError::NotFound(format!( @@ -223,8 +228,9 @@ pub async fn set_squad_manager_for_squad( .bind(squad_id) .bind(owned_card_id) .bind(&now) - .execute(pool) + .execute(&mut *tx) .await?; + tx.commit().await?; Ok(()) } diff --git a/tests/integration_test.rs b/tests/integration_test.rs index ca098b3..b0c6152 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -3098,6 +3098,162 @@ async fn test_squad_replace_refuses_to_empty_a_populated_squad() { ); } +/// `PUT /club/manager` must keep three states apart: absent = say nothing, +/// explicit null = remove, id = assign. +/// +/// Regression: `owned_card_id` was a plain `Option`, so serde collapsed +/// "field absent" and "field null" into the same `None` and the route treated +/// both as a clear. A caller with nothing to say therefore DELETED the manager — +/// how a FIFA 17 client with a destroyed squad model wiped a real manager row +/// (WAL commit 468, squad_managers 1 -> 0). +#[tokio::test] +async fn test_manager_absent_field_leaves_assignment_untouched() { + let app = build_test_app().await; + auth(&app, "ManagerGuardUser").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(3) + .map(|c| c["owned_card_id"].as_str().unwrap().to_string()) + .collect(); + + // A squad must exist for a manager to attach to. + let (s, _) = json_put( + &app, + "/squad/replace", + serde_json::json!({ + "name": "OpenFUT", "formation": "f442", + "slots": [{"owned_card_id": ids[0], "slot": 0, "is_captain": true, "is_on_bench": false}], + "client_reported": {"client_reported_chemistry": 50, "client_reported_rating": 80, + "client_reported_star_rating": 80}, + "extension": {"namespace": "fifa17.squad", "schema_version": 1, "payload": "{}"}, + }), + ) + .await; + assert_eq!(s, StatusCode::OK); + + // Assign. + let (s, body) = json_put( + &app, + "/club/manager", + serde_json::json!({"owned_card_id": ids[1]}), + ) + .await; + assert_eq!(s, StatusCode::OK, "{body}"); + assert_eq!(body["manager"]["id"], ids[1].as_str()); + + // ABSENT field: the destructive shape. Must change nothing. + let (s, body) = json_put(&app, "/club/manager", serde_json::json!({})).await; + assert_eq!(s, StatusCode::OK, "{body}"); + assert_eq!( + body["manager"]["id"], + ids[1].as_str(), + "an absent owned_card_id must LEAVE the manager, never clear it" + ); + + // Reassign to a different owned card: authentic, still allowed. + let (s, body) = json_put( + &app, + "/club/manager", + serde_json::json!({"owned_card_id": ids[2]}), + ) + .await; + assert_eq!(s, StatusCode::OK, "{body}"); + assert_eq!(body["manager"]["id"], ids[2].as_str()); + + // Same manager again: idempotent no-op, still assigned. + let (s, body) = json_put( + &app, + "/club/manager", + serde_json::json!({"owned_card_id": ids[2]}), + ) + .await; + assert_eq!(s, StatusCode::OK, "{body}"); + assert_eq!(body["manager"]["id"], ids[2].as_str()); + + // A card this club does not own is refused. + let (s, _) = json_put( + &app, + "/club/manager", + serde_json::json!({"owned_card_id": "not-a-real-owned-card"}), + ) + .await; + assert_eq!( + s, + StatusCode::NOT_FOUND, + "an unowned manager must be refused" + ); + let (_, body) = json_get(&app, "/club/manager").await; + assert_eq!( + body["manager"]["id"], + ids[2].as_str(), + "a refused assignment must not disturb the current manager" + ); + + // EXPLICIT null: a deliberate removal is legitimate and still works. + let (s, body) = json_put( + &app, + "/club/manager", + serde_json::json!({"owned_card_id": null}), + ) + .await; + assert_eq!(s, StatusCode::OK, "{body}"); + assert!( + body["manager"].is_null(), + "an explicit null must still remove the manager: {body}" + ); + + // Absent against a squad with NO manager: not over-guarded, plain no-op. + let (s, body) = json_put(&app, "/club/manager", serde_json::json!({})).await; + assert_eq!(s, StatusCode::OK, "{body}"); + assert!(body["manager"].is_null()); + + // The squad's player assignment survived every one of those manager writes. + let (_, ext) = json_get(&app, "/squad/ext?namespace=fifa17.squad").await; + assert_eq!( + ext["players"].as_array().unwrap().len(), + 1, + "manager writes must never disturb player assignments" + ); +} + +/// A malformed manager body is a PARSER rejection, distinguishable from the +/// guard's behaviour: a wrong-typed field is refused outright rather than being +/// silently treated as "absent" and passed through as a no-op. +#[tokio::test] +async fn test_manager_malformed_body_is_rejected_not_treated_as_absent() { + let app = build_test_app().await; + auth(&app, "ManagerMalformedUser").await; + let resp = app + .clone() + .oneshot( + Request::builder() + .method("PUT") + .uri("/club/manager") + .header("content-type", "application/json") + .body(Body::from(r#"{"owned_card_id": 12345}"#)) + .unwrap(), + ) + .await + .unwrap(); + let s = resp.status(); + assert!( + s == StatusCode::UNPROCESSABLE_ENTITY || s == StatusCode::BAD_REQUEST, + "a non-string owned_card_id must be a parser rejection, got {s}" + ); +} + // ─────────────────────────── economy HTTP boundary ────────────────────────── #[tokio::test]