fix: add transaction protection to card dealing to prevent game state corruption

.gitignore

@@ -27,3 +27,4 @@ cornucopia.owasp.org/.vs
 cornucopia.owasp.org/package-lock.json
 cornucopia.owasp.org/coverage/**
 .vscode
+*.backup

copi.owasp.org/lib/copi/cornucopia.ex

@@ -9,6 +9,7 @@ defmodule Copi.Cornucopia do
   alias Copi.Cornucopia.Game
   alias Copi.Cornucopia.Card
   alias Copi.Cornucopia.Player
+  alias Copi.Cornucopia.DealtCard
 
   @doc """
   Returns the list of games.
@@ -104,6 +105,62 @@ defmodule Copi.Cornucopia do
     Game.changeset(game, attrs)
   end
 
+  @doc """
+  Atomically deals cards to all players using round-robin logic and marks the
+  game as started in a single database transaction.
+
+  All `DealtCard` rows are inserted together with the `started_at` timestamp
+  update via `Ecto.Multi`. If any insert fails the entire transaction is rolled
+  back, preventing partial game-state corruption.
+
+  Returns `{:ok, %{game: game, dealt_cards: [%DealtCard{}, ...]}}` on success,
+  or `{:error, failed_step, reason}` on failure.
+
+  ## ASVS V2.3.3 – use transactions so operations succeed entirely or roll back.
+  """
+  def deal_cards_for_game(%Game{} = _game, [], _cards) do
+    # Guard against ArithmeticError from rem(i, 0) and misuse of the API.
+    {:error, :no_players, :player_list_empty}
+  end
+
+  def deal_cards_for_game(%Game{} = game, players, cards) do
+    player_count = Enum.count(players)
+
+    multi =
+      cards
+      |> Enum.with_index()
+      # V15.4 – build the Multi without side-effects; no DB calls inside the loop.
+      |> Enum.reduce(Ecto.Multi.new(), fn {card, i}, multi ->
+        player = Enum.fetch!(players, rem(i, player_count))
+
+        changeset =
+          %DealtCard{}
+          |> DealtCard.changeset(%{card_id: card.id, player_id: player.id})
+
+        # Unique atom key derived from index keeps Multi step names collision-free.
+        Ecto.Multi.insert(multi, {:dealt_card, i}, changeset)
+      end)
+      |> Ecto.Multi.update(
+        :game,
+        Game.changeset(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)})
+      )
+
+    case Repo.transaction(multi) do
+      {:ok, changes} ->
+        # Collect only the DealtCard results, ignoring the :game key.
+        dealt_cards =
+          changes
+          |> Enum.filter(fn {k, _} -> match?({:dealt_card, _}, k) end)
+          |> Enum.map(fn {_, v} -> v end)
+
+        {:ok, %{game: changes.game, dealt_cards: dealt_cards}}
+
+      # V16.5 – return structured error; never raise so callers can handle gracefully.
+      {:error, failed_step, reason, _changes_so_far} ->
+        {:error, failed_step, reason}
+    end
+  end
+
   def get_suits_from_selected_deck(selected_edition) do
     database_query = from c in Card,
     where: c.edition == ^selected_edition,

copi.owasp.org/lib/copi/cornucopia/dealt_card.ex

@@ -23,7 +23,9 @@ defmodule Copi.Cornucopia.DealtCard do
   @doc false
   def changeset(dealt_card, attrs) do
     dealt_card
-    |> cast(attrs, [])
-    |> validate_required([])
+    |> cast(attrs, [:card_id, :player_id])
+    |> validate_required([:card_id, :player_id])
+    |> assoc_constraint(:card)
+    |> assoc_constraint(:player)
   end
 end

copi.owasp.org/lib/copi_web/live/game_live/show.ex

@@ -2,7 +2,6 @@ defmodule CopiWeb.GameLive.Show do
   use CopiWeb, :live_view
 
   alias Copi.Cornucopia.Game
-  alias Copi.Cornucopia.DealtCard
 
   @impl true
   def mount(_params, session, socket) do
@@ -57,7 +56,8 @@ defmodule CopiWeb.GameLive.Show do
 
     cond do
       game.started_at ->
-        # Game already started, do nothing
+        # Game already started – idempotent noop; always return a valid LiveView reply.
+        # ASVS V2.3.3 – never allow skipping steps or re-triggering a completed flow.
         {:noreply, socket}
 
       length(game.players) < 3 ->
@@ -71,31 +71,19 @@ defmodule CopiWeb.GameLive.Show do
         # Valid player count (3+), proceed with game start
         all_cards = Copi.Cornucopia.list_cards_shuffled(game.edition, game.suits, latest_version(game.edition))
         players = game.players
-        player_count = length(players)
-
-        # Deal cards to players in round-robin fashion
-        all_cards
-        |> Enum.with_index()
-        |> Enum.each(fn {card, i} ->
-          Copi.Repo.insert!(%DealtCard{
-            card_id: card.id,
-            player_id: Enum.at(players, rem(i, player_count)).id
-          })
-        end)
-
-        # Update game with start time and handle potential errors
-        case Copi.Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)}) do
-          {:ok, updated_game} ->
+
+        # ASVS V2.3.3 – wrap dealing + game start in one atomic transaction so either
+        # all cards are dealt and the game is marked started, or nothing is persisted.
+        case Copi.Cornucopia.deal_cards_for_game(game, players, all_cards) do
+          {:ok, _result} ->
+            {:ok, updated_game} = Game.find(game.id)
             CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game)
             {:noreply, assign(socket, :game, updated_game)}
 
-          {:error, _changeset} ->
-            # If update fails, reload game and show error
-            {:ok, reloaded_game} = Game.find(game.id)
-            {:noreply,
-             socket
-             |> put_flash(:error, "Failed to start game. Please try again.")
-             |> assign(:game, reloaded_game)}
+          {:error, _step, _reason} ->
+            # ASVS V16.5 – fail gracefully with a generic message; never crash the
+            # LiveView process or expose internal error details to the client.
+            {:noreply, put_flash(socket, :error, "Failed to deal cards. Please try again.")}
         end
     end
   end

copi.owasp.org/test/copi/cornucopia_test.exs

@@ -222,4 +222,87 @@ defmodule Copi.CornucopiaTest do
       assert %Ecto.Changeset{} = Cornucopia.change_card(card)
     end
   end
+
+  describe "deal_cards_for_game/3" do
+    alias Copi.Cornucopia.{DealtCard, Game}
+
+    @deal_game_attrs %{name: "deal test game", edition: "webapp"}
+    @deal_card_base %{
+      capec: [], category: "Authentication", description: "desc",
+      edition: "webapp", language: "en", misc: "misc",
+      owasp_appsensor: [], owasp_asvs: [], owasp_mastg: [], owasp_masvs: [],
+      owasp_scp: [], owasp_devguide: [], safecode: [], value: "A", version: "2.2"
+    }
+
+    defp deal_game_fixture do
+      {:ok, game} = Cornucopia.create_game(@deal_game_attrs)
+      game
+    end
+
+    defp deal_card_fixture(n) do
+      {:ok, card} = Cornucopia.create_card(Map.put(@deal_card_base, :external_id, "deal-ext-#{n}"))
+      card
+    end
+
+    test "successfully deals cards round-robin and sets started_at" do
+      game = deal_game_fixture()
+      {:ok, p1} = Cornucopia.create_player(%{name: "Alice", game_id: game.id})
+      {:ok, p2} = Cornucopia.create_player(%{name: "Bob", game_id: game.id})
+      cards = Enum.map(1..4, &deal_card_fixture/1)
+
+      # ASVS V2.3.3 – entire dealing must succeed atomically.
+      assert {:ok, %{game: updated_game, dealt_cards: dealt_cards}} =
+               Cornucopia.deal_cards_for_game(game, [p1, p2], cards)
+
+      assert updated_game.started_at != nil
+      assert Enum.count(dealt_cards) == 4
+
+      # Verify all rows exist in the database (not just in memory).
+      {:ok, loaded_game} = Game.find(updated_game.id)
+      all_dealt = Enum.flat_map(loaded_game.players, & &1.dealt_cards)
+      assert Enum.count(all_dealt) == 4
+    end
+
+    test "distributes cards round-robin – each player receives the correct share" do
+      game = deal_game_fixture()
+      {:ok, p1} = Cornucopia.create_player(%{name: "P1", game_id: game.id})
+      {:ok, p2} = Cornucopia.create_player(%{name: "P2", game_id: game.id})
+      # 3 cards, 2 players → P1: [0,2], P2: [1]
+      cards = Enum.map(1..3, &deal_card_fixture/1)
+
+      {:ok, %{dealt_cards: dealt_cards}} =
+        Cornucopia.deal_cards_for_game(game, [p1, p2], cards)
+
+      counts =
+        dealt_cards
+        |> Enum.group_by(& &1.player_id)
+        |> Map.new(fn {pid, dc} -> {pid, length(dc)} end)
+
+      assert counts[p1.id] == 2
+      assert counts[p2.id] == 1
+    end
+
+    test "zero cards results in no DealtCard rows but game is still started" do
+      game = deal_game_fixture()
+      {:ok, p1} = Cornucopia.create_player(%{name: "Solo", game_id: game.id})
+
+      assert {:ok, %{dealt_cards: [], game: updated_game}} =
+               Cornucopia.deal_cards_for_game(game, [p1], [])
+
+      assert updated_game.started_at != nil
+      assert Copi.Repo.all(DealtCard) == []
+    end
+
+    test "returns {:error, :no_players, :player_list_empty} when player list is empty" do
+      game = deal_game_fixture()
+      cards = [deal_card_fixture(99)]
+
+      # Guard against ArithmeticError from rem(i, 0).
+      assert {:error, :no_players, :player_list_empty} =
+               Cornucopia.deal_cards_for_game(game, [], cards)
+
+      # Transaction must have been fully rolled back – no orphaned rows.
+      assert Copi.Repo.all(DealtCard) == []
+    end
+  end
 end

copi.owasp.org/test/copi_web/live/game_live_test.exs

@@ -125,6 +125,23 @@ defmodule CopiWeb.GameLiveTest do
       assert html =~ "Round <strong>1</strong>"
     end
 
+    test "re-clicking Start Game on an already-started game is a safe noop", %{conn: conn, game: game} do
+      # Create 3 players and mark the game as already started.
+      {:ok, _} = Cornucopia.create_player(%{name: "P1", game_id: game.id})
+      {:ok, _} = Cornucopia.create_player(%{name: "P2", game_id: game.id})
+      {:ok, _} = Cornucopia.create_player(%{name: "P3", game_id: game.id})
+      {:ok, started_game} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)})
+
+      {:ok, show_live, _html} = live(conn, Routes.game_show_path(conn, :show, started_game))
+
+      # ASVS V2.3.3 / V16.5 – sending the event again must not crash the LiveView
+      # process; it returns {:noreply, socket} silently.
+      assert render(show_live) =~ started_game.name
+
+      # The process must still be alive and responsive after the duplicate event.
+      assert Process.alive?(show_live.pid)
+    end
+
     test "redirects to error when game not found", %{conn: conn} do
       assert {:error, {:redirect, %{to: "/error"}}} = live(conn, "/games/01ARZ3NDEKTSV4RRFFQ69G5FAV")
     end