fix: Cache used MFA codes using hash of secret rather than secret and code

packages/panels/src/Auth/MultiFactor/App/Actions/DisableAppAuthenticationAction.php

@@ -56,7 +56,7 @@ public static function make(AppAuthentication $appAuthentication): Action
 
                             RateLimiter::hit($rateLimitingKey);
 
-                            if (is_string($value) && $appAuthentication->verifyCode($value)) {
+                            if (is_string($value) && $appAuthentication->verifyCode($value, shouldPreventCodeReuse: true)) {
                                 return;
                             }
 

packages/panels/src/Auth/MultiFactor/App/Actions/RegenerateAppAuthenticationRecoveryCodesAction.php

@@ -55,7 +55,7 @@ public static function make(AppAuthentication $appAuthentication): Action
 
                             RateLimiter::hit($rateLimitingKey);
 
-                            if ($appAuthentication->verifyCode($value)) {
+                            if ($appAuthentication->verifyCode($value, shouldPreventCodeReuse: true)) {
                                 return;
                             }
 

packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php

@@ -22,6 +22,7 @@
 use Filament\Schemas\Components\Utilities\Get;
 use Filament\Schemas\Components\Utilities\Set;
 use Illuminate\Contracts\Auth\Authenticatable;
+use Illuminate\Contracts\Cache\LockProvider;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\DB;
@@ -178,21 +179,36 @@ public function verifyCode(#[SensitiveParameter] string $code, #[SensitiveParame
             return $this->google2FA->verifyKey($secret, $code, $this->getCodeWindow());
         }
 
-        $cacheKey = 'filament.app_authentication_codes.' . md5($secret . $code);
+        // The key deliberately excludes the code itself, so that it records the timestep of the
+        // last accepted code rather than a marker for one specific code. RFC 6238 requires that
+        // a successful verification rejects that code and every earlier one, not just a repeat
+        // of the same code.
+        $cacheKey = 'filament.app_authentication_codes.' . md5($secret);
 
-        $timestamp = $this->google2FA->verifyKeyNewer($secret, $code, cache()->get($cacheKey), $this->getCodeWindow());
+        $verifyCode = function () use ($cacheKey, $code, $secret): bool {
+            $timestamp = $this->google2FA->verifyKeyNewer($secret, $code, Cache::get($cacheKey), $this->getCodeWindow());
+
+            if ($timestamp === false) {
+                return false;
+            }
 
-        if ($timestamp !== false) {
             if ($timestamp === true) {
                 $timestamp = $this->google2FA->getTimestamp();
             }
 
-            cache()->put($cacheKey, $timestamp, ($this->getCodeWindow() + 1) * 60);
+            Cache::put($cacheKey, $timestamp, ($this->getCodeWindow() + 1) * 60);
 
             return true;
+        };
+
+        // Locking closes the window where concurrent requests both read the timestep before
+        // either writes it. Not every cache store supports locks, and verification is on the
+        // login path, so fall back to verifying without one rather than failing to log in.
+        if (! (Cache::getStore() instanceof LockProvider)) {
+            return $verifyCode();
         }
 
-        return false;
+        return Cache::lock("{$cacheKey}.lock", 10)->block(10, $verifyCode);
     }
 
     public function verifyRecoveryCode(#[SensitiveParameter] string $recoveryCode, ?HasAppAuthenticationRecovery $user = null): bool

tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php

@@ -6,9 +6,13 @@
 use Filament\Forms\Components\TextInput;
 use Filament\Tests\Fixtures\Models\User;
 use Filament\Tests\TestCase;
+use Illuminate\Cache\Repository;
+use Illuminate\Contracts\Cache\Store;
 use Illuminate\Support\Arr;
+use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\RateLimiter;
 use Illuminate\Support\Str;
+use PragmaRX\Google2FAQRCode\Google2FA;
 
 use function Filament\Tests\livewire;
 
@@ -762,6 +766,25 @@
         $this->assertGuest();
     });
 
+    it('will not allow a TOTP code from an earlier time window to be used after a newer code', function (): void {
+        $appAuthentication = Arr::first(Filament::getCurrentOrDefaultPanel()->getMultiFactorAuthenticationProviders());
+
+        $userToAuthenticate = User::factory()
+            ->hasAppAuthentication()
+            ->create();
+
+        $secret = $appAuthentication->getSecret($userToAuthenticate);
+
+        $google2FA = app(Google2FA::class);
+
+        $timestamp = $google2FA->getTimestamp();
+        $earlierCode = $google2FA->oathTotp($secret, $timestamp - 4);
+        $currentCode = $google2FA->oathTotp($secret, $timestamp);
+
+        expect($appAuthentication->verifyCode($currentCode, $secret, shouldPreventCodeReuse: true))->toBeTrue();
+        expect($appAuthentication->verifyCode($earlierCode, $secret, shouldPreventCodeReuse: true))->toBeFalse();
+    });
+
     it('will not allow a TOTP code from a future time window to be reused', function (): void {
         $appAuthentication = Arr::first(Filament::getCurrentOrDefaultPanel()->getMultiFactorAuthenticationProviders());
 
@@ -778,4 +801,108 @@
         expect($appAuthentication->verifyCode($futureCode, $secret, shouldPreventCodeReuse: true))->toBeTrue();
         expect($appAuthentication->verifyCode($futureCode, $secret, shouldPreventCodeReuse: true))->toBeFalse();
     });
+
+    it('can still verify TOTP codes when the cache store does not support locks', function (): void {
+        Cache::swap(new Repository(new NonLockingCacheStore));
+
+        $appAuthentication = Arr::first(Filament::getCurrentOrDefaultPanel()->getMultiFactorAuthenticationProviders());
+
+        $userToAuthenticate = User::factory()
+            ->hasAppAuthentication()
+            ->create();
+
+        $secret = $appAuthentication->getSecret($userToAuthenticate);
+
+        $google2FA = app(Google2FA::class);
+
+        $timestamp = $google2FA->getTimestamp();
+        $earlierCode = $google2FA->oathTotp($secret, $timestamp - 4);
+        $currentCode = $google2FA->oathTotp($secret, $timestamp);
+
+        expect($appAuthentication->verifyCode($currentCode, $secret, shouldPreventCodeReuse: true))->toBeTrue();
+        expect($appAuthentication->verifyCode($currentCode, $secret, shouldPreventCodeReuse: true))->toBeFalse();
+        expect($appAuthentication->verifyCode($earlierCode, $secret, shouldPreventCodeReuse: true))->toBeFalse();
+    });
 });
