consuldemocracy icon indicating copy to clipboard operation
consuldemocracy copied to clipboard

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

Open dreadlocked opened this issue 6 years ago • 1 comments

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 avatar Feb 01 '18 23:02 dreadlocked

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 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 :thinking:)
  6. Release another version of CONSUL

javierm avatar Jun 27 '22 14:06 javierm