From: Andy Allan <git@gravitystorm.co.uk>
Date: Sat, 2 Mar 2024 15:48:54 +0000 (+0000)
Subject: Be paranoid when sending password reset emails
X-Git-Tag: live~680^2
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/4e237db3902fd9cd9d2f55131c8bba2e830e87fd

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.
---

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