Merge branch 'devise_paranoid_mode' into 'master'
Enable Devise paranoid mode and ensure the returned message is the same every time. This will prevent user enumeration (low impact). Prior to this change a user could type an email in the password reset field and if the email didn't exist it returned an error. If the email was valid it returned a message saying the forgot password link had been emailed. After this change the user will receive a message that if the email is in our database the reset link will be emailed. I also changed the throttle mechanism so it still works the same but now returns the exact same message as above. Previously it would say 'You've already sent a request. Wait a few minutes'. This also allows user enumeration, although it requires a double-check. Related to https://dev.gitlab.org/gitlab/gitlabhq/issues/2624 See merge request !2044
This commit is contained in:
commit
ba79d1e5b8
5 changed files with 21 additions and 17 deletions
|
@ -33,6 +33,9 @@ v 8.2.3
|
|||
- Fix Error 500s when creating global milestones with Unicode characters (Stan Hu)
|
||||
- Update documentation for "Guest" permissions
|
||||
- Properly convert Emoji-only comments into Award Emojis
|
||||
- Enable devise paranoid mode to prevent user enumeration attack
|
||||
|
||||
v 8.2.3
|
||||
- Webhook payload has an added, modified and removed properties for each commit
|
||||
- Fix 500 error when creating a merge request that removes a submodule
|
||||
|
||||
|
|
|
@ -40,7 +40,9 @@ class PasswordsController < Devise::PasswordsController
|
|||
def throttle_reset
|
||||
return unless resource && resource.recently_sent_password_reset?
|
||||
|
||||
redirect_to new_password_path(resource_name),
|
||||
alert: I18n.t('devise.passwords.recently_reset')
|
||||
# Throttle reset attempts, but return a normal message to
|
||||
# avoid user enumeration attack.
|
||||
redirect_to new_user_session_path,
|
||||
notice: I18n.t('devise.passwords.send_paranoid_instructions')
|
||||
end
|
||||
end
|
||||
|
|
|
@ -60,7 +60,7 @@ Devise.setup do |config|
|
|||
# It will change confirmation, password recovery and other workflows
|
||||
# to behave the same regardless if the e-mail provided was right or wrong.
|
||||
# Does not affect registerable.
|
||||
# config.paranoid = true
|
||||
config.paranoid = true
|
||||
|
||||
# ==> Configuration for :database_authenticatable
|
||||
# For bcrypt, this is the cost for hashing the password and defaults to 10. If
|
||||
|
|
|
@ -30,7 +30,6 @@ en:
|
|||
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."
|
||||
recently_reset: "Instructions about how to reset your password have already been sent recently. Please wait a few minutes to try again."
|
||||
send_instructions: "You will receive an email with instructions on how to reset your password 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."
|
||||
|
|
|
@ -3,11 +3,12 @@ require 'spec_helper'
|
|||
feature 'Password reset', feature: true do
|
||||
describe 'throttling' do
|
||||
it 'sends reset instructions when not previously sent' do
|
||||
visit root_path
|
||||
forgot_password(create(:user))
|
||||
user = create(:user)
|
||||
forgot_password(user)
|
||||
|
||||
expect(page).to have_content(I18n.t('devise.passwords.send_instructions'))
|
||||
expect(page).to have_content(I18n.t('devise.passwords.send_paranoid_instructions'))
|
||||
expect(current_path).to eq new_user_session_path
|
||||
expect(user.recently_sent_password_reset?).to be_truthy
|
||||
end
|
||||
|
||||
it 'sends reset instructions when previously sent more than a minute ago' do
|
||||
|
@ -15,26 +16,25 @@ feature 'Password reset', feature: true do
|
|||
user.send_reset_password_instructions
|
||||
user.update_attribute(:reset_password_sent_at, 5.minutes.ago)
|
||||
|
||||
visit root_path
|
||||
forgot_password(user)
|
||||
|
||||
expect(page).to have_content(I18n.t('devise.passwords.send_instructions'))
|
||||
expect{ forgot_password(user) }.to change{ user.reset_password_sent_at }
|
||||
expect(page).to have_content(I18n.t('devise.passwords.send_paranoid_instructions'))
|
||||
expect(current_path).to eq new_user_session_path
|
||||
end
|
||||
|
||||
it "throttles multiple resets in a short timespan" do
|
||||
it 'throttles multiple resets in a short timespan' do
|
||||
user = create(:user)
|
||||
user.send_reset_password_instructions
|
||||
# Reload because PG handles datetime less precisely than Ruby/Rails
|
||||
user.reload
|
||||
|
||||
visit root_path
|
||||
forgot_password(user)
|
||||
|
||||
expect(page).to have_content(I18n.t('devise.passwords.recently_reset'))
|
||||
expect(current_path).to eq new_user_password_path
|
||||
expect{ forgot_password(user) }.not_to change{ user.reset_password_sent_at }
|
||||
expect(page).to have_content(I18n.t('devise.passwords.send_paranoid_instructions'))
|
||||
expect(current_path).to eq new_user_session_path
|
||||
end
|
||||
end
|
||||
|
||||
def forgot_password(user)
|
||||
visit root_path
|
||||
click_on 'Forgot your password?'
|
||||
fill_in 'Email', with: user.email
|
||||
click_button 'Reset password'
|
||||
|
|
Loading…
Reference in a new issue