From ef821192267f075996efc9b98eabfb2fbb3ff4ad Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 21 May 2026 12:27:38 +0100 Subject: [PATCH] MFA: Added verify attempt rate limiting --- .../Controllers/HandlesPartialLogins.php | 6 ++ .../Controllers/MfaBackupCodesController.php | 13 ++++ app/Access/Controllers/MfaTotpController.php | 11 +++- app/Access/LoginService.php | 2 +- app/Access/Mfa/MfaVerificationLimiter.php | 62 +++++++++++++++++++ lang/en/auth.php | 1 + tests/Auth/MfaVerificationTest.php | 42 +++++++++++++ 7 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 app/Access/Mfa/MfaVerificationLimiter.php diff --git a/app/Access/Controllers/HandlesPartialLogins.php b/app/Access/Controllers/HandlesPartialLogins.php index 47a63d19b..8afad2776 100644 --- a/app/Access/Controllers/HandlesPartialLogins.php +++ b/app/Access/Controllers/HandlesPartialLogins.php @@ -22,4 +22,10 @@ trait HandlesPartialLogins return $user; } + + protected function clearLastAttemptedUser(): void + { + $loginService = app()->make(LoginService::class); + $loginService->clearLastLoginAttempted(); + } } diff --git a/app/Access/Controllers/MfaBackupCodesController.php b/app/Access/Controllers/MfaBackupCodesController.php index 5c334674e..6ab3e1d14 100644 --- a/app/Access/Controllers/MfaBackupCodesController.php +++ b/app/Access/Controllers/MfaBackupCodesController.php @@ -6,6 +6,7 @@ use BookStack\Access\LoginService; use BookStack\Access\Mfa\BackupCodeService; use BookStack\Access\Mfa\MfaSession; use BookStack\Access\Mfa\MfaValue; +use BookStack\Access\Mfa\MfaVerificationLimiter; use BookStack\Activity\ActivityType; use BookStack\Exceptions\NotFoundException; use BookStack\Http\Controller; @@ -19,6 +20,11 @@ class MfaBackupCodesController extends Controller protected const SETUP_SECRET_SESSION_KEY = 'mfa-setup-backup-codes'; + public function __construct( + protected MfaVerificationLimiter $limiter, + ) { + } + /** * Show a view that generates and displays backup codes. */ @@ -71,6 +77,12 @@ class MfaBackupCodesController extends Controller public function verify(Request $request, BackupCodeService $codeService, MfaSession $mfaSession, LoginService $loginService) { $user = $this->currentOrLastAttemptedUser(); + $this->limiter->incrementAttempts($user, $request); + if ($this->limiter->hasHitLimit($user, $request)) { + $this->clearLastAttemptedUser(); + $this->limiter->throwException(); + } + $codes = MfaValue::getValueForUser($user, MfaValue::METHOD_BACKUP_CODES) ?? '[]'; $this->validate($request, [ @@ -89,6 +101,7 @@ class MfaBackupCodesController extends Controller $mfaSession->markVerifiedForUser($user); $loginService->reattemptLoginFor($user); + $this->limiter->decrementAttempts($user, $request); if ($codeService->countCodesInSet($updatedCodes) < 5) { $this->showWarningNotification(trans('auth.mfa_backup_codes_usage_limit_warning')); diff --git a/app/Access/Controllers/MfaTotpController.php b/app/Access/Controllers/MfaTotpController.php index 5202fedc0..b8a333228 100644 --- a/app/Access/Controllers/MfaTotpController.php +++ b/app/Access/Controllers/MfaTotpController.php @@ -5,6 +5,7 @@ namespace BookStack\Access\Controllers; use BookStack\Access\LoginService; use BookStack\Access\Mfa\MfaSession; use BookStack\Access\Mfa\MfaValue; +use BookStack\Access\Mfa\MfaVerificationLimiter; use BookStack\Access\Mfa\TotpService; use BookStack\Access\Mfa\TotpValidationRule; use BookStack\Activity\ActivityType; @@ -20,7 +21,8 @@ class MfaTotpController extends Controller protected const SETUP_SECRET_SESSION_KEY = 'mfa-setup-totp-secret'; public function __construct( - protected TotpService $totp + protected TotpService $totp, + protected MfaVerificationLimiter $limiter, ) { } @@ -86,6 +88,12 @@ class MfaTotpController extends Controller public function verify(Request $request, LoginService $loginService, MfaSession $mfaSession) { $user = $this->currentOrLastAttemptedUser(); + $this->limiter->incrementAttempts($user, $request); + if ($this->limiter->hasHitLimit($user, $request)) { + $this->clearLastAttemptedUser(); + $this->limiter->throwException(); + } + $totpSecret = MfaValue::getValueForUser($user, MfaValue::METHOD_TOTP); $this->validate($request, [ @@ -98,6 +106,7 @@ class MfaTotpController extends Controller $mfaSession->markVerifiedForUser($user); $loginService->reattemptLoginFor($user); + $this->limiter->decrementAttempts($user, $request); return redirect()->intended(); } diff --git a/app/Access/LoginService.php b/app/Access/LoginService.php index c81e95572..f089f5ba9 100644 --- a/app/Access/LoginService.php +++ b/app/Access/LoginService.php @@ -126,7 +126,7 @@ class LoginService /** * Clear the last login attempted session value. */ - protected function clearLastLoginAttempted(): void + public function clearLastLoginAttempted(): void { session()->remove(self::LAST_LOGIN_ATTEMPTED_SESSION_KEY); } diff --git a/app/Access/Mfa/MfaVerificationLimiter.php b/app/Access/Mfa/MfaVerificationLimiter.php new file mode 100644 index 000000000..3ff0adad4 --- /dev/null +++ b/app/Access/Mfa/MfaVerificationLimiter.php @@ -0,0 +1,62 @@ + 60]), + '/login', + Response::HTTP_TOO_MANY_REQUESTS + ); + } + + public function incrementAttempts(User $user, Request $request): void + { + $this->rateLimiter->hit($this->getUserKey($user)); + $this->rateLimiter->hit($this->getRequestKey($request)); + } + + public function decrementAttempts(User $user, Request $request): void + { + $this->rateLimiter->decrement($this->getUserKey($user)); + $this->rateLimiter->decrement($this->getRequestKey($request)); + } + + public function hasHitLimit(User $user, Request $request): bool + { + return $this->rateLimiter->tooManyAttempts($this->getUserKey($user), $this->maxUserAttemptsPerMinute + 1) + || $this->rateLimiter->tooManyAttempts($this->getRequestKey($request), $this->maxIpAttemptsPerMinute + 1); + } + + protected function getUserKey(User $user): string + { + return "mfa-attempt::user::{$user->id}"; + } + + protected function getRequestKey(Request $request): string + { + return "mfa-attempt::request::{$request->ip()}"; + } +} diff --git a/lang/en/auth.php b/lang/en/auth.php index 57f0cb5c6..47be4ea72 100644 --- a/lang/en/auth.php +++ b/lang/en/auth.php @@ -8,6 +8,7 @@ return [ 'failed' => 'These credentials do not match our records.', 'throttle' => 'Too many login attempts. Please try again in :seconds seconds.', + 'mfa_throttle' => 'Too many multi-factor verification attempts. Please try again in :seconds seconds.', // Login & Register 'sign_up' => 'Sign up', diff --git a/tests/Auth/MfaVerificationTest.php b/tests/Auth/MfaVerificationTest.php index 76c59bc74..967be1c6a 100644 --- a/tests/Auth/MfaVerificationTest.php +++ b/tests/Auth/MfaVerificationTest.php @@ -66,6 +66,27 @@ class MfaVerificationTest extends TestCase $html->assertElementExists('input[autocomplete="one-time-code"][name="code"]'); } + public function test_totp_verification_is_rate_limited() + { + [$user, $secret, $loginResp] = $this->startTotpLogin(); + $loginService = $this->app->make(LoginService::class); + + $resp = $this->get('/mfa/verify'); + for ($i = 0; $i < 5; $i++) { + $this->post('/mfa/totp/verify', [ + 'code' => '123456', + ])->assertRedirect('/mfa/verify'); + $this->assertNotNull($loginService->getLastLoginAttemptUser()); + } + + $resp = $this->post('/mfa/totp/verify', [ + 'code' => '123456', + ]); + $resp->assertRedirect('/login'); + $this->assertSessionError('Too many multi-factor verification attempts. Please try again in 60 seconds.'); + $this->assertNull($loginService->getLastLoginAttemptUser()); + } + public function test_backup_code_verification() { [$user, $codes, $loginResp] = $this->startBackupCodeLogin(); @@ -147,6 +168,27 @@ class MfaVerificationTest extends TestCase $resp->assertSeeText('You have less than 5 backup codes remaining, Please generate and store a new set before you run out of codes to prevent being locked out of your account.'); } + public function test_backup_code_verification_is_rate_limited() + { + [$user, $codes, $loginResp] = $this->startBackupCodeLogin(['abc12-def45', 'abc12-def46']); + $loginService = $this->app->make(LoginService::class); + + $resp = $this->get('/mfa/verify'); + for ($i = 0; $i < 5; $i++) { + $this->post('/mfa/backup_codes/verify', [ + 'code' => '123456abcd', + ])->assertRedirect('/mfa/verify'); + $this->assertNotNull($loginService->getLastLoginAttemptUser()); + } + + $resp = $this->post('/mfa/backup_codes/verify', [ + 'code' => '123456abcd', + ]); + $resp->assertRedirect('/login'); + $this->assertSessionError('Too many multi-factor verification attempts. Please try again in 60 seconds.'); + $this->assertNull($loginService->getLastLoginAttemptUser()); + } + public function test_backup_code_form_has_autofill_configured() { [$user, $codes, $loginResp] = $this->startBackupCodeLogin();