From 3379fa448ac5b81153d88ea056f2a42f9b60ece2 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 6 Nov 2026 09:31:44 +0100 Subject: [PATCH] fix(audit): keep personal data and credentials out of audit events Anonymize the audit events of a deleted user, store only the ID and name of other users, and drop the metadata copy of actor_email. Store URLs without query strings, registries without user info, and the exception class instead of raw SSH errors. Co-Authored-By: Claude Opus 5.5 --- .../UpdateIntegrationToken.php | 23 +------- .../Api/ServerTransferController.php | 2 +- app/Http/Controllers/Controller.php | 1 - app/Livewire/Admin/Index.php | 1 - app/Livewire/Server/Security/Patches.php | 4 +- app/Livewire/Server/Transfer.php | 4 +- app/Livewire/Team/AdminView.php | 2 - app/Livewire/Team/Member.php | 2 - app/Livewire/Team/Member/Access.php | 1 - app/Models/AuditEvent.php | 49 ++++++++++++++++- app/Models/User.php | 2 + app/Services/DockerRegistryLogins.php | 2 +- bootstrap/helpers/audit.php | 21 ++++++++ tests/Feature/AccountAuditTest.php | 53 +++++++++++++++++-- 14 files changed, 129 insertions(+), 38 deletions(-) diff --git a/app/Actions/IntegrationToken/UpdateIntegrationToken.php b/app/Actions/IntegrationToken/UpdateIntegrationToken.php index be7ebf7653..3aae44a73c 100644 --- a/app/Actions/IntegrationToken/UpdateIntegrationToken.php +++ b/app/Actions/IntegrationToken/UpdateIntegrationToken.php @@ -110,8 +110,8 @@ class UpdateIntegrationToken 'integration_token_uuid' => $integrationToken->uuid, 'integration_token_name' => $integrationToken->name, 'provider' => $provider, - 'previous_base_url' => $this->baseUrlForAudit(data_get($storedMetadata, 'base_url')), - 'base_url' => $this->baseUrlForAudit(data_get($metadata, 'base_url')), + 'previous_base_url' => auditUrl(data_get($storedMetadata, 'base_url')), + 'base_url' => auditUrl(data_get($metadata, 'base_url')), ], 'warning'); } @@ -146,25 +146,6 @@ class UpdateIntegrationToken )); } - /** - * Base URL without user info, query, or fragment, which can carry credentials. - */ - private function baseUrlForAudit(mixed $url): ?string - { - if (! is_string($url) || $url === '') { - return null; - } - - $parts = parse_url($url); - if (! is_array($parts) || ! isset($parts['scheme'], $parts['host'])) { - return '[invalid-url]'; - } - - return $parts['scheme'].'://'.$parts['host'] - .(isset($parts['port']) ? ':'.$parts['port'] : '') - .($parts['path'] ?? ''); - } - private function reenterSecretMessage(string $provider): string { return match ($provider) { diff --git a/app/Http/Controllers/Api/ServerTransferController.php b/app/Http/Controllers/Api/ServerTransferController.php index 6d7e366f03..a6b50f26f6 100644 --- a/app/Http/Controllers/Api/ServerTransferController.php +++ b/app/Http/Controllers/Api/ServerTransferController.php @@ -112,7 +112,7 @@ class ServerTransferController extends Controller auditLog('api.server.migrate', [ 'team_id' => $teamId, 'server_uuid' => $server->uuid, - 'target_url' => data_get($server->server_metadata, 'migration.target_url'), + 'target_url' => auditUrl(data_get($server->server_metadata, 'migration.target_url')), ]); return response()->json([ diff --git a/app/Http/Controllers/Controller.php b/app/Http/Controllers/Controller.php index 175eb30e91..4a5e4af141 100644 --- a/app/Http/Controllers/Controller.php +++ b/app/Http/Controllers/Controller.php @@ -279,7 +279,6 @@ class Controller extends BaseController 'already_member' => $alreadyMember, 'member_id' => $user->id, 'member_name' => $user->name, - 'member_email' => $user->email, 'actor_id' => $user->id, 'actor_name' => $user->name, 'actor_email' => $user->email, diff --git a/app/Livewire/Admin/Index.php b/app/Livewire/Admin/Index.php index 71e0f6eb71..b96793ed1c 100644 --- a/app/Livewire/Admin/Index.php +++ b/app/Livewire/Admin/Index.php @@ -75,7 +75,6 @@ class Index extends Component 'resource' => 'user', 'user_name' => $user->name, 'target_user_id' => $user->id, - 'target_user_email' => $user->email, 'target_team_id' => $team_to_switch_to?->id, ]); Auth::login($user); diff --git a/app/Livewire/Server/Security/Patches.php b/app/Livewire/Server/Security/Patches.php index bcb7cf882c..965eb59963 100644 --- a/app/Livewire/Server/Security/Patches.php +++ b/app/Livewire/Server/Security/Patches.php @@ -125,7 +125,7 @@ class Patches extends Component try { RebootServer::run($this->server); } catch (\Throwable $e) { - $this->auditPatchAction('reboot_requested', ['result' => 'failed', 'error' => $e->getMessage()], 'error'); + $this->auditPatchAction('reboot_requested', ['result' => 'failed', 'error' => class_basename($e)], 'error'); throw $e; } @@ -212,7 +212,7 @@ class Patches extends Component try { ConfigureAutomaticSecurityUpdates::run($this->server, $packageManager, $enabled, $time); } catch (\Throwable $e) { - $this->auditPatchAction('auto_security_updates_updated', [...$auditContext, 'result' => 'failed', 'error' => $e->getMessage()], 'error'); + $this->auditPatchAction('auto_security_updates_updated', [...$auditContext, 'result' => 'failed', 'error' => class_basename($e)], 'error'); throw $e; } diff --git a/app/Livewire/Server/Transfer.php b/app/Livewire/Server/Transfer.php index f6f01cc975..55df57901b 100644 --- a/app/Livewire/Server/Transfer.php +++ b/app/Livewire/Server/Transfer.php @@ -116,7 +116,7 @@ class Transfer extends Component auditLog('ui.server.migrate', [ 'team_id' => $this->server->team_id, 'server_uuid' => $this->server->uuid, - 'target_url' => $this->targetUrl, + 'target_url' => auditUrl($this->targetUrl), ]); $this->server->refresh(); @@ -141,7 +141,7 @@ class Transfer extends Component auditLog('ui.server.transfer_back', [ 'team_id' => $this->server->team_id, 'server_uuid' => $this->server->uuid, - 'from' => $this->managerUrl, + 'from' => auditUrl($this->managerUrl), ]); $this->server->refresh(); diff --git a/app/Livewire/Team/AdminView.php b/app/Livewire/Team/AdminView.php index 85f5c1c683..62aa5d6bbd 100644 --- a/app/Livewire/Team/AdminView.php +++ b/app/Livewire/Team/AdminView.php @@ -89,9 +89,7 @@ class AdminView extends Component auditLog('ui.user.deleted', [ 'team_id' => currentTeam()?->id, 'resource' => 'user', - 'user_name' => $user->name, 'deleted_user_id' => $user->id, - 'deleted_user_email' => $user->email, ]); $this->resetPage(); diff --git a/app/Livewire/Team/Member.php b/app/Livewire/Team/Member.php index eca84e2e1a..a5494e21f5 100644 --- a/app/Livewire/Team/Member.php +++ b/app/Livewire/Team/Member.php @@ -83,7 +83,6 @@ class Member extends Component 'team_id' => $teamId, 'member_id' => $this->member->id, 'member_name' => $this->member->name, - 'member_email' => $this->member->email, ]); // Clear cache for the removed user - both old and new key formats Cache::forget("team:{$this->member->id}"); @@ -117,7 +116,6 @@ class Member extends Component 'team_id' => $teamId, 'member_id' => $this->member->id, 'member_name' => $this->member->name, - 'member_email' => $this->member->email, 'role' => $role->value, ]); } diff --git a/app/Livewire/Team/Member/Access.php b/app/Livewire/Team/Member/Access.php index a52fac3910..148f4357d7 100644 --- a/app/Livewire/Team/Member/Access.php +++ b/app/Livewire/Team/Member/Access.php @@ -122,7 +122,6 @@ class Access extends Component 'team_id' => $this->teamId, 'member_id' => $member->id, 'member_name' => $member->name, - 'member_email' => $member->email, 'access' => Grant::listToArray($grants), ]); diff --git a/app/Models/AuditEvent.php b/app/Models/AuditEvent.php index f483ba8b19..f90e9f947b 100644 --- a/app/Models/AuditEvent.php +++ b/app/Models/AuditEvent.php @@ -203,7 +203,7 @@ class AuditEvent extends Model 'resource_name' => $resourceName, 'description' => data_get($context, 'audit_description') ?? trim(($resourceName ?? Str::headline((string) $resourceType)).' '.Str::headline($action)), - 'metadata' => self::redact(Arr::except($context, ['audit_changes'])), + 'metadata' => self::redact(Arr::except($context, ['audit_changes', 'actor_email'])), 'changes' => $changes, 'ip_address' => app()->bound('request') ? (client_ip(request()) ?: null) : null, 'user_agent' => app()->bound('request') ? Str::limit((string) request()->userAgent(), 200, '') : null, @@ -235,6 +235,53 @@ class AuditEvent extends Model ->first()?->team()?->id; } + /** + * Removes the personal data of a deleted user: from the events that the user did, + * and from the events about the user (team member, impersonation, deletion). + */ + public static function anonymizeUser(User $user): void + { + $replacement = '[deleted user]'; + $personalValues = array_filter([Str::lower((string) $user->email), Str::lower((string) $user->name)]); + + self::query() + ->where(fn (Builder $query) => $query + ->where(fn (Builder $query) => $query->where('actor_type', 'user')->where('actor_id', $user->id)) + ->orWhereJsonContains('metadata->member_id', $user->id) + ->orWhereJsonContains('metadata->target_user_id', $user->id) + ->orWhereJsonContains('metadata->deleted_user_id', $user->id)) + ->chunkById(self::PRUNE_BATCH_SIZE, function ($events) use ($personalValues, $replacement, $user): void { + foreach ($events as $event) { + if ($event->actor_type === 'user' && (int) $event->actor_id === (int) $user->id) { + $event->actor_name = $replacement; + $event->actor_email = null; + $event->ip_address = null; + $event->user_agent = null; + } + + if ($event->resource_name !== null && in_array(Str::lower($event->resource_name), $personalValues, true)) { + $event->description = $replacement.substr($event->description, strlen($event->resource_name)); + $event->resource_name = $replacement; + } + + $event->metadata = self::replacePersonalValues($event->metadata, $personalValues, $replacement); + $event->save(); + } + }); + } + + /** + * @param array $personalValues + */ + private static function replacePersonalValues(mixed $value, array $personalValues, string $replacement): mixed + { + if (is_array($value)) { + return array_map(fn (mixed $item): mixed => self::replacePersonalValues($item, $personalValues, $replacement), $value); + } + + return is_string($value) && in_array(Str::lower($value), $personalValues, true) ? $replacement : $value; + } + /** * Deletes in batches, so a large table is not locked by one long DELETE. */ diff --git a/app/Models/User.php b/app/Models/User.php index b8c82530b4..149216c82d 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -186,6 +186,8 @@ class User extends Authenticatable implements PasskeyUser, SendsEmail } }); }); + + static::deleted(fn (User $user) => AuditEvent::anonymizeUser($user)); } /** diff --git a/app/Services/DockerRegistryLogins.php b/app/Services/DockerRegistryLogins.php index 8b3d62b4fc..3fdc928aba 100644 --- a/app/Services/DockerRegistryLogins.php +++ b/app/Services/DockerRegistryLogins.php @@ -255,7 +255,7 @@ class DockerRegistryLogins 'server_uuid' => $server->uuid, 'server_name' => $server->name, 'team_id' => $server->team_id, - 'registry' => $registry, + 'registry' => preg_replace('#^[^/]*@#', '', $registry), 'outcome' => $succeeded ? 'success' : 'failed', ], $succeeded ? 'info' : 'warning'); } diff --git a/bootstrap/helpers/audit.php b/bootstrap/helpers/audit.php index 79c1c10cf1..51a18b6411 100644 --- a/bootstrap/helpers/audit.php +++ b/bootstrap/helpers/audit.php @@ -53,6 +53,27 @@ if (! function_exists('auditLog')) { } } +if (! function_exists('auditUrl')) { + /** + * Scheme, host, port and path of a URL, without user info, query string or fragment. + */ + function auditUrl(mixed $url): ?string + { + if (! is_string($url) || $url === '') { + return null; + } + + $parts = parse_url($url); + if (! is_array($parts) || ! isset($parts['scheme'], $parts['host'])) { + return '[invalid-url]'; + } + + return $parts['scheme'].'://'.$parts['host'] + .(isset($parts['port']) ? ':'.$parts['port'] : '') + .($parts['path'] ?? ''); + } +} + if (! function_exists('auditChangedFields')) { /** * Names of the attributes that the next save() will really change, without timestamps. diff --git a/tests/Feature/AccountAuditTest.php b/tests/Feature/AccountAuditTest.php index 0115a99656..8284f0be4d 100644 --- a/tests/Feature/AccountAuditTest.php +++ b/tests/Feature/AccountAuditTest.php @@ -140,7 +140,8 @@ test('records an accepted team invitation', function () { ->and($event->actor_id)->toBe($this->user->id) ->and($event->resource_uuid)->toBe('account-audit-invitation') ->and(data_get($event->metadata, 'role'))->toBe('admin') - ->and(data_get($event->metadata, 'member_email'))->toBe($this->user->email) + ->and(data_get($event->metadata, 'member_id'))->toBe($this->user->id) + ->and($event->metadata)->not->toHaveKeys(['member_email', 'actor_email']) ->and(data_get($event->metadata, 'already_member'))->toBeFalse(); }); @@ -195,7 +196,53 @@ test('records a user deleted by an instance admin', function () { ->and($event->team_id)->toBe($this->team->id) ->and($event->actor_id)->toBe($this->user->id) ->and(data_get($event->metadata, 'deleted_user_id'))->toBe($deletedUser->id) - ->and(data_get($event->metadata, 'deleted_user_email'))->toBe('deleted-user@example.com'); + ->and(accountAuditSerialized($event))->not->toContain('deleted-user@example.com'); +}); + +test('deleting a user removes their personal data from earlier audit events', function () { + $rootTeam = Team::factory()->create(['id' => 0]); + $rootTeam->members()->attach($this->user->id, ['role' => 'admin']); + $deletedUser = User::factory()->create(['name' => 'Jane Doe', 'email' => 'jane@example.com']); + $ownEvent = AuditEvent::factory()->create([ + 'event' => 'auth.user.login', + 'actor_type' => 'user', + 'actor_id' => $deletedUser->id, + 'actor_name' => 'Jane Doe', + 'actor_email' => 'jane@example.com', + 'resource_type' => 'user', + 'resource_name' => 'Jane Doe', + 'description' => 'Jane Doe Login', + 'metadata' => ['user_name' => 'Jane Doe'], + 'ip_address' => '203.0.113.7', + 'user_agent' => 'Jane Browser', + ]); + $aboutEvent = AuditEvent::factory()->create([ + 'event' => 'ui.team_member.removed', + 'actor_type' => 'user', + 'actor_id' => $this->user->id, + 'actor_email' => $this->user->email, + 'metadata' => ['member_id' => $deletedUser->id, 'member_name' => 'Jane Doe', 'member_email' => 'JANE@example.com'], + ]); + $unrelatedEvent = AuditEvent::factory()->create([ + 'event' => 'ui.team_member.removed', + 'metadata' => ['member_id' => $deletedUser->id + 1000, 'member_name' => 'Jane Doe'], + ]); + + Livewire::test(AdminView::class) + ->call('delete', $deletedUser->id, 'password') + ->assertReturned(true); + + $ownEvent->refresh(); + $aboutEvent->refresh(); + + expect(accountAuditSerialized($ownEvent))->not->toContain('Jane Doe') + ->not->toContain('jane@example.com') + ->not->toContain('203.0.113.7') + ->not->toContain('Jane Browser') + ->and($ownEvent->description)->toBe('[deleted user] Login') + ->and($aboutEvent->metadata)->toBe(['member_id' => $deletedUser->id, 'member_name' => '[deleted user]', 'member_email' => '[deleted user]']) + ->and($aboutEvent->actor_email)->toBe($this->user->email) + ->and($unrelatedEvent->refresh()->metadata['member_name'])->toBe('Jane Doe'); }); test('does not record a user deletion when the password is wrong', function () { @@ -229,7 +276,7 @@ test('records impersonation with the impersonating root user as actor', function expect($event->actor_id)->toBe(0) ->and($event->team_id)->toBe(0) ->and(data_get($event->metadata, 'target_user_id'))->toBe($targetUser->id) - ->and(data_get($event->metadata, 'target_user_email'))->toBe('impersonated@example.com'); + ->and(accountAuditSerialized($event))->not->toContain('impersonated@example.com'); }); test('records an oauth identity linked to an existing user', function () {