diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 134b705bca..9987bc960b 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -311,6 +311,13 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return; } + try { + $this->validateDeploymentEnvironmentVariableKeys(); + } catch (Exception $e) { + $this->fail($e); + throw $e; + } + $this->application_deployment_queue->update([ 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, 'horizon_job_worker' => gethostname(), @@ -2049,6 +2056,17 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue } } + private function validateDeploymentEnvironmentVariableKeys(): void + { + $environmentVariables = $this->pull_request_id === 0 + ? $this->application->environment_variables()->get(['key']) + : $this->application->environment_variables_preview()->get(['key']); + + foreach ($environmentVariables as $environmentVariable) { + $this->validatedBuildtimeEnvironmentVariableKey((string) $environmentVariable->key, 'the deployment environment'); + } + } + private function logInvalidBuildtimeEnvironmentVariableKey(string $key, string $origin): void { $displayKey = ValidationPatterns::displayShellEnvironmentVariableKey($key); @@ -2102,6 +2120,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue $this->application_deployment_queue->addLogEntry('Creating build-time .env file in /artifacts (outside Docker context).', hidden: true); $this->execute_remote_command([ executeInDocker($this->deployment_uuid, "echo '$envs_base64' | base64 -d | tee ".self::BUILD_TIME_ENV_PATH.' > /dev/null'), + 'skip_command_log' => true, ]); if (isDev()) { @@ -2147,6 +2166,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue $contents_base64 = base64_encode($contents); $this->execute_remote_command([ executeInDocker($this->deployment_uuid, "echo '$contents_base64' | base64 -d | tee {$path} > /dev/null"), + 'skip_command_log' => true, ]); } @@ -3024,6 +3044,8 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return 'env '.$variables ->map(function ($value, $key) { + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $key, 'the Railpack environment'); + return escapeShellValue("{$key}={$value}"); }) ->implode(' ').' '; @@ -3037,6 +3059,8 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return ' '.$variables ->map(function ($value, $key) { + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $key, 'the Railpack environment'); + return '--secret '.escapeShellValue("id={$key},env={$key}"); }) ->implode(' '); @@ -3951,7 +3975,8 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); // Traditional build args approach - generate COOLIFY_ variables locally $coolify_envs = $this->generate_coolify_env_variables(forBuildTime: true); $coolify_envs->each(function ($value, $key) { - $this->build_args->push("--build-arg '{$key}'"); + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $key, 'the Coolify build environment'); + $this->build_args->push('--build-arg '.escapeshellarg($key)); }); } @@ -4506,7 +4531,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); $this->build_args = generateDockerBuildArgs($vars_with_metadata); if ($secrets_hash) { - $this->build_args->push("--build-arg COOLIFY_BUILD_SECRETS_HASH={$secrets_hash}"); + $this->build_args->push('--build-arg '.escapeshellarg("COOLIFY_BUILD_SECRETS_HASH={$secrets_hash}")); } } } @@ -4543,6 +4568,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); // Map to simple array format for the helper function $vars_array = $variables->map(function ($value, $key) use ($env_vars) { + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $key, 'the build secret environment'); $env = $env_vars->firstWhere('key', $key); return [ @@ -4553,7 +4579,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); }); $env_flags = generateDockerEnvFlags($vars_array); - $env_flags .= " -e COOLIFY_BUILD_SECRETS_HASH={$secrets_hash}"; + $env_flags .= ' -e '.escapeshellarg("COOLIFY_BUILD_SECRETS_HASH={$secrets_hash}"); return $env_flags; } @@ -4568,7 +4594,9 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); $this->build_secrets = $variables ->map(function ($value, $key) { - return "--secret id={$key},env={$key}"; + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $key, 'the build secret environment'); + + return '--secret '.escapeshellarg("id={$key},env={$key}"); }) ->implode(' '); @@ -4659,11 +4687,8 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); ->where('is_buildtime', true) ->get(); foreach ($envs as $env) { - if (data_get($env, 'is_multiline') === true) { - $argsToInsert->push("ARG {$env->key}"); - } else { - $argsToInsert->push("ARG {$env->key}=".escapeBashEnvValue($this->resolve_environment_variable_raw($env))); - } + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $env->key, 'the generated Dockerfile'); + $argsToInsert->push("ARG {$key}"); } // Add Coolify variables as ARGs if ($this->coolify_variables) { @@ -4681,11 +4706,8 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); ->where('is_buildtime', true) ->get(); foreach ($envs as $env) { - if (data_get($env, 'is_multiline') === true) { - $argsToInsert->push("ARG {$env->key}"); - } else { - $argsToInsert->push("ARG {$env->key}=".escapeBashEnvValue($this->resolve_environment_variable_raw($env))); - } + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $env->key, 'the generated Dockerfile'); + $argsToInsert->push("ARG {$key}"); } // Add Coolify variables as ARGs if ($this->coolify_variables) { @@ -4805,7 +4827,11 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); } // Generate mount strings for all secrets - $mountStrings = $variables->map(fn ($value, $key) => "--mount=type=secret,id={$key},env={$key}")->implode(' '); + $mountStrings = $variables->map(function ($value, $key) { + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $key, 'the generated Dockerfile'); + + return "--mount=type=secret,id={$key},env={$key}"; + })->implode(' '); // Add mount for the secrets hash to ensure cache invalidation $mountStrings .= ' --mount=type=secret,id=COOLIFY_BUILD_SECRETS_HASH,env=COOLIFY_BUILD_SECRETS_HASH'; @@ -4917,6 +4943,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); $argsToAdd = collect([]); foreach ($variables as $key => $value) { + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $key, 'the generated Dockerfile'); $argsToAdd->push("ARG {$key}"); } @@ -5068,6 +5095,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); $secrets = []; foreach ($variables as $key => $value) { + $key = $this->validatedBuildtimeEnvironmentVariableKey((string) $key, 'the Compose build secret environment'); $secrets[$key] = [ 'environment' => $key, ]; @@ -5084,7 +5112,7 @@ COPY ./nginx.conf /etc/nginx/conf.d/default.conf"); if (! isset($service['build']['secrets'])) { $service['build']['secrets'] = []; } - foreach ($variables as $key => $value) { + foreach (array_keys($secrets) as $key) { if (! in_array($key, $service['build']['secrets'])) { $service['build']['secrets'][] = $key; } diff --git a/bootstrap/helpers/docker.php b/bootstrap/helpers/docker.php index 1195c9ca31..6704902f88 100644 --- a/bootstrap/helpers/docker.php +++ b/bootstrap/helpers/docker.php @@ -1713,6 +1713,10 @@ function generateDockerBuildArgs($variables): Collection return $variables->map(function ($var) { $key = is_array($var) ? data_get($var, 'key') : $var->key; + if (! ValidationPatterns::isValidEnvironmentVariableKey((string) $key)) { + throw new InvalidArgumentException('Invalid environment variable key.'); + } + // Only return the key - Docker will get the value from the environment return '--build-arg '.escapeshellarg((string) $key); }); @@ -1732,19 +1736,17 @@ function generateDockerEnvFlags($variables): string ->map(function ($var) { $key = is_array($var) ? data_get($var, 'key') : $var->key; $value = is_array($var) ? data_get($var, 'value') : $var->value; - $isMultiline = is_array($var) ? data_get($var, 'is_multiline', false) : ($var->is_multiline ?? false); - if ($isMultiline) { - // For multiline variables, strip surrounding quotes and escape for bash - $raw_value = trim($value, "'"); - $escaped_value = str_replace(['\\', '"', '$', '`'], ['\\\\', '\\"', '\\$', '\\`'], $raw_value); - - return "-e {$key}=\"{$escaped_value}\""; + if (! ValidationPatterns::isValidEnvironmentVariableKey((string) $key)) { + throw new InvalidArgumentException('Invalid environment variable key.'); } - $escaped_value = escapeshellarg($value); + $isMultiline = is_array($var) ? data_get($var, 'is_multiline', false) : ($var->is_multiline ?? false); + if ($isMultiline) { + $value = trim($value, "'"); + } - return "-e {$key}={$escaped_value}"; + return '-e '.escapeshellarg("{$key}={$value}"); }) ->implode(' '); } diff --git a/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php b/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php index afec2fe9fb..e8fb9cb84d 100644 --- a/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php +++ b/tests/Feature/ApplicationDeploymentControlVarFilteringTest.php @@ -12,6 +12,7 @@ use App\Models\Server; use App\Models\Team; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\DB; use Symfony\Component\Process\Process; uses(RefreshDatabase::class); @@ -439,7 +440,7 @@ it('uses BuildKit secrets for dotted Nixpacks variables instead of invalid Docke invokeDeploymentJobMethod($job, $reflection, 'generate_build_env_variables'); expect(readDeploymentJobProperty($job, $reflection, 'dockerSecretsSupported'))->toBeTrue(); - expect(readDeploymentJobProperty($job, $reflection, 'build_secrets'))->toContain('--secret id=X.VALUE,env=X.VALUE'); + expect(readDeploymentJobProperty($job, $reflection, 'build_secrets'))->toContain("--secret 'id=X.VALUE,env=X.VALUE'"); invokeDeploymentJobMethod($job, $reflection, 'modify_dockerfile_for_secrets', '/artifacts/test-app/.nixpacks/Dockerfile'); @@ -810,6 +811,7 @@ it('filters buildpack control vars from dockerfile arg injection', function () { ]); [$job, $reflection] = makeControlVarFilteringJob($application, $server, [ + 'remote_secrets_cache' => [], 'saved_outputs' => [ 'dockerfile' => "FROM php:8.4-cli\nRUN php -v", ], @@ -817,12 +819,78 @@ it('filters buildpack control vars from dockerfile arg injection', function () { invokeDeploymentJobMethod($job, $reflection, 'add_build_env_variables_to_dockerfile'); - expect($job->writtenDockerfile)->toContain('ARG APP_ENV=production'); + expect($job->writtenDockerfile)->toContain('ARG APP_ENV'); expect($job->writtenDockerfile)->toContain('ARG COOLIFY_BUILD_SECRETS_HASH='); expect($job->writtenDockerfile)->not->toContain('ARG NIXPACKS_NODE_VERSION='); expect($job->writtenDockerfile)->not->toContain('ARG RAILPACK_NODE_VERSION='); }); +it('does not write environment values into generated Dockerfile ARG declarations', function () { + [$application, $server] = makeDeploymentControlVarFixture(); + createApplicationEnvironmentVariable($application, [ + 'key' => 'SAFE_KEY', + 'value' => "value\nRUN touch /tmp/injected", + 'is_runtime' => false, + 'is_buildtime' => true, + ]); + + [$job, $reflection] = makeControlVarFilteringJob($application, $server, [ + 'remote_secrets_cache' => [], + 'saved_outputs' => ['dockerfile' => 'FROM alpine'], + ]); + + invokeDeploymentJobMethod($job, $reflection, 'add_build_env_variables_to_dockerfile'); + + expect($job->writtenDockerfile) + ->toContain('ARG SAFE_KEY') + ->not->toContain('RUN touch /tmp/injected') + ->not->toContain('value'); +}); + +it('rejects unsafe keys before generating Railpack secret flags', function () { + [$application, $server] = makeDeploymentControlVarFixture(); + [$job, $reflection] = makeControlVarFilteringJob($application, $server); + + expect(fn () => invokeDeploymentJobMethod( + $job, + $reflection, + 'railpack_build_secret_flags', + collect(['BAD$(id)' => 'x']), + ))->toThrow(DeploymentException::class, 'Invalid environment variable name from the Railpack environment'); + + expect($job->recordedCommands)->toBeEmpty(); +}); + +it('rejects unsafe legacy keys before generating BuildKit secret flags', function () { + [$application, $server] = makeDeploymentControlVarFixture(); + [$job, $reflection] = makeControlVarFilteringJob($application, $server); + + expect(fn () => invokeDeploymentJobMethod( + $job, + $reflection, + 'generate_build_secrets', + collect(['BAD$(id)' => 'secret']), + ))->toThrow(DeploymentException::class, 'Invalid environment variable name from the build secret environment'); + + expect($job->recordedCommands)->toBeEmpty(); +}); + +it('rejects an unsafe stored key before running a deployment command', function () { + [$application, $server] = makeDeploymentControlVarFixture(); + $environmentVariable = createApplicationEnvironmentVariable($application, [ + 'key' => 'SAFE_KEY', + 'value' => 'secret', + ]); + DB::table('environment_variables')->where('id', $environmentVariable->id)->update(['key' => 'BAD$(id)']); + + [$job, $reflection] = makeControlVarFilteringJob($application->fresh(), $server); + + expect(fn () => invokeDeploymentJobMethod($job, $reflection, 'validateDeploymentEnvironmentVariableKeys')) + ->toThrow(DeploymentException::class, 'Invalid environment variable name from the deployment environment'); + + expect($job->recordedCommands)->toBeEmpty(); +}); + it('injects raw escaped remote secrets into Dockerfile args and hashes the same values', function (int $pullRequestId, bool $isPreview) { [$application, $server] = makeDeploymentControlVarFixture(); @@ -854,7 +922,8 @@ it('injects raw escaped remote secrets into Dockerfile args and hashes the same ); expect($job->writtenDockerfile) - ->toContain("ARG SECRET_TOKEN={$escapedSecret}") + ->toContain('ARG SECRET_TOKEN') + ->not->toContain($secret) ->toContain("ARG COOLIFY_BUILD_SECRETS_HASH={$expectedHash}") ->not->toContain('$$'); })->with([ diff --git a/tests/Feature/EnvironmentVariable/MultilineEnvironmentVariableTest.php b/tests/Feature/EnvironmentVariable/MultilineEnvironmentVariableTest.php index 4205c5cc2e..f71811b24a 100644 --- a/tests/Feature/EnvironmentVariable/MultilineEnvironmentVariableTest.php +++ b/tests/Feature/EnvironmentVariable/MultilineEnvironmentVariableTest.php @@ -64,8 +64,8 @@ test('generateDockerEnvFlags produces correct format', function () { $envFlags = generateDockerEnvFlags($variables); - expect($envFlags)->toContain('-e NORMAL_VAR='); - expect($envFlags)->toContain('-e MULTILINE_VAR="'); + expect($envFlags)->toContain("-e 'NORMAL_VAR=value'"); + expect($envFlags)->toContain("-e 'MULTILINE_VAR=line1"); expect($envFlags)->toContain('line1'); expect($envFlags)->toContain('line2'); }); @@ -78,20 +78,41 @@ test('generateDockerEnvFlags works with collection input', function () { $envFlags = generateDockerEnvFlags($variables); expect($envFlags)->toBeString(); - expect($envFlags)->toContain('-e VAR1='); - expect($envFlags)->toContain('-e VAR2="'); + expect($envFlags)->toContain("-e 'VAR1=value1'"); + expect($envFlags)->toContain("-e 'VAR2=multiline"); }); -test('generateDockerBuildArgs escapes legacy keys', function () { - $variables = [ - ['key' => 'BAD$(id)', 'value' => '1'], - ['key' => "BAD'KEY", 'value' => '1'], - ]; +test('docker argument helpers reject unsafe legacy keys', function (string $key) { + $variables = [['key' => $key, 'value' => '1']]; - $buildArgs = generateDockerBuildArgs($variables); + expect(fn () => generateDockerBuildArgs($variables))->toThrow(InvalidArgumentException::class); + expect(fn () => generateDockerEnvFlags($variables))->toThrow(InvalidArgumentException::class); +})->with([ + 'semicolon' => 'BAD;id', + 'command substitution' => 'BAD$(id)', + 'backticks' => 'BAD`id`', + 'pipe' => 'BAD|id', + 'logical operator' => 'BAD&&id', + 'single quote' => "BAD'KEY", + 'double quote' => 'BAD"KEY', + 'backslash' => 'BAD\\KEY', + 'space' => 'BAD KEY', + 'newline' => "BAD\nKEY", + 'control character' => "BAD\x1BKEY", + 'equals sign' => 'BAD=KEY', + 'option prefix' => '--BAD', + 'parameter expansion' => 'BAD${PATH}', + 'braces' => 'BAD{KEY}', + 'unicode lookalike' => 'BＡD', + 'empty' => '', + 'leading digit' => '1BAD', +]); - expect($buildArgs->values()->toArray())->toBe([ - "--build-arg 'BAD$(id)'", - "--build-arg 'BAD'\''KEY'", +test('generateDockerEnvFlags quotes the complete assignment', function () { + $flags = generateDockerEnvFlags([ + ['key' => 'NORMAL_VAR', 'value' => 'value with spaces', 'is_multiline' => false], + ['key' => 'MULTILINE_VAR', 'value' => "line1\nline2", 'is_multiline' => true], ]); + + expect($flags)->toBe("-e 'NORMAL_VAR=value with spaces' -e 'MULTILINE_VAR=line1\nline2'"); }); diff --git a/tests/Feature/EnvironmentVariableKeyValidationTest.php b/tests/Feature/EnvironmentVariableKeyValidationTest.php index 4f41ed3d68..cf8db23f32 100644 --- a/tests/Feature/EnvironmentVariableKeyValidationTest.php +++ b/tests/Feature/EnvironmentVariableKeyValidationTest.php @@ -21,12 +21,24 @@ it('allows Docker-compatible environment variable keys in the add form', functio return data_get($data, 'key') === $key || data_get($data, '0.key') === $key; }); })->with([ - 'starts with digit' => '1BAD', - 'hyphen' => 'BAD-KEY', + 'letters and underscore' => 'APP_ENV', 'dot' => 'node.name', 'uppercase dots' => 'XPACK.SECURITY.ENABLED', ]); +it('rejects environment variable keys that are not portable identifiers in the add form', function (string $key) { + Livewire::test(Add::class) + ->set('key', $key) + ->set('value', 'value') + ->call('submit') + ->assertHasErrors(['key' => 'regex']); +})->with([ + 'starts with digit' => '1BAD', + 'hyphen' => 'BAD-KEY', + 'command substitution' => 'BAD$(id)', + 'semicolon' => 'BAD;KEY', +]); + it('trims surrounding whitespace in environment variable keys in the add form', function () { Livewire::test(Add::class) ->set('key', ' node.name ') diff --git a/tests/Unit/EnvironmentVariableBuildpackControlTest.php b/tests/Unit/EnvironmentVariableBuildpackControlTest.php index c24956c0d0..8068748232 100644 --- a/tests/Unit/EnvironmentVariableBuildpackControlTest.php +++ b/tests/Unit/EnvironmentVariableBuildpackControlTest.php @@ -50,25 +50,29 @@ it('allows Docker-compatible environment variable keys on the model', function ( expect($env->key)->toBe($key); })->with([ - 'starts with digit' => '1BAD', - 'hyphen' => 'BAD-KEY', + 'letters and underscore' => 'APP_ENV', 'dot' => 'node.name', 'uppercase dots' => 'XPACK.SECURITY.ENABLED', - 'semicolon' => 'BAD;KEY', ]); -it('rejects environment variable keys Docker cannot represent on the model', function () { +it('rejects environment variable keys Docker cannot represent on the model', function (string $key) { $env = new EnvironmentVariable; - expect(function () use ($env) { - $env->key = 'BAD=KEY'; - })->toThrow(InvalidArgumentException::class, 'Docker-compatible'); -}); + expect(function () use ($env, $key) { + $env->key = $key; + })->toThrow(InvalidArgumentException::class, 'must start with a letter or underscore'); +})->with([ + 'equals' => 'BAD=KEY', + 'starts with digit' => '1BAD', + 'hyphen' => 'BAD-KEY', + 'semicolon' => 'BAD;KEY', + 'command substitution' => 'BAD$(id)', +]); it('rejects shared environment variable keys Docker cannot represent on the model', function () { $env = new SharedEnvironmentVariable; expect(function () use ($env) { $env->key = 'BAD=KEY'; - })->toThrow(InvalidArgumentException::class, 'Docker-compatible'); + })->toThrow(InvalidArgumentException::class, 'must start with a letter or underscore'); });