Skip to content

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

Merged
sydseter merged 27 commits into
OWASP:masterfrom
immortal71:feat/rate-limit-implementation-clean
Feb 19, 2026
Merged

Add IP-based rate limiting for Copi (fixes #1877)#2179
sydseter merged 27 commits into
OWASP:masterfrom
immortal71:feat/rate-limit-implementation-clean

Conversation

@immortal71

Copy link
Copy Markdown
Contributor

Summary

This PR implements IP-based rate limiting to protect Copi against CAPEC-212 (Functionality Misuse) attacks and ensure service availability under potential abuse scenarios.

Changes

Core Implementation

  • RateLimiter GenServer (lib/copi/rate_limiter.ex): Tracks rate limits per IP address with configurable limits and time windows
  • IPHelper module (lib/copi/ip_helper.ex): Provides DRY interface for IP extraction across LiveView sockets, Phoenix sockets, and Plug connections
  • Application supervision: Added RateLimiter to the supervision tree for automatic startup and management

Protection Points

  1. Game Creation - Rate limited in lib/copi_web/live/game_live/create_game_form.ex
  2. Player Creation - Rate limited in lib/copi_web/live/player_live/form_component.ex
  3. WebSocket Connections - Rate limited in lib/copi_web/channels/user_socket.ex

Rate Limits (Configurable via Environment Variables)

  • Game creation: 10 per hour per IP
  • Player creation: 20 per hour per IP
  • WebSocket connections: 50 per 5 minutes per IP

Testing & Documentation

  • Comprehensive test suite in test/copi/rate_limiter_test.exs (200+ assertions)
  • WebSocket connection tests in test/copi_web/channels/user_socket_test.exs
  • Complete security documentation in copi.owasp.org/SECURITY.md

User Experience

When rate limits are exceeded:

  • Users see friendly error messages: "Too many [action] attempts. Please try again later."
  • WebSocket connections are gracefully denied
  • Violations are logged for monitoring

Configuration Example

# Customize limits via environment variables
RATE_LIMIT_GAME_CREATION_LIMIT=10
RATE_LIMIT_GAME_CREATION_WINDOW=3600
RATE_LIMIT_PLAYER_CREATION_LIMIT=20
RATE_LIMIT_PLAYER_CREATION_WINDOW=3600
RATE_LIMIT_CONNECTION_LIMIT=50
RATE_LIMIT_CONNECTION_WINDOW=300

Future Enhancements

As documented in SECURITY.md, if IP-based limiting proves insufficient:

  • User authentication with per-user limits
  • Browser fingerprinting
  • CAPTCHA for repeated violations
  • IP allowlisting for trusted networks

Fixes

Closes #1877

Testing

All existing tests pass. New tests cover:

  • Rate limit enforcement across all three action types
  • Independent limits for different IPs
  • Independent limits for different actions
  • Concurrent request handling
  • Graceful handling of missing IP data
  • Time-based window expiration

This is a clean implementation to replace PR #1998 which had some issues. This version is production-ready with proper error handling, comprehensive tests, and complete documentation.

@immortal71 immortal71 requested a review from rewtd as a code owner February 8, 2026 11:15
Copilot AI review requested due to automatic review settings February 8, 2026 11:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces IP-based rate limiting to protect Copi from functionality-misuse/availability attacks by tracking per-IP quotas for game creation, player creation, and (intended) WebSocket connections.

Changes:

  • Added Copi.RateLimiter GenServer with configurable limits/windows and periodic cleanup.
  • Added Copi.IPHelper and integrated rate limit checks into LiveView game/player creation flows.
  • Updated LiveView socket connect_info to include :peer_data, added tests, and added Copi-specific security documentation.

Reviewed changes

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

Show a summary per file
File Description
copi.owasp.org/lib/copi/rate_limiter.ex New GenServer implementing per-IP/action rate limiting with env-configured limits/windows.
copi.owasp.org/lib/copi/ip_helper.ex New helper for IP extraction across socket/conn types.
copi.owasp.org/lib/copi/application.ex Starts the RateLimiter under the application supervision tree.
copi.owasp.org/lib/copi_web/live/game_live/create_game_form.ex Enforces rate limiting on game creation.
copi.owasp.org/lib/copi_web/live/player_live/form_component.ex Enforces rate limiting on player creation.
copi.owasp.org/lib/copi_web/endpoint.ex Adds :peer_data to LiveView socket connect_info to support IP extraction.
copi.owasp.org/lib/copi_web/channels/user_socket.ex Adds rate limiting to Phoenix channel socket connect callback.
copi.owasp.org/test/copi/rate_limiter_test.exs New tests for rate limiter behavior and concurrency.
copi.owasp.org/test/copi_web/channels/user_socket_test.exs New tests for socket-connection rate limiting behavior.
copi.owasp.org/SECURITY.md New documentation describing the rate limiting design/configuration.

Comment thread copi.owasp.org/SECURITY.md Outdated
Comment thread copi.owasp.org/SECURITY.md
Comment thread copi.owasp.org/lib/copi/rate_limiter.ex
Comment thread copi.owasp.org/lib/copi_web/endpoint.ex Outdated
Comment thread copi.owasp.org/lib/copi_web/channels/user_socket.ex
Comment thread copi.owasp.org/lib/copi/rate_limiter.ex Outdated
Comment thread copi.owasp.org/lib/copi/ip_helper.ex
Comment thread copi.owasp.org/lib/copi_web/endpoint.ex Outdated
Comment thread copi.owasp.org/lib/copi/rate_limiter.ex
Comment thread copi.owasp.org/lib/copi/rate_limiter.ex
@immortal71 immortal71 marked this pull request as draft February 8, 2026 12:47
@immortal71 immortal71 marked this pull request as ready for review February 8, 2026 18:02
@immortal71

Copy link
Copy Markdown
Contributor Author

@sydseter finally this is ready to be review I have close previous pr and open this after noticing issue with branch itself
Test has been passed and issued revolved feel free to say if i need to make some changes here,
thanks for letting me work on this I have learn many things while working on this issue !!

@sydseter

sydseter commented Feb 9, 2026

Copy link
Copy Markdown
Collaborator

Some or your commits lack a verified signature. Can you pleade make sure all of the has?

@immortal71

Copy link
Copy Markdown
Contributor Author

Some or your commits lack a verified signature. Can you pleade make sure all of the has?

Okay

immortal71 and others added 18 commits February 8, 2026 23:50
Implemented IP-based rate limiting to protect against CAPEC-212 (Functionality Misuse) attacks:

- Added RateLimiter GenServer to track rate limits per IP address
- Added IPHelper module for DRY IP extraction across the application
- Integrated rate limiting into game creation workflow
- Integrated rate limiting into player creation workflow
- Added RateLimiter to application supervision tree
- Comprehensive test suite for rate limiter functionality

Rate limits (configurable via environment variables):
- Game creation: 10 per hour per IP
- Player creation: 20 per hour per IP
- WebSocket connections: 50 per 5 minutes per IP

This ensures service availability under attack while maintaining usability for legitimate users.
- Implemented rate limiting for WebSocket connections (50 per 5 minutes per IP)
- Updated endpoint to pass peer_data for IP extraction
- Added comprehensive tests for WebSocket rate limiting
- Created SECURITY.md documenting the complete rate limiting implementation
- Includes configuration, architecture details, and future enhancements

This completes the full protection against CAPEC-212 attacks for all entry points.
…in previous commit)

