From bbff70c8d0c60ef4f49ed22fca8e1750b1d07fe9 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 2 Jul 2026 16:52:07 +0200 Subject: [PATCH] fix: improve application URL handling --- .../Api/ApplicationsController.php | 67 ++------ .../Controllers/Api/ServicesController.php | 25 +-- app/Jobs/ApplicationDeploymentJob.php | 32 +++- app/Support/ValidationPatterns.php | 156 ++++++++++++++++++ bootstrap/helpers/api.php | 2 +- .../Security/CommandInjectionSecurityTest.php | 103 +++++++++++- 6 files changed, 306 insertions(+), 79 deletions(-) diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index 9570026c1..d125c01ec 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -952,6 +952,10 @@ private function create_application(Request $request, $type) } $serverUuid = $request->server_uuid; $fqdn = $request->domains; + if ($request->has('domains') && is_string($request->domains)) { + $fqdn = ValidationPatterns::normalizeApplicationDomains($request->domains); + $request->offsetSet('domains', $fqdn); + } $autogenerateDomain = $request->boolean('autogenerate_domain', true); $instantDeploy = $request->instant_deploy; $githubAppUuid = $request->github_app_uuid; @@ -1031,7 +1035,7 @@ private function create_application(Request $request, $type) 'docker_compose_domains' => 'array|nullable', 'docker_compose_domains.*' => 'array:name,domain', 'docker_compose_domains.*.name' => 'string|required', - 'docker_compose_domains.*.domain' => 'string|nullable', + 'docker_compose_domains.*.domain' => ValidationPatterns::applicationDomainRules(), ]; // ports_exposes is not required for dockercompose if ($request->build_pack === 'dockercompose') { @@ -1239,7 +1243,7 @@ private function create_application(Request $request, $type) 'docker_compose_domains' => 'array|nullable', 'docker_compose_domains.*' => 'array:name,domain', 'docker_compose_domains.*.name' => 'string|required', - 'docker_compose_domains.*.domain' => 'string|nullable', + 'docker_compose_domains.*.domain' => ValidationPatterns::applicationDomainRules(), ]; $validationRules = array_merge(sharedDataApplications(), $validationRules); $validationMessages = [ @@ -1479,7 +1483,7 @@ private function create_application(Request $request, $type) 'docker_compose_domains' => 'array|nullable', 'docker_compose_domains.*' => 'array:name,domain', 'docker_compose_domains.*.name' => 'string|required', - 'docker_compose_domains.*.domain' => 'string|nullable', + 'docker_compose_domains.*.domain' => ValidationPatterns::applicationDomainRules(), ]; $validationRules = array_merge(sharedDataApplications(), $validationRules); @@ -2378,7 +2382,7 @@ public function update_by_uuid(Request $request) 'docker_compose_domains' => 'array|nullable', 'docker_compose_domains.*' => 'array:name,domain', 'docker_compose_domains.*.name' => 'string|required', - 'docker_compose_domains.*.domain' => 'string|nullable', + 'docker_compose_domains.*.domain' => ValidationPatterns::applicationDomainRules(), 'custom_nginx_configuration' => 'string|nullable', 'is_http_basic_auth_enabled' => 'boolean|nullable', 'http_basic_auth_username' => 'string', @@ -2482,29 +2486,7 @@ public function update_by_uuid(Request $request) $requestHasDomains = $request->has('domains'); if ($requestHasDomains && $server->isProxyShouldRun()) { $uuid = $request->uuid; - $urls = $request->domains; - $urls = str($urls)->replaceStart(',', '')->replaceEnd(',', '')->trim(); - $errors = []; - $urls = str($urls)->trim()->explode(',')->map(function ($url) use (&$errors) { - $url = trim($url); - - // If "domains" is empty clear all URLs from the fqdn column - if (blank($url)) { - return null; - } - - if (! filter_var($url, FILTER_VALIDATE_URL)) { - $errors[] = 'Invalid URL: '.$url; - - return $url; - } - $scheme = parse_url($url, PHP_URL_SCHEME) ?? ''; - if (! in_array(strtolower($scheme), ['http', 'https'])) { - $errors[] = "Invalid URL scheme: {$scheme} for URL: {$url}. Only http and https are supported."; - } - - return str($url)->lower(); - }); + $errors = ValidationPatterns::validateApplicationDomains($request->domains); if (count($errors) > 0) { return response()->json([ @@ -2512,6 +2494,9 @@ public function update_by_uuid(Request $request) 'errors' => $errors, ], 422); } + $domains = ValidationPatterns::normalizeApplicationDomains($request->domains); + $request->offsetSet('domains', $domains); + $urls = collect(ValidationPatterns::applicationDomainList($domains)); // Check for domain conflicts $result = checkIfDomainIsAlreadyUsedViaAPI($urls, $teamId, $uuid); if (isset($result['error'])) { @@ -3871,36 +3856,16 @@ private function validateDataApplications(Request $request, Server $server) } if ($request->has('domains') && $server->isProxyShouldRun()) { $uuid = $request->uuid; - $urls = $request->domains; - $urls = str($urls)->replaceEnd(',', '')->trim(); - $urls = str($urls)->replaceStart(',', '')->trim(); - $errors = []; - $urls = str($urls)->trim()->explode(',')->map(function ($url) use (&$errors) { - $url = trim($url); - - // If "domains" is empty clear all URLs from the fqdn column - if (blank($url)) { - return null; - } - - if (! filter_var($url, FILTER_VALIDATE_URL)) { - $errors[] = 'Invalid URL: '.$url; - - return str($url)->lower(); - } - $scheme = parse_url($url, PHP_URL_SCHEME) ?? ''; - if (! in_array(strtolower($scheme), ['http', 'https'])) { - $errors[] = "Invalid URL scheme: {$scheme} for URL: {$url}. Only http and https are supported."; - } - - return str($url)->lower(); - }); + $errors = ValidationPatterns::validateApplicationDomains($request->domains); if (count($errors) > 0) { return response()->json([ 'message' => 'Validation failed.', 'errors' => $errors, ], 422); } + $normalizedDomains = ValidationPatterns::normalizeApplicationDomains($request->domains); + $request->offsetSet('domains', $normalizedDomains); + $urls = collect(ValidationPatterns::applicationDomainList($normalizedDomains)); // Check for domain conflicts $result = checkIfDomainIsAlreadyUsedViaAPI($urls, $teamId, $uuid); if (isset($result['error'])) { diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index 6c121bcf8..97fd41c5c 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -60,19 +60,10 @@ private function applyServiceUrls(Service $service, array $urlsArray, string $te return str($urlValue)->replaceStart(',', '')->replaceEnd(',', '')->trim()->explode(',')->map(fn ($url) => trim($url))->filter(); }); - $urls = $urls->map(function ($url) use (&$errors) { - if (! filter_var($url, FILTER_VALIDATE_URL)) { - $errors[] = "Invalid URL: {$url}"; - - return $url; - } - $scheme = parse_url($url, PHP_URL_SCHEME) ?? ''; - if (! in_array(strtolower($scheme), ['http', 'https'])) { - $errors[] = "Invalid URL scheme: {$scheme} for URL: {$url}. Only http and https are supported."; - } - - return $url; - }); + $errors = ValidationPatterns::validateApplicationDomains($urls->implode(',')); + $urls = collect(ValidationPatterns::applicationDomainList( + ValidationPatterns::normalizeApplicationDomains($urls->implode(',')) + )); $duplicates = $urls->duplicates()->unique()->values(); if ($duplicates->isNotEmpty() && ! $forceDomainOverride) { @@ -101,10 +92,10 @@ private function applyServiceUrls(Service $service, array $urlsArray, string $te } if (filled($containerUrls)) { - $containerUrls = str($containerUrls)->replaceStart(',', '')->replaceEnd(',', '')->trim(); - $containerUrls = str($containerUrls)->explode(',')->map(fn ($url) => str(trim($url))->lower()); + $containerUrls = ValidationPatterns::normalizeApplicationDomains($containerUrls); + $containerUrlCollection = collect(ValidationPatterns::applicationDomainList($containerUrls)); - $result = checkIfDomainIsAlreadyUsedViaAPI($containerUrls, $teamId, $application->uuid); + $result = checkIfDomainIsAlreadyUsedViaAPI($containerUrlCollection, $teamId, $application->uuid); if (isset($result['error'])) { $errors[] = $result['error']; @@ -116,8 +107,6 @@ private function applyServiceUrls(Service $service, array $urlsArray, string $te return; } - - $containerUrls = $containerUrls->filter(fn ($u) => filled($u))->unique()->implode(','); } else { $containerUrls = null; } diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 545735cf6..7427df54e 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -2229,7 +2229,7 @@ private function set_coolify_variables() // Only include SOURCE_COMMIT in build context if enabled in settings if ($this->application->settings->include_source_commit_in_build) { - $this->coolify_variables .= "SOURCE_COMMIT={$this->commit} "; + $this->coolify_variables .= 'SOURCE_COMMIT='.escapeShellValue($this->commit).' '; } if ($this->pull_request_id === 0) { $fqdn = $this->application->fqdn; @@ -2241,17 +2241,33 @@ private function set_coolify_variables() $fqdn = $url->getHost(); $url = $url->withHost($fqdn)->withPort(null)->__toString(); if ((int) $this->application->compose_parsing_version >= 3) { - $this->coolify_variables .= "COOLIFY_URL={$url} "; - $this->coolify_variables .= "COOLIFY_FQDN={$fqdn} "; + $this->coolify_variables .= 'COOLIFY_URL='.escapeShellValue($url).' '; + $this->coolify_variables .= 'COOLIFY_FQDN='.escapeShellValue($fqdn).' '; } else { - $this->coolify_variables .= "COOLIFY_URL={$fqdn} "; - $this->coolify_variables .= "COOLIFY_FQDN={$url} "; + $this->coolify_variables .= 'COOLIFY_URL='.escapeShellValue($fqdn).' '; + $this->coolify_variables .= 'COOLIFY_FQDN='.escapeShellValue($url).' '; } } if (isset($this->application->git_branch)) { $this->coolify_variables .= 'COOLIFY_BRANCH='.escapeShellValue($this->application->git_branch).' '; } - $this->coolify_variables .= "COOLIFY_RESOURCE_UUID={$this->application->uuid} "; + $this->coolify_variables .= 'COOLIFY_RESOURCE_UUID='.escapeShellValue($this->application->uuid).' '; + } + + private function shellAssignmentForDockerfileArg(string $assignment): string + { + [$key, $value] = array_pad(explode('=', $assignment, 2), 2, null); + + if ($value === null) { + return $assignment; + } + + if (str_starts_with($value, "'") && str_ends_with($value, "'")) { + $value = substr($value, 1, -1); + $value = str_replace("'\\''", "'", $value); + } + + return "{$key}={$value}"; } private function gitLsRemoteCommand(string $lsRemoteRef, ?string $identityFile = null): string @@ -4220,7 +4236,7 @@ private function add_build_env_variables_to_dockerfile() $coolify_vars = collect(explode(' ', trim($this->coolify_variables))) ->filter() ->map(function ($var) { - return "ARG {$var}"; + return 'ARG '.$this->shellAssignmentForDockerfileArg($var); }); $argsToInsert = $argsToInsert->merge($coolify_vars); } @@ -4242,7 +4258,7 @@ private function add_build_env_variables_to_dockerfile() $coolify_vars = collect(explode(' ', trim($this->coolify_variables))) ->filter() ->map(function ($var) { - return "ARG {$var}"; + return 'ARG '.$this->shellAssignmentForDockerfileArg($var); }); $argsToInsert = $argsToInsert->merge($coolify_vars); } diff --git a/app/Support/ValidationPatterns.php b/app/Support/ValidationPatterns.php index d781c7416..f88f78c34 100644 --- a/app/Support/ValidationPatterns.php +++ b/app/Support/ValidationPatterns.php @@ -108,6 +108,12 @@ class ValidationPatterns */ public const ENVIRONMENT_VARIABLE_KEY_PATTERN = '/\A[A-Za-z_][A-Za-z0-9_.]*\z/u'; + /** + * Characters that are valid in some URL positions but unsafe for values + * that are later reused in shell assignment contexts. + */ + public const APPLICATION_DOMAIN_FORBIDDEN_PATTERN = '/[`$;&|<>()\\\\\r\n]/'; + /** * Pattern for SQL-safe unquoted database identifiers (usernames, database names). * Allows letters, digits, underscore; first char must be letter or underscore. @@ -511,6 +517,156 @@ public static function shellSafeCommandRules(int $maxLength = 1000): array return ['nullable', 'string', 'max:'.$maxLength, 'regex:'.self::SHELL_SAFE_COMMAND_PATTERN]; } + /** + * Get validation rules for comma-separated application URL fields. + */ + public static function applicationDomainRules(int $maxLength = 2048): array + { + return [ + 'nullable', + 'string', + 'max:'.$maxLength, + function (string $attribute, mixed $value, \Closure $fail): void { + foreach (self::validateApplicationDomains($value) as $error) { + $fail($error); + } + }, + ]; + } + + /** + * Validate a comma-separated list of application URLs. + * + * @return array + */ + public static function validateApplicationDomains(mixed $value): array + { + if (blank($value)) { + return []; + } + + if (! is_string($value)) { + return ['The domains field must be a string.']; + } + + $errors = []; + foreach (self::applicationDomainList($value) as $url) { + if (preg_match(self::APPLICATION_DOMAIN_FORBIDDEN_PATTERN, $url) === 1) { + $errors[] = "Invalid URL: {$url}"; + + continue; + } + + if (! filter_var($url, FILTER_VALIDATE_URL)) { + $errors[] = "Invalid URL: {$url}"; + + continue; + } + + $scheme = parse_url($url, PHP_URL_SCHEME) ?? ''; + if (! in_array(strtolower($scheme), ['http', 'https'], true)) { + $errors[] = "Invalid URL scheme: {$scheme} for URL: {$url}. Only http and https are supported."; + + continue; + } + + if (blank(parse_url($url, PHP_URL_HOST))) { + $errors[] = "Invalid URL: {$url}"; + } + } + + return $errors; + } + + /** + * Normalize a comma-separated application URL list for storage. + */ + public static function normalizeApplicationDomains(?string $value): ?string + { + $urls = self::applicationDomainList($value); + + if ($urls === []) { + return null; + } + + return collect($urls) + ->map(fn (string $url) => self::normalizeApplicationDomainUrl($url)) + ->implode(','); + } + + /** + * Normalize URL components that are case-insensitive while preserving + * case-sensitive path, query, and fragment components. + */ + private static function normalizeApplicationDomainUrl(string $url): string + { + $components = parse_url($url); + + if ($components === false) { + return $url; + } + + $normalized = ''; + + if (isset($components['scheme'])) { + $normalized .= strtolower($components['scheme']).'://'; + } + + if (isset($components['user'])) { + $normalized .= $components['user']; + + if (isset($components['pass'])) { + $normalized .= ':'.$components['pass']; + } + + $normalized .= '@'; + } + + if (isset($components['host'])) { + $normalized .= strtolower($components['host']); + } + + if (isset($components['port'])) { + $normalized .= ':'.$components['port']; + } + + if (isset($components['path'])) { + $normalized .= $components['path']; + } + + if (array_key_exists('query', $components)) { + $normalized .= '?'.$components['query']; + } + + if (array_key_exists('fragment', $components)) { + $normalized .= '#'.$components['fragment']; + } + + return $normalized; + } + + /** + * Split a comma-separated application URL list into trimmed URL strings. + * + * @return array + */ + public static function applicationDomainList(?string $value): array + { + if (blank($value)) { + return []; + } + + return str($value) + ->replaceStart(',', '') + ->replaceEnd(',', '') + ->trim() + ->explode(',') + ->map(fn (string $url) => trim($url)) + ->filter(fn (string $url) => filled($url)) + ->values() + ->all(); + } + /** * Get validation rules for Docker volume name fields */ diff --git a/bootstrap/helpers/api.php b/bootstrap/helpers/api.php index 6a288a064..200bc6856 100644 --- a/bootstrap/helpers/api.php +++ b/bootstrap/helpers/api.php @@ -98,7 +98,7 @@ function sharedDataApplications() 'is_auto_deploy_enabled' => 'boolean', 'is_force_https_enabled' => 'boolean', 'static_image' => Rule::enum(StaticImageTypes::class), - 'domains' => 'string|nullable', + 'domains' => ValidationPatterns::applicationDomainRules(), 'redirect' => Rule::enum(RedirectTypes::class), 'git_commit_sha' => ['string', 'regex:/^[a-zA-Z0-9][a-zA-Z0-9._\-\/]*$/'], 'docker_registry_image_name' => ValidationPatterns::dockerImageNameRules(), diff --git a/tests/Feature/Security/CommandInjectionSecurityTest.php b/tests/Feature/Security/CommandInjectionSecurityTest.php index 5fa5c08d2..45101635b 100644 --- a/tests/Feature/Security/CommandInjectionSecurityTest.php +++ b/tests/Feature/Security/CommandInjectionSecurityTest.php @@ -130,6 +130,69 @@ }); describe('API validation rules for path fields', function () { + test('domains validation rejects command injection payloads', function (string $payload) { + $rules = sharedDataApplications(); + + $validator = validator( + ['domains' => $payload], + ['domains' => $rules['domains']] + ); + + expect($validator->fails())->toBeTrue(); + })->with([ + 'host command substitution' => 'http://$(whoami).example.com', + 'path command substitution' => 'http://example.com/$(whoami)', + 'query command substitution' => 'http://example.com/path?next=$(id)', + 'host backtick substitution' => 'http://`whoami`.example.com', + 'path backtick substitution' => 'http://example.com/`whoami`', + 'semicolon command separator' => 'http://example.com/path;id', + 'newline injection' => "http://example.com\nwhoami", + 'carriage return injection' => "http://example.com\rwhoami", + 'pipe injection' => 'http://example.com/path|id', + ]); + + test('domains validation rejects non http schemes', function () { + $rules = sharedDataApplications(); + + $validator = validator( + ['domains' => 'ftp://example.com'], + ['domains' => $rules['domains']] + ); + + expect($validator->fails())->toBeTrue(); + }); + + test('domains validation allows comma separated http and https urls', function () { + $rules = sharedDataApplications(); + + $validator = validator( + ['domains' => 'https://app.example.com,http://api.example.com/path'], + ['domains' => $rules['domains']] + ); + + expect($validator->fails())->toBeFalse(); + }); + + test('docker compose service domains validation rejects command injection payloads', function () { + $rules = [ + 'docker_compose_domains' => 'array|nullable', + 'docker_compose_domains.*' => 'array:name,domain', + 'docker_compose_domains.*.name' => 'string|required', + 'docker_compose_domains.*.domain' => ValidationPatterns::applicationDomainRules(), + ]; + + $validator = validator( + [ + 'docker_compose_domains' => [ + ['name' => 'app', 'domain' => 'https://app.example.com/$(whoami)'], + ], + ], + $rules + ); + + expect($validator->fails())->toBeTrue(); + }); + test('git_branch validation rejects shell metacharacters', function (string $branch) { $rules = sharedDataApplications(); @@ -276,7 +339,45 @@ expect($coolifyVariables->getValue($instance)) ->toContain("COOLIFY_BRANCH='main`id`' ") - ->toContain('COOLIFY_RESOURCE_UUID=app-uuid '); + ->toContain("COOLIFY_RESOURCE_UUID='app-uuid' "); + }); + + test('coolify url and fqdn shell assignments are quoted', function () { + $job = new ReflectionClass(ApplicationDeploymentJob::class); + $instance = $job->newInstanceWithoutConstructor(); + + $application = new Application; + $application->uuid = 'app-uuid'; + $application->git_branch = 'main'; + $application->fqdn = 'https://app.example.com/path'; + $application->compose_parsing_version = '3'; + + $settings = new ApplicationSetting; + $settings->include_source_commit_in_build = true; + $application->setRelation('settings', $settings); + + foreach ([ + 'application' => $application, + 'commit' => 'HEAD$(id)', + '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("SOURCE_COMMIT='HEAD$(id)' ") + ->toContain("COOLIFY_URL='https://app.example.com/path' ") + ->toContain("COOLIFY_FQDN='app.example.com' ") + ->toContain("COOLIFY_RESOURCE_UUID='app-uuid' "); }); });