From a1c30cb0e70b84e075d1c444362e7b198ad459e3 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 10 Mar 2026 22:22:48 +0100 Subject: [PATCH] fix(git-ref-validation): prevent command injection via git references Add validateGitRef() helper function that uses an allowlist approach to prevent OS command injection through git commit SHAs, branch names, and tags. Only allows alphanumeric characters, dots, hyphens, underscores, and slashes. Changes include: - Add validateGitRef() helper in bootstrap/helpers/shared.php - Apply validation in Rollback component when accepting rollback commit - Add regex validation to git commit SHA fields in Livewire components - Apply regex validation to API rules for git_commit_sha - Use escapeshellarg() in git log and git checkout commands - Add comprehensive unit tests covering injection payloads Addresses GHSA-mw5w-2vvh-mgf4 --- app/Jobs/ApplicationDeploymentJob.php | 2 +- app/Livewire/Project/Application/General.php | 4 +- app/Livewire/Project/Application/Rollback.php | 2 + app/Livewire/Project/Application/Source.php | 2 +- app/Models/Application.php | 3 +- bootstrap/helpers/api.php | 2 +- bootstrap/helpers/shared.php | 33 +++++ tests/Unit/GitRefValidationTest.php | 123 ++++++++++++++++++ 8 files changed, 165 insertions(+), 6 deletions(-) create mode 100644 tests/Unit/GitRefValidationTest.php diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index c9f0f1eef..91c81b6ff 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -2196,7 +2196,7 @@ private function clone_repository() $this->create_workdir(); $this->execute_remote_command( [ - executeInDocker($this->deployment_uuid, "cd {$this->workdir} && git log -1 {$this->commit} --pretty=%B"), + executeInDocker($this->deployment_uuid, "cd {$this->workdir} && git log -1 ".escapeshellarg($this->commit)." --pretty=%B"), 'hidden' => true, 'save' => 'commit_message', ] diff --git a/app/Livewire/Project/Application/General.php b/app/Livewire/Project/Application/General.php index 008bd3905..fccd17217 100644 --- a/app/Livewire/Project/Application/General.php +++ b/app/Livewire/Project/Application/General.php @@ -37,7 +37,7 @@ class General extends Component #[Validate(['required'])] public string $gitBranch; - #[Validate(['string', 'nullable'])] + #[Validate(['string', 'nullable', 'regex:/^[a-zA-Z0-9][a-zA-Z0-9._\-\/]*$/'])] public ?string $gitCommitSha = null; #[Validate(['string', 'nullable'])] @@ -184,7 +184,7 @@ protected function rules(): array 'fqdn' => 'nullable', 'gitRepository' => 'required', 'gitBranch' => 'required', - 'gitCommitSha' => 'nullable', + 'gitCommitSha' => ['nullable', 'regex:/^[a-zA-Z0-9][a-zA-Z0-9._\-\/]*$/'], 'installCommand' => 'nullable', 'buildCommand' => 'nullable', 'startCommand' => 'nullable', diff --git a/app/Livewire/Project/Application/Rollback.php b/app/Livewire/Project/Application/Rollback.php index e8edf72fa..3edd77833 100644 --- a/app/Livewire/Project/Application/Rollback.php +++ b/app/Livewire/Project/Application/Rollback.php @@ -50,6 +50,8 @@ public function rollbackImage($commit) { $this->authorize('deploy', $this->application); + $commit = validateGitRef($commit, 'rollback commit'); + $deployment_uuid = new Cuid2; $result = queue_application_deployment( diff --git a/app/Livewire/Project/Application/Source.php b/app/Livewire/Project/Application/Source.php index ab2517f2b..422dd6b28 100644 --- a/app/Livewire/Project/Application/Source.php +++ b/app/Livewire/Project/Application/Source.php @@ -30,7 +30,7 @@ class Source extends Component #[Validate(['required', 'string'])] public string $gitBranch; - #[Validate(['nullable', 'string'])] + #[Validate(['nullable', 'string', 'regex:/^[a-zA-Z0-9][a-zA-Z0-9._\-\/]*$/'])] public ?string $gitCommitSha = null; #[Locked] diff --git a/app/Models/Application.php b/app/Models/Application.php index a4f51780e..34ab4141e 100644 --- a/app/Models/Application.php +++ b/app/Models/Application.php @@ -1686,7 +1686,8 @@ public function fqdns(): Attribute protected function buildGitCheckoutCommand($target): string { - $command = "git checkout $target"; + $escapedTarget = escapeshellarg($target); + $command = "git checkout {$escapedTarget}"; if ($this->settings->is_git_submodules_enabled) { $command .= ' && git submodule update --init --recursive'; diff --git a/bootstrap/helpers/api.php b/bootstrap/helpers/api.php index edb1e59a1..1b03a4d37 100644 --- a/bootstrap/helpers/api.php +++ b/bootstrap/helpers/api.php @@ -92,7 +92,7 @@ function sharedDataApplications() 'static_image' => Rule::enum(StaticImageTypes::class), 'domains' => 'string|nullable', 'redirect' => Rule::enum(RedirectTypes::class), - 'git_commit_sha' => 'string', + 'git_commit_sha' => ['string', 'regex:/^[a-zA-Z0-9][a-zA-Z0-9._\-\/]*$/'], 'docker_registry_image_name' => 'string|nullable', 'docker_registry_image_tag' => 'string|nullable', 'install_command' => 'string|nullable', diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index 3e993dbf3..ce40466b2 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -147,6 +147,39 @@ function validateShellSafePath(string $input, string $context = 'path'): string return $input; } +/** + * Validate that a string is a safe git ref (commit SHA, branch name, tag, or HEAD). + * + * Prevents command injection by enforcing an allowlist of characters valid for git refs. + * Valid: hex SHAs, HEAD, branch/tag names (alphanumeric, dots, hyphens, underscores, slashes). + * + * @param string $input The git ref to validate + * @param string $context Descriptive name for error messages + * @return string The validated input (trimmed) + * + * @throws \Exception If the input contains disallowed characters + */ +function validateGitRef(string $input, string $context = 'git ref'): string +{ + $input = trim($input); + + if ($input === '' || $input === 'HEAD') { + return $input; + } + + // Must not start with a hyphen (git flag injection) + if (str_starts_with($input, '-')) { + throw new \Exception("Invalid {$context}: must not start with a hyphen."); + } + + // Allow only alphanumeric characters, dots, hyphens, underscores, and slashes + if (! preg_match('/^[a-zA-Z0-9][a-zA-Z0-9._\-\/]*$/', $input)) { + throw new \Exception("Invalid {$context}: contains disallowed characters. Only alphanumeric characters, dots, hyphens, underscores, and slashes are allowed."); + } + + return $input; +} + function generate_readme_file(string $name, string $updated_at): string { $name = sanitize_string($name); diff --git a/tests/Unit/GitRefValidationTest.php b/tests/Unit/GitRefValidationTest.php new file mode 100644 index 000000000..58d07f4b7 --- /dev/null +++ b/tests/Unit/GitRefValidationTest.php @@ -0,0 +1,123 @@ +toBe('abc123def456'); + expect(validateGitRef('a3e59e5c9'))->toBe('a3e59e5c9'); + expect(validateGitRef('abc123def456abc123def456abc123def456abc123'))->toBe('abc123def456abc123def456abc123def456abc123'); + }); + + test('accepts HEAD', function () { + expect(validateGitRef('HEAD'))->toBe('HEAD'); + }); + + test('accepts empty string', function () { + expect(validateGitRef(''))->toBe(''); + }); + + test('accepts branch and tag names', function () { + expect(validateGitRef('main'))->toBe('main'); + expect(validateGitRef('feature/my-branch'))->toBe('feature/my-branch'); + expect(validateGitRef('v1.2.3'))->toBe('v1.2.3'); + expect(validateGitRef('release-2.0'))->toBe('release-2.0'); + expect(validateGitRef('my_branch'))->toBe('my_branch'); + }); + + test('trims whitespace', function () { + expect(validateGitRef(' abc123 '))->toBe('abc123'); + }); + + test('rejects single quote injection', function () { + expect(fn () => validateGitRef("HEAD'; id >/tmp/poc; #")) + ->toThrow(Exception::class); + }); + + test('rejects semicolon command separator', function () { + expect(fn () => validateGitRef('abc123; rm -rf /')) + ->toThrow(Exception::class); + }); + + test('rejects command substitution with $()', function () { + expect(fn () => validateGitRef('$(whoami)')) + ->toThrow(Exception::class); + }); + + test('rejects backtick command substitution', function () { + expect(fn () => validateGitRef('`whoami`')) + ->toThrow(Exception::class); + }); + + test('rejects pipe operator', function () { + expect(fn () => validateGitRef('abc | cat /etc/passwd')) + ->toThrow(Exception::class); + }); + + test('rejects ampersand operator', function () { + expect(fn () => validateGitRef('abc & whoami')) + ->toThrow(Exception::class); + }); + + test('rejects hash comment injection', function () { + expect(fn () => validateGitRef('abc #')) + ->toThrow(Exception::class); + }); + + test('rejects newline injection', function () { + expect(fn () => validateGitRef("abc\nwhoami")) + ->toThrow(Exception::class); + }); + + test('rejects redirect operators', function () { + expect(fn () => validateGitRef('abc > /tmp/out')) + ->toThrow(Exception::class); + }); + + test('rejects hyphen-prefixed input (git flag injection)', function () { + expect(fn () => validateGitRef('--upload-pack=malicious')) + ->toThrow(Exception::class); + }); + + test('rejects the exact PoC payload from advisory', function () { + expect(fn () => validateGitRef("HEAD'; whoami >/tmp/coolify_poc_git; #")) + ->toThrow(Exception::class); + }); +}); + +describe('executeInDocker git log escaping', function () { + test('git log command escapes commit SHA to prevent injection', function () { + $maliciousCommit = "HEAD'; id; #"; + $command = "cd /workdir && git log -1 ".escapeshellarg($maliciousCommit).' --pretty=%B'; + $result = executeInDocker('test-container', $command); + + // The malicious payload must not be able to break out of quoting + expect($result)->not->toContain("id;"); + expect($result)->toContain("'HEAD'\\''"); + }); +}); + +describe('buildGitCheckoutCommand escaping', function () { + test('checkout command escapes target to prevent injection', function () { + $app = new \App\Models\Application; + $app->forceFill(['uuid' => 'test-uuid']); + + $settings = new \App\Models\ApplicationSetting; + $settings->is_git_submodules_enabled = false; + $app->setRelation('settings', $settings); + + $method = new \ReflectionMethod($app, 'buildGitCheckoutCommand'); + + $result = $method->invoke($app, 'abc123'); + expect($result)->toContain("git checkout 'abc123'"); + + $result = $method->invoke($app, "abc'; id; #"); + expect($result)->not->toContain("id;"); + expect($result)->toContain("git checkout 'abc'"); + }); +});