diff --git a/bootstrap/helpers/sudo.php b/bootstrap/helpers/sudo.php index ec79300e30..acbe45c1c9 100644 --- a/bootstrap/helpers/sudo.php +++ b/bootstrap/helpers/sudo.php @@ -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 `. 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; } diff --git a/tests/Unit/ParseCommandsByLineForSudoTest.php b/tests/Unit/ParseCommandsByLineForSudoTest.php index f9309ac5b9..5a98398da2 100644 --- a/tests/Unit/ParseCommandsByLineForSudoTest.php +++ b/tests/Unit/ParseCommandsByLineForSudoTest.php @@ -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',