From 672143e15af8daa9ec87b4e2e0b230b6f4c82d77 Mon Sep 17 00:00:00 2001 From: Dan Harrin Date: Wed, 5 Aug 2026 14:39:34 +0100 Subject: [PATCH 1/4] fix: Cache used MFA codes using hash of secret rather than secret and code --- .../MultiFactor/App/AppAuthentication.php | 16 ++++++++------- .../App/AppAuthenticationChallengeTest.php | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php b/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php index 12d01f1087a..a48a00fa0e3 100644 --- a/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php +++ b/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php @@ -178,21 +178,23 @@ public function verifyCode(#[SensitiveParameter] string $code, #[SensitiveParame return $this->google2FA->verifyKey($secret, $code, $this->getCodeWindow()); } - $cacheKey = 'filament.app_authentication_codes.' . md5($secret . $code); + $cacheKey = 'filament.app_authentication_codes.' . md5($secret); - $timestamp = $this->google2FA->verifyKeyNewer($secret, $code, cache()->get($cacheKey), $this->getCodeWindow()); + return Cache::lock("{$cacheKey}.lock", 10)->block(10, 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; - } - - return false; + }); } 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..70b8ed719ed 100644 --- a/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php +++ b/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php @@ -9,6 +9,7 @@ use Illuminate\Support\Arr; use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\Str; +use PragmaRX\Google2FAQRCode\Google2FA; use function Filament\Tests\livewire; @@ -762,6 +763,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()); From 97abd3658ac4d001f95e09f3e92908cb8b0a352b Mon Sep 17 00:00:00 2001 From: Dan Harrin Date: Wed, 5 Aug 2026 15:03:18 +0100 Subject: [PATCH 2/4] support non-locking drivers --- .../MultiFactor/App/AppAuthentication.php | 18 ++- .../App/AppAuthenticationChallengeTest.php | 107 ++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php b/packages/panels/src/Auth/MultiFactor/App/AppAuthentication.php index a48a00fa0e3..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,9 +179,13 @@ public function verifyCode(#[SensitiveParameter] string $code, #[SensitiveParame return $this->google2FA->verifyKey($secret, $code, $this->getCodeWindow()); } + // 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); - return Cache::lock("{$cacheKey}.lock", 10)->block(10, function () use ($cacheKey, $code, $secret): bool { + $verifyCode = function () use ($cacheKey, $code, $secret): bool { $timestamp = $this->google2FA->verifyKeyNewer($secret, $code, Cache::get($cacheKey), $this->getCodeWindow()); if ($timestamp === false) { @@ -194,7 +199,16 @@ public function verifyCode(#[SensitiveParameter] string $code, #[SensitiveParame 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 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 70b8ed719ed..173ea3ed0fe 100644 --- a/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php +++ b/tests/src/Panels/Auth/MultiFactor/App/AppAuthenticationChallengeTest.php @@ -6,7 +6,10 @@ 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; @@ -798,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 ''; + } +} From 7ae381c1b80bd9333b0bfa5cc83b6861eb44196f Mon Sep 17 00:00:00 2001 From: Dan Harrin Date: Wed, 5 Aug 2026 15:23:47 +0100 Subject: [PATCH 3/4] consistency --- .../MultiFactor/App/Actions/DisableAppAuthenticationAction.php | 2 +- .../Actions/RegenerateAppAuthenticationRecoveryCodesAction.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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; } From 13fa727489e8979f52279d2205a7d991862956b4 Mon Sep 17 00:00:00 2001 From: Dan Harrin Date: Wed, 5 Aug 2026 15:38:06 +0100 Subject: [PATCH 4/4] fix: return an array from `SpatieTagsEntry`/`SpatieTagsColumn` `getState()` when state is not a Collection or array The early-return branch returned the raw state from a method typed `array`, which threw a `TypeError` for any truthy scalar state. PHPStan 2.2.8 narrows `mixed` more precisely and surfaced this, failing CI on every branch. --- .../src/Infolists/Components/SpatieTagsEntry.php | 2 +- .../src/Tables/Columns/SpatieTagsColumn.php | 2 +- .../Fixtures/Livewire/SpatieTagsColumnTable.php | 6 ++++++ .../src/SpatieTagsPlugin/SpatieTagsColumnTest.php | 7 +++++++ tests/src/SpatieTagsPlugin/SpatieTagsEntryTest.php | 14 ++++++++++++++ 5 files changed, 29 insertions(+), 2 deletions(-) diff --git a/packages/spatie-laravel-tags-plugin/src/Infolists/Components/SpatieTagsEntry.php b/packages/spatie-laravel-tags-plugin/src/Infolists/Components/SpatieTagsEntry.php index 603f7c79c81..1e051e14f77 100644 --- a/packages/spatie-laravel-tags-plugin/src/Infolists/Components/SpatieTagsEntry.php +++ b/packages/spatie-laravel-tags-plugin/src/Infolists/Components/SpatieTagsEntry.php @@ -29,7 +29,7 @@ public function getState(): array $state = parent::getState(); if ($state && (! $state instanceof Collection) && (! is_array($state))) { - return $state; + return Arr::wrap($state); } $record = $this->getRecord(); diff --git a/packages/spatie-laravel-tags-plugin/src/Tables/Columns/SpatieTagsColumn.php b/packages/spatie-laravel-tags-plugin/src/Tables/Columns/SpatieTagsColumn.php index 12abeba2bdf..c4b3e65aef6 100644 --- a/packages/spatie-laravel-tags-plugin/src/Tables/Columns/SpatieTagsColumn.php +++ b/packages/spatie-laravel-tags-plugin/src/Tables/Columns/SpatieTagsColumn.php @@ -32,7 +32,7 @@ public function getState(): array $state = parent::getState(); if ($state && (! $state instanceof Collection) && (! is_array($state))) { - return $state; + return Arr::wrap($state); } $record = $this->getRecord(); diff --git a/tests/src/Fixtures/Livewire/SpatieTagsColumnTable.php b/tests/src/Fixtures/Livewire/SpatieTagsColumnTable.php index 22d6585c378..87fadd06b85 100644 --- a/tests/src/Fixtures/Livewire/SpatieTagsColumnTable.php +++ b/tests/src/Fixtures/Livewire/SpatieTagsColumnTable.php @@ -23,6 +23,8 @@ class SpatieTagsColumnTable extends Component implements HasActions, HasSchemas, public ?string $tagType = null; + public ?string $customState = null; + protected function getTableQuery(): Builder { return Article::query(); @@ -36,6 +38,10 @@ public function table(Table $table): Table $column->type($this->tagType); } + if ($this->customState !== null) { + $column->state($this->customState); + } + return $table ->columns([$column]); } diff --git a/tests/src/SpatieTagsPlugin/SpatieTagsColumnTest.php b/tests/src/SpatieTagsPlugin/SpatieTagsColumnTest.php index e67b447a449..5fe8c1302a0 100644 --- a/tests/src/SpatieTagsPlugin/SpatieTagsColumnTest.php +++ b/tests/src/SpatieTagsPlugin/SpatieTagsColumnTest.php @@ -102,6 +102,13 @@ ->assertCanRenderTableColumn('tags'); }); + it('wraps a non-array custom state in an array', function (): void { + $record = Article::factory()->create(); + + livewire(SpatieTagsColumnTable::class, ['customState' => 'Laravel']) + ->assertTableColumnStateSet('tags', ['Laravel'], record: $record); + }); + it('can render column with typed tags', function (): void { $record = Article::factory()->create(); $record->attachTag('Laravel', 'framework'); diff --git a/tests/src/SpatieTagsPlugin/SpatieTagsEntryTest.php b/tests/src/SpatieTagsPlugin/SpatieTagsEntryTest.php index de63ca35350..d3d6eea2dd1 100644 --- a/tests/src/SpatieTagsPlugin/SpatieTagsEntryTest.php +++ b/tests/src/SpatieTagsPlugin/SpatieTagsEntryTest.php @@ -138,6 +138,20 @@ expect($state)->toBe([]); }); + it('wraps a non-array custom state in an array', function (): void { + $record = Article::factory()->create(); + $record->load('tags'); + + $entry = SpatieTagsEntry::make('tags') + ->state('Laravel') + ->container( + Schema::make(Livewire::make()) + ->record($record) + ); + + expect($entry->getState())->toBe(['Laravel']); + }); + it('deduplicates tag names in state', function (): void { $record = Article::factory()->create(); $record->attachTags(['Laravel', 'PHP']);