SECURITY: Delete email tokens when a user's email is changed or deleted (#19736)

Co-authored-by: OsamaSayegh <asooomaasoooma90@gmail.com>

app/models/user_email.rb

@@ -18,6 +18,10 @@ class UserEmail < ActiveRecord::Base
 
   scope :secondary, -> { where(primary: false) }
 
+  before_save ->() { destroy_email_tokens(self.email_was) }, if: :will_save_change_to_email?
+
+  after_destroy { destroy_email_tokens(self.email) }
+
   def normalize_email
     self.normalized_email = if self.email.present?
       username, domain = self.email.split('@', 2)
@@ -62,6 +66,10 @@ class UserEmail < ActiveRecord::Base
       )
     end
   end
+
+  def destroy_email_tokens(email)
+    EmailToken.where(email: email).destroy_all
+  end
 end
 
 # == Schema Information

spec/requests/users_controller_spec.rb

@@ -3063,17 +3063,18 @@ describe UsersController do
       expect(user1.email_change_requests).to contain_exactly(request_1)
     end
 
-    it "can destroy associated email tokens" do
+    it "destroys associated email tokens and email change requests" do
       new_email = 'new.n.cool@example.com'
       updater = EmailUpdater.new(guardian: user1.guardian, user: user1)
+      updater.change_to(new_email)
 
-      expect { updater.change_to(new_email) }
+      email_token = updater.change_req.new_email_token
-        .to change { user1.email_tokens.count }.by(1)
+      expect(email_token).to be_present
 
-      expect { delete "/u/#{user1.username}/preferences/email.json", params: { email: new_email } }
+      delete "/u/#{user1.username}/preferences/email.json", params: { email: new_email }
-        .to change { user1.email_tokens.count }.by(-1)
 
-      expect(user1.email_tokens.first.email).to eq(user1.email)
+      expect(EmailToken.find_by(id: email_token.id)).to eq(nil)
+      expect(EmailChangeRequest.find_by(id: updater.change_req.id)).to eq(nil)
     end
   end
 
@@ -3439,8 +3440,7 @@ describe UsersController do
         expect(user.email).to eq('updatedemail@example.com')
         expect(user.email_tokens.where(email: 'updatedemail@example.com', expired: false)).to be_present
 
-        token.reload
+        expect(EmailToken.find_by(id: token.id)).to eq(nil)
-        expect(token.expired?).to eq(true)
       end
 
       it 'tells the user to slow down after many requests' do
@@ -3530,8 +3530,7 @@ describe UsersController do
         expect(user.email).to eq('updatedemail@example.com')
         expect(user.email_tokens.where(email: 'updatedemail@example.com', expired: false)).to be_present
 
-        token.reload
+        expect(EmailToken.find_by(id: token.id)).to eq(nil)
-        expect(token.expired?).to eq(true)
       end
 
       it 'tells the user to slow down after many requests' do

spec/requests/users_email_controller_spec.rb

@@ -244,6 +244,28 @@ describe UsersEmailController do
         end
       end
     end
+
+    it "destroys email tokens associated with the old email after the new email is confirmed" do
+      SiteSetting.enable_secondary_emails = true
+
+      email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:password_reset])
+
+      updater = EmailUpdater.new(guardian: user.guardian, user: user)
+      updater.change_to('bubblegum@adventuretime.ooo')
+
+      sign_in(user)
+      put "/u/confirm-new-email", params: {
+        token: "#{updater.change_req.new_email_token.token}"
+      }
+
+      new_password = SecureRandom.hex
+      put "/u/password-reset/#{email_token.token}.json", params: {
+        password: new_password
+      }
+      expect(response.parsed_body["success"]).to eq(false)
+      expect(response.parsed_body["message"]).to eq(I18n.t("password_reset.no_token", base_url: Discourse.base_url))
+      expect(user.reload.confirm_password?(new_password)).to eq(false)
+    end
   end
 
   describe '#confirm-old-email' do