Fix race condition in voting system

copi.owasp.org/coveralls.json

@@ -1,7 +1,10 @@
 {
     "skip_files": [
         "lib/copi/migrations/",
-        "test/support/"
+        "test/support/",
+        "lib/copi/release.ex",
+        "lib/copi_web.ex",
+        "lib/copi_web/controllers/error_html.ex"
 
     ],
     "coverage_options": {

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

@@ -2,6 +2,7 @@ defmodule CopiWeb.GameLive.Index do
   use CopiWeb, :live_view
   use Phoenix.Component
 
+  alias Copi.Cornucopia
   alias Copi.Cornucopia.Game
 
   @impl true
@@ -28,9 +29,36 @@ defmodule CopiWeb.GameLive.Index do
     |> assign(:game, nil)
   end
 
+  @impl true
+  def handle_event("delete", %{"id" => id}, socket) do
+    case Ecto.ULID.cast(id) do
+      {:ok, game_id} ->
+        case Game.find(game_id) do
+          {:ok, %Game{} = game} ->
+            case Cornucopia.delete_game(game) do
+              {:ok, _deleted_game} ->
+                {:noreply,
+                 socket
+                 |> put_flash(:info, "Game deleted successfully.")
+                 |> assign(:games, nil)}
+
+              {:error, _changeset} ->
+                # V16.5: Fail securely with a generic user-facing error and keep state consistent.
+                {:noreply, put_flash(socket, :error, "Unable to delete game.")}
+            end
+
+          {:error, _reason} ->
+            # V16.5: Handle invalid/missing IDs safely without raising.
+            {:noreply, put_flash(socket, :error, "Game not found.")}
+        end
+
+      :error ->
+        {:noreply, put_flash(socket, :error, "Game not found.")}
+    end
+  end
+
   @impl true
   def handle_info({:update_parent, new_state}, socket) do
     {:noreply, assign(socket, :games, new_state)}
   end
-
 end

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

@@ -7,11 +7,15 @@ defmodule CopiWeb.PlayerLive.Show do
 
   alias Copi.Cornucopia.Player
   alias Copi.Cornucopia.Game
-  alias Copi.Cornucopia.DealtCard
+  alias Copi.Cornucopia.Vote
+  alias Copi.Cornucopia.ContinueVote
 
   @impl true
   def mount(_params, session, socket) do
-    ip = socket.assigns[:client_ip] || Map.get(session, "client_ip") || Copi.IPHelper.get_ip_from_socket(socket)
+    ip =
+      socket.assigns[:client_ip] || Map.get(session, "client_ip") ||
+        Copi.IPHelper.get_ip_from_socket(socket)
+
     {:ok, assign(socket, :client_ip, ip)}
   end
 
@@ -32,6 +36,7 @@ defmodule CopiWeb.PlayerLive.Show do
     case Player.find(socket.assigns.player.id) do
       {:ok, updated_player} ->
         {:noreply, socket |> assign(:game, updated_game) |> assign(:player, updated_player)}
+
       {:error, _reason} ->
         {:noreply, socket}
     end
@@ -48,7 +53,9 @@ defmodule CopiWeb.PlayerLive.Show do
     Copi.Cornucopia.update_game(game, %{rounds_played: game.rounds_played + 1, round_open: true})
 
     if last_round?(game) do
-      Copi.Cornucopia.update_game(game, %{finished_at: DateTime.truncate(DateTime.utc_now(), :second)} )
+      Copi.Cornucopia.update_game(game, %{
+        finished_at: DateTime.truncate(DateTime.utc_now(), :second)
+      })
     end
 
     {:ok, updated_game} = Game.find(game.id)
@@ -80,7 +87,9 @@ defmodule CopiWeb.PlayerLive.Show do
       Copi.Cornucopia.update_game(game, %{rounds_played: game.rounds_played + 1, round_open: true})
 
       if last_round?(game) do
-        Copi.Cornucopia.update_game(game, %{finished_at: DateTime.truncate(DateTime.utc_now(), :second)} )
+        Copi.Cornucopia.update_game(game, %{
+          finished_at: DateTime.truncate(DateTime.utc_now(), :second)
+        })
       end
 
       {:ok, updated_game} = Game.find(game.id)
@@ -96,17 +105,26 @@ defmodule CopiWeb.PlayerLive.Show do
     game = socket.assigns.game
     player = socket.assigns.player
 
-    # Check if player already voted
-    if Copi.Cornucopia.Game.has_continue_vote?(game, player) do
-      # Remove their vote
-      continue_vote = Enum.find(game.continue_votes, fn vote -> vote.player_id == player.id end)
-      if continue_vote do
-        Copi.Repo.delete!(continue_vote)
-      end
-    else
-      # Add their vote
-      Logger.debug("Adding continue vote for player_id: #{player.id}, game_id: #{game.id}")
-      Copi.Repo.insert(%Copi.Cornucopia.ContinueVote{player_id: player.id, game_id: game.id})
+    # Use atomic delete to avoid TOCTOU race condition
+    # Try to delete first - if none exists, this is a no-op
+    case Copi.Repo.delete_all(
+           from cv in ContinueVote,
+             where: cv.player_id == ^player.id and cv.game_id == ^game.id
+         ) do
+      {deleted_count, _} when deleted_count > 0 ->
+        # Handle duplicate rows safely without crashing the LiveView process.
+        # ASVS V16.5: fail securely on unexpected state.
+        Logger.debug("Continue vote removed for player #{player.id} (#{deleted_count} row(s))")
+
+      {0, _} ->
+        # No vote existed, so insert one
+        Copi.Repo.insert(
+          %ContinueVote{player_id: player.id, game_id: game.id},
+          on_conflict: :nothing,
+          conflict_target: [:player_id, :game_id]
+        )
+
+        Logger.debug("Continue vote insert attempted for player #{player.id}")
     end
 
     {:ok, updated_game} = Game.find(game.id)
@@ -121,34 +139,53 @@ defmodule CopiWeb.PlayerLive.Show do
     game = socket.assigns.game
     player = socket.assigns.player
 
-    {:ok, dealt_card} = DealtCard.find(dealt_card_id)
-
-    game_card_ids = game.players
-      |> Enum.flat_map(fn p -> p.dealt_cards end)
-      |> Enum.map(fn dc -> dc.id end)
-
-    if dealt_card.id in game_card_ids do
-      vote = get_vote(dealt_card, player)
-
-      if vote do
-        Logger.debug("Player has voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}")
-        Copi.Repo.delete!(vote)
-      else
-        Logger.debug("Player has not voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}")
-        case Copi.Repo.insert(%Copi.Cornucopia.Vote{dealt_card_id: String.to_integer(dealt_card_id), player_id: player.id}) do
-          {:ok, _vote} ->
-            Logger.debug("Vote added successfully for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}")
-          {:error, changeset} ->
-            Logger.warning("Voting failed for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}, errors: #{inspect(changeset.errors)}")
+    # Accept only fully numeric IDs. Integer.parse/1 accepts trailing junk by default.
+    case Integer.parse(dealt_card_id) do
+      {dealt_card_id_int, ""} ->
+        game_card_ids =
+          game.players
+          |> Enum.flat_map(fn p -> p.dealt_cards end)
+          |> Enum.map(fn dc -> dc.id end)
+
+        if dealt_card_id_int in game_card_ids do
+          # Use atomic delete to avoid TOCTOU race condition
+          # Try to delete first - if none exists, this is a no-op
+          case Copi.Repo.delete_all(
+                 from v in Vote,
+                   where: v.player_id == ^player.id and v.dealt_card_id == ^dealt_card_id_int
+               ) do
+            {deleted_count, _} when deleted_count > 0 ->
+              Logger.debug(
+                "Vote removed for player #{player.id} on card #{dealt_card_id_int} (#{deleted_count} row(s))"
+              )
+
+            {0, _} ->
+              # No vote existed, so insert one
+              Copi.Repo.insert(
+                %Vote{dealt_card_id: dealt_card_id_int, player_id: player.id},
+                on_conflict: :nothing,
+                conflict_target: [:player_id, :dealt_card_id]
+              )
+
+              Logger.debug(
+                "Vote insert attempted for player #{player.id} on card #{dealt_card_id_int}"
+              )
+          end
+
+          {:ok, updated_game} = Game.find(game.id)
+          CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game)
+          {:noreply, assign(socket, :game, updated_game)}
+        else
+          Logger.warning(
+            "Unauthorized vote attempt: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}"
+          )
+
+          {:noreply, socket |> put_flash(:error, "Invalid card selection")}
         end
-      end
 
-      {:ok, updated_game} = Game.find(game.id)
-      CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game)
-      {:noreply, assign(socket, :game, updated_game)}
-    else
-      Logger.warning("Unauthorized vote attempt: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}")
-      {:noreply, socket |> put_flash(:error, "Invalid card selection")}
+      _ ->
+        Logger.warning("Invalid dealt_card_id: #{inspect(dealt_card_id)}")
+        {:noreply, socket |> put_flash(:error, "Invalid card selection")}
     end
   end
 
@@ -157,7 +194,7 @@ defmodule CopiWeb.PlayerLive.Show do
   end
 
   def ordered_cards(cards) do
-    Enum.sort_by(cards, &(&1.card.id))
+    Enum.sort_by(cards, & &1.card.id)
   end
 
   def unplayed_cards(cards) do
@@ -179,7 +216,11 @@ defmodule CopiWeb.PlayerLive.Show do
   def round_open?(game) do
     latest_round = game.rounds_played + 1
 
-    players_still_to_play = game.players |> Enum.filter(fn player -> Enum.find(player.dealt_cards, fn card -> card.played_in_round == latest_round end) == nil end)
+    players_still_to_play =
+      game.players
+      |> Enum.filter(fn player ->
+        Enum.find(player.dealt_cards, fn card -> card.played_in_round == latest_round end) == nil
+      end)
 
     Enum.count(players_still_to_play) > 0
   end
@@ -189,7 +230,11 @@ defmodule CopiWeb.PlayerLive.Show do
   end
 
   def last_round?(game) do
-    players_with_no_cards = game.players |> Enum.filter(fn player -> Enum.find(player.dealt_cards, fn card -> card.played_in_round == nil end) == nil end)
+    players_with_no_cards =
+      game.players
+      |> Enum.filter(fn player ->
+        Enum.find(player.dealt_cards, fn card -> card.played_in_round == nil end) == nil
+      end)
 
     Enum.count(players_with_no_cards) > 0
   end
