mirror of
https://github.com/coollabsio/coolify.git
synced 2026-11-06 13:10:37 -04:00
fix(server): keep commands after mkdir -p for non-root users
The sudo parser read everything after `mkdir -p` as the path, so the ownership step ended up after the last command of the line. For `mkdir -p /data/coolify/proxy/dynamic && ls ... | head -n 1000`, `head` got the `find` options and the proxy dynamic configurations failed on every server with a non-root user. - Add the ownership step right after the mkdir path; commands after `&&` or `;` stay unchanged - Lines with other forms (redirects, `||`, pipes) get no ownership step
This commit is contained in:
+21
-18
@@ -30,6 +30,25 @@ function ownershipCommand(string $path, Server $server): string
|
||||
return "find $path -user root -exec chown $server->user:$server->user {} + && chmod o-rwx $path";
|
||||
}
|
||||
|
||||
/**
|
||||
* Adds the ownership step right after `sudo mkdir -p <path>`. When more commands follow on the line
|
||||
* (`&& ...` or `; ...`), they stay unchanged after the ownership step. Other forms (redirects, `||`,
|
||||
* pipes, several paths) are left as they are.
|
||||
*/
|
||||
function addOwnershipAfterMkdir(string $line, Server $server): string
|
||||
{
|
||||
if (! preg_match('/^(sudo mkdir -p ([^\s;&|<>]+))(\s*(?:(?:&&|;).*)?)$/s', $line, $matches)) {
|
||||
return $line;
|
||||
}
|
||||
$path = $matches[2];
|
||||
if (! shouldChangeOwnership($path)) {
|
||||
return $line;
|
||||
}
|
||||
|
||||
// No sudo here: the && rule adds it. `sudo sudo` fails where root is not in sudoers (Alpine).
|
||||
return "{$matches[1]} && ".ownershipCommand($path, $server).($matches[3] ?? '');
|
||||
}
|
||||
|
||||
/**
|
||||
* A line that is one `sudo sh -c '...'` call already runs completely as root. Adding sudo inside the script
|
||||
* fails where root is not in sudoers (Alpine), and wrapping it again in `bash -c` needs bash.
|
||||
@@ -135,17 +154,7 @@ function parseCommandsByLineForSudo(Collection $commands, Server $server): array
|
||||
});
|
||||
|
||||
$commands = $commands->map(function ($line) use ($server) {
|
||||
if (Str::startsWith($line, 'sudo mkdir -p')) {
|
||||
$path = trim(Str::after($line, 'sudo mkdir -p'));
|
||||
if (shouldChangeOwnership($path)) {
|
||||
// No sudo here: the && rule below adds it. `sudo sudo` fails where root is not in sudoers (Alpine).
|
||||
return "$line && ".ownershipCommand($path, $server);
|
||||
}
|
||||
|
||||
return $line;
|
||||
}
|
||||
|
||||
return $line;
|
||||
return addOwnershipAfterMkdir($line, $server);
|
||||
});
|
||||
|
||||
$commands = $commands->map(function ($line) {
|
||||
@@ -197,13 +206,7 @@ function parseLineForSudo(string $command, Server $server): string
|
||||
if (! str($command)->startSwith('cd') && ! str($command)->startSwith('command')) {
|
||||
$command = "sudo $command";
|
||||
}
|
||||
if (Str::startsWith($command, 'sudo mkdir -p')) {
|
||||
$path = trim(Str::after($command, 'sudo mkdir -p'));
|
||||
if (shouldChangeOwnership($path)) {
|
||||
// No sudo here: the && rule below adds it.
|
||||
$command = "$command && ".ownershipCommand($path, $server);
|
||||
}
|
||||
}
|
||||
$command = addOwnershipAfterMkdir($command, $server);
|
||||
if (isSingleSudoShellScript($command)) {
|
||||
return $command;
|
||||
}
|
||||
|
||||
@@ -355,6 +355,42 @@ test('ownership changes keep container-owned files and file modes', function (st
|
||||
(new Process(['rm', '-rf', $directory, $path]))->run();
|
||||
})->with(['command list', 'deployment line']);
|
||||
|
||||
test('ownership changes apply only to the mkdir path when more commands follow', function (string $parser, string $separator) {
|
||||
$directory = sys_get_temp_dir().'/coolify-sudo-'.bin2hex(random_bytes(4));
|
||||
mkdir($directory);
|
||||
file_put_contents("{$directory}/sudo", "#!/bin/sh\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));
|
||||
$line = "mkdir -p {$path}/dynamic{$separator} ls -1 {$path}/dynamic | head -n 1000";
|
||||
|
||||
$command = $parser === 'command list'
|
||||
? parseCommandsByLineForSudo(collect([$line]), $this->server)[0]
|
||||
: parseLineForSudo($line, $this->server);
|
||||
$process = new Process(['bash', '-c', $command], env: ['PATH' => "{$directory}:".getenv('PATH'), 'LOG' => "{$directory}/log"]);
|
||||
$process->run();
|
||||
|
||||
expect($command)->toContain("find {$path}/dynamic -user root -exec chown ubuntu:ubuntu {} + && ")
|
||||
->and($command)->toContain("chmod o-rwx {$path}/dynamic{$separator} ")
|
||||
->and($command)->toEndWith('head -n 1000'.($parser === 'command list' ? "'" : ''))
|
||||
->and($process->getErrorOutput())->toBe('')
|
||||
->and($process->isSuccessful())->toBeTrue()
|
||||
->and(is_dir("{$path}/dynamic"))->toBeTrue()
|
||||
->and(file_get_contents("{$directory}/log"))->toEndWith("chmod o-rwx {$path}/dynamic\n");
|
||||
|
||||
(new Process(['rm', '-rf', $directory, $path]))->run();
|
||||
})->with(['command list', 'deployment line'])->with(['and' => ' &&', 'semicolon' => ';']);
|
||||
|
||||
test('does not add ownership changes when mkdir is followed by other operators', function () {
|
||||
$result = parseCommandsByLineForSudo(collect([
|
||||
'mkdir -p /data/coolify/proxy > /dev/null 2>&1 || true',
|
||||
]), $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo mkdir -p /data/coolify/proxy > /dev/null 2>&1 || sudo true');
|
||||
});
|
||||
|
||||
test('does not add ownership changes for system paths', function () {
|
||||
$commands = collect([
|
||||
'mkdir -p /var/log',
|
||||
|
||||
Reference in New Issue
Block a user