mirror of
https://github.com/coollabsio/coolify.git
synced 2026-06-13 19:09:50 +00:00
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.
This commit is contained in:
@@ -2334,13 +2334,13 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue
|
||||
$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 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue
|
||||
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 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue
|
||||
$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}"));
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -88,11 +88,11 @@ it('filters out null environment variables from nixpacks build command', functio
|
||||
$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 @@ it('filters out null environment variables from nixpacks build command', functio
|
||||
|
||||
// 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 @@ it('filters out null environment variables from nixpacks preview deployments', f
|
||||
$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 @@ it('preserves environment variables with zero values', function () {
|
||||
$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'");
|
||||
});
|
||||
|
||||
@@ -0,0 +1,57 @@
|
||||
<?php
|
||||
|
||||
it('wraps a simple value in single quotes', function () {
|
||||
expect(escapeShellValue('hello'))->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 '");
|
||||
});
|
||||
Reference in New Issue
Block a user