diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 66a834c1db..005a4334dd 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -549,19 +549,25 @@ function composeExternalVolumeMounts(?string $compose): array } /** - * The key and the name of an external volume must be literal Docker volume names. Docker Compose - * can resolve variables in `name:`, but then only the deployment `.env` sets which existing volume - * the resource mounts, and Coolify cannot show or check that volume. Thus variables are rejected. + * The key of an external volume must be a literal Docker volume name, because Docker Compose does not + * resolve variables in keys. The name (`name:` or the old `external: {name: x}`) may also contain + * variables (see isComposeVolumeNameWithVariables()): Docker Compose resolves them from the deployment + * `.env` when it starts the resource. Coolify never removes an external volume, because it gets no + * storage entry, so Coolify does not need the resolved name. * * @param array $declaration * - * @throws Exception If the key or the name is not a literal, valid Docker volume name + * @throws Exception If the key or the name is not a valid Docker volume name */ function validateComposeExternalVolume(int|string $key, array $declaration): void { $key = (string) $key; - $label = preg_match(ValidationPatterns::VOLUME_NAME_PATTERN, $key) === 1 ? "volume {$key}" : 'volume'; - $names = [$key]; + if (preg_match(ValidationPatterns::VOLUME_NAME_PATTERN, $key) !== 1) { + throw new Exception( + 'Invalid external Docker Compose volume. Volume names must start with an alphanumeric character and contain only alphanumeric characters, dots, hyphens, and underscores.' + ); + } + $names = []; if (array_key_exists('name', $declaration)) { $names[] = $declaration['name']; } @@ -570,19 +576,25 @@ function validateComposeExternalVolume(int|string $key, array $declaration): voi } foreach ($names as $name) { - if (is_string($name) && str_contains($name, '$')) { + if (! is_string($name) || (preg_match(ValidationPatterns::VOLUME_NAME_PATTERN, $name) !== 1 && ! isComposeVolumeNameWithVariables($name))) { throw new Exception( - "Invalid external Docker Compose {$label}: Coolify does not resolve variables in the name of an external volume. Use the literal name of the existing Docker volume." - ); - } - if (! is_string($name) || preg_match(ValidationPatterns::VOLUME_NAME_PATTERN, $name) !== 1) { - throw new Exception( - "Invalid external Docker Compose {$label}. Volume names must start with an alphanumeric character and contain only alphanumeric characters, dots, hyphens, and underscores." + "Invalid external Docker Compose volume {$key}. The name must be a Docker volume name (alphanumeric characters, dots, hyphens, and underscores) and can contain variables such as \${NAME} or \${NAME:-default}." ); } } } +/** + * Tells if a volume name contains Compose variables and otherwise only Docker volume name characters. + * The variables are `$NAME`, `${NAME}`, `${NAME-default}` and `${NAME:-default}`, with a default + * that contains only Docker volume name characters. + */ +function isComposeVolumeNameWithVariables(string $name): bool +{ + return str_contains($name, '$') + && preg_match('/^(?:[A-Za-z0-9._-]|\$[A-Za-z_][A-Za-z0-9_]*|\$\{[A-Za-z_][A-Za-z0-9_]*(?::?-[A-Za-z0-9._-]*)?\})+$/', $name) === 1; +} + /** * Validates a Docker volume string (format: "source:target" or "source:target:mode") * diff --git a/tests/Feature/ApplicationComposeInjectionValidationTest.php b/tests/Feature/ApplicationComposeInjectionValidationTest.php index 5ed51f2160..aee515917f 100644 --- a/tests/Feature/ApplicationComposeInjectionValidationTest.php +++ b/tests/Feature/ApplicationComposeInjectionValidationTest.php @@ -151,6 +151,15 @@ test('loading a safe Compose file from Git saves it', function () { expect($this->application->refresh()->docker_compose_raw)->toBe(trim($compose)); }); +test('loading a Compose file with a variable external volume name saves it', function () { + $compose = "services:\n web:\n image: nginx\n volumes:\n - 'shared-data:/data'\nvolumes:\n shared-data:\n external: true\n name: \${SHARED_VOLUME}\n"; + fakeRepositoryCompose($compose); + + $this->application->loadComposeFile(); + + expect($this->application->fresh()->docker_compose_raw)->toBe(trim($compose)); +}); + test('the queued LoadComposeFile action rejects injection', function () { fakeRepositoryCompose(composeInjectionPayloads()['service name command substitution'][0]); diff --git a/tests/Feature/ComposeExternalVolumeTest.php b/tests/Feature/ComposeExternalVolumeTest.php index e66797826d..9b97887a32 100644 --- a/tests/Feature/ComposeExternalVolumeTest.php +++ b/tests/Feature/ComposeExternalVolumeTest.php @@ -4,6 +4,7 @@ use App\Actions\Service\DeployServiceApplication; use App\Actions\Service\StartService; use App\Jobs\ApplicationDeploymentJob; use App\Jobs\DeleteResourceJob; +use App\Livewire\Project\Service\StackForm; use App\Livewire\Project\Service\Storage as StoragePage; use App\Livewire\Project\Shared\Storages\All; use App\Models\Application; @@ -609,24 +610,112 @@ describe('legacy parsers', function () { }); }); +function variableExternalVolumeCompose(string $name, bool $oldSyntax = false): string +{ + $declaration = $oldSyntax ? " external:\n name: '{$name}'\n" : " external: true\n name: '{$name}'\n"; + + return "services:\n web:\n image: nginx\n volumes:\n - 'shared-data:/data'\nvolumes:\n shared-data:\n{$declaration}"; +} + +dataset('external volume names with a variable', [ + 'variable' => ['${SHARED_VOLUME}', false], + 'variable with a default' => ['${SHARED_VOLUME:-shared}', false], + 'variable with text around it' => ['stack-${STACK}_data', false], + 'two variables' => ['${PROJECT}-${ENVIRONMENT}', false], + 'old syntax' => ['$SHARED_VOLUME', true], +]); + describe('validation', function () { - it('rejects an external volume name with a variable', function (string $compose) { - expect(fn () => validateDockerComposeForInjection($compose)) - ->toThrow(Exception::class, 'Coolify does not resolve variables in the name of an external volume'); + it('accepts an external volume name with a variable and leaves it to Docker Compose', function (string $name, bool $oldSyntax) { + $compose = variableExternalVolumeCompose($name, $oldSyntax); + $declaration = $oldSyntax ? ['external' => ['name' => $name]] : ['external' => true, 'name' => $name]; + validateDockerComposeForInjection($compose); - $application = externalVolumeApplication($compose); - expect(fn () => applicationParser($application)) - ->toThrow(Exception::class, 'Coolify does not resolve variables in the name of an external volume'); + $application = applicationParser(externalVolumeApplication($compose))->toArray(); + $service = serviceParser(externalVolumeService($compose))->toArray(); + expect($application['services']['web']['volumes'])->toBe(['shared-data:/data']) + ->and($application['volumes'])->toBe(['shared-data' => $declaration]) + ->and($service['services']['web']['volumes'])->toBe(['shared-data:/data']) + ->and($service['volumes'])->toBe(['shared-data' => $declaration]) + ->and(persistentVolumeNames())->toBe([]); + })->with('external volume names with a variable'); + + it('keeps the old volume of an existing application whose external volume name has a variable', function (string $name, bool $oldSyntax) { + $application = externalVolumeApplication(variableExternalVolumeCompose($name, $oldSyntax)); + $uuid = $application->uuid; + legacyExternalVolumeRow($application, "{$uuid}_shared-data"); + + $parsed = applicationParser($application)->toArray(); + + expect($parsed['services']['web']['volumes'])->toBe(["{$uuid}_shared-data:/data"]) + ->and($parsed['volumes']["{$uuid}_shared-data"])->toBe(['name' => "{$uuid}_shared-data"]) + ->and($application->composeVolumeWarnings())->toBe([legacyExternalVolumeWarning("{$uuid}_shared-data", $name)]); + })->with('external volume names with a variable'); + + it('keeps the old volume of an existing service whose external volume name has a variable', function (string $name, bool $oldSyntax) { + $compose = variableExternalVolumeCompose($name, $oldSyntax); $service = externalVolumeService($compose); - expect(fn () => serviceParser($service)) - ->toThrow(Exception::class, 'Coolify does not resolve variables in the name of an external volume'); + $uuid = $service->uuid; + legacyExternalVolumeRow(legacyExternalVolumeServiceResource($service, 'web', $compose), "{$uuid}_shared-data"); + + $parsed = serviceParser($service)->toArray(); + + expect($parsed['services']['web']['volumes'])->toBe(["{$uuid}_shared-data:/data"]) + ->and($parsed['volumes']["{$uuid}_shared-data"])->toBe(['name' => "{$uuid}_shared-data"]) + ->and($service->composeVolumeWarnings())->toBe([legacyExternalVolumeWarning("{$uuid}_shared-data", $name)]); + })->with('external volume names with a variable'); + + it('saves a service Compose file whose external volume name has a variable', function () { + $compose = variableExternalVolumeCompose('${SHARED_VOLUME}'); + $service = externalVolumeService(EXTERNAL_VOLUME_SHORT_COMPOSE); + $owner = User::factory()->create(); + $team = $service->environment->project->team; + $team->members()->attach($owner, ['role' => 'owner']); + $this->actingAs($owner); + session(['currentTeam' => $team]); + + Livewire::test(StackForm::class, ['service' => $service]) + ->set('dockerComposeRaw', $compose) + ->call('submit') + ->assertDispatched('success'); + + expect($service->fresh()->docker_compose_raw)->toBe($compose); + }); + + it('keeps the old behavior of a legacy Compose application whose external volume name has a variable', function (string $name, bool $oldSyntax) { + $application = externalVolumeApplication(variableExternalVolumeCompose($name, $oldSyntax), '2'); + + $parsed = parseDockerComposeFile($application)->toArray(); + + expect($parsed['services']['web']['volumes'])->toBe(["{$application->uuid}-shared-data:/data"]); + })->with('external volume names with a variable'); + + it('rejects an external volume name with an unsafe or unsupported variable', function (string $name) { + $compose = variableExternalVolumeCompose($name); + $application = externalVolumeApplication($compose); + + expect(fn () => validateDockerComposeForInjection($compose)) + ->toThrow(Exception::class, 'Invalid external Docker Compose volume shared-data') + ->and(fn () => applicationParser($application)) + ->toThrow(Exception::class, 'Invalid external Docker Compose volume shared-data'); })->with([ - 'name field' => ["services:\n web:\n image: nginx\n volumes:\n - 'shared-data:/data'\nvolumes:\n shared-data:\n external: true\n name: \${SHARED_VOLUME}\n"], - 'name field with default' => ["services:\n web:\n image: nginx\n volumes:\n - 'shared-data:/data'\nvolumes:\n shared-data:\n external: true\n name: \${SHARED_VOLUME:-shared}\n"], - 'old syntax' => ["services:\n web:\n image: nginx\n volumes:\n - 'shared-data:/data'\nvolumes:\n shared-data:\n external:\n name: \$SHARED_VOLUME\n"], + 'command after a variable' => ['${SHARED_VOLUME}; rm -rf /'], + 'command substitution' => ['$(id)'], + 'space' => ['shared ${VOLUME}'], + 'dollar sign alone' => ['shared-$'], + 'escaped dollar sign' => ['$$SHARED_VOLUME'], + 'required variable syntax' => ['${SHARED_VOLUME:?required}'], + 'default with a space' => ['${SHARED_VOLUME:-a b}'], ]); + it('rejects a variable in the key of an external volume', function () { + $compose = "services:\n web:\n image: nginx\nvolumes:\n \${SHARED_VOLUME}:\n external: true\n"; + + expect(fn () => validateDockerComposeForInjection($compose)) + ->toThrow(Exception::class, 'Invalid external Docker Compose volume. Volume names must start with an alphanumeric character'); + }); + it('rejects an external volume name that is not a valid Docker volume name', function () { $compose = "services:\n web:\n image: nginx\nvolumes:\n shared-data:\n external: true\n name: 'shared data;rm -rf /'\n";