From 468d5fe7d77dfe1f1f34770a81e45062c272c92d Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 7 Nov 2025 14:03:19 +0100 Subject: [PATCH] refactor: improve docker compose validation and transaction handling in StackForm --- app/Livewire/Project/Service/StackForm.php | 25 ++++++++++------ bootstrap/helpers/parsers.php | 30 +++++++++++++------- tests/Unit/VolumeArrayFormatSecurityTest.php | 30 ++++++++++++++++++++ tests/Unit/VolumeSecurityTest.php | 21 ++++++++++++++ 4 files changed, 88 insertions(+), 18 deletions(-) diff --git a/app/Livewire/Project/Service/StackForm.php b/app/Livewire/Project/Service/StackForm.php index 85cd21a7f..8a7b6e090 100644 --- a/app/Livewire/Project/Service/StackForm.php +++ b/app/Livewire/Project/Service/StackForm.php @@ -5,6 +5,7 @@ use App\Models\Service; use App\Support\ValidationPatterns; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\DB; use Livewire\Component; class StackForm extends Component @@ -22,7 +23,7 @@ class StackForm extends Component public string $dockerComposeRaw; - public string $dockerCompose; + public ?string $dockerCompose = null; public ?bool $connectToDockerNetwork = null; @@ -30,7 +31,7 @@ protected function rules(): array { $baseRules = [ 'dockerComposeRaw' => 'required', - 'dockerCompose' => 'required', + 'dockerCompose' => 'nullable', 'name' => ValidationPatterns::nameRules(), 'description' => ValidationPatterns::descriptionRules(), 'connectToDockerNetwork' => 'nullable', @@ -140,18 +141,26 @@ public function submit($notify = true) $this->validate(); $this->syncData(true); - // Validate for command injection BEFORE saving to database + // Validate for command injection BEFORE any database operations validateDockerComposeForInjection($this->service->docker_compose_raw); - $this->service->save(); - $this->service->saveExtraFields($this->fields); - $this->service->parse(); - $this->service->refresh(); - $this->service->saveComposeConfigs(); + // Use transaction to ensure atomicity - if parse fails, save is rolled back + DB::transaction(function () { + $this->service->save(); + $this->service->saveExtraFields($this->fields); + $this->service->parse(); + $this->service->refresh(); + $this->service->saveComposeConfigs(); + }); + $this->dispatch('refreshEnvs'); $this->dispatch('refreshServices'); $notify && $this->dispatch('success', 'Service saved.'); } catch (\Throwable $e) { + // On error, refresh from database to restore clean state + $this->service->refresh(); + $this->syncData(false); + return handleError($e, $this); } finally { if (is_null($this->service->config_hash)) { diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 1deec45d7..a210aa1cc 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -59,11 +59,13 @@ function validateDockerComposeForInjection(string $composeYaml): void if (isset($volume['source'])) { $source = $volume['source']; if (is_string($source)) { - // Allow simple env vars and env vars with defaults (validated in parseDockerVolumeString) + // Allow env vars and env vars with defaults (validated in parseDockerVolumeString) + // Also allow env vars followed by safe path concatenation (e.g., ${VAR}/path) $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source); $isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $source); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $source); - if (! $isSimpleEnvVar && ! $isEnvVarWithDefault) { + if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) { try { validateShellSafePath($source, 'volume source'); } catch (\Exception $e) { @@ -310,15 +312,17 @@ function parseDockerVolumeString(string $volumeString): array // Validate source path for command injection attempts // We validate the final source value after environment variable processing if ($source !== null) { - // Allow simple environment variables like ${VAR_NAME} or ${VAR} - // but validate everything else for shell metacharacters + // Allow environment variables like ${VAR_NAME} or ${VAR} + // Also allow env vars followed by safe path concatenation (e.g., ${VAR}/path) $sourceStr = is_string($source) ? $source : $source; // Skip validation for simple environment variable references - // Pattern: ${WORD_CHARS} with no special characters inside + // Pattern 1: ${WORD_CHARS} with no special characters inside + // Pattern 2: ${WORD_CHARS}/path/to/file (env var with path concatenation) $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceStr); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceStr); - if (! $isSimpleEnvVar) { + if (! $isSimpleEnvVar && ! $isEnvVarWithPath) { try { validateShellSafePath($sourceStr, 'volume source'); } catch (\Exception $e) { @@ -711,9 +715,12 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int // Validate source and target for command injection (array/long syntax) if ($source !== null && ! empty($source->value())) { $sourceValue = $source->value(); - // Allow simple environment variable references + // Allow environment variable references and env vars with path concatenation $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); - if (! $isSimpleEnvVar) { + $isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $sourceValue); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue); + + if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) { try { validateShellSafePath($sourceValue, 'volume source'); } catch (\Exception $e) { @@ -1812,9 +1819,12 @@ function serviceParser(Service $resource): Collection // Validate source and target for command injection (array/long syntax) if ($source !== null && ! empty($source->value())) { $sourceValue = $source->value(); - // Allow simple environment variable references + // Allow environment variable references and env vars with path concatenation $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); - if (! $isSimpleEnvVar) { + $isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $sourceValue); + $isEnvVarWithPath = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}[\/\w\.\-]*$/', $sourceValue); + + if (! $isSimpleEnvVar && ! $isEnvVarWithDefault && ! $isEnvVarWithPath) { try { validateShellSafePath($sourceValue, 'volume source'); } catch (\Exception $e) { diff --git a/tests/Unit/VolumeArrayFormatSecurityTest.php b/tests/Unit/VolumeArrayFormatSecurityTest.php index 97a6819b2..08174fff3 100644 --- a/tests/Unit/VolumeArrayFormatSecurityTest.php +++ b/tests/Unit/VolumeArrayFormatSecurityTest.php @@ -194,6 +194,36 @@ ->not->toThrow(Exception::class); }); +test('array-format with environment variable and path concatenation', function () { + // This is the reported issue #7127 - ${VAR}/path should be allowed + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - type: bind + source: '${VOLUMES_PATH}/mysql' + target: /var/lib/mysql + - type: bind + source: '${DATA_PATH}/config' + target: /etc/config + - type: bind + source: '${VOLUME_PATH}/app_data' + target: /app/data +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + + // Verify all three volumes have the correct source format + expect($parsed['services']['web']['volumes'][0]['source'])->toBe('${VOLUMES_PATH}/mysql'); + expect($parsed['services']['web']['volumes'][1]['source'])->toBe('${DATA_PATH}/config'); + expect($parsed['services']['web']['volumes'][2]['source'])->toBe('${VOLUME_PATH}/app_data'); + + // The validation should allow this - the reported bug was that it was blocked + expect(fn () => validateDockerComposeForInjection($dockerComposeYaml)) + ->not->toThrow(Exception::class); +}); + test('array-format with malicious environment variable default', function () { $dockerComposeYaml = <<<'YAML' services: diff --git a/tests/Unit/VolumeSecurityTest.php b/tests/Unit/VolumeSecurityTest.php index d7f20fc0e..f4cd6c268 100644 --- a/tests/Unit/VolumeSecurityTest.php +++ b/tests/Unit/VolumeSecurityTest.php @@ -94,6 +94,27 @@ } }); +test('parseDockerVolumeString accepts environment variables with path concatenation', function () { + $volumes = [ + '${VOLUMES_PATH}/mysql:/var/lib/mysql', + '${DATA_PATH}/config:/etc/config', + '${VOLUME_PATH}/app_data:/app', + '${MY_VAR_123}/deep/nested/path:/data', + '${VAR}/path:/app', + '${VAR}_suffix:/app', + '${VAR}-suffix:/app', + '${VAR}.ext:/app', + '${VOLUMES_PATH}/mysql:/var/lib/mysql:ro', + '${DATA_PATH}/config:/etc/config:rw', + ]; + + foreach ($volumes as $volume) { + $result = parseDockerVolumeString($volume); + expect($result)->toBeArray(); + expect($result['source'])->not->toBeNull(); + } +}); + test('parseDockerVolumeString rejects environment variables with command injection in default', function () { $maliciousVolumes = [ '${VAR:-`whoami`}:/app',