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,6 +29,14 @@ defmodule CopiWeb.GameLive.Index do
     |> assign(:game, nil)
   end
 
+  @impl true
+  def handle_event("delete", %{"id" => id}, socket) do
+    game = Cornucopia.get_game!(id)
+    {:ok, _} = Cornucopia.delete_game(game)
+
+    {:noreply, assign(socket, :games, nil)}
+  end
+
   @impl true
   def handle_info({:update_parent, new_state}, socket) do
     {:noreply, assign(socket, :games, new_state)}

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

@@ -7,7 +7,8 @@ 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
@@ -96,17 +97,23 @@ 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
+      {1, _} ->
+        Logger.debug("Continue vote removed for player #{player.id}")
+
+      {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 +128,45 @@ 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)}")
+    # Safely parse dealt_card_id to prevent crashes from invalid input
+    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
+            {1, _} ->
+              Logger.debug("Vote removed for player #{player.id} on card #{dealt_card_id_int}")
+
+            {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")}
+      :error ->
+        Logger.warning("Invalid dealt_card_id: #{inspect(dealt_card_id)}")
+        {:noreply, socket}
     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

copi.owasp.org/test/copi_web/live/game_live/show_test.exs

@@ -4,6 +4,7 @@ defmodule CopiWeb.GameLive.ShowTest do
   import Phoenix.LiveViewTest
 
   alias Copi.Cornucopia
+  alias CopiWeb.GameLive.Show
 
   @game_attrs %{name: "Edge Case Test Game", edition: "webapp", suits: ["hearts", "clubs"]}
 
@@ -19,27 +20,27 @@ defmodule CopiWeb.GameLive.ShowTest do
       # Test with 0 players
       {:ok, view, html} = live(conn, "/games/#{game.id}")
       refute html =~ "Start Game"
-      
+
       # Directly trigger start_game event to test server-side validation
       render_click(view, "start_game", %{})
       assert render(view) =~ "At least 3 players are required"
-      
+
       {:ok, updated_game} = Cornucopia.Game.find(game.id)
       assert updated_game.started_at == nil
 
       # Test with 1 player
       {:ok, _player1} = Cornucopia.create_player(%{name: "Player 1", game_id: game.id})
       {:ok, view, html} = live(conn, "/games/#{game.id}")
       refute html =~ "Start Game"
-      
+
       render_click(view, "start_game", %{})
       assert render(view) =~ "At least 3 players are required"
 
       # Test with 2 players
       {:ok, _player2} = Cornucopia.create_player(%{name: "Player 2", game_id: game.id})
       {:ok, view, html} = live(conn, "/games/#{game.id}")
       refute html =~ "Start Game"
-      
+
       render_click(view, "start_game", %{})
       assert render(view) =~ "At least 3 players are required"
     end
@@ -51,7 +52,7 @@ defmodule CopiWeb.GameLive.ShowTest do
       {:ok, _player3} = Cornucopia.create_player(%{name: "Player 3", game_id: game.id})
 
       {:ok, view, html} = live(conn, "/games/#{game.id}")
-      
+
       # Button should be visible with 3+ players
       assert html =~ "Start Game"
 
@@ -70,7 +71,9 @@ defmodule CopiWeb.GameLive.ShowTest do
       {:ok, _player3} = Cornucopia.create_player(%{name: "Player 3", game_id: game.id})
 
       # Start game first
-      {:ok, started_game} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)})
+      {:ok, started_game} =
+        Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)})
+
       original_time = started_game.started_at
 
       {:ok, view, html} = live(conn, "/games/#{started_game.id}")
@@ -86,6 +89,22 @@ defmodule CopiWeb.GameLive.ShowTest do
       assert DateTime.compare(updated_game.started_at, original_time) == :eq
     end
 
+    test "handle_info ignores game:updated events for a different game", %{conn: conn, game: game} do
+      {:ok, show_live, _html} = live(conn, "/games/#{game.id}")
+      {:ok, other_game} = Cornucopia.create_game(%{name: "Other Game", edition: "webapp"})
+
+      # Send a broadcast whose topic doesn't match what show.ex subscribed to,
+      # so the `true ->` branch in the cond fires and socket is returned unchanged
+      send(show_live.pid, %{
+        topic: "game:mismatch-id",
+        event: "game:updated",
+        payload: other_game
+      })
+
+      :timer.sleep(50)
+      assert render(show_live) =~ game.name
+    end
+
     test "handle_info updates game when matching topic received", %{conn: conn, game: game} do
       {:ok, show_live, _html} = live(conn, "/games/#{game.id}")
 
