From 13db52f730293331a7484c4ab4d08e4ea44d679b Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:18:48 +0200 Subject: [PATCH 01/25] fix(scheduler): accept ScheduledJobManager jobs queued before the upgrade Jobs that v4.3.23 put in the queue have no type property. The promoted constructor property had no default, so these jobs failed with "must not be accessed before initialization" after the upgrade. - Declare type as a normal property with a null default --- app/Jobs/ScheduledJobManager.php | 10 +++++++++- .../ScheduledJobManagerLegacyPayloadTest.php | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 tests/Unit/ScheduledJobManagerLegacyPayloadTest.php diff --git a/app/Jobs/ScheduledJobManager.php b/app/Jobs/ScheduledJobManager.php index 4c3beb97cd..c218a53652 100644 --- a/app/Jobs/ScheduledJobManager.php +++ b/app/Jobs/ScheduledJobManager.php @@ -64,15 +64,23 @@ class ScheduledJobManager implements ShouldQueue private int $skippedCount = 0; + /** + * One key of TYPES, or null for all types. + * + * Declared with a default instead of promoted, so jobs queued by older versions without this property still run. + */ + public ?string $type = null; + /** * @param string|null $type One key of TYPES, or null for all types. */ - public function __construct(public ?string $type = null) + public function __construct(?string $type = null) { if ($type !== null && ! array_key_exists($type, self::TYPES)) { throw new \InvalidArgumentException("Unknown scheduled job type [{$type}]."); } + $this->type = $type; $this->onQueue(crons_queue()); } diff --git a/tests/Unit/ScheduledJobManagerLegacyPayloadTest.php b/tests/Unit/ScheduledJobManagerLegacyPayloadTest.php new file mode 100644 index 0000000000..4aa846c684 --- /dev/null +++ b/tests/Unit/ScheduledJobManagerLegacyPayloadTest.php @@ -0,0 +1,18 @@ +middleware(); + + expect($job->type)->toBeNull() + ->and($middleware[0])->toBeInstanceOf(WithoutOverlapping::class) + ->and($middleware[0]->key)->toBe('scheduled-job-manager'); +}); From eb2a6a5c82f52d9c27d82ca7a0481fc0ac4723f6 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:18:55 +0200 Subject: [PATCH 02/25] fix(deployments): fail a deployment when its job cannot be dispatched When the job constructor or the dispatch threw (for example, an invalid registry image name), the deployment stayed in_progress. It blocked later deployments of the application and used a build slot. - Mark the deployment failed with fail_undispatchable_deployment() - Throw the error again, so the UI and API still show it --- bootstrap/helpers/applications.php | 12 +++-- ...ImmediateDeploymentDispatchFailureTest.php | 53 +++++++++++++++++++ 2 files changed, 62 insertions(+), 3 deletions(-) create mode 100644 tests/Feature/ImmediateDeploymentDispatchFailureTest.php diff --git a/bootstrap/helpers/applications.php b/bootstrap/helpers/applications.php index b518abff7d..e7731e7ae0 100644 --- a/bootstrap/helpers/applications.php +++ b/bootstrap/helpers/applications.php @@ -116,9 +116,15 @@ function queue_application_deployment(Application $application, string $deployme } if ($admission['started']) { - ApplicationDeploymentJob::dispatch( - application_deployment_queue_id: $deployment->id, - ); + try { + ApplicationDeploymentJob::dispatch( + application_deployment_queue_id: $deployment->id, + ); + } catch (Throwable $exception) { + fail_undispatchable_deployment($deployment, $exception); + + throw $exception; + } } return [ diff --git a/tests/Feature/ImmediateDeploymentDispatchFailureTest.php b/tests/Feature/ImmediateDeploymentDispatchFailureTest.php new file mode 100644 index 0000000000..61817a87cf --- /dev/null +++ b/tests/Feature/ImmediateDeploymentDispatchFailureTest.php @@ -0,0 +1,53 @@ + InstanceSettings::firstOrCreate(['id' => 0])); + + $team = Team::factory()->create(); + $server = Server::factory()->create(['team_id' => $team->id]); + $destination = StandaloneDocker::where('server_id', $server->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = $project->environments()->first() + ?? Environment::factory()->create(['project_id' => $project->id]); + $this->application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + 'docker_registry_image_name' => 'bad name;x', + ]); + + Queue::fake(); + Notification::fake(); +}); + +test('a deployment whose job cannot be dispatched fails and does not block the next deployment', function () { + expect(fn () => queue_application_deployment($this->application, 'prr-undispatchable')) + ->toThrow(DeploymentException::class, 'Docker registry image name contains invalid characters.'); + + $failedDeployment = ApplicationDeploymentQueue::where('deployment_uuid', 'prr-undispatchable')->firstOrFail(); + expect($failedDeployment->status)->toBe('failed') + ->and($failedDeployment->logs)->toContain('Deployment could not be started'); + + $this->application->update(['docker_registry_image_name' => 'ghcr.io/coollabs/app']); + queue_application_deployment($this->application->fresh(), 'prr-next-deployment'); + + expect(ApplicationDeploymentQueue::where('deployment_uuid', 'prr-next-deployment')->value('status'))->toBe('in_progress'); + Queue::assertPushed(ApplicationDeploymentJob::class, 1); +}); From d599d04053c4d11c872d415e09e88c2008efab8e Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:19:08 +0200 Subject: [PATCH 03/25] fix(deployments): advance the queue of an additional server When a deployment on an additional server finished, only the queue of the primary server advanced. Deployments queued on the additional server could wait with no limit. - Also advance the queue of the server of the finished deployment --- app/Jobs/ApplicationDeploymentJob.php | 2 +- bootstrap/helpers/applications.php | 13 ++- ...tionalServerDeploymentQueueAdvanceTest.php | 80 +++++++++++++++++++ 3 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 tests/Feature/AdditionalServerDeploymentQueueAdvanceTest.php diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 3379b267d4..3a63d36e34 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -5667,7 +5667,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); } try { - queue_next_deployment($this->application); + queue_next_deployment($this->application, $this->application_deployment_queue->server_id); } catch (Throwable $e) { $this->logStatusTransitionSideEffectFailure('Starting the next queued deployment failed', $e); } diff --git a/bootstrap/helpers/applications.php b/bootstrap/helpers/applications.php index e7731e7ae0..23dfbe0832 100644 --- a/bootstrap/helpers/applications.php +++ b/bootstrap/helpers/applications.php @@ -142,10 +142,17 @@ function force_start_deployment(ApplicationDeploymentQueue $deployment): bool { return start_queued_deployment($deployment, force: true); } -function queue_next_deployment(Application $application) +/** + * Start the queued deployments that can run now on the application's primary server and on the + * server of the deployment that just ended (an additional server). + */ +function queue_next_deployment(Application $application, ?int $finished_deployment_server_id = null) { - $server_id = $application->destination->server_id; - $queued_deployments = ApplicationDeploymentQueue::where('server_id', $server_id) + $server_ids = collect([$application->destination->server_id, $finished_deployment_server_id]) + ->filter(fn ($server_id) => $server_id !== null) + ->unique() + ->values(); + $queued_deployments = ApplicationDeploymentQueue::whereIn('server_id', $server_ids) ->where('status', ApplicationDeploymentStatus::QUEUED) ->get() ->sortBy('created_at'); diff --git a/tests/Feature/AdditionalServerDeploymentQueueAdvanceTest.php b/tests/Feature/AdditionalServerDeploymentQueueAdvanceTest.php new file mode 100644 index 0000000000..65a53e6f59 --- /dev/null +++ b/tests/Feature/AdditionalServerDeploymentQueueAdvanceTest.php @@ -0,0 +1,80 @@ + InstanceSettings::firstOrCreate(['id' => 0])); + + $team = Team::factory()->create(); + $primaryServer = Server::factory()->create(['team_id' => $team->id]); + $this->additionalServer = Server::factory()->create(['team_id' => $team->id]); + $this->additionalServer->settings->update(['concurrent_builds' => 1]); + $primaryDestination = StandaloneDocker::where('server_id', $primaryServer->id)->firstOrFail(); + $this->additionalDestination = StandaloneDocker::where('server_id', $this->additionalServer->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = $project->environments()->first() + ?? Environment::factory()->create(['project_id' => $project->id]); + + $makeApplication = function (Server $deploymentServer, StandaloneDocker $deploymentDestination) use ($environment): Application { + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $deploymentDestination->id, + 'destination_type' => $deploymentDestination->getMorphClass(), + ]); + $application->settings->update(['is_consistent_container_name_enabled' => true]); + + return $application; + }; + $this->multiServerApplication = $makeApplication($primaryServer, $primaryDestination); + $this->multiServerApplication->additional_servers()->attach($this->additionalServer->id, [ + 'standalone_docker_id' => $this->additionalDestination->id, + ]); + $this->otherApplication = $makeApplication($this->additionalServer, $this->additionalDestination); + + Process::fake(['*' => Process::result(output: '')]); + Queue::fake(); + Notification::fake(); +}); + +test('a finished additional-server deployment starts the next deployment queued on that server', function () { + $childDeployment = ApplicationDeploymentQueue::create([ + 'application_id' => $this->multiServerApplication->id, + 'deployment_uuid' => 'prr-child-deployment', + 'status' => 'in_progress', + 'server_id' => $this->additionalServer->id, + 'destination_id' => $this->additionalDestination->id, + 'commit' => 'HEAD', + 'pull_request_id' => 0, + 'parent_deployment_uuid' => 'prr-parent-deployment', + ]); + $waitingDeployment = ApplicationDeploymentQueue::create([ + 'application_id' => $this->otherApplication->id, + 'deployment_uuid' => 'prr-waiting-deployment', + 'status' => 'queued', + 'server_id' => $this->additionalServer->id, + 'destination_id' => $this->additionalDestination->id, + 'commit' => 'HEAD', + 'pull_request_id' => 0, + ]); + + (new ApplicationDeploymentJob($childDeployment->id))->failed(new RuntimeException('build failed')); + + expect($childDeployment->fresh()->status)->toBe('failed') + ->and($waitingDeployment->fresh()->status)->toBe('in_progress'); + Queue::assertPushed(ApplicationDeploymentJob::class, fn (ApplicationDeploymentJob $job) => $job->application_deployment_queue_id === $waitingDeployment->id); +}); From df51b23bc28b450d468d3bedbb7d0e35dc6139e9 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:19:11 +0200 Subject: [PATCH 04/25] fix(deployments): start queued deployments of the application on all servers When a deployment on the primary server finished, deployments of the same application queued on an additional server did not start. They waited for the primary deployment, and then no queue event came for their server. - Also advance every server that has a queued deployment of the application; the per-server and per-application limits still apply --- bootstrap/helpers/applications.php | 10 ++- ...ymentAdvancesAdditionalServerQueueTest.php | 87 +++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/PrimaryDeploymentAdvancesAdditionalServerQueueTest.php diff --git a/bootstrap/helpers/applications.php b/bootstrap/helpers/applications.php index 23dfbe0832..8b90de0f40 100644 --- a/bootstrap/helpers/applications.php +++ b/bootstrap/helpers/applications.php @@ -143,12 +143,18 @@ function force_start_deployment(ApplicationDeploymentQueue $deployment): bool return start_queued_deployment($deployment, force: true); } /** - * Start the queued deployments that can run now on the application's primary server and on the - * server of the deployment that just ended (an additional server). + * Start the queued deployments that can run now on the application's primary server, on the + * server of the deployment that just ended (an additional server), and on every other server + * with a queued deployment of this application, because those waited for this one to end. */ function queue_next_deployment(Application $application, ?int $finished_deployment_server_id = null) { + $application_queued_server_ids = ApplicationDeploymentQueue::where('application_id', $application->id) + ->where('status', ApplicationDeploymentStatus::QUEUED) + ->distinct() + ->pluck('server_id'); $server_ids = collect([$application->destination->server_id, $finished_deployment_server_id]) + ->merge($application_queued_server_ids) ->filter(fn ($server_id) => $server_id !== null) ->unique() ->values(); diff --git a/tests/Feature/PrimaryDeploymentAdvancesAdditionalServerQueueTest.php b/tests/Feature/PrimaryDeploymentAdvancesAdditionalServerQueueTest.php new file mode 100644 index 0000000000..6d93948e88 --- /dev/null +++ b/tests/Feature/PrimaryDeploymentAdvancesAdditionalServerQueueTest.php @@ -0,0 +1,87 @@ + InstanceSettings::firstOrCreate(['id' => 0])); + + $team = Team::factory()->create(); + $this->primaryServer = Server::factory()->create(['team_id' => $team->id]); + $this->additionalServer = Server::factory()->create(['team_id' => $team->id]); + $this->primaryServer->settings->update(['concurrent_builds' => 1]); + $this->additionalServer->settings->update(['concurrent_builds' => 1]); + $this->primaryDestination = StandaloneDocker::where('server_id', $this->primaryServer->id)->firstOrFail(); + $this->additionalDestination = StandaloneDocker::where('server_id', $this->additionalServer->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = $project->environments()->first() + ?? Environment::factory()->create(['project_id' => $project->id]); + + $makeApplication = function (StandaloneDocker $destination) use ($environment): Application { + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + ]); + $application->settings->update(['is_consistent_container_name_enabled' => true]); + + return $application; + }; + $this->application = $makeApplication($this->primaryDestination); + $this->application->additional_servers()->attach($this->additionalServer->id, [ + 'standalone_docker_id' => $this->additionalDestination->id, + ]); + $this->otherApplication = $makeApplication($this->additionalDestination); + + $this->makeDeployment = fn (Application $application, Server $server, StandaloneDocker $destination, string $status, string $uuid): ApplicationDeploymentQueue => ApplicationDeploymentQueue::create([ + 'application_id' => $application->id, + 'deployment_uuid' => $uuid, + 'status' => $status, + 'server_id' => $server->id, + 'destination_id' => $destination->id, + 'commit' => 'HEAD', + 'pull_request_id' => 0, + 'only_this_server' => true, + ]); + + Process::fake(['*' => Process::result(output: '')]); + Queue::fake(); + Notification::fake(); +}); + +test('a finished primary-server deployment starts the deployment of the same application queued on an additional server', function () { + $primaryDeployment = ($this->makeDeployment)($this->application, $this->primaryServer, $this->primaryDestination, 'in_progress', 'prr-primary-deployment'); + $additionalDeployment = ($this->makeDeployment)($this->application, $this->additionalServer, $this->additionalDestination, 'queued', 'prr-additional-deployment'); + + (new ApplicationDeploymentJob($primaryDeployment->id))->failed(new RuntimeException('build failed')); + + expect($primaryDeployment->fresh()->status)->toBe('failed') + ->and($additionalDeployment->fresh()->status)->toBe('in_progress'); + Queue::assertPushed(ApplicationDeploymentJob::class, 1); + Queue::assertPushed(ApplicationDeploymentJob::class, fn (ApplicationDeploymentJob $job) => $job->application_deployment_queue_id === $additionalDeployment->id); +}); + +test('a finished primary-server deployment keeps the concurrent build limit of the additional server', function () { + $primaryDeployment = ($this->makeDeployment)($this->application, $this->primaryServer, $this->primaryDestination, 'in_progress', 'prr-primary-deployment'); + ($this->makeDeployment)($this->otherApplication, $this->additionalServer, $this->additionalDestination, 'in_progress', 'prr-busy-deployment'); + $additionalDeployment = ($this->makeDeployment)($this->application, $this->additionalServer, $this->additionalDestination, 'queued', 'prr-additional-deployment'); + + (new ApplicationDeploymentJob($primaryDeployment->id))->failed(new RuntimeException('build failed')); + + expect($additionalDeployment->fresh()->status)->toBe('queued'); + Queue::assertNotPushed(ApplicationDeploymentJob::class); +}); From 0facde67a80d63d979bdfc14b39457bee0dcb591 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:19:19 +0200 Subject: [PATCH 05/25] fix(storage): let the S3 revalidation unique lock expire RevalidateUnusableS3StoragesJob had no uniqueFor. A lock left by a stopped worker blocked the job for up to 24 hours, so unusable S3 storages were not checked again and their backups were skipped. - Set uniqueFor to 1800 seconds --- app/Jobs/RevalidateUnusableS3StoragesJob.php | 5 ++++ ...ateUnusableS3StoragesJobUniqueLockTest.php | 25 +++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 tests/Feature/RevalidateUnusableS3StoragesJobUniqueLockTest.php diff --git a/app/Jobs/RevalidateUnusableS3StoragesJob.php b/app/Jobs/RevalidateUnusableS3StoragesJob.php index 2240b457fb..b02d440dea 100644 --- a/app/Jobs/RevalidateUnusableS3StoragesJob.php +++ b/app/Jobs/RevalidateUnusableS3StoragesJob.php @@ -26,6 +26,11 @@ class RevalidateUnusableS3StoragesJob implements ShouldBeEncrypted, ShouldBeUniq public $timeout = 1800; + /** + * Releases the unique lock of a killed worker after one run, instead of after the queue retry_after. + */ + public int $uniqueFor = 1800; + /** * Stay well below $timeout, because a single unreachable endpoint can take a while to fail. */ diff --git a/tests/Feature/RevalidateUnusableS3StoragesJobUniqueLockTest.php b/tests/Feature/RevalidateUnusableS3StoragesJobUniqueLockTest.php new file mode 100644 index 0000000000..da93f0c0b6 --- /dev/null +++ b/tests/Feature/RevalidateUnusableS3StoragesJobUniqueLockTest.php @@ -0,0 +1,25 @@ +set('cache.default', 'array'); + Queue::fake(); +}); + +it('does not queue a second revalidation while the unique lock is held', function () { + RevalidateUnusableS3StoragesJob::dispatch(); + $this->travel(29)->minutes(); + RevalidateUnusableS3StoragesJob::dispatch(); + + Queue::assertPushed(RevalidateUnusableS3StoragesJob::class, 1); +}); + +it('queues the revalidation again after a lock left by a killed worker expires', function () { + RevalidateUnusableS3StoragesJob::dispatch(); + $this->travel(31)->minutes(); + RevalidateUnusableS3StoragesJob::dispatch(); + + Queue::assertPushed(RevalidateUnusableS3StoragesJob::class, 2); +}); From 6a527051ceec063e50eddf1e7cee1ff8b9b18dc9 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:19:23 +0200 Subject: [PATCH 06/25] fix(jobs): let the unique locks of scheduled instance jobs expire CleanupInstanceStuffsJob, CleanupOrphanedPreviewContainersJob, and CheckMissingDatabaseBackupsJob had no uniqueFor. A lock left by a stopped worker blocked them for up to 24 hours. - Set uniqueFor to 60, 600, and 1800 seconds - Give CheckMissingDatabaseBackupsJob a 1800 second timeout, so a run cannot outlive its lock --- app/Jobs/CheckMissingDatabaseBackupsJob.php | 8 +++++ app/Jobs/CleanupInstanceStuffsJob.php | 6 ++++ .../CleanupOrphanedPreviewContainersJob.php | 6 ++++ .../ScheduledInstanceJobsUniqueLockTest.php | 33 +++++++++++++++++++ 4 files changed, 53 insertions(+) create mode 100644 tests/Feature/ScheduledInstanceJobsUniqueLockTest.php diff --git a/app/Jobs/CheckMissingDatabaseBackupsJob.php b/app/Jobs/CheckMissingDatabaseBackupsJob.php index 61e7ced397..ca91156574 100644 --- a/app/Jobs/CheckMissingDatabaseBackupsJob.php +++ b/app/Jobs/CheckMissingDatabaseBackupsJob.php @@ -18,6 +18,14 @@ class CheckMissingDatabaseBackupsJob implements ShouldBeEncrypted, ShouldBeUniqu { use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + public $timeout = 1800; + + /** + * Releases the unique lock of a killed worker after one run (the timeout), so the next hourly run is not + * blocked until the queue retry_after. + */ + public int $uniqueFor = 1800; + public function handle(): void { ScheduledDatabaseBackup::query() diff --git a/app/Jobs/CleanupInstanceStuffsJob.php b/app/Jobs/CleanupInstanceStuffsJob.php index 85c39a8e08..87a95d547f 100644 --- a/app/Jobs/CleanupInstanceStuffsJob.php +++ b/app/Jobs/CleanupInstanceStuffsJob.php @@ -23,6 +23,12 @@ class CleanupInstanceStuffsJob implements ShouldBeEncrypted, ShouldBeUnique, Sho public $timeout = 60; + /** + * Releases the unique lock of a killed worker after one run (the timeout), so the next run every one or + * two minutes is not blocked until the queue retry_after. + */ + public int $uniqueFor = 60; + public function __construct() {} public function middleware(): array diff --git a/app/Jobs/CleanupOrphanedPreviewContainersJob.php b/app/Jobs/CleanupOrphanedPreviewContainersJob.php index d3286699fa..46396c7f0f 100644 --- a/app/Jobs/CleanupOrphanedPreviewContainersJob.php +++ b/app/Jobs/CleanupOrphanedPreviewContainersJob.php @@ -30,6 +30,12 @@ class CleanupOrphanedPreviewContainersJob implements ShouldBeEncrypted, ShouldBe public $timeout = 600; // 10 minutes max + /** + * Releases the unique lock of a killed worker after one run (the timeout), instead of after the queue + * retry_after, so the next daily run is not blocked. + */ + public int $uniqueFor = 600; + public function __construct() {} public function middleware(): array diff --git a/tests/Feature/ScheduledInstanceJobsUniqueLockTest.php b/tests/Feature/ScheduledInstanceJobsUniqueLockTest.php new file mode 100644 index 0000000000..4ee8d66780 --- /dev/null +++ b/tests/Feature/ScheduledInstanceJobsUniqueLockTest.php @@ -0,0 +1,33 @@ +set('cache.default', 'array'); + Queue::fake(); +}); + +dataset('scheduled instance jobs', [ + 'orphaned preview containers cleanup' => [CleanupOrphanedPreviewContainersJob::class, 600], + 'instance cleanup' => [CleanupInstanceStuffsJob::class, 60], + 'missing database backups check' => [CheckMissingDatabaseBackupsJob::class, 1800], +]); + +it('does not queue the job again while its unique lock is held', function (string $jobClass, int $lockSeconds) { + $jobClass::dispatch(); + $this->travel($lockSeconds - 1)->seconds(); + $jobClass::dispatch(); + + Queue::assertPushed($jobClass, 1); +})->with('scheduled instance jobs'); + +it('queues the job again after a unique lock left by a killed worker expires', function (string $jobClass, int $lockSeconds) { + $jobClass::dispatch(); + $this->travel($lockSeconds + 1)->seconds(); + $jobClass::dispatch(); + + Queue::assertPushed($jobClass, 2); +})->with('scheduled instance jobs'); From 798e3aafc9f8ab9d69fd28d04e74c26a1f146d9f Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:19:44 +0200 Subject: [PATCH 07/25] fix(server): retry a manual Docker cleanup while another cleanup runs A manual cleanup that started while a stop-triggered cleanup held the server lock was dropped. The UI said it was queued, but it never ran. - Release a manual run and try again after 60 seconds - Stop the retries 780 seconds after dispatch (the lock expiry plus a margin) - Scheduled and stop-triggered runs are still dropped --- app/Jobs/DockerCleanupJob.php | 38 +++++++++-- tests/Feature/DockerCleanupJobTest.php | 87 ++++++++++++++++++++++++-- 2 files changed, 114 insertions(+), 11 deletions(-) diff --git a/app/Jobs/DockerCleanupJob.php b/app/Jobs/DockerCleanupJob.php index 03a9207827..6730e7a976 100644 --- a/app/Jobs/DockerCleanupJob.php +++ b/app/Jobs/DockerCleanupJob.php @@ -27,24 +27,52 @@ class DockerCleanupJob implements ShouldBeEncrypted, ShouldQueue public ?string $usageBefore = null; + /** + * A manual run waits for a running cleanup (released and tried again), so its exceptions must not + * count as reasons to try again. + */ + public $maxExceptions = 1; + + /** + * Seconds a manual run waits before it tries again while another cleanup holds the server lock. + */ + public const MANUAL_RELEASE_DELAY = 60; + public ?DockerCleanupExecution $execution_log = null; /** * Shares the per-server lock with queued CleanupDocker runs, so two cleanups never run on * one server at once. A scheduled run that finds the lock held marks its occurrence - * skipped (not missed); a manual run is dropped as before. The lock outlives the job + * skipped (not missed); a stop-triggered run is dropped. A manual run is released and tried + * again until the running cleanup finishes (see retryUntil()). The lock outlives the job * timeout, so a new run cannot start while a timed-out run is still finishing. */ public function middleware(): array { + $withoutOverlapping = ScheduledJobDeliveryService::withoutOverlapping(CleanupDocker::overlapLockKey($this->server), $this->occurrenceUuid) + ->shared() + ->expireAfter(CleanupDocker::overlapLockExpiresAfter()); + return [ - ScheduledJobDeliveryService::withoutOverlapping(CleanupDocker::overlapLockKey($this->server), $this->occurrenceUuid) - ->shared() - ->expireAfter(CleanupDocker::overlapLockExpiresAfter()) - ->dontRelease(), + $this->manualCleanup + ? $withoutOverlapping->releaseAfter(self::MANUAL_RELEASE_DELAY) + : $withoutOverlapping->dontRelease(), ]; } + /** + * A manual run may wait for one full cleanup that holds the lock. Fixed at dispatch, so a manual run of a + * killed worker, which comes back after retry_after, fails instead of running late. Other runs try once. + */ + public function retryUntil(): ?\DateTimeInterface + { + if (! $this->manualCleanup) { + return null; + } + + return now()->addSeconds(CleanupDocker::overlapLockExpiresAfter() + self::MANUAL_RELEASE_DELAY * 2); + } + public function __construct( public Server $server, public bool $manualCleanup = false, diff --git a/tests/Feature/DockerCleanupJobTest.php b/tests/Feature/DockerCleanupJobTest.php index 85607cabf4..37cafbd54d 100644 --- a/tests/Feature/DockerCleanupJobTest.php +++ b/tests/Feature/DockerCleanupJobTest.php @@ -1,6 +1,7 @@ team, DockerCleanupFailed::class); }); -it('drops a manual cleanup while another cleanup holds the server lock without touching occurrences', function () { +function dockerCleanupDatabaseQueue(): void +{ + Schema::create('jobs', function (Blueprint $table) { + $table->id(); + $table->string('queue')->index(); + $table->longText('payload'); + $table->unsignedTinyInteger('attempts'); + $table->unsignedInteger('reserved_at')->nullable(); + $table->unsignedInteger('available_at'); + $table->unsignedInteger('created_at'); + }); + config(['queue.default' => 'database']); +} + +function dockerCleanupWorkOnce(): void +{ + Artisan::call('queue:work', ['connection' => 'database', '--once' => true, '--queue' => maintenance_queue(), '--sleep' => 0]); +} + +it('runs a manual cleanup after another cleanup releases the server lock instead of dropping it', function () { + Carbon::setTestNow(Carbon::create(2026, 9, 17, 0, 0, 0, 'UTC')); + Notification::fake(); + Event::fake([DockerCleanupDone::class]); + dockerCleanupDatabaseQueue(); $server = dockerCleanupReachableServer(); - $job = new DockerCleanupJob($server, true); - $runningCleanupLock = Cache::lock($job->middleware()[0]->getLockKey($job), 3600); + $runningCleanupLock = Cache::lock('laravel-queue-overlap:docker-cleanup-'.$server->uuid, 3600); $runningCleanupLock->get(); $commands = dockerCleanupFakeProcesses(); - dispatch_sync($job); + DockerCleanupJob::dispatch($server, true); + dockerCleanupWorkOnce(); + + expect(DB::table('jobs')->count())->toBe(1) + ->and(DB::table('jobs')->value('available_at'))->toBe(now()->addSeconds(DockerCleanupJob::MANUAL_RELEASE_DELAY)->getTimestamp()) + ->and(DockerCleanupExecution::query()->exists())->toBeFalse() + ->and($commands)->toBeEmpty(); + + $runningCleanupLock->release(); + Carbon::setTestNow(now()->addSeconds(DockerCleanupJob::MANUAL_RELEASE_DELAY + 1)); + dockerCleanupWorkOnce(); + + expect(DB::table('jobs')->count())->toBe(0) + ->and(DB::table('failed_jobs')->count())->toBe(0) + ->and(DockerCleanupExecution::query()->sole()->status)->toBe('success') + ->and(ScheduledJobDelivery::query()->exists())->toBeFalse(); +}); + +it('fails a waiting manual cleanup instead of running it after its wait window', function () { + Carbon::setTestNow(Carbon::create(2026, 9, 17, 0, 0, 0, 'UTC')); + dockerCleanupDatabaseQueue(); + $server = dockerCleanupReachableServer(); + $runningCleanupLock = Cache::lock('laravel-queue-overlap:docker-cleanup-'.$server->uuid, 3600); + $runningCleanupLock->get(); + $commands = dockerCleanupFakeProcesses(); + + DockerCleanupJob::dispatch($server, true); + dockerCleanupWorkOnce(); + $runningCleanupLock->release(); + Carbon::setTestNow(now()->addDay()); + dockerCleanupWorkOnce(); + + expect(DB::table('jobs')->count())->toBe(0) + ->and(DB::table('failed_jobs')->count())->toBe(1) + ->and(DockerCleanupExecution::query()->exists())->toBeFalse() + ->and($commands)->toBeEmpty(); +}); + +it('drops a stop-triggered cleanup and a scheduled run while another cleanup holds the server lock', function () { + dockerCleanupDatabaseQueue(); + $server = dockerCleanupReachableServer(); + $runningCleanupLock = Cache::lock('laravel-queue-overlap:docker-cleanup-'.$server->uuid, 3600); + $runningCleanupLock->get(); + $commands = dockerCleanupFakeProcesses(); + + DockerCleanupJob::dispatch($server); + dockerCleanupWorkOnce(); $runningCleanupLock->release(); - expect(DockerCleanupExecution::query()->exists())->toBeFalse() - ->and(ScheduledJobDelivery::query()->exists())->toBeFalse() + expect(DB::table('jobs')->count())->toBe(0) + ->and(DB::table('failed_jobs')->count())->toBe(0) + ->and(DockerCleanupExecution::query()->exists())->toBeFalse() ->and($commands)->toBeEmpty(); }); From f9e513b7a304707e45c614fab51c6088b27bd4e4 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:19:52 +0200 Subject: [PATCH 08/25] fix(server): fail only the execution of the failed Docker cleanup job failed() marked the latest running execution of the server as failed. A manual run that never started, or a run that timed out, could mark a different running cleanup as failed. - Keep the execution id in the cache under the queue job uuid, because failed() runs on a fresh copy of the job - Change nothing when the job created no execution --- app/Jobs/DockerCleanupJob.php | 34 +++++++++--- tests/Feature/DockerCleanupJobTest.php | 73 ++++++++++++++++++++------ 2 files changed, 84 insertions(+), 23 deletions(-) diff --git a/app/Jobs/DockerCleanupJob.php b/app/Jobs/DockerCleanupJob.php index 6730e7a976..1a338f093f 100644 --- a/app/Jobs/DockerCleanupJob.php +++ b/app/Jobs/DockerCleanupJob.php @@ -16,6 +16,7 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Facades\Cache; class DockerCleanupJob implements ShouldBeEncrypted, ShouldQueue { @@ -95,6 +96,7 @@ class DockerCleanupJob implements ShouldBeEncrypted, ShouldQueue $this->execution_log = DockerCleanupExecution::create([ 'server_id' => $this->server->id, ]); + $this->rememberExecution(); if (! $this->server->isFunctional()) { $this->execution_log->update([ @@ -204,21 +206,41 @@ class DockerCleanupJob implements ShouldBeEncrypted, ShouldQueue 'finished_at' => Carbon::now()->toImmutable(), ]); } + + if (! $failed && $this->executionCacheKey()) { + Cache::forget($this->executionCacheKey()); + } } } + /** + * failed() runs on a fresh job copy built from the queue payload (for example after a worker timeout), + * so the execution this run created is kept under the queue job uuid instead of on the job. + */ + private function rememberExecution(): void + { + if ($this->executionCacheKey()) { + Cache::put($this->executionCacheKey(), $this->execution_log->id, now()->addSeconds((int) config('queue.connections.redis.retry_after', 86400) + 3600)); + } + } + + private function executionCacheKey(): ?string + { + $jobUuid = $this->job?->uuid(); + + return $jobUuid ? 'docker-cleanup-execution:'.$jobUuid : null; + } + public function failed(?\Throwable $exception): void { if ($this->occurrenceUuid) { app(ScheduledJobDeliveryService::class)->fail($this->occurrenceUuid, $this->job?->uuid() ?? $this->occurrenceUuid); } - $execution = DockerCleanupExecution::query() - ->where('server_id', $this->server->id) - ->where('status', 'running') - ->whereNull('finished_at') - ->latest('id') - ->first(); + // Only this run's own execution. A run that never started (e.g. a manual run whose wait for the + // server lock ran out) created none and must not fail another running cleanup. + $executionId = $this->executionCacheKey() ? Cache::pull($this->executionCacheKey()) : null; + $execution = $executionId ? DockerCleanupExecution::query()->find($executionId) : null; if (! $execution) { return; diff --git a/tests/Feature/DockerCleanupJobTest.php b/tests/Feature/DockerCleanupJobTest.php index 37cafbd54d..31b12bb7ac 100644 --- a/tests/Feature/DockerCleanupJobTest.php +++ b/tests/Feature/DockerCleanupJobTest.php @@ -19,6 +19,8 @@ use App\Services\ScheduledJobDeliveryService; use App\Support\Actions\UniqueUntilProcessingJobDecorator; use Illuminate\Database\Schema\Blueprint; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Queue\Jobs\FakeJob; +use Illuminate\Queue\MaxAttemptsExceededException; use Illuminate\Support\Carbon; use Illuminate\Support\Collection; use Illuminate\Support\Facades\Artisan; @@ -134,26 +136,63 @@ it('creates a failed execution record when server is force disabled', function ( ->and($execution->message)->toContain('not functional'); }); -it('finishes the latest running execution when the job fails after a timeout', function () { - $user = User::factory()->create(); - $team = $user->teams()->first(); - $server = Server::factory()->create(['team_id' => $team->id]); +function dockerCleanupQueueJob(string $uuid): FakeJob +{ + return new class($uuid) extends FakeJob + { + public function __construct(private string $jobUuid) {} - $olderExecution = DockerCleanupExecution::create([ - 'server_id' => $server->id, - ]); - $timedOutExecution = DockerCleanupExecution::create([ - 'server_id' => $server->id, - ]); + public function getRawBody() + { + return json_encode(['uuid' => $this->jobUuid]); + } + }; +} - $job = new DockerCleanupJob($server); - $job->failed(new RuntimeException('Docker cleanup job has timed out.')); +it('finishes its own execution when the job fails after a timeout, not a newer running cleanup', function () { + Notification::fake(); + Event::fake([DockerCleanupDone::class]); + $server = dockerCleanupReachableServer(); + $queueJob = dockerCleanupQueueJob('prr-fix-timed-out-cleanup'); + $statesWhenTimedOut = null; + dockerCleanupFakeProcesses(function () use ($server, $queueJob, &$statesWhenTimedOut) { + if ($statesWhenTimedOut !== null) { + return; + } + $ownExecution = DockerCleanupExecution::query()->sole(); + $scheduledExecution = DockerCleanupExecution::create(['server_id' => $server->id]); - expect($timedOutExecution->refresh()->status)->toBe('failed') - ->and($timedOutExecution->message)->toBe('Docker cleanup job has timed out.') - ->and($timedOutExecution->finished_at)->not->toBeNull() - ->and($olderExecution->refresh()->status)->toBe('running') - ->and($olderExecution->finished_at)->toBeNull(); + // The worker timeout calls failed() on a fresh job copy built from the same queue payload. + (new DockerCleanupJob($server, true))->setJob($queueJob)->failed(new RuntimeException('Docker cleanup job has timed out.')); + + $statesWhenTimedOut = [ + 'own' => $ownExecution->fresh()->only(['status', 'message']), + 'ownFinished' => $ownExecution->fresh()->finished_at !== null, + 'scheduled' => $scheduledExecution->fresh()->only(['status', 'finished_at']), + ]; + }); + + (new DockerCleanupJob($server, true))->setJob($queueJob)->handle(); + + expect($statesWhenTimedOut['own'])->toBe(['status' => 'failed', 'message' => 'Docker cleanup job has timed out.']) + ->and($statesWhenTimedOut['ownFinished'])->toBeTrue() + ->and($statesWhenTimedOut['scheduled'])->toBe(['status' => 'running', 'finished_at' => null]); +}); + +it('leaves a running scheduled cleanup untouched when a manual cleanup fails before it started', function () { + Notification::fake(); + Event::fake([DockerCleanupDone::class]); + $server = dockerCleanupReachableServer(); + $scheduledExecution = DockerCleanupExecution::create(['server_id' => $server->id]); + + (new DockerCleanupJob($server, true)) + ->setJob(dockerCleanupQueueJob('prr-fix-waiting-manual-cleanup')) + ->failed(new MaxAttemptsExceededException('App\Jobs\DockerCleanupJob has been attempted too many times.')); + + expect($scheduledExecution->fresh()->status)->toBe('running') + ->and($scheduledExecution->fresh()->finished_at)->toBeNull(); + Notification::assertNothingSent(); + Event::assertNotDispatched(DockerCleanupDone::class); }); it('skips a scheduled cleanup occurrence while another cleanup holds the server lock, so it is not reported missed later', function () { From c2d82b542d85ec94704ee3ce58a2b43524782700 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:20:10 +0200 Subject: [PATCH 09/25] fix(github-runners): do not reconcile servers with disabled runners ReconcileGithubRunnersJob connected over SSH every minute to every server with a runner configuration, also when the runners were disabled. - Reconcile a server only when its runners are enabled or it still has active runner executions, so jobs that run after disabling still end --- app/Jobs/ReconcileGithubRunnersJob.php | 6 +- ...concileGithubRunnersDisabledConfigTest.php | 116 ++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 tests/Feature/ReconcileGithubRunnersDisabledConfigTest.php diff --git a/app/Jobs/ReconcileGithubRunnersJob.php b/app/Jobs/ReconcileGithubRunnersJob.php index 61566e9e6e..073b25e57e 100644 --- a/app/Jobs/ReconcileGithubRunnersJob.php +++ b/app/Jobs/ReconcileGithubRunnersJob.php @@ -10,6 +10,7 @@ use App\Services\GithubRunner\GithubRunnerContainer; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; @@ -42,8 +43,11 @@ class ReconcileGithubRunnersJob implements ShouldBeUnique, ShouldQueue public function handle(): void { + // Disabling keeps running jobs, so a disabled server is reconciled until its last runner is done. Server::query() - ->whereHas('githubRunnerConfig') + ->where(fn (Builder $query) => $query + ->whereHas('githubRunnerConfig', fn (Builder $config) => $config->where('is_enabled', true)) + ->orWhereHas('githubRunnerExecutions', fn (Builder $execution) => $execution->whereIn('status', GithubRunnerStatus::occupying()))) ->with('settings') ->get() ->filter(fn (Server $server) => $server->isFunctional()) diff --git a/tests/Feature/ReconcileGithubRunnersDisabledConfigTest.php b/tests/Feature/ReconcileGithubRunnersDisabledConfigTest.php new file mode 100644 index 0000000000..48cc9a95ce --- /dev/null +++ b/tests/Feature/ReconcileGithubRunnersDisabledConfigTest.php @@ -0,0 +1,116 @@ + 0]); + Server::flushIdentityMap(); + Queue::fake(); + Process::fake(['*' => Process::result(output: '')]); + + $this->team = Team::factory()->create(); + $rsaKey = openssl_pkey_new(['private_key_bits' => 2048, 'private_key_type' => OPENSSL_KEYTYPE_RSA]); + openssl_pkey_export($rsaKey, $pemKey); + $privateKey = PrivateKey::create(['name' => 'Runner key', 'private_key' => $pemKey, 'team_id' => $this->team->id]); + $this->githubApp = GithubApp::create([ + 'name' => 'reconcile-disabled-app', + 'organization' => 'acme', + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'custom_user' => 'git', + 'custom_port' => 22, + 'app_id' => 2222, + 'installation_id' => 2223, + 'webhook_secret' => 'secret', + 'private_key_id' => $privateKey->id, + 'team_id' => $this->team->id, + 'is_system_wide' => false, + 'runner_group_id' => 7, + ]); +}); + +afterEach(function () { + Server::flushIdentityMap(); +}); + +function reconcileDisabledTestServer(Team $team, GithubApp $githubApp, bool $enabled): Server +{ + $server = Server::factory()->create(['team_id' => $team->id]); + $server->settings()->update([ + 'is_reachable' => true, + 'is_usable' => true, + 'server_role' => 'build', + 'is_build_server' => true, + 'force_disabled' => false, + ]); + GithubRunnerConfig::create([ + 'server_id' => $server->id, + 'github_app_id' => $githubApp->id, + 'labels' => ['coolify'], + 'max_runners' => 2, + 'docker_mode' => 'dind', + 'is_enabled' => $enabled, + ]); + + return $server->fresh(); +} + +function reconcileDisabledTestCommandFor(Server $server): Closure +{ + return fn (PendingProcess $process) => str_contains($process->command, $server->ip); +} + +it('reconciles a server with enabled runners', function () { + $server = reconcileDisabledTestServer($this->team, $this->githubApp, enabled: true); + + (new ReconcileGithubRunnersJob)->handle(); + + Process::assertRan(reconcileDisabledTestCommandFor($server)); +}); + +it('does not connect to a server whose runners are disabled and have no active runners left', function () { + $server = reconcileDisabledTestServer($this->team, $this->githubApp, enabled: false); + GithubRunnerExecution::create([ + 'github_app_id' => $this->githubApp->id, + 'server_id' => $server->id, + 'trigger_workflow_job_id' => 3001, + 'labels' => ['self-hosted', 'coolify'], + 'status' => GithubRunnerStatus::Completed, + ]); + + (new ReconcileGithubRunnersJob)->handle(); + + Process::assertDidntRun(reconcileDisabledTestCommandFor($server)); +}); + +it('keeps reconciling a disabled server until its running jobs finish', function () { + $server = reconcileDisabledTestServer($this->team, $this->githubApp, enabled: false); + $running = GithubRunnerExecution::create([ + 'github_app_id' => $this->githubApp->id, + 'server_id' => $server->id, + 'github_runner_config_id' => $server->githubRunnerConfig->id, + 'trigger_workflow_job_id' => 3002, + 'labels' => ['self-hosted', 'coolify'], + 'status' => GithubRunnerStatus::Running, + 'started_at' => now(), + ]); + + (new ReconcileGithubRunnersJob)->handle(); + + Process::assertRan(reconcileDisabledTestCommandFor($server)); + expect($running->fresh()->status)->toBe(GithubRunnerStatus::Failed); +}); From 99d4071f2cb7a61a1bb605e468c96dbd0c6c338c Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:20:19 +0200 Subject: [PATCH 10/25] fix(service): read compose up and pull input from /dev/null The service start script goes to the server over stdin. When the configuration of an existing volume changed, docker compose asked to recreate the volume and read the next script line as the answer. That line, for example the proxy network connect, did not run. - Add < /dev/null to the service compose up and pull commands - Do not use --yes, because it recreates the volume and deletes data --- .../Service/DeployServiceApplication.php | 5 +- app/Actions/Service/StartService.php | 8 +- tests/Feature/ServiceComposeStdinTest.php | 140 ++++++++++++++++++ 3 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 tests/Feature/ServiceComposeStdinTest.php diff --git a/app/Actions/Service/DeployServiceApplication.php b/app/Actions/Service/DeployServiceApplication.php index b8d1d832fd..4db4834d4c 100644 --- a/app/Actions/Service/DeployServiceApplication.php +++ b/app/Actions/Service/DeployServiceApplication.php @@ -39,7 +39,7 @@ class DeployServiceApplication if ($pullLatestImages) { $commands->push('echo Pulling image for service.'); - $commands->push("docker compose --project-directory {$safeWorkdir} -f {$safeComposeFile} --project-name {$safeProjectName} pull {$safeComposeServiceName}"); + $commands->push("docker compose --project-directory {$safeWorkdir} -f {$safeComposeFile} --project-name {$safeProjectName} pull {$safeComposeServiceName} < /dev/null"); } if ($service->networks()->count() > 0) { @@ -51,7 +51,8 @@ class DeployServiceApplication if ($forceRebuild) { $upCommand .= ' --build'; } - $upCommand .= " {$safeComposeServiceName}"; + // No stdin for Compose prompts: they would read the next script lines (see StartService). + $upCommand .= " {$safeComposeServiceName} < /dev/null"; $commands->push('echo Starting service container.'); $commands->push($upCommand); diff --git a/app/Actions/Service/StartService.php b/app/Actions/Service/StartService.php index ca26ccbd23..aacf2bc0cc 100644 --- a/app/Actions/Service/StartService.php +++ b/app/Actions/Service/StartService.php @@ -42,16 +42,20 @@ class StartService $commands[] = "touch {$workdir}/.env"; $commands = array_merge($commands, EnsureContentFilesOnServer::echoCommands($this->contentFileStorages($service), $service->server)); $commands = array_merge($commands, self::composeVolumeWarningCommands($service)); + // The script reaches the server on stdin. Compose prompts (for example "Volume ... exists but + // doesn't match configuration. Recreate?") would read the next script lines as the answer. + // Give Compose no stdin so it uses the default answer (keep the volume). Never pass --yes: + // it recreates the volume and deletes its data. if ($pullLatestImages) { $commands[] = "echo 'Pulling images.'"; - $commands[] = "docker compose --project-directory {$workdir} pull"; + $commands[] = "docker compose --project-directory {$workdir} pull < /dev/null"; } if ($service->networks()->count() > 0) { $commands[] = "echo 'Creating Docker network.'"; $commands[] = "docker network inspect $service->uuid >/dev/null 2>&1 || docker network create --attachable $service->uuid"; } $commands[] = 'echo Starting service.'; - $commands[] = "docker compose --project-directory {$workdir} -f {$workdir}/docker-compose.yml --project-name {$service->uuid} up -d --remove-orphans --force-recreate --build"; + $commands[] = "docker compose --project-directory {$workdir} -f {$workdir}/docker-compose.yml --project-name {$service->uuid} up -d --remove-orphans --force-recreate --build < /dev/null"; $commands[] = "docker network connect $service->uuid coolify-proxy >/dev/null 2>&1 || true"; if (data_get($service, 'connect_to_docker_network')) { $compose = data_get($service, 'docker_compose', []); diff --git a/tests/Feature/ServiceComposeStdinTest.php b/tests/Feature/ServiceComposeStdinTest.php new file mode 100644 index 0000000000..f5d48e9a15 --- /dev/null +++ b/tests/Feature/ServiceComposeStdinTest.php @@ -0,0 +1,140 @@ + 0]); + config([ + 'app.maintenance.store' => 'array', + 'constants.ssh.mux_enabled' => false, + ]); + $this->stubDirectory = sys_get_temp_dir().'/service-compose-stdin-'.bin2hex(random_bytes(6)); +}); + +afterEach(function () { + File::deleteDirectory($this->stubDirectory); +}); + +function serviceComposeStdinService(string $sshUser): Service +{ + $team = Team::factory()->create(); + $server = Server::factory()->create([ + 'team_id' => $team->id, + 'user' => $sshUser, + 'private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id, + ]); + $destination = StandaloneDocker::query()->where('server_id', $server->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + return Service::factory()->create([ + 'environment_id' => $environment->id, + 'server_id' => $server->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + 'connect_to_docker_network' => true, + 'docker_compose_raw' => "services:\n app:\n image: busybox:latest\n volumes:\n - data:/data\nvolumes:\n data:\n labels:\n example: changed\n", + ]); +} + +/** + * Runs the generated script the way SSH does (`bash -se` with the script on stdin). + * Process is faked for the SSH calls of the action, so this uses Symfony Process directly. + * The `docker compose up`/`pull` stub reads one line from stdin, like the Compose prompt + * "Volume ... exists but doesn't match configuration in compose file. Recreate?". + * + * @return list the commands that ran, and the lines the prompt consumed + */ +function runServiceScriptLikeSsh(string $script, string $stubDirectory): array +{ + File::ensureDirectoryExists($stubDirectory); + $log = "{$stubDirectory}/calls.log"; + File::put("{$stubDirectory}/docker", <<<'SH' +#!/bin/sh +echo "docker $*" >> "$STUB_LOG" +case " $* " in + *" compose "*" up "*|*" compose "*" pull"*) + if IFS= read -r answer; then echo "prompt read: $answer" >> "$STUB_LOG"; fi + ;; +esac +exit 0 +SH); + File::put("{$stubDirectory}/touch", "#!/bin/sh\nexit 0\n"); + File::put("{$stubDirectory}/sudo", "#!/bin/sh\nexec \"\$@\"\n"); + foreach (['docker', 'touch', 'sudo'] as $stub) { + chmod("{$stubDirectory}/{$stub}", 0755); + } + + $process = new SymfonyProcess(['bash', '-se'], env: ['PATH' => $stubDirectory.':'.getenv('PATH'), 'STUB_LOG' => $log], input: $script."\n", timeout: 30); + $process->run(); + + expect($process->isSuccessful())->toBeTrue($process->getErrorOutput()); + + return File::exists($log) ? explode("\n", trim(File::get($log))) : []; +} + +function serviceComposeCommandLines(Activity $activity): array +{ + return array_values(array_filter( + explode("\n", RemoteProcessCommand::read($activity)), + fn (string $line) => str_contains($line, 'docker compose ') && (str_contains($line, ' up -d') || str_contains($line, ' pull')), + )); +} + +it('runs the proxy network connect after a service compose up that reads stdin', function (string $sshUser) { + $service = serviceComposeStdinService($sshUser); + + $activity = StartService::run($service, pullLatestImages: true); + + $calls = runServiceScriptLikeSsh(RemoteProcessCommand::read($activity), $this->stubDirectory); + expect($calls) + ->toContain("docker network connect {$service->uuid} coolify-proxy") + ->and(collect($calls)->filter(fn (string $call) => str_starts_with($call, 'prompt read:'))->all())->toBe([]); +})->with(['root' => 'root', 'non-root' => 'deploy']); + +it('runs the proxy network connect after a single service application compose up that reads stdin', function (string $sshUser) { + $service = serviceComposeStdinService($sshUser); + $service->parse(); + + $activity = DeployServiceApplication::run($service->applications()->firstOrFail(), pullLatestImages: true); + + $calls = runServiceScriptLikeSsh(RemoteProcessCommand::read($activity), $this->stubDirectory); + expect($calls) + ->toContain("docker network connect {$service->uuid} coolify-proxy") + ->and(collect($calls)->filter(fn (string $call) => str_starts_with($call, 'prompt read:'))->all())->toBe([]); +})->with(['root' => 'root', 'non-root' => 'deploy']); + +it('gives service compose up and pull no stdin and never answers the recreate prompt', function () { + $service = serviceComposeStdinService('deploy'); + + $lines = serviceComposeCommandLines(StartService::run($service, pullLatestImages: true)); + + expect($lines)->toHaveCount(2); + foreach ($lines as $line) { + expect($line)->toStartWith('sudo docker compose ') + ->toEndWith(' < /dev/null') + ->not->toContain(' --yes') + ->not->toContain(' -y '); + } +}); From 14523ed738eceaa40f792f425169b7cd62dd2214 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:20:32 +0200 Subject: [PATCH 11/25] fix(backups): reset per-database state in a multi-database backup When the execution of the next database could not be created, the error handler used the state of the previous database. It marked the successful backup of that database as failed and could delete its file. - Reset the execution, location, and output at the start of each database --- app/Jobs/DatabaseBackupJob.php | 7 ++ .../DatabaseBackupPerDatabaseStateTest.php | 75 +++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 tests/Feature/DatabaseBackupPerDatabaseStateTest.php diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index 0cb2b4d9b0..b0117a4039 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -323,6 +323,13 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue $this->backup_dir = backup_dir().'/coolify'."/coolify-db-$ip"; } foreach ($databasesToBackup as $database) { + // Reset per-database state, so a failure before this database's execution exists + // does not mark the previous database's execution failed or delete its backup file. + $this->backup_log = null; + $this->backup_location = null; + $this->backup_output = null; + $this->error_output = null; + // Generate unique UUID for each database backup execution $attempts = 0; do { diff --git a/tests/Feature/DatabaseBackupPerDatabaseStateTest.php b/tests/Feature/DatabaseBackupPerDatabaseStateTest.php new file mode 100644 index 0000000000..24e318c712 --- /dev/null +++ b/tests/Feature/DatabaseBackupPerDatabaseStateTest.php @@ -0,0 +1,75 @@ + InstanceSettings::firstOrCreate(['id' => 0])); +}); + +afterEach(function () { + ScheduledDatabaseBackupExecution::flushEventListeners(); +}); + +it('keeps the successful execution of an earlier database when creating the next execution fails', function () { + $team = Team::factory()->create(); + $server = Server::factory()->create([ + 'team_id' => $team->id, + 'private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id, + ]); + $server->settings()->update(['is_reachable' => true, 'is_usable' => true]); + $destination = StandaloneDocker::where('server_id', $server->id)->firstOrFail(); + $environment = Environment::factory()->create(['project_id' => Project::factory()->create(['team_id' => $team->id])->id]); + $database = create_standalone_postgresql($environment->id, $destination, ['status' => 'running:healthy']); + $backup = new ScheduledDatabaseBackup; + $backup->forceFill([ + 'enabled' => true, + 'save_s3' => false, + 'frequency' => 'daily', + 'databases_to_backup' => 'first,second', + 'database_id' => $database->id, + 'database_type' => $database->getMorphClass(), + 'team_id' => $team->id, + ])->save(); + $commands = collect(); + Process::fake(function ($process) use ($commands) { + $command = is_array($process->command) ? implode(' ', $process->command) : $process->command; + $commands->push($command); + + return Process::result(output: str_contains($command, 'du -b') ? '128' : ''); + }); + ScheduledDatabaseBackupExecution::creating(function (ScheduledDatabaseBackupExecution $execution) { + if ($execution->database_name === 'second') { + throw new RuntimeException('database is unavailable'); + } + }); + + (new DatabaseBackupJob($backup->fresh()))->handle(); + + $first = ScheduledDatabaseBackupExecution::query()->sole(); + expect($first->database_name)->toBe('first') + ->and($first->status)->toBe('success') + ->and($first->filename)->not->toBeNull() + ->and($commands->contains(fn (string $command) => str_contains($command, 'rm ') && str_contains($command, $first->filename)))->toBeFalse(); +}); From b794da098358975608815b7bf7e940560e42702b Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:20:35 +0200 Subject: [PATCH 12/25] fix(backups): set finished_at for each database of a backup run In a backup of several databases, only the last execution got finished_at. - Set finished_at when each database backup succeeds or fails --- app/Jobs/DatabaseBackupJob.php | 2 + .../DatabaseBackupPerDatabaseStateTest.php | 40 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index b0117a4039..2eba0216de 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -464,6 +464,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue 'size' => $size, 'filename' => null, 's3_uploaded' => null, + 'finished_at' => Carbon::now()->toImmutable(), ]); } try { @@ -512,6 +513,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue 'size' => $size, 's3_uploaded' => $this->backup->save_s3 ? $this->s3_uploaded : null, 'local_storage_deleted' => $localStorageDeleted, + 'finished_at' => Carbon::now()->toImmutable(), ]); // Send appropriate notification (wrapped in try-catch so notification diff --git a/tests/Feature/DatabaseBackupPerDatabaseStateTest.php b/tests/Feature/DatabaseBackupPerDatabaseStateTest.php index 24e318c712..65821b03b5 100644 --- a/tests/Feature/DatabaseBackupPerDatabaseStateTest.php +++ b/tests/Feature/DatabaseBackupPerDatabaseStateTest.php @@ -73,3 +73,43 @@ it('keeps the successful execution of an earlier database when creating the next ->and($first->filename)->not->toBeNull() ->and($commands->contains(fn (string $command) => str_contains($command, 'rm ') && str_contains($command, $first->filename)))->toBeFalse(); }); + +it('finishes the execution of every database in a run with several databases', function () { + $team = Team::factory()->create(); + $server = Server::factory()->create([ + 'team_id' => $team->id, + 'private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id, + ]); + $server->settings()->update(['is_reachable' => true, 'is_usable' => true]); + $destination = StandaloneDocker::where('server_id', $server->id)->firstOrFail(); + $environment = Environment::factory()->create(['project_id' => Project::factory()->create(['team_id' => $team->id])->id]); + $database = create_standalone_postgresql($environment->id, $destination, ['status' => 'running:healthy']); + $backup = new ScheduledDatabaseBackup; + $backup->forceFill([ + 'enabled' => true, + 'save_s3' => false, + 'frequency' => 'daily', + 'databases_to_backup' => 'first,second,third', + 'database_id' => $database->id, + 'database_type' => $database->getMorphClass(), + 'team_id' => $team->id, + ])->save(); + Process::fake(function ($process) { + $command = is_array($process->command) ? implode(' ', $process->command) : $process->command; + if (str_contains($command, 'pg_dump') && str_contains($command, 'second')) { + return Process::result(errorOutput: 'pg_dump: database "second" does not exist', exitCode: 1); + } + + return Process::result(output: str_contains($command, 'du -b') ? '128' : ''); + }); + + (new DatabaseBackupJob($backup->fresh()))->handle(); + + $executions = ScheduledDatabaseBackupExecution::query()->orderBy('id')->get(); + expect($executions->map->only(['database_name', 'status'])->all())->toBe([ + ['database_name' => 'first', 'status' => 'success'], + ['database_name' => 'second', 'status' => 'failed'], + ['database_name' => 'third', 'status' => 'success'], + ]) + ->and($executions->whereNull('finished_at')->pluck('database_name')->all())->toBe([]); +}); From 4f43c96e53e33d89b8b2954e3e590fb8f8fd6d80 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:21:34 +0200 Subject: [PATCH 13/25] fix(backups): treat removed containers as recovered after a volume backup When a container that a volume backup stopped no longer existed (for example after a redeploy), recovery kept it as remaining. The execution needed attention and every later run of the schedule was skipped. - Treat "no such container" or "no such object" from docker inspect as recovered; other failures still need attention --- app/Jobs/VolumeBackupRecoveryJob.php | 7 +- ...lumeBackupRecoveryMissingContainerTest.php | 171 ++++++++++++++++++ 2 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/VolumeBackupRecoveryMissingContainerTest.php diff --git a/app/Jobs/VolumeBackupRecoveryJob.php b/app/Jobs/VolumeBackupRecoveryJob.php index bd3310712e..cda0fdd046 100644 --- a/app/Jobs/VolumeBackupRecoveryJob.php +++ b/app/Jobs/VolumeBackupRecoveryJob.php @@ -276,10 +276,13 @@ class VolumeBackupRecoveryJob implements ShouldBeEncrypted, ShouldBeUnique, Shou $execution->update(['stop_container_ids' => $containers]); $remainingFile = $stateFile.'.remaining'; + // A container that no longer exists (for example after a redeploy) has nothing to restart; only + // other inspect or start failures keep the container for another recovery attempt. $script = 'status=0; : > '.escapeshellarg($remainingFile).'; ' .'if [ -f '.escapeshellarg($stateFile).' ]; then while IFS= read -r container; do ' - .'[ -z "$container" ] && continue; running=$(docker inspect --format \'{{.State.Running}}\' "$container" 2>/dev/null) ' - .'|| { echo "$container" >> '.escapeshellarg($remainingFile).'; status=1; continue; }; ' + .'[ -z "$container" ] && continue; running=$(docker inspect --format \'{{.State.Running}}\' "$container" 2>&1) ' + .'|| { case "$running" in *"o such object"*|*"o such container"*) continue ;; esac; ' + .'echo "$container" >> '.escapeshellarg($remainingFile).'; status=1; continue; }; ' .'if [ "$running" != true ] && ! docker start "$container" >/dev/null; then echo "$container" >> ' .escapeshellarg($remainingFile).'; status=1; fi; done < '.escapeshellarg($stateFile).'; fi; ' .'if [ -s '.escapeshellarg($remainingFile).' ]; then mv '.escapeshellarg($remainingFile).' '.escapeshellarg($stateFile) diff --git a/tests/Feature/VolumeBackupRecoveryMissingContainerTest.php b/tests/Feature/VolumeBackupRecoveryMissingContainerTest.php new file mode 100644 index 0000000000..fe5190a7b1 --- /dev/null +++ b/tests/Feature/VolumeBackupRecoveryMissingContainerTest.php @@ -0,0 +1,171 @@ + 'null', 'constants.ssh.mux_enabled' => false, 'constants.ssh.max_retries' => 1]); + InstanceSettings::unguarded(fn () => InstanceSettings::firstOrCreate(['id' => 0])); + + $this->fakeDockerDir = sys_get_temp_dir().'/prr-fix-fake-docker-'.uniqid(); + File::ensureDirectoryExists($this->fakeDockerDir); + $this->dockerLog = $this->fakeDockerDir.'/calls.log'; + File::put($this->fakeDockerDir.'/docker', <<<'SH' +#!/bin/sh +echo "$*" >> "$(dirname "$0")/calls.log" +case "$1" in + inspect) + case "$4" in + aaaaaaaaaaaa) echo "Error: no such object: $4" >&2; exit 1 ;; + *) echo false ;; + esac ;; + start) + case "$2" in + bbbbbbbbbbbb) echo "Error response from daemon: cannot start" >&2; exit 1 ;; + esac ;; +esac +SH); + chmod($this->fakeDockerDir.'/docker', 0755); +}); + +afterEach(function () { + File::deleteDirectory($this->fakeDockerDir); + foreach (ScheduledVolumeBackupExecution::query()->get() as $execution) { + File::delete([VolumeBackupRecoveryJob::stateFile($execution), VolumeBackupRecoveryJob::stateFile($execution).'.remaining']); + } + Carbon::setTestNow(); +}); + +/** + * @return array{0: Server, 1: ScheduledVolumeBackup} + */ +function missingContainerRecoveryBackup(string $user): array +{ + $team = Team::factory()->create(); + $server = Server::factory()->create([ + 'team_id' => $team->id, + 'private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id, + 'user' => $user, + ]); + $server->settings()->update(['is_reachable' => true, 'is_usable' => true, 'force_disabled' => false]); + $destination = StandaloneDocker::where('server_id', $server->id)->firstOrFail(); + $environment = Environment::factory()->create(['project_id' => Project::factory()->create(['team_id' => $team->id])->id]); + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + ]); + $volume = LocalPersistentVolume::create([ + 'name' => 'app-data', + 'mount_path' => '/data', + 'resource_id' => $application->id, + 'resource_type' => $application->getMorphClass(), + ]); + $backup = $volume->scheduledBackups()->create([ + 'team_id' => $team->id, + 'frequency' => '* * * * *', + 'enabled' => true, + 'save_s3' => false, + 'stop_during_backup' => true, + ]); + + return [Server::find($server->id), $backup]; +} + +/** + * Runs the remote container recovery script with a local sh and a fake docker binary, as the server would. + */ +function fakeMissingContainerRemote(string $fakeDockerDir): void +{ + Process::fake(function ($process) use ($fakeDockerDir) { + $command = is_array($process->command) ? implode(' ', $process->command) : $process->command; + $lines = explode(PHP_EOL, $command); + $remote = implode(PHP_EOL, array_slice($lines, 1, -1)); + if (str_starts_with($remote, 'sudo ')) { + $remote = substr($remote, strlen('sudo ')); + } + if (! str_starts_with($remote, 'sh -c ') && ! str_starts_with($remote, 'cat ')) { + return Process::result(output: ''); + } + + $result = new SymfonyProcess(['sh', '-c', $remote], null, ['PATH' => $fakeDockerDir.':'.getenv('PATH')]); + $result->run(); + + return Process::result(output: $result->getOutput(), errorOutput: $result->getErrorOutput(), exitCode: $result->getExitCode()); + }); +} + +it('treats stopped containers that no longer exist as recovered and does not block the schedule', function (string $user) { + Carbon::setTestNow('2026-10-03 12:00:00'); + config(['constants.coolify.self_hosted' => true]); + [$server, $backup] = missingContainerRecoveryBackup($user); + $execution = ScheduledVolumeBackupExecution::create([ + 'scheduled_volume_backup_id' => $backup->id, + 'status' => 'failed', + 'stop_recovery_pending' => true, + ]); + File::put(VolumeBackupRecoveryJob::stateFile($execution), MISSING_CONTAINER_GONE."\n".MISSING_CONTAINER_STOPPED."\n"); + fakeMissingContainerRemote($this->fakeDockerDir); + + VolumeBackupRecoveryJob::recoverWithBounds($execution); + + $execution->refresh(); + expect($execution->stop_recovery_pending)->toBeFalse() + ->and($execution->recovery_needs_attention)->toBeFalse() + ->and(File::exists(VolumeBackupRecoveryJob::stateFile($execution)))->toBeFalse() + ->and(File::get($this->dockerLog))->toContain('start '.MISSING_CONTAINER_STOPPED); + + Queue::fake(); + Cache::forget("scheduled-volume-backup:{$backup->id}"); + (new ScheduledJobManager)->handle(); + Queue::assertPushed(VolumeBackupJob::class, fn (VolumeBackupJob $job) => $job->backup->is($backup)); +})->with(['root user' => ['root'], 'non-root user' => ['ubuntu']]); + +it('still needs attention when an existing container fails to start', function (string $user) { + [, $backup] = missingContainerRecoveryBackup($user); + $execution = ScheduledVolumeBackupExecution::create([ + 'scheduled_volume_backup_id' => $backup->id, + 'status' => 'failed', + 'stop_recovery_pending' => true, + ]); + File::put(VolumeBackupRecoveryJob::stateFile($execution), MISSING_CONTAINER_GONE."\n".MISSING_CONTAINER_BROKEN."\n"); + fakeMissingContainerRemote($this->fakeDockerDir); + + VolumeBackupRecoveryJob::recoverWithBounds($execution); + + $execution->refresh(); + expect($execution->stop_recovery_pending)->toBeTrue() + ->and($execution->recovery_needs_attention)->toBeTrue() + ->and(trim(File::get(VolumeBackupRecoveryJob::stateFile($execution))))->toBe(MISSING_CONTAINER_BROKEN); +})->with(['root user' => ['root'], 'non-root user' => ['ubuntu']]); From 379838251b662fbf9761bdffa9a60856340ffbc9 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:22:33 +0200 Subject: [PATCH 14/25] fix(auth): use the resource team in database import, SQLite, and tokens These paths used the session team after the resource was loaded. After a team switch in another tab they used the wrong team or denied access. - ImportForm: use the database team for the server, S3 storages, and the import - SQLite ConnectApplication: list applications of the database team - IntegrationTokenPolicy: check admin of the token team for view, update, and delete --- app/Livewire/Project/Database/ImportForm.php | 19 ++- .../Database/Sqlite/ConnectApplication.php | 8 +- app/Policies/IntegrationTokenPolicy.php | 12 +- .../DatabaseImportCommandInjectionTest.php | 4 +- .../DatabaseImportFormResourceTeamTest.php | 153 ++++++++++++++++++ ...IntegrationTokenPolicyResourceTeamTest.php | 36 +++++ ...liteConnectApplicationResourceTeamTest.php | 110 +++++++++++++ 7 files changed, 329 insertions(+), 13 deletions(-) create mode 100644 tests/Feature/DatabaseImportFormResourceTeamTest.php create mode 100644 tests/Feature/IntegrationTokenPolicyResourceTeamTest.php create mode 100644 tests/Feature/SqliteConnectApplicationResourceTeamTest.php diff --git a/app/Livewire/Project/Database/ImportForm.php b/app/Livewire/Project/Database/ImportForm.php index 638101b3c1..23fafd969a 100644 --- a/app/Livewire/Project/Database/ImportForm.php +++ b/app/Livewire/Project/Database/ImportForm.php @@ -216,7 +216,16 @@ class ImportForm extends Component return null; } - return Server::ownedByCurrentTeam()->find($this->serverId); + return Server::query()->where('team_id', $this->resourceTeamId())->find($this->serverId); + } + + /** + * Team of the loaded database. The session team can differ: a user can switch teams in + * another tab while this form stays open. + */ + private function resourceTeamId(): ?int + { + return $this->resource?->team()?->id; } protected $listeners = [ @@ -467,7 +476,7 @@ class ImportForm extends Component $source = Storage::exists("upload/{$this->resourceUuid}/restore") ? new DatabaseImportSource('upload', dumpAll: $this->dumpAll, replaceExisting: $this->replaceExisting, keepOwners: $this->keepOwners, sqliteDatabase: $this->sqliteDatabase, restoreMysqlUsers: $this->restoreMysqlUsers) : new DatabaseImportSource('server', path: $this->customLocation, dumpAll: $this->dumpAll, replaceExisting: $this->replaceExisting, keepOwners: $this->keepOwners, sqliteDatabase: $this->sqliteDatabase, restoreMysqlUsers: $this->restoreMysqlUsers); - $activity = StartDatabaseImport::run($this->resource, $source, (int) currentTeam()->id); + $activity = StartDatabaseImport::run($this->resource, $source, (int) $this->resourceTeamId()); $this->activityId = $activity->id; $this->dispatch('activityMonitor', $activity->id); $this->dispatch('databaserestore'); @@ -497,7 +506,7 @@ class ImportForm extends Component public function loadAvailableS3Storages() { try { - $this->availableS3Storages = S3Storage::ownedByCurrentTeam(['id', 'name', 'description']) + $this->availableS3Storages = S3Storage::ownedByCurrentTeamAPI((int) $this->resourceTeamId(), ['id', 'name', 'description']) ->where('is_usable', true) ->get() ->map(fn ($s) => ['id' => $s->id, 'name' => $s->name, 'description' => $s->description]) @@ -551,7 +560,7 @@ class ImportForm extends Component } try { - $s3Storage = S3Storage::ownedByCurrentTeam()->findOrFail($this->s3StorageId); + $s3Storage = S3Storage::ownedByCurrentTeamAPI((int) $this->resourceTeamId())->findOrFail($this->s3StorageId); // Validate bucket name early if (! $this->validateBucketName($s3Storage->bucket)) { @@ -629,7 +638,7 @@ class ImportForm extends Component try { $this->importRunning = true; $source = new DatabaseImportSource('s3', path: $this->s3Path, s3StorageUuid: (string) $this->s3StorageId, dumpAll: $this->dumpAll, replaceExisting: $this->replaceExisting, keepOwners: $this->keepOwners, sqliteDatabase: $this->sqliteDatabase, restoreMysqlUsers: $this->restoreMysqlUsers); - $activity = StartDatabaseImport::run($this->resource, $source, (int) currentTeam()->id); + $activity = StartDatabaseImport::run($this->resource, $source, (int) $this->resourceTeamId()); $this->activityId = $activity->id; $this->dispatch('activityMonitor', $activity->id); $this->dispatch('databaserestore'); diff --git a/app/Livewire/Project/Database/Sqlite/ConnectApplication.php b/app/Livewire/Project/Database/Sqlite/ConnectApplication.php index c600810c1c..b2482378a7 100644 --- a/app/Livewire/Project/Database/Sqlite/ConnectApplication.php +++ b/app/Livewire/Project/Database/Sqlite/ConnectApplication.php @@ -184,18 +184,20 @@ class ConnectApplication extends Component } /** - * Applications owned by the current team that run on the same server as the database. + * Applications of the database's team that run on the same server as the database. The session + * team can differ: a user can switch teams in another tab while this component is still open. * * @return Collection */ private function connectableApplications(): Collection { $serverId = $this->database->destination?->server_id; - if ($serverId === null) { + $teamId = $this->database->team()?->id; + if ($serverId === null || $teamId === null) { return collect(); } - return Application::ownedByCurrentTeam() + return Application::ownedByCurrentTeamAPI($teamId) ->with(['destination', 'environment.project']) ->get() ->filter(fn (Application $application) => $application->destination?->server_id === $serverId) diff --git a/app/Policies/IntegrationTokenPolicy.php b/app/Policies/IntegrationTokenPolicy.php index 309c8167f2..671a8b33c6 100644 --- a/app/Policies/IntegrationTokenPolicy.php +++ b/app/Policies/IntegrationTokenPolicy.php @@ -7,6 +7,9 @@ use App\Models\User; class IntegrationTokenPolicy { + /** + * Listing and creating tokens happen on the session team's security pages. + */ public function viewAny(User $user): bool { return $user->isAdmin(); @@ -17,18 +20,21 @@ class IntegrationTokenPolicy return $user->isAdmin(); } + /** + * A loaded token is checked against its own team: the session team can differ. + */ public function view(User $user, IntegrationToken $integrationToken): bool { - return $user->isAdmin() && $integrationToken->team_id === currentTeam()->id; + return $user->isAdminOfTeam((int) $integrationToken->team_id); } public function update(User $user, IntegrationToken $integrationToken): bool { - return $user->isAdmin() && $integrationToken->team_id === currentTeam()->id; + return $user->isAdminOfTeam((int) $integrationToken->team_id); } public function delete(User $user, IntegrationToken $integrationToken): bool { - return $user->isAdmin() && $integrationToken->team_id === currentTeam()->id; + return $user->isAdminOfTeam((int) $integrationToken->team_id); } } diff --git a/tests/Feature/DatabaseImportCommandInjectionTest.php b/tests/Feature/DatabaseImportCommandInjectionTest.php index e2985e44bf..932709d30d 100644 --- a/tests/Feature/DatabaseImportCommandInjectionTest.php +++ b/tests/Feature/DatabaseImportCommandInjectionTest.php @@ -89,7 +89,7 @@ describe('locked properties', function () { }); describe('server method uses team scoping', function () { - test('server computed property calls ownedByCurrentTeam', function () { + test('server computed property is scoped to the database team', function () { $method = new ReflectionMethod(ImportForm::class, 'server'); // Extract the server method body @@ -98,7 +98,7 @@ describe('server method uses team scoping', function () { $lines = array_slice(file($method->getFileName()), $startLine - 1, $endLine - $startLine + 1); $methodBody = implode('', $lines); - expect($methodBody)->toContain('ownedByCurrentTeam'); + expect($methodBody)->toContain('where(\'team_id\', $this->resourceTeamId())'); expect($methodBody)->not->toContain('Server::find($this->serverId)'); }); }); diff --git a/tests/Feature/DatabaseImportFormResourceTeamTest.php b/tests/Feature/DatabaseImportFormResourceTeamTest.php new file mode 100644 index 0000000000..29db848a8b --- /dev/null +++ b/tests/Feature/DatabaseImportFormResourceTeamTest.php @@ -0,0 +1,153 @@ + 0, + 'disable_two_step_confirmation' => true, + ]); + + $this->resourceTeam = Team::factory()->create(); + $this->otherTeam = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->resourceTeam->members()->attach($this->user, ['role' => 'admin']); + $this->otherTeam->members()->attach($this->user, ['role' => 'owner']); + $this->actingAs($this->user); + session(['currentTeam' => $this->resourceTeam]); + + $this->server = Server::factory()->create(['team_id' => $this->resourceTeam->id]); + $destination = StandaloneDocker::firstOrCreate( + ['server_id' => $this->server->id, 'network' => 'coolify'], + ['uuid' => (string) Str::uuid(), 'name' => 'docker'] + ); + $project = Project::factory()->create(['team_id' => $this->resourceTeam->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + $this->database = StandalonePostgresql::create([ + 'uuid' => (string) Str::uuid(), + 'name' => 'db', + 'postgres_user' => 'postgres', + 'postgres_password' => 'password', + 'postgres_db' => 'db', + 'image' => 'postgres:17', + 'status' => 'running', + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + ]); + + $s3Attributes = ['region' => 'us-east-1', 'key' => 'key', 'secret' => 'secret', 'bucket' => 'backups', 'endpoint' => 'https://s3.example.com', 'is_usable' => true]; + $this->resourceTeamStorage = S3Storage::create([...$s3Attributes, 'team_id' => $this->resourceTeam->id, 'name' => 'resource-team-s3']); + $this->otherTeamStorage = S3Storage::create([...$s3Attributes, 'team_id' => $this->otherTeam->id, 'name' => 'other-team-s3']); +}); + +class ImportFormResourceTeamTestComponent extends ImportForm +{ + public function mount($database = null, $server = null): void + { + $this->resourceId = $database->id; + $this->resourceType = $database::class; + $this->serverId = $server->id; + $this->container = $database->uuid; + $this->resourceUuid = $database->uuid; + $this->resourceStatus = $database->status ?? ''; + $this->resourceDbType = $database->type(); + $this->loadAvailableS3Storages(); + } + + public function render() + { + return view('livewire.project.database.import-form'); + } +} + +function importFormAfterTeamSwitch(): Testable +{ + $component = Livewire::test(ImportFormResourceTeamTestComponent::class, [ + 'database' => test()->database, + 'server' => test()->server, + ]); + + session(['currentTeam' => test()->otherTeam]); + + return $component; +} + +function fakeStartedDatabaseImport(): Activity +{ + return Activity::create([ + 'log_name' => 'default', + 'description' => 'queued', + 'properties' => ['status' => 'queued'], + ]); +} + +test('a file import started after a session team switch runs for the database team', function () { + StartDatabaseImport::shouldRun()->once() + ->withArgs(fn ($resource, $source, int $teamId): bool => $teamId === $this->resourceTeam->id) + ->andReturn(fakeStartedDatabaseImport()); + + importFormAfterTeamSwitch() + ->set('filename', 'backup.dump') + ->call('runImport') + ->assertNotDispatched('error') + ->assertDispatched('databaserestore'); +}); + +test('an S3 restore started after a session team switch runs for the database team', function () { + StartDatabaseImport::shouldRun()->once() + ->withArgs(fn ($resource, $source, int $teamId): bool => $teamId === $this->resourceTeam->id + && $source->s3StorageUuid === (string) $this->resourceTeamStorage->id) + ->andReturn(fakeStartedDatabaseImport()); + + importFormAfterTeamSwitch() + ->set('s3StorageId', $this->resourceTeamStorage->id) + ->set('s3Path', '/backups/backup.dump') + ->set('s3FileSize', 1024) + ->call('restoreFromS3') + ->assertNotDispatched('error') + ->assertDispatched('databaserestore'); +}); + +test('the S3 storage list contains only storages of the database team', function () { + importFormAfterTeamSwitch() + ->call('loadAvailableS3Storages') + ->assertSet('availableS3Storages', fn (array $storages): bool => collect($storages)->pluck('id')->all() === [$this->resourceTeamStorage->id]); +}); + +test('an S3 storage of another team cannot be checked for the database', function () { + importFormAfterTeamSwitch() + ->set('s3StorageId', $this->otherTeamStorage->id) + ->set('s3Path', '/backups/backup.dump') + ->call('checkS3File') + ->assertNotFound(); +}); + +test('a member of the database team cannot start an import after switching to a team they own', function () { + $this->resourceTeam->members()->updateExistingPivot($this->user->id, ['role' => 'member']); + $this->user->refresh(); + StartDatabaseImport::shouldRun()->never(); + + importFormAfterTeamSwitch() + ->set('filename', 'backup.dump') + ->call('runImport') + ->assertForbidden(); +}); diff --git a/tests/Feature/IntegrationTokenPolicyResourceTeamTest.php b/tests/Feature/IntegrationTokenPolicyResourceTeamTest.php new file mode 100644 index 0000000000..a7cb034ab8 --- /dev/null +++ b/tests/Feature/IntegrationTokenPolicyResourceTeamTest.php @@ -0,0 +1,36 @@ +tokenTeam = Team::factory()->create(); + $this->otherTeam = Team::factory()->create(); + $this->token = IntegrationToken::factory()->create(['team_id' => $this->tokenTeam->id]); + $this->user = User::factory()->create(); + $this->otherTeam->members()->attach($this->user->id, ['role' => 'owner']); + $this->actingAs($this->user); + session(['currentTeam' => $this->otherTeam]); +}); + +it('lets an admin of the token team manage the token while the session team is another team', function (string $ability) { + $this->tokenTeam->members()->attach($this->user->id, ['role' => 'admin']); + + expect($this->user->fresh()->can($ability, $this->token))->toBeTrue(); +})->with(['view', 'update', 'delete']); + +it('denies a member of the token team who owns the session team', function (string $ability) { + $this->tokenTeam->members()->attach($this->user->id, ['role' => 'member']); + + expect($this->user->fresh()->can($ability, $this->token))->toBeFalse(); +})->with(['view', 'update', 'delete']); + +it('denies a user outside the token team even when the session points to the token team', function (string $ability) { + session(['currentTeam' => $this->tokenTeam]); + + expect($this->user->fresh()->can($ability, $this->token))->toBeFalse(); +})->with(['view', 'update', 'delete']); diff --git a/tests/Feature/SqliteConnectApplicationResourceTeamTest.php b/tests/Feature/SqliteConnectApplicationResourceTeamTest.php new file mode 100644 index 0000000000..c30cdbc597 --- /dev/null +++ b/tests/Feature/SqliteConnectApplicationResourceTeamTest.php @@ -0,0 +1,110 @@ +withoutVite(); + InstanceSettings::unguarded(fn () => InstanceSettings::updateOrCreate(['id' => 0], ['id' => 0])); + + $this->resourceTeam = Team::factory()->create(); + $this->otherTeam = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->resourceTeam->members()->attach($this->user->id, ['role' => 'admin']); + $this->otherTeam->members()->attach($this->user->id, ['role' => 'owner']); + $this->actingAs($this->user); + session(['currentTeam' => $this->resourceTeam]); + + $server = Server::factory()->create(['team_id' => $this->resourceTeam->id]); + $this->destination = StandaloneDocker::withoutEvents(fn () => StandaloneDocker::firstOrCreate( + ['server_id' => $server->id, 'network' => 'coolify'], + ['uuid' => (string) Str::uuid(), 'name' => 'test-docker'] + )); + + $this->sqlite = StandaloneSqlite::create([ + 'name' => 'app-sqlite', + 'environment_id' => sqliteResourceTeamEnvironment($this->resourceTeam)->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + ]); + + $this->resourceTeamApplication = sqliteResourceTeamApplication(sqliteResourceTeamEnvironment($this->resourceTeam), $this->destination, 'Resource Team App'); + $this->otherTeamApplication = sqliteResourceTeamApplication(sqliteResourceTeamEnvironment($this->otherTeam), $this->destination, 'Other Team App'); +}); + +function sqliteResourceTeamEnvironment(Team $team): Environment +{ + return Environment::factory()->create(['project_id' => Project::factory()->create(['team_id' => $team->id])->id]); +} + +function sqliteResourceTeamApplication(Environment $environment, StandaloneDocker $destination, string $name): Application +{ + return Application::factory()->create([ + 'uuid' => (string) Str::uuid(), + 'name' => $name, + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + 'build_pack' => 'dockerimage', + 'docker_registry_image_name' => 'nginx', + ]); +} + +it('offers only applications of the database team when the session team is another team', function () { + session(['currentTeam' => $this->otherTeam]); + + Livewire::test(ConnectApplication::class, ['database' => $this->sqlite]) + ->assertSet('applicationOptions', fn (array $options): bool => collect($options)->pluck('value')->all() === [$this->resourceTeamApplication->uuid]); +}); + +it('connects an application of the database team after a session team switch', function () { + $component = Livewire::test(ConnectApplication::class, ['database' => $this->sqlite]); + session(['currentTeam' => $this->otherTeam]); + + $component->set('applicationUuid', $this->resourceTeamApplication->uuid) + ->call('connect') + ->assertNotDispatched('error') + ->assertRedirect(); + + expect($this->resourceTeamApplication->persistentStorages()->where('standalone_sqlite_id', $this->sqlite->id)->exists())->toBeTrue(); +}); + +it('does not connect an application of another team after a session team switch', function () { + $component = Livewire::test(ConnectApplication::class, ['database' => $this->sqlite]); + session(['currentTeam' => $this->otherTeam]); + + $component->set('applicationUuid', $this->otherTeamApplication->uuid) + ->call('connect') + ->assertDispatched('error') + ->assertNoRedirect(); + + expect($this->otherTeamApplication->persistentStorages()->count())->toBe(0); +}); + +it('forbids a member of the database team from connecting after switching to a team they own', function () { + $this->resourceTeam->members()->updateExistingPivot($this->user->id, ['role' => 'member']); + $this->user->refresh(); + + $component = Livewire::test(ConnectApplication::class, ['database' => $this->sqlite]); + session(['currentTeam' => $this->otherTeam]); + + $component->set('applicationUuid', $this->resourceTeamApplication->uuid) + ->call('connect') + ->assertForbidden(); + + expect($this->resourceTeamApplication->persistentStorages()->count())->toBe(0); +}); From a331f91c4915db4abae7929d75eb31af74530d8f Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:23:29 +0200 Subject: [PATCH 15/25] fix(ui): validate secret manager settings and lock server-built lists - SecretManagerLinks: validate settings when the integration token changes, and save only validated keys - Service Domains: lock domainRows - Server DockerImages: lock images --- app/Livewire/Project/Service/Domains.php | 1 + .../Project/Shared/SecretManagerLinks.php | 21 ++++- app/Livewire/Server/DockerImages.php | 5 + .../LockedDomainRowsAndDockerImagesTest.php | 78 ++++++++++++++++ ...retManagerLinksTokenChangeSettingsTest.php | 93 +++++++++++++++++++ 5 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 tests/Feature/LockedDomainRowsAndDockerImagesTest.php create mode 100644 tests/Feature/SecretManagers/SecretManagerLinksTokenChangeSettingsTest.php diff --git a/app/Livewire/Project/Service/Domains.php b/app/Livewire/Project/Service/Domains.php index f70d521150..8747b0f619 100644 --- a/app/Livewire/Project/Service/Domains.php +++ b/app/Livewire/Project/Service/Domains.php @@ -49,6 +49,7 @@ class Domains extends Component public ?int $pendingRedirectServiceApplicationId = null; /** @var array> */ + #[Locked] public array $domainRows = []; public ?int $newServiceApplicationId = null; diff --git a/app/Livewire/Project/Shared/SecretManagerLinks.php b/app/Livewire/Project/Shared/SecretManagerLinks.php index afe4c85f31..36d51252d4 100644 --- a/app/Livewire/Project/Shared/SecretManagerLinks.php +++ b/app/Livewire/Project/Shared/SecretManagerLinks.php @@ -8,6 +8,7 @@ use App\Support\ValidationPatterns; use Illuminate\Contracts\View\View; use Illuminate\Database\Eloquent\Builder; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; +use Illuminate\Validation\ValidationException; use Livewire\Component; /** @@ -124,7 +125,8 @@ class SecretManagerLinks extends Component $this->settings = []; } - $settings = array_filter($this->settings, fn ($value) => filled($value)); + $validated = $this->validate($this->tokenChangeRules()); + $settings = array_filter(data_get($validated, 'settings', []), fn ($value) => filled($value)); $this->resource->secretManagerLink()->updateOrCreate([], [ 'integration_token_id' => $token->id, @@ -138,11 +140,28 @@ class SecretManagerLinks extends Component $this->resetKeys(); $this->loadData(); $this->dispatch('success', 'Secret manager source saved. References resolve at the next deployment.'); + } catch (ValidationException $e) { + throw $e; } catch (\Throwable $e) { handleError($e, $this); } } + /** + * The settings rules of the selected provider, with the fields still optional: a token is + * saved before the user fills in its settings. + * + * @return array> + */ + private function tokenChangeRules(): array + { + return collect($this->rules()) + ->map(fn (array $rules, string $field): array => str_starts_with($field, 'settings.') + ? array_map(fn (string $rule): string => $rule === 'required' ? 'nullable' : $rule, $rules) + : $rules) + ->all(); + } + /** * Auto-save of the provider-specific settings fields (called on blur). */ diff --git a/app/Livewire/Server/DockerImages.php b/app/Livewire/Server/DockerImages.php index dab873c650..e9f6b5bef8 100644 --- a/app/Livewire/Server/DockerImages.php +++ b/app/Livewire/Server/DockerImages.php @@ -4,6 +4,7 @@ namespace App\Livewire\Server; use App\Models\Server; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; +use Livewire\Attributes\Locked; use Livewire\Component; class DockerImages extends Component @@ -14,6 +15,10 @@ class DockerImages extends Component public array $parameters = []; + /** + * Images read from the server. Deletion checks container usage against this list. + */ + #[Locked] public array $images = []; public ?array $usage = null; diff --git a/tests/Feature/LockedDomainRowsAndDockerImagesTest.php b/tests/Feature/LockedDomainRowsAndDockerImagesTest.php new file mode 100644 index 0000000000..72f31cf87e --- /dev/null +++ b/tests/Feature/LockedDomainRowsAndDockerImagesTest.php @@ -0,0 +1,78 @@ +withoutVite(); + InstanceSettings::unguarded(fn () => InstanceSettings::updateOrCreate(['id' => 0], ['id' => 0, 'is_dns_validation_enabled' => false])); + + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'owner']); + $this->actingAs($this->user); + session(['currentTeam' => $this->team]); + + $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.10']); + $this->server->settings()->update(['is_reachable' => true, 'is_usable' => true]); +}); + +it('rejects a client update of the service domain rows', function () { + $destination = StandaloneDocker::withoutEvents(fn () => StandaloneDocker::firstOrCreate( + ['server_id' => $this->server->id, 'network' => 'coolify'], + ['uuid' => (string) Str::uuid(), 'name' => 'test-docker'] + )); + $environment = Environment::factory()->create(['project_id' => Project::factory()->create(['team_id' => $this->team->id])->id]); + $service = Service::factory()->create([ + 'server_id' => $this->server->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + 'environment_id' => $environment->id, + 'docker_compose_raw' => "services:\n web:\n image: nginx:alpine\n", + ]); + ServiceApplication::create([ + 'uuid' => (string) Str::uuid(), + 'service_id' => $service->id, + 'name' => 'web', + 'image' => 'nginx:alpine', + 'fqdn' => 'https://web.example.com', + ]); + + Livewire::test(Domains::class, ['service' => $service->fresh(['applications', 'server'])]) + ->assertSet('domainRows', fn (array $rows): bool => collect($rows)->pluck('url')->all() === ['https://web.example.com']) + ->set('domainRows', [['url' => 'https://attacker.example.com']]); +})->throws(CannotUpdateLockedPropertyException::class); + +it('rejects a client update of the docker image list', function () { + $imageId = 'sha256:'.str_repeat('a', 64); + Process::fake([ + '*docker image ls*' => Process::result(output: json_encode(['Repository' => 'nginx', 'Tag' => '1.27', 'ID' => $imageId, 'Size' => '52.5MB', 'CreatedSince' => '3 weeks ago'])), + '*docker ps -a*' => Process::result(output: "{$imageId}#/web\n"), + '*docker system df*' => Process::result(output: ''), + '*docker image rm*' => Process::result(output: 'Deleted'), + ]); + + Livewire::test(DockerImages::class, ['server_uuid' => $this->server->uuid]) + ->call('load') + ->assertSet('images.0.containers', ['web']) + ->set('images.0.containers', []); +})->throws(CannotUpdateLockedPropertyException::class); diff --git a/tests/Feature/SecretManagers/SecretManagerLinksTokenChangeSettingsTest.php b/tests/Feature/SecretManagers/SecretManagerLinksTokenChangeSettingsTest.php new file mode 100644 index 0000000000..87183bac3b --- /dev/null +++ b/tests/Feature/SecretManagers/SecretManagerLinksTokenChangeSettingsTest.php @@ -0,0 +1,93 @@ +withoutDefer(); + InstanceSettings::unguarded(fn () => InstanceSettings::updateOrCreate(['id' => 0], ['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]); + $this->actingAs($this->user); + + $server = Server::factory()->create(['team_id' => $this->team->id]); + $destination = $server->standaloneDockers()->firstOrFail(); + $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(), + ]); + + $vaultToken = fn (string $name) => IntegrationToken::query()->create([ + 'team_id' => $this->team->id, + 'provider' => 'vault', + 'name' => $name, + 'token' => 'hvs.token', + 'capabilities' => ['secrets'], + 'metadata' => ['base_url' => 'https://vault.internal:8200'], + ]); + $this->currentToken = $vaultToken('Vault current'); + $this->nextToken = $vaultToken('Vault next'); + + $this->application->secretManagerLink()->create([ + 'integration_token_id' => $this->currentToken->id, + 'settings' => ['mount' => 'secret', 'path' => 'my-app/production'], + ]); +}); + +test('switching to a token of the same provider rejects invalid settings and keeps the saved source', function () { + Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]) + ->set([ + 'settings' => ['mount' => ['nested' => 'value'], 'path' => 'my-app/production'], + 'integration_token_uuid' => $this->nextToken->uuid, + ]) + ->assertHasErrors(['settings.mount']); + + $link = $this->application->secretManagerLink()->firstOrFail(); + expect($link->integration_token_id)->toBe($this->currentToken->id) + ->and($link->settings)->toBe(['mount' => 'secret', 'path' => 'my-app/production']); +}); + +test('switching to a token of the same provider saves only the provider settings fields', function () { + Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]) + ->set([ + 'settings' => ['mount' => 'kv', 'path' => 'my-app/staging', 'unexpected' => 'value'], + 'integration_token_uuid' => $this->nextToken->uuid, + ]) + ->assertHasNoErrors() + ->assertDispatched('success'); + + $link = $this->application->secretManagerLink()->firstOrFail(); + expect($link->integration_token_id)->toBe($this->nextToken->id) + ->and($link->settings)->toBe(['mount' => 'kv', 'path' => 'my-app/staging']); +}); + +test('switching to a token of the same provider keeps working while the settings are still empty', function () { + $this->application->secretManagerLink()->update(['settings' => null]); + + Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]) + ->set('integration_token_uuid', $this->nextToken->uuid) + ->assertHasNoErrors() + ->assertDispatched('success'); + + $link = $this->application->secretManagerLink()->firstOrFail(); + expect($link->integration_token_id)->toBe($this->nextToken->id) + ->and($link->settings)->toBeNull(); +}); From babe43da3212f154d94310fc9fd2abfecef8dfdc Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:23:36 +0200 Subject: [PATCH 16/25] fix(audit): return 403 on the audit log without a session team When the user had no session team (for example after removal from the team), the audit log page threw an error and returned 500. - Return 403 when there is no session team or the user is not an admin --- app/Livewire/Team/AuditLog.php | 4 +- .../TeamAuditLogWithoutSessionTeamTest.php | 48 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 tests/Feature/TeamAuditLogWithoutSessionTeamTest.php diff --git a/app/Livewire/Team/AuditLog.php b/app/Livewire/Team/AuditLog.php index 53cb203eff..9cf3ada8c8 100644 --- a/app/Livewire/Team/AuditLog.php +++ b/app/Livewire/Team/AuditLog.php @@ -22,7 +22,9 @@ class AuditLog extends Component public function boot(): void { - abort_unless(auth()->user()->isAdminOfTeam(currentTeam()->id), 403); + $team = currentTeam(); + + abort_unless($team && auth()->user()->isAdminOfTeam($team->id), 403); } public function updatedSearch(): void diff --git a/tests/Feature/TeamAuditLogWithoutSessionTeamTest.php b/tests/Feature/TeamAuditLogWithoutSessionTeamTest.php new file mode 100644 index 0000000000..2d861d0fb6 --- /dev/null +++ b/tests/Feature/TeamAuditLogWithoutSessionTeamTest.php @@ -0,0 +1,48 @@ + InstanceSettings::updateOrCreate(['id' => 0], ['id' => 0])); + + $this->team = Team::factory()->create(); + $this->secondTeam = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'admin']); + $this->secondTeam->members()->attach($this->user->id, ['role' => 'owner']); + $this->actingAs($this->user); + session(['currentTeam' => $this->team]); +}); + +test('the audit log is forbidden when the session has no team', function () { + session()->forget('currentTeam'); + + Livewire::test(AuditLog::class)->assertForbidden(); +}); + +test('an open audit log is forbidden after the session team is removed from the user', function () { + $component = Livewire::test(AuditLog::class)->assertOk(); + + $this->team->members()->detach($this->user->id); + auth()->setUser($this->user->fresh()); + + $component->set('search', 'deployment')->assertForbidden(); +}); + +test('team admins can still open the audit log', function () { + Livewire::test(AuditLog::class)->assertOk(); +}); + +test('team members cannot open the audit log', function () { + $this->team->members()->updateExistingPivot($this->user->id, ['role' => 'member']); + auth()->setUser($this->user->fresh()); + + Livewire::test(AuditLog::class)->assertForbidden(); +}); From ac5a0c289f0bf256a2e7e24dec6b7823ed2dfd50 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:23:43 +0200 Subject: [PATCH 17/25] fix(api): require the deploy ability for database imports from a server path An import with source=server reads a file at an absolute path on the server, but it needed only the write ability. - Return 403 without the deploy or root ability, the same as other host paths outside the resource directory --- .../Concerns/HandlesDatabaseImportsApi.php | 6 ++ tests/Feature/Api/DatabaseImportApiTest.php | 2 +- ...ImportServerSourceDeployAbilityApiTest.php | 89 +++++++++++++++++++ 3 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 tests/Feature/Api/DatabaseImportServerSourceDeployAbilityApiTest.php diff --git a/app/Http/Controllers/Api/Concerns/HandlesDatabaseImportsApi.php b/app/Http/Controllers/Api/Concerns/HandlesDatabaseImportsApi.php index 07156a9d4e..cd86224ef7 100644 --- a/app/Http/Controllers/Api/Concerns/HandlesDatabaseImportsApi.php +++ b/app/Http/Controllers/Api/Concerns/HandlesDatabaseImportsApi.php @@ -82,6 +82,12 @@ trait HandlesDatabaseImportsApi return response()->json(['message' => 'Validation failed.', 'errors' => $errors], 422); } + // A server import reads any absolute path on the host, so it needs the same permission as host mounts outside the resource directory. + if ($payload['source'] === 'server' && ! $request->user()->tokenCan('deploy') && ! $request->user()->tokenCan('root')) { + return response()->json([ + 'message' => 'Missing required permissions: deploy. An import from a server path needs a token with the deploy permission.', + ], 403); + } try { $source = new DatabaseImportSource((string) $payload['source'], $payload['upload_id'] ?? null, $payload['path'] ?? null, $payload['s3_storage_uuid'] ?? null, (bool) ($payload['dump_all'] ?? false), (bool) ($payload['replace_existing'] ?? false), (bool) ($payload['keep_owners'] ?? false), isset($payload['sqlite_database']) ? (string) $payload['sqlite_database'] : null, (bool) ($payload['restore_mysql_users'] ?? false)); diff --git a/tests/Feature/Api/DatabaseImportApiTest.php b/tests/Feature/Api/DatabaseImportApiTest.php index 3effb8fabb..eaaf2d55a4 100644 --- a/tests/Feature/Api/DatabaseImportApiTest.php +++ b/tests/Feature/Api/DatabaseImportApiTest.php @@ -26,7 +26,7 @@ beforeEach(function () { $this->user = User::factory()->create(); $this->team->members()->attach($this->user, ['role' => 'owner']); session(['currentTeam' => $this->team]); - $this->token = $this->user->tokens()->create(['name' => 'imports', 'token' => hash('sha256', 'secret'), 'abilities' => ['write', 'read'], 'team_id' => $this->team->id]); + $this->token = $this->user->tokens()->create(['name' => 'imports', 'token' => hash('sha256', 'secret'), 'abilities' => ['write', 'read', 'deploy'], 'team_id' => $this->team->id]); $this->headers = ['Authorization' => 'Bearer '.$this->token->id.'|secret']; $this->server = Server::factory()->create(['team_id' => $this->team->id]); $this->destination = StandaloneDocker::firstOrCreate(['server_id' => $this->server->id, 'network' => 'coolify'], ['uuid' => (string) Str::uuid(), 'name' => 'docker']); diff --git a/tests/Feature/Api/DatabaseImportServerSourceDeployAbilityApiTest.php b/tests/Feature/Api/DatabaseImportServerSourceDeployAbilityApiTest.php new file mode 100644 index 0000000000..45095aa56f --- /dev/null +++ b/tests/Feature/Api/DatabaseImportServerSourceDeployAbilityApiTest.php @@ -0,0 +1,89 @@ + 0, 'is_api_enabled' => true]); + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user, ['role' => 'owner']); + session(['currentTeam' => $this->team]); + + $server = Server::factory()->create(['team_id' => $this->team->id]); + $destination = StandaloneDocker::firstOrCreate(['server_id' => $server->id, 'network' => 'coolify'], ['uuid' => (string) Str::uuid(), 'name' => 'docker']); + $project = Project::factory()->create(['team_id' => $this->team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + $database = StandalonePostgresql::create(['uuid' => (string) Str::uuid(), 'name' => 'db', 'postgres_user' => 'postgres', 'postgres_password' => 'password', 'postgres_db' => 'db', 'image' => 'postgres:17', 'status' => 'running', 'environment_id' => $environment->id, 'destination_id' => $destination->id, 'destination_type' => $destination->getMorphClass()]); + $service = Service::factory()->create(['environment_id' => $environment->id, 'server_id' => $server->id, 'destination_id' => $destination->id, 'destination_type' => $destination->getMorphClass(), 'docker_compose_raw' => "services:\n postgres:\n image: postgres:17\n"]); + $serviceDatabase = ServiceDatabase::create(['uuid' => (string) Str::uuid(), 'name' => 'postgres', 'service_id' => $service->id, 'image' => 'postgres:17']); + + $this->importPaths = [ + 'standalone' => "/api/v1/databases/{$database->uuid}/imports", + 'service' => "/api/v1/services/{$service->uuid}/databases/{$serviceDatabase->uuid}/imports", + ]; +}); + +function serverSourceImportToken(User $user, Team $team, array $abilities): string +{ + $token = $user->createToken('server-source-import', $abilities); + $token->accessToken->forceFill(['team_id' => $team->id])->save(); + + return $token->plainTextToken; +} + +function expectServerSourceImportStarts(): void +{ + $activity = Activity::create(['log_name' => 'default', 'description' => 'queued', 'properties' => ['status' => 'queued']]); + $action = Mockery::mock(StartDatabaseImport::class); + $action->shouldReceive('handle')->once()->andReturn($activity); + app()->instance(StartDatabaseImport::class, $action); +} + +dataset('server source import variants', ['standalone', 'service']); + +test('a write token cannot import from a server path', function (string $variant) { + $action = Mockery::mock(StartDatabaseImport::class); + $action->shouldNotReceive('handle'); + app()->instance(StartDatabaseImport::class, $action); + + $this->withToken(serverSourceImportToken($this->user, $this->team, ['read', 'write'])) + ->postJson($this->importPaths[$variant], ['source' => 'server', 'path' => '/etc/hostname']) + ->assertForbidden() + ->assertJsonPath('message', 'Missing required permissions: deploy. An import from a server path needs a token with the deploy permission.'); +})->with('server source import variants'); + +test('a write token with deploy or a root token can import from a server path', function (string $variant, array $abilities) { + expectServerSourceImportStarts(); + + $this->withToken(serverSourceImportToken($this->user, $this->team, $abilities)) + ->postJson($this->importPaths[$variant], ['source' => 'server', 'path' => '/tmp/backup.sql']) + ->assertAccepted(); +})->with('server source import variants')->with([ + 'write and deploy' => [['read', 'write', 'deploy']], + 'root' => [['root']], +]); + +test('a write token can still import from an upload without deploy', function () { + expectServerSourceImportStarts(); + + $this->withToken(serverSourceImportToken($this->user, $this->team, ['read', 'write'])) + ->postJson($this->importPaths['standalone'], ['source' => 'upload', 'upload_id' => (string) Str::uuid()]) + ->assertAccepted(); +}); From 50268139be512ba3ae38cc4addccd692134df596 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:23:55 +0200 Subject: [PATCH 18/25] fix(api): reject custom_internal_name without consistent container naming Container names use custom_internal_name only when consistent container naming is on. The API accepted the name and then ignored it. - Return 422 with a message that says to set is_consistent_container_name_enabled to true - Accept the name when the request or the application turns the setting on - Document the requirement in the OpenAPI spec --- .../Api/ApplicationsController.php | 38 +++++- openapi.json | 12 +- openapi.yaml | 12 +- ...tomInternalNameConsistentNamingApiTest.php | 126 ++++++++++++++++++ .../Api/ApplicationSettingsApiTest.php | 1 + 5 files changed, 171 insertions(+), 18 deletions(-) create mode 100644 tests/Feature/Api/ApplicationCustomInternalNameConsistentNamingApiTest.php diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index a611a60f4f..0886ca812f 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -183,6 +183,26 @@ class ApplicationsController extends Controller ], 422); } + /** + * Container names honor custom_internal_name only with consistent container naming, so reject a name that would be ignored. + */ + private function customInternalNameValidationResponse(array $settings, ?Application $application = null): ?JsonResponse + { + if (! filled($settings['custom_internal_name'] ?? null)) { + return null; + } + $consistentNaming = $settings['is_consistent_container_name_enabled'] + ?? (bool) $application?->settings?->is_consistent_container_name_enabled; + if ($consistentNaming) { + return null; + } + + return response()->json([ + 'message' => 'Validation failed.', + 'errors' => ['custom_internal_name' => ['Set is_consistent_container_name_enabled to true to use custom_internal_name. Coolify ignores the custom internal name while consistent container naming is turned off.']], + ], 422); + } + private function applyApplicationSettings(Application $application, array $settings): void { if ($settings === []) { @@ -419,7 +439,7 @@ class ApplicationsController extends Controller 'gpu_device_ids' => ['type' => 'string', 'nullable' => true, 'description' => 'Comma-separated GPU device IDs.'], 'gpu_options' => ['type' => 'string', 'nullable' => true, 'description' => 'Additional GPU options.'], 'is_consistent_container_name_enabled' => ['type' => 'boolean', 'description' => 'Use a consistent container name across deployments.'], - 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name.'], + 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.'], 'custom_container_name_prefix' => ['type' => 'string', 'nullable' => true, 'description' => 'Prefix for generated container names (prefix-20260908T141530). Slugified and unique across the instance.'], 'preview_url_template' => ['type' => 'string', 'description' => 'Preview URL template.'], 'max_restart_count' => ['type' => 'integer', 'minimum' => 0, 'description' => 'Maximum container restart count before stopping.'], @@ -614,7 +634,7 @@ class ApplicationsController extends Controller 'gpu_device_ids' => ['type' => 'string', 'nullable' => true, 'description' => 'Comma-separated GPU device IDs.'], 'gpu_options' => ['type' => 'string', 'nullable' => true, 'description' => 'Additional GPU options.'], 'is_consistent_container_name_enabled' => ['type' => 'boolean', 'description' => 'Use a consistent container name across deployments.'], - 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name.'], + 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.'], 'custom_container_name_prefix' => ['type' => 'string', 'nullable' => true, 'description' => 'Prefix for generated container names (prefix-20260908T141530). Slugified and unique across the instance.'], 'preview_url_template' => ['type' => 'string', 'description' => 'Preview URL template.'], 'max_restart_count' => ['type' => 'integer', 'minimum' => 0, 'description' => 'Maximum container restart count before stopping.'], @@ -809,7 +829,7 @@ class ApplicationsController extends Controller 'gpu_device_ids' => ['type' => 'string', 'nullable' => true, 'description' => 'Comma-separated GPU device IDs.'], 'gpu_options' => ['type' => 'string', 'nullable' => true, 'description' => 'Additional GPU options.'], 'is_consistent_container_name_enabled' => ['type' => 'boolean', 'description' => 'Use a consistent container name across deployments.'], - 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name.'], + 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.'], 'custom_container_name_prefix' => ['type' => 'string', 'nullable' => true, 'description' => 'Prefix for generated container names (prefix-20260908T141530). Slugified and unique across the instance.'], 'preview_url_template' => ['type' => 'string', 'description' => 'Preview URL template.'], 'max_restart_count' => ['type' => 'integer', 'minimum' => 0, 'description' => 'Maximum container restart count before stopping.'], @@ -975,7 +995,7 @@ class ApplicationsController extends Controller 'gpu_device_ids' => ['type' => 'string', 'nullable' => true, 'description' => 'Comma-separated GPU device IDs.'], 'gpu_options' => ['type' => 'string', 'nullable' => true, 'description' => 'Additional GPU options.'], 'is_consistent_container_name_enabled' => ['type' => 'boolean', 'description' => 'Use a consistent container name across deployments.'], - 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name.'], + 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.'], 'custom_container_name_prefix' => ['type' => 'string', 'nullable' => true, 'description' => 'Prefix for generated container names (prefix-20260908T141530). Slugified and unique across the instance.'], 'preview_url_template' => ['type' => 'string', 'description' => 'Preview URL template.'], 'max_restart_count' => ['type' => 'integer', 'minimum' => 0, 'description' => 'Maximum container restart count before stopping.'], @@ -1137,7 +1157,7 @@ class ApplicationsController extends Controller 'gpu_device_ids' => ['type' => 'string', 'nullable' => true, 'description' => 'Comma-separated GPU device IDs.'], 'gpu_options' => ['type' => 'string', 'nullable' => true, 'description' => 'Additional GPU options.'], 'is_consistent_container_name_enabled' => ['type' => 'boolean', 'description' => 'Use a consistent container name across deployments.'], - 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name.'], + 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.'], 'custom_container_name_prefix' => ['type' => 'string', 'nullable' => true, 'description' => 'Prefix for generated container names (prefix-20260908T141530). Slugified and unique across the instance.'], 'preview_url_template' => ['type' => 'string', 'description' => 'Preview URL template.'], 'max_restart_count' => ['type' => 'integer', 'minimum' => 0, 'description' => 'Maximum container restart count before stopping.'], @@ -1391,6 +1411,9 @@ class ApplicationsController extends Controller if ($prefixValidation = $this->containerNamePrefixValidationResponse($applicationSettings, $destination->server)) { return $prefixValidation; } + if ($internalNameValidation = $this->customInternalNameValidationResponse($applicationSettings)) { + return $internalNameValidation; + } if ($type === 'public') { $validationRules = [ 'git_repository' => ['string', 'required', new ValidGitRepositoryUrl], @@ -3128,7 +3151,7 @@ class ApplicationsController extends Controller 'gpu_device_ids' => ['type' => 'string', 'nullable' => true, 'description' => 'Comma-separated GPU device IDs.'], 'gpu_options' => ['type' => 'string', 'nullable' => true, 'description' => 'Additional GPU options.'], 'is_consistent_container_name_enabled' => ['type' => 'boolean', 'description' => 'Use a consistent container name across deployments.'], - 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name.'], + 'custom_internal_name' => ['type' => 'string', 'nullable' => true, 'description' => 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.'], 'custom_container_name_prefix' => ['type' => 'string', 'nullable' => true, 'description' => 'Prefix for generated container names (prefix-20260908T141530). Slugified and unique across the instance.'], 'preview_url_template' => ['type' => 'string', 'description' => 'Preview URL template.'], 'max_restart_count' => ['type' => 'integer', 'minimum' => 0, 'description' => 'Maximum container restart count before stopping.'], @@ -3312,6 +3335,9 @@ class ApplicationsController extends Controller if ($prefixValidation = $this->containerNamePrefixValidationResponse($applicationSettings, $application->destination->server, $application)) { return $prefixValidation; } + if ($internalNameValidation = $this->customInternalNameValidationResponse($applicationSettings, $application)) { + return $internalNameValidation; + } $requestedBuildPack = $request->input('build_pack', $application->build_pack); if (($applicationSettings['is_raw_compose_deployment_enabled'] ?? false) && $requestedBuildPack !== 'dockercompose') { return response()->json([ diff --git a/openapi.json b/openapi.json index bfc925779c..1f0537a281 100644 --- a/openapi.json +++ b/openapi.json @@ -949,7 +949,7 @@ "custom_internal_name": { "type": "string", "nullable": true, - "description": "Custom internal container name." + "description": "Custom internal container name. Requires is_consistent_container_name_enabled to be true." }, "custom_container_name_prefix": { "type": "string", @@ -1540,7 +1540,7 @@ "custom_internal_name": { "type": "string", "nullable": true, - "description": "Custom internal container name." + "description": "Custom internal container name. Requires is_consistent_container_name_enabled to be true." }, "custom_container_name_prefix": { "type": "string", @@ -2131,7 +2131,7 @@ "custom_internal_name": { "type": "string", "nullable": true, - "description": "Custom internal container name." + "description": "Custom internal container name. Requires is_consistent_container_name_enabled to be true." }, "custom_container_name_prefix": { "type": "string", @@ -2617,7 +2617,7 @@ "custom_internal_name": { "type": "string", "nullable": true, - "description": "Custom internal container name." + "description": "Custom internal container name. Requires is_consistent_container_name_enabled to be true." }, "custom_container_name_prefix": { "type": "string", @@ -3083,7 +3083,7 @@ "custom_internal_name": { "type": "string", "nullable": true, - "description": "Custom internal container name." + "description": "Custom internal container name. Requires is_consistent_container_name_enabled to be true." }, "custom_container_name_prefix": { "type": "string", @@ -3794,7 +3794,7 @@ "custom_internal_name": { "type": "string", "nullable": true, - "description": "Custom internal container name." + "description": "Custom internal container name. Requires is_consistent_container_name_enabled to be true." }, "custom_container_name_prefix": { "type": "string", diff --git a/openapi.yaml b/openapi.yaml index 28ed4f49ed..69f2a33bc3 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -622,7 +622,7 @@ paths: custom_internal_name: type: string nullable: true - description: 'Custom internal container name.' + description: 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.' custom_container_name_prefix: type: string nullable: true @@ -1008,7 +1008,7 @@ paths: custom_internal_name: type: string nullable: true - description: 'Custom internal container name.' + description: 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.' custom_container_name_prefix: type: string nullable: true @@ -1394,7 +1394,7 @@ paths: custom_internal_name: type: string nullable: true - description: 'Custom internal container name.' + description: 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.' custom_container_name_prefix: type: string nullable: true @@ -1721,7 +1721,7 @@ paths: custom_internal_name: type: string nullable: true - description: 'Custom internal container name.' + description: 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.' custom_container_name_prefix: type: string nullable: true @@ -2034,7 +2034,7 @@ paths: custom_internal_name: type: string nullable: true - description: 'Custom internal container name.' + description: 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.' custom_container_name_prefix: type: string nullable: true @@ -2501,7 +2501,7 @@ paths: custom_internal_name: type: string nullable: true - description: 'Custom internal container name.' + description: 'Custom internal container name. Requires is_consistent_container_name_enabled to be true.' custom_container_name_prefix: type: string nullable: true diff --git a/tests/Feature/Api/ApplicationCustomInternalNameConsistentNamingApiTest.php b/tests/Feature/Api/ApplicationCustomInternalNameConsistentNamingApiTest.php new file mode 100644 index 0000000000..f745425781 --- /dev/null +++ b/tests/Feature/Api/ApplicationCustomInternalNameConsistentNamingApiTest.php @@ -0,0 +1,126 @@ + 'array']); + InstanceSettings::unguarded(fn () => 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]); + + $this->bearerToken = $this->user->createToken('custom-internal-name-test', ['*'])->plainTextToken; + $this->server = Server::factory()->create(['team_id' => $this->team->id]); + $this->destination = StandaloneDocker::where('server_id', $this->server->id)->first(); + $this->project = Project::factory()->create(['team_id' => $this->team->id]); + $this->environment = Environment::factory()->create(['project_id' => $this->project->id]); + $this->application = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + ]); +}); + +function customInternalNameCreatePayload(array $overrides): array +{ + return array_merge([ + 'project_uuid' => test()->project->uuid, + 'environment_uuid' => test()->environment->uuid, + 'server_uuid' => test()->server->uuid, + 'git_repository' => 'https://gitlab.com/coolify/custom-internal-name', + 'git_branch' => 'main', + 'build_pack' => 'nixpacks', + 'ports_exposes' => '3000', + 'autogenerate_domain' => false, + ], $overrides); +} + +const CUSTOM_INTERNAL_NAME_NEEDS_CONSISTENT_NAMING = 'Set is_consistent_container_name_enabled to true to use custom_internal_name. Coolify ignores the custom internal name while consistent container naming is turned off.'; + +test('updating only custom_internal_name is rejected while consistent container naming is off', function () { + $this->withToken($this->bearerToken) + ->patchJson("/api/v1/applications/{$this->application->uuid}", ['custom_internal_name' => 'my-app']) + ->assertUnprocessable() + ->assertJsonPath('errors.custom_internal_name.0', CUSTOM_INTERNAL_NAME_NEEDS_CONSISTENT_NAMING); + + expect($this->application->fresh()->settings->custom_internal_name)->toBeNull(); +}); + +test('updating custom_internal_name while turning consistent container naming off is rejected', function () { + $this->application->settings->update(['is_consistent_container_name_enabled' => true]); + + $this->withToken($this->bearerToken) + ->patchJson("/api/v1/applications/{$this->application->uuid}", [ + 'custom_internal_name' => 'my-app', + 'is_consistent_container_name_enabled' => false, + ]) + ->assertUnprocessable() + ->assertJsonPath('errors.custom_internal_name.0', CUSTOM_INTERNAL_NAME_NEEDS_CONSISTENT_NAMING); +}); + +test('updating custom_internal_name together with consistent container naming succeeds', function () { + $this->withToken($this->bearerToken) + ->patchJson("/api/v1/applications/{$this->application->uuid}", [ + 'custom_internal_name' => 'my-app', + 'is_consistent_container_name_enabled' => true, + ]) + ->assertOk(); + + $settings = $this->application->fresh()->settings; + expect($settings->custom_internal_name)->toBe('my-app') + ->and($settings->is_consistent_container_name_enabled)->toBeTrue(); +}); + +test('updating only custom_internal_name succeeds when consistent container naming is already on', function () { + $this->application->settings->update(['is_consistent_container_name_enabled' => true]); + + $this->withToken($this->bearerToken) + ->patchJson("/api/v1/applications/{$this->application->uuid}", ['custom_internal_name' => 'my-app']) + ->assertOk(); + + expect($this->application->fresh()->settings->custom_internal_name)->toBe('my-app'); +}); + +test('clearing custom_internal_name does not need consistent container naming', function (?string $value) { + $this->withToken($this->bearerToken) + ->patchJson("/api/v1/applications/{$this->application->uuid}", ['custom_internal_name' => $value]) + ->assertOk(); +})->with(['null' => [null], 'empty string' => ['']]); + +test('creating an application with custom_internal_name requires consistent container naming', function () { + Queue::fake(); + + $this->withToken($this->bearerToken) + ->postJson('/api/v1/applications/public', customInternalNameCreatePayload(['custom_internal_name' => 'my-app'])) + ->assertUnprocessable() + ->assertJsonPath('errors.custom_internal_name.0', CUSTOM_INTERNAL_NAME_NEEDS_CONSISTENT_NAMING); + + expect(Application::query()->count())->toBe(1); +}); + +test('creating an application with custom_internal_name and consistent container naming succeeds', function () { + Queue::fake(); + + $response = $this->withToken($this->bearerToken) + ->postJson('/api/v1/applications/public', customInternalNameCreatePayload([ + 'custom_internal_name' => 'my-app', + 'is_consistent_container_name_enabled' => true, + ])) + ->assertCreated(); + + expect(Application::query()->where('uuid', $response->json('uuid'))->sole()->settings->custom_internal_name)->toBe('my-app'); +}); diff --git a/tests/Feature/Api/ApplicationSettingsApiTest.php b/tests/Feature/Api/ApplicationSettingsApiTest.php index 8074be8bbe..7742d7ddbd 100644 --- a/tests/Feature/Api/ApplicationSettingsApiTest.php +++ b/tests/Feature/Api/ApplicationSettingsApiTest.php @@ -351,6 +351,7 @@ test('PATCH /api/v1/applications/{uuid} accepts Docker-compatible custom interna $this->withHeaders(applicationSettingsApiHeaders($this->bearerToken)) ->patchJson("/api/v1/applications/{$this->application->uuid}", [ 'custom_internal_name' => $name, + 'is_consistent_container_name_enabled' => true, ]) ->assertOk(); From 0abf82e631599bf3477a3e4aa9a0f8aefd4880e6 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:24:05 +0200 Subject: [PATCH 19/25] fix(api): limit pull request ids to the database integer range A pull request id above 2147483647 caused a database error and a 500. - Previews store: validate max 2147483647 (422) - /deploy: return 400 for pr or pull_request_id out of range - Preview show, logs, update, and delete: return 422 for ids out of range - Document the maximum in the OpenAPI spec --- .../Api/ApplicationPreviewsController.php | 6 +- .../Api/ApplicationsController.php | 6 +- app/Http/Controllers/Api/DeployController.php | 7 +- openapi.json | 7 +- openapi.yaml | 3 + .../Api/PreviewPullRequestIdRangeApiTest.php | 92 +++++++++++++++++++ 6 files changed, 111 insertions(+), 10 deletions(-) create mode 100644 tests/Feature/Api/PreviewPullRequestIdRangeApiTest.php diff --git a/app/Http/Controllers/Api/ApplicationPreviewsController.php b/app/Http/Controllers/Api/ApplicationPreviewsController.php index 1e82df5816..453d605695 100644 --- a/app/Http/Controllers/Api/ApplicationPreviewsController.php +++ b/app/Http/Controllers/Api/ApplicationPreviewsController.php @@ -91,7 +91,7 @@ class ApplicationPreviewsController extends Controller $this->authorize('view', $application); $pullRequestIdRaw = $request->route('pull_request_id'); - if (! ctype_digit((string) $pullRequestIdRaw) || (int) $pullRequestIdRaw <= 0) { + if (! ctype_digit((string) $pullRequestIdRaw) || (int) $pullRequestIdRaw <= 0 || (int) $pullRequestIdRaw > 2147483647) { return response()->json(['message' => 'Invalid pull_request_id.'], 422); } @@ -116,7 +116,7 @@ class ApplicationPreviewsController extends Controller requestBody: new OA\RequestBody(required: true, content: new OA\JsonContent( required: ['pull_request_id'], properties: [ - new OA\Property(property: 'pull_request_id', type: 'integer', minimum: 1, example: 42), + new OA\Property(property: 'pull_request_id', type: 'integer', minimum: 1, maximum: 2147483647, example: 42), new OA\Property(property: 'pull_request_html_url', type: 'string', nullable: true, example: 'https://github.com/org/repo/pull/42'), new OA\Property(property: 'git_type', type: 'string', nullable: true, enum: self::GIT_TYPES, description: 'Git provider that hosts the pull request. Sets the ref Coolify fetches (pull/{id}/head or merge-requests/{id}/head).'), new OA\Property(property: 'commit', type: 'string', nullable: true, description: 'Commit SHA to deploy. Required for Bitbucket.'), @@ -186,7 +186,7 @@ class ApplicationPreviewsController extends Controller } $rules = [ - 'pull_request_id' => 'required|integer|min:1', + 'pull_request_id' => 'required|integer|min:1|max:2147483647', 'pull_request_html_url' => 'nullable|url|max:2048', 'force' => 'boolean', 'instant_deploy' => 'boolean', diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index 0886ca812f..8513dc1180 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -2621,7 +2621,7 @@ class ApplicationsController extends Controller $pullRequestId = null; $pullRequestIdRaw = $request->route('pull_request_id'); if ($pullRequestIdRaw !== null) { - if (! ctype_digit((string) $pullRequestIdRaw) || (int) $pullRequestIdRaw <= 0) { + if (! ctype_digit((string) $pullRequestIdRaw) || (int) $pullRequestIdRaw <= 0 || (int) $pullRequestIdRaw > 2147483647) { return response()->json(['message' => 'Invalid pull_request_id.'], 422); } $pullRequestId = (int) $pullRequestIdRaw; @@ -2728,7 +2728,7 @@ class ApplicationsController extends Controller $this->authorize('update', $application); $pullRequestIdRaw = $request->route('pull_request_id'); - if (! ctype_digit((string) $pullRequestIdRaw) || (int) $pullRequestIdRaw <= 0) { + if (! ctype_digit((string) $pullRequestIdRaw) || (int) $pullRequestIdRaw <= 0 || (int) $pullRequestIdRaw > 2147483647) { return response()->json(['message' => 'Invalid pull_request_id.'], 422); } @@ -5655,7 +5655,7 @@ class ApplicationsController extends Controller $this->authorize('delete', $application); $pullRequestIdRaw = $request->route('pull_request_id'); - if (! ctype_digit((string) $pullRequestIdRaw) || (int) $pullRequestIdRaw <= 0) { + if (! ctype_digit((string) $pullRequestIdRaw) || (int) $pullRequestIdRaw <= 0 || (int) $pullRequestIdRaw > 2147483647) { return response()->json(['message' => 'Invalid pull_request_id.'], 422); } $pullRequestId = (int) $pullRequestIdRaw; diff --git a/app/Http/Controllers/Api/DeployController.php b/app/Http/Controllers/Api/DeployController.php index e5aac43b85..fc7a9d5460 100644 --- a/app/Http/Controllers/Api/DeployController.php +++ b/app/Http/Controllers/Api/DeployController.php @@ -315,8 +315,8 @@ class DeployController extends Controller new OA\Parameter(name: 'tag', in: 'query', description: 'Tag name(s). Comma separated list is also accepted.', schema: new OA\Schema(type: 'string')), new OA\Parameter(name: 'uuid', in: 'query', description: 'Resource UUID(s). Comma separated list is also accepted.', schema: new OA\Schema(type: 'string')), new OA\Parameter(name: 'force', in: 'query', description: 'Force rebuild (without cache)', schema: new OA\Schema(type: 'boolean')), - new OA\Parameter(name: 'pr', in: 'query', description: 'Pull Request Id for deploying specific PR builds. Cannot be used with tag parameter.', schema: new OA\Schema(type: 'integer')), - new OA\Parameter(name: 'pull_request_id', in: 'query', description: 'Preview deployment identifier. Alias of pr.', schema: new OA\Schema(type: 'integer')), + new OA\Parameter(name: 'pr', in: 'query', description: 'Pull Request Id for deploying specific PR builds. Cannot be used with tag parameter.', schema: new OA\Schema(type: 'integer', maximum: 2147483647)), + new OA\Parameter(name: 'pull_request_id', in: 'query', description: 'Preview deployment identifier. Alias of pr.', schema: new OA\Schema(type: 'integer', maximum: 2147483647)), new OA\Parameter(name: 'docker_tag', in: 'query', description: 'Docker image tag for Docker Image preview deployments. Requires pull_request_id.', schema: new OA\Schema(type: 'string')), ], @@ -372,6 +372,9 @@ class DeployController extends Controller $pr = $pullRequestId ? max((int) $pullRequestId, 0) : 0; $dockerTag = $request->string('docker_tag')->trim()->value() ?: null; + if ($pr > 2147483647) { + return response()->json(['message' => 'pull_request_id must be between 1 and 2147483647.'], 400); + } if ($uuids && $tags) { return response()->json(['message' => 'You can only use uuid or tag, not both.'], 400); } diff --git a/openapi.json b/openapi.json index 1f0537a281..a500a6d03f 100644 --- a/openapi.json +++ b/openapi.json @@ -86,6 +86,7 @@ "properties": { "pull_request_id": { "type": "integer", + "maximum": 2147483647, "minimum": 1, "example": 42 }, @@ -10274,7 +10275,8 @@ "in": "query", "description": "Pull Request Id for deploying specific PR builds. Cannot be used with tag parameter.", "schema": { - "type": "integer" + "type": "integer", + "maximum": 2147483647 } }, { @@ -10282,7 +10284,8 @@ "in": "query", "description": "Preview deployment identifier. Alias of pr.", "schema": { - "type": "integer" + "type": "integer", + "maximum": 2147483647 } }, { diff --git a/openapi.yaml b/openapi.yaml index 69f2a33bc3..16cbdc4a30 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -62,6 +62,7 @@ paths: properties: pull_request_id: type: integer + maximum: 2147483647 minimum: 1 example: 42 pull_request_html_url: @@ -6722,12 +6723,14 @@ paths: description: 'Pull Request Id for deploying specific PR builds. Cannot be used with tag parameter.' schema: type: integer + maximum: 2147483647 - name: pull_request_id in: query description: 'Preview deployment identifier. Alias of pr.' schema: type: integer + maximum: 2147483647 - name: docker_tag in: query diff --git a/tests/Feature/Api/PreviewPullRequestIdRangeApiTest.php b/tests/Feature/Api/PreviewPullRequestIdRangeApiTest.php new file mode 100644 index 0000000000..ce83a938fc --- /dev/null +++ b/tests/Feature/Api/PreviewPullRequestIdRangeApiTest.php @@ -0,0 +1,92 @@ +set('app.maintenance.store', 'array'); + InstanceSettings::unguarded(fn () => 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]); + + $this->bearerToken = $this->user->createToken('preview-range', ['*'])->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([ + 'build_pack' => 'dockerimage', + 'docker_registry_image_name' => 'nginx', + 'docker_registry_image_tag' => 'alpine', + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + ]); +}); + +test('creating a preview rejects a pull request id above the database integer range', function () { + $this->withToken($this->bearerToken) + ->postJson("/api/v1/applications/{$this->application->uuid}/previews", [ + 'pull_request_id' => 2147483648, + 'docker_tag' => 'pr-1', + 'instant_deploy' => false, + ]) + ->assertUnprocessable() + ->assertJsonValidationErrors('pull_request_id'); + + expect($this->application->previews()->count())->toBe(0); +}); + +test('creating a preview accepts the largest database integer pull request id', function () { + $this->withToken($this->bearerToken) + ->postJson("/api/v1/applications/{$this->application->uuid}/previews", [ + 'pull_request_id' => 2147483647, + 'docker_tag' => 'pr-1', + 'instant_deploy' => false, + ]) + ->assertCreated() + ->assertJsonPath('preview.pull_request_id', 2147483647); +}); + +test('deploy rejects a pull request id above the database integer range', function (string $field) { + $this->withToken($this->bearerToken) + ->postJson('/api/v1/deploy', ['uuid' => $this->application->uuid, $field => '2147483648']) + ->assertStatus(400) + ->assertJsonPath('message', 'pull_request_id must be between 1 and 2147483647.'); + + Bus::assertNothingDispatched(); +})->with(['pr', 'pull_request_id']); + +test('deploy passes the largest database integer pull request id to the preview lookup', function () { + $this->withToken($this->bearerToken) + ->postJson('/api/v1/deploy', ['uuid' => $this->application->uuid, 'pr' => 2147483647]) + ->assertOk() + ->assertJsonPath('deployments.0.message', 'Pull request 2147483647 not found for this resource.'); +}); + +test('preview routes reject a pull request id above the database integer range', function (string $method, string $suffix) { + $this->withToken($this->bearerToken) + ->json($method, "/api/v1/applications/{$this->application->uuid}/previews/2147483648{$suffix}") + ->assertUnprocessable() + ->assertJsonPath('message', 'Invalid pull_request_id.'); +})->with([ + 'show' => ['GET', ''], + 'update' => ['PATCH', ''], + 'delete' => ['DELETE', ''], + 'logs' => ['GET', '/logs'], +]); From e11be922b453a3464885d0d0ed7ec326d9b0fbe3 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:24:14 +0200 Subject: [PATCH 20/25] fix(mcp): limit the deploy tool pull request id to the integer range The MCP deploy tool passed pull_request_id without a range check. - Return an error for ids below 0 or above 2147483647 --- app/Mcp/Tools/Deploy.php | 3 + .../Mcp/McpDeployPullRequestIdRangeTest.php | 77 +++++++++++++++++++ 2 files changed, 80 insertions(+) create mode 100644 tests/Feature/Mcp/McpDeployPullRequestIdRangeTest.php diff --git a/app/Mcp/Tools/Deploy.php b/app/Mcp/Tools/Deploy.php index 769b7cacd4..980e9e051d 100644 --- a/app/Mcp/Tools/Deploy.php +++ b/app/Mcp/Tools/Deploy.php @@ -42,6 +42,9 @@ class Deploy extends Tool $force = filter_var($request->get('force'), FILTER_VALIDATE_BOOLEAN); $pullRequestId = (int) ($request->get('pull_request_id') ?? 0); + if ($pullRequestId < 0 || $pullRequestId > 2147483647) { + return $this->mcpError($request, 'pull_request_id must be between 0 and 2147483647.', ['resource_uuid' => $uuid]); + } $deploymentUuid = new_public_id(); $result = queue_application_deployment( diff --git a/tests/Feature/Mcp/McpDeployPullRequestIdRangeTest.php b/tests/Feature/Mcp/McpDeployPullRequestIdRangeTest.php new file mode 100644 index 0000000000..a1b88dd015 --- /dev/null +++ b/tests/Feature/Mcp/McpDeployPullRequestIdRangeTest.php @@ -0,0 +1,77 @@ + 'array', + 'session.driver' => 'array', + 'app.maintenance.driver' => 'file', + ]); + Queue::fake(); + + InstanceSettings::query()->delete(); + $settings = new InstanceSettings(['is_mcp_server_enabled' => true]); + $settings->id = 0; + $settings->save(); + + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'owner']); + session(['currentTeam' => $this->team]); + + $server = Server::factory()->create(['team_id' => $this->team->id]); + $destination = StandaloneDocker::query()->where('server_id', $server->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $this->team->id]); + $environment = $project->environments()->first() + ?? 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 mcpDeployPullRequestCall(array $arguments) +{ + auth()->forgetGuards(); + $token = test()->user->createToken('mcp-deploy-pr', ['read', 'deploy'])->plainTextToken; + + return test()->withHeaders([ + 'Content-Type' => 'application/json', + 'Accept' => 'application/json, text/event-stream', + 'Authorization' => 'Bearer '.$token, + ])->postJson('/mcp', [ + 'jsonrpc' => '2.0', + 'id' => 1, + 'method' => 'tools/call', + 'params' => ['name' => 'deploy', 'arguments' => (object) $arguments], + ]); +} + +test('deploy tool rejects a pull request id above the database integer range', function (int $pullRequestId) { + $response = mcpDeployPullRequestCall([ + 'uuid' => $this->application->uuid, + 'pull_request_id' => $pullRequestId, + ]); + + $response->assertOk(); + expect($response->json('result.isError'))->toBeTrue() + ->and($response->json('result.content.0.text'))->toContain('pull_request_id must be between 0 and 2147483647') + ->and(ApplicationDeploymentQueue::query()->count())->toBe(0); +})->with([ + 'above int4' => 3000000000, + 'negative' => -1, +]); From 111ff9adaebd1d0d5d621503cd3ea6673270ecbb Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:24:20 +0200 Subject: [PATCH 21/25] docs(api): document audit events and fix the private key update path - Add the OpenAPI attribute for GET /audit-events - Document PATCH /security/keys/{uuid} with its uuid parameter instead of /security/keys --- .../Controllers/Api/AuditEventsController.php | 71 ++++ .../Controllers/Api/SecurityController.php | 9 +- openapi.json | 318 ++++++++++++++---- openapi.yaml | 173 +++++++--- ...itEventsAndPrivateKeyUpdateOpenApiTest.php | 24 ++ 5 files changed, 490 insertions(+), 105 deletions(-) create mode 100644 tests/Unit/AuditEventsAndPrivateKeyUpdateOpenApiTest.php diff --git a/app/Http/Controllers/Api/AuditEventsController.php b/app/Http/Controllers/Api/AuditEventsController.php index 19e916dc10..fdb63c3ead 100644 --- a/app/Http/Controllers/Api/AuditEventsController.php +++ b/app/Http/Controllers/Api/AuditEventsController.php @@ -8,9 +8,80 @@ use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\Validator; use Illuminate\Validation\Rule; +use OpenApi\Attributes as OA; class AuditEventsController extends Controller { + #[OA\Get( + summary: 'List', + description: 'List audit events of the team. Only team admins and owners can view audit events. Actor e-mail, token, metadata, changes, IP address, and user agent are returned only for tokens with the read:sensitive or root permission.', + path: '/audit-events', + operationId: 'list-audit-events', + security: [ + ['bearerAuth' => []], + ], + tags: ['Audit Events'], + parameters: [ + new OA\Parameter(name: 'per_page', in: 'query', description: 'Items per page.', required: false, schema: new OA\Schema(type: 'integer', default: 25, minimum: 1, maximum: 100)), + new OA\Parameter(name: 'page', in: 'query', description: 'Page number.', required: false, schema: new OA\Schema(type: 'integer', default: 1, minimum: 1)), + new OA\Parameter(name: 'search', in: 'query', description: 'Search text.', required: false, schema: new OA\Schema(type: 'string', maxLength: 255)), + new OA\Parameter(name: 'action', in: 'query', description: 'Filter by action. Use all for every action.', required: false, schema: new OA\Schema(type: 'string', maxLength: 255)), + new OA\Parameter(name: 'source', in: 'query', description: 'Filter by source.', required: false, schema: new OA\Schema(type: 'string', enum: ['all', 'ui', 'api', 'mcp', 'webhook', 'system', 'scheduler'])), + ], + responses: [ + new OA\Response( + response: 200, + description: 'A page of audit events, newest first.', + content: [ + new OA\MediaType( + mediaType: 'application/json', + schema: new OA\Schema( + type: 'object', + properties: [ + 'current_page' => ['type' => 'integer'], + 'data' => [ + 'type' => 'array', + 'items' => [ + 'type' => 'object', + 'properties' => [ + 'id' => ['type' => 'integer'], + 'team_id' => ['type' => 'integer', 'nullable' => true], + 'event' => ['type' => 'string'], + 'source' => ['type' => 'string'], + 'action' => ['type' => 'string'], + 'level' => ['type' => 'string'], + 'actor_type' => ['type' => 'string', 'nullable' => true], + 'actor_id' => ['type' => 'integer', 'nullable' => true], + 'actor_name' => ['type' => 'string', 'nullable' => true], + 'resource_type' => ['type' => 'string', 'nullable' => true], + 'resource_uuid' => ['type' => 'string', 'nullable' => true], + 'resource_name' => ['type' => 'string', 'nullable' => true], + 'description' => ['type' => 'string', 'nullable' => true], + 'created_at' => ['type' => 'string', 'format' => 'date-time'], + ], + ], + ], + 'per_page' => ['type' => 'integer'], + 'last_page' => ['type' => 'integer'], + 'total' => ['type' => 'integer'], + ] + ) + ), + ]), + new OA\Response( + response: 401, + ref: '#/components/responses/401', + ), + new OA\Response( + response: 403, + description: 'Only team admins and owners can view audit logs.', + ), + new OA\Response( + response: 422, + ref: '#/components/responses/422', + ), + ] + )] public function index(Request $request): JsonResponse { $teamId = getTeamIdFromToken(); diff --git a/app/Http/Controllers/Api/SecurityController.php b/app/Http/Controllers/Api/SecurityController.php index 036a6e9328..f5b0b0366b 100644 --- a/app/Http/Controllers/Api/SecurityController.php +++ b/app/Http/Controllers/Api/SecurityController.php @@ -253,12 +253,15 @@ class SecurityController extends Controller #[OA\Patch( summary: 'Update', description: 'Update a private key.', - path: '/security/keys', + path: '/security/keys/{uuid}', operationId: 'update-private-key', security: [ ['bearerAuth' => []], ], tags: ['Private Keys'], + parameters: [ + new OA\Parameter(name: 'uuid', in: 'path', required: true, description: 'Private Key UUID', schema: new OA\Schema(type: 'string')), + ], requestBody: new OA\RequestBody( required: true, content: [ @@ -300,6 +303,10 @@ class SecurityController extends Controller response: 400, ref: '#/components/responses/400', ), + new OA\Response( + response: 404, + description: 'Private Key not found.', + ), new OA\Response( response: 422, ref: '#/components/responses/422', diff --git a/openapi.json b/openapi.json index a500a6d03f..f3a237b856 100644 --- a/openapi.json +++ b/openapi.json @@ -5732,6 +5732,178 @@ ] } }, + "\/audit-events": { + "get": { + "tags": [ + "Audit Events" + ], + "summary": "List", + "description": "List audit events of the team. Only team admins and owners can view audit events. Actor e-mail, token, metadata, changes, IP address, and user agent are returned only for tokens with the read:sensitive or root permission.", + "operationId": "list-audit-events", + "parameters": [ + { + "name": "per_page", + "in": "query", + "description": "Items per page.", + "required": false, + "schema": { + "type": "integer", + "default": 25, + "maximum": 100, + "minimum": 1 + } + }, + { + "name": "page", + "in": "query", + "description": "Page number.", + "required": false, + "schema": { + "type": "integer", + "default": 1, + "minimum": 1 + } + }, + { + "name": "search", + "in": "query", + "description": "Search text.", + "required": false, + "schema": { + "type": "string", + "maxLength": 255 + } + }, + { + "name": "action", + "in": "query", + "description": "Filter by action. Use all for every action.", + "required": false, + "schema": { + "type": "string", + "maxLength": 255 + } + }, + { + "name": "source", + "in": "query", + "description": "Filter by source.", + "required": false, + "schema": { + "type": "string", + "enum": [ + "all", + "ui", + "api", + "mcp", + "webhook", + "system", + "scheduler" + ] + } + } + ], + "responses": { + "200": { + "description": "A page of audit events, newest first.", + "content": { + "application\/json": { + "schema": { + "properties": { + "current_page": { + "type": "integer" + }, + "data": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "integer" + }, + "team_id": { + "type": "integer", + "nullable": true + }, + "event": { + "type": "string" + }, + "source": { + "type": "string" + }, + "action": { + "type": "string" + }, + "level": { + "type": "string" + }, + "actor_type": { + "type": "string", + "nullable": true + }, + "actor_id": { + "type": "integer", + "nullable": true + }, + "actor_name": { + "type": "string", + "nullable": true + }, + "resource_type": { + "type": "string", + "nullable": true + }, + "resource_uuid": { + "type": "string", + "nullable": true + }, + "resource_name": { + "type": "string", + "nullable": true + }, + "description": { + "type": "string", + "nullable": true + }, + "created_at": { + "type": "string", + "format": "date-time" + } + } + } + }, + "per_page": { + "type": "integer" + }, + "last_page": { + "type": "integer" + }, + "total": { + "type": "integer" + } + }, + "type": "object" + } + } + } + }, + "401": { + "$ref": "#\/components\/responses\/401" + }, + "403": { + "description": "Only team admins and owners can view audit logs." + }, + "422": { + "$ref": "#\/components\/responses\/422" + } + }, + "security": [ + { + "bearerAuth": [] + } + ] + } + }, "\/cloud-init-scripts": { "get": { "tags": [ @@ -15231,70 +15403,6 @@ "bearerAuth": [] } ] - }, - "patch": { - "tags": [ - "Private Keys" - ], - "summary": "Update", - "description": "Update a private key.", - "operationId": "update-private-key", - "requestBody": { - "required": true, - "content": { - "application\/json": { - "schema": { - "required": [ - "private_key" - ], - "properties": { - "name": { - "type": "string" - }, - "description": { - "type": "string" - }, - "private_key": { - "type": "string" - } - }, - "type": "object", - "additionalProperties": false - } - } - } - }, - "responses": { - "201": { - "description": "The updated private key's UUID.", - "content": { - "application\/json": { - "schema": { - "properties": { - "uuid": { - "type": "string" - } - }, - "type": "object" - } - } - } - }, - "401": { - "$ref": "#\/components\/responses\/401" - }, - "400": { - "$ref": "#\/components\/responses\/400" - }, - "422": { - "$ref": "#\/components\/responses\/422" - } - }, - "security": [ - { - "bearerAuth": [] - } - ] } }, "\/security\/keys\/{uuid}": { @@ -15409,6 +15517,84 @@ "bearerAuth": [] } ] + }, + "patch": { + "tags": [ + "Private Keys" + ], + "summary": "Update", + "description": "Update a private key.", + "operationId": "update-private-key", + "parameters": [ + { + "name": "uuid", + "in": "path", + "description": "Private Key UUID", + "required": true, + "schema": { + "type": "string" + } + } + ], + "requestBody": { + "required": true, + "content": { + "application\/json": { + "schema": { + "required": [ + "private_key" + ], + "properties": { + "name": { + "type": "string" + }, + "description": { + "type": "string" + }, + "private_key": { + "type": "string" + } + }, + "type": "object", + "additionalProperties": false + } + } + } + }, + "responses": { + "201": { + "description": "The updated private key's UUID.", + "content": { + "application\/json": { + "schema": { + "properties": { + "uuid": { + "type": "string" + } + }, + "type": "object" + } + } + } + }, + "401": { + "$ref": "#\/components\/responses\/401" + }, + "400": { + "$ref": "#\/components\/responses\/400" + }, + "404": { + "description": "Private Key not found." + }, + "422": { + "$ref": "#\/components\/responses\/422" + } + }, + "security": [ + { + "bearerAuth": [] + } + ] } }, "\/servers\/{uuid}\/cloudflare-tunnel": { @@ -24606,6 +24792,10 @@ "name": "Secret Managers", "description": "Secret Managers" }, + { + "name": "Audit Events", + "description": "Audit Events" + }, { "name": "Cloud-init Scripts", "description": "Cloud-init Scripts" diff --git a/openapi.yaml b/openapi.yaml index 16cbdc4a30..405551392b 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -3707,6 +3707,86 @@ paths: security: - bearerAuth: [] + /audit-events: + get: + tags: + - 'Audit Events' + summary: List + description: 'List audit events of the team. Only team admins and owners can view audit events. Actor e-mail, token, metadata, changes, IP address, and user agent are returned only for tokens with the read:sensitive or root permission.' + operationId: list-audit-events + parameters: + - + name: per_page + in: query + description: 'Items per page.' + required: false + schema: + type: integer + default: 25 + maximum: 100 + minimum: 1 + - + name: page + in: query + description: 'Page number.' + required: false + schema: + type: integer + default: 1 + minimum: 1 + - + name: search + in: query + description: 'Search text.' + required: false + schema: + type: string + maxLength: 255 + - + name: action + in: query + description: 'Filter by action. Use all for every action.' + required: false + schema: + type: string + maxLength: 255 + - + name: source + in: query + description: 'Filter by source.' + required: false + schema: + type: string + enum: + - all + - ui + - api + - mcp + - webhook + - system + - scheduler + responses: + '200': + description: 'A page of audit events, newest first.' + content: + application/json: + schema: + properties: + current_page: { type: integer } + data: { type: array, items: { type: object, properties: { id: { type: integer }, team_id: { type: integer, nullable: true }, event: { type: string }, source: { type: string }, action: { type: string }, level: { type: string }, actor_type: { type: string, nullable: true }, actor_id: { type: integer, nullable: true }, actor_name: { type: string, nullable: true }, resource_type: { type: string, nullable: true }, resource_uuid: { type: string, nullable: true }, resource_name: { type: string, nullable: true }, description: { type: string, nullable: true }, created_at: { type: string, format: date-time } } } } + per_page: { type: integer } + last_page: { type: integer } + total: { type: integer } + type: object + '401': + $ref: '#/components/responses/401' + '403': + description: 'Only team admins and owners can view audit logs.' + '422': + $ref: '#/components/responses/422' + security: + - + bearerAuth: [] /cloud-init-scripts: get: tags: @@ -9763,46 +9843,6 @@ paths: security: - bearerAuth: [] - patch: - tags: - - 'Private Keys' - summary: Update - description: 'Update a private key.' - operationId: update-private-key - requestBody: - required: true - content: - application/json: - schema: - required: - - private_key - properties: - name: - type: string - description: - type: string - private_key: - type: string - type: object - additionalProperties: false - responses: - '201': - description: "The updated private key's UUID." - content: - application/json: - schema: - properties: - uuid: { type: string } - type: object - '401': - $ref: '#/components/responses/401' - '400': - $ref: '#/components/responses/400' - '422': - $ref: '#/components/responses/422' - security: - - - bearerAuth: [] '/security/keys/{uuid}': get: tags: @@ -9874,6 +9914,56 @@ paths: security: - bearerAuth: [] + patch: + tags: + - 'Private Keys' + summary: Update + description: 'Update a private key.' + operationId: update-private-key + parameters: + - + name: uuid + in: path + description: 'Private Key UUID' + required: true + schema: + type: string + requestBody: + required: true + content: + application/json: + schema: + required: + - private_key + properties: + name: + type: string + description: + type: string + private_key: + type: string + type: object + additionalProperties: false + responses: + '201': + description: "The updated private key's UUID." + content: + application/json: + schema: + properties: + uuid: { type: string } + type: object + '401': + $ref: '#/components/responses/401' + '400': + $ref: '#/components/responses/400' + '404': + description: 'Private Key not found.' + '422': + $ref: '#/components/responses/422' + security: + - + bearerAuth: [] '/servers/{uuid}/cloudflare-tunnel': get: tags: @@ -15779,6 +15869,9 @@ tags: - name: 'Secret Managers' description: 'Secret Managers' + - + name: 'Audit Events' + description: 'Audit Events' - name: 'Cloud-init Scripts' description: 'Cloud-init Scripts' diff --git a/tests/Unit/AuditEventsAndPrivateKeyUpdateOpenApiTest.php b/tests/Unit/AuditEventsAndPrivateKeyUpdateOpenApiTest.php new file mode 100644 index 0000000000..4af926f3d6 --- /dev/null +++ b/tests/Unit/AuditEventsAndPrivateKeyUpdateOpenApiTest.php @@ -0,0 +1,24 @@ +not->toBeNull() + ->and($operation['operationId'])->toBe('list-audit-events') + ->and($operation['security'])->toBe([['bearerAuth' => []]]) + ->and(collect($operation['parameters'])->pluck('name')->all())->toBe(['per_page', 'page', 'search', 'action', 'source']) + ->and($operation['responses'])->toHaveKeys(['200', '401', '403', '422']); +}); + +test('documents the private key update on the uuid path', function () { + $paths = auditAndPrivateKeyOpenApiDocument()['paths']; + + expect($paths['/security/keys'])->not->toHaveKey('patch') + ->and($paths['/security/keys/{uuid}']['patch']['operationId'])->toBe('update-private-key') + ->and($paths['/security/keys/{uuid}']['patch']['parameters'][0])->toMatchArray(['name' => 'uuid', 'in' => 'path', 'required' => true]); +}); From e09876adcd3518dcb033a5ea1dd67e10f3cf8843 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:24:30 +0200 Subject: [PATCH 22/25] fix(config): use docker.io as the default registry A merge brought back ghcr.io as the default registry and helper image registry. The install and upgrade scripts and the compose files use docker.io. - Default REGISTRY_URL and the helper image registry to docker.io --- config/constants.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/constants.php b/config/constants.php index b833814762..c25c29ca9b 100644 --- a/config/constants.php +++ b/config/constants.php @@ -15,8 +15,8 @@ return [ // devHostDockerPath() uses them only for that server. The defaults are the legacy docker-compose.dev.yml names. 'dev_data_volume' => env('DEV_COOLIFY_DATA_VOLUME', 'coolify_dev_coolify_data'), 'dev_backups_volume' => env('DEV_COOLIFY_BACKUPS_VOLUME', 'coolify_dev_backups_data'), - 'registry_url' => env('REGISTRY_URL', 'ghcr.io'), - 'helper_image' => env('HELPER_IMAGE', env('REGISTRY_URL', 'ghcr.io').'/coollabsio/coolify-helper'), + 'registry_url' => env('REGISTRY_URL', 'docker.io'), + 'helper_image' => env('HELPER_IMAGE', env('REGISTRY_URL', 'docker.io').'/coollabsio/coolify-helper'), 'is_windows_docker_desktop' => env('IS_WINDOWS_DOCKER_DESKTOP', false), 'cdn_url' => env('CDN_URL', 'https://cdn.coollabs.io'), 'versions_url' => env('VERSIONS_URL', env('CDN_URL', 'https://cdn.coollabs.io').'/coolify/versions.json'), From 8564741d38eee133fb315e123ffe16b9f6a804c4 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:24:32 +0200 Subject: [PATCH 23/25] fix(notifications): send a disk usage alert again after usage drops The high disk usage throttle stayed for 24 hours after usage dropped, so a new spike in that time sent no alert. - Release the throttle when disk usage is at or below the threshold --- app/Jobs/ServerStorageCheckJob.php | 4 ++++ ...erStorageCheckNotificationIntervalTest.php | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/app/Jobs/ServerStorageCheckJob.php b/app/Jobs/ServerStorageCheckJob.php index a457207951..1e1b2c3c2a 100644 --- a/app/Jobs/ServerStorageCheckJob.php +++ b/app/Jobs/ServerStorageCheckJob.php @@ -2,6 +2,7 @@ namespace App\Jobs; +use App\Models\NotificationThrottle; use App\Models\Server; use App\Notifications\Server\HighDiskUsage; use Illuminate\Bus\Queueable; @@ -59,6 +60,9 @@ class ServerStorageCheckJob implements ShouldBeEncrypted, ShouldQueue, Silenced } if ($this->percentage > $serverDiskUsageNotificationThreshold) { $team->notify(new HighDiskUsage($this->server, $this->percentage, $serverDiskUsageNotificationThreshold)); + } else { + // Usage recovered: the next spike should alert again instead of waiting for the interval. + NotificationThrottle::release($this->server, HighDiskUsage::class); } } catch (\Throwable $e) { return handleError($e); diff --git a/tests/Feature/Server/ServerStorageCheckNotificationIntervalTest.php b/tests/Feature/Server/ServerStorageCheckNotificationIntervalTest.php index ff0caf1a98..d4e9563396 100644 --- a/tests/Feature/Server/ServerStorageCheckNotificationIntervalTest.php +++ b/tests/Feature/Server/ServerStorageCheckNotificationIntervalTest.php @@ -67,6 +67,25 @@ it('sends one high disk usage notification per configured interval', function () Notification::assertSentToTimes($server->team, HighDiskUsage::class, 2); }); +it('notifies again on a new spike after disk usage recovered within the interval', function () { + $server = diskUsageIntervalServer(intervalHours: 24); + + runDiskUsageCheck($server, 93); + Notification::assertSentToTimes($server->team, HighDiskUsage::class, 1); + + Carbon::setTestNow(now()->addMinutes(10)); + runDiskUsageCheck($server, 95); + Notification::assertSentToTimes($server->team, HighDiskUsage::class, 1); + + Carbon::setTestNow(now()->addHour()); + runDiskUsageCheck($server, 40); + expect(NotificationThrottle::wasSent($server, HighDiskUsage::class))->toBeFalse(); + + Carbon::setTestNow(now()->addHour()); + runDiskUsageCheck($server, 92); + Notification::assertSentToTimes($server->team, HighDiskUsage::class, 2); +}); + it('defaults to one notification per 24 hours', function () { $team = Team::factory()->create(); $server = Server::factory()->create(['team_id' => $team->id]); From 8aa57d66e8d7d5c5382e2388b5fea1c905273c52 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:25:40 +0200 Subject: [PATCH 24/25] fix(previews): give each preview its own volume for external volumes Previews of Docker Compose applications used external volumes as written, so a preview mounted the production volume. Two database containers on the same data can corrupt it. - A preview uses {uuid}_-pr-N with a name-only declaration, the same name as v4.3.23, so old preview volumes are used again - Production still uses the external volume as written - Preview cleanup removes the preview volume, never the external one - Do not show the external volume warning for previews --- app/Models/LocalPersistentVolume.php | 8 +- bootstrap/helpers/parsers.php | 41 +++++--- .../ApplicationPreviewComposeCleanupTest.php | 3 +- tests/Feature/ComposeExternalVolumeTest.php | 95 +++++++++++++++---- 4 files changed, 112 insertions(+), 35 deletions(-) diff --git a/app/Models/LocalPersistentVolume.php b/app/Models/LocalPersistentVolume.php index 0ff3a1903f..cd251aa2d4 100644 --- a/app/Models/LocalPersistentVolume.php +++ b/app/Models/LocalPersistentVolume.php @@ -227,8 +227,9 @@ class LocalPersistentVolume extends BaseModel /** * The external Compose volume that this storage entry still replaces, or null. Before Coolify * used external volumes as written, the parsers gave them a generated name, for example - * "{uuid}_{volume}" or "{uuid}_{volume}-pr-{id}". While this storage entry exists, the parsers - * keep that name so that the resource keeps its data (see useComposeExternalVolumeAsWritten()). + * "{uuid}_{volume}". While this storage entry exists, the parsers keep that name so that the + * resource keeps its data (see useComposeExternalVolumeAsWritten()). A preview volume + * ("{uuid}_{volume}-pr-{id}") replaces nothing: a preview never uses the external volume. */ public function replacedExternalComposeVolume(): ?string { @@ -256,8 +257,7 @@ class LocalPersistentVolume extends BaseModel $resource instanceof Application => $resource->uuid.'_'.Str::slug($key, '-'), default => data_get($resource, 'service.uuid').'_'.Str::slug($key, '-'), }; - $isLegacyName = $legacyName !== $key && $this->name === $legacyName; - if ($isLegacyName || preg_match('/^'.preg_quote($legacyName, '/').'-pr-\d+$/', $this->name) === 1) { + if ($legacyName !== $key && $this->name === $legacyName) { return $key; } } diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 8947b3964b..85cb6e2bf1 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -342,8 +342,9 @@ function isComposeExternalVolume(mixed $declaration): bool /** * Returns the top-level declaration of a volume source when the Compose file declares it as * external, and null for all other sources. The parsers use an external volume as written: no - * "{uuid}_" prefix, no preview suffix and no LocalPersistentVolume row, so that Coolify never - * removes a volume that it does not own. + * "{uuid}_" prefix and no LocalPersistentVolume row, so that Coolify never removes a volume that it + * does not own. A preview deployment never uses an external volume; it gets its own volume + * "{uuid}_{volume}-pr-{id}" like any other named volume. * * @param iterable $topLevelVolumes * @return array|null @@ -364,9 +365,11 @@ function composeExternalVolumeDeclaration(iterable $topLevelVolumes, string $sou /** * Tells if a parser uses the volume source as written because it is an external volume. * + * Only for production deployments: a preview deployment always gets its own volume. + * * Before Coolify used external volumes as written, the parsers renamed them like all other - * volumes (the old name, for example "{uuid}_{volume}" or "{uuid}_{volume}-pr-{id}"), so the - * resource wrote its data into the renamed volume. When the owner still has the storage entry + * volumes (the old name, for example "{uuid}_{volume}"), so the resource wrote its data into the + * renamed volume. When the owner still has the storage entry * with the old name, the parser must keep the old name, or the resource loses its data. Then * this function records a warning on the resource and returns false. When the user deletes that * storage entry, the next parse uses the external volume. @@ -431,7 +434,8 @@ function legacyApplicationRenamedVolumeDeclaration(string $name): array * Records a warning when a legacy Compose application (parser version 1 or 2) does not use an * external volume as written. These parsers keep the old volume name (see * legacyApplicationComposeVolumeName()), so the resource keeps its data. The parser version 1 keeps - * the name of a production volume, so it uses the external volume and gets no warning. + * the name of a production volume, so it uses the external volume and gets no warning. A preview + * always uses its own volume, so it gets no warning either. * * The volume names are not validated here, so that the legacy parsers keep their old behavior. The * warning shows the Docker volume name only when it is a literal, valid Docker volume name. @@ -440,6 +444,9 @@ function legacyApplicationRenamedVolumeDeclaration(string $name): array */ function warnLegacyApplicationComposeExternalVolume(Application $resource, iterable $topLevelVolumes, string $source, int $pull_request_id): void { + if ($pull_request_id !== 0) { + return; + } $declaration = collect($topLevelVolumes)->get($source); if (! isComposeExternalVolume($declaration)) { return; @@ -1331,6 +1338,7 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } // Parse the rest of the services + $previewOwnExternalVolumes = collect([]); foreach ($services as $serviceName => $service) { $image = data_get_str($service, 'image'); $restart = data_get_str($service, 'restart', RESTART_MODE); @@ -1481,16 +1489,21 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } } } elseif ($type->value() === 'volume') { - $legacyName = "{$uuid}_".Str::slug($source, '-'); - if ($isPullRequest) { - $legacyName = addPreviewDeploymentSuffix($legacyName, $pull_request_id); - } - if (useComposeExternalVolumeAsWritten($resource, $originalResource, $topLevel->get('volumes'), $source->value(), $legacyName)) { - // Previews share the external volume. It gets no row, so Coolify never removes it. + // A preview never mounts an external volume: two deployments that write into the same + // data directory (for example two databases) can corrupt it. The preview gets its own + // volume with the name that older Coolify versions gave it, so it keeps its data, and + // the preview cleanup removes it. + $isPreviewOwnExternalVolume = $isPullRequest + && composeExternalVolumeDeclaration($topLevel->get('volumes'), $source->value()) !== null; + if (! $isPullRequest && useComposeExternalVolumeAsWritten($resource, $originalResource, $topLevel->get('volumes'), $source->value(), "{$uuid}_".Str::slug($source, '-'))) { + // The external volume gets no row, so Coolify never removes it. $volumesParsed->put($index, $volume); continue; } + if ($isPreviewOwnExternalVolume) { + $previewOwnExternalVolumes->put($source->value(), true); + } $declaration = $topLevel->get('volumes')->get($source->value()); if (in_array(data_get($declaration, 'driver_opts.type'), ['cifs', 'nfs'], true)) { // Network volumes are used as written. @@ -1529,7 +1542,9 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int 'resource_type' => get_class($originalResource), ] ); - $topLevel->get('volumes')->put($name, composeRenamedVolumeDeclarationFor($declaration, $name, $persistentVolume, $isPullRequest)); + $topLevel->get('volumes')->put($name, $isPreviewOwnExternalVolume + ? ['name' => $name] + : composeRenamedVolumeDeclarationFor($declaration, $name, $persistentVolume, $isPullRequest)); } dispatch(new ServerFilesFromServerJob($originalResource)); $volumesParsed->put($index, $volume); @@ -2134,6 +2149,8 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $parsedServices->put($serviceName, $payload); } + // The preview mounts its own volumes instead of these external volumes (see above), so it does not declare them. + $topLevel->put('volumes', $topLevel->get('volumes')->except($previewOwnExternalVolumes->keys()->all())); $topLevel->put('services', $parsedServices); $customOrder = ['services', 'volumes', 'networks', 'configs', 'secrets']; diff --git a/tests/Feature/ApplicationPreviewComposeCleanupTest.php b/tests/Feature/ApplicationPreviewComposeCleanupTest.php index 41c904d35c..dfb4f546cf 100644 --- a/tests/Feature/ApplicationPreviewComposeCleanupTest.php +++ b/tests/Feature/ApplicationPreviewComposeCleanupTest.php @@ -131,7 +131,8 @@ it('removes only the network and volumes that Coolify generated for the preview' expect($resourceCommands) ->toContain("sudo docker volume rm -f '{$this->uuid}_app-data-pr-42'") - ->not->toContain('shared-data-pr-42') + // A preview gets its own volume instead of the external one. + ->toContain("sudo docker volume rm -f '{$this->uuid}_shared-data-pr-42'") ->toContain("sudo docker network disconnect '{$this->previewNetwork}' coolify-proxy") ->toContain("sudo docker network rm '{$this->previewNetwork}'") ->not->toContain("'app-data'") diff --git a/tests/Feature/ComposeExternalVolumeTest.php b/tests/Feature/ComposeExternalVolumeTest.php index 9b97887a32..4577b9b2b5 100644 --- a/tests/Feature/ComposeExternalVolumeTest.php +++ b/tests/Feature/ComposeExternalVolumeTest.php @@ -304,18 +304,34 @@ describe('applicationParser', function () { ->and(persistentVolumeNames())->toBe([]); }); - it('shares the external volume with a preview deployment', function () { + it('gives a preview deployment its own volume instead of the external volume', function () { $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); $uuid = $application->uuid; $preview = externalVolumePreview($application); $compose = applicationParser($application, 42, $preview->id)->toArray(); - expect($compose['services']['web-pr-42']['volumes'])->toContain('shared-data:/data:ro') + expect($compose['services']['web-pr-42']['volumes'])->toContain("{$uuid}_shared-data-pr-42:/data:ro") ->toContain("{$uuid}_app-data-pr-42:/app") - ->and($compose['volumes']['shared-data'])->toBe(['external' => true]) - ->and($compose['volumes'])->not->toHaveKey("{$uuid}_shared-data-pr-42") - ->and(persistentVolumeNames())->toBe(["{$uuid}_app-data-pr-42"]); + ->not->toContain('shared-data:/data:ro') + ->and($compose['volumes']["{$uuid}_shared-data-pr-42"])->toBe(['name' => "{$uuid}_shared-data-pr-42"]) + ->and($compose['volumes'])->not->toHaveKey('shared-data') + ->and(persistentVolumeNames())->toBe(["{$uuid}_app-data-pr-42", "{$uuid}_shared-data-pr-42"]) + ->and($application->composeVolumeWarnings())->toBe([]); + }); + + it('gives a preview deployment its own volume instead of a named external volume in long syntax', function () { + $application = externalVolumeApplication(EXTERNAL_VOLUME_LONG_COMPOSE); + $uuid = $application->uuid; + $preview = externalVolumePreview($application); + + $compose = applicationParser($application, 42, $preview->id); + $sources = collect($compose->toArray()['services']['web-pr-42']['volumes'])->pluck('source')->all(); + + expect($sources)->toBe(["{$uuid}_shared-data-pr-42", "{$uuid}_app-data-pr-42"]) + ->and($compose->toArray()['volumes']["{$uuid}_shared-data-pr-42"])->toBe(['name' => "{$uuid}_shared-data-pr-42"]) + ->and(Yaml::dump(convertToArray($compose), 10, 2))->not->toContain('existing-shared-volume') + ->and($application->composeVolumeWarnings())->toBe([]); }); it('does not show a warning when the resource has no old storage entry', function () { @@ -354,21 +370,22 @@ describe('applicationParser', function () { ->and($application->composeVolumeWarnings())->toBe([legacyExternalVolumeWarning("{$uuid}_shared-data", 'existing-shared-volume')]); }); - it('keeps the old prefixed preview volume when the resource has its old storage entry', function () { + it('reuses the preview volume of an older Coolify version without a warning', function () { $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); $uuid = $application->uuid; $preview = externalVolumePreview($application); - legacyExternalVolumeRow($application, "{$uuid}_shared-data-pr-42"); + $oldVolume = legacyExternalVolumeRow($application, "{$uuid}_shared-data-pr-42"); $compose = applicationParser($application, 42, $preview->id)->toArray(); expect($compose['services']['web-pr-42']['volumes'])->toContain("{$uuid}_shared-data-pr-42:/data:ro") ->and($compose['volumes']["{$uuid}_shared-data-pr-42"])->toBe(['name' => "{$uuid}_shared-data-pr-42"]) + ->and(LocalPersistentVolume::find($oldVolume->id)?->name)->toBe("{$uuid}_shared-data-pr-42") ->and(persistentVolumeNames())->toBe(["{$uuid}_app-data-pr-42", "{$uuid}_shared-data-pr-42"]) - ->and($application->composeVolumeWarnings())->toBe([legacyExternalVolumeWarning("{$uuid}_shared-data-pr-42")]); + ->and($application->composeVolumeWarnings())->toBe([]); }); - it('uses the external volume in a preview when only the production volume has an old storage entry', function () { + it('gives a preview its own volume when only the production volume has an old storage entry', function () { $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); $uuid = $application->uuid; $preview = externalVolumePreview($application); @@ -376,7 +393,9 @@ describe('applicationParser', function () { $compose = applicationParser($application, 42, $preview->id)->toArray(); - expect($compose['services']['web-pr-42']['volumes'])->toContain('shared-data:/data:ro') + expect($compose['services']['web-pr-42']['volumes'])->toContain("{$uuid}_shared-data-pr-42:/data:ro") + ->not->toContain('shared-data:/data:ro') + ->not->toContain("{$uuid}_shared-data:/data:ro") ->and($application->composeVolumeWarnings())->toBe([]); }); @@ -532,7 +551,7 @@ describe('legacy parsers', function () { ->and($service->composeVolumeWarnings())->toBe([]); }); - it('keeps the old volume names of a legacy application and shows a warning when the name is not the external name', function (string $parsingVersion, int $pullRequestId, string $compose, string $expectedVolume, ?string $dockerVolume) { + it('keeps the old volume names of a legacy application and shows a warning when production does not use the external name', function (string $parsingVersion, int $pullRequestId, string $compose, string $expectedVolume, ?string $dockerVolume) { // Legacy Compose applications (parser versions 1 and 2) never stored their volumes, so Coolify // cannot tell which external volume already holds data: they always keep the old names. $application = externalVolumeApplication($compose, $parsingVersion); @@ -548,7 +567,8 @@ describe('legacy parsers', function () { ->and(array_keys($parsed['volumes'] ?? []))->toContain($expectedName) ->and($application->composeVolumeWarnings())->toBe( // Parser version 1 keeps the name of a production volume, so it uses the external volume. - $expectedName === 'shared-data' ? [] : [legacyParserExternalVolumeWarning($expectedName, $dockerVolume)] + // A preview always uses its own volume, so it gets no warning. + $pullRequestId !== 0 || $expectedName === 'shared-data' ? [] : [legacyParserExternalVolumeWarning($expectedName, $dockerVolume)] ); })->with([ 'v1 short' => ['1', 0, EXTERNAL_VOLUME_SHORT_COMPOSE, 'shared-data:/data:ro', null], @@ -572,8 +592,20 @@ describe('legacy parsers', function () { ->and($logEntries)->toBe([['Warning: '.legacyParserExternalVolumeWarning($keptName), 'stderr']]); })->with([ 'v2 production' => ['2', 0, '{uuid}-shared-data'], - 'v2 preview' => ['2', 42, '{uuid}-shared-data-pr-42'], - 'v1 preview' => ['1', 42, 'shared-data-pr-42'], + ]); + + it('writes no warning to the deployment log of a legacy application preview, which always uses its own volume', function (string $parsingVersion, string $keptName) { + $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE, $parsingVersion); + $keptName = str_replace('{uuid}', $application->uuid, $keptName); + $preview = externalVolumePreview($application); + + [$composeFile, $logEntries] = parseExternalVolumeDeployment($application, 42, $preview); + + expect(data_get($composeFile, 'services.web-pr-42.volumes'))->toContain("{$keptName}:/data:ro") + ->and($logEntries)->toBe([]); + })->with([ + 'v2 preview' => ['2', '{uuid}-shared-data-pr-42'], + 'v1 preview' => ['1', 'shared-data-pr-42'], ]); it('writes no warning to the deployment log for a production deployment of a parser version 1 application', function () { @@ -842,8 +874,8 @@ describe('delete', function () { expect(LocalPersistentVolume::find($volume->id))->not->toBeNull(); }); - it('does not remove the external volume when a preview is deleted', function () { - $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); + it('removes the own volume of the preview, but not the external volume, when a preview is deleted', function (string $compose, string $externalVolume) { + $application = externalVolumeApplication($compose); $uuid = $application->uuid; $preview = externalVolumePreview($application); $commands = []; @@ -853,7 +885,23 @@ describe('delete', function () { $volumeCommands = collect($commands)->filter(fn (string $command): bool => str_contains($command, 'docker volume'))->implode("\n"); expect($volumeCommands)->toContain("docker volume rm -f '{$uuid}_app-data-pr-42'") - ->not->toContain('shared-data'); + ->toContain("docker volume rm -f '{$uuid}_shared-data-pr-42'") + ->not->toContain("'{$externalVolume}'"); + })->with([ + 'short syntax' => [EXTERNAL_VOLUME_SHORT_COMPOSE, 'shared-data'], + 'named external volume' => [EXTERNAL_VOLUME_LONG_COMPOSE, 'existing-shared-volume'], + ]); + + it('does not mark the own volume of a preview as an entry that replaces the external volume', function () { + $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); + $uuid = $application->uuid; + $preview = externalVolumePreview($application); + $productionEntry = legacyExternalVolumeRow($application, "{$uuid}_shared-data"); + applicationParser($application, 42, $preview->id); + $previewEntry = LocalPersistentVolume::query()->where('name', "{$uuid}_shared-data-pr-42")->firstOrFail(); + + expect($previewEntry->replacedExternalComposeVolume())->toBeNull() + ->and($productionEntry->replacedExternalComposeVolume())->toBe('shared-data'); }); it('removes the old prefixed preview volume, but not the external volume, when a preview is deleted', function () { @@ -886,9 +934,20 @@ describe('warnings', function () { ->and($logEntries)->toBe([['Warning: '.legacyExternalVolumeWarning(str_replace('{uuid}', $uuid, $oldName)), 'stderr']]); })->with([ 'production' => [0, '{uuid}_shared-data', '{uuid}_shared-data:/data:ro'], - 'preview' => [42, '{uuid}_shared-data-pr-42', '{uuid}_shared-data-pr-42:/data:ro'], ]); + it('writes no warning to the deployment log of a preview, which always uses its own volume', function () { + $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); + $uuid = $application->uuid; + $preview = externalVolumePreview($application); + legacyExternalVolumeRow($application, "{$uuid}_shared-data-pr-42"); + + [$composeFile, $logEntries] = parseExternalVolumeDeployment($application, 42, $preview); + + expect(data_get($composeFile, 'services.web-pr-42.volumes'))->toContain("{$uuid}_shared-data-pr-42:/data:ro") + ->and($logEntries)->toBe([]); + }); + it('writes no warning to the deployment log when the external volume is used as written', function () { $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); From 152e06cf43efee6db196bab34245d8b8bac96379 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:25:57 +0200 Subject: [PATCH 25/25] fix(databases): let REDIS_PASSWORD override the KeyDB and Dragonfly password The stored password is the default. Before, a local REDIS_PASSWORD was rewritten to the stored password, and a shared one replaced the stored password, so the old password was lost. - A local, shared, or remote secret REDIS_PASSWORD sets the server password; the stored password and the variable are not changed - Removing the variable, or an empty value, uses the stored password - Connection URLs show the password in use; for a remote secret they show the reference, not the secret - Databases created before this release keep the v4.3.23 behavior --- app/Actions/Database/StartDragonfly.php | 18 +-- app/Actions/Database/StartKeydb.php | 18 +-- app/Models/StandaloneDragonfly.php | 45 +++++++- app/Models/StandaloneKeydb.php | 45 +++++++- ...eydbDragonflyLegacyPasswordQuotingTest.php | 104 ++++++++++++++++-- 5 files changed, 195 insertions(+), 35 deletions(-) diff --git a/app/Actions/Database/StartDragonfly.php b/app/Actions/Database/StartDragonfly.php index 8327cf2dfe..58d1607aa8 100644 --- a/app/Actions/Database/StartDragonfly.php +++ b/app/Actions/Database/StartDragonfly.php @@ -250,22 +250,16 @@ class StartDragonfly { $environment_variables = collect(); $this->resolvedRedisPassword = (string) $this->database->dragonfly_password; + $passwordVariable = $this->database->serverPasswordEnvironmentVariable(); foreach ($this->database->runtime_environment_variables as $env) { - $usesSecretManager = $this->database->environmentVariableUsesSecretManager($env); - if ($env->key === 'REDIS_PASSWORD' && ! $env->is_shared && ! $usesSecretManager) { - $env->update(['value' => $this->database->dragonfly_password]); - } $rawValue = (string) $this->database->resolveSecretManagerEnvironmentVariableValue($env); $resolvedValue = (string) $this->database->formatEnvironmentVariableValue($env, $rawValue); - // Credentials below are placed directly in the compose file (healthcheck, command). - $composeFileValue = $this->database->formatComposeFileValue($env, $rawValue); $environment_variables->push($env->key.'='.$resolvedValue); - if ($env->key === 'REDIS_PASSWORD') { - if ($env->is_shared && ! $usesSecretManager) { - $this->database->update(['dragonfly_password' => $rawValue]); - } - $this->resolvedRedisPassword = $composeFileValue; - $this->redisPasswordFromSecretManager = $usesSecretManager; + // A REDIS_PASSWORD variable overrides the stored password, which stays unchanged. + if ($passwordVariable?->is($env) && $rawValue !== '') { + // Credentials below are placed directly in the compose file (healthcheck, command). + $this->resolvedRedisPassword = $this->database->formatComposeFileValue($env, $rawValue); + $this->redisPasswordFromSecretManager = $this->database->environmentVariableUsesSecretManager($env); } } diff --git a/app/Actions/Database/StartKeydb.php b/app/Actions/Database/StartKeydb.php index 6bd3c4b236..770d055001 100644 --- a/app/Actions/Database/StartKeydb.php +++ b/app/Actions/Database/StartKeydb.php @@ -250,22 +250,16 @@ class StartKeydb { $environment_variables = collect(); $this->resolvedRedisPassword = (string) $this->database->keydb_password; + $passwordVariable = $this->database->serverPasswordEnvironmentVariable(); foreach ($this->database->runtime_environment_variables as $env) { - $usesSecretManager = $this->database->environmentVariableUsesSecretManager($env); - if ($env->key === 'REDIS_PASSWORD' && ! $env->is_shared && ! $usesSecretManager) { - $env->update(['value' => $this->database->keydb_password]); - } $rawValue = (string) $this->database->resolveSecretManagerEnvironmentVariableValue($env); $resolvedValue = (string) $this->database->formatEnvironmentVariableValue($env, $rawValue); - // Credentials below are placed directly in the compose file (healthcheck, command). - $composeFileValue = $this->database->formatComposeFileValue($env, $rawValue); $environment_variables->push($env->key.'='.$resolvedValue); - if ($env->key === 'REDIS_PASSWORD') { - if ($env->is_shared && ! $usesSecretManager) { - $this->database->update(['keydb_password' => $rawValue]); - } - $this->resolvedRedisPassword = $composeFileValue; - $this->redisPasswordFromSecretManager = $usesSecretManager; + // A REDIS_PASSWORD variable overrides the stored password, which stays unchanged. + if ($passwordVariable?->is($env) && $rawValue !== '') { + // Credentials below are placed directly in the compose file (healthcheck, command). + $this->resolvedRedisPassword = $this->database->formatComposeFileValue($env, $rawValue); + $this->redisPasswordFromSecretManager = $this->database->environmentVariableUsesSecretManager($env); } } diff --git a/app/Models/StandaloneDragonfly.php b/app/Models/StandaloneDragonfly.php index e3d70534dc..15dea750f8 100644 --- a/app/Models/StandaloneDragonfly.php +++ b/app/Models/StandaloneDragonfly.php @@ -312,7 +312,7 @@ class StandaloneDragonfly extends BaseModel get: function () { $scheme = $this->enable_ssl ? 'rediss' : 'redis'; $port = $this->enable_ssl ? 6380 : 6379; - $encodedPass = rawurlencode($this->dragonfly_password); + $encodedPass = rawurlencode($this->connectionPassword()); $url = "{$scheme}://:{$encodedPass}@{$this->uuid}:{$port}/0"; if ($this->enable_ssl && $this->ssl_mode === 'verify-ca') { @@ -334,7 +334,7 @@ class StandaloneDragonfly extends BaseModel return null; } $scheme = $this->enable_ssl ? 'rediss' : 'redis'; - $encodedPass = rawurlencode($this->dragonfly_password); + $encodedPass = rawurlencode($this->connectionPassword()); $url = "{$scheme}://:{$encodedPass}@{$serverIp}:{$this->public_port}/0"; if ($this->enable_ssl && $this->ssl_mode === 'verify-ca') { @@ -369,6 +369,47 @@ class StandaloneDragonfly extends BaseModel return $this->morphMany(EnvironmentVariable::class, 'resourceable'); } + /** + * The REDIS_PASSWORD variable that sets the server password instead of the stored password. + * Databases created before this release (legacy_password_quoting) keep their v4.3.23 server + * password, the stored one, unless the variable reads a remote secret. + */ + public function serverPasswordEnvironmentVariable(): ?EnvironmentVariable + { + $environmentVariable = $this->runtime_environment_variables()->where('key', 'REDIS_PASSWORD')->first(); + + if (! $environmentVariable) { + return null; + } + + if ($this->legacy_password_quoting && ! $this->environmentVariableUsesSecretManager($environmentVariable)) { + return null; + } + + return $environmentVariable; + } + + /** + * The server password for connection URLs. A remote secret is not fetched here, so the URL + * shows its reference instead of a stale or wrong password. + */ + public function connectionPassword(): string + { + $environmentVariable = $this->serverPasswordEnvironmentVariable(); + + if ($environmentVariable) { + $password = $this->environmentVariableUsesSecretManager($environmentVariable) + ? (string) $environmentVariable->value + : (string) $this->resolveSecretManagerEnvironmentVariableValue($environmentVariable); + + if ($password !== '') { + return $password; + } + } + + return (string) $this->dragonfly_password; + } + public function persistentStorages() { return $this->morphMany(LocalPersistentVolume::class, 'resource'); diff --git a/app/Models/StandaloneKeydb.php b/app/Models/StandaloneKeydb.php index 5961ead8ce..35818a8742 100644 --- a/app/Models/StandaloneKeydb.php +++ b/app/Models/StandaloneKeydb.php @@ -313,7 +313,7 @@ class StandaloneKeydb extends BaseModel get: function () { $scheme = $this->enable_ssl ? 'rediss' : 'redis'; $port = $this->enable_ssl ? 6380 : 6379; - $encodedPass = rawurlencode($this->keydb_password); + $encodedPass = rawurlencode($this->connectionPassword()); $url = "{$scheme}://:{$encodedPass}@{$this->uuid}:{$port}/0"; if ($this->enable_ssl && $this->ssl_mode === 'verify-ca') { @@ -335,7 +335,7 @@ class StandaloneKeydb extends BaseModel return null; } $scheme = $this->enable_ssl ? 'rediss' : 'redis'; - $encodedPass = rawurlencode($this->keydb_password); + $encodedPass = rawurlencode($this->connectionPassword()); $url = "{$scheme}://:{$encodedPass}@{$serverIp}:{$this->public_port}/0"; if ($this->enable_ssl && $this->ssl_mode === 'verify-ca') { @@ -370,6 +370,47 @@ class StandaloneKeydb extends BaseModel return $this->morphMany(EnvironmentVariable::class, 'resourceable'); } + /** + * The REDIS_PASSWORD variable that sets the server password instead of the stored password. + * Databases created before this release (legacy_password_quoting) keep their v4.3.23 server + * password, the stored one, unless the variable reads a remote secret. + */ + public function serverPasswordEnvironmentVariable(): ?EnvironmentVariable + { + $environmentVariable = $this->runtime_environment_variables()->where('key', 'REDIS_PASSWORD')->first(); + + if (! $environmentVariable) { + return null; + } + + if ($this->legacy_password_quoting && ! $this->environmentVariableUsesSecretManager($environmentVariable)) { + return null; + } + + return $environmentVariable; + } + + /** + * The server password for connection URLs. A remote secret is not fetched here, so the URL + * shows its reference instead of a stale or wrong password. + */ + public function connectionPassword(): string + { + $environmentVariable = $this->serverPasswordEnvironmentVariable(); + + if ($environmentVariable) { + $password = $this->environmentVariableUsesSecretManager($environmentVariable) + ? (string) $environmentVariable->value + : (string) $this->resolveSecretManagerEnvironmentVariableValue($environmentVariable); + + if ($password !== '') { + return $password; + } + } + + return (string) $this->keydb_password; + } + public function persistentStorages() { return $this->morphMany(LocalPersistentVolume::class, 'resource'); diff --git a/tests/Feature/KeydbDragonflyLegacyPasswordQuotingTest.php b/tests/Feature/KeydbDragonflyLegacyPasswordQuotingTest.php index 703a52f306..883ae97d84 100644 --- a/tests/Feature/KeydbDragonflyLegacyPasswordQuotingTest.php +++ b/tests/Feature/KeydbDragonflyLegacyPasswordQuotingTest.php @@ -197,30 +197,58 @@ it('quotes shell control characters in the password of a database created after 'pipe' => 'x|y', ]); -it('runs with the stored password and syncs a plain REDIS_PASSWORD variable to it', function (string $create, string $start, string $passwordColumn, string $commandFormat) { +it('runs with a plain REDIS_PASSWORD variable instead of the stored password without changing either', function (string $create, string $start, string $passwordColumn, string $commandFormat) { $database = $create($this->environment->id, $this->destination, [$passwordColumn => 'storedpass']); $env = $database->runtime_environment_variables()->create(['key' => 'REDIS_PASSWORD', 'value' => 'envpass']); $start::run($database->fresh(), new Activity); + $service = legacyPasswordQuotingStartedService($this->executor, $database->uuid); + expect($service['command'])->toBe(sprintf($commandFormat, "'envpass'")) + ->and($service['healthcheck'][3])->toBe('envpass') + ->and($service['environment'])->toContain('REDIS_PASSWORD=envpass') + ->and($env->fresh()->value)->toBe('envpass') + ->and($database->fresh()->{$passwordColumn})->toBe('storedpass'); +})->with('legacy-password-quoting-engines'); + +it('runs with the stored password again after the REDIS_PASSWORD variable is removed', function (string $create, string $start, string $passwordColumn, string $commandFormat) { + $database = $create($this->environment->id, $this->destination, [$passwordColumn => 'storedpass']); + $env = $database->runtime_environment_variables()->create(['key' => 'REDIS_PASSWORD', 'value' => 'envpass']); + $start::run($database->fresh(), new Activity); + + $env->delete(); + $start::run($database->fresh(), new Activity); + $service = legacyPasswordQuotingStartedService($this->executor, $database->uuid); expect($service['command'])->toBe(sprintf($commandFormat, "'storedpass'")) ->and($service['healthcheck'][3])->toBe('storedpass') - ->and($service['environment'])->toContain('REDIS_PASSWORD=storedpass') - ->and($env->fresh()->value)->toBe('storedpass') - ->and($database->fresh()->{$passwordColumn})->toBe('storedpass'); + ->and($service['environment'])->toContain('REDIS_PASSWORD=storedpass'); +})->with('legacy-password-quoting-engines'); + +it('runs with the stored password when the REDIS_PASSWORD variable is empty', function (string $create, string $start, string $passwordColumn, string $commandFormat) { + $database = $create($this->environment->id, $this->destination, [$passwordColumn => 'storedpass']); + $database->runtime_environment_variables()->create(['key' => 'REDIS_PASSWORD', 'value' => '']); + + $start::run($database->fresh(), new Activity); + + $service = legacyPasswordQuotingStartedService($this->executor, $database->uuid); + expect($service['command'])->toBe(sprintf($commandFormat, "'storedpass'")) + ->and($service['healthcheck'][3])->toBe('storedpass'); })->with('legacy-password-quoting-engines'); it('keeps the exact v4.3.23 command of a legacy database with a different plain REDIS_PASSWORD variable', function (string $create, string $start, string $passwordColumn, string $commandFormat) { $database = $create($this->environment->id, $this->destination, [$passwordColumn => 'stored;pass']); DB::table($database->getTable())->where('id', $database->id)->update(['legacy_password_quoting' => true]); - $database->runtime_environment_variables()->create(['key' => 'REDIS_PASSWORD', 'value' => 'envpass']); + $env = $database->runtime_environment_variables()->create(['key' => 'REDIS_PASSWORD', 'value' => 'envpass']); $start::run($database->fresh(), new Activity); $service = legacyPasswordQuotingStartedService($this->executor, $database->uuid); expect($service['command'])->toBe(sprintf($commandFormat, 'stored;pass')) ->and($service['healthcheck'][3])->toBe('stored;pass') + ->and($service['environment'])->toContain('REDIS_PASSWORD=envpass') + ->and($env->fresh()->value)->toBe('envpass') + ->and($database->fresh()->{$passwordColumn})->toBe('stored;pass') ->and($database->fresh()->legacy_password_quoting)->toBeTrue(); })->with('legacy-password-quoting-engines'); @@ -246,7 +274,7 @@ it('runs with a remote secret REDIS_PASSWORD without storing it', function (stri ->and($database->fresh()->{$passwordColumn})->toBe('storedpass'); })->with('legacy-password-quoting-engines'); -it('copies a shared REDIS_PASSWORD variable into the stored password', function (string $create, string $start, string $passwordColumn, string $commandFormat) { +it('runs with a shared REDIS_PASSWORD variable without storing it', function (string $create, string $start, string $passwordColumn, string $commandFormat) { SharedEnvironmentVariable::query()->create([ 'key' => 'REDIS_PASSWORD', 'value' => 'sharedpass', @@ -262,5 +290,67 @@ it('copies a shared REDIS_PASSWORD variable into the stored password', function expect($service['command'])->toBe(sprintf($commandFormat, "'sharedpass'")) ->and($service['healthcheck'][3])->toBe('sharedpass') ->and($env->fresh()->value)->toBe('{{team.REDIS_PASSWORD}}') - ->and($database->fresh()->{$passwordColumn})->toBe('sharedpass'); + ->and($database->fresh()->{$passwordColumn})->toBe('storedpass'); +})->with('legacy-password-quoting-engines'); + +it('keeps the stored password of a legacy database with a shared REDIS_PASSWORD variable', function (string $create, string $start, string $passwordColumn, string $commandFormat) { + SharedEnvironmentVariable::query()->create([ + 'key' => 'REDIS_PASSWORD', + 'value' => 'sharedpass', + 'type' => 'team', + 'team_id' => $this->environment->project->team_id, + ]); + $database = $create($this->environment->id, $this->destination, [$passwordColumn => 'stored;pass']); + DB::table($database->getTable())->where('id', $database->id)->update(['legacy_password_quoting' => true]); + $database->runtime_environment_variables()->create(['key' => 'REDIS_PASSWORD', 'value' => '{{team.REDIS_PASSWORD}}']); + + $start::run($database->fresh(), new Activity); + + $service = legacyPasswordQuotingStartedService($this->executor, $database->uuid); + expect($service['command'])->toBe(sprintf($commandFormat, 'stored;pass')) + ->and($service['healthcheck'][3])->toBe('stored;pass') + ->and($database->fresh()->{$passwordColumn})->toBe('stored;pass') + ->and($database->fresh()->legacy_password_quoting)->toBeTrue(); +})->with('legacy-password-quoting-engines'); + +it('shows the password the server uses in the connection URLs', function (string $create, string $start, string $passwordColumn, string $commandFormat) { + SharedEnvironmentVariable::query()->create([ + 'key' => 'REDIS_PASSWORD', + 'value' => 'shared/pass', + 'type' => 'team', + 'team_id' => $this->environment->project->team_id, + ]); + $database = $create($this->environment->id, $this->destination, [$passwordColumn => 'stored/pass', 'is_public' => true, 'public_port' => 16379]); + expect($database->fresh()->internal_db_url)->toContain(':stored%2Fpass@') + ->and($database->fresh()->external_db_url)->toContain(':stored%2Fpass@'); + + $env = $database->runtime_environment_variables()->create(['key' => 'REDIS_PASSWORD', 'value' => 'env/pass']); + expect($database->fresh()->internal_db_url)->toContain(':env%2Fpass@') + ->and($database->fresh()->external_db_url)->toContain(':env%2Fpass@'); + + $env->update(['value' => '{{team.REDIS_PASSWORD}}']); + expect($database->fresh()->internal_db_url)->toContain(':shared%2Fpass@') + ->and($database->fresh()->external_db_url)->toContain(':shared%2Fpass@'); + + DB::table($database->getTable())->where('id', $database->id)->update(['legacy_password_quoting' => true]); + expect($database->fresh()->internal_db_url)->toContain(':stored%2Fpass@') + ->and($database->fresh()->external_db_url)->toContain(':stored%2Fpass@'); +})->with('legacy-password-quoting-engines'); + +it('shows the unresolved remote secret reference in the connection URL without fetching it', function (string $create, string $start, string $passwordColumn, string $commandFormat) { + Http::fake(); + $token = IntegrationToken::query()->create([ + 'team_id' => $this->environment->project->team_id, + 'provider' => 'doppler', + 'name' => 'Doppler', + 'token' => 'the-secret-token', + 'capabilities' => ['secrets'], + ]); + $database = $create($this->environment->id, $this->destination, [$passwordColumn => 'storedpass']); + $database->secretManagerLink()->create(['integration_token_id' => $token->id]); + $database->runtime_environment_variables()->create(['key' => 'REDIS_PASSWORD', 'value' => '{{vault.REDIS_PASSWORD}}']); + + expect($database->fresh()->internal_db_url)->toContain(':'.rawurlencode('{{vault.REDIS_PASSWORD}}').'@') + ->not->toContain('storedpass'); + Http::assertNothingSent(); })->with('legacy-password-quoting-engines');