From 410a9a6195a2b939d4a429f6c464ff56e61177f8 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 20 Apr 2026 11:27:10 +0200 Subject: [PATCH] refactor(volumes): validate input and escape shell args Tighten validation on volume name and host path inputs across Livewire + API storage endpoints and escape shell arguments in volume clone and compose preview cleanup paths. --- .../Api/ApplicationsController.php | 4 +- .../Controllers/Api/DatabasesController.php | 4 +- .../Controllers/Api/ServicesController.php | 4 +- app/Jobs/VolumeCloneJob.php | 29 ++++--- app/Livewire/Project/Service/Storage.php | 8 +- app/Livewire/Project/Shared/Storages/Show.php | 29 +++++-- app/Models/ApplicationPreview.php | 14 ++- tests/Unit/PersistentVolumeSecurityTest.php | 85 +++++++++++++++++++ 8 files changed, 148 insertions(+), 29 deletions(-) diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index cc7c0dbbe..eb2e7fc53 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -4121,7 +4121,7 @@ public function update_storage(Request $request): JsonResponse 'is_preview_suffix_enabled' => 'boolean', 'name' => ['string', 'regex:'.ValidationPatterns::VOLUME_NAME_PATTERN], 'mount_path' => 'string', - 'host_path' => 'string|nullable', + 'host_path' => ['string', 'nullable', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN], 'content' => 'string|nullable', ]); @@ -4299,7 +4299,7 @@ public function create_storage(Request $request): JsonResponse 'type' => 'required|string|in:persistent,file', 'name' => ['string', 'regex:'.ValidationPatterns::VOLUME_NAME_PATTERN], 'mount_path' => 'required|string', - 'host_path' => 'string|nullable', + 'host_path' => ['string', 'nullable', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN], 'content' => 'string|nullable', 'is_directory' => 'boolean', 'fs_path' => 'string', diff --git a/app/Http/Controllers/Api/DatabasesController.php b/app/Http/Controllers/Api/DatabasesController.php index 8241e5fba..3067d98e7 100644 --- a/app/Http/Controllers/Api/DatabasesController.php +++ b/app/Http/Controllers/Api/DatabasesController.php @@ -3496,7 +3496,7 @@ public function create_storage(Request $request): JsonResponse 'type' => 'required|string|in:persistent,file', 'name' => ['string', 'regex:'.ValidationPatterns::VOLUME_NAME_PATTERN], 'mount_path' => 'required|string', - 'host_path' => 'string|nullable', + 'host_path' => ['string', 'nullable', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN], 'content' => 'string|nullable', 'is_directory' => 'boolean', 'fs_path' => 'string', @@ -3694,7 +3694,7 @@ public function update_storage(Request $request): JsonResponse 'is_preview_suffix_enabled' => 'boolean', 'name' => ['string', 'regex:'.ValidationPatterns::VOLUME_NAME_PATTERN], 'mount_path' => 'string', - 'host_path' => 'string|nullable', + 'host_path' => ['string', 'nullable', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN], 'content' => 'string|nullable', ]); diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index 23ba30998..20560635e 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -2018,7 +2018,7 @@ public function create_storage(Request $request): JsonResponse 'resource_uuid' => 'required|string', 'name' => ['string', 'regex:'.ValidationPatterns::VOLUME_NAME_PATTERN], 'mount_path' => 'required|string', - 'host_path' => 'string|nullable', + 'host_path' => ['string', 'nullable', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN], 'content' => 'string|nullable', 'is_directory' => 'boolean', 'fs_path' => 'string', @@ -2227,7 +2227,7 @@ public function update_storage(Request $request): JsonResponse 'is_preview_suffix_enabled' => 'boolean', 'name' => ['string', 'regex:'.ValidationPatterns::VOLUME_NAME_PATTERN], 'mount_path' => 'string', - 'host_path' => 'string|nullable', + 'host_path' => ['string', 'nullable', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN], 'content' => 'string|nullable', ]); diff --git a/app/Jobs/VolumeCloneJob.php b/app/Jobs/VolumeCloneJob.php index f37a9704e..060ec3ac6 100644 --- a/app/Jobs/VolumeCloneJob.php +++ b/app/Jobs/VolumeCloneJob.php @@ -43,27 +43,34 @@ public function handle() protected function cloneLocalVolume() { + $srcVol = escapeshellarg($this->sourceVolume); + $tgtVol = escapeshellarg($this->targetVolume); + instant_remote_process([ - "docker volume create $this->targetVolume", - "docker run --rm -v $this->sourceVolume:/source -v $this->targetVolume:/target alpine sh -c 'cp -a /source/. /target/ && chown -R 1000:1000 /target'", + "docker volume create {$tgtVol}", + "docker run --rm -v {$srcVol}:/source -v {$tgtVol}:/target alpine sh -c 'cp -a /source/. /target/ && chown -R 1000:1000 /target'", ], $this->sourceServer); } protected function cloneRemoteVolume() { + $srcVol = escapeshellarg($this->sourceVolume); + $tgtVol = escapeshellarg($this->targetVolume); $sourceCloneDir = "{$this->cloneDir}/{$this->sourceVolume}"; $targetCloneDir = "{$this->cloneDir}/{$this->targetVolume}"; + $srcDir = escapeshellarg($sourceCloneDir); + $tgtDir = escapeshellarg($targetCloneDir); try { instant_remote_process([ - "mkdir -p $sourceCloneDir", - "chmod 777 $sourceCloneDir", - "docker run --rm -v $this->sourceVolume:/source -v $sourceCloneDir:/clone alpine sh -c 'cd /source && tar czf /clone/volume-data.tar.gz .'", + "mkdir -p {$srcDir}", + "chmod 777 {$srcDir}", + "docker run --rm -v {$srcVol}:/source -v {$srcDir}:/clone alpine sh -c 'cd /source && tar czf /clone/volume-data.tar.gz .'", ], $this->sourceServer); instant_remote_process([ - "mkdir -p $targetCloneDir", - "chmod 777 $targetCloneDir", + "mkdir -p {$tgtDir}", + "chmod 777 {$tgtDir}", ], $this->targetServer); instant_scp( @@ -74,8 +81,8 @@ protected function cloneRemoteVolume() ); instant_remote_process([ - "docker volume create $this->targetVolume", - "docker run --rm -v $this->targetVolume:/target -v $targetCloneDir:/clone alpine sh -c 'cd /target && tar xzf /clone/volume-data.tar.gz && chown -R 1000:1000 /target'", + "docker volume create {$tgtVol}", + "docker run --rm -v {$tgtVol}:/target -v {$tgtDir}:/clone alpine sh -c 'cd /target && tar xzf /clone/volume-data.tar.gz && chown -R 1000:1000 /target'", ], $this->targetServer); } catch (\Exception $e) { @@ -84,7 +91,7 @@ protected function cloneRemoteVolume() } finally { try { instant_remote_process([ - "rm -rf $sourceCloneDir", + "rm -rf {$srcDir}", ], $this->sourceServer, false); } catch (\Exception $e) { \Log::warning('Failed to clean up source server clone directory: '.$e->getMessage()); @@ -93,7 +100,7 @@ protected function cloneRemoteVolume() try { if ($this->targetServer) { instant_remote_process([ - "rm -rf $targetCloneDir", + "rm -rf {$tgtDir}", ], $this->targetServer, false); } } catch (\Exception $e) { diff --git a/app/Livewire/Project/Service/Storage.php b/app/Livewire/Project/Service/Storage.php index 433c2b13c..6f43662d5 100644 --- a/app/Livewire/Project/Service/Storage.php +++ b/app/Livewire/Project/Service/Storage.php @@ -106,8 +106,12 @@ public function submitPersistentVolume() $this->validate([ 'name' => ValidationPatterns::volumeNameRules(), 'mount_path' => 'required|string', - 'host_path' => $this->isSwarm ? 'required|string' : 'string|nullable', - ], ValidationPatterns::volumeNameMessages()); + 'host_path' => $this->isSwarm + ? ['required', 'string', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN] + : ['nullable', 'string', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN], + ], array_merge(ValidationPatterns::volumeNameMessages(), [ + 'host_path.regex' => 'Host path must start with / and only contain safe path characters.', + ])); $name = $this->resource->uuid.'-'.$this->name; diff --git a/app/Livewire/Project/Shared/Storages/Show.php b/app/Livewire/Project/Shared/Storages/Show.php index eee5a0776..2aaca5e6f 100644 --- a/app/Livewire/Project/Shared/Storages/Show.php +++ b/app/Livewire/Project/Shared/Storages/Show.php @@ -3,6 +3,7 @@ namespace App\Livewire\Project\Shared\Storages; use App\Models\LocalPersistentVolume; +use App\Support\ValidationPatterns; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Livewire\Component; @@ -31,19 +32,33 @@ class Show extends Component public bool $isPreviewSuffixEnabled = true; - protected $rules = [ - 'name' => 'required|string', - 'mountPath' => 'required|string', - 'hostPath' => 'string|nullable', - 'isPreviewSuffixEnabled' => 'required|boolean', - ]; - protected $validationAttributes = [ 'name' => 'name', 'mountPath' => 'mount', 'hostPath' => 'host', ]; + protected function rules(): array + { + return [ + 'name' => ValidationPatterns::volumeNameRules(), + 'mountPath' => ['required', 'string', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN], + 'hostPath' => ['nullable', 'string', 'regex:'.ValidationPatterns::DIRECTORY_PATH_PATTERN], + 'isPreviewSuffixEnabled' => 'required|boolean', + ]; + } + + protected function messages(): array + { + return array_merge( + ValidationPatterns::volumeNameMessages(), + [ + 'mountPath.regex' => 'Mount path must start with / and only contain safe path characters.', + 'hostPath.regex' => 'Host path must start with / and only contain safe path characters.', + ] + ); + } + /** * Sync data between component properties and model * diff --git a/app/Models/ApplicationPreview.php b/app/Models/ApplicationPreview.php index f08a48cea..9159fd0d8 100644 --- a/app/Models/ApplicationPreview.php +++ b/app/Models/ApplicationPreview.php @@ -2,6 +2,7 @@ namespace App\Models; +use App\Support\ValidationPatterns; use Illuminate\Database\Eloquent\SoftDeletes; use Spatie\Url\Url; use Visus\Cuid2\Cuid2; @@ -42,11 +43,18 @@ protected static function booted() $networkKeys = collect($networks)->keys(); $volumeKeys = collect($volumes)->keys(); $volumeKeys->each(function ($key) use ($server) { - instant_remote_process(["docker volume rm -f $key"], $server, false); + if (! preg_match(ValidationPatterns::VOLUME_NAME_PATTERN, $key)) { + return; + } + instant_remote_process(['docker volume rm -f '.escapeshellarg($key)], $server, false); }); $networkKeys->each(function ($key) use ($server) { - instant_remote_process(["docker network disconnect $key coolify-proxy"], $server, false); - instant_remote_process(["docker network rm $key"], $server, false); + if (! preg_match(ValidationPatterns::DOCKER_NETWORK_PATTERN, $key)) { + return; + } + $k = escapeshellarg($key); + instant_remote_process(["docker network disconnect {$k} coolify-proxy"], $server, false); + instant_remote_process(["docker network rm {$k}"], $server, false); }); } else { // Regular application volume cleanup diff --git a/tests/Unit/PersistentVolumeSecurityTest.php b/tests/Unit/PersistentVolumeSecurityTest.php index fdce223d3..287045534 100644 --- a/tests/Unit/PersistentVolumeSecurityTest.php +++ b/tests/Unit/PersistentVolumeSecurityTest.php @@ -96,3 +96,88 @@ expect($messages)->toHaveKey('volume_name.regex'); }); + +// --- escapeshellarg Defense Tests for docker volume create --- + +it('escapeshellarg neutralizes injection in docker volume create command', function (string $maliciousName) { + $escaped = escapeshellarg($maliciousName); + $command = "docker volume create {$escaped}"; + + expect($command)->toStartWith('docker volume create ') + ->and($escaped)->toStartWith("'") + ->and($escaped)->toEndWith("'"); +})->with([ + 'semicolon' => 'vol; rm -rf /', + 'pipe' => 'vol | cat /etc/passwd', + 'ampersand' => 'vol && whoami', + 'backtick' => 'vol`id`', + 'command substitution' => 'vol$(whoami)', +]); + +// --- escapeshellarg Defense Tests for docker run -v --- + +it('escapeshellarg neutralizes injection in docker run -v command', function (string $maliciousName) { + $escaped = escapeshellarg($maliciousName); + $command = "docker run --rm -v {$escaped}:/source -v {$escaped}:/target alpine sh -c 'cp -a /source/. /target/'"; + + expect($command)->toContain('docker run --rm -v ') + ->and($escaped)->toStartWith("'") + ->and($escaped)->toEndWith("'"); +})->with([ + 'semicolon' => 'vol; rm -rf /', + 'pipe' => 'vol | cat /etc/passwd', + 'command substitution' => 'vol$(whoami)', +]); + +// --- escapeshellarg Defense Tests for docker network commands --- + +it('escapeshellarg neutralizes injection in docker network disconnect command', function (string $maliciousName) { + $escaped = escapeshellarg($maliciousName); + $command = "docker network disconnect {$escaped} coolify-proxy"; + + expect($command)->toStartWith('docker network disconnect ') + ->and($escaped)->toStartWith("'") + ->and($escaped)->toEndWith("'"); +})->with([ + 'semicolon' => 'net; rm -rf /', + 'pipe' => 'net | cat /etc/passwd', + 'command substitution' => 'net$(whoami)', +]); + +it('escapeshellarg neutralizes injection in docker network rm command', function (string $maliciousName) { + $escaped = escapeshellarg($maliciousName); + $command = "docker network rm {$escaped}"; + + expect($command)->toStartWith('docker network rm ') + ->and($escaped)->toStartWith("'") + ->and($escaped)->toEndWith("'"); +})->with([ + 'semicolon' => 'net; rm -rf /', + 'pipe' => 'net | cat /etc/passwd', + 'command substitution' => 'net$(whoami)', +]); + +// --- DIRECTORY_PATH_PATTERN Tests --- + +it('accepts valid directory paths', function (string $path) { + expect(preg_match(ValidationPatterns::DIRECTORY_PATH_PATTERN, $path))->toBe(1); +})->with([ + 'root' => '/', + 'simple path' => '/data', + 'nested path' => '/data/coolify/volumes', + 'with dots' => '/data/my.app/storage', + 'with hyphens' => '/data/my-app/storage', + 'with underscores' => '/data/my_app/storage', +]); + +it('rejects directory paths with shell metacharacters', function (string $path) { + expect(preg_match(ValidationPatterns::DIRECTORY_PATH_PATTERN, $path))->toBe(0); +})->with([ + 'semicolon injection' => '/etc; rm -rf /', + 'pipe injection' => '/etc | cat /etc/passwd', + 'command substitution' => '/etc$(whoami)', + 'backtick injection' => '/etc`id`', + 'space injection' => '/etc /tmp', + 'relative traversal' => '../../../etc/passwd', + 'no leading slash' => 'etc/passwd', +]);