From 858757489acee34f0e1cdc52eabe4be365f87227 Mon Sep 17 00:00:00 2001
From: Mradul Tiwari <mradultiwari1708@gmail.com>
Date: Fri, 6 Mar 2026 01:39:42 +0530
Subject: [PATCH 1/3] fix: prevent joining and spectating games already in
 progress

- Block access to /games/:game_id once game has started (prevents spectators)

- Block access to /games/:game_id/players/new for started games

- Add defense-in-depth validation in form_component before creating player

- Redirect unauthorized users to /games with appropriate error message

Fixes: knowing a game_id allows you to join (and vote in) games already started
---
 .../lib/copi_web/live/game_live/show.ex       | 33 ++++++++-----
 .../live/player_live/form_component.ex        | 49 ++++++++++++-------
 .../lib/copi_web/live/player_live/index.ex    | 28 +++++++++--
 3 files changed, 78 insertions(+), 32 deletions(-)

diff --git a/copi.owasp.org/lib/copi_web/live/game_live/show.ex b/copi.owasp.org/lib/copi_web/live/game_live/show.ex
index aa84ab4ce..d63476f02 100644
--- a/copi.owasp.org/lib/copi_web/live/game_live/show.ex
+++ b/copi.owasp.org/lib/copi_web/live/game_live/show.ex
@@ -20,21 +20,30 @@ defmodule CopiWeb.GameLive.Show do
   @impl true
   def handle_params(params, _, socket) do
     with {:ok, game} <- Game.find(params["game_id"]) do
-      CopiWeb.Endpoint.subscribe(topic(params["game_id"]))
-
-      current_round = if game.finished_at do
-        game.rounds_played
+      # V2.2: Once a game has started, block public access to the game show page.
+      # Players must access their game through /games/:game_id/players/:player_id.
+      # This prevents unauthorized spectators from watching games in progress.
+      if not is_nil(game.started_at) do
+        {:noreply,
+         socket
+         |> put_flash(:error, "This game is in progress. If you are a player, please use your player-specific link.")
+         |> redirect(to: ~p"/games")}
       else
-        game.rounds_played + 1
-      end
+        CopiWeb.Endpoint.subscribe(topic(params["game_id"]))
 
-      case Want.integer(params["round"], min: 1, max: current_round, default: current_round) do
-        {:ok, requested_round} ->
-          {:noreply, socket |> assign(:game, game) |> assign(:requested_round, requested_round)}
-        {:error, _reason} ->
-          {:noreply, redirect(socket, to: "/error")}
-      end
+        current_round = if game.finished_at do
+          game.rounds_played
+        else
+          game.rounds_played + 1
+        end
 
+        case Want.integer(params["round"], min: 1, max: current_round, default: current_round) do
+          {:ok, requested_round} ->
+            {:noreply, socket |> assign(:game, game) |> assign(:requested_round, requested_round)}
+          {:error, _reason} ->
+            {:noreply, redirect(socket, to: "/error")}
+        end
+      end
     else
       {:error, _reason} ->
         {:noreply, redirect(socket, to: "/error")}
diff --git a/copi.owasp.org/lib/copi_web/live/player_live/form_component.ex b/copi.owasp.org/lib/copi_web/live/player_live/form_component.ex
index 93591a68c..efcf8a0ca 100644
--- a/copi.owasp.org/lib/copi_web/live/player_live/form_component.ex
+++ b/copi.owasp.org/lib/copi_web/live/player_live/form_component.ex
@@ -90,28 +90,43 @@ defmodule CopiWeb.PlayerLive.FormComponent do
 
   defp save_player(socket, :new, player_params) do
     ip = socket.assigns[:client_ip] || {127, 0, 0, 1}
+    game_id = player_params["game_id"]
 
