From 164145463c45de17fc25b2d7031ec14291981f79 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 18:09:22 +0200 Subject: [PATCH] Send an operator with an expired session to sign in, not a 500 VpnConfigAccess::reveal() dereferenced Auth::guard('operator')->user() unguarded, which is null when a modal's session has ended since it was opened (a Livewire action reaches its component through /livewire/update on its own, without the page's original route middleware). The same unguarded pattern was in TwoFactorSetup, InstanceAdminAccess and Settings. Add App\Livewire\Concerns\ResolvesOperator with currentOperator(), the one place that resolves the operator and redirects to admin.login when there isn't one, and use it everywhere the pattern occurred. Co-Authored-By: Claude Opus 5 --- app/Livewire/Admin/InstanceAdminAccess.php | 10 ++-- app/Livewire/Admin/Settings.php | 54 +++++++++++++------ app/Livewire/Admin/TwoFactorSetup.php | 37 ++++++++++--- app/Livewire/Admin/VpnConfigAccess.php | 10 ++-- app/Livewire/Concerns/ResolvesOperator.php | 52 ++++++++++++++++++ .../Admin/OperatorTwoFactorEnrollmentTest.php | 16 ++++++ tests/Feature/Admin/VpnTest.php | 28 ++++++++++ 7 files changed, 180 insertions(+), 27 deletions(-) create mode 100644 app/Livewire/Concerns/ResolvesOperator.php diff --git a/app/Livewire/Admin/InstanceAdminAccess.php b/app/Livewire/Admin/InstanceAdminAccess.php index 0115aaf..86a9951 100644 --- a/app/Livewire/Admin/InstanceAdminAccess.php +++ b/app/Livewire/Admin/InstanceAdminAccess.php @@ -2,10 +2,10 @@ namespace App\Livewire\Admin; +use App\Livewire\Concerns\ResolvesOperator; use App\Models\Instance; use App\Provisioning\Jobs\IssueInstanceAdminAccess; use App\Services\Wireguard\ConfigHandoff; -use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\Str; @@ -25,6 +25,8 @@ use LivewireUI\Modal\ModalComponent; */ class InstanceAdminAccess extends ModalComponent { + use ResolvesOperator; + public string $uuid; public string $subdomain = ''; @@ -55,9 +57,11 @@ class InstanceAdminAccess extends ModalComponent $this->authorize('instances.adminlogin'); $this->resetErrorBag('password'); - $operator = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } - $key = 'instance-admin:'.$operator?->id; + $key = 'instance-admin:'.$operator->id; if (RateLimiter::tooManyAttempts($key, 5)) { $this->addError('password', __('instances.too_many_attempts', ['seconds' => RateLimiter::availableIn($key)])); diff --git a/app/Livewire/Admin/Settings.php b/app/Livewire/Admin/Settings.php index 39dfb11..454bf88 100644 --- a/app/Livewire/Admin/Settings.php +++ b/app/Livewire/Admin/Settings.php @@ -4,6 +4,7 @@ namespace App\Livewire\Admin; use App\Http\Middleware\RestrictConsoleNetwork; use App\Livewire\Concerns\ChangesOwnPassword; +use App\Livewire\Concerns\ResolvesOperator; use App\Models\Customer; use App\Models\Operator; use App\Models\VpnPeer; @@ -34,6 +35,7 @@ use Symfony\Component\HttpFoundation\IpUtils; class Settings extends Component { use ChangesOwnPassword; + use ResolvesOperator; /** This page changes the signed-in OPERATOR's password, never a portal one. */ protected function passwordAccountGuard(): string @@ -65,8 +67,12 @@ class Settings extends Component public function mount(): void { - $this->name = Auth::guard('operator')->user()->name; - $this->email = Auth::guard('operator')->user()->email; + if (! $operator = $this->currentOperator()) { + return; + } + + $this->name = $operator->name; + $this->email = $operator->email; $this->requireTwoFactor = AppSettings::bool('console.require_2fa', false); } @@ -89,10 +95,13 @@ class Settings extends Component public function saveAccount(): void { - $user = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } + $data = $this->validate([ 'name' => 'required|string|max:255', - 'email' => 'required|email|max:255|unique:operators,email,'.$user->id, + 'email' => 'required|email|max:255|unique:operators,email,'.$operator->id, ]); // An operator email must never collide with a customer's — that would @@ -105,13 +114,18 @@ class Settings extends Component return; } - $user->update($data); + $operator->update($data); $this->dispatch('notify', message: __('admin_settings.account_saved')); } public function inviteStaff(): void { $this->authorize('staff.manage'); + + if (! $operator = $this->currentOperator()) { + return; + } + $data = $this->validate([ 'staffName' => 'required|string|max:255', 'staffEmail' => 'required|email|max:255|unique:operators,email', @@ -119,7 +133,7 @@ class Settings extends Component ]); // Only an Owner may create another Owner. - if ($data['staffRole'] === 'Owner' && ! Auth::guard('operator')->user()->hasRole('Owner')) { + if ($data['staffRole'] === 'Owner' && ! $operator->hasRole('Owner')) { $this->addError('staffRole', __('admin_settings.owner_only')); return; @@ -135,12 +149,12 @@ class Settings extends Component // password and surface it once to the Owner to share securely — the // account is usable immediately (a proper invite link follows with mail). $temp = Str::password(14); - $operator = Operator::create([ + $newStaff = Operator::create([ 'name' => $data['staffName'], 'email' => $data['staffEmail'], 'password' => Hash::make($temp), ]); - $operator->assignRole($data['staffRole']); + $newStaff->assignRole($data['staffRole']); $this->invitedEmail = $data['staffEmail']; $this->invitedPassword = $temp; @@ -156,7 +170,11 @@ class Settings extends Component return; } - $result = DB::transaction(function () use ($id, $role) { + if (! $operator = $this->currentOperator()) { + return; + } + + $result = DB::transaction(function () use ($id, $role, $operator) { // Serialize on the Owner role so the global owner count can't be // raced to zero by concurrent demotions of different owners. Role::query()->where('name', 'Owner')->lockForUpdate()->first(); @@ -165,7 +183,7 @@ class Settings extends Component if ($target === null) { return 'gone'; } - if ($target->id === Auth::guard('operator')->id()) { + if ($target->id === $operator->id) { return 'self'; } // Only existing operators may be re-roled. The Customer check below @@ -176,7 +194,7 @@ class Settings extends Component return 'not_staff'; } // Granting or revoking Owner is Owner-only. - if (($role === 'Owner' || $target->hasRole('Owner')) && ! Auth::guard('operator')->user()->hasRole('Owner')) { + if (($role === 'Owner' || $target->hasRole('Owner')) && ! $operator->hasRole('Owner')) { return 'owner_only'; } // Never demote the last Owner. @@ -377,7 +395,11 @@ class Settings extends Component { $this->authorize('site.manage'); - $accepted = app(UpdateChannel::class)->request(Auth::guard('operator')->user()->email); + if (! $operator = $this->currentOperator()) { + return; + } + + $accepted = app(UpdateChannel::class)->request($operator->email); $this->dispatch('notify', message: __($accepted ? 'admin_settings.update_requested' @@ -386,6 +408,8 @@ class Settings extends Component public function render() { + $operator = $this->currentOperator(); + $staff = Operator::query() ->whereHas('roles', fn ($q) => $q->whereIn('name', Operator::OPERATOR_ROLES)) ->with('roles') @@ -403,7 +427,7 @@ class Settings extends Component 'update' => app(UpdateChannel::class)->state(), 'updateLog' => app(UpdateChannel::class)->lastLog(), 'sitePublic' => AppSettings::bool('site.public', true), - 'canManageSite' => Auth::guard('operator')->user()?->can('site.manage') ?? false, + 'canManageSite' => $operator?->can('site.manage') ?? false, // Shown so nobody has to guess why they still see the real site. 'viewerOnVpn' => IpUtils::checkIp( (string) request()->ip(), @@ -417,8 +441,8 @@ class Settings extends Component 'viewerIp' => (string) request()->ip(), 'staff' => $staff, 'roles' => Operator::OPERATOR_ROLES, - 'canManageStaff' => Auth::guard('operator')->user()->can('staff.manage'), - 'isOwner' => Auth::guard('operator')->user()->hasRole('Owner'), + 'canManageStaff' => $operator?->can('staff.manage') ?? false, + 'isOwner' => $operator?->hasRole('Owner') ?? false, ]); } } diff --git a/app/Livewire/Admin/TwoFactorSetup.php b/app/Livewire/Admin/TwoFactorSetup.php index 1b9343f..0bb2c97 100644 --- a/app/Livewire/Admin/TwoFactorSetup.php +++ b/app/Livewire/Admin/TwoFactorSetup.php @@ -3,7 +3,7 @@ namespace App\Livewire\Admin; use App\Livewire\Concerns\ConfirmsPassword; -use Illuminate\Support\Facades\Auth; +use App\Livewire\Concerns\ResolvesOperator; use Illuminate\Validation\ValidationException; use Laravel\Fortify\Actions\ConfirmTwoFactorAuthentication; use Laravel\Fortify\Actions\DisableTwoFactorAuthentication; @@ -38,6 +38,7 @@ use Livewire\Component; class TwoFactorSetup extends Component { use ConfirmsPassword; + use ResolvesOperator; /** Two-factor is confirmed against the operator's own record, not a portal one. */ protected function confirmationGuard(): string @@ -61,7 +62,11 @@ class TwoFactorSetup extends Component { $this->requireConfirmedPassword(); - app(EnableTwoFactorAuthentication::class)(Auth::guard('operator')->user()); + if (! $operator = $this->currentOperator()) { + return; + } + + app(EnableTwoFactorAuthentication::class)($operator); } /** Accept a code from the app, which is what actually turns it on. */ @@ -69,7 +74,9 @@ class TwoFactorSetup extends Component { $this->requireConfirmedPassword(); - $operator = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } try { app(ConfirmTwoFactorAuthentication::class)( @@ -94,7 +101,10 @@ class TwoFactorSetup extends Component { $this->requireConfirmedPassword(); - $operator = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } + app(GenerateNewRecoveryCodes::class)($operator); $this->recoveryCodes = json_decode(decrypt($operator->refresh()->two_factor_recovery_codes), true); } @@ -103,7 +113,11 @@ class TwoFactorSetup extends Component { $this->requireConfirmedPassword(); - app(DisableTwoFactorAuthentication::class)(Auth::guard('operator')->user()); + if (! $operator = $this->currentOperator()) { + return; + } + + app(DisableTwoFactorAuthentication::class)($operator); $this->recoveryCodes = null; $this->dispatch('notify', message: __('two_factor_setup.off')); } @@ -122,7 +136,18 @@ class TwoFactorSetup extends Component public function render() { - $operator = Auth::guard('operator')->user(); + // Unlike the actions above, render() has no requireConfirmedPassword() + // gate in front of it — Livewire calls it on every update, including a + // bare property sync, so it is its own reachable path for a session + // that ended after the page loaded. + if (! $operator = $this->currentOperator()) { + return view('livewire.admin.two-factor-setup', [ + 'twoFactorPending' => false, + 'twoFactorOn' => false, + 'twoFactorQr' => null, + 'passwordConfirmed' => false, + ]); + } return view('livewire.admin.two-factor-setup', [ // Never the secret itself — only whether it exists, and the SVG diff --git a/app/Livewire/Admin/VpnConfigAccess.php b/app/Livewire/Admin/VpnConfigAccess.php index 52972fd..310b558 100644 --- a/app/Livewire/Admin/VpnConfigAccess.php +++ b/app/Livewire/Admin/VpnConfigAccess.php @@ -2,11 +2,11 @@ namespace App\Livewire\Admin; +use App\Livewire\Concerns\ResolvesOperator; use App\Models\VpnPeer; use App\Services\Wireguard\ConfigHandoff; use App\Services\Wireguard\ConfigVault; use App\Services\Wireguard\QrCode; -use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Gate; use Illuminate\Support\Facades\Hash; @@ -25,6 +25,8 @@ use LivewireUI\Modal\ModalComponent; */ class VpnConfigAccess extends ModalComponent { + use ResolvesOperator; + public string $uuid; public string $name = ''; @@ -70,10 +72,12 @@ class VpnConfigAccess extends ModalComponent $peer = VpnPeer::query()->where('uuid', $this->uuid)->firstOrFail(); $this->authorize('downloadConfig', $peer); - $operator = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } // Rate-limited per user, so a stolen session cannot grind the password. - $key = 'vpn-config:'.$operator?->id; + $key = 'vpn-config:'.$operator->id; if (RateLimiter::tooManyAttempts($key, 5)) { $this->addError('password', __('vpn.too_many_attempts', ['seconds' => RateLimiter::availableIn($key)])); diff --git a/app/Livewire/Concerns/ResolvesOperator.php b/app/Livewire/Concerns/ResolvesOperator.php new file mode 100644 index 0000000..5e0df8a --- /dev/null +++ b/app/Livewire/Concerns/ResolvesOperator.php @@ -0,0 +1,52 @@ +user() + * comes back null. Four components each dereferenced it unguarded + * ($operator->password, ->id, ->two_factor_recovery_codes, …), turning that + * into a 500 instead of a sign-in prompt. + * + * currentOperator() cannot throw its way out of the null case. Livewire only + * turns $this->redirect() into a browser navigation once the calling action + * returns NORMALLY: the redirect is queued as component state and picked up + * during Livewire's own dehydrate step (Livewire\Features\SupportRedirects), + * which runs strictly after the called method returns — an uncaught + * exception during the call skips it entirely and Livewire ships back a bare + * error response instead (confirmed by reading + * Livewire\Mechanisms\HandleComponents::update() and + * Livewire\Mechanisms\HandleRequests::handleUpdate(), which catches nothing + * but \TypeError). So this returns null and every caller checks for it: + * + * if (! $operator = $this->currentOperator()) { + * return; + * } + * + * That one-line guard is the only thing left for a caller to get wrong; the + * redirect target and the mechanism live here once, so the four cannot drift + * apart the way the original unguarded dereferences did. + */ +trait ResolvesOperator +{ + protected function currentOperator(): ?Operator + { + $operator = Auth::guard('operator')->user(); + + if ($operator === null) { + $this->redirect(route('admin.login')); + } + + return $operator; + } +} diff --git a/tests/Feature/Admin/OperatorTwoFactorEnrollmentTest.php b/tests/Feature/Admin/OperatorTwoFactorEnrollmentTest.php index cde5d85..69d6a28 100644 --- a/tests/Feature/Admin/OperatorTwoFactorEnrollmentTest.php +++ b/tests/Feature/Admin/OperatorTwoFactorEnrollmentTest.php @@ -1,6 +1,7 @@ refresh()->two_factor_confirmed_at)->toBeNull() ->and($op->two_factor_secret)->toBeNull(); }); + +it('sends the operator to sign in instead of a 500 when the session has ended', function () { + // render() has no requireConfirmedPassword() gate in front of it — unlike + // every action above, Livewire calls it on every update including a bare + // property sync, so it is directly reachable with no other guard in the + // way once the session that owned the page has ended. + $op = operator('Support'); + + $page = Livewire::actingAs($op, 'operator')->test(TwoFactorSetup::class); + + // The page sat open long enough for the operator's own session to end. + Auth::guard('operator')->logout(); + + $page->call('$refresh')->assertRedirect(route('admin.login')); +}); diff --git a/tests/Feature/Admin/VpnTest.php b/tests/Feature/Admin/VpnTest.php index 7e4bc7c..988ba91 100644 --- a/tests/Feature/Admin/VpnTest.php +++ b/tests/Feature/Admin/VpnTest.php @@ -4,6 +4,7 @@ use App\Livewire\Admin\ConfirmDeleteVpnPeer; use App\Livewire\Admin\Vpn; use App\Livewire\Admin\VpnConfigAccess; use App\Models\Host; +use App\Models\Operator; use App\Models\VpnPeer; use App\Provisioning\Jobs\ApplyVpnPeer; use App\Provisioning\Jobs\SyncVpnPeers; @@ -12,6 +13,8 @@ use App\Services\Wireguard\Keypair; use App\Services\Wireguard\LocalWireguardHub; use App\Services\Wireguard\PeerSnapshot; use App\Services\Wireguard\WireguardHub; +use Illuminate\Support\Facades\Auth; +use Illuminate\Support\Facades\Gate; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Queue; use Livewire\Livewire; @@ -620,6 +623,31 @@ it('locks out password guessing', function () { expect($modal->get('token'))->toBeNull(); }); +it('sends the operator to sign in instead of a 500 when the session has ended', function () { + // reveal() re-checks $this->authorize('downloadConfig', $peer) on every + // call, which already refuses a guest (Gate auto-denies a policy whose + // first parameter is a non-nullable Operator). That is a separate, + // already-correct guard — not the one this test is for. A Gate::before + // with a NULLABLE $user isolates the one line this regression is about: + // Auth::guard('operator')->user() coming back null past that point, + // which used to be dereferenced unguarded ($operator->password). + Gate::before(fn (?Operator $user, string $ability) => $ability === 'downloadConfig' ? true : null); + + vpnHub(); + $support = operator('Support'); + $peer = VpnPeer::factory()->ownedBy($support)->create([ + 'config_secret' => App\Services\Wireguard\ConfigVault::encrypt("[Interface]\nPrivateKey = XYZ\n"), + ]); + + $modal = Livewire::actingAs($support, 'operator')->test(VpnConfigAccess::class, ['uuid' => $peer->uuid]); + + // The modal sat open long enough for the operator's own session to end. + Auth::guard('operator')->logout(); + + $modal->set('password', 'geheim-1234')->call('reveal') + ->assertRedirect(route('admin.login')); +}); + it('destroys the stored config when the access is revoked', function () { vpnHub(); Queue::fake();