@@ -100,33 +119,68 @@ defmodule CopiWeb.GameLive.ShowTest do
       :timer.sleep(50)
       assert render(show_live) =~ game.name
     end
+    test "redirects to error when game not found in handle_params", %{conn: conn} do
+      assert {:error, {:redirect, %{to: "/error"}}} =
+               live(conn, "/games/01ARZ3NDEKTSV4RRFFQ69G5FAV")
+    end
 
-    test "display_game_session/1 returns correct label for each edition", %{conn: _conn, game: _game} do
-      alias CopiWeb.GameLive.Show
-      assert Show.display_game_session("webapp")    == "Cornucopia Web Session:"
+    test "uses rounds_played when game is finished", %{conn: conn, game: game} do
+      {:ok, finished_game} =
+        Cornucopia.update_game(game, %{
+          started_at: DateTime.truncate(DateTime.utc_now(), :second),
+          finished_at: DateTime.truncate(DateTime.utc_now(), :second),
+          rounds_played: 2
+        })
+
+      {:ok, _view, html} = live(conn, "/games/#{finished_game.id}")
+      assert html =~ finished_game.name
+    end
+  end
+
+  describe "Show helper functions" do
+    setup [:create_game]
+
+    test "topic returns expected pubsub topic" do
+      assert Show.topic("abc123") == "game:abc123"
+    end
+
+    test "card_played_in_round finds a card in requested round" do
+      cards = [
+        %{played_in_round: 1, card: %{id: 1}},
+        %{played_in_round: 2, card: %{id: 2}}
+      ]
+
+      assert Show.card_played_in_round(cards, 2) == %{played_in_round: 2, card: %{id: 2}}
+      assert Show.card_played_in_round(cards, 3) == nil
+    end
+
+    test "display_game_session returns labels for known and unknown editions" do
+      assert Show.display_game_session("webapp") == "Cornucopia Web Session:"
       assert Show.display_game_session("ecommerce") == "Cornucopia Web Session:"
       assert Show.display_game_session("mobileapp") == "Cornucopia Mobile Session:"
-      assert Show.display_game_session("masvs")     == "Cornucopia Mobile Session:"
-      assert Show.display_game_session("mlsec")     == "Elevation of MLSec Session:"
-      assert Show.display_game_session("cumulus")   == "OWASP Cumulus Session:"
-      assert Show.display_game_session("eop")       == "EoP Session:"
+      assert Show.display_game_session("masvs") == "Cornucopia Mobile Session:"
+      assert Show.display_game_session("cumulus") == "OWASP Cumulus Session:"
+      assert Show.display_game_session("mlsec") == "Elevation of MLSec Session:"
+      assert Show.display_game_session("eop") == "EoP Session:"
+      assert Show.display_game_session("unknown") == "EoP Session:"
     end
 
-    test "latest_version/1 returns correct version string for each edition", %{conn: _conn, game: _game} do
-      alias CopiWeb.GameLive.Show
-      assert Show.latest_version("webapp")    == "2.2"
+    test "latest_version returns expected version by edition" do
+      assert Show.latest_version("webapp") == "2.2"
       assert Show.latest_version("ecommerce") == "1.22"
       assert Show.latest_version("mobileapp") == "1.1"
-      assert Show.latest_version("mlsec")     == "1.0"
-      assert Show.latest_version("cumulus")   == "1.1"
-      assert Show.latest_version("masvs")     == "1.1"
-      assert Show.latest_version("eop")       == "5.1"
-      assert Show.latest_version("unknown")   == "1.0"
+      assert Show.latest_version("mlsec") == "1.0"
+      assert Show.latest_version("cumulus") == "1.1"
+      assert Show.latest_version("masvs") == "1.1"
+      assert Show.latest_version("eop") == "5.1"
+      assert Show.latest_version("unknown") == "1.0"
     end
 
-    test "card_played_in_round/2 returns nil when no card matches", %{conn: _conn, game: _game} do
-      alias CopiWeb.GameLive.Show
-      assert Show.card_played_in_round([], 1) == nil
+    test "put_uri_hook stores uri in socket assigns" do
+      socket = %Phoenix.LiveView.Socket{assigns: %{__changed__: %{}}}
+
+      assert {:cont, updated_socket} = Show.put_uri_hook(%{}, "/games/round/1", socket)
+      assert updated_socket.assigns.uri == "/games/round/1"
     end
 
     test "card_played_in_round/2 returns the matching card", %{conn: _conn, game: _game} do