-    case RateLimiter.check_rate(ip, :player_creation) do
-      {:ok, _remaining} ->
-        case Cornucopia.create_player(player_params) do
-          {:ok, player} ->
-            {:ok, updated_game} = Cornucopia.Game.find(socket.assigns.player.game_id)
-            CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game)
-
+    # V2.2: Re-fetch authoritative game state from DB at the trusted service layer
+    # to prevent bypassing the UI check via direct form submission or race conditions
+    case Cornucopia.Game.find(game_id) do
+      {:ok, game} when not is_nil(game.started_at) ->
+        {:noreply,
+         socket
+         |> put_flash(:error, "This game has already started. New players cannot join a game in progress.")
+         |> push_navigate(to: ~p"/games/#{game_id}")}
+
+      {:ok, _game} ->
+        case RateLimiter.check_rate(ip, :player_creation) do
+          {:ok, _remaining} ->
+            case Cornucopia.create_player(player_params) do
+              {:ok, player} ->
+                {:ok, updated_game} = Cornucopia.Game.find(socket.assigns.player.game_id)
+                CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game)
+
+                {:noreply,
+                 socket
+                 |> assign(:game, updated_game)
+                 |> push_navigate(to: ~p"/games/#{player.game_id}/players/#{player.id}")}
+
+              {:error, %Ecto.Changeset{} = changeset} ->
+                {:noreply, assign_form(socket, changeset)}
+            end
+
+          {:error, :rate_limit_exceeded} ->
             {:noreply,
              socket
-             |> assign(:game, updated_game)
-             |> push_navigate(to: ~p"/games/#{player.game_id}/players/#{player.id}")}
-
-          {:error, %Ecto.Changeset{} = changeset} ->
-            {:noreply, assign_form(socket, changeset)}
+             |> put_flash(:error, "Too many player creation attempts. Please try again later.")
+             |> assign_form(Cornucopia.change_player(socket.assigns.player))}
         end
 
-      {:error, :rate_limit_exceeded} ->
-        {:noreply,
-         socket
-         |> put_flash(:error, "Too many player creation attempts. Please try again later.")
-         |> assign_form(Cornucopia.change_player(socket.assigns.player))}
+      {:error, _} ->
+        {:noreply, push_navigate(socket, to: ~p"/")}
     end
   end
 
diff --git a/copi.owasp.org/lib/copi_web/live/player_live/index.ex b/copi.owasp.org/lib/copi_web/live/player_live/index.ex
index 71a85c803..5bc26db63 100644
--- a/copi.owasp.org/lib/copi_web/live/player_live/index.ex
+++ b/copi.owasp.org/lib/copi_web/live/player_live/index.ex
@@ -7,12 +7,34 @@ defmodule CopiWeb.PlayerLive.Index do
   @impl true
   def mount(%{"game_id" => game_id}, session, socket) do
     ip = socket.assigns[:client_ip] || Map.get(session, "client_ip") || Copi.IPHelper.get_ip_from_socket(socket)
-    {:ok, assign(assign(socket, :client_ip, ip), players: list_players(game_id), game: Cornucopia.get_game!(game_id))}
+    game = Cornucopia.get_game!(game_id)
+
+    # V2.2: Block at mount — returning redirect from mount sends a true HTTP 302
+    # during the dead (static) render, before any HTML or WebSocket reaches the client.
+    if not is_nil(game.started_at) do
+      {:ok,
+       socket
+       |> put_flash(:error, "This game has already started. New players cannot join a game in progress.")
+       |> redirect(to: ~p"/games")}
+    else
+      {:ok, assign(assign(socket, :client_ip, ip), players: list_players(game_id), game: game)}
+    end
   end
 
   @impl true
-  def handle_params(params, _url, socket) do
-    {:noreply, apply_action(socket, socket.assigns.live_action, params)}
+  def handle_params(%{"game_id" => game_id} = params, _url, socket) do
+    # Re-fetch game state for LiveView client-side navigations (when mount isn't called)
+    game = Cornucopia.get_game!(game_id)
+
+    # V2.2: Also check in handle_params for LiveView navigation scenarios
+    if not is_nil(game.started_at) do
+      {:noreply,
+       socket
+       |> put_flash(:error, "This game has already started. New players cannot join a game in progress.")
+       |> redirect(to: ~p"/games")}
+    else
+      {:noreply, apply_action(socket, socket.assigns.live_action, params)}
+    end
   end
 
   defp apply_action(socket, :edit, %{"id" => id}) do

From a21b05bf7cba7894d304e91c51bb58e6f5ae5c57 Mon Sep 17 00:00:00 2001
From: Mradul Tiwari <mradultiwari1708@gmail.com>
Date: Fri, 6 Mar 2026 02:04:51 +0530
Subject: [PATCH 2/3] refactor: address Copilot review suggestions

- Use idiomatic Elixir style (if game.started_at do)

- Assign fresh game/players in handle_params for LiveView navigations

- Add TOCTOU protection with transaction and row locking in create_player

