diff --git a/app/controllers/devise/passwords_controller.rb b/app/controllers/devise/passwords_controller.rb index 3af1f864b7..2174ed3544 100644 --- a/app/controllers/devise/passwords_controller.rb +++ b/app/controllers/devise/passwords_controller.rb @@ -15,11 +15,9 @@ def create self.resource = resource_class.send_reset_password_instructions(resource_params) yield resource if block_given? - if successfully_sent?(resource) - respond_with({}, location: after_sending_reset_password_instructions_path_for(resource_name)) - else - respond_with(resource) - end + # Always show success message to prevent email enumeration + set_flash_message! :notice, :send_instructions + respond_with({}, location: after_sending_reset_password_instructions_path_for(resource_name)) end # GET /resource/password/edit?reset_password_token=abcdef diff --git a/app/controllers/devise/registrations_controller.rb b/app/controllers/devise/registrations_controller.rb index f1292b4d90..7c232269f2 100644 --- a/app/controllers/devise/registrations_controller.rb +++ b/app/controllers/devise/registrations_controller.rb @@ -16,6 +16,32 @@ def new def create build_resource(sign_up_params) + # Check if email already exists when in paranoid mode + if Devise.paranoid + # Apply same transformations as the model would + email_key = resource_class.authentication_keys.first + email_value = resource.send(email_key) + + # Apply case insensitive search if configured + if resource_class.case_insensitive_keys.include?(email_key) + existing_user = resource_class.where("lower(#{email_key}) = ?", email_value.downcase).first + else + existing_user = resource_class.find_by(email_key => email_value) + end + + if existing_user + # Send "already registered" email instead of showing error + devise_mailer.email_already_registered(existing_user).deliver + + # Show same success message as normal registration with confirmation + set_flash_message! :notice, :signed_up_but_unconfirmed + expire_data_after_sign_in! + redirect_to new_session_path(resource_name) + return + end + end + + # Normal registration flow resource.save yield resource if block_given? if resource.persisted? @@ -82,6 +108,10 @@ def cancel protected + def devise_mailer + Devise.mailer + end + def update_needs_confirmation?(resource, previous) resource.respond_to?(:pending_reconfirmation?) && resource.pending_reconfirmation? && diff --git a/app/mailers/devise/mailer.rb b/app/mailers/devise/mailer.rb index e617edcd0b..44e06803de 100644 --- a/app/mailers/devise/mailer.rb +++ b/app/mailers/devise/mailer.rb @@ -26,5 +26,9 @@ def email_changed(record, opts = {}) def password_change(record, opts = {}) devise_mail(record, :password_change, opts) end + + def email_already_registered(record, opts = {}) + devise_mail(record, :email_already_registered, opts) + end end end diff --git a/app/views/devise/mailer/email_already_registered.html.erb b/app/views/devise/mailer/email_already_registered.html.erb new file mode 100644 index 0000000000..0634692a65 --- /dev/null +++ b/app/views/devise/mailer/email_already_registered.html.erb @@ -0,0 +1,11 @@ +

Hello <%= @resource.email %>!

+ +

Someone tried to create an account using your email address, but an account already exists.

+ +

If this was you, you can:

+ + +

If this wasn't you, you can safely ignore this email. Your account is secure.

