From 2cf1793ca4d96eac6663ccde3ccc323543b5c948 Mon Sep 17 00:00:00 2001 From: Khushal Malhotra Date: Tue, 10 Mar 2026 19:44:00 +0530 Subject: [PATCH 1/4] Fix voting security: block voting before game starts and after game ends --- copi.owasp.org/lib/copi/cornucopia/game.ex | 4 ++ .../lib/copi_web/live/player_live/show.ex | 39 ++++++----- .../copi_web/live/player_live/show_test.exs | 65 +++++++++++++++++++ 3 files changed, 91 insertions(+), 17 deletions(-) diff --git a/copi.owasp.org/lib/copi/cornucopia/game.ex b/copi.owasp.org/lib/copi/cornucopia/game.ex index 652eccec2..9a6fcc3fc 100644 --- a/copi.owasp.org/lib/copi/cornucopia/game.ex +++ b/copi.owasp.org/lib/copi/cornucopia/game.ex @@ -62,4 +62,8 @@ defmodule Copi.Cornucopia.Game do Enum.count(players_still_to_play) > 0 end + + def game_active?(game) do + game.started_at != nil and game.finished_at == nil + 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..4c77c9b50 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 @@ -129,28 +129,33 @@ defmodule CopiWeb.PlayerLive.Show do game = socket.assigns.game player = socket.assigns.player - {:ok, dealt_card} = DealtCard.find(dealt_card_id) - - vote = get_vote(dealt_card, player) - - if vote do - Logger.debug("Player has voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") - Copi.Repo.delete!(vote) + # Validate game lifecycle - voting only allowed during active games + unless Copi.Cornucopia.Game.game_active?(game) do + Logger.warning("Voting attempt on inactive game: player_id: #{player.id}, game_id: #{game.id}, started_at: #{game.started_at}, finished_at: #{game.finished_at}") + {:noreply, socket} else - Logger.debug("Player has not voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") - case Copi.Repo.insert(%Copi.Cornucopia.Vote{dealt_card_id: String.to_integer(dealt_card_id), player_id: player.id}) do - {:ok, _vote} -> - Logger.debug("Vote added successfully for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") - {:error, changeset} -> - Logger.warning("Voting failed for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}, errors: #{inspect(changeset.errors)}") + {:ok, dealt_card} = DealtCard.find(dealt_card_id) + vote = get_vote(dealt_card, player) + + if vote do + Logger.debug("Player has voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") + Copi.Repo.delete!(vote) + else + Logger.debug("Player has not voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") + case Copi.Repo.insert(%Copi.Cornucopia.Vote{dealt_card_id: String.to_integer(dealt_card_id), player_id: player.id}) do + {:ok, _vote} -> + Logger.debug("Vote added successfully for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") + {:error, changeset} -> + Logger.warning("Voting failed for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}, errors: #{inspect(changeset.errors)}") + end end - end - {:ok, updated_game} = Game.find(game.id) + {:ok, updated_game} = Game.find(game.id) - CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) + CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) - {:noreply, assign(socket, :game, updated_game)} + {:noreply, assign(socket, :game, updated_game)} + end end def topic(game_id) do diff --git a/copi.owasp.org/test/copi_web/live/player_live/show_test.exs b/copi.owasp.org/test/copi_web/live/player_live/show_test.exs index 30d135ee4..4263b2251 100644 --- a/copi.owasp.org/test/copi_web/live/player_live/show_test.exs +++ b/copi.owasp.org/test/copi_web/live/player_live/show_test.exs @@ -286,5 +286,70 @@ defmodule CopiWeb.PlayerLive.ShowTest do {:ok, updated_dealt2} = Copi.Cornucopia.DealtCard.find(to_string(dealt.id)) assert length(updated_dealt2.votes) == 0 end + + test "toggle_vote should not work before game starts", %{conn: conn, player: player} do + game_id = player.game_id + {:ok, game} = Cornucopia.Game.find(game_id) + + # Ensure game has NOT started (started_at is nil) + assert game.started_at == nil + + {:ok, card} = + Cornucopia.create_card(%{ + category: "C", value: "TV1", description: "D", edition: "webapp", + version: "2.2", external_id: "TV_CARD1", language: "en", misc: "m", + owasp_scp: [], owasp_devguide: [], owasp_asvs: [], owasp_appsensor: [], + capec: [], safecode: [], owasp_mastg: [], owasp_masvs: [] + }) + + dealt = Copi.Repo.insert!(%Copi.Cornucopia.DealtCard{ + player_id: player.id, card_id: card.id, played_in_round: 1 + }) + + {:ok, show_live, _html} = live(conn, "/games/#{game_id}/players/#{player.id}") + + # Attempt to vote before game starts - should be ignored + render_click(show_live, "toggle_vote", %{"dealt_card_id" => to_string(dealt.id)}) + :timer.sleep(100) + + {:ok, updated_dealt} = Copi.Cornucopia.DealtCard.find(to_string(dealt.id)) + assert length(updated_dealt.votes) == 0 + end + + test "toggle_vote should not work after game ends", %{conn: conn, player: player} do + game_id = player.game_id + {:ok, game} = Cornucopia.Game.find(game_id) + + # Start the game + Copi.Repo.update!( + Ecto.Changeset.change(game, started_at: DateTime.truncate(DateTime.utc_now(), :second)) + ) + + {:ok, card} = + Cornucopia.create_card(%{ + category: "C", value: "TV1", description: "D", edition: "webapp", + version: "2.2", external_id: "TV_CARD1", language: "en", misc: "m", + owasp_scp: [], owasp_devguide: [], owasp_asvs: [], owasp_appsensor: [], + capec: [], safecode: [], owasp_mastg: [], owasp_masvs: [] + }) + + dealt = Copi.Repo.insert!(%Copi.Cornucopia.DealtCard{ + player_id: player.id, card_id: card.id, played_in_round: 1 + }) + + # Now end the game + Copi.Repo.update!( + Ecto.Changeset.change(game, finished_at: DateTime.truncate(DateTime.utc_now(), :second)) + ) + + {:ok, show_live, _html} = live(conn, "/games/#{game_id}/players/#{player.id}") + + # Attempt to vote after game ends - should be ignored + render_click(show_live, "toggle_vote", %{"dealt_card_id" => to_string(dealt.id)}) + :timer.sleep(100) + + {:ok, updated_dealt} = Copi.Cornucopia.DealtCard.find(to_string(dealt.id)) + assert length(updated_dealt.votes) == 0 + end end end From 8a139464675cc707b7003bd7a62942e50ecec833 Mon Sep 17 00:00:00 2001 From: Khushal Malhotra Date: Tue, 10 Mar 2026 21:22:20 +0530 Subject: [PATCH 2/4] Fix code review findings: unique test IDs and continue vote validation --- .../lib/copi_web/live/player_live/show.ex | 40 +++++++++++-------- .../copi_web/live/player_live/show_test.exs | 4 +- 2 files changed, 25 insertions(+), 19 deletions(-) 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 4c77c9b50..4c64610b2 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 @@ -100,28 +100,34 @@ defmodule CopiWeb.PlayerLive.Show do game = socket.assigns.game player = socket.assigns.player - # Check if player already voted - if Copi.Cornucopia.Game.has_continue_vote?(game, player) do - # Remove their vote - continue_vote = Enum.find(game.continue_votes, fn vote -> vote.player_id == player.id end) - if continue_vote do - Copi.Repo.delete!(continue_vote) - end + # Validate game lifecycle - continue voting only allowed during active games + unless Copi.Cornucopia.Game.game_active?(game) do + Logger.warning("Continue vote attempt on inactive game: player_id: #{player.id}, game_id: #{game.id}, started_at: #{game.started_at}, finished_at: #{game.finished_at}") + {:noreply, socket} else - # Add their vote - case Copi.Repo.insert(%Copi.Cornucopia.ContinueVote{player_id: player.id, game_id: game.id}) do - {:ok, _vote} -> - Logger.debug("Continue vote added successfully for player_id: #{player.id}, game_id: #{game.id}") - {:error, changeset} -> - Logger.warning("Continue voting failed for player_id: #{player.id}, game_id: #{game.id}, errors: #{inspect(changeset.errors)}") + # Check if player already voted + if Copi.Cornucopia.Game.has_continue_vote?(game, player) do + # Remove their vote + continue_vote = Enum.find(game.continue_votes, fn vote -> vote.player_id == player.id end) + if continue_vote do + Copi.Repo.delete!(continue_vote) + end + else + # Add their vote + case Copi.Repo.insert(%Copi.Cornucopia.ContinueVote{player_id: player.id, game_id: game.id}) do + {:ok, _vote} -> + Logger.debug("Continue vote added successfully for player_id: #{player.id}, game_id: #{game.id}") + {:error, changeset} -> + Logger.warning("Continue voting failed for player_id: #{player.id}, game_id: #{game.id}, errors: #{inspect(changeset.errors)}") + end end - end - {:ok, updated_game} = Game.find(game.id) + {:ok, updated_game} = Game.find(game.id) - CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) + CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) - {:noreply, assign(socket, :game, updated_game)} + {:noreply, assign(socket, :game, updated_game)} + end end @impl true diff --git a/copi.owasp.org/test/copi_web/live/player_live/show_test.exs b/copi.owasp.org/test/copi_web/live/player_live/show_test.exs index 4263b2251..2d0d35e21 100644 --- a/copi.owasp.org/test/copi_web/live/player_live/show_test.exs +++ b/copi.owasp.org/test/copi_web/live/player_live/show_test.exs @@ -297,7 +297,7 @@ defmodule CopiWeb.PlayerLive.ShowTest do {:ok, card} = Cornucopia.create_card(%{ category: "C", value: "TV1", description: "D", edition: "webapp", - version: "2.2", external_id: "TV_CARD1", language: "en", misc: "m", + version: "2.2", external_id: "TV_CARD_PRE_GAME", language: "en", misc: "m", owasp_scp: [], owasp_devguide: [], owasp_asvs: [], owasp_appsensor: [], capec: [], safecode: [], owasp_mastg: [], owasp_masvs: [] }) @@ -328,7 +328,7 @@ defmodule CopiWeb.PlayerLive.ShowTest do {:ok, card} = Cornucopia.create_card(%{ category: "C", value: "TV1", description: "D", edition: "webapp", - version: "2.2", external_id: "TV_CARD1", language: "en", misc: "m", + version: "2.2", external_id: "TV_CARD_AFTER_END", language: "en", misc: "m", owasp_scp: [], owasp_devguide: [], owasp_asvs: [], owasp_appsensor: [], capec: [], safecode: [], owasp_mastg: [], owasp_masvs: [] }) From 9b1bbf0a5ac10764c20d03998d55b443cbf2b1c5 Mon Sep 17 00:00:00 2001 From: Khushal Malhotra Date: Tue, 10 Mar 2026 22:00:15 +0530 Subject: [PATCH 3/4] Add safe pattern matching for Game.find and DealtCard.find calls --- .../lib/copi_web/live/player_live/show.ex | 68 ++++++++++++------- 1 file changed, 43 insertions(+), 25 deletions(-) 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 4c64610b2..86981c120 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 @@ -122,11 +122,15 @@ defmodule CopiWeb.PlayerLive.Show do end end - {:ok, updated_game} = Game.find(game.id) - - CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) - - {:noreply, assign(socket, :game, updated_game)} + # Reload fresh game record after continue vote mutations + case Game.find(game.id) do + {:ok, updated_game} -> + CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) + {:noreply, assign(socket, :game, updated_game)} + {:error, reason} -> + Logger.warning("Failed to reload game after continue vote: game_id: #{game.id}, reason: #{inspect(reason)}") + {:noreply, socket} + end end end @@ -140,27 +144,41 @@ defmodule CopiWeb.PlayerLive.Show do Logger.warning("Voting attempt on inactive game: player_id: #{player.id}, game_id: #{game.id}, started_at: #{game.started_at}, finished_at: #{game.finished_at}") {:noreply, socket} else - {:ok, dealt_card} = DealtCard.find(dealt_card_id) - vote = get_vote(dealt_card, player) - - if vote do - Logger.debug("Player has voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") - Copi.Repo.delete!(vote) - else - Logger.debug("Player has not voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") - case Copi.Repo.insert(%Copi.Cornucopia.Vote{dealt_card_id: String.to_integer(dealt_card_id), player_id: player.id}) do - {:ok, _vote} -> - Logger.debug("Vote added successfully for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") - {:error, changeset} -> - Logger.warning("Voting failed for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}, errors: #{inspect(changeset.errors)}") - end + case DealtCard.find(dealt_card_id) do + {:ok, dealt_card} -> + # Validate that dealt card belongs to current game + unless dealt_card_belongs_to_game?(dealt_card, game) do + Logger.warning("Unauthorized voting attempt: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") + {:noreply, socket} + else + vote = get_vote(dealt_card, player) + + if vote do + Logger.debug("Player has voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") + Copi.Repo.delete!(vote) + else + Logger.debug("Player has not voted: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") + case Copi.Repo.insert(%Copi.Cornucopia.Vote{dealt_card_id: String.to_integer(dealt_card_id), player_id: player.id}) do + {:ok, _vote} -> + Logger.debug("Vote added successfully for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") + {:error, changeset} -> + Logger.warning("Voting failed for player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}, errors: #{inspect(changeset.errors)}") + end + end + + case Game.find(game.id) do + {:ok, updated_game} -> + CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) + {:noreply, assign(socket, :game, updated_game)} + {:error, reason} -> + Logger.warning("Failed to reload game after vote: game_id: #{game.id}, reason: #{inspect(reason)}") + {:noreply, socket} + end + end + {:error, reason} -> + Logger.warning("Failed to find dealt card: dealt_card_id: #{dealt_card_id}, player_id: #{player.id}, game_id: #{game.id}, reason: #{inspect(reason)}") + {:noreply, socket} end - - {:ok, updated_game} = Game.find(game.id) - - CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) - - {:noreply, assign(socket, :game, updated_game)} end end From 6b2ce807506c61f1f83d4f688bb33563d2611e4d Mon Sep 17 00:00:00 2001 From: Khushal Malhotra Date: Tue, 10 Mar 2026 23:26:52 +0530 Subject: [PATCH 4/4] Add dealt_card_belongs_to_game? helper and continue vote lifecycle tests --- .../lib/copi_web/live/player_live/show.ex | 18 ++++---- .../copi_web/live/player_live/show_test.exs | 41 +++++++++++++++++++ 2 files changed, 48 insertions(+), 11 deletions(-) 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 86981c120..03bfa091b 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 @@ -228,16 +228,12 @@ defmodule CopiWeb.PlayerLive.Show do Enum.find(dealt_card.votes, fn vote -> vote.player_id == player.id end) end - def display_game_session(edition) do - case edition do - "webapp" -> "Cornucopia Web Session:" - "ecommerce" -> "Cornucopia Web Session:" - "mobileapp" -> "Cornucopia Mobile Session:" - "masvs" -> "Cornucopia Mobile Session:" - "cumulus" -> "OWASP Cumulus Session:" - "mlsec" -> "Elevation of MLSec Session:" - _ -> "EoP Session:" - end + defp dealt_card_belongs_to_game?(dealt_card, game) do + # Check if the dealt card's player belongs to the current game + player_ids = Enum.map(game.players, & &1.id) + dealt_card.player_id in player_ids end -end + def topic(game_id) do + "game:#{game_id}" + end diff --git a/copi.owasp.org/test/copi_web/live/player_live/show_test.exs b/copi.owasp.org/test/copi_web/live/player_live/show_test.exs index 2d0d35e21..467c1be2e 100644 --- a/copi.owasp.org/test/copi_web/live/player_live/show_test.exs +++ b/copi.owasp.org/test/copi_web/live/player_live/show_test.exs @@ -351,5 +351,46 @@ defmodule CopiWeb.PlayerLive.ShowTest do {:ok, updated_dealt} = Copi.Cornucopia.DealtCard.find(to_string(dealt.id)) assert length(updated_dealt.votes) == 0 end + + test "toggle_continue_vote should not work before game starts", %{conn: conn, player: player} do + game_id = player.game_id + {:ok, game} = Cornucopia.Game.find(game_id) + + # Ensure game has NOT started (started_at is nil) + assert game.started_at == nil + + {:ok, show_live, _html} = live(conn, "/games/#{game_id}/players/#{player.id}") + + # Attempt to continue vote before game starts - should be ignored + render_click(show_live, "toggle_continue_vote", %{}) + :timer.sleep(100) + + {:ok, updated_game} = Cornucopia.Game.find(game_id) + assert length(updated_game.continue_votes) == 0 + end + + test "toggle_continue_vote should not work after game ends", %{conn: conn, player: player} do + game_id = player.game_id + {:ok, game} = Cornucopia.Game.find(game_id) + + # Start the game + Copi.Repo.update!( + Ecto.Changeset.change(game, started_at: DateTime.truncate(DateTime.utc_now(), :second)) + ) + + # Now end the game + Copi.Repo.update!( + Ecto.Changeset.change(game, finished_at: DateTime.truncate(DateTime.utc_now(), :second)) + ) + + {:ok, show_live, _html} = live(conn, "/games/#{game_id}/players/#{player.id}") + + # Attempt to continue vote after game ends - should be ignored + render_click(show_live, "toggle_continue_vote", %{}) + :timer.sleep(100) + + {:ok, updated_game} = Cornucopia.Game.find(game_id) + assert length(updated_game.continue_votes) == 0 + end end end