- Improve error handling with Game not found flash message
---
 copi.owasp.org/lib/copi/cornucopia.ex         | 32 +++++++++++++++++--
 .../lib/copi_web/live/game_live/show.ex       |  2 +-
 .../live/player_live/form_component.ex        | 14 ++++++--
 .../lib/copi_web/live/player_live/index.ex    | 13 ++++++--
 4 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/copi.owasp.org/lib/copi/cornucopia.ex b/copi.owasp.org/lib/copi/cornucopia.ex
index cdcdc0e4b..527854c32 100644
--- a/copi.owasp.org/lib/copi/cornucopia.ex
+++ b/copi.owasp.org/lib/copi/cornucopia.ex
@@ -158,9 +158,35 @@ defmodule Copi.Cornucopia do
 
   """
   def create_player(attrs \\ %{}) do
-    %Player{}
-    |> Player.changeset(attrs)
-    |> Repo.insert()
+    # V15.4: Use transaction with row locking to prevent TOCTOU race condition
+    # when checking if game has started before creating player
+    game_id = attrs["game_id"] || attrs[:game_id]
+
+    Repo.transaction(fn ->
+      # Lock the game row for update to prevent race conditions
+      case game_id && Repo.get(Game, game_id, lock: "FOR UPDATE") do
+        nil ->
+          # No game_id provided or game not found - let changeset validation handle it
+          case %Player{} |> Player.changeset(attrs) |> Repo.insert() do
+            {:ok, player} -> player
+            {:error, changeset} -> Repo.rollback(changeset)
+          end
+
+        %Game{started_at: started_at} when started_at != nil ->
+          Repo.rollback(:game_already_started)
+
+        %Game{} ->
+          case %Player{} |> Player.changeset(attrs) |> Repo.insert() do
+            {:ok, player} -> player
+            {:error, changeset} -> Repo.rollback(changeset)
+          end
+      end
+    end)
+    |> case do
+      {:ok, player} -> {:ok, player}
+      {:error, :game_already_started} -> {:error, :game_already_started}
+      {:error, %Ecto.Changeset{} = changeset} -> {:error, changeset}
+    end
   end
 
   @doc """
diff --git a/copi.owasp.org/lib/copi_web/live/game_live/show.ex b/copi.owasp.org/lib/copi_web/live/game_live/show.ex
index d63476f02..e9605f9bd 100644
--- a/copi.owasp.org/lib/copi_web/live/game_live/show.ex
+++ b/copi.owasp.org/lib/copi_web/live/game_live/show.ex
@@ -23,7 +23,7 @@ defmodule CopiWeb.GameLive.Show do
       # V2.2: Once a game has started, block public access to the game show page.
       # Players must access their game through /games/:game_id/players/:player_id.
       # This prevents unauthorized spectators from watching games in progress.
