diff --git a/migrations/0019_sbc_submissions_club_id.sql b/migrations/0019_sbc_submissions_club_id.sql new file mode 100644 index 0000000..72e2cf2 --- /dev/null +++ b/migrations/0019_sbc_submissions_club_id.sql @@ -0,0 +1,11 @@ +-- Issue 1: sbc_submissions was created (0001_initial.sql) without a club_id column, +-- but the MY CLUB milestone query (routes/club.rs get_milestones) counts +-- SELECT COUNT(*) FROM sbc_submissions WHERE club_id = ? AND passed = 1 +-- so SQLite errored on the unknown column and the error was swallowed by +-- `.unwrap_or(0)` -> the `sbcs_completed` milestone always read 0. Add the column +-- and backfill it from the profile's club so historical submissions count. +ALTER TABLE sbc_submissions ADD COLUMN club_id TEXT; + +UPDATE sbc_submissions +SET club_id = (SELECT c.id FROM clubs c WHERE c.profile_id = sbc_submissions.profile_id) +WHERE club_id IS NULL; diff --git a/src/services/checkin.rs b/src/services/checkin.rs index dbeacad..eaf1fd5 100644 --- a/src/services/checkin.rs +++ b/src/services/checkin.rs @@ -44,7 +44,7 @@ pub async fn get_status(pool: &Pool, profile_id: &str) -> AppResult AppResult AppResult { - let balance = sqlx::query_scalar::<_, i64>("SELECT coins FROM clubs WHERE id = ?") - .bind(club_id) - .fetch_one(pool) - .await?; + if amount < 0 { + return Err(AppError::BadRequest(format!( + "cannot spend a negative amount: {amount}" + ))); + } - if balance < amount { + let now = Utc::now(); + // Atomic compare-and-swap: the `coins >= ?` guard makes the debit conditional in a + // single statement, so two concurrent spends can never both pass a stale balance + // check and drive coins negative (the old SELECT-then-UPDATE was a TOCTOU race). + let affected = sqlx::query( + "UPDATE clubs SET coins = coins - ?, updated_at = ? WHERE id = ? AND coins >= ?", + ) + .bind(amount) + .bind(now) + .bind(club_id) + .bind(amount) + .execute(pool) + .await? + .rows_affected(); + + if affected == 0 { + // No row updated: the club is missing, or it could not afford the debit. + // Disambiguate so callers keep the NotFound vs BadRequest distinction. + let balance = sqlx::query_scalar::<_, i64>("SELECT coins FROM clubs WHERE id = ?") + .bind(club_id) + .fetch_optional(pool) + .await? + .ok_or_else(|| AppError::NotFound(format!("club not found: {club_id}")))?; return Err(AppError::BadRequest(format!( "insufficient coins: have {balance}, need {amount}" ))); } - let now = Utc::now(); - sqlx::query("UPDATE clubs SET coins = coins - ?, updated_at = ? WHERE id = ?") - .bind(amount) - .bind(now) + let new_balance = sqlx::query_scalar::<_, i64>("SELECT coins FROM clubs WHERE id = ?") .bind(club_id) - .execute(pool) + .fetch_one(pool) .await?; - - Ok(balance - amount) + Ok(new_balance) } diff --git a/src/services/market.rs b/src/services/market.rs index 83074b5..0ccee01 100644 --- a/src/services/market.rs +++ b/src/services/market.rs @@ -141,12 +141,23 @@ pub async fn buy_listing( .await? .ok_or_else(|| AppError::NotFound("listing not found or already sold".into()))?; - club::spend_coins(pool, club_id, listing.price).await?; - - sqlx::query("UPDATE market_listings SET sold = 1 WHERE id = ?") + // Atomically claim the listing (flip sold 0->1) before charging, so two concurrent + // buyers cannot both mint the same card. If the debit then fails, release the claim. + let claimed = sqlx::query("UPDATE market_listings SET sold = 1 WHERE id = ? AND sold = 0") .bind(&listing.id) .execute(pool) - .await?; + .await? + .rows_affected(); + if claimed == 0 { + return Err(AppError::NotFound("listing not found or already sold".into())); + } + if let Err(e) = club::spend_coins(pool, club_id, listing.price).await { + let _ = sqlx::query("UPDATE market_listings SET sold = 0 WHERE id = ?") + .bind(&listing.id) + .execute(pool) + .await; + return Err(e); + } let owned_id = Uuid::new_v4().to_string(); sqlx::query( @@ -206,10 +217,17 @@ pub async fn sell_card( .await? .ok_or_else(|| AppError::NotFound("owned card not found".into()))?; - sqlx::query("DELETE FROM owned_cards WHERE id = ?") + // Atomically claim the card: guard the DELETE with the owner + rows_affected so two + // concurrent sells of the same card cannot both credit (double payout). + let deleted = sqlx::query("DELETE FROM owned_cards WHERE id = ? AND club_id = ?") .bind(&req.owned_card_id) + .bind(club_id) .execute(pool) - .await?; + .await? + .rows_affected(); + if deleted == 0 { + return Err(AppError::NotFound("owned card not found".into())); + } let coins = (req.price as f64 * 0.4) as i64; let new_balance = club::add_coins(pool, club_id, coins).await?; diff --git a/src/services/pack.rs b/src/services/pack.rs index 6348ad3..3f95c23 100644 --- a/src/services/pack.rs +++ b/src/services/pack.rs @@ -71,7 +71,17 @@ pub async fn open_pack( .await? .ok_or_else(|| AppError::NotFound(format!("pack {pack_id} not found")))?; - if pack.opened { + // Atomically claim the pack before minting any cards: only one concurrent opener + // flips opened 0->1, so a double-open cannot mint the reward twice (duplication). + let claimed = sqlx::query( + "UPDATE packs SET opened = 1 WHERE id = ? AND club_id = ? AND opened = 0", + ) + .bind(pack_id) + .bind(club_id) + .execute(pool) + .await? + .rows_affected(); + if claimed == 0 { return Err(AppError::BadRequest("pack already opened".into())); } @@ -128,7 +138,7 @@ pub async fn open_pack( .unwrap_or_default(); let now = chrono::Utc::now().to_rfc3339(); - sqlx::query("UPDATE packs SET opened = 1, opened_cards = ?, opened_at = ? WHERE id = ?") + sqlx::query("UPDATE packs SET opened_cards = ?, opened_at = ? WHERE id = ?") .bind(&card_ids_json) .bind(&now) .bind(pack_id) diff --git a/src/services/sbc.rs b/src/services/sbc.rs index 65199b5..c69087d 100644 --- a/src/services/sbc.rs +++ b/src/services/sbc.rs @@ -12,6 +12,10 @@ use anyhow::Context; use std::path::Path; use uuid::Uuid; +/// Upper bound on cards in one SBC submission (a real squad SBC is 11; consumables +/// push it higher, but 30 is well beyond any legitimate challenge and caps a DoS). +const MAX_SBC_CARDS: usize = 30; + pub fn load_sbc_definitions(data_dir: &str) -> anyhow::Result> { let dir = Path::new(data_dir).join("sbcs"); let mut defs = Vec::new(); @@ -46,6 +50,26 @@ pub async fn submit_sbc( .find(|d| d.id == req.sbc_id) .ok_or_else(|| AppError::NotFound(format!("SBC {} not found", req.sbc_id)))?; + // Reject duplicate owned-card ids and bound the list length. A repeated id would + // resolve the SAME owned card N times (each fetch succeeds), so `validate_sbc` + // counts it toward the squad size and passes, while the DELETE loop removes it + // only once — i.e. any SBC satisfiable with a single duplicated card = free + // reward. An unbounded list is also a cheap DoS. + if req.owned_card_ids.len() > MAX_SBC_CARDS { + return Err(AppError::BadRequest(format!( + "too many cards in submission ({}, max {MAX_SBC_CARDS})", + req.owned_card_ids.len() + ))); + } + { + let mut seen = std::collections::HashSet::with_capacity(req.owned_card_ids.len()); + if let Some(dup) = req.owned_card_ids.iter().find(|id| !seen.insert(*id)) { + return Err(AppError::BadRequest(format!( + "duplicate card in submission: {dup}" + ))); + } + } + // Resolve cards from DB let mut cards: Vec = Vec::new(); for owned_id in &req.owned_card_ids { @@ -79,10 +103,11 @@ pub async fn submit_sbc( let sub_id = Uuid::new_v4().to_string(); let card_ids_json = serde_json::to_string(&req.owned_card_ids)?; sqlx::query( - "INSERT INTO sbc_submissions (id, profile_id, sbc_id, submitted_card_ids, passed, submitted_at) VALUES (?, ?, ?, ?, 1, ?)" + "INSERT INTO sbc_submissions (id, profile_id, club_id, sbc_id, submitted_card_ids, passed, submitted_at) VALUES (?, ?, ?, ?, ?, 1, ?)" ) .bind(&sub_id) .bind(profile_id) + .bind(club_id) .bind(&req.sbc_id) .bind(&card_ids_json) .bind(chrono::Utc::now().to_rfc3339()) diff --git a/src/services/season.rs b/src/services/season.rs index b67a5af..3909fe8 100644 --- a/src/services/season.rs +++ b/src/services/season.rs @@ -1,6 +1,6 @@ use crate::{ db::Pool, - error::AppResult, + error::{AppError, AppResult}, models::season::{Season, SeasonEndSummary, SeasonHistoryEntry, SeasonResult}, services::{club, pack}, }; @@ -20,7 +20,9 @@ pub async fn get_or_create(pool: &Pool, profile_id: &str) -> AppResult { .bind(&now) .execute(pool) .await?; - Ok(fetch(pool, profile_id).await?.expect("just inserted")) + fetch(pool, profile_id) + .await? + .ok_or_else(|| AppError::Internal(anyhow::anyhow!("season row missing immediately after insert"))) } async fn fetch(pool: &Pool, profile_id: &str) -> AppResult> { @@ -66,7 +68,9 @@ pub async fn record_match( .execute(pool) .await?; - let season = fetch(pool, profile_id).await?.expect("season must exist"); + let season = fetch(pool, profile_id) + .await? + .ok_or_else(|| AppError::Internal(anyhow::anyhow!("season row missing after record_match update")))?; if !season.is_complete() { return Ok((season, None)); @@ -141,7 +145,9 @@ pub async fn record_match( pack_awarded: pack_id.map(String::from), }; - let updated = fetch(pool, profile_id).await?.expect("season must exist"); + let updated = fetch(pool, profile_id) + .await? + .ok_or_else(|| AppError::Internal(anyhow::anyhow!("season row missing after season rollover")))?; Ok((updated, Some(summary))) } diff --git a/tests/integration_test.rs b/tests/integration_test.rs index a1fcc75..11a22f5 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -269,6 +269,56 @@ async fn test_sbc_submit_with_bronze_cards() { assert!(result["reward"].is_object()); } +#[tokio::test] +async fn test_sbc_rejects_duplicate_cards() { + // Regression: a single owned card repeated to fill an SBC must be rejected. Before + // the dedup guard the same id resolved N times, passed validation, and granted the + // reward while only one card was consumed (free-reward exploit). + let app = build_test_app().await; + auth(&app, "SBCDupePlayer").await; + + let (s, _) = json_post( + &app, + "/packs/buy", + serde_json::json!({ "pack_definition_id": "bronze_pack" }), + ) + .await; + assert_eq!(s, StatusCode::OK); + let (_, packs_json) = json_get(&app, "/packs").await; + let pack_id = packs_json["packs"] + .as_array() + .unwrap() + .iter() + .find(|p| p["definition_id"] == "bronze_pack") + .expect("bronze pack in inventory")["pack_id"] + .as_str() + .unwrap() + .to_string(); + let (s, _) = json_post(&app, &format!("/packs/open/{pack_id}"), serde_json::json!({})).await; + assert_eq!(s, StatusCode::OK); + + let (_, coll) = json_get(&app, "/collection").await; + let one_card = coll["collection"].as_array().unwrap()[0]["owned_card_id"] + .as_str() + .unwrap() + .to_string(); + + let (s, result) = json_post( + &app, + "/sbc/submit", + serde_json::json!({ + "sbc_id": "sbc_bronze_upgrade", + "owned_card_ids": vec![one_card; 11] + }), + ) + .await; + assert_eq!(s, StatusCode::BAD_REQUEST, "duplicate submission must be rejected: {result}"); + assert!( + result["error"].as_str().unwrap_or_default().contains("duplicate"), + "expected a duplicate-card error, got: {result}" + ); +} + #[tokio::test] async fn test_settings_read_write() { let app = build_test_app().await;