From 246e3cd8a2861f8d981f5f917819deb4fb6c6d35 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 27 Nov 2025 09:04:42 +0100 Subject: [PATCH] fix: resolve Docker validation race conditions and sudo prefix bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix sudo prefix bug: Use word boundary matching to prevent 'do' keyword from matching 'docker' commands - Add ensureProxyNetworksExist() helper to create networks before docker compose up - Ensure networks exist synchronously before dispatching async proxy startup to prevent race conditions - Update comprehensive unit tests for sudo parsing (50 tests passing) This resolves issues where Docker commands failed to execute with sudo on non-root servers and where proxy networks were not created before the proxy container started. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- app/Actions/Proxy/StartProxy.php | 4 + app/Jobs/ValidateAndInstallServerJob.php | 3 + app/Livewire/Server/ValidateAndInstall.php | 3 + bootstrap/helpers/proxy.php | 31 +++++ bootstrap/helpers/sudo.php | 69 ++++++----- tests/Unit/ParseCommandsByLineForSudoTest.php | 113 +++++++++++++++++- 6 files changed, 191 insertions(+), 32 deletions(-) diff --git a/app/Actions/Proxy/StartProxy.php b/app/Actions/Proxy/StartProxy.php index bfc65d8d2..20c997656 100644 --- a/app/Actions/Proxy/StartProxy.php +++ b/app/Actions/Proxy/StartProxy.php @@ -75,6 +75,10 @@ public function handle(Server $server, bool $async = true, bool $force = false, ' done', " echo 'Successfully stopped and removed existing coolify-proxy.'", 'fi', + ]); + // Ensure required networks exist BEFORE docker compose up (networks are declared as external) + $commands = $commands->merge(ensureProxyNetworksExist($server)); + $commands = $commands->merge([ "echo 'Starting coolify-proxy.'", 'docker compose up -d --wait --remove-orphans', "echo 'Successfully started coolify-proxy.'", diff --git a/app/Jobs/ValidateAndInstallServerJob.php b/app/Jobs/ValidateAndInstallServerJob.php index ff5c2e4f5..b5e1929de 100644 --- a/app/Jobs/ValidateAndInstallServerJob.php +++ b/app/Jobs/ValidateAndInstallServerJob.php @@ -168,6 +168,9 @@ public function handle(): void if (! $this->server->isBuildServer()) { $proxyShouldRun = CheckProxy::run($this->server, true); if ($proxyShouldRun) { + // Ensure networks exist BEFORE dispatching async proxy startup + // This prevents race condition where proxy tries to start before networks are created + instant_remote_process(ensureProxyNetworksExist($this->server)->toArray(), $this->server, false); StartProxy::dispatch($this->server); } } diff --git a/app/Livewire/Server/ValidateAndInstall.php b/app/Livewire/Server/ValidateAndInstall.php index c2dcd877b..1a5bd381b 100644 --- a/app/Livewire/Server/ValidateAndInstall.php +++ b/app/Livewire/Server/ValidateAndInstall.php @@ -206,6 +206,9 @@ public function validateDockerVersion() if (! $proxyShouldRun) { return; } + // Ensure networks exist BEFORE dispatching async proxy startup + // This prevents race condition where proxy tries to start before networks are created + instant_remote_process(ensureProxyNetworksExist($this->server)->toArray(), $this->server, false); StartProxy::dispatch($this->server); } else { $requiredDockerVersion = str(config('constants.docker.minimum_required_version'))->before('.'); diff --git a/bootstrap/helpers/proxy.php b/bootstrap/helpers/proxy.php index 08fad4958..6672f8b6f 100644 --- a/bootstrap/helpers/proxy.php +++ b/bootstrap/helpers/proxy.php @@ -108,6 +108,37 @@ function connectProxyToNetworks(Server $server) return $commands->flatten(); } + +/** + * Ensures all required networks exist before docker compose up. + * This must be called BEFORE docker compose up since the compose file declares networks as external. + * + * @param Server $server The server to ensure networks on + * @return \Illuminate\Support\Collection Commands to create networks if they don't exist + */ +function ensureProxyNetworksExist(Server $server) +{ + ['allNetworks' => $networks] = collectDockerNetworksByServer($server); + + if ($server->isSwarm()) { + $commands = $networks->map(function ($network) { + return [ + "echo 'Ensuring network $network exists...'", + "docker network ls --format '{{.Name}}' | grep -q '^{$network}$' || docker network create --driver overlay --attachable $network", + ]; + }); + } else { + $commands = $networks->map(function ($network) { + return [ + "echo 'Ensuring network $network exists...'", + "docker network ls --format '{{.Name}}' | grep -q '^{$network}$' || docker network create --attachable $network", + ]; + }); + } + + return $commands->flatten(); +} + function extractCustomProxyCommands(Server $server, string $existing_config): array { $custom_commands = []; diff --git a/bootstrap/helpers/sudo.php b/bootstrap/helpers/sudo.php index 7a7fc3680..c25e44a72 100644 --- a/bootstrap/helpers/sudo.php +++ b/bootstrap/helpers/sudo.php @@ -23,38 +23,51 @@ function shouldChangeOwnership(string $path): bool function parseCommandsByLineForSudo(Collection $commands, Server $server): array { $commands = $commands->map(function ($line) { - if ( - ! str(trim($line))->startsWith([ - 'cd', - 'command', - 'echo', - 'true', - 'if', - 'fi', - 'for', - 'do', - 'done', - 'while', - 'until', - 'case', - 'esac', - 'select', - 'then', - 'else', - 'elif', - 'break', - 'continue', - '#', - ]) - ) { - return "sudo $line"; + $trimmedLine = trim($line); + + // All bash keywords that should not receive sudo prefix + // Using word boundary matching to avoid prefix collisions (e.g., 'do' vs 'docker', 'if' vs 'ifconfig', 'fi' vs 'find') + $bashKeywords = [ + 'cd', + 'command', + 'echo', + 'true', + 'if', + 'fi', + 'for', + 'done', + 'while', + 'until', + 'case', + 'esac', + 'select', + 'then', + 'else', + 'elif', + 'break', + 'continue', + 'do', + ]; + + // Special case: comments (no collision risk with '#') + if (str_starts_with($trimmedLine, '#')) { + return $line; } - if (str(trim($line))->startsWith('if')) { - return str_replace('if', 'if sudo', $line); + // Check all keywords with word boundary matching + // Match keyword followed by space, semicolon, or end of line + foreach ($bashKeywords as $keyword) { + if (preg_match('/^'.preg_quote($keyword, '/').'(\s|;|$)/', $trimmedLine)) { + // Special handling for 'if' - insert sudo after 'if ' + if ($keyword === 'if') { + return preg_replace('/^(\s*)if\s+/', '$1if sudo ', $line); + } + + return $line; + } } - return $line; + return "sudo $line"; }); $commands = $commands->map(function ($line) use ($server) { diff --git a/tests/Unit/ParseCommandsByLineForSudoTest.php b/tests/Unit/ParseCommandsByLineForSudoTest.php index 3f49c1c88..f294de35f 100644 --- a/tests/Unit/ParseCommandsByLineForSudoTest.php +++ b/tests/Unit/ParseCommandsByLineForSudoTest.php @@ -272,11 +272,9 @@ $result = parseCommandsByLineForSudo($commands, $this->server); - // Note: 'docker' starts with 'do' which is an excluded keyword, so it doesn't get sudo prefix - // This is a known limitation of the startsWith approach. Docker commands still work because - // they typically appear in more complex command sequences or are handled separately. + // docker commands now correctly get sudo prefix (word boundary fix for 'do' keyword) // The || operator adds sudo to what follows, and subshell adds sudo inside $() - expect($result[0])->toBe('docker ps || sudo echo $(sudo date)'); + expect($result[0])->toBe('sudo docker ps || sudo echo $(sudo date)'); }); test('handles whitespace-only commands gracefully', function () { @@ -516,3 +514,110 @@ expect($result[0])->not->toContain('sudo select'); expect($result[2])->toBe(' break'); }); + +// Tests for word boundary matching - ensuring commands are not confused with bash keywords + +test('adds sudo for ifconfig command (not confused with if keyword)', function () { + $commands = collect(['ifconfig eth0']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('sudo ifconfig eth0'); + expect($result[0])->not->toContain('if sudo'); +}); + +test('adds sudo for ifup command (not confused with if keyword)', function () { + $commands = collect(['ifup eth0']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('sudo ifup eth0'); +}); + +test('adds sudo for ifdown command (not confused with if keyword)', function () { + $commands = collect(['ifdown eth0']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('sudo ifdown eth0'); +}); + +test('adds sudo for find command (not confused with fi keyword)', function () { + $commands = collect(['find /var -name "*.log"']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('sudo find /var -name "*.log"'); +}); + +test('adds sudo for file command (not confused with fi keyword)', function () { + $commands = collect(['file /tmp/test']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('sudo file /tmp/test'); +}); + +test('adds sudo for finger command (not confused with fi keyword)', function () { + $commands = collect(['finger user']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('sudo finger user'); +}); + +test('adds sudo for docker command (not confused with do keyword)', function () { + $commands = collect(['docker ps']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('sudo docker ps'); +}); + +test('adds sudo for fortune command (not confused with for keyword)', function () { + $commands = collect(['fortune']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('sudo fortune'); +}); + +test('adds sudo for formail command (not confused with for keyword)', function () { + $commands = collect(['formail -s procmail']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('sudo formail -s procmail'); +}); + +test('if keyword with word boundary gets sudo inserted correctly', function () { + $commands = collect(['if [ -f /tmp/test ]; then']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('if sudo [ -f /tmp/test ]; then'); +}); + +test('fi keyword with word boundary is not given sudo', function () { + $commands = collect(['fi']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('fi'); +}); + +test('for keyword with word boundary is not given sudo', function () { + $commands = collect(['for i in 1 2 3; do']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('for i in 1 2 3; do'); +}); + +test('do keyword with word boundary is not given sudo', function () { + $commands = collect(['do']); + + $result = parseCommandsByLineForSudo($commands, $this->server); + + expect($result[0])->toBe('do'); +});