From eefb480a5a8290f8d9a3453e37002a1e354fbe4c Mon Sep 17 00:00:00 2001 From: Steven Van Ingelgem Date: Sat, 15 Aug 2026 08:35:57 +0200 Subject: [PATCH 1/5] fix(upgrade): make p:upgrade survive its own dependency swap The self-upgrade has been documented as inoperable for years. The cause is that the command replaces vendor/ underneath the PHP process running it and then keeps calling into the framework: composer install swaps the autoloader and every class under vendor/, but PHP cannot unload classes already held in memory, so every step after it ran against a mix of old and new definitions. Re-requiring bootstrap/app.php could not fix that, because the container is not what is stale. The upgrade is now split across two processes. The first runs from the installed code and stops once the new code is on disk; the second is a fresh 'artisan p:upgrade --finalize', so it boots the code it is about to migrate. Alongside that: - Every external command's exit status is now checked. All eleven were discarded before, so a 404 on the download unpacked nothing, ran migrations against the old tree, and still reported a successful upgrade. - Maintenance mode is entered before the archive is unpacked rather than after, so new code is never served against the old schema. - --user and --group were only ever tested for null and their values never used, so every run chowned to www-data. They are honoured now, and detection also works when the command runs non-interactively. - The PHP version guard printed an error and then carried on regardless. It is replaced by composer check-platform-reqs against the manifests inside the downloaded archive, so an unsupported PHP version or a missing extension is caught while the Panel is still online and untouched. - The archive is downloaded to a temporary file and can be verified with --checksum, rather than piped straight into tar where a truncated transfer overwrote the installation with nothing to fall back on. - chown -R targets '.' rather than '*', which silently skipped every dotfile, .env included. - Pre-flight checks for the required binaries, a writable tree, free disk space and a reachable database run before anything is touched. - A failure once the Panel is offline leaves it in maintenance mode on purpose and prints how to resume, rather than exposing a half-upgraded tree. --- app/Console/Commands/UpgradeCommand.php | 474 +++++++++++++----- .../Console/Commands/UpgradeCommandTest.php | 90 ++++ 2 files changed, 440 insertions(+), 124 deletions(-) create mode 100644 tests/Unit/Console/Commands/UpgradeCommandTest.php diff --git a/app/Console/Commands/UpgradeCommand.php b/app/Console/Commands/UpgradeCommand.php index 735427690c..530b9757d2 100644 --- a/app/Console/Commands/UpgradeCommand.php +++ b/app/Console/Commands/UpgradeCommand.php @@ -3,185 +3,411 @@ namespace Pterodactyl\Console\Commands; use Illuminate\Console\Command; -use Pterodactyl\Console\Kernel; -use Symfony\Component\Process\Process; -use Symfony\Component\Console\Helper\ProgressBar; +use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Process; +use Symfony\Component\Process\ExecutableFinder; +use Symfony\Component\Process\Process as SymfonyProcess; class UpgradeCommand extends Command { protected const DEFAULT_URL = 'https://github.com/pterodactyl/panel/releases/%s/panel.tar.gz'; + /** + * Rough lower bound on the space needed to unpack an archive over the existing + * tree and reinstall dependencies on top of it. + */ + protected const REQUIRED_DISK_BYTES = 512 * 1024 * 1024; + protected $signature = 'p:upgrade {--user= : The user that PHP runs under. All files will be owned by this user.} {--group= : The group that PHP runs under. All files will be owned by this group.} {--url= : The specific archive to download.} {--release= : A specific Pterodactyl version to download from GitHub. Leave blank to use latest.} - {--skip-download : If set no archive will be downloaded.}'; + {--checksum= : Expected SHA256 hash of the archive. The upgrade is aborted if the download does not match.} + {--skip-download : If set no archive will be downloaded.} + {--finalize : Internal. Runs the second half of the upgrade; the command invokes this on itself.}'; protected $description = 'Downloads a new archive for Pterodactyl from GitHub and then executes the normal upgrade commands.'; /** - * Executes an upgrade command which will run through all of our standard - * commands for Pterodactyl and enable users to basically just download - * the archive and execute this and be done. + * An upgrade is split across two processes on purpose. * - * This places the application in maintenance mode as well while the commands - * are being executed. + * The first half runs from the code that is currently installed and stops the + * moment the new code is on disk. The second half is a brand new PHP process, + * so it boots the code that was just installed. This is not a stylistic choice: + * `composer install` replaces the autoloader and every class under vendor/ while + * the first process is still running, and PHP cannot unload the classes it has + * already loaded. Anything the old process touches after that point is a coin + * flip between the definition it holds in memory and the file now on disk. + */ + public function handle(): int + { + return $this->option('finalize') ? $this->finalize() : $this->stage(); + } + + /** + * Runs from the currently installed code, and does everything up to and + * including putting the new code on disk. * - * @throws \Exception + * The ordering matters. Every check that can fail cheaply happens before the + * Panel is taken offline and before a single file is written, so that an + * upgrade which was never going to work costs the operator a message rather + * than an outage. */ - public function handle() + protected function stage(): int { $skipDownload = $this->option('skip-download'); + if (!$skipDownload) { - $this->output->warning('This command does not verify the integrity of downloaded assets. Please ensure that you trust the download source before continuing. If you do not wish to download an archive, please indicate that using the --skip-download flag, or answering "no" to the question below.'); + $this->output->warning('This command does not verify the authenticity of downloaded assets. Please ensure that you trust the download source before continuing. Pass --checksum= to have the download checked against a hash you obtained separately. If you do not wish to download an archive, please indicate that using the --skip-download flag, or answering "no" to the question below.'); $this->output->comment('Download Source (set with --url=):'); $this->line($this->getUrl()); } - if (version_compare(PHP_VERSION, '8.2.0', '<')) { - $this->error('Cannot execute self-upgrade process. The minimum required PHP version required is 8.2.0, you have [' . PHP_VERSION . '].'); - } + [$user, $group] = $this->resolveOwnership(); - $user = 'www-data'; - $group = 'www-data'; if ($this->input->isInteractive()) { if (!$skipDownload) { $skipDownload = !$this->confirm('Would you like to download and unpack the archive files for the latest version?', true); } - if (is_null($this->option('user'))) { - $userDetails = posix_getpwuid(fileowner('public')); - $user = $userDetails['name'] ?? 'www-data'; - - if (!$this->confirm("Your webserver user has been detected as [{$user}]: is this correct?", true)) { - $user = $this->anticipate( - 'Please enter the name of the user running your webserver process. This varies from system to system, but is generally "www-data", "nginx", or "apache".', - [ - 'www-data', - 'nginx', - 'apache', - ] - ); - } - } - - if (is_null($this->option('group'))) { - $groupDetails = posix_getgrgid(filegroup('public')); - $group = $groupDetails['name'] ?? 'www-data'; - - if (!$this->confirm("Your webserver group has been detected as [{$group}]: is this correct?", true)) { - $group = $this->anticipate( - 'Please enter the name of the group running your webserver process. Normally this is the same as your user.', - [ - 'www-data', - 'nginx', - 'apache', - ] - ); - } - } - if (!$this->confirm('Are you sure you want to run the upgrade process for your Panel?')) { $this->warn('Upgrade process terminated by user.'); - return; + return self::SUCCESS; } } - ini_set('output_buffering', '0'); - $bar = $this->output->createProgressBar($skipDownload ? 9 : 10); - $bar->start(); + $this->step('Running pre-flight checks'); + if ($problem = $this->preflight($skipDownload)) { + $this->error($problem); + $this->warn('Nothing has been changed and your Panel is still online.'); - if (!$skipDownload) { - $this->withProgress($bar, function () { - $this->line("\$upgrader> curl -L \"{$this->getUrl()}\" | tar -xzv"); - $process = Process::fromShellCommandline("curl -L \"{$this->getUrl()}\" | tar -xzv"); - $process->run(function ($type, $buffer) { - $this->{$type === Process::ERR ? 'error' : 'line'}($buffer); - }); - }); + return self::FAILURE; } - $this->withProgress($bar, function () { - $this->line('$upgrader> php artisan down'); + $archive = null; + + // Fetching and vetting the archive is kept in its own attempt because none + // of it touches the installation. Anything that goes wrong here is still a + // clean abort, with the Panel serving traffic exactly as it was. + try { + if (!$skipDownload) { + $archive = tempnam(sys_get_temp_dir(), 'pterodactyl-panel-'); + + // Downloading to a temporary file rather than piping curl straight + // into tar means a truncated or failed transfer cannot leave a + // half-written Panel behind, and gives us something to hash. + $this->step('Downloading the release archive'); + $this->runProcess(['curl', '-L', '--fail', '-o', $archive, $this->getUrl()]); + + $this->step('Verifying the release archive'); + $this->verifyChecksum($archive); + $this->assertPlatformRequirementsMet($archive); + } + } catch (\Exception $exception) { + $this->discard($archive); + $this->newLine(2); + $this->error('The upgrade did not start: ' . $exception->getMessage()); + $this->warn('Nothing has been changed and your Panel is still online.'); + + return self::FAILURE; + } + + try { + // From here on the Panel is offline and files start moving. + $this->step('Putting the Panel into maintenance mode'); $this->call('down'); - }); - - $this->withProgress($bar, function () { - $this->line('$upgrader> chmod -R 755 storage bootstrap/cache'); - $process = new Process(['chmod', '-R', '755', 'storage', 'bootstrap/cache']); - $process->run(function ($type, $buffer) { - $this->{$type === Process::ERR ? 'error' : 'line'}($buffer); - }); - }); - - $this->withProgress($bar, function () { - $command = ['composer', 'install', '--no-ansi']; - if (config('app.env') === 'production' && !config('app.debug')) { - $command[] = '--optimize-autoloader'; - $command[] = '--no-dev'; + + if (!is_null($archive)) { + $this->step('Unpacking the release archive'); + $this->runProcess(['tar', '-xzf', $archive]); } - $this->line('$upgrader> ' . implode(' ', $command)); - $process = new Process($command); - $process->setTimeout(10 * 60); - $process->run(function ($type, $buffer) { - $this->line($buffer); - }); - }); - - /** @var \Illuminate\Foundation\Application $app */ - $app = require __DIR__ . '/../../../bootstrap/app.php'; - /** @var Kernel $kernel */ - $kernel = $app->make(Kernel::class); - $kernel->bootstrap(); - $this->setLaravel($app); - - $this->withProgress($bar, function () { - $this->line('$upgrader> php artisan view:clear'); - $this->call('view:clear'); - }); + $this->step('Fixing storage permissions'); + $this->runProcess(['chmod', '-R', '755', 'storage', 'bootstrap/cache']); + + $this->step('Installing dependencies'); + $this->runProcess($this->composerInstallCommand()); + + // Past this line the classes held in memory no longer match the ones on + // disk, so the rest of the upgrade is handed to a fresh process. + $this->step('Handing over to the newly installed code'); + $handoff = [PHP_BINARY, 'artisan', 'p:upgrade', '--finalize', '--no-interaction', '--user=' . $user, '--group=' . $group]; + $this->runProcess($handoff, 900); + } catch (\Exception $exception) { + return $this->abort($exception); + } finally { + $this->discard($archive); + } + + return self::SUCCESS; + } + + /** + * Runs as a fresh process from the code that was just installed, which is why + * it is safe to boot the framework and call other Artisan commands here. + */ + protected function finalize(): int + { + $user = $this->option('user') ?: 'www-data'; + $group = $this->option('group') ?: 'www-data'; - $this->withProgress($bar, function () { - $this->line('$upgrader> php artisan config:clear'); + try { + $this->step('Clearing cached views and configuration'); + $this->call('view:clear'); $this->call('config:clear'); - }); - $this->withProgress($bar, function () { - $this->line('$upgrader> php artisan migrate --force --seed'); + $this->step('Running database migrations'); $this->call('migrate', ['--force' => true, '--seed' => true]); - }); - - $this->withProgress($bar, function () use ($user, $group) { - $this->line("\$upgrader> chown -R {$user}:{$group} *"); - $process = Process::fromShellCommandline("chown -R {$user}:{$group} *", $this->getLaravel()->basePath()); - $process->setTimeout(10 * 60); - $process->run(function ($type, $buffer) { - $this->{$type === Process::ERR ? 'error' : 'line'}($buffer); - }); - }); - - $this->withProgress($bar, function () { - $this->line('$upgrader> php artisan queue:restart'); + + $this->step("Setting file ownership to {$user}:{$group}"); + try { + // "." rather than "*" so that dotfiles, .env above all, are included. + $this->runProcess(['chown', '-R', "{$user}:{$group}", '.']); + } catch (\Exception $exception) { + // Wrong ownership is worth shouting about but it is recoverable by + // hand, and aborting here would strand a Panel that is otherwise + // fully upgraded in maintenance mode. + $this->warn('Could not set file ownership: ' . $exception->getMessage()); + $this->warn("Run \"chown -R {$user}:{$group} .\" from the Panel directory yourself."); + } + + $this->step('Restarting queue workers'); $this->call('queue:restart'); - }); - $this->withProgress($bar, function () { - $this->line('$upgrader> php artisan up'); + $this->step('Taking the Panel out of maintenance mode'); $this->call('up'); - }); + } catch (\Exception $exception) { + return $this->abort($exception); + } $this->newLine(2); $this->info('Panel has been successfully upgraded. Please ensure you also update any Wings instances: https://pterodactyl.io/wings/1.0/upgrading.html'); + + return self::SUCCESS; + } + + /** + * Everything that can be established while the Panel is still serving traffic. + */ + protected function preflight(bool $skipDownload): ?string + { + $finder = new ExecutableFinder(); + foreach ($skipDownload ? ['composer'] : ['curl', 'tar', 'composer'] as $binary) { + if (is_null($finder->find($binary))) { + return "Required executable [{$binary}] could not be found in your PATH."; + } + } + + $base = $this->getLaravel()->basePath(); + foreach ([$base, $base . '/vendor'] as $path) { + if (file_exists($path) && !is_writable($path)) { + return "The upgrade needs to write to [{$path}], but it is not writable by the current user."; + } + } + + $free = disk_free_space($base); + if ($free !== false && $free < self::REQUIRED_DISK_BYTES) { + return sprintf( + 'Only %dMB of free disk space is available at [%s]; the upgrade needs roughly %dMB.', + $free / 1024 / 1024, + $base, + self::REQUIRED_DISK_BYTES / 1024 / 1024 + ); + } + + try { + DB::connection()->getPdo(); + } catch (\Exception $exception) { + return 'Could not connect to the database, so the migration step would fail: ' . $exception->getMessage(); + } + + return null; + } + + /** + * Compare the downloaded archive against a hash the operator supplied. + * + * A hash fetched over the same channel as the archive would prove nothing about + * its authenticity, so this deliberately only accepts one passed on the command + * line, from a source the operator trusts. + */ + protected function verifyChecksum(string $archive): void + { + $expected = $this->option('checksum'); + if (is_null($expected)) { + $this->warn('No --checksum was given, so the archive could not be verified.'); + + return; + } + + $actual = hash_file('sha256', $archive); + if (!hash_equals(strtolower($expected), $actual)) { + throw new \RuntimeException("Checksum mismatch: expected [{$expected}] but the download hashes to [{$actual}]."); + } + + $this->line('Checksum matches.'); + } + + /** + * Ask Composer whether this machine can actually run the release we are about to + * unpack, using the manifests from inside the archive rather than the ones that + * are already installed. Catching a PHP or extension mismatch here means finding + * out while the Panel is still up and the tree is still untouched, rather than + * halfway through `composer install` with the Panel already offline. + */ + protected function assertPlatformRequirementsMet(string $archive): void + { + $directory = tempnam(sys_get_temp_dir(), 'pterodactyl-reqs-'); + @unlink($directory); + @mkdir($directory); + + try { + foreach (['composer.json', 'composer.lock'] as $file) { + if (!$this->extractFile($archive, $file, $directory . '/' . $file)) { + $this->warn("Could not read {$file} from the archive; skipping the platform requirement check."); + + return; + } + } + + $result = Process::path($directory)->run(['composer', 'check-platform-reqs', '--no-interaction']); + + if ($result->failed()) { + $report = $result->output() . $result->errorOutput(); + + throw new \RuntimeException("This release cannot run on this machine:\n" . $report); + } + + $this->line('Platform requirements satisfied.'); + } finally { + foreach (['composer.json', 'composer.lock'] as $file) { + @unlink($directory . '/' . $file); + } + @rmdir($directory); + } + } + + /** + * Pull a single file out of the archive without unpacking any of the rest of it. + */ + protected function extractFile(string $archive, string $file, string $destination): bool + { + // Archives have been published both with and without a leading "./". + foreach ([$file, './' . $file] as $candidate) { + $result = Process::run(['tar', '-xzOf', $archive, $candidate]); + + if ($result->successful() && $result->output() !== '') { + file_put_contents($destination, $result->output()); + + return true; + } + } + + return false; + } + + /** + * Work out who should own the files once the upgrade is done. + * + * An explicitly passed option always wins. Detection is only a fallback, and the + * guess is only put to the operator when there is somebody there to answer, so + * that the flags behave the same whether or not the command is run by hand. + */ + protected function resolveOwnership(): array + { + $user = $this->option('user'); + if (is_null($user)) { + $user = function_exists('posix_getpwuid') + ? (posix_getpwuid(fileowner('public'))['name'] ?? 'www-data') + : 'www-data'; + + if ($this->input->isInteractive() && !$this->confirm("Your webserver user has been detected as [{$user}]: is this correct?", true)) { + $user = $this->anticipate( + 'Please enter the name of the user running your webserver process. This varies from system to system, but is generally "www-data", "nginx", or "apache".', + ['www-data', 'nginx', 'apache'] + ); + } + } + + $group = $this->option('group'); + if (is_null($group)) { + $group = function_exists('posix_getgrgid') + ? (posix_getgrgid(filegroup('public'))['name'] ?? 'www-data') + : 'www-data'; + + if ($this->input->isInteractive() && !$this->confirm("Your webserver group has been detected as [{$group}]: is this correct?", true)) { + $group = $this->anticipate( + 'Please enter the name of the group running your webserver process. Normally this is the same as your user.', + ['www-data', 'nginx', 'apache'] + ); + } + } + + return [$user, $group]; + } + + protected function composerInstallCommand(): array + { + $command = ['composer', 'install', '--no-ansi', '--no-interaction']; + + if (config('app.env') === 'production' && !config('app.debug')) { + $command[] = '--optimize-autoloader'; + $command[] = '--no-dev'; + } + + return $command; + } + + /** + * Run an external command and abort the upgrade if it does not succeed. + * + * Every step of an upgrade is load-bearing. A step that fails quietly, which is + * what happened while the exit status went unchecked, leaves the tree in a state + * nothing downstream is expecting and still reports success at the end. + */ + protected function runProcess(array $command, int $timeout = 600): void + { + $this->line('$upgrader> ' . implode(' ', $command)); + + $stream = fn ($type, $buffer) => $this->{$type === SymfonyProcess::ERR ? 'error' : 'line'}($buffer); + + $result = Process::path($this->getLaravel()->basePath()) + ->timeout($timeout) + ->run($command, $stream); + + if ($result->failed()) { + throw new \RuntimeException(sprintf('[%s] exited with status %s.', $command[0], $result->exitCode())); + } + } + + /** + * The Panel is deliberately left in maintenance mode. A half upgraded tree will + * serve errors or, worse, write bad data; an honest maintenance page is the + * better of those two outcomes. + */ + protected function abort(\Exception $exception): int + { + $this->newLine(2); + $this->error('The upgrade did not complete: ' . $exception->getMessage()); + $this->warn('Your Panel has been left in maintenance mode on purpose, because the installation may be half upgraded.'); + $this->warn('Once the problem above is resolved, re-run this command with --skip-download to pick up where it left off, or run "php artisan up" to bring the Panel back online as it is.'); + + return self::FAILURE; + } + + protected function discard(?string $archive): void + { + if (!is_null($archive) && file_exists($archive)) { + @unlink($archive); + } } - protected function withProgress(ProgressBar $bar, \Closure $callback) + protected function step(string $message): void { - $bar->clear(); - $callback(); - $bar->advance(); - $bar->display(); + $this->newLine(); + $this->line("==> {$message}"); } protected function getUrl(): string diff --git a/tests/Unit/Console/Commands/UpgradeCommandTest.php b/tests/Unit/Console/Commands/UpgradeCommandTest.php new file mode 100644 index 0000000000..388149adf5 --- /dev/null +++ b/tests/Unit/Console/Commands/UpgradeCommandTest.php @@ -0,0 +1,90 @@ + 'sqlite', + 'database.connections.sqlite' => ['driver' => 'sqlite', 'database' => ':memory:', 'prefix' => ''], + ]); + + $this->bringApplicationUp(); + } + + protected function tearDown(): void + { + $this->bringApplicationUp(); + + parent::tearDown(); + } + + /** + * Any test that reaches the second half of the upgrade really does put the + * application into maintenance mode, and the process that would lift it back + * out is faked away. + */ + private function bringApplicationUp(): void + { + @unlink(storage_path('framework/down')); + @unlink(storage_path('framework/maintenance.php')); + } + + public function testExplicitUserAndGroupAreHandedToTheSecondHalf(): void + { + Process::fake(); + + $this->upgrade(['--user' => 'nginx', '--group' => 'web'])->assertExitCode(Command::SUCCESS); + + Process::assertRan(fn ($process) => $this->isHandoff($process) + && in_array('--user=nginx', $process->command, true) + && in_array('--group=web', $process->command, true)); + } + + public function testUpgradeIsAbandonedWhenAStepExitsNonZero(): void + { + Process::fake(['*' => Process::result(exitCode: 1)]); + + $this->upgrade() + ->expectsOutputToContain('The upgrade did not complete') + ->assertExitCode(Command::FAILURE); + + // The old command discarded every exit status, so a failed step still ran + // the migrations behind it and reported a successful upgrade at the end. + Process::assertDidntRun(fn ($process) => $this->isHandoff($process)); + } + + public function testChecksumMismatchStopsBeforeTheApplicationGoesOffline(): void + { + Process::fake(); + + $this->artisan('p:upgrade', ['--no-interaction' => true, '--checksum' => str_repeat('a', 64)]) + ->expectsOutputToContain('Checksum mismatch') + ->assertExitCode(Command::FAILURE); + + $this->assertFalse($this->app->isDownForMaintenance()); + Process::assertDidntRun(fn ($process) => $this->isHandoff($process)); + } + + private function isHandoff(object $process): bool + { + return is_array($process->command) && in_array('--finalize', $process->command, true); + } + + private function upgrade(array $options = []): PendingCommand + { + return $this->artisan('p:upgrade', array_merge(['--skip-download' => true, '--no-interaction' => true], $options)); + } +} From 6a8373752c1586f8eec9eb224ec3c259f98e6241 Mon Sep 17 00:00:00 2001 From: Steven Van Ingelgem Date: Sun, 16 Aug 2026 23:18:10 +0200 Subject: [PATCH 2/5] style(upgrade): trim comments to the repo's density The docblocks explained the reasoning at review length rather than describing the methods. Cut to a line or two each; the rationale belongs in the pull request, not in every header. --- app/Console/Commands/UpgradeCommand.php | 81 ++++++------------- .../Console/Commands/UpgradeCommandTest.php | 11 +-- 2 files changed, 27 insertions(+), 65 deletions(-) diff --git a/app/Console/Commands/UpgradeCommand.php b/app/Console/Commands/UpgradeCommand.php index 530b9757d2..7790ec8346 100644 --- a/app/Console/Commands/UpgradeCommand.php +++ b/app/Console/Commands/UpgradeCommand.php @@ -13,8 +13,7 @@ class UpgradeCommand extends Command protected const DEFAULT_URL = 'https://github.com/pterodactyl/panel/releases/%s/panel.tar.gz'; /** - * Rough lower bound on the space needed to unpack an archive over the existing - * tree and reinstall dependencies on top of it. + * Rough lower bound for unpacking an archive and reinstalling dependencies. */ protected const REQUIRED_DISK_BYTES = 512 * 1024 * 1024; @@ -30,15 +29,9 @@ class UpgradeCommand extends Command protected $description = 'Downloads a new archive for Pterodactyl from GitHub and then executes the normal upgrade commands.'; /** - * An upgrade is split across two processes on purpose. - * - * The first half runs from the code that is currently installed and stops the - * moment the new code is on disk. The second half is a brand new PHP process, - * so it boots the code that was just installed. This is not a stylistic choice: - * `composer install` replaces the autoloader and every class under vendor/ while - * the first process is still running, and PHP cannot unload the classes it has - * already loaded. Anything the old process touches after that point is a coin - * flip between the definition it holds in memory and the file now on disk. + * Splits the upgrade over two processes: composer install replaces every class + * under vendor/ while this one is running, and PHP cannot unload what it has + * already loaded. */ public function handle(): int { @@ -46,13 +39,8 @@ public function handle(): int } /** - * Runs from the currently installed code, and does everything up to and - * including putting the new code on disk. - * - * The ordering matters. Every check that can fail cheaply happens before the - * Panel is taken offline and before a single file is written, so that an - * upgrade which was never going to work costs the operator a message rather - * than an outage. + * Runs from the installed code and stops once the new code is on disk. Checks + * that can fail cheaply run before the Panel goes offline. */ protected function stage(): int { @@ -88,16 +76,12 @@ protected function stage(): int $archive = null; - // Fetching and vetting the archive is kept in its own attempt because none - // of it touches the installation. Anything that goes wrong here is still a - // clean abort, with the Panel serving traffic exactly as it was. try { if (!$skipDownload) { $archive = tempnam(sys_get_temp_dir(), 'pterodactyl-panel-'); - // Downloading to a temporary file rather than piping curl straight - // into tar means a truncated or failed transfer cannot leave a - // half-written Panel behind, and gives us something to hash. + // A temporary file rather than curl piped into tar, so a truncated + // transfer cannot leave a half-written Panel behind. $this->step('Downloading the release archive'); $this->runProcess(['curl', '-L', '--fail', '-o', $archive, $this->getUrl()]); @@ -115,7 +99,6 @@ protected function stage(): int } try { - // From here on the Panel is offline and files start moving. $this->step('Putting the Panel into maintenance mode'); $this->call('down'); @@ -130,8 +113,7 @@ protected function stage(): int $this->step('Installing dependencies'); $this->runProcess($this->composerInstallCommand()); - // Past this line the classes held in memory no longer match the ones on - // disk, so the rest of the upgrade is handed to a fresh process. + // Memory and disk stop agreeing here, so the rest runs elsewhere. $this->step('Handing over to the newly installed code'); $handoff = [PHP_BINARY, 'artisan', 'p:upgrade', '--finalize', '--no-interaction', '--user=' . $user, '--group=' . $group]; $this->runProcess($handoff, 900); @@ -145,8 +127,8 @@ protected function stage(): int } /** - * Runs as a fresh process from the code that was just installed, which is why - * it is safe to boot the framework and call other Artisan commands here. + * Runs as a fresh process from the newly installed code, so booting the + * framework and calling other Artisan commands is safe here. */ protected function finalize(): int { @@ -163,12 +145,10 @@ protected function finalize(): int $this->step("Setting file ownership to {$user}:{$group}"); try { - // "." rather than "*" so that dotfiles, .env above all, are included. + // "." rather than "*", which skips dotfiles such as .env. $this->runProcess(['chown', '-R', "{$user}:{$group}", '.']); } catch (\Exception $exception) { - // Wrong ownership is worth shouting about but it is recoverable by - // hand, and aborting here would strand a Panel that is otherwise - // fully upgraded in maintenance mode. + // Recoverable by hand, and aborting would strand an upgraded Panel offline. $this->warn('Could not set file ownership: ' . $exception->getMessage()); $this->warn("Run \"chown -R {$user}:{$group} .\" from the Panel directory yourself."); } @@ -227,11 +207,9 @@ protected function preflight(bool $skipDownload): ?string } /** - * Compare the downloaded archive against a hash the operator supplied. - * - * A hash fetched over the same channel as the archive would prove nothing about - * its authenticity, so this deliberately only accepts one passed on the command - * line, from a source the operator trusts. + * Verifies the archive against a hash the operator supplied. Only one passed on + * the command line is accepted, since a hash fetched over the same channel as + * the archive would prove nothing about it. */ protected function verifyChecksum(string $archive): void { @@ -251,11 +229,8 @@ protected function verifyChecksum(string $archive): void } /** - * Ask Composer whether this machine can actually run the release we are about to - * unpack, using the manifests from inside the archive rather than the ones that - * are already installed. Catching a PHP or extension mismatch here means finding - * out while the Panel is still up and the tree is still untouched, rather than - * halfway through `composer install` with the Panel already offline. + * Checks this machine against the manifests inside the archive, so an unsupported + * PHP version or a missing extension surfaces while the Panel is still up. */ protected function assertPlatformRequirementsMet(string $archive): void { @@ -290,7 +265,7 @@ protected function assertPlatformRequirementsMet(string $archive): void } /** - * Pull a single file out of the archive without unpacking any of the rest of it. + * Pulls a single file out of the archive without unpacking the rest of it. */ protected function extractFile(string $archive, string $file, string $destination): bool { @@ -309,11 +284,8 @@ protected function extractFile(string $archive, string $file, string $destinatio } /** - * Work out who should own the files once the upgrade is done. - * - * An explicitly passed option always wins. Detection is only a fallback, and the - * guess is only put to the operator when there is somebody there to answer, so - * that the flags behave the same whether or not the command is run by hand. + * Resolves the eventual file owner. An explicit option wins; detection is the + * fallback, and the guess is only confirmed when somebody is there to answer. */ protected function resolveOwnership(): array { @@ -361,11 +333,7 @@ protected function composerInstallCommand(): array } /** - * Run an external command and abort the upgrade if it does not succeed. - * - * Every step of an upgrade is load-bearing. A step that fails quietly, which is - * what happened while the exit status went unchecked, leaves the tree in a state - * nothing downstream is expecting and still reports success at the end. + * Runs an external command and aborts the upgrade if it does not succeed. */ protected function runProcess(array $command, int $timeout = 600): void { @@ -383,9 +351,8 @@ protected function runProcess(array $command, int $timeout = 600): void } /** - * The Panel is deliberately left in maintenance mode. A half upgraded tree will - * serve errors or, worse, write bad data; an honest maintenance page is the - * better of those two outcomes. + * Leaves the Panel in maintenance mode on purpose: a half upgraded tree serving + * traffic is worse than a maintenance page. */ protected function abort(\Exception $exception): int { diff --git a/tests/Unit/Console/Commands/UpgradeCommandTest.php b/tests/Unit/Console/Commands/UpgradeCommandTest.php index 388149adf5..8a5fc2140f 100644 --- a/tests/Unit/Console/Commands/UpgradeCommandTest.php +++ b/tests/Unit/Console/Commands/UpgradeCommandTest.php @@ -13,9 +13,7 @@ public function setUp(): void { parent::setUp(); - // The unit suite runs without a database, but the pre-flight check wants a - // working connection: a migration that cannot run is the whole reason the - // command used to strand Panels in maintenance mode. + // The unit suite runs without a database; the pre-flight check needs one. config([ 'database.default' => 'sqlite', 'database.connections.sqlite' => ['driver' => 'sqlite', 'database' => ':memory:', 'prefix' => ''], @@ -32,9 +30,8 @@ protected function tearDown(): void } /** - * Any test that reaches the second half of the upgrade really does put the - * application into maintenance mode, and the process that would lift it back - * out is faked away. + * Tests that reach the second half really do go into maintenance mode, and the + * process that would lift it back out is faked away. */ private function bringApplicationUp(): void { @@ -61,8 +58,6 @@ public function testUpgradeIsAbandonedWhenAStepExitsNonZero(): void ->expectsOutputToContain('The upgrade did not complete') ->assertExitCode(Command::FAILURE); - // The old command discarded every exit status, so a failed step still ran - // the migrations behind it and reported a successful upgrade at the end. Process::assertDidntRun(fn ($process) => $this->isHandoff($process)); } From 226eaf610bc183dff73e6485d1ebe774a1f512f0 Mon Sep 17 00:00:00 2001 From: Steven Van Ingelgem Date: Mon, 17 Aug 2026 06:36:24 +0200 Subject: [PATCH 3/5] refactor(upgrade): flatten the flow and restore the progress bar upgrade() read as one long body with the sequence buried in it. The phases are now named calls: preflight, downloadArchive, replaceInstallation. preflight throws like everything else instead of returning an error string. The progress bar came back with it. It cannot span the handover, so each process draws its own: seven steps in the first half, four when the download is skipped, six in --finalize. Both abort paths clear it before printing. --- app/Console/Commands/UpgradeCommand.php | 243 +++++++++++++----------- 1 file changed, 137 insertions(+), 106 deletions(-) diff --git a/app/Console/Commands/UpgradeCommand.php b/app/Console/Commands/UpgradeCommand.php index 7790ec8346..ab9154e57c 100644 --- a/app/Console/Commands/UpgradeCommand.php +++ b/app/Console/Commands/UpgradeCommand.php @@ -6,6 +6,7 @@ use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Process; use Symfony\Component\Process\ExecutableFinder; +use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Process\Process as SymfonyProcess; class UpgradeCommand extends Command @@ -28,6 +29,8 @@ class UpgradeCommand extends Command protected $description = 'Downloads a new archive for Pterodactyl from GitHub and then executes the normal upgrade commands.'; + private ?ProgressBar $bar = null; + /** * Splits the upgrade over two processes: composer install replaces every class * under vendor/ while this one is running, and PHP cannot unload what it has @@ -35,90 +38,46 @@ class UpgradeCommand extends Command */ public function handle(): int { - return $this->option('finalize') ? $this->finalize() : $this->stage(); + return $this->option('finalize') ? $this->finalize() : $this->upgrade(); } /** - * Runs from the installed code and stops once the new code is on disk. Checks - * that can fail cheaply run before the Panel goes offline. + * Runs from the installed code and stops once the new code is on disk. */ - protected function stage(): int + protected function upgrade(): int { - $skipDownload = $this->option('skip-download'); - - if (!$skipDownload) { - $this->output->warning('This command does not verify the authenticity of downloaded assets. Please ensure that you trust the download source before continuing. Pass --checksum= to have the download checked against a hash you obtained separately. If you do not wish to download an archive, please indicate that using the --skip-download flag, or answering "no" to the question below.'); - $this->output->comment('Download Source (set with --url=):'); - $this->line($this->getUrl()); - } - + $skipDownload = !$this->wantsDownload(); [$user, $group] = $this->resolveOwnership(); - if ($this->input->isInteractive()) { - if (!$skipDownload) { - $skipDownload = !$this->confirm('Would you like to download and unpack the archive files for the latest version?', true); - } - - if (!$this->confirm('Are you sure you want to run the upgrade process for your Panel?')) { - $this->warn('Upgrade process terminated by user.'); + if (!$this->confirmUpgrade()) { + $this->warn('Upgrade process terminated by user.'); - return self::SUCCESS; - } + return self::SUCCESS; } - $this->step('Running pre-flight checks'); - if ($problem = $this->preflight($skipDownload)) { - $this->error($problem); - $this->warn('Nothing has been changed and your Panel is still online.'); - - return self::FAILURE; + try { + $this->preflight($skipDownload); + } catch (\Exception $exception) { + return $this->abortWhileOnline($exception); } - $archive = null; + $archive = $skipDownload ? null : tempnam(sys_get_temp_dir(), 'pterodactyl-panel-'); + $this->startProgress($skipDownload ? 4 : 7); try { - if (!$skipDownload) { - $archive = tempnam(sys_get_temp_dir(), 'pterodactyl-panel-'); - - // A temporary file rather than curl piped into tar, so a truncated - // transfer cannot leave a half-written Panel behind. - $this->step('Downloading the release archive'); - $this->runProcess(['curl', '-L', '--fail', '-o', $archive, $this->getUrl()]); - - $this->step('Verifying the release archive'); - $this->verifyChecksum($archive); - $this->assertPlatformRequirementsMet($archive); + if (!is_null($archive)) { + $this->downloadArchive($archive); } } catch (\Exception $exception) { $this->discard($archive); - $this->newLine(2); - $this->error('The upgrade did not start: ' . $exception->getMessage()); - $this->warn('Nothing has been changed and your Panel is still online.'); - return self::FAILURE; + return $this->abortWhileOnline($exception); } try { - $this->step('Putting the Panel into maintenance mode'); - $this->call('down'); - - if (!is_null($archive)) { - $this->step('Unpacking the release archive'); - $this->runProcess(['tar', '-xzf', $archive]); - } - - $this->step('Fixing storage permissions'); - $this->runProcess(['chmod', '-R', '755', 'storage', 'bootstrap/cache']); - - $this->step('Installing dependencies'); - $this->runProcess($this->composerInstallCommand()); - - // Memory and disk stop agreeing here, so the rest runs elsewhere. - $this->step('Handing over to the newly installed code'); - $handoff = [PHP_BINARY, 'artisan', 'p:upgrade', '--finalize', '--no-interaction', '--user=' . $user, '--group=' . $group]; - $this->runProcess($handoff, 900); + $this->replaceInstallation($archive, $user, $group); } catch (\Exception $exception) { - return $this->abort($exception); + return $this->abortWhileOffline($exception); } finally { $this->discard($archive); } @@ -135,31 +94,17 @@ protected function finalize(): int $user = $this->option('user') ?: 'www-data'; $group = $this->option('group') ?: 'www-data'; - try { - $this->step('Clearing cached views and configuration'); - $this->call('view:clear'); - $this->call('config:clear'); - - $this->step('Running database migrations'); - $this->call('migrate', ['--force' => true, '--seed' => true]); - - $this->step("Setting file ownership to {$user}:{$group}"); - try { - // "." rather than "*", which skips dotfiles such as .env. - $this->runProcess(['chown', '-R', "{$user}:{$group}", '.']); - } catch (\Exception $exception) { - // Recoverable by hand, and aborting would strand an upgraded Panel offline. - $this->warn('Could not set file ownership: ' . $exception->getMessage()); - $this->warn("Run \"chown -R {$user}:{$group} .\" from the Panel directory yourself."); - } - - $this->step('Restarting queue workers'); - $this->call('queue:restart'); + $this->startProgress(6); - $this->step('Taking the Panel out of maintenance mode'); - $this->call('up'); + try { + $this->withProgress(fn () => $this->call('view:clear')); + $this->withProgress(fn () => $this->call('config:clear')); + $this->withProgress(fn () => $this->call('migrate', ['--force' => true, '--seed' => true])); + $this->withProgress(fn () => $this->setOwnership($user, $group)); + $this->withProgress(fn () => $this->call('queue:restart')); + $this->withProgress(fn () => $this->call('up')); } catch (\Exception $exception) { - return $this->abort($exception); + return $this->abortWhileOffline($exception); } $this->newLine(2); @@ -168,42 +113,71 @@ protected function finalize(): int return self::SUCCESS; } + /** + * Fetches and vets the archive while the Panel is still serving traffic. + */ + protected function downloadArchive(string $archive): void + { + // A temporary file rather than curl piped into tar, so a truncated + // transfer cannot leave a half-written Panel behind. + $this->withProgress(fn () => $this->runProcess(['curl', '-L', '--fail', '-o', $archive, $this->getUrl()])); + + $this->withProgress(function () use ($archive) { + $this->verifyChecksum($archive); + $this->assertPlatformRequirementsMet($archive); + }); + } + + /** + * The offline half, where the installation is actually overwritten. + */ + protected function replaceInstallation(?string $archive, string $user, string $group): void + { + $this->withProgress(fn () => $this->call('down')); + + if (!is_null($archive)) { + $this->withProgress(fn () => $this->runProcess(['tar', '-xzf', $archive])); + } + + $this->withProgress(fn () => $this->runProcess(['chmod', '-R', '755', 'storage', 'bootstrap/cache'])); + $this->withProgress(fn () => $this->runProcess($this->composerInstallCommand())); + + // Memory and disk stop agreeing here, so the rest runs elsewhere. + $handoff = [PHP_BINARY, 'artisan', 'p:upgrade', '--finalize', '--no-interaction', '--user=' . $user, '--group=' . $group]; + $this->withProgress(fn () => $this->runProcess($handoff, 900)); + } + /** * Everything that can be established while the Panel is still serving traffic. */ - protected function preflight(bool $skipDownload): ?string + protected function preflight(bool $skipDownload): void { $finder = new ExecutableFinder(); foreach ($skipDownload ? ['composer'] : ['curl', 'tar', 'composer'] as $binary) { if (is_null($finder->find($binary))) { - return "Required executable [{$binary}] could not be found in your PATH."; + throw new \RuntimeException("Required executable [{$binary}] could not be found in your PATH."); } } $base = $this->getLaravel()->basePath(); foreach ([$base, $base . '/vendor'] as $path) { if (file_exists($path) && !is_writable($path)) { - return "The upgrade needs to write to [{$path}], but it is not writable by the current user."; + throw new \RuntimeException("The upgrade needs to write to [{$path}], but it is not writable by the current user."); } } $free = disk_free_space($base); if ($free !== false && $free < self::REQUIRED_DISK_BYTES) { - return sprintf( - 'Only %dMB of free disk space is available at [%s]; the upgrade needs roughly %dMB.', - $free / 1024 / 1024, - $base, - self::REQUIRED_DISK_BYTES / 1024 / 1024 - ); + $available = sprintf('Only %dMB of free disk space is available at [%s]; the upgrade needs roughly %dMB.', $free / 1024 / 1024, $base, self::REQUIRED_DISK_BYTES / 1024 / 1024); + + throw new \RuntimeException($available); } try { DB::connection()->getPdo(); } catch (\Exception $exception) { - return 'Could not connect to the database, so the migration step would fail: ' . $exception->getMessage(); + throw new \RuntimeException('Could not connect to the database, so the migration step would fail: ' . $exception->getMessage()); } - - return null; } /** @@ -320,6 +294,39 @@ protected function resolveOwnership(): array return [$user, $group]; } + /** + * Ownership failures are recoverable by hand, so they do not strand a Panel + * that is otherwise fully upgraded. + */ + protected function setOwnership(string $user, string $group): void + { + try { + // "." rather than "*", which skips dotfiles such as .env. + $this->runProcess(['chown', '-R', "{$user}:{$group}", '.']); + } catch (\Exception $exception) { + $this->warn('Could not set file ownership: ' . $exception->getMessage()); + $this->warn("Run \"chown -R {$user}:{$group} .\" from the Panel directory yourself."); + } + } + + protected function wantsDownload(): bool + { + if ($this->option('skip-download')) { + return false; + } + + $this->output->warning('This command does not verify the authenticity of downloaded assets. Please ensure that you trust the download source before continuing. Pass --checksum= to have the download checked against a hash you obtained separately. If you do not wish to download an archive, please indicate that using the --skip-download flag, or answering "no" to the question below.'); + $this->output->comment('Download Source (set with --url=):'); + $this->line($this->getUrl()); + + return !$this->input->isInteractive() || $this->confirm('Would you like to download and unpack the archive files for the latest version?', true); + } + + protected function confirmUpgrade(): bool + { + return !$this->input->isInteractive() || $this->confirm('Are you sure you want to run the upgrade process for your Panel?'); + } + protected function composerInstallCommand(): array { $command = ['composer', 'install', '--no-ansi', '--no-interaction']; @@ -350,12 +357,42 @@ protected function runProcess(array $command, int $timeout = 600): void } } + protected function startProgress(int $steps): void + { + ini_set('output_buffering', '0'); + + $this->bar = $this->output->createProgressBar($steps); + $this->bar->start(); + } + + protected function withProgress(\Closure $callback): void + { + $this->bar?->clear(); + $callback(); + $this->bar?->advance(); + $this->bar?->display(); + } + + /** + * Nothing has been written yet, so the Panel keeps serving traffic. + */ + protected function abortWhileOnline(\Exception $exception): int + { + $this->bar?->clear(); + $this->newLine(2); + $this->error('The upgrade did not start: ' . $exception->getMessage()); + $this->warn('Nothing has been changed and your Panel is still online.'); + + return self::FAILURE; + } + /** - * Leaves the Panel in maintenance mode on purpose: a half upgraded tree serving - * traffic is worse than a maintenance page. + * Maintenance mode is kept on purpose: a half upgraded tree serving traffic is + * worse than a maintenance page. */ - protected function abort(\Exception $exception): int + protected function abortWhileOffline(\Exception $exception): int { + $this->bar?->clear(); $this->newLine(2); $this->error('The upgrade did not complete: ' . $exception->getMessage()); $this->warn('Your Panel has been left in maintenance mode on purpose, because the installation may be half upgraded.'); @@ -371,12 +408,6 @@ protected function discard(?string $archive): void } } - protected function step(string $message): void - { - $this->newLine(); - $this->line("==> {$message}"); - } - protected function getUrl(): string { if ($this->option('url')) { From 004f935e539101742cdd2b62be513a30e678f5d7 Mon Sep 17 00:00:00 2001 From: Steven Van Ingelgem Date: Mon, 17 Aug 2026 06:48:49 +0200 Subject: [PATCH 4/5] test(upgrade): raise line and branch coverage to ~97% Adds seventeen cases: the --finalize half (with the Artisan commands it calls stubbed out, so no migrated database is needed), ownership defaults and a failing chown, checksum match and absence, the platform requirement check passing, failing and being skipped when the archive has no manifests, an unreachable database, a missing binary, a read-only tree, both interactive confirmations, the production composer flags, and --url and --release. UpgradeCommand goes from 57.59% to 97.47% of lines and 60.43% to 97.84% of branches. What is left is the disk space guard, which cannot be provoked without a seam in the command, and one side of each posix_* function_exists ternary, which is dead on whichever platform the suite happens to run on. --- .../Console/Commands/UpgradeCommandTest.php | 309 +++++++++++++++++- 1 file changed, 298 insertions(+), 11 deletions(-) diff --git a/tests/Unit/Console/Commands/UpgradeCommandTest.php b/tests/Unit/Console/Commands/UpgradeCommandTest.php index 8a5fc2140f..8ab50a32e4 100644 --- a/tests/Unit/Console/Commands/UpgradeCommandTest.php +++ b/tests/Unit/Console/Commands/UpgradeCommandTest.php @@ -6,6 +6,8 @@ use Pterodactyl\Tests\TestCase; use Illuminate\Testing\PendingCommand; use Illuminate\Support\Facades\Process; +use Illuminate\Contracts\Console\Kernel; +use Illuminate\Foundation\Console\ClosureCommand; class UpgradeCommandTest extends TestCase { @@ -29,16 +31,6 @@ protected function tearDown(): void parent::tearDown(); } - /** - * Tests that reach the second half really do go into maintenance mode, and the - * process that would lift it back out is faked away. - */ - private function bringApplicationUp(): void - { - @unlink(storage_path('framework/down')); - @unlink(storage_path('framework/maintenance.php')); - } - public function testExplicitUserAndGroupAreHandedToTheSecondHalf(): void { Process::fake(); @@ -65,7 +57,7 @@ public function testChecksumMismatchStopsBeforeTheApplicationGoesOffline(): void { Process::fake(); - $this->artisan('p:upgrade', ['--no-interaction' => true, '--checksum' => str_repeat('a', 64)]) + $this->download(['--checksum' => str_repeat('a', 64)]) ->expectsOutputToContain('Checksum mismatch') ->assertExitCode(Command::FAILURE); @@ -73,13 +65,308 @@ public function testChecksumMismatchStopsBeforeTheApplicationGoesOffline(): void Process::assertDidntRun(fn ($process) => $this->isHandoff($process)); } + public function testMatchingChecksumAllowsTheUpgradeToProceed(): void + { + Process::fake($this->platformRequirementsMet()); + + // curl is faked, so the archive stays the empty file tempnam() created. + $this->download(['--checksum' => hash('sha256', '')]) + ->expectsOutputToContain('Checksum matches.') + ->assertExitCode(Command::SUCCESS); + + Process::assertRan(fn ($process) => is_array($process->command) && $process->command[0] === 'tar' + && in_array('-xzf', $process->command, true)); + } + + public function testMissingChecksumIsReportedButNotFatal(): void + { + Process::fake($this->platformRequirementsMet()); + + $this->download() + ->expectsOutputToContain('No --checksum was given') + ->assertExitCode(Command::SUCCESS); + } + + public function testReleaseIsRefusedWhenThisMachineCannotRunIt(): void + { + Process::fake([ + '*check-platform-reqs*' => Process::result(exitCode: 2, output: 'ext-example missing'), + '*-xzOf*' => Process::result(output: '{"require":{"php":"^8.2"}}'), + '*' => Process::result(), + ]); + + $this->download() + ->expectsOutputToContain('This release cannot run on this machine') + ->assertExitCode(Command::FAILURE); + + $this->assertFalse($this->app->isDownForMaintenance()); + } + + public function testPlatformCheckIsSkippedWhenTheArchiveHasNoManifests(): void + { + Process::fake(['*-xzOf*' => Process::result(output: ''), '*' => Process::result()]); + + $this->download() + ->expectsOutputToContain('skipping the platform requirement check') + ->assertExitCode(Command::SUCCESS); + + // Both the plain and the "./" prefixed candidate are attempted. + Process::assertRan(fn ($process) => is_array($process->command) + && in_array('./composer.json', $process->command, true)); + } + + public function testFinalizeRunsTheRemainingStepsAndSetsOwnership(): void + { + Process::fake(); + $this->stubRemainingSteps(); + + $this->finalize(['--user' => 'nginx', '--group' => 'web']) + ->expectsOutputToContain('Panel has been successfully upgraded') + ->assertExitCode(Command::SUCCESS); + + Process::assertRan(fn ($process) => $process->command === ['chown', '-R', 'nginx:web', '.']); + } + + public function testFinalizeDefaultsOwnershipToTheWebserverUser(): void + { + Process::fake(); + $this->stubRemainingSteps(); + + $this->finalize()->assertExitCode(Command::SUCCESS); + + Process::assertRan(fn ($process) => $process->command === ['chown', '-R', 'www-data:www-data', '.']); + } + + public function testFinalizeSurvivesAFailingChown(): void + { + Process::fake(['*chown*' => Process::result(exitCode: 1), '*' => Process::result()]); + $this->stubRemainingSteps(); + + $this->finalize() + ->expectsOutputToContain('Could not set file ownership') + ->assertExitCode(Command::SUCCESS); + } + + public function testFinalizeLeavesMaintenanceModeOnWhenMigrationsFail(): void + { + Process::fake(); + $this->stubRemainingSteps(); + $this->stubArtisan('migrate {--force} {--seed}', fn () => throw new \RuntimeException('migration blew up')); + + $this->finalize() + ->expectsOutputToContain('migration blew up') + ->expectsOutputToContain('left in maintenance mode') + ->assertExitCode(Command::FAILURE); + } + + public function testUpgradeStopsWhenTheDatabaseIsUnreachable(): void + { + config([ + 'database.default' => 'unreachable', + 'database.connections.unreachable' => [ + 'driver' => 'mysql', 'host' => '127.0.0.1', 'port' => 1, + 'database' => 'testing', 'username' => 'testing', 'password' => 'testing', + ], + ]); + Process::fake(); + + $this->upgrade() + ->expectsOutputToContain('Could not connect to the database') + ->expectsOutputToContain('still online') + ->assertExitCode(Command::FAILURE); + + Process::assertNothingRan(); + } + + public function testUpgradeStopsWhenARequiredBinaryIsMissing(): void + { + Process::fake(); + $path = getenv('PATH'); + putenv('PATH='); + $_SERVER['PATH'] = ''; + + try { + $this->upgrade() + ->expectsOutputToContain('could not be found in your PATH') + ->assertExitCode(Command::FAILURE); + } finally { + putenv('PATH=' . $path); + $_SERVER['PATH'] = $path; + } + + Process::assertNothingRan(); + } + + public function testUpgradeStopsWhenTheInstallationIsNotWritable(): void + { + $directory = sys_get_temp_dir() . '/pterodactyl-readonly-' . getmypid(); + @mkdir($directory); + @chmod($directory, 0o500); + + if (is_writable($directory)) { + @rmdir($directory); + $this->markTestSkipped('This filesystem does not honour a read-only directory.'); + } + + Process::fake(); + $base = $this->app->basePath(); + $this->app->setBasePath($directory); + + try { + $this->upgrade() + ->expectsOutputToContain('is not writable by the current user') + ->assertExitCode(Command::FAILURE); + } finally { + $this->app->setBasePath($base); + @chmod($directory, 0o700); + @rmdir($directory); + } + } + + public function testDetectedOwnershipCanBeCorrectedInteractively(): void + { + Process::fake(); + + $this->artisan('p:upgrade', ['--skip-download' => true]) + ->expectsConfirmation("Your webserver user has been detected as [{$this->detected('posix_getpwuid')}]: is this correct?", 'no') + ->expectsQuestion('Please enter the name of the user running your webserver process. This varies from system to system, but is generally "www-data", "nginx", or "apache".', 'nginx') + ->expectsConfirmation("Your webserver group has been detected as [{$this->detected('posix_getgrgid')}]: is this correct?", 'no') + ->expectsQuestion('Please enter the name of the group running your webserver process. Normally this is the same as your user.', 'web') + ->expectsConfirmation('Are you sure you want to run the upgrade process for your Panel?', 'yes') + ->assertExitCode(Command::SUCCESS); + + Process::assertRan(fn ($process) => $this->isHandoff($process) + && in_array('--user=nginx', $process->command, true) + && in_array('--group=web', $process->command, true)); + } + + public function testDeclinedFinalConfirmationChangesNothing(): void + { + Process::fake(); + + $this->artisan('p:upgrade', ['--skip-download' => true, '--user' => 'nginx', '--group' => 'web']) + ->expectsConfirmation('Are you sure you want to run the upgrade process for your Panel?', 'no') + ->expectsOutputToContain('terminated by user') + ->assertExitCode(Command::SUCCESS); + + Process::assertNothingRan(); + } + + public function testDecliningTheDownloadStillUpgradesFromWhatIsOnDisk(): void + { + Process::fake(); + + $this->artisan('p:upgrade', ['--user' => 'nginx', '--group' => 'web']) + ->expectsConfirmation('Would you like to download and unpack the archive files for the latest version?', 'no') + ->expectsConfirmation('Are you sure you want to run the upgrade process for your Panel?', 'yes') + ->assertExitCode(Command::SUCCESS); + + Process::assertDidntRun(fn ($process) => is_array($process->command) && $process->command[0] === 'curl'); + Process::assertRan(fn ($process) => $this->isHandoff($process)); + } + + public function testProductionInstallsWithoutDevelopmentDependencies(): void + { + config(['app.env' => 'production', 'app.debug' => false]); + Process::fake(); + + $this->upgrade()->assertExitCode(Command::SUCCESS); + + Process::assertRan(fn ($process) => is_array($process->command) && $process->command[0] === 'composer' + && in_array('--no-dev', $process->command, true) + && in_array('--optimize-autoloader', $process->command, true)); + } + + public function testAnExplicitUrlIsUsedVerbatim(): void + { + Process::fake($this->platformRequirementsMet()); + + $this->download(['--url' => 'https://example.com/panel.tar.gz'])->assertExitCode(Command::SUCCESS); + + Process::assertRan(fn ($process) => is_array($process->command) + && in_array('https://example.com/panel.tar.gz', $process->command, true)); + } + + public function testAnExplicitReleaseIsResolvedToATaggedArchive(): void + { + Process::fake($this->platformRequirementsMet()); + + $this->download(['--release' => '1.11.3'])->assertExitCode(Command::SUCCESS); + + Process::assertRan(fn ($process) => is_array($process->command) && in_array( + 'https://github.com/pterodactyl/panel/releases/download/v1.11.3/panel.tar.gz', + $process->command, + true + )); + } + + /** + * Mirrors the owner the command detects, which differs between platforms. + */ + private function detected(string $lookup): string + { + if (!function_exists($lookup)) { + return 'www-data'; + } + + $id = $lookup === 'posix_getpwuid' ? fileowner('public') : filegroup('public'); + + return $lookup($id)['name'] ?? 'www-data'; + } + private function isHandoff(object $process): bool { return is_array($process->command) && in_array('--finalize', $process->command, true); } + /** + * Fakes an archive whose manifests read cleanly and satisfy Composer. + */ + private function platformRequirementsMet(): array + { + return ['*-xzOf*' => Process::result(output: '{"require":{"php":"^8.2"}}'), '*' => Process::result()]; + } + + /** + * Replaces the Artisan commands the second half calls, so the tests do not + * need a migrated database. + */ + private function stubRemainingSteps(): void + { + $this->stubArtisan('view:clear'); + $this->stubArtisan('config:clear'); + $this->stubArtisan('migrate {--force} {--seed}'); + $this->stubArtisan('queue:restart'); + $this->stubArtisan('up'); + } + + private function stubArtisan(string $signature, ?\Closure $callback = null): void + { + $this->app[Kernel::class]->registerCommand(new ClosureCommand($signature, $callback ?? fn () => 0)); + } + + /** + * Tests that reach the second half really do go into maintenance mode, and the + * process that would lift it back out is faked away. + */ + private function bringApplicationUp(): void + { + @unlink(storage_path('framework/down')); + @unlink(storage_path('framework/maintenance.php')); + } + private function upgrade(array $options = []): PendingCommand { return $this->artisan('p:upgrade', array_merge(['--skip-download' => true, '--no-interaction' => true], $options)); } + + private function download(array $options = []): PendingCommand + { + return $this->artisan('p:upgrade', array_merge(['--no-interaction' => true], $options)); + } + + private function finalize(array $options = []): PendingCommand + { + return $this->artisan('p:upgrade', array_merge(['--finalize' => true, '--no-interaction' => true], $options)); + } } From 0848258ec55af6380a87d4f7670f371d04a0b521 Mon Sep 17 00:00:00 2001 From: Steven Van Ingelgem Date: Mon, 17 Aug 2026 06:55:53 +0200 Subject: [PATCH 5/5] refactor(upgrade): call posix_* directly, as the rest of the code does ext-posix is a hard requirement in composer.json and only exists on Unix, so function_exists() around posix_getpwuid and posix_getgrgid can never be false anywhere the Panel runs. The guard was dead code hiding a fallback that would never fire, and 1.0-develop already calls posix_getpwuid unguarded. Coverage of the command reaches 98.70% of lines and 99.25% of branches; only the free disk space guard is left, which cannot be provoked without adding a seam to the command. --- app/Console/Commands/UpgradeCommand.php | 8 ++------ tests/Unit/Console/Commands/UpgradeCommandTest.php | 6 +----- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/app/Console/Commands/UpgradeCommand.php b/app/Console/Commands/UpgradeCommand.php index ab9154e57c..67e06d013a 100644 --- a/app/Console/Commands/UpgradeCommand.php +++ b/app/Console/Commands/UpgradeCommand.php @@ -265,9 +265,7 @@ protected function resolveOwnership(): array { $user = $this->option('user'); if (is_null($user)) { - $user = function_exists('posix_getpwuid') - ? (posix_getpwuid(fileowner('public'))['name'] ?? 'www-data') - : 'www-data'; + $user = posix_getpwuid(fileowner('public'))['name'] ?? 'www-data'; if ($this->input->isInteractive() && !$this->confirm("Your webserver user has been detected as [{$user}]: is this correct?", true)) { $user = $this->anticipate( @@ -279,9 +277,7 @@ protected function resolveOwnership(): array $group = $this->option('group'); if (is_null($group)) { - $group = function_exists('posix_getgrgid') - ? (posix_getgrgid(filegroup('public'))['name'] ?? 'www-data') - : 'www-data'; + $group = posix_getgrgid(filegroup('public'))['name'] ?? 'www-data'; if ($this->input->isInteractive() && !$this->confirm("Your webserver group has been detected as [{$group}]: is this correct?", true)) { $group = $this->anticipate( diff --git a/tests/Unit/Console/Commands/UpgradeCommandTest.php b/tests/Unit/Console/Commands/UpgradeCommandTest.php index 8ab50a32e4..2d2e1bbc10 100644 --- a/tests/Unit/Console/Commands/UpgradeCommandTest.php +++ b/tests/Unit/Console/Commands/UpgradeCommandTest.php @@ -301,14 +301,10 @@ public function testAnExplicitReleaseIsResolvedToATaggedArchive(): void } /** - * Mirrors the owner the command detects, which differs between platforms. + * Mirrors the owner the command detects, which varies per machine. */ private function detected(string $lookup): string { - if (!function_exists($lookup)) { - return 'www-data'; - } - $id = $lookup === 'posix_getpwuid' ? fileowner('public') : filegroup('public'); return $lookup($id)['name'] ?? 'www-data';