Updated password reset process not to indicate if email exists

- Intended to prevent enumeration to check if a user exists.
- Updated messages on both the reqest-reset and set-password elements.
- Also updated notification auto-hide to be dynamic based upon the
amount of words within the notification.
- Added tests to cover.

For #2016
This commit is contained in:
Dan Brown 2020-04-10 13:38:08 +01:00
parent 053cbbd5b6
commit ba1be9d710
No known key found for this signature in database
GPG Key ID: 46D9F943C24A2EF9
6 changed files with 63 additions and 13 deletions

View File

@ -5,7 +5,7 @@ namespace BookStack\Http\Controllers\Auth;
use BookStack\Http\Controllers\Controller;
use Illuminate\Foundation\Auth\SendsPasswordResetEmails;
use Illuminate\Http\Request;
use Password;
use Illuminate\Support\Facades\Password;
class ForgotPasswordController extends Controller
{
@ -52,8 +52,8 @@ class ForgotPasswordController extends Controller
$request->only('email')
);
if ($response === Password::RESET_LINK_SENT) {
$message = trans('auth.reset_password_sent_success', ['email' => $request->get('email')]);
if ($response === Password::RESET_LINK_SENT || $response === Password::INVALID_USER) {
$message = trans('auth.reset_password_sent', ['email' => $request->get('email')]);
$this->showSuccessNotification($message);
return back()->with('status', trans($response));
}

View File

@ -5,6 +5,7 @@ namespace BookStack\Http\Controllers\Auth;
use BookStack\Http\Controllers\Controller;
use Illuminate\Foundation\Auth\ResetsPasswords;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Password;
class ResetPasswordController extends Controller
{
@ -49,4 +50,24 @@ class ResetPasswordController extends Controller
return redirect($this->redirectPath())
->with('status', trans($response));
}
/**
* Get the response for a failed password reset.
*
* @param \Illuminate\Http\Request $request
* @param string $response
* @return \Illuminate\Http\RedirectResponse|\Illuminate\Http\JsonResponse
*/
protected function sendResetFailedResponse(Request $request, $response)
{
// We show invalid users as invalid tokens as to not leak what
// users may exist in the system.
if ($response === Password::INVALID_USER) {
$response = Password::INVALID_TOKEN;
}
return redirect()->back()
->withInput($request->only('email'))
->withErrors(['email' => trans($response)]);
}
}

View File

@ -28,7 +28,11 @@ class Notification {
this.elem.classList.add('showing');
}, 1);
if (this.autohide) setTimeout(this.hide.bind(this), 2000);
if (this.autohide) {
const words = textToShow.split(' ').length;
const timeToShow = Math.max(2000, 1000 + (250 * words));
setTimeout(this.hide.bind(this), timeToShow);
}
}
hide() {

View File

@ -43,7 +43,7 @@ return [
'reset_password' => 'Reset Password',
'reset_password_send_instructions' => 'Enter your email below and you will be sent an email with a password reset link.',
'reset_password_send_button' => 'Send Reset Link',
'reset_password_sent_success' => 'A password reset link has been sent to :email.',
'reset_password_sent' => 'A password reset link will be sent to :email if that email address is found in the system.',
'reset_password_success' => 'Your password has been successfully reset.',
'email_reset_subject' => 'Reset your :appName password',
'email_reset_text' => 'You are receiving this email because we received a password reset request for your account.',

View File

@ -8,7 +8,7 @@ return [
'password' => 'Passwords must be at least eight characters and match the confirmation.',
'user' => "We can't find a user with that e-mail address.",
'token' => 'This password reset token is invalid.',
'token' => 'The password reset token is invalid for this email address.',
'sent' => 'We have e-mailed your password reset link!',
'reset' => 'Your password has been reset!',

View File

@ -1,9 +1,13 @@
<?php namespace Tests\Auth;
use BookStack\Auth\Role;
use BookStack\Auth\User;
use BookStack\Entities\Page;
use BookStack\Notifications\ConfirmEmail;
use BookStack\Notifications\ResetPassword;
use BookStack\Settings\SettingService;
use DB;
use Hash;
use Illuminate\Support\Facades\Notification;
use Tests\BrowserKitTest;
@ -129,7 +133,7 @@ class AuthTest extends BrowserKitTest
->press('Resend Confirmation Email');
// Get confirmation and confirm notification matches
$emailConfirmation = \DB::table('email_confirmations')->where('user_id', '=', $dbUser->id)->first();
$emailConfirmation = DB::table('email_confirmations')->where('user_id', '=', $dbUser->id)->first();
Notification::assertSentTo($dbUser, ConfirmEmail::class, function($notification, $channels) use ($emailConfirmation) {
return $notification->token === $emailConfirmation->token;
});
@ -257,7 +261,7 @@ class AuthTest extends BrowserKitTest
->seePageIs('/settings/users');
$userPassword = User::find($user->id)->password;
$this->assertTrue(\Hash::check('newpassword', $userPassword));
$this->assertTrue(Hash::check('newpassword', $userPassword));
}
public function test_user_deletion()
@ -276,7 +280,7 @@ class AuthTest extends BrowserKitTest
public function test_user_cannot_be_deleted_if_last_admin()
{
$adminRole = \BookStack\Auth\Role::getRole('admin');
$adminRole = Role::getRole('admin');
// Delete all but one admin user if there are more than one
$adminUsers = $adminRole->users;
@ -309,14 +313,13 @@ class AuthTest extends BrowserKitTest
public function test_reset_password_flow()
{
Notification::fake();
$this->visit('/login')->click('Forgot Password?')
->seePageIs('/password/email')
->type('admin@admin.com', 'email')
->press('Send Reset Link')
->see('A password reset link has been sent to admin@admin.com');
->see('A password reset link will be sent to admin@admin.com if that email address is found in the system.');
$this->seeInDatabase('password_resets', [
'email' => 'admin@admin.com'
@ -324,8 +327,8 @@ class AuthTest extends BrowserKitTest
$user = User::where('email', '=', 'admin@admin.com')->first();
Notification::assertSentTo($user, \BookStack\Notifications\ResetPassword::class);
$n = Notification::sent($user, \BookStack\Notifications\ResetPassword::class);
Notification::assertSentTo($user, ResetPassword::class);
$n = Notification::sent($user, ResetPassword::class);
$this->visit('/password/reset/' . $n->first()->token)
->see('Reset Password')
@ -337,6 +340,28 @@ class AuthTest extends BrowserKitTest
->see('Your password has been successfully reset');
}
public function test_reset_password_flow_shows_success_message_even_if_wrong_password_to_prevent_user_discovery()
{
$this->visit('/login')->click('Forgot Password?')
->seePageIs('/password/email')
->type('barry@admin.com', 'email')
->press('Send Reset Link')
->see('A password reset link will be sent to barry@admin.com if that email address is found in the system.')
->dontSee('We can\'t find a user');
$this->visit('/password/reset/arandometokenvalue')
->see('Reset Password')
->submitForm('Reset Password', [
'email' => 'barry@admin.com',
'password' => 'randompass',
'password_confirmation' => 'randompass'
])->followRedirects()
->seePageIs('/password/reset/arandometokenvalue')
->dontSee('We can\'t find a user')
->see('The password reset token is invalid for this email address.');
}
public function test_reset_password_page_shows_sign_links()
{
$this->setSettings(['registration-enabled' => 'true']);