-      if not is_nil(game.started_at) do
+      if game.started_at do
         {:noreply,
          socket
          |> put_flash(:error, "This game is in progress. If you are a player, please use your player-specific link.")
diff --git a/copi.owasp.org/lib/copi_web/live/player_live/form_component.ex b/copi.owasp.org/lib/copi_web/live/player_live/form_component.ex
index efcf8a0ca..e67d9f017 100644
--- a/copi.owasp.org/lib/copi_web/live/player_live/form_component.ex
+++ b/copi.owasp.org/lib/copi_web/live/player_live/form_component.ex
@@ -95,7 +95,7 @@ defmodule CopiWeb.PlayerLive.FormComponent do
     # V2.2: Re-fetch authoritative game state from DB at the trusted service layer
     # to prevent bypassing the UI check via direct form submission or race conditions
     case Cornucopia.Game.find(game_id) do
-      {:ok, game} when not is_nil(game.started_at) ->
+      {:ok, game} when game.started_at != nil ->
         {:noreply,
          socket
          |> put_flash(:error, "This game has already started. New players cannot join a game in progress.")
@@ -114,6 +114,13 @@ defmodule CopiWeb.PlayerLive.FormComponent do
                  |> assign(:game, updated_game)
                  |> push_navigate(to: ~p"/games/#{player.game_id}/players/#{player.id}")}
 
+              {:error, :game_already_started} ->
+                # V15.4: Race condition caught by transaction - game started between check and insert
+                {:noreply,
+                 socket
+                 |> put_flash(:error, "This game has already started. New players cannot join a game in progress.")
+                 |> push_navigate(to: ~p"/games")}
+
               {:error, %Ecto.Changeset{} = changeset} ->
                 {:noreply, assign_form(socket, changeset)}
             end
@@ -126,7 +133,10 @@ defmodule CopiWeb.PlayerLive.FormComponent do
         end
 
       {:error, _} ->
-        {:noreply, push_navigate(socket, to: ~p"/")}
+        {:noreply,
+         socket
+         |> put_flash(:error, "Game not found")
+         |> push_navigate(to: ~p"/games")}
     end
   end
 
diff --git a/copi.owasp.org/lib/copi_web/live/player_live/index.ex b/copi.owasp.org/lib/copi_web/live/player_live/index.ex
index 5bc26db63..ac530da12 100644
--- a/copi.owasp.org/lib/copi_web/live/player_live/index.ex
+++ b/copi.owasp.org/lib/copi_web/live/player_live/index.ex
@@ -11,7 +11,7 @@ defmodule CopiWeb.PlayerLive.Index do
 
     # V2.2: Block at mount — returning redirect from mount sends a true HTTP 302
     # during the dead (static) render, before any HTML or WebSocket reaches the client.
-    if not is_nil(game.started_at) do
+    if game.started_at do
       {:ok,
        socket
        |> put_flash(:error, "This game has already started. New players cannot join a game in progress.")
@@ -27,13 +27,20 @@ defmodule CopiWeb.PlayerLive.Index do
     game = Cornucopia.get_game!(game_id)
 
     # V2.2: Also check in handle_params for LiveView navigation scenarios
-    if not is_nil(game.started_at) do
+    if game.started_at do
       {:noreply,
        socket
        |> put_flash(:error, "This game has already started. New players cannot join a game in progress.")
        |> redirect(to: ~p"/games")}
     else
-      {:noreply, apply_action(socket, socket.assigns.live_action, params)}
+      # Assign freshly loaded game and players for LiveView client-side navigations
+      players = list_players(game_id)
+
+      {:noreply,
+       socket
+       |> assign(:game, game)
+       |> assign(:players, players)
+       |> apply_action(socket.assigns.live_action, params)}
     end
   end
 

From 7454d327a58e0f2d81bdd10cbc6adcee10acc27b Mon Sep 17 00:00:00 2001
From: Mradul Tiwari <mradultiwari1708@gmail.com>
Date: Fri, 6 Mar 2026 17:04:08 +0530
Subject: [PATCH 3/3] fix: allow spectating games, only prevent joining started
 games

Spectating is an intentional design feature - sharing game_id allows watching.

The fix now correctly prevents only player creation/joining for started games.
---
 .../lib/copi_web/live/game_live/show.ex       | 32 +++++++------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/copi.owasp.org/lib/copi_web/live/game_live/show.ex b/copi.owasp.org/lib/copi_web/live/game_live/show.ex
index e9605f9bd..dcd8f61a3 100644
--- a/copi.owasp.org/lib/copi_web/live/game_live/show.ex
+++ b/copi.owasp.org/lib/copi_web/live/game_live/show.ex
@@ -20,29 +20,19 @@ defmodule CopiWeb.GameLive.Show do
   @impl true
   def handle_params(params, _, socket) do
     with {:ok, game} <- Game.find(params["game_id"]) do
-      # V2.2: Once a game has started, block public access to the game show page.
-      # Players must access their game through /games/:game_id/players/:player_id.
-      # This prevents unauthorized spectators from watching games in progress.
-      if game.started_at do
-        {:noreply,
-         socket
-         |> put_flash(:error, "This game is in progress. If you are a player, please use your player-specific link.")
-         |> redirect(to: ~p"/games")}
-      else
-        CopiWeb.Endpoint.subscribe(topic(params["game_id"]))
+      CopiWeb.Endpoint.subscribe(topic(params["game_id"]))
 
-        current_round = if game.finished_at do
-          game.rounds_played
-        else
-          game.rounds_played + 1
-        end
+      current_round = if game.finished_at do
+        game.rounds_played
+      else
+        game.rounds_played + 1
+      end
 
-        case Want.integer(params["round"], min: 1, max: current_round, default: current_round) do
-          {:ok, requested_round} ->
-            {:noreply, socket |> assign(:game, game) |> assign(:requested_round, requested_round)}
-          {:error, _reason} ->
-            {:noreply, redirect(socket, to: "/error")}
-        end
+      case Want.integer(params["round"], min: 1, max: current_round, default: current_round) do
+        {:ok, requested_round} ->
+          {:noreply, socket |> assign(:game, game) |> assign(:requested_round, requested_round)}
+        {:error, _reason} ->
+          {:noreply, redirect(socket, to: "/error")}
       end
     else
       {:error, _reason} ->