diff --git a/copi.owasp.org/lib/copi/rate_limiter.ex b/copi.owasp.org/lib/copi/rate_limiter.ex index c12834520..964bd695d 100644 --- a/copi.owasp.org/lib/copi/rate_limiter.ex +++ b/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: %{} } 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..bb1f65441 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 @@ -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 diff --git a/copi.owasp.org/lib/copi_web/router.ex b/copi.owasp.org/lib/copi_web/router.ex index 980a8ba88..cd3ae80c4 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, action: :play_card end scope "/", CopiWeb do diff --git a/copi.owasp.org/test/copi/rate_limiter_test.exs b/copi.owasp.org/test/copi/rate_limiter_test.exs index 95712de5b..26054024d 100644 --- a/copi.owasp.org/test/copi/rate_limiter_test.exs +++ b/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