mirror of
https://github.com/coollabsio/coolify.git
synced 2026-09-26 01:10:30 -04:00
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.
This commit is contained in:
@@ -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.'",
|
||||
|
||||
@@ -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.'",
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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('`')) {
|
||||
|
||||
@@ -0,0 +1,65 @@
|
||||
<?php
|
||||
|
||||
use App\Actions\Database\StartMongodb;
|
||||
use App\Actions\Database\StartPostgresql;
|
||||
use App\Models\Environment;
|
||||
use App\Models\InstanceSettings;
|
||||
use App\Models\PrivateKey;
|
||||
use App\Models\Project;
|
||||
use App\Models\Server;
|
||||
use App\Models\StandaloneDocker;
|
||||
use App\Models\Team;
|
||||
use App\Services\DatabaseStartCommandExecutor;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Illuminate\Support\Facades\Process;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
use Spatie\Activitylog\Models\Activity;
|
||||
|
||||
uses(RefreshDatabase::class);
|
||||
|
||||
beforeEach(function () {
|
||||
InstanceSettings::forceCreate(['id' => 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']);
|
||||
@@ -0,0 +1,43 @@
|
||||
<?php
|
||||
|
||||
/**
|
||||
* Returns the real path of a file when it exists with a different letter case.
|
||||
*/
|
||||
function differentlyCasedFile(string $root, string $relativePath): ?string
|
||||
{
|
||||
$current = $root;
|
||||
foreach (explode('/', $relativePath) as $part) {
|
||||
$entries = is_dir($current) ? scandir($current) : [];
|
||||
$match = collect($entries)->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([]);
|
||||
});
|
||||
@@ -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',
|
||||
|
||||
Reference in New Issue
Block a user