From 3e0d48faeaab950bfd063dfca908f1d140316ede Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 26 Mar 2026 13:26:16 +0100 Subject: [PATCH] refactor: simplify remote process chain and harden ActivityMonitor - Inline PrepareCoolifyTask and CoolifyTaskArgs into remote_process(), removing two single-consumer abstraction layers - Add #[Locked] attribute to ActivityMonitor $activityId property - Add team ownership verification in ActivityMonitor.hydrateActivity() with server_uuid fallback and fail-closed default - Store team_id in activity properties for proper scoping - Update CLAUDE.md to remove stale reference - Add comprehensive tests for activity monitor authorization Co-Authored-By: Claude Opus 4.6 --- CLAUDE.md | 2 +- .../CoolifyTask/PrepareCoolifyTask.php | 54 ------------- app/Data/CoolifyTaskArgs.php | 30 ------- app/Livewire/ActivityMonitor.php | 51 +++++++++--- bootstrap/helpers/remoteProcess.php | 64 +++++++++------ .../views/livewire/activity-monitor.blade.php | 4 +- .../Feature/ActivityMonitorCrossTeamTest.php | 81 +++++++++++++++++-- 7 files changed, 155 insertions(+), 131 deletions(-) delete mode 100644 app/Actions/CoolifyTask/PrepareCoolifyTask.php delete mode 100644 app/Data/CoolifyTaskArgs.php diff --git a/CLAUDE.md b/CLAUDE.md index 99e996756..bb65da405 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,7 +43,7 @@ ### Backend Structure (app/) - **Models/** — Eloquent models extending `BaseModel` which provides auto-CUID2 UUID generation. Key models: `Server`, `Application`, `Service`, `Project`, `Environment`, `Team`, plus standalone database models (`StandalonePostgresql`, `StandaloneMysql`, etc.). Common traits: `HasConfiguration`, `HasMetrics`, `HasSafeStringAttribute`, `ClearsGlobalSearchCache`. - **Services/** — Business logic services (ConfigurationGenerator, DockerImageParser, ContainerStatusAggregator, HetznerService, etc.). Use Services for complex orchestration; use Actions for single-purpose domain operations. - **Helpers/** — Global helpers loaded via `bootstrap/includeHelpers.php` from `bootstrap/helpers/` — organized into `shared.php`, `constants.php`, `versions.php`, `subscriptions.php`, `domains.php`, `docker.php`, `services.php`, `github.php`, `proxy.php`, `notifications.php`. -- **Data/** — Spatie Laravel Data DTOs (e.g., `CoolifyTaskArgs`, `ServerMetadata`). +- **Data/** — Spatie Laravel Data DTOs (e.g., `ServerMetadata`). - **Enums/** — PHP enums (TitleCase keys). Key enums: `ProcessStatus`, `Role` (MEMBER/ADMIN/OWNER with rank comparison), `BuildPackTypes`, `ProxyTypes`, `ContainerStatusTypes`. - **Rules/** — Custom validation rules (`ValidGitRepositoryUrl`, `ValidServerIp`, `ValidHostname`, `DockerImageFormat`, etc.). diff --git a/app/Actions/CoolifyTask/PrepareCoolifyTask.php b/app/Actions/CoolifyTask/PrepareCoolifyTask.php deleted file mode 100644 index 3f76a2e3c..000000000 --- a/app/Actions/CoolifyTask/PrepareCoolifyTask.php +++ /dev/null @@ -1,54 +0,0 @@ -remoteProcessArgs = $remoteProcessArgs; - - if ($remoteProcessArgs->model) { - $properties = $remoteProcessArgs->toArray(); - unset($properties['model']); - - $this->activity = activity() - ->withProperties($properties) - ->performedOn($remoteProcessArgs->model) - ->event($remoteProcessArgs->type) - ->log('[]'); - } else { - $this->activity = activity() - ->withProperties($remoteProcessArgs->toArray()) - ->event($remoteProcessArgs->type) - ->log('[]'); - } - } - - public function __invoke(): Activity - { - $job = new CoolifyTask( - activity: $this->activity, - ignore_errors: $this->remoteProcessArgs->ignore_errors, - call_event_on_finish: $this->remoteProcessArgs->call_event_on_finish, - call_event_data: $this->remoteProcessArgs->call_event_data, - ); - dispatch($job); - $this->activity->refresh(); - - return $this->activity; - } -} diff --git a/app/Data/CoolifyTaskArgs.php b/app/Data/CoolifyTaskArgs.php deleted file mode 100644 index 24132157a..000000000 --- a/app/Data/CoolifyTaskArgs.php +++ /dev/null @@ -1,30 +0,0 @@ -status = ProcessStatus::QUEUED->value; - } - } -} diff --git a/app/Livewire/ActivityMonitor.php b/app/Livewire/ActivityMonitor.php index 85ba60c33..665d14ba0 100644 --- a/app/Livewire/ActivityMonitor.php +++ b/app/Livewire/ActivityMonitor.php @@ -2,7 +2,9 @@ namespace App\Livewire; +use App\Models\Server; use App\Models\User; +use Livewire\Attributes\Locked; use Livewire\Component; use Spatie\Activitylog\Models\Activity; @@ -10,6 +12,7 @@ class ActivityMonitor extends Component { public ?string $header = null; + #[Locked] public $activityId = null; public $eventToDispatch = 'activityFinished'; @@ -57,25 +60,47 @@ public function hydrateActivity() $activity = Activity::find($this->activityId); - if ($activity) { - $teamId = data_get($activity, 'properties.team_id'); - if ($teamId && $teamId !== currentTeam()?->id) { + if (! $activity) { + $this->activity = null; + + return; + } + + $currentTeamId = currentTeam()?->id; + + // Check team_id stored directly in activity properties + $activityTeamId = data_get($activity, 'properties.team_id'); + if ($activityTeamId !== null) { + if ((int) $activityTeamId !== (int) $currentTeamId) { $this->activity = null; return; } + + $this->activity = $activity; + + return; + } + + // Fallback: verify ownership via the server that ran the command + $serverUuid = data_get($activity, 'properties.server_uuid'); + if ($serverUuid) { + $server = Server::where('uuid', $serverUuid)->first(); + if ($server && (int) $server->team_id !== (int) $currentTeamId) { + $this->activity = null; + + return; + } + + if ($server) { + $this->activity = $activity; + + return; + } } - $this->activity = $activity; - } - - public function updatedActivityId($value) - { - if ($value) { - $this->hydrateActivity(); - $this->isPollingActive = true; - self::$eventDispatched = false; - } + // Fail closed: no team_id and no server_uuid means we cannot verify ownership + $this->activity = null; } public function polling() diff --git a/bootstrap/helpers/remoteProcess.php b/bootstrap/helpers/remoteProcess.php index f819df380..2544719fc 100644 --- a/bootstrap/helpers/remoteProcess.php +++ b/bootstrap/helpers/remoteProcess.php @@ -1,9 +1,10 @@ teams->pluck('id'); if (! $teams->contains($server->team_id) && ! $teams->contains(0)) { - throw new \Exception('User is not part of the team that owns this server'); + throw new Exception('User is not part of the team that owns this server'); } } SshMultiplexingHelper::ensureMultiplexedConnection($server); - return resolve(PrepareCoolifyTask::class, [ - 'remoteProcessArgs' => new CoolifyTaskArgs( - server_uuid: $server->uuid, - command: $command_string, - type: $type, - type_uuid: $type_uuid, - model: $model, - ignore_errors: $ignore_errors, - call_event_on_finish: $callEventOnFinish, - call_event_data: $callEventData, - ), - ])(); + $properties = [ + 'server_uuid' => $server->uuid, + 'command' => $command_string, + 'type' => $type, + 'type_uuid' => $type_uuid, + 'status' => ProcessStatus::QUEUED->value, + 'team_id' => $server->team_id, + ]; + + $activityLog = activity() + ->withProperties($properties) + ->event($type); + + if ($model) { + $activityLog->performedOn($model); + } + + $activity = $activityLog->log('[]'); + + dispatch(new CoolifyTask( + activity: $activity, + ignore_errors: $ignore_errors, + call_event_on_finish: $callEventOnFinish, + call_event_data: $callEventData, + )); + + $activity->refresh(); + + return $activity; } function instant_scp(string $source, string $dest, Server $server, $throwError = true) { - return \App\Helpers\SshRetryHandler::retry( + return SshRetryHandler::retry( function () use ($source, $dest, $server) { $scp_command = SshMultiplexingHelper::generateScpCommand($server, $source, $dest); $process = Process::timeout(config('constants.ssh.command_timeout'))->run($scp_command); @@ -92,7 +110,7 @@ function instant_remote_process_with_timeout(Collection|array $command, Server $ } $command_string = implode("\n", $command); - return \App\Helpers\SshRetryHandler::retry( + return SshRetryHandler::retry( function () use ($server, $command_string) { $sshCommand = SshMultiplexingHelper::generateSshCommand($server, $command_string); $process = Process::timeout(30)->run($sshCommand); @@ -128,7 +146,7 @@ function instant_remote_process(Collection|array $command, Server $server, bool $command_string = implode("\n", $command); $effectiveTimeout = $timeout ?? config('constants.ssh.command_timeout'); - return \App\Helpers\SshRetryHandler::retry( + return SshRetryHandler::retry( function () use ($server, $command_string, $effectiveTimeout, $disableMultiplexing) { $sshCommand = SshMultiplexingHelper::generateSshCommand($server, $command_string, $disableMultiplexing); $process = Process::timeout($effectiveTimeout)->run($sshCommand); @@ -170,9 +188,9 @@ function excludeCertainErrors(string $errorOutput, ?int $exitCode = null) if ($ignored) { // TODO: Create new exception and disable in sentry - throw new \RuntimeException($errorMessage, $exitCode); + throw new RuntimeException($errorMessage, $exitCode); } - throw new \RuntimeException($errorMessage, $exitCode); + throw new RuntimeException($errorMessage, $exitCode); } function decode_remote_command_output(?ApplicationDeploymentQueue $application_deployment_queue = null, bool $includeAll = false): Collection @@ -194,7 +212,7 @@ function decode_remote_command_output(?ApplicationDeploymentQueue $application_d associative: true, flags: JSON_THROW_ON_ERROR ); - } catch (\JsonException $e) { + } catch (JsonException $e) { // If JSON decoding fails, try to clean up the logs and retry try { // Ensure valid UTF-8 encoding @@ -204,7 +222,7 @@ function decode_remote_command_output(?ApplicationDeploymentQueue $application_d associative: true, flags: JSON_THROW_ON_ERROR ); - } catch (\JsonException $e) { + } catch (JsonException $e) { // If it still fails, return empty collection to prevent crashes return collect([]); } @@ -353,7 +371,7 @@ function checkRequiredCommands(Server $server) } try { instant_remote_process(["docker run --rm --privileged --net=host --pid=host --ipc=host --volume /:/host busybox chroot /host bash -c 'apt update && apt install -y {$command}'"], $server); - } catch (\Throwable) { + } catch (Throwable) { break; } $commandFound = instant_remote_process(["docker run --rm --privileged --net=host --pid=host --ipc=host --volume /:/host busybox chroot /host bash -c 'command -v {$command}'"], $server, false); diff --git a/resources/views/livewire/activity-monitor.blade.php b/resources/views/livewire/activity-monitor.blade.php index 386d8622d..290a91857 100644 --- a/resources/views/livewire/activity-monitor.blade.php +++ b/resources/views/livewire/activity-monitor.blade.php @@ -34,10 +34,10 @@ } }" x-init="// Initial scroll $nextTick(() => scrollToBottom()); - + // Add scroll event listener $el.addEventListener('scroll', () => handleScroll()); - + // Set up mutation observer to watch for content changes observer = new MutationObserver(() => { $nextTick(() => scrollToBottom()); diff --git a/tests/Feature/ActivityMonitorCrossTeamTest.php b/tests/Feature/ActivityMonitorCrossTeamTest.php index 7e4aebc2f..9966ac2dd 100644 --- a/tests/Feature/ActivityMonitorCrossTeamTest.php +++ b/tests/Feature/ActivityMonitorCrossTeamTest.php @@ -1,9 +1,11 @@ otherTeam = Team::factory()->create(); }); -test('hydrateActivity blocks access to another teams activity', function () { +test('hydrateActivity blocks access to another teams activity via team_id', function () { $otherActivity = Activity::create([ 'log_name' => 'default', 'description' => 'test activity', @@ -27,12 +29,12 @@ $this->actingAs($this->user); session(['currentTeam' => ['id' => $this->team->id]]); - $component = Livewire::test(ActivityMonitor::class) - ->set('activityId', $otherActivity->id) + Livewire::test(ActivityMonitor::class) + ->call('newMonitorActivity', $otherActivity->id) ->assertSet('activity', null); }); -test('hydrateActivity allows access to own teams activity', function () { +test('hydrateActivity allows access to own teams activity via team_id', function () { $ownActivity = Activity::create([ 'log_name' => 'default', 'description' => 'test activity', @@ -43,13 +45,13 @@ session(['currentTeam' => ['id' => $this->team->id]]); $component = Livewire::test(ActivityMonitor::class) - ->set('activityId', $ownActivity->id); + ->call('newMonitorActivity', $ownActivity->id); expect($component->get('activity'))->not->toBeNull(); expect($component->get('activity')->id)->toBe($ownActivity->id); }); -test('hydrateActivity allows access to activity without team_id in properties', function () { +test('hydrateActivity blocks access to activity without team_id or server_uuid', function () { $legacyActivity = Activity::create([ 'log_name' => 'default', 'description' => 'legacy activity', @@ -59,9 +61,72 @@ $this->actingAs($this->user); session(['currentTeam' => ['id' => $this->team->id]]); + Livewire::test(ActivityMonitor::class) + ->call('newMonitorActivity', $legacyActivity->id) + ->assertSet('activity', null); +}); + +test('hydrateActivity blocks access to activity from another teams server via server_uuid', function () { + $otherServer = Server::factory()->create([ + 'team_id' => $this->otherTeam->id, + ]); + + $otherActivity = Activity::create([ + 'log_name' => 'default', + 'description' => 'test activity', + 'properties' => ['server_uuid' => $otherServer->uuid], + ]); + + $this->actingAs($this->user); + session(['currentTeam' => ['id' => $this->team->id]]); + + Livewire::test(ActivityMonitor::class) + ->call('newMonitorActivity', $otherActivity->id) + ->assertSet('activity', null); +}); + +test('hydrateActivity allows access to activity from own teams server via server_uuid', function () { + $ownServer = Server::factory()->create([ + 'team_id' => $this->team->id, + ]); + + $ownActivity = Activity::create([ + 'log_name' => 'default', + 'description' => 'test activity', + 'properties' => ['server_uuid' => $ownServer->uuid], + ]); + + $this->actingAs($this->user); + session(['currentTeam' => ['id' => $this->team->id]]); + $component = Livewire::test(ActivityMonitor::class) - ->set('activityId', $legacyActivity->id); + ->call('newMonitorActivity', $ownActivity->id); expect($component->get('activity'))->not->toBeNull(); - expect($component->get('activity')->id)->toBe($legacyActivity->id); + expect($component->get('activity')->id)->toBe($ownActivity->id); }); + +test('hydrateActivity returns null for non-existent activity id', function () { + $this->actingAs($this->user); + session(['currentTeam' => ['id' => $this->team->id]]); + + Livewire::test(ActivityMonitor::class) + ->call('newMonitorActivity', 99999) + ->assertSet('activity', null); +}); + +test('activityId property is locked and cannot be set from client', function () { + $otherActivity = Activity::create([ + 'log_name' => 'default', + 'description' => 'test activity', + 'properties' => ['team_id' => $this->otherTeam->id], + ]); + + $this->actingAs($this->user); + session(['currentTeam' => ['id' => $this->team->id]]); + + // Attempting to set a #[Locked] property from the client should throw + Livewire::test(ActivityMonitor::class) + ->set('activityId', $otherActivity->id) + ->assertStatus(500); +})->throws(CannotUpdateLockedPropertyException::class);