From 0935bf141e3befdc325201e298d19de17349dedb Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 24 Sep 2026 23:22:35 +0200 Subject: [PATCH] Improve volume path handling (#11993) --- bootstrap/helpers/parsers.php | 84 +++++++--------- bootstrap/helpers/services.php | 44 +++++++-- tests/Feature/FileStorageParserStateTest.php | 30 ++++++ tests/Unit/LocalFileVolumeContentSizeTest.php | 99 ++++++++++++++++++- tests/Unit/ParseCommandsByLineForSudoTest.php | 20 ++++ tests/Unit/VolumeArrayFormatSecurityTest.php | 33 +++++++ 6 files changed, 248 insertions(+), 62 deletions(-) diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 31eae9e4c9..951876926b 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -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 { diff --git a/bootstrap/helpers/services.php b/bootstrap/helpers/services.php index 96257a6323..8e56f06fcb 100644 --- a/bootstrap/helpers/services.php +++ b/bootstrap/helpers/services.php @@ -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) { diff --git a/tests/Feature/FileStorageParserStateTest.php b/tests/Feature/FileStorageParserStateTest.php index 7244c9683b..0311711adb 100644 --- a/tests/Feature/FileStorageParserStateTest.php +++ b/tests/Feature/FileStorageParserStateTest.php @@ -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}"; diff --git a/tests/Unit/LocalFileVolumeContentSizeTest.php b/tests/Unit/LocalFileVolumeContentSizeTest.php index 2a22234012..abd03c32d3 100644 --- a/tests/Unit/LocalFileVolumeContentSizeTest.php +++ b/tests/Unit/LocalFileVolumeContentSizeTest.php @@ -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) diff --git a/tests/Unit/ParseCommandsByLineForSudoTest.php b/tests/Unit/ParseCommandsByLineForSudoTest.php index d9ffa8c7da..673b6a023f 100644 --- a/tests/Unit/ParseCommandsByLineForSudoTest.php +++ b/tests/Unit/ParseCommandsByLineForSudoTest.php @@ -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'); diff --git a/tests/Unit/VolumeArrayFormatSecurityTest.php b/tests/Unit/VolumeArrayFormatSecurityTest.php index 08174fff3c..da5d9ec4af 100644 --- a/tests/Unit/VolumeArrayFormatSecurityTest.php +++ b/tests/Unit/VolumeArrayFormatSecurityTest.php @@ -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: