diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index 6ce22c30f..c920e7e32 100644 --- a/lib/devise/models/confirmable.rb +++ b/lib/devise/models/confirmable.rb @@ -262,6 +262,7 @@ def generate_confirmation_token! def postpone_email_change_until_confirmation_and_regenerate_confirmation_token @reconfirmation_required = true self.unconfirmed_email = self.email + unconfirmed_email_will_change! self.email = self.devise_email_in_database self.confirmation_token = nil generate_confirmation_token diff --git a/test/integration/confirmable_test.rb b/test/integration/confirmable_test.rb index c951eb0bb..6231ab7c6 100644 --- a/test/integration/confirmable_test.rb +++ b/test/integration/confirmable_test.rb @@ -345,4 +345,33 @@ 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