diff --git a/CHANGELOG.md b/CHANGELOG.md index 14fc8bbd4..4f8ea035f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +### Unreleased + +* security fixes + * Fix race condition vulnerability on confirmable "change email" which would allow confirming an email they don't own [#5783](https://github.com/heartcombo/devise/pull/5783) [#5784](https://github.com/heartcombo/devise/pull/5784) + ### 5.0.2 - 2026-02-18 * enhancements diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index 6ce22c30f..1930086aa 100644 --- a/lib/devise/models/confirmable.rb +++ b/lib/devise/models/confirmable.rb @@ -258,9 +258,11 @@ def generate_confirmation_token! generate_confirmation_token && save(validate: false) end - def postpone_email_change_until_confirmation_and_regenerate_confirmation_token @reconfirmation_required = true + # Force unconfirmed_email to be updated, even if the value hasn't changed, to prevent a + # race condition which could allow an attacker to confirm an email they don't own. See #5783. + devise_unconfirmed_email_will_change! self.unconfirmed_email = self.email self.email = self.devise_email_in_database self.confirmation_token = nil diff --git a/lib/devise/orm.rb b/lib/devise/orm.rb index 3f3ac86db..f00f397f0 100644 --- a/lib/devise/orm.rb +++ b/lib/devise/orm.rb @@ -35,6 +35,10 @@ def devise_will_save_change_to_email? will_save_change_to_email? end + def devise_unconfirmed_email_will_change! + unconfirmed_email_will_change! + end + def devise_respond_to_and_will_save_change_to_attribute?(attribute) respond_to?("will_save_change_to_#{attribute}?") && send("will_save_change_to_#{attribute}?") end @@ -61,6 +65,13 @@ def devise_will_save_change_to_email? email_changed? end + def devise_unconfirmed_email_will_change! + # Mongoid's will_change! doesn't force unchanged attributes into updates, + # so we override changed_attributes to make it see a difference. + unconfirmed_email_will_change! + changed_attributes["unconfirmed_email"] = nil + end + def devise_respond_to_and_will_save_change_to_attribute?(attribute) respond_to?("#{attribute}_changed?") && send("#{attribute}_changed?") end diff --git a/test/integration/confirmable_test.rb b/test/integration/confirmable_test.rb index 8e6f68ef2..f9185e87f 100644 --- a/test/integration/confirmable_test.rb +++ b/test/integration/confirmable_test.rb @@ -354,4 +354,32 @@ def visit_admin_confirmation_with_token(confirmation_token) assert_contain(/Email.*already.*taken/) assert admin.reload.pending_reconfirmation? end + + test 'concurrent "update email" requests should not allow confirming a victim email address' do + attacker_email = "attacker@example.com" + victim_email = "victim@example.com" + + attacker = create_admin + # update the email address of the attacker, but do not confirm it yet + attacker.update!(email: attacker_email) + + # A new request starts, to update the unconfirmed email again. + attacker = Admin.find_by(id: attacker.id) + + # A concurrent request also updates the email address to the victim, while the `attacker` request's model is in memory + Admin.where(id: attacker.id).update_all( + unconfirmed_email: victim_email, + confirmation_token: "different token" + ) + + # Now the attacker updates to the same prior unconfirmed email address, and confirm. + # This should update the `unconfirmed_email` in the database, even though it is unchanged from the models point of view. + attacker.update!(email: attacker_email) + attacker_token = attacker.raw_confirmation_token + visit_admin_confirmation_with_token(attacker_token) + + attacker.reload + assert attacker.confirmed? + assert_equal attacker_email, attacker.email + end end