- Added IPHelper module tests covering all functions
- Added game creation rate limiting integration test
- Added player creation rate limiting integration test
- Changed async: false for rate limiter tests to avoid conflicts
- All tests verify rate limiting enforcement and error messages
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot Feedback Fixes:
- Added X-Forwarded-For support for reverse proxy scenarios
- Added error handling for env var parsing (prevents boot failures)
- Already added logging for rate limit violations (previous commit)
- Fixed markdown table formatting in SECURITY.md

New Features:
- IPHelper now reads X-Forwarded-For header to get real client IP
- Falls back to remote_ip if header missing/invalid
- Environment variable parsing validates positive integers
- Logs warnings for invalid config, uses defaults gracefully

Additional Tests:
- CreateGameForm component tests
- FormComponent (player creation) tests
- Application supervision tree tests
- Endpoint configuration tests
- X-Forwarded-For header handling tests
- Invalid environment variable handling tests

Documentation Updates:
- Documented X-Forwarded-For proxy support in SECURITY.md
- Documented env var validation requirements
- Updated all Copilot suggestions resolved
Test Setup Fixes:
- Fixed LiveView test setup to provide IP addresses via connect_info
- Added peer_data configuration to all LiveView test files
- Resolves 'Unable to determine IP address from socket' RuntimeError

