Skip to content

fix(copi): prevent duplicate votes under concurrent requests - #3465

Open
prajakta128 wants to merge 7 commits into
OWASP:masterfrom
prajakta128:fix/2288-vote-race-condition
Open

fix(copi): prevent duplicate votes under concurrent requests#3465
prajakta128 wants to merge 7 commits into
OWASP:masterfrom
prajakta128:fix/2288-vote-race-condition

Conversation

@prajakta128

Copy link
Copy Markdown
Contributor

Fixes #2288

Problem

The voting system used a check-then-act pattern: check whether a vote exists, then separately insert or delete it. Concurrent requests could both pass the check before either had written, creating duplicate votes. votesalso had no unique constraint backing(player_id, dealt_card_id)`.

Changes

  • New migration: de-duplicates any existing duplicate vote rows, then adds a unique index on votes(player_id, dealt_card_id).
  • Added a changeset/2 to Copi.Cornucopia.Vote (previously missing).
  • toggle_vote and toggle_continue_vote now use conflict-safeRepo.delete_all/Repo.insert(on_conflict: :nothing, conflict_target: ...) instead of racy check-then-act, matching the pattern continue_votes already partially had.
  • Added regression tests simulating concurrent removal/insert races for both card votes and continue votes.

Testing

  • Full suite passes: 510 tests, 0 failures.
  • Coverage: 95.1% (meets the 95% threshold).
  • Manually verified normal vote/un-vote and continue-vote behavior still works.

Related

Supersedes #3332 and #3455, which were closed for insufficient coverage nd a build failure respectively this PR adds the missing race-branch tests and a unique DB constraint that #3332 lacked.

Note

Apologies for the delay and the two earlier closed attempts on this one — I've had college coursework and exams running alongside this, which slowed things down. This version has been tested more thoroughly and should be in better shape. Happy to make any further changes needed.

@prajakta128

Copy link
Copy Markdown
Contributor Author

The failing build-test-copi check looks unrelated to this PR — the Docker build fails trying to fetch libc-l10n from the Debian security mirror (404, package appears to have been pulled/moved at that pinned version):

E: Failed to fetch http://deb.debian.org/debian-security/.../libc-l10n_2.31-13+deb11u14_all.deb 404 Not Found

This happens during apt-get install in the Dockerfile, before any Elixir code or tests run, so I don't believe it's something introduced by this change. Happy to help if a Dockerfile fix is wanted separately — let me know if you'd like me to look into pinning a different base image or package version.

@sydseter

sydseter commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks' I'll have a look asap

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.

🟡 Changes recommended

New tests introduce avoidable global Application env leakage and some minor clarity/perf issues that should be addressed to keep the suite reliable and maintainable.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens copi.owasp.org’s voting system against concurrent request races by moving uniqueness guarantees into the database (unique index) and by switching vote toggling to conflict-safe delete_all + insert(on_conflict: :nothing) patterns.

Changes:

  • Adds a migration that removes duplicate votes rows and then enforces uniqueness on (player_id, dealt_card_id).
  • Introduces Copi.Cornucopia.Vote.changeset/2 and updates LiveView vote/continue-vote toggles to be concurrency-safe.
  • Adds/extends tests covering vote changesets and regressions for conflict/race branches.
File summaries
File Description
copi.owasp.org/priv/repo/migrations/20260907033917_add_unique_constraint_to_votes.exs De-dupes existing votes and adds a unique index to prevent future duplicates.
copi.owasp.org/lib/copi/cornucopia/vote.ex Adds a changeset/2 with required fields + unique constraint mapping.
copi.owasp.org/lib/copi_web/live/player_live/show.ex Replaces racy check-then-act vote toggles with conflict-safe delete_all/insert ... on_conflict.
copi.owasp.org/test/copi_web/live/player_live/show_test.exs Adds regression coverage for vote/continue-vote race branches and related redirect/validation behavior.
copi.owasp.org/test/copi/cornucopia/vote_test.exs Adds changeset validation tests for Vote.
copi.owasp.org/test/copi_web/live/sponsors_live/index_test.exs Adds a basic LiveView render test for /sponsors.
Review details

Suppressed comments (1)

copi.owasp.org/test/copi_web/live/player_live/show_test.exs:553

  • Same as above: the fixed sleep after render_click/3 is probably redundant and adds avoidable runtime/flakiness.
      render_click(show_live, "toggle_continue_vote", %{})
      :timer.sleep(100)

  • Files reviewed: 6/6 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread copi.owasp.org/test/copi_web/live/player_live/show_test.exs
Comment thread copi.owasp.org/test/copi_web/live/player_live/show_test.exs Outdated
Comment thread copi.owasp.org/test/copi_web/live/player_live/show_test.exs
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.

Race condition in voting system allows duplicate votes and data corruption

3 participants