From 66ddc851c9b8e5480365f4d73e3e519f5280b3a4 Mon Sep 17 00:00:00 2001
From: Khushal Malhotra <redmi5a3217@gmail.com>
Date: Sun, 8 Mar 2026 21:22:11 +0530
Subject: [PATCH 1/6] feat: Implement atomic operations and rate limiting

- Add atomic card play operations to prevent race conditions
- Implement comprehensive rate limiting for API endpoints
- Add rate limiting plug to API pipeline
- Add comprehensive test coverage for rate limiting
- Fix CAPEC-212 functionality misuse vulnerabilities

Security improvements: prevents DoS attacks and race conditions
---
 copi.owasp.org/lib/copi/rate_limiter.ex       |  15 +-
 .../copi_web/controllers/api_controller.ex    |  26 ++-
 .../lib/copi_web/plugs/rate_limiter_plug.ex   |  25 ++-
 copi.owasp.org/lib/copi_web/router.ex         |   1 +
 .../copi/rate_limiter_integration_test.exs    | 204 ++++++++++++++++++
 .../plugs/rate_limiter_plug_api_test.exs      | 159 ++++++++++++++
 6 files changed, 413 insertions(+), 17 deletions(-)
 create mode 100644 copi.owasp.org/test/copi/rate_limiter_integration_test.exs
 create mode 100644 copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs

diff --git a/copi.owasp.org/lib/copi/rate_limiter.ex b/copi.owasp.org/lib/copi/rate_limiter.ex
index c12834520..9c0805d51 100644
--- a/copi.owasp.org/lib/copi/rate_limiter.ex
+++ b/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,16 +31,16 @@ 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
@@ -76,12 +77,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, 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),
+        api_action: get_env_config(:api_action_window, 60)
       },
       requests: %{}
     }
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 f181b8e41..76cc6cc11 100644
--- a/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
+++ b/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
@@ -1,11 +1,11 @@
 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)
 
@@ -18,10 +18,9 @@ defmodule CopiWeb.ApiController do
           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} ->
+            # Atomic update to prevent race conditions
+            case play_card_atomically(dealt_card, 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)
                 else
@@ -29,7 +28,9 @@ defmodule CopiWeb.ApiController do
                     conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not find updated game"})
                 end
 
-                conn |> json(%{"id" => dealt_card.id})
+                conn |> json(%{"id" => updated_card.id})
+              {:error, :already_played} ->
+                conn |> put_status(:conflict) |> json(%{"error" => "Card was already played by another request"})
               {:error, _changeset} ->
                 conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not update dealt card"})
             end
@@ -42,6 +43,17 @@ defmodule CopiWeb.ApiController do
     end
   end
 
+  # Atomic card play operation to prevent race conditions
+  defp play_card_atomically(dealt_card, current_round) do
+    # Use Ecto's atomic operations to prevent race conditions
+    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
+
   def topic(game_id) do
     "game:#{game_id}"
   end
diff --git a/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex b/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
index 9d3a29f52..390330f8b 100644
--- a/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
+++ b/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
@@ -9,28 +9,31 @@ 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))
 
           {:error, :rate_limit_exceeded} ->
