feat: Implement atomic operations and rate limiting

copi.owasp.org/lib/copi/rate_limiter.ex

@@ -6,6 +6,7 @@ defmodule Copi.RateLimiter do
   - Game creation
   - Player creation
   - WebSocket connections
+  - API actions
 
   Rate limits are configured via environment variables and automatically clean up expired entries.
   """
@@ -30,21 +31,22 @@ defmodule Copi.RateLimiter do
 
   ## Parameters
     - ip: IP address as a tuple (e.g., {127, 0, 0, 1}) or string
-    - action: atom representing the action (:game_creation, :player_creation, :connection)
+    - action: atom representing the action (:game_creation, :player_creation, :connection, :api_action)
 
   ## Examples
       iex> Copi.RateLimiter.check_rate("127.0.0.1", :game_creation)
       {:ok, 9}
 
-      iex> Copi.RateLimiter.check_rate({127, 0, 0, 1}, :connection)
-      {:error, :rate_limit_exceeded}
+      iex> Copi.RateLimiter.check_rate({127, 0, 0, 1}, :api_action)
+      {:ok, 9}
   """
-  def check_rate(ip, action) when action in [:game_creation, :player_creation, :connection] do
+  def check_rate(ip, action) when action in [:game_creation, :player_creation, :connection, :api_action] do
     normalized_ip = normalize_ip(ip)
 
     # In production, don't rate limit localhost to prevent DoS'ing ourselves
+    # Only bypass rate limiting if the actual connection IP is loopback, not X-Forwarded-For
     Logger.debug("check_rate: Checking rate limit for IP #{inspect(normalized_ip)} on action #{action}")
-    if Application.get_env(:copi, :env) == :prod and normalized_ip == {127, 0, 0, 1} do
+    if Application.get_env(:copi, :env) == :prod and normalized_ip == {127, 0, 0, 1} and ip == {127, 0, 0, 1} do
       {:ok, :unlimited}
     else
       GenServer.call(__MODULE__, {:check_rate, normalized_ip, action})
@@ -58,6 +60,13 @@ defmodule Copi.RateLimiter do
     GenServer.cast(__MODULE__, {:clear_ip, normalize_ip(ip)})
   end
 
+  @doc """
+  Synchronously clears all rate limit data for a specific IP address (useful for testing).
+  """
+  def clear_ip_sync(ip) do
+    GenServer.call(__MODULE__, {:clear_ip, normalize_ip(ip)})
+  end
+
   @doc """
   Gets the current configuration for rate limits.
   """
@@ -76,12 +85,14 @@ defmodule Copi.RateLimiter do
       limits: %{
         game_creation: get_env_config(:game_creation_limit, 20),
         player_creation: get_env_config(:player_creation_limit, 60),
-        connection: get_env_config(:connection_limit, 133)
+        connection: get_env_config(:connection_limit, 133),
+        api_action: get_env_config(:api_action_limit, 133)
       },
       windows: %{
         game_creation: get_env_config(:game_creation_window, 3600),
         player_creation: get_env_config(:player_creation_window, 3600),
-        connection: get_env_config(:connection_window, 1)
+        connection: get_env_config(:connection_window, 1),
+        api_action: get_env_config(:api_action_window, 133)
       },
       requests: %{}
     }
@@ -127,6 +138,17 @@ defmodule Copi.RateLimiter do
     {:reply, config, state}
   end
 
+  @impl true
+  def handle_call({:clear_ip, ip}, _from, state) do
+    new_requests =
+      state.requests
+      |> Enum.reject(fn {{request_ip, _action}, _timestamps} -> request_ip == ip end)
+      |> Enum.into(%{})
+
+    new_state = %{state | requests: new_requests}
+    {:reply, :ok, new_state}
+  end
+
   @impl true
   def handle_cast({:clear_ip, ip}, state) do
     new_requests =

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

@@ -1,47 +1,80 @@
 defmodule CopiWeb.ApiController do
   use CopiWeb, :controller
   alias Copi.Cornucopia.Game
-
+  alias Copi.Repo
+  import Ecto.Query
 
   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)
-
-      if player && dealt_card do
-        current_round = game.rounds_played + 1
+
+      if player do
+        dealt_card = Enum.find(player.dealt_cards, fn dealt_card -> Integer.to_string(dealt_card.id) == dealt_card_id end)
 
-        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
+        if dealt_card do
+          current_round = game.rounds_played + 1
 
