From 9e021c4037ff9591511bb02491e9ea9413d929bd Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 15 Jun 2026 17:15:56 +0200 Subject: [PATCH] fix(api): enforce destination access and cleanup networks Require admin team membership for destination mutations, return invalid-token responses for tokenless requests, and remove standalone Docker networks when deleting destinations. --- .../RemoveStandaloneDockerNetwork.php | 16 ++ .../Api/DestinationsController.php | 139 +++++++++++------- tests/Feature/Api/DestinationsApiTest.php | 62 +++++++- 3 files changed, 162 insertions(+), 55 deletions(-) create mode 100644 app/Actions/Destination/RemoveStandaloneDockerNetwork.php diff --git a/app/Actions/Destination/RemoveStandaloneDockerNetwork.php b/app/Actions/Destination/RemoveStandaloneDockerNetwork.php new file mode 100644 index 000000000..21c40a50a --- /dev/null +++ b/app/Actions/Destination/RemoveStandaloneDockerNetwork.php @@ -0,0 +1,16 @@ +network); + + instant_remote_process(["docker network disconnect {$safeNetwork} coolify-proxy"], $destination->server, throwError: false); + instant_remote_process(["docker network rm -f {$safeNetwork}"], $destination->server); + } +} diff --git a/app/Http/Controllers/Api/DestinationsController.php b/app/Http/Controllers/Api/DestinationsController.php index 8b54913e7..26836b89a 100644 --- a/app/Http/Controllers/Api/DestinationsController.php +++ b/app/Http/Controllers/Api/DestinationsController.php @@ -2,38 +2,37 @@ namespace App\Http\Controllers\Api; +use App\Actions\Destination\RemoveStandaloneDockerNetwork; use App\Http\Controllers\Controller; use App\Models\Server; use App\Models\StandaloneDocker; use App\Models\SwarmDocker; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; -use Illuminate\Support\Facades\Validator; class DestinationsController extends Controller { - private function transform($d): array + private function transform(StandaloneDocker|SwarmDocker $destination): array { return [ - 'id' => $d->id, - 'uuid' => $d->uuid, - 'name' => $d->name, - 'network' => $d->network, - 'type' => $d instanceof SwarmDocker ? 'swarm' : 'standalone', - 'server_uuid' => $d->server?->uuid, - 'created_at' => $d->created_at, - 'updated_at' => $d->updated_at, + 'uuid' => $destination->uuid, + 'name' => $destination->name, + 'network' => $destination->network, + 'type' => $destination instanceof SwarmDocker ? 'swarm' : 'standalone', + 'server_uuid' => $destination->server?->uuid, + 'created_at' => $destination->created_at, + 'updated_at' => $destination->updated_at, ]; } /** - * Resolve the calling token's team id, or return a 403 response. + * Resolve the calling token's team id, or return an invalid-token response. */ private function teamIdOrAbort(): int|JsonResponse { $teamId = getTeamIdFromToken(); if (is_null($teamId)) { - return response()->json(['message' => 'You are not allowed to access the API.'], 403); + return invalidTokenResponse(); } return $teamId; @@ -45,15 +44,21 @@ private function teamIdOrAbort(): int|JsonResponse * controller works on Coolify versions that pre-date that scope being added * to the destination models (e.g. 4.0.0-beta.470). */ - private function teamScopedDockers(int $teamId) + private function teamScopedDockers(int $teamId): array { return [ - 'standalone' => StandaloneDocker::whereHas('server', fn ($q) => $q->whereTeamId($teamId))->get(), - 'swarm' => SwarmDocker::whereHas('server', fn ($q) => $q->whereTeamId($teamId))->get(), + 'standalone' => StandaloneDocker::with('server:id,uuid')->whereHas('server', fn ($query) => $query->whereTeamId($teamId))->get(), + 'swarm' => SwarmDocker::with('server:id,uuid')->whereHas('server', fn ($query) => $query->whereTeamId($teamId))->get(), ]; } - public function index(Request $request) + private function findDestinationForTeam(int $teamId, string $uuid): StandaloneDocker|SwarmDocker + { + return StandaloneDocker::with('server:id,uuid,team_id,ip,user,port,private_key_id')->whereHas('server', fn ($query) => $query->whereTeamId($teamId))->whereUuid($uuid)->first() + ?? SwarmDocker::with('server:id,uuid,team_id')->whereHas('server', fn ($query) => $query->whereTeamId($teamId))->whereUuid($uuid)->firstOrFail(); + } + + public function index(Request $request): JsonResponse { $teamId = $this->teamIdOrAbort(); if (! is_int($teamId)) { @@ -63,36 +68,38 @@ public function index(Request $request) return response()->json( $sets['standalone']->concat($sets['swarm']) - ->map(fn ($d) => $this->transform($d)) + ->map(fn ($destination) => $this->transform($destination)) ->values() ); } - public function index_by_server(Request $request, string $server_uuid) + public function index_by_server(Request $request, string $server_uuid): JsonResponse { $teamId = $this->teamIdOrAbort(); if (! is_int($teamId)) { return $teamId; } - $server = Server::whereTeamId($teamId)->whereUuid($server_uuid)->firstOrFail(); + $server = Server::with(['standaloneDockers.server:id,uuid', 'swarmDockers.server:id,uuid']) + ->whereTeamId($teamId) + ->whereUuid($server_uuid) + ->firstOrFail(); $list = $server->standaloneDockers->concat($server->swarmDockers); - return response()->json($list->map(fn ($d) => $this->transform($d))->values()); + return response()->json($list->map(fn ($destination) => $this->transform($destination))->values()); } - public function show(Request $request, string $uuid) + public function show(Request $request, string $uuid): JsonResponse { $teamId = $this->teamIdOrAbort(); if (! is_int($teamId)) { return $teamId; } - $d = StandaloneDocker::whereHas('server', fn ($q) => $q->whereTeamId($teamId))->whereUuid($uuid)->first() - ?? SwarmDocker::whereHas('server', fn ($q) => $q->whereTeamId($teamId))->whereUuid($uuid)->firstOrFail(); + $destination = $this->findDestinationForTeam($teamId, $uuid); - return response()->json($this->transform($d)); + return response()->json($this->transform($destination)); } - public function create(Request $request, string $server_uuid) + public function create(Request $request, string $server_uuid): JsonResponse { $teamId = $this->teamIdOrAbort(); if (! is_int($teamId)) { @@ -107,18 +114,22 @@ public function create(Request $request, string $server_uuid) $server = Server::whereTeamId($teamId)->whereUuid($server_uuid)->firstOrFail(); $allowed = ['name', 'network', 'type']; - $extra = array_diff(array_keys($request->all()), $allowed); - if (! empty($extra)) { - return response()->json(['message' => 'Unknown fields', 'fields' => array_values($extra)], 422); - } - $validator = Validator::make($request->all(), [ + $validator = customApiValidator($request->all(), [ 'name' => 'nullable|string|max:255', 'network' => ['required', 'string', 'max:255', 'regex:/^[a-zA-Z0-9][a-zA-Z0-9._-]*$/'], 'type' => 'nullable|in:standalone,swarm', ]); - if ($validator->fails()) { - return response()->json(['message' => 'Validation failed', 'errors' => $validator->errors()], 422); + $extra = array_diff(array_keys($request->all()), $allowed); + if ($validator->fails() || ! empty($extra)) { + $errors = $validator->errors(); + if (! empty($extra)) { + foreach ($extra as $field) { + $errors->add($field, 'This field is not allowed.'); + } + } + + return response()->json(['message' => 'Validation failed.', 'errors' => $errors], 422); } $expectedType = $server->isSwarm() ? 'swarm' : 'standalone'; @@ -130,52 +141,80 @@ public function create(Request $request, string $server_uuid) $name = $request->input('name') ?: ($server->name.'-'.$request->input('network')); $class = $type === 'swarm' ? SwarmDocker::class : StandaloneDocker::class; + $this->authorize('create', $class); + $exists = $class::where('server_id', $server->id)->where('network', $request->input('network'))->exists(); if ($exists) { return response()->json(['message' => 'A destination with this network already exists on the server.'], 409); } - $d = $class::create([ + $destination = $class::create([ 'name' => $name, 'network' => $request->input('network'), 'server_id' => $server->id, ]); - return response()->json(['uuid' => $d->uuid], 201); + auditLog('api.destination.created', [ + 'team_id' => $teamId, + 'destination_uuid' => $destination->uuid, + 'destination_name' => $destination->name, + 'destination_type' => $type, + 'server_uuid' => $server->uuid, + ]); + + return response()->json($this->transform($destination->load('server:id,uuid')), 201); } - public function delete(Request $request, string $uuid) + public function delete(Request $request, string $uuid): JsonResponse { $teamId = $this->teamIdOrAbort(); if (! is_int($teamId)) { return $teamId; } - $d = StandaloneDocker::whereHas('server', fn ($q) => $q->whereTeamId($teamId))->whereUuid($uuid)->first() - ?? SwarmDocker::whereHas('server', fn ($q) => $q->whereTeamId($teamId))->whereUuid($uuid)->firstOrFail(); + $destination = $this->findDestinationForTeam($teamId, $uuid); + + $this->authorize('delete', $destination); // Guard against deleting destinations with attached resources. attachedTo() // is recent on the destination models; fall back to a manual check for // older Coolify versions (e.g. 4.0.0-beta.470). - if (method_exists($d, 'attachedTo')) { - if ($d->attachedTo()) { + if (method_exists($destination, 'attachedTo')) { + if ($destination->attachedTo()) { return response()->json(['message' => 'Destination has attached resources, detach first.'], 409); } } else { - $hasAttached = $d->applications()->exists() - || $d->postgresqls()->exists() - || (method_exists($d, 'mysqls') && $d->mysqls()->exists()) - || (method_exists($d, 'mariadbs') && $d->mariadbs()->exists()) - || (method_exists($d, 'mongodbs') && $d->mongodbs()->exists()) - || (method_exists($d, 'redis') && $d->redis()->exists()) - || (method_exists($d, 'keydbs') && $d->keydbs()->exists()) - || (method_exists($d, 'dragonflies') && $d->dragonflies()->exists()) - || (method_exists($d, 'clickhouses') && $d->clickhouses()->exists()) - || (method_exists($d, 'services') && $d->services()->exists()); + $hasAttached = $destination->applications()->exists() + || $destination->postgresqls()->exists() + || (method_exists($destination, 'mysqls') && $destination->mysqls()->exists()) + || (method_exists($destination, 'mariadbs') && $destination->mariadbs()->exists()) + || (method_exists($destination, 'mongodbs') && $destination->mongodbs()->exists()) + || (method_exists($destination, 'redis') && $destination->redis()->exists()) + || (method_exists($destination, 'keydbs') && $destination->keydbs()->exists()) + || (method_exists($destination, 'dragonflies') && $destination->dragonflies()->exists()) + || (method_exists($destination, 'clickhouses') && $destination->clickhouses()->exists()) + || (method_exists($destination, 'services') && $destination->services()->exists()); if ($hasAttached) { return response()->json(['message' => 'Destination has attached resources, detach first.'], 409); } } - $d->delete(); + if ($destination instanceof StandaloneDocker) { + app(RemoveStandaloneDockerNetwork::class)->handle($destination); + } + + $destinationUuid = $destination->uuid; + $destinationName = $destination->name; + $destinationType = $destination instanceof SwarmDocker ? 'swarm' : 'standalone'; + $serverUuid = $destination->server?->uuid; + + $destination->delete(); + + auditLog('api.destination.deleted', [ + 'team_id' => $teamId, + 'destination_uuid' => $destinationUuid, + 'destination_name' => $destinationName, + 'destination_type' => $destinationType, + 'server_uuid' => $serverUuid, + ]); return response()->json(['message' => 'Deleted.']); } diff --git a/tests/Feature/Api/DestinationsApiTest.php b/tests/Feature/Api/DestinationsApiTest.php index cbbd6d0a6..9639c5d9e 100644 --- a/tests/Feature/Api/DestinationsApiTest.php +++ b/tests/Feature/Api/DestinationsApiTest.php @@ -1,5 +1,6 @@ assertOk(); $uuids = collect($response->json())->pluck('uuid'); - expect($uuids)->toContain($this->destination->uuid) + expect($response->json('0'))->not->toHaveKey('id') + ->and($uuids)->toContain($this->destination->uuid) ->not->toContain($otherDestination->uuid); }); }); @@ -110,6 +112,20 @@ function destinationsApiToken(User $user, Team $team, array $abilities): string $response->assertForbidden(); }); + test('rejects create requests from non-admin team members', function () { + $member = User::factory()->create(); + $this->team->members()->attach($member->id, ['role' => 'member']); + $memberToken = destinationsApiToken($member, $this->team, ['*']); + + $response = $this->withHeaders(destinationsApiHeaders($memberToken)) + ->postJson("/api/v1/servers/{$this->server->uuid}/destinations", [ + 'network' => 'member-network', + ]); + + $response->assertForbidden(); + expect(StandaloneDocker::where('server_id', $this->server->id)->where('network', 'member-network')->exists())->toBeFalse(); + }); + test('rejects non-json requests before creating a destination', function () { $response = $this->withHeaders([ 'Authorization' => 'Bearer '.$this->bearerToken, @@ -129,8 +145,8 @@ function destinationsApiToken(User $user, Team $team, array $abilities): string 'unexpected' => 'value', ]); - $response->assertStatus(422); - $response->assertJsonPath('fields.0', 'unexpected'); + $response->assertUnprocessable(); + $response->assertJsonValidationErrors(['unexpected']); }); test('rejects unsafe docker network names', function () { @@ -139,7 +155,7 @@ function destinationsApiToken(User $user, Team $team, array $abilities): string 'network' => 'bad;network', ]); - $response->assertStatus(422); + $response->assertUnprocessable(); $response->assertJsonValidationErrors(['network']); }); @@ -150,7 +166,7 @@ function destinationsApiToken(User $user, Team $team, array $abilities): string 'type' => 'swarm', ]); - $response->assertStatus(422); + $response->assertUnprocessable(); expect(SwarmDocker::where('server_id', $this->server->id)->where('network', 'wrong-type-network')->exists())->toBeFalse(); }); @@ -180,6 +196,42 @@ function destinationsApiToken(User $user, Team $team, array $abilities): string }); describe('DELETE /api/v1/destinations/{uuid}', function () { + test('requires a write token', function () { + $readOnlyToken = destinationsApiToken($this->user, $this->team, ['read']); + + $response = $this->withHeaders(destinationsApiHeaders($readOnlyToken)) + ->deleteJson("/api/v1/destinations/{$this->destination->uuid}"); + + $response->assertForbidden(); + $this->assertModelExists($this->destination); + }); + + test('rejects delete requests from non-admin team members', function () { + $member = User::factory()->create(); + $this->team->members()->attach($member->id, ['role' => 'member']); + $memberToken = destinationsApiToken($member, $this->team, ['*']); + + $response = $this->withHeaders(destinationsApiHeaders($memberToken)) + ->deleteJson("/api/v1/destinations/{$this->destination->uuid}"); + + $response->assertForbidden(); + $this->assertModelExists($this->destination); + }); + + test('deletes standalone destinations after removing the docker network', function () { + $cleanup = Mockery::mock(RemoveStandaloneDockerNetwork::class); + $cleanup->shouldReceive('handle') + ->once() + ->with(Mockery::on(fn (StandaloneDocker $destination) => $destination->is($this->destination))); + $this->app->instance(RemoveStandaloneDockerNetwork::class, $cleanup); + + $response = $this->withHeaders(destinationsApiHeaders($this->bearerToken)) + ->deleteJson("/api/v1/destinations/{$this->destination->uuid}"); + + $response->assertOk(); + $this->assertModelMissing($this->destination); + }); + test('blocks deleting a destination with an attached service', function () { $project = Project::factory()->create(['team_id' => $this->team->id]); $environment = $project->environments()->first();