From 4e237db3902fd9cd9d2f55131c8bba2e830e87fd Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Sat, 2 Mar 2024 15:48:54 +0000 Subject: [PATCH] Be paranoid when sending password reset emails This implements what is known as "paranoid" password reset flash messages (using the terminology from Devise). It avoids revealing whether the supplied email address is already registered. Added an explicit test for this situation, so that the test for email non-existance is separate from the duplicate-case tests. --- app/controllers/passwords_controller.rb | 8 +++--- config/locales/en.yml | 5 ++-- test/controllers/passwords_controller_test.rb | 26 ++++++++++++++----- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index 8025fd700..25b2b9607 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -43,12 +43,10 @@ class PasswordsController < ApplicationController if user token = user.generate_token_for(:password_reset) UserMailer.lost_password(user, token).deliver_later - flash[:notice] = t ".notice email on way" - redirect_to login_path - else - flash.now[:error] = t ".notice email cannot find" - render :new end + + flash[:notice] = t ".send_paranoid_instructions" + redirect_to login_path end def update diff --git a/config/locales/en.yml b/config/locales/en.yml index 429786fcc..0cc35863b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1777,7 +1777,7 @@ en: wrong_user: "You are logged in as `%{user}' but the message you have asked to read was not sent by or to that user. Please login as the correct user in order to read it." sent_message_summary: destroy_button: "Delete" - heading: + heading: my_inbox: "My Inbox" my_outbox: "My Outbox" muted_messages: "Muted messages" @@ -1797,8 +1797,7 @@ en: new password button: "Reset password" help_text: "Enter the email address you used to sign up, we will send a link to it that you can use to reset your password." create: - notice email on way: "Sorry you lost it :-( but an email is on its way so you can reset it soon." - notice email cannot find: "Could not find that email address, sorry." + send_paranoid_instructions: "If your email address exists in our database, you will receive a password recovery link at your email address in a few minutes." edit: title: "Reset password" heading: "Reset Password for %{user}" diff --git a/test/controllers/passwords_controller_test.rb b/test/controllers/passwords_controller_test.rb index 25cfdd4e5..24d3f1893 100644 --- a/test/controllers/passwords_controller_test.rb +++ b/test/controllers/passwords_controller_test.rb @@ -51,12 +51,23 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest end assert_response :redirect assert_redirected_to login_path - assert_match(/^Sorry you lost it/, flash[:notice]) + assert_match(/^If your email address exists/, flash[:notice]) email = ActionMailer::Base.deliveries.first assert_equal 1, email.to.count assert_equal user.email, email.to.first ActionMailer::Base.deliveries.clear + # Test resetting using an address that does not exist + assert_no_difference "ActionMailer::Base.deliveries.size" do + perform_enqueued_jobs do + post user_forgot_password_path, :params => { :email => "nobody@example.com" } + end + end + # Be paranoid about revealing there was no match + assert_response :redirect + assert_redirected_to login_path + assert_match(/^If your email address exists/, flash[:notice]) + # Test resetting using an address that matches a different user # that has the same address in a different case assert_difference "ActionMailer::Base.deliveries.size", 1 do @@ -66,7 +77,7 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest end assert_response :redirect assert_redirected_to login_path - assert_match(/^Sorry you lost it/, flash[:notice]) + assert_match(/^If your email address exists/, flash[:notice]) email = ActionMailer::Base.deliveries.first assert_equal 1, email.to.count assert_equal uppercase_user.email, email.to.first @@ -79,9 +90,10 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest post user_forgot_password_path, :params => { :email => user.email.titlecase } end end - assert_response :success - assert_template :new - assert_select ".alert.alert-danger", /^Could not find that email address/ + # Be paranoid about revealing there was no match + assert_response :redirect + assert_redirected_to login_path + assert_match(/^If your email address exists/, flash[:notice]) # Test resetting using the address as recorded for a user that has an # address which is case insensitively unique @@ -93,7 +105,7 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest end assert_response :redirect assert_redirected_to login_path - assert_match(/^Sorry you lost it/, flash[:notice]) + assert_match(/^If your email address exists/, flash[:notice]) email = ActionMailer::Base.deliveries.first assert_equal 1, email.to.count assert_equal third_user.email, email.to.first @@ -108,7 +120,7 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest end assert_response :redirect assert_redirected_to login_path - assert_match(/^Sorry you lost it/, flash[:notice]) + assert_match(/^If your email address exists/, flash[:notice]) email = ActionMailer::Base.deliveries.first assert_equal 1, email.to.count assert_equal third_user.email, email.to.first -- 2.39.5