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 aa84ab4ce..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 @@ -34,7 +34,6 @@ defmodule CopiWeb.GameLive.Show do {:error, _reason} -> {:noreply, redirect(socket, to: "/error")} 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..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 @@ -90,28 +90,53 @@ 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 game.started_at != nil -> + {: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, :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 + + {: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} -> + {:error, _} -> {:noreply, socket - |> put_flash(:error, "Too many player creation attempts. Please try again later.") - |> assign_form(Cornucopia.change_player(socket.assigns.player))} + |> 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 71a85c803..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 @@ -7,12 +7,41 @@ 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 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 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 + # 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 defp apply_action(socket, :edit, %{"id" => id}) do