\ No newline at end of file diff --git a/config/locales/en.yml b/config/locales/en.yml index 260e1c4ba6..b78663df8e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -27,12 +27,14 @@ en: subject: "Email Changed" password_change: subject: "Password Changed" + email_already_registered: + subject: "Account already exists" omniauth_callbacks: failure: "Could not authenticate you from %{kind} because \"%{reason}\"." success: "Successfully authenticated from %{kind} account." passwords: no_token: "You can't access this page without coming from a password reset email. If you do come from a password reset email, please make sure you used the full URL provided." - send_instructions: "You will receive an email with instructions on how to reset your password in a few minutes." + send_instructions: "If your email address exists in our database, you will receive a password recovery link at your email address in a few minutes." 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." updated: "Your password has been changed successfully. You are now signed in." updated_not_active: "Your password has been changed successfully." diff --git a/lib/devise/models/recoverable.rb b/lib/devise/models/recoverable.rb index b17c42aae6..e20c7e9db5 100644 --- a/lib/devise/models/recoverable.rb +++ b/lib/devise/models/recoverable.rb @@ -118,11 +118,23 @@ def with_reset_password_token(token) # Attempt to find a user by its email. If a record is found, send new # password instructions to it. If user is not found, returns a new user - # with an email not found error. + # with no errors to prevent email enumeration attacks. # Attributes must contain the user's email def send_reset_password_instructions(attributes = {}) recoverable = find_or_initialize_with_errors(reset_password_keys, attributes, :not_found) - recoverable.send_reset_password_instructions if recoverable.persisted? + + if recoverable.persisted? + recoverable.send_reset_password_instructions + else + # Perform similar work to mitigate timing attacks + # Generate a token even though we won't use it + Devise.friendly_token(20) + + # Always return a new user with no errors to prevent email enumeration + recoverable = new + recoverable.errors.clear + end + recoverable end diff --git a/mise.toml b/mise.toml new file mode 100644 index 0000000000..3bc88e60d6 --- /dev/null +++ b/mise.toml @@ -0,0 +1,2 @@ +[tools] +ruby = "3.1.6" diff --git a/test/integration/recoverable_test.rb b/test/integration/recoverable_test.rb index c391b0b2eb..f374220d21 100644 --- a/test/integration/recoverable_test.rb +++ b/test/integration/recoverable_test.rb @@ -52,7 +52,7 @@ def reset_password(options = {}, &block) end assert_current_url '/users/sign_in' - assert_contain 'You will receive an email with instructions on how to reset your password in a few minutes.' + assert_contain 'If your email address exists in our database' end test 'reset password with email should send an email from a custom mailer' do @@ -68,7 +68,7 @@ def reset_password(options = {}, &block) assert_match edit_user_password_path(reset_password_token: 'abcdef'), mail.body.encoded end - test 'reset password with email of different case should fail when email is NOT the list of case insensitive keys' do + test 'reset password with email of different case should show success message when email is NOT the list of case insensitive keys' do swap Devise, case_insensitive_keys: [] do create_user(email: 'Foo@Bar.com') @@ -76,10 +76,9 @@ def reset_password(options = {}, &block) fill_in 'email', with: 'foo@bar.com' end - assert_response :success - assert_current_url '/users/password' - assert_have_selector "input[type=email][value='foo@bar.com']" - assert_contain 'not found' + # Now always shows success to prevent email enumeration + assert_current_url '/users/sign_in' + assert_contain 'If your email address exists in our database' end end @@ -91,10 +90,10 @@ def reset_password(options = {}, &block) end assert_current_url '/users/sign_in' - assert_contain 'You will receive an email with instructions on how to reset your password in a few minutes.' + assert_contain 'If your email address exists in our database' end - test 'reset password with email with extra whitespace should fail when email is NOT the list of strip whitespace keys' do + test 'reset password with email with extra whitespace should show success message when email is NOT the list of strip whitespace keys' do swap Devise, strip_whitespace_keys: [] do create_user(email: 'foo@bar.com') @@ -102,10 +101,9 @@ def reset_password(options = {}, &block) fill_in 'email', with: ' foo@bar.com ' end - assert_response :success - assert_current_url '/users/password' - assert_have_selector "input[type=email][value=' foo@bar.com ']" - assert_contain 'not found' + # Now always shows success to prevent email enumeration + assert_current_url '/users/sign_in' + assert_contain 'If your email address exists in our database' end end @@ -124,18 +122,17 @@ def reset_password(options = {}, &block) request_forgot_password assert_current_url '/users/sign_in' - assert_contain 'You will receive an email with instructions on how to reset your password in a few minutes.' + assert_contain 'If your email address exists in our database' end - test 'not authenticated user with invalid email should receive an error message' do + test 'not authenticated user with invalid email should still see success message' do request_forgot_password do fill_in 'email', with: 'invalid.test@test.com' end - assert_response :success - assert_current_url '/users/password' - assert_have_selector "input[type=email][value='invalid.test@test.com']" - assert_contain 'not found' + # Now always shows success to prevent email enumeration + assert_current_url '/users/sign_in' + assert_contain 'If your email address exists in our database' end test 'authenticated user should not be able to visit edit password page' do @@ -293,14 +290,15 @@ def reset_password(options = {}, &block) assert_equal({}.to_json, response.body) end - test 'reset password request with invalid e-mail in JSON format should return valid response' do + test 'reset password request with invalid e-mail in JSON format should return success response' do create_user post user_password_path(format: 'json'), params: { user: {email: "invalid.test@test.com"} } - assert_response :unprocessable_entity - assert_includes response.body, '{"errors":{' + # Now always returns success to prevent email enumeration + assert_response :success + assert_equal({}.to_json, response.body) end - test 'reset password request with invalid e-mail in JSON format should return empty and valid response in paranoid mode' do + test 'reset password request with invalid e-mail in JSON format also returns success in paranoid mode' do swap Devise, paranoid: true do create_user post user_password_path(format: 'json'), params: { user: {email: "invalid@test.com"} } diff --git a/test/integration/registerable_test.rb b/test/integration/registerable_test.rb index 038fcf7b91..2f3060b6eb 100644 --- a/test/integration/registerable_test.rb +++ b/test/integration/registerable_test.rb @@ -138,6 +138,31 @@ def user_sign_up assert_not warden.authenticated?(:user) end + test 'in paranoid mode, duplicate registration shows success message and sends email to existing user' do + swap Devise, paranoid: true do + create_user + get new_user_registration_path + + fill_in 'email', with: 'user@test.com' + fill_in 'password', with: '123456' + fill_in 'password confirmation', with: '123456' + + assert_email_sent 'user@test.com' do + click_button 'Sign up' + end + + # Should redirect to sign in page with success message + assert_current_url '/users/sign_in' + assert_contain 'A message with a confirmation link has been sent to your email address' + assert_not warden.authenticated?(:user) + + # Check that the email sent is the "already registered" email + mail = ActionMailer::Base.deliveries.last + assert_equal 'Account already exists', mail.subject + assert_match 'Someone tried to create an account using your email address', mail.body.encoded + end + end + test 'a guest should not be able to change account' do get edit_user_registration_path assert_redirected_to new_user_session_path diff --git a/test/models/recoverable_test.rb b/test/models/recoverable_test.rb index b2234ac6ac..1cadf3c034 100644 --- a/test/models/recoverable_test.rb +++ b/test/models/recoverable_test.rb @@ -119,10 +119,11 @@ def setup assert_equal user, reset_password_user end - test 'should return a new record with errors if user was not found by e-mail' do + test 'should return a new record with no errors if user was not found by e-mail' do reset_password_user = User.send_reset_password_instructions(email: "invalid@example.com") assert_not reset_password_user.persisted? - assert_equal "not found", reset_password_user.errors[:email].join + # No longer exposes email not found to prevent enumeration + assert reset_password_user.errors.empty? end test 'should find a user to send instructions by authentication_keys' do @@ -138,7 +139,8 @@ def setup user = create_user reset_password_user = User.send_reset_password_instructions(email: user.email) assert_not reset_password_user.persisted? - assert reset_password_user.errors.added?(:username, :blank) + # No longer shows specific errors to prevent enumeration + assert reset_password_user.errors.empty? end end @@ -260,4 +262,28 @@ def setup assert_nil User.with_reset_password_token('random-token') end + test 'should not expose email existence by default' do + user = User.send_reset_password_instructions(email: 'nonexistent@example.com') + assert_not user.persisted? + assert user.errors.empty? + end + + test 'should send reset instructions only for existing users' do + existing_user = create_user + + # Should send email for existing user + assert_email_sent do + user = User.send_reset_password_instructions(email: existing_user.email) + assert user.persisted? + assert user.errors.empty? + end + + # Should not send email for non-existing user + assert_email_not_sent do + user = User.send_reset_password_instructions(email: 'nonexistent@example.com') + assert_not user.persisted? + assert user.errors.empty? + end + end + end