-            Logger.warning("HTTP connection rate limit exceeded for IP: #{inspect(ip)}")
+            Logger.warning("HTTP #{action} rate limit exceeded for IP: #{inspect(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 +42,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.contains?(conn.request_path, "/card") do
+          :api_action
+        else
+          :connection
+        end
+      _ ->
+        :connection
+    end
+  end
 end
diff --git a/copi.owasp.org/lib/copi_web/router.ex b/copi.owasp.org/lib/copi_web/router.ex
index 8ff3ffaa0..f65ba8636 100644
--- a/copi.owasp.org/lib/copi_web/router.ex
+++ b/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
diff --git a/copi.owasp.org/test/copi/rate_limiter_integration_test.exs b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
new file mode 100644
index 000000000..476c7a13d
--- /dev/null
+++ b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
@@ -0,0 +1,204 @@
+defmodule Copi.RateLimiterIntegrationTest do
+  use ExUnit.Case, async: false
+  use Plug.Test
+  alias Copi.RateLimiter
+  alias CopiWeb.Plugs.RateLimiterPlug
+
+  setup do
+    # Clear all rate limiting data before each test
+    RateLimiter.clear_ip({192, 168, 1, 100})
+    RateLimiter.clear_ip({10, 0, 0, 1})
+    RateLimiter.clear_ip({127, 0, 0, 1})
+    :ok
+  end
+
+  describe "Attack Scenario Tests" do
+    test "prevents 100 concurrent requests like in the vulnerability report" do
+      ip = "192.168.1.100"
+      
+      # Simulate the attack scenario from the vulnerability report
+      tasks = for i <- 1..100 do
+        Task.async(fn ->
+          conn = conn(:put, "/api/games/01KK4ANZFV6XMR14VX7R1X6T55/players/01KK4APC64N6Z5B346S960XTQJ/card")
+                 |> put_req_header("x-forwarded-for", ip)
+                 |> put_req_header("content-type", "application/json")
+                 |> RateLimiterPlug.call([])
+          
+          {i, conn.status}
+        end)
+      end
+      
+      results = Task.await_many(tasks, 10_000)
+      
+      # Count different response types
+      rate_limited = Enum.count(results, fn {_, status} -> status == 429 end)
+      allowed = Enum.count(results, fn {_, status} -> status != 429 end)
+      
+      # With rate limiting of 10 requests per minute, most should be blocked
+      config = RateLimiter.get_config()
+      limit = config.limits.api_action
+      
+      assert rate_limited >= 90  # At least 90 should be rate limited
+      assert allowed <= limit     # Only the limit should be allowed
+      
+      IO.puts("\nAttack Simulation Results:")
+      IO.puts("Rate limited (429): #{rate_limited}")
+      IO.puts("Allowed: #{allowed}")
+      IO.puts("Total: #{length(results)}")
+    end
+
+    test "rate limiting window resets after time passes" do
+      ip = "10.0.0.1"
+      config = RateLimiter.get_config()
+      limit = config.limits.api_action
+      window = config.windows.api_action
+      
+      # Exhaust the rate limit
+      for _ <- 1..limit do
+        RateLimiter.check_rate(ip, :api_action)
+      end
+      
+      # Should be rate limited now
+      assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip, :api_action)
+      
+      # Wait for window to pass (simulate time passing)
+      # In real tests, you'd need to manipulate time or use a shorter window
+      # For this test, we'll verify the rate limiter is working correctly
+      
+      # Try again - should still be rate limited
+      assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip, :api_action)
+    end
+
+    test "different IPs have independent rate limits" do
+      ip1 = "192.168.1.100"
+      ip2 = "192.168.1.101"
+      config = RateLimiter.get_config()
+      limit = config.limits.api_action
+      
+      # Exhaust limit for IP1
+      for _ <- 1..limit do
+        assert {:ok, _} = RateLimiter.check_rate(ip1, :api_action)
+      end
+      
+      # IP1 should be rate limited
+      assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip1, :api_action)
+      
+      # IP2 should still be able to make requests
+      assert {:ok, _} = RateLimiter.check_rate(ip2, :api_action)
+    end
+
+    test "rate limiting works across different action types" do
+      ip = "10.0.0.1"
+      config = RateLimiter.get_config()
+      
+      # Test that different action types have separate limits
+      api_limit = config.limits.api_action
+      connection_limit = config.limits.connection
+      
+      # Exhaust API action limit
+      for _ <- 1..api_limit do
+        assert {:ok, _} = RateLimiter.check_rate(ip, :api_action)
+      end
+      
+      # API actions should be rate limited
+      assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip, :api_action)
+      
+      # But connection limit should still work independently
+      assert {:ok, _} = RateLimiter.check_rate(ip, :connection)
+    end
+
+    test "rate limiter handles high concurrency safely" do
+      ip = "192.168.1.200"
+      
+      # Test with many concurrent requests to ensure no race conditions in rate limiter
+      tasks = for _ <- 1..50 do
+        Task.async(fn ->
+          RateLimiter.check_rate(ip, :api_action)
+        end)
+      end
+      
+      results = Task.await_many(tasks, 5000)
+      
+      # Count successes vs failures
+      successes = Enum.count(results, fn result -> match?({:ok, _}, result) end)
+      failures = Enum.count(results, fn result -> match?({:error, _}, result) end)
+      
+      config = RateLimiter.get_config()
+      limit = config.limits.api_action
+      
+      # Should not exceed the limit
+      assert successes <= limit
+      assert failures >= 50 - limit
+      
+      # Ensure no crashes or unexpected results
+      assert length(results) == 50
+    end
+  end
+
+  describe "Rate Limiter Configuration" do
+    test "respects environment variable configuration" do
+      # Test that the rate limiter reads from environment variables
+      config = RateLimiter.get_config()
+      
+      # Verify default values are set
+      assert config.limits.api_action == 10
+      assert config.windows.api_action == 60
+      assert config.limits.connection == 133
+      assert config.windows.connection == 1
+    end
+
+    test "handles localhost differently in production" do
+      # This test would need to be run in production mode to fully test
+      # For now, we verify the logic exists
+      ip = {127, 0, 0, 1}
+      
+      # In test mode, localhost should be rate limited
+      result = RateLimiter.check_rate(ip, :api_action)
+      assert match?({:ok, _}, result)
+    end
+  end
+
+  describe "Error Handling" do
+    test "handles invalid IP addresses gracefully" do
+      # Test with various invalid IP formats
+      invalid_ips = ["invalid", "", "not-an-ip", "999.999.999.999"]
+      
+      for ip <- invalid_ips do
+        # Should not crash, should handle gracefully
+        result = RateLimiter.check_rate(ip, :api_action)
+        case result do
+          {:ok, _} -> :ok
+          {:error, :rate_limit_exceeded} -> :ok
+        end
+      end
+    end
+
+    test "rate limiter is thread-safe" do
+      ip = "10.0.0.100"
+      
+      # Test concurrent access to the same IP
+      tasks = for _ <- 1..20 do
+        Task.async(fn ->
+          # Mix of different operations
+          case :rand.uniform(3) do
+            1 -> RateLimiter.check_rate(ip, :api_action)
+            2 -> RateLimiter.check_rate(ip, :connection)
+            3 -> RateLimiter.get_config()
+          end
+        end)
+      end
+      
+      results = Task.await_many(tasks, 5000)
+      
+      # All operations should complete successfully
+      assert length(results) == 20
+      Enum.each(results, fn result ->
+        case result do
+          {:ok, _} -> :ok
+          {:error, :rate_limit_exceeded} -> :ok
+          %{} -> :ok
+        end
+      end)
+    end
+  end
+end
diff --git a/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs b/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
new file mode 100644
index 000000000..536a72218
--- /dev/null
+++ b/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
@@ -0,0 +1,159 @@
+defmodule CopiWeb.Plugs.RateLimiterPlugTest do
+  use ExUnit.Case, async: false
+  use Plug.Test
+
+  alias CopiWeb.Plugs.RateLimiterPlug
+  alias Copi.RateLimiter
+
+  setup do
+    RateLimiter.clear_ip({10, 0, 0, 1})
+    RateLimiter.clear_ip({192, 168, 1, 100})
+    RateLimiter.clear_ip({127, 0, 0, 1})
+    :ok
+  end
+
+  describe "API endpoint rate limiting" do
+    test "allows API requests under the rate limit" do
+      conn =
+        conn(:put, "/api/games/test/players/test/card")
+        |> put_req_header("x-forwarded-for", "10.0.0.1")
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+
+      assert conn.status != 429
+      assert get_session(conn, "client_ip") == "10.0.0.1"
+    end
+
+    test "applies api_action rate limit to PUT /card requests" do
+      ip = "192.168.1.100"
+      config = RateLimiter.get_config()
+      limit = config.limits.api_action
+
+      # Exhaust the API action limit first
+      for _ <- 1..limit do
+        RateLimiter.check_rate(ip, :api_action)
+      end
+
+      conn =
+        conn(:put, "/api/games/test/players/test/card")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+
+      assert conn.status == 429
+      assert conn.resp_body == "Too many requests, try again later."
+      assert conn.halted
+    end
+
+    test "applies connection rate limit to non-card API requests" do
+      ip = "192.168.1.100"
+      config = RateLimiter.get_config()
+      limit = config.limits.connection
+
+      # Exhaust the connection limit first
+      for _ <- 1..limit do
+        RateLimiter.check_rate(ip, :connection)
+      end
+
+      conn =
+        conn(:get, "/api/some-other-endpoint")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+
+      assert conn.status == 429
+      assert conn.halted
+    end
+
+    test "uses api_action for PUT requests containing /card" do
+      conn =
+        conn(:put, "/api/games/123/players/456/card")
+        |> RateLimiterPlug.call([])
+
+      # The plug should determine the action correctly
+      # We can't easily test the internal action determination, 
+      # but we can verify it doesn't crash
+      refute conn.halted
+    end
+
+    test "uses connection for PUT requests not containing /card" do
+      conn =
+        conn(:put, "/api/games/123/players/456/some-other-action")
+        |> RateLimiterPlug.call([])
+
+      refute conn.halted
+    end
+
+    test "uses connection for GET requests" do
+      conn =
+        conn(:get, "/api/games/123/players/456/card")
+        |> RateLimiterPlug.call([])
+
+      refute conn.halted
+    end
+
+    test "uses connection for POST requests" do
+      conn =
+        conn(:post, "/api/games/123/players/456/card")
+        |> RateLimiterPlug.call([])
+
+      refute conn.halted
+    end
+  end
+
+  describe "Rate limiting behavior" do
+    test "skips rate limiting when only remote IP is available" do
+      ip = {127, 0, 0, 1}
+      config = RateLimiter.get_config()
+      limit = config.limits.api_action
+
+      # Exhaust the limit on RateLimiter manually
+      for _ <- 1..limit do
+        RateLimiter.check_rate(ip, :api_action)
+      end
+
+      # The plug should still let it through because it skips non-forwarded IPs
+      conn =
+        conn(:put, "/api/games/test/players/test/card")
+        |> Map.put(:remote_ip, ip)
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+
+      assert conn.status != 429
+      refute conn.halted
+    end
+
+    test "skips rate limiting when no IP info is available" do
+      # No headers, no remote_ip
+      conn =
+        conn(:put, "/api/games/test/players/test/card")
+        |> RateLimiterPlug.call([])
+
+      assert conn.status != 429
+      refute conn.halted
+    end
+  end
+
+  describe "Error messages" do
+    test "returns appropriate error message for API rate limiting" do
+      ip = "192.168.1.100"
+      config = RateLimiter.get_config()
+      limit = config.limits.api_action
+
+      # Exhaust the API action limit
+      for _ <- 1..limit do
+        RateLimiter.check_rate(ip, :api_action)
+      end
+
+      conn =
+        conn(:put, "/api/games/test/players/test/card")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+
+      assert conn.status == 429
+      assert conn.resp_body == "Too many requests, try again later."
+      assert get_resp_header(conn, "content-type") == ["text/plain; charset=utf-8"]
+    end
+  end
+end

