Merge pull request 'Merge Further v26.03 changes' (#6133) from v26-03 into development

Reviewed-on: https://codeberg.org/bookstack/bookstack/pulls/6133

app/Access/Controllers/HandlesPartialLogins.php

@@ -22,4 +22,10 @@ trait HandlesPartialLogins
 
         return $user;
     }
+
+    protected function clearLastAttemptedUser(): void
+    {
+        $loginService = app()->make(LoginService::class);
+        $loginService->clearLastLoginAttempted();
+    }
 }

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'));

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();
     }

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);
     }

app/Access/Mfa/MfaVerificationLimiter.php

@@ -0,0 +1,62 @@
+<?php
+
+namespace BookStack\Access\Mfa;
+
+use BookStack\Exceptions\NotifyException;
+use BookStack\Users\Models\User;
+use Illuminate\Cache\RateLimiter;
+use Illuminate\Http\Request;
+use Illuminate\Http\Response;
+
+/**
+ * A rate limit specifically for MFA verification.
+ * Limits across both the attempted user (on a tight limit) and the
+ * request IP (on a less strict limit).
+ */
+class MfaVerificationLimiter
+{
+    protected int $maxUserAttemptsPerMinute = 5;
+    protected int $maxIpAttemptsPerMinute = 60;
+
+    public function __construct(
+        protected RateLimiter $rateLimiter
+    ) {
+    }
+
+    public function throwException(): never
+    {
+        throw new NotifyException(
+            trans('auth.mfa_throttle', ['seconds' => 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()}";
+    }
+}

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',

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();