diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index 824101be8..bdb6f8d90 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -2876,8 +2876,12 @@ public function update_env_by_uuid(Request $request) $this->authorize('manageEnvironment', $application); + if ($request->has('key')) { + $request->merge(['key' => ValidationPatterns::normalizeEnvironmentVariableKey((string) $request->key)]); + } + $validator = customApiValidator($request->all(), [ - 'key' => 'string|required', + 'key' => ValidationPatterns::environmentVariableKeyRules(), 'value' => 'string|nullable', 'is_preview' => 'boolean', 'is_literal' => 'boolean', @@ -3100,12 +3104,18 @@ public function create_bulk_envs(Request $request) ], 400); } $bulk_data = collect($bulk_data)->map(function ($item) { - return collect($item)->only(['key', 'value', 'is_preview', 'is_literal', 'is_multiline', 'is_shown_once', 'is_runtime', 'is_buildtime', 'comment']); + $item = collect($item)->only(['key', 'value', 'is_preview', 'is_literal', 'is_multiline', 'is_shown_once', 'is_runtime', 'is_buildtime', 'comment']); + + if ($item->has('key')) { + $item->put('key', ValidationPatterns::normalizeEnvironmentVariableKey((string) $item->get('key'))); + } + + return $item; }); $returnedEnvs = collect(); foreach ($bulk_data as $item) { $validator = customApiValidator($item, [ - 'key' => 'string|required', + 'key' => ValidationPatterns::environmentVariableKeyRules(), 'value' => 'string|nullable', 'is_preview' => 'boolean', 'is_literal' => 'boolean', @@ -3302,8 +3312,12 @@ public function create_env(Request $request) $this->authorize('manageEnvironment', $application); + if ($request->has('key')) { + $request->merge(['key' => ValidationPatterns::normalizeEnvironmentVariableKey((string) $request->key)]); + } + $validator = customApiValidator($request->all(), [ - 'key' => 'string|required', + 'key' => ValidationPatterns::environmentVariableKeyRules(), 'value' => 'string|nullable', 'is_preview' => 'boolean', 'is_literal' => 'boolean', diff --git a/app/Http/Controllers/Api/DatabasesController.php b/app/Http/Controllers/Api/DatabasesController.php index bceef4d39..9a463e8d3 100644 --- a/app/Http/Controllers/Api/DatabasesController.php +++ b/app/Http/Controllers/Api/DatabasesController.php @@ -3133,8 +3133,12 @@ public function update_env_by_uuid(Request $request) $this->authorize('manageEnvironment', $database); + if ($request->has('key')) { + $request->merge(['key' => ValidationPatterns::normalizeEnvironmentVariableKey((string) $request->key)]); + } + $validator = customApiValidator($request->all(), [ - 'key' => 'string|required', + 'key' => ValidationPatterns::environmentVariableKeyRules(), 'value' => 'string|nullable', 'is_literal' => 'boolean', 'is_multiline' => 'boolean', @@ -3281,8 +3285,12 @@ public function create_bulk_envs(Request $request) $updatedEnvs = collect(); foreach ($bulk_data as $item) { + if (array_key_exists('key', $item)) { + $item['key'] = ValidationPatterns::normalizeEnvironmentVariableKey((string) $item['key']); + } + $validator = customApiValidator($item, [ - 'key' => 'string|required', + 'key' => ValidationPatterns::environmentVariableKeyRules(), 'value' => 'string|nullable', 'is_literal' => 'boolean', 'is_multiline' => 'boolean', @@ -3399,8 +3407,12 @@ public function create_env(Request $request) $this->authorize('manageEnvironment', $database); + if ($request->has('key')) { + $request->merge(['key' => ValidationPatterns::normalizeEnvironmentVariableKey((string) $request->key)]); + } + $validator = customApiValidator($request->all(), [ - 'key' => 'string|required', + 'key' => ValidationPatterns::environmentVariableKeyRules(), 'value' => 'string|nullable', 'is_literal' => 'boolean', 'is_multiline' => 'boolean', diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index 89c99ff9f..32137b866 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -1251,8 +1251,12 @@ public function update_env_by_uuid(Request $request) $this->authorize('manageEnvironment', $service); + if ($request->has('key')) { + $request->merge(['key' => ValidationPatterns::normalizeEnvironmentVariableKey((string) $request->key)]); + } + $validator = customApiValidator($request->all(), [ - 'key' => 'string|required', + 'key' => ValidationPatterns::environmentVariableKeyRules(), 'value' => 'string|nullable', 'is_literal' => 'boolean', 'is_multiline' => 'boolean', @@ -1400,8 +1404,12 @@ public function create_bulk_envs(Request $request) $updatedEnvs = collect(); foreach ($bulk_data as $item) { + if (array_key_exists('key', $item)) { + $item['key'] = ValidationPatterns::normalizeEnvironmentVariableKey((string) $item['key']); + } + $validator = customApiValidator($item, [ - 'key' => 'string|required', + 'key' => ValidationPatterns::environmentVariableKeyRules(), 'value' => 'string|nullable', 'is_literal' => 'boolean', 'is_multiline' => 'boolean', @@ -1519,8 +1527,12 @@ public function create_env(Request $request) $this->authorize('manageEnvironment', $service); + if ($request->has('key')) { + $request->merge(['key' => ValidationPatterns::normalizeEnvironmentVariableKey((string) $request->key)]); + } + $validator = customApiValidator($request->all(), [ - 'key' => 'string|required', + 'key' => ValidationPatterns::environmentVariableKeyRules(), 'value' => 'string|nullable', 'is_literal' => 'boolean', 'is_multiline' => 'boolean', diff --git a/app/Support/ValidationPatterns.php b/app/Support/ValidationPatterns.php index bdb8654b9..2f553a4b9 100644 --- a/app/Support/ValidationPatterns.php +++ b/app/Support/ValidationPatterns.php @@ -95,9 +95,9 @@ class ValidationPatterns /** * Pattern for Docker-compatible environment variable keys. - * Docker environment entries are KEY=value strings, so keys must be non-empty and cannot contain '=' or NUL. + * Environment variable keys are later interpolated into shell commands as Docker build args, so only shell-safe identifier characters are allowed. */ - public const ENVIRONMENT_VARIABLE_KEY_PATTERN = '/\A[^=\x00]+\z/u'; + public const ENVIRONMENT_VARIABLE_KEY_PATTERN = '/\A[A-Za-z_][A-Za-z0-9_.]*\z/u'; /** * Pattern for SQL-safe unquoted database identifiers (usernames, database names). @@ -164,7 +164,7 @@ public static function environmentVariableKeyRules(bool $required = true, int $m public static function environmentVariableKeyMessages(string $field = 'key', string $label = 'key'): array { return [ - "{$field}.regex" => "The {$label} must be a non-empty Docker-compatible environment variable key and cannot contain '=' or NUL characters.", + "{$field}.regex" => "The {$label} must start with a letter or underscore and may only contain letters, numbers, underscores, and dots.", "{$field}.max" => "The {$label} may not be greater than :max characters.", ]; } diff --git a/bootstrap/helpers/docker.php b/bootstrap/helpers/docker.php index 1b389c77c..2f7cc95ef 100644 --- a/bootstrap/helpers/docker.php +++ b/bootstrap/helpers/docker.php @@ -1363,7 +1363,7 @@ function generateDockerBuildArgs($variables): Collection $key = is_array($var) ? data_get($var, 'key') : $var->key; // Only return the key - Docker will get the value from the environment - return "--build-arg {$key}"; + return '--build-arg '.escapeshellarg((string) $key); }); } diff --git a/tests/Feature/Api/EnvironmentVariableUpdateApiTest.php b/tests/Feature/Api/EnvironmentVariableUpdateApiTest.php index 1ff528bbf..14821756e 100644 --- a/tests/Feature/Api/EnvironmentVariableUpdateApiTest.php +++ b/tests/Feature/Api/EnvironmentVariableUpdateApiTest.php @@ -15,7 +15,7 @@ uses(RefreshDatabase::class); beforeEach(function () { - InstanceSettings::updateOrCreate(['id' => 0]); + InstanceSettings::unguarded(fn () => InstanceSettings::firstOrCreate(['id' => 0], ['is_api_enabled' => true])); $this->team = Team::factory()->create(); $this->user = User::factory()->create(); @@ -252,3 +252,94 @@ $response->assertJsonFragment(['uuid' => ['This field is not allowed.']]); }); }); + +describe('environment variable key validation for app and service APIs', function () { + test('rejects invalid service environment variable keys on create update and bulk', function () { + $service = Service::factory()->create([ + 'server_id' => $this->server->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + 'environment_id' => $this->environment->id, + ]); + + EnvironmentVariable::create([ + 'key' => 'SAFE_KEY', + 'value' => 'old-value', + 'resourceable_type' => Service::class, + 'resourceable_id' => $service->id, + 'is_preview' => false, + ]); + + $headers = [ + 'Authorization' => 'Bearer '.$this->bearerToken, + 'Content-Type' => 'application/json', + ]; + + $this->withHeaders($headers) + ->postJson("/api/v1/services/{$service->uuid}/envs", [ + 'key' => 'BAD$(id)', + 'value' => '1', + ]) + ->assertStatus(422); + + $this->withHeaders($headers) + ->patchJson("/api/v1/services/{$service->uuid}/envs", [ + 'key' => 'BAD$(id)', + 'value' => '1', + ]) + ->assertStatus(422); + + $this->withHeaders($headers) + ->patchJson("/api/v1/services/{$service->uuid}/envs/bulk", [ + 'data' => [[ + 'key' => 'BAD$(id)', + 'value' => '1', + ]], + ]) + ->assertStatus(422); + }); + + test('rejects invalid application environment variable keys on create update and bulk', function () { + $application = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + ]); + + EnvironmentVariable::create([ + 'key' => 'SAFE_KEY', + 'value' => 'old-value', + 'resourceable_type' => Application::class, + 'resourceable_id' => $application->id, + 'is_preview' => false, + ]); + + $headers = [ + 'Authorization' => 'Bearer '.$this->bearerToken, + 'Content-Type' => 'application/json', + ]; + + $this->withHeaders($headers) + ->postJson("/api/v1/applications/{$application->uuid}/envs", [ + 'key' => 'BAD$(id)', + 'value' => '1', + ]) + ->assertStatus(422); + + $this->withHeaders($headers) + ->patchJson("/api/v1/applications/{$application->uuid}/envs", [ + 'key' => 'BAD$(id)', + 'value' => '1', + ]) + ->assertStatus(422); + + $this->withHeaders($headers) + ->patchJson("/api/v1/applications/{$application->uuid}/envs/bulk", [ + 'data' => [[ + 'key' => 'BAD$(id)', + 'value' => '1', + ]], + ]) + ->assertStatus(422); + }); +}); diff --git a/tests/Feature/DatabaseEnvironmentVariableApiTest.php b/tests/Feature/DatabaseEnvironmentVariableApiTest.php index f3297cf17..1f1d46483 100644 --- a/tests/Feature/DatabaseEnvironmentVariableApiTest.php +++ b/tests/Feature/DatabaseEnvironmentVariableApiTest.php @@ -14,7 +14,7 @@ uses(RefreshDatabase::class); beforeEach(function () { - InstanceSettings::updateOrCreate(['id' => 0]); + InstanceSettings::unguarded(fn () => InstanceSettings::firstOrCreate(['id' => 0], ['is_api_enabled' => true])); $this->team = Team::factory()->create(); $this->user = User::factory()->create(); @@ -344,3 +344,44 @@ function createDatabase($context): StandalonePostgresql $response->assertStatus(404); }); }); + +describe('environment variable key validation for database APIs', function () { + test('rejects invalid database environment variable keys on create update and bulk', function () { + $database = createDatabase($this); + + EnvironmentVariable::create([ + 'key' => 'SAFE_KEY', + 'value' => 'old-value', + 'resourceable_type' => StandalonePostgresql::class, + 'resourceable_id' => $database->id, + ]); + + $headers = [ + 'Authorization' => 'Bearer '.$this->bearerToken, + 'Content-Type' => 'application/json', + ]; + + $this->withHeaders($headers) + ->postJson("/api/v1/databases/{$database->uuid}/envs", [ + 'key' => 'BAD$(id)', + 'value' => '1', + ]) + ->assertStatus(422); + + $this->withHeaders($headers) + ->patchJson("/api/v1/databases/{$database->uuid}/envs", [ + 'key' => 'BAD$(id)', + 'value' => '1', + ]) + ->assertStatus(422); + + $this->withHeaders($headers) + ->patchJson("/api/v1/databases/{$database->uuid}/envs/bulk", [ + 'data' => [[ + 'key' => 'BAD$(id)', + 'value' => '1', + ]], + ]) + ->assertStatus(422); + }); +}); diff --git a/tests/Feature/EnvironmentVariable/MultilineEnvironmentVariableTest.php b/tests/Feature/EnvironmentVariable/MultilineEnvironmentVariableTest.php index 453e11109..4205c5cc2 100644 --- a/tests/Feature/EnvironmentVariable/MultilineEnvironmentVariableTest.php +++ b/tests/Feature/EnvironmentVariable/MultilineEnvironmentVariableTest.php @@ -9,8 +9,8 @@ $buildArgs = generateDockerBuildArgs($variables); // Docker gets values from the environment, so only keys should be in build args - expect($buildArgs->first())->toBe('--build-arg SSH_PRIVATE_KEY'); - expect($buildArgs->last())->toBe('--build-arg REGULAR_VAR'); + expect($buildArgs->first())->toBe("--build-arg 'SSH_PRIVATE_KEY'"); + expect($buildArgs->last())->toBe("--build-arg 'REGULAR_VAR'"); }); test('generateDockerBuildArgs works with collection of objects', function () { @@ -22,8 +22,8 @@ $buildArgs = generateDockerBuildArgs($variables); expect($buildArgs)->toHaveCount(2); expect($buildArgs->values()->toArray())->toBe([ - '--build-arg VAR1', - '--build-arg VAR2', + "--build-arg 'VAR1'", + "--build-arg 'VAR2'", ]); }); @@ -38,7 +38,7 @@ // The collection must be imploded to a string for command interpolation // This was the bug: Collection was interpolated as JSON instead of a space-separated string $argsString = $buildArgs->implode(' '); - expect($argsString)->toBe('--build-arg COOLIFY_URL --build-arg COOLIFY_BRANCH'); + expect($argsString)->toBe("--build-arg 'COOLIFY_URL' --build-arg 'COOLIFY_BRANCH'"); // Verify it does NOT produce JSON when cast to string expect($argsString)->not->toContain('{'); @@ -53,7 +53,7 @@ $buildArgs = generateDockerBuildArgs($variables); $arg = $buildArgs->first(); - expect($arg)->toBe('--build-arg NO_FLAG_VAR'); + expect($arg)->toBe("--build-arg 'NO_FLAG_VAR'"); }); test('generateDockerEnvFlags produces correct format', function () { @@ -81,3 +81,17 @@ expect($envFlags)->toContain('-e VAR1='); expect($envFlags)->toContain('-e VAR2="'); }); + +test('generateDockerBuildArgs escapes legacy keys', function () { + $variables = [ + ['key' => 'BAD$(id)', 'value' => '1'], + ['key' => "BAD'KEY", 'value' => '1'], + ]; + + $buildArgs = generateDockerBuildArgs($variables); + + expect($buildArgs->values()->toArray())->toBe([ + "--build-arg 'BAD$(id)'", + "--build-arg 'BAD'\''KEY'", + ]); +}); diff --git a/tests/Unit/ValidationPatternsTest.php b/tests/Unit/ValidationPatternsTest.php index a959b18d5..2b5763177 100644 --- a/tests/Unit/ValidationPatternsTest.php +++ b/tests/Unit/ValidationPatternsTest.php @@ -132,24 +132,31 @@ ->not->toContain('required'); }); -it('accepts Docker-compatible environment variable keys', function (string $key) { +it('accepts shell-safe environment variable keys', function (string $key) { expect(ValidationPatterns::isValidEnvironmentVariableKey($key))->toBeTrue(); })->with([ 'letters' => 'APP_ENV', 'leading underscore' => '_TOKEN', 'railpack control variable' => 'RAILPACK_NODE_VERSION', 'digits after first character' => 'NODE_VERSION_20', - 'starts with digit' => '1BAD', - 'hyphen' => 'BAD-KEY', - 'dot' => 'node.name', + 'lowercase' => 'node_version', + 'dot notation' => 'node.name', 'uppercase dots' => 'XPACK.SECURITY.ENABLED', - 'semicolon' => 'BAD;KEY', - 'space' => 'BAD KEY', ]); -it('rejects environment variable keys Docker cannot represent', function (string $key) { +it('rejects invalid environment variable keys', function (string $key) { expect(ValidationPatterns::isValidEnvironmentVariableKey($key))->toBeFalse(); })->with([ + 'starts with digit' => '1BAD', + 'hyphen' => 'BAD-KEY', + 'semicolon' => 'BAD;KEY', + 'space' => 'BAD KEY', + 'command substitution' => 'BAD$(id)', + 'backticks' => 'BAD`id`', + 'pipe' => 'BAD|id', + 'ampersand' => 'BAD&id', + 'newline' => 'BAD +KEY', 'equals' => 'BAD=KEY', 'empty' => '', ]); @@ -164,7 +171,7 @@ }); it('normalizes environment variable keys by trimming surrounding whitespace', function () { - expect(ValidationPatterns::normalizeEnvironmentVariableKey(' node.name '))->toBe('node.name'); + expect(ValidationPatterns::normalizeEnvironmentVariableKey(' APP_ENV '))->toBe('APP_ENV'); }); it('normalizes environment variable keys before model validation', function () {