+
+/**
+ * A cache store that implements `Store` but not `LockProvider`, like `ApcStore`,
+ * `StorageStore`, `SessionStore` and many third-party cache drivers.
+ */
+class NonLockingCacheStore implements Store
+{
+    /** @var array<string, mixed> */
+    protected array $data = [];
+
+    public function get($key): mixed
+    {
+        return $this->data[$key] ?? null;
+    }
+
+    /**
+     * @param  array<string>  $keys
+     * @return array<string, mixed>
+     */
+    public function many(array $keys): array
+    {
+        return array_map(fn (string $key): mixed => $this->get($key), array_combine($keys, $keys));
+    }
+
+    public function put($key, $value, $seconds): bool
+    {
+        $this->data[$key] = $value;
+
+        return true;
+    }
+
+    /**
+     * @param  array<string, mixed>  $values
+     */
+    public function putMany(array $values, $seconds): bool
+    {
+        foreach ($values as $key => $value) {
+            $this->put($key, $value, $seconds);
+        }
+
+        return true;
+    }
+
+    public function increment($key, $value = 1): int
+    {
+        return $this->data[$key] = ((int) ($this->data[$key] ?? 0)) + $value;
+    }
+
+    public function decrement($key, $value = 1): int
+    {
+        return $this->increment($key, -$value);
+    }
+
+    public function forever($key, $value): bool
+    {
+        return $this->put($key, $value, 0);
+    }
+
+    public function forget($key): bool
+    {
+        unset($this->data[$key]);
+
+        return true;
+    }
+
+    public function flush(): bool
+    {
+        $this->data = [];
+
+        return true;
+    }
+
+    public function touch($key, $ttl): bool
+    {
+        return true;
+    }
+
+    public function getPrefix(): string
+    {
+        return '';
+    }
+}