diff --git a/packages/panels/src/Auth/MultiFactor/App/Actions/DisableAppAuthenticationAction.php b/packages/panels/src/Auth/MultiFactor/App/Actions/DisableAppAuthenticationAction.php index a64e188babb..4159f4aadd4 100644 --- a/packages/panels/src/Auth/MultiFactor/App/Actions/DisableAppAuthenticationAction.php +++ b/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; } diff --git a/packages/panels/src/Auth/MultiFactor/App/Actions/RegenerateAppAuthenticationRecoveryCodesAction.php b/packages/panels/src/Auth/MultiFactor/App/Actions/RegenerateAppAuthenticationRecoveryCodesAction.php index 8d0620f4878..ae04e913e1c 100644 --- a/packages/panels/src/Auth/MultiFactor/App/Actions/RegenerateAppAuthenticationRecoveryCodesAction.php +++ b/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; } diff --git a/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php b/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php index 12d01f1087a..c4fd30ab668 100644 --- a/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php +++ b/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 diff --git a/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php b/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php index 4018530e00e..173ea3ed0fe 100644 --- a/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php +++ b/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 */ + protected array $data = []; + + public function get($key): mixed + { + return $this->data[$key] ?? null; + } + + /** + * @param array $keys + * @return array + */ + 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 $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 ''; + } +}