From 4b8220a2508988bb5eccbf2601561482008a7375 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Wed, 26 Aug 2026 15:57:51 -0300 Subject: [PATCH] Narrow the dashboard's file widgets to the viewer's own library The sweep after #1685 turned up the same leak two widgets further down the same controller. largestFiles() and expiredFiles() already take the viewer -- to decide whether their rows get links -- but queried with a bare File::query(), so a client-scoped staff member's dashboard named files belonging to clients they hold nothing of. The note above largestFiles() says a link that 403s is accepted rather than adding per-row scope checks. That reasoning is about the link. A row that should not be there at all is a different problem, and the name is the part that leaks: "Q3 delinquent accounts" says plenty without ever being downloadable. Scoping the query is also cheaper than the per-row check that note declined -- StaffLibraryScope builds a scoped user's query once per request. Reachable in the default configuration, unlike the last few of these: the Client Manager role ships client-scoped and holds view_statistics. topClientsByStorage() goes with them; it names clients rather than files, which is the thing MembershipRequest::approvableBy and ActivityLogScope already exist to keep inside a roster. counters() and transferSeries() stay installation-wide, and now say so. A total carries no names -- "417 files" tells a scoped viewer nothing about whose they are -- and if that ever stops being the line, both move together. One consequence worth stating rather than discovering: scopeVisibleToClient ends in notExpired(), so a scoped viewer's expired-files widget now lists only their own expired uploads, not a client's. Safe, and under-inclusive -- telling them about a file auto-delete is about to take needs a library query that keeps expired rows, which is a boundary to decide rather than to invent inside a leak fix. --- .../Http/Controllers/DashboardController.php | 42 ++++++++++-- tests/Feature/Audit/DashboardTest.php | 68 +++++++++++++++++++ 2 files changed, 104 insertions(+), 6 deletions(-) diff --git a/app/Modules/Audit/Http/Controllers/DashboardController.php b/app/Modules/Audit/Http/Controllers/DashboardController.php index 5f9c69df..13040904 100644 --- a/app/Modules/Audit/Http/Controllers/DashboardController.php +++ b/app/Modules/Audit/Http/Controllers/DashboardController.php @@ -13,6 +13,7 @@ use App\Modules\Audit\ActivityLogScope; use App\Modules\Audit\ActivityPresenter; use App\Modules\Audit\DashboardWidgetPreferences; use App\Modules\Clients\ClientStorageUsage; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\Models\File; use App\Modules\Groups\Models\Group; use App\Modules\Identity\UserType; @@ -54,6 +55,7 @@ class DashboardController extends Controller private readonly SystemEnvironment $environment, private readonly ActivityPresenter $presenter, private readonly ActivityLogScope $scope, + private readonly StaffLibraryScope $library, ) {} public function __invoke(Request $request): Response @@ -85,7 +87,7 @@ class DashboardController extends Controller ? ['preset' => $preset, 'from' => $from->toDateString(), 'to' => $to->toDateString()] : null, 'top_clients_by_storage' => $canStatistics && $prefs->isEnabled($user, 'top_clients_by_storage') - ? $this->topClientsByStorage() + ? $this->topClientsByStorage($user) : null, 'largest_files' => $canStatistics && $prefs->isEnabled($user, 'largest_files') ? $this->largestFiles($user) : null, 'recent' => $canActionsLog && $prefs->isEnabled($user, 'recent') ? $this->recentActivity($user) : null, @@ -210,6 +212,12 @@ class DashboardController extends Controller */ private function counters(): array { + // Deliberately installation-wide, unlike the three widgets below. + // A total carries no names — "417 files" tells a scoped viewer + // nothing about whose they are — and the same reasoning leaves + // transferSeries() alone. If that ever stops being the line, both + // move together. + return [ 'files' => File::query()->count(), 'files_bytes' => (int) File::query()->sum('size'), @@ -280,9 +288,12 @@ class DashboardController extends Controller * * @return list */ - private function topClientsByStorage(): array + private function topClientsByStorage(User $viewer): array { - $rows = File::query() + // Scoped like the other two: this one names clients rather than + // files, which is the same thing MembershipRequest::approvableBy + // and ActivityLogScope exist to keep inside a viewer's roster. + $rows = $this->library->files($viewer) ->select('uploaded_by', DB::raw('SUM(size) as total_bytes')) ->whereHas('uploader', fn ($query) => $query->where('type', UserType::Client)) ->groupBy('uploaded_by') @@ -342,7 +353,14 @@ class DashboardController extends Controller $staffModule = $this->capabilities->has(Capability::UsersManage) && $viewer->can('manage_users'); $canStaffUsers = $staffModule && $viewer->can('edit_users'); - return array_values(File::query() + // Narrowed to the viewer's library, not just its links. The + // note above is about a link that 403s; a row that should not be + // here at all is a different problem, and the file's *name* is + // the part that leaks — "Q3 delinquent accounts" says plenty + // without being downloadable. Scoping the query costs one call: + // StaffLibraryScope builds a scoped user's query once per + // request, so this is not a per-row check. + return array_values($this->library->files($viewer) ->with('uploader:id,name,type') ->orderByDesc('size') ->limit(10) @@ -381,8 +399,20 @@ class DashboardController extends Controller $canFiles = $viewer->can('upload') || $viewer->can('edit_files') || $viewer->can('edit_others_files'); return [ - 'count' => File::query()->expired()->count(), - 'files' => array_values(File::query()->expired()->orderBy('expires_at')->limit(10) + // Both the count and the list read the viewer's library, so + // the number cannot describe files the list is not allowed to + // name. Same reason largestFiles() is scoped. + // + // Narrower than it looks for a client-scoped viewer: + // File::scopeVisibleToClient ends in notExpired(), so an + // expired file belonging to one of their clients is not in + // their library, and only their own expired uploads reach + // this list. Safe, and under-inclusive — telling them about + // a client's file that auto-delete is about to take would + // need a library query that keeps expired rows, which is a + // boundary to decide rather than to invent here. + 'count' => $this->library->files($viewer)->expired()->count(), + 'files' => array_values($this->library->files($viewer)->expired()->orderBy('expires_at')->limit(10) ->get(['id', 'name', 'expires_at']) ->map(fn (File $file): array => [ 'id' => $file->id, diff --git a/tests/Feature/Audit/DashboardTest.php b/tests/Feature/Audit/DashboardTest.php index 873967a3..60786986 100644 --- a/tests/Feature/Audit/DashboardTest.php +++ b/tests/Feature/Audit/DashboardTest.php @@ -396,3 +396,71 @@ test('an unscoped viewer still sees the whole installation in the widget', funct fn (AssertableInertia $page) => $page->has('recent', 1)->where('recent.0.replacements.subject', 'Anything'), ); }); + +// The shipped Client Manager role is client-scoped and holds +// view_statistics, so these three widgets are the default configuration, +// not a custom one. A file's name is the part that leaks: "Q3 delinquent +// accounts" says plenty without ever being downloadable. +test('the statistics widgets name only files inside the viewer scope', function () { + $role = Role::query()->create(['name' => 'Scoped stats', 'client_scoped' => true]); + RolePermission::query()->insert([ + ['role_id' => $role->id, 'permission' => 'view_statistics'], + ['role_id' => $role->id, 'permission' => 'upload'], + ]); + + $scoped = User::factory()->create(['role_id' => $role->id]); + $client = User::factory()->client()->create(); + $scoped->assignedClients()->attach($client->id); + + $mine = File::factory()->create([ + 'uploaded_by' => $this->admin->id, + 'name' => 'Statement', + 'size' => 10_000, + ]); + shareFileWith($mine, $client); + + // Expired files reach a scoped viewer only through their own uploads: + // File::scopeVisibleToClient ends in notExpired(), so an expired file + // belonging to one of their clients is not in their library at all. + $ownExpired = File::factory()->create([ + 'uploaded_by' => $scoped->id, + 'name' => 'My Own Expired', + 'size' => 1_000, + 'expires_at' => now()->subDay(), + ]); + + // Bigger, sooner-expired, and none of this viewer's business. + File::factory()->create([ + 'uploaded_by' => $this->admin->id, + 'name' => 'Q3 delinquent accounts', + 'size' => 99_000_000, + 'expires_at' => now()->subDays(5), + ]); + + $this->actingAs($scoped)->get('/dashboard')->assertInertia( + fn (AssertableInertia $page) => $page + ->has('largest_files', 2) + ->where('largest_files.0.name', 'Statement') + ->where('largest_files.1.name', 'My Own Expired') + ->has('expired_files.files', 1) + ->where('expired_files.files.0.name', 'My Own Expired') + // The count has to agree with the list, or the number + // describes files the list is not allowed to name. + ->where('expired_files.count', 1), + ); +}); + +test('an unscoped viewer still sees the whole installation in those widgets', function () { + File::factory()->create([ + 'uploaded_by' => $this->admin->id, + 'name' => 'Anything', + 'size' => 5_000, + 'expires_at' => now()->subDay(), + ]); + + $this->actingAs($this->admin)->get('/dashboard')->assertInertia( + fn (AssertableInertia $page) => $page + ->where('largest_files.0.name', 'Anything') + ->where('expired_files.count', 1), + ); +});