New Test Files:
- Added router_test.exs: Tests all routes and pipelines
- Added telemetry_test.exs: Tests telemetry supervisor and metrics

Enhanced Existing Tests:
- rate_limiter_test.exs: Added IP normalization, cleanup, and malformed input tests
- ip_helper_test.exs: Added X-Forwarded-For edge cases, IPv6, and whitespace handling

Coverage improvements target 80% threshold for CI/CD pipeline success
Added new test files:
- game_form_helpers_test.exs: Tests all suit formatting and edition handling functions
- gettext_test.exs: Tests internationalization helpers
- error_json_test.exs: Tests JSON error rendering for multiple status codes
- page_html_test.exs: Tests page template embedding

Enhanced existing tests:
- cornucopia_logic_test.exs: Added 9 new tests for scoring logic, card filtering,
  vote requirements, joker/trump handling, and card aggregation functions

Coverage improvements:
- Tests GameFormHelpers: generate_suit_list, format_suits, display_appropriate_suits
- Tests Cornucopia scoring: highest_scoring_cards, played_cards, unplayed_cards, voting
- Tests error handling: 404, 500, 401, 403, 422, and custom status codes
- Tests gettext: translations, interpolation, domains, plurals
- Tests all edge cases: empty lists, wild cards, suit mismatches, vote thresholds

These additions target untested modules and complex logic branches to exceed 80% coverage
Moved import CopiWeb.Gettext to module level so functions are available to all tests
- Simplified gettext test to only check module configuration (gettext macros are compile-time)

- Updated 422 error message to 'Unprocessable Content' (Phoenix updated)

- Fixed unused variable warning in form_component_test.exs
…alhost IP instead of raising exceptions when IP unavailable, add comprehensive LiveView tests for delete/broadcast scenarios
…direct assertions, API params, and endpoint config
@immortal71 immortal71 force-pushed the feat/rate-limit-implementation-clean branch from eaad018 to 676a30a Compare February 9, 2026 07:52
@sydseter

Copy link
Copy Markdown
Collaborator

It looks good now. Great!

@sydseter

Copy link
Copy Markdown
Collaborator

I need some time reviewing this.

Comment thread copi.owasp.org/test/copi/rate_limiter_test.exs
Comment thread copi.owasp.org/lib/copi/rate_limiter.ex Outdated
Comment thread copi.owasp.org/lib/copi/rate_limiter.ex
Comment thread copi.owasp.org/SECURITY.md
@sydseter

Copy link
Copy Markdown
Collaborator

@immortal71 Fantastic great work! You are almost there!

@sydseter

Copy link
Copy Markdown
Collaborator

@immortal71 Please have a look at my comments. I will need to do some extensive load testing. Thank you so much for taking this this far and not giving in.

Comment thread copi.owasp.org/SECURITY.md Outdated
Comment thread copi.owasp.org/SECURITY.md Outdated
Comment thread copi.owasp.org/SECURITY.md Outdated
Comment thread copi.owasp.org/SECURITY.md Outdated
Comment thread copi.owasp.org/SECURITY.md Outdated
Comment thread copi.owasp.org/SECURITY.md Outdated
Comment thread copi.owasp.org/SECURITY.md Outdated
Changes requested by sydseter:
- Skip rate limiting for 127.0.0.1 in production to prevent self-DoS
- Update default limits: game (10->20), player (20->60), connections (50->333)
- Update connection window from 300s to 1s for connections/second limiting
- Add production environment variables to fly.toml
- Update SECURITY.md documentation with new limits

