From 82bb574760bbf058bb70ab6a2923f79a3fd6f3cd Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 25 Sep 2026 13:15:57 +0200 Subject: [PATCH] fix(servers): stop double sudo on chown/chmod after mkdir The mkdir ownership rule prepended `sudo` to chown/chmod, and the && rule then added another, producing `sudo sudo`. That fails on hosts like Alpine, where root is not in sudoers. Both parseCommandsByLineForSudo and parseLineForSudo now leave the prefix to the && rule. Also: - Fix the `App\Helpers\SSLHelper` reference in RegenerateSslCertJob to `SslHelper`, so it autoloads on case-sensitive filesystems. - Stop StartPostgresql and StartMongodb from overriding the configuration dir with a hardcoded Docker volume path in development. - Add tests for nested sudo behaviour, the class reference letter case and database configuration dirs in development. --- app/Actions/Database/StartMongodb.php | 3 - app/Actions/Database/StartPostgresql.php | 3 - app/Jobs/RegenerateSslCertJob.php | 4 +- bootstrap/helpers/sudo.php | 6 +- .../DatabaseStartDevConfigurationDirTest.php | 65 +++++++++++++++++++ tests/Unit/ClassReferenceCaseTest.php | 43 ++++++++++++ tests/Unit/ParseCommandsByLineForSudoTest.php | 34 ++++++++-- 7 files changed, 142 insertions(+), 16 deletions(-) create mode 100644 tests/Feature/DatabaseStartDevConfigurationDirTest.php create mode 100644 tests/Unit/ClassReferenceCaseTest.php diff --git a/app/Actions/Database/StartMongodb.php b/app/Actions/Database/StartMongodb.php index 1f389a101f..f85d0c86fa 100644 --- a/app/Actions/Database/StartMongodb.php +++ b/app/Actions/Database/StartMongodb.php @@ -36,9 +36,6 @@ class StartMongodb $container_name = $this->database->uuid; $this->configuration_dir = database_configuration_dir().'/'.$container_name; - if (isDev()) { - $this->configuration_dir = '/var/lib/docker/volumes/coolify_dev_coolify_data/_data/databases/'.$container_name; - } $this->commands = [ "echo 'Starting database.'", diff --git a/app/Actions/Database/StartPostgresql.php b/app/Actions/Database/StartPostgresql.php index 4766fd8157..18e5413200 100644 --- a/app/Actions/Database/StartPostgresql.php +++ b/app/Actions/Database/StartPostgresql.php @@ -33,9 +33,6 @@ class StartPostgresql $this->database = $database; $container_name = $this->database->uuid; $this->configuration_dir = database_configuration_dir().'/'.$container_name; - if (isDev()) { - $this->configuration_dir = '/var/lib/docker/volumes/coolify_dev_coolify_data/_data/databases/'.$container_name; - } $this->commands = [ "echo 'Starting database.'", diff --git a/app/Jobs/RegenerateSslCertJob.php b/app/Jobs/RegenerateSslCertJob.php index ed2d1c4546..3f6ca8e472 100644 --- a/app/Jobs/RegenerateSslCertJob.php +++ b/app/Jobs/RegenerateSslCertJob.php @@ -2,7 +2,7 @@ namespace App\Jobs; -use App\Helpers\SSLHelper; +use App\Helpers\SslHelper; use App\Models\SslCertificate; use App\Models\Team; use App\Notifications\SslExpirationNotification; @@ -55,7 +55,7 @@ class RegenerateSslCertJob implements ShouldBeEncrypted, ShouldQueue return; } - SSLHelper::generateSslCertificate( + SslHelper::generateSslCertificate( commonName: $certificate->common_name, subjectAlternativeNames: $certificate->subject_alternative_names, resourceType: $certificate->resource_type, diff --git a/bootstrap/helpers/sudo.php b/bootstrap/helpers/sudo.php index 98dbe3af7a..1622bbac99 100644 --- a/bootstrap/helpers/sudo.php +++ b/bootstrap/helpers/sudo.php @@ -84,7 +84,8 @@ function parseCommandsByLineForSudo(Collection $commands, Server $server): array if (Str::startsWith($line, 'sudo mkdir -p')) { $path = trim(Str::after($line, 'sudo mkdir -p')); if (shouldChangeOwnership($path)) { - return "$line && sudo chown -R $server->user:$server->user $path && sudo chmod -R o-rwx $path"; + // No sudo here: the && rule below adds it. `sudo sudo` fails where root is not in sudoers (Alpine). + return "$line && chown -R $server->user:$server->user $path && chmod -R o-rwx $path"; } return $line; @@ -141,7 +142,8 @@ function parseLineForSudo(string $command, Server $server): string if (Str::startsWith($command, 'sudo mkdir -p')) { $path = trim(Str::after($command, 'sudo mkdir -p')); if (shouldChangeOwnership($path)) { - $command = "$command && sudo chown -R $server->user:$server->user $path && sudo chmod -R o-rwx $path"; + // No sudo here: the && rule below adds it. + $command = "$command && chown -R $server->user:$server->user $path && chmod -R o-rwx $path"; } } if (str($command)->contains('$(') || str($command)->contains('`')) { diff --git a/tests/Feature/DatabaseStartDevConfigurationDirTest.php b/tests/Feature/DatabaseStartDevConfigurationDirTest.php new file mode 100644 index 0000000000..a03c91c32e --- /dev/null +++ b/tests/Feature/DatabaseStartDevConfigurationDirTest.php @@ -0,0 +1,65 @@ + 0]); + config(['app.env' => 'local', 'app.maintenance.store' => 'array', 'cache.default' => 'array']); + Process::fake(); + Queue::fake(); + + $team = Team::factory()->create(); + $privateKey = PrivateKey::factory()->create(['team_id' => $team->id]); + $server = Server::factory()->create(['team_id' => $team->id, 'private_key_id' => $privateKey->id]); + $this->destination = StandaloneDocker::query()->where('server_id', $server->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $team->id]); + $this->environment = Environment::factory()->create(['project_id' => $project->id]); + + $this->executor = new class + { + public array $commands = []; + + public function execute(array $commands, $database, Activity $activity): Activity + { + $this->commands = $commands; + + return $activity; + } + }; + app()->instance(DatabaseStartCommandExecutor::class, $this->executor); +}); + +test('databases with SSL use their normal configuration folder in development', function (string $type) { + $database = $type === 'postgresql' + ? create_standalone_postgresql($this->environment->id, $this->destination) + : create_standalone_mongodb($this->environment->id, $this->destination); + $database->update(['enable_ssl' => true]); + + ($type === 'postgresql' ? StartPostgresql::class : StartMongodb::class)::run($database->fresh(), new Activity); + + $certificateFiles = $database->fileStorages()->pluck('fs_path'); + + expect(isDev())->toBeTrue() + ->and(implode("\n", $this->executor->commands)) + ->toContain('mkdir -p '.$database->workdir()) + ->not->toContain('coolify_dev_coolify_data') + ->and($certificateFiles)->not->toBeEmpty() + ->each->toStartWith($database->workdir().'/ssl/'); + // The file-mount path check accepts every certificate file. + $certificateFiles->each(fn (string $path) => confinePathToBase($database->workdir(), $path, 'storage path')); +})->with(['postgresql', 'mongodb']); diff --git a/tests/Unit/ClassReferenceCaseTest.php b/tests/Unit/ClassReferenceCaseTest.php new file mode 100644 index 0000000000..23fd174509 --- /dev/null +++ b/tests/Unit/ClassReferenceCaseTest.php @@ -0,0 +1,43 @@ +first(fn (string $entry) => strcasecmp($entry, $part) === 0); + if ($match === null) { + return null; + } + $current .= '/'.$match; + } + + return substr($current, strlen($root) + 1); +} + +test('every App class reference matches the letter case of its file', function () { + // Autoloading on Linux is case-sensitive: `App\Helpers\SSLHelper` does not load SslHelper.php. + $root = dirname(__DIR__, 2); + $mismatches = []; + + foreach (['app', 'bootstrap', 'routes', 'database', 'config', 'resources/views'] as $directory) { + $files = new RecursiveIteratorIterator(new RecursiveDirectoryIterator("{$root}/{$directory}", FilesystemIterator::SKIP_DOTS)); + foreach ($files as $file) { + if ($file->getExtension() !== 'php') { + continue; + } + preg_match_all('/\bApp\\\\((?:\w+\\\\)*\w+)(?=::|\(|\s|;|,|\))/', file_get_contents($file->getPathname()), $matches); + foreach (array_unique($matches[1]) as $class) { + $path = 'app/'.str_replace('\\', '/', $class).'.php'; + if (! file_exists("{$root}/{$path}") && ($realPath = differentlyCasedFile($root, $path))) { + $mismatches[] = substr($file->getPathname(), strlen($root) + 1).": App\\{$class} -> {$realPath}"; + } + } + } + } + + expect($mismatches)->toBe([]); +}); diff --git a/tests/Unit/ParseCommandsByLineForSudoTest.php b/tests/Unit/ParseCommandsByLineForSudoTest.php index 673b6a023f..b80cb71812 100644 --- a/tests/Unit/ParseCommandsByLineForSudoTest.php +++ b/tests/Unit/ParseCommandsByLineForSudoTest.php @@ -276,9 +276,7 @@ test('adds ownership changes for Coolify data paths', function () { $result = parseCommandsByLineForSudo($commands, $this->server); - // Note: The && operator adds another sudo, creating double sudo for chown/chmod - // This is existing behavior that may need refactoring but isn't part of this bug fix - expect($result[0])->toBe('sudo mkdir -p /data/coolify/logs && sudo sudo chown -R ubuntu:ubuntu /data/coolify/logs && sudo sudo chmod -R o-rwx /data/coolify/logs'); + expect($result[0])->toBe('sudo mkdir -p /data/coolify/logs && sudo chown -R ubuntu:ubuntu /data/coolify/logs && sudo chmod -R o-rwx /data/coolify/logs'); }); test('adds ownership changes for Coolify tmp paths', function () { @@ -288,11 +286,35 @@ test('adds ownership changes for Coolify tmp paths', function () { $result = parseCommandsByLineForSudo($commands, $this->server); - // Note: The && operator adds another sudo, creating double sudo for chown/chmod - // This is existing behavior that may need refactoring but isn't part of this bug fix - expect($result[0])->toBe('sudo mkdir -p /tmp/coolify/cache && sudo sudo chown -R ubuntu:ubuntu /tmp/coolify/cache && sudo sudo chmod -R o-rwx /tmp/coolify/cache'); + expect($result[0])->toBe('sudo mkdir -p /tmp/coolify/cache && sudo chown -R ubuntu:ubuntu /tmp/coolify/cache && sudo chmod -R o-rwx /tmp/coolify/cache'); }); +test('ownership changes work where root may not use sudo', function (string $parser) { + $directory = sys_get_temp_dir().'/coolify-sudo-'.bin2hex(random_bytes(4)); + mkdir($directory); + // Like Alpine: root is not in the sudoers file, so a nested sudo fails. + file_put_contents("{$directory}/sudo", "#!/bin/sh\nif [ -n \"\$IN_SUDO\" ]; then echo 'root is not in the sudoers file' >&2; exit 1; fi\nexport IN_SUDO=1\nexec \"\$@\"\n"); + foreach (['chown', 'chmod'] as $tool) { + file_put_contents("{$directory}/{$tool}", "#!/bin/sh\necho \"{$tool} \$*\" >> \"\$LOG\"\n"); + } + array_map(fn (string $tool) => chmod("{$directory}/{$tool}", 0755), ['sudo', 'chown', 'chmod']); + $path = '/tmp/coolify/sudo-test-'.bin2hex(random_bytes(4)); + + $command = $parser === 'command list' + ? parseCommandsByLineForSudo(collect(["mkdir -p {$path}"]), $this->server)[0] + : parseLineForSudo("mkdir -p {$path}", $this->server); + $process = new Process(['/bin/sh', '-c', $command], env: ['PATH' => "{$directory}:".getenv('PATH'), 'LOG' => "{$directory}/log"]); + $process->run(); + + expect($command)->not->toMatch('/sudo\s+sudo/') + ->and($process->getErrorOutput())->toBe('') + ->and($process->isSuccessful())->toBeTrue() + ->and(is_dir($path))->toBeTrue() + ->and(file_get_contents("{$directory}/log"))->toBe("chown -R ubuntu:ubuntu {$path}\nchmod -R o-rwx {$path}\n"); + + (new Process(['rm', '-rf', $directory, $path]))->run(); +})->with(['command list', 'deployment line']); + test('does not add ownership changes for system paths', function () { $commands = collect([ 'mkdir -p /var/log',