@@ -209,5 +254,4 @@ defmodule CopiWeb.PlayerLive.Show do
       _ -> "EoP Session:"
     end
   end
-
-end
+end

copi.owasp.org/priv/repo/migrations/20260218120000_add_unique_constraint_to_votes.exs

@@ -0,0 +1,24 @@
+defmodule Copi.Repo.Migrations.AddUniqueConstraintToVotes do
+  use Ecto.Migration
+
+  def up do
+    # Remove any existing duplicate votes, keeping only the oldest one
+    execute("""
+    DELETE FROM votes
+    WHERE id NOT IN (
+      SELECT MIN(id)
+      FROM votes
+      GROUP BY player_id, dealt_card_id
+    )
+    """)
+
+    # Now add the unique constraint
+    create unique_index(:votes, [:player_id, :dealt_card_id], 
+      name: :votes_player_dealt_card_unique_index)
+  end
+
+  def down do
+    drop index(:votes, [:player_id, :dealt_card_id], 
+      name: :votes_player_dealt_card_unique_index)
+  end
+end

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

@@ -274,4 +274,36 @@ defmodule Copi.CornucopiaTest do
       assert %Ecto.Changeset{} = Cornucopia.change_card(card)
     end
   end
+
+  describe "dealt_cards" do
+    alias Copi.Cornucopia.DealtCard
+
+    test "find/1 returns dealt card with preloaded associations when found" do
+      {:ok, game} = Cornucopia.create_game(%{name: "DC Test", edition: "webapp"})
+      {:ok, player} = Cornucopia.create_player(%{name: "P1", game_id: game.id})
+      {:ok, card} = Cornucopia.create_card(%{
+        category: "C", value: "V", description: "D", edition: "webapp",
+        version: "2.2", external_id: "DCT1", language: "en", misc: "m",
+        owasp_scp: [], owasp_devguide: [], owasp_asvs: [], owasp_appsensor: [],
+        capec: [], safecode: [], owasp_mastg: [], owasp_masvs: []
+      })
+      {:ok, dealt} = Copi.Repo.insert(%DealtCard{player_id: player.id, card_id: card.id})
+
+      assert {:ok, found} = DealtCard.find(dealt.id)
+      assert found.id == dealt.id
+      assert found.card.id == card.id
+    end
+
+    test "find/1 returns error when not found" do
+      assert {:error, :not_found} = DealtCard.find(999_999_999)
+    end
+  end
+
+  describe "votes" do
+    alias Copi.Cornucopia.Vote
+
+    test "changeset/2 returns a valid changeset" do
+      assert %Ecto.Changeset{valid?: true} = Vote.changeset(%Vote{}, %{})
+    end
+  end
 end