From 4227ec6ebfb2d9cfe19fb66bab8e15eae06e4dc3 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 12:03:37 +0200 Subject: [PATCH] fix(storage): require deploy permission for external host writes Require deploy or root token access when creating managed mounts outside resource directories or changing their file contents through the API. Synchronize KeyDB and Dragonfly passwords with local and shared REDIS_PASSWORD values while preserving secret manager resolution. --- app/Actions/Database/StartDragonfly.php | 9 +- app/Actions/Database/StartKeydb.php | 9 +- .../Api/ApplicationsController.php | 31 +++- .../RequiresDeployForOutsideHostPaths.php | 41 +++++ .../Controllers/Api/DatabasesController.php | 31 +++- .../Controllers/Api/ServicesController.php | 31 +++- app/Http/Controllers/Webhook/Github.php | 13 +- app/Jobs/ApplicationDeploymentJob.php | 2 +- app/Jobs/ScheduledJobManager.php | 8 +- app/Jobs/VolumeBackupRecoveryJob.php | 54 +++---- .../Project/Shared/Storages/VolumeBackups.php | 2 - app/Models/GithubRunnerExecution.php | 62 ++++++++ app/Models/ScheduledVolumeBackupExecution.php | 4 - .../VolumeBackup/RecoveryFailed.php | 134 ++++++++++++++++ bootstrap/helpers/parsers.php | 76 +++++---- ...heduled_volume_backup_executions_table.php | 6 +- .../volume-backup-recovery-failed.blade.php | 7 + ...licationComposeInjectionValidationTest.php | 21 +++ tests/Feature/GithubRunnerFlowTest.php | 148 ++++++++++++++++++ ...eydbDragonflyLegacyPasswordQuotingTest.php | 73 ++++++++- tests/Feature/MultiServerApplicationTest.php | 7 + tests/Feature/StorageHostPathTest.php | 90 ++++++++++- .../VolumeBackupRecoveryBoundsTest.php | 90 ++++++----- tests/Feature/VolumeBackupTest.php | 1 + ...ockerComposeInjectionCompatibilityTest.php | 41 +++++ 25 files changed, 863 insertions(+), 128 deletions(-) create mode 100644 app/Http/Controllers/Api/Concerns/RequiresDeployForOutsideHostPaths.php create mode 100644 app/Notifications/VolumeBackup/RecoveryFailed.php create mode 100644 resources/views/emails/volume-backup-recovery-failed.blade.php diff --git a/app/Actions/Database/StartDragonfly.php b/app/Actions/Database/StartDragonfly.php index 20f69bb715..8327cf2dfe 100644 --- a/app/Actions/Database/StartDragonfly.php +++ b/app/Actions/Database/StartDragonfly.php @@ -251,14 +251,21 @@ class StartDragonfly $environment_variables = collect(); $this->resolvedRedisPassword = (string) $this->database->dragonfly_password; 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 = $this->database->environmentVariableUsesSecretManager($env); + $this->redisPasswordFromSecretManager = $usesSecretManager; } } diff --git a/app/Actions/Database/StartKeydb.php b/app/Actions/Database/StartKeydb.php index f6c2ff0bc7..6bd3c4b236 100644 --- a/app/Actions/Database/StartKeydb.php +++ b/app/Actions/Database/StartKeydb.php @@ -251,14 +251,21 @@ class StartKeydb $environment_variables = collect(); $this->resolvedRedisPassword = (string) $this->database->keydb_password; 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 = $this->database->environmentVariableUsesSecretManager($env); + $this->redisPasswordFromSecretManager = $usesSecretManager; } } diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index b2d84113f2..238a318ce3 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -40,6 +40,7 @@ use Symfony\Component\Yaml\Yaml; class ApplicationsController extends Controller { use Concerns\HandlesTagsApi; + use Concerns\RequiresDeployForOutsideHostPaths; private const APPLICATION_SETTING_FIELDS = [ 'is_git_submodules_enabled', @@ -5042,7 +5043,7 @@ class ApplicationsController extends Controller 'is_preview_suffix_enabled' => ['type' => 'boolean', 'description' => 'Whether to add -pr-N suffix for preview deployments.'], 'name' => ['type' => 'string', 'description' => 'The volume name (persistent only, not allowed for read-only storages).'], 'mount_path' => ['type' => 'string', 'description' => 'The container mount path (not allowed for read-only storages).'], - 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages).'], + 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission.'], ], additionalProperties: false, ), @@ -5067,6 +5068,10 @@ class ApplicationsController extends Controller response: 404, ref: '#/components/responses/404', ), + new OA\Response( + response: 403, + description: 'The token needs the deploy permission to change the content of a file outside the resource directory.', + ), new OA\Response( response: 422, ref: '#/components/responses/422', @@ -5173,6 +5178,10 @@ class ApplicationsController extends Controller } } + if (! $isReadOnly && ($forbidden = $this->outsideContentChangeForbiddenResponse($request, $storage))) { + return $forbidden; + } + // Always allowed if ($request->has('is_preview_suffix_enabled')) { $storage->is_preview_suffix_enabled = $request->is_preview_suffix_enabled; @@ -5243,7 +5252,7 @@ class ApplicationsController extends Controller 'mount_path' => ['type' => 'string', 'description' => 'The container mount path.'], 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'File content (file only, optional).'], 'is_directory' => ['type' => 'boolean', 'description' => 'Whether this is a directory mount (file only, default false).'], - 'fs_path' => ['type' => 'string', 'description' => 'Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. Coolify never deletes a path outside the resource directory.'], + 'fs_path' => ['type' => 'string', 'description' => 'Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. A directory or file mount outside the resource directory needs a token with the deploy permission. Coolify never deletes a path outside the resource directory.'], ], additionalProperties: false, ), @@ -5259,6 +5268,7 @@ class ApplicationsController extends Controller new OA\Response(response: 401, ref: '#/components/responses/401'), new OA\Response(response: 400, ref: '#/components/responses/400'), new OA\Response(response: 404, ref: '#/components/responses/404'), + new OA\Response(response: 403, description: 'The token needs the deploy permission for a mount outside the resource directory.'), new OA\Response(response: 422, ref: '#/components/responses/422'), ] )] @@ -5369,6 +5379,15 @@ class ApplicationsController extends Controller try { $fsPath = LocalFileVolume::resolveHostPath(application_configuration_dir().'/'.$application->uuid, $request->fs_path, 'storage source path'); $mountPath = validateFileMountPath($request->mount_path, 'storage destination path'); + $forbidden = $this->outsideHostPathForbiddenResponse($request, new LocalFileVolume([ + 'fs_path' => $fsPath, + 'is_directory' => true, + 'resource_id' => $application->id, + 'resource_type' => get_class($application), + ])); + if ($forbidden) { + return $forbidden; + } LocalFileVolume::assertHostPathOnServer(application_configuration_dir().'/'.$application->uuid, $fsPath, $application->destination->server, isDirectory: true); } catch (\Throwable $e) { return response()->json([ @@ -5431,6 +5450,14 @@ class ApplicationsController extends Controller try { if ($request->filled('fs_path')) { $fsPath = LocalFileVolume::resolveHostPath(application_configuration_dir().'/'.$application->uuid, $request->fs_path, 'file storage source path'); + $forbidden = $this->outsideHostPathForbiddenResponse($request, new LocalFileVolume([ + 'fs_path' => $fsPath, + 'resource_id' => $application->id, + 'resource_type' => get_class($application), + ])); + if ($forbidden) { + return $forbidden; + } LocalFileVolume::assertHostPathOnServer(application_configuration_dir().'/'.$application->uuid, $fsPath, $application->destination->server, isDirectory: false); } else { $fsPath = confineFileMountPath(application_configuration_dir().'/'.$application->uuid, $mountPath, 'file storage path'); diff --git a/app/Http/Controllers/Api/Concerns/RequiresDeployForOutsideHostPaths.php b/app/Http/Controllers/Api/Concerns/RequiresDeployForOutsideHostPaths.php new file mode 100644 index 0000000000..90d3a136fa --- /dev/null +++ b/app/Http/Controllers/Api/Concerns/RequiresDeployForOutsideHostPaths.php @@ -0,0 +1,41 @@ +is_host_file || ! $storage->isOutsideResourceDirectory()) { + return null; + } + if ($request->user()->tokenCan('deploy') || $request->user()->tokenCan('root')) { + return null; + } + + return response()->json([ + 'message' => 'Missing required permissions: deploy. A mount outside the resource directory needs a token with the deploy permission.', + ], 403); + } + + private function outsideContentChangeForbiddenResponse(Request $request, LocalFileVolume|LocalPersistentVolume $storage): ?JsonResponse + { + if (! $storage instanceof LocalFileVolume || $storage->is_directory || ! $request->has('content')) { + return null; + } + if ((string) $request->input('content') === (string) $storage->content) { + return null; + } + + return $this->outsideHostPathForbiddenResponse($request, $storage); + } +} diff --git a/app/Http/Controllers/Api/DatabasesController.php b/app/Http/Controllers/Api/DatabasesController.php index 38c304f3f1..b32a16fc61 100644 --- a/app/Http/Controllers/Api/DatabasesController.php +++ b/app/Http/Controllers/Api/DatabasesController.php @@ -36,6 +36,7 @@ class DatabasesController extends Controller { use Concerns\HandlesDatabaseImportsApi; use Concerns\HandlesTagsApi; + use Concerns\RequiresDeployForOutsideHostPaths; #[OA\Post( path: '/databases/{uuid}/imports/uploads', @@ -4290,7 +4291,7 @@ class DatabasesController extends Controller 'mount_path' => ['type' => 'string', 'description' => 'The container mount path.'], 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'File content (file only, optional).'], 'is_directory' => ['type' => 'boolean', 'description' => 'Whether this is a directory mount (file only, default false).'], - 'fs_path' => ['type' => 'string', 'description' => 'Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. Coolify never deletes a path outside the resource directory.'], + 'fs_path' => ['type' => 'string', 'description' => 'Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. A directory or file mount outside the resource directory needs a token with the deploy permission. Coolify never deletes a path outside the resource directory.'], ], additionalProperties: false, ), @@ -4306,6 +4307,7 @@ class DatabasesController extends Controller new OA\Response(response: 401, ref: '#/components/responses/401'), new OA\Response(response: 400, ref: '#/components/responses/400'), new OA\Response(response: 404, ref: '#/components/responses/404'), + new OA\Response(response: 403, description: 'The token needs the deploy permission for a mount outside the resource directory.'), new OA\Response(response: 422, ref: '#/components/responses/422'), ] )] @@ -4412,6 +4414,15 @@ class DatabasesController extends Controller try { $fsPath = LocalFileVolume::resolveHostPath(database_configuration_dir().'/'.$database->uuid, $request->fs_path, 'storage source path'); $mountPath = validateFileMountPath($request->mount_path, 'storage destination path'); + $forbidden = $this->outsideHostPathForbiddenResponse($request, new LocalFileVolume([ + 'fs_path' => $fsPath, + 'is_directory' => true, + 'resource_id' => $database->id, + 'resource_type' => get_class($database), + ])); + if ($forbidden) { + return $forbidden; + } LocalFileVolume::assertHostPathOnServer(database_configuration_dir().'/'.$database->uuid, $fsPath, $database->destination->server, isDirectory: true); } catch (\Throwable $e) { return response()->json([ @@ -4474,6 +4485,14 @@ class DatabasesController extends Controller try { if ($request->filled('fs_path')) { $fsPath = LocalFileVolume::resolveHostPath(database_configuration_dir().'/'.$database->uuid, $request->fs_path, 'file storage source path'); + $forbidden = $this->outsideHostPathForbiddenResponse($request, new LocalFileVolume([ + 'fs_path' => $fsPath, + 'resource_id' => $database->id, + 'resource_type' => get_class($database), + ])); + if ($forbidden) { + return $forbidden; + } LocalFileVolume::assertHostPathOnServer(database_configuration_dir().'/'.$database->uuid, $fsPath, $database->destination->server, isDirectory: false); } else { $fsPath = confineFileMountPath(database_configuration_dir().'/'.$database->uuid, $mountPath, 'file storage path'); @@ -4543,7 +4562,7 @@ class DatabasesController extends Controller 'is_preview_suffix_enabled' => ['type' => 'boolean', 'description' => 'Whether to add -pr-N suffix for preview deployments.'], 'name' => ['type' => 'string', 'description' => 'The volume name (persistent only, not allowed for read-only storages).'], 'mount_path' => ['type' => 'string', 'description' => 'The container mount path (not allowed for read-only storages).'], - 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages).'], + 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission.'], ], additionalProperties: false, ), @@ -4568,6 +4587,10 @@ class DatabasesController extends Controller response: 404, ref: '#/components/responses/404', ), + new OA\Response( + response: 403, + description: 'The token needs the deploy permission to change the content of a file outside the resource directory.', + ), new OA\Response( response: 422, ref: '#/components/responses/422', @@ -4671,6 +4694,10 @@ class DatabasesController extends Controller } } + if (! $isReadOnly && ($forbidden = $this->outsideContentChangeForbiddenResponse($request, $storage))) { + return $forbidden; + } + // Always allowed if ($request->has('is_preview_suffix_enabled')) { $storage->is_preview_suffix_enabled = $request->is_preview_suffix_enabled; diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index 7ad3684f43..521e848a88 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -31,6 +31,7 @@ use Symfony\Component\Yaml\Yaml; class ServicesController extends Controller { use Concerns\HandlesTagsApi; + use Concerns\RequiresDeployForOutsideHostPaths; protected function findTaggableResource(string $uuid, int|string $teamId): mixed { @@ -2476,7 +2477,7 @@ class ServicesController extends Controller 'mount_path' => ['type' => 'string', 'description' => 'The container mount path.'], 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'File content (file only, optional).'], 'is_directory' => ['type' => 'boolean', 'description' => 'Whether this is a directory mount (file only, default false).'], - 'fs_path' => ['type' => 'string', 'description' => 'Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. Coolify never deletes a path outside the resource directory.'], + 'fs_path' => ['type' => 'string', 'description' => 'Host path. Required for directory mounts and host file mounts. Optional for file mounts with content (default: inside the resource directory). An absolute path can be anywhere on the host; a relative path is inside the resource directory. A directory or file mount outside the resource directory needs a token with the deploy permission. Coolify never deletes a path outside the resource directory.'], ], additionalProperties: false, ), @@ -2492,6 +2493,7 @@ class ServicesController extends Controller new OA\Response(response: 401, ref: '#/components/responses/401'), new OA\Response(response: 400, ref: '#/components/responses/400'), new OA\Response(response: 404, ref: '#/components/responses/404'), + new OA\Response(response: 403, description: 'The token needs the deploy permission for a mount outside the resource directory.'), new OA\Response(response: 422, ref: '#/components/responses/422'), ] )] @@ -2607,6 +2609,15 @@ class ServicesController extends Controller try { $fsPath = LocalFileVolume::resolveHostPath(service_configuration_dir().'/'.$service->uuid, $request->fs_path, 'storage source path'); $mountPath = validateFileMountPath($request->mount_path, 'storage destination path'); + $forbidden = $this->outsideHostPathForbiddenResponse($request, new LocalFileVolume([ + 'fs_path' => $fsPath, + 'is_directory' => true, + 'resource_id' => $subResource->id, + 'resource_type' => get_class($subResource), + ])); + if ($forbidden) { + return $forbidden; + } LocalFileVolume::assertHostPathOnServer(service_configuration_dir().'/'.$service->uuid, $fsPath, $service->server, isDirectory: true); } catch (\Throwable $e) { return response()->json([ @@ -2669,6 +2680,14 @@ class ServicesController extends Controller try { if ($request->filled('fs_path')) { $fsPath = LocalFileVolume::resolveHostPath(service_configuration_dir().'/'.$service->uuid, $request->fs_path, 'file storage source path'); + $forbidden = $this->outsideHostPathForbiddenResponse($request, new LocalFileVolume([ + 'fs_path' => $fsPath, + 'resource_id' => $subResource->id, + 'resource_type' => get_class($subResource), + ])); + if ($forbidden) { + return $forbidden; + } LocalFileVolume::assertHostPathOnServer(service_configuration_dir().'/'.$service->uuid, $fsPath, $service->server, isDirectory: false); } else { $fsPath = confineFileMountPath(service_configuration_dir().'/'.$service->uuid, $mountPath, 'file storage path'); @@ -2738,7 +2757,7 @@ class ServicesController extends Controller 'is_preview_suffix_enabled' => ['type' => 'boolean', 'description' => 'Whether to add -pr-N suffix for preview deployments.'], 'name' => ['type' => 'string', 'description' => 'The volume name (persistent only, not allowed for read-only storages).'], 'mount_path' => ['type' => 'string', 'description' => 'The container mount path (not allowed for read-only storages).'], - 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages).'], + 'content' => ['type' => 'string', 'nullable' => true, 'description' => 'The file content (file only, not allowed for read-only storages). Changing the content of a file outside the resource directory needs the deploy permission.'], ], additionalProperties: false, ), @@ -2763,6 +2782,10 @@ class ServicesController extends Controller response: 404, ref: '#/components/responses/404', ), + new OA\Response( + response: 403, + description: 'The token needs the deploy permission to change the content of a file outside the resource directory.', + ), new OA\Response( response: 422, ref: '#/components/responses/422', @@ -2896,6 +2919,10 @@ class ServicesController extends Controller } } + if (! $isReadOnly && ($forbidden = $this->outsideContentChangeForbiddenResponse($request, $storage))) { + return $forbidden; + } + // Always allowed if ($request->has('is_preview_suffix_enabled')) { $storage->is_preview_suffix_enabled = $request->is_preview_suffix_enabled; diff --git a/app/Http/Controllers/Webhook/Github.php b/app/Http/Controllers/Webhook/Github.php index 1dec6b71d9..8f44152306 100644 --- a/app/Http/Controllers/Webhook/Github.php +++ b/app/Http/Controllers/Webhook/Github.php @@ -316,10 +316,15 @@ class Github extends Controller ? GithubRunnerExecution::query()->where('github_app_id', $githubApp->id)->where('runner_name', $runnerName)->first() : null; - if ($action === 'in_progress' && $execution && in_array($execution->status, [GithubRunnerStatus::Provisioning, GithubRunnerStatus::Idle], true)) { - $execution->update([...$jobDetails, 'status' => GithubRunnerStatus::Running, 'started_at' => now()]); - - return response('Runner marked running.'); + // The completed hook can arrive before the in_progress hook. + if (in_array($action, ['in_progress', 'completed'], true) && $execution && in_array($execution->status, [GithubRunnerStatus::Provisioning, GithubRunnerStatus::Idle], true)) { + $replacement = $execution->assignJob($jobId, $jobDetails); + if ($replacement) { + ProvisionGithubRunnerJob::dispatch($replacement->id); + } + if ($action === 'in_progress') { + return response('Runner marked running.'); + } } if ($action === 'completed') { diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 7ffafc33c8..3379b267d4 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -2265,7 +2265,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue */ private function ensureRegistryImageForMultipleServers(): void { - if ($this->pull_request_id !== 0) { + if ($this->pull_request_id !== 0 || $this->application->build_pack === 'dockercompose') { return; } if ($this->application->additional_servers()->doesntExist()) { diff --git a/app/Jobs/ScheduledJobManager.php b/app/Jobs/ScheduledJobManager.php index e1411412fb..4c3beb97cd 100644 --- a/app/Jobs/ScheduledJobManager.php +++ b/app/Jobs/ScheduledJobManager.php @@ -497,15 +497,17 @@ class ScheduledJobManager implements ShouldQueue ->where('stop_recovery_pending', true) ->orWhere('s3_cleanup_pending', true)) ->where('recovery_needs_attention', false) - ->where(fn (Builder $query) => $query - ->whereNull('recovery_next_retry_at') - ->orWhere('recovery_next_retry_at', '<=', now())) + ->with('scheduledVolumeBackup.backupable.resource') ->chunkById(self::CHUNK_SIZE, function ($executions) use (&$dispatched): bool { foreach ($executions as $execution) { if ($dispatched >= self::VOLUME_BACKUP_RECOVERY_MAX_DISPATCHES) { return false; } + if ($execution->stop_recovery_pending && $execution->scheduledVolumeBackup?->server()?->isFunctional() === false) { + continue; + } + if (Cache::add(VolumeBackupRecoveryJob::dispatchCacheKey($execution->id), true, now()->addMinutes(self::VOLUME_BACKUP_RECOVERY_INTERVAL_MINUTES))) { VolumeBackupRecoveryJob::dispatch($execution); $dispatched++; diff --git a/app/Jobs/VolumeBackupRecoveryJob.php b/app/Jobs/VolumeBackupRecoveryJob.php index 7f475d24ce..bd3310712e 100644 --- a/app/Jobs/VolumeBackupRecoveryJob.php +++ b/app/Jobs/VolumeBackupRecoveryJob.php @@ -3,6 +3,7 @@ namespace App\Jobs; use App\Models\ScheduledVolumeBackupExecution; +use App\Notifications\VolumeBackup\RecoveryFailed; use Aws\S3\Exception\S3Exception; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldBeEncrypted; @@ -21,7 +22,7 @@ class VolumeBackupRecoveryJob implements ShouldBeEncrypted, ShouldBeUnique, Shou use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; /** - * ScheduledJobManager dispatches pending recovery again after a backoff, so this job does not retry by itself. + * ScheduledJobManager dispatches recovery again once an unreachable server is functional, so this job does not retry by itself. */ public int $tries = 1; @@ -29,12 +30,6 @@ class VolumeBackupRecoveryJob implements ShouldBeEncrypted, ShouldBeUnique, Shou public int $uniqueFor = 300; - public const MAX_ATTEMPTS = 8; - - public const BACKOFF_BASE_MINUTES = 5; - - public const BACKOFF_CAP_MINUTES = 360; - public const CATEGORY_S3_AUTH = 's3_auth'; public const CATEGORY_S3_BUCKET = 's3_bucket'; @@ -86,8 +81,8 @@ class VolumeBackupRecoveryJob implements ShouldBeEncrypted, ShouldBeUnique, Shou } /** - * Runs one recovery attempt and records its outcome instead of throwing, so a persistent failure backs off - * and eventually needs attention rather than creating a failed job every few minutes. + * Runs one recovery attempt and records its outcome instead of throwing. An unreachable server waits for the + * server to be functional again; any other failure needs attention and notifies the team. */ public static function recoverWithBounds(ScheduledVolumeBackupExecution $execution): void { @@ -157,16 +152,6 @@ class VolumeBackupRecoveryJob implements ShouldBeEncrypted, ShouldBeUnique, Shou return $failure; } - /** - * Returns the minutes to wait before the next automatic attempt after the given number of failed attempts. - */ - public static function backoffMinutes(int $attempts): int - { - $exponent = max(0, min($attempts - 1, 16)); - - return (int) min(self::BACKOFF_BASE_MINUTES * (2 ** $exponent), self::BACKOFF_CAP_MINUTES); - } - public static function dispatchCacheKey(int $executionId): string { return "volume-backup-recovery-dispatched:{$executionId}"; @@ -175,9 +160,7 @@ class VolumeBackupRecoveryJob implements ShouldBeEncrypted, ShouldBeUnique, Shou private static function recordSuccess(ScheduledVolumeBackupExecution $execution): void { $execution->update([ - 'recovery_attempts' => 0, 'recovery_last_attempt_at' => null, - 'recovery_next_retry_at' => null, 'recovery_error' => null, 'recovery_needs_attention' => false, ]); @@ -185,18 +168,18 @@ class VolumeBackupRecoveryJob implements ShouldBeEncrypted, ShouldBeUnique, Shou private static function recordFailure(ScheduledVolumeBackupExecution $execution, string $category): void { - $recorded = $execution->getConnection()->transaction(function () use ($execution, $category): ?ScheduledVolumeBackupExecution { + $needsAttention = $category !== self::CATEGORY_SERVER_UNREACHABLE; + $enteredNeedsAttention = false; + + $recorded = $execution->getConnection()->transaction(function () use ($execution, $category, $needsAttention, &$enteredNeedsAttention): ?ScheduledVolumeBackupExecution { $current = ScheduledVolumeBackupExecution::query()->whereKey($execution->id)->lockForUpdate()->first(); if (! $current) { return null; } - $attempts = $current->recovery_attempts + 1; - $needsAttention = $attempts >= self::MAX_ATTEMPTS; + $enteredNeedsAttention = $needsAttention && ! $current->recovery_needs_attention; $current->update([ - 'recovery_attempts' => $attempts, 'recovery_last_attempt_at' => now(), - 'recovery_next_retry_at' => $needsAttention ? null : now()->addMinutes(self::backoffMinutes($attempts)), 'recovery_error' => $category, 'recovery_needs_attention' => $needsAttention, ]); @@ -214,10 +197,25 @@ class VolumeBackupRecoveryJob implements ShouldBeEncrypted, ShouldBeUnique, Shou 'execution_id' => $recorded->id, 'execution_uuid' => $recorded->uuid, 'category' => $category, - 'attempts' => $recorded->recovery_attempts, 'needs_attention' => $recorded->recovery_needs_attention, - 'next_retry_at' => $recorded->recovery_next_retry_at?->toIso8601String(), ]); + + if ($enteredNeedsAttention) { + self::notifyTeam($recorded); + } + } + + private static function notifyTeam(ScheduledVolumeBackupExecution $execution): void + { + try { + $execution->loadMissing('scheduledVolumeBackup.team'); + $execution->scheduledVolumeBackup?->team?->notify(new RecoveryFailed($execution)); + } catch (Throwable $exception) { + Log::channel('scheduled-errors')->error('Failed to send volume backup recovery notification', [ + 'execution_id' => $execution->id, + 'error' => $exception->getMessage(), + ]); + } } private static function categorizeS3Failure(Throwable $exception): string diff --git a/app/Livewire/Project/Shared/Storages/VolumeBackups.php b/app/Livewire/Project/Shared/Storages/VolumeBackups.php index 9768d67a55..4ceffef502 100644 --- a/app/Livewire/Project/Shared/Storages/VolumeBackups.php +++ b/app/Livewire/Project/Shared/Storages/VolumeBackups.php @@ -316,8 +316,6 @@ class VolumeBackups extends Component } $execution->update([ - 'recovery_attempts' => 0, - 'recovery_next_retry_at' => null, 'recovery_needs_attention' => false, ]); Cache::forget(VolumeBackupRecoveryJob::dispatchCacheKey($execution->id)); diff --git a/app/Models/GithubRunnerExecution.php b/app/Models/GithubRunnerExecution.php index 77a55fd075..e7c36d7d8a 100644 --- a/app/Models/GithubRunnerExecution.php +++ b/app/Models/GithubRunnerExecution.php @@ -4,8 +4,11 @@ namespace App\Models; use App\Enums\GithubRunnerStatus; use App\Jobs\DeregisterGithubRunnerJob; +use App\Jobs\ProvisionGithubRunnerJob; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Relations\BelongsTo; +use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Log; class GithubRunnerExecution extends BaseModel { @@ -81,6 +84,65 @@ class GithubRunnerExecution extends BaseModel (clone $query)->delete(); } + /** + * Marks the runner running. When a job without a pending runner took it, returns a new queued execution for the original job. + * + * @param array $jobDetails + */ + public function assignJob(int $jobId, array $jobDetails): ?self + { + $replacement = DB::transaction(function () use ($jobId, $jobDetails): ?self { + $execution = self::query()->whereKey($this->id)->lockForUpdate()->first(); + if (! $execution || ! in_array($execution->status, [GithubRunnerStatus::Provisioning, GithubRunnerStatus::Idle], true)) { + return null; + } + + $running = [...$jobDetails, 'workflow_job_id' => $jobId, 'status' => GithubRunnerStatus::Running, 'started_at' => now()]; + $jobExecution = $execution->trigger_workflow_job_id === $jobId ? $execution : self::query() + ->where('github_app_id', $execution->github_app_id) + ->where('trigger_workflow_job_id', $jobId) + ->lockForUpdate() + ->first(); + if ($jobExecution?->isActive()) { + $execution->update($running); + + return null; + } + + $replacement = [ + ...$execution->only(['github_app_id', 'trigger_workflow_job_id', 'workflow_job_html_url', 'workflow_name', 'job_name', 'repository_full_name', 'labels', 'is_pull_request', 'provision_attempts']), + 'workflow_job_id' => $execution->trigger_workflow_job_id, + 'status' => GithubRunnerStatus::Queued, + 'queued_at' => now(), + ]; + $jobExecution?->delete(); + $execution->update([...$running, 'trigger_workflow_job_id' => $jobId]); + + if ($replacement['provision_attempts'] >= ProvisionGithubRunnerJob::MAX_ATTEMPTS) { + Log::warning('GitHub runner taken by another job too often; no new runner is started.', [ + 'github_app_id' => $execution->github_app_id, + 'workflow_job_id' => $replacement['trigger_workflow_job_id'], + 'attempts' => $replacement['provision_attempts'], + ]); + + return null; + } + + $hasMatchingConfig = GithubRunnerConfig::query() + ->where('github_app_id', $execution->github_app_id) + ->where('is_enabled', true) + ->get() + ->contains(fn (GithubRunnerConfig $config) => $config->matchesLabels($replacement['labels'] ?? []) + && ($config->allow_pull_requests || ! $replacement['is_pull_request'])); + + return $hasMatchingConfig ? self::create($replacement) : null; + }); + + $this->refresh(); + + return $replacement; + } + public function isActive(): bool { return $this->status->isActive(); diff --git a/app/Models/ScheduledVolumeBackupExecution.php b/app/Models/ScheduledVolumeBackupExecution.php index eec799218e..664efa11c1 100644 --- a/app/Models/ScheduledVolumeBackupExecution.php +++ b/app/Models/ScheduledVolumeBackupExecution.php @@ -21,9 +21,7 @@ class ScheduledVolumeBackupExecution extends BaseModel 'local_storage_deleted', 's3_storage_deleted', 's3_uploaded', - 'recovery_attempts', 'recovery_last_attempt_at', - 'recovery_next_retry_at', 'recovery_error', 'recovery_needs_attention', ]; @@ -39,9 +37,7 @@ class ScheduledVolumeBackupExecution extends BaseModel 'local_storage_deleted' => 'boolean', 's3_storage_deleted' => 'boolean', 's3_uploaded' => 'boolean', - 'recovery_attempts' => 'integer', 'recovery_last_attempt_at' => 'datetime', - 'recovery_next_retry_at' => 'datetime', 'recovery_needs_attention' => 'boolean', ]; } diff --git a/app/Notifications/VolumeBackup/RecoveryFailed.php b/app/Notifications/VolumeBackup/RecoveryFailed.php new file mode 100644 index 0000000000..ecb2220ec5 --- /dev/null +++ b/app/Notifications/VolumeBackup/RecoveryFailed.php @@ -0,0 +1,134 @@ +onQueue('high'); + $backup = $execution->scheduledVolumeBackup; + $resource = $backup?->targetResource(); + $this->resourceName = $resource?->name ?? 'Unknown resource'; + $this->target = $backup ? $backup->targetType().' '.$backup->targetName() : 'Unknown storage'; + $this->reason = match ($execution->recovery_error) { + VolumeBackupRecoveryJob::CATEGORY_S3_AUTH => 'S3 credentials were rejected', + VolumeBackupRecoveryJob::CATEGORY_S3_BUCKET => 'S3 bucket is missing', + VolumeBackupRecoveryJob::CATEGORY_REMOTE_COMMAND => 'A command on the server failed', + default => 'Unknown error', + }; + $this->containersStopped = (bool) $execution->stop_recovery_pending; + + $linkable = $resource instanceof ServiceApplication || $resource instanceof ServiceDatabase ? $resource->service : $resource; + if ($linkable && method_exists($linkable, 'link')) { + $this->url = $linkable->link(); + } + } + + public function via(object $notifiable): array + { + return $notifiable->getEnabledChannels('backup_failure'); + } + + public function summary(): string + { + $summary = "Recovery after the storage backup of {$this->target} ({$this->resourceName}) failed: {$this->reason}."; + + return $this->containersStopped + ? $summary.' The containers stopped for the backup are still stopped, and new backups of this storage are skipped until you retry the recovery.' + : $summary.' The partial S3 upload was not removed. Retry the recovery after fixing the S3 storage.'; + } + + public function toMail(): MailMessage + { + $mail = new MailMessage; + $mail->subject("Coolify: [ACTION REQUIRED] Storage backup recovery FAILED for {$this->resourceName}"); + $mail->view('emails.volume-backup-recovery-failed', [ + 'summary' => $this->summary(), + 'url' => $this->url, + ]); + + return $mail; + } + + public function toDiscord(): DiscordMessage + { + $message = new DiscordMessage( + title: ':cross_mark: Storage backup recovery failed', + description: $this->summary(), + color: DiscordMessage::errorColor(), + isCritical: $this->containersStopped, + ); + + if ($this->url) { + $message->addField('Resource', '[Link]('.$this->url.')'); + } + + return $message; + } + + public function toTelegram(): array + { + $data = ['message' => "Coolify: [ACTION REQUIRED] {$this->summary()}"]; + + if ($this->url) { + $data['buttons'] = [['text' => 'Open resource in Coolify', 'url' => $this->url]]; + } + + return $data; + } + + public function toPushover(): PushoverMessage + { + return new PushoverMessage( + title: 'Storage backup recovery failed', + level: 'error', + message: "[ACTION REQUIRED] {$this->summary()}", + buttons: $this->url ? [['text' => 'Open resource in Coolify', 'url' => $this->url]] : [], + ); + } + + public function toSlack(): SlackMessage + { + return new SlackMessage( + title: 'Coolify: [ACTION REQUIRED] Storage backup recovery failed', + description: $this->summary().($this->url ? "\n\n<{$this->url}|Open resource in Coolify>" : ''), + color: SlackMessage::errorColor(), + ); + } + + public function toWebhook(): array + { + return [ + 'success' => false, + 'message' => 'Storage backup recovery failed', + 'event' => 'volume_backup_recovery_failed', + 'resource_name' => $this->resourceName, + 'storage' => $this->target, + 'execution_uuid' => $this->execution->uuid, + 'reason' => $this->execution->recovery_error, + 'containers_stopped' => $this->containersStopped, + 'url' => $this->url, + ]; + } +} diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 7f39cfb408..8947b3964b 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -87,9 +87,10 @@ function validateDockerComposeForInjection(string $composeYaml, ?string $resourc if (is_array($serviceConfig) && isset($serviceConfig['networks']) && is_array($serviceConfig['networks'])) { foreach ($serviceConfig['networks'] as $networkKey => $networkDetails) { if (is_int($networkKey) && (is_string($networkDetails) || is_int($networkDetails))) { - validateComposeNetworkName((string) $networkDetails, 'service network'); + validateComposeNetworkNameWithVariables((string) $networkDetails, 'service network', (string) $serviceName); } elseif (is_string($networkKey) || is_int($networkKey)) { - validateComposeNetworkName((string) $networkKey, 'service network'); + // Compose does not interpolate keys, so a key must be a plain network name. + validateComposeNetworkName((string) $networkKey, 'service network', (string) $serviceName); } } } @@ -259,40 +260,44 @@ function ensureComposeNetworkNameVariables(Application|Service $resource, iterab } /** - * A network `name:` may be such a variable: only Docker Compose reads this value and it never runs a - * shell; Coolify's own network commands use the network keys. The default must still be a valid - * network name, and nothing else is allowed around the variable. + * A network `name:` may contain variables: only Docker Compose reads this value and it never runs a + * shell; Coolify's own network commands use the network keys. * - * @throws Exception If the value is not a valid network name or such a variable + * @throws Exception If the value is not a valid network name with safe variables */ function validateComposeNetworkNameField(string $name): void +{ + validateComposeNetworkNameWithVariables($name, 'network name field'); +} + +/** + * Allows $VAR, ${VAR}, ${VAR:-default} and ${VAR-default} with safe defaults in a network value that Compose interpolates. + */ +function validateComposeNetworkNameWithVariables(string $name, string $context, ?string $serviceName = null): void { $variable = composeNetworkNameVariable($name); if ($variable !== null) { - if ($variable['default'] !== null) { - validateComposeNetworkName($variable['default'], 'network name field'); - } + $isValid = $variable['default'] === null || ValidationPatterns::isValidDockerNetwork($variable['default']); + } else { + $hasSafeDefaults = true; + $withoutVariables = preg_replace_callback( + '/\$\{([A-Za-z_][A-Za-z0-9_]*)(?::?-([^}]*))?\}|\$[A-Za-z_][A-Za-z0-9_]*/', + function (array $matches) use (&$hasSafeDefaults): string { + $default = $matches[2] ?? ''; + if ($default !== '' && preg_match('/\A[A-Za-z0-9_.-]+\z/', $default) !== 1) { + $hasSafeDefaults = false; + } - return; + return 'x'; + }, + $name, + ); + $isValid = $hasSafeDefaults && $withoutVariables !== null && ValidationPatterns::isValidDockerNetwork($withoutVariables); } - // Compose also resolves variables inside a longer name, for example ${COMPOSE_PROJECT_NAME}_default. - // Each variable must be $VAR, ${VAR}, ${VAR:-default} or ${VAR-default} with a safe default; the name - // is then checked with each variable replaced, so no shell syntax can remain. - $withoutVariables = preg_replace_callback( - '/\$\{([A-Za-z_][A-Za-z0-9_]*)(?::?-([^}]*))?\}|\$[A-Za-z_][A-Za-z0-9_]*/', - function (array $matches): string { - $default = $matches[2] ?? ''; - if ($default !== '' && preg_match('/\A[A-Za-z0-9_.-]+\z/', $default) !== 1) { - throw new Exception('Invalid Docker Compose network name field. Variable defaults may contain only alphanumeric characters, dots, hyphens, and underscores.'); - } - - return 'x'; - }, - $name, - ); - - validateComposeNetworkName($withoutVariables ?? $name, 'network name field'); + if (! $isValid) { + throw invalidComposeNetworkNameException($name, $context, $serviceName, allowsVariables: true); + } } /** @@ -300,16 +305,23 @@ function validateComposeNetworkNameField(string $name): void * * @throws Exception If the network name is not a valid Docker network identifier */ -function validateComposeNetworkName(string $networkName, string $context = 'network name'): void +function validateComposeNetworkName(string $networkName, string $context = 'network name', ?string $serviceName = null): void { if ($networkName === '' || ! ValidationPatterns::isValidDockerNetwork($networkName)) { - throw new Exception( - 'Invalid Docker Compose '.$context. - '. Network names must start with an alphanumeric character and contain only alphanumeric characters, dots, hyphens, and underscores.' - ); + throw invalidComposeNetworkNameException($networkName, $context, $serviceName); } } +function invalidComposeNetworkNameException(string $name, string $context, ?string $serviceName, bool $allowsVariables = false): Exception +{ + $location = $serviceName === null ? '' : " in service {$serviceName}"; + + return new Exception( + "Invalid Docker Compose {$context} \"{$name}\"{$location}. Network names must start with an alphanumeric character and contain only alphanumeric characters, dots, hyphens, and underscores" + .($allowsVariables ? ', and can use variables such as ${NETWORK:-default}.' : '.') + ); +} + /** * Tells if a top-level Compose volume declaration is external: `external: true` or the old * `external: {name: x}` syntax. Docker Compose does not create or remove an external volume. diff --git a/database/migrations/2026_10_03_172204_add_recovery_bounds_to_scheduled_volume_backup_executions_table.php b/database/migrations/2026_10_03_172204_add_recovery_bounds_to_scheduled_volume_backup_executions_table.php index f10f334912..2a2d518019 100644 --- a/database/migrations/2026_10_03_172204_add_recovery_bounds_to_scheduled_volume_backup_executions_table.php +++ b/database/migrations/2026_10_03_172204_add_recovery_bounds_to_scheduled_volume_backup_executions_table.php @@ -12,14 +12,12 @@ return new class extends Migration public function up(): void { Schema::table('scheduled_volume_backup_executions', function (Blueprint $table) { - $table->unsignedSmallInteger('recovery_attempts')->default(0); $table->timestampTz('recovery_last_attempt_at')->nullable(); - $table->timestampTz('recovery_next_retry_at')->nullable(); $table->string('recovery_error', 32)->nullable(); $table->boolean('recovery_needs_attention')->default(false); $table->index( - ['stop_recovery_pending', 's3_cleanup_pending', 'recovery_needs_attention', 'recovery_next_retry_at'], + ['stop_recovery_pending', 's3_cleanup_pending', 'recovery_needs_attention'], 'scheduled_volume_executions_recovery_index', ); }); @@ -33,9 +31,7 @@ return new class extends Migration Schema::table('scheduled_volume_backup_executions', function (Blueprint $table) { $table->dropIndex('scheduled_volume_executions_recovery_index'); $table->dropColumn([ - 'recovery_attempts', 'recovery_last_attempt_at', - 'recovery_next_retry_at', 'recovery_error', 'recovery_needs_attention', ]); diff --git a/resources/views/emails/volume-backup-recovery-failed.blade.php b/resources/views/emails/volume-backup-recovery-failed.blade.php new file mode 100644 index 0000000000..a1653341d1 --- /dev/null +++ b/resources/views/emails/volume-backup-recovery-failed.blade.php @@ -0,0 +1,7 @@ + +{{ $summary }} + +@if ($url) +Click [here]({{ $url }}) to open the resource. +@endif + diff --git a/tests/Feature/ApplicationComposeInjectionValidationTest.php b/tests/Feature/ApplicationComposeInjectionValidationTest.php index aee515917f..4db2d25dad 100644 --- a/tests/Feature/ApplicationComposeInjectionValidationTest.php +++ b/tests/Feature/ApplicationComposeInjectionValidationTest.php @@ -356,3 +356,24 @@ test('the Compose deployment validates the repository file before any command us ->and($loadPosition)->toBeLessThan(strpos($body, 'base64 -d')) ->and($body)->toContain('"stat -c \'%F\' ".escapeshellarg($realPathInGit)'); }); + +test('loading a Compose file with a variable service network saves it', function () { + $compose = "services:\n web:\n image: nginx\n networks:\n - \${NET:-proxy}\nnetworks:\n proxy:\n external: true\n"; + fakeRepositoryCompose($compose); + + $this->application->loadComposeFile(); + + expect($this->application->fresh()->docker_compose_raw)->toBe(trim($compose)); +}); + +test('the deployment log names the service and network that failed validation', function () { + fakeRepositoryCompose("services:\n web:\n image: nginx\n networks:\n - '\$(id)'\n"); + $logEntries = []; + $job = composeDeploymentJob($this->application, 0, $logEntries); + + expect(fn () => (new ReflectionMethod(ApplicationDeploymentJob::class, 'loadComposeFileForDeployment'))->invoke($job)) + ->toThrow(DeploymentException::class); + + expect(collect($logEntries)->pluck(0)->implode("\n")) + ->toContain('Invalid Docker Compose service network "$(id)" in service web.'); +}); diff --git a/tests/Feature/GithubRunnerFlowTest.php b/tests/Feature/GithubRunnerFlowTest.php index c452f3b7bd..37663a26a9 100644 --- a/tests/Feature/GithubRunnerFlowTest.php +++ b/tests/Feature/GithubRunnerFlowTest.php @@ -270,6 +270,154 @@ describe('workflow_job webhook', function () { ->and($execution->started_at)->not->toBeNull(); }); + it('starts a new runner for the job whose runner another job took', function () { + $stolen = runnerTestExecution($this->githubApp, [ + 'trigger_workflow_job_id' => 5001, + 'workflow_job_id' => 5001, + 'job_name' => 'build', + 'status' => GithubRunnerStatus::Idle, + 'server_id' => $this->server->id, + 'runner_name' => 'coolify-runner-a', + 'provision_attempts' => 1, + ]); + + foreach ([1, 2] as $delivery) { + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'in_progress', [ + 'id' => 6001, + 'name' => 'pr-test', + 'runner_name' => 'coolify-runner-a', + ])->assertOk(); + } + + $stolen->refresh(); + $replacement = GithubRunnerExecution::query()->where('trigger_workflow_job_id', 5001)->sole(); + expect($stolen->status)->toBe(GithubRunnerStatus::Running) + ->and($stolen->trigger_workflow_job_id)->toBe(6001) + ->and($stolen->workflow_job_id)->toBe(6001) + ->and($replacement->id)->not->toBe($stolen->id) + ->and($replacement->status)->toBe(GithubRunnerStatus::Queued) + ->and($replacement->job_name)->toBe('build') + ->and($replacement->labels)->toBe(['self-hosted', 'coolify']) + ->and($replacement->provision_attempts)->toBe(1); + Queue::assertPushed(ProvisionGithubRunnerJob::class, 1); + Queue::assertPushed(ProvisionGithubRunnerJob::class, fn ($job) => $job->executionId === $replacement->id); + + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'completed', [ + 'id' => 6001, + 'runner_name' => 'coolify-runner-a', + 'conclusion' => 'success', + ])->assertOk(); + + expect($stolen->fresh()->status)->toBe(GithubRunnerStatus::Completed) + ->and($replacement->fresh()->status)->toBe(GithubRunnerStatus::Queued); + }); + + it('starts a new runner when the completed hook of the other job arrives before its in_progress hook', function () { + $stolen = runnerTestExecution($this->githubApp, [ + 'trigger_workflow_job_id' => 5001, + 'status' => GithubRunnerStatus::Idle, + 'server_id' => $this->server->id, + 'runner_name' => 'coolify-runner-a', + 'provision_attempts' => 1, + ]); + + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'completed', [ + 'id' => 6001, + 'runner_name' => 'coolify-runner-a', + 'conclusion' => 'success', + ])->assertOk(); + + expect($stolen->fresh()->status)->toBe(GithubRunnerStatus::Completed) + ->and($stolen->fresh()->trigger_workflow_job_id)->toBe(6001) + ->and(GithubRunnerExecution::query()->where('trigger_workflow_job_id', 5001)->sole()->status)->toBe(GithubRunnerStatus::Queued); + Queue::assertPushed(ProvisionGithubRunnerJob::class, 1); + }); + + it('replaces a stale execution of the job that took the runner', function () { + $stale = runnerTestExecution($this->githubApp, ['trigger_workflow_job_id' => 6001, 'status' => GithubRunnerStatus::TimedOut]); + $stolen = runnerTestExecution($this->githubApp, [ + 'trigger_workflow_job_id' => 5001, + 'status' => GithubRunnerStatus::Idle, + 'server_id' => $this->server->id, + 'runner_name' => 'coolify-runner-a', + 'provision_attempts' => 1, + ]); + + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'in_progress', [ + 'id' => 6001, + 'runner_name' => 'coolify-runner-a', + ])->assertOk(); + + expect($stale->fresh())->toBeNull() + ->and($stolen->fresh()->trigger_workflow_job_id)->toBe(6001) + ->and(GithubRunnerExecution::query()->where('trigger_workflow_job_id', 5001)->sole()->status)->toBe(GithubRunnerStatus::Queued); + Queue::assertPushed(ProvisionGithubRunnerJob::class, 1); + }); + + it('does not start a new runner when the runner took another job with its own pending runner', function () { + $first = runnerTestExecution($this->githubApp, [ + 'trigger_workflow_job_id' => 5001, + 'status' => GithubRunnerStatus::Idle, + 'server_id' => $this->server->id, + 'runner_name' => 'coolify-runner-a', + ]); + $second = runnerTestExecution($this->githubApp, ['trigger_workflow_job_id' => 6001]); + + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'in_progress', [ + 'id' => 6001, + 'runner_name' => 'coolify-runner-a', + ])->assertOk(); + + expect(GithubRunnerExecution::count())->toBe(2) + ->and($first->fresh()->status)->toBe(GithubRunnerStatus::Running) + ->and($first->fresh()->workflow_job_id)->toBe(6001) + ->and($second->fresh()->status)->toBe(GithubRunnerStatus::Queued); + Queue::assertNotPushed(ProvisionGithubRunnerJob::class); + }); + + it('does not start a new runner for a job whose runner was taken', function (Closure $setup, array $attributes) { + $setup(); + $stolen = runnerTestExecution($this->githubApp, [ + 'trigger_workflow_job_id' => 5001, + 'status' => GithubRunnerStatus::Idle, + 'server_id' => $this->server->id, + 'runner_name' => 'coolify-runner-a', + 'provision_attempts' => 1, + ...$attributes, + ]); + + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'in_progress', [ + 'id' => 6001, + 'runner_name' => 'coolify-runner-a', + ])->assertOk(); + + expect(GithubRunnerExecution::count())->toBe(1) + ->and($stolen->fresh()->status)->toBe(GithubRunnerStatus::Running); + Queue::assertNotPushed(ProvisionGithubRunnerJob::class); + })->with([ + 'runner limit of the job reached' => [fn () => null, ['provision_attempts' => ProvisionGithubRunnerJob::MAX_ATTEMPTS]], + 'configuration disabled' => [fn () => GithubRunnerConfig::query()->update(['is_enabled' => false]), []], + 'pull request job no longer allowed' => [fn () => null, ['is_pull_request' => true]], + ]); + + it('marks the runner running without a new runner when it took the job it was started for', function () { + $execution = runnerTestExecution($this->githubApp, [ + 'trigger_workflow_job_id' => 5001, + 'status' => GithubRunnerStatus::Idle, + 'server_id' => $this->server->id, + 'runner_name' => 'coolify-runner-a', + ]); + + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'in_progress', ['runner_name' => 'coolify-runner-a'])->assertOk(); + + $execution->refresh(); + expect(GithubRunnerExecution::count())->toBe(1) + ->and($execution->status)->toBe(GithubRunnerStatus::Running) + ->and($execution->trigger_workflow_job_id)->toBe(5001) + ->and($execution->workflow_job_id)->toBe(5001); + Queue::assertNotPushed(ProvisionGithubRunnerJob::class); + }); + it('completes the runner and queues its cleanup', function () { $execution = runnerTestExecution($this->githubApp, [ 'status' => GithubRunnerStatus::Running, diff --git a/tests/Feature/KeydbDragonflyLegacyPasswordQuotingTest.php b/tests/Feature/KeydbDragonflyLegacyPasswordQuotingTest.php index 823017c0a8..703a52f306 100644 --- a/tests/Feature/KeydbDragonflyLegacyPasswordQuotingTest.php +++ b/tests/Feature/KeydbDragonflyLegacyPasswordQuotingTest.php @@ -8,6 +8,7 @@ use App\Models\IntegrationToken; use App\Models\PrivateKey; use App\Models\Project; use App\Models\Server; +use App\Models\SharedEnvironmentVariable; use App\Models\StandaloneDocker; use App\Models\Team; use App\Services\DatabaseStartCommandExecutor; @@ -51,7 +52,7 @@ beforeEach(function () { }); /** - * @return array{command: string, healthcheck: list} + * @return array{command: string, healthcheck: list, environment: list} */ function legacyPasswordQuotingStartedService(object $executor, string $uuid): array { @@ -59,7 +60,7 @@ function legacyPasswordQuotingStartedService(object $executor, string $uuid): ar if (preg_match("#^echo '([^']+)' \| base64 -d \| tee \S+/docker-compose\.yml#", $command, $matches)) { $service = Yaml::parse(base64_decode($matches[1]))['services'][$uuid]; - return ['command' => $service['command'], 'healthcheck' => $service['healthcheck']['test']]; + return ['command' => $service['command'], 'healthcheck' => $service['healthcheck']['test'], 'environment' => $service['environment']]; } } @@ -195,3 +196,71 @@ it('quotes shell control characters in the password of a database created after 'space' => 'a b', '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) { + $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, "'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'); +})->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']); + + $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()->legacy_password_quoting)->toBeTrue(); +})->with('legacy-password-quoting-engines'); + +it('runs with a remote secret REDIS_PASSWORD without storing it', function (string $create, string $start, string $passwordColumn, string $commandFormat) { + Http::fake(['https://api.doppler.com/*' => Http::response(['REDIS_PASSWORD' => 'secretpass'])]); + $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]); + $env = $database->runtime_environment_variables()->create(['key' => 'REDIS_PASSWORD', 'value' => '{{vault.REDIS_PASSWORD}}']); + + $start::run($database->fresh(), new Activity); + + $service = legacyPasswordQuotingStartedService($this->executor, $database->uuid); + expect($service['command'])->toBe(sprintf($commandFormat, "'secretpass'")) + ->and($service['healthcheck'][3])->toBe('secretpass') + ->and($env->fresh()->value)->toBe('{{vault.REDIS_PASSWORD}}') + ->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) { + 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 => 'storedpass']); + $env = $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, "'sharedpass'")) + ->and($service['healthcheck'][3])->toBe('sharedpass') + ->and($env->fresh()->value)->toBe('{{team.REDIS_PASSWORD}}') + ->and($database->fresh()->{$passwordColumn})->toBe('sharedpass'); +})->with('legacy-password-quoting-engines'); diff --git a/tests/Feature/MultiServerApplicationTest.php b/tests/Feature/MultiServerApplicationTest.php index 4e7214ffca..2f586e07b8 100644 --- a/tests/Feature/MultiServerApplicationTest.php +++ b/tests/Feature/MultiServerApplicationTest.php @@ -112,6 +112,13 @@ describe('registry image guard', function () { callMultiServerDeploymentJob($job, 'ensureRegistryImageForMultipleServers'); })->throwsNoExceptions(); + test('allows docker compose applications, which build on each server', function () { + $this->application->update(['build_pack' => 'dockercompose']); + $job = makeMultiServerDeploymentJob(['application' => $this->application->fresh()]); + + callMultiServerDeploymentJob($job, 'ensureRegistryImageForMultipleServers'); + })->throwsNoExceptions(); + test('allows preview deployments, which run only on the main server', function () { $job = makeMultiServerDeploymentJob(['application' => $this->application, 'pull_request_id' => 7]); diff --git a/tests/Feature/StorageHostPathTest.php b/tests/Feature/StorageHostPathTest.php index 166cd07563..c22cce8ca1 100644 --- a/tests/Feature/StorageHostPathTest.php +++ b/tests/Feature/StorageHostPathTest.php @@ -1,5 +1,6 @@ $abilities + */ +function storageHostPathToken(User $user, array $abilities = ['*']): string { // A token request must not use the session user of the Livewire tests. auth()->forgetGuards(); @@ -78,7 +82,7 @@ function storageHostPathToken(User $user): string $token = $user->tokens()->create([ 'name' => 'host-path-token', 'token' => hash('sha256', $plainTextToken), - 'abilities' => ['*'], + 'abilities' => $abilities, 'team_id' => test()->team->id, ]); @@ -363,3 +367,85 @@ test('relative host paths stay inside the resource directory', function () { Process::assertRan(fn ($process) => str_contains($process->command, 'readlink -f') && str_contains($process->command, "'{$workdir}' '{$workdir}/data/zz'")); }); + +test('a token without deploy cannot create a mount outside the resource directory', function (string $resourceType, array $mount) { + [$url, $payload] = storageHostPathApiTarget($resourceType); + + $this->withHeaders(['Authorization' => 'Bearer '.storageHostPathToken($this->admin, ['read', 'write'])]) + ->postJson($url, [...$payload, 'type' => 'file', ...$mount]) + ->assertForbidden() + ->assertJsonPath('message', 'Missing required permissions: deploy. A mount outside the resource directory needs a token with the deploy permission.'); + + expect(LocalFileVolume::query()->count())->toBe(0); + Bus::assertNotDispatched(ServerStorageSaveJob::class); + Process::assertNothingRan(); +})->with(['application', 'database', 'service'])->with([ + 'directory' => [['is_directory' => true, 'fs_path' => '/root/.ssh', 'mount_path' => '/data']], + 'file with content' => [['fs_path' => '/root/.ssh/authorized_keys', 'mount_path' => '/etc/keys', 'content' => 'ssh-ed25519 AAAA']], + 'empty file' => [['fs_path' => '/etc/zz/app.conf', 'mount_path' => '/etc/app.conf']], +]); + +test('a deploy or root token creates a mount outside the resource directory', function (string $resourceType, array $abilities) { + [$url, $payload, $resource] = storageHostPathApiTarget($resourceType); + $token = storageHostPathToken($this->admin, $abilities); + + $this->withHeaders(['Authorization' => 'Bearer '.$token]) + ->postJson($url, [...$payload, 'type' => 'file', 'is_directory' => true, 'fs_path' => '/srv/zz', 'mount_path' => '/data']) + ->assertCreated(); + $this->withHeaders(['Authorization' => 'Bearer '.$token]) + ->postJson($url, [...$payload, 'type' => 'file', 'fs_path' => '/etc/zz/app.conf', 'mount_path' => '/etc/app.conf', 'content' => 'listen 8080;']) + ->assertCreated(); + + expect($resource->fileStorages()->orderBy('id')->pluck('fs_path')->all())->toBe(['/srv/zz', '/etc/zz/app.conf']); +})->with(['application', 'database', 'service'])->with([ + 'deploy' => [['read', 'write', 'deploy']], + 'root' => [['root']], +]); + +test('a write token creates mounts inside the resource directory', function (string $resourceType) { + [$url, $payload, $resource] = storageHostPathApiTarget($resourceType); + $token = storageHostPathToken($this->admin, ['read', 'write']); + $workdir = $resource instanceof ServiceApplication ? $resource->service->workdir() : $resource->workdir(); + + $this->withHeaders(['Authorization' => 'Bearer '.$token]) + ->postJson($url, [...$payload, 'type' => 'file', 'is_directory' => true, 'fs_path' => 'data/zz', 'mount_path' => '/data']) + ->assertCreated(); + $this->withHeaders(['Authorization' => 'Bearer '.$token]) + ->postJson($url, [...$payload, 'type' => 'file', 'fs_path' => $workdir.'/config/app.conf', 'mount_path' => '/etc/app.conf', 'content' => 'listen 8080;']) + ->assertCreated(); + $this->withHeaders(['Authorization' => 'Bearer '.$token]) + ->postJson($url, [...$payload, 'type' => 'file', 'mount_path' => '/etc/other.conf', 'content' => 'listen 8081;']) + ->assertCreated(); + $this->withHeaders(['Authorization' => 'Bearer '.$token]) + ->postJson($url, [...$payload, 'type' => 'file', 'is_host_file' => true, 'fs_path' => '/etc/hosts', 'mount_path' => '/etc/hosts']) + ->assertCreated(); + + expect($resource->fileStorages()->count())->toBe(4); +})->with(['application', 'database', 'service']); + +test('changing the content of a mount outside the resource directory needs deploy', function (string $resourceType) { + [$url, , $resource] = storageHostPathApiTarget($resourceType); + $storage = LocalFileVolume::create([ + 'fs_path' => '/etc/zz/app.conf', + 'mount_path' => '/etc/app.conf', + 'content' => 'listen 8080;', + 'resource_id' => $resource->id, + 'resource_type' => $resource->getMorphClass(), + ]); + + $this->withHeaders(['Authorization' => 'Bearer '.storageHostPathToken($this->admin, ['read', 'write'])]) + ->patchJson($url, ['type' => 'file', 'uuid' => $storage->uuid, 'content' => 'ssh-ed25519 AAAA']) + ->assertForbidden() + ->assertJsonPath('message', 'Missing required permissions: deploy. A mount outside the resource directory needs a token with the deploy permission.'); + expect($storage->fresh()->content)->toBe('listen 8080;'); + + $this->withHeaders(['Authorization' => 'Bearer '.storageHostPathToken($this->admin, ['read', 'write'])]) + ->patchJson($url, ['type' => 'file', 'uuid' => $storage->uuid, 'mount_path' => '/etc/app2.conf', 'content' => 'listen 8080;']) + ->assertOk(); + expect($storage->fresh()->mount_path)->toBe('/etc/app2.conf'); + + $this->withHeaders(['Authorization' => 'Bearer '.storageHostPathToken($this->admin, ['read', 'write', 'deploy'])]) + ->patchJson($url, ['type' => 'file', 'uuid' => $storage->uuid, 'content' => 'listen 9090;']) + ->assertOk(); + expect($storage->fresh()->content)->toBe('listen 9090;'); +})->with(['application', 'database', 'service']); diff --git a/tests/Feature/VolumeBackupRecoveryBoundsTest.php b/tests/Feature/VolumeBackupRecoveryBoundsTest.php index f65c17ae63..ff3ed2d031 100644 --- a/tests/Feature/VolumeBackupRecoveryBoundsTest.php +++ b/tests/Feature/VolumeBackupRecoveryBoundsTest.php @@ -17,12 +17,15 @@ use App\Models\Server; use App\Models\StandaloneDocker; use App\Models\Team; use App\Models\User; +use App\Notifications\Channels\DiscordChannel; +use App\Notifications\VolumeBackup\RecoveryFailed; use Aws\Command; use Aws\S3\Exception\S3Exception; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Log; +use Illuminate\Support\Facades\Notification; use Illuminate\Support\Facades\Process; use Illuminate\Support\Facades\Queue; use Illuminate\Support\Facades\Storage; @@ -33,6 +36,7 @@ uses(RefreshDatabase::class); beforeEach(function () { Server::flushIdentityMap(); + Notification::fake(); config(['broadcasting.default' => 'null']); InstanceSettings::unguarded(fn () => InstanceSettings::firstOrCreate(['id' => 0])); }); @@ -130,9 +134,22 @@ function makeRecoveryBoundsServerFunctional(Server $server): void ]); } -it('doubles the recovery backoff and caps it at six hours', function () { - expect(collect(range(1, 9))->map(fn (int $attempts) => VolumeBackupRecoveryJob::backoffMinutes($attempts))->all()) - ->toBe([5, 10, 20, 40, 80, 160, 320, 360, 360]); +it('notifies the team once when recovery needs attention', function () { + $team = Team::factory()->create(); + $team->discordNotificationSettings()->update(['discord_enabled' => true, 'backup_failure_discord_notifications' => true]); + $team->webhookNotificationSettings()->update(['webhook_enabled' => true, 'backup_failure_webhook_notifications' => false]); + [, , , $backup] = createRecoveryBoundsBackup($team); + $execution = createRecoveryBoundsExecution($backup); + failRecoveryBoundsS3Delete('AccessDenied'); + + VolumeBackupRecoveryJob::recoverWithBounds($execution); + VolumeBackupRecoveryJob::recoverWithBounds($execution); + + Notification::assertSentToTimes($team, RecoveryFailed::class, 1); + Notification::assertSentTo($team, RecoveryFailed::class, fn (RecoveryFailed $notification, array $channels) => $notification->execution->is($execution) + && $channels === [DiscordChannel::class] + && str_contains($notification->toDiscord()->description, 'S3 credentials were rejected') + && str_contains((string) $notification->toMail()->render(), 'S3 credentials were rejected')); }); it('records a categorized S3 cleanup failure without throwing or leaking the S3 message', function () { @@ -148,11 +165,9 @@ it('records a categorized S3 cleanup failure without throwing or leaking the S3 $execution->refresh(); expect($execution->s3_cleanup_pending)->toBeTrue() ->and($execution->s3_storage_deleted)->toBeFalse() - ->and($execution->recovery_attempts)->toBe(1) ->and($execution->recovery_error)->toBe('s3_auth') ->and($execution->recovery_last_attempt_at->equalTo(now()))->toBeTrue() - ->and($execution->recovery_next_retry_at->equalTo(now()->addMinutes(5)))->toBeTrue() - ->and($execution->recovery_needs_attention)->toBeFalse() + ->and($execution->recovery_needs_attention)->toBeTrue() ->and($execution->message)->toBe('Worker timed out'); $logger->shouldHaveReceived('warning')->once()->with( 'Volume backup recovery failed', @@ -162,38 +177,37 @@ it('records a categorized S3 cleanup failure without throwing or leaking the S3 ); }); -it('categorizes a missing S3 bucket and grows the backoff on each failure', function () { +it('categorizes a missing S3 bucket and does not try again after the first failure', function () { Carbon::setTestNow('2026-10-03 12:00:00'); [, , , $backup] = createRecoveryBoundsBackup(Team::factory()->create()); $execution = createRecoveryBoundsExecution($backup); - failRecoveryBoundsS3Delete('NoSuchBucket'); + $disk = Mockery::mock(); + $disk->shouldReceive('delete')->once()->andThrow(UnableToDeleteFile::atLocation('partial.tar.gz', '', new S3Exception( + 'The specified bucket does not exist', + new Command('DeleteObject'), + ['code' => 'NoSuchBucket'], + ))); + Storage::shouldReceive('build')->once()->andReturn($disk); VolumeBackupRecoveryJob::recoverWithBounds($execution); VolumeBackupRecoveryJob::recoverWithBounds($execution); $execution->refresh(); expect($execution->recovery_error)->toBe('s3_bucket') - ->and($execution->recovery_attempts)->toBe(2) - ->and($execution->recovery_next_retry_at->equalTo(now()->addMinutes(10)))->toBeTrue(); + ->and($execution->recovery_needs_attention)->toBeTrue(); }); -it('needs attention after the maximum attempts and stops automatic recovery', function () { +it('stops automatic recovery after the first failure that is not an unreachable server', function () { Carbon::setTestNow('2026-10-03 12:00:00'); config(['constants.coolify.self_hosted' => true]); Queue::fake(); [, , , $backup] = createRecoveryBoundsBackup(Team::factory()->create()); - $execution = createRecoveryBoundsExecution($backup, [ - 'recovery_attempts' => VolumeBackupRecoveryJob::MAX_ATTEMPTS - 1, - ]); + $execution = createRecoveryBoundsExecution($backup); failRecoveryBoundsS3Delete('AccessDenied'); - VolumeBackupRecoveryJob::recoverWithBounds($execution); VolumeBackupRecoveryJob::recoverWithBounds($execution); - $execution->refresh(); - expect($execution->recovery_needs_attention)->toBeTrue() - ->and($execution->recovery_attempts)->toBe(VolumeBackupRecoveryJob::MAX_ATTEMPTS) - ->and($execution->recovery_next_retry_at)->toBeNull(); + expect($execution->fresh()->recovery_needs_attention)->toBeTrue(); Carbon::setTestNow('2026-10-04 12:00:00'); (new ScheduledJobManager)->handle(); @@ -201,20 +215,27 @@ it('needs attention after the maximum attempts and stops automatic recovery', fu Queue::assertNotPushed(VolumeBackupRecoveryJob::class); }); -it('waits for the backoff before dispatching recovery again', function () { +it('dispatches container recovery again only when the server is functional, once per execution', function () { Carbon::setTestNow('2026-10-03 12:00:00'); config(['constants.coolify.self_hosted' => true]); + Storage::fake('ssh-mux'); Queue::fake(); - [, , , $backup] = createRecoveryBoundsBackup(Team::factory()->create()); + [, , $server, $backup] = createRecoveryBoundsBackup(Team::factory()->create()); + $server->settings()->update(['is_reachable' => false]); createRecoveryBoundsExecution($backup, [ - 'recovery_attempts' => 3, - 'recovery_next_retry_at' => now()->addMinutes(20), + 'stop_recovery_pending' => true, + 'recovery_error' => 'server_unreachable', + 'recovery_last_attempt_at' => now(), ]); + (new ScheduledJobManager)->handle(); + Carbon::setTestNow('2026-10-04 12:00:00'); (new ScheduledJobManager)->handle(); Queue::assertNotPushed(VolumeBackupRecoveryJob::class); - Carbon::setTestNow('2026-10-03 12:20:00'); + makeRecoveryBoundsServerFunctional($server); + Server::flushIdentityMap(); + (new ScheduledJobManager)->handle(); (new ScheduledJobManager)->handle(); Queue::assertPushed(VolumeBackupRecoveryJob::class, 1); }); @@ -242,16 +263,15 @@ it('cleans the S3 upload even when container recovery fails', function () { ->and($execution->s3_cleanup_pending)->toBeFalse() ->and($execution->s3_storage_deleted)->toBeTrue() ->and($execution->recovery_error)->toBe('server_unreachable') - ->and($execution->recovery_attempts)->toBe(1); + ->and($execution->recovery_needs_attention)->toBeFalse(); + Notification::assertNothingSent(); }); it('resets the recovery state after a successful attempt', function () { [, , , $backup] = createRecoveryBoundsBackup(Team::factory()->create()); $execution = createRecoveryBoundsExecution($backup, [ - 'recovery_attempts' => 4, - 'recovery_error' => 's3_auth', + 'recovery_error' => 'server_unreachable', 'recovery_last_attempt_at' => now()->subHour(), - 'recovery_next_retry_at' => now()->subMinute(), ]); $disk = Mockery::mock(); $disk->shouldReceive('delete')->once()->andReturnTrue(); @@ -262,9 +282,9 @@ it('resets the recovery state after a successful attempt', function () { $execution->refresh(); expect($execution->s3_cleanup_pending)->toBeFalse() ->and($execution->s3_storage_deleted)->toBeTrue() - ->and($execution->recovery_attempts)->toBe(0) ->and($execution->recovery_error)->toBeNull() - ->and($execution->recovery_next_retry_at)->toBeNull(); + ->and($execution->recovery_last_attempt_at)->toBeNull() + ->and($execution->recovery_needs_attention)->toBeFalse(); }); it('only blocks new scheduled backups while stopped containers await recovery', function (array $pending, bool $dispatched) { @@ -340,7 +360,6 @@ it('shows the recovery state and retries recovery manually', function () { signInForRecoveryBounds($this, $team); [$application, $volume, , $backup] = createRecoveryBoundsBackup($team); $execution = createRecoveryBoundsExecution($backup, [ - 'recovery_attempts' => VolumeBackupRecoveryJob::MAX_ATTEMPTS, 'recovery_error' => 's3_auth', 'recovery_needs_attention' => true, ]); @@ -354,10 +373,9 @@ it('shows the recovery state and retries recovery manually', function () { ->assertSee('Recovery pending'); $execution->refresh(); - expect($execution->recovery_attempts)->toBe(0) - ->and($execution->recovery_needs_attention)->toBeFalse() - ->and($execution->recovery_next_retry_at)->toBeNull() + expect($execution->recovery_needs_attention)->toBeFalse() ->and(Cache::has(VolumeBackupRecoveryJob::dispatchCacheKey($execution->id)))->toBeFalse(); + Queue::assertPushed(VolumeBackupRecoveryJob::class, 1); Queue::assertPushed(VolumeBackupRecoveryJob::class, fn (VolumeBackupRecoveryJob $job) => $job->execution->is($execution)); }); @@ -365,7 +383,6 @@ it('prevents another team from retrying recovery', function () { Queue::fake(); [$application, $volume, , $backup] = createRecoveryBoundsBackup(Team::factory()->create()); $execution = createRecoveryBoundsExecution($backup, [ - 'recovery_attempts' => VolumeBackupRecoveryJob::MAX_ATTEMPTS, 'recovery_needs_attention' => true, ]); $otherTeam = Team::factory()->create(); @@ -378,7 +395,6 @@ it('prevents another team from retrying recovery', function () { ->call('retryRecovery', $execution->id) ->assertDispatched('error'); - expect($execution->fresh()->recovery_needs_attention)->toBeTrue() - ->and($execution->fresh()->recovery_attempts)->toBe(VolumeBackupRecoveryJob::MAX_ATTEMPTS); + expect($execution->fresh()->recovery_needs_attention)->toBeTrue(); Queue::assertNotPushed(VolumeBackupRecoveryJob::class); }); diff --git a/tests/Feature/VolumeBackupTest.php b/tests/Feature/VolumeBackupTest.php index 55aec44db1..a4d2c2baac 100644 --- a/tests/Feature/VolumeBackupTest.php +++ b/tests/Feature/VolumeBackupTest.php @@ -2676,6 +2676,7 @@ it('dispatches pending recovery for an execution at most once every five minutes Queue::fake(); $team = Team::factory()->create(); [$application, $volume] = createVolumeBackupApplication($team); + $application->destination->server->settings()->update(['is_reachable' => true, 'is_usable' => true, 'force_disabled' => false]); $backup = $volume->scheduledBackups()->create([ 'team_id' => $team->id, 'frequency' => 'daily', diff --git a/tests/Unit/DockerComposeInjectionCompatibilityTest.php b/tests/Unit/DockerComposeInjectionCompatibilityTest.php index 21db47628e..8192a41ddd 100644 --- a/tests/Unit/DockerComposeInjectionCompatibilityTest.php +++ b/tests/Unit/DockerComposeInjectionCompatibilityTest.php @@ -229,3 +229,44 @@ test('Compose injection payloads are rejected', function (string $compose, strin 'mixed network name with an unsafe default' => ["services:\n web:\n image: nginx\nnetworks:\n edge:\n name: 'app-\${ENV:-x y}'\n", 'Invalid Docker Compose network name field'], 'network name variable with an unsafe default' => ["services:\n web:\n image: nginx\nnetworks:\n edge:\n name: '\${NET:-x\$(touch /tmp/pwned)}'\n", 'Invalid Docker Compose network name field'], ]); + +test('service network list entries accept the same variables as network names', function (string $network) { + $compose = "services:\n web:\n image: nginx\n networks:\n - '{$network}'\nnetworks:\n proxy:\n external: true\n"; + + expect(fn () => validateDockerComposeForInjection($compose))->not->toThrow(Exception::class); +})->with([ + 'variable with a default' => ['${NET:-proxy}'], + 'variable with an unset-only default' => ['${NET-proxy}'], + 'braced variable' => ['${NET}'], + 'plain variable' => ['$NET'], + 'variable mixed with text' => ['${COMPOSE_PROJECT_NAME}_default'], + 'text with a variable default' => ['app-${ENV:-prod}'], +]); + +test('unsafe service network list entries with variables are rejected', function (string $network) { + $compose = "services:\n web:\n image: nginx\n networks:\n - '{$network}'\n"; + + expect(fn () => validateDockerComposeForInjection($compose))->toThrow(Exception::class, 'Invalid Docker Compose service network'); +})->with([ + 'command substitution' => ['$(id)'], + 'backticks' => ['`id`'], + 'variable with command substitution' => ['${NET}$(id)'], + 'default with command substitution' => ['${NET:-$(id)}'], + 'default with a space' => ['${NET:-x y}'], + 'default with backticks' => ['${NET:-`id`}'], +]); + +test('a service network key with a variable is rejected because Compose does not interpolate keys', function () { + $compose = "services:\n web:\n image: nginx\n networks:\n '\${NET:-proxy}': {}\n"; + + expect(fn () => validateDockerComposeForInjection($compose))->toThrow(Exception::class, 'Invalid Docker Compose service network'); +}); + +test('network errors name the service and the invalid value', function () { + $compose = "services:\n web:\n image: nginx\n networks:\n - 'net;id'\n"; + + expect(fn () => validateDockerComposeForInjection($compose)) + ->toThrow(Exception::class, 'Invalid Docker Compose service network "net;id" in service web. Network names must start with an alphanumeric character and contain only alphanumeric characters, dots, hyphens, and underscores, and can use variables such as ${NETWORK:-default}.'); + expect(fn () => validateDockerComposeForInjection("services:\n web:\n image: nginx\nnetworks:\n 'net;id': {}\n")) + ->toThrow(Exception::class, 'Invalid Docker Compose network name "net;id".'); +});