From 0da202b39df52880ff39204478ff9d3f32accd8b Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:02:56 +0200 Subject: [PATCH] fix: protect restores, preview imports, and analytics access --- app/Livewire/Analytics.php | 6 + app/Livewire/Dashboard/TrafficAnalytics.php | 6 + .../ServerTransfer/ServerTransferImporter.php | 44 +++-- .../DatabaseImportCommandBuilder.php | 3 + .../ServerTransferPreviewOwnershipTest.php | 154 ++++++++++++++++++ .../TrafficAnalyticsAuthorizationTest.php | 98 +++++++++++ .../DatabaseImportRestoreScriptTest.php | 34 +++- 7 files changed, 331 insertions(+), 14 deletions(-) create mode 100644 tests/Feature/ServerTransferPreviewOwnershipTest.php create mode 100644 tests/Feature/TrafficAnalyticsAuthorizationTest.php diff --git a/app/Livewire/Analytics.php b/app/Livewire/Analytics.php index 327d9d220b..08014942fc 100644 --- a/app/Livewire/Analytics.php +++ b/app/Livewire/Analytics.php @@ -329,6 +329,12 @@ class Analytics extends Component public function loadData(): void { + $this->resetData(); + + foreach ($this->servers as $server) { + $this->authorize('view', $server); + } + if ($this->servers->isEmpty()) { return; } diff --git a/app/Livewire/Dashboard/TrafficAnalytics.php b/app/Livewire/Dashboard/TrafficAnalytics.php index 047f461d32..2d3228e139 100644 --- a/app/Livewire/Dashboard/TrafficAnalytics.php +++ b/app/Livewire/Dashboard/TrafficAnalytics.php @@ -56,6 +56,12 @@ class TrafficAnalytics extends Component public function loadData(): void { + $this->reset('overview', 'latencyApproximate', 'uniquesApproximate', 'series'); + + foreach ($this->servers as $server) { + $this->authorize('view', $server); + } + if ($this->servers->isEmpty()) { return; } diff --git a/app/Services/ServerTransfer/ServerTransferImporter.php b/app/Services/ServerTransfer/ServerTransferImporter.php index 92945b8dc4..90fe5954d0 100644 --- a/app/Services/ServerTransfer/ServerTransferImporter.php +++ b/app/Services/ServerTransfer/ServerTransferImporter.php @@ -814,7 +814,7 @@ class ServerTransferImporter $this->importFileStorages(data_get($payload, 'file_storages', []), $application); $this->importScheduledTasks(data_get($payload, 'scheduled_tasks', []), $application, $teamId); $this->importTags(data_get($payload, 'tags', []), $application, $teamId); - $this->importPreviews(data_get($payload, 'previews', []), $application, $preserveUuids, $adoptMode); + $this->importPreviews(data_get($payload, 'previews', []), $application, $preserveUuids, $adoptMode, $teamId); return $application; } @@ -1451,7 +1451,7 @@ class ServerTransferImporter /** * @param list> $previews */ - private function importPreviews(array $previews, Application $application, bool $preserveUuids, bool $adoptMode): void + private function importPreviews(array $previews, Application $application, bool $preserveUuids, bool $adoptMode, int $teamId): void { foreach ($previews as $previewPayload) { $uuid = $preserveUuids && filled(data_get($previewPayload, 'uuid')) @@ -1459,6 +1459,11 @@ class ServerTransferImporter : new_public_id(); $preview = ApplicationPreview::withTrashed()->where('uuid', $uuid)->first(); + if ($preview && ! $this->previewBelongsToTeam($preview, $teamId)) { + throw ValidationException::withMessages([ + 'previews' => ['An imported preview UUID conflicts with a preview outside the target team.'], + ]); + } if (! $preview) { $pullRequestId = data_get($previewPayload, 'pull_request_id'); if ($pullRequestId !== null) { @@ -1475,17 +1480,24 @@ class ServerTransferImporter } $fqdn = data_get($previewPayload, 'fqdn'); - // FQDN is globally unique; free it from other rows (including soft-deleted leftovers). + // FQDN is globally unique; only release conflicts owned by the target team. if (filled($fqdn)) { - ApplicationPreview::withoutEvents(function () use ($fqdn, $preview) { - ApplicationPreview::withTrashed() - ->where('fqdn', $fqdn) - ->when($preview, fn ($q) => $q->where('id', '!=', $preview->id)) - ->get() - ->each(function (ApplicationPreview $conflict) { - $conflict->fqdn = null; - $conflict->saveQuietly(); - }); + $conflicts = ApplicationPreview::withTrashed() + ->where('fqdn', $fqdn) + ->when($preview, fn ($q) => $q->where('id', '!=', $preview->id)) + ->get(); + foreach ($conflicts as $conflict) { + if (! $this->previewBelongsToTeam($conflict, $teamId)) { + throw ValidationException::withMessages([ + 'previews' => ['An imported preview domain conflicts with a preview outside the target team.'], + ]); + } + } + ApplicationPreview::withoutEvents(function () use ($conflicts) { + $conflicts->each(function (ApplicationPreview $conflict) { + $conflict->fqdn = null; + $conflict->saveQuietly(); + }); }); } @@ -1530,6 +1542,14 @@ class ServerTransferImporter } } + private function previewBelongsToTeam(ApplicationPreview $preview, int $teamId): bool + { + return Application::withTrashed() + ->whereKey($preview->application_id) + ->whereRelation('environment.project', 'team_id', $teamId) + ->exists(); + } + /** * @param list> $certificates */ diff --git a/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php b/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php index 7e7c05ae6f..bdc37974d0 100644 --- a/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php +++ b/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php @@ -22,6 +22,9 @@ is_tar() { [ "$(stream | head -c 262 | tail -c 5)" = ustar ]; } is_text() { [ "$(stream | head -c 65536 | tr -d '\000' | wc -c | tr -d ' ')" = "$(stream | head -c 65536 | wc -c | tr -d ' ')" ]; } extract_tar() { work=$(mktemp -d) && trap 'rm -rf "$work"' EXIT && stream | tar -xf - -C "$work" || fail 'The tar backup cannot be extracted.'; } use_single_tar_member() { [ "$(find "$work" -type f | wc -l | tr -d ' ')" = 1 ] && backup=$(find "$work" -type f); } +if is_gzip; then + gunzip -t "$backup" || fail 'The gzip backup is corrupt or incomplete. Nothing was changed.' +fi SH; diff --git a/tests/Feature/ServerTransferPreviewOwnershipTest.php b/tests/Feature/ServerTransferPreviewOwnershipTest.php new file mode 100644 index 0000000000..03cabfa874 --- /dev/null +++ b/tests/Feature/ServerTransferPreviewOwnershipTest.php @@ -0,0 +1,154 @@ +set('app.env', 'local'); + config()->set('app.maintenance.driver', 'file'); + Server::flushIdentityMap(); + Storage::fake('ssh-keys'); + Storage::fake('ssh-mux'); + Process::fake(); + InstanceSettings::forceCreate(['id' => 0, 'is_api_enabled' => true]); + $this->mock(ServerTransferClaimer::class)->shouldReceive('claim')->andReturn([]); +}); + +/** @return array{application: Application, preview: ApplicationPreview, bundle: array} */ +function previewOwnershipFixture(Team $team, bool $trashed = false): array +{ + $key = PrivateKey::factory()->create(['team_id' => $team->id]); + $server = Server::factory()->create(['team_id' => $team->id, 'private_key_id' => $key->id]); + $destination = StandaloneDocker::where('server_id', $server->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $team->id]); + $application = Application::factory()->create([ + 'environment_id' => $project->environments()->firstOrFail()->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + ]); + $preview = ApplicationPreview::create([ + 'application_id' => $application->id, + 'pull_request_id' => 42, + 'pull_request_html_url' => 'https://github.com/example/app/pull/42', + 'fqdn' => 'https://preview.example.com', + ]); + $bundle = app(ServerTransferExporter::class)->export($server); + $bundle['server']['uuid'] = new_public_id(); + $bundle['server']['ip'] = '192.0.2.99'; + if ($trashed) { + $application->delete(); + $preview->delete(); + } + + return compact('application', 'preview', 'bundle'); +} + +test('preview imports reject another team UUID and roll back all imported records', function (bool $trashed, string $role) { + $fixture = previewOwnershipFixture(Team::factory()->create(), $trashed); + $preview = $fixture['preview']; + $volume = LocalPersistentVolume::create([ + 'resource_type' => $preview->getMorphClass(), + 'resource_id' => $preview->id, + 'name' => 'victim-preview-data', + 'mount_path' => '/data', + ]); + $bundle = $fixture['bundle']; + $bundle['projects'][0]['environments'][0]['applications'][0]['previews'][0]['pull_request_id'] = 99; + $bundle['projects'][0]['environments'][0]['applications'][0]['previews'][0]['fqdn'] = 'https://imported.example.com'; + $team = Team::factory()->create(); + $user = User::factory()->create(); + $team->members()->attach($user->id, ['role' => $role]); + session(['currentTeam' => $team]); + $token = $user->createToken('preview-import', ['write']); + $token->accessToken->forceFill(['team_id' => $team->id])->save(); + $counts = [Server::count(), Application::withTrashed()->count(), PrivateKey::count(), Project::count()]; + + $this->withToken($token->plainTextToken)->postJson('/api/v1/servers/import', ['bundle' => $bundle]) + ->assertUnprocessable() + ->assertJsonValidationErrors('previews'); + + expect($preview->fresh()->application_id)->toBe($fixture['application']->id) + ->and($preview->fresh()->pull_request_id)->toBe(42) + ->and($preview->fresh()->fqdn)->toBe('https://preview.example.com') + ->and($preview->fresh()->trashed())->toBe($trashed); + expect($volume->fresh()->resource_id)->toBe($preview->id); + expect([Server::count(), Application::withTrashed()->count(), PrivateKey::count(), Project::count()])->toBe($counts); +})->with([ + 'active preview, owner' => [false, 'owner'], + 'deleted preview and application, admin' => [true, 'admin'], +]); + +test('preview imports reject another team domain without clearing it', function (bool $trashed, bool $preserveUuids) { + $fixture = previewOwnershipFixture(Team::factory()->create(), $trashed); + $bundle = $fixture['bundle']; + $bundle['projects'][0]['environments'][0]['applications'][0]['previews'][0]['uuid'] = new_public_id(); + $team = Team::factory()->create(); + $serverCount = Server::count(); + + try { + app(ServerTransferImporter::class)->import($bundle, $team->id, preserveUuids: $preserveUuids); + $this->fail('A foreign preview domain must be rejected.'); + } catch (ValidationException $exception) { + expect($exception->errors())->toHaveKey('previews'); + } + + expect($fixture['preview']->fresh()->fqdn)->toBe('https://preview.example.com') + ->and($fixture['preview']->fresh()->application_id)->toBe($fixture['application']->id) + ->and($fixture['preview']->fresh()->trashed())->toBe($trashed); + expect(Server::count())->toBe($serverCount); + $this->assertDatabaseCount('application_previews', 1); +})->with([ + 'active, preserve UUIDs' => [false, true], + 'deleted, replace UUIDs' => [true, false], +]); + +test('preview imports preserve same-team UUID handoffs and preview volumes', function (bool $trashed) { + $team = Team::factory()->create(); + $fixture = previewOwnershipFixture($team, $trashed); + $preview = $fixture['preview']; + $volume = LocalPersistentVolume::create([ + 'resource_type' => $preview->getMorphClass(), + 'resource_id' => $preview->id, + 'name' => 'same-team-preview-data', + 'mount_path' => '/data', + ]); + + $result = app(ServerTransferImporter::class)->import($fixture['bundle'], $team->id); + + $imported = Server::whereUuid($result['server_uuid'])->firstOrFail()->applications()->firstOrFail(); + expect($preview->fresh()->application_id)->toBe($imported->id) + ->and($preview->fresh()->fqdn)->toBe('https://preview.example.com') + ->and($preview->fresh()->trashed())->toBeFalse(); + expect($volume->fresh()->resource_id)->toBe($preview->id); + $this->assertDatabaseCount('application_previews', 1); +})->with(['active' => false, 'deleted preview and application' => true]); + +test('preview imports can release a same-team domain collision', function () { + $team = Team::factory()->create(); + $fixture = previewOwnershipFixture($team); + $bundle = $fixture['bundle']; + $bundle['projects'][0]['environments'][0]['applications'][0]['previews'][0]['uuid'] = new_public_id(); + + $result = app(ServerTransferImporter::class)->import($bundle, $team->id); + + $imported = Server::whereUuid($result['server_uuid'])->firstOrFail()->applications()->firstOrFail(); + expect($fixture['preview']->fresh()->fqdn)->toBeNull(); + expect($imported->previews()->firstOrFail()->fqdn)->toBe('https://preview.example.com'); +}); diff --git a/tests/Feature/TrafficAnalyticsAuthorizationTest.php b/tests/Feature/TrafficAnalyticsAuthorizationTest.php new file mode 100644 index 0000000000..55d47648bd --- /dev/null +++ b/tests/Feature/TrafficAnalyticsAuthorizationTest.php @@ -0,0 +1,98 @@ +reads++; + + return str_contains($url, '/traffic/overview') + ? json_encode(['requests' => 123, 'unique_visitors' => 12]) + : '[]'; + } +} + +beforeEach(function () { + Server::flushIdentityMap(); + InstanceSettings::forceCreate(['id' => 0]); + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'member']); + $this->actingAs($this->user); + session(['currentTeam' => $this->team]); + + $key = PrivateKey::factory()->create(['team_id' => $this->team->id]); + $this->server = Server::factory()->create(['team_id' => $this->team->id, 'private_key_id' => $key->id]); + $this->server->settings()->update(['is_traffic_analytics_enabled' => true]); + $this->client = new AuthorizationTrafficClient($this->server); + app()->bind(SentinelTrafficClient::class, fn () => $this->client); +}); + +test('removed members cannot refresh mounted traffic analytics', function (string $componentClass) { + $component = loadLazy(Livewire::test($componentClass))->assertSet('overview.requests', 123); + $reads = $this->client->reads; + + $this->team->members()->detach($this->user->id); + $this->actingAs($this->user->fresh()); + + $component->call('loadData')->assertForbidden(); + expect($this->client->reads)->toBe($reads); +})->with([[Analytics::class], [TrafficAnalytics::class]]); + +test('members can refresh mounted traffic after switching to their own team', function (string $componentClass) { + $component = loadLazy(Livewire::test($componentClass))->assertSet('overview.requests', 123); + $personalTeam = Team::factory()->create(); + $personalTeam->members()->attach($this->user->id, ['role' => 'owner']); + session(['currentTeam' => $personalTeam]); + + $component->call('loadData')->assertOk()->assertSet('overview.requests', 123); +})->with([[Analytics::class], [TrafficAnalytics::class]]); + +test('traffic analytics denies another teams mounted servers and clears previous traffic', function (string $componentClass) { + $component = loadLazy(Livewire::test($componentClass))->assertSet('overview.requests', 123)->instance(); + $otherTeam = Team::factory()->create(); + $foreignServer = Server::factory()->create(['team_id' => $otherTeam->id]); + $component->servers = collect([$this->server, $foreignServer]); + $reads = $this->client->reads; + + expect(fn () => $component->loadData())->toThrow(AuthorizationException::class); + expect($component->overview)->toBeNull() + ->and($component->series)->toBe([]) + ->and($this->client->reads)->toBe($reads); + + if ($component instanceof Analytics) { + expect($component->topApps)->toBe([]) + ->and($component->topHosts)->toBe([]) + ->and($component->topPaths)->toBe([]) + ->and($component->breakdowns)->toBe([]); + } +})->with([[Analytics::class], [TrafficAnalytics::class]]); + +test('another teams owner cannot refresh the mounted traffic analytics', function (string $componentClass) { + $component = loadLazy(Livewire::test($componentClass))->assertSet('overview.requests', 123); + $otherTeam = Team::factory()->create(); + $otherUser = User::factory()->create(); + $otherTeam->members()->attach($otherUser->id, ['role' => 'owner']); + $this->actingAs($otherUser); + session(['currentTeam' => $otherTeam]); + $reads = $this->client->reads; + + $component->call('loadData')->assertForbidden(); + expect($this->client->reads)->toBe($reads); +})->with([[Analytics::class], [TrafficAnalytics::class]]); diff --git a/tests/Unit/DatabaseImport/DatabaseImportRestoreScriptTest.php b/tests/Unit/DatabaseImport/DatabaseImportRestoreScriptTest.php index b0ceb807df..d5cb871d95 100644 --- a/tests/Unit/DatabaseImport/DatabaseImportRestoreScriptTest.php +++ b/tests/Unit/DatabaseImport/DatabaseImportRestoreScriptTest.php @@ -352,13 +352,13 @@ function restoreScriptNormalize(string $contents): array * * @return array{exit: int, stdout: string, stderr: string, databases: array} */ -function postgresReplacementScriptRun(string $target = 'app', string $failure = '', bool $gzip = false): array +function postgresReplacementScriptRun(string $target = 'app', string $failure = '', bool $gzip = false, ?string $contents = null): array { $dir = restoreScriptTempDir(); try { $backup = $dir.'/backup.sql'; - file_put_contents($backup, $gzip ? gzencode('SELECT 1;') : 'SELECT 1;'); + file_put_contents($backup, $contents ?? ($gzip ? gzencode('SELECT 1;') : 'SELECT 1;')); $script = (new DatabaseImportCommandBuilder)->buildRestoreCommand(restoreScriptResource('postgresql'), $backup, false, true); preg_match('/new=(\w+)/', $script, $new); preg_match('/old=(\w+)/', $script, $old); @@ -449,6 +449,36 @@ test('failed SQL replacement preserves every existing PostgreSQL database and re ->and($run['databases'])->toEqual(['coolify_restore_new' => 'unrelated-new', 'coolify_restore_old' => 'unrelated-old', 'app' => 'original']); })->with(['restore', 'swap']); +dataset('corrupt restore gzip', [ + 'missing footer' => fn (string $contents): string => substr(gzencode($contents), 0, -8), + 'truncated later member' => fn (string $contents): string => gzencode($contents).substr(gzencode('SELECT 2;'), 0, 10), +]); + +test('corrupt gzip SQL replacement preserves every existing PostgreSQL database', function (Closure $corrupt) { + $run = postgresReplacementScriptRun(contents: $corrupt('SELECT 1;')); + + expect($run['exit'])->toBe(1) + ->and($run['stderr'])->toContain('The gzip backup is corrupt or incomplete. Nothing was changed.') + ->and($run['databases'])->toEqual(['coolify_restore_new' => 'unrelated-new', 'coolify_restore_old' => 'unrelated-old', 'app' => 'original']); +})->with('corrupt restore gzip'); + +test('rejects corrupt gzip backups before any database client runs', function (string $engine, string $fixture, bool $dumpAll, bool $replaceExisting, Closure $corrupt) { + $run = restoreScriptRun($engine, $corrupt(restoreScriptFixture($fixture)), $dumpAll, $replaceExisting); + + expect($run['exit'])->toBe(1) + ->and($run['stderr'])->toContain('The gzip backup is corrupt or incomplete. Nothing was changed.') + ->and($run['calls'])->toBe([]) + ->and($run['leftovers'])->toBe([]); +})->with([ + 'single PostgreSQL SQL' => ['postgresql', 'pg-sql', false, false], + 'PostgreSQL archive replacement' => ['postgresql', 'pg-custom', false, true], + 'all PostgreSQL databases' => ['postgresql', 'pg-sql', true, false], + 'single MySQL database' => ['mysql', 'mysql-sql', false, false], + 'all MySQL databases' => ['mysql', 'mysql-sql', true, false], + 'all MariaDB databases' => ['mariadb', 'mysql-sql', true, false], + 'MongoDB replacement' => ['mongodb', 'mongo-archive', false, true], +])->with('corrupt restore gzip'); + test('SQL replacement refuses scratch database name collisions without changing existing data', function (string $failure) { $run = postgresReplacementScriptRun(failure: $failure);