Fix race condition in voting system with unique constraint and safe upsert by immortal71 · Pull Request #2289 · OWASP/cornucopia

immortal71 · 2026-02-19T00:35:49Z

Summary

This PR fixes a critical race condition in the voting system (#2288) that could allow players to cast duplicate votes on the same card, corrupting game outcomes and data integrity.

Problem

The voting system had two critical issues:

Missing Database Constraint: The votes table lacked a unique constraint on (player_id, dealt_card_id), unlike the newer continue_votes table which properly includes such a constraint.
Race Condition in Application Logic: Both toggle_vote and toggle_continue_vote handlers used unsafe check-then-act patterns without proper transaction isolation, allowing concurrent requests to both pass the check and insert duplicate votes.

Changes

Database Migration

Added migration 20260218120000_add_unique_constraint_to_votes.exs
Cleans up any existing duplicate votes
Creates unique index on votes(player_id, dealt_card_id)
Ensures data integrity at the database level

Application Code Refactoring

lib/copi_web/live/player_live/show.ex:

Added Vote and ContinueVote aliases
Added Logger requirement
Refactored toggle_continue_vote to:
- Use Repo.get_by for atomic vote checking
- Use on_conflict: :nothing with conflict_target for safe inserts
- Handle errors gracefully without crashing
- Replace IO.puts debug statements with Logger.debug/warning
Refactored toggle_vote to:
- Use Repo.get_by instead of preloading and searching in memory
- Use on_conflict: :nothing with conflict_target for safe inserts
- Handle errors gracefully
- Replace IO.puts debug statements with Logger.debug/warning

Testing Strategy

The unique constraint ensures that:

Concurrent insert attempts will result in only one successful insert
The on_conflict: :nothing parameter prevents crashes on constraint violations
The application gracefully handles race conditions without user-visible errors

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>

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

…assword fallback

…HTML from successful form submissions

…direct assertions, API params, and endpoint config

…t patterns, and simplify UI tests

…t, redirect patterns, validation tests

…lear rate limits in all setups

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.

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%.

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

@sydseter

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.

Add unique constraint on votes(player_id, dealt_card_id) Add database migration to cleanup duplicates and add constraint Refactor toggle_vote to use safe upsert pattern with on_conflict Refactor toggle_continue_vote to use database query instead of memory check Replace IO.puts debug statements with Logger.debug/warning calls Use Repo.get_by to atomically check for existing votes Handle constraint violations gracefully with on_conflict: :nothing Fixes OWASP#2288

Copilot

Pull request overview

This PR addresses concurrency and abuse-resilience in the Copi Phoenix app by (1) enforcing vote uniqueness at the database layer and (2) refactoring vote toggling to use safe inserts. It also introduces an IP-based rate limiter used by game/player creation and socket connections, plus a large set of new tests.

Changes:

Add a migration to deduplicate votes and enforce uniqueness on (player_id, dealt_card_id).
Refactor toggle_vote / toggle_continue_vote to use Repo.get_by and conflict-safe inserts with logging.
Introduce an in-memory IP-based RateLimiter (+ IPHelper and endpoint/socket wiring) and add tests/docs/config for the new behavior.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
copi.owasp.org/priv/repo/migrations/20260218120000_add_unique_constraint_to_votes.exs	Dedupes and adds a unique index to prevent duplicate votes.
copi.owasp.org/lib/copi_web/live/player_live/show.ex	Refactors vote toggles to DB lookups + safe upserts; switches to Logger.
copi.owasp.org/lib/copi_web/live/player_live/form_component.ex	Adds rate limiting to player creation via socket-derived IP.
copi.owasp.org/lib/copi_web/live/game_live/create_game_form.ex	Adds rate limiting to game creation via socket-derived IP.
copi.owasp.org/lib/copi/rate_limiter.ex	New GenServer implementing per-IP sliding-window rate limits.
copi.owasp.org/lib/copi/ip_helper.ex	New helper for extracting IPs from sockets/connections.
copi.owasp.org/lib/copi_web/endpoint.ex	Expands LiveView socket `connect_info` to include peer data / headers.
copi.owasp.org/lib/copi_web/channels/user_socket.ex	Adds connection rate limiting on socket connect.
copi.owasp.org/lib/copi/application.ex	Starts the RateLimiter under the supervision tree.
copi.owasp.org/fly.toml	Adds RATE_LIMIT_* environment variables.
copi.owasp.org/SECURITY.md	Documents the new rate limiting design and configuration.
copi.owasp.org/test/copi_web/telemetry_test.exs	Adds telemetry startup/metrics smoke tests.
copi.owasp.org/test/copi_web/router_test.exs	Adds router/API route coverage.
copi.owasp.org/test/copi_web/live/player_live_test.exs	Adds LiveView coverage for rate limiting and continue-vote toggling.
copi.owasp.org/test/copi_web/live/player_live/form_component_test.exs	Adds rate limiting tests for player creation flow.
copi.owasp.org/test/copi_web/live/game_live_test.exs	Adds rate limiting tests for game creation flow.
copi.owasp.org/test/copi_web/live/game_live/game_form_helpers_test.exs	Adds unit tests for game form helper transformations.
copi.owasp.org/test/copi_web/live/game_live/create_game_form_test.exs	Adds rate limiting tests for create-game LiveComponent.
copi.owasp.org/test/copi_web/gettext_test.exs	Adds smoke tests for Gettext backend functions.
copi.owasp.org/test/copi_web/endpoint_test.exs	Adds smoke tests for endpoint socket config and process startup.
copi.owasp.org/test/copi_web/controllers/page_html_test.exs	Adds smoke tests for PageHTML component/template functions.
copi.owasp.org/test/copi_web/controllers/error_json_test.exs	Adds tests for ErrorJSON rendering.
copi.owasp.org/test/copi_web/channels/user_socket_test.exs	Adds tests for connection rate limiting in UserSocket.
copi.owasp.org/test/copi/rate_limiter_test.exs	Adds unit tests for rate limiter behavior and concurrency.
copi.owasp.org/test/copi/ip_helper_test.exs	Adds tests for IP extraction logic.
copi.owasp.org/test/copi/cornucopia_logic_test.exs	Expands Cornucopia logic tests around scoring / suits.
copi.owasp.org/test/copi/application_test.exs	Adds a test asserting RateLimiter is supervised and app is started.

Copilot · 2026-02-19T00:44:07Z

copi.owasp.org/lib/copi/ip_helper.ex

+  defp get_connect_info_ip(socket) do
+    # Access peer_data from connect_info
+    # This is safe in both production and test environments
+    case socket.private[:connect_info][:peer_data] do
+      %{address: address} -> address
+      _ -> nil
+    end
+  end


get_connect_info_ip/1 can raise when socket.private[:connect_info] is missing (e.g., in tests or if connect_info isn’t configured), because socket.private[:connect_info][:peer_data] attempts to index into nil. Use a nil-safe access pattern (e.g., get_in(socket.private, [:connect_info, :peer_data])) or pattern match on %{connect_info: %{peer_data: ...}} so get_ip_from_socket/1 can reliably fall back instead of crashing.

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

Copilot · 2026-02-19T00:44:07Z

copi.owasp.org/lib/copi_web/live/player_live/show.ex

    player = socket.assigns.player
-
-    {:ok, dealt_card} = DealtCard.find(dealt_card_id)
-
-    vote = get_vote(dealt_card, player)
-
-    if vote do
-      IO.puts("player has voted")
-      Copi.Repo.delete!(vote)
-    else
-      IO.puts("player hasn't voted")
-      case Copi.Repo.insert(%Copi.Cornucopia.Vote{dealt_card_id: String.to_integer(dealt_card_id), player_id: player.id}) do
-        {:ok, _vote} ->
-          IO.puts("voted successfully")
-        {:error, _changeset} ->
-          IO.puts("voting failed")
-      end
+    dealt_card_id_int = String.to_integer(dealt_card_id)
+
+    # Use database query to avoid race condition


String.to_integer/1 will raise if dealt_card_id is not a valid integer, which can crash the LiveView process (user-controlled params can reach this handler). Prefer safe parsing (e.g., Integer.parse/1 with a guarded match) and return {:noreply, socket} (optionally with a flash) on invalid input to fail securely.

Copilot · 2026-02-19T00:44:08Z

copi.owasp.org/priv/repo/migrations/20260218120000_add_unique_constraint_to_votes.exs

+  def change do
+    # Remove any existing duplicate votes first
+    execute """
+    DELETE FROM votes a USING votes b
+    WHERE a.id > b.id 
+    AND a.player_id = b.player_id 
+    AND a.dealt_card_id = b.dealt_card_id
+    """
+
+    create unique_index(:votes, [:player_id, :dealt_card_id], 
+      name: :votes_player_dealt_card_unique_index)
+  end


This migration uses execute inside change/0, which makes the migration non-reversible. If rollbacks are part of your operational workflow, split into explicit up/0 and down/0 (or use execute(up_sql, down_sql)) so the index can be dropped on rollback.

Copilot · 2026-02-19T00:44:08Z

copi.owasp.org/priv/repo/migrations/20260218120000_add_unique_constraint_to_votes.exs

+    create unique_index(:votes, [:player_id, :dealt_card_id], 
+      name: :votes_player_dealt_card_unique_index)
+  end


Creating a unique index on votes without CONCURRENTLY will take an exclusive lock on the table in Postgres during index build, blocking writes (and potentially reads depending on the plan). For production safety, consider create unique_index(..., concurrently: true) and add @disable_ddl_transaction true to the migration, especially if votes can be large.

copi.owasp.org/test/copi_web/router_test.exs

Copilot · 2026-02-19T00:44:08Z

copi.owasp.org/lib/copi/application.ex

      {Phoenix.PubSub, name: Copi.PubSub},
+      # Start the RateLimiter for IP-based rate limiting (CAPEC-212 protection)
+      Copi.RateLimiter,
      # Start the DNS clustering
      {DNSCluster, query: Application.get_env(:copi, :dns_cluster_query) || :ignore},
      # Start the Endpoint (http/https)


The PR title/description focuses on fixing the voting race condition, but this change also introduces a new global IP-based RateLimiter supervision child and related endpoint/socket behavior. Please either update the PR description/title to include the rate limiting feature (and its rationale/impact), or split rate limiting into a separate PR to keep scope and review risk manageable.

Add tests for preventing duplicate votes (concurrent inserts) Add tests for preventing duplicate continue votes Add test for toggling card vote off Tests verify on_conflict behavior and unique constraint protection Improves code coverage for voting handlers

immortal71 · 2026-02-19T01:00:51Z

@sydseter is this good to go ?

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Use get_in for nil-safe socket.private access in ip_helper Use Integer.parse instead of String.to_integer to prevent crashes Make migration reversible with up/down instead of change Create unique index concurrently to avoid table locks in production Add @disable_ddl_transaction for concurrent index creation

…71/cornucopia into fix/vote-race-condition

Replace invalid 'return' keyword with proper 'with' statement Elixir doesn't have return - use with/else for early exit pattern

Replace with statement with simpler case for Integer.parse Remove unused DealtCard alias that was causing warnings

Remove @disable_ddl_transaction and concurrently option Wrap DELETE in DO block with exception handling Standard index creation works better with CI/test environments

immortal71 · 2026-02-19T03:56:00Z

its seems test keep failing I will start fresh again ...

immortal71 · 2026-02-19T04:03:59Z

new pr here-#2291

immortal71 and others added 28 commits February 8, 2026 23:50

Update copi.owasp.org/lib/copi_web/endpoint.ex

445b1c7

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update copi.owasp.org/SECURITY.md

8f14ab6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

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

39b7b4a

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Fix gettext test import issue

3f550a6

Moved import CopiWeb.Gettext to module level so functions are available to all tests

Fix test compilation and assertion errors

9185c5b

- 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

Fix IPHelper fallback behavior and enhance test coverage - Return loc…

162f10d

…alhost IP instead of raising exceptions when IP unavailable, add comprehensive LiveView tests for delete/broadcast scenarios

Fix test failures: struct comparison, rate limit test, and database p…

9fcfae0

…assword fallback

Fix LiveView test assertions - handle redirects instead of expecting …

ef9e991

…HTML from successful form submissions

Fix test failures: database ownership, rate limit flash rendering, re…

2a28e9a

…direct assertions, API params, and endpoint config

Fix remaining test failures: API ULID, card filter reloading, redirec…

ad04fd2

…t patterns, and simplify UI tests

Fix all 7 test failures: module name, flash capture from render_submi…

6fc9d4b

…t, redirect patterns, validation tests

Fix 6 test failures: verify rate limiting via state not flash HTML, c…

676a30a

…lear rate limits in all setups

fix: Address bugs found in code review

5714610

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

Merge branch 'master' of https://github.com/OWASP/cornucopia into fea…

f73b693

…t/rate-limit-implementation-clean Resolved conflict in player_live_test.exs by accepting incoming test changes from master

fix: Improve broadcast test to properly verify message handling

24a9400

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.

fix: Simplify connect_info access with safe pattern matching

f05acf3

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

Copilot AI review requested due to automatic review settings February 19, 2026 00:35

immortal71 requested a review from rewtd as a code owner February 19, 2026 00:35

immortal71 requested review from cw-owasp and sydseter as code owners February 19, 2026 00:35

Copilot started reviewing on behalf of immortal71 February 19, 2026 00:36 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

immortal71 and others added 9 commits February 18, 2026 17:07

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

6a18849

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update copi.owasp.org/test/copi_web/router_test.exs

7634545

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Merge branch 'fix/vote-race-condition' of https://github.com/immortal…

788e801

…71/cornucopia into fix/vote-race-condition

Fix invalid return syntax in toggle_vote handler

c800d6b

Replace invalid 'return' keyword with proper 'with' statement Elixir doesn't have return - use with/else for early exit pattern

Simplify integer parsing and remove unused alias

d02539f

Replace with statement with simpler case for Integer.parse Remove unused DealtCard alias that was causing warnings

Fix migration to work in test environment

5d6a850

Remove @disable_ddl_transaction and concurrently option Wrap DELETE in DO block with exception handling Standard index creation works better with CI/test environments

Fix migration for test compatibility

e880c1a

Simplify migration - just create unique index without DELETE

d533510

immortal71 closed this Feb 19, 2026

immortal71 deleted the fix/vote-race-condition branch February 19, 2026 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix race condition in voting system with unique constraint and safe upsert#2289

Fix race condition in voting system with unique constraint and safe upsert#2289
immortal71 wants to merge 38 commits intoOWASP:masterfrom
immortal71:fix/vote-race-condition

immortal71 commented Feb 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

immortal71 commented Feb 19, 2026

Uh oh!

immortal71 commented Feb 19, 2026

Uh oh!

immortal71 commented Feb 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

immortal71 commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Changes

Database Migration

Application Code Refactoring

Testing Strategy

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

immortal71 commented Feb 19, 2026

Uh oh!

immortal71 commented Feb 19, 2026

Uh oh!

immortal71 commented Feb 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

immortal71 commented Feb 19, 2026 •

edited

Loading