From eaabed5c4b18ba51d70882c0fcd2bf3b28c94058 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 27 Oct 2026 22:36:25 +0100 Subject: [PATCH 1/8] fix: scope finish events of background tasks to their team (#12355) Co-authored-by: Claude Opus 5.5 --- app/Actions/Proxy/StartProxy.php | 4 +- app/Actions/Proxy/StopProxy.php | 2 +- app/Actions/Server/ConfigureCloudflared.php | 1 + .../Service/DeployServiceApplication.php | 3 +- app/Actions/Service/StartService.php | 3 +- app/Events/ProxyStatusChanged.php | 12 +++ app/Events/ServiceStartFinished.php | 14 +++- app/Jobs/RestartProxyJob.php | 3 +- .../CloudflareTunnelChangedNotification.php | 11 ++- .../ProxyStatusChangedNotification.php | 7 +- .../RefreshServiceStatusAfterStart.php | 5 +- app/Livewire/ActivityMonitor.php | 23 ++++-- .../Feature/ActivityMonitorCrossTeamTest.php | 60 +++++++++++++++ .../ProxyStatusChangedNotificationTest.php | 75 +++++++++++++++++++ .../PushServerUpdateJobOptimizationTest.php | 2 +- tests/Feature/ResourceStatusRefresherTest.php | 22 +++++- ...loudflareTunnelChangedNotificationTest.php | 17 +++++ 17 files changed, 241 insertions(+), 23 deletions(-) create mode 100644 tests/Feature/Proxy/ProxyStatusChangedNotificationTest.php diff --git a/app/Actions/Proxy/StartProxy.php b/app/Actions/Proxy/StartProxy.php index 641e47469f..99992c0cae 100644 --- a/app/Actions/Proxy/StartProxy.php +++ b/app/Actions/Proxy/StartProxy.php @@ -48,13 +48,13 @@ class StartProxy if ($async) { $server->markProxyTransitionStarted(); - return remote_process($commands, $server, callEventOnFinish: 'ProxyStatusChanged', callEventData: $server->id, queue: deployment_queue()); + return remote_process($commands, $server, callEventOnFinish: 'ProxyStatusChanged', callEventData: ProxyStatusChanged::dataFor($server), queue: deployment_queue()); } else { instant_remote_process($commands, $server); $server->proxy->set('type', $proxyType); $server->save(); - ProxyStatusChanged::dispatch($server->id); + ProxyStatusChanged::dispatch(ProxyStatusChanged::dataFor($server)); return 'OK'; } diff --git a/app/Actions/Proxy/StopProxy.php b/app/Actions/Proxy/StopProxy.php index 881aa0252f..0fa045d84b 100644 --- a/app/Actions/Proxy/StopProxy.php +++ b/app/Actions/Proxy/StopProxy.php @@ -51,7 +51,7 @@ class StopProxy ProxyDashboardCacheService::clearCache($server); if (! $restarting) { - ProxyStatusChanged::dispatch($server->id); + ProxyStatusChanged::dispatch(ProxyStatusChanged::dataFor($server)); } } } diff --git a/app/Actions/Server/ConfigureCloudflared.php b/app/Actions/Server/ConfigureCloudflared.php index 0f7786d7cf..f0d5e6266f 100644 --- a/app/Actions/Server/ConfigureCloudflared.php +++ b/app/Actions/Server/ConfigureCloudflared.php @@ -54,6 +54,7 @@ class ConfigureCloudflared return remote_process($commands, $server, callEventOnFinish: 'CloudflareTunnelChanged', callEventData: [ 'server_id' => $server->id, + 'team_id' => $server->team_id, 'ssh_domain' => $ssh_domain, ]); } catch (\Throwable $e) { diff --git a/app/Actions/Service/DeployServiceApplication.php b/app/Actions/Service/DeployServiceApplication.php index abf66941a6..c5370eb51f 100644 --- a/app/Actions/Service/DeployServiceApplication.php +++ b/app/Actions/Service/DeployServiceApplication.php @@ -3,6 +3,7 @@ namespace App\Actions\Service; use App\Actions\Shared\EnsureContentFilesOnServer; +use App\Events\ServiceStartFinished; use App\Models\ServiceApplication; use App\Models\ServiceDatabase; use Lorisleiva\Actions\Concerns\AsAction; @@ -72,6 +73,6 @@ class DeployServiceApplication $commands->push("docker network connect {$safeProjectName} coolify-proxy >/dev/null 2>&1 || true"); - return remote_process($commands->toArray(), $service->server, type_uuid: $service->uuid, callEventOnFinish: 'ServiceStartFinished', callEventData: $service->id, queue: deployment_queue()); + return remote_process($commands->toArray(), $service->server, type_uuid: $service->uuid, callEventOnFinish: 'ServiceStartFinished', callEventData: ServiceStartFinished::dataFor($service), queue: deployment_queue()); } } diff --git a/app/Actions/Service/StartService.php b/app/Actions/Service/StartService.php index c9f0d6e079..fa4d517487 100644 --- a/app/Actions/Service/StartService.php +++ b/app/Actions/Service/StartService.php @@ -3,6 +3,7 @@ namespace App\Actions\Service; use App\Actions\Shared\EnsureContentFilesOnServer; +use App\Events\ServiceStartFinished; use App\Jobs\ServerFilesFromServerJob; use App\Models\LocalFileVolume; use App\Models\Service; @@ -84,7 +85,7 @@ class StartService } } - return remote_process($commands, $service->server, type_uuid: $service->uuid, callEventOnFinish: 'ServiceStartFinished', callEventData: $service->id, queue: deployment_queue()); + return remote_process($commands, $service->server, type_uuid: $service->uuid, callEventOnFinish: 'ServiceStartFinished', callEventData: ServiceStartFinished::dataFor($service), queue: deployment_queue()); } /** diff --git a/app/Events/ProxyStatusChanged.php b/app/Events/ProxyStatusChanged.php index 0cb775eb35..7dc455b935 100644 --- a/app/Events/ProxyStatusChanged.php +++ b/app/Events/ProxyStatusChanged.php @@ -2,6 +2,7 @@ namespace App\Events; +use App\Models\Server; use Illuminate\Broadcasting\InteractsWithSockets; use Illuminate\Foundation\Events\Dispatchable; use Illuminate\Queue\SerializesModels; @@ -10,5 +11,16 @@ class ProxyStatusChanged { use Dispatchable, InteractsWithSockets, SerializesModels; + /** + * @param array{server_id: int, team_id: int}|mixed $data Build it with dataFor(); the listener ignores any other shape. + */ public function __construct(public $data) {} + + /** + * @return array{server_id: int, team_id: int} + */ + public static function dataFor(Server $server): array + { + return ['server_id' => $server->id, 'team_id' => $server->team_id]; + } } diff --git a/app/Events/ServiceStartFinished.php b/app/Events/ServiceStartFinished.php index 3496a490ae..3ddde6b1a8 100644 --- a/app/Events/ServiceStartFinished.php +++ b/app/Events/ServiceStartFinished.php @@ -2,6 +2,7 @@ namespace App\Events; +use App\Models\Service; use Illuminate\Foundation\Events\Dispatchable; /** @@ -12,5 +13,16 @@ class ServiceStartFinished { use Dispatchable; - public function __construct(public int $serviceId) {} + /** + * @param array{service_id: int, team_id: int} $data Build it with dataFor(); the listener ignores a service of another team. + */ + public function __construct(public array $data) {} + + /** + * @return array{service_id: int, team_id: int} + */ + public static function dataFor(Service $service): array + { + return ['service_id' => $service->id, 'team_id' => $service->team()?->id]; + } } diff --git a/app/Jobs/RestartProxyJob.php b/app/Jobs/RestartProxyJob.php index 5008ff4ce9..45672fec70 100644 --- a/app/Jobs/RestartProxyJob.php +++ b/app/Jobs/RestartProxyJob.php @@ -5,6 +5,7 @@ namespace App\Jobs; use App\Actions\Proxy\GetProxyConfiguration; use App\Actions\Proxy\SaveProxyConfiguration; use App\Actions\Proxy\StartProxy; +use App\Events\ProxyStatusChanged; use App\Events\ProxyStatusChangedUI; use App\Models\Server; use App\Services\ProxyDashboardCacheService; @@ -62,7 +63,7 @@ class RestartProxyJob implements ShouldBeEncrypted, ShouldQueue $commands, $this->server, callEventOnFinish: 'ProxyStatusChanged', - callEventData: $this->server->id, + callEventData: ProxyStatusChanged::dataFor($this->server), queue: deployment_queue(), ); diff --git a/app/Listeners/CloudflareTunnelChangedNotification.php b/app/Listeners/CloudflareTunnelChangedNotification.php index c1804f0955..81c1f46ce1 100644 --- a/app/Listeners/CloudflareTunnelChangedNotification.php +++ b/app/Listeners/CloudflareTunnelChangedNotification.php @@ -16,13 +16,18 @@ class CloudflareTunnelChangedNotification public function handle(CloudflareTunnelChanged $event): void { $server_id = data_get($event, 'data.server_id'); + $team_id = data_get($event, 'data.team_id'); $ssh_domain = data_get($event, 'data.ssh_domain'); - - $this->server = Server::where('id', $server_id)->firstOrFail(); - if ($this->server->isTransferredAway()) { + if (is_null($server_id) || is_null($team_id)) { return; } + $server = Server::where('id', $server_id)->where('team_id', $team_id)->first(); + if (is_null($server) || $server->isTransferredAway()) { + return; + } + $this->server = $server; + // Check if cloudflare tunnel is running (container is healthy) - try 3 times with 5 second intervals $cloudflareHealthy = false; $attempts = 3; diff --git a/app/Listeners/ProxyStatusChangedNotification.php b/app/Listeners/ProxyStatusChangedNotification.php index 73b43d551c..7752e96519 100644 --- a/app/Listeners/ProxyStatusChangedNotification.php +++ b/app/Listeners/ProxyStatusChangedNotification.php @@ -16,11 +16,12 @@ class ProxyStatusChangedNotification implements ShouldQueueAfterCommit public function handle(ProxyStatusChanged $event) { - $serverId = $event->data; - if (is_null($serverId)) { + $serverId = data_get($event->data, 'server_id'); + $teamId = data_get($event->data, 'team_id'); + if (is_null($serverId) || is_null($teamId)) { return; } - $server = Server::where('id', $serverId)->first(); + $server = Server::where('id', $serverId)->where('team_id', $teamId)->first(); if (is_null($server)) { return; } diff --git a/app/Listeners/RefreshServiceStatusAfterStart.php b/app/Listeners/RefreshServiceStatusAfterStart.php index 094d9b3425..aa89034ce3 100644 --- a/app/Listeners/RefreshServiceStatusAfterStart.php +++ b/app/Listeners/RefreshServiceStatusAfterStart.php @@ -19,8 +19,9 @@ class RefreshServiceStatusAfterStart public function handle(ServiceStartFinished $event): void { - $service = Service::query()->find($event->serviceId); - if (! $service) { + $service = Service::query()->find($event->data['service_id'] ?? null); + $teamId = $event->data['team_id'] ?? null; + if (! $service || is_null($teamId) || $service->team()?->id !== $teamId) { return; } diff --git a/app/Livewire/ActivityMonitor.php b/app/Livewire/ActivityMonitor.php index 12095942c0..cabf9bd320 100644 --- a/app/Livewire/ActivityMonitor.php +++ b/app/Livewire/ActivityMonitor.php @@ -2,6 +2,7 @@ namespace App\Livewire; +use App\Events\ServiceStatusChanged; use App\Models\Server; use App\Models\Service; use App\Models\User; @@ -12,13 +13,27 @@ use Spatie\Activitylog\Models\Activity; class ActivityMonitor extends Component { + /** + * Events that may be dispatched when an activity finishes. The event name comes from the + * client, so application events must be listed here explicitly. + */ + private const ALLOWED_EVENTS = [ + 'activityFinished', + 'init', + 'prerequisitesInstalled', + 'sysboxInstalled', + ServiceStatusChanged::class, + ]; + public ?string $header = null; #[Locked] public $activityId = null; + #[Locked] public $eventToDispatch = 'activityFinished'; + #[Locked] public $eventData = null; public $isPollingActive = false; @@ -42,7 +57,7 @@ class ActivityMonitor extends Component self::$eventDispatched = false; $this->activityId = $activityId; - $this->eventToDispatch = $eventToDispatch; + $this->eventToDispatch = in_array($eventToDispatch, self::ALLOWED_EVENTS, true) ? $eventToDispatch : null; $this->eventData = $eventData; // Update header if provided @@ -130,11 +145,7 @@ class ActivityMonitor extends Component ?? $user->currentTeam()?->id ?? $user->teams->first()?->id; if ($teamId && ! self::$eventDispatched) { - if (filled($this->eventData)) { - $this->eventToDispatch::dispatch($teamId, $this->eventData); - } else { - $this->eventToDispatch::dispatch($teamId); - } + $this->eventToDispatch::dispatch($teamId); self::$eventDispatched = true; } } diff --git a/tests/Feature/ActivityMonitorCrossTeamTest.php b/tests/Feature/ActivityMonitorCrossTeamTest.php index 49d0b33491..54aefa1483 100644 --- a/tests/Feature/ActivityMonitorCrossTeamTest.php +++ b/tests/Feature/ActivityMonitorCrossTeamTest.php @@ -1,5 +1,8 @@ [0, 'finished'], 'failed' => [1, 'failed'], ]); + +describe('event dispatched when an activity finishes', function () { + beforeEach(function () { + $this->server = Server::factory()->create(['team_id' => $this->team->id]); + $this->activity = Activity::create([ + 'log_name' => 'default', + 'description' => 'finished activity', + 'causer_id' => $this->user->id, + 'causer_type' => User::class, + 'properties' => ['team_id' => $this->team->id, 'server_uuid' => $this->server->uuid, 'exitCode' => 0], + ]); + ActivityMonitor::$eventDispatched = false; + + $this->actingAs($this->user); + session(['currentTeam' => ['id' => $this->team->id]]); + }); + + test('the event properties are locked', function (string $property) { + Livewire::test(ActivityMonitor::class)->set($property, ProxyStatusChanged::class); + })->with(['eventToDispatch', 'eventData'])->throws(CannotUpdateLockedPropertyException::class); + + test('an event that is not allowed is not dispatched', function (string $event) { + Event::fake([ProxyStatusChanged::class, ServerValidated::class]); + + Livewire::test(ActivityMonitor::class) + ->call('newMonitorActivity', $this->activity->id, $event) + ->assertSet('eventToDispatch', null) + ->call('polling') + ->assertNotDispatched($event); + + Event::assertNotDispatched(ProxyStatusChanged::class); + Event::assertNotDispatched(ServerValidated::class); + })->with([ProxyStatusChanged::class, ServerValidated::class, 'someOtherEvent']); + + test('an allowed application event is dispatched with the team of the activity and no client data', function () { + Event::fake([ServiceStatusChanged::class]); + + Livewire::test(ActivityMonitor::class) + ->call('newMonitorActivity', $this->activity->id, ServiceStatusChanged::class, $this->otherTeam->id) + ->call('polling'); + + Event::assertDispatched(ServiceStatusChanged::class, fn (ServiceStatusChanged $event) => $event->teamId === $this->team->id); + }); + + test('allowed browser events are dispatched with their data', function (string $event, mixed $data) { + Livewire::test(ActivityMonitor::class) + ->call('newMonitorActivity', $this->activity->id, $event, $data) + ->call('polling') + ->assertDispatched($event); + })->with([ + ['activityFinished', null], + ['prerequisitesInstalled', null], + ['sysboxInstalled', null], + ['init', 2], + ]); +}); diff --git a/tests/Feature/Proxy/ProxyStatusChangedNotificationTest.php b/tests/Feature/Proxy/ProxyStatusChangedNotificationTest.php new file mode 100644 index 0000000000..2b7ebd43b5 --- /dev/null +++ b/tests/Feature/Proxy/ProxyStatusChangedNotificationTest.php @@ -0,0 +1,75 @@ + Server::flushIdentityMap()); + +beforeEach(function () { + Server::flushIdentityMap(); + InstanceSettings::forceCreate(['id' => 0]); + config(['constants.ssh.mux_enabled' => false]); + Process::fake(); + + $this->team = Team::factory()->create(); + $this->server = Server::factory()->create([ + 'team_id' => $this->team->id, + 'private_key_id' => PrivateKey::factory()->create(['team_id' => $this->team->id])->id, + ]); + $this->server->settings->update(['is_reachable' => true, 'is_usable' => true, 'force_disabled' => false]); + $this->server->markProxyTransitionStarted(); +}); + +test('the listener updates the proxy of the server of the event team', function () { + Event::fake([ProxyStatusChangedUI::class]); + + (new ProxyStatusChangedNotification)->handle(new ProxyStatusChanged(ProxyStatusChanged::dataFor($this->server))); + + expect($this->server->fresh()->isProxyTransitionInProgress())->toBeFalse(); + Process::assertRan(fn () => true); +}); + +test('the listener ignores a server that is not in the event team', function () { + $otherTeam = Team::factory()->create(); + + (new ProxyStatusChangedNotification)->handle(new ProxyStatusChanged([ + 'server_id' => $this->server->id, + 'team_id' => $otherTeam->id, + ])); + + expect($this->server->fresh()->isProxyTransitionInProgress())->toBeTrue(); + Process::assertNothingRan(); +}); + +test('the listener ignores event data without a team', function (mixed $data) { + (new ProxyStatusChangedNotification)->handle(new ProxyStatusChanged($data)); + + expect($this->server->fresh()->isProxyTransitionInProgress())->toBeTrue(); + Process::assertNothingRan(); +})->with([ + 'bare server id' => fn () => $this->server->id, + 'no team id' => fn () => ['server_id' => $this->server->id], +]); + +test('stopping the proxy dispatches the event with the server and its team', function () { + Event::fake([ProxyStatusChanged::class, ProxyStatusChangedUI::class]); + + StopProxy::run($this->server); + + Event::assertDispatched(ProxyStatusChanged::class, fn (ProxyStatusChanged $event) => $event->data === [ + 'server_id' => $this->server->id, + 'team_id' => $this->team->id, + ]); +}); diff --git a/tests/Feature/PushServerUpdateJobOptimizationTest.php b/tests/Feature/PushServerUpdateJobOptimizationTest.php index 0ae7a420cd..1083420121 100644 --- a/tests/Feature/PushServerUpdateJobOptimizationTest.php +++ b/tests/Feature/PushServerUpdateJobOptimizationTest.php @@ -216,7 +216,7 @@ it('ends the proxy restart when the proxy status listener runs', function () { $server->markProxyTransitionStarted(); Process::fake(); - (new ProxyStatusChangedNotification)->handle(new ProxyStatusChanged($server->id)); + (new ProxyStatusChangedNotification)->handle(new ProxyStatusChanged(ProxyStatusChanged::dataFor($server))); expect($server->isProxyTransitionInProgress())->toBeFalse(); }); diff --git a/tests/Feature/ResourceStatusRefresherTest.php b/tests/Feature/ResourceStatusRefresherTest.php index 35b589cad6..ad19885465 100644 --- a/tests/Feature/ResourceStatusRefresherTest.php +++ b/tests/Feature/ResourceStatusRefresherTest.php @@ -176,9 +176,29 @@ test('a finished service start stores the status of each service part and broadc inspectedContainer("db-{$service->uuid}", 'running', 'healthy', $labels('database', $db->uuid, 'db')), ])]); - event(new ServiceStartFinished($service->id)); + event(new ServiceStartFinished(ServiceStartFinished::dataFor($service))); expect($app->fresh()->status)->toBe('running:unknown') ->and($db->fresh()->status)->toBe('running:healthy'); Event::assertDispatched(ServiceStatusChanged::class, fn (ServiceStatusChanged $event) => $event->teamId === $this->team->id); }); + +test('a finished service start is ignored when the event team does not own the service', function (string $case) { + Event::fake([ServiceStatusChanged::class]); + Process::fake(); + $service = Service::factory()->create([ + 'environment_id' => $this->environment->id, + 'server_id' => $this->server->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + ]); + $data = match ($case) { + 'other team' => ['service_id' => $service->id, 'team_id' => Team::factory()->create()->id], + 'no team' => ['service_id' => $service->id], + }; + + event(new ServiceStartFinished($data)); + + Process::assertNothingRan(); + Event::assertNotDispatched(ServiceStatusChanged::class); +})->with(['other team', 'no team']); diff --git a/tests/Feature/Server/CloudflareTunnelChangedNotificationTest.php b/tests/Feature/Server/CloudflareTunnelChangedNotificationTest.php index a6481e4489..51ee5f3098 100644 --- a/tests/Feature/Server/CloudflareTunnelChangedNotificationTest.php +++ b/tests/Feature/Server/CloudflareTunnelChangedNotificationTest.php @@ -5,6 +5,7 @@ use App\Listeners\CloudflareTunnelChangedNotification; use App\Models\InstanceSettings; use App\Models\PrivateKey; use App\Models\Server; +use App\Models\Team; use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Event; @@ -41,9 +42,25 @@ it('uses sudo for the health check on a non-root server', function () { (new CloudflareTunnelChangedNotification)->handle(new CloudflareTunnelChanged([ 'server_id' => $this->server->id, + 'team_id' => $this->server->team_id, 'ssh_domain' => 'ssh.example.com', ])); expect($this->server->settings->refresh()->is_cloudflare_tunnel)->toBeTruthy() ->and($this->server->refresh()->ip)->toBe('ssh.example.com'); }); + +it('ignores a server that is not in the event team', function (string $case) { + Event::fake(); + Process::fake(); + $data = match ($case) { + 'other team' => ['server_id' => $this->server->id, 'team_id' => Team::factory()->create()->id, 'ssh_domain' => 'ssh.example.com'], + 'no team' => ['server_id' => $this->server->id, 'ssh_domain' => 'ssh.example.com'], + }; + + (new CloudflareTunnelChangedNotification)->handle(new CloudflareTunnelChanged($data)); + + Process::assertNothingRan(); + expect($this->server->settings->refresh()->is_cloudflare_tunnel)->toBeFalsy() + ->and($this->server->refresh()->ip)->not->toBe('ssh.example.com'); +})->with(['other team', 'no team']); From 5145aa53dc7eb68a3a3c40e65728a33c88a694f6 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 27 Oct 2026 22:39:52 +0100 Subject: [PATCH 2/8] fix(services): align service configuration form with role permissions (#12356) Co-authored-by: Claude Opus 5.5 --- app/Livewire/Project/Service/StackForm.php | 12 ++-- .../project/service/stack-form.blade.php | 2 +- .../StackFormPasswordVisibilityTest.php | 56 +++++++++++++++++++ 3 files changed, 64 insertions(+), 6 deletions(-) diff --git a/app/Livewire/Project/Service/StackForm.php b/app/Livewire/Project/Service/StackForm.php index cc6a994b36..d410ffeda1 100644 --- a/app/Livewire/Project/Service/StackForm.php +++ b/app/Livewire/Project/Service/StackForm.php @@ -135,7 +135,7 @@ class StackForm extends Component $lockedKeys = $this->lockedFieldKeys(); $this->fields = $this->fields->map(function ($field) use ($lockedKeys) { $field['isLocked'] = $lockedKeys->contains(data_get($field, 'key')); - if ($field['isLocked'] || ($this->isPasswordHiddenForMember && data_get($field, 'isPassword'))) { + if ($field['isLocked'] || $this->isPasswordHiddenForMember) { $field['value'] = null; } @@ -190,10 +190,12 @@ class StackForm extends Component // Use transaction to ensure atomicity - if parse fails, save is rolled back DB::transaction(function () { $this->service->save(); - $lockedKeys = $this->lockedFieldKeys(); - $this->service->saveExtraFields( - $this->fields->reject(fn ($field) => $lockedKeys->contains(data_get($field, 'key'))) - ); + if (auth()->user()->can('manageEnvironment', $this->service)) { + $lockedKeys = $this->lockedFieldKeys(); + $this->service->saveExtraFields( + $this->fields->reject(fn ($field) => $lockedKeys->contains(data_get($field, 'key'))) + ); + } $this->service->parse(); }); // Refresh and write files after a successful commit diff --git a/resources/views/livewire/project/service/stack-form.blade.php b/resources/views/livewire/project/service/stack-form.blade.php index 59405e03f1..4e0ecaf185 100644 --- a/resources/views/livewire/project/service/stack-form.blade.php +++ b/resources/views/livewire/project/service/stack-form.blade.php @@ -79,7 +79,7 @@ @if (data_get($field, 'isLocked')) - @elseif ($isPasswordHiddenForMember && data_get($field, 'isPassword')) + @elseif ($isPasswordHiddenForMember) @else masterPassword->fresh()->value)->toBe('$SERVICE_PASSWORD_64_DRIZZLE') ->and($lockedTarget->fresh()->value)->toBe('referenced-locked-secret'); }); + +function addVaultwardenDatabaseUrl(): void +{ + ServiceApplication::create([ + 'uuid' => (string) Str::uuid(), + 'service_id' => test()->service->id, + 'name' => 'vaultwarden', + 'image' => 'vaultwarden/server:latest', + ]); + test()->service->environment_variables()->create([ + 'key' => 'DATABASE_URL', + 'value' => 'postgresql://vw:vaultwarden-db-secret@db:5432/vw', + ]); +} + +it('hides extra-field values without a password flag from a member', function () { + addVaultwardenDatabaseUrl(); + $ownTeam = Team::factory()->create(); + $ownTeam->members()->attach($this->member, ['role' => 'owner']); + + $this->actingAs($this->member); + foreach ([$this->team, $ownTeam] as $sessionTeam) { + session(['currentTeam' => $sessionTeam]); + + $component = mountStackForm(); + expect($component->get('fields')['DATABASE_URL']['value'])->toBeNull() + ->and(json_encode($component->snapshot))->not->toContain('vaultwarden-db-secret') + ->and($component->html())->not->toContain('vaultwarden-db-secret') + ->and($component->html())->toContain('Hidden (only admins can view)'); + } +}); + +it('does not let a member save overwrite stored extra-field values', function () { + addVaultwardenDatabaseUrl(); + $this->actingAs($this->member); + session(['currentTeam' => $this->team]); + + mountStackForm() + ->set('name', 'renamed-by-member') + ->set('fields.DATABASE_URL.value', 'member-value') + ->call('submit'); + + expect($this->service->fresh()->name)->not->toBe('renamed-by-member') + ->and($this->service->environment_variables()->where('key', 'DATABASE_URL')->value('value')) + ->toBe('postgresql://vw:vaultwarden-db-secret@db:5432/vw') + ->and($this->masterPassword->fresh()->value)->toBe('drizzle-master-secret'); +}); + +it('shows extra-field values without a password flag to an owner of the service team', function () { + addVaultwardenDatabaseUrl(); + $this->actingAs($this->owner); + session(['currentTeam' => $this->team]); + + expect(mountStackForm()->get('fields')['DATABASE_URL']['value']) + ->toBe('postgresql://vw:vaultwarden-db-secret@db:5432/vw'); +}); From c2de0d83d893c0572659c527c765761a59e028f4 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 27 Oct 2026 22:58:21 +0100 Subject: [PATCH 3/8] docs(security): clarify threat model and vulnerability severity --- AGENTS.md | 19 +++++++++++++++++++ SECURITY.md | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 1ab68ce6b8..467a37aef1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -177,6 +177,25 @@ Because the "server" and the test share one PHP process, they share the phpunit - Add authorization regression tests for protected changes. Cover permitted access, member restrictions where applicable, and cross-team access; verify unauthorized reads and writes return `403` or otherwise reveal no protected data. - Do not add a `TRUSTED_PROXIES` setting or change `TrustProxies` to use one. This caused problems in supported Coolify deployments. Fix IP-based rate limits and allow-lists at their call sites instead of changing proxy trust as a shortcut. +### Security threat model (read before you report or rate a vulnerability) + +Before you rate a vulnerability or write an advisory, find the security boundary that the attack crosses. The boundary decides the severity. A finding that crosses no boundary is still a valid finding, but its severity is low. The full scope is in [`SECURITY.md`](SECURITY.md). + +- Trusted by design (attacks that need these roles are low severity): + - A team admin or owner, and an API token with `write` or `root` ability, already has root on every server of that team (terminal, any Compose file with host mounts, scheduled commands, all team secrets). + - An instance admin (admin or owner of the root team) already controls the Coolify instance and its host. + - Repository, Dockerfile, and Compose content that a team admin deploys is trusted for that deployment. +- Real boundaries: + - No login -> any access. + - Team member (read-only) -> write, command execution, or secrets in the own team. + - One team -> another team. + - Team user -> the Coolify container or the Coolify host, when the user is not an instance admin. + - Token -> more than its abilities allow. +- Rate with the real role that the attack needs, not the role that the reporter claims. Read the policy or gate that protects the path. Admin needed means `PR:H`. Use `S:C` only when the impact reaches another team, the Coolify instance, or a server that the attacker does not manage. +- If a path is reachable by a member only because of a different missing authorization check, report that check as the main issue, with high severity. Rate each injection behind it by the role it needs when the check works. +- Always fix low-severity findings (escape shell arguments, validate input, confine paths) when you find them. Do not dismiss them because the attacker is an admin. +- Do not put exploit details, payloads, or unreleased advisory IDs in commits, PR descriptions, code comments, or this file. + ### Event Broadcasting - Laravel Reverb WebSocket server for real-time updates (port 6001) and a Node terminal WebSocket server (port 6002), both run inside the `coolify` container as s6 services - Server-side broadcasts use `PUSHER_BACKEND_HOST`/`PUSHER_BACKEND_PORT` (defaults `127.0.0.1:6001`); `PUSHER_HOST`/`PUSHER_PORT` are browser-facing only diff --git a/SECURITY.md b/SECURITY.md index e491737ef6..3a361342ab 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -23,3 +23,43 @@ If you discover a security vulnerability, please follow these steps: - A description of the vulnerability - Steps to reproduce the issue - Potential impact + +## Scope and Threat Model + +Please read this section before you send a report. The severity of a report depends on the security boundary that it crosses. + +### Roles and what they can already do + +- **Team member**: read-only access to the resources of the team. A member must not see secrets or change anything. +- **Team admin / owner**: full control of the team's servers. An admin can open a terminal on these servers, deploy any Docker Compose file (including privileged containers and bind mounts of the host file system), run scheduled commands, and read all team secrets. An admin already has root access to every server of the team. +- **API tokens**: a token has the permissions of its abilities. A token with `write` or `root` ability is equal to a team admin. +- **Instance admin** (admin or owner of the root team): already controls the Coolify instance and the server it runs on. +- **Repository and Compose content**: Coolify runs what the configured repository, Dockerfile, or Compose file tells it to run. People who can change this content are trusted for the deployments that use it. + +### In scope + +- A user who is not logged in can read data, change data, or run code. +- A team member gets write access, runs commands, or reads secrets of their own team. +- A user of one team reads or changes resources of another team. +- A team user runs code inside the Coolify container or on the Coolify host, without being an instance admin. +- An API token does more than its abilities allow. +- A public webhook or callback can be used without a valid secret or signature. + +### Low severity (still welcome) + +Please still report these. They are often good findings, and we fix them. Because the attacker already has the same power through normal features, we rate them as low severity: + +- Command injection, path traversal, file read or write, or container escape on a server that the attacker's own team manages, when the attack needs the admin or owner role (or a `write` token). +- Problems that need a malicious repository, Dockerfile, or Compose file that a team admin chose to deploy. +- Server-side requests that a team admin can already make from the team's own servers. + +If you find a way to reach one of these with less privilege (for example, as a member, from another team, or without login), it is in scope with a higher severity. + +### Out of scope + +- Attacks that need the instance admin role. +- Reports based only on automated scanner output, without a working attack path. + +### Severity + +Rate the report with the real privilege that the attack needs. If an attack needs the admin role, use `PR:H`. Use `S:C` only when the impact reaches another team, the Coolify instance, or a server that the attacker does not manage. From ef57eea32037633c8e8ad89b35e2ce2f1d4f4387 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 28 Oct 2026 10:35:46 +0100 Subject: [PATCH 4/8] fix(backups): sign mc requests with S3v4 and retry failed S3 uploads --- app/Actions/Database/StartDatabaseImport.php | 2 +- app/Jobs/DatabaseBackupJob.php | 6 +-- app/Jobs/VolumeBackupJob.php | 6 +-- app/Models/S3Storage.php | 33 ++++++++++-- tests/Feature/BackupS3CredentialsArgvTest.php | 4 +- .../BackupS3DestinationRedactionTest.php | 2 + .../StartDatabaseImportS3CredentialsTest.php | 2 +- ...VolumeBackupMultipleS3DestinationsTest.php | 24 +++++++++ tests/Unit/S3StorageTest.php | 54 +++++++++++++++++-- 9 files changed, 115 insertions(+), 18 deletions(-) diff --git a/app/Actions/Database/StartDatabaseImport.php b/app/Actions/Database/StartDatabaseImport.php index d9e491345c..f8c3c861ba 100644 --- a/app/Actions/Database/StartDatabaseImport.php +++ b/app/Actions/Database/StartDatabaseImport.php @@ -318,7 +318,7 @@ class StartDatabaseImport private function setMinioClientAlias(S3Storage $storage, Server $server, string $helper): void { $remoteCommand = ($server->isNonRoot() ? 'sudo ' : '').'docker exec -i '.escapeshellarg($helper) - .' mc alias set'.$storage->minioClientPathOption().' s3temp '.escapeshellarg((string) $storage->endpoint).' >/dev/null'; + .' mc alias set'.$storage->minioClientAliasOptions().' s3temp '.escapeshellarg((string) $storage->endpoint).' >/dev/null'; $timeout = (int) config('constants.ssh.command_timeout'); SshRetryHandler::retry( diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index 07e2fd56e3..2fe350e37d 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -949,10 +949,10 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue instant_remote_process($commands, $this->server, true, false, $this->commandTimeout(), disableMultiplexing: true); $this->setMinioClientAlias($this->server, "backup-of-{$this->backup_log_uuid}", $s3, $resolveOptions); - instant_remote_process( + S3Storage::retryMinioClientUpload(fn () => instant_remote_process( ["docker exec backup-of-{$this->backup_log_uuid} mc cp{$resolveOptions} {$escapedBackupLocation} {$escapedS3Destination}"], $this->server, true, false, $this->commandTimeout(), disableMultiplexing: true, - ); + )); } catch (Throwable $e) { $this->add_to_error_output($e->getMessage()); @@ -970,7 +970,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue private function setMinioClientAlias(Server $server, string $container, S3Storage $s3, string $resolveOptions): void { $remoteCommand = ($server->isNonRoot() ? 'sudo ' : '').'docker exec -i '.escapeshellarg($container) - .' mc alias set'.$resolveOptions.$s3->minioClientPathOption().' temporary '.escapeshellarg($s3->endpoint).' >/dev/null'; + .' mc alias set'.$resolveOptions.$s3->minioClientAliasOptions().' temporary '.escapeshellarg($s3->endpoint).' >/dev/null'; $timeout = (int) config('constants.ssh.command_timeout'); SshRetryHandler::retry( diff --git a/app/Jobs/VolumeBackupJob.php b/app/Jobs/VolumeBackupJob.php index b7524c7012..42f209d1e5 100644 --- a/app/Jobs/VolumeBackupJob.php +++ b/app/Jobs/VolumeBackupJob.php @@ -449,10 +449,10 @@ class VolumeBackupJob implements ShouldBeEncrypted, ShouldQueue .escapeshellarg($backupLocation.':'.$backupLocation.':ro').' '.escapeshellarg($image), ], $server, timeout: $this->timeout, disableMultiplexing: true); $this->setMinioClientAlias($server, $containerName, $s3, $resolveOptions); - instant_remote_process([ + S3Storage::retryMinioClientUpload(fn () => instant_remote_process([ 'docker exec '.escapeshellarg($containerName).' mc cp'.$resolveOptions.' '.escapeshellarg($backupLocation).' ' .escapeshellarg('temporary/'.$s3->bucket.$backupDirectory.'/'), - ], $server, timeout: $this->timeout, disableMultiplexing: true); + ], $server, timeout: $this->timeout, disableMultiplexing: true)); } finally { instant_remote_process( ['docker rm -f '.escapeshellarg($containerName)], @@ -487,7 +487,7 @@ class VolumeBackupJob implements ShouldBeEncrypted, ShouldQueue private function setMinioClientAlias(Server $server, string $container, S3Storage $s3, string $resolveOptions): void { $remoteCommand = ($server->isNonRoot() ? 'sudo ' : '').'docker exec -i '.escapeshellarg($container) - .' mc alias set'.$resolveOptions.$s3->minioClientPathOption().' temporary '.escapeshellarg($s3->endpoint).' >/dev/null'; + .' mc alias set'.$resolveOptions.$s3->minioClientAliasOptions().' temporary '.escapeshellarg($s3->endpoint).' >/dev/null'; $timeout = (int) config('constants.ssh.command_timeout'); SshRetryHandler::retry( diff --git a/app/Models/S3Storage.php b/app/Models/S3Storage.php index 6bd9f88322..84bf072dc1 100644 --- a/app/Models/S3Storage.php +++ b/app/Models/S3Storage.php @@ -34,6 +34,16 @@ class S3Storage extends BaseModel */ private const PATH_STYLE_REJECTION_ERRORS = ['secondleveldomainforbidden', 'pathstyledomainforbidden', 'virtual hosted style', 'virtual-hosted style', 'virtual-styled']; + /** + * Waits before the second and the third attempt of an upload with the MinIO client. + */ + private const MINIO_CLIENT_UPLOAD_RETRY_DELAYS_MS = [10_000, 30_000]; + + /** + * The exception code of runSshProcess() when a remote command times out. + */ + private const REMOTE_COMMAND_TIMEOUT_EXIT_CODE = 124; + protected $fillable = [ 'team_id', 'name', @@ -349,12 +359,27 @@ class S3Storage extends BaseModel } /** - * The `mc alias set` option that makes the MinIO client use virtual-hosted-style URLs, as filesystem() does. - * With path-style URLs, mc keeps its own `auto` lookup, as before. + * The `mc alias set` options. `--api S3v4` signs as filesystem() does. Without it, mc probes the signature + * version on every run, and a network error during the probe makes it try S3v2, which most providers reject. + * `--path off` uses virtual-hosted-style URLs, as filesystem() does. With path-style URLs, mc keeps its own + * `auto` lookup, as before. */ - public function minioClientPathOption(): string + public function minioClientAliasOptions(): string { - return $this->usesPathStyleEndpoint() ? '' : ' --path off'; + return ' --api S3v4'.($this->usesPathStyleEndpoint() ? '' : ' --path off'); + } + + /** + * Runs an upload of a local file with the MinIO client again when it fails, for example after a short network + * outage. A timeout is not retried, because another full attempt can pass the timeout of the backup job. + */ + public static function retryMinioClientUpload(callable $upload): mixed + { + return retry( + self::MINIO_CLIENT_UPLOAD_RETRY_DELAYS_MS, + $upload, + when: fn (\Throwable $exception): bool => $exception->getCode() !== self::REMOTE_COMMAND_TIMEOUT_EXIT_CODE, + ); } /** diff --git a/tests/Feature/BackupS3CredentialsArgvTest.php b/tests/Feature/BackupS3CredentialsArgvTest.php index 5a2775751b..3ecd360a76 100644 --- a/tests/Feature/BackupS3CredentialsArgvTest.php +++ b/tests/Feature/BackupS3CredentialsArgvTest.php @@ -201,8 +201,8 @@ it('makes mc use virtual-hosted-style URLs when the storage turns path-style URL $aliasProcess = $processes->sole(fn (array $process) => str_contains($process['command'], 'mc alias set')); expect(s3ArgvTestRunAliasProcess($aliasProcess)['argv'])->toContain($expectedAlias); })->with([ - 'path-style' => [true, 'mc alias set temporary https://8.8.8.8'], - 'virtual-hosted-style' => [false, 'mc alias set --path off temporary https://8.8.8.8'], + 'path-style' => [true, 'mc alias set --api S3v4 temporary https://8.8.8.8'], + 'virtual-hosted-style' => [false, 'mc alias set --api S3v4 --path off temporary https://8.8.8.8'], ]); it('passes the S3 keys of a volume backup to mc on stdin, not in a command line', function (string $sshUser, bool $streamToS3) { diff --git a/tests/Feature/BackupS3DestinationRedactionTest.php b/tests/Feature/BackupS3DestinationRedactionTest.php index 92deab9ded..81cf689f99 100644 --- a/tests/Feature/BackupS3DestinationRedactionTest.php +++ b/tests/Feature/BackupS3DestinationRedactionTest.php @@ -24,6 +24,7 @@ use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Notification; use Illuminate\Support\Facades\Process; use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Sleep; uses(RefreshDatabase::class); @@ -41,6 +42,7 @@ beforeEach(function () { defined('CURLOPT_RESOLVE') || define('CURLOPT_RESOLVE', 10203); Event::fake([BackupCreated::class]); Notification::fake(); + Sleep::fake(); InstanceSettings::unguarded(fn () => InstanceSettings::query()->firstOrCreate(['id' => 0])); $this->team = Team::factory()->create(); diff --git a/tests/Feature/StartDatabaseImportS3CredentialsTest.php b/tests/Feature/StartDatabaseImportS3CredentialsTest.php index c53b343711..15b0041535 100644 --- a/tests/Feature/StartDatabaseImportS3CredentialsTest.php +++ b/tests/Feature/StartDatabaseImportS3CredentialsTest.php @@ -300,7 +300,7 @@ it('passes the S3 keys of an import to mc on stdin, not in a command line', func $run = importS3ArgvTestRunAliasProcess($aliasProcess); expect($run['mcStdin'])->toBe(IMPORT_S3_ARGV_KEY."\n".IMPORT_S3_ARGV_SECRET."\n") - ->and($run['argv'])->toContain('mc alias set s3temp https://8.8.8.8') + ->and($run['argv'])->toContain('mc alias set --api S3v4 s3temp https://8.8.8.8') ->and($run['argv'])->not->toContain(IMPORT_S3_ARGV_SECRET) ->and($run['argv'])->not->toContain(IMPORT_S3_ARGV_KEY) ->and($processes->contains(fn (array $process) => str_contains($process['command']."\n".$process['input'], "-e 'MC_REGION=ru-central-1'")))->toBeTrue() diff --git a/tests/Feature/VolumeBackupMultipleS3DestinationsTest.php b/tests/Feature/VolumeBackupMultipleS3DestinationsTest.php index 45e5a81200..ae6826f584 100644 --- a/tests/Feature/VolumeBackupMultipleS3DestinationsTest.php +++ b/tests/Feature/VolumeBackupMultipleS3DestinationsTest.php @@ -22,12 +22,14 @@ use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Notification; use Illuminate\Support\Facades\Process; use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Sleep; uses(RefreshDatabase::class); beforeEach(function () { Server::flushIdentityMap(); Notification::fake(); + Sleep::fake(); config(['broadcasting.default' => 'null']); defined('CURLOPT_RESOLVE') || define('CURLOPT_RESOLVE', 10203); }); @@ -169,6 +171,28 @@ it('keeps the local archive and cleans the partial upload when one destination f ->and($secondReplica->message)->not->toBeEmpty(); }); +it('uploads again when the first upload to a destination fails', function () { + [$backup] = createMultiDestinationVolumeBackup(); + fakeMultiDestinationS3Disks(); + $firstBucketUploads = 0; + Process::fake(function ($process) use (&$firstBucketUploads) { + $script = remoteProcessScript($process); + if (str_contains($script, 'mc cp') && str_contains($script, 'temporary/first-bucket/') && ++$firstBucketUploads === 1) { + return Process::result(errorOutput: 'dial tcp: connect: network is unreachable', exitCode: 1); + } + + return Process::result(output: str_contains($script, 'du -b') ? '128' : ''); + }); + + (new VolumeBackupJob($backup))->handle(); + + $execution = ScheduledVolumeBackupExecution::query()->sole(); + expect($firstBucketUploads)->toBe(2) + ->and($execution->status)->toBe('success') + ->and($execution->message)->toBeNull() + ->and($execution->s3_uploaded)->toBeTrue(); +}); + it('applies S3 retention to each destination separately', function () { [$backup, $first, $second, $server] = createMultiDestinationVolumeBackup([ 'retention_amount_locally' => 0, diff --git a/tests/Unit/S3StorageTest.php b/tests/Unit/S3StorageTest.php index 2778d0a741..4e06b725aa 100644 --- a/tests/Unit/S3StorageTest.php +++ b/tests/Unit/S3StorageTest.php @@ -3,6 +3,7 @@ use App\Models\S3Storage; use Illuminate\Filesystem\FilesystemAdapter; use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Sleep; use Tests\TestCase; uses(TestCase::class); @@ -114,11 +115,11 @@ test('S3Storage uses path-style URLs unless the storage turns them off', functio ]); expect($s3Storage->filesystem())->toBe($disk) - ->and($s3Storage->minioClientPathOption())->toBe($mcOption); + ->and($s3Storage->minioClientAliasOptions())->toBe($mcOption); })->with([ - 'not saved yet' => [null, true, ''], - 'path-style' => [true, true, ''], - 'virtual-hosted-style' => [false, false, ' --path off'], + 'not saved yet' => [null, true, ' --api S3v4'], + 'path-style' => [true, true, ' --api S3v4'], + 'virtual-hosted-style' => [false, false, ' --api S3v4 --path off'], ]); test('S3Storage detects the URL style from the endpoint host', function (?string $endpoint, bool $usesPathStyle) { @@ -227,3 +228,48 @@ test('S3Storage testConnection rejects invalid bucket before building client', f 'newline' => ["lab\nid"], 'underscore' => ['lab_bucket'], ]); + +test('S3Storage retries a failed MinIO client upload and waits longer before each attempt', function () { + Sleep::fake(); + $attempts = 0; + + $result = S3Storage::retryMinioClientUpload(function () use (&$attempts) { + if (++$attempts < 3) { + throw new RuntimeException('dial tcp: connect: network is unreachable', 1); + } + + return 'uploaded'; + }); + + expect($result)->toBe('uploaded')->and($attempts)->toBe(3); + Sleep::assertSequence([Sleep::for(10)->seconds(), Sleep::for(30)->seconds()]); +}); + +test('S3Storage stops retrying a MinIO client upload after three attempts', function () { + Sleep::fake(); + $attempts = 0; + + $upload = function () use (&$attempts) { + $attempts++; + + throw new RuntimeException('Access Denied.', 1); + }; + + expect(fn () => S3Storage::retryMinioClientUpload($upload))->toThrow(RuntimeException::class, 'Access Denied.'); + expect($attempts)->toBe(3); +}); + +test('S3Storage does not retry a MinIO client upload that timed out', function () { + Sleep::fake(); + $attempts = 0; + + $upload = function () use (&$attempts) { + $attempts++; + + throw new RuntimeException('The command on the server timed out after 3600 seconds.', 124); + }; + + expect(fn () => S3Storage::retryMinioClientUpload($upload))->toThrow(RuntimeException::class, 'timed out'); + expect($attempts)->toBe(1); + Sleep::assertNeverSlept(); +}); From 5dacaed9f27d19f722a1b9769a3aae33bc11e4c6 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 28 Oct 2026 12:10:27 +0100 Subject: [PATCH 5/8] fix(proxy): keep the application proxy labels of v4.5.0 The retry labels (Traefik retry middleware, Caddy lb_try_duration) changed the labels of every application. On the first rolling update after a settings save, the old and the new container defined the same Traefik router differently, so Traefik dropped the route and returned 503 until the old container stopped. Remove the retry labels from this release. The rolling update drain wait stays. --- bootstrap/helpers/docker.php | 21 ++-------------- tests/Feature/RollingUpdateProxyDrainTest.php | 25 ++++++------------- 2 files changed, 10 insertions(+), 36 deletions(-) diff --git a/bootstrap/helpers/docker.php b/bootstrap/helpers/docker.php index f179df1d96..d3ca519766 100644 --- a/bootstrap/helpers/docker.php +++ b/bootstrap/helpers/docker.php @@ -801,7 +801,7 @@ function caddySiteAddress(string $scheme, string $host, bool $isForceHttpsEnable return "{$scheme}://{$host}"; } -function fqdnLabelsForCaddy(string $network, string $uuid, Collection $domains, bool $is_force_https_enabled = false, $onlyPort = null, ?Collection $serviceLabels = null, ?bool $is_gzip_enabled = true, ?bool $is_stripprefix_enabled = true, ?string $service_name = null, ?string $image = null, string $redirect_direction = 'both', ?string $predefinedPort = null, bool $is_http_basic_auth_enabled = false, ?string $http_basic_auth_username = null, ?string $http_basic_auth_password = null, ?Collection $noindex_domains = null, bool $is_traffic_analytics_enabled = false, array $domainPortOverrides = [], bool $supports_log_append = false, bool $supports_basic_auth_directive = false, ?int $http_basic_auth_bcrypt_cost = null, array $http_basic_auth_argon2id_options = [], bool $is_permanent_redirect_enabled = false, bool $is_retry_enabled = false) +function fqdnLabelsForCaddy(string $network, string $uuid, Collection $domains, bool $is_force_https_enabled = false, $onlyPort = null, ?Collection $serviceLabels = null, ?bool $is_gzip_enabled = true, ?bool $is_stripprefix_enabled = true, ?string $service_name = null, ?string $image = null, string $redirect_direction = 'both', ?string $predefinedPort = null, bool $is_http_basic_auth_enabled = false, ?string $http_basic_auth_username = null, ?string $http_basic_auth_password = null, ?Collection $noindex_domains = null, bool $is_traffic_analytics_enabled = false, array $domainPortOverrides = [], bool $supports_log_append = false, bool $supports_basic_auth_directive = false, ?int $http_basic_auth_bcrypt_cost = null, array $http_basic_auth_argon2id_options = [], bool $is_permanent_redirect_enabled = false) { $labels = collect([]); if ($serviceLabels) { @@ -854,11 +854,6 @@ function fqdnLabelsForCaddy(string $network, string $uuid, Collection $domains, } else { $labels->push("caddy_{$loop}.{$handle}.{$loop}_reverse_proxy={{upstreams}}"); } - if ($is_retry_enabled) { - // A failed connection, for example to the old container that stops during a rolling - // update, is retried on another upstream. Caddy always retries a connection that failed. - $labels->push("caddy_{$loop}.{$handle}.{$loop}_reverse_proxy.lb_try_duration=5s"); - } $labels->push("caddy_{$loop}.{$handle}={$path}*"); if ($is_gzip_enabled) { $labels->push("caddy_{$loop}.encode=zstd gzip"); @@ -963,7 +958,7 @@ function traefikHostRulesWithoutIpv6Brackets(Collection $labels): Collection ) : $label); } -function fqdnLabelsForTraefik(string $uuid, Collection $domains, bool $is_force_https_enabled = false, $onlyPort = null, ?Collection $serviceLabels = null, ?bool $is_gzip_enabled = true, ?bool $is_stripprefix_enabled = true, ?string $service_name = null, bool $generate_unique_uuid = false, ?string $image = null, string $redirect_direction = 'both', bool $is_http_basic_auth_enabled = false, ?string $http_basic_auth_username = null, ?string $http_basic_auth_password = null, ?Collection $noindex_domains = null, bool $escape_redirect_replacement_for_compose = true, array $domainPortOverrides = [], ?string $certificate_resolver = 'letsencrypt', ?int $http_basic_auth_bcrypt_cost = null, bool $is_permanent_redirect_enabled = false, bool $is_retry_enabled = false) +function fqdnLabelsForTraefik(string $uuid, Collection $domains, bool $is_force_https_enabled = false, $onlyPort = null, ?Collection $serviceLabels = null, ?bool $is_gzip_enabled = true, ?bool $is_stripprefix_enabled = true, ?string $service_name = null, bool $generate_unique_uuid = false, ?string $image = null, string $redirect_direction = 'both', bool $is_http_basic_auth_enabled = false, ?string $http_basic_auth_username = null, ?string $http_basic_auth_password = null, ?Collection $noindex_domains = null, bool $escape_redirect_replacement_for_compose = true, array $domainPortOverrides = [], ?string $certificate_resolver = 'letsencrypt', ?int $http_basic_auth_bcrypt_cost = null, bool $is_permanent_redirect_enabled = false) { $labels = collect([]); $labels->push('traefik.enable=true'); @@ -1018,14 +1013,6 @@ function fqdnLabelsForTraefik(string $uuid, Collection $domains, bool $is_force_ ->filter() ->unique(); } - if ($is_retry_enabled) { - // Sends a request to another container when the connection fails, for example to the old - // container that stops during a rolling update. Traefik does not retry after a response. - // A retry can pick the old container again under load; 3 attempts left about 5% of the - // requests failing in a live test, 5 attempts less than 1% (#8627). - $labels->push("traefik.http.middlewares.retry-{$uuid}.retry.attempts=5"); - $middlewares_from_labels->push("retry-{$uuid}"); - } foreach ($domains as $loop => $domain) { try { if ($generate_unique_uuid) { @@ -1291,7 +1278,6 @@ function generateLabelsApplication(Application $application, ?ApplicationPreview http_basic_auth_bcrypt_cost: $application->http_basic_auth_bcrypt_cost, noindex_domains: $noindexDomains, domainPortOverrides: $application->domain_port_overrides ?? [], - is_retry_enabled: true, certificate_resolver: $application->destination->server->traefikCertificateResolver($application->settings?->traefik_certificate_resolver), )); break; @@ -1316,7 +1302,6 @@ function generateLabelsApplication(Application $application, ?ApplicationPreview supports_log_append: $application->destination->server->caddySupportsLogAppend(), supports_basic_auth_directive: $application->destination->server->caddySupportsBasicAuthDirective(), domainPortOverrides: $application->domain_port_overrides ?? [], - is_retry_enabled: true, )); break; } @@ -1337,7 +1322,6 @@ function generateLabelsApplication(Application $application, ?ApplicationPreview noindex_domains: $noindexDomains, escape_redirect_replacement_for_compose: false, domainPortOverrides: $application->domain_port_overrides ?? [], - is_retry_enabled: true, certificate_resolver: $application->destination->server->traefikCertificateResolver($application->settings?->traefik_certificate_resolver), )); $labels = $labels->merge(fqdnLabelsForCaddy( @@ -1360,7 +1344,6 @@ function generateLabelsApplication(Application $application, ?ApplicationPreview supports_log_append: $application->destination->server->caddySupportsLogAppend(), supports_basic_auth_directive: $application->destination->server->caddySupportsBasicAuthDirective(), domainPortOverrides: $application->domain_port_overrides ?? [], - is_retry_enabled: true, )); } } diff --git a/tests/Feature/RollingUpdateProxyDrainTest.php b/tests/Feature/RollingUpdateProxyDrainTest.php index 4ad85d0d63..2ad2e355d3 100644 --- a/tests/Feature/RollingUpdateProxyDrainTest.php +++ b/tests/Feature/RollingUpdateProxyDrainTest.php @@ -110,27 +110,18 @@ function runRollingUpdateDrainStep(Application $application, bool $newVersionIsH return $logs; } -it('retries failed connections on another container in Traefik application labels', function (bool $exactLabels) { - $application = createApplicationForRollingUpdateDrainTest(ProxyTypes::TRAEFIK->value, $exactLabels); +it('keeps the application proxy labels of v4.5.0 without a retry, so a redeploy after an upgrade does not change them', function (string $proxyType, bool $exactLabels) { + $application = createApplicationForRollingUpdateDrainTest($proxyType, $exactLabels); - $labels = collect(generateLabelsApplication($application)); - - expect($labels)->toContain("traefik.http.middlewares.retry-{$application->uuid}.retry.attempts=5"); - $routerMiddlewares = $labels->filter(fn (string $label) => str_contains($label, "routers.https-0-{$application->uuid}.middlewares=")); - expect($routerMiddlewares)->toHaveCount(1) - ->and($routerMiddlewares->first())->toContain("retry-{$application->uuid}"); + expect(implode("\n", generateLabelsApplication($application))) + ->not->toContain('retry') + ->not->toContain('lb_try_duration'); })->with([ - 'generic labels' => false, - 'exact labels' => true, + 'Traefik generic labels' => [ProxyTypes::TRAEFIK->value, false], + 'Traefik exact labels' => [ProxyTypes::TRAEFIK->value, true], + 'Caddy exact labels' => [ProxyTypes::CADDY->value, true], ]); -it('retries failed connections on another upstream in Caddy application labels', function () { - $application = createApplicationForRollingUpdateDrainTest(ProxyTypes::CADDY->value, exactLabels: true); - - expect(generateLabelsApplication($application)) - ->toContain('caddy_0.handle_path.0_reverse_proxy.lb_try_duration=5s'); -}); - it('does not add retry labels to preview deployments', function () { $application = createApplicationForRollingUpdateDrainTest(); $preview = ApplicationPreview::forceCreate([ From 2dcf8eeca3b26fdec7b5b34591217b433d68c2f0 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 28 Oct 2026 12:10:27 +0100 Subject: [PATCH 6/8] fix(previews): keep production values for previews from before the upgrade Public pull request previews no longer get production values, and no preview falls back to production variables. Existing previews were deployed with these values; for example, their databases were initialized with the production password, so they broke on redeploy. Previews that exist at the upgrade get keeps_production_values and keep the earlier behavior. Previews created later do not. Server transfer moves the flag. --- app/Jobs/ApplicationDeploymentJob.php | 10 ++++++- app/Models/ApplicationPreview.php | 2 ++ .../ServerTransfer/ServerTransferExporter.php | 1 + .../ServerTransfer/ServerTransferImporter.php | 1 + bootstrap/helpers/parsers.php | 7 +++-- ...n_values_to_application_previews_table.php | 30 +++++++++++++++++++ docs/server-transfer-full-coverage.md | 2 +- ...ationDeploymentControlVarFilteringTest.php | 26 ++++++++++++++++ .../ComposePreviewMagicVariablesTest.php | 21 +++++++++++++ .../ServerTransferExporterImporterTest.php | 2 ++ 10 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 database/migrations/2026_10_28_103619_add_keeps_production_values_to_application_previews_table.php diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 944f9b3bb5..70931ece2e 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -2018,6 +2018,14 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue $envs->push($this->runtimeEnvironmentAssignment($env)); } + // A preview from before v4.5.1 also gets the production variables that have no preview copy. + if ($this->preview?->keeps_production_values && $runtime_environment_variables_preview->isNotEmpty()) { + $previewKeys = $runtime_environment_variables_preview->pluck('key')->all(); + $sorted_environment_variables + ->filter(fn (EnvironmentVariable $env) => $env->is_runtime && ! in_array($env->key, $previewKeys, true) && ! $this->shouldOmitBlankComposeEnvironmentVariable($env)) + ->each(fn (EnvironmentVariable $env) => $envs->push($this->runtimeEnvironmentAssignment($env))); + } + // Add PORT if not exists, use the first port as default if ($this->build_pack !== 'dockercompose') { if ($this->deployedPreviewVariables()->where('key', 'PORT')->isEmpty()) { @@ -2619,7 +2627,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return $this->previewVariableIdsWithProductionValues; } - if ($this->pull_request_id === 0 || ! $this->application->settings->is_pr_deployments_public_enabled) { + if ($this->pull_request_id === 0 || ! $this->application->settings->is_pr_deployments_public_enabled || $this->preview?->keeps_production_values) { return $this->previewVariableIdsWithProductionValues = []; } diff --git a/app/Models/ApplicationPreview.php b/app/Models/ApplicationPreview.php index 22b1856814..2309fb128e 100644 --- a/app/Models/ApplicationPreview.php +++ b/app/Models/ApplicationPreview.php @@ -34,6 +34,7 @@ class ApplicationPreview extends BaseModel 'last_online_at', 'domain_dns_statuses', 'domain_port_overrides', + 'keeps_production_values', ]; protected $hidden = [ @@ -44,6 +45,7 @@ class ApplicationPreview extends BaseModel 'pull_request_id' => 'integer', 'domain_dns_statuses' => 'array', 'domain_port_overrides' => 'array', + 'keeps_production_values' => 'boolean', ]; protected static function booted(): void diff --git a/app/Services/ServerTransfer/ServerTransferExporter.php b/app/Services/ServerTransfer/ServerTransferExporter.php index 66254ae647..20957defe6 100644 --- a/app/Services/ServerTransfer/ServerTransferExporter.php +++ b/app/Services/ServerTransfer/ServerTransferExporter.php @@ -1644,6 +1644,7 @@ class ServerTransferExporter 'docker_registry_image_tag' => $preview->docker_registry_image_tag, 'domain_port_overrides' => $preview->domain_port_overrides, 'max_restart_count' => $preview->max_restart_count, + 'keeps_production_values' => $preview->keeps_production_values, 'persistent_storages' => method_exists($preview, 'persistentStorages') ? $this->exportPersistentStorages($preview->persistentStorages) : [], diff --git a/app/Services/ServerTransfer/ServerTransferImporter.php b/app/Services/ServerTransfer/ServerTransferImporter.php index 7362160471..1cc4aed13e 100644 --- a/app/Services/ServerTransfer/ServerTransferImporter.php +++ b/app/Services/ServerTransfer/ServerTransferImporter.php @@ -2333,6 +2333,7 @@ class ServerTransferImporter 'docker_registry_image_tag' => data_get($previewPayload, 'docker_registry_image_tag'), 'domain_port_overrides' => data_get($previewPayload, 'domain_port_overrides'), 'max_restart_count' => (int) data_get($previewPayload, 'max_restart_count', 0), + 'keeps_production_values' => (bool) data_get($previewPayload, 'keeps_production_values', false), ]; if ($preview) { diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 041e56ff0a..7aa5ef279e 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -1150,8 +1150,11 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $pullRequestId = $pull_request_id; $isPullRequest = $pullRequestId == 0 ? false : true; - // A public preview can run the compose file of a fork pull request. - $isPublicPullRequest = $isPullRequest && (bool) data_get($resource, 'settings.is_pr_deployments_public_enabled'); + $pullRequestPreview = $isPullRequest + ? ($resource->previews()->find($preview_id) ?? ApplicationPreview::where('application_id', $resource->id)->where('pull_request_id', $pullRequestId)->first()) + : null; + // A public preview can run the compose file of a fork pull request. A preview from before v4.5.1 keeps the production values. + $isPublicPullRequest = $isPullRequest && (bool) data_get($resource, 'settings.is_pr_deployments_public_enabled') && ! $pullRequestPreview?->keeps_production_values; $server = data_get($resource, 'destination.server'); try { $yaml = parseDockerComposeYaml($compose); diff --git a/database/migrations/2026_10_28_103619_add_keeps_production_values_to_application_previews_table.php b/database/migrations/2026_10_28_103619_add_keeps_production_values_to_application_previews_table.php new file mode 100644 index 0000000000..3b187d1502 --- /dev/null +++ b/database/migrations/2026_10_28_103619_add_keeps_production_values_to_application_previews_table.php @@ -0,0 +1,30 @@ +boolean('keeps_production_values')->default(false); + }); + + DB::table('application_previews')->update(['keeps_production_values' => true]); + } + + public function down(): void + { + Schema::table('application_previews', function (Blueprint $table) { + $table->dropColumn('keeps_production_values'); + }); + } +}; diff --git a/docs/server-transfer-full-coverage.md b/docs/server-transfer-full-coverage.md index 25a7835e03..3391120322 100644 --- a/docs/server-transfer-full-coverage.md +++ b/docs/server-transfer-full-coverage.md @@ -103,7 +103,7 @@ When you add a model or a setting to the transfer, add it to this file too. | ✅ | Volume backup | on the persistent volume: S3 on, stop during backup, all retention values, timeout, missing-backup days | | ✅ | Scheduled tasks | the seed has one; add a disabled one with container + timeout | | ✅ | Tag | | -| ✅ | Preview deployment | PR preview with domain port overrides + max restart count (needs the GitHub App, or create it in the database) | +| ✅ | Preview deployment | PR preview with domain port overrides + max restart count + keeps production values (previews from before v4.5.1) (needs the GitHub App, or create it in the database) | | ✅ | Secret manager link | the app loads variables from the secret manager token | | ✅ | Managed DNS record | owned record in the Cloudflare zone for the app domain. Expected: the target takes ownership (provider comment changes) | diff --git a/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php b/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php index caf92c69e4..4abe239679 100644 --- a/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php +++ b/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php @@ -2050,3 +2050,29 @@ it('does not warn about a PORT mismatch when readonly labels are disabled', func expect(collect($job->recordedLogEntries)->contains(fn (string $entry) => str_contains($entry, 'PORT environment variable')))->toBeFalse(); }); + +it('gives a preview from before v4.5.1 the production values it had', function () { + [$application, $server] = makeDeploymentControlVarFixture(); + $application->settings()->update(['is_pr_deployments_public_enabled' => true]); + + createApplicationEnvironmentVariable($application, ['key' => 'DB_PASSWORD', 'value' => 'production-secret', 'is_buildtime' => false]); + createApplicationEnvironmentVariable($application, ['key' => 'APP_NAME', 'value' => 'coolify', 'is_buildtime' => false]); + $application->environment_variables_preview()->where('key', 'APP_NAME')->delete(); + + [$job, $reflection] = makeControlVarFilteringJob($application->fresh(), $server, [ + 'pull_request_id' => 99, + 'remote_secrets_cache' => [], + 'preview' => ApplicationPreview::create([ + 'application_id' => $application->id, + 'pull_request_id' => 99, + 'pull_request_html_url' => 'https://example.com/pr/99', + 'keeps_production_values' => true, + ]), + ]); + + /** @var Collection $runtimeEnvs */ + $runtimeEnvs = invokeDeploymentJobMethod($job, $reflection, 'generate_runtime_environment_variables'); + + expect($runtimeEnvs->contains(fn (string $env) => str($env)->startsWith('DB_PASSWORD=') && str($env)->contains('production-secret')))->toBeTrue() + ->and($runtimeEnvs->contains(fn (string $env) => str($env)->startsWith('APP_NAME=') && str($env)->contains('coolify')))->toBeTrue(); +}); diff --git a/tests/Feature/ComposePreviewMagicVariablesTest.php b/tests/Feature/ComposePreviewMagicVariablesTest.php index fcdf777695..d7c393c011 100644 --- a/tests/Feature/ComposePreviewMagicVariablesTest.php +++ b/tests/Feature/ComposePreviewMagicVariablesTest.php @@ -215,3 +215,24 @@ it('replaces an existing preview copy of a production magic value for a public p ->not->toBeEmpty() ->not->toBe($production['SERVICE_PASSWORD_DB']); }); + +it('keeps the production magic value of a public pull request preview from before v4.5.1', function () { + composePreviewMagicParse($this->application); + $production = composePreviewMagicValues($this->application)['production']; + $this->application->environment_variables_preview()->create([ + 'key' => 'SERVICE_PASSWORD_DB', + 'value' => $production['SERVICE_PASSWORD_DB'], + 'is_preview' => true, + ]); + ApplicationPreview::query()->create([ + 'application_id' => $this->application->id, + 'pull_request_id' => 7, + 'pull_request_html_url' => 'https://github.com/example/repository/pull/7', + 'keeps_production_values' => true, + ]); + $this->application->settings->update(['is_pr_deployments_public_enabled' => true]); + + composePreviewMagicParse($this->application, 7); + + expect(composePreviewMagicValues($this->application)['preview']['SERVICE_PASSWORD_DB'])->toBe($production['SERVICE_PASSWORD_DB']); +}); diff --git a/tests/Unit/ServerTransfer/ServerTransferExporterImporterTest.php b/tests/Unit/ServerTransfer/ServerTransferExporterImporterTest.php index 63f416eb6b..c5a55af722 100644 --- a/tests/Unit/ServerTransfer/ServerTransferExporterImporterTest.php +++ b/tests/Unit/ServerTransfer/ServerTransferExporterImporterTest.php @@ -859,6 +859,7 @@ test('service nested apps dbs volumes backups and db file storages round-trip', 'fqdn' => 'https://pr-42.app.example.com', 'status' => 'running:healthy', 'git_type' => 'github', + 'keeps_production_values' => true, ]); $preview->uuid = 'preview-pr-42'; $preview->save(); @@ -969,6 +970,7 @@ test('service nested apps dbs volumes backups and db file storages round-trip', expect($importedApp)->not->toBeNull() ->and($importedApp->previews)->toHaveCount(1) ->and($importedApp->previews->first()->uuid)->toBe('preview-pr-42') + ->and($importedApp->previews->first()->keeps_production_values)->toBeTrue() ->and($importedApp->previews->first()->persistentStorages)->toHaveCount(1); $importedVolBackup = ScheduledVolumeBackup::where('uuid', 'svc-app-vol-backup')->first(); From 9aca61fbd6ec5d582f0731fd7a4ee2f72294f67c Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 28 Oct 2026 12:10:27 +0100 Subject: [PATCH 7/8] fix(ssh): start mux masters with a command that exits When another process held the control socket, `ssh -fN` disabled multiplexing and stayed open as a plain connection forever. Each one kept an sshd process on the server, which used up its memory. Run `true` instead of -fN. ControlPersist moves a new master to the background; a process that cannot become the master exits. --- app/Helpers/SshMultiplexingHelper.php | 7 +++- tests/Feature/SshMultiplexingLockTest.php | 47 ++++++++++++++--------- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/app/Helpers/SshMultiplexingHelper.php b/app/Helpers/SshMultiplexingHelper.php index 86034b8cbd..0fe9caaff2 100644 --- a/app/Helpers/SshMultiplexingHelper.php +++ b/app/Helpers/SshMultiplexingHelper.php @@ -95,14 +95,17 @@ class SshMultiplexingHelper $serverInterval = config('constants.ssh.server_interval'); $muxPersistTime = config('constants.ssh.mux_persist_time'); - $establishCommand = 'ssh -fN -o ControlMaster=auto -o ControlPath='.escapeshellarg($muxSocket)." -o ControlPersist={$muxPersistTime} "; + // Not `ssh -fN`: when another process holds the control socket, ssh disables multiplexing and an + // `-fN` connection then stays open forever. This form runs `true` and exits. Only a master that + // ControlPersist moves to the background stays. + $establishCommand = 'ssh -o ControlMaster=auto -o ControlPath='.escapeshellarg($muxSocket)." -o ControlPersist={$muxPersistTime} "; if (data_get($server, 'settings.is_cloudflare_tunnel')) { $establishCommand .= ' -o ProxyCommand="cloudflared access ssh --hostname %h" '; } $establishCommand .= self::getCommonSshOptions($server, $sshKeyLocation, $connectionTimeout, $serverInterval); - $establishCommand .= self::escapedUserAtHost($server); + $establishCommand .= self::escapedUserAtHost($server).' true'; $establishProcess = Process::run($establishCommand); if ($establishProcess->exitCode() !== 0) { diff --git a/tests/Feature/SshMultiplexingLockTest.php b/tests/Feature/SshMultiplexingLockTest.php index 7f01a543f1..b71ac3c45a 100644 --- a/tests/Feature/SshMultiplexingLockTest.php +++ b/tests/Feature/SshMultiplexingLockTest.php @@ -78,19 +78,28 @@ function muxProcessTimeout(array &$timeouts = []): Closure }; } -it('establishes a master with ssh -fN and never the orphan-prone ssh -fNM', function () { +/** + * The command that starts a mux master. It runs `true`, so it exits when another process owns the socket. + */ +function isMuxEstablishCommand(string $command): bool +{ + return str_starts_with($command, 'ssh -o ControlMaster=auto ') && str_ends_with($command, ' true'); +} + +it('establishes a master with a command that exits, never with an ssh -fN that can stay open forever', function () { config(['constants.ssh.mux_enabled' => true]); $server = makeMuxServer(); Process::fake([ '*-O check*' => Process::result(exitCode: 1), - '*-fN *' => Process::result(exitCode: 0), + '* true' => Process::result(exitCode: 0), ]); expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue(); - Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ') - && ! str_contains($process->command, 'ssh -fNM')); + Process::assertRan(fn ($process) => isMuxEstablishCommand($process->command) + && str_contains($process->command, 'ControlPersist=') + && ! str_contains($process->command, '-fN')); }); it('reuses an existing healthy master without spawning a new one', function () { @@ -107,7 +116,7 @@ it('reuses an existing healthy master without spawning a new one', function () { expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue(); - Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -fN')); + Process::assertNotRan(fn ($process) => isMuxEstablishCommand($process->command)); }); it('reuses a healthy master regardless of its absolute age', function () { @@ -125,7 +134,7 @@ it('reuses a healthy master regardless of its absolute age', function () { expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue(); Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -O stop')); - Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); + Process::assertNotRan(fn ($process) => isMuxEstablishCommand($process->command)); }); it('bounds mux check commands so a wedged master cannot block callers', function () { @@ -137,7 +146,7 @@ it('bounds mux check commands so a wedged master cannot block callers', function Process::fake([ '*-O check*' => Process::result(exitCode: 1), - '*-fN *' => Process::result(exitCode: 0), + '* true' => Process::result(exitCode: 0), ]); SshMultiplexingHelper::ensureMultiplexedConnection($server); @@ -158,13 +167,13 @@ it('replaces a wedged master whose control check times out', function () { $timeouts = []; Process::fake([ '*-O check*' => muxProcessTimeout($timeouts), - '*-fN *' => Process::result(exitCode: 0), + '* true' => Process::result(exitCode: 0), ]); expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue() ->and($timeouts)->each->toBe(5); - Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); + Process::assertRan(fn ($process) => isMuxEstablishCommand($process->command)); Process::assertNotRan(fn ($process) => str_contains($process->command, '-O stop')); }); @@ -175,7 +184,7 @@ it('does not delete the control socket when the master is simply absent', functi File::partialMock()->shouldNotReceive('delete'); Process::fake([ '*-O check*' => Process::result(exitCode: 255), - '*-fN *' => Process::result(exitCode: 0), + '* true' => Process::result(exitCode: 0), ]); expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue(); @@ -206,13 +215,13 @@ it('replaces a master whose health check times out', function () { '*-O check*' => Process::result(output: 'Master running (pid=222)', exitCode: 0), '*health_check_ok*' => muxProcessTimeout(), '*-O stop*' => Process::result(exitCode: 0), - '*-fN *' => Process::result(exitCode: 0), + '* true' => Process::result(exitCode: 0), ]); expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue(); Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -O stop')); - Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); + Process::assertRan(fn ($process) => isMuxEstablishCommand($process->command)); }); it('gives the mux health check at least the server connection timeout', function () { @@ -252,7 +261,7 @@ it('does not spawn a master when the per-server lock is already held', function expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeFalse(); - Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); + Process::assertNotRan(fn ($process) => isMuxEstablishCommand($process->command)); $held->release(); }); @@ -274,7 +283,7 @@ it('adds mux options to ssh commands only after the explicit master is ready', f Process::fake([ '*-O check*' => Process::result(exitCode: 1), - '*-fN *' => Process::result(exitCode: 0), + '* true' => Process::result(exitCode: 0), ]); $command = SshMultiplexingHelper::generateSshCommand($server, 'echo ok'); @@ -286,7 +295,7 @@ it('adds mux options to ssh commands only after the explicit master is ready', f ->toContain("'sh -c \"if command -v bash >/dev/null 2>&1; then exec bash -se; else exec sh -se; fi\"' << \\") ->not->toContain('<< $delimiter'); - Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); + Process::assertRan(fn ($process) => isMuxEstablishCommand($process->command)); }); it('quotes the mux socket and key paths in every local ssh command', function () { @@ -300,7 +309,7 @@ it('quotes the mux socket and key paths in every local ssh command', function () Process::fake([ '*-O check*' => Process::result(exitCode: 1), - '*-fN *' => Process::result(exitCode: 0), + '* true' => Process::result(exitCode: 0), ]); $socket = escapeshellarg('/var/www/html/storage/app/ssh/mux/mux_x;touch /tmp/pwn;#'); @@ -312,7 +321,7 @@ it('quotes the mux socket and key paths in every local ssh command', function () foreach ([$sshCommand, $scpCommand] as $command) { expect($command)->toContain("-o ControlPath={$socket} ")->toContain("-i {$key} "); } - Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ') + Process::assertRan(fn ($process) => isMuxEstablishCommand($process->command) && str_contains($process->command, "-o ControlPath={$socket} ") && str_contains($process->command, "-i {$key} ")); Process::assertRan(fn ($process) => str_contains($process->command, "ssh -O check -o ControlPath={$socket} ")); @@ -354,7 +363,7 @@ it('adds mux options to scp commands only after the explicit master is ready', f Process::fake([ '*-O check*' => Process::result(exitCode: 1), - '*-fN *' => Process::result(exitCode: 0), + '* true' => Process::result(exitCode: 0), ]); $command = SshMultiplexingHelper::generateScpCommand($server, '/tmp/source', '/tmp/dest'); @@ -364,7 +373,7 @@ it('adds mux options to scp commands only after the explicit master is ready', f ->toContain("-o ControlPath='/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}'") ->toContain('-o ControlPersist=3600'); - Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); + Process::assertRan(fn ($process) => isMuxEstablishCommand($process->command)); }); it('kills only old orphaned ssh masters whose control socket no longer exists', function () { From 7e6de3872fd74b280ccd24ac197402b95059638d Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 28 Oct 2026 12:16:17 +0100 Subject: [PATCH 8/8] chore(release): bump Coolify version to 4.5.2 --- config/constants.php | 2 +- other/nightly/versions.json | 2 +- versions.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/constants.php b/config/constants.php index 744eaf1b1f..e3046cf1dc 100644 --- a/config/constants.php +++ b/config/constants.php @@ -5,7 +5,7 @@ $sshCommandTimeout = (int) env('SSH_COMMAND_TIMEOUT', 3600); return [ 'coolify' => [ - 'version' => env('COOLIFY_VERSION') ?: '4.5.1', + 'version' => env('COOLIFY_VERSION') ?: '4.5.2', 'helper_version' => '1.0.19', 'railpack_version' => '0.43.0', 'self_hosted' => env('SELF_HOSTED', true), diff --git a/other/nightly/versions.json b/other/nightly/versions.json index 3c8c027b57..7fa9bf794c 100644 --- a/other/nightly/versions.json +++ b/other/nightly/versions.json @@ -1,7 +1,7 @@ { "coolify": { "v4": { - "version": "4.5.1" + "version": "4.5.2" }, "nightly": { "version": "4.6-rc.1" diff --git a/versions.json b/versions.json index ba18416349..c280fee84d 100644 --- a/versions.json +++ b/versions.json @@ -1,7 +1,7 @@ { "coolify": { "v4": { - "version": "4.5.1" + "version": "4.5.2" }, "nightly": { "version": "4.6-rc.1"