Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Race Condition due to lack of UNIQUE constraint on "votes" table #2415

Open
dreadlocked opened this issue Feb 1, 2018 · 1 comment
Open
Labels
Bug help wanted security Pull requests that address a security vulnerability

Comments

@dreadlocked
Copy link

dreadlocked commented Feb 1, 2018

By default, the tuple (user_id, proposal_id) is not UNIQUE on votes table. This makes ActAsVotable vulnerable to race condition, where the same user can vote twice (or more) the same proposal.

ActAsVotable code:

votes = find_votes_by(options[:voter], options[:vote_scope])

if votes.count == (0) || options[:duplicate]
	# this voter has never voted
	vote = ActsAsVotable::Vote.new(
  	votable: self,
  	voter: options[:voter],
  	vote_scope: options[:vote_scope]
	)
else
	# this voter is potentially changing his vote
	vote = votes.last

A thread can request database if a voter have already voted a proposal while the first thread have not executed insert statement yet leading to a race condition where the database responds votes.count = 0, the second thread will then enter into the condition and insert a second register because the database definition permits this behavior by default.

@dreadlocked dreadlocked changed the title Race Condition due to lack of UNIQUE constrain on "votes" table Race Condition due to lack of UNIQUE constraint on "votes" table Feb 1, 2018
@javierm javierm added security Pull requests that address a security vulnerability Bug labels Oct 20, 2019
@javierm
Copy link
Member

javierm commented Jun 27, 2022

A possible plan for fixing this issue:

  1. Avoid duplicate votes in the future, maybe by locking current_user every time we use vote_by (note: I'm not sure this is the best option)
  2. Add a task removing existing duplicate votes
  3. Release a version of CONSUL so duplicate votes are removed before the unique index is added
  4. Add a unique index to the votes table or upgrade to a version of acts_as_votable implementing this restriction (see Fix races that lead to duplicate votes creation ryanto/acts_as_votable#211)
  5. Remove the current_user locks in the code (we might also look for ways to recover from a RecordNotUnique exception 🤔)
  6. Release another version of CONSUL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug help wanted security Pull requests that address a security vulnerability
Projects
Status: Pending (no particular order)
Consul Democracy
  
Pending (no particular order)
Development

No branches or pull requests

2 participants