From 5ba67d036138fe9603c95610cd2f35ab7eaf661e Mon Sep 17 00:00:00 2001
From: Khushal Malhotra <redmi5a3217@gmail.com>
Date: Sun, 8 Mar 2026 21:50:41 +0530
Subject: [PATCH 2/6] fix: Address code review feedback and improve test
 coverage

- Fix atomic operations to enforce one-card-per-round invariant in DB transaction
- Fix error handling to propagate failures to HTTP responses instead of always returning 200
- Fix rate limiter plug to handle stateless API requests without sessions
- Add synchronous clear_ip_sync function for reliable test setup
- Fix API rate limiter tests to include x-forwarded-for headers and test actual rate limiting
- Fix integration tests to wait for rate limit windows to expire and use synchronous clears
- Improve test reliability by handling GenServer already_started cases

Security improvements: ensures atomic invariants and proper error propagation
---
 copi.owasp.org/lib/copi/rate_limiter.ex       | 17 ++++
 .../copi_web/controllers/api_controller.ex    | 59 +++++++------
 .../lib/copi_web/plugs/rate_limiter_plug.ex   |  7 +-
 .../copi/rate_limiter_integration_test.exs    | 23 +++--
 .../plugs/rate_limiter_plug_api_test.exs      | 86 +++++++++++++++++--
 5 files changed, 147 insertions(+), 45 deletions(-)

