Security fix player access

copi.owasp.org/lib/copi_web/controllers/api_controller.ex

@@ -1,44 +1,71 @@
 defmodule CopiWeb.ApiController do
   use CopiWeb, :controller
+  require Logger
+
   alias Copi.Cornucopia.Game
 
-
   def play_card(conn, %{"game_id" => game_id, "player_id" => player_id, "dealt_card_id" => dealt_card_id}) do
     with {:ok, game} <- Game.find(game_id) do
-
       player = Enum.find(game.players, fn player -> player.id == player_id end)
-      dealt_card = Enum.find(player.dealt_cards, fn dealt_card -> Integer.to_string(dealt_card.id) == dealt_card_id end)
+
+      # CRITICAL SECURITY VALIDATION: Ensure player belongs to the specified game
+      if player do
+        if player.game_id != game_id do
+          Logger.warning("Security: API play_card attempted - player #{player_id} does not belong to game #{game_id}, IP: #{get_client_ip(conn)}")
+          conn |> put_status(:forbidden) |> json(%{"error" => "Player does not belong to this game"})
+        else
+          dealt_card = Enum.find(player.dealt_cards, fn dealt_card -> Integer.to_string(dealt_card.id) == dealt_card_id end)
 
-      if player && dealt_card do
-        current_round = game.rounds_played + 1
+          if dealt_card do
+            current_round = game.rounds_played + 1
 
-        cond do
-          dealt_card.played_in_round ->
-            conn |> put_status(:not_acceptable) |> json(%{"error" => "Card already played"})
-          Enum.find(player.dealt_cards, fn dealt_card -> dealt_card.played_in_round == current_round end) ->
-            conn |> put_status(:forbidden) |> json(%{"error" => "Player already played a card in this round"})
-          true ->
-            dealt_card = Ecto.Changeset.change dealt_card, played_in_round: current_round
+            cond do
+              dealt_card.played_in_round ->
+                conn |> put_status(:not_acceptable) |> json(%{"error" => "Card already played"})
+              Enum.find(player.dealt_cards, fn dealt_card -> dealt_card.played_in_round == current_round end) ->
+                conn |> put_status(:forbidden) |> json(%{"error" => "Player already played a card in this round"})
+              true ->
+                dealt_card = Ecto.Changeset.change dealt_card, played_in_round: current_round
 
-            case Copi.Repo.update dealt_card do
-              {:ok, dealt_card} ->
-                with {:ok, updated_game} <- Game.find(game.id) do
-                  CopiWeb.Endpoint.broadcast(topic(game.id), "game:updated", updated_game)
-                else
-                  {:error, _reason} ->
-                    conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not find updated game"})
-                end
+                case Copi.Repo.update dealt_card do
+                  {:ok, dealt_card} ->
+                    with {:ok, updated_game} <- Game.find(game.id) do
+                      CopiWeb.Endpoint.broadcast(topic(game.id), "game:updated", updated_game)
+                    else
+                      {:error, _reason} ->
+                        conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not find updated game"})
+                    end
 
-                conn |> json(%{"id" => dealt_card.id})
-              {:error, _changeset} ->
-                conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not update dealt card"})
+                    conn |> json(%{"id" => dealt_card.id})
+                  {:error, _changeset} ->
+                    conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not update dealt card"})
+                end
             end
+          else
+            Logger.warning("Security: API play_card attempted - dealt_card #{dealt_card_id} not found for player #{player_id}, IP: #{get_client_ip(conn)}")
+            conn |> put_status(:not_found) |> json(%{"error" => "Could not find dealt card"})
+          end
         end
       else
-        conn |> put_status(:not_found) |> json(%{"error" => "Could not find player and dealt card"})
+        Logger.warning("Security: API play_card attempted - player #{player_id} not found in game #{game_id}, IP: #{get_client_ip(conn)}")
+        conn |> put_status(:not_found) |> json(%{"error" => "Could not find player"})
       end
     else
-      {:error, _reason} -> conn |> put_status(:not_found) |> json(%{"error" => "Could not find game"})
+      {:error, _reason} ->
+        Logger.warning("Security: API play_card attempted - game #{game_id} not found, IP: #{get_client_ip(conn)}")
+        conn |> put_status(:not_found) |> json(%{"error" => "Could not find game"})
+    end
+  end
+
+  defp get_client_ip(conn) do
+    # Get IP from various headers, fallback to remote_ip
+    case get_req_header(conn, "x-forwarded-for") do
+      [ip | _] -> ip
+      [] -> 
+        case get_req_header(conn, "x-real-ip") do
+          [ip | _] -> ip
+          [] -> to_string(:inet_parse.ntoa(conn.remote_ip))
+        end
     end
   end
 

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

@@ -16,17 +16,25 @@ defmodule CopiWeb.PlayerLive.Show do
   end
 
   @impl true
-  def handle_params(%{"id" => player_id}, _, socket) do
+  def handle_params(%{"id" => player_id, "game_id" => url_game_id}, _, socket) do
     with {:ok, player} <- Player.find(player_id) do
-      with {:ok, game} <- Game.find(player.game_id) do
-        CopiWeb.Endpoint.subscribe(topic(player.game_id))
-        {:noreply, socket |> assign(:game, game) |> assign(:player, player)}
+      # CRITICAL SECURITY VALIDATION: Ensure player belongs to URL game context
+      if player.game_id != url_game_id do
+        Logger.warning("Security: Player #{player.id} access attempted from wrong game context. URL game_id: #{player.game_id} vs actual game_id: #{player.game_id}, IP: #{socket.assigns[:client_ip]}")
+        {:ok, redirect(socket, to: "/error")}
       else
-        {:error, _reason} ->
-          {:ok, redirect(socket, to: "/error")}
+        with {:ok, game} <- Game.find(player.game_id) do
+          CopiWeb.Endpoint.subscribe(topic(player.game_id))
+          {:noreply, socket |> assign(:game, game) |> assign(:player, player)}
+        else
+          {:error, _reason} ->
+            Logger.warning("Security: Game lookup failed for player #{player.id}, game_id: #{player.game_id}, IP: #{socket.assigns[:client_ip]}")
+            {:ok, redirect(socket, to: "/error")}
+        end
       end
     else
       {:error, _reason} ->
+        Logger.warning("Security: Player lookup failed for player_id: #{player_id}, IP: #{socket.assigns[:client_ip]}")
         {:ok, redirect(socket, to: "/error")}
     end
   end