From 3a67aa99ec16c6fa0a025222cda4f88984d90201 Mon Sep 17 00:00:00 2001 From: boban Date: Sun, 14 Jun 2026 17:04:12 +0200 Subject: [PATCH] fix(auth): rotate remember_token on reset + atomic recovery-code use Address Codex security review: - ForgotPassword + clusev:reset-admin now rotate remember_token, so a stolen remember-me cookie cannot survive a password reset (the email-token path already did this). - useRecoveryCode reads/checks/removes under a row lock (DB transaction + lockForUpdate), so two concurrent requests can't both spend the same one-time code (replay). Co-Authored-By: Claude Opus 4.8 (1M context) --- app/Console/Commands/ResetAdmin.php | 7 ++++++- app/Livewire/Auth/ForgotPassword.php | 8 +++++++- app/Models/User.php | 29 +++++++++++++++++++--------- tests/Feature/ForgotPasswordTest.php | 5 ++++- 4 files changed, 37 insertions(+), 12 deletions(-) diff --git a/app/Console/Commands/ResetAdmin.php b/app/Console/Commands/ResetAdmin.php index d3b4739..b00b3c6 100644 --- a/app/Console/Commands/ResetAdmin.php +++ b/app/Console/Commands/ResetAdmin.php @@ -35,7 +35,12 @@ class ResetAdmin extends Command } $password = (string) ($this->option('password') ?: Str::password(20)); - $user->forceFill(['password' => Hash::make($password), 'must_change_password' => false]); + // Rotate remember_token so any previously issued remember-me cookie is revoked. + $user->forceFill([ + 'password' => Hash::make($password), + 'must_change_password' => false, + 'remember_token' => Str::random(60), + ]); if ($this->option('disable-2fa')) { $user->forceFill([ diff --git a/app/Livewire/Auth/ForgotPassword.php b/app/Livewire/Auth/ForgotPassword.php index 6fd8dea..5f249cb 100644 --- a/app/Livewire/Auth/ForgotPassword.php +++ b/app/Livewire/Auth/ForgotPassword.php @@ -6,6 +6,7 @@ use App\Models\AuditEvent; use App\Models\User; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\RateLimiter; +use Illuminate\Support\Str; use Illuminate\Validation\Rules\Password; use Illuminate\Validation\ValidationException; use Livewire\Attributes\Layout; @@ -73,7 +74,12 @@ class ForgotPassword extends Component } RateLimiter::clear($key); - $user->forceFill(['password' => Hash::make($this->password), 'must_change_password' => false])->save(); + // Rotate remember_token too, so a stolen remember-me cookie can't survive the reset. + $user->forceFill([ + 'password' => Hash::make($this->password), + 'must_change_password' => false, + 'remember_token' => Str::random(60), + ])->save(); AuditEvent::create([ 'user_id' => $user->id, diff --git a/app/Models/User.php b/app/Models/User.php index 127251d..1aae113 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -8,6 +8,7 @@ use Illuminate\Database\Eloquent\Attributes\Hidden; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Foundation\Auth\User as Authenticatable; use Illuminate\Notifications\Notifiable; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Str; #[Fillable(['name', 'email', 'password', 'must_change_password', 'locale'])] @@ -53,21 +54,31 @@ class User extends Authenticatable return count($this->recoveryCodes()) > 0; } - /** Consume one recovery code (one-time). Returns true if it was valid + removed. */ + /** + * Consume one recovery code (one-time). Returns true if it was valid + removed. + * Atomic: the read/check/remove runs under a row lock so two concurrent requests + * can never both spend the same code (replay). + */ public function useRecoveryCode(string $code): bool { $code = trim($code); - $codes = $this->recoveryCodes(); - if (! in_array($code, $codes, true)) { - return false; - } + return DB::transaction(function () use ($code) { + $locked = static::whereKey($this->getKey())->lockForUpdate()->first(); + $codes = $locked?->recoveryCodes() ?? []; - $this->forceFill([ - 'two_factor_recovery_codes' => array_values(array_filter($codes, fn ($c) => $c !== $code)), - ])->save(); + if (! in_array($code, $codes, true)) { + return false; + } - return true; + $remaining = array_values(array_filter($codes, fn ($c) => $c !== $code)); + $locked->forceFill(['two_factor_recovery_codes' => $remaining])->save(); + + // Keep the in-memory instance consistent for the rest of the request. + $this->setRawAttributes($locked->getAttributes(), true); + + return true; + }); } public function hasTwoFactorEnabled(): bool diff --git a/tests/Feature/ForgotPasswordTest.php b/tests/Feature/ForgotPasswordTest.php index 2dce51d..88a985d 100644 --- a/tests/Feature/ForgotPasswordTest.php +++ b/tests/Feature/ForgotPasswordTest.php @@ -23,9 +23,10 @@ class ForgotPasswordTest extends TestCase ]); } - public function test_totp_resets_password(): void + public function test_totp_resets_password_and_rotates_remember_token(): void { $user = $this->user(); + $user->forceFill(['remember_token' => 'stale-cookie-token'])->save(); $otp = (new Google2FA)->getCurrentOtp($user->two_factor_secret); Livewire::test(ForgotPassword::class) @@ -34,6 +35,8 @@ class ForgotPasswordTest extends TestCase ->call('resetPassword')->assertHasNoErrors(); $this->assertTrue(Hash::check('NewPassw0rd123', $user->fresh()->password)); + // a stolen remember-me cookie must not survive the reset + $this->assertNotSame('stale-cookie-token', $user->fresh()->remember_token); } public function test_backup_code_resets_and_is_consumed(): void