From 439ee6faeaa4717eee00beedde32f62033df0632 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 13:20:19 +0200 Subject: [PATCH] fix: restrict proxy content access and scope backup storage to owners - Hide proxy configuration contents from users without update permission. - Select and validate backup S3 storage against the resource's team. - Keep scheduled branch syncs green while a conflict PR remains open. - Document external mount permissions and Sentinel traffic limits. - Add regression coverage for authorization and backup storage selection. --- .github/workflows/sync-main-to-next.yml | 21 ++--- .../Api/ServerSentinelController.php | 6 +- .../Project/Shared/Storages/VolumeBackups.php | 13 ++- .../Server/Proxy/DynamicConfigurations.php | 8 ++ openapi.json | 45 ++++++++--- openapi.yaml | 31 +++++-- .../proxy/dynamic-configurations.blade.php | 10 ++- ...DynamicConfigurationsAuthorizationTest.php | 81 +++++++++++++++++++ tests/Feature/VolumeBackupTest.php | 37 +++++++++ tests/Unit/RemoteOutputSizeLimitsTest.php | 1 + 10 files changed, 220 insertions(+), 33 deletions(-) create mode 100644 tests/Feature/ServerProxyDynamicConfigurationsAuthorizationTest.php diff --git a/.github/workflows/sync-main-to-next.yml b/.github/workflows/sync-main-to-next.yml index adff8ef82d..cc53b0ca88 100644 --- a/.github/workflows/sync-main-to-next.yml +++ b/.github/workflows/sync-main-to-next.yml @@ -48,15 +48,18 @@ jobs: sync_branch='automation/sync-main-to-next' existing_pr=$(gh pr list --base next --head "$sync_branch" --state open --json url --jq '.[0].url') if [ -n "$existing_pr" ]; then - echo "A main to next pull request already exists: $existing_pr" - else - git push --force origin origin/main:"refs/heads/$sync_branch" - gh pr create \ - --base next \ - --head "$sync_branch" \ - --title 'chore: merge main into next' \ - --body 'This pull request was created automatically because main could not be merged into next without conflicts. Resolve conflicts on this temporary branch; never update main with next.' + # The conflict was already reported when this pull request was opened. + # Stay green until it is merged instead of failing every scheduled run. + echo "::notice title=main to next sync blocked::main still conflicts with next; resolve the open pull request: $existing_pr" + exit 0 fi - echo 'main could not be merged into next without conflicts.' + git push --force origin origin/main:"refs/heads/$sync_branch" + gh pr create \ + --base next \ + --head "$sync_branch" \ + --title 'chore: merge main into next' \ + --body 'This pull request was created automatically because main could not be merged into next without conflicts. Resolve conflicts on this temporary branch; never update main with next.' + + echo '::error title=main to next sync blocked::main could not be merged into next without conflicts; a pull request was opened to resolve them.' exit 1 diff --git a/app/Http/Controllers/Api/ServerSentinelController.php b/app/Http/Controllers/Api/ServerSentinelController.php index 53d90c7617..f3e15b5c14 100644 --- a/app/Http/Controllers/Api/ServerSentinelController.php +++ b/app/Http/Controllers/Api/ServerSentinelController.php @@ -145,10 +145,10 @@ class ServerSentinelController extends Controller new OA\Property(property: 'sentinel_metrics_history_days', type: 'integer', minimum: 1), new OA\Property(property: 'sentinel_push_interval_seconds', type: 'integer', minimum: 10), new OA\Property(property: 'sentinel_custom_url', type: 'string', nullable: true), - new OA\Property(property: 'traffic_topn', type: 'integer', minimum: 1), + new OA\Property(property: 'traffic_topn', type: 'integer', maximum: 1000, minimum: 1), new OA\Property(property: 'traffic_sample_threshold', type: 'integer', minimum: 0), - new OA\Property(property: 'traffic_retention_1h_days', type: 'integer', minimum: 1), - new OA\Property(property: 'traffic_retention_1d_days', type: 'integer', minimum: 1), + new OA\Property(property: 'traffic_retention_1h_days', type: 'integer', maximum: 365, minimum: 1), + new OA\Property(property: 'traffic_retention_1d_days', type: 'integer', maximum: 3650, minimum: 1), new OA\Property(property: 'is_geoip_enabled', type: 'boolean'), new OA\Property(property: 'geoip_refresh_days', type: 'integer', minimum: 1), new OA\Property(property: 'geoip_maxmind_license_key', type: 'string', nullable: true), diff --git a/app/Livewire/Project/Shared/Storages/VolumeBackups.php b/app/Livewire/Project/Shared/Storages/VolumeBackups.php index 4ceffef502..215c8a68f9 100644 --- a/app/Livewire/Project/Shared/Storages/VolumeBackups.php +++ b/app/Livewire/Project/Shared/Storages/VolumeBackups.php @@ -100,8 +100,10 @@ class VolumeBackups extends Component public function mount(): void { $this->authorize('view', $this->resource); - $this->availableS3Storages = S3Storage::ownedByCurrentTeam() + $this->availableS3Storages = S3Storage::query() + ->where('team_id', $this->resourceTeamId()) ->where('is_usable', true) + ->orderBy('name') ->get(); $this->backup = $this->storage->scheduledBackups()->first(); $server = $this->backup?->server() ?? data_get($this->resource, 'destination.server'); @@ -419,7 +421,7 @@ class VolumeBackups extends Component { $backup = $this->storage->scheduledBackups()->firstOrNew(); $backup->fill([ - 'team_id' => $this->resource->environment->project->team_id, + 'team_id' => $this->resourceTeamId(), 'frequency' => $this->frequency, 'enabled' => $enabled, 'save_s3' => $this->saveToS3, @@ -483,11 +485,16 @@ class VolumeBackups extends Component return $this->s3StorageId !== null && S3Storage::query() ->whereKey($this->s3StorageId) - ->where('team_id', currentTeam()->id) + ->where('team_id', $this->resourceTeamId()) ->where('is_usable', true) ->exists(); } + private function resourceTeamId(): int + { + return $this->resource->environment->project->team_id; + } + private function routeName(string $section): string { return $this->resource instanceof Service diff --git a/app/Livewire/Server/Proxy/DynamicConfigurations.php b/app/Livewire/Server/Proxy/DynamicConfigurations.php index a090872d66..ce305f0c4e 100644 --- a/app/Livewire/Server/Proxy/DynamicConfigurations.php +++ b/app/Livewire/Server/Proxy/DynamicConfigurations.php @@ -57,6 +57,14 @@ class DynamicConfigurations extends Component $files = collect(explode("\n", $files))->filter(fn ($file) => ! empty($file)); $files = $files->map(fn ($file) => trim($file)); $files = $files->sort(); + if (! auth()->user()?->can('update', $this->server)) { + // Members see file names only; contents can hold credentials. + $this->contents = $files->take(self::MAX_CONFIGURATION_FILES) + ->mapWithKeys(fn ($file) => [str_replace('.', '|', $file) => null]); + $this->dispatch('$refresh'); + + return; + } $contents = collect([]); $skippedFiles = collect([]); $totalBytes = 0; diff --git a/openapi.json b/openapi.json index 313fefa63a..bfc925779c 100644 --- a/openapi.json +++ b/openapi.json @@ -4969,7 +4969,7 @@ }, "fs_path": { "type": "string", - "description": "Host directory path (required when is_directory is true)." + "description": "Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. A directory or file mount outside the resource directory needs a token with the deploy permission. Coolify never deletes a path outside the resource directory." } }, "type": "object", @@ -4998,6 +4998,9 @@ "404": { "$ref": "#\/components\/responses\/404" }, + "403": { + "description": "The token needs the deploy permission for a mount outside the resource directory." + }, "422": { "$ref": "#\/components\/responses\/422" } @@ -5067,7 +5070,7 @@ "content": { "type": "string", "nullable": true, - "description": "The file content (file only, not allowed for read-only storages)." + "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." } }, "type": "object", @@ -5096,6 +5099,9 @@ "404": { "$ref": "#\/components\/responses\/404" }, + "403": { + "description": "The token needs the deploy permission to change the content of a file outside the resource directory." + }, "422": { "$ref": "#\/components\/responses\/422" } @@ -9605,7 +9611,7 @@ }, "fs_path": { "type": "string", - "description": "Host directory path (required when is_directory is true)." + "description": "Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. A directory or file mount outside the resource directory needs a token with the deploy permission. Coolify never deletes a path outside the resource directory." } }, "type": "object", @@ -9634,6 +9640,9 @@ "404": { "$ref": "#\/components\/responses\/404" }, + "403": { + "description": "The token needs the deploy permission for a mount outside the resource directory." + }, "422": { "$ref": "#\/components\/responses\/422" } @@ -9703,7 +9712,7 @@ "content": { "type": "string", "nullable": true, - "description": "The file content (file only, not allowed for read-only storages)." + "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." } }, "type": "object", @@ -9732,6 +9741,9 @@ "404": { "$ref": "#\/components\/responses\/404" }, + "403": { + "description": "The token needs the deploy permission to change the content of a file outside the resource directory." + }, "422": { "$ref": "#\/components\/responses\/422" } @@ -14426,7 +14438,8 @@ "timeout": { "type": "integer", "description": "The timeout of the scheduled task in seconds.", - "default": 300 + "default": 300, + "maximum": 36000 }, "enabled": { "type": "boolean", @@ -14579,7 +14592,8 @@ "timeout": { "type": "integer", "description": "The timeout of the scheduled task in seconds.", - "default": 300 + "default": 300, + "maximum": 36000 }, "enabled": { "type": "boolean", @@ -14772,7 +14786,8 @@ "timeout": { "type": "integer", "description": "The timeout of the scheduled task in seconds.", - "default": 300 + "default": 300, + "maximum": 36000 }, "enabled": { "type": "boolean", @@ -14925,7 +14940,8 @@ "timeout": { "type": "integer", "description": "The timeout of the scheduled task in seconds.", - "default": 300 + "default": 300, + "maximum": 36000 }, "enabled": { "type": "boolean", @@ -16919,6 +16935,7 @@ }, "traffic_topn": { "type": "integer", + "maximum": 1000, "minimum": 1 }, "traffic_sample_threshold": { @@ -16927,10 +16944,12 @@ }, "traffic_retention_1h_days": { "type": "integer", + "maximum": 365, "minimum": 1 }, "traffic_retention_1d_days": { "type": "integer", + "maximum": 3650, "minimum": 1 }, "is_geoip_enabled": { @@ -20362,7 +20381,7 @@ }, "fs_path": { "type": "string", - "description": "Host directory path (required when is_directory is true)." + "description": "Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. A directory or file mount outside the resource directory needs a token with the deploy permission. Coolify never deletes a path outside the resource directory." } }, "type": "object", @@ -20391,6 +20410,9 @@ "404": { "$ref": "#\/components\/responses\/404" }, + "403": { + "description": "The token needs the deploy permission for a mount outside the resource directory." + }, "422": { "$ref": "#\/components\/responses\/422" } @@ -20460,7 +20482,7 @@ "content": { "type": "string", "nullable": true, - "description": "The file content (file only, not allowed for read-only storages)." + "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." } }, "type": "object", @@ -20489,6 +20511,9 @@ "404": { "$ref": "#\/components\/responses\/404" }, + "403": { + "description": "The token needs the deploy permission to change the content of a file outside the resource directory." + }, "422": { "$ref": "#\/components\/responses\/422" } diff --git a/openapi.yaml b/openapi.yaml index dc2b3aebf9..28ed4f49ed 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -3225,7 +3225,7 @@ paths: description: 'Whether this is a directory mount (file only, default false).' fs_path: type: string - description: 'Host directory path (required when is_directory is true).' + description: 'Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. A directory or file mount outside the resource directory needs a token with the deploy permission. Coolify never deletes a path outside the resource directory.' type: object additionalProperties: false responses: @@ -3241,6 +3241,8 @@ paths: $ref: '#/components/responses/400' '404': $ref: '#/components/responses/404' + '403': + description: 'The token needs the deploy permission for a mount outside the resource directory.' '422': $ref: '#/components/responses/422' security: @@ -3291,7 +3293,7 @@ paths: content: type: string nullable: true - description: 'The file content (file only, not allowed for read-only storages).' + 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.' type: object additionalProperties: false responses: @@ -3307,6 +3309,8 @@ paths: $ref: '#/components/responses/400' '404': $ref: '#/components/responses/404' + '403': + description: 'The token needs the deploy permission to change the content of a file outside the resource directory.' '422': $ref: '#/components/responses/422' security: @@ -6290,7 +6294,7 @@ paths: description: 'Whether this is a directory mount (file only, default false).' fs_path: type: string - description: 'Host directory path (required when is_directory is true).' + description: 'Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. A directory or file mount outside the resource directory needs a token with the deploy permission. Coolify never deletes a path outside the resource directory.' type: object additionalProperties: false responses: @@ -6306,6 +6310,8 @@ paths: $ref: '#/components/responses/400' '404': $ref: '#/components/responses/404' + '403': + description: 'The token needs the deploy permission for a mount outside the resource directory.' '422': $ref: '#/components/responses/422' security: @@ -6356,7 +6362,7 @@ paths: content: type: string nullable: true - description: 'The file content (file only, not allowed for read-only storages).' + 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.' type: object additionalProperties: false responses: @@ -6372,6 +6378,8 @@ paths: $ref: '#/components/responses/400' '404': $ref: '#/components/responses/404' + '403': + description: 'The token needs the deploy permission to change the content of a file outside the resource directory.' '422': $ref: '#/components/responses/422' security: @@ -9222,6 +9230,7 @@ paths: type: integer description: 'The timeout of the scheduled task in seconds.' default: 300 + maximum: 36000 enabled: type: boolean description: 'The flag to indicate if the scheduled task is enabled.' @@ -9326,6 +9335,7 @@ paths: type: integer description: 'The timeout of the scheduled task in seconds.' default: 300 + maximum: 36000 enabled: type: boolean description: 'The flag to indicate if the scheduled task is enabled.' @@ -9458,6 +9468,7 @@ paths: type: integer description: 'The timeout of the scheduled task in seconds.' default: 300 + maximum: 36000 enabled: type: boolean description: 'The flag to indicate if the scheduled task is enabled.' @@ -9562,6 +9573,7 @@ paths: type: integer description: 'The timeout of the scheduled task in seconds.' default: 300 + maximum: 36000 enabled: type: boolean description: 'The flag to indicate if the scheduled task is enabled.' @@ -10703,15 +10715,18 @@ paths: type: [string, 'null'] traffic_topn: type: integer + maximum: 1000 minimum: 1 traffic_sample_threshold: type: integer minimum: 0 traffic_retention_1h_days: type: integer + maximum: 365 minimum: 1 traffic_retention_1d_days: type: integer + maximum: 3650 minimum: 1 is_geoip_enabled: type: boolean @@ -12845,7 +12860,7 @@ paths: description: 'Whether this is a directory mount (file only, default false).' fs_path: type: string - description: 'Host directory path (required when is_directory is true).' + description: 'Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. A directory or file mount outside the resource directory needs a token with the deploy permission. Coolify never deletes a path outside the resource directory.' type: object additionalProperties: false responses: @@ -12861,6 +12876,8 @@ paths: $ref: '#/components/responses/400' '404': $ref: '#/components/responses/404' + '403': + description: 'The token needs the deploy permission for a mount outside the resource directory.' '422': $ref: '#/components/responses/422' security: @@ -12911,7 +12928,7 @@ paths: content: type: string nullable: true - description: 'The file content (file only, not allowed for read-only storages).' + 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.' type: object additionalProperties: false responses: @@ -12927,6 +12944,8 @@ paths: $ref: '#/components/responses/400' '404': $ref: '#/components/responses/404' + '403': + description: 'The token needs the deploy permission to change the content of a file outside the resource directory.' '422': $ref: '#/components/responses/422' security: diff --git a/resources/views/livewire/server/proxy/dynamic-configurations.blade.php b/resources/views/livewire/server/proxy/dynamic-configurations.blade.php index 2ffc8ae779..05c40d3eae 100644 --- a/resources/views/livewire/server/proxy/dynamic-configurations.blade.php +++ b/resources/views/livewire/server/proxy/dynamic-configurations.blade.php @@ -63,8 +63,14 @@ wire:key="proxy-navbar-{{ $fileName }}" /> @endif - + @can('update', $server) + + @else +

