From c8de16101f604b4bd8f23b0d206dd4c64c412115 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Fri, 28 Aug 2026 02:12:44 +0200 Subject: [PATCH] Gate the comment moderation surfaces on reading, not just on the library FilePolicy::view() has two halves for staff: one of the three file keys (upload / edit_files / edit_others_files), AND StaffLibraryScope. Every comment surface that spans files narrowed by the library half alone. A role holding moderate_comments and no file key therefore got a 403 on every file in the installation while reading every comment written about them on /comments: the text, staff-only notes, the client name a Clients-visibility comment carries, and a visitor's IP address. The API queue answered the same way, and approving through it hands the body back in the response, so it was a reading door as well as a writing one. The class says this is not supposed to happen -- across()'s own docblock ("a moderation screen is not a way around the visibility model"), the route comment on /comments ("the list itself is still narrowed by VisibleCommentScope, so holding the permission does not widen what a viewer may read"), and routes/api.php ("reading and writing a comment is gated by 'may see this file', the same three keys the file endpoints use"). FileCommentPolicy::view() enforces it for a single comment, by running the file's own gate first. Only the cross-file queries did not. So they now take their files from ViewableFileScope, which is FilePolicy::view() expressed as a query, instead of from StaffLibraryScope, which is only its second half: across(), pendingTotal() and the API's pending list. The permission half moves into a named method on that class, since three modules now ask the same question. FileCommentPolicy::moderate() gets it too, in both forms. Its row form is otherwise unchanged -- the library check still runs by file id, so a comment on a soft-deleted file behaves exactly as before. No system role changes behaviour: Account Manager and System Administrator are the two that ship with moderate_comments, and both hold upload. What changes is a hand-built role that holds moderation and nothing else. Seven tests. Without the fix, five go red; the other two are the premise (that the viewer really is refused the file itself) and the guard that a moderator who may read files still moderates the whole installation. docs/api/openapi.json regenerated for the one changed description. --- .../Comments/Access/VisibleCommentScope.php | 19 ++- app/Modules/Comments/FileCommentPolicy.php | 11 ++ .../Api/CommentModerationController.php | 15 +- .../Files/Access/ViewableFileScope.php | 22 ++- docs/api/openapi.json | 2 +- .../Comments/ModerationReadPermissionTest.php | 146 ++++++++++++++++++ 6 files changed, 199 insertions(+), 16 deletions(-) create mode 100644 tests/Feature/Comments/ModerationReadPermissionTest.php diff --git a/app/Modules/Comments/Access/VisibleCommentScope.php b/app/Modules/Comments/Access/VisibleCommentScope.php index 8f8e923f..aef46f85 100644 --- a/app/Modules/Comments/Access/VisibleCommentScope.php +++ b/app/Modules/Comments/Access/VisibleCommentScope.php @@ -10,6 +10,7 @@ use App\Modules\Comments\GuestCommentIdentity; use App\Modules\Comments\Models\FileComment; use App\Modules\Files\Access\ShareTargets; use App\Modules\Files\Access\StaffLibraryScope; +use App\Modules\Files\Access\ViewableFileScope; use App\Modules\Files\Models\File; use App\Modules\Files\Models\Folder; use App\Modules\Identity\UserType; @@ -51,6 +52,7 @@ class VisibleCommentScope { public function __construct( private readonly StaffLibraryScope $scope, + private readonly ViewableFileScope $viewable, private readonly ShareTargets $shareTargets, private readonly GuestCommentIdentity $guests, ) {} @@ -123,6 +125,13 @@ class VisibleCommentScope * way around the visibility model** — moderating means deciding about * comments you can already see. * + * Which is why the files come from ViewableFileScope rather than from + * StaffLibraryScope: FilePolicy::view() is a permission half AND a + * library half, and narrowing by the library alone would hand every + * comment in the installation to a role holding moderate_comments and + * none of the three file keys — somebody who gets a 403 on every file + * these comments are about. + * * Staff only. A client has no cross-file view of comments and asking * for one is a mistake rather than an empty result, but returning * nothing is the safe way to be wrong. @@ -136,7 +145,7 @@ class VisibleCommentScope } return $this->applyVisibility( - FileComment::query()->whereIn('file_id', $this->scope->files($viewer)->select('files.id')), + FileComment::query()->whereIn('file_id', $this->viewable->for($viewer)->select('files.id')), $viewer, // Publicness is a property of each file, so it cannot be one // value for a query spanning many. It does not have to be: the @@ -156,6 +165,12 @@ class VisibleCommentScope * than about what this viewer may read, and a moderator who cannot see * a particular client's thread must still be told the file has * something waiting. + * + * The file boundary is still the same one, though. ViewableFileScope + * rather than StaffLibraryScope: which files is the part that varies + * per client, whether any is the part that does not, and a badge + * counting the whole installation for somebody who may open none of it + * is a number about other people's files. */ public function pendingTotal(User $viewer): int { @@ -165,7 +180,7 @@ class VisibleCommentScope return FileComment::query() ->whereNull('approved_at') - ->whereIn('file_id', $this->scope->files($viewer)->select('files.id')) + ->whereIn('file_id', $this->viewable->for($viewer)->select('files.id')) ->count(); } diff --git a/app/Modules/Comments/FileCommentPolicy.php b/app/Modules/Comments/FileCommentPolicy.php index 420ef2f1..dd179567 100644 --- a/app/Modules/Comments/FileCommentPolicy.php +++ b/app/Modules/Comments/FileCommentPolicy.php @@ -8,6 +8,7 @@ use App\Models\User; use App\Modules\Comments\Access\VisibleCommentScope; use App\Modules\Comments\Models\FileComment; use App\Modules\Files\Access\StaffLibraryScope; +use App\Modules\Files\Access\ViewableFileScope; use Illuminate\Support\Facades\Gate; /** @@ -22,6 +23,7 @@ class FileCommentPolicy private readonly VisibleCommentScope $scope, private readonly CommentingRules $rules, private readonly StaffLibraryScope $library, + private readonly ViewableFileScope $viewable, ) {} public function view(User $user, FileComment $comment): bool @@ -69,6 +71,15 @@ class FileCommentPolicy return false; } + // Moderating is deciding about comments you can already see, so the + // permission half of file reading is part of the answer in both + // forms. Without one of the three file keys this user gets a 403 on + // every file these comments are about, and approving one hands back + // its body — so this is a reading door, not only a writing one. + if (! $this->viewable->permitsAnyFile($user)) { + return false; + } + if ($comment === null || ! $user->isClientScoped()) { return true; } diff --git a/app/Modules/Comments/Http/Controllers/Api/CommentModerationController.php b/app/Modules/Comments/Http/Controllers/Api/CommentModerationController.php index fdaea2d1..14de669b 100644 --- a/app/Modules/Comments/Http/Controllers/Api/CommentModerationController.php +++ b/app/Modules/Comments/Http/Controllers/Api/CommentModerationController.php @@ -8,7 +8,7 @@ use App\Http\Controllers\Controller; use App\Modules\Comments\FileComments; use App\Modules\Comments\Http\Resources\Api\FileCommentResource; use App\Modules\Comments\Models\FileComment; -use App\Modules\Files\Access\StaffLibraryScope; +use App\Modules\Files\Access\ViewableFileScope; use Illuminate\Http\Request; use Illuminate\Http\Resources\Json\AnonymousResourceCollection; use Illuminate\Support\Facades\Gate; @@ -30,16 +30,17 @@ class CommentModerationController extends Controller { public function __construct( private readonly FileComments $comments, - private readonly StaffLibraryScope $library, + private readonly ViewableFileScope $viewable, ) {} /** * List comments awaiting approval. * - * Scoped by the same library boundary as everything else: a - * client-scoped token sees pending comments only on files its owner - * could already open. Oldest first, so working through the list means - * working through the backlog. + * Scoped by the same file boundary as everything else — the whole of + * it, not just its library half: a client-scoped token sees pending + * comments only on files its owner could already open, and a token + * whose owner holds no file key at all sees none. Oldest first, so + * working through the list means working through the backlog. */ public function index(Request $request): AnonymousResourceCollection { @@ -49,7 +50,7 @@ class CommentModerationController extends Controller $pending = FileComment::query() ->whereNull('approved_at') - ->whereIn('file_id', $this->library->files($viewer)->select('id')) + ->whereIn('file_id', $this->viewable->for($viewer)->select('id')) ->with(['author', 'clientContext']) ->orderBy('created_at') ->orderBy('id') diff --git a/app/Modules/Files/Access/ViewableFileScope.php b/app/Modules/Files/Access/ViewableFileScope.php index 2e25ceac..26d0ae55 100644 --- a/app/Modules/Files/Access/ViewableFileScope.php +++ b/app/Modules/Files/Access/ViewableFileScope.php @@ -40,15 +40,25 @@ class ViewableFileScope return File::query()->visibleToClient($user); } - // Mirrors FilePolicy::view()'s staff branch: the permission half is - // a property of the viewer, not the row, so it either opens the - // whole scope or closes it entirely. - $permitted = $user->can('upload') || $user->can('edit_files') || $user->can('edit_others_files'); - - if (! $permitted) { + if (! $this->permitsAnyFile($user)) { return File::query()->whereRaw('1 = 0'); } return $this->scope->files($user); } + + /** + * Whether a staff member holds any of the three keys that open file + * reading at all — the permission half of FilePolicy::view()'s staff + * branch, named once because more than one module has to ask it. + * + * It is a property of the viewer rather than of a row, so it either + * opens the whole scope or closes it entirely. That is also why a + * query narrowed by StaffLibraryScope alone is only half the check: + * the library says *which* files, this says *whether any*. + */ + public function permitsAnyFile(User $user): bool + { + return $user->can('upload') || $user->can('edit_files') || $user->can('edit_others_files'); + } } diff --git a/docs/api/openapi.json b/docs/api/openapi.json index aa87ab86..e364ca90 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -1068,7 +1068,7 @@ "/comments/pending": { "get": { "operationId": "comments.pending", - "description": "Scoped by the same library boundary as everything else: a\nclient-scoped token sees pending comments only on files its owner\ncould already open. Oldest first, so working through the list means\nworking through the backlog.\n\nRequires a token with the ability: `moderate_comments`.", + "description": "Scoped by the same file boundary as everything else \u2014 the whole of\nit, not just its library half: a client-scoped token sees pending\ncomments only on files its owner could already open, and a token\nwhose owner holds no file key at all sees none. Oldest first, so\nworking through the list means working through the backlog.\n\nRequires a token with the ability: `moderate_comments`.", "summary": "List comments awaiting approval", "tags": [ "CommentModeration" diff --git a/tests/Feature/Comments/ModerationReadPermissionTest.php b/tests/Feature/Comments/ModerationReadPermissionTest.php new file mode 100644 index 00000000..db0b05da --- /dev/null +++ b/tests/Feature/Comments/ModerationReadPermissionTest.php @@ -0,0 +1,146 @@ +admin = User::factory()->create(); + $this->settings = app(Settings::class); + + $this->settings->set(Setting::CommentsScope, 'all'); + $this->settings->set(Setting::CommentsAuthors, 'everyone'); + $this->settings->set(Setting::PublicCommentsEnabled, true); + $this->settings->set(Setting::CommentsGuestModeration, true); +}); + +/** A moderator holding moderate_comments and no file key whatsoever. */ +function moderatorWithoutFileKeys(): User +{ + $role = Role::query()->create(['name' => 'Comment moderator']); + RolePermission::query()->insert([ + ['role_id' => $role->id, 'permission' => 'moderate_comments'], + ]); + + return User::factory()->create(['role_id' => $role->id]); +} + +/** A moderator who may also read files — the shipped Account Manager shape. */ +function moderatorWithFileKeys(): User +{ + $role = Role::query()->create(['name' => 'Account manager shape']); + RolePermission::query()->insert([ + ['role_id' => $role->id, 'permission' => 'moderate_comments'], + ['role_id' => $role->id, 'permission' => 'upload'], + ]); + + return User::factory()->create(['role_id' => $role->id]); +} + +/** @return array{0: File, 1: FileComment} */ +function commentedFile(User $uploader): array +{ + $file = File::factory()->public()->create(['uploaded_by' => $uploader->id, 'name' => 'merger-terms']); + + return [$file, FileComment::factory()->for($file)->fromGuest()->pending()->create()]; +} + +test('a moderator who may read no file is refused the file itself', function () { + // The premise the rest of this file rests on, measured rather than + // assumed: the door next to the comment is shut for this viewer. + [$file] = commentedFile($this->admin); + + $this->actingAs(moderatorWithoutFileKeys()) + ->get("/files/{$file->id}/download") + ->assertForbidden(); +}); + +test('the moderation screen shows no comment about a file the viewer cannot open', function () { + [, $comment] = commentedFile($this->admin); + + $props = $this->actingAs(moderatorWithoutFileKeys()) + ->get('/comments')->assertOk()->viewData('page')['props']; + + expect(collect($props['entries'])->pluck('id')->all())->not->toContain($comment->id) + ->and($props['entries'])->toBe([]); +}); + +test('the pending badge counts nothing a viewer may not open', function () { + commentedFile($this->admin); + + $props = $this->actingAs(moderatorWithoutFileKeys()) + ->get('/comments')->assertOk()->viewData('page')['props']; + + expect($props['pending_total'])->toBe(0) + ->and($props['pending']['comments'] ?? 0)->toBe(0); +}); + +test('the API queue refuses a moderator without a file key', function () { + // The class form of the policy answers "does this user moderate at + // all", and without a file key the answer is no — so the queue is + // refused rather than returned empty, exactly as it is for somebody + // holding no moderation permission. + commentedFile($this->admin); + + Sanctum::actingAs(moderatorWithoutFileKeys(), ['moderate_comments']); + + $this->getJson('/api/v1/comments/pending')->assertForbidden(); +}); + +test('approving is refused, so the response cannot hand back the body', function () { + [, $comment] = commentedFile($this->admin); + + Sanctum::actingAs(moderatorWithoutFileKeys(), ['moderate_comments']); + + $this->postJson("/api/v1/comments/{$comment->id}/approve")->assertForbidden(); + + expect($comment->fresh()->isPending())->toBeTrue(); +}); + +test('deleting from the moderation screen is refused too', function () { + [, $comment] = commentedFile($this->admin); + + $this->actingAs(moderatorWithoutFileKeys()) + ->delete("/comments/{$comment->id}/moderate") + ->assertForbidden(); + + expect(FileComment::query()->whereKey($comment->id)->exists())->toBeTrue(); +}); + +test('a moderator who may read files still moderates the whole installation', function () { + // Guards against narrowing by more than the permission half: this role + // is unscoped, so every comment stays in reach. + [, $comment] = commentedFile($this->admin); + + $moderator = moderatorWithFileKeys(); + + $props = $this->actingAs($moderator)->get('/comments')->assertOk()->viewData('page')['props']; + + expect(collect($props['entries'])->pluck('id')->all())->toContain($comment->id) + ->and($props['pending_total'])->toBe(1); + + $this->actingAs($moderator)->delete("/comments/{$comment->id}/moderate")->assertRedirect(); + + expect(FileComment::query()->whereKey($comment->id)->exists())->toBeFalse(); +});