Added IP-based rate limiting for Copi (fixes #1877)

[immortal71]

Implemented separate IP-based rate limits for game creation, player creation, and WebSocket connections to protect against CAPEC-212 (Functionality Misuse) attacks and ensure service availability.

Changes:

• Implemented Copi.RateLimiter GenServer for tracking rate limits per IP
• Added separate rate limiting for:
  • Game creation: 10 per IP per hour
  • Player creation: 20 per IP per hour
  • Connections: 50 per IP per 5 minutes
• Created shared IPHelper module for DRY IP extraction
• Moved configuration to runtime.exs for proper env var handling
• Removed 'unknown' IP fallback for security (raises error instead)
• Adds comprehensive tests and SECURITY.md documentation

Addressed feedback from PR #1920:

• Config moved to runtime.exs
• Refactored duplicated get_connect_ip/1
• Removed 'unknown' IP security issue

#1920

[immortal71]

@sydseter waiting for your review !!

[Copilot]

Pull request overview

This PR implements IP-based rate limiting to protect against CAPEC-212 (Functionality Misuse) attacks. However, there are several critical issues that prevent the feature from working correctly and compromise its security effectiveness.

Key Changes:

• Added Copi.RateLimiter GenServer to track rate limits per IP address across three action types (game creation, player creation, connections)
• Created CopiWeb.Helpers.IPHelper module to extract IP addresses from LiveView sockets
• Integrated rate limiting checks into game creation, player creation, and WebSocket connection flows

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
copi.owasp.org/lib/copi/rate_limiter.ex	Implements GenServer for tracking IP-based rate limits with configurable thresholds and time windows
copi.owasp.org/lib/copi_web/helpers/ip_helper.ex	Extracts IP addresses from LiveView socket connections for rate limiting
copi.owasp.org/lib/copi/application.ex	Adds RateLimiter to application supervision tree
copi.owasp.org/lib/copi_web/live/game_live/index.ex	Adds connection rate limiting to mount function
copi.owasp.org/lib/copi_web/live/game_live/create_game_form.ex	Adds game creation rate limiting with user-facing error messages
copi.owasp.org/lib/copi_web/live/player_live/form_component.ex	Adds player creation rate limiting with user-facing error messages
copi.owasp.org/config/runtime.exs	Configures rate limit thresholds via environment variables
copi.owasp.org/test/copi/rate_limiter_test.exs	Comprehensive test suite for rate limiter functionality
copi.owasp.org/SECURITY.md	Documents rate limiting configuration and security policies
copi.owasp.org/mix.lock	Removes depth limitation from heroicons git dependency (unrelated change)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Connection rate limit is not enforced. The code checks the rate limit but never calls record_action for the connection action type, which means the connection count never increases and the limit can never be reached. After the check passes, you should call Copi.RateLimiter.record_action(ip_address, :connection) to actually track the connection attempt.

[sydseter]

Why aren't you calling record_action?

[Copilot]

Test doesn't validate its stated purpose. This test claims to verify that "requests are allowed after window expires" but it doesn't actually wait for any time window to expire. The test only verifies that two consecutive requests are allowed, which is already covered by other tests. Either remove this test or implement proper window expiration testing using techniques mentioned in the comment (e.g., allow injecting a clock/time function for testing).

[sydseter]

Please implement what the comment says.

[Copilot]

Race condition vulnerability: The separation of check_rate and record_action creates a TOCTOU (Time-of-Check-Time-of-Use) issue. Multiple concurrent requests from the same IP could all pass the check_rate call before any of them calls record_action, effectively bypassing the rate limit. For example, if the limit is 10 and there are currently 9 recorded actions, 5 concurrent requests could all check (seeing 9 < 10), all pass, and all record, resulting in 14 total actions. Consider making check_rate atomically record the action when it returns {:ok, _}, or create a check_and_record/2 function that does both operations in a single GenServer call.

[immortal71]

@sydseter waiting for review

[sydseter]

We definetly need test coverage on this.

[sydseter]

The test is failing.

Tests are failing. Please ensure that they pass:

..09:15:37.703 [error] GenServer Copi.RateLimiter terminating
** (ArgumentError) could not put/update key :connection on a nil value
(elixir 1.19.2) lib/access.ex:436: Access.get_and_update/3
(elixir 1.19.2) lib/map.ex:965: Map.get_and_update/3
(elixir 1.19.2) lib/kernel.ex:2950: Kernel.put_in/3
(copi 0.1.0) lib/copi/rate_limiter.ex:144: Copi.RateLimiter.handle_call/3
(stdlib 7.1) gen_server.erl:2470: :gen_server.try_handle_call/4
(stdlib 7.1) gen_server.erl:2499: :gen_server.handle_msg/3
(stdlib 7.1) proc_lib.erl:333: :proc_lib.init_p_do_apply/3
Last message (from #PID<0.8256.0>): {:check_and_record, "127.0.0.1", :connection}

1. test Index saves new game (CopiWeb.GameLiveTest)
   test/copi_web/live/game_live_test.exs:31
   ** (exit) exited in: GenServer.call(Copi.RateLimiter, {:check_and_record, "127.0.0.1", :connection}, 5000)
   ** (EXIT) an exception was raised:
   ** (ArgumentError) could not put/update key :connection on a nil value
   (elixir 1.19.2) lib/access.ex:436: Access.get_and_update/3
   (elixir 1.19.2) lib/map.ex:965: Map.get_and_update/3
   (elixir 1.19.2) lib/kernel.ex:2950: Kernel.put_in/3
   (copi 0.1.0) lib/copi/rate_limiter.ex:144: Copi.RateLimiter.handle_call/3
   (stdlib 7.1) gen_server.erl:2470: :gen_server.try_handle_call/4
   (stdlib 7.1) gen_server.erl:2499: :gen_server.handle_msg/3
   (stdlib 7.1) proc_lib.erl:333: :proc_lib.init_p_do_apply/3
   code: {:ok, index_live, _html} = live(conn, "/games")
   stacktrace:
   (elixir 1.19.2) lib/gen_server.ex:1142: GenServer.call/3
   (copi 0.1.0) lib/copi_web/live/game_live/index.ex:14: CopiWeb.GameLive.Index.mount/3
   (phoenix_live_view 1.0.17) lib/phoenix_live_view/utils.ex:348: anonymous fn/6 in Phoenix.LiveView.Utils.maybe_call_live_view_mount!/5
   (telemetry 1.3.0) c:/Users/JSydseter/src/cornucopia-test/copi.owasp.org/deps/telemetry/src/telemetry.erl:324: :telemetry.span/3
   (phoenix_live_view 1.0.17) lib/phoenix_live_view/static.ex:321: Phoenix.LiveView.Static.call_mount_and_handle_params!/5
   (phoenix_live_view 1.0.17) lib/phoenix_live_view/static.ex:155: Phoenix.LiveView.Static.do_render/4
   (phoenix_live_view 1.0.17) lib/phoenix_live_view/controller.ex:39: Phoenix.LiveView.Controller.live_render/3
   (phoenix 1.8.3) lib/phoenix/router.ex:416: Phoenix.Router.call/5
   (copi 0.1.0) lib/copi_web/endpoint.ex:1: CopiWeb.Endpoint.plug_builder_call/2
   (copi 0.1.0) lib/copi_web/endpoint.ex:1: CopiWeb.Endpoint.call/2
   (phoenix 1.8.3) lib/phoenix/test/conn_test.ex:225: Phoenix.ConnTest.dispatch/5
   test/copi_web/live/game_live_test.exs:32: (test)

.09:15:37.808 [error] GenServer #PID<0.8278.0> terminating
** (RuntimeError) attempted to read connect_info outside of CopiWeb.PlayerLive.Index.mount/3.

connect_info only exists while mounting. If you require access to this information
after mount, store the state in socket assigns.

(phoenix_live_view 1.0.17) lib/phoenix_live_view.ex:1338: Phoenix.LiveView.raise_root_and_mount_only!/2
(copi 0.1.0) lib/copi_web/helpers/ip_helper.ex:22: CopiWeb.Helpers.IPHelper.get_connect_ip/1
(copi 0.1.0) lib/copi_web/live/player_live/form_component.ex:79: CopiWeb.PlayerLive.FormComponent.save_player/3
(phoenix_live_view 1.0.17) lib/phoenix_live_view/channel.ex:742: anonymous fn/4 in Phoenix.LiveView.Channel.inner_component_handle_event/4
(telemetry 1.3.0) c:/Users/JSydseter/src/cornucopia-test/copi.owasp.org/deps/telemetry/src/telemetry.erl:324: :telemetry.span/3
(phoenix_live_view 1.0.17) lib/phoenix_live_view/diff.ex:209: Phoenix.LiveView.Diff.write_component/4
(phoenix_live_view 1.0.17) lib/phoenix_live_view/channel.ex:663: Phoenix.LiveView.Channel.component_handle/4
(stdlib 7.1) gen_server.erl:2434: :gen_server.try_handle_info/3
(stdlib 7.1) gen_server.erl:2420: :gen_server.handle_msg/3
(stdlib 7.1) proc_lib.erl:333: :proc_lib.init_p_do_apply/3

Last message: %Phoenix.Socket.Message{topic: "lv:phx-GIizbcVqAYCuoFwC", event: "event", payload: %{"cid" => 1, "event" => "save", "type" => "form", "value" => "player[game_id]=01KEEASVRXNZQE49TEV4V3W80F&player[name]=some+updated+name"}, ref: "2", join_ref: 0}
09:15:37.811 [error] GenServer #PID<0.8274.0> terminating
** (RuntimeError) attempted to read connect_info outside of CopiWeb.PlayerLive.Index.mount/3.

connect_info only exists while mounting. If you require access to this information
after mount, store the state in socket assigns.

(phoenix_live_view 1.0.17) lib/phoenix_live_view.ex:1338: Phoenix.LiveView.raise_root_and_mount_only!/2
(copi 0.1.0) lib/copi_web/helpers/ip_helper.ex:22: CopiWeb.Helpers.IPHelper.get_connect_ip/1
(copi 0.1.0) lib/copi_web/live/player_live/form_component.ex:79: CopiWeb.PlayerLive.FormComponent.save_player/3
(phoenix_live_view 1.0.17) lib/phoenix_live_view/channel.ex:742: anonymous fn/4 in Phoenix.LiveView.Channel.inner_component_handle_event/4
(telemetry 1.3.0) c:/Users/JSydseter/src/cornucopia-test/copi.owasp.org/deps/telemetry/src/telemetry.erl:324: :telemetry.span/3
(phoenix_live_view 1.0.17) lib/phoenix_live_view/diff.ex:209: Phoenix.LiveView.Diff.write_component/4
(phoenix_live_view 1.0.17) lib/phoenix_live_view/channel.ex:663: Phoenix.LiveView.Channel.component_handle/4
(stdlib 7.1) gen_server.erl:2434: :gen_server.try_handle_info/3
(stdlib 7.1) gen_server.erl:2420: :gen_server.handle_msg/3
(stdlib 7.1) proc_lib.erl:333: :proc_lib.init_p_do_apply/3

Last message: {:EXIT, #PID<0.8272.0>, {%RuntimeError{message: "attempted to read connect_info outside of CopiWeb.PlayerLive.Index.mount/3.\n\nconnect_info only exists while mounting. If you require access to this information\nafter mount, store the state in socket assigns.\n"}, [{Phoenix.LiveView, :raise_root_and_mount_only!, 2, [file: ~c"lib/phoenix_live_view.ex", line: 1338]}, {CopiWeb.Helpers.IPHelper, :get_connect_ip, 1, [file: ~c"lib/copi_web/helpers/ip_helper.ex", line: 22]}, {CopiWeb.PlayerLive.FormComponent, :save_player, 3, [file: ~c"lib/copi_web/live/player_live/form_component.ex", line: 79]}, {Phoenix.LiveView.Channel, :"-inner_component_handle_event/4-fun-0-", 4, [file: ~c"lib/phoenix_live_view/channel.ex", line: 742]}, {:telemetry, :span, 3, [file: ~c"c:/Users/JSydseter/src/cornucopia-test/copi.owasp.org/deps/telemetry/src/telemetry.erl", line: 324]}, {Phoenix.LiveView.Diff, :write_component, 4, [file: ~c"lib/phoenix_live_view/diff.ex", line: 209]}, {Phoenix.LiveView.Channel, :component_handle, 4, [file: ~c"lib/phoenix_live_view/channel.ex", line: 663]}, {:gen_server, :try_handle_info, 3, [file: ~c"gen_server.erl", line: 2434]}, {:gen_server, :handle_msg, 3, [file: ~c"gen_server.erl", line: 2420]}, {:proc_lib, :init_p_do_apply, 3, [file: ~c"proc_lib.erl", line: 333]}]}}

2. test Index saves new player (CopiWeb.PlayerLiveTest)
   test/copi_web/live/player_live_test.exs:33
   ** (EXIT from #PID<0.8272.0>) an exception was raised:
   ** (RuntimeError) attempted to read connect_info outside of CopiWeb.PlayerLive.Index.mount/3.

   connect_info only exists while mounting. If you require access to this information
   after mount, store the state in socket assigns.

       (phoenix_live_view 1.0.17) lib/phoenix_live_view.ex:1338: Phoenix.LiveView.raise_root_and_mount_only!/2
       (copi 0.1.0) lib/copi_web/helpers/ip_helper.ex:22: CopiWeb.Helpers.IPHelper.get_connect_ip/1
       (copi 0.1.0) lib/copi_web/live/player_live/form_component.ex:79: CopiWeb.PlayerLive.FormComponent.save_player/3
       (phoenix_live_view 1.0.17) lib/phoenix_live_view/channel.ex:742: anonymous fn/4 in Phoenix.LiveView.Channel.inner_component_handle_event/4
       (telemetry 1.3.0) c:/Users/JSydseter/src/cornucopia-test/copi.owasp.org/deps/telemetry/src/telemetry.erl:324: :telemetry.span/3
       (phoenix_live_view 1.0.17) lib/phoenix_live_view/diff.ex:209: Phoenix.LiveView.Diff.write_component/4
       (phoenix_live_view 1.0.17) lib/phoenix_live_view/channel.ex:663: Phoenix.LiveView.Channel.component_handle/4
       (stdlib 7.1) gen_server.erl:2434: :gen_server.try_handle_info/3
       (stdlib 7.1) gen_server.erl:2420: :gen_server.handle_msg/3
       (stdlib 7.1) proc_lib.erl:333: :proc_lib.init_p_do_apply/3
   

3. test player creation rate limiting blocks player creation over the limit (Copi.RateLimiterTest)
   test/copi/rate_limiter_test.exs:111
   ** (MatchError) no match of right hand side value:

   {:error, {:already_started, #PID<0.8258.0>}}
   

   stacktrace:
   test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
   test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

4. test connection rate limiting blocks connections over the limit (Copi.RateLimiterTest)
   test/copi/rate_limiter_test.exs:82
   ** (MatchError) no match of right hand side value:

   {:error, {:already_started, #PID<0.8258.0>}}
   

   stacktrace:
   test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
   test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

5. test player creation rate limiting allows player creation under the limit (Copi.RateLimiterTest)
   test/copi/rate_limiter_test.exs:100
   ** (MatchError) no match of right hand side value:

   {:error, {:already_started, #PID<0.8258.0>}}
   

   stacktrace:
   test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
   test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

6. test connection rate limiting allows connections under the limit (Copi.RateLimiterTest)
   test/copi/rate_limiter_test.exs:71
   ** (MatchError) no match of right hand side value:

   {:error, {:already_started, #PID<0.8258.0>}}
   

   stacktrace:
   test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
   test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

7. test IP clearing clears rate limit data for an IP (Copi.RateLimiterTest)
   test/copi/rate_limiter_test.exs:167
   ** (MatchError) no match of right hand side value:

   {:error, {:already_started, #PID<0.8258.0>}}
   

   stacktrace:
   test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
   test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

8. test player creation rate limiting player creation limit is separate from game creation limit (Copi.RateLimiterTest)
   test/copi/rate_limiter_test.exs:127
   ** (MatchError) no match of right hand side value:

   {:error, {:already_started, #PID<0.8258.0>}}
   

   stacktrace:
   test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
   test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

9. test configuration returns current configuration (Copi.RateLimiterTest)
   test/copi/rate_limiter_test.exs:147
   ** (MatchError) no match of right hand side value:

   {:error, {:already_started, #PID<0.8258.0>}}
   

   stacktrace:
   test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
   test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

10. test game creation rate limiting different IPs have independent limits (Copi.RateLimiterTest)
    test/copi/rate_limiter_test.exs:50
    ** (MatchError) no match of right hand side value:

    {:error, {:already_started, #PID<0.8258.0>}}
    

    stacktrace:
    test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
    test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

11. test game creation rate limiting allows requests under the limit (Copi.RateLimiterTest)
    test/copi/rate_limiter_test.exs:21
    ** (MatchError) no match of right hand side value:

    {:error, {:already_started, #PID<0.8258.0>}}
    

    stacktrace:
    test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
    test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

12. test game creation rate limiting blocks requests over the limit (Copi.RateLimiterTest)
    test/copi/rate_limiter_test.exs:34
    ** (MatchError) no match of right hand side value:

    {:error, {:already_started, #PID<0.8258.0>}}
    

    stacktrace:
    test/copi/rate_limiter_test.exs:8: Copi.RateLimiterTest.__ex_unit_setup_0/1
    test/copi/rate_limiter_test.exs:1: Copi.RateLimiterTest.ex_unit/2

[sydseter]

I am still getting the same test errors.

[sydseter]

Please have a look at my comments. Thank you for your effort.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When rate limiting succeeds, list_games() is called but the function is not visible in this diff. The original code assigned nil to games. If list_games/0 doesn't exist in this module, this will cause a compilation error.

[Copilot]

The nesting creates complex control flow with two levels of case statements. Consider extracting the player creation logic into a separate private function (e.g., do_create_player/2) to improve readability and reduce nesting depth.

[Copilot]

Using put_in/3 with a list of keys on a potentially non-existent nested path will raise an error if the IP address key doesn't exist in the map. Use Kernel.put_in/3 or ensure the path exists first, or consider using Map.put/3 with Map.get/3 for safer nested map updates.

[sydseter]

This can be an issue and should be dealt with.

[Copilot]

Similar to the issue in check_and_record, using put_in/3 with a path that may not exist will raise an error. This occurs when recording an action for a new IP address that hasn't been seen before.

[sydseter]

Same here.

[sydseter]

The tests are still throwing errors which indicate that there is something wrong with how the rate limiter is implemented.

[sydseter]

Please update your branch a before continuing.

[immortal71]

@sydseter Can you check, i have updated as you have suggested !!