+ You do not have permission to view the contents of this configuration. +

+ @endcan @endforeach @else diff --git a/tests/Feature/ServerProxyDynamicConfigurationsAuthorizationTest.php b/tests/Feature/ServerProxyDynamicConfigurationsAuthorizationTest.php new file mode 100644 index 0000000000..674c829b1b --- /dev/null +++ b/tests/Feature/ServerProxyDynamicConfigurationsAuthorizationTest.php @@ -0,0 +1,81 @@ + 0]); + + $this->team = Team::factory()->create(); + $privateKey = PrivateKey::factory()->create(['team_id' => $this->team->id]); + $this->server = Server::factory()->create([ + 'team_id' => $this->team->id, + 'private_key_id' => $privateKey->id, + 'ip' => '203.0.113.20', + 'proxy' => ['type' => ProxyTypes::TRAEFIK->value, 'status' => 'running'], + ]); + $this->server->settings->forceFill(['is_reachable' => true, 'is_usable' => true, 'force_disabled' => false])->save(); + + Process::fake(function ($process) { + if (str_contains($process->command, 'ls -1')) { + return Process::result(output: "zzfix-auth.yaml\n"); + } + if (str_contains($process->command, 'zzfix-auth.yaml')) { + return Process::result(output: "http:\n middlewares:\n auth:\n basicAuth:\n users:\n - 'zzfix:\$apr1\$zzfixsecrethash'\n"); + } + + return Process::result(); + }); +}); + +function actingAsDynamicConfigurationsUser(Team $team, string $role): User +{ + $user = User::factory()->create(); + $team->members()->attach($user->id, ['role' => $role]); + test()->actingAs($user); + session(['currentTeam' => $team]); + + return $user; +} + +it('does not load dynamic configuration contents for a team member', function () { + actingAsDynamicConfigurationsUser($this->team, 'member'); + + $component = Livewire::withQueryParams(['server_uuid' => $this->server->uuid]) + ->test(DynamicConfigurations::class) + ->call('initLoadDynamicConfigurations') + ->assertOk() + ->assertNotDispatched('error'); + + expect($component->get('contents')->filter()->all())->toBe([]) + ->and(json_encode($component->snapshot))->not->toContain('zzfixsecrethash') + ->and($component->html())->not->toContain('zzfixsecrethash') + ->and($component->html())->toContain('zzfix-auth.yaml'); + Process::assertNotRan(fn ($process) => str_contains($process->command, 'head -c')); +}); + +it('loads dynamic configuration contents for a team admin', function () { + actingAsDynamicConfigurationsUser($this->team, 'admin'); + + $component = Livewire::withQueryParams(['server_uuid' => $this->server->uuid]) + ->test(DynamicConfigurations::class) + ->call('initLoadDynamicConfigurations') + ->assertNotDispatched('error'); + + expect($component->get('contents')->get('zzfix-auth|yaml'))->toContain('zzfixsecrethash') + ->and($component->html())->toContain('zzfixsecrethash'); +}); diff --git a/tests/Feature/VolumeBackupTest.php b/tests/Feature/VolumeBackupTest.php index a4d2c2baac..2b440eaac2 100644 --- a/tests/Feature/VolumeBackupTest.php +++ b/tests/Feature/VolumeBackupTest.php @@ -1220,6 +1220,43 @@ it('creates a local scheduled backup for a persistent volume', function () { ->and($backup->save_s3)->toBeFalse(); }); +it('lists and accepts only S3 storages of the resource team when the session team differs', function (bool $rootTeam) { + $team = $rootTeam ? Team::factory()->create(['id' => 0]) : Team::factory()->create(); + $user = signInForVolumeBackups($this, $team); + [$application, $volume] = createVolumeBackupApplication($team); + $otherTeam = Team::factory()->create(); + $user->teams()->attach($otherTeam, ['role' => 'owner']); + $s3Attributes = ['region' => 'us-east-1', 'key' => 'key', 'secret' => 'secret', 'bucket' => 'bucket', 'endpoint' => 'https://s3.example.com', 'is_usable' => true]; + $resourceTeamStorage = S3Storage::create([...$s3Attributes, 'name' => 'Resource team S3', 'team_id' => $team->id]); + $otherTeamStorage = S3Storage::create([...$s3Attributes, 'name' => 'Other team S3', 'team_id' => $otherTeam->id]); + session(['currentTeam' => $otherTeam]); + + $component = Livewire::test(VolumeBackups::class, ['storage' => $volume, 'resource' => $application]); + + expect($component->get('availableS3Storages')->pluck('id')->all())->toBe([$resourceTeamStorage->id]) + ->and($component->get('s3StorageId'))->toBe($resourceTeamStorage->id); + + $component->set('frequency', 'daily') + ->set('saveToS3', true) + ->set('s3StorageId', $otherTeamStorage->id) + ->assertHasErrors('s3StorageId') + ->call('save') + ->assertHasErrors('s3StorageId'); + + expect(ScheduledVolumeBackup::query()->count())->toBe(0); + + $component->set('s3StorageId', $resourceTeamStorage->id) + ->call('save') + ->assertHasNoErrors(); + + expect(ScheduledVolumeBackup::query()->sole()) + ->s3_storage_id->toBe($resourceTeamStorage->id) + ->save_s3->toBeTrue(); +})->with([ + 'team' => [false], + 'root team' => [true], +]); + it('only accepts a usable S3 storage owned by the current team', function () { $team = Team::factory()->create(); signInForVolumeBackups($this, $team); diff --git a/tests/Unit/RemoteOutputSizeLimitsTest.php b/tests/Unit/RemoteOutputSizeLimitsTest.php index dda82a0fb1..8720144946 100644 --- a/tests/Unit/RemoteOutputSizeLimitsTest.php +++ b/tests/Unit/RemoteOutputSizeLimitsTest.php @@ -32,6 +32,7 @@ function remoteOutputTestServer(): Server it('bounds dynamic proxy configuration files and their combined Livewire payload', function () { $server = Mockery::mock(remoteOutputTestServer())->makePartial(); $server->shouldReceive('proxyPath')->andReturn('/data/proxy'); + $this->actingAs(User::query()->latest('id')->firstOrFail()); $files = collect(range(1, 101))->map(fn (int $number) => sprintf('file%03d.yml', $number))->implode("\n"); Process::fake(function ($process) use ($files) {