From dac940807a88f5a236fbfd7923b1f1f336e3c43f Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 23 Mar 2026 21:55:46 +0100 Subject: [PATCH] fix(deployment): properly escape shell arguments in nixpacks commands Add escapeShellValue() helper function to safely escape shell values by wrapping them in single quotes and escaping embedded quotes. Use this function throughout the nixpacks command building to prevent shell injection vulnerabilities when passing user-provided build commands, start commands, and environment variables. This fixes unsafe string concatenation that could allow command injection when user input contains special shell characters like &&, |, ;, etc. --- app/Jobs/ApplicationDeploymentJob.php | 14 +++-- bootstrap/helpers/docker.php | 5 ++ ...plicationDeploymentNixpacksNullEnvTest.php | 24 ++++---- tests/Unit/EscapeShellValueTest.php | 57 +++++++++++++++++++ 4 files changed, 82 insertions(+), 18 deletions(-) create mode 100644 tests/Unit/EscapeShellValueTest.php diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index e30af5cc7..9d927d10c 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -2334,13 +2334,13 @@ private function nixpacks_build_cmd() $this->generate_nixpacks_env_variables(); $nixpacks_command = "nixpacks plan -f json {$this->env_nixpacks_args}"; if ($this->application->build_command) { - $nixpacks_command .= " --build-cmd \"{$this->application->build_command}\""; + $nixpacks_command .= ' --build-cmd '.escapeShellValue($this->application->build_command); } if ($this->application->start_command) { - $nixpacks_command .= " --start-cmd \"{$this->application->start_command}\""; + $nixpacks_command .= ' --start-cmd '.escapeShellValue($this->application->start_command); } if ($this->application->install_command) { - $nixpacks_command .= " --install-cmd \"{$this->application->install_command}\""; + $nixpacks_command .= ' --install-cmd '.escapeShellValue($this->application->install_command); } $nixpacks_command .= " {$this->workdir}"; @@ -2353,13 +2353,15 @@ private function generate_nixpacks_env_variables() if ($this->pull_request_id === 0) { foreach ($this->application->nixpacks_environment_variables as $env) { if (! is_null($env->real_value) && $env->real_value !== '') { - $this->env_nixpacks_args->push("--env {$env->key}={$env->real_value}"); + $value = ($env->is_literal || $env->is_multiline) ? trim($env->real_value, "'") : $env->real_value; + $this->env_nixpacks_args->push('--env '.escapeShellValue("{$env->key}={$value}")); } } } else { foreach ($this->application->nixpacks_environment_variables_preview as $env) { if (! is_null($env->real_value) && $env->real_value !== '') { - $this->env_nixpacks_args->push("--env {$env->key}={$env->real_value}"); + $value = ($env->is_literal || $env->is_multiline) ? trim($env->real_value, "'") : $env->real_value; + $this->env_nixpacks_args->push('--env '.escapeShellValue("{$env->key}={$value}")); } } } @@ -2369,7 +2371,7 @@ private function generate_nixpacks_env_variables() $coolify_envs->each(function ($value, $key) { // Only add environment variables with non-null and non-empty values if (! is_null($value) && $value !== '') { - $this->env_nixpacks_args->push("--env {$key}={$value}"); + $this->env_nixpacks_args->push('--env '.escapeShellValue("{$key}={$value}")); } }); diff --git a/bootstrap/helpers/docker.php b/bootstrap/helpers/docker.php index 7b74392cf..5905ed3c1 100644 --- a/bootstrap/helpers/docker.php +++ b/bootstrap/helpers/docker.php @@ -137,6 +137,11 @@ function checkMinimumDockerEngineVersion($dockerVersion) return $dockerVersion; } +function escapeShellValue(string $value): string +{ + return "'".str_replace("'", "'\\''", $value)."'"; +} + function executeInDocker(string $containerId, string $command) { $escapedCommand = str_replace("'", "'\\''", $command); diff --git a/tests/Unit/ApplicationDeploymentNixpacksNullEnvTest.php b/tests/Unit/ApplicationDeploymentNixpacksNullEnvTest.php index c2a8d46fa..4c7ec9d9d 100644 --- a/tests/Unit/ApplicationDeploymentNixpacksNullEnvTest.php +++ b/tests/Unit/ApplicationDeploymentNixpacksNullEnvTest.php @@ -88,11 +88,11 @@ $envArgsProperty->setAccessible(true); $envArgs = $envArgsProperty->getValue($job); - // Verify that only valid environment variables are included - expect($envArgs)->toContain('--env VALID_VAR=valid_value'); - expect($envArgs)->toContain('--env ANOTHER_VALID_VAR=another_value'); - expect($envArgs)->toContain('--env COOLIFY_FQDN=example.com'); - expect($envArgs)->toContain('--env SOURCE_COMMIT=abc123'); + // Verify that only valid environment variables are included (values are now single-quote escaped) + expect($envArgs)->toContain("--env 'VALID_VAR=valid_value'"); + expect($envArgs)->toContain("--env 'ANOTHER_VALID_VAR=another_value'"); + expect($envArgs)->toContain("--env 'COOLIFY_FQDN=example.com'"); + expect($envArgs)->toContain("--env 'SOURCE_COMMIT=abc123'"); // Verify that null and empty environment variables are filtered out expect($envArgs)->not->toContain('NULL_VAR'); @@ -102,7 +102,7 @@ // Verify no environment variables end with just '=' (which indicates null/empty value) expect($envArgs)->not->toMatch('/--env [A-Z_]+=$/'); - expect($envArgs)->not->toMatch('/--env [A-Z_]+= /'); + expect($envArgs)->not->toMatch("/--env '[A-Z_]+='$/"); }); it('filters out null environment variables from nixpacks preview deployments', function () { @@ -164,9 +164,9 @@ $envArgsProperty->setAccessible(true); $envArgs = $envArgsProperty->getValue($job); - // Verify that only valid environment variables are included - expect($envArgs)->toContain('--env PREVIEW_VAR=preview_value'); - expect($envArgs)->toContain('--env COOLIFY_FQDN=preview.example.com'); + // Verify that only valid environment variables are included (values are now single-quote escaped) + expect($envArgs)->toContain("--env 'PREVIEW_VAR=preview_value'"); + expect($envArgs)->toContain("--env 'COOLIFY_FQDN=preview.example.com'"); // Verify that null environment variables are filtered out expect($envArgs)->not->toContain('NULL_PREVIEW_VAR'); @@ -335,7 +335,7 @@ $envArgsProperty->setAccessible(true); $envArgs = $envArgsProperty->getValue($job); - // Verify that zero and false string values are preserved - expect($envArgs)->toContain('--env ZERO_VALUE=0'); - expect($envArgs)->toContain('--env FALSE_VALUE=false'); + // Verify that zero and false string values are preserved (values are now single-quote escaped) + expect($envArgs)->toContain("--env 'ZERO_VALUE=0'"); + expect($envArgs)->toContain("--env 'FALSE_VALUE=false'"); }); diff --git a/tests/Unit/EscapeShellValueTest.php b/tests/Unit/EscapeShellValueTest.php new file mode 100644 index 000000000..eed25e164 --- /dev/null +++ b/tests/Unit/EscapeShellValueTest.php @@ -0,0 +1,57 @@ +toBe("'hello'"); +}); + +it('escapes single quotes in the value', function () { + expect(escapeShellValue("it's"))->toBe("'it'\\''s'"); +}); + +it('handles empty string', function () { + expect(escapeShellValue(''))->toBe("''"); +}); + +it('preserves && in a single-quoted value', function () { + $result = escapeShellValue('npx prisma generate && npm run build'); + expect($result)->toBe("'npx prisma generate && npm run build'"); +}); + +it('preserves special shell characters in value', function () { + $result = escapeShellValue('echo $HOME; rm -rf /'); + expect($result)->toBe("'echo \$HOME; rm -rf /'"); +}); + +it('handles value with double quotes', function () { + $result = escapeShellValue('say "hello"'); + expect($result)->toBe("'say \"hello\"'"); +}); + +it('produces correct output when passed through executeInDocker', function () { + // Simulate the exact issue from GitHub #9042: + // NIXPACKS_BUILD_CMD with chained && commands + $envValue = 'npx prisma generate && npx prisma db push && npm run build'; + $escapedEnv = '--env '.escapeShellValue("NIXPACKS_BUILD_CMD={$envValue}"); + + $command = "nixpacks plan -f json {$escapedEnv} /app"; + $dockerCmd = executeInDocker('test-container', $command); + + // The && must NOT appear unquoted at the bash -c level + // The full docker command should properly nest the quoting + expect($dockerCmd)->toContain('NIXPACKS_BUILD_CMD=npx prisma generate && npx prisma db push && npm run build'); + // Verify it's wrapped in docker exec bash -c + expect($dockerCmd)->toStartWith("docker exec test-container bash -c '"); + expect($dockerCmd)->toEndWith("'"); +}); + +it('produces correct output for build-cmd with chained commands through executeInDocker', function () { + $buildCmd = 'npx prisma generate && npm run build'; + $escapedCmd = escapeShellValue($buildCmd); + + $command = "nixpacks plan -f json --build-cmd {$escapedCmd} /app"; + $dockerCmd = executeInDocker('test-container', $command); + + // The build command value must remain intact inside the quoting + expect($dockerCmd)->toContain('npx prisma generate && npm run build'); + expect($dockerCmd)->toStartWith("docker exec test-container bash -c '"); +});