From d40113be8e2f6f331629c52d3fc3bfbf3342fa7c Mon Sep 17 00:00:00 2001 From: nexxo Date: Sun, 26 Jul 2026 05:32:53 +0200 Subject: [PATCH] fix(admin): never reset a stranger's VM, never leave the console spinning Two from Codex: - The job accepted any stored instance with a host. A closed one's VMID can have been reused on the same machine, and the reset would then land on another customer's VM. It now requires a live instance, and the action is not offered for anything else. - An unreachable Proxmox or guest agent throws rather than returning an exit code, so nothing was ever written to the handoff and the modal polled forever. The throw is caught, and a failed() handler covers anything that still escapes. Co-Authored-By: Claude Opus 4.8 --- app/Livewire/Admin/Customers.php | 2 +- .../Jobs/IssueInstanceAdminAccess.php | 38 +++++++++++++++---- .../Feature/Admin/InstanceAdminAccessTest.php | 29 ++++++++++++++ 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/app/Livewire/Admin/Customers.php b/app/Livewire/Admin/Customers.php index fff5c14..57abfa1 100644 --- a/app/Livewire/Admin/Customers.php +++ b/app/Livewire/Admin/Customers.php @@ -54,7 +54,7 @@ class Customers extends Component 'mrr' => Number::currency($priceCents / 100, in: 'EUR', locale: $locale), 'instance' => $instance->subdomain ?? '—', // Only an instance that exists can hand out an admin login. - 'instance_uuid' => $instance->uuid ?? null, + 'instance_uuid' => ($instance?->status === 'active') ? $instance->uuid : null, 'closed' => $c->closed_at !== null || $c->status === 'closed', 'suspended' => $c->status === 'suspended', 'status' => match (true) { diff --git a/app/Provisioning/Jobs/IssueInstanceAdminAccess.php b/app/Provisioning/Jobs/IssueInstanceAdminAccess.php index efc0c10..f1edd87 100644 --- a/app/Provisioning/Jobs/IssueInstanceAdminAccess.php +++ b/app/Provisioning/Jobs/IssueInstanceAdminAccess.php @@ -12,6 +12,7 @@ use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; +use Throwable; /** * Hands an operator working administrator credentials for a customer's @@ -41,11 +42,20 @@ class IssueInstanceAdminAccess implements ShouldQueue $this->onQueue('provisioning'); } + /** Anything that escapes handle() still has to reach the waiting console. */ + public function failed(?Throwable $e): void + { + ConfigHandoff::put(json_encode(['error' => 'failed']), $this->handoffToken); + } + public function handle(ProxmoxClient $proxmox): void { $instance = Instance::query()->with('host')->where('uuid', $this->instanceUuid)->first(); - if ($instance === null || $instance->host === null || blank($instance->nc_admin_ref)) { + // Only a live instance: a closed one's VMID can have been reused on the + // same host, and the reset would then land on a stranger's VM. + if ($instance === null || $instance->status !== 'active' + || $instance->host === null || blank($instance->vmid) || blank($instance->nc_admin_ref)) { ConfigHandoff::put(json_encode(['error' => 'unavailable']), $this->handoffToken); return; @@ -54,13 +64,25 @@ class IssueInstanceAdminAccess implements ShouldQueue $username = $instance->nc_admin_ref; $password = Str::password(24, symbols: false); - $result = $proxmox->forHost($instance->host)->guestExec( - $instance->host->node ?? 'pve', - (int) $instance->vmid, - 'cd /opt/nextcloud && OC_PASS='.escapeshellarg($password). - ' docker compose exec -T -e OC_PASS app php occ user:resetpassword --password-from-env '. - escapeshellarg($username), - ); + try { + $result = $proxmox->forHost($instance->host)->guestExec( + $instance->host->node ?? 'pve', + (int) $instance->vmid, + 'cd /opt/nextcloud && OC_PASS='.escapeshellarg($password). + ' docker compose exec -T -e OC_PASS app php occ user:resetpassword --password-from-env '. + escapeshellarg($username), + ); + } catch (Throwable $e) { + // An unreachable Proxmox or guest agent throws rather than + // returning a code. Without this the console would poll forever + // instead of saying what happened. + Log::warning('instance admin access errored', [ + 'instance' => $instance->uuid, 'error' => $e->getMessage(), + ]); + ConfigHandoff::put(json_encode(['error' => 'failed']), $this->handoffToken); + + return; + } if ((int) ($result['exitcode'] ?? 1) !== 0) { Log::warning('instance admin access failed', [ diff --git a/tests/Feature/Admin/InstanceAdminAccessTest.php b/tests/Feature/Admin/InstanceAdminAccessTest.php index eb962c8..62a8f2c 100644 --- a/tests/Feature/Admin/InstanceAdminAccessTest.php +++ b/tests/Feature/Admin/InstanceAdminAccessTest.php @@ -85,3 +85,32 @@ it('reports a silent instance instead of pretending it worked', function () { expect(json_decode(ConfigHandoff::get($token), true))->toBe(['error' => 'failed']); }); + +it('refuses an instance that is no longer live', function () { + $pve = new FakeProxmoxClient; + app()->instance(ProxmoxClient::class, $pve); + $instance = adminAccessInstance(); + $instance->update(['status' => 'terminated']); + + (new IssueInstanceAdminAccess($instance->uuid, 'handoff-dead', 1))->handle($pve); + + // Its VMID may belong to someone else by now. + expect(json_decode(ConfigHandoff::get('handoff-dead'), true))->toBe(['error' => 'unavailable']); +}); + +it('tells the console when Proxmox is unreachable', function () { + $pve = new class extends FakeProxmoxClient + { + public function guestExec(string $node, int $vmid, string $command): array + { + throw new RuntimeException('no route to host'); + } + }; + app()->instance(ProxmoxClient::class, $pve); + $instance = adminAccessInstance(); + + (new IssueInstanceAdminAccess($instance->uuid, 'handoff-throw', 1))->handle($pve); + + // Otherwise the modal polls forever with a spinner. + expect(json_decode(ConfigHandoff::get('handoff-throw'), true))->toBe(['error' => 'failed']); +});