From 2cbc22a1c971ed73e315473ec4077981f3ba7a75 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 1 Apr 2026 09:55:53 +0200 Subject: [PATCH 1/5] fix(volumes): resolve env-var file volume paths and refresh on env edits Normalize `${VAR}` and `${VAR:-default}` volume sources via `resolveEnvVarDefault()`, use preview env vars during application parsing, and re-resolve persisted file-volume paths after environment-variable updates. Also surface env-var source info in the file storage UI and add unit coverage for env-var/default resolution and path safety. --- app/Livewire/Project/Service/FileStorage.php | 58 ++++++ .../Shared/EnvironmentVariable/All.php | 3 +- .../Shared/EnvironmentVariable/Show.php | 32 ++-- app/Models/LocalFileVolume.php | 168 +++++++++++++++++- bootstrap/helpers/parsers.php | 36 ++-- bootstrap/helpers/shared.php | 35 ++++ .../project/service/file-storage.blade.php | 3 +- tests/Unit/VolumeArrayFormatSecurityTest.php | 137 ++++++++++++++ 8 files changed, 442 insertions(+), 30 deletions(-) diff --git a/app/Livewire/Project/Service/FileStorage.php b/app/Livewire/Project/Service/FileStorage.php index 844e378546..dcaa02c0da 100644 --- a/app/Livewire/Project/Service/FileStorage.php +++ b/app/Livewire/Project/Service/FileStorage.php @@ -17,6 +17,7 @@ use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Livewire\Attributes\Validate; use Livewire\Component; +use Symfony\Component\Yaml\Yaml; class FileStorage extends Component { @@ -30,6 +31,8 @@ class FileStorage extends Component public ?string $workdir = null; + public ?string $resolvedFromEnvVar = null; + public bool $permanently_delete = true; public bool $isReadOnly = false; @@ -64,6 +67,7 @@ public function mount() } $this->isReadOnly = $this->fileStorage->shouldBeReadOnlyInUI(); + $this->resolvedFromEnvVar = $this->detectEnvVarSource(); $this->syncData(); } @@ -86,6 +90,60 @@ public function syncData(bool $toModel = false): void } } + /** + * Check the raw docker-compose for env var references in this volume's source. + * Returns the variable name if found, null otherwise. + */ + private function detectEnvVarSource(): ?string + { + try { + $resource = $this->resource; + $dockerComposeRaw = $resource->service?->docker_compose_raw + ?? $resource->docker_compose_raw + ?? null; + + if (! $dockerComposeRaw) { + return null; + } + + $compose = Yaml::parse($dockerComposeRaw); + + if (! isset($compose['services'])) { + return null; + } + + $mountPath = $this->fileStorage->mount_path; + + // Search all services in the compose for a volume matching this mount path + foreach ($compose['services'] as $serviceVolumes) { + $volumes = data_get($serviceVolumes, 'volumes', []); + foreach ($volumes as $volume) { + $source = null; + $target = null; + + if (is_string($volume)) { + $parts = explode(':', $volume); + if (count($parts) >= 2) { + $source = $parts[0]; + $target = $parts[1]; + } + } elseif (is_array($volume)) { + $source = data_get($volume, 'source'); + $target = data_get($volume, 'target'); + } + + if ($target === $mountPath && $source && preg_match('/\$\{([a-zA-Z_][a-zA-Z0-9_]*)/', $source, $matches)) { + return $matches[1]; + } + } + } + } catch (\Throwable $e) { + // Silently fail — this is just a UI hint + } + + return null; + } + public function convertToDirectory() { try { diff --git a/app/Livewire/Project/Shared/EnvironmentVariable/All.php b/app/Livewire/Project/Shared/EnvironmentVariable/All.php index f250a860b8..76d02dfa5f 100644 --- a/app/Livewire/Project/Shared/EnvironmentVariable/All.php +++ b/app/Livewire/Project/Shared/EnvironmentVariable/All.php @@ -2,6 +2,7 @@ namespace App\Livewire\Project\Shared\EnvironmentVariable; +use App\Models\Application; use App\Models\EnvironmentVariable; use App\Traits\EnvironmentVariableProtection; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; @@ -38,7 +39,7 @@ public function mount() $this->is_env_sorting_enabled = data_get($this->resource, 'settings.is_env_sorting_enabled', false); $this->use_build_secrets = data_get($this->resource, 'settings.use_build_secrets', false); $this->resourceClass = get_class($this->resource); - $resourceWithPreviews = [\App\Models\Application::class]; + $resourceWithPreviews = [Application::class]; $simpleDockerfile = filled(data_get($this->resource, 'dockerfile')); if (str($this->resourceClass)->contains($resourceWithPreviews) && ! $simpleDockerfile) { $this->showPreview = true; diff --git a/app/Livewire/Project/Shared/EnvironmentVariable/Show.php b/app/Livewire/Project/Shared/EnvironmentVariable/Show.php index 4e8521f27d..4c67d5466b 100644 --- a/app/Livewire/Project/Shared/EnvironmentVariable/Show.php +++ b/app/Livewire/Project/Shared/EnvironmentVariable/Show.php @@ -2,12 +2,17 @@ namespace App\Livewire\Project\Shared\EnvironmentVariable; +use App\Models\Application; use App\Models\Environment; use App\Models\EnvironmentVariable as ModelsEnvironmentVariable; +use App\Models\LocalFileVolume; use App\Models\Project; +use App\Models\Server; +use App\Models\Service; use App\Models\SharedEnvironmentVariable; use App\Traits\EnvironmentVariableAnalyzer; use App\Traits\EnvironmentVariableProtection; +use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Livewire\Attributes\Computed; use Livewire\Component; @@ -80,7 +85,7 @@ class Show extends Component public function mount() { $this->syncData(); - if ($this->env->getMorphClass() === \App\Models\SharedEnvironmentVariable::class) { + if ($this->env->getMorphClass() === SharedEnvironmentVariable::class) { $this->isSharedVariable = true; } $this->parameters = get_route_parameters(); @@ -203,6 +208,13 @@ public function submit() $this->serialize(); $this->syncData(true); $this->syncData(false); + + // Re-resolve volume paths that reference env vars (e.g., ${VAR:-./path}) + $resourceable = $this->env->resourceable ?? null; + if ($resourceable) { + LocalFileVolume::reResolveVolumePaths($resourceable); + } + $this->dispatch('success', 'Environment variable updated.'); $this->dispatch('envsUpdated'); $this->dispatch('configurationChanged'); @@ -233,7 +245,7 @@ public function availableSharedVariables(): array $result['team'] = $team->environment_variables() ->pluck('key') ->toArray(); - } catch (\Illuminate\Auth\Access\AuthorizationException $e) { + } catch (AuthorizationException $e) { // User not authorized to view team variables } @@ -264,12 +276,12 @@ public function availableSharedVariables(): array $result['environment'] = $environment->environment_variables() ->pluck('key') ->toArray(); - } catch (\Illuminate\Auth\Access\AuthorizationException $e) { + } catch (AuthorizationException $e) { // User not authorized to view environment variables } } } - } catch (\Illuminate\Auth\Access\AuthorizationException $e) { + } catch (AuthorizationException $e) { // User not authorized to view project variables } } @@ -279,7 +291,7 @@ public function availableSharedVariables(): array $serverUuid = data_get($this->parameters, 'server_uuid'); if ($serverUuid) { // If we have a specific server_uuid, show variables for that server - $server = \App\Models\Server::where('team_id', $team->id) + $server = Server::where('team_id', $team->id) ->where('uuid', $serverUuid) ->first(); @@ -289,7 +301,7 @@ public function availableSharedVariables(): array $result['server'] = $server->environment_variables() ->pluck('key') ->toArray(); - } catch (\Illuminate\Auth\Access\AuthorizationException $e) { + } catch (AuthorizationException $e) { // User not authorized to view server variables } } @@ -297,7 +309,7 @@ public function availableSharedVariables(): array // For application environment variables, try to use the application's destination server $applicationUuid = data_get($this->parameters, 'application_uuid'); if ($applicationUuid) { - $application = \App\Models\Application::whereRelation('environment.project.team', 'id', $team->id) + $application = Application::whereRelation('environment.project.team', 'id', $team->id) ->where('uuid', $applicationUuid) ->with('destination.server') ->first(); @@ -308,7 +320,7 @@ public function availableSharedVariables(): array $result['server'] = $application->destination->server->environment_variables() ->pluck('key') ->toArray(); - } catch (\Illuminate\Auth\Access\AuthorizationException $e) { + } catch (AuthorizationException $e) { // User not authorized to view server variables } } @@ -316,7 +328,7 @@ public function availableSharedVariables(): array // For service environment variables, try to use the service's server $serviceUuid = data_get($this->parameters, 'service_uuid'); if ($serviceUuid) { - $service = \App\Models\Service::whereRelation('environment.project.team', 'id', $team->id) + $service = Service::whereRelation('environment.project.team', 'id', $team->id) ->where('uuid', $serviceUuid) ->with('server') ->first(); @@ -327,7 +339,7 @@ public function availableSharedVariables(): array $result['server'] = $service->server->environment_variables() ->pluck('key') ->toArray(); - } catch (\Illuminate\Auth\Access\AuthorizationException $e) { + } catch (AuthorizationException $e) { // User not authorized to view server variables } } diff --git a/app/Models/LocalFileVolume.php b/app/Models/LocalFileVolume.php index 4b5c602c27..2665c90b76 100644 --- a/app/Models/LocalFileVolume.php +++ b/app/Models/LocalFileVolume.php @@ -6,6 +6,7 @@ use App\Jobs\ServerStorageSaveJob; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Support\Stringable; use Symfony\Component\Yaml\Yaml; class LocalFileVolume extends BaseModel @@ -43,6 +44,18 @@ protected static function booted() }); } + /** + * Resolve env var default syntax from fs_path (e.g., ${VAR:-./path} -> ./path). + * Normally fs_path is stored already resolved by the parser, but legacy records + * may still contain raw Docker Compose variable syntax. + */ + public function resolvedFsPath(): string + { + $resolved = resolveEnvVarDefault(str($this->fs_path)); + + return $resolved->value(); + } + protected function isBinary(): Attribute { return Attribute::make( @@ -57,6 +70,144 @@ public function service() return $this->morphTo('resource'); } + /** + * Re-resolve fs_path for file volumes whose raw compose source references + * environment variables. Call this after env vars are updated. + * + * Works for Application and Service resources. + */ + public static function reResolveVolumePaths($resource): void + { + try { + if ($resource instanceof Application) { + static::reResolveForApplication($resource); + } elseif ($resource instanceof Service) { + static::reResolveForService($resource); + } + } catch (\Throwable) { + // Best-effort — don't break env var saves + } + } + + private static function reResolveForApplication(Application $application): void + { + $dockerComposeRaw = $application->docker_compose_raw; + if (! $dockerComposeRaw) { + return; + } + + $compose = Yaml::parse($dockerComposeRaw); + if (! isset($compose['services'])) { + return; + } + + $envVars = $application->environment_variables()->get(['key', 'value']) + ->mapWithKeys(fn ($item) => [$item['key'] => $item['value']]); + + $mainDirectory = str(base_configuration_dir().'/applications/'.$application->uuid); + + // Build raw source map from ALL compose services (Application owns all volumes) + $rawSources = []; + foreach ($compose['services'] as $service) { + $volumes = data_get($service, 'volumes', []); + foreach ($volumes as $volume) { + $source = null; + $target = null; + if (is_string($volume)) { + $parsed = parseDockerVolumeString($volume); + $source = $parsed['source']; + $target = $parsed['target']; + } elseif (is_array($volume)) { + $source = data_get_str($volume, 'source'); + $target = data_get_str($volume, 'target'); + } + if ($source && $target) { + $targetValue = $target instanceof Stringable ? $target->value() : (string) $target; + $rawSources[$targetValue] = $source; + } + } + } + + static::updateFileVolumePaths($application, $rawSources, $envVars, $mainDirectory); + } + + private static function reResolveForService(Service $service): void + { + $dockerComposeRaw = $service->docker_compose_raw; + if (! $dockerComposeRaw) { + return; + } + + $compose = Yaml::parse($dockerComposeRaw); + if (! isset($compose['services'])) { + return; + } + + $serviceEnvVars = $service->environment_variables()->get(['key', 'value']) + ->mapWithKeys(fn ($item) => [$item['key'] => $item['value']]); + + $mainDirectory = str(base_configuration_dir().'/applications/'.$service->uuid); + + $volumeOwners = collect() + ->merge($service->applications ?? collect()) + ->merge($service->databases ?? collect()); + + foreach ($volumeOwners as $owner) { + $childEnvVars = $owner->environment_variables()->get(['key', 'value']) + ->mapWithKeys(fn ($item) => [$item['key'] => $item['value']]); + $mergedEnvVars = $serviceEnvVars->merge($childEnvVars); + + $serviceName = $owner->name ?? null; + if (! $serviceName) { + continue; + } + + $rawSources = []; + $volumes = data_get($compose, "services.{$serviceName}.volumes", []); + foreach ($volumes as $volume) { + $source = null; + $target = null; + if (is_string($volume)) { + $parsed = parseDockerVolumeString($volume); + $source = $parsed['source']; + $target = $parsed['target']; + } elseif (is_array($volume)) { + $source = data_get_str($volume, 'source'); + $target = data_get_str($volume, 'target'); + } + if ($source && $target) { + $targetValue = $target instanceof Stringable ? $target->value() : (string) $target; + $rawSources[$targetValue] = $source; + } + } + + static::updateFileVolumePaths($owner, $rawSources, $mergedEnvVars, $mainDirectory); + } + } + + private static function updateFileVolumePaths($resource, array $rawSources, $envVars, Stringable $mainDirectory): void + { + foreach ($resource->fileStorages()->get() as $fileVolume) { + $rawSource = $rawSources[$fileVolume->mount_path] ?? null; + if (! $rawSource) { + continue; + } + + $sourceStr = $rawSource instanceof Stringable ? $rawSource : str($rawSource); + if (! str_contains($sourceStr->value(), '${')) { + continue; + } + + $resolved = resolveEnvVarDefault($sourceStr, $envVars); + if (sourceIsLocal($resolved)) { + $resolved = replaceLocalSource($resolved, $mainDirectory); + if ($fileVolume->fs_path !== $resolved->value()) { + $fileVolume->update(['fs_path' => $resolved->value()]); + } + } + } + } + public function loadStorageOnServer() { $this->load(['service']); @@ -69,7 +220,7 @@ public function loadStorageOnServer() $server = $this->resource->destination->server; } $commands = collect([]); - $path = data_get_str($this, 'fs_path'); + $path = str($this->resolvedFsPath()); if ($path->startsWith('.')) { $path = $path->after('.'); $path = $workdir.$path; @@ -104,7 +255,7 @@ public function deleteStorageOnServer() $server = $this->resource->destination->server; } $commands = collect([]); - $path = data_get_str($this, 'fs_path'); + $path = str($this->resolvedFsPath()); if ($path->startsWith('.')) { $path = $path->after('.'); $path = $workdir.$path; @@ -142,9 +293,12 @@ public function saveStorageOnServer() } $commands = collect([]); + // Resolve env var syntax (e.g., ${VAR:-./path} -> ./path) for legacy records + $fsPath = $this->resolvedFsPath(); + // Validate fs_path early before any shell interpolation - validateShellSafePath($this->fs_path, 'storage path'); - $escapedFsPath = escapeshellarg($this->fs_path); + validateShellSafePath($fsPath, 'storage path'); + $escapedFsPath = escapeshellarg($fsPath); $escapedWorkdir = escapeshellarg($workdir); if ($this->is_directory) { @@ -152,14 +306,14 @@ public function saveStorageOnServer() $commands->push("mkdir -p {$escapedWorkdir} > /dev/null 2>&1 || true"); $commands->push("cd {$escapedWorkdir}"); } - if (str($this->fs_path)->startsWith('.') || str($this->fs_path)->startsWith('/') || str($this->fs_path)->startsWith('~')) { - $parent_dir = str($this->fs_path)->beforeLast('/'); + if (str($fsPath)->startsWith('.') || str($fsPath)->startsWith('/') || str($fsPath)->startsWith('~')) { + $parent_dir = str($fsPath)->beforeLast('/'); if ($parent_dir != '') { $escapedParentDir = escapeshellarg($parent_dir); $commands->push("mkdir -p {$escapedParentDir} > /dev/null 2>&1 || true"); } } - $path = data_get_str($this, 'fs_path'); + $path = str($fsPath); $content = data_get($this, 'content'); if ($path->startsWith('.')) { $path = $path->after('.'); diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 123cf906a7..ec328a3e42 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -289,24 +289,20 @@ function parseDockerVolumeString(string $volumeString): array // Handle environment variable expansion in source // Example: ${VOLUME_DB_PATH:-db} should extract default value if present + // Only extract the default value here — full env var resolution happens in the parsers if ($source && preg_match('/^\$\{([^}]+)\}$/', $source, $matches)) { $varContent = $matches[1]; - - // Check if there's a default value with :- if (strpos($varContent, ':-') !== false) { $parts = explode(':-', $varContent, 2); $varName = $parts[0]; - $defaultValue = isset($parts[1]) ? $parts[1] : ''; - - // If there's a non-empty default value, use it for source + $defaultValue = $parts[1] ?? ''; if ($defaultValue !== '') { $source = $defaultValue; } else { - // Empty default value, keep the variable reference for env resolution + // Empty default — clean up to simple variable reference $source = '${'.$varName.'}'; } } - // Otherwise keep the variable as-is for later expansion (no default value) } // Validate source path for command injection attempts @@ -435,6 +431,12 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $allEnvironments = $allEnvironments->mapWithKeys(function ($item) { return [$item['key'] => $item['value']]; }); + // For preview deployments, merge preview env vars (they override normal ones) + if ($isPullRequest) { + $previewEnvironments = $resource->environment_variables_preview()->get(['key', 'value']) + ->mapWithKeys(fn ($item) => [$item['key'] => $item['value']]); + $allEnvironments = $allEnvironments->merge($previewEnvironments); + } // filter and add magic environments foreach ($environment as $key => $value) { // Get all SERVICE_ variables from keys and values @@ -703,6 +705,8 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $parsed = parseDockerVolumeString($volume); $source = $parsed['source']; $target = $parsed['target']; + // Resolve env vars in source (e.g., ${VAR} -> value from Coolify env vars) + $source = resolveEnvVarDefault($source, $allEnvironments); // Mode is available in $parsed['mode'] if needed $foundConfig = $fileStorages->whereMountPath($target)->first(); if (sourceIsLocal($source)) { @@ -727,15 +731,19 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $content = data_get($volume, 'content'); $isDirectory = (bool) data_get($volume, 'isDirectory', null) || (bool) data_get($volume, 'is_directory', null); + // Resolve environment variable defaults (e.g., ${VAR:-./path} -> ./path) + if ($source !== null && ! empty($source->value())) { + $source = resolveEnvVarDefault($source, $allEnvironments); + } + // Validate source and target for command injection (array/long syntax) if ($source !== null && ! empty($source->value())) { $sourceValue = $source->value(); // Allow environment variable references and env vars with path concatenation $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); - $isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $sourceValue); $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue); - if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) { + if (! $isSimpleEnvVar && ! $isEnvVarWithPath) { try { validateShellSafePath($sourceValue, 'volume source'); } catch (Exception $e) { @@ -2090,6 +2098,8 @@ function serviceParser(Service $resource): Collection $parsed = parseDockerVolumeString($volume); $source = $parsed['source']; $target = $parsed['target']; + // Resolve env vars in source (e.g., ${VAR} -> value from Coolify env vars) + $source = resolveEnvVarDefault($source, $allEnvironments); // Mode is available in $parsed['mode'] if needed $foundConfig = $fileStorages->whereMountPath($target)->first(); if (sourceIsLocal($source)) { @@ -2114,15 +2124,19 @@ function serviceParser(Service $resource): Collection $content = data_get($volume, 'content'); $isDirectory = (bool) data_get($volume, 'isDirectory', null) || (bool) data_get($volume, 'is_directory', null); + // Resolve environment variable defaults (e.g., ${VAR:-./path} -> ./path) + if ($source !== null && ! empty($source->value())) { + $source = resolveEnvVarDefault($source, $allEnvironments); + } + // Validate source and target for command injection (array/long syntax) if ($source !== null && ! empty($source->value())) { $sourceValue = $source->value(); // Allow environment variable references and env vars with path concatenation $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); - $isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $sourceValue); $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue); - if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) { + if (! $isSimpleEnvVar && ! $isEnvVarWithPath) { try { validateShellSafePath($sourceValue, 'volume source'); } catch (Exception $e) { diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index cd773f6a9f..6f896e11c8 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -1234,6 +1234,41 @@ function getTopLevelNetworks(Service|Application $resource) return $topLevelNetworks->keys(); } } +function resolveEnvVarDefault(Stringable $source, ?Collection $environmentVariables = null): Stringable +{ + $value = $source->value(); + if (preg_match('/^\$\{([^}]+)\}$/', $value, $matches)) { + $varContent = $matches[1]; + + // Extract variable name and optional default from ${VAR:-default} or ${VAR} + $varName = $varContent; + $defaultValue = null; + if (strpos($varContent, ':-') !== false) { + $parts = explode(':-', $varContent, 2); + $varName = $parts[0]; + $defaultValue = $parts[1] ?? ''; + } + + // 1. If env var is set in Coolify, use its value + if ($environmentVariables !== null && $environmentVariables->has($varName)) { + $envValue = $environmentVariables->get($varName); + if ($envValue !== null && $envValue !== '') { + return str($envValue); + } + } + + // 2. If a non-empty default exists, use it + if ($defaultValue !== null && $defaultValue !== '') { + return str($defaultValue); + } + + // 3. No env var and no default — fall back to current directory (coolify data dir) + return str('.'); + } + + return $source; +} + function sourceIsLocal(Stringable $source) { if ($source->startsWith('./') || $source->startsWith('/') || $source->startsWith('~') || $source->startsWith('..') || $source->startsWith('~/') || $source->startsWith('../')) { diff --git a/resources/views/livewire/project/service/file-storage.blade.php b/resources/views/livewire/project/service/file-storage.blade.php index 4bd88d761e..4c34ae849c 100644 --- a/resources/views/livewire/project/service/file-storage.blade.php +++ b/resources/views/livewire/project/service/file-storage.blade.php @@ -11,7 +11,8 @@ @endif
- +
diff --git a/tests/Unit/VolumeArrayFormatSecurityTest.php b/tests/Unit/VolumeArrayFormatSecurityTest.php index 08174fff3c..f920f5bd9f 100644 --- a/tests/Unit/VolumeArrayFormatSecurityTest.php +++ b/tests/Unit/VolumeArrayFormatSecurityTest.php @@ -1,5 +1,6 @@ validateShellSafePath($target, 'volume target')) ->toThrow(Exception::class, 'backtick'); }); + +// Issue #8854: resolveEnvVarDefault tests — without env vars (backward compat) + +test('resolveEnvVarDefault resolves variable with default path', function () { + $result = resolveEnvVarDefault(str('${CONFIG_FILE:-./default-config.yaml}')); + expect($result->value())->toBe('./default-config.yaml'); +}); + +test('resolveEnvVarDefault resolves variable with absolute default path', function () { + $result = resolveEnvVarDefault(str('${DATA_PATH:-/var/lib/data}')); + expect($result->value())->toBe('/var/lib/data'); +}); + +test('resolveEnvVarDefault falls back to dot when no default and no env vars', function () { + $result = resolveEnvVarDefault(str('${DATA_PATH}')); + expect($result->value())->toBe('.'); +}); + +test('resolveEnvVarDefault falls back to dot when empty default and no env vars', function () { + $result = resolveEnvVarDefault(str('${DATA_PATH:-}')); + expect($result->value())->toBe('.'); +}); + +test('resolveEnvVarDefault keeps non-env-var string unchanged', function () { + $result = resolveEnvVarDefault(str('./data')); + expect($result->value())->toBe('./data'); +}); + +test('resolveEnvVarDefault resolved path passes validateShellSafePath', function () { + $source = str('${CONFIG_FILE:-./default-config.yaml}'); + $resolved = resolveEnvVarDefault($source); + + expect(fn () => validateShellSafePath($resolved->value(), 'storage path')) + ->not->toThrow(Exception::class); +}); + +test('resolveEnvVarDefault with malicious default still fails validation', function () { + $source = str('${VAR:-/tmp/evil`whoami`}'); + $resolved = resolveEnvVarDefault($source); + + expect(fn () => validateShellSafePath($resolved->value(), 'storage path')) + ->toThrow(Exception::class, 'backtick'); +}); + +// Issue #8854: resolveEnvVarDefault tests — with env vars + +test('resolveEnvVarDefault uses env var value when set', function () { + $envVars = collect(['CONFIG_FILE' => '/custom/config.yaml']); + $result = resolveEnvVarDefault(str('${CONFIG_FILE:-./default-config.yaml}'), $envVars); + expect($result->value())->toBe('/custom/config.yaml'); +}); + +test('resolveEnvVarDefault uses env var for simple variable reference', function () { + $envVars = collect(['DATA_PATH' => '/custom/data']); + $result = resolveEnvVarDefault(str('${DATA_PATH}'), $envVars); + expect($result->value())->toBe('/custom/data'); +}); + +test('resolveEnvVarDefault falls back to default when env var not in collection', function () { + $envVars = collect(['OTHER_VAR' => '/other/path']); + $result = resolveEnvVarDefault(str('${CONFIG_FILE:-./default-config.yaml}'), $envVars); + expect($result->value())->toBe('./default-config.yaml'); +}); + +test('resolveEnvVarDefault falls back to default when env var is empty string', function () { + $envVars = collect(['CONFIG_FILE' => '']); + $result = resolveEnvVarDefault(str('${CONFIG_FILE:-./default-config.yaml}'), $envVars); + expect($result->value())->toBe('./default-config.yaml'); +}); + +test('resolveEnvVarDefault falls back to dot when env var not set and no default', function () { + $envVars = collect(['OTHER_VAR' => '/other/path']); + $result = resolveEnvVarDefault(str('${DATA_PATH}'), $envVars); + expect($result->value())->toBe('.'); +}); + +test('resolveEnvVarDefault preview env var overrides normal env var', function () { + // Simulate merged collection where preview overrides normal + $normalEnvs = collect(['CONFIG_FILE' => '/normal/config.yaml']); + $previewEnvs = collect(['CONFIG_FILE' => '/preview/config.yaml']); + $merged = $normalEnvs->merge($previewEnvs); + + $result = resolveEnvVarDefault(str('${CONFIG_FILE:-./default.yaml}'), $merged); + expect($result->value())->toBe('/preview/config.yaml'); +}); + +test('resolveEnvVarDefault with malicious env var value still fails validation', function () { + $envVars = collect(['CONFIG_FILE' => '/tmp/evil`whoami`']); + $resolved = resolveEnvVarDefault(str('${CONFIG_FILE:-./safe.yaml}'), $envVars); + + // Env var value is used, but it contains backticks so validation catches it + expect($resolved->value())->toBe('/tmp/evil`whoami`'); + expect(fn () => validateShellSafePath($resolved->value(), 'storage path')) + ->toThrow(Exception::class, 'backtick'); +}); + +// Issue #8854: LocalFileVolume::resolvedFsPath tests + +test('LocalFileVolume resolvedFsPath resolves env var with default path', function () { + $volume = new LocalFileVolume; + $volume->fs_path = '${AO_CONFIG_FILE:-./agent-orchestrator.yaml}'; + + expect($volume->resolvedFsPath())->toBe('./agent-orchestrator.yaml'); +}); + +test('LocalFileVolume resolvedFsPath passes through normal paths unchanged', function () { + $volume = new LocalFileVolume; + $volume->fs_path = '/data/coolify/services/abc123/config.yaml'; + + expect($volume->resolvedFsPath())->toBe('/data/coolify/services/abc123/config.yaml'); +}); + +test('LocalFileVolume resolvedFsPath with relative path unchanged', function () { + $volume = new LocalFileVolume; + $volume->fs_path = './data/config.yaml'; + + expect($volume->resolvedFsPath())->toBe('./data/config.yaml'); +}); + +test('LocalFileVolume resolvedFsPath resolved value passes validateShellSafePath', function () { + $volume = new LocalFileVolume; + $volume->fs_path = '${AO_CONFIG_FILE:-./agent-orchestrator.yaml}'; + + $resolved = $volume->resolvedFsPath(); + + // This should NOT throw — the resolved path is safe + expect(fn () => validateShellSafePath($resolved, 'storage path')) + ->not->toThrow(Exception::class); +}); + +test('LocalFileVolume resolvedFsPath with simple env var falls back to dot', function () { + $volume = new LocalFileVolume; + $volume->fs_path = '${DATA_PATH}'; + + expect($volume->resolvedFsPath())->toBe('.'); +}); From 35cba46bbb58d62f80d9d061b10047f2b1195388 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 1 Apr 2026 12:44:18 +0200 Subject: [PATCH 2/5] fix(volumes): preserve env defaults and allow read-only actions Keep `${VAR:-default}` intact in volume parsing so env overrides can be resolved later, then re-resolve local file volume paths after env var updates or deletions. Also treat `.` as a local source path, update file-storage UI copy/actions for read-only mounts, and add regression/unit tests for parser, source locality, and service volume path handling. --- .../Shared/EnvironmentVariable/All.php | 4 + .../Shared/EnvironmentVariable/Show.php | 7 ++ app/Models/LocalFileVolume.php | 56 +++++++++- bootstrap/helpers/parsers.php | 33 +++--- bootstrap/helpers/shared.php | 2 +- .../project/service/file-storage.blade.php | 103 +++++++++--------- tests/Unit/ParseDockerVolumeStringTest.php | 34 +++++- tests/Unit/ServiceVolumePathTest.php | 41 +++++++ tests/Unit/SourceIsLocalTest.php | 41 +++++++ tests/Unit/VolumeArrayFormatSecurityTest.php | 5 +- 10 files changed, 246 insertions(+), 80 deletions(-) create mode 100644 tests/Unit/ServiceVolumePathTest.php create mode 100644 tests/Unit/SourceIsLocalTest.php diff --git a/app/Livewire/Project/Shared/EnvironmentVariable/All.php b/app/Livewire/Project/Shared/EnvironmentVariable/All.php index 76d02dfa5f..f889b818b7 100644 --- a/app/Livewire/Project/Shared/EnvironmentVariable/All.php +++ b/app/Livewire/Project/Shared/EnvironmentVariable/All.php @@ -4,6 +4,7 @@ use App\Models\Application; use App\Models\EnvironmentVariable; +use App\Models\LocalFileVolume; use App\Traits\EnvironmentVariableProtection; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Livewire\Component; @@ -186,6 +187,9 @@ public function submit($data = null) $this->updateOrder(); $this->getDevView(); + + // Re-resolve volume paths that reference env vars (e.g., ${VAR:-./path}) + LocalFileVolume::reResolveVolumePaths($this->resource); } catch (\Throwable $e) { return handleError($e, $this); } finally { diff --git a/app/Livewire/Project/Shared/EnvironmentVariable/Show.php b/app/Livewire/Project/Shared/EnvironmentVariable/Show.php index 4c67d5466b..6458771304 100644 --- a/app/Livewire/Project/Shared/EnvironmentVariable/Show.php +++ b/app/Livewire/Project/Shared/EnvironmentVariable/Show.php @@ -366,7 +366,14 @@ public function delete() } } + $resourceable = $this->env->resourceable ?? null; $this->env->delete(); + + // Re-resolve volume paths that reference env vars (e.g., ${VAR:-./path}) + if ($resourceable) { + LocalFileVolume::reResolveVolumePaths($resourceable); + } + $this->dispatch('environmentVariableDeleted'); $this->dispatch('success', 'Environment variable deleted successfully.'); } catch (\Exception $e) { diff --git a/app/Models/LocalFileVolume.php b/app/Models/LocalFileVolume.php index 2665c90b76..7599435381 100644 --- a/app/Models/LocalFileVolume.php +++ b/app/Models/LocalFileVolume.php @@ -6,6 +6,7 @@ use App\Jobs\ServerStorageSaveJob; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Support\Collection; use Illuminate\Support\Stringable; use Symfony\Component\Yaml\Yaml; @@ -46,14 +47,59 @@ protected static function booted() /** * Resolve env var default syntax from fs_path (e.g., ${VAR:-./path} -> ./path). - * Normally fs_path is stored already resolved by the parser, but legacy records - * may still contain raw Docker Compose variable syntax. + * Resolves against the owning resource's environment variables so that + * Coolify-defined values take precedence over defaults. + * + * @throws \RuntimeException if the resolved path is bare '.' or empty (unresolvable variable with no default) */ public function resolvedFsPath(): string { - $resolved = resolveEnvVarDefault(str($this->fs_path)); + $envVars = $this->getResourceEnvironmentVariables(); + $resolved = resolveEnvVarDefault(str($this->fs_path), $envVars); + $value = $resolved->value(); + + if ($value === '.' || $value === '') { + throw new \RuntimeException( + 'Cannot resolve storage path: environment variable has no value and no default was provided. ' + .'Original path: '.$this->fs_path + ); + } + + return $value; + } + + /** + * Gather environment variables from the owning resource (and its parent + * service when the resource is a ServiceApplication or ServiceDatabase). + */ + private function getResourceEnvironmentVariables(): Collection + { + if ($this->exists) { + $this->loadMissing(['service']); + } + $resource = $this->resource; + + if (! $resource) { + return collect(); + } + + $envVars = collect(); + + // Service children (ServiceApplication / ServiceDatabase) — merge parent service env vars + $parentService = data_get($resource, 'service'); + if ($parentService && method_exists($parentService, 'environment_variables')) { + $envVars = $parentService->environment_variables()->get(['key', 'value']) + ->mapWithKeys(fn ($item) => [$item['key'] => $item['value']]); + } + + // Resource's own env vars (overrides parent service vars) + if (method_exists($resource, 'environment_variables')) { + $resourceEnvVars = $resource->environment_variables()->get(['key', 'value']) + ->mapWithKeys(fn ($item) => [$item['key'] => $item['value']]); + $envVars = $envVars->merge($resourceEnvVars); + } - return $resolved->value(); + return $envVars; } protected function isBinary(): Attribute @@ -146,7 +192,7 @@ private static function reResolveForService(Service $service): void $serviceEnvVars = $service->environment_variables()->get(['key', 'value']) ->mapWithKeys(fn ($item) => [$item['key'] => $item['value']]); - $mainDirectory = str(base_configuration_dir().'/applications/'.$service->uuid); + $mainDirectory = str(base_configuration_dir().'/services/'.$service->uuid); $volumeOwners = collect() ->merge($service->applications ?? collect()) diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index ec328a3e42..4e5c247409 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -288,20 +288,21 @@ function parseDockerVolumeString(string $volumeString): array } // Handle environment variable expansion in source - // Example: ${VOLUME_DB_PATH:-db} should extract default value if present - // Only extract the default value here — full env var resolution happens in the parsers + // Preserve ${VAR:-default} so that resolveEnvVarDefault() in applicationParser/serviceParser + // can check $allEnvironments first before falling back to the default value. + // Only strip to ${VAR} when the default part is explicitly empty. if ($source && preg_match('/^\$\{([^}]+)\}$/', $source, $matches)) { $varContent = $matches[1]; if (strpos($varContent, ':-') !== false) { $parts = explode(':-', $varContent, 2); $varName = $parts[0]; $defaultValue = $parts[1] ?? ''; - if ($defaultValue !== '') { - $source = $defaultValue; - } else { + if ($defaultValue === '') { // Empty default — clean up to simple variable reference $source = '${'.$varName.'}'; } + // Non-empty default: keep $source as-is (e.g. ${VAR:-./path}) + // so resolveEnvVarDefault() can apply env overrides } } @@ -312,11 +313,11 @@ function parseDockerVolumeString(string $volumeString): array // 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 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); + // Skip validation for environment variable references (with or without defaults) + // Pattern 1: ${VAR} or ${VAR:-safe_default} + // Pattern 2: ${VAR}/path or ${VAR:-safe_default}/path + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*(:-[a-zA-Z0-9_.\/~\-]*)?\}$/', $sourceStr); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*(:-[a-zA-Z0-9_.\/~\-]*)?\}[\/\w\.\-]*$/', $sourceStr); if (! $isSimpleEnvVar && ! $isEnvVarWithPath) { try { @@ -739,9 +740,9 @@ 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 environment variable references and env vars with path concatenation - $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); - $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue); + // Allow environment variable references (with or without defaults) and path concatenation + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*(:-[a-zA-Z0-9_.\/~\-]*)?\}$/', $sourceValue); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*(:-[a-zA-Z0-9_.\/~\-]*)?\}[\/\w\.\-]*$/', $sourceValue); if (! $isSimpleEnvVar && ! $isEnvVarWithPath) { try { @@ -2132,9 +2133,9 @@ 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 environment variable references and env vars with path concatenation - $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); - $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue); + // Allow environment variable references (with or without defaults) and path concatenation + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*(:-[a-zA-Z0-9_.\/~\-]*)?\}$/', $sourceValue); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*(:-[a-zA-Z0-9_.\/~\-]*)?\}[\/\w\.\-]*$/', $sourceValue); if (! $isSimpleEnvVar && ! $isEnvVarWithPath) { try { diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index 6f896e11c8..3f25351afa 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -1271,7 +1271,7 @@ function resolveEnvVarDefault(Stringable $source, ?Collection $environmentVariab function sourceIsLocal(Stringable $source) { - if ($source->startsWith('./') || $source->startsWith('/') || $source->startsWith('~') || $source->startsWith('..') || $source->startsWith('~/') || $source->startsWith('../')) { + if ($source->value() === '.' || $source->startsWith('./') || $source->startsWith('/') || $source->startsWith('~') || $source->startsWith('..') || $source->startsWith('~/') || $source->startsWith('../')) { return true; } diff --git a/resources/views/livewire/project/service/file-storage.blade.php b/resources/views/livewire/project/service/file-storage.blade.php index 4c34ae849c..4cd1e4f488 100644 --- a/resources/views/livewire/project/service/file-storage.blade.php +++ b/resources/views/livewire/project/service/file-storage.blade.php @@ -3,9 +3,9 @@ @if ($isReadOnly)
@if ($fileStorage->is_directory) - This directory is mounted as read-only and cannot be modified from the UI. + This directory is mounted as read-only inside the container. Content editing is disabled, but you can still convert or delete it. @else - This file is mounted as read-only and cannot be modified from the UI. + This file is mounted as read-only inside the container. Content editing is disabled, but you can still convert or delete it. @endif
@endif @@ -26,47 +26,59 @@ @endcan @endif
- @if (!$isReadOnly) - @can('update', $resource) -
- @if ($fileStorage->is_directory) - - + @if ($fileStorage->is_directory) + + + @else + @if (!$fileStorage->is_binary) + - @else - @if (!$fileStorage->is_binary) - - @endif - Load from - server - + shortConfirmationLabel="Filepath" :confirmWithPassword="false" + step2ButtonText="Convert to directory" /> @endif -
- @endcan + Load from + server + + @endif + + @else @if (!$fileStorage->is_directory) - @can('update', $resource) + @can('view', $resource) +
+ Load from + server +
+ @endcan + @endif + @endcan + + {{-- Content editing: gated by read-only status --}} + @if (!$fileStorage->is_directory) + @can('update', $resource) + @if (!$isReadOnly) @if (data_get($resource, 'settings.is_preserve_repository_enabled'))
- @endcan - @endif - @else - {{-- Read-only view --}} - @if (!$fileStorage->is_directory) - @can('view', $resource) -
- Load from - server -
- @endcan + @endif + @else @if (data_get($resource, 'settings.is_preserve_repository_enabled'))
- @endif + @endcan @endif
diff --git a/tests/Unit/ParseDockerVolumeStringTest.php b/tests/Unit/ParseDockerVolumeStringTest.php index 6d31725e31..2fbeb6be03 100644 --- a/tests/Unit/ParseDockerVolumeStringTest.php +++ b/tests/Unit/ParseDockerVolumeStringTest.php @@ -61,9 +61,9 @@ }); test('parses volumes with environment variables', function () { - // Variable with default value + // Variable with default value — preserved for env-aware resolution $result = parseDockerVolumeString('${VOLUME_DB_PATH:-db}:/data/db'); - expect($result['source']->value())->toBe('db'); + expect($result['source']->value())->toBe('${VOLUME_DB_PATH:-db}'); expect($result['target']->value())->toBe('/data/db'); expect($result['mode'])->toBeNull(); @@ -79,9 +79,9 @@ expect($result['target']->value())->toBe('/data'); expect($result['mode'])->toBeNull(); - // Variable with mode + // Variable with mode — preserved for env-aware resolution $result = parseDockerVolumeString('${DATA_PATH:-./data}:/app/data:ro'); - expect($result['source']->value())->toBe('./data'); + expect($result['source']->value())->toBe('${DATA_PATH:-./data}'); expect($result['target']->value())->toBe('/app/data'); expect($result['mode']->value())->toBe('ro'); }); @@ -160,6 +160,28 @@ expect($result['mode']->value())->toBe('ro'); }); +test('preserved env var syntax allows resolveEnvVarDefault to apply env overrides', function () { + // Regression: parseDockerVolumeString used to eagerly replace ${VAR:-default} with the default, + // preventing resolveEnvVarDefault() from checking $allEnvironments for an override. + $parsed = parseDockerVolumeString('${AO_CONFIG_FILE:-./agent-orchestrator.yaml}:/app/agent-orchestrator.yaml:ro'); + + // Source must still contain the full env var syntax + expect($parsed['source']->value())->toBe('${AO_CONFIG_FILE:-./agent-orchestrator.yaml}'); + + // When an env override IS provided, resolveEnvVarDefault should use it + $envOverrides = collect(['AO_CONFIG_FILE' => './custom-config.yaml']); + $resolved = resolveEnvVarDefault($parsed['source'], $envOverrides); + expect($resolved->value())->toBe('./custom-config.yaml'); + + // When no env override is provided, resolveEnvVarDefault should fall back to the default + $resolved = resolveEnvVarDefault($parsed['source'], collect()); + expect($resolved->value())->toBe('./agent-orchestrator.yaml'); + + // When env override is empty string, resolveEnvVarDefault should fall back to the default + $resolved = resolveEnvVarDefault($parsed['source'], collect(['AO_CONFIG_FILE' => ''])); + expect($resolved->value())->toBe('./agent-orchestrator.yaml'); +}); + test('parses all valid Docker volume modes', function () { $validModes = ['ro', 'rw', 'z', 'Z', 'rslave', 'rprivate', 'rshared', 'slave', 'private', 'shared', 'cached', 'delegated', 'consistent']; @@ -173,9 +195,9 @@ }); test('parses complex real-world examples', function () { - // MongoDB volume with environment variable + // MongoDB volume with environment variable — preserved for env-aware resolution $result = parseDockerVolumeString('${VOLUME_DB_PATH:-./data/db}:/data/db'); - expect($result['source']->value())->toBe('./data/db'); + expect($result['source']->value())->toBe('${VOLUME_DB_PATH:-./data/db}'); expect($result['target']->value())->toBe('/data/db'); expect($result['mode'])->toBeNull(); diff --git a/tests/Unit/ServiceVolumePathTest.php b/tests/Unit/ServiceVolumePathTest.php new file mode 100644 index 0000000000..7a50cd8c5e --- /dev/null +++ b/tests/Unit/ServiceVolumePathTest.php @@ -0,0 +1,41 @@ +not->toBeFalse('reResolveForService method should exist in LocalFileVolume'); + + // Find the next method boundary to scope our search + $nextMethod = strpos($source, 'private static function ', $methodStart + 1); + $methodBody = substr($source, $methodStart, $nextMethod ? $nextMethod - $methodStart : null); + + // The mainDirectory for services must use /services/, not /applications/ + expect($methodBody)->toContain("base_configuration_dir().'/services/'") + ->and($methodBody)->not->toContain("base_configuration_dir().'/applications/'"); +}); + +it('service and application re-resolve methods use distinct storage roots', function () { + // Verify that the service_configuration_dir helper returns /services/ + expect(service_configuration_dir())->toBe(base_configuration_dir().'/services'); + + // Verify that the application_configuration_dir helper returns /applications/ + expect(application_configuration_dir())->toBe(base_configuration_dir().'/applications'); + + // These must be different + expect(service_configuration_dir())->not->toBe(application_configuration_dir()); +}); diff --git a/tests/Unit/SourceIsLocalTest.php b/tests/Unit/SourceIsLocalTest.php new file mode 100644 index 0000000000..2f22a901ae --- /dev/null +++ b/tests/Unit/SourceIsLocalTest.php @@ -0,0 +1,41 @@ +toBeTrue(); +}); + +test('sourceIsLocal recognizes dot-slash as local path', function () { + expect(sourceIsLocal(str('./data')))->toBeTrue(); +}); + +test('sourceIsLocal recognizes absolute paths as local', function () { + expect(sourceIsLocal(str('/var/data')))->toBeTrue(); +}); + +test('sourceIsLocal recognizes tilde paths as local', function () { + expect(sourceIsLocal(str('~/data')))->toBeTrue(); + expect(sourceIsLocal(str('~')))->toBeTrue(); +}); + +test('sourceIsLocal recognizes double-dot paths as local', function () { + expect(sourceIsLocal(str('../data')))->toBeTrue(); + expect(sourceIsLocal(str('..')))->toBeTrue(); +}); + +test('sourceIsLocal rejects named volumes', function () { + expect(sourceIsLocal(str('my_volume')))->toBeFalse(); + expect(sourceIsLocal(str('data')))->toBeFalse(); +}); + +test('resolveEnvVarDefault bare dot fallback is recognized as local by sourceIsLocal', function () { + // When no env var is set and no default exists, resolveEnvVarDefault returns "." + $resolved = resolveEnvVarDefault(str('${UNDEFINED_VAR}'), collect()); + expect(sourceIsLocal($resolved))->toBeTrue(); +}); + +test('resolveEnvVarDefault with fallback path is recognized as local', function () { + // ${VAR:-./agent-orchestrator.yaml} with no env var set should resolve to the default + $resolved = resolveEnvVarDefault(str('${AO_CONFIG_FILE:-./agent-orchestrator.yaml}'), collect()); + expect($resolved->value())->toBe('./agent-orchestrator.yaml'); + expect(sourceIsLocal($resolved))->toBeTrue(); +}); diff --git a/tests/Unit/VolumeArrayFormatSecurityTest.php b/tests/Unit/VolumeArrayFormatSecurityTest.php index f920f5bd9f..e1f588fdd2 100644 --- a/tests/Unit/VolumeArrayFormatSecurityTest.php +++ b/tests/Unit/VolumeArrayFormatSecurityTest.php @@ -429,9 +429,10 @@ ->not->toThrow(Exception::class); }); -test('LocalFileVolume resolvedFsPath with simple env var falls back to dot', function () { +test('LocalFileVolume resolvedFsPath with unresolvable env var throws RuntimeException', function () { $volume = new LocalFileVolume; $volume->fs_path = '${DATA_PATH}'; - expect($volume->resolvedFsPath())->toBe('.'); + expect(fn () => $volume->resolvedFsPath()) + ->toThrow(RuntimeException::class, 'Cannot resolve storage path'); }); From 5fac72c7a71088faf9d7efdf8dc0ad2b1e08c760 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 1 Apr 2026 14:02:04 +0200 Subject: [PATCH 3/5] fix(navigation): stop using wire:navigate.hover in helper Switch `wireNavigate()` to return `wire:navigate` when enabled, including the exception fallback path. Add a unit test to assert the helper no longer contains `wire:navigate.hover`. --- bootstrap/helpers/shared.php | 6 +++--- tests/Unit/WireNavigateHelperTest.php | 10 ++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) create mode 100644 tests/Unit/WireNavigateHelperTest.php diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index 3f25351afa..445607138e 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -3488,10 +3488,10 @@ function wireNavigate(): string try { $settings = instanceSettings(); - // Return wire:navigate.hover for SPA navigation with prefetching, or empty string if disabled - return ($settings->is_wire_navigate_enabled ?? true) ? 'wire:navigate.hover' : ''; + // Return wire:navigate for SPA navigation without hover-prefetching, or empty string if disabled + return ($settings->is_wire_navigate_enabled ?? true) ? 'wire:navigate' : ''; } catch (Exception $e) { - return 'wire:navigate.hover'; + return 'wire:navigate'; } } diff --git a/tests/Unit/WireNavigateHelperTest.php b/tests/Unit/WireNavigateHelperTest.php new file mode 100644 index 0000000000..21d92e8fc7 --- /dev/null +++ b/tests/Unit/WireNavigateHelperTest.php @@ -0,0 +1,10 @@ +toContain("return (\$settings->is_wire_navigate_enabled ?? true) ? 'wire:navigate' : '';") + ->toContain("return 'wire:navigate';") + ->not->toContain('wire:navigate.hover'); +}); From c94aa9d812dd56e71784bfcc484516fab714c898 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 1 Apr 2026 14:36:24 +0200 Subject: [PATCH 4/5] fix(volumes): treat unresolved env refs as non-local paths Return unresolved ${VAR} references unchanged in resolveEnvVarDefault instead of coercing them to ".", so sourceIsLocal() does not classify missing env vars as local bind mounts. Also hard-fail LocalFileVolume::resolvedFsPath() when unresolved placeholders remain in the resolved value, and update unit tests to cover the regression and expected local-default behavior. --- app/Models/LocalFileVolume.php | 2 +- bootstrap/helpers/shared.php | 5 ++-- tests/Unit/SourceIsLocalTest.php | 7 ++--- tests/Unit/VolumeArrayFormatSecurityTest.php | 28 +++++++++++++++----- 4 files changed, 30 insertions(+), 12 deletions(-) diff --git a/app/Models/LocalFileVolume.php b/app/Models/LocalFileVolume.php index 7599435381..60dba1db77 100644 --- a/app/Models/LocalFileVolume.php +++ b/app/Models/LocalFileVolume.php @@ -58,7 +58,7 @@ public function resolvedFsPath(): string $resolved = resolveEnvVarDefault(str($this->fs_path), $envVars); $value = $resolved->value(); - if ($value === '.' || $value === '') { + if ($value === '.' || $value === '' || str_contains($value, '${')) { throw new \RuntimeException( 'Cannot resolve storage path: environment variable has no value and no default was provided. ' .'Original path: '.$this->fs_path diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index 445607138e..9cf0df2f6d 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -1262,8 +1262,9 @@ function resolveEnvVarDefault(Stringable $source, ?Collection $environmentVariab return str($defaultValue); } - // 3. No env var and no default — fall back to current directory (coolify data dir) - return str('.'); + // 3. No env var and no default — return original reference unchanged so callers + // (e.g. sourceIsLocal()) don't misclassify it as a local bind mount. + return $source; } return $source; diff --git a/tests/Unit/SourceIsLocalTest.php b/tests/Unit/SourceIsLocalTest.php index 2f22a901ae..d47436f808 100644 --- a/tests/Unit/SourceIsLocalTest.php +++ b/tests/Unit/SourceIsLocalTest.php @@ -27,10 +27,11 @@ expect(sourceIsLocal(str('data')))->toBeFalse(); }); -test('resolveEnvVarDefault bare dot fallback is recognized as local by sourceIsLocal', function () { - // When no env var is set and no default exists, resolveEnvVarDefault returns "." +test('resolveEnvVarDefault unresolvable bare env var is NOT local', function () { + // When no env var is set and no default exists, resolveEnvVarDefault returns the + // original string — sourceIsLocal must NOT treat it as a local bind mount. $resolved = resolveEnvVarDefault(str('${UNDEFINED_VAR}'), collect()); - expect(sourceIsLocal($resolved))->toBeTrue(); + expect(sourceIsLocal($resolved))->toBeFalse(); }); test('resolveEnvVarDefault with fallback path is recognized as local', function () { diff --git a/tests/Unit/VolumeArrayFormatSecurityTest.php b/tests/Unit/VolumeArrayFormatSecurityTest.php index e1f588fdd2..1ab3caa73c 100644 --- a/tests/Unit/VolumeArrayFormatSecurityTest.php +++ b/tests/Unit/VolumeArrayFormatSecurityTest.php @@ -312,14 +312,14 @@ expect($result->value())->toBe('/var/lib/data'); }); -test('resolveEnvVarDefault falls back to dot when no default and no env vars', function () { +test('resolveEnvVarDefault returns original string when no default and no env vars', function () { $result = resolveEnvVarDefault(str('${DATA_PATH}')); - expect($result->value())->toBe('.'); + expect($result->value())->toBe('${DATA_PATH}'); }); -test('resolveEnvVarDefault falls back to dot when empty default and no env vars', function () { +test('resolveEnvVarDefault returns original string when empty default and no env vars', function () { $result = resolveEnvVarDefault(str('${DATA_PATH:-}')); - expect($result->value())->toBe('.'); + expect($result->value())->toBe('${DATA_PATH:-}'); }); test('resolveEnvVarDefault keeps non-env-var string unchanged', function () { @@ -369,10 +369,10 @@ expect($result->value())->toBe('./default-config.yaml'); }); -test('resolveEnvVarDefault falls back to dot when env var not set and no default', function () { +test('resolveEnvVarDefault returns original string when env var not set and no default', function () { $envVars = collect(['OTHER_VAR' => '/other/path']); $result = resolveEnvVarDefault(str('${DATA_PATH}'), $envVars); - expect($result->value())->toBe('.'); + expect($result->value())->toBe('${DATA_PATH}'); }); test('resolveEnvVarDefault preview env var overrides normal env var', function () { @@ -436,3 +436,19 @@ expect(fn () => $volume->resolvedFsPath()) ->toThrow(RuntimeException::class, 'Cannot resolve storage path'); }); + +// Regression: bare ${VAR} must NOT be misclassified as a local bind mount +test('bare unresolved env var is not treated as local path by sourceIsLocal', function () { + $result = resolveEnvVarDefault(str('${DATA_VOLUME}')); + expect(sourceIsLocal($result))->toBeFalse(); +}); + +test('bare unresolved env var with empty default is not treated as local path', function () { + $result = resolveEnvVarDefault(str('${DATA_VOLUME:-}')); + expect(sourceIsLocal($result))->toBeFalse(); +}); + +test('env var with local default IS treated as local path by sourceIsLocal', function () { + $result = resolveEnvVarDefault(str('${CONFIG_FILE:-./config.yaml}')); + expect(sourceIsLocal($result))->toBeTrue(); +}); From 573dc6d31ea7b1b6a329bc817e838f2f95c7c9fc Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 1 Apr 2026 16:00:40 +0200 Subject: [PATCH 5/5] fix(volumes): make env-default parsing version-aware in v6 Switch default parser version to v6 for Application and Service, and only preserve/resolve `${VAR:-default}` sources in v6 parsing flows. Also update file storage path re-resolution to run for v6 resources and reclassify volumes between LocalFileVolume and LocalPersistentVolume when env var resolution changes. --- app/Livewire/Project/Service/FileStorage.php | 11 ++++- app/Models/Application.php | 2 +- app/Models/LocalFileVolume.php | 51 ++++++++++++++++++++ app/Models/Service.php | 2 +- bootstrap/helpers/parsers.php | 42 +++++++++------- tests/Unit/ParseDockerVolumeStringTest.php | 20 ++++++-- 6 files changed, 104 insertions(+), 24 deletions(-) diff --git a/app/Livewire/Project/Service/FileStorage.php b/app/Livewire/Project/Service/FileStorage.php index dcaa02c0da..965935eed8 100644 --- a/app/Livewire/Project/Service/FileStorage.php +++ b/app/Livewire/Project/Service/FileStorage.php @@ -67,7 +67,7 @@ public function mount() } $this->isReadOnly = $this->fileStorage->shouldBeReadOnlyInUI(); - $this->resolvedFromEnvVar = $this->detectEnvVarSource(); + $this->resolvedFromEnvVar = $this->isV6Parser() ? $this->detectEnvVarSource() : null; $this->syncData(); } @@ -90,6 +90,15 @@ public function syncData(bool $toModel = false): void } } + private function isV6Parser(): bool + { + $version = $this->resource instanceof Application + ? $this->resource->compose_parsing_version + : ($this->resource->service->compose_parsing_version ?? null); + + return (int) $version >= 6; + } + /** * Check the raw docker-compose for env var references in this volume's source. * Returns the variable name if found, null otherwise. diff --git a/app/Models/Application.php b/app/Models/Application.php index fef6f6e4c3..6c9067bf91 100644 --- a/app/Models/Application.php +++ b/app/Models/Application.php @@ -116,7 +116,7 @@ class Application extends BaseModel { use ClearsGlobalSearchCache, HasConfiguration, HasFactory, HasMetrics, HasSafeStringAttribute, SoftDeletes; - private static $parserVersion = '5'; + private static $parserVersion = '6'; protected $fillable = [ 'name', diff --git a/app/Models/LocalFileVolume.php b/app/Models/LocalFileVolume.php index 60dba1db77..dec3a9b66c 100644 --- a/app/Models/LocalFileVolume.php +++ b/app/Models/LocalFileVolume.php @@ -7,6 +7,7 @@ use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Support\Collection; +use Illuminate\Support\Str; use Illuminate\Support\Stringable; use Symfony\Component\Yaml\Yaml; @@ -137,6 +138,10 @@ public static function reResolveVolumePaths($resource): void private static function reResolveForApplication(Application $application): void { + if ((int) $application->compose_parsing_version < 6) { + return; + } + $dockerComposeRaw = $application->docker_compose_raw; if (! $dockerComposeRaw) { return; @@ -179,6 +184,10 @@ private static function reResolveForApplication(Application $application): void private static function reResolveForService(Service $service): void { + if ((int) $service->compose_parsing_version < 6) { + return; + } + $dockerComposeRaw = $service->docker_compose_raw; if (! $dockerComposeRaw) { return; @@ -250,7 +259,49 @@ private static function updateFileVolumePaths($resource, array $rawSources, $env if ($fileVolume->fs_path !== $resolved->value()) { $fileVolume->update(['fs_path' => $resolved->value()]); } + } else { + // Env var removed or no longer resolves to local path — convert back to named volume + $slugWithoutUuid = Str::slug($resolved, '-'); + $name = $resource->uuid.'_'.$slugWithoutUuid; + LocalPersistentVolume::updateOrCreate( + [ + 'mount_path' => $fileVolume->mount_path, + 'resource_id' => $resource->id, + 'resource_type' => get_class($resource), + ], + [ + 'name' => $name, + ] + ); + $fileVolume->delete(); + } + } + + // Reclassify: when env var now resolves to a local path, create LocalFileVolume + // and remove stale LocalPersistentVolume + foreach ($rawSources as $mountPath => $rawSource) { + $sourceStr = $rawSource instanceof Stringable ? $rawSource : str($rawSource); + if (! str_contains($sourceStr->value(), '${')) { + continue; + } + + $resolved = resolveEnvVarDefault($sourceStr, $envVars); + if (! sourceIsLocal($resolved)) { + continue; } + + if (! $resource->fileStorages()->where('mount_path', $mountPath)->exists()) { + $fsPath = replaceLocalSource($resolved, $mainDirectory); + LocalFileVolume::create([ + 'mount_path' => $mountPath, + 'fs_path' => $fsPath, + 'is_directory' => true, + 'resource_id' => $resource->id, + 'resource_type' => get_class($resource), + ]); + } + + $resource->persistentStorages()->where('mount_path', $mountPath)->delete(); } } diff --git a/app/Models/Service.php b/app/Models/Service.php index 11189b4ac6..9e087a22af 100644 --- a/app/Models/Service.php +++ b/app/Models/Service.php @@ -46,7 +46,7 @@ class Service extends BaseModel { use ClearsGlobalSearchCache, HasFactory, HasSafeStringAttribute, SoftDeletes; - private static $parserVersion = '5'; + private static $parserVersion = '6'; protected $fillable = [ 'uuid', diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 4e5c247409..037c8df144 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -113,7 +113,7 @@ function validateVolumeStringForInjection(string $volumeString): void parseDockerVolumeString($volumeString); } -function parseDockerVolumeString(string $volumeString): array +function parseDockerVolumeString(string $volumeString, bool $preserveEnvDefaults = false): array { $volumeString = trim($volumeString); $source = null; @@ -288,21 +288,23 @@ function parseDockerVolumeString(string $volumeString): array } // Handle environment variable expansion in source - // Preserve ${VAR:-default} so that resolveEnvVarDefault() in applicationParser/serviceParser - // can check $allEnvironments first before falling back to the default value. - // Only strip to ${VAR} when the default part is explicitly empty. if ($source && preg_match('/^\$\{([^}]+)\}$/', $source, $matches)) { $varContent = $matches[1]; if (strpos($varContent, ':-') !== false) { $parts = explode(':-', $varContent, 2); $varName = $parts[0]; $defaultValue = $parts[1] ?? ''; - if ($defaultValue === '') { + if ($defaultValue !== '') { + if ($preserveEnvDefaults) { + // v6+: keep ${VAR:-default} so resolveEnvVarDefault() can apply env overrides + } else { + // v5 and below: strip to just the default value (v4.x behavior) + $source = $defaultValue; + } + } else { // Empty default — clean up to simple variable reference $source = '${'.$varName.'}'; } - // Non-empty default: keep $source as-is (e.g. ${VAR:-./path}) - // so resolveEnvVarDefault() can apply env overrides } } @@ -694,6 +696,7 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $predefinedPort = null; $originalResource = $resource; + $isV6 = (int) $resource->compose_parsing_version >= 6; if ($volumes->count() > 0) { foreach ($volumes as $index => $volume) { @@ -703,11 +706,13 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $content = null; $isDirectory = false; if (is_string($volume)) { - $parsed = parseDockerVolumeString($volume); + $parsed = parseDockerVolumeString($volume, $isV6); $source = $parsed['source']; $target = $parsed['target']; - // Resolve env vars in source (e.g., ${VAR} -> value from Coolify env vars) - $source = resolveEnvVarDefault($source, $allEnvironments); + // v6+: resolve env vars in source (e.g., ${VAR} -> value from Coolify env vars) + if ($isV6) { + $source = resolveEnvVarDefault($source, $allEnvironments); + } // Mode is available in $parsed['mode'] if needed $foundConfig = $fileStorages->whereMountPath($target)->first(); if (sourceIsLocal($source)) { @@ -732,8 +737,8 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $content = data_get($volume, 'content'); $isDirectory = (bool) data_get($volume, 'isDirectory', null) || (bool) data_get($volume, 'is_directory', null); - // Resolve environment variable defaults (e.g., ${VAR:-./path} -> ./path) - if ($source !== null && ! empty($source->value())) { + // v6+: resolve environment variable defaults (e.g., ${VAR:-./path} -> ./path) + if ($isV6 && $source !== null && ! empty($source->value())) { $source = resolveEnvVarDefault($source, $allEnvironments); } @@ -2087,6 +2092,7 @@ function serviceParser(Service $resource): Collection } $originalResource = $savedService; + $isV6 = (int) $resource->compose_parsing_version >= 6; if ($volumes->count() > 0) { foreach ($volumes as $index => $volume) { @@ -2096,11 +2102,13 @@ function serviceParser(Service $resource): Collection $content = null; $isDirectory = false; if (is_string($volume)) { - $parsed = parseDockerVolumeString($volume); + $parsed = parseDockerVolumeString($volume, $isV6); $source = $parsed['source']; $target = $parsed['target']; - // Resolve env vars in source (e.g., ${VAR} -> value from Coolify env vars) - $source = resolveEnvVarDefault($source, $allEnvironments); + // v6+: resolve env vars in source (e.g., ${VAR} -> value from Coolify env vars) + if ($isV6) { + $source = resolveEnvVarDefault($source, $allEnvironments); + } // Mode is available in $parsed['mode'] if needed $foundConfig = $fileStorages->whereMountPath($target)->first(); if (sourceIsLocal($source)) { @@ -2125,8 +2133,8 @@ function serviceParser(Service $resource): Collection $content = data_get($volume, 'content'); $isDirectory = (bool) data_get($volume, 'isDirectory', null) || (bool) data_get($volume, 'is_directory', null); - // Resolve environment variable defaults (e.g., ${VAR:-./path} -> ./path) - if ($source !== null && ! empty($source->value())) { + // v6+: resolve environment variable defaults (e.g., ${VAR:-./path} -> ./path) + if ($isV6 && $source !== null && ! empty($source->value())) { $source = resolveEnvVarDefault($source, $allEnvironments); } diff --git a/tests/Unit/ParseDockerVolumeStringTest.php b/tests/Unit/ParseDockerVolumeStringTest.php index 2fbeb6be03..9ab09df5f1 100644 --- a/tests/Unit/ParseDockerVolumeStringTest.php +++ b/tests/Unit/ParseDockerVolumeStringTest.php @@ -61,8 +61,12 @@ }); test('parses volumes with environment variables', function () { - // Variable with default value — preserved for env-aware resolution + // v5 and below: default is stripped $result = parseDockerVolumeString('${VOLUME_DB_PATH:-db}:/data/db'); + expect($result['source']->value())->toBe('db'); + + // v6+: preserved for env-aware resolution + $result = parseDockerVolumeString('${VOLUME_DB_PATH:-db}:/data/db', preserveEnvDefaults: true); expect($result['source']->value())->toBe('${VOLUME_DB_PATH:-db}'); expect($result['target']->value())->toBe('/data/db'); expect($result['mode'])->toBeNull(); @@ -79,8 +83,12 @@ expect($result['target']->value())->toBe('/data'); expect($result['mode'])->toBeNull(); - // Variable with mode — preserved for env-aware resolution + // Variable with mode — v5: default is stripped $result = parseDockerVolumeString('${DATA_PATH:-./data}:/app/data:ro'); + expect($result['source']->value())->toBe('./data'); + + // Variable with mode — v6+: preserved for env-aware resolution + $result = parseDockerVolumeString('${DATA_PATH:-./data}:/app/data:ro', preserveEnvDefaults: true); expect($result['source']->value())->toBe('${DATA_PATH:-./data}'); expect($result['target']->value())->toBe('/app/data'); expect($result['mode']->value())->toBe('ro'); @@ -163,7 +171,7 @@ test('preserved env var syntax allows resolveEnvVarDefault to apply env overrides', function () { // Regression: parseDockerVolumeString used to eagerly replace ${VAR:-default} with the default, // preventing resolveEnvVarDefault() from checking $allEnvironments for an override. - $parsed = parseDockerVolumeString('${AO_CONFIG_FILE:-./agent-orchestrator.yaml}:/app/agent-orchestrator.yaml:ro'); + $parsed = parseDockerVolumeString('${AO_CONFIG_FILE:-./agent-orchestrator.yaml}:/app/agent-orchestrator.yaml:ro', preserveEnvDefaults: true); // Source must still contain the full env var syntax expect($parsed['source']->value())->toBe('${AO_CONFIG_FILE:-./agent-orchestrator.yaml}'); @@ -195,8 +203,12 @@ }); test('parses complex real-world examples', function () { - // MongoDB volume with environment variable — preserved for env-aware resolution + // MongoDB volume with environment variable — v5: default is stripped $result = parseDockerVolumeString('${VOLUME_DB_PATH:-./data/db}:/data/db'); + expect($result['source']->value())->toBe('./data/db'); + + // v6+: preserved for env-aware resolution + $result = parseDockerVolumeString('${VOLUME_DB_PATH:-./data/db}:/data/db', preserveEnvDefaults: true); expect($result['source']->value())->toBe('${VOLUME_DB_PATH:-./data/db}'); expect($result['target']->value())->toBe('/data/db'); expect($result['mode'])->toBeNull();