diff --git a/copi.owasp.org/lib/copi_web/controllers/api_controller.ex b/copi.owasp.org/lib/copi_web/controllers/api_controller.ex index f181b8e41..b7cd0bb66 100644 --- a/copi.owasp.org/lib/copi_web/controllers/api_controller.ex +++ b/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 diff --git a/copi.owasp.org/lib/copi_web/live/player_live/show.ex b/copi.owasp.org/lib/copi_web/live/player_live/show.ex index bc71e3d8d..dbf631ddb 100644 --- a/copi.owasp.org/lib/copi_web/live/player_live/show.ex +++ b/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