From 7f3e6ffbeb43ed1870e806736b4e6c3600a70f66 Mon Sep 17 00:00:00 2001 From: Dan Harrin Date: Thu, 21 May 2026 18:47:17 +0100 Subject: [PATCH] security: Prevent 2FA concurrent recovery code reuse (#19891) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 Co-authored-by: Dan Harrin --------- Co-authored-by: ThanhVu <72287279+StarPlatinu@users.noreply.github.com> Co-authored-by: StarPlatinu --- .../02-multi-factor-authentication.md | 9 ++++ .../MultiFactor/App/AppAuthentication.php | 42 +++++++++++++------ .../App/AppAuthenticationChallengeTest.php | 38 +++++++++++++++++ 3 files changed, 77 insertions(+), 12 deletions(-) diff --git a/docs/07-users/02-multi-factor-authentication.md b/docs/07-users/02-multi-factor-authentication.md index ab2c7ce510..9cff3657bf 100644 --- a/docs/07-users/02-multi-factor-authentication.md +++ b/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). + + diff --git a/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php b/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php index 45150aaf7f..5599124eba 100644 --- a/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php +++ b/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; + })); } /** diff --git a/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php b/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php index 1528651060..cfb4fcc0f2 100644 --- a/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php +++ b/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 {