starred/coolify
1
0
Fork 0
mirror of https://github.com/coollabsio/coolify.git synced 2026-09-26 01:10:30 -04:00
Code Issues Packages Projects Releases Wiki Activity

Improve volume path handling (#11993)

Browse Source
This commit is contained in:
Andras Bacsai
2026-09-24 23:22:35 +02:00
committed by GitHub
parent 50f49f093e
commit 0935bf141e
6 changed files with 248 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
bootstrap/helpers/parsers.php
+34 -50
View File
@@ -60,24 +60,7 @@ function validateDockerComposeForInjection(string $composeYaml): void
if (isset($volume['source'])) {
$source = $volume['source'];
if (is_string($source)) {
// Allow env vars and env vars with defaults (validated in parseDockerVolumeString)
// Also allow env vars followed by safe path concatenation (e.g., ${VAR}/path)
$isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source);
$isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $source);
$isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $source);
if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) {
try {
validateShellSafePath($source, 'volume source');
} catch (Exception $e) {
throw new Exception(
'Invalid Docker volume definition (array syntax): '.$e->getMessage().
' Please use safe path names without shell metacharacters.',
0,
$e
);
}
}
validateComposeArrayVolumeSource($source);
}
}
if (isset($volume['target'])) {
@@ -122,6 +105,37 @@ function validateDockerComposeForInjection(string $composeYaml): void
}
}
/**
* Keep the existing array-source forms, but inspect the default that was previously skipped.
*/
function validateComposeArrayVolumeSource(string $source): void
{
try {
if (preg_match('/[\x00-\x1F\x7F]/', $source)) {
throw new Exception('Invalid volume source: contains a control character.');
}
if (preg_match('/^\$\{[A-Za-z_][A-Za-z0-9_]*\}[\/\w.\-]*$/', $source)) {
return;
}
if (preg_match('/^\$\{[A-Za-z_][A-Za-z0-9_]*:-([^}]*)\}$/', $source, $matches)) {
validateShellSafePath($matches[1], 'volume source');
return;
}
validateShellSafePath($source, 'volume source');
} catch (Exception $e) {
throw new Exception(
'Invalid Docker volume definition (array syntax): '.$e->getMessage().
' Please use safe path names without shell metacharacters.',
0,
$e
);
}
}
/**
* Reject Docker Compose network names that are not valid Docker identifiers.
*
@@ -881,22 +895,7 @@ 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);
$isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $sourceValue);
$isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue);
if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) {
try {
validateShellSafePath($sourceValue, 'volume source');
} catch (Exception $e) {
throw new Exception(
'Invalid Docker volume definition (array syntax): '.$e->getMessage().
' Please use safe path names without shell metacharacters.'
);
}
}
validateComposeArrayVolumeSource($source->value());
}
if ($target !== null && ! empty($target->value())) {
try {
@@ -2248,22 +2247,7 @@ 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);
$isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $sourceValue);
$isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue);
if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) {
try {
validateShellSafePath($sourceValue, 'volume source');
} catch (Exception $e) {
throw new Exception(
'Invalid Docker volume definition (array syntax): '.$e->getMessage().
' Please use safe path names without shell metacharacters.'
);
}
}
validateComposeArrayVolumeSource($source->value());
}
if ($target !== null && ! empty($target->value())) {
try {
bootstrap/helpers/services.php
+35 -9
View File
@@ -161,6 +161,31 @@ function replaceVariables(string $variable): Stringable
return $str;
}
/**
* Quote a storage path without changing its existing simple shell-variable expansion.
*/
function filesystemVolumeShellArgument(string $path): string
{
if (trim($path) === '') {
throw new Exception('Invalid storage path: path is empty.');
}
validateComposeArrayVolumeSource($path);
$tilde = '';
if (preg_match('/^~[A-Za-z0-9_-]*(?:\/|$)/', $path, $matches)) {
$tilde = $matches[0];
$path = substr($path, strlen($tilde));
}
if (! str_contains($path, '$')) {
return $tilde === '' ? escapeshellarg($path) : $tilde.($path === '' ? '' : escapeshellarg($path));
}
$quotedPath = str_replace(['\\', '"'], ['\\\\', '\\"'], $path);
return $tilde.'"'.$quotedPath.'"';
}
function getFilesystemVolumesFromServer(ServiceApplication|ServiceDatabase|Application $oneService, bool $isInit = false)
{
try {
@@ -172,9 +197,10 @@ function getFilesystemVolumesFromServer(ServiceApplication|ServiceDatabase|Appli
$server = $oneService->service->server;
}
$fileVolumes = $oneService->fileStorages()->get();
$escapedWorkdir = escapeshellarg($workdir);
$commands = collect([
"mkdir -p $workdir > /dev/null 2>&1 || true",
"cd $workdir",
"mkdir -p -- {$escapedWorkdir} > /dev/null 2>&1 || true",
"cd {$escapedWorkdir}",
]);
instant_remote_process($commands, $server);
foreach ($fileVolumes as $fileVolume) {
@@ -186,10 +212,11 @@ function getFilesystemVolumesFromServer(ServiceApplication|ServiceDatabase|Appli
} else {
$fileLocation = $path;
}
$escapedFileLocation = filesystemVolumeShellArgument((string) $fileLocation);
// Exists and is a file
$isFile = instant_remote_process(["test -f $fileLocation && echo OK || echo NOK"], $server);
$isFile = instant_remote_process(["test -f {$escapedFileLocation} && echo OK || echo NOK"], $server);
// Exists and is a directory
$isDir = instant_remote_process(["test -d $fileLocation && echo OK || echo NOK"], $server);
$isDir = instant_remote_process(["test -d {$escapedFileLocation} && echo OK || echo NOK"], $server);
if ($isFile === 'OK') {
$fileVolume->is_directory = false;
@@ -208,23 +235,22 @@ function getFilesystemVolumesFromServer(ServiceApplication|ServiceDatabase|Appli
$fileVolume->is_directory = false;
$fileVolume->save();
$content = base64_encode($content);
$dir = str($fileLocation)->dirname();
instant_remote_process([
"mkdir -p $dir",
"echo '$content' | base64 -d | tee $fileLocation",
'mkdir -p -- "$(dirname -- '.$escapedFileLocation.')"',
"echo '$content' | base64 -d | tee -- {$escapedFileLocation}",
], $server);
} elseif ($isFile === 'NOK' && $isDir === 'NOK' && $fileVolume->is_directory && $isInit) {
// Does not exists (no dir or file), flagged as directory, is init
$fileVolume->content = null;
$fileVolume->is_directory = true;
$fileVolume->save();
instant_remote_process(["mkdir -p $fileLocation"], $server);
instant_remote_process(["mkdir -p -- {$escapedFileLocation}"], $server);
} elseif ($isFile === 'NOK' && $isDir === 'NOK' && ! $fileVolume->is_directory && $isInit && is_null($content)) {
// Does not exists (no dir or file), not flagged as directory, is init, has no content => create directory
$fileVolume->content = null;
$fileVolume->is_directory = true;
$fileVolume->save();
instant_remote_process(["mkdir -p $fileLocation"], $server);
instant_remote_process(["mkdir -p -- {$escapedFileLocation}"], $server);
}
}
} catch (Throwable $e) {
tests/Feature/FileStorageParserStateTest.php
+30
View File
@@ -139,6 +139,36 @@ it('keeps valid Compose when one-time fields use flow syntax', function () {
->and($cleanedSource)->not->toContain('content: initial');
});
it('rejects unsafe array source defaults in the application parser', function (string $source) {
$application = makeComposeApplication("services:\n app:\n image: nginx\n volumes:\n - type: bind\n source: '".$source."'\n target: /app/data\n");
expect(fn () => applicationParser($application))->toThrow(Exception::class, 'Invalid Docker volume definition');
})->with(['${DATA:-/tmp/evil`id`}', '${DATA:-/tmp/evil$(id)}', '${DATA:-/tmp/evil;id}']);
it('rejects unsafe array source defaults in the service parser', function (string $source) {
[$service] = makeComposeService("services:\n app:\n image: nginx\n volumes:\n - type: bind\n source: '".$source."'\n target: /app/data\n");
expect(fn () => serviceParser($service))->toThrow(Exception::class, 'Invalid Docker volume definition');
})->with(['${DATA:-/tmp/evil`id`}', '${DATA:-/tmp/evil$(id)}', '${DATA:-/tmp/evil;id}']);
it('keeps safe array source expressions in both parsers', function (string $source) {
$compose = "services:\n app:\n image: nginx\n volumes:\n - type: bind\n source: '".$source."'\n target: /app/data\n";
$application = makeComposeApplication($compose);
[$service] = makeComposeService($compose);
expect(fn () => applicationParser($application))->not->toThrow(Exception::class)
->and(fn () => serviceParser($service))->not->toThrow(Exception::class);
})->with(['${DATA}', '${DATA}/config', '${DATA}//config', '${DATA:-/srv/app/data}', '/srv/$HOME/config.yml', '$HOME/$FILE', '${DATA:-/srv/$HOME/config.yml}']);
it('keeps unsupported array source forms rejected in both parsers', function (string $source) {
$compose = "services:\n app:\n image: nginx\n volumes:\n - type: bind\n source: '".$source."'\n target: /app/data\n";
$application = makeComposeApplication($compose);
[$service] = makeComposeService($compose);
expect(fn () => applicationParser($application))->toThrow(Exception::class, 'Invalid Docker volume definition')
->and(fn () => serviceParser($service))->toThrow(Exception::class, 'Invalid Docker volume definition');
})->with(['${DATA:+/srv/app}', '${DATA:-${HOME}/config.yml}', '${DATA:-/srv/app}/file', '${DATA:?missing}', '${DATA?missing}', '${DATA-/srv/app}', '${DATA+/srv/app}']);
it('preserves existing application file volume content when reparsing compose bind mounts', function () {
$application = makeComposeApplication(TWO_FILE_COMPOSE);
$baseDir = application_configuration_dir()."/{$application->uuid}";
tests/Unit/LocalFileVolumeContentSizeTest.php
+96 -3
View File
@@ -18,6 +18,7 @@ use Illuminate\Database\Eloquent\Relations\MorphMany;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\Process;
use Illuminate\Support\Facades\Storage;
use Symfony\Component\Process\Process as SymfonyProcess;
use Tests\TestCase;
uses(TestCase::class, RefreshDatabase::class);
@@ -98,11 +99,11 @@ it('does not read regular bind-mounted file contents while loading service setti
$application->setRelation('destination', (object) ['server' => $server]);
Process::fake(function ($process) {
if (str_contains($process->command, 'test -f /data/large.bin')) {
if (str_contains($process->command, "test -f '/data/large.bin'")) {
return Process::result(output: 'OK');
}
if (str_contains($process->command, 'test -d /data/large.bin')) {
if (str_contains($process->command, "test -d '/data/large.bin'")) {
return Process::result(output: 'NOK');
}
@@ -112,10 +113,102 @@ it('does not read regular bind-mounted file contents while loading service setti
getFilesystemVolumesFromServer($application);
expect($volume->is_directory)->toBeFalse();
Process::assertRan(fn ($process) => str_contains($process->command, 'test -f /data/large.bin'));
Process::assertRan(fn ($process) => str_contains($process->command, "test -f '/data/large.bin'"));
Process::assertNotRan(fn ($process) => str_contains($process->command, 'cat /data/large.bin') || str_contains($process->command, 'head -c'));
});
it('does not interpolate unsafe persisted file-storage paths into remote commands', function (string $path) {
$user = User::factory()->create();
$privateKey = PrivateKey::factory()->create(['team_id' => $user->teams()->first()->id]);
Storage::fake('ssh-keys');
$server = Server::factory()->create([
'team_id' => $user->teams()->first()->id,
'private_key_id' => $privateKey->id,
]);
$volume = Mockery::mock(LocalFileVolume::class)->makePartial();
$volume->fs_path = $path;
$fileStorages = Mockery::mock(MorphMany::class);
$fileStorages->shouldReceive('get')->once()->andReturn(collect([$volume]));
$application = Mockery::mock(Application::class)->makePartial();
$application->shouldReceive('getMorphClass')->andReturn(Application::class);
$application->shouldReceive('workdir')->once()->andReturn('/data/application');
$application->shouldReceive('fileStorages')->once()->andReturn($fileStorages);
$application->setRelation('destination', (object) ['server' => $server]);
Process::fake();
expect(fn () => getFilesystemVolumesFromServer($application, true))->toThrow(Exception::class);
Process::assertNotRan(fn ($process) => str_contains($process->command, $path));
})->with(['/tmp/evil`id`', '/tmp/evil$(id)', '/tmp/evil;id', '/tmp/evil|id', '${DATA:-/tmp/evil$(id)}', '/srv/$HOME;id', '${DATA:-/srv/app;id}/config.yml', '${DATA:-${HOME:-$(id)}}', '${DATA:+/srv/app;id}/config.yml']);
it('preserves safe remote-shell path expansion', function (string $path, array $environment, string $expected) {
$argument = filesystemVolumeShellArgument($path);
$process = new SymfonyProcess(['bash', '-c', "printf '%s' {$argument}"], env: $environment);
$process->mustRun();
expect($process->getOutput())->toBe($expected);
})->with([
'bare variable' => ['$HOME/config.yml', ['HOME' => '/tmp/test-home'], '/tmp/test-home/config.yml'],
'variable within absolute path' => ['/srv/$HOME/config.yml', ['HOME' => 'tenant'], '/srv/tenant/config.yml'],
'multiple variables' => ['$HOME/$FILE', ['HOME' => '/tmp/test-home', 'FILE' => 'config.yml'], '/tmp/test-home/config.yml'],
'home shortcut' => ['~/config.yml', ['HOME' => '/tmp/test-home'], '/tmp/test-home/config.yml'],
'braced variable' => ['${DATA_PATH}/config.yml', ['DATA_PATH' => '/srv/my data'], '/srv/my data/config.yml'],
'default when unset' => ['${DATA_PATH:-/srv/app/config.yml}', ['DATA_PATH' => ''], '/srv/app/config.yml'],
'set value over default' => ['${DATA_PATH:-/srv/app/config.yml}', ['DATA_PATH' => '/mnt/config.yml'], '/mnt/config.yml'],
'variable in default' => ['${DATA:-/srv/$HOME/config.yml}', ['DATA' => '', 'HOME' => 'tenant'], '/srv/tenant/config.yml'],
'quotes in default' => ['${DATA:-/srv/my "data"/config.yml}', ['DATA' => ''], '/srv/my "data"/config.yml'],
'expanded value is not shell code' => ['$DATA_PATH/config.yml', ['DATA_PATH' => '$(printf injected)'], '$(printf injected)/config.yml'],
]);
it('rejects unsupported persisted Compose expressions before shell use', function (string $path) {
expect(fn () => filesystemVolumeShellArgument($path))->toThrow(Exception::class);
})->with(['${DATA:+/srv/app}', '${DATA:-${HOME}/config.yml}', '${DATA:-/srv/app}/file', '${DATA:?missing}', '${DATA?missing}', '${DATA-/srv/app}', '${DATA+/srv/app}']);
it('quotes literal file-storage paths and safely expands persisted expressions', function () {
$user = User::factory()->create();
$privateKey = PrivateKey::factory()->create(['team_id' => $user->teams()->first()->id]);
Storage::fake('ssh-keys');
$server = Server::factory()->create([
'team_id' => $user->teams()->first()->id,
'private_key_id' => $privateKey->id,
]);
$literal = Mockery::mock(LocalFileVolume::class)->makePartial();
$literal->fs_path = '/data/my files/config.yaml';
$literal->is_directory = true;
$literal->shouldReceive('save')->once();
$file = Mockery::mock(LocalFileVolume::class)->makePartial();
$file->fs_path = '/data/my files/settings.json';
$file->content = '{}';
$file->is_directory = false;
$file->shouldReceive('save')->once();
$expression = Mockery::mock(LocalFileVolume::class)->makePartial();
$expression->fs_path = '${DATA_PATH:-/srv/app/config.yaml}';
$expression->is_directory = true;
$expression->shouldReceive('save')->once();
$fileStorages = Mockery::mock(MorphMany::class);
$fileStorages->shouldReceive('get')->once()->andReturn(collect([$literal, $file, $expression]));
$application = Mockery::mock(Application::class)->makePartial();
$application->shouldReceive('getMorphClass')->andReturn(Application::class);
$application->shouldReceive('workdir')->once()->andReturn('/data/application');
$application->shouldReceive('fileStorages')->once()->andReturn($fileStorages);
$application->setRelation('destination', (object) ['server' => $server]);
Process::fake(fn ($process) => Process::result(output: str_contains($process->command, 'test -') ? 'NOK' : ''));
getFilesystemVolumesFromServer($application, true);
Process::assertRan(fn ($process) => str_contains($process->command, "test -f '/data/my files/config.yaml'"));
Process::assertRan(fn ($process) => str_contains($process->command, "mkdir -p -- '/data/my files/config.yaml'"));
Process::assertRan(fn ($process) => str_contains($process->command, "dirname -- '/data/my files/settings.json'"));
Process::assertRan(fn ($process) => str_contains($process->command, "tee -- '/data/my files/settings.json'"));
Process::assertRan(fn ($process) => str_contains($process->command, '${DATA_PATH:-/srv/app/config.yaml}'));
});
it('bounds the remote file read itself to prevent a size-check race', function () {
$source = remoteOutputSource('app/Models/LocalFileVolume.php');
$loadStorage = str($source)
tests/Unit/ParseCommandsByLineForSudoTest.php
+20
View File
@@ -43,6 +43,26 @@ test('preserves command substitutions inside database and volume backup scripts'
->not->toContain('$(sudo if');
});
test('keeps safe file-storage path expansion in non-root commands', function () {
$argument = filesystemVolumeShellArgument('${DATA_PATH:-/srv/app/config.yml}');
$commands = collect([
"test -f {$argument} && echo OK || echo NOK",
'mkdir -p -- "$(dirname -- '.$argument.')"',
"echo 'e30=' | base64 -d | tee -- {$argument}",
]);
$result = parseCommandsByLineForSudo($commands, $this->server);
expect($result[0])->toContain('"${DATA_PATH:-/srv/app/config.yml}"')
->and($result[1])->toContain('$(sudo dirname -- "${DATA_PATH:-/srv/app/config.yml}")')
->and($result[2])->toContain('"${DATA_PATH:-/srv/app/config.yml}"');
});
test('rejects unsupported nested Compose defaults before non-root file commands', function () {
expect(fn () => filesystemVolumeShellArgument('${DATA:-${HOME}/config.yml}'))
->toThrow(Exception::class);
});
test('preserves quoted backup container and file arguments for a non-root server', function () {
$container = escapeshellarg('db-name-uuid');
$path = escapeshellarg('/backups/db-name.dump');
tests/Unit/VolumeArrayFormatSecurityTest.php
+33
View File
@@ -243,6 +243,39 @@ YAML;
->toThrow(Exception::class);
});
test('compose validator rejects unsafe array source defaults', function (string $source) {
$compose = "services:\n web:\n image: nginx\n volumes:\n - type: bind\n source: '".$source."'\n target: /app\n";
expect(fn () => validateDockerComposeForInjection($compose))
->toThrow(Exception::class, 'Invalid Docker volume definition');
})->with([
'${DATA:-/tmp/evil`id`}',
'${DATA:-/tmp/evil$(id)}',
'${DATA:-/tmp/evil;id}',
'${DATA:-/tmp/evil|id}',
'${DATA:-/tmp/evil$(id)}/config.yml',
'${DATA:-${HOME:-$(id)}/config.yml}',
'${DATA:+/srv/app;id}/config.yml',
'${DATA:?missing;id}',
'${DATA?$(id)}',
'${DATA-/tmp/evil`id`}',
'${DATA+/tmp/evil|id}',
'/srv/$HOME/evil`id`',
]);
test('compose validator keeps safe array source expressions', function (string $source) {
$compose = "services:\n web:\n image: nginx\n volumes:\n - type: bind\n source: '".$source."'\n target: /app\n";
expect(fn () => validateDockerComposeForInjection($compose))->not->toThrow(Exception::class);
})->with(['${DATA}', '${DATA}/config', '${DATA}//config', '${DATA:-/srv/app/data}', '/srv/$HOME/config.yml', '$HOME/$FILE', '${DATA:-/srv/$HOME/config.yml}']);
test('compose validator keeps unsupported array source forms rejected', function (string $source) {
$compose = "services:\n web:\n image: nginx\n volumes:\n - type: bind\n source: '".$source."'\n target: /app\n";
expect(fn () => validateDockerComposeForInjection($compose))
->toThrow(Exception::class, 'Invalid Docker volume definition');
})->with(['${DATA:+/srv/app}', '${DATA:-${HOME}/config.yml}', '${DATA:-/srv/app}/file', '${DATA:?missing}', '${DATA?missing}', '${DATA-/srv/app}', '${DATA+/srv/app}']);
test('mixed string and array format volumes in same compose', function () {
$dockerComposeYaml = <<<'YAML'
services: