Skip to content

Commit 238f73c

Browse files
authored
Merge pull request #146 from basecamp/flavorjones/fix-account-creation-race
Address race condition during "first run" account creation
2 parents 49c0ce4 + 1feb2d9 commit 238f73c

File tree

4 files changed

+35
-1
lines changed

4 files changed

+35
-1
lines changed

app/controllers/first_runs_controller.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ def create
1111
user = FirstRun.create!(user_params)
1212
start_new_session_for user
1313

14+
redirect_to root_url
15+
rescue ActiveRecord::RecordNotUnique
1416
redirect_to root_url
1517
end
1618

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
class AddSingletonConstraintToAccounts < ActiveRecord::Migration[8.2]
2+
def change
3+
add_column :accounts, :singleton_guard, :integer, default: 0, null: false
4+
add_index :accounts, :singleton_guard, unique: true
5+
end
6+
end

db/schema.rb

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

test/controllers/first_runs_controller_test.rb

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,28 @@ class FirstRunsControllerTest < ActionDispatch::IntegrationTest
3030

3131
assert parsed_cookies.signed[:session_token]
3232
end
33+
34+
test "create is not vulnerable to race conditions" do
35+
num_attackers = 5
36+
url = first_run_url
37+
barrier = Concurrent::CyclicBarrier.new(num_attackers)
38+
39+
num_attackers.times.map do |i|
40+
Thread.new do
41+
session = ActionDispatch::Integration::Session.new(Rails.application)
42+
barrier.wait # All threads wait here, then fire simultaneously
43+
44+
session.post url, params: {
45+
user: {
46+
name: "Attacker#{i}",
47+
email_address: "attacker#{i}@example.com",
48+
password: "password123"
49+
}
50+
}
51+
end
52+
end.each(&:join)
53+
54+
assert_equal 1, Account.count, "Race condition allowed #{Account.count} accounts to be created!"
55+
assert_equal 1, User.where(role: :administrator).count, "Race condition allowed multiple admin users!"
56+
end
3357
end

0 commit comments

Comments
 (0)