From 99dd634ba38cf0afaddf592f8741f0baab8614a8 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:13:39 +0200 Subject: [PATCH] Improve file storage handling --- .../Api/ApplicationsController.php | 15 ++-- .../Controllers/Api/DatabasesController.php | 15 ++-- .../Controllers/Api/ServicesController.php | 15 ++-- app/Livewire/Project/Service/Storage.php | 19 ++-- app/Models/LocalFileVolume.php | 73 ++++++++++++++++ tests/Feature/FileStorageMountPathTest.php | 75 +++++++++++++++- tests/Feature/StorageApiTest.php | 87 +++++++++++++++++++ tests/Unit/FileStorageSecurityTest.php | 3 +- 8 files changed, 277 insertions(+), 25 deletions(-) diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index 2ceed613dc..da06428b50 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -5294,11 +5294,16 @@ class ApplicationsController extends Controller ], 422); } - $fsPath = str($request->fs_path)->trim()->start('/')->value(); - $mountPath = str($request->mount_path)->trim()->start('/')->value(); - - validateShellSafePath($fsPath, 'storage source path'); - validateShellSafePath($mountPath, 'storage destination path'); + try { + $fsPath = confinePathToBase(application_configuration_dir().'/'.$application->uuid, $request->fs_path, 'storage source path'); + $mountPath = validateFileMountPath($request->mount_path, 'storage destination path'); + LocalFileVolume::assertRemotePathIsConfined($application->workdir(), $fsPath, $application->destination->server); + } catch (\Throwable $e) { + return response()->json([ + 'message' => 'Validation failed.', + 'errors' => ['fs_path' => $e->getMessage()], + ], 422); + } $storage = LocalFileVolume::create([ 'fs_path' => $fsPath, diff --git a/app/Http/Controllers/Api/DatabasesController.php b/app/Http/Controllers/Api/DatabasesController.php index f6074e0a1e..6019ee2c78 100644 --- a/app/Http/Controllers/Api/DatabasesController.php +++ b/app/Http/Controllers/Api/DatabasesController.php @@ -4264,11 +4264,16 @@ class DatabasesController extends Controller ], 422); } - $fsPath = str($request->fs_path)->trim()->start('/')->value(); - $mountPath = str($request->mount_path)->trim()->start('/')->value(); - - validateShellSafePath($fsPath, 'storage source path'); - validateShellSafePath($mountPath, 'storage destination path'); + try { + $fsPath = confinePathToBase(database_configuration_dir().'/'.$database->uuid, $request->fs_path, 'storage source path'); + $mountPath = validateFileMountPath($request->mount_path, 'storage destination path'); + LocalFileVolume::assertRemotePathIsConfined($database->workdir(), $fsPath, $database->destination->server); + } catch (\Throwable $e) { + return response()->json([ + 'message' => 'Validation failed.', + 'errors' => ['fs_path' => $e->getMessage()], + ], 422); + } $storage = LocalFileVolume::create([ 'fs_path' => $fsPath, diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index 099c126624..f9440818d3 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -2551,11 +2551,16 @@ class ServicesController extends Controller ], 422); } - $fsPath = str($request->fs_path)->trim()->start('/')->value(); - $mountPath = str($request->mount_path)->trim()->start('/')->value(); - - validateShellSafePath($fsPath, 'storage source path'); - validateShellSafePath($mountPath, 'storage destination path'); + try { + $fsPath = confinePathToBase(service_configuration_dir().'/'.$service->uuid, $request->fs_path, 'storage source path'); + $mountPath = validateFileMountPath($request->mount_path, 'storage destination path'); + LocalFileVolume::assertRemotePathIsConfined($service->workdir(), $fsPath, $service->server); + } catch (\Throwable $e) { + return response()->json([ + 'message' => 'Validation failed.', + 'errors' => ['fs_path' => $e->getMessage()], + ], 422); + } $storage = LocalFileVolume::create([ 'fs_path' => $fsPath, diff --git a/app/Livewire/Project/Service/Storage.php b/app/Livewire/Project/Service/Storage.php index 10079276f2..3bf5001f4d 100644 --- a/app/Livewire/Project/Service/Storage.php +++ b/app/Livewire/Project/Service/Storage.php @@ -312,14 +312,17 @@ class Storage extends Component 'file_storage_directory_destination' => 'required|string', ]); - $this->file_storage_directory_source = trim($this->file_storage_directory_source); - $this->file_storage_directory_source = str($this->file_storage_directory_source)->start('/')->value(); - $this->file_storage_directory_destination = trim($this->file_storage_directory_destination); - $this->file_storage_directory_destination = str($this->file_storage_directory_destination)->start('/')->value(); - - // Validate paths to prevent command injection - validateShellSafePath($this->file_storage_directory_source, 'storage source path'); - validateShellSafePath($this->file_storage_directory_destination, 'storage destination path'); + $this->file_storage_directory_source = confinePathToBase( + $this->fileStorageHostPath(), + $this->file_storage_directory_source, + 'storage source path' + ); + $this->file_storage_directory_destination = validateFileMountPath( + $this->file_storage_directory_destination, + 'storage destination path' + ); + $server = $this->resource->service?->server ?? $this->resource->destination->server; + LocalFileVolume::assertRemotePathIsConfined($this->fileStorageHostPath(), $this->file_storage_directory_source, $server); LocalFileVolume::create([ 'fs_path' => $this->file_storage_directory_source, diff --git a/app/Models/LocalFileVolume.php b/app/Models/LocalFileVolume.php index 92b7853400..8be4f8f2a1 100644 --- a/app/Models/LocalFileVolume.php +++ b/app/Models/LocalFileVolume.php @@ -125,6 +125,11 @@ class LocalFileVolume extends BaseModel $path = $workdir.$path; } + if (! $this->isAdminControlledComposeMount()) { + $path = str(confinePathToBase($workdir, $path->value(), 'storage path')); + $this->assertRemotePathIsConfined($workdir, $path->value(), $server); + } + // Validate and escape path to prevent command injection validateShellSafePath($path, 'storage path'); $escapedPath = escapeshellarg($path); @@ -204,6 +209,11 @@ class LocalFileVolume extends BaseModel $path = $workdir.$path; } + if (! $this->isAdminControlledComposeMount()) { + $path = str(confinePathToBase($workdir, $path->value(), 'storage path')); + $this->assertRemotePathIsConfined($workdir, $path->value(), $server); + } + // Validate and escape path to prevent command injection validateShellSafePath($path, 'storage path'); $escapedPath = escapeshellarg($path); @@ -264,6 +274,11 @@ class LocalFileVolume extends BaseModel $path = $workdir.$path; } + if (! $this->isAdminControlledComposeMount()) { + $path = str(confinePathToBase($workdir, $path->value(), 'storage path')); + $this->assertRemotePathIsConfined($workdir, $path->value(), $server); + } + // Validate and escape resolved path (may differ from fs_path if relative) validateShellSafePath($path, 'storage path'); $escapedPath = escapeshellarg($path); @@ -315,6 +330,64 @@ class LocalFileVolume extends BaseModel return instant_remote_process($commands, $server); } + /** + * Reject symlink escapes immediately before a managed path is used remotely. + */ + public static function assertRemotePathIsConfined(string $baseDirectory, string $path, Server $server): void + { + $escapedBase = escapeshellarg($baseDirectory); + $escapedPath = escapeshellarg($path); + $result = instant_remote_process([ + "base=\$(realpath -m -- {$escapedBase}) && target=\$(realpath -m -- {$escapedPath}) && case \"\$target\" in \"\$base\"|\"\$base\"/*) echo OK ;; *) echo NOK ;; esac", + ], $server, false); + + if (trim((string) $result) !== 'OK') { + throw new \RuntimeException('Invalid storage path: resolved path must stay inside the resource configuration directory.'); + } + } + + /** + * Raw Compose bind mounts keep administrator-selected host path semantics. + */ + protected function isAdminControlledComposeMount(): bool + { + $compose = data_get($this->resource, 'docker_compose_raw') + ?? data_get($this->resource, 'service.docker_compose_raw'); + + if (! is_string($compose) || $compose === '') { + return false; + } + + try { + $services = data_get(Yaml::parse($compose), 'services', []); + foreach ($services as $service) { + foreach (data_get($service, 'volumes', []) as $volume) { + if (is_string($volume)) { + $parsed = parseDockerVolumeString($volume); + $source = data_get($parsed, 'source'); + $target = data_get($parsed, 'target'); + } else { + $source = data_get($volume, 'source'); + $target = data_get($volume, 'target'); + } + + if ((string) $target !== $this->mount_path || ! sourceIsLocal(str((string) $source))) { + continue; + } + + $resolvedSource = replaceLocalSource(str((string) $source), str($this->resource->workdir())); + if (normalizeUnixPath($resolvedSource->value()) === normalizeUnixPath($this->fs_path)) { + return true; + } + } + } + } catch (\Throwable) { + return false; + } + + return false; + } + // Accessor for convenient access protected function plainMountPath(): Attribute { diff --git a/tests/Feature/FileStorageMountPathTest.php b/tests/Feature/FileStorageMountPathTest.php index cd301c2557..fb83da1990 100644 --- a/tests/Feature/FileStorageMountPathTest.php +++ b/tests/Feature/FileStorageMountPathTest.php @@ -17,7 +17,9 @@ use App\Models\Team; use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Bus; +use Illuminate\Support\Facades\Crypt; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Process; use Illuminate\Support\Str; use Livewire\Livewire; @@ -35,7 +37,7 @@ beforeEach(function () { $keyId = DB::table('private_keys')->insertGetId([ 'uuid' => (string) Str::uuid(), 'name' => 'Test Key', - 'private_key' => 'test-key', + 'private_key' => Crypt::encryptString('test-key'), 'team_id' => $this->team->id, 'created_at' => now(), 'updated_at' => now(), @@ -45,6 +47,7 @@ beforeEach(function () { 'team_id' => $this->team->id, 'private_key_id' => $keyId, ]); + Process::fake(fn () => Process::result(output: 'OK')); StandaloneDocker::withoutEvents(function () { $this->destination = StandaloneDocker::firstOrCreate( @@ -84,6 +87,76 @@ test('livewire file storage rejects parent segments and does not create a local expect(LocalFileVolume::query()->count())->toBe(0); }); +test('livewire directory storage rejects a server absolute path and has no side effects', function () { + Livewire::test(Storage::class, ['resource' => $this->application]) + ->set('file_storage_directory_source', '/root/.ssh') + ->set('file_storage_directory_destination', '/data') + ->call('submitFileStorageDirectory') + ->assertDispatched('error'); + + expect(LocalFileVolume::query()->count())->toBe(0); + Bus::assertNotDispatched(ServerStorageSaveJob::class); +}); + +test('livewire directory storage accepts a path inside the application root', function () { + $source = application_configuration_dir().'/'.$this->application->uuid.'/data/new'; + + $component = Livewire::test(Storage::class, ['resource' => $this->application]) + ->set('file_storage_directory_source', $source) + ->set('file_storage_directory_destination', '/data') + ->call('submitFileStorageDirectory'); + $component->assertDispatched('success'); + + expect(LocalFileVolume::query()->sole()->fs_path)->toBe($source); +}); + +test('livewire directory storage rejects a remote symlink escape without side effects', function () { + Process::fake(fn () => Process::result(output: 'NOK')); + + Livewire::test(Storage::class, ['resource' => $this->application]) + ->set('file_storage_directory_source', $this->application->workdir().'/linked/outside') + ->set('file_storage_directory_destination', '/data') + ->call('submitFileStorageDirectory') + ->assertDispatched('error'); + + expect(LocalFileVolume::query()->count())->toBe(0); + Bus::assertNotDispatched(ServerStorageSaveJob::class); +}); + +test('normal team members cannot create directory storage', function () { + $member = User::factory()->create(); + $member->teams()->attach($this->team, ['role' => 'member']); + $this->actingAs($member); + session(['currentTeam' => $this->team]); + + Livewire::test(Storage::class, ['resource' => $this->application]) + ->set('file_storage_directory_source', application_configuration_dir().'/'.$this->application->uuid.'/data') + ->set('file_storage_directory_destination', '/data') + ->call('submitFileStorageDirectory') + ->assertDispatched('error'); + + expect(LocalFileVolume::query()->count())->toBe(0); +}); + +test('an administrator cannot create storage on a cross-team resource', function () { + $otherTeam = Team::factory()->create(); + $otherProject = Project::factory()->create(['team_id' => $otherTeam->id]); + $otherEnvironment = Environment::factory()->create(['project_id' => $otherProject->id]); + $otherApplication = Application::factory()->create([ + 'environment_id' => $otherEnvironment->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + ]); + + Livewire::test(Storage::class, ['resource' => $otherApplication]) + ->set('file_storage_directory_source', application_configuration_dir().'/'.$otherApplication->uuid.'/data') + ->set('file_storage_directory_destination', '/data') + ->call('submitFileStorageDirectory') + ->assertDispatched('error'); + + expect(LocalFileVolume::query()->count())->toBe(0); +}); + test('file mount modal shows the calculated host file path above the destination input', function () { Livewire::test(Storage::class, ['resource' => $this->application]) ->assertSeeText('Create a managed file on the host and mount it inside the container.') diff --git a/tests/Feature/StorageApiTest.php b/tests/Feature/StorageApiTest.php index aadcd163b5..30d4849b1c 100644 --- a/tests/Feature/StorageApiTest.php +++ b/tests/Feature/StorageApiTest.php @@ -16,6 +16,9 @@ use App\Models\Team; use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Bus; +use Illuminate\Support\Facades\Crypt; +use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Process; use Illuminate\Support\Str; uses(RefreshDatabase::class); @@ -39,6 +42,16 @@ beforeEach(function () { $this->bearerToken = $token->getKey().'|'.$plainTextToken; $this->server = Server::factory()->create(['team_id' => $this->team->id]); + $keyId = DB::table('private_keys')->insertGetId([ + 'uuid' => (string) Str::uuid(), + 'name' => 'Storage Test Key', + 'private_key' => Crypt::encryptString('test-key'), + 'team_id' => $this->team->id, + 'created_at' => now(), + 'updated_at' => now(), + ]); + $this->server->update(['private_key_id' => $keyId]); + Process::fake(fn () => Process::result(output: 'OK')); $this->destination = StandaloneDocker::where('server_id', $this->server->id)->first(); $this->project = Project::factory()->create(['team_id' => $this->team->id]); $this->environment = Environment::factory()->create(['project_id' => $this->project->id]); @@ -89,6 +102,7 @@ function createTestServiceApplication($context): array { $service = Service::factory()->create([ 'environment_id' => $context->environment->id, + 'server_id' => $context->server->id, 'destination_id' => $context->destination->id, 'destination_type' => $context->destination->getMorphClass(), ]); @@ -170,6 +184,22 @@ describe('GET /api/v1/applications/{uuid}/storages', function () { }); describe('POST /api/v1/applications/{uuid}/storages', function () { + test('rejects an application directory mount outside its managed root without side effects', function () { + $app = createTestApplication($this); + + $this->withHeaders([ + 'Authorization' => 'Bearer '.$this->bearerToken, + ])->postJson("/api/v1/applications/{$app->uuid}/storages", [ + 'type' => 'file', + 'is_directory' => true, + 'fs_path' => '/root/.ssh/authorized_keys', + 'mount_path' => '/data', + ])->assertUnprocessable(); + + expect($app->fileStorages()->exists())->toBeFalse(); + Bus::assertNotDispatched(ServerStorageSaveJob::class); + }); + test('creates a persistent storage', function () { $app = createTestApplication($this); @@ -655,6 +685,63 @@ test('rejects host paths when creating persistent storage through the API', func expect($resource->persistentStorages()->count())->toBe($storageCountBefore); })->with(['application', 'database', 'service']); +test('rejects directory mounts outside each resource configuration root', function (string $resourceType) { + if ($resourceType === 'database') { + $resource = createTestDatabase($this); + $url = "/api/v1/databases/{$resource->uuid}/storages"; + $payload = []; + } else { + [$service, $resource] = createTestServiceApplication($this); + $url = "/api/v1/services/{$service->uuid}/storages"; + $payload = ['resource_uuid' => $resource->uuid]; + } + + $this->withHeaders([ + 'Authorization' => 'Bearer '.$this->bearerToken, + ])->postJson($url, [...$payload, ...[ + 'type' => 'file', + 'is_directory' => true, + 'fs_path' => '/etc/shadow', + 'mount_path' => '/data', + ]])->assertUnprocessable(); + + expect($resource->fileStorages()->exists())->toBeFalse(); + Bus::assertNotDispatched(ServerStorageSaveJob::class); +})->with(['database', 'service']); + +test('rejects a directory mount through a remote symlink before creating storage', function (string $resourceType) { + if ($resourceType === 'application') { + $resource = createTestApplication($this); + $url = "/api/v1/applications/{$resource->uuid}/storages"; + $payload = []; + $base = application_configuration_dir().'/'.$resource->uuid; + } elseif ($resourceType === 'database') { + $resource = createTestDatabase($this); + $url = "/api/v1/databases/{$resource->uuid}/storages"; + $payload = []; + $base = database_configuration_dir().'/'.$resource->uuid; + } else { + [$service, $resource] = createTestServiceApplication($this); + $url = "/api/v1/services/{$service->uuid}/storages"; + $payload = ['resource_uuid' => $resource->uuid]; + $base = service_configuration_dir().'/'.$service->uuid; + } + + Process::fake(fn () => Process::result(output: 'NOK')); + + $response = $this->withHeaders(['Authorization' => 'Bearer '.$this->bearerToken]) + ->postJson($url, [...$payload, ...[ + 'type' => 'file', + 'is_directory' => true, + 'fs_path' => $base.'/linked/outside', + 'mount_path' => '/data', + ]]); + $response->assertUnprocessable(); + + expect($resource->fileStorages()->exists())->toBeFalse(); + Bus::assertNotDispatched(ServerStorageSaveJob::class); +})->with(['application', 'database', 'service']); + test('rejects host paths when updating persistent storage through the API', function (string $resourceType) { if ($resourceType === 'application') { $resource = createTestApplication($this); diff --git a/tests/Unit/FileStorageSecurityTest.php b/tests/Unit/FileStorageSecurityTest.php index b5e1c74042..0623c9ac72 100644 --- a/tests/Unit/FileStorageSecurityTest.php +++ b/tests/Unit/FileStorageSecurityTest.php @@ -204,7 +204,8 @@ test('confined path resolver rejects paths that escape the resource configuratio test('local file volume write sink keeps saved managed file paths for compatibility', function () { $source = file_get_contents(__DIR__.'/../../app/Models/LocalFileVolume.php'); - expect($source)->not->toContain('confinePathToBase($workdir, $path->value(), \'storage path\')') + expect($source)->toContain('confinePathToBase($workdir, $path->value(), \'storage path\')') + ->and($source)->toContain('assertRemotePathIsConfined') ->and($source)->toContain('tee {$escapedPath}'); });