diff --git a/copi.owasp.org/lib/copi/rate_limiter.ex b/copi.owasp.org/lib/copi/rate_limiter.ex
index 9c0805d51..90ced63c0 100644
--- a/copi.owasp.org/lib/copi/rate_limiter.ex
+++ b/copi.owasp.org/lib/copi/rate_limiter.ex
@@ -59,6 +59,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.
   """
@@ -130,6 +137,16 @@ 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)
+    
+    new_state = %{state | requests: new_requests}
+    {:reply, :ok, new_state}
+  end
+
   @impl true
   def handle_cast({:clear_ip, ip}, state) do
     new_requests =
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 76cc6cc11..69cb164f5 100644
--- a/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
+++ b/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
@@ -12,28 +12,22 @@ defmodule CopiWeb.ApiController do
       if player && 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 ->
-            # Atomic update to prevent race conditions
-            case play_card_atomically(dealt_card, 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)
-                else
-                  {:error, _reason} ->
-                    conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not find updated game"})
-                end
-
-                conn |> json(%{"id" => updated_card.id})
-              {:error, :already_played} ->
-                conn |> put_status(:conflict) |> json(%{"error" => "Card was already played by another request"})
-              {:error, _changeset} ->
-                conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not update dealt card"})
+        # 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 dealt card"})
         end
       else
         conn |> put_status(:not_found) |> json(%{"error" => "Could not find player and dealt card"})
@@ -43,13 +37,26 @@ defmodule CopiWeb.ApiController do
     end
   end
 
-  # Atomic card play operation to prevent race conditions
-  defp play_card_atomically(dealt_card, current_round) do
-    # Use Ecto's atomic operations to prevent race conditions
-    from(dc in Copi.Cornucopia.DealtCard, where: dc.id == ^dealt_card.id and is_nil(dc.played_in_round))
+  # 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 Ecto's atomic operations to prevent race conditions and enforce invariants
+    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}
+      {1, [updated_card]} -> 
+        # Check if this player already played a card in this round
+        case Repo.get_by(Copi.Cornucopia.DealtCard, 
+               [player_id: player_id, played_in_round: current_round], 
+               limit: 2) do
+          nil -> {:ok, updated_card}
+          [%{id: id}] when id == updated_card.id -> {:ok, updated_card}
+          _ -> 
+            # Player already played a different card this round, rollback
+            Repo.delete_all(from(dc in Copi.Cornucopia.DealtCard, 
+                              where: dc.id == ^updated_card.id))
+            {:error, :player_already_played}
+        end
       {0, []} -> {:error, :already_played}
     end
   end
diff --git a/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex b/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
index 390330f8b..a98a7a49d 100644
--- a/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
+++ b/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
@@ -20,7 +20,12 @@ defmodule CopiWeb.Plugs.RateLimiterPlug 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 #{action} rate limit exceeded for IP: #{inspect(ip)}")
diff --git a/copi.owasp.org/test/copi/rate_limiter_integration_test.exs b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
index 476c7a13d..048827d34 100644
--- a/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
+++ b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
@@ -5,10 +5,16 @@ defmodule Copi.RateLimiterIntegrationTest do
   alias CopiWeb.Plugs.RateLimiterPlug
 
   setup do
-    # Clear all rate limiting data before each test
-    RateLimiter.clear_ip({192, 168, 1, 100})
-    RateLimiter.clear_ip({10, 0, 0, 1})
-    RateLimiter.clear_ip({127, 0, 0, 1})
+    # Start the RateLimiter GenServer for testing if not already started
+    case RateLimiter.start_link([]) do
+      {:ok, pid} -> {:ok, pid}
+      {:error, {:already_started, pid}} -> {:ok, pid}
+    end
+    
+    # Clear all rate limiting data before each test (synchronous)
+    RateLimiter.clear_ip_sync({192, 168, 1, 100})
+    RateLimiter.clear_ip_sync({10, 0, 0, 1})
+    RateLimiter.clear_ip_sync({127, 0, 0, 1})
     :ok
   end
 
@@ -61,12 +67,11 @@ defmodule Copi.RateLimiterIntegrationTest do
       # Should be rate limited now
       assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip, :api_action)
       
-      # Wait for window to pass (simulate time passing)
-      # In real tests, you'd need to manipulate time or use a shorter window
-      # For this test, we'll verify the rate limiter is working correctly
+      # Wait for window to pass plus a small buffer
+      :timer.sleep(window + 100)  # window is in milliseconds
       
-      # Try again - should still be rate limited
-      assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip, :api_action)
+      # Should be allowed again after window expires
+      assert {:ok, _remaining} = RateLimiter.check_rate(ip, :api_action)
     end
 
     test "different IPs have independent rate limits" do
diff --git a/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs b/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
index 536a72218..c70c187f5 100644
--- a/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
+++ b/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
@@ -6,9 +6,15 @@ defmodule CopiWeb.Plugs.RateLimiterPlugTest do
   alias Copi.RateLimiter
 
   setup do
-    RateLimiter.clear_ip({10, 0, 0, 1})
-    RateLimiter.clear_ip({192, 168, 1, 100})
-    RateLimiter.clear_ip({127, 0, 0, 1})
+    # Start the RateLimiter GenServer for testing if not already started
+    case RateLimiter.start_link([]) do
+      {:ok, pid} -> {:ok, pid}
+      {:error, {:already_started, pid}} -> {:ok, pid}
+    end
+    
+    RateLimiter.clear_ip_sync({10, 0, 0, 1})
+    RateLimiter.clear_ip_sync({192, 168, 1, 100})
+    RateLimiter.clear_ip_sync({127, 0, 0, 1})
     :ok
   end
 
@@ -66,38 +72,100 @@ defmodule CopiWeb.Plugs.RateLimiterPlugTest do
     end
 
     test "uses api_action for PUT requests containing /card" do
+      ip = "192.168.1.100"
+      
+      # First request should pass
       conn =
         conn(:put, "/api/games/123/players/456/card")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
         |> RateLimiterPlug.call([])
 
-      # The plug should determine the action correctly
-      # We can't easily test the internal action determination, 
-      # but we can verify it doesn't crash
       refute conn.halted
+      
+      # Exhaust the api_action rate limit (10 requests per minute)
+      for _ <- 1..10 do
+        conn(:put, "/api/games/123/players/456/card")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+      end
+      
+      # Next request should be rate limited
+      conn =
+        conn(:put, "/api/games/123/players/456/card")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+
+      assert conn.status == 429
+      assert conn.halted
     end
 
     test "uses connection for PUT requests not containing /card" do
+      ip = "192.168.1.100"
+      
+      # Exhaust the connection rate limit (133 requests per second)
+      for _ <- 1..135 do
+        conn(:put, "/api/games/123/players/456/some-other-action")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+      end
+      
+      # Next request should be rate limited
       conn =
         conn(:put, "/api/games/123/players/456/some-other-action")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
         |> RateLimiterPlug.call([])
 
-      refute conn.halted
+      assert conn.status == 429
+      assert conn.halted
     end
 
     test "uses connection for GET requests" do
+      ip = "192.168.1.100"
+      
+      # Exhaust the connection rate limit (133 requests per second)
+      for _ <- 1..135 do
+        conn(:get, "/api/games/123/players/456/card")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+      end
+      
+      # Next request should be rate limited
       conn =
         conn(:get, "/api/games/123/players/456/card")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
         |> RateLimiterPlug.call([])
 
-      refute conn.halted
+      assert conn.status == 429
+      assert conn.halted
     end
 
     test "uses connection for POST requests" do
+      ip = "192.168.1.100"
+      
+      # Exhaust the connection rate limit (133 requests per second)
+      for _ <- 1..135 do
+        conn(:post, "/api/games/123/players/456/card")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
+        |> RateLimiterPlug.call([])
+      end
+      
+      # Next request should be rate limited
       conn =
         conn(:post, "/api/games/123/players/456/card")
+        |> put_req_header("x-forwarded-for", ip)
+        |> init_test_session(%{})
         |> RateLimiterPlug.call([])
 
-      refute conn.halted
+      assert conn.status == 429
+      assert conn.halted
     end
   end
 

From 0bee443e74f61e6bab6467486d6f606a03df21b9 Mon Sep 17 00:00:00 2001
From: Khushal Malhotra <redmi5a3217@gmail.com>
Date: Sun, 8 Mar 2026 22:12:20 +0530
Subject: [PATCH 3/6] fix: Implement database transactions for atomic
 operations

- Use Repo.transaction to enforce one-card-per-round invariant atomically
- Fix rate limiter handle_call to preserve map structure properly
- Fix rate limiter plug to use String.ends_with? for stricter path matching
- Fix API rate limiter test loop range (1..9 instead of 1..10)
- Fix integration test to use CopiWeb.Router.call for proper endpoint testing
---
 copi.owasp.org/lib/copi/rate_limiter.ex       |  1 +
 .../copi_web/controllers/api_controller.ex    | 39 ++++++++++---------
 .../lib/copi_web/plugs/rate_limiter_plug.ex   |  2 +-
 .../copi/rate_limiter_integration_test.exs    | 11 ++++--
 .../plugs/rate_limiter_plug_api_test.exs      |  2 +-
 5 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/copi.owasp.org/lib/copi/rate_limiter.ex b/copi.owasp.org/lib/copi/rate_limiter.ex
index 90ced63c0..952390432 100644
--- a/copi.owasp.org/lib/copi/rate_limiter.ex
+++ b/copi.owasp.org/lib/copi/rate_limiter.ex
@@ -142,6 +142,7 @@ defmodule Copi.RateLimiter 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}
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 69cb164f5..d691a3aa4 100644
--- a/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
+++ b/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
@@ -39,26 +39,27 @@ defmodule CopiWeb.ApiController do
 
   # 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 Ecto's atomic operations to prevent race conditions and enforce invariants
-    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]} -> 
-        # Check if this player already played a card in this round
-        case Repo.get_by(Copi.Cornucopia.DealtCard, 
-               [player_id: player_id, played_in_round: current_round], 
-               limit: 2) do
-          nil -> {:ok, updated_card}
-          [%{id: id}] when id == updated_card.id -> {:ok, updated_card}
-          _ -> 
-            # Player already played a different card this round, rollback
-            Repo.delete_all(from(dc in Copi.Cornucopia.DealtCard, 
-                              where: dc.id == ^updated_card.id))
-            {:error, :player_already_played}
+    # Use database transaction to ensure atomicity and prevent race conditions
+    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
-      {0, []} -> {:error, :already_played}
-    end
+      end
+    end)
   end
 
   def topic(game_id) do
diff --git a/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex b/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
index a98a7a49d..d7eda64bb 100644
--- a/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
+++ b/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
@@ -52,7 +52,7 @@ defmodule CopiWeb.Plugs.RateLimiterPlug do
   defp determine_action(conn) do
     case conn.method do
       "PUT" ->
-        if String.contains?(conn.request_path, "/card") do
+        if String.ends_with?(conn.request_path, "/card") do
           :api_action
         else
           :connection
diff --git a/copi.owasp.org/test/copi/rate_limiter_integration_test.exs b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
index 048827d34..049206854 100644
--- a/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
+++ b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
@@ -22,15 +22,20 @@ defmodule Copi.RateLimiterIntegrationTest do
     test "prevents 100 concurrent requests like in the vulnerability report" do
       ip = "192.168.1.100"
       
-      # Simulate the attack scenario from the vulnerability report
+      # Simulate attack scenario from vulnerability report
       tasks = for i <- 1..100 do
         Task.async(fn ->
           conn = conn(:put, "/api/games/01KK4ANZFV6XMR14VX7R1X6T55/players/01KK4APC64N6Z5B346S960XTQJ/card")
                  |> put_req_header("x-forwarded-for", ip)
                  |> put_req_header("content-type", "application/json")
                  |> RateLimiterPlug.call([])
-          
-          {i, conn.status}
+                 |> case do
+                   {:ok, modified_conn} ->
+                     # Send through router to get actual response
+                     CopiWeb.Router.call(modified_conn, [])
+                   {:error, _} ->
+                     {i, modified_conn.status}
+                 end
         end)
       end
       
diff --git a/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs b/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
index c70c187f5..9245054b9 100644
--- a/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
+++ b/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
@@ -84,7 +84,7 @@ defmodule CopiWeb.Plugs.RateLimiterPlugTest do
       refute conn.halted
       
       # Exhaust the api_action rate limit (10 requests per minute)
-      for _ <- 1..10 do
+      for _ <- 1..9 do
         conn(:put, "/api/games/123/players/456/card")
         |> put_req_header("x-forwarded-for", ip)
         |> init_test_session(%{})

From cd819f9ef06b0a94e1ec574ace0ebfce6b942b3d Mon Sep 17 00:00:00 2001
From: Khushal Malhotra <redmi5a3217@gmail.com>
Date: Sun, 8 Mar 2026 22:26:52 +0530
Subject: [PATCH 4/6] fix: Complete code review feedback implementation

- Unwrap transaction result to return flat tuple structure
- Anonymize IP addresses in rate limiter logs using SHA-256 hash
- Fix integration test Task.async block to handle %Plug.Conn{} properly
- Fix integration test assertions to count specific status codes (200/409/429)
- Fix time window test to convert seconds to milliseconds
- Fix API rate limiter tests to use dynamic limits from config
---
 .../copi_web/controllers/api_controller.ex    | 10 ++++++-
 .../lib/copi_web/plugs/rate_limiter_plug.ex   |  4 ++-
 .../copi/rate_limiter_integration_test.exs    | 15 ++++++----
 .../plugs/rate_limiter_plug_api_test.exs      | 28 +++++++++++++------
 4 files changed, 41 insertions(+), 16 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 d691a3aa4..d3ca6e643 100644
--- a/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
+++ b/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
@@ -40,7 +40,7 @@ defmodule CopiWeb.ApiController do
   # 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
-    Repo.transaction(fn ->
+    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, 
@@ -60,6 +60,14 @@ defmodule CopiWeb.ApiController do
         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
diff --git a/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex b/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
index d7eda64bb..eec898184 100644
--- a/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
+++ b/copi.owasp.org/lib/copi_web/plugs/rate_limiter_plug.ex
@@ -28,7 +28,9 @@ defmodule CopiWeb.Plugs.RateLimiterPlug do
             end
 
           {:error, :rate_limit_exceeded} ->
-            Logger.warning("HTTP #{action} 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 requests, try again later.")
diff --git a/copi.owasp.org/test/copi/rate_limiter_integration_test.exs b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
index 049206854..acac390d7 100644
--- a/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
+++ b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
@@ -30,9 +30,10 @@ defmodule Copi.RateLimiterIntegrationTest do
                  |> put_req_header("content-type", "application/json")
                  |> RateLimiterPlug.call([])
                  |> case do
-                   {:ok, modified_conn} ->
+                   %Plug.Conn{} = modified_conn ->
                      # Send through router to get actual response
-                     CopiWeb.Router.call(modified_conn, [])
+                     final_conn = CopiWeb.Router.call(modified_conn, [])
+                     {i, final_conn.status}
                    {:error, _} ->
                      {i, modified_conn.status}
                  end
@@ -43,18 +44,19 @@ defmodule Copi.RateLimiterIntegrationTest do
       
       # Count different response types
       rate_limited = Enum.count(results, fn {_, status} -> status == 429 end)
-      allowed = Enum.count(results, fn {_, status} -> status != 429 end)
+      success_200 = Enum.count(results, fn {_, status} -> status == 200 end)
+      conflict_409 = Enum.count(results, fn {_, status} -> status == 409 end)
       
       # With rate limiting of 10 requests per minute, most should be blocked
       config = RateLimiter.get_config()
       limit = config.limits.api_action
       
       assert rate_limited >= 90  # At least 90 should be rate limited
-      assert allowed <= limit     # Only the limit should be allowed
+      assert success_200 + conflict_409 <= limit     # Only limit should be allowed
       
       IO.puts("\nAttack Simulation Results:")
       IO.puts("Rate limited (429): #{rate_limited}")
-      IO.puts("Allowed: #{allowed}")
+      IO.puts("Allowed: #{success_200 + conflict_409}")
       IO.puts("Total: #{length(results)}")
     end
 
@@ -73,7 +75,8 @@ defmodule Copi.RateLimiterIntegrationTest do
       assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip, :api_action)
       
       # Wait for window to pass plus a small buffer
-      :timer.sleep(window + 100)  # window is in milliseconds
+      ms_window = window * 1000  # Convert seconds to milliseconds
+      :timer.sleep(ms_window + 100)
       
       # Should be allowed again after window expires
       assert {:ok, _remaining} = RateLimiter.check_rate(ip, :api_action)
diff --git a/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs b/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
index 9245054b9..53429f779 100644
--- a/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
+++ b/copi.owasp.org/test/copi_web/plugs/rate_limiter_plug_api_test.exs
@@ -83,8 +83,11 @@ defmodule CopiWeb.Plugs.RateLimiterPlugTest do
 
       refute conn.halted
       
-      # Exhaust the api_action rate limit (10 requests per minute)
-      for _ <- 1..9 do
+      # Exhaust the api_action rate limit dynamically
+      config = RateLimiter.get_config()
+      api_limit = config.limits.api_action
+      
+      for _ <- 1..(api_limit - 1) do
         conn(:put, "/api/games/123/players/456/card")
         |> put_req_header("x-forwarded-for", ip)
         |> init_test_session(%{})
@@ -105,8 +108,11 @@ defmodule CopiWeb.Plugs.RateLimiterPlugTest do
     test "uses connection for PUT requests not containing /card" do
       ip = "192.168.1.100"
       
-      # Exhaust the connection rate limit (133 requests per second)
-      for _ <- 1..135 do
+      # Exhaust the connection rate limit dynamically
+      config = RateLimiter.get_config()
+      conn_limit = config.limits.connection
+      
+      for _ <- 1..(conn_limit + 2) do
         conn(:put, "/api/games/123/players/456/some-other-action")
         |> put_req_header("x-forwarded-for", ip)
         |> init_test_session(%{})
@@ -127,8 +133,11 @@ defmodule CopiWeb.Plugs.RateLimiterPlugTest do
     test "uses connection for GET requests" do
       ip = "192.168.1.100"
       
-      # Exhaust the connection rate limit (133 requests per second)
-      for _ <- 1..135 do
+      # Exhaust the connection rate limit dynamically
+      config = RateLimiter.get_config()
+      conn_limit = config.limits.connection
+      
+      for _ <- 1..(conn_limit + 2) do
         conn(:get, "/api/games/123/players/456/card")
         |> put_req_header("x-forwarded-for", ip)
         |> init_test_session(%{})
@@ -149,8 +158,11 @@ defmodule CopiWeb.Plugs.RateLimiterPlugTest do
     test "uses connection for POST requests" do
       ip = "192.168.1.100"
       
-      # Exhaust the connection rate limit (133 requests per second)
-      for _ <- 1..135 do
+      # Exhaust the connection rate limit dynamically
+      config = RateLimiter.get_config()
+      conn_limit = config.limits.connection
+      
+      for _ <- 1..(conn_limit + 2) do
         conn(:post, "/api/games/123/players/456/card")
         |> put_req_header("x-forwarded-for", ip)
         |> init_test_session(%{})

From e8dd7d43e43a20709dad320df9f5b8cdafd6f569 Mon Sep 17 00:00:00 2001
From: Khushal Malhotra <redmi5a3217@gmail.com>
Date: Sun, 8 Mar 2026 22:58:08 +0530
Subject: [PATCH 5/6] fix: Address critical security vulnerabilities and test
 issues

- Fix play_card to verify player exists before accessing dealt_cards
- Remove localhost rate limiter bypass that could be spoofed via X-Forwarded-For
- Only bypass rate limiting when actual connection IP is loopback
- Fix integration test to use proper router calls instead of direct plug calls
- Fix integration test to use short test windows instead of production durations
- Add proper test config restoration to prevent test interference
---
 copi.owasp.org/lib/copi/rate_limiter.ex       |  3 +-
 .../copi_web/controllers/api_controller.ex    | 45 ++++++++++---------
 .../copi/rate_limiter_integration_test.exs    | 44 ++++++++++++------
 3 files changed, 57 insertions(+), 35 deletions(-)

diff --git a/copi.owasp.org/lib/copi/rate_limiter.ex b/copi.owasp.org/lib/copi/rate_limiter.ex
index 952390432..8faa7fc98 100644
--- a/copi.owasp.org/lib/copi/rate_limiter.ex
+++ b/copi.owasp.org/lib/copi/rate_limiter.ex
@@ -44,8 +44,9 @@ defmodule Copi.RateLimiter 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})
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 d3ca6e643..607b238c0 100644
--- a/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
+++ b/copi.owasp.org/lib/copi_web/controllers/api_controller.ex
@@ -7,30 +7,35 @@ defmodule CopiWeb.ApiController do
   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 do
+        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 dealt_card do
+          current_round = game.rounds_played + 1
 
-        # 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 dealt card"})
+          # 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"})
diff --git a/copi.owasp.org/test/copi/rate_limiter_integration_test.exs b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
index acac390d7..164f3c4bb 100644
--- a/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
+++ b/copi.owasp.org/test/copi/rate_limiter_integration_test.exs
@@ -25,18 +25,15 @@ defmodule Copi.RateLimiterIntegrationTest do
       # Simulate attack scenario from vulnerability report
       tasks = for i <- 1..100 do
         Task.async(fn ->
-          conn = conn(:put, "/api/games/01KK4ANZFV6XMR14VX7R1X6T55/players/01KK4APC64N6Z5B346S960XTQJ/card")
+          # Create a realistic request body
+          body = Jason.encode!(%{"dealt_card_id" => "12345"})
+          
+          conn = conn(:put, "/api/games/01KK4ANZFV6XMR14VX7R1X6T55/players/01KK4APC64N6Z5B346S960XTQJ/card", body)
                  |> put_req_header("x-forwarded-for", ip)
                  |> put_req_header("content-type", "application/json")
-                 |> RateLimiterPlug.call([])
-                 |> case do
-                   %Plug.Conn{} = modified_conn ->
-                     # Send through router to get actual response
-                     final_conn = CopiWeb.Router.call(modified_conn, [])
-                     {i, final_conn.status}
-                   {:error, _} ->
-                     {i, modified_conn.status}
-                 end
+                 |> CopiWeb.Router.call([])
+          
+          {i, conn.status}
         end)
       end
       
@@ -62,9 +59,23 @@ defmodule Copi.RateLimiterIntegrationTest do
 
     test "rate limiting window resets after time passes" do
       ip = "10.0.0.1"
-      config = RateLimiter.get_config()
-      limit = config.limits.api_action
-      window = config.windows.api_action
+      original_config = RateLimiter.get_config()
+      
+      # Use a short test window (1 second instead of 60)
+      test_window = 1
+      test_config = %{
+        original_config | 
+        windows: %{original_config.windows | api_action: test_window}
+      }
+      
+      # Temporarily update config for test
+      Application.put_env(:copi, :rate_limiter, test_config)
+      
+      # Restart rate limiter with new config
+      RateLimiter.stop()
+      RateLimiter.start_link([])
+      
+      limit = test_config.limits.api_action
       
       # Exhaust the rate limit
       for _ <- 1..limit do
@@ -75,11 +86,16 @@ defmodule Copi.RateLimiterIntegrationTest do
       assert {:error, :rate_limit_exceeded} = RateLimiter.check_rate(ip, :api_action)
       
       # Wait for window to pass plus a small buffer
-      ms_window = window * 1000  # Convert seconds to milliseconds
+      ms_window = test_window * 1000  # Convert seconds to milliseconds
       :timer.sleep(ms_window + 100)
       
       # Should be allowed again after window expires
       assert {:ok, _remaining} = RateLimiter.check_rate(ip, :api_action)
+      
+      # Restore original config
+      Application.put_env(:copi, :rate_limiter, original_config)
+      RateLimiter.stop()
+      RateLimiter.start_link([])
     end
 
     test "different IPs have independent rate limits" do

From 58cac8ceca7c3e2a85f24b97674d425a37845510 Mon Sep 17 00:00:00 2001
From: Khushal Malhotra <redmi5a3217@gmail.com>
Date: Fri, 13 Mar 2026 19:29:11 +0530
Subject: [PATCH 6/6] Update rate limiter configuration: set api_action limits
 and windows to 133

---
 copi.owasp.org/lib/copi/rate_limiter.ex | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/copi.owasp.org/lib/copi/rate_limiter.ex b/copi.owasp.org/lib/copi/rate_limiter.ex
index 8faa7fc98..074e725ad 100644
--- a/copi.owasp.org/lib/copi/rate_limiter.ex
+++ b/copi.owasp.org/lib/copi/rate_limiter.ex
@@ -86,13 +86,13 @@ defmodule Copi.RateLimiter do
         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),
-        api_action: get_env_config(:api_action_limit, 10)
+        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),
-        api_action: get_env_config(:api_action_window, 60)
+        api_action: get_env_config(:api_action_window, 133)
       },
       requests: %{}
     }