From 9a6c09e48893c747b80e914c67147163a5ad9a44 Mon Sep 17 00:00:00 2001 From: boban Date: Mon, 15 Jun 2026 06:21:45 +0200 Subject: [PATCH] fix(2fa): close two re-view holes in backup-codes show-once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stored recovery codes were re-viewable two ways after their one-time reveal. Both paths are now closed so codes are only viewable/downloadable right after a fresh generation (enrollment or regenerate). HOLE 1 — RecoveryCodes::$revealed was a mutable public Livewire prop, so a crafted /livewire/update could flip it to true and make render() emit stored codes. Annotated with #[Locked]; Livewire now rejects any client-side write (set still happens server-side in mount()/regenerate()). HOLE 2 — two-factor.recovery.download was auth-gated only, so any authed user could GET it anytime to re-download stored codes. Added a one-time session grant (2fa.download_grant) put alongside the existing 2fa.codes_fresh wherever codes are freshly generated (TwoFactorSetup, WebauthnKeys, RecoveryCodes::regenerate); the route now abort_unless(pull(grant), 403) before streaming — one download per fresh generation, later direct hits 403. Tests: lock rejection + download-grant lifecycle (forbidden → granted ok → consumed forbidden). Full suite 137 passed. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/Livewire/Auth/TwoFactorSetup.php | 3 ++ app/Livewire/Modals/RecoveryCodes.php | 8 +++++ app/Livewire/Settings/WebauthnKeys.php | 3 ++ routes/web.php | 5 +++ tests/Feature/RecoveryCodesModalTest.php | 41 ++++++++++++++++++++++++ 5 files changed, 60 insertions(+) diff --git a/app/Livewire/Auth/TwoFactorSetup.php b/app/Livewire/Auth/TwoFactorSetup.php index 6d63a85..4c6fbd0 100644 --- a/app/Livewire/Auth/TwoFactorSetup.php +++ b/app/Livewire/Auth/TwoFactorSetup.php @@ -54,6 +54,9 @@ class TwoFactorSetup extends Component // put (not flash): must survive the redirect AND the later openModal Livewire // request; the modal pulls it on mount to reveal the freshly generated codes once. session()->put('2fa.codes_fresh', true); + // One-time download grant — the download route requires + consumes it, so the + // fresh set can be downloaded once and a later direct hit 403s. + session()->put('2fa.download_grant', true); } return $this->redirect(route('settings'), navigate: true); diff --git a/app/Livewire/Modals/RecoveryCodes.php b/app/Livewire/Modals/RecoveryCodes.php index e6db92b..3bb54ed 100644 --- a/app/Livewire/Modals/RecoveryCodes.php +++ b/app/Livewire/Modals/RecoveryCodes.php @@ -4,6 +4,7 @@ namespace App\Livewire\Modals; use App\Models\AuditEvent; use Illuminate\Support\Facades\Auth; +use Livewire\Attributes\Locked; use LivewireUI\Modal\ModalComponent; /** @@ -17,7 +18,10 @@ class RecoveryCodes extends ModalComponent * "Backup-Codes verwalten". Set true by a one-time server flag on mount (first-factor * enrollment) or by regenerate(). A client cannot tamper this into showing stored codes: * render() only emits codes when $revealed, and $revealed is only set server-side. + * #[Locked] rejects any client-side update of this prop, so a crafted /livewire/update + * cannot flip it to re-view stored codes. */ + #[Locked] public bool $revealed = false; public static function modalMaxWidth(): string @@ -50,6 +54,10 @@ class RecoveryCodes extends ModalComponent // The freshly generated set may be shown once in this same modal instance. $this->revealed = true; + // One-time download grant for the freshly regenerated set (consumed by the + // download route). Without it a later direct hit of the route 403s. + session()->put('2fa.download_grant', true); + $this->dispatch('notify', message: __('auth.recovery_regenerated')); } diff --git a/app/Livewire/Settings/WebauthnKeys.php b/app/Livewire/Settings/WebauthnKeys.php index 43c6f8e..7d6cd46 100644 --- a/app/Livewire/Settings/WebauthnKeys.php +++ b/app/Livewire/Settings/WebauthnKeys.php @@ -40,6 +40,9 @@ class WebauthnKeys extends Component Auth::user()->replaceRecoveryCodes(); // Server-set one-time flag so the opening modal reveals the fresh set once. session()->put('2fa.codes_fresh', true); + // One-time download grant — the download route requires + consumes it, so the + // fresh set can be downloaded once and a later direct hit 403s. + session()->put('2fa.download_grant', true); $this->dispatch('openModal', component: 'modals.recovery-codes'); } diff --git a/routes/web.php b/routes/web.php index 137a50c..093fda7 100644 --- a/routes/web.php +++ b/routes/web.php @@ -55,6 +55,11 @@ Route::middleware('auth')->group(function () { // 2FA backup-codes download — the codes themselves are shown in a modal (Modals\RecoveryCodes). Route::get('/two-factor/recovery-codes/download', function () { + // One download per fresh generation: a one-time grant is put alongside the codes + // when they are freshly generated/regenerated. pull() consumes it, so re-downloading + // stored codes by hitting this route directly 403s. + abort_unless(session()->pull('2fa.download_grant', false), 403); + $codes = AuthFacade::user()->recoveryCodes(); return response()->streamDownload( diff --git a/tests/Feature/RecoveryCodesModalTest.php b/tests/Feature/RecoveryCodesModalTest.php index 7374548..4fa037d 100644 --- a/tests/Feature/RecoveryCodesModalTest.php +++ b/tests/Feature/RecoveryCodesModalTest.php @@ -6,6 +6,7 @@ use App\Livewire\Modals\RecoveryCodes; use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Route; +use Livewire\Features\SupportLockedProperties\CannotUpdateLockedPropertyException; use Livewire\Livewire; use Tests\TestCase; @@ -61,6 +62,46 @@ class RecoveryCodesModalTest extends TestCase ->assertSee($user->fresh()->recoveryCodes()[0]); } + public function test_revealed_property_is_locked_against_client_tampering(): void + { + $user = User::factory()->create(); + $codes = $user->replaceRecoveryCodes(); + + // Manage view (no fresh flag): codes stay hidden. A crafted client update that flips + // the #[Locked] $revealed prop must be rejected — Livewire throws on a locked set. + $component = Livewire::actingAs($user)->test(RecoveryCodes::class) + ->assertSet('revealed', false) + ->assertDontSee($codes[0]); + + $this->expectException(CannotUpdateLockedPropertyException::class); + $component->set('revealed', true); + } + + public function test_download_requires_a_fresh_grant(): void + { + $user = User::factory()->create(); + $user->replaceRecoveryCodes(); + + // Without a fresh-generation grant, a direct hit of the download route is forbidden, + // so stored codes can never be re-downloaded by an authed user at will. + $this->actingAs($user) + ->get(route('two-factor.recovery.download')) + ->assertForbidden(); + + // With the one-time grant (set when codes are freshly generated/regenerated) the + // download succeeds exactly once. + session(['2fa.download_grant' => true]); + $this->actingAs($user) + ->get(route('two-factor.recovery.download')) + ->assertOk() + ->assertDownload('clusev-recovery-codes.txt'); + + // The grant is single-use: pull() consumed it, so an immediate re-download 403s. + $this->actingAs($user) + ->get(route('two-factor.recovery.download')) + ->assertForbidden(); + } + public function test_old_recovery_route_is_gone(): void { $this->assertFalse(Route::has('two-factor.recovery'));