Fix DoS vulnerability in play_card API by adding rate limiting

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

@@ -30,7 +30,7 @@ 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, :play_card)
 
   ## Examples
       iex> Copi.RateLimiter.check_rate("127.0.0.1", :game_creation)
@@ -39,7 +39,7 @@ defmodule Copi.RateLimiter do
       iex> Copi.RateLimiter.check_rate({127, 0, 0, 1}, :connection)
       {:error, :rate_limit_exceeded}
   """
-  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, :play_card] do
     normalized_ip = normalize_ip(ip)
 
     # In production, don't rate limit localhost to prevent DoS'ing ourselves
@@ -76,12 +76,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),
+        play_card: get_env_config(:play_card_limit, 10)
       },
       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),
+        play_card: get_env_config(:play_card_window, 60)
       },
       requests: %{}
     }

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

@@ -6,31 +6,38 @@ defmodule CopiWeb.Plugs.RateLimiterPlug do
 
   def init(opts), do: opts
 
-  def call(conn, _opts) do
+  def call(conn, opts) do
+    action = Keyword.get(opts, :action, :connection)
+
     case IPHelper.get_ip_source(conn) do
       {:forwarded, ip} ->
-        # Only enforce connection rate limits when we have a forwarded
+        # Only enforce 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))
+            if action == :connection do
+              # 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))
+            else
+              conn
+            end
 
           {:error, :rate_limit_exceeded} ->
-            Logger.warning("HTTP connection rate limit exceeded for IP: #{inspect(ip)}")
+            Logger.warning("Rate limit exceeded for IP: #{inspect(ip)} on action #{action}")
+
             conn
-            |> put_resp_content_type("text/plain")
-            |> send_resp(429, "Too many connections, try again later.")
+            |> put_resp_content_type(content_type_for(action))
+            |> send_resp(429, response_body_for(action))
             |> halt()
         end
 
       {:remote, _ip} ->
-        # We only have a transport (remote) IP; skip connection rate limiting
+        # We only have a transport (remote) IP; skip 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 +46,10 @@ defmodule CopiWeb.Plugs.RateLimiterPlug do
         conn
     end
   end
+
+  defp content_type_for(:connection), do: "text/plain"
+  defp content_type_for(_action), do: "application/json"
+
+  defp response_body_for(:connection), do: "Too many connections, try again later."
+  defp response_body_for(_action), do: ~s({"error":"Rate limit exceeded. Please try again later."})
 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, action: :play_card
   end
 
   scope "/", CopiWeb do

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

@@ -30,6 +30,19 @@ defmodule Copi.RateLimiterTest do
       assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip, :game_creation)
     end
 
+    test "blocks requests over the play card limit", %{ip: ip} do
+      config = RateLimiter.get_config()
+      limit = config.limits.play_card
+
+      # Make requests up to the limit
+      for _ <- 1..limit do
+        assert {:ok, _} = RateLimiter.check_rate(ip, :play_card)
+      end
+
+      # Next request should be blocked
+      assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip, :play_card)
+    end
+
     test "blocks requests over the player creation limit", %{ip: ip} do
       config = RateLimiter.get_config()
       limit = config.limits.player_creation
@@ -135,10 +148,12 @@ defmodule Copi.RateLimiterTest do
       assert Map.has_key?(config.limits, :game_creation)
       assert Map.has_key?(config.limits, :player_creation)
       assert Map.has_key?(config.limits, :connection)
+      assert Map.has_key?(config.limits, :play_card)
 
       assert Map.has_key?(config.windows, :game_creation)
       assert Map.has_key?(config.windows, :player_creation)
       assert Map.has_key?(config.windows, :connection)
+      assert Map.has_key?(config.windows, :play_card)
     end
 
     test "has sensible default values" do
@@ -154,11 +169,17 @@ defmodule Copi.RateLimiterTest do
       # Connection limit should be the highest
       assert config.limits.connection >= config.limits.player_creation
 
+      # Play card limit should be reasonable (10 per 60s by default)
+      assert config.limits.play_card > 0
+      assert config.limits.play_card <= 30
+
       # Windows should be in reasonable ranges (seconds)
       assert config.windows.game_creation >= 60
       assert config.windows.player_creation >= 60
       # Connection window can be as low as 1 second for high-frequency limits
       assert config.windows.connection >= 1
+      # Play card window should be at least 1 second
+      assert config.windows.play_card >= 1
     end
 
     test "handles invalid environment variable gracefully" do
@@ -170,6 +191,7 @@ defmodule Copi.RateLimiterTest do
       assert is_integer(config.limits.game_creation) and config.limits.game_creation > 0
       assert is_integer(config.limits.player_creation) and config.limits.player_creation > 0
       assert is_integer(config.limits.connection) and config.limits.connection > 0
+      assert is_integer(config.limits.play_card) and config.limits.play_card > 0
     end
   end