From 4b541011c98dc38a88cf39dec83cd4541c16bfc8 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 30 Oct 2026 11:23:16 +0100 Subject: [PATCH 1/4] fix(api): hide locked values in resource and environment lists Closes #12391 Co-Authored-By: Claude Opus 5.5 --- .../Api/ApplicationsController.php | 7 +--- .../Controllers/Api/DatabasesController.php | 14 +------- .../Controllers/Api/ProjectController.php | 16 +++++---- .../Controllers/Api/ResourcesController.php | 3 +- .../Controllers/Api/ServicesController.php | 7 +--- bootstrap/helpers/api.php | 36 +++++++++++++++++++ ...eConfigAndLockedPasswordVisibilityTest.php | 12 +++++++ ...enderedComposeLockedValueRedactionTest.php | 21 +++++++++++ 8 files changed, 83 insertions(+), 33 deletions(-) diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index 94b03c889f..f17e29cb4c 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -154,12 +154,7 @@ class ApplicationsController extends Controller $application->settings?->makeHidden(['id', 'application_id', 'created_at', 'updated_at']); } - $payload = serializeApiResponse($application); - if ($application instanceof Application && filled(data_get($payload, 'docker_compose'))) { - $payload['docker_compose'] = redactLockedValuesInGeneratedCompose($payload['docker_compose'], $application); - } - - return $payload; + return redactLockedApiResourceValues($application, serializeApiResponse($application)); } private function applicationSettingsFromRequest(Request $request): array diff --git a/app/Http/Controllers/Api/DatabasesController.php b/app/Http/Controllers/Api/DatabasesController.php index 5522994dd0..9990cd74af 100644 --- a/app/Http/Controllers/Api/DatabasesController.php +++ b/app/Http/Controllers/Api/DatabasesController.php @@ -258,19 +258,7 @@ class DatabasesController extends Controller $this->hideNestedServerSecrets($database, $loadNestedServerSecrets); } - $payload = serializeApiResponse($database); - - // A locked password variable is never shown again, also not inside the connection URLs. - if (request()->attributes->get('can_read_sensitive', false) === true - && method_exists($database, 'isCredentialLocked') && $database->isCredentialLocked()) { - foreach (['internal_db_url', 'external_db_url'] as $key) { - if ($payload->has($key)) { - $payload->put($key, null); - } - } - } - - return $payload; + return redactLockedApiResourceValues($database, serializeApiResponse($database)); } private function hideNestedServerSecrets(Model $model, bool $loadRelations = false): void diff --git a/app/Http/Controllers/Api/ProjectController.php b/app/Http/Controllers/Api/ProjectController.php index ad515f3fc7..a79dc542cd 100644 --- a/app/Http/Controllers/Api/ProjectController.php +++ b/app/Http/Controllers/Api/ProjectController.php @@ -7,6 +7,7 @@ use App\Models\Project; use App\Support\ValidationPatterns; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; +use Illuminate\Support\Arr; use Illuminate\Support\Facades\Validator; use OpenApi\Attributes as OA; @@ -168,13 +169,16 @@ class ProjectController extends Controller return response()->json(['message' => 'Environment not found.'], 404); } // Redis connection URLs read REDIS_PASSWORD and REDIS_USERNAME; load the variables once, not per database. - $environment = $environment->load(['applications', 'postgresqls', 'redis.runtime_environment_variables', 'mongodbs', 'mysqls', 'mariadbs', 'services']); - collect(['applications', 'postgresqls', 'redis', 'mongodbs', 'mysqls', 'mariadbs', 'services']) - ->flatMap(fn (string $relation) => $environment->{$relation}) - ->each(fn ($resource) => exposeSensitiveFields($resource)); - $environment->redis->each(fn ($redis) => $redis->makeHidden('runtime_environment_variables')); + $relations = ['applications', 'postgresqls', 'redis', 'mongodbs', 'mysqls', 'mariadbs', 'services']; + $environment = $environment->load([...$relations, 'redis.runtime_environment_variables']); + $payload = $environment->withoutRelations()->toArray(); + foreach ($relations as $relation) { + $payload[$relation] = $environment->{$relation} + ->map(fn ($resource) => Arr::except(serializeApiResource($resource), 'runtime_environment_variables')) + ->all(); + } - return response()->json(serializeApiResponse($environment)); + return response()->json(serializeApiResponse($payload)); } #[OA\Post( diff --git a/app/Http/Controllers/Api/ResourcesController.php b/app/Http/Controllers/Api/ResourcesController.php index 5b7923e19f..16e4e89c09 100644 --- a/app/Http/Controllers/Api/ResourcesController.php +++ b/app/Http/Controllers/Api/ResourcesController.php @@ -69,8 +69,7 @@ class ResourcesController extends Controller } $resources = $resources->flatten(); $resources = $resources->map(function ($resource) { - exposeSensitiveFields($resource); - $payload = $resource->toArray(); + $payload = serializeApiResource($resource); $payload['status'] = $resource->status; $payload['type'] = $resource->type(); diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index 30d13ac286..1408ce2897 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -78,12 +78,7 @@ class ServicesController extends Controller $service->makeHidden(['value', 'real_value']); } - $payload = serializeApiResponse($service); - if ($service instanceof Service && filled(data_get($payload, 'docker_compose'))) { - $payload['docker_compose'] = redactLockedValuesInGeneratedCompose($payload['docker_compose'], $service); - } - - return $payload; + return redactLockedApiResourceValues($service, serializeApiResponse($service)); } /** diff --git a/bootstrap/helpers/api.php b/bootstrap/helpers/api.php index c4c5eb7a27..226df2a33a 100644 --- a/bootstrap/helpers/api.php +++ b/bootstrap/helpers/api.php @@ -10,6 +10,7 @@ use App\Models\ApplicationSetting; use App\Models\Environment; use App\Models\EnvironmentVariable; use App\Models\ServerSetting; +use App\Models\Service; use App\Models\StandaloneDocker; use App\Models\SwarmDocker; use App\Rules\ManualWebhookSecret; @@ -178,6 +179,41 @@ function exposeSensitiveFields(Model $model): Model return $model; } +/** + * Hide the locked (shown once) values that a serialized resource still contains, also for tokens + * that can read sensitive data: the generated compose of an application or service, and the + * connection URLs of a database with a locked password. + * + * @template T of array|SupportCollection + * + * @param T $payload + * @return T + */ +function redactLockedApiResourceValues(Model $resource, array|SupportCollection $payload): array|SupportCollection +{ + if (($resource instanceof Application || $resource instanceof Service) && filled($payload['docker_compose'] ?? null)) { + $payload['docker_compose'] = redactLockedValuesInGeneratedCompose($payload['docker_compose'], $resource); + } + + $urlKeys = array_filter(['internal_db_url', 'external_db_url'], fn (string $key): bool => filled($payload[$key] ?? null)); + if ($urlKeys !== [] && method_exists($resource, 'isCredentialLocked') && $resource->isCredentialLocked()) { + foreach ($urlKeys as $key) { + $payload[$key] = null; + } + } + + return $payload; +} + +/** + * Serialize a resource for a list API response: sensitive fields when the token can read them, + * without locked values. + */ +function serializeApiResource(Model $resource): array +{ + return redactLockedApiResourceValues($resource, exposeSensitiveFields($resource)->toArray()); +} + /** * Shared validation rules for application create and update API endpoints. * diff --git a/tests/Feature/Authorization/DatabaseConfigAndLockedPasswordVisibilityTest.php b/tests/Feature/Authorization/DatabaseConfigAndLockedPasswordVisibilityTest.php index f53b5d9cd4..4bcb117274 100644 --- a/tests/Feature/Authorization/DatabaseConfigAndLockedPasswordVisibilityTest.php +++ b/tests/Feature/Authorization/DatabaseConfigAndLockedPasswordVisibilityTest.php @@ -228,6 +228,18 @@ it('redacts a locked redis password and its connection urls in the api', functio $list = $this->withHeaders($headers)->getJson('/api/v1/databases')->assertOk(); expect($list->getContent())->not->toContain('locked-redis-secret'); + + $resources = $this->withHeaders($headers)->getJson('/api/v1/resources')->assertOk(); + expect($resources->getContent())->not->toContain('locked-redis-secret') + ->and($resources->json('0.uuid'))->toBe($redis->uuid) + ->and($resources->json('0.internal_db_url'))->toBeNull(); + + $environment = $this->withHeaders($headers) + ->getJson("/api/v1/projects/{$this->environment->project->uuid}/{$this->environment->uuid}") + ->assertOk(); + expect($environment->getContent())->not->toContain('locked-redis-secret') + ->and($environment->json('redis.0.uuid'))->toBe($redis->uuid) + ->and($environment->json('redis.0.internal_db_url'))->toBeNull(); }); it('still returns the connection url of an unlocked redis password in the api', function () { diff --git a/tests/Feature/Authorization/RenderedComposeLockedValueRedactionTest.php b/tests/Feature/Authorization/RenderedComposeLockedValueRedactionTest.php index a4fb6566cc..9406a80c16 100644 --- a/tests/Feature/Authorization/RenderedComposeLockedValueRedactionTest.php +++ b/tests/Feature/Authorization/RenderedComposeLockedValueRedactionTest.php @@ -164,3 +164,24 @@ it('hides locked values in the generated compose returned by the API', function expectLockedValuesRedacted($response->json('docker_compose')); expect($response->json('docker_compose_raw'))->toBe(LOCKED_COMPOSE_RAW); })->with(['service', 'application']); + +it('hides locked values in the generated compose returned by the resource list API', function () { + $response = $this->withToken(lockedComposeApiToken())->getJson('/api/v1/resources')->assertOk(); + + foreach ([$this->service, $this->application] as $resource) { + expectLockedValuesRedacted(collect($response->json())->firstWhere('uuid', $resource->uuid)['docker_compose']); + } +}); + +it('hides locked values in the generated compose returned by the environment API', function (string $relation, string $resource) { + $response = $this->withToken(lockedComposeApiToken()) + ->getJson("/api/v1/projects/{$this->project->uuid}/{$this->environment->uuid}") + ->assertOk(); + + expectLockedValuesRedacted($response->json("{$relation}.0.docker_compose")); + expect($response->json("{$relation}.0.uuid"))->toBe($this->{$resource}->uuid) + ->and($response->json("{$relation}.0.docker_compose_raw"))->toBe(LOCKED_COMPOSE_RAW); +})->with([ + 'service' => ['services', 'service'], + 'application' => ['applications', 'application'], +]); From 3e2fff0609ead3c802d674c0cce72fbb51fbea74 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 30 Oct 2026 11:43:04 +0100 Subject: [PATCH 2/4] fix(deployments): stop the build server helper when a deployment is cancelled Save build_server_id as soon as a build server is selected, so the cancel cleanup finds the helper container on the build server. Let the final helper stop run after a cancel. Closes #12383 Co-Authored-By: Claude Opus 5.5 --- app/Jobs/ApplicationDeploymentJob.php | 6 +- app/Traits/ExecuteRemoteCommand.php | 13 ++-- .../Feature/BuildServerFallbackPolicyTest.php | 38 +++++++--- .../DeploymentCancelHelperShutdownTest.php | 70 +++++++++++++++++++ 4 files changed, 111 insertions(+), 16 deletions(-) create mode 100644 tests/Feature/DeploymentCancelHelperShutdownTest.php diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index b8c0f1160d..404d3a9ad3 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -508,7 +508,8 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue } $this->build_server = $buildServers->random(); - $this->application_deployment_queue->build_server_id = $this->build_server->id; + // Saved now: the cancel action looks for the helper container on this server. + $this->application_deployment_queue->update(['build_server_id' => $this->build_server->id]); $this->application_deployment_queue->addLogEntry("Found a suitable build server ({$this->build_server->name})."); $this->use_build_server = true; } @@ -5244,8 +5245,9 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); $timeout = $this->application->settings->deploymentStopGracePeriodSeconds(); if ($skipRemove) { + // The helper container (started with --rm) must also stop when the deployment was cancelled. $this->execute_remote_command( - [dockerStopCommand($timeout, escapeshellarg($containerName), $this->server), 'hidden' => true, 'ignore_errors' => true] + [dockerStopCommand($timeout, escapeshellarg($containerName), $this->server), 'hidden' => true, 'ignore_errors' => true, 'skip_cancellation_check' => true] ); } else { $this->execute_remote_command( diff --git a/app/Traits/ExecuteRemoteCommand.php b/app/Traits/ExecuteRemoteCommand.php index 21912fb82f..fe9d0a4c7b 100644 --- a/app/Traits/ExecuteRemoteCommand.php +++ b/app/Traits/ExecuteRemoteCommand.php @@ -87,6 +87,7 @@ trait ExecuteRemoteCommand $command_hidden = data_get($single_command, 'command_hidden', false); $skip_command_log = data_get($single_command, 'skip_command_log', false); $input = data_get($single_command, 'input'); + $skip_cancellation_check = data_get($single_command, 'skip_cancellation_check', false); $this->save = data_get($single_command, 'save'); if ($this->server->isNonRoot()) { if (str($command)->startsWith('docker exec')) { @@ -96,8 +97,8 @@ trait ExecuteRemoteCommand } } - // Check for cancellation before executing commands - if (isset($this->application_deployment_queue)) { + // Check for cancellation before executing commands. Cleanup commands must also run after a cancel. + if (! $skip_cancellation_check && isset($this->application_deployment_queue)) { $this->application_deployment_queue->refresh(); if ($this->application_deployment_queue->status === ApplicationDeploymentStatus::CANCELLED_BY_USER->value) { throw new \RuntimeException('Deployment cancelled by user', 69420); @@ -126,9 +127,11 @@ trait ExecuteRemoteCommand $this->addRetryLogEntry($attempt, $maxRetries, $delay, $errorMessage); // Check for cancellation during retry wait - $this->application_deployment_queue->refresh(); - if ($this->application_deployment_queue->status === ApplicationDeploymentStatus::CANCELLED_BY_USER->value) { - throw new \RuntimeException('Deployment cancelled by user during retry', 69420); + if (! $skip_cancellation_check) { + $this->application_deployment_queue->refresh(); + if ($this->application_deployment_queue->status === ApplicationDeploymentStatus::CANCELLED_BY_USER->value) { + throw new \RuntimeException('Deployment cancelled by user during retry', 69420); + } } } diff --git a/tests/Feature/BuildServerFallbackPolicyTest.php b/tests/Feature/BuildServerFallbackPolicyTest.php index aee3c54384..f7a366e1c4 100644 --- a/tests/Feature/BuildServerFallbackPolicyTest.php +++ b/tests/Feature/BuildServerFallbackPolicyTest.php @@ -148,7 +148,7 @@ test('strict teams use an available dedicated build server', function () { ]); [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer); - $deploymentQueue->shouldReceive('setAttribute')->with('build_server_id', $buildServer->id)->once()->andReturnSelf(); + $deploymentQueue->shouldReceive('update')->with(['build_server_id' => $buildServer->id])->once()->andReturnTrue(); $deploymentQueue->shouldReceive('addLogEntry') ->once() ->with("Found a suitable build server ({$buildServer->name})."); @@ -179,7 +179,7 @@ test('build server selection only picks dedicated build servers', function () { $buildServer = makeRoleServer($team, 'build'); [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer); - $deploymentQueue->shouldReceive('setAttribute')->with('build_server_id', $buildServer->id)->once()->andReturnSelf(); + $deploymentQueue->shouldReceive('update')->with(['build_server_id' => $buildServer->id])->once()->andReturnTrue(); $deploymentQueue->shouldReceive('addLogEntry')->once()->with("Found a suitable build server ({$buildServer->name})."); invokeBuildServerSelection($job); @@ -194,7 +194,7 @@ test('fallback builds on the deployment server instead of another combined serve makeRoleServer($team, 'both'); [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer); - $deploymentQueue->shouldNotReceive('setAttribute'); + $deploymentQueue->shouldNotReceive('update'); $deploymentQueue->shouldReceive('addLogEntry')->once()->with('No suitable build server found. Using the deployment server.'); invokeBuildServerSelection($job); @@ -210,7 +210,7 @@ test('deployments-only servers build on a dedicated build server without the app $buildServer = makeRoleServer($team, 'build'); [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer, ['docker_registry_image_name' => 'ghcr.io/coollabsio/app'], buildServerEnabled: false); - $deploymentQueue->shouldReceive('setAttribute')->with('build_server_id', $buildServer->id)->once()->andReturnSelf(); + $deploymentQueue->shouldReceive('update')->with(['build_server_id' => $buildServer->id])->once()->andReturnTrue(); $deploymentQueue->shouldReceive('addLogEntry')->once()->with("Found a suitable build server ({$buildServer->name})."); invokeBuildServerSelection($job); @@ -252,7 +252,7 @@ test('deployments-only servers restart without a registry image', function () { $buildServer = makeRoleServer($team, 'build'); [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer, buildServerEnabled: false, restartOnly: true); - $deploymentQueue->shouldReceive('setAttribute')->with('build_server_id', $buildServer->id)->once()->andReturnSelf(); + $deploymentQueue->shouldReceive('update')->with(['build_server_id' => $buildServer->id])->once()->andReturnTrue(); $deploymentQueue->shouldReceive('addLogEntry')->once()->with("Found a suitable build server ({$buildServer->name})."); invokeBuildServerSelection($job); @@ -312,7 +312,7 @@ test('additional deployments-only servers pull the registry image without a buil [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer, ['docker_registry_image_name' => 'ghcr.io/coollabsio/app'], $buildServerEnabled); markAsAdditionalServer($job); - $deploymentQueue->shouldNotReceive('setAttribute'); + $deploymentQueue->shouldNotReceive('update'); $deploymentQueue->shouldReceive('addLogEntry')->once()->with('Additional server: pulls the image from the registry, no build server needed.'); invokeBuildServerSelection($job); @@ -331,7 +331,7 @@ test('additional servers do not use a build server when the application enables [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer, ['docker_registry_image_name' => 'ghcr.io/coollabsio/app']); markAsAdditionalServer($job); - $deploymentQueue->shouldNotReceive('setAttribute'); + $deploymentQueue->shouldNotReceive('update'); $deploymentQueue->shouldReceive('addLogEntry')->once()->with('Additional server: pulls the image from the registry, no build server needed.'); invokeBuildServerSelection($job); @@ -436,7 +436,7 @@ test('strict teams restart on the deployment server when no build server is avai $deploymentServer = makeRoleServer($team, $role); [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer, buildServerEnabled: $buildServerEnabled, restartOnly: true); - $deploymentQueue->shouldNotReceive('setAttribute'); + $deploymentQueue->shouldNotReceive('update'); $deploymentQueue->shouldReceive('addLogEntry')->once()->with('No suitable build server found. Using the deployment server.'); invokeBuildServerSelection($job); @@ -454,7 +454,7 @@ test('strict teams restart on an available build server', function () { $buildServer = makeRoleServer($team, 'build'); [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer, buildServerEnabled: false, restartOnly: true); - $deploymentQueue->shouldReceive('setAttribute')->with('build_server_id', $buildServer->id)->once()->andReturnSelf(); + $deploymentQueue->shouldReceive('update')->with(['build_server_id' => $buildServer->id])->once()->andReturnTrue(); $deploymentQueue->shouldReceive('addLogEntry')->once()->with("Found a suitable build server ({$buildServer->name})."); invokeBuildServerSelection($job); @@ -481,3 +481,23 @@ test('a restart that has to rebuild applies the build server rules again', funct 'deployments-only server' => ['deployment', false, 'is set to deployments only, and no usable build server was found.'], 'application uses a build server' => ['both', true, 'No available dedicated build server was found.'], ]); + +test('the selected build server is saved on the deployment so a cancel finds the helper container', function () { + $team = Team::factory()->create(); + $deploymentServer = makeRoleServer($team, 'both'); + $buildServer = makeRoleServer($team, 'build'); + [$job] = makeBuildServerSelectionJob($team, $deploymentServer); + $deployment = ApplicationDeploymentQueue::query()->create([ + 'application_id' => '1', + 'deployment_uuid' => 'build-server-cancel-test', + 'server_id' => $deploymentServer->id, + ]); + (new ReflectionProperty(ApplicationDeploymentJob::class, 'application_deployment_queue'))->setValue($job, $deployment); + + invokeBuildServerSelection($job); + // execute_remote_command() refreshes the deployment before each command. + $deployment->refresh(); + + expect($deployment->build_server_id)->toBe($buildServer->id) + ->and(ApplicationDeploymentQueue::query()->find($deployment->id)->build_server_id)->toBe($buildServer->id); +}); diff --git a/tests/Feature/DeploymentCancelHelperShutdownTest.php b/tests/Feature/DeploymentCancelHelperShutdownTest.php new file mode 100644 index 0000000000..06bb2aae70 --- /dev/null +++ b/tests/Feature/DeploymentCancelHelperShutdownTest.php @@ -0,0 +1,70 @@ + 0]); + Process::fake(); +}); + +function makeCancelledDeploymentJob(): array +{ + $team = Team::factory()->create(); + $server = Server::factory()->create([ + 'team_id' => $team->id, + 'private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id, + ]); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + $application = Application::factory()->create(['environment_id' => $environment->id]); + $deployment = ApplicationDeploymentQueue::query()->create([ + 'application_id' => $application->id, + 'deployment_uuid' => 'cancelled-helper', + 'server_id' => $server->id, + 'status' => ApplicationDeploymentStatus::CANCELLED_BY_USER->value, + ]); + + $job = (new ReflectionClass(ApplicationDeploymentJob::class))->newInstanceWithoutConstructor(); + foreach ([ + 'application' => $application, + 'application_deployment_queue' => $deployment, + 'server' => $server, + 'saved_outputs' => collect(), + ] as $property => $value) { + (new ReflectionProperty(ApplicationDeploymentJob::class, $property))->setValue($job, $value); + } + + return [$job, $deployment]; +} + +test('a cancelled deployment still stops its helper container', function () { + [$job, $deployment] = makeCancelledDeploymentJob(); + + (new ReflectionMethod(ApplicationDeploymentJob::class, 'graceful_shutdown_container'))->invoke($job, 'cancelled-helper', true); + + Process::assertRan(fn (PendingProcess $process) => str_contains(remoteProcessScript($process), 'docker stop') && str_contains(remoteProcessScript($process), 'cancelled-helper')); + expect($deployment->refresh()->logs ?? '')->not->toContain('Error stopping container'); +}); + +test('a cancelled deployment does not run other remote commands', function () { + [$job] = makeCancelledDeploymentJob(); + + expect(fn () => $job->execute_remote_command(['echo build', 'hidden' => true])) + ->toThrow(RuntimeException::class, 'Deployment cancelled by user'); + + Process::assertNothingRan(); +}); From 98b7de0ea77a4c81a817cb3aa4905fadd169b7d8 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 30 Oct 2026 11:43:13 +0100 Subject: [PATCH 3/4] fix(api): accept the same port mappings as the UI Closes #12388 --- .../Api/ApplicationsController.php | 19 ++-- bootstrap/helpers/api.php | 2 +- .../ApplicationPortMappingsApiTest.php | 93 +++++++++++++++++++ 3 files changed, 106 insertions(+), 8 deletions(-) create mode 100644 tests/Feature/ApplicationPortMappingsApiTest.php diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index f17e29cb4c..7c6b35589c 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -5037,20 +5037,25 @@ class ApplicationsController extends Controller ], 422); } - // Validate ports_mappings - if ($request->has('ports_mappings')) { - $ports = []; + // Validate that each host binding (IP, port, protocol) of ports_mappings is unique + if (is_string($request->ports_mappings)) { + $hostBindings = []; foreach (explode(',', $request->ports_mappings) as $portMapping) { - $port = explode(':', $portMapping); - if (in_array($port[0], $ports)) { + // The container port is after the last ":", an empty host port makes Docker pick a free port + $hostBinding = preg_replace('#/(tcp|udp|sctp)#', '', substr($portMapping, 0, (int) strrpos($portMapping, ':'))); + if ($hostBinding === '' || str_ends_with($hostBinding, ':')) { + continue; + } + $protocol = preg_match('#/(udp|sctp)#', $portMapping, $matches) === 1 ? $matches[1] : 'tcp'; + if (in_array("{$hostBinding}/{$protocol}", $hostBindings, true)) { return response()->json([ 'message' => 'Validation failed.', 'errors' => [ - 'ports_mappings' => 'The first number before : should be unique between mappings.', + 'ports_mappings' => 'Each host IP, port, and protocol can only be used once.', ], ], 422); } - $ports[] = $port[0]; + $hostBindings[] = "{$hostBinding}/{$protocol}"; } } // Validate custom_labels diff --git a/bootstrap/helpers/api.php b/bootstrap/helpers/api.php index 226df2a33a..42e739c5bd 100644 --- a/bootstrap/helpers/api.php +++ b/bootstrap/helpers/api.php @@ -278,7 +278,7 @@ function sharedDataApplications(?Application $application = null): array 'build_command' => ValidationPatterns::shellSafeCommandRules(), 'start_command' => ValidationPatterns::shellSafeCommandRules(), 'ports_exposes' => 'string|regex:/^(\d+)(,\d+)*$/', - 'ports_mappings' => 'string|regex:/^(\d+:\d+)(,\d+:\d+)*$/|nullable', + 'ports_mappings' => ValidationPatterns::portMappingRules(), 'custom_network_aliases' => 'string|nullable', 'base_directory' => ValidationPatterns::directoryPathRules(), 'publish_directory' => ValidationPatterns::directoryPathRules(), diff --git a/tests/Feature/ApplicationPortMappingsApiTest.php b/tests/Feature/ApplicationPortMappingsApiTest.php new file mode 100644 index 0000000000..addb19355c --- /dev/null +++ b/tests/Feature/ApplicationPortMappingsApiTest.php @@ -0,0 +1,93 @@ + InstanceSettings::firstOrCreate(['id' => 0])); + + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'owner']); + session(['currentTeam' => $this->team]); + + $plainTextToken = Str::random(40); + $token = $this->user->tokens()->create([ + 'name' => 'port-mappings-api-test-'.Str::random(6), + 'token' => hash('sha256', $plainTextToken), + 'abilities' => ['*'], + 'team_id' => $this->team->id, + ]); + $this->bearerToken = $token->getKey().'|'.$plainTextToken; + + $server = Server::factory()->create(['team_id' => $this->team->id]); + $destination = StandaloneDocker::where('server_id', $server->id)->first(); + $project = Project::factory()->create(['team_id' => $this->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(), + ]); +}); + +function patchPortMappings(string $bearerToken, Application $application, ?string $portsMappings) +{ + return test()->withHeaders(['Authorization' => 'Bearer '.$bearerToken]) + ->patchJson("/api/v1/applications/{$application->uuid}", ['ports_mappings' => $portsMappings]); +} + +test('accepts every port mapping format that the UI accepts', function (string $portsMappings) { + patchPortMappings($this->bearerToken, $this->application, $portsMappings)->assertOk(); + + expect($this->application->refresh()->ports_mappings)->toBe($portsMappings); +})->with([ + 'plain' => '8080:80', + 'udp protocol' => '8080:80/udp', + 'sctp protocol' => '8080:80/sctp', + 'port range' => '8000-8010:8000-8010', + 'ipv4 binding' => '127.0.0.1:8080:80', + 'ipv6 binding with random host port' => '[::1]::80', + 'same port for tcp and udp' => '53:53/tcp,53:53/udp', + 'same port on different host ips' => '127.0.0.1:8080:80,10.0.0.1:8080:80', + 'random host ports on one ip' => '127.0.0.1::80,127.0.0.1::81', +]); + +test('clears port mappings with null', function () { + $this->application->update(['ports_mappings' => '8080:80']); + + patchPortMappings($this->bearerToken, $this->application, null)->assertOk(); + + expect($this->application->refresh()->ports_mappings)->toBeNull(); +}); + +test('rejects invalid port mappings', function (string $portsMappings) { + patchPortMappings($this->bearerToken, $this->application, $portsMappings) + ->assertUnprocessable() + ->assertJsonValidationErrors('ports_mappings'); +})->with([ + 'missing container port' => '8080', + 'unknown protocol' => '8080:80/icmp', + 'shell characters' => '8080:80;id', +]); + +test('rejects duplicate host bindings', function (string $portsMappings) { + patchPortMappings($this->bearerToken, $this->application, $portsMappings) + ->assertUnprocessable() + ->assertJsonPath('errors.ports_mappings', 'Each host IP, port, and protocol can only be used once.'); +})->with([ + 'same host port' => '8080:80,8080:81', + 'same host port with explicit tcp' => '8080:80,8080:81/tcp', + 'same ip and host port' => '127.0.0.1:8080:80,127.0.0.1:8080:81', +]); From b3ec26c3b1771654cae4b4ebc06d14a164bd8824 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:01:21 +0100 Subject: [PATCH 4/4] fix(proxy): stop routing the Reverb HTTP API through the dashboard domain The browser uses only the /app/{key} WebSocket. Laravel sends events to Reverb over 127.0.0.1, so the generated Traefik and Caddy configs no longer route /apps to port 6001, and the WebSocket rule only matches /app/. Closes #12390 Co-Authored-By: Claude Opus 5.5 --- app/Models/Server.php | 22 ++----------------- .../Proxy/TraefikCertificateResolverTest.php | 6 ++++- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/app/Models/Server.php b/app/Models/Server.php index d13f1f468f..c2e36b1fd6 100644 --- a/app/Models/Server.php +++ b/app/Models/Server.php @@ -1136,14 +1136,7 @@ class Server extends BaseModel 0 => 'http', ], 'service' => 'coolify-reverb', - 'rule' => "Host(`{$host}`) && PathPrefix(`/app`)", - ], - 'coolify-reverb-api' => [ - 'entryPoints' => [ - 0 => 'http', - ], - 'service' => 'coolify-reverb', - 'rule' => "Host(`{$host}`) && PathPrefix(`/apps`)", + 'rule' => "Host(`{$host}`) && PathPrefix(`/app/`)", ], 'coolify-terminal-ws' => [ 'entryPoints' => [ @@ -1206,15 +1199,7 @@ class Server extends BaseModel 0 => 'https', ], 'service' => 'coolify-reverb', - 'rule' => "Host(`{$host}`) && PathPrefix(`/app`)", - 'tls' => $tls, - ]; - $traefik_dynamic_conf['http']['routers']['coolify-reverb-api-https'] = [ - 'entryPoints' => [ - 0 => 'https', - ], - 'service' => 'coolify-reverb', - 'rule' => "Host(`{$host}`) && PathPrefix(`/apps`)", + 'rule' => "Host(`{$host}`) && PathPrefix(`/app/`)", 'tls' => $tls, ]; $traefik_dynamic_conf['http']['routers']['coolify-terminal-wss'] = [ @@ -1256,9 +1241,6 @@ $siteAddress { handle /app/* { reverse_proxy coolify:6001 } - handle /apps* { - reverse_proxy coolify:6001 - } handle /terminal/ws { reverse_proxy coolify:6002 } diff --git a/tests/Feature/Proxy/TraefikCertificateResolverTest.php b/tests/Feature/Proxy/TraefikCertificateResolverTest.php index 51bbe605ca..e9de25c522 100644 --- a/tests/Feature/Proxy/TraefikCertificateResolverTest.php +++ b/tests/Feature/Proxy/TraefikCertificateResolverTest.php @@ -95,10 +95,14 @@ test('coolify dashboard routes follow the server certificate resolver', function preg_match("/echo '([A-Za-z0-9+\/=]+)' \| base64 -d/", $command, $matches); $routers = Yaml::parse(base64_decode($matches[1]))['http']['routers']; - foreach (['coolify-https', 'coolify-reverb-wss', 'coolify-reverb-api-https', 'coolify-terminal-wss'] as $router) { + foreach (['coolify-https', 'coolify-reverb-wss', 'coolify-terminal-wss'] as $router) { expect($routers[$router])->toHaveKey('tls') ->and($routers[$router]['tls'] ?? [])->toBe($expectedTls); } + + expect($routers)->not->toHaveKeys(['coolify-reverb-api', 'coolify-reverb-api-https']) + ->and($routers['coolify-reverb-ws']['rule'])->toBe('Host(`coolify.example.com`) && PathPrefix(`/app/`)') + ->and($routers['coolify-reverb-wss']['rule'])->toBe('Host(`coolify.example.com`) && PathPrefix(`/app/`)'); })->with([ 'letsencrypt' => ['letsencrypt', ['certresolver' => 'letsencrypt']], 'no resolver' => [null, []],