From 2ade6963758266a244fe787152b79ecd35e8849d Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 20 Oct 2026 11:41:54 +0200 Subject: [PATCH] fix(storage): resolve ${VAR:-default} bind sources and safe conversion (#12227) --- app/Livewire/Project/Service/FileStorage.php | 8 +- bootstrap/helpers/parsers.php | 76 +++++++++++----- .../project/service/file-storage.blade.php | 86 +++++++++---------- tests/Feature/ComposeLongBindOptionsTest.php | 46 +++++++++- tests/Feature/StorageHostPathTest.php | 14 +++ 5 files changed, 164 insertions(+), 66 deletions(-) diff --git a/app/Livewire/Project/Service/FileStorage.php b/app/Livewire/Project/Service/FileStorage.php index 7c6a2917ba..fcf042c4fb 100644 --- a/app/Livewire/Project/Service/FileStorage.php +++ b/app/Livewire/Project/Service/FileStorage.php @@ -167,10 +167,12 @@ class FileStorage extends Component $this->fileStorage->content = null; $this->fileStorage->is_based_on_git = false; $changedFields = auditChangedFields($this->fileStorage); - $this->fileStorage->save(); $this->fileStorage->saveStorageOnServer(); + $this->fileStorage->save(); $this->auditFileStorageUpdate($changedFields, 'converted_to_directory'); } catch (\Throwable $e) { + $this->fileStorage->refresh(); + return handleError($e, $this); } finally { $this->dispatch('storageCountsChanged')->to(Storage::class); @@ -216,10 +218,12 @@ class FileStorage extends Component $this->fileStorage->is_based_on_git = true; } $changedFields = auditChangedFields($this->fileStorage); - $this->fileStorage->save(); $this->fileStorage->saveStorageOnServer(); + $this->fileStorage->save(); $this->auditFileStorageUpdate($changedFields, 'converted_to_file'); } catch (\Throwable $e) { + $this->fileStorage->refresh(); + return handleError($e, $this); } finally { $this->dispatch('storageCountsChanged')->to(Storage::class); diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 8493a26bd1..64d16c2c94 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -237,6 +237,52 @@ function validateComposeContentVolumeSource(array $volume): void validateComposeArrayVolumeSource($source); } +/** + * Replaces a `${VAR:-default}` volume source with its default, as the short volume syntax does. + * An empty default becomes `${VAR}`. Other sources do not change. + */ +function resolveComposeVolumeSourceDefault(string $source): string +{ + if (! preg_match('/^\$\{([^}]+)\}$/', $source, $matches) || ! str_contains($matches[1], ':-')) { + return $source; + } + + [$varName, $defaultValue] = explode(':-', $matches[1], 2); + + return $defaultValue !== '' ? $defaultValue : '${'.$varName.'}'; +} + +/** + * Resolves a long-syntax bind source as Docker Compose does with the `.env` file that Coolify writes: + * `${VAR:-default}` is the value of VAR when the resource sets it to a non-empty value, else the default. + * + * @throws Exception If the value of the variable is not a shell-safe path + */ +function resolveComposeLongBindSource(string $source, Application|Service $resource, bool $isPreview = false): string +{ + if (! preg_match('/^\$\{([A-Za-z_][A-Za-z0-9_]*):-[^}]*\}$/', $source, $matches)) { + return resolveComposeVolumeSourceDefault($source); + } + + $variables = match (true) { + $resource instanceof Service => $resource->environment_variables(), + $isPreview => $resource->runtime_environment_variables_preview()->where('is_runtime', true), + default => $resource->runtime_environment_variables()->where('is_runtime', true), + }; + $value = $variables->where('key', $matches[1])->first()?->real_value; + if (blank($value)) { + return resolveComposeVolumeSourceDefault($source); + } + + try { + validateShellSafePath($value, 'volume source'); + } catch (Exception $e) { + throw new Exception("Invalid Docker volume definition (array syntax): the value of {$matches[1]} is not a safe path. ".$e->getMessage(), 0, $e); + } + + return $value; +} + /** * Keep the existing array-source forms, but inspect the default that was previously skipped. */ @@ -932,26 +978,8 @@ function parseDockerVolumeString(string $volumeString): array } } - // Handle environment variable expansion in source - // Example: ${VOLUME_DB_PATH:-db} should extract default value if present - 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 - if ($defaultValue !== '') { - $source = $defaultValue; - } else { - // Empty default value, keep the variable reference for env resolution - $source = '${'.$varName.'}'; - } - } - // Otherwise keep the variable as-is for later expansion (no default value) + if ($source) { + $source = resolveComposeVolumeSourceDefault((string) $source); } // Validate source path for command injection attempts @@ -1500,6 +1528,10 @@ 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())) { validateComposeArrayVolumeSource($source->value()); + // A named volume keeps its source as written, so its volume name does not change. + if ($type->value() === 'bind') { + $source = str(resolveComposeLongBindSource($source->value(), $resource, $isPullRequest)); + } } validateComposeContentVolumeSource($volume); if ($target !== null && ! empty($target->value())) { @@ -2921,6 +2953,10 @@ function serviceParser(Service $resource): Collection // Validate source and target for command injection (array/long syntax) if ($source !== null && ! empty($source->value())) { validateComposeArrayVolumeSource($source->value()); + // A named volume keeps its source as written, so its volume name does not change. + if ($type->value() === 'bind') { + $source = str(resolveComposeLongBindSource($source->value(), $resource)); + } } validateComposeContentVolumeSource($volume); if ($target !== null && ! empty($target->value())) { diff --git a/resources/views/livewire/project/service/file-storage.blade.php b/resources/views/livewire/project/service/file-storage.blade.php index c3be252697..f24d9347e4 100644 --- a/resources/views/livewire/project/service/file-storage.blade.php +++ b/resources/views/livewire/project/service/file-storage.blade.php @@ -11,9 +11,9 @@ @elseif ($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. You can convert it to a file, but you cannot delete it from the UI. @else - This file is mounted as read-only and cannot be modified from the UI. + This file is mounted as read-only. You cannot edit or delete it from the UI. @endif @endif @@ -45,58 +45,64 @@ @endif
- @if (!$isReadOnly) - @can('update', $resource) -
- @if ($fileStorage->is_host_file) + @can('update', $resource) +
+ @if ($fileStorage->is_host_file) + @if (!$isReadOnly) - @elseif ($fileStorage->is_directory) - - @if ($resource instanceof \App\Models\Application) - - - - @endif + @endif + @elseif ($fileStorage->is_directory) + + @if ($resource instanceof \App\Models\Application && !$isReadOnly) + + + + @endif + @if (!$isReadOnly) - @else - @if (!$fileStorage->is_binary && !$fileStorage->is_too_large) - - @endif - Load from - server + @endif + @else + @if (!$fileStorage->is_binary && !$fileStorage->is_too_large) + + @endif + Load from + server + @if (!$isReadOnly) @endif -
- @endcan + @endif +
+ @endcan + @if (!$isReadOnly) @if (!$fileStorage->is_directory && !$fileStorage->is_host_file) @can('update', $resource) @if (data_get($resource, 'settings.is_preserve_repository_enabled')) @@ -125,12 +131,6 @@ @else {{-- Read-only view --}} @if (!$fileStorage->is_directory && !$fileStorage->is_host_file) - @can('update', $resource) -
- Load from - server -
- @endcan @if (data_get($resource, 'settings.is_preserve_repository_enabled'))
$variables Environment variables that the resource sets before the parse * @return array{volume: mixed, volumes: array, storages: Collection, directory: string} */ -function parseLongBind(string $kind, string $compose): array +function parseLongBind(string $kind, string $compose, array $variables = []): array { if ($kind === 'service') { $resource = Service::factory()->create([ @@ -77,6 +78,9 @@ function parseLongBind(string $kind, string $compose): array 'destination_id' => test()->destination->id, 'destination_type' => test()->destination->getMorphClass(), ])->fresh(); + foreach ($variables as $key => $value) { + $resource->environment_variables()->create(['key' => $key, 'value' => $value]); + } serviceParser($resource); $parsed = serviceParser($resource->fresh())->toArray(); $owner = $resource->applications()->firstOrFail(); @@ -89,6 +93,9 @@ function parseLongBind(string $kind, string $compose): array 'destination_id' => test()->destination->id, 'destination_type' => test()->destination->getMorphClass(), ])->fresh(); + foreach ($variables as $key => $value) { + $resource->environment_variables()->create(['key' => $key, 'value' => $value]); + } applicationParser($resource); $parsed = applicationParser($resource->fresh())->toArray(); $owner = $resource; @@ -180,3 +187,40 @@ YAML); 'bind' => ['create_host_path' => true], ]); })->with(['service', 'application']); + +it('uses the default of a ${VAR:-default} long-form bind source as the host path', function (string $kind) { + $result = parseLongBind($kind, longBindCompose("'\${AO_CONFIG_FILE:-./agent-orchestrator.yaml}'", LONG_BIND_READ_ONLY_OPTIONS)); + + expect($result['volume']['source'])->toBe($result['directory'].'/agent-orchestrator.yaml') + ->and($result['storages']->pluck('fs_path')->all())->toBe([$result['directory'].'/agent-orchestrator.yaml']); +})->with(['service', 'application']); + +it('keeps the volume name of a long-form named volume with a ${VAR:-default} source', function (string $kind) { + $result = parseLongBind($kind, <<<'YAML' +services: + probe: + image: alpine:3.22 + volumes: + - type: volume + source: '${DATA_VOLUME:-pgdata}' + target: /fixture +YAML); + + expect($result['volume']['source'])->toEndWith('_data-volume-pgdata'); +})->with(['service', 'application']); + +it('uses the value that the resource sets for a ${VAR:-default} long-form bind source', function (string $kind, string $value, string $expected) { + $result = parseLongBind($kind, longBindCompose("'\${AO_CONFIG_FILE:-./agent-orchestrator.yaml}'", LONG_BIND_READ_ONLY_OPTIONS), ['AO_CONFIG_FILE' => $value]); + $expected = str_replace('{directory}', $result['directory'], $expected); + + expect($result['volume']['source'])->toBe($expected) + ->and($result['storages']->pluck('fs_path')->all())->toBe([$expected]); +})->with(['service', 'application'])->with([ + 'absolute value' => ['/opt/ao-custom.yaml', '/opt/ao-custom.yaml'], + 'relative value' => ['./custom.yaml', '{directory}/custom.yaml'], + 'empty value' => ['', '{directory}/agent-orchestrator.yaml'], +]); + +it('rejects an unsafe value for a ${VAR:-default} long-form bind source', function (string $kind) { + parseLongBind($kind, longBindCompose("'\${AO_CONFIG_FILE:-./agent-orchestrator.yaml}'", LONG_BIND_READ_ONLY_OPTIONS), ['AO_CONFIG_FILE' => '/opt/x;touch /tmp/pwned']); +})->with(['service', 'application'])->throws(Exception::class, 'the value of AO_CONFIG_FILE is not a safe path'); diff --git a/tests/Feature/StorageHostPathTest.php b/tests/Feature/StorageHostPathTest.php index ac526078a8..3f60abf56b 100644 --- a/tests/Feature/StorageHostPathTest.php +++ b/tests/Feature/StorageHostPathTest.php @@ -449,3 +449,17 @@ test('changing the content of a mount outside the resource directory needs deplo ->assertOk(); expect($storage->fresh()->content)->toBe('listen 9090;'); })->with(['application', 'database', 'service']); + +test('a failed conversion keeps the storage type that is on the server', function (string $action, bool $isDirectory) { + // The legacy parser stored this raw source, so the server step rejects it. + $volume = storageHostPathFileVolume('${AO_CONFIG_FILE:-./agent-orchestrator.yaml}', null, $isDirectory); + + Livewire::test(FileStorage::class, ['fileStorage' => $volume]) + ->call($action) + ->assertDispatched('error'); + + expect($volume->fresh()->is_directory)->toBe($isDirectory); +})->with([ + 'to file' => ['convertToFile', true], + 'to directory' => ['convertToDirectory', false], +]);