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

diff --git a/app/Http/Controllers/Auth/ForgotPasswordController.php b/app/Http/Controllers/Auth/ForgotPasswordController.php
index a60fcc1c9a6142d450d318375a930e0fce294994..fadac641ecdb810b916560611029a1b517d3d6fe 100644 (file)
--- a/app/Http/Controllers/Auth/ForgotPasswordController.php
+++ b/app/Http/Controllers/Auth/ForgotPasswordController.php
@@ -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));
         }

diff --git a/app/Http/Controllers/Auth/ResetPasswordController.php b/app/Http/Controllers/Auth/ResetPasswordController.php
index 214647cd5746a0eff3798a1a9deb1d8803183787..efdf0015924f6d831a0233a737e7209ff246b7e0 100644 (file)
--- a/app/Http/Controllers/Auth/ResetPasswordController.php
+++ b/app/Http/Controllers/Auth/ResetPasswordController.php
@@ -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)]);
+    }
 }

diff --git a/resources/js/components/notification.js b/resources/js/components/notification.js
index f7edb08aa1327fda20c76728f2fb8add65119b60..35bab4ea656b1c052e875fac2bcdf0e4ef27d268 100644 (file)
--- a/resources/js/components/notification.js
+++ b/resources/js/components/notification.js
@@ -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() {

diff --git a/resources/lang/en/auth.php b/resources/lang/en/auth.php
index 6961e049b3dc2ad15d25d2a1369f4dc6988e2a6b..d64fce93a62d90889b2297a9e4f6482ad9046475 100644 (file)
--- a/resources/lang/en/auth.php
+++ b/resources/lang/en/auth.php
@@ -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.',

diff --git a/resources/lang/en/passwords.php b/resources/lang/en/passwords.php
index f41ca7868f66f2937e8ad49e7e2daee1eff3c569..b408f3c2fdaf1e80e9cdafa36ae9507db9fbda48 100644 (file)
--- a/resources/lang/en/passwords.php
+++ b/resources/lang/en/passwords.php
@@ -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!',
 

diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php
index cb27de96170ec8517a4befc6a27d8b99e95f3d0e..40bcda713d6affc1cd1097346c2b1986eeaf7c15 100644 (file)
--- a/tests/Auth/AuthTest.php
+++ b/tests/Auth/AuthTest.php
@@ -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']);