The rate limiter now checks if running in production before applying
limits to localhost. Warning logging for rate_limit_exceeded is in place.
@immortal71

Copy link
Copy Markdown
Contributor Author

@sydseter done !!

Comment thread copi.owasp.org/lib/copi/rate_limiter.ex Outdated
Comment thread copi.owasp.org/lib/copi/rate_limiter.ex Outdated
Comment thread copi.owasp.org/config/test.exs Outdated
@sydseter

Copy link
Copy Markdown
Collaborator

You have a bug. See comments. Otherwise. Everything looks good.

@sydseter sydseter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at my remaining comments.

Fixed issues identified by sydseter:
- Use monotonic_time in cleanup to match check_rate timing
- Convert window to milliseconds in cleanup filter
- Remove password fallback in test config
…t/rate-limit-implementation-clean

Resolved conflict in player_live_test.exs by accepting incoming test changes from master
Restores two tests that were accidentally removed during merge:
- 'displays player information' test
- 'handles game updates via broadcast' test

These tests cover important code paths for LiveView rendering
and broadcast handling, restoring coverage from 79.7% to ~81%.
The previous test just sent a message without verifying it was
processed. Now using Phoenix.PubSub.broadcast with proper topic
and adding assertion to verify LiveView processes the update.
Adds two new tests to cover untested code paths:
- Toggle continue vote off (remove existing vote)
- Handle next round event when round is open

These tests cover the continue voting logic added in the
master merge, pushing coverage back above 80%.
@sydseter

Copy link
Copy Markdown
Collaborator

@immortal71 regarding the coverage. I’ll allow it. Close enough.

@sydseter

Copy link
Copy Markdown
Collaborator

Just make sure the tests doesn’t fail. I’ll create a new task to improve test coverage.

1. Fixed broadcast test to send message in correct format:
   - handle_info expects map with topic/event/payload
   - Was sending tuple {:game_updated, game}
   - Now sends %{topic:, event:, payload:}

2. Fixed rate limiter test assertion:
   - Connection window is 1 second (for 333 req/s)
   - Changed assertion from >= 60 to >= 1

3. Removed redundant next round test that had same issue
@immortal71

Copy link
Copy Markdown
Contributor Author

@sydseter done

@immortal71

Copy link
Copy Markdown
Contributor Author

Just make sure the tests doesn’t fail. I’ll create a new task to improve test coverage.

sounds great ,U can assign it to me ,I have almost completed all the pr I was working on

Comment on lines +73 to +78
defp get_connect_info_ip(socket) do
if Map.has_key?(socket.private, :connect_info) do
socket.private.connect_info[:peer_data][:address]
else
nil
end

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@immortal71 could you have a look at this one?

@sydseter

sydseter commented Feb 17, 2026

Copy link
Copy Markdown
Collaborator

@immortal71 this is very close now. There is a comment from Copilot that seems valid. The part you implemented could potentially become a hard to track bug. We should use the public api if possible since we will get issues in the future otherwise.

Changed from accessing socket.private.connect_info directly to using
Phoenix.LiveView.get_connect_info/2 which is the recommended public API.

This prevents potential bugs from internal implementation changes and
follows Phoenix best practices.

Also added :peer_data to endpoint connect_info configuration to ensure
the data is available via the public API.

Addresses feedback from @sydseter
Use safe [:key] access instead of .key to avoid raising
when keys don't exist. This works with both production sockets
and test mocks.
@immortal71

Copy link
Copy Markdown
Contributor Author

@sydseter done

@sydseter

Copy link
Copy Markdown
Collaborator

Great!

@sydseter sydseter merged commit f05acf3 into OWASP:master Feb 19, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security overhal of Copi

3 participants