security: Prevent 2FA concurrent recovery code reuse (#19891)

* security: 2FA concurrent recovery code reuse

* Update AppAuthentication.php

* Merge commit from fork

* security: also serialise recovery code verification on SQLite

The `lockForUpdate()` row lock does not exist on SQLite — Laravel's SQLite
query grammar compiles `compileLockForUpdate()` to an empty string — so the
transaction provides no mutual exclusion there. Two concurrent requests still
both read the full recovery code set before either commits, and the losing
write resurrects the just-consumed code.

Verified against a true `pcntl_fork()` race on v5.6.4: the transaction-only
fix is bypassed 6/6 runs on SQLite while holding 7/7 on MySQL.

Wrap the transaction in a per-user `Cache::lock()` so verification is
serialised regardless of database driver. The DB transaction and row lock are
kept for drivers that support them. Re-verified: combined fix holds 6/6 on
SQLite and 5/5 on MySQL.

* cleanup

---------

Co-authored-by: StarPlatinu <thanhnolove21@gmail.com>
Co-authored-by: Dan Harrin <git@danharrin.com>

---------

Co-authored-by: ThanhVu <72287279+StarPlatinu@users.noreply.github.com>
Co-authored-by: StarPlatinu <thanhnolove21@gmail.com>

docs/07-users/02-multi-factor-authentication.md

@@ -317,3 +317,12 @@ In Filament, the multi-factor authentication process occurs before the user is a
 
 However, if you have other parts of your Laravel app that authenticate users, please bear in mind that they will not be challenged for multi-factor authentication if they are already authenticated elsewhere and then visit the panel, unless [multi-factor authentication is required](#requiring-multi-factor-authentication) and they have not set it up yet.
 
+### Concurrent recovery code submissions
+
+When a user signs in with a recovery code, Filament's `verifyRecoveryCode()` method wraps the read-validate-write sequence in a per-user `Cache::lock` and a database transaction with a `lockForUpdate()` row lock on the user's row. The cache lock serializes concurrent submissions across PHP workers regardless of the underlying database driver, so two parallel sign-in requests cannot both consume the same code or resurrect a just-consumed code from a stale snapshot — even when the storage is a non-SQL store, a different database connection, or a driver without `SELECT ... FOR UPDATE` support (such as SQLite).
+
+<Aside variant="warning">
+    The cache lock relies on a shared lock store. Filament's default `file` cache store, as well as `redis`, `memcached`, `database`, and `dynamodb`, all provide a shared lock across PHP-FPM workers on the same machine (or across machines, for the network-backed stores). The `array` store is per-process and does not serialize across workers — it is intended for testing only.
+
+    If you override `getAppAuthenticationRecoveryCodes()` / `saveAppAuthenticationRecoveryCodes()`, the cache lock still wraps the full read-validate-write sequence, so your override is protected. Your override is only responsible for making the storage write itself atomic — for example, a single Eloquent `update()` or an equivalent atomic primitive on your chosen store.
+</Aside>

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

@@ -23,6 +23,8 @@ use Filament\Schemas\Components\Utilities\Get;
 use Filament\Schemas\Components\Utilities\Set;
 use Illuminate\Contracts\Auth\Authenticatable;
 use Illuminate\Support\Collection;
+use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Hash;
 use Illuminate\Support\Str;
 use LogicException;
@@ -197,24 +199,40 @@ class AppAuthentication implements MultiFactorAuthenticationProvider
     {
         $user ??= Filament::auth()->user();
 
-        $remainingCodes = [];
-        $isValid = false;
+        $lockKey = 'filament.app_authentication_recovery_codes.' . md5(
+            $user::class . ':' . (($user instanceof Authenticatable) ? $user->getAuthIdentifier() : spl_object_id($user)),
+        );
 
-        foreach ($this->getRecoveryCodes($user) as $hashedRecoveryCode) { /** @phpstan-ignore-line */
-            if (Hash::check($recoveryCode, $hashedRecoveryCode)) {
-                $isValid = true;
+        return Cache::lock($lockKey, 10)->block(10, fn (): bool => DB::transaction(function () use ($user, $recoveryCode): bool {
+            $lockedUser = $user
+                ->newQuery() /** @phpstan-ignore-line */
+                ->whereKey($user->getKey()) /** @phpstan-ignore-line */
+                ->lockForUpdate()
+                ->first();
 
-                continue;
+            if ($lockedUser === null) {
+                return false;
             }
 
-            $remainingCodes[] = $hashedRecoveryCode;
-        }
+            $remainingCodes = [];
+            $isValid = false;
 
-        if ($isValid) {
-            $user->saveAppAuthenticationRecoveryCodes($remainingCodes);
-        }
+            foreach ($this->getRecoveryCodes($lockedUser) as $hashedRecoveryCode) { /** @phpstan-ignore-line */
+                if (Hash::check($recoveryCode, $hashedRecoveryCode)) {
+                    $isValid = true;
 
-        return $isValid;
+                    continue;
+                }
+
+                $remainingCodes[] = $hashedRecoveryCode;
+            }
+
+            if ($isValid) {
+                $lockedUser->saveAppAuthenticationRecoveryCodes($remainingCodes); /** @phpstan-ignore-line */
+            }
+
+            return $isValid;
+        }));
     }
 
     /**

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

@@ -409,6 +409,44 @@ describe('recovery codes', function (): void {
 
         $this->assertGuest();
     });
+
+    it('will not preserve a recovery code when a different code is used concurrently', function (): void {
+        $appAuthentication = Arr::first(Filament::getCurrentOrDefaultPanel()->getMultiFactorAuthenticationProviders());
+
+        $userToAuthenticate = User::factory()
+            ->hasAppAuthentication($recoveryCodes = $appAuthentication->generateRecoveryCodes())
+            ->create();
+
+        $initialCount = count($userToAuthenticate->app_authentication_recovery_codes);
+
+        $userInstanceA = User::find($userToAuthenticate->getKey());
+        $userInstanceB = User::find($userToAuthenticate->getKey());
+
+        expect($appAuthentication->verifyRecoveryCode($recoveryCodes[0], $userInstanceA))->toBeTrue();
+        expect($appAuthentication->verifyRecoveryCode($recoveryCodes[1], $userInstanceB))->toBeTrue();
+
+        $userToAuthenticate->refresh();
+
+        expect($userToAuthenticate->app_authentication_recovery_codes)->toHaveCount($initialCount - 2);
+
+        expect($appAuthentication->verifyRecoveryCode($recoveryCodes[0], $userToAuthenticate->fresh()))->toBeFalse();
+    });
+
+    it('will not allow the same recovery code to authenticate two concurrent requests', function (): void {
+        $appAuthentication = Arr::first(Filament::getCurrentOrDefaultPanel()->getMultiFactorAuthenticationProviders());
+
+        $userToAuthenticate = User::factory()
+            ->hasAppAuthentication($recoveryCodes = $appAuthentication->generateRecoveryCodes())
+            ->create();
+
+        $userInstanceA = User::find($userToAuthenticate->getKey());
+        $userInstanceB = User::find($userToAuthenticate->getKey());
+
+        $code = $recoveryCodes[0];
+
+        expect($appAuthentication->verifyRecoveryCode($code, $userInstanceA))->toBeTrue();
+        expect($appAuthentication->verifyRecoveryCode($code, $userInstanceB))->toBeFalse();
+    });
 });
 
 describe('security', function (): void {