From 7ebc9b090568d5f60d0e21bf063a25262b27aedb Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Wed, 26 Aug 2026 06:05:29 +0200 Subject: [PATCH] Spend a recovery code once, the way the docblock says consumeRecoveryCode() reads the whole list, filters the used code out, and writes the whole list back. Two requests that both read before either writes each store their own copy, and the second write puts back the code the first removed. So a spent code comes back, and the same code offered twice is accepted twice -- while the method's first line says "each code works exactly once". Nobody gets in through this who was not already holding a valid code, so it is a promise not being kept rather than a door standing open. The promise is worth keeping anyway: it is the whole reason a printed sheet of recovery codes can be crossed off, and it is what makes a code that somebody watched being typed in stop working. The decision now comes from the row as it stands, re-read under a lock inside the transaction that writes it -- the shape SendNotificationDigest already uses to claim the rows it is about to delete. A conditional update, as in PublicShareController's downloads_count and the delivered_at claim in #1692, is the other precedent in the tree, but the column is `encrypted:array`: there is nothing in it a database can compare, so the comparison has to happen after decryption, under something that holds the row while it does. The lock is what makes it atomic against a request arriving at the same moment. The re-read is what makes the decision right, and it is the half a test can show: SQLite ignores lockForUpdate, so the accompanying tests pin the re-read and say so rather than claiming to prove the locking. config/database.php runs MySQL or Postgres in production, and both honour it. Saving through the caller's own instance keeps that instance in step with the row, so a caller cannot go on to decide from a list the database no longer has. --- .../Identity/TwoFactor/TwoFactorService.php | 44 +++++++-- .../Identity/RecoveryCodeSingleUseTest.php | 96 +++++++++++++++++++ 2 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 tests/Feature/Identity/RecoveryCodeSingleUseTest.php diff --git a/app/Modules/Identity/TwoFactor/TwoFactorService.php b/app/Modules/Identity/TwoFactor/TwoFactorService.php index 073869ac..b97f89a4 100644 --- a/app/Modules/Identity/TwoFactor/TwoFactorService.php +++ b/app/Modules/Identity/TwoFactor/TwoFactorService.php @@ -14,6 +14,7 @@ use BaconQrCode\Renderer\RendererStyle\Fill; use BaconQrCode\Renderer\RendererStyle\RendererStyle; use BaconQrCode\Writer; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Str; use PragmaRX\Google2FA\Google2FA; @@ -112,20 +113,45 @@ class TwoFactorService /** * Consume a recovery code; each code works exactly once. + * + * Read the list, filter it, write the whole list back is not once. + * Two requests that both read before either writes each store their + * own filtered copy, and the second write puts back the code the + * first removed -- so a spent code is available again, and the same + * code offered twice is accepted twice. Neither lets in anybody who + * was not already holding a code, which is why this is a promise not + * being kept rather than a door standing open. The promise is the + * sentence above, and it is the reason recovery codes are printed + * out and crossed off. + * + * Decide from the row as it stands, read back under a lock inside + * the transaction that writes it -- the same shape + * SendNotificationDigest uses to claim the rows it is about to + * delete. The lock is what makes it atomic against a request + * arriving at the same moment; the re-read is what makes the + * decision right, and it is the half that can be demonstrated in a + * test, since SQLite ignores lockForUpdate. + * + * The caller's own instance is what gets saved, so it does not walk + * away holding a list the database no longer has. */ public function consumeRecoveryCode(User $user, string $code): bool { - /** @var list|null $codes */ - $codes = $user->two_factor_recovery_codes; + return DB::transaction(function () use ($user, $code): bool { + $locked = User::query()->whereKey($user->getKey())->lockForUpdate()->first(); - if ($codes === null || ! in_array($code, $codes, true)) { - return false; - } + /** @var list|null $codes */ + $codes = $locked?->two_factor_recovery_codes; - $user->forceFill([ - 'two_factor_recovery_codes' => array_values(array_diff($codes, [$code])), - ])->save(); + if ($codes === null || ! in_array($code, $codes, true)) { + return false; + } - return true; + $user->forceFill([ + 'two_factor_recovery_codes' => array_values(array_diff($codes, [$code])), + ])->save(); + + return true; + }); } } diff --git a/tests/Feature/Identity/RecoveryCodeSingleUseTest.php b/tests/Feature/Identity/RecoveryCodeSingleUseTest.php new file mode 100644 index 00000000..cff9d62b --- /dev/null +++ b/tests/Feature/Identity/RecoveryCodeSingleUseTest.php @@ -0,0 +1,96 @@ +user = User::factory()->create(); + $this->user->forceFill([ + 'two_factor_secret' => 'SECRET', + 'two_factor_recovery_codes' => ['aaa-111', 'bbb-222', 'ccc-333'], + 'two_factor_confirmed_at' => now(), + ])->save(); + + $this->service = app(TwoFactorService::class); +}); + +function storedCodes(User $user): array +{ + /** @var list $codes */ + $codes = User::query()->findOrFail($user->id)->two_factor_recovery_codes ?? []; + + return $codes; +} + +test('spending two different codes at once does not put either back', function () { + $first = User::query()->findOrFail($this->user->id); + $second = User::query()->findOrFail($this->user->id); + + expect($this->service->consumeRecoveryCode($first, 'aaa-111'))->toBeTrue() + ->and($this->service->consumeRecoveryCode($second, 'bbb-222'))->toBeTrue() + ->and(storedCodes($this->user))->toBe(['ccc-333']); +}); + +test('the same code is not accepted twice', function () { + $first = User::query()->findOrFail($this->user->id); + $second = User::query()->findOrFail($this->user->id); + + expect($this->service->consumeRecoveryCode($first, 'aaa-111'))->toBeTrue() + ->and($this->service->consumeRecoveryCode($second, 'aaa-111'))->toBeFalse() + ->and(storedCodes($this->user))->toBe(['bbb-222', 'ccc-333']); +}); + +test('the caller instance is left holding what the database holds', function () { + $instance = User::query()->findOrFail($this->user->id); + + $this->service->consumeRecoveryCode($instance, 'aaa-111'); + + expect($instance->two_factor_recovery_codes)->toBe(['bbb-222', 'ccc-333']) + ->and($instance->isDirty())->toBeFalse(); +}); + +test('a code still works, once, in the ordinary case', function () { + expect($this->service->consumeRecoveryCode($this->user, 'bbb-222'))->toBeTrue() + ->and(storedCodes($this->user))->toBe(['aaa-111', 'ccc-333']) + ->and($this->service->consumeRecoveryCode($this->user, 'bbb-222'))->toBeFalse(); +}); + +test('a code nobody issued is refused, and an account with none at all', function () { + expect($this->service->consumeRecoveryCode($this->user, 'zzz-999'))->toBeFalse() + ->and(storedCodes($this->user))->toBe(['aaa-111', 'bbb-222', 'ccc-333']); + + $plain = User::factory()->create(); + + expect($this->service->consumeRecoveryCode($plain, 'aaa-111'))->toBeFalse(); +}); + +test('spending the last code empties the list', function () { + $this->user->forceFill(['two_factor_recovery_codes' => ['only-one']])->save(); + + expect($this->service->consumeRecoveryCode($this->user, 'only-one'))->toBeTrue() + ->and(storedCodes($this->user))->toBe([]); +}); + +test('the challenge screen still signs somebody in with one, and spends it', function () { + $this->withSession([ + SignIn::TWO_FACTOR_ID => $this->user->id, + SignIn::TWO_FACTOR_REMEMBER => false, + ])->post('/two-factor-challenge', ['recovery_code' => ' aaa-111 ']) + ->assertRedirect(route('dashboard', absolute: false)); + + $this->assertAuthenticatedAs($this->user); + expect(storedCodes($this->user))->toBe(['bbb-222', 'ccc-333']); +});