From afde2cb47155890e91cb317c720403e35908934f Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 26 Oct 2026 12:45:36 +0100 Subject: [PATCH 1/3] fix(deployments): write generated compose file after image build (#12331) --- app/Jobs/ApplicationDeploymentJob.php | 9 ++ .../DeploymentSensitiveCommandFailureTest.php | 3 +- ...eneratedComposeOutsideBuildContextTest.php | 101 ++++++++++++++++++ 3 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 tests/Feature/GeneratedComposeOutsideBuildContextTest.php diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index cf206973c6..658dc2f131 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -2717,6 +2717,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue { try { $this->checkForCancellation(); + $this->write_compose_file_to_workdir(); if ($this->server->isSwarm()) { $this->application_deployment_queue->addLogEntry('Rolling update started.'); $this->execute_remote_command( @@ -4467,6 +4468,14 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); $this->docker_compose = Yaml::dump($docker_compose, 10); $this->generated_docker_compose = $this->docker_compose; + } + + /** + * The workdir is the build context, so the generated compose file is written only after the image + * build. Otherwise `COPY . .`, Nixpacks, and Railpack put it (with the proxy labels) into the image. + */ + private function write_compose_file_to_workdir(): void + { $this->execute_remote_command([ "docker exec {$this->deployment_uuid} rm -f {$this->workdir}/docker-compose.yaml", 'hidden' => true, diff --git a/tests/Feature/DeploymentSensitiveCommandFailureTest.php b/tests/Feature/DeploymentSensitiveCommandFailureTest.php index c0c4882168..fe7346a5cd 100644 --- a/tests/Feature/DeploymentSensitiveCommandFailureTest.php +++ b/tests/Feature/DeploymentSensitiveCommandFailureTest.php @@ -457,7 +457,8 @@ it('streams the compose file over stdin so its content stays out of commands, er return Process::result(); }); - $exception = captureDeploymentException(fn () => invokeSensitiveFailureJobMethod($job, $reflection, 'generate_compose_file')); + invokeSensitiveFailureJobMethod($job, $reflection, 'generate_compose_file'); + $exception = captureDeploymentException(fn () => invokeSensitiveFailureJobMethod($job, $reflection, 'write_compose_file_to_workdir')); $compose = (string) readSensitiveFailureJobProperty($job, $reflection, 'docker_compose'); expect($composeWrite)->not->toBeNull() diff --git a/tests/Feature/GeneratedComposeOutsideBuildContextTest.php b/tests/Feature/GeneratedComposeOutsideBuildContextTest.php new file mode 100644 index 0000000000..4b3f1413db --- /dev/null +++ b/tests/Feature/GeneratedComposeOutsideBuildContextTest.php @@ -0,0 +1,101 @@ + InstanceSettings::firstOrCreate(['id' => 0])); + config(['constants.ssh.mux_enabled' => false]); + + $team = Team::factory()->create(); + $server = Server::factory()->create([ + 'team_id' => $team->id, + 'private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id, + ]); + $destination = StandaloneDocker::query()->where('server_id', $server->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + $this->application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + 'build_pack' => 'dockerfile', + 'dockerfile' => "FROM nginx:alpine\nCOPY . /usr/share/nginx/html", + ]); + $this->deployment = ApplicationDeploymentQueue::create([ + 'application_id' => $this->application->id, + 'deployment_uuid' => 'compose-outside-build-context', + 'status' => 'in_progress', + 'server_id' => $server->id, + 'destination_id' => $destination->id, + 'commit' => 'HEAD', + 'pull_request_id' => 0, + ]); + + $this->commands = collect(); + Process::fake(function (PendingProcess $process) { + $command = (string) $process->command; + $this->commands->push($command); + + return Process::result(output: match (true) { + str_contains($command, 'State.Health.Status') => "\"healthy\"\n", + str_contains($command, 'echo available') => "available\n", + default => '', + }); + }); + Queue::fake(); + Notification::fake(); + Sleep::fake(); +}); + +it('writes the generated compose file to the workdir only after the image build', function (string $buildPack, string $deployMethod) { + // Application clears the inline Dockerfile when the build pack changes away from dockerfile. + $this->application->update(['build_pack' => $buildPack]); + $job = new ApplicationDeploymentJob($this->deployment->id); + // handle() detects buildx on the server before it starts a deployment. + (new ReflectionProperty($job, 'dockerBuildxAvailable'))->setValue($job, true); + try { + (new ReflectionMethod($job, $deployMethod))->invoke($job); + } catch (Throwable) { + // The faked server stops the deployment after the start; only the order of the commands matters. + } + + $buildIndex = $this->commands->search(fn (string $command) => str_contains($command, 'bash /artifacts/build.sh')); + $composeWriteIndex = $this->commands->search(fn (string $command) => str_contains($command, 'tee /artifacts/compose-outside-build-context/docker-compose.yaml')); + + if ($buildPack === 'nixpacks') { + expect($this->commands->contains(fn (string $command) => str_contains($command, 'nixpacks build')))->toBeTrue(); + } + + expect($buildIndex)->not->toBeFalse() + ->and($composeWriteIndex)->not->toBeFalse() + ->and($composeWriteIndex)->toBeGreaterThan($buildIndex); +})->with([ + 'inline Dockerfile' => ['dockerfile', 'deploy_simple_dockerfile'], + 'Dockerfile' => ['dockerfile', 'deploy_dockerfile_buildpack'], + 'Nixpacks' => ['nixpacks', 'deploy_nixpacks_buildpack'], + 'Railpack' => ['railpack', 'deploy_railpack_buildpack'], + 'static' => ['static', 'deploy_static_buildpack'], +]); From 435645ce1501735c8c80d9d300b595bbe013ad3c Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 26 Oct 2026 12:54:11 +0100 Subject: [PATCH 2/3] fix(api): write file storage content to server on PATCH (#12248) --- .../Api/ApplicationsController.php | 9 ++- .../RequiresDeployForOutsideHostPaths.php | 9 ++- .../Controllers/Api/DatabasesController.php | 9 ++- .../Controllers/Api/ServicesController.php | 9 ++- openapi.json | 6 +- openapi.yaml | 6 +- .../Api/StorageContentDeployAbilityTest.php | 61 +++++++++++++++++++ tests/Feature/StorageHostPathTest.php | 2 +- 8 files changed, 95 insertions(+), 16 deletions(-) diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index c800c8d94f..61bd2bd6f2 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -10,6 +10,7 @@ use App\Enums\BuildPackTypes; use App\Enums\HttpBasicAuthHashAlgorithm; use App\Http\Controllers\Controller; use App\Jobs\DeleteResourceJob; +use App\Jobs\ServerStorageSaveJob; use App\Models\Application; use App\Models\ApplicationPreview; use App\Models\ApplicationSetting; @@ -5202,7 +5203,7 @@ class ApplicationsController extends Controller 'is_preview_suffix_enabled' => ['type' => 'boolean', 'description' => 'Whether to add -pr-N suffix for preview deployments.'], 'name' => ['type' => 'string', 'description' => 'The volume name (persistent only, not allowed for read-only storages).'], 'mount_path' => ['type' => 'string', 'description' => 'The container mount path (not allowed for read-only storages).'], - 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission.'], + 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages). Changing the content needs the deploy permission.'], ], additionalProperties: false, ), @@ -5337,7 +5338,7 @@ class ApplicationsController extends Controller } } - if (! $isReadOnly && ($forbidden = $this->outsideContentChangeForbiddenResponse($request, $storage))) { + if (! $isReadOnly && ($forbidden = $this->contentChangeForbiddenResponse($request, $storage))) { return $forbidden; } @@ -5367,6 +5368,10 @@ class ApplicationsController extends Controller $storage->save(); + if ($storage instanceof LocalFileVolume && $storage->wasChanged('content')) { + ServerStorageSaveJob::dispatch($storage); + } + auditLog('api.application.storage_updated', [ 'team_id' => $teamId, 'application_uuid' => $application->uuid, diff --git a/app/Http/Controllers/Api/Concerns/RequiresDeployForOutsideHostPaths.php b/app/Http/Controllers/Api/Concerns/RequiresDeployForOutsideHostPaths.php index fe8934cec7..9b862ef18d 100644 --- a/app/Http/Controllers/Api/Concerns/RequiresDeployForOutsideHostPaths.php +++ b/app/Http/Controllers/Api/Concerns/RequiresDeployForOutsideHostPaths.php @@ -24,15 +24,18 @@ trait RequiresDeployForOutsideHostPaths return $this->deployAbilityForbiddenResponse($request, 'A mount outside the resource directory needs a token with the deploy permission.'); } - private function outsideContentChangeForbiddenResponse(Request $request, LocalFileVolume|LocalPersistentVolume $storage): ?JsonResponse + /** + * Coolify writes changed file content on the server immediately, also inside the resource directory. + */ + private function contentChangeForbiddenResponse(Request $request, LocalFileVolume|LocalPersistentVolume $storage): ?JsonResponse { - if (! $storage instanceof LocalFileVolume || $storage->is_directory || ! $request->has('content')) { + if (! $storage instanceof LocalFileVolume || $storage->is_directory || $storage->is_host_file || ! $request->has('content')) { return null; } if ((string) $request->input('content') === (string) $storage->content) { return null; } - return $this->outsideHostPathForbiddenResponse($request, $storage); + return $this->deployAbilityForbiddenResponse($request, 'Coolify writes the file content on the server immediately, so it needs a token with the deploy permission.'); } } diff --git a/app/Http/Controllers/Api/DatabasesController.php b/app/Http/Controllers/Api/DatabasesController.php index 45a2ad54f3..41ee9ffb7d 100644 --- a/app/Http/Controllers/Api/DatabasesController.php +++ b/app/Http/Controllers/Api/DatabasesController.php @@ -11,6 +11,7 @@ use App\Enums\NewDatabaseTypes; use App\Http\Controllers\Controller; use App\Jobs\DatabaseBackupJob; use App\Jobs\DeleteResourceJob; +use App\Jobs\ServerStorageSaveJob; use App\Jobs\VolumeCloneJob; use App\Models\EnvironmentVariable; use App\Models\LocalFileVolume; @@ -4792,7 +4793,7 @@ class DatabasesController extends Controller 'is_preview_suffix_enabled' => ['type' => 'boolean', 'description' => 'Whether to add -pr-N suffix for preview deployments.'], 'name' => ['type' => 'string', 'description' => 'The volume name (persistent only, not allowed for read-only storages).'], 'mount_path' => ['type' => 'string', 'description' => 'The container mount path (not allowed for read-only storages).'], - 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission.'], + 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages). Changing the content needs the deploy permission.'], ], additionalProperties: false, ), @@ -4924,7 +4925,7 @@ class DatabasesController extends Controller } } - if (! $isReadOnly && ($forbidden = $this->outsideContentChangeForbiddenResponse($request, $storage))) { + if (! $isReadOnly && ($forbidden = $this->contentChangeForbiddenResponse($request, $storage))) { return $forbidden; } @@ -4954,6 +4955,10 @@ class DatabasesController extends Controller $storage->save(); + if ($storage instanceof LocalFileVolume && $storage->wasChanged('content')) { + ServerStorageSaveJob::dispatch($storage); + } + auditLog('api.database.storage_updated', [ 'team_id' => $teamId, 'database_uuid' => $database->uuid, diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index aa4ea63110..1e5231400a 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -7,6 +7,7 @@ use App\Actions\Service\StartService; use App\Actions\Service\StopService; use App\Http\Controllers\Controller; use App\Jobs\DeleteResourceJob; +use App\Jobs\ServerStorageSaveJob; use App\Models\EnvironmentVariable; use App\Models\LocalFileVolume; use App\Models\LocalPersistentVolume; @@ -2793,7 +2794,7 @@ class ServicesController extends Controller 'is_preview_suffix_enabled' => ['type' => 'boolean', 'description' => 'Whether to add -pr-N suffix for preview deployments.'], 'name' => ['type' => 'string', 'description' => 'The volume name (persistent only, not allowed for read-only storages).'], 'mount_path' => ['type' => 'string', 'description' => 'The container mount path (not allowed for read-only storages).'], - 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission.'], + 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages). Changing the content needs the deploy permission.'], ], additionalProperties: false, ), @@ -2955,7 +2956,7 @@ class ServicesController extends Controller } } - if (! $isReadOnly && ($forbidden = $this->outsideContentChangeForbiddenResponse($request, $storage))) { + if (! $isReadOnly && ($forbidden = $this->contentChangeForbiddenResponse($request, $storage))) { return $forbidden; } @@ -2985,6 +2986,10 @@ class ServicesController extends Controller $storage->save(); + if ($storage instanceof LocalFileVolume && $storage->wasChanged('content')) { + ServerStorageSaveJob::dispatch($storage); + } + auditLog('api.service.storage_updated', [ 'team_id' => $teamId, 'service_uuid' => $service->uuid, diff --git a/openapi.json b/openapi.json index 7af9db1b47..4ed696c239 100644 --- a/openapi.json +++ b/openapi.json @@ -5449,7 +5449,7 @@ "content": { "type": "string", "nullable": true, - "description": "The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission." + "description": "The file content (file only, not allowed for read-only storages). Changing the content needs the deploy permission." } }, "type": "object", @@ -10340,7 +10340,7 @@ "content": { "type": "string", "nullable": true, - "description": "The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission." + "description": "The file content (file only, not allowed for read-only storages). Changing the content needs the deploy permission." } }, "type": "object", @@ -21789,7 +21789,7 @@ "content": { "type": "string", "nullable": true, - "description": "The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission." + "description": "The file content (file only, not allowed for read-only storages). Changing the content needs the deploy permission." } }, "type": "object", diff --git a/openapi.yaml b/openapi.yaml index 4018755766..f1f4987405 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -3586,7 +3586,7 @@ paths: content: type: string nullable: true - description: 'The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission.' + description: 'The file content (file only, not allowed for read-only storages). Changing the content needs the deploy permission.' type: object additionalProperties: false responses: @@ -6779,7 +6779,7 @@ paths: content: type: string nullable: true - description: 'The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission.' + description: 'The file content (file only, not allowed for read-only storages). Changing the content needs the deploy permission.' type: object additionalProperties: false responses: @@ -13753,7 +13753,7 @@ paths: content: type: string nullable: true - description: 'The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission.' + description: 'The file content (file only, not allowed for read-only storages). Changing the content needs the deploy permission.' type: object additionalProperties: false responses: diff --git a/tests/Feature/Api/StorageContentDeployAbilityTest.php b/tests/Feature/Api/StorageContentDeployAbilityTest.php index ae76e45dc4..bbf726910c 100644 --- a/tests/Feature/Api/StorageContentDeployAbilityTest.php +++ b/tests/Feature/Api/StorageContentDeployAbilityTest.php @@ -111,3 +111,64 @@ test('write and deploy token creates a file storage with content', function (str expect($resource->fileStorages()->where('mount_path', '/etc/app.conf')->value('content'))->toBe('listen 8080;'); Queue::assertPushed(ServerStorageSaveJob::class); })->with(['applications', 'services', 'databases']); + +function createSavedFileStorage(mixed $resource): LocalFileVolume +{ + $storage = LocalFileVolume::create([ + 'fs_path' => '/data/coolify/app.conf', + 'mount_path' => '/etc/app.conf', + 'content' => 'listen 80;', + 'resource_id' => $resource->id, + 'resource_type' => $resource->getMorphClass(), + ]); + Queue::fake(); + + return $storage; +} + +test('updating file storage content writes the file on the server', function (string $type) { + [$url, , $resource] = $this->targets[$type]; + $storage = createSavedFileStorage($resource); + + $this->withHeaders(storageContentAbilityHeaders($this->user, $this->team->id, ['write', 'deploy'])) + ->patchJson($url, ['uuid' => $storage->uuid, 'type' => 'file', 'content' => 'listen 8080;']) + ->assertOk(); + + expect($storage->fresh()->content)->toBe('listen 8080;'); + Queue::assertPushed(ServerStorageSaveJob::class, fn (ServerStorageSaveJob $job) => $job->localFileVolume->is($storage)); +})->with(['applications', 'services', 'databases']); + +test('write-only token cannot change file storage content', function (string $type) { + [$url, , $resource] = $this->targets[$type]; + $storage = createSavedFileStorage($resource); + + $this->withHeaders(storageContentAbilityHeaders($this->user, $this->team->id, ['write'])) + ->patchJson($url, ['uuid' => $storage->uuid, 'type' => 'file', 'content' => 'listen 8080;']) + ->assertForbidden() + ->assertJsonPath('message', 'Missing required permissions: deploy. Coolify writes the file content on the server immediately, so it needs a token with the deploy permission.'); + + expect($storage->fresh()->content)->toBe('listen 80;'); + Queue::assertNotPushed(ServerStorageSaveJob::class); +})->with(['applications', 'services', 'databases']); + +test('write-only token can update a file storage without changing its content', function (string $type) { + [$url, , $resource] = $this->targets[$type]; + $storage = createSavedFileStorage($resource); + + $this->withHeaders(storageContentAbilityHeaders($this->user, $this->team->id, ['write'])) + ->patchJson($url, ['uuid' => $storage->uuid, 'type' => 'file', 'content' => 'listen 80;', 'mount_path' => '/etc/app2.conf']) + ->assertOk(); + + expect($storage->fresh()->mount_path)->toBe('/etc/app2.conf'); +})->with(['applications', 'services', 'databases']); + +test('updating a file storage without changing its content does not write the file', function (string $type) { + [$url, , $resource] = $this->targets[$type]; + $storage = createSavedFileStorage($resource); + + $this->withHeaders(storageContentAbilityHeaders($this->user, $this->team->id, ['write', 'deploy'])) + ->patchJson($url, ['uuid' => $storage->uuid, 'type' => 'file', 'content' => 'listen 80;', 'is_preview_suffix_enabled' => false]) + ->assertOk(); + + Queue::assertNotPushed(ServerStorageSaveJob::class); +})->with(['applications', 'services', 'databases']); diff --git a/tests/Feature/StorageHostPathTest.php b/tests/Feature/StorageHostPathTest.php index 3f60abf56b..37239aa74d 100644 --- a/tests/Feature/StorageHostPathTest.php +++ b/tests/Feature/StorageHostPathTest.php @@ -436,7 +436,7 @@ test('changing the content of a mount outside the resource directory needs deplo $this->withHeaders(['Authorization' => 'Bearer '.storageHostPathToken($this->admin, ['read', 'write'])]) ->patchJson($url, ['type' => 'file', 'uuid' => $storage->uuid, 'content' => 'ssh-ed25519 AAAA']) ->assertForbidden() - ->assertJsonPath('message', 'Missing required permissions: deploy. A mount outside the resource directory needs a token with the deploy permission.'); + ->assertJsonPath('message', 'Missing required permissions: deploy. Coolify writes the file content on the server immediately, so it needs a token with the deploy permission.'); expect($storage->fresh()->content)->toBe('listen 8080;'); $this->withHeaders(['Authorization' => 'Bearer '.storageHostPathToken($this->admin, ['read', 'write'])]) From 0a0d6cf0751f7169d480b6b9017de4bad2e264a2 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 26 Oct 2026 12:54:32 +0100 Subject: [PATCH 3/3] fix(postgres): block restore scan bypasses and stop on SQL errors (#12333) Co-authored-by: Claude Opus 5.5 --- app/Support/DatabaseBackupFileValidator.php | 24 ++-- .../DatabaseImportCommandBuilder.php | 35 +++-- .../project/database/import-form.blade.php | 6 +- .../DatabaseBackupUploadValidationTest.php | 135 +++++++++++++++++- .../DatabaseImportCommandBuilderTest.php | 6 +- 5 files changed, 177 insertions(+), 29 deletions(-) diff --git a/app/Support/DatabaseBackupFileValidator.php b/app/Support/DatabaseBackupFileValidator.php index b7052411aa..6e885123f3 100644 --- a/app/Support/DatabaseBackupFileValidator.php +++ b/app/Support/DatabaseBackupFileValidator.php @@ -100,13 +100,24 @@ class DatabaseBackupFileValidator return false; } - $withoutComments = self::stripSqlComments($sql); + // Token separator PostgreSQL treats as whitespace: real whitespace, a block comment + // (the lazy match can extend to the outer end of a nested comment), or a line + // comment. Comment forms also match inside string literals, which can only block + // more input, never less. + $sep = '(?:\s|/\*.*?\*/|--[^\r\n]*[\r\n])'; - if (preg_match('/^\s*\\\\(?:!|copy\b[^\r\n]*\bprogram\b|(?:o|g)\s*\|)/mi', $withoutComments) === 1) { + // psql meta-commands that can run a shell command, anywhere on a line. Same rules as + // the restore scan in DatabaseImportCommandBuilder::buildPostgresRestoreScanScript(). + // A regex error (for example the backtrack limit) returns false and blocks. + if (preg_match('~(?:^|[^\\\\])(?:\\\\\\\\)*\\\\(?:!|(?:o|out|gx?|w|write)[^\S\n][^\\\\\n]*\||copy[^\S\n].*program|(?:setenv|e|edit|ef|ev|i|ir|include|include_relative)(?:\s|$)|(?:[^nbv\s\\\\]|[nbv][^\s\\\\])[^\s\\\\]*[^\S\n][^\\\\\n]*`)~mi', $sql) !== 0) { return true; } - return preg_match('/(?:^|;)\s*copy\b[^;]{0,2000}\b(?:from|to)\s+program\b/i', $withoutComments) === 1; + // A TO/FROM PROGRAM token pair anywhere (dynamic SQL, DO blocks, function bodies), + // a server-side large-object import or export, or ALTER SYSTEM. Comments are no + // longer stripped first, because that cut inside string literals; a commented-out + // PROGRAM statement is now blocked too, which is the safer default. + return preg_match('~(?:from|to)'.$sep.'+program|lo_(?:import|export)'.$sep.'*\(|alter'.$sep.'+system'.$sep.'+(?:set|reset)~is', $sql) !== 0; } private static function extensionFor(string $name): ?string @@ -210,11 +221,4 @@ class DatabaseBackupFileValidator return $documentLength >= 5 && $documentLength <= $fileSize; } - - private static function stripSqlComments(string $sql): string - { - $sql = preg_replace('/\/\*[\s\S]*?\*\//', ' ', $sql) ?? $sql; - - return preg_replace('/--[^\r\n]*/', ' ', $sql) ?? $sql; - } } diff --git a/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php b/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php index 61c4d2af9e..4535420e6f 100644 --- a/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php +++ b/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php @@ -109,19 +109,36 @@ SH; $escapedPath = escapeshellarg($path); - // Token separator PostgreSQL treats as whitespace: real whitespace or a - // /* ... */ block comment (used to split keywords like FROM/**/PROGRAM). - $sep = '([[:space:]]|/\\*[^*]*\\*/)'; + // Token separator PostgreSQL treats as whitespace: real whitespace, a /* ... */ + // block comment (also nested or holding asterisks), or a -- comment through the + // line end. The joined scan maps line ends to '~' (after it deletes every real + // '~'), so a -- comment ends at the next '~'. Comment forms also match inside + // string literals, which can only block more input, never less. + $sep = '([[:space:]]|~|/\\*.*\\*/|--[^~]*~)'; - $sqlPattern = "(^|;){$sep}*copy{$sep}+[^;]*(from|to){$sep}+program"; - $psqlPattern = "^{$sep}*\\\\(!|copy{$sep}+[^[:space:]]+.*{$sep}+program|(o|g){$sep}*\\|)"; + // A TO/FROM PROGRAM token pair anywhere (so it is also found inside DO blocks, + // EXECUTE strings and function bodies), a server-side large-object import or + // export, or ALTER SYSTEM. The pair is matched without the former (^|;) anchor + // and [^;]* limit, which a ';' inside a string literal or dynamic SQL bypassed. + $sqlPattern = "(from|to){$sep}+program|lo_(import|export){$sep}*\\(|alter{$sep}+system{$sep}+(set|reset)"; + // psql meta-commands that can run a shell command, anywhere on a line (a meta-command + // may follow SQL without a ';'). An odd backslash run starts a meta-command; COPY data + // writes a literal backslash as "\\", so its rows do not match. Blocked: \!, output + // piped by \o/\out/\g/\gx/\w/\write, \copy ... program, \setenv and the editor commands, + // the include commands (they run a file that \o or \copy wrote), and backquotes in the + // arguments of a command. \n, \b, \v and \N are COPY escapes that psql does not know, + // and psql does not expand backquotes for unknown commands. + $psqlPattern = '(^|[^\\\\])(\\\\\\\\)*\\\\(!|(o|out|gx?|w|write)[[:space:]][^\\\\]*\||copy[[:space:]].*program|(setenv|e|edit|ef|ev|i|ir|include|include_relative)([[:space:]]|$)|([^nbv[:space:]\\\\]|[nbv][^[:space:]\\\\])[^[:space:]\\\\]*[[:space:]][^\\\\]*`)'; $escapedSqlPattern = escapeshellarg($sqlPattern); $escapedPsqlPattern = escapeshellarg($psqlPattern); $contents = "{ gunzip -cf {$escapedPath} 2>/dev/null || cat {$escapedPath}; }"; - $scan = static fn (string $source): string => "{$source} | sed 's/--.*//' | grep -Eiq {$escapedPsqlPattern} || {$source} | sed 's/--.*//' | tr '\\n\\r\\t' ' ' | grep -Eiq {$escapedSqlPattern}"; + // Comments are no longer stripped: deleting the rest of a line on "--" also cut + // inside string literals, which let "SELECT '--'; COPY ... PROGRAM" slip through. + // A commented-out PROGRAM statement is now blocked too, which is the safer default. + $scan = static fn (string $source): string => "{$source} | LC_ALL=C grep -Eiq {$escapedPsqlPattern} || {$source} | tr -d '~' | tr '\\n\\r' '~~' | LC_ALL=C grep -Eiq {$escapedSqlPattern}"; $customScan = $scan('pg_restore -f - "$inspect" 2>/dev/null'); $sqlScan = $scan($contents); - $blockedProgram = 'echo \'Blocked PostgreSQL restore: COPY ... PROGRAM and psql shell commands are not allowed.\'; exit 1'; + $blockedProgram = 'echo \'Blocked PostgreSQL restore: the backup contains a COPY ... PROGRAM, psql shell command, lo_import/lo_export, or ALTER SYSTEM pattern. If you trust this dump, restore it from the database container terminal.\'; exit 1'; $blockedInspect = 'echo \'Blocked PostgreSQL restore: unable to inspect custom archive.\'; exit 1'; return <<
@if ($resourceDbType === 'standalone-postgresql' && $dumpAll) - - The backup replaces PostgreSQL administrator role passwords, including the destination administrator password. - If the administrator password changes, update it in Coolify's database configuration after the restore. + + Restoring all databases needs a PostgreSQL superuser (it recreates databases and roles), so Coolify cannot restrict it. Only use a backup you trust. + The backup also replaces PostgreSQL administrator role passwords, including the destination administrator password. If the administrator password changes, update it in Coolify's database configuration after the restore. @endif ["COPY pwned FROM PROGRAM 'id';"], 'copy to program' => ["COPY pwned TO PROGRAM 'cat > /tmp/out';"], 'copy with block comment' => ["COPY pwned FROM/**/PROGRAM 'id';"], + 'copy with asterisks in block comment' => ["COPY pwned FROM/*a*b*/PROGRAM 'id';"], + 'copy with nested block comment' => ["COPY pwned FROM /* a /* b */ c */ PROGRAM 'id';"], + 'copy with line comment' => ["COPY pwned FROM--note\nPROGRAM 'id';"], + 'copy with mixed comments' => ["COPY pwned FROM /* -- */\n-- c\r\nPROGRAM 'id';"], 'psql shell command' => ["\\! id\n"], 'psql copy program' => ["\\copy pwned from program 'id'\n"], + 'psql shell command after statement' => ["SELECT 1; \\! id\n"], + 'psql write pipe' => ["\\w |id\n"], + 'program inside DO block' => ["DO \$\$ BEGIN COPY t TO PROGRAM 'id'; END \$\$;"], + 'program inside EXECUTE string' => ["DO \$\$ BEGIN EXECUTE 'COPY t TO PROGRAM ''id'''; END \$\$;"], + 'program inside function body' => ["CREATE FUNCTION f() RETURNS void AS 'COPY t TO PROGRAM ''id''' LANGUAGE sql;"], + 'copy with semicolon in string' => ["COPY (SELECT ';') TO PROGRAM 'id';"], + 'program after string holding comment' => ["SELECT '--'; COPY t TO PROGRAM 'id';"], + 'large object import' => ["SELECT lo_import('/etc/passwd');"], + 'alter system' => ["ALTER SYSTEM SET archive_command = 'id';"], ]); test('postgresql backup safety scanner allows ordinary sql dumps', function () { @@ -237,12 +251,36 @@ test('file scanner allows custom format archives without program execution', fun expect(DatabaseBackupFileValidator::fileContainsPostgresqlProgramExecution($archive))->toBeFalse(); }); -test('postgresql backup safety scanner allows copy words in table data', function () { - $dump = "COPY notes FROM stdin;\n1\tcopy files from program storage\n\\.\n"; +test('postgresql backup safety scanner allows copy from stdin data', function () { + $dump = "COPY notes FROM stdin;\n1\tTaylor wrote a note\n\\.\n"; expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($dump))->toBeFalse(); }); +// Decided trade-off: an unanchored "TO/FROM PROGRAM" match cannot tell a real COPY +// directive from the same two words in a data row, and we keep it unanchored so it +// also catches dynamic SQL. A dump whose data literally contains "from program" or +// "to program" is therefore blocked, with an error that points to the container +// terminal for trusted dumps. This is rare in practice. +test('postgresql backup safety scanner blocks the program token pair even inside table data', function () { + $dump = "COPY notes FROM stdin;\n1\tcopy files from program storage\n\\.\n"; + + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($dump))->toBeTrue(); +}); + +test('postgresql backup safety scanner blocks program execution hidden in dynamic sql', function (string $payload) { + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($payload))->toBeTrue(); +})->with([ + 'DO block' => ["DO \$\$ BEGIN COPY t TO PROGRAM 'id'; END \$\$;"], + 'EXECUTE string' => ["DO \$\$ BEGIN EXECUTE 'COPY t TO PROGRAM ''id'''; END \$\$;"], + 'function body' => ["CREATE FUNCTION f() RETURNS void AS 'COPY t TO PROGRAM ''id''' LANGUAGE sql;"], + 'semicolon in string' => ["COPY (SELECT ';') TO PROGRAM 'id';"], + 'string holding comment marker' => ["SELECT '--'; COPY t TO PROGRAM 'id';"], + 'lo_import' => ["SELECT lo_import('/etc/passwd');"], + 'alter system' => ["ALTER SYSTEM SET archive_command = 'id';"], + 'mid-statement psql shell' => ["SELECT 1; \\! id\n"], +]); + test('backup validator rejects plaintext .dump containing program execution', function () { $file = makeTemporaryUpload('evil.dump', "COPY x FROM PROGRAM 'id';\n"); @@ -258,13 +296,31 @@ test('remote postgresql scanner blocks bypass payloads', function (string $conte 'psql shell escape' => ["\\! id\n", false], 'copy from program' => ["COPY x FROM PROGRAM 'id';\n", false], 'copy with block comment' => ["COPY x FROM/**/PROGRAM 'id';\n", false], + 'copy with asterisks in block comment' => ["COPY x FROM/*a*b*/PROGRAM 'id';\n", false], + 'copy with nested block comment' => ["COPY x FROM /* a /* b */ c */ PROGRAM 'id';\n", false], + 'copy with line comment' => ["COPY x FROM--note\nPROGRAM 'id';\n", false], + 'copy with marker char in line comment' => ["COPY x FROM -- a~b\nPROGRAM 'id';\n", false], + 'copy with mixed comments' => ["COPY x FROM /* -- */\n-- c\r\nPROGRAM 'id';\n", false], + 'copy with block comment across lines' => ["COPY x FROM /* a\n-- */ PROGRAM 'id';\n", true], 'copy split across lines' => ["COPY x FROM\nPROGRAM 'id';\n", false], 'copy to program' => ["COPY x TO PROGRAM 'cat > /tmp/x';\n", false], 'psql pipe redirect' => ["\\o | id\n", false], 'psql query pipe redirect' => ["\\g | id\n", false], + 'psql write pipe redirect' => ["\\w |id\n", false], + 'psql shell after statement' => ["SELECT 1; \\! id\n", false], 'gzipped comment bypass' => ["COPY x FROM/**/PROGRAM 'id';\n", true], 'custom format archive' => ["PGDMP\0binary COPY records FROM PROGRAM payload", false], 'custom format gzip archive' => ["PGDMP\0binary COPY records FROM PROGRAM payload", true], + // Bypasses of the former (^|;) ... [^;]* COPY-only pattern. + 'program inside DO block' => ["DO \$\$ BEGIN COPY t TO PROGRAM 'id'; END \$\$;\n", false], + 'program inside EXECUTE string' => ["DO \$\$ BEGIN EXECUTE 'COPY t TO PROGRAM ''id'''; END \$\$;\n", false], + 'program inside function body' => ["CREATE FUNCTION f() RETURNS void AS 'COPY t TO PROGRAM ''id''' LANGUAGE sql;\n", false], + 'copy with semicolon in string' => ["COPY (SELECT ';') TO PROGRAM 'id';\n", false], + 'program after string holding comment' => ["SELECT '--'; COPY t TO PROGRAM 'id';\n", false], + 'large object import' => ["SELECT lo_import('/etc/passwd');\n", false], + 'alter system' => ["ALTER SYSTEM SET archive_command = 'id';\n", false], + 'commented out payload is now blocked' => ["-- COPY x FROM PROGRAM 'id'\nSELECT 1;\n", false], + 'program words in table data are blocked' => ["COPY notes FROM stdin;\n1\tcopy files from program storage\n\\.\n", false], ]); test('remote postgresql scanner allows legitimate restores', function (string $content, bool $gzip) { @@ -273,11 +329,38 @@ test('remote postgresql scanner allows legitimate restores', function (string $c expect(scannerBlocks($script))->toBeFalse(); })->with([ - 'commented out payload' => ["-- COPY x FROM PROGRAM 'id'\nSELECT 1;\n", false], 'copy from stdin' => ["COPY users FROM stdin;\n1\tTaylor\n\\.\n", false], 'plain select' => ["SELECT * FROM users;\n", false], 'gzipped clean dump' => ["CREATE TABLE users (id int);\n", true], - 'copy words in table data' => ["COPY notes FROM stdin;\n1\tcopy files from program storage\n\\.\n", false], + 'copy keyword without program' => ["COPY users (id, name) FROM stdin;\n1\tTaylor\n\\.\n", false], + 'program word alone in data' => ["COPY notes FROM stdin;\n1\tfinished the program today\n\\.\n", false], + 'comments between other words' => ["SELECT a FROM /* x */ t -- note\nWHERE b ~ 'program';\n", false], +]); + +test('the postgres safety command survives the non-root sudo parser', function (string $content, bool $blocked) { + $payload = writeScanPayload($content); + + $database = Mockery::mock(StandalonePostgresql::class); + $database->shouldReceive('getMorphClass')->andReturn(StandalonePostgresql::class); + + $server = Mockery::mock(Server::class)->makePartial(); + $server->user = 'ubuntu'; + + $command = (new DatabaseImportCommandBuilder)->buildPostgresSafetyCommand($database, 'scan-container', $payload); + $elevated = parseCommandsByLineForSudo(collect([$command]), $server)[0]; + + expect($elevated)->toStartWith('sudo bash -c ') + ->and($elevated)->not->toContain(' | sudo '); + + // Strip the host elevation and the container indirection so the embedded scan runs + // locally against the payload file; this proves the parser kept the script quoting intact. + $local = str_replace(['sudo bash -c', 'docker exec scan-container sh -c'], ['bash -c', 'sh -c'], $elevated); + + expect(Process::run(['sh', '-c', $local])->exitCode() === 1)->toBe($blocked); +})->with([ + 'blocks program payload' => ["COPY t TO PROGRAM 'id';\n", true], + 'blocks dynamic sql payload' => ["DO \$\$ BEGIN COPY t TO PROGRAM 'id'; END \$\$;\n", true], + 'allows a clean dump' => ["COPY users FROM stdin;\n1\tTaylor\n\\.\n", false], ]); test('remote postgresql scanner inspects custom archives instead of skipping them', function () { @@ -310,3 +393,47 @@ test('ALLOWED_EXTENSIONS does not include executable formats', function () { expect($constant)->not->toContain($bad); } }); + +// psql meta-commands can follow SQL on the same line, so they are matched anywhere after an +// odd backslash run. COPY data writes a literal backslash as "\\" and uses \n, \b, \v and \N, +// which psql does not know as commands, so ordinary dumps must still pass. +dataset('psql shell meta-commands', [ + 'shell escape after sql without semicolon' => ["SELECT 1 \\! id\n"], + 'output long name piped' => ["\\out |id\nSELECT 1;\n"], + 'output piped after sql' => ["SELECT 1 \\o |id\n"], + 'query piped with options' => ["SELECT 1 \\g (format=csv) |id\n"], + 'expanded query piped' => ["SELECT 1 \\gx |id\n"], + 'write long name piped' => ["\\write |id\n"], + 'copy program after sql' => ["SELECT 1 \\copy t from program 'id'\n"], + 'backquote in set' => ["\\set x `id`\n"], + 'backquote in short command after sql' => ["SELECT 1 \\t `id`\n"], + 'include file' => ["\\i /tmp/generated.sql\n"], + 'include relative file' => ["\\include_relative generated.sql\n"], + 'editor environment' => ["\\setenv PSQL_EDITOR id\n"], + 'editor' => ["\\e\n"], + 'shell escape after escaped backslash' => ["SELECT 1 \\\\\\! id\n"], +]); + +dataset('psql safe dump content', [ + 'escaped backslash before bang in copy data' => ["COPY notes FROM stdin;\n1\tC:\\\\! dir\n\\.\n"], + 'copy escapes before backquotes' => ["COPY notes FROM stdin;\n1\tline\\n `code` \\b `x` \\v `y`\t\\N\n\\.\n"], + 'copy hex and octal escapes' => ["COPY notes FROM stdin;\n1\t\\x41 \\101 |pipe\n\\.\n"], + 'pg_dumpall connect' => ["\\connect app\nSELECT 1;\n"], + 'pg_dump restrict keys' => ["\\restrict abc123\nSELECT 1;\n\\unrestrict abc123\n"], +]); + +test('postgresql backup safety scanner blocks psql shell meta-commands anywhere on a line', function (string $payload) { + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($payload))->toBeTrue(); +})->with('psql shell meta-commands'); + +test('postgresql backup safety scanner allows backslash sequences of ordinary dumps', function (string $payload) { + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($payload))->toBeFalse(); +})->with('psql safe dump content'); + +test('remote postgresql scanner blocks psql shell meta-commands anywhere on a line', function (string $payload) { + expect(scannerBlocks(postgresScanScript(writeScanPayload($payload))))->toBeTrue(); +})->with('psql shell meta-commands'); + +test('remote postgresql scanner allows backslash sequences of ordinary dumps', function (string $payload) { + expect(scannerBlocks(postgresScanScript(writeScanPayload($payload))))->toBeFalse(); +})->with('psql safe dump content'); diff --git a/tests/Unit/DatabaseImport/DatabaseImportCommandBuilderTest.php b/tests/Unit/DatabaseImport/DatabaseImportCommandBuilderTest.php index c6966c1706..c78c56d066 100644 --- a/tests/Unit/DatabaseImport/DatabaseImportCommandBuilderTest.php +++ b/tests/Unit/DatabaseImport/DatabaseImportCommandBuilderTest.php @@ -75,7 +75,7 @@ test('builds dump-all commands and postgres safety scan', function () { ->toContain('unable to inspect custom archive') ->not->toContain('then exit 0') ->and($script) - ->toContain("tr '\\n\\r\\t'"); + ->toContain("tr '\\n\\r' '~~'"); }); test('restores dump-all PostgreSQL custom archives with pg_restore from the decompressed stream', function () { @@ -83,8 +83,8 @@ test('restores dump-all PostgreSQL custom archives with pg_restore from the deco // pg_restore cannot read gzip files, so it must receive the decompressed archive on stdin. expect($command) - ->toContain('stream | pg_restore -U "$pg_user" -d "$db"') - ->toContain('stream | psql -U "$pg_user" -d "$db"') + ->toContain('stream | pg_restore --exit-on-error -U "$pg_user" -d "$db"') + ->toContain('stream | psql -v ON_ERROR_STOP=1 -U "$pg_user" -d "$db"') ->not->toContain("-d \"\$db\" '/tmp/backup.dump.gz'"); });