fix: handle invalid dealt_card_id in PlayerLive toggle_vote gracefully (fixes #2843)

copi.owasp.org/lib/copi_web/live/player_live/index.html.heex

@@ -1,4 +1,5 @@
 
+<%= if @player do %>
   <.live_component
     module={CopiWeb.PlayerLive.FormComponent}
     id={:new}
@@ -8,6 +9,7 @@
     client_ip={@client_ip}
     patch={~p"/games/#{@game.id}"}
   />
+<% end %>
 
 
 <!--

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

@@ -121,34 +121,39 @@ defmodule CopiWeb.PlayerLive.Show do
     game = socket.assigns.game
     player = socket.assigns.player
 
-    {:ok, dealt_card} = DealtCard.find(dealt_card_id)
-
-    game_card_ids = game.players
-      |> Enum.flat_map(fn p -> p.dealt_cards end)
-      |> Enum.map(fn dc -> dc.id end)
-
-    if dealt_card.id in game_card_ids do
-      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)}")
+    case DealtCard.find(dealt_card_id) do
+      {:error, _reason} ->
+        Logger.warning("toggle_vote: dealt_card_id=#{dealt_card_id} not found for player_id=#{player.id}, game_id=#{game.id}")
+        {:noreply, socket |> put_flash(:error, "Invalid card selection")}
+
+      {:ok, dealt_card} ->
+        game_card_ids = game.players
+          |> Enum.flat_map(fn p -> p.dealt_cards end)
+          |> Enum.map(fn dc -> dc.id end)
+
+        if dealt_card.id in game_card_ids do
+          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
+
+          {:ok, updated_game} = Game.find(game.id)
+          CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game)
+          {:noreply, assign(socket, :game, updated_game)}
+        else
+          Logger.warning("Unauthorized vote attempt: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}")
+          {:noreply, socket |> put_flash(:error, "Invalid card selection")}
         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)}
-    else
-      Logger.warning("Unauthorized vote attempt: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}")
-      {:noreply, socket |> put_flash(:error, "Invalid card selection")}
     end
   end
 

copi.owasp.org/test/copi/cornucopia/dealt_card_test.exs

@@ -0,0 +1,24 @@
+defmodule Copi.Cornucopia.DealtCardTest do
+  use Copi.DataCase, async: true
+
+  alias Copi.Cornucopia.DealtCard
+
+  describe "changeset/2" do
+    test "returns a valid changeset with empty attrs" do
+      changeset = DealtCard.changeset(%DealtCard{}, %{})
+      assert changeset.valid?
+    end
+
+    test "casts played_in_round when provided" do
+      changeset = DealtCard.changeset(%DealtCard{}, %{played_in_round: 3})
+      # changeset/2 only casts [], so played_in_round is ignored but changeset is valid
+      assert changeset.valid?
+    end
+  end
+
+  describe "find/1" do
+    test "returns error for nonexistent dealt card id" do
+      assert {:error, :not_found} = DealtCard.find(-1)
+    end
+  end
+end

copi.owasp.org/test/copi/cornucopia/player_test.exs

@@ -0,0 +1,32 @@
+defmodule Copi.Cornucopia.PlayerTest do
+  use ExUnit.Case, async: true
+
+  alias Copi.Cornucopia.Player
+
+  describe "changeset/2" do
+    test "valid with name only" do
+      # game_id is not in validate_required, so name alone is enough
+      changeset = Player.changeset(%Player{}, %{name: "Alice"})
+      assert changeset.valid?
+    end
+
+    test "invalid without name" do
+      changeset = Player.changeset(%Player{}, %{})
+      refute changeset.valid?
+      assert %{name: ["can't be blank"]} = errors_on(changeset)
+    end
+
+    test "invalid when name too long" do
+      changeset = Player.changeset(%Player{}, %{name: String.duplicate("a", 51)})
+      refute changeset.valid?
+    end
+  end
+
+  defp errors_on(changeset) do
+    Ecto.Changeset.traverse_errors(changeset, fn {msg, opts} ->
+      Enum.reduce(opts, msg, fn {key, value}, acc ->
+        String.replace(acc, "%{#{key}}", to_string(value))
+      end)
+    end)
+  end
+end

copi.owasp.org/test/copi/cornucopia/vote_test.exs

@@ -0,0 +1,12 @@
+defmodule Copi.Cornucopia.VoteTest do
+  use ExUnit.Case, async: true
+
+  alias Copi.Cornucopia.Vote
+
+  describe "changeset/2" do
+    test "valid with empty attrs" do
+      changeset = Vote.changeset(%Vote{}, %{})
+      assert changeset.valid?
+    end
+  end
+end

copi.owasp.org/test/copi/ip_helper_test.exs

@@ -128,10 +128,37 @@ defmodule Copi.IPHelperTest do
       socket = %Phoenix.Socket{transport_pid: self()}
       Process.put(:peer, {{10, 0, 0, 5}, 12345})
       assert IPHelper.get_ip_from_socket(socket) == {10, 0, 0, 5}
-      
+
       socket2 = %Phoenix.Socket{transport_pid: nil}
       assert IPHelper.get_ip_from_socket(socket2) == {127, 0, 0, 1}
     end
+
+    test "falls back to localhost for LiveView socket with empty map connect_info" do
+      # connect_info is a map with no headers and no peer_data
+      socket = %Phoenix.LiveView.Socket{private: %{connect_info: %{}}}
+      assert IPHelper.get_ip_from_socket(socket) == {127, 0, 0, 1}
+    end
+
+    test "skips atom-key req_header tuple and falls back to binary-keyed one" do
+      socket = %Phoenix.LiveView.Socket{
+        private: %{connect_info: %{req_headers: [{:some_key, "ignored"}, {"x-forwarded-for", "10.0.2.5"}]}}
+      }
+      assert IPHelper.get_ip_from_socket(socket) == {10, 0, 2, 5}
+    end
+
+    test "falls back to peer_data when connect_info map x_headers has no x-forwarded-for" do
+      socket = %Phoenix.LiveView.Socket{
+        private: %{connect_info: %{x_headers: [{"other-header", "val"}], peer_data: %{address: {10, 0, 2, 6}}}}
+      }
+      assert IPHelper.get_ip_from_socket(socket) == {10, 0, 2, 6}
+    end
+
+    test "returns localhost when connect_info map has no peer_data and no useful headers" do
+      socket = %Phoenix.LiveView.Socket{
+        private: %{connect_info: %{x_headers: []}}
+      }
+      assert IPHelper.get_ip_from_socket(socket) == {127, 0, 0, 1}
+    end
   end
 
   describe "get_ip_from_connect_info/1" do

copi.owasp.org/test/copi/rate_limiter_test.exs

@@ -247,6 +247,9 @@ defmodule Copi.RateLimiterTest do
       assert {:ok, _} = RateLimiter.check_rate("invalid-ip", :game_creation)
     end
 
+  end
+
+  describe "prod env bypass" do
     test "bypasses rate limit in production mode for localhost" do
       Application.put_env(:copi, :env, :prod)
 

copi.owasp.org/test/copi_web/live/game_live/show_pure_test.exs

@@ -0,0 +1,30 @@
+defmodule CopiWeb.GameLive.ShowPureTest do
+  use ExUnit.Case, async: true
+
+  alias CopiWeb.GameLive.Show
+
+  test "display_game_session returns expected labels" do
+    assert Show.display_game_session("webapp") == "Cornucopia Web Session:"
+    assert Show.display_game_session("mobileapp") == "Cornucopia Mobile Session:"
+    assert Show.display_game_session("mlsec") == "Elevation of MLSec Session:"
+    assert Show.display_game_session("unknown") == "EoP Session:"
+  end
+
+  test "latest_version returns expected versions" do
+    assert Show.latest_version("webapp") == "2.2"
+    assert Show.latest_version("ecommerce") == "1.22"
+    assert Show.latest_version("eop") == "5.1"
+    assert Show.latest_version("other") == "1.0"
+  end
+
+  test "card_played_in_round finds card or returns nil" do
+    cards = [%{played_in_round: 1, id: "a"}, %{played_in_round: 3, id: "b"}]
+    assert Show.card_played_in_round(cards, 3) == %{played_in_round: 3, id: "b"}
+    assert Show.card_played_in_round(cards, 2) == nil
+  end
+
+  test "topic builds topic string" do
+    assert Show.topic(42) == "game:42"
+    assert Show.topic("abc") == "game:abc"
+  end
+end

copi.owasp.org/test/copi_web/live/player_live/show_pure_test.exs

@@ -0,0 +1,85 @@
+defmodule CopiWeb.PlayerLive.ShowPureTest do
+  use ExUnit.Case, async: true
+
+  alias CopiWeb.PlayerLive.Show
+  alias CopiWeb.PlayerLive.FormComponent
+
+  test "topic/1 builds topic strings" do
+    assert Show.topic(1) == "game:1"
+    assert Show.topic("abc") == "game:abc"
+  end
+
+  test "FormComponent.topic/1 builds topic strings" do
+    assert FormComponent.topic(99) == "game:99"
+    assert FormComponent.topic("xyz") == "game:xyz"
+  end
+
+  test "ordered_cards/1 sorts by card.id" do
+    cards = [%{card: %{id: 3}}, %{card: %{id: 1}}, %{card: %{id: 2}}]
+    sorted = Show.ordered_cards(cards)
+    assert Enum.map(sorted, & &1.card.id) == [1, 2, 3]
+  end
+
+  test "unplayed_cards/1 filters unplayed (0 or nil)" do
+    cards = [%{played_in_round: 0, card: %{id: 1}}, %{played_in_round: nil, card: %{id: 2}}, %{played_in_round: 2, card: %{id: 3}}]
+    res = Show.unplayed_cards(cards)
+    assert Enum.map(res, & &1.card.id) == [1, 2]
+  end
+
+  test "played_cards/2 returns those in given round" do
+    cards = [%{played_in_round: 1, card: %{id: 1}}, %{played_in_round: 2, card: %{id: 2}}]
+    assert Show.played_cards(cards, 2) == [%{played_in_round: 2, card: %{id: 2}}]
+  end
+
+  test "card_played_in_round/2 finds first matching card" do
+    cards = [%{played_in_round: 1, card: %{id: 1}}, %{played_in_round: 2, card: %{id: 2}}]
+    assert Show.card_played_in_round(cards, 2) == %{played_in_round: 2, card: %{id: 2}}
+    assert Show.card_played_in_round([], 1) == nil
+  end
+
+  test "player_first/2 places given player first" do
+    players = [%{id: 2}, %{id: 1}, %{id: 3}]
+    res = Show.player_first(players, %{id: 1})
+    assert hd(res).id == 1
+  end
+
+  test "round_open?/1 and round_closed?/1 reflect player dealt cards" do
+    # latest_round = rounds_played + 1 = 2
+    # player1 has a card for round 2 -> has played
+    player1 = %{id: 1, dealt_cards: [%{played_in_round: 2}]}
+    # player2 has no card for round 2 -> still to play
+    player2 = %{id: 2, dealt_cards: [%{played_in_round: 1}]}
+    game = %{rounds_played: 1, players: [player1, player2]}
+
+    assert Show.round_open?(game) == true
+    assert Show.round_closed?(game) == false
+  end
+
+  test "last_round?/1 detects when players have no nil played_in_round" do
+    # player with no nil played_in_round => they have no cards left
+    player1 = %{dealt_cards: [%{played_in_round: 1}]} # no nil cards
+    player2 = %{dealt_cards: [%{played_in_round: nil}]} # has nil -> still has cards
+    game = %{players: [player1, player2]}
+
+    # last_round? returns true when there is at least one player with no nil cards
+    assert Show.last_round?(game) == true
+  end
+
+  test "get_vote/2 finds vote by player id" do
+    votes = [%{player_id: 1, id: 10}, %{player_id: 2, id: 11}]
+    dealt_card = %{votes: votes}
+    player = %{id: 2}
+    assert Show.get_vote(dealt_card, player) == %{player_id: 2, id: 11}
+  end
+
+  test "display_game_session/1 returns correct label for every edition" do
+    assert Show.display_game_session("webapp")    == "Cornucopia Web Session:"
+    assert Show.display_game_session("ecommerce") == "Cornucopia Web Session:"
+    assert Show.display_game_session("mobileapp") == "Cornucopia Mobile Session:"
+    assert Show.display_game_session("masvs")     == "Cornucopia Mobile Session:"
+    assert Show.display_game_session("cumulus")   == "OWASP Cumulus Session:"
+    assert Show.display_game_session("mlsec")     == "Elevation of MLSec Session:"
+    assert Show.display_game_session("eop")       == "EoP Session:"
+    assert Show.display_game_session("unknown")   == "EoP Session:"
+  end
+end

