From 47adcc56f2e6f563514e8b55bf517276deb65311 Mon Sep 17 00:00:00 2001 From: immortal71 Date: Fri, 20 Mar 2026 07:39:26 -0700 Subject: [PATCH 1/3] fix: lifecycle guard for play_card API endpoint (#2631) Reject card plays with 422 Unprocessable Entity when: - game.started_at == nil (game has not started yet) - game.finished_at != nil (game has already ended) Add tests covering both lifecycle guard cases, and update existing tests that exercise the success and card/player validation paths to first start the game, since those paths now require an active game. --- .../copi_web/controllers/api_controller.ex | 61 +++++++++++-------- .../controllers/api_controller_test.exs | 29 +++++++++ 2 files changed, 63 insertions(+), 27 deletions(-) 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 adb3711a0..69c787ef5 100644 --- a/copi.owasp.org/lib/copi_web/controllers/api_controller.ex +++ b/copi.owasp.org/lib/copi_web/controllers/api_controller.ex @@ -3,36 +3,43 @@ defmodule CopiWeb.ApiController do 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) - if player do - dealt_card = Enum.find(player.dealt_cards, fn dealt_card -> Integer.to_string(dealt_card.id) == dealt_card_id end) - 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 - 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"}) + cond do + is_nil(game.started_at) -> + conn |> put_status(:unprocessable_entity) |> json(%{"error" => "Game has not started yet"}) + not is_nil(game.finished_at) -> + conn |> put_status(:unprocessable_entity) |> json(%{"error" => "Game has already ended"}) + true -> + player = Enum.find(game.players, fn player -> player.id == player_id end) + if player do + dealt_card = Enum.find(player.dealt_cards, fn dealt_card -> Integer.to_string(dealt_card.id) == dealt_card_id end) + 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 + 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"}) end - conn |> json(%{"id" => dealt_card.id}) - {:error, _changeset} -> - conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not update dealt card"}) end + else + conn |> put_status(:not_found) |> json(%{"error" => "Could not find player and dealt card"}) + end + else + conn |> put_status(:not_found) |> json(%{"error" => "Player not found in this game"}) end - else - conn |> put_status(:not_found) |> json(%{"error" => "Could not find player and dealt card"}) - end - else - conn |> put_status(:not_found) |> json(%{"error" => "Player not found in this game"}) end else {:error, _reason} -> conn |> put_status(:not_found) |> json(%{"error" => "Could not find game"}) diff --git a/copi.owasp.org/test/copi_web/controllers/api_controller_test.exs b/copi.owasp.org/test/copi_web/controllers/api_controller_test.exs index 0d80890e9..fe3357965 100644 --- a/copi.owasp.org/test/copi_web/controllers/api_controller_test.exs +++ b/copi.owasp.org/test/copi_web/controllers/api_controller_test.exs @@ -22,6 +22,8 @@ defmodule CopiWeb.ApiControllerTest do end test "play_card success", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do + {:ok, _} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)}) + conn = put(conn, "/api/games/#{game.id}/players/#{player.id}/card", %{ "game_id" => game.id, "player_id" => player.id, @@ -35,6 +37,7 @@ defmodule CopiWeb.ApiControllerTest do end test "play_card fails if card already played", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do + {:ok, _} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)}) {:ok, _} = Repo.update(Ecto.Changeset.change(dealt_card, played_in_round: 1)) conn = put(conn, "/api/games/#{game.id}/players/#{player.id}/card", %{ @@ -75,7 +78,33 @@ defmodule CopiWeb.ApiControllerTest do assert json_response(conn, 404)["error"] == "Player not found in this game" end + test "play_card returns 422 when game has not started", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do + conn = put(conn, "/api/games/#{game.id}/players/#{player.id}/card", %{ + "game_id" => game.id, + "player_id" => player.id, + "dealt_card_id" => to_string(dealt_card.id) + }) + + assert json_response(conn, 422)["error"] == "Game has not started yet" + end + + test "play_card returns 422 when game has already ended", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do + {:ok, _} = Cornucopia.update_game(game, %{ + started_at: DateTime.truncate(DateTime.utc_now(), :second), + finished_at: DateTime.truncate(DateTime.utc_now(), :second) + }) + + conn = put(conn, "/api/games/#{game.id}/players/#{player.id}/card", %{ + "game_id" => game.id, + "player_id" => player.id, + "dealt_card_id" => to_string(dealt_card.id) + }) + + assert json_response(conn, 422)["error"] == "Game has already ended" + end + test "play_card fails if player already played in round", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do + {:ok, _} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)}) {:ok, card2} = Cornucopia.create_card(%{ category: "Cornucopia", value: "K", description: "desc", misc: "misc", edition: "webapp", external_id: "2", language: "en", version: "1", From 787568795090cbee2f417be707e7729b56dba4e4 Mon Sep 17 00:00:00 2001 From: immortal71 Date: Fri, 20 Mar 2026 08:06:53 -0700 Subject: [PATCH 2/3] test: fix 404 tests that need game started_at set --- .../test/copi_web/controllers/api_controller_test.exs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/copi.owasp.org/test/copi_web/controllers/api_controller_test.exs b/copi.owasp.org/test/copi_web/controllers/api_controller_test.exs index fe3357965..a3df780ee 100644 --- a/copi.owasp.org/test/copi_web/controllers/api_controller_test.exs +++ b/copi.owasp.org/test/copi_web/controllers/api_controller_test.exs @@ -58,6 +58,7 @@ defmodule CopiWeb.ApiControllerTest do end test "play_card returns 404 when dealt card not found for player", %{conn: conn, game: game} do + {:ok, _} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)}) {:ok, other_game} = Cornucopia.create_game(%{name: "Other Game"}) {:ok, other_player} = Cornucopia.create_player(%{name: "Other", game_id: other_game.id}) {:ok, card2} = Cornucopia.create_card(%{ @@ -124,6 +125,7 @@ defmodule CopiWeb.ApiControllerTest do end test "play_card returns 404 when player_id doesn't belong to game", %{conn: conn, game: game} do + {:ok, _} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)}) conn = put(conn, "/api/games/#{game.id}/players/99999/card", %{ "game_id" => game.id, "player_id" => "99999", From 753dc47e2ae29f0c4dcbe1aa25b9747d78d49610 Mon Sep 17 00:00:00 2001 From: Aashish kharel Date: Sat, 11 Apr 2026 17:23:11 +0000 Subject: [PATCH 3/3] fix: return 500 when updated game reload fails in play_card --- copi.owasp.org/lib/copi_web/controllers/api_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 69c787ef5..f465eac0a 100644 --- a/copi.owasp.org/lib/copi_web/controllers/api_controller.ex +++ b/copi.owasp.org/lib/copi_web/controllers/api_controller.ex @@ -25,11 +25,11 @@ defmodule CopiWeb.ApiController do {:ok, dealt_card} -> with {:ok, updated_game} <- Game.find(game.id) do CopiWeb.Endpoint.broadcast(topic(game.id), "game:updated", updated_game) + conn |> json(%{"id" => dealt_card.id}) 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"}) end