From fbd6c5be21ac6d99d11ea2871cdd99fa3dc64a0f Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 23:10:52 +0200 Subject: [PATCH] fix(deployments): validate build-time variable names only when building Since 17ca63ff4 every deployment except Docker image apps failed on a build-time variable with a name such as MY-VAR, also restarts and deployments that reuse an existing image. v4.3.23 failed only when it built an image. - Check build-time names right before the build steps; the start of the deployment only logs a warning - Names that break the .env file still fail every deployment - Leave invalid names out of the build secrets helper container, so restarts with build secrets work --- app/Jobs/ApplicationDeploymentJob.php | 32 +++++++--- ...ationDeploymentControlVarFilteringTest.php | 62 ++++++++++++++++--- 2 files changed, 77 insertions(+), 17 deletions(-) diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 3a63d36e34..36fb321fa6 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -646,6 +646,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return; } + $this->validateDeploymentEnvironmentVariableKeys(buildsImage: true); // Save build-time .env file BEFORE the build $this->save_buildtime_environment_variables(); @@ -792,6 +793,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue } $this->generate_image_names(); $this->cleanup_git(); + $this->validateDeploymentEnvironmentVariableKeys(buildsImage: true); $this->generate_build_env_variables(); @@ -1108,6 +1110,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return; } } + $this->validateDeploymentEnvironmentVariableKeys(buildsImage: true); $this->cleanup_git(); $this->generate_compose_file(); @@ -1141,6 +1144,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return; } } + $this->validateDeploymentEnvironmentVariableKeys(buildsImage: true); $this->clone_repository(); $this->cleanup_git(); $this->generate_nixpacks_confs(); @@ -1174,6 +1178,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return; } } + $this->validateDeploymentEnvironmentVariableKeys(buildsImage: true); $this->clone_repository(); $this->cleanup_git(); $this->generate_compose_file(); @@ -1205,6 +1210,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return; } } + $this->validateDeploymentEnvironmentVariableKeys(buildsImage: true); $this->clone_repository(); $this->cleanup_git(); $this->generate_compose_file(); @@ -2280,26 +2286,31 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue /** * Build-time names go into shell and Docker build commands, so they must be valid when the - * deployment builds an image. Runtime-only variables only go into the .env file: existing ones - * with names that new variables can no longer use (such as my-var) keep working, unless the - * name would break a .env line. Deployments without a build step (Docker image) treat - * build-time variables the same way, because no build receives them. + * deployment builds an image ($buildsImage, checked right before the build steps). Restarts and + * deployments that reuse an existing image do not pass them to a build, so handle() only warns. + * Runtime-only variables only go into the .env file: existing ones with names that new variables + * can no longer use (such as my-var) keep working, unless the name would break a .env line. + * Deployments without a build step (Docker image) never validate build-time names. */ - private function validateDeploymentEnvironmentVariableKeys(): void + private function validateDeploymentEnvironmentVariableKeys(bool $buildsImage = false): void { $environmentVariables = $this->pull_request_id === 0 ? $this->application->environment_variables()->get(['key', 'is_buildtime', 'is_runtime']) : $this->application->environment_variables_preview()->get(['key', 'is_buildtime', 'is_runtime']); - $passesBuildtimeVariables = $this->deploymentPassesBuildtimeVariables(); + $validatesBuildtimeKeys = $buildsImage && $this->deploymentPassesBuildtimeVariables(); foreach ($environmentVariables as $environmentVariable) { $key = (string) $environmentVariable->key; $isEnvFileSafe = $key !== '' && strpbrk($key, "=\n\r\0") === false; - if (($environmentVariable->is_buildtime && $passesBuildtimeVariables) || ! $isEnvFileSafe) { + if (($environmentVariable->is_buildtime && $validatesBuildtimeKeys) || ! $isEnvFileSafe) { $this->validatedBuildtimeEnvironmentVariableKey($key, 'the deployment environment'); continue; } + // The deployment start already logged the warnings. + if ($buildsImage) { + continue; + } if (! ValidationPatterns::isValidEnvironmentVariableKey($key)) { $this->logLegacyEnvironmentVariableKey($key, (bool) $environmentVariable->is_buildtime, (bool) $environmentVariable->is_runtime); } @@ -2325,7 +2336,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue $displayKey = ValidationPatterns::displayShellEnvironmentVariableKey($key); if ($isBuildtime) { - $this->application_deployment_queue->addLogEntry("⚠️ Build-time variable {$displayKey} uses a name that new variables cannot use. This deployment does not build an image, so it is not used as a build-time variable. Rename it before you use it in a build.", 'stderr'); + $this->application_deployment_queue->addLogEntry("⚠️ Build-time variable {$displayKey} uses a name that new variables cannot use. It is not used as a build-time variable unless the deployment builds an image, and that build fails until you rename it.", 'stderr'); } if ($isRuntime) { $this->application_deployment_queue->addLogEntry("⚠️ Runtime variable {$displayKey} uses a name that new variables cannot use. It is still passed to the container, but shell scripts cannot read it.", 'stderr'); @@ -2665,6 +2676,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue $this->check_git_if_build_needed(); $this->clone_repository(); $this->cleanup_git(); + $this->validateDeploymentEnvironmentVariableKeys(buildsImage: true); if ($this->application->build_pack === 'nixpacks') { $this->generate_nixpacks_confs(); } @@ -4875,7 +4887,9 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); $this->generate_env_variables(); } - $variables = $this->env_args; + // Invalid names stay out of the helper container: a build fails on them before it starts, + // and restarts or deployments that reuse an image do not need them. + $variables = $this->env_args->filter(fn ($value, $key): bool => ValidationPatterns::isValidEnvironmentVariableKey((string) $key)); if ($this->build_pack === 'railpack') { $variables = $this->without_reserved_docker_client_variables($variables); diff --git a/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php b/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php index d9ad7f9f5c..076bbae527 100644 --- a/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php +++ b/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php @@ -1026,7 +1026,7 @@ it('rejects unsafe legacy keys before generating BuildKit secret flags', functio expect($job->recordedCommands)->toBeEmpty(); }); -it('rejects an unsafe stored key before running a deployment command', function () { +it('rejects an unsafe stored build-time key before running a build command', function () { [$application, $server] = makeDeploymentControlVarFixture(); $environmentVariable = createApplicationEnvironmentVariable($application, [ 'key' => 'SAFE_KEY', @@ -1036,7 +1036,7 @@ it('rejects an unsafe stored key before running a deployment command', function [$job, $reflection] = makeControlVarFilteringJob($application->fresh(), $server); - expect(fn () => invokeDeploymentJobMethod($job, $reflection, 'validateDeploymentEnvironmentVariableKeys')) + expect(fn () => invokeDeploymentJobMethod($job, $reflection, 'validateDeploymentEnvironmentVariableKeys', true)) ->toThrow(DeploymentException::class, 'Invalid environment variable name from the deployment environment'); expect($job->recordedCommands)->toBeEmpty(); @@ -1061,7 +1061,7 @@ it('keeps deploying existing runtime-only variables whose names new variables ca expect($job->recordedCommands)->toBeEmpty(); }); -it('rejects existing variable names that would break the .env file or build commands', function (string $key, bool $isBuildtime) { +it('rejects existing variable names that would break the .env file or build commands', function (string $key, bool $isBuildtime, bool $buildsImage) { [$application, $server] = makeDeploymentControlVarFixture(); $environmentVariable = createApplicationEnvironmentVariable($application, [ 'key' => 'SAFE_KEY', @@ -1072,14 +1072,60 @@ it('rejects existing variable names that would break the .env file or build comm [$job, $reflection] = makeControlVarFilteringJob($application->fresh(), $server); - expect(fn () => invokeDeploymentJobMethod($job, $reflection, 'validateDeploymentEnvironmentVariableKeys')) + expect(fn () => invokeDeploymentJobMethod($job, $reflection, 'validateDeploymentEnvironmentVariableKeys', $buildsImage)) ->toThrow(DeploymentException::class, 'Invalid environment variable name from the deployment environment'); })->with([ - 'runtime-only name with =' => ['A=B', false], - 'runtime-only name with a newline' => ["A\nB", false], - 'build-time name with a hyphen' => ['my-var', true], + 'runtime-only name with = before the build is decided' => ['A=B', false, false], + 'runtime-only name with a newline before the build is decided' => ["A\nB", false, false], + 'build-time name with a hyphen when the deployment builds' => ['my-var', true, true], ]); +it('does not fail restarts or deployments that reuse an image on invalid build-time names', function (string $buildPack) { + [$application, $server] = makeDeploymentControlVarFixture(['build_pack' => $buildPack]); + $environmentVariable = createApplicationEnvironmentVariable($application, [ + 'key' => 'SAFE_KEY', + 'value' => 'secret', + 'is_buildtime' => true, + ]); + DB::table('environment_variables')->where('id', $environmentVariable->id)->update(['key' => 'MY-VAR']); + + [$job, $reflection] = makeControlVarFilteringJob($application->fresh(), $server, ['build_pack' => $buildPack]); + // handle() runs this check before it is known whether the deployment builds an image. + invokeDeploymentJobMethod($job, $reflection, 'validateDeploymentEnvironmentVariableKeys'); + + expect(collect($job->recordedLogEntries)->implode("\n")) + ->toContain('MY-VAR') + ->toContain('Suggested name: MY_VAR') + ->not->toContain('Invalid environment variable name'); + expect($job->recordedCommands)->toBeEmpty(); +})->with(['dockerfile', 'nixpacks', 'static', 'railpack', 'dockercompose']); + +it('keeps invalid build-time names out of the helper container so restarts with build secrets work', function () { + [$application, $server] = makeDeploymentControlVarFixture(['build_pack' => 'nixpacks']); + $application->settings()->update(['use_build_secrets' => true]); + createApplicationEnvironmentVariable($application, [ + 'key' => 'SAFE_KEY', + 'value' => 'safe-build-value', + 'is_buildtime' => true, + ]); + $invalidVariable = createApplicationEnvironmentVariable($application, [ + 'key' => 'OTHER_KEY', + 'value' => 'invalid-build-value', + 'is_buildtime' => true, + ]); + DB::table('environment_variables')->where('id', $invalidVariable->id)->update(['key' => 'MY-VAR']); + + [$job, $reflection] = makeControlVarFilteringJob($application->fresh(), $server, ['build_pack' => 'nixpacks']); + + $envFlags = invokeDeploymentJobMethod($job, $reflection, 'generate_docker_env_flags_for_secrets'); + + expect($envFlags) + ->toContain("-e 'SAFE_KEY=safe-build-value'") + ->toContain('COOLIFY_BUILD_SECRETS_HASH=') + ->not->toContain('MY-VAR') + ->not->toContain('invalid-build-value'); +}); + it('warns instead of failing for invalid build-time names when the deployment does not build an image', function (bool $isRuntime) { [$application, $server] = makeDeploymentControlVarFixture(['build_pack' => 'dockerimage']); $environmentVariable = createApplicationEnvironmentVariable($application, [ @@ -1147,7 +1193,7 @@ it('fails with a clear message for invalid build-time names when the deployment [$job, $reflection] = makeControlVarFilteringJob($application->fresh(), $server, ['build_pack' => $buildPack]); - expect(fn () => invokeDeploymentJobMethod($job, $reflection, 'validateDeploymentEnvironmentVariableKeys')) + expect(fn () => invokeDeploymentJobMethod($job, $reflection, 'validateDeploymentEnvironmentVariableKeys', true)) ->toThrow(DeploymentException::class, 'Invalid environment variable name from the deployment environment: my-var'); expect(collect($job->recordedLogEntries)->implode("\n")) ->toContain('Build-time variable names must start with a letter or underscore');