Files
ignacionelson 7da4635f13 Say which clients a scoped staff member may be told about
A staff member limited to their own assigned clients could read the names
and ids of clients on nobody's roster but their own, out of ordinary file
metadata.

The file boundary was never wrong. Sharing means a file can legitimately
reach a scoped viewer through client A while client B uploaded it, or
while B also receives it -- StaffLibraryScope::buildFiles is right to
permit that, and a B-only file is still a 403. What was wrong is that
every response then went on to name B. FileResource serialised the loaded
uploader and each assignment unfiltered; ShareTargets::assigned took no
viewer at all, so the details panel published the recipient list as it
stands and forSubject narrowed available_clients while handing
assigned_clients straight through. FoldersController::fileRow,
FilesController::edit, FileDetailsController and ClientFilesController
each named the uploader the same way. The API's uploaded_by filter asked
the question without any name attached: it answered "does this client of
yours put files in front of a client of mine" for any id a caller cared
to try.

12a8ebe3 said the rule out loud while fixing topClientsByStorage -- "the
file was theirs to read and the uploader's name was not theirs to see" --
and then the rule stayed in that widget. So it is a class now.
ClientIdentityScope is the one decision, asked by every surface that
names a client, and it deliberately answers about clients only: a
colleague's name is not a client identity, and hiding it would hide who
uploaded most of the library from the people who work in it. Groups go
through it too, on the same argument -- a group is a list of clients
wearing one name -- which the report did not cover but is the same leak.

Two judgement calls worth naming. assigned() keeps returning the whole
truth and gains a warning, because VisibleCommentScope resolves
notification recipients from it and a recipient filtered out of that list
is one who never hears about a message addressed to them; assignedFor()
is the display half. And FileResource asks at serialisation rather than
in its callers' eager loads, which is the opposite of how the version
counterparts next door are narrowed: that one is set-shaped and folds
into a query, this one is a per-row roster check across eight call sites
in four controllers, two of them re-loading assignments after a write.

The tests assert on whole response bodies rather than on named keys. The
leak was never in one field -- the same name arrived through the
uploader, through the recipient list and through four screens -- so a
body that does not contain the name anywhere is the only assertion that
would have caught all of it. Ten of the eighteen fail without this
change; the rest are the negative controls, including that an unscoped
administrator still sees every name and that the uploaded_by filter still
works for a client on the roster and for staff.

Reported by @Noorkhalel, GHSA-whmp-p9hv-r7j7. Their write-up named every
affected surface and the root cause in each, which is most of why this
took one pass.
2026-09-03 00:56:41 -03:00

228 lines
7.4 KiB
PHP