copi.owasp.org/test/copi_web/live/player_live/show_test.exs

@@ -37,6 +37,11 @@ defmodule CopiWeb.PlayerLive.ShowTest do
   describe "Show - additional coverage" do
     setup [:create_player]
 
+    test "handle_params redirects to /error for nonexistent player_id", %{conn: conn, player: _player} do
+      assert {:error, {:redirect, %{to: "/error"}}} =
+               live(conn, "/games/00000000000000000000000001/players/00000000000000000000000002")
+    end
+
     test "handle_info :proceed_to_next_round advances rounds_played", %{conn: conn, player: player} do
       game_id = player.game_id
       {:ok, game} = Cornucopia.Game.find(game_id)
@@ -294,6 +299,21 @@ defmodule CopiWeb.PlayerLive.ShowTest do
     end
   end
 
+  describe "toggle_vote with invalid dealt_card_id" do
+    test "returns flash error and keeps socket alive for non-existent dealt_card_id", %{conn: conn} do
+      {:ok, game} = Cornucopia.create_game(%{name: "Invalid ID Test Game", edition: "webapp"})
+      {:ok, player} = Cornucopia.create_player(%{name: "Player One", game_id: game.id})
+
+      {:ok, view, _html} = live(conn, "/games/#{game.id}/players/#{player.id}")
+
+      # Send a phx-click payload with a non-existent dealt_card_id
+      render_click(view, "toggle_vote", %{"dealt_card_id" => "999999999"})
+
+      # LiveView should still be alive and show a flash error
+      assert render(view) =~ "Invalid card selection"
+    end
+  end
+
   describe "toggle_vote authorization" do
     test "rejects cross-game vote and shows error flash", %{conn: conn} do
       {game1, player1, _dc1} = create_game_with_dealt_card("Auth Game One", "AUTH_G1_C1")

copi.owasp.org/test/copi_web/live/player_live_test.exs

@@ -70,6 +70,24 @@ defmodule CopiWeb.PlayerLiveTest do
       assert html =~ "Hi some updated name, waiting for the game to start..."
     end
 
+    test "lists players on index route", %{conn: conn, player: player} do
+      {:ok, _index_live, html} = live(conn, "/games/#{player.game_id}/players")
+      assert html =~ "Listing Players"
+    end
+
+    test "shows validation error when submitting empty player name", %{conn: conn, player: player} do
+      RateLimiter.clear_ip({127, 0, 0, 1})
+      {:ok, index_live, _html} = live(conn, "/games/#{player.game_id}/players/new")
+
+      # Submit with empty name → triggers {:error, changeset} in save_player(:new, ...)
+      html =
+        index_live
+        |> form("#player-form", player: %{name: "", game_id: player.game_id})
+        |> render_submit()
+
+      assert html =~ "can't be blank"
+    end
+
     test "blocks player creation when rate limit exceeded", %{conn: conn, player: player} do
       test_ip = {127, 0, 0, 1}
       RateLimiter.clear_ip(test_ip)

copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_test.exs

@@ -97,4 +97,9 @@ defmodule CopiWeb.Plugs.RateLimiterPlugTest do
     assert conn.status != 429
     refute conn.halted
   end
+
+  test "init/1 passes opts through unchanged" do
+    assert RateLimiterPlug.init([]) == []
+    assert RateLimiterPlug.init(foo: :bar) == [foo: :bar]
+  end
 end