diff --git a/app/Actions/Service/StartService.php b/app/Actions/Service/StartService.php index dfef6a566..50011c74f 100644 --- a/app/Actions/Service/StartService.php +++ b/app/Actions/Service/StartService.php @@ -22,6 +22,10 @@ public function handle(Service $service, bool $pullLatestImages = false, bool $s $service->isConfigurationChanged(save: true); $commands[] = 'cd '.$service->workdir(); $commands[] = "echo 'Saved configuration files to {$service->workdir()}.'"; + // Ensure .env file exists before docker compose tries to load it + // This is defensive programming - saveComposeConfigs() already creates it, + // but we guarantee it here in case of any edge cases or manual deployments + $commands[] = 'touch .env'; if ($pullLatestImages) { $commands[] = "echo 'Pulling images.'"; $commands[] = 'docker compose pull'; diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index ea8cdff95..0fd007e9a 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -3029,6 +3029,12 @@ private function stop_running_container(bool $force = false) private function start_by_compose_file() { + // Ensure .env file exists before docker compose tries to load it (defensive programming) + $this->execute_remote_command( + ["touch {$this->workdir}/.env", 'hidden' => true], + ["touch {$this->configuration_dir}/.env", 'hidden' => true], + ); + if ($this->application->build_pack === 'dockerimage') { $this->application_deployment_queue->addLogEntry('Pulling latest images from the registry.'); $this->execute_remote_command( diff --git a/app/Livewire/Project/Service/StackForm.php b/app/Livewire/Project/Service/StackForm.php index 85cd21a7f..8a7b6e090 100644 --- a/app/Livewire/Project/Service/StackForm.php +++ b/app/Livewire/Project/Service/StackForm.php @@ -5,6 +5,7 @@ use App\Models\Service; use App\Support\ValidationPatterns; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\DB; use Livewire\Component; class StackForm extends Component @@ -22,7 +23,7 @@ class StackForm extends Component public string $dockerComposeRaw; - public string $dockerCompose; + public ?string $dockerCompose = null; public ?bool $connectToDockerNetwork = null; @@ -30,7 +31,7 @@ protected function rules(): array { $baseRules = [ 'dockerComposeRaw' => 'required', - 'dockerCompose' => 'required', + 'dockerCompose' => 'nullable', 'name' => ValidationPatterns::nameRules(), 'description' => ValidationPatterns::descriptionRules(), 'connectToDockerNetwork' => 'nullable', @@ -140,18 +141,26 @@ public function submit($notify = true) $this->validate(); $this->syncData(true); - // Validate for command injection BEFORE saving to database + // Validate for command injection BEFORE any database operations validateDockerComposeForInjection($this->service->docker_compose_raw); - $this->service->save(); - $this->service->saveExtraFields($this->fields); - $this->service->parse(); - $this->service->refresh(); - $this->service->saveComposeConfigs(); + // Use transaction to ensure atomicity - if parse fails, save is rolled back + DB::transaction(function () { + $this->service->save(); + $this->service->saveExtraFields($this->fields); + $this->service->parse(); + $this->service->refresh(); + $this->service->saveComposeConfigs(); + }); + $this->dispatch('refreshEnvs'); $this->dispatch('refreshServices'); $notify && $this->dispatch('success', 'Service saved.'); } catch (\Throwable $e) { + // On error, refresh from database to restore clean state + $this->service->refresh(); + $this->syncData(false); + return handleError($e, $this); } finally { if (is_null($this->service->config_hash)) { diff --git a/app/Models/Service.php b/app/Models/Service.php index 12d3d6a11..ef755d105 100644 --- a/app/Models/Service.php +++ b/app/Models/Service.php @@ -1287,6 +1287,11 @@ public function workdir() public function saveComposeConfigs() { + // Guard against null or empty docker_compose + if (! $this->docker_compose) { + return; + } + $workdir = $this->workdir(); instant_remote_process([ diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 1deec45d7..9b17e6810 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -59,11 +59,13 @@ function validateDockerComposeForInjection(string $composeYaml): void if (isset($volume['source'])) { $source = $volume['source']; if (is_string($source)) { - // Allow simple env vars and env vars with defaults (validated in parseDockerVolumeString) + // Allow env vars and env vars with defaults (validated in parseDockerVolumeString) + // Also allow env vars followed by safe path concatenation (e.g., ${VAR}/path) $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source); $isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $source); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $source); - if (! $isSimpleEnvVar && ! $isEnvVarWithDefault) { + if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) { try { validateShellSafePath($source, 'volume source'); } catch (\Exception $e) { @@ -310,15 +312,17 @@ function parseDockerVolumeString(string $volumeString): array // Validate source path for command injection attempts // We validate the final source value after environment variable processing if ($source !== null) { - // Allow simple environment variables like ${VAR_NAME} or ${VAR} - // but validate everything else for shell metacharacters + // Allow environment variables like ${VAR_NAME} or ${VAR} + // Also allow env vars followed by safe path concatenation (e.g., ${VAR}/path) $sourceStr = is_string($source) ? $source : $source; // Skip validation for simple environment variable references - // Pattern: ${WORD_CHARS} with no special characters inside + // Pattern 1: ${WORD_CHARS} with no special characters inside + // Pattern 2: ${WORD_CHARS}/path/to/file (env var with path concatenation) $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceStr); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceStr); - if (! $isSimpleEnvVar) { + if (! $isSimpleEnvVar && ! $isEnvVarWithPath) { try { validateShellSafePath($sourceStr, 'volume source'); } catch (\Exception $e) { @@ -711,9 +715,12 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int // Validate source and target for command injection (array/long syntax) if ($source !== null && ! empty($source->value())) { $sourceValue = $source->value(); - // Allow simple environment variable references + // Allow environment variable references and env vars with path concatenation $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); - if (! $isSimpleEnvVar) { + $isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $sourceValue); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue); + + if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) { try { validateShellSafePath($sourceValue, 'volume source'); } catch (\Exception $e) { @@ -1293,6 +1300,9 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int if ($depends_on->count() > 0) { $payload['depends_on'] = $depends_on; } + // Auto-inject .env file so Coolify environment variables are available inside containers + // This makes Applications behave consistently with manual .env file usage + $payload['env_file'] = ['.env']; if ($isPullRequest) { $serviceName = addPreviewDeploymentSuffix($serviceName, $pullRequestId); } @@ -1812,9 +1822,12 @@ function serviceParser(Service $resource): Collection // Validate source and target for command injection (array/long syntax) if ($source !== null && ! empty($source->value())) { $sourceValue = $source->value(); - // Allow simple environment variable references + // Allow environment variable references and env vars with path concatenation $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); - if (! $isSimpleEnvVar) { + $isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $sourceValue); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue); + + if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) { try { validateShellSafePath($sourceValue, 'volume source'); } catch (\Exception $e) { @@ -2269,6 +2282,9 @@ function serviceParser(Service $resource): Collection if ($depends_on->count() > 0) { $payload['depends_on'] = $depends_on; } + // Auto-inject .env file so Coolify environment variables are available inside containers + // This makes Services behave consistently with Applications + $payload['env_file'] = ['.env']; $parsedServices->put($serviceName, $payload); } diff --git a/tests/Unit/VolumeArrayFormatSecurityTest.php b/tests/Unit/VolumeArrayFormatSecurityTest.php index 97a6819b2..08174fff3 100644 --- a/tests/Unit/VolumeArrayFormatSecurityTest.php +++ b/tests/Unit/VolumeArrayFormatSecurityTest.php @@ -194,6 +194,36 @@ ->not->toThrow(Exception::class); }); +test('array-format with environment variable and path concatenation', function () { + // This is the reported issue #7127 - ${VAR}/path should be allowed + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - type: bind + source: '${VOLUMES_PATH}/mysql' + target: /var/lib/mysql + - type: bind + source: '${DATA_PATH}/config' + target: /etc/config + - type: bind + source: '${VOLUME_PATH}/app_data' + target: /app/data +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + + // Verify all three volumes have the correct source format + expect($parsed['services']['web']['volumes'][0]['source'])->toBe('${VOLUMES_PATH}/mysql'); + expect($parsed['services']['web']['volumes'][1]['source'])->toBe('${DATA_PATH}/config'); + expect($parsed['services']['web']['volumes'][2]['source'])->toBe('${VOLUME_PATH}/app_data'); + + // The validation should allow this - the reported bug was that it was blocked + expect(fn () => validateDockerComposeForInjection($dockerComposeYaml)) + ->not->toThrow(Exception::class); +}); + test('array-format with malicious environment variable default', function () { $dockerComposeYaml = <<<'YAML' services: diff --git a/tests/Unit/VolumeSecurityTest.php b/tests/Unit/VolumeSecurityTest.php index d7f20fc0e..f4cd6c268 100644 --- a/tests/Unit/VolumeSecurityTest.php +++ b/tests/Unit/VolumeSecurityTest.php @@ -94,6 +94,27 @@ } }); +test('parseDockerVolumeString accepts environment variables with path concatenation', function () { + $volumes = [ + '${VOLUMES_PATH}/mysql:/var/lib/mysql', + '${DATA_PATH}/config:/etc/config', + '${VOLUME_PATH}/app_data:/app', + '${MY_VAR_123}/deep/nested/path:/data', + '${VAR}/path:/app', + '${VAR}_suffix:/app', + '${VAR}-suffix:/app', + '${VAR}.ext:/app', + '${VOLUMES_PATH}/mysql:/var/lib/mysql:ro', + '${DATA_PATH}/config:/etc/config:rw', + ]; + + foreach ($volumes as $volume) { + $result = parseDockerVolumeString($volume); + expect($result)->toBeArray(); + expect($result['source'])->not->toBeNull(); + } +}); + test('parseDockerVolumeString rejects environment variables with command injection in default', function () { $maliciousVolumes = [ '${VAR:-`whoami`}:/app',