<?php
declare(strict_types=1);
namespace App\Modules\Files\Access;
use App\Models\User;
use App\Modules\Groups\Models\Group;
/**
* Whether a viewer may be told who a client is.
*
* A different question from whether they may read a file, and the gap
* between the two is the whole reason this exists. A stranger client's
* upload can sit legitimately inside a client-scoped staff member's
* library — shared with a group one of their own clients belongs to, or
* assigned to one of their clients alongside somebody else's. The file is
* theirs to read. The other client's name is not theirs to see.
*
* Commit 12a8ebe3 said exactly that while fixing one dashboard widget, and
* then the rule stayed in that widget. Every other place that serialises a
* file went on publishing the uploader and each recipient by name, so a
* manager assigned to one client could read the names and ids of clients
* on nobody's roster but their own out of ordinary file metadata. That is
* what this class ends: one statement of the rule, asked by every surface
* that names a client.
*
* Two things it deliberately is not:
*
* - It is not a download check. The file boundary is StaffLibraryScope's
* and FilePolicy's, and it is already correct — a file belonging only
* to a client off the roster is a 403 today. This narrows what a
* permitted response is allowed to say, nothing more.
* - It is not applied to staff. A colleague's name is not a client
* identity, and hiding it would hide who uploaded most of the library
* from the people who work in it.
*
* Unscoped staff are unaffected: they may identify everyone, which is what
* `null` means everywhere StaffLibraryScope answers this shape of question.
*/
class ClientIdentityScope
{
/**
* Memoised per viewer, since the listings ask once per row and each
* miss is a roster query. Registered as `scoped`, so this lasts a
* request and is dropped between queue jobs — the same lifetime, and
* for the same reason, as StaffLibraryScope's own memo.
*
* @var array<int, list<int>|null>
*/
private array $clientIds = [];
/** @var array<int, list<int>|null> */
private array $groupIds = [];
public function __construct(private readonly StaffLibraryScope $scope) {}
/**
* Whether $viewer may be told that $subject exists, and what they are
* called.
*
* A null subject is permitted: there is no identity to leak, and every
* caller here is reading an optional relation.
*/
public function permits(?User $viewer, ?User $subject): bool
{
if ($subject === null) {
return true;
}
if (! $subject->isClient()) {
return true;
}
if ($viewer === null) {
return false;
}
if ($viewer->is($subject)) {
return true;
}
$ids = $this->identifiableClientIds($viewer);
return $ids === null || in_array($subject->id, $ids, true);
}
/**
* The same question about a client known only by id — used where a
* caller has a foreign key rather than a loaded model.
*
* An id that belongs to nobody, or to a staff member, is permitted:
* there is no client identity behind it to protect.
*/
public function permitsClientId(?User $viewer, ?int $id): bool
{
if ($id === null) {
return true;
}
return $this->permits($viewer, User::query()->find($id));
}
/**
* Whether $viewer may be told a group exists.
*
* A group is a list of clients wearing one name, so naming one to
* somebody who may reach none of its members says the same thing
* naming a client would. The set is StaffLibraryScope's
* assignableGroupIds — every group holding at least one of the
* viewer's own clients.
*/
public function permitsGroupId(?User $viewer, ?int $id): bool
{
if ($id === null) {
return true;
}
if ($viewer === null) {
return false;
}
$ids = $this->identifiableGroupIds($viewer);
return $ids === null || in_array($id, $ids, true);
}
/**
* A client's name, or null when this viewer may not be told it.
*
* Null rather than a placeholder on purpose: every consumer of these
* fields already renders "no uploader recorded" for a null, because a
* deleted account leaves one behind. Inventing a "Hidden" string would
* be a new thing for sixteen locales to translate and would itself
* announce that there is somebody there to hide.
*/
public function nameOf(?User $viewer, ?User $subject): ?string
{
return $this->permits($viewer, $subject) ? $subject?->name : null;
}
/**
* Drop the entries this viewer may not be told about from a list of
* id/name pairs describing clients.
*
* @param list<array{id: int, name: string}> $pairs
* @return list<array{id: int, name: string}>
*/
public function filterClientPairs(?User $viewer, array $pairs): array
{
if ($this->identifiableClientIds($viewer) === null) {
return $pairs;
}
return array_values(array_filter(
$pairs,
fn (array $pair): bool => $this->permitsClientId($viewer, $pair['id']),
));
}
/**
* @param list<array{id: int, name: string}> $pairs
* @return list<array{id: int, name: string}>
*/
public function filterGroupPairs(?User $viewer, array $pairs): array
{
if ($this->identifiableGroupIds($viewer) === null) {
return $pairs;
}
return array_values(array_filter(
$pairs,
fn (array $pair): bool => $this->permitsGroupId($viewer, $pair['id']),
));
}
/**
* Both halves of a `shares` payload at once, since the two lists are
* always filtered together.
*
* @param array{clients: list<array{id: int, name: string}>, groups: list<array{id: int, name: string}>} $shares
* @return array{clients: list<array{id: int, name: string}>, groups: list<array{id: int, name: string}>}
*/
public function filterShares(?User $viewer, array $shares): array
{
return [
'clients' => $this->filterClientPairs($viewer, $shares['clients']),
'groups' => $this->filterGroupPairs($viewer, $shares['groups']),
];
}
/**
* Whether this viewer is narrowed at all. Callers use it to skip
* per-row work for the common unscoped case.
*/
public function isNarrowed(?User $viewer): bool
{
return $viewer === null || $this->identifiableClientIds($viewer) !== null;
}
/**
* @return list<int>|null
*/
private function identifiableClientIds(?User $viewer): ?array
{
if ($viewer === null) {
return [];
}
// Deliberately the same set as "who may I share with". A client on
// the roster is one this viewer already works with by name; a
// client off it is one they have no business knowing exists.
return $this->clientIds[$viewer->id] ??= $this->scope->assignableClientIds($viewer);
}
/**
* @return list<int>|null
*/
private function identifiableGroupIds(?User $viewer): ?array
{
if ($viewer === null) {
return [];
}
return $this->groupIds[$viewer->id] ??= $this->scope->assignableGroupIds($viewer);
}
}