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'"); + }); +});