-            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
+          # Atomic update to prevent race conditions and enforce one-card-per-round invariant
+          case play_card_atomically(dealt_card, player.id, current_round) do
+            {:ok, updated_card} ->
+              with {:ok, updated_game} <- Game.find(game.id) do
+                CopiWeb.Endpoint.broadcast(topic(game.id), "game:updated", updated_game)
+                conn |> json(%{"id" => updated_card.id})
+              else
+                {:error, _reason} ->
+                  conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not find updated game"})
+              end
+            {:error, :already_played} ->
+              conn |> put_status(:conflict) |> json(%{"error" => "Card was already played by another request"})
+            {:error, :player_already_played} ->
+              conn |> put_status(:forbidden) |> json(%{"error" => "Player already played a card in this round"})
+            {:error, _changeset} ->
+              conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not update card"})
+          end
+        else
+          conn |> put_status(:not_found) |> json(%{"error" => "Could not find dealt card"})
         end
       else
-        conn |> put_status(:not_found) |> json(%{"error" => "Could not find player and dealt card"})
+        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"})
     end
   end
 
+  # Atomic card play operation to prevent race conditions and enforce one-card-per-round invariant
+  defp play_card_atomically(dealt_card, player_id, current_round) do
+    # Use database transaction to ensure atomicity and prevent race conditions
+    result = Repo.transaction(fn ->
+      # First, try to lock player's cards for this round by checking existing plays
+      existing_cards = Repo.all(
+        from(dc in Copi.Cornucopia.DealtCard, 
+          where: dc.player_id == ^player_id and dc.played_in_round == ^current_round)
+      )
+
+      if length(existing_cards) > 0 do
+        {:error, :player_already_played}
+      else
+        # Now atomically update the card with played_in_round
+        from(dc in Copi.Cornucopia.DealtCard, 
+          where: dc.id == ^dealt_card.id and is_nil(dc.played_in_round))
+        |> Repo.update_all([set: [played_in_round: current_round]], returning: true)
+        |> case do
+          {1, [updated_card]} -> {:ok, updated_card}
+          {0, []} -> {:error, :already_played}
+        end
+      end
+    end)
+
+    # Unwrap transaction result to return flat tuple
+    case result do
+      {:ok, {:ok, val}} -> {:ok, val}
+      {:ok, {:error, reason}} -> {:error, reason}
+      {:error, reason} -> {:error, reason}
+      other -> other
+    end
+  end
+
   def topic(game_id) do
     "game:#{game_id}"
   end

copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex

@@ -9,28 +9,38 @@ defmodule CopiWeb.Plugs.RateLimiterPlug do
   def call(conn, _opts) do
     case IPHelper.get_ip_source(conn) do
       {:forwarded, ip} ->
+        # Determine action type based on request path and method
+        action = determine_action(conn)
+
         # Only enforce connection rate limits when we have a forwarded
         # client IP (left-most value from X-Forwarded-For). This avoids
         # rate-limiting on internal/transport addresses injected by the
         # reverse proxy or adapter.
-        case RateLimiter.check_rate(ip, :connection) do
+        case RateLimiter.check_rate(ip, action) do
           {:ok, _remaining} ->
             # Persist the chosen client IP into the session so LiveView
             # receives it on websocket connect (connect_info.session).
-            put_session(conn, "client_ip", IPHelper.ip_to_string(ip))
+            # Only write to session if it's available (stateless API requests won't have sessions)
+            if Map.has_key?(conn.private, :plug_session) do
+              put_session(conn, "client_ip", IPHelper.ip_to_string(ip))
+            else
+              conn
+            end
 
           {:error, :rate_limit_exceeded} ->
-            Logger.warning("HTTP connection rate limit exceeded for IP: #{inspect(ip)}")
+            # Anonymize IP for logging to avoid PII in logs
+            anonymized_ip = ip |> IPHelper.ip_to_string() |> :crypto.hash(:sha256) |> Base.encode16()
+            Logger.warning("HTTP #{action} rate limit exceeded for IP: #{anonymized_ip}")
             conn
             |> put_resp_content_type("text/plain")
-            |> send_resp(429, "Too many connections, try again later.")
+            |> send_resp(429, "Too many requests, try again later.")
             |> halt()
         end
 
       {:remote, _ip} ->
         # We only have a transport (remote) IP; skip connection rate limiting
         # to avoid false positives caused by proxies or adapter internals.
-        Logger.debug("Skipping connection rate limiting: only transport IP available")
+        Logger.debug("Skipping rate limiting: only transport IP available")
         conn
 
       {:none, _} ->
@@ -39,4 +49,18 @@ defmodule CopiWeb.Plugs.RateLimiterPlug do
         conn
     end
   end
+
+  # Determine the action type for rate limiting based on the request
+  defp determine_action(conn) do
+    case conn.method do
+      "PUT" ->
+        if String.ends_with?(conn.request_path, "/card") do
+          :api_action
+        else
+          :connection
+        end
+      _ ->
+        :connection
+    end
+  end
 end

copi.owasp.org/lib/copi_web/router.ex

@@ -13,6 +13,7 @@ defmodule CopiWeb.Router do
 
   pipeline :api do
     plug :accepts, ["json"]
+    plug CopiWeb.Plugs.RateLimiterPlug
   end
 
   scope "/", CopiWeb do