Search and the category dropdown were the whole filter bar, which is thin
for a library of any size. It now also narrows by:
- who uploaded the file, and separately by the role they hold
- public or private
- never downloaded, or downloaded at least once
- current version, or outdated
Each one forces the same flat, whole-library view search already used, and
they combine.
Two decisions worth naming.
"Public" means what the badge on the row means -- File::isEffectivelyPublic(),
the file's own flag or a public folder anywhere above it. Filtering on the
`public` column alone would have hidden files this very screen labels
Public, which is a filter arguing with the list it filters. The private half
needs its own null branch, because `folder_id NOT IN (...)` is never true for
a NULL folder_id: without it a file at the library root belonged to neither
half and vanished from both. Removing that branch turns the private filter
from one row to zero, which is the test.
The uploader filter carries the same guard /api/v1/files puts on
`uploaded_by`. fileRow() already withholds an uploader's name from a viewer
who may not identify them, so answering this filter plainly would have handed
the same identity straight back as a row count. An id the caller may not
identify now matches nothing, which is indistinguishable from someone who
uploaded nothing, and the dropdown is built through filterClientPairs so it
never offers the name either. Without the guard the scoped-staff test gets
its stranger's file back.
"Outdated" rather than "superseded" throughout, because that is the word the
version badge already uses and the two should not disagree. A file nothing
has replaced counts as current, including one never versioned at all.
The ids are cast out of the validated input: `integer` validates "5" without
converting it, and permitsClientId() takes a strict ?int.
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.
ignacionelson