From 1a5e481e6e51c043639f829abf3bbde3671e06b6 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Wed, 24 Aug 2022 14:03:08 +0100 Subject: [PATCH 1/9] test: request password reset does not leak user existence Signed-off-by: Sami Mazouz --- .../api/users/SendPasswordResetEmailTest.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php b/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php index 09494edb7e..fece596106 100644 --- a/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php +++ b/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php @@ -70,4 +70,19 @@ public function users_cant_send_confirmation_emails_too_fast() $this->assertEquals(429, $response->getStatusCode()); } + + /** @test */ + public function request_password_reset_does_not_leak_user_existence() + { + $response = $this->send( + $this->request('POST','/api/forgot', [ + 'authenticatedAs' => 3, + 'json' => [ + 'email' => 'missing_user@machine.local' + ] + ]) + ); + + $this->assertEquals(422, $response->getStatusCode()); + } } From 43e65901cb668be81946326eafbe22c9322c594f Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Wed, 24 Aug 2022 14:09:31 +0100 Subject: [PATCH 2/9] fix: request password leaks user existence Signed-off-by: Sami Mazouz --- .../src/User/Command/RequestPasswordResetHandler.php | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/framework/core/src/User/Command/RequestPasswordResetHandler.php b/framework/core/src/User/Command/RequestPasswordResetHandler.php index ddaa71bad3..0882469c7d 100644 --- a/framework/core/src/User/Command/RequestPasswordResetHandler.php +++ b/framework/core/src/User/Command/RequestPasswordResetHandler.php @@ -17,7 +17,8 @@ use Illuminate\Contracts\Queue\Queue; use Illuminate\Contracts\Validation\Factory; use Illuminate\Database\Eloquent\ModelNotFoundException; -use Illuminate\Validation\ValidationException; +use Illuminate\Validation\ValidationException as IlluminateValidationException; +use Flarum\Foundation\ValidationException; use Symfony\Contracts\Translation\TranslatorInterface; class RequestPasswordResetHandler @@ -91,13 +92,17 @@ public function handle(RequestPasswordReset $command) ); if ($validation->fails()) { - throw new ValidationException($validation); + throw new IlluminateValidationException($validation); } $user = $this->users->findByEmail($email); if (! $user) { - throw new ModelNotFoundException; + throw new ValidationException([ + 'message' => strtr($this->translator->trans('validation.email'), [ + ':attribute' => $this->translator->trans('validation.attributes.email') + ]) + ]); } $token = PasswordToken::generate($user->id); From 4bb15c6f652f0e1311d1bd1b4730ec15886f0329 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Wed, 24 Aug 2022 14:11:02 +0100 Subject: [PATCH 3/9] chore: add comment Signed-off-by: Sami Mazouz --- .../core/src/User/Command/RequestPasswordResetHandler.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/framework/core/src/User/Command/RequestPasswordResetHandler.php b/framework/core/src/User/Command/RequestPasswordResetHandler.php index 0882469c7d..08fc3ad896 100644 --- a/framework/core/src/User/Command/RequestPasswordResetHandler.php +++ b/framework/core/src/User/Command/RequestPasswordResetHandler.php @@ -17,7 +17,6 @@ use Illuminate\Contracts\Queue\Queue; use Illuminate\Contracts\Validation\Factory; use Illuminate\Database\Eloquent\ModelNotFoundException; -use Illuminate\Validation\ValidationException as IlluminateValidationException; use Flarum\Foundation\ValidationException; use Symfony\Contracts\Translation\TranslatorInterface; @@ -91,13 +90,10 @@ public function handle(RequestPasswordReset $command) ['email' => 'required|email'] ); - if ($validation->fails()) { - throw new IlluminateValidationException($validation); - } - $user = $this->users->findByEmail($email); - if (! $user) { + // Prevents leaking user existence. + if ($validation->fails() || ! $user) { throw new ValidationException([ 'message' => strtr($this->translator->trans('validation.email'), [ ':attribute' => $this->translator->trans('validation.attributes.email') From 8a4d338368d928f09ebcebc34885f65247936b3a Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Wed, 24 Aug 2022 14:20:28 +0100 Subject: [PATCH 4/9] fix: prevent leaking user existence by duration. Signed-off-by: Sami Mazouz --- .../Command/RequestPasswordResetHandler.php | 44 ++------------- .../src/User/Job/RequestPasswordResetJob.php | 54 +++++++++++++++++++ 2 files changed, 58 insertions(+), 40 deletions(-) create mode 100644 framework/core/src/User/Job/RequestPasswordResetJob.php diff --git a/framework/core/src/User/Command/RequestPasswordResetHandler.php b/framework/core/src/User/Command/RequestPasswordResetHandler.php index 08fc3ad896..4346e959ce 100644 --- a/framework/core/src/User/Command/RequestPasswordResetHandler.php +++ b/framework/core/src/User/Command/RequestPasswordResetHandler.php @@ -9,10 +9,7 @@ namespace Flarum\User\Command; -use Flarum\Http\UrlGenerator; -use Flarum\Mail\Job\SendRawEmailJob; -use Flarum\Settings\SettingsRepositoryInterface; -use Flarum\User\PasswordToken; +use Flarum\User\Job\RequestPasswordResetJob; use Flarum\User\UserRepository; use Illuminate\Contracts\Queue\Queue; use Illuminate\Contracts\Validation\Factory; @@ -27,21 +24,11 @@ class RequestPasswordResetHandler */ protected $users; - /** - * @var SettingsRepositoryInterface - */ - protected $settings; - /** * @var Queue */ protected $queue; - /** - * @var UrlGenerator - */ - protected $url; - /** * @var TranslatorInterface */ @@ -52,26 +39,14 @@ class RequestPasswordResetHandler */ protected $validatorFactory; - /** - * @param UserRepository $users - * @param SettingsRepositoryInterface $settings - * @param Queue $queue - * @param UrlGenerator $url - * @param TranslatorInterface $translator - * @param Factory $validatorFactory - */ public function __construct( UserRepository $users, - SettingsRepositoryInterface $settings, Queue $queue, - UrlGenerator $url, TranslatorInterface $translator, Factory $validatorFactory ) { $this->users = $users; - $this->settings = $settings; $this->queue = $queue; - $this->url = $url; $this->translator = $translator; $this->validatorFactory = $validatorFactory; } @@ -79,7 +54,7 @@ public function __construct( /** * @param RequestPasswordReset $command * @return \Flarum\User\User - * @throws ModelNotFoundException + * @throws ValidationException */ public function handle(RequestPasswordReset $command) { @@ -101,19 +76,8 @@ public function handle(RequestPasswordReset $command) ]); } - $token = PasswordToken::generate($user->id); - $token->save(); - - $data = [ - 'username' => $user->display_name, - 'url' => $this->url->to('forum')->route('resetPassword', ['token' => $token->token]), - 'forum' => $this->settings->get('forum_title'), - ]; - - $body = $this->translator->trans('core.email.reset_password.body', $data); - $subject = $this->translator->trans('core.email.reset_password.subject'); - - $this->queue->push(new SendRawEmailJob($user->email, $subject, $body)); + // Prevents leak user existence by duration (requires a queue). + $this->queue->push(new RequestPasswordResetJob($user)); return $user; } diff --git a/framework/core/src/User/Job/RequestPasswordResetJob.php b/framework/core/src/User/Job/RequestPasswordResetJob.php new file mode 100644 index 0000000000..913699a61a --- /dev/null +++ b/framework/core/src/User/Job/RequestPasswordResetJob.php @@ -0,0 +1,54 @@ +user = $user; + } + + public function handle( + SettingsRepositoryInterface $settings, + UrlGenerator $url, + TranslatorInterface $translator, + Queue $queue + ) { + $user = $this->user; + $token = PasswordToken::generate($user->id); + $token->save(); + + $data = [ + 'username' => $user->display_name, + 'url' => $url->to('forum')->route('resetPassword', ['token' => $token->token]), + 'forum' => $settings->get('forum_title'), + ]; + + $body = $translator->trans('core.email.reset_password.body', $data); + $subject = $translator->trans('core.email.reset_password.subject'); + + $queue->push(new SendRawEmailJob($user->email, $subject, $body)); + } +} From ba25b3529bfcb8bf7bfd603bbdabec2a172928fe Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Wed, 24 Aug 2022 13:29:37 +0000 Subject: [PATCH 5/9] Apply fixes from StyleCI --- .../core/src/User/Command/RequestPasswordResetHandler.php | 3 +-- .../tests/integration/api/users/SendPasswordResetEmailTest.php | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/framework/core/src/User/Command/RequestPasswordResetHandler.php b/framework/core/src/User/Command/RequestPasswordResetHandler.php index 4346e959ce..a2f2f0985c 100644 --- a/framework/core/src/User/Command/RequestPasswordResetHandler.php +++ b/framework/core/src/User/Command/RequestPasswordResetHandler.php @@ -9,12 +9,11 @@ namespace Flarum\User\Command; +use Flarum\Foundation\ValidationException; use Flarum\User\Job\RequestPasswordResetJob; use Flarum\User\UserRepository; use Illuminate\Contracts\Queue\Queue; use Illuminate\Contracts\Validation\Factory; -use Illuminate\Database\Eloquent\ModelNotFoundException; -use Flarum\Foundation\ValidationException; use Symfony\Contracts\Translation\TranslatorInterface; class RequestPasswordResetHandler diff --git a/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php b/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php index fece596106..f378b1b605 100644 --- a/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php +++ b/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php @@ -75,7 +75,7 @@ public function users_cant_send_confirmation_emails_too_fast() public function request_password_reset_does_not_leak_user_existence() { $response = $this->send( - $this->request('POST','/api/forgot', [ + $this->request('POST', '/api/forgot', [ 'authenticatedAs' => 3, 'json' => [ 'email' => 'missing_user@machine.local' From 22d2c6bafa0d3b19b601dbc1c1dcfd3ad36ebbd3 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Wed, 24 Aug 2022 15:56:38 +0100 Subject: [PATCH 6/9] fix(review): don't throw any errors at all if the user does not exist Signed-off-by: Sami Mazouz --- framework/core/locale/core.yml | 2 +- .../Command/RequestPasswordResetHandler.php | 44 ++++--------------- .../src/User/Job/RequestPasswordResetJob.php | 18 +++++--- .../api/users/SendPasswordResetEmailTest.php | 2 +- 4 files changed, 23 insertions(+), 43 deletions(-) diff --git a/framework/core/locale/core.yml b/framework/core/locale/core.yml index 2dd6e52192..87d693154d 100644 --- a/framework/core/locale/core.yml +++ b/framework/core/locale/core.yml @@ -350,7 +350,7 @@ core: forgot_password: dismiss_button: => core.ref.okay email_placeholder: => core.ref.email - email_sent_message: We've sent you an email containing a link to reset your password. Check your spam folder if you don't receive it within the next minute or two. + email_sent_message: If this user exists, we've sent an email containing a link to reset your password. Check your spam folder if you don't receive it within the next minute or two. not_found_message: There is no user registered with that email address. submit_button: Recover Password text: Enter your email address and we will send you a link to reset your password. diff --git a/framework/core/src/User/Command/RequestPasswordResetHandler.php b/framework/core/src/User/Command/RequestPasswordResetHandler.php index a2f2f0985c..6c90b038e1 100644 --- a/framework/core/src/User/Command/RequestPasswordResetHandler.php +++ b/framework/core/src/User/Command/RequestPasswordResetHandler.php @@ -9,50 +9,32 @@ namespace Flarum\User\Command; -use Flarum\Foundation\ValidationException; use Flarum\User\Job\RequestPasswordResetJob; -use Flarum\User\UserRepository; use Illuminate\Contracts\Queue\Queue; use Illuminate\Contracts\Validation\Factory; -use Symfony\Contracts\Translation\TranslatorInterface; +use Illuminate\Validation\ValidationException; class RequestPasswordResetHandler { - /** - * @var UserRepository - */ - protected $users; - /** * @var Queue */ protected $queue; - /** - * @var TranslatorInterface - */ - protected $translator; - /** * @var Factory */ protected $validatorFactory; - public function __construct( - UserRepository $users, - Queue $queue, - TranslatorInterface $translator, - Factory $validatorFactory - ) { - $this->users = $users; + public function __construct(Queue $queue, Factory $validatorFactory) + { $this->queue = $queue; - $this->translator = $translator; $this->validatorFactory = $validatorFactory; } /** * @param RequestPasswordReset $command - * @return \Flarum\User\User + * @return void * @throws ValidationException */ public function handle(RequestPasswordReset $command) @@ -64,20 +46,12 @@ public function handle(RequestPasswordReset $command) ['email' => 'required|email'] ); - $user = $this->users->findByEmail($email); - - // Prevents leaking user existence. - if ($validation->fails() || ! $user) { - throw new ValidationException([ - 'message' => strtr($this->translator->trans('validation.email'), [ - ':attribute' => $this->translator->trans('validation.attributes.email') - ]) - ]); + if ($validation->fails()) { + throw new ValidationException($validation); } - // Prevents leak user existence by duration (requires a queue). - $this->queue->push(new RequestPasswordResetJob($user)); - - return $user; + // Prevents leaking user existence by not throwing an error. + // Prevents leaking user existence by duration by using a queued job. + $this->queue->push(new RequestPasswordResetJob($email)); } } diff --git a/framework/core/src/User/Job/RequestPasswordResetJob.php b/framework/core/src/User/Job/RequestPasswordResetJob.php index 913699a61a..5399532d75 100644 --- a/framework/core/src/User/Job/RequestPasswordResetJob.php +++ b/framework/core/src/User/Job/RequestPasswordResetJob.php @@ -14,29 +14,35 @@ use Flarum\Queue\AbstractJob; use Flarum\Settings\SettingsRepositoryInterface; use Flarum\User\PasswordToken; -use Flarum\User\User; +use Flarum\User\UserRepository; use Illuminate\Contracts\Queue\Queue; use Symfony\Contracts\Translation\TranslatorInterface; class RequestPasswordResetJob extends AbstractJob { /** - * @var User + * @var string */ - protected $user; + protected $email; - public function __construct(User $user) + public function __construct(string $email) { - $this->user = $user; + $this->email = $email; } public function handle( SettingsRepositoryInterface $settings, UrlGenerator $url, TranslatorInterface $translator, + UserRepository $users, Queue $queue ) { - $user = $this->user; + $user = $users->findByEmail($this->email); + + if (! $user) { + return; + } + $token = PasswordToken::generate($user->id); $token->save(); diff --git a/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php b/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php index f378b1b605..ac1f3bc0b8 100644 --- a/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php +++ b/framework/core/tests/integration/api/users/SendPasswordResetEmailTest.php @@ -83,6 +83,6 @@ public function request_password_reset_does_not_leak_user_existence() ]) ); - $this->assertEquals(422, $response->getStatusCode()); + $this->assertEquals(204, $response->getStatusCode()); } } From 02da03b72d556da1fd0c31bc9c2ba7af2db5349d Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Wed, 24 Aug 2022 18:13:23 +0100 Subject: [PATCH 7/9] chore(review): better response message wording Co-authored-by: David Wheatley --- framework/core/locale/core.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/locale/core.yml b/framework/core/locale/core.yml index 87d693154d..aa09db1e8d 100644 --- a/framework/core/locale/core.yml +++ b/framework/core/locale/core.yml @@ -350,7 +350,7 @@ core: forgot_password: dismiss_button: => core.ref.okay email_placeholder: => core.ref.email - email_sent_message: If this user exists, we've sent an email containing a link to reset your password. Check your spam folder if you don't receive it within the next minute or two. + email_sent_message: If this email is registered, we'll send an email containing a link to reset their password. Check your spam folder if you don't receive it within the next minute or two. not_found_message: There is no user registered with that email address. submit_button: Recover Password text: Enter your email address and we will send you a link to reset your password. From 7ff028ea7a00742dcfc253dccb9f2fa1f6934f87 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 6 Sep 2022 19:14:29 +0100 Subject: [PATCH 8/9] chore(review): response message wording improvement Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com> --- framework/core/locale/core.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/locale/core.yml b/framework/core/locale/core.yml index aa09db1e8d..c0e43a8499 100644 --- a/framework/core/locale/core.yml +++ b/framework/core/locale/core.yml @@ -350,7 +350,7 @@ core: forgot_password: dismiss_button: => core.ref.okay email_placeholder: => core.ref.email - email_sent_message: If this email is registered, we'll send an email containing a link to reset their password. Check your spam folder if you don't receive it within the next minute or two. + email_sent_message: If the email you entered is registered with this site, we'll send you an email containing a link to reset your password. Check your spam folder if you don't receive it within the next minute or two. not_found_message: There is no user registered with that email address. submit_button: Recover Password text: Enter your email address and we will send you a link to reset your password. From 1fa2bdb28eb6ac8be193a57358944e5a3993f295 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 6 Sep 2022 20:33:58 +0100 Subject: [PATCH 9/9] chore: ditch the command bus layer Signed-off-by: Sami Mazouz --- .../Controller/ForgotPasswordController.php | 38 +++++++------ .../src/User/Command/RequestPasswordReset.php | 28 --------- .../Command/RequestPasswordResetHandler.php | 57 ------------------- 3 files changed, 22 insertions(+), 101 deletions(-) delete mode 100644 framework/core/src/User/Command/RequestPasswordReset.php delete mode 100644 framework/core/src/User/Command/RequestPasswordResetHandler.php diff --git a/framework/core/src/Api/Controller/ForgotPasswordController.php b/framework/core/src/Api/Controller/ForgotPasswordController.php index e6aba95f75..43c5a15c4a 100644 --- a/framework/core/src/Api/Controller/ForgotPasswordController.php +++ b/framework/core/src/Api/Controller/ForgotPasswordController.php @@ -9,10 +9,11 @@ namespace Flarum\Api\Controller; -use Flarum\User\Command\RequestPasswordReset; -use Flarum\User\UserRepository; -use Illuminate\Contracts\Bus\Dispatcher; +use Flarum\User\Job\RequestPasswordResetJob; +use Illuminate\Contracts\Queue\Queue; +use Illuminate\Contracts\Validation\Factory; use Illuminate\Support\Arr; +use Illuminate\Validation\ValidationException; use Laminas\Diactoros\Response\EmptyResponse; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; @@ -21,23 +22,19 @@ class ForgotPasswordController implements RequestHandlerInterface { /** - * @var \Flarum\User\UserRepository + * @var Queue */ - protected $users; + protected $queue; /** - * @var Dispatcher + * @var Factory */ - protected $bus; + protected $validatorFactory; - /** - * @param \Flarum\User\UserRepository $users - * @param Dispatcher $bus - */ - public function __construct(UserRepository $users, Dispatcher $bus) + public function __construct(Queue $queue, Factory $validatorFactory) { - $this->users = $users; - $this->bus = $bus; + $this->queue = $queue; + $this->validatorFactory = $validatorFactory; } /** @@ -47,10 +44,19 @@ public function handle(ServerRequestInterface $request): ResponseInterface { $email = Arr::get($request->getParsedBody(), 'email'); - $this->bus->dispatch( - new RequestPasswordReset($email) + $validation = $this->validatorFactory->make( + compact('email'), + ['email' => 'required|email'] ); + if ($validation->fails()) { + throw new ValidationException($validation); + } + + // Prevents leaking user existence by not throwing an error. + // Prevents leaking user existence by duration by using a queued job. + $this->queue->push(new RequestPasswordResetJob($email)); + return new EmptyResponse; } } diff --git a/framework/core/src/User/Command/RequestPasswordReset.php b/framework/core/src/User/Command/RequestPasswordReset.php deleted file mode 100644 index a884b74429..0000000000 --- a/framework/core/src/User/Command/RequestPasswordReset.php +++ /dev/null @@ -1,28 +0,0 @@ -email = $email; - } -} diff --git a/framework/core/src/User/Command/RequestPasswordResetHandler.php b/framework/core/src/User/Command/RequestPasswordResetHandler.php deleted file mode 100644 index 6c90b038e1..0000000000 --- a/framework/core/src/User/Command/RequestPasswordResetHandler.php +++ /dev/null @@ -1,57 +0,0 @@ -queue = $queue; - $this->validatorFactory = $validatorFactory; - } - - /** - * @param RequestPasswordReset $command - * @return void - * @throws ValidationException - */ - public function handle(RequestPasswordReset $command) - { - $email = $command->email; - - $validation = $this->validatorFactory->make( - compact('email'), - ['email' => 'required|email'] - ); - - if ($validation->fails()) { - throw new ValidationException($validation); - } - - // Prevents leaking user existence by not throwing an error. - // Prevents leaking user existence by duration by using a queued job. - $this->queue->push(new RequestPasswordResetJob($email)); - } -}