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.
Branding moved into the application on 2026-08-28 and its two read-only
endpoints came with it unchanged -- same paths under
/api/v1/modules/branding, same capability, same ability. Their
documentation did not: the guide still sent readers to
packages/cloud-modules/docs/api.md, so endpoints that every installation
now carries were described in a private repository almost none of their
callers can open.
The OpenAPI document is still not the place for them. OpenApiContractTest
skips api/v1/modules/* on purpose: that document is served
unauthenticated and has to be identical on every installation, while a
module's paths exist only where the module does. So this is a plain
markdown file beside the guide, and published like it -- ignored docs are
maintainer notes, and this one is for integrators.
It is the cloud-modules file moved across, minus the attribution switch:
that half stayed Cloud-only and has no API surface at all. The gate is
described as every edition holding branding.customize, with a hosted plan
able to subtract it, because that is what the enum now says.
FilePolicy::view() has two halves for a staff member: 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 -- VisibleCommentScope::across(), pendingTotal(), and the API's GET /comments/pending. A role holding moderate_comments and no file key at all therefore read, on /comments, every comment in the installation: the text, staff-only notes, the client's name in conversation, and a visitor's IP, while getting a 403 on every file those comments were about. POST /api/v1/comments/{id}/approve was the same door on the write side, and its response carries the comment body, so an id was enough to read one.
The project already states the rule this breaks in four places, including across()'s own docblock -- "a moderation screen is not a way around the visibility model: moderating means deciding about comments you can already see" -- and only the cross-file queries did not ask it.
The cross-file queries now take their files from ViewableFileScope, which is FilePolicy::view() expressed as a query and already in the codebase for exactly this, instead of from StaffLibraryScope, which is only its second half. The permission half becomes a named method there, permitsAnyFile(), because three modules now ask it, and FileCommentPolicy::moderate() asks it in both of its forms. This is the other half of #1698, which library-scoped the same screen: library is not readability.
Verified before merging: tests/Feature/Comments at 180 passed on the trial-merge; with app/ reset and the new test file kept, 5 failed / 2 passed. The two green either way are the right two -- the premise, that the file itself 403s for this viewer, and the guard that a moderator who does hold a file key still moderates the whole installation.
Compatibility was the question worth asking, and it is clean: the only shipped roles holding moderate_comments are Account Manager, which also holds Upload, EditFiles and EditOthersFiles, and System Administrator, which holds everything. No shipped role loses moderation. The only configuration whose behaviour changes is a custom role granting moderate_comments with no file key, which is precisely the leaking one.
This PR also edits docs/api/openapi.json, which #1727 edited too, so the merged result was checked rather than trusted: scramble:export on the merged tree reproduces the committed file byte for byte, with both endpoints' descriptions present.
Reported and fixed by @denkfabrik-li.
FilesController::expiryInstant exists because a calendar day ends where
the person naming it lives: the web form posts a bare YYYY-MM-DD, and
storing that as it arrives would cut a file off at midnight UTC -- "expires
on the 12th" ending partway through the 11th for anyone in the Americas.
The API takes the same field, validates it as a date, and stores it raw:
web → 2026-09-12T23:59:59+00:00 (end of the day, as the docblock means)
API → 2026-09-12T00:00:00+00:00 (raw)
Same value, same field, same file, two meanings -- and the earlier of the
two is a file that dies at the start of the day it was promised.
A bare date now means the end of that day in the caller's timezone, as it
does on the web. A value carrying a time is unchanged: it is an instant
the caller named on purpose, the API can express one and a date input
cannot. The endpoint's docblock says both, so the OpenAPI document does
too.
Three tests: the day, the timestamp, and clearing. Without the fix the
first goes red.
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.
A managed installation's staff accounts were expected to arrive from
outside it, so users.manage was Community-only and /users, /roles and
their API twins answered 404 there. The platform side spent a long
document designing its way around that gate; opening it is cheaper than
routing around it, and more honest about where the knowledge sits.
The division that settles it is the one managed storage already uses. We
do not manage a tenant's files from outside — a bucket is provisioned, a
scoped credential handed over, and what goes in it is the tenant's
business. Seats are the same kind of thing. A platform knows how many
staff accounts it sold; it does not know whether Alice should be an
Account Manager, and it certainly does not know where her files go when
she leaves. Capacity is the platform's, occupancy is the tenant's, and
the cap belongs in an environment variable rather than in a closed
screen.
The capability stays in front of the routes rather than being deleted.
It is currently true in both editions, but it is the seam an edition
difference has to travel through, and removing it would mean inventing
one again later.
Seven test files asserted the old rule, which is the tests doing their
job. Most flip. Two needed a different example instead: EnsureCapability
and AbilityCapability were both using users.manage to stand for
"Community-only", so they now use storage.configure and manage_updates —
keys that still are.
Two rationales half-expired and say so rather than being quietly
rewritten. CommentAuthors gave two reasons for being a setting rather
than a permission; the first was that roles are uneditable on cloud,
which stopped being true here, and the second — that `Everyone` includes
anonymous visitors, who have no role to hold a key — was always the
stronger and is now the whole of it.
The seat cap this makes necessary is the next commit, not this one. On
its own this change lets a managed tenant create staff accounts without
limit, which is why the two belong in the same release.
Nobody hands out reach they do not hold either
Two resolutions against branches that landed first. #1678 and this one
each add a constructor property and an import to StaffAccounts, so both
are kept. And #1702's merge note called this one exactly: its
"converting an account to staff cannot hand out clients either" case
promoted a stranger client, which #1702 now refuses at 404 before
validation runs. Pointed at a client the actor holds, as that note
proposed, so the request reaches the assigned_clients rule the case is
actually about.
Two things found by checking #1696 and #1699 -- open branches carrying
the same fixes I wrote this morning -- against what I actually shipped.
**topClientsByStorage was scoped with the wrong question.** 4b8220a
narrowed it with StaffLibraryScope::files(), which is right for the two
widgets that name files and wrong for the one that names clients: a
stranger client's upload can sit legitimately inside a scoped viewer's
library, shared with a group one of their own clients belongs to. So the
file was theirs to read and the uploader's name was not theirs to see.
Measured: "Stranger Client Ltd", on nobody's roster, ranked on a scoped
dashboard. assignableClientIds is what the widget is actually asking, and
it is what #1699 used. Their version was right and mine was not.
**The client guard is one method now, not eight copies.** #1696 wrote it
as a private guardTarget() rather than repeating viewer-resolve plus
abort at each site, which is better, and this is a change whose whole
argument is that a rule stated in many places drifts. Behaviour is
identical; the eight sites now read as one rule.
The published document reorders a 404 below a 422 on one path. Scramble
reads abort_unless out of a method body but not out of a helper it calls,
so the 404 now comes from route model binding instead of from the inline
abort -- same response, different position. #1701's body names this trap;
worth knowing it costs ordering and not content.
Credit where it is due: both come from denkfabrik-li's #1696 and #1699,
which were open while I was writing the same fixes. Those two are closed
against this and against e7b5b6a, 4b8220a and 67e9204.
This overturns something #1701 decided, so it should say so. That PR
closed the membership hole and left GroupsController::update and
destroy installation-wide on purpose, on the grounds that managing the
group object is a different question from managing who is in it.
What decides it is a measurement that was not in front of that decision.
An assignment to a group is how its members reach a file, so deleting a
group revokes that access for every member. Measured before this guard,
with a client-scoped role holding the group permissions:
stranger client can read the shared file true
PATCH /groups/{stranger group} 302, renamed
DELETE /groups/{stranger group} 302, group gone
stranger client can read the shared file false
So a staff member who may not add somebody to a group out of their reach
could delete it out from under the people already in it. That is not a
gentler version of the membership rule, it is a harder one, and the two
sitting on opposite sides of the same boundary was the odd part.
StaffLibraryScope::allowsGroupChange is the reach half of
allowsGroupMembership on its own, since no client appears in this
question -- one predicate, two callers, rather than a second statement of
it. Both surfaces take it, at 404, matching the membership guards.
A group that shares nothing beyond the actor's library still passes, so a
group they created or one holding their own clients stays theirs, and
unscoped staff are unaffected by construction.
The API document moves a 404 above a 422 on two paths. Both already
documented the 404 -- route model binding produced one -- and Scramble
orders responses by where they appear in the method, so the guard landing
before the validate() call is the whole of the change.
The four routes that edit a group's membership -- add and remove, web
and API -- contain no authorization call of any kind. `can:edit_groups`
in front of them is the whole of it, and a permission is not a boundary.
The authorization sweep looked at these and let them stand, on the
grounds that groups are installation-wide by design: GroupsController
::index lists every group unfiltered, so list and single-object access
agree, and there is no listing/direct-access mismatch to fix. That is
true, and it is the answer to the question of who may *see* a group.
This is a different question: what a write to one *does*.
Joining a group hands the new member everything shared with it. When
that member is one of a client-scoped staff member's own clients,
File::scopeVisibleToClient hands the same content straight back to them
-- that scope is what StaffLibraryScope::files() is built out of. So the
one write turns a file they get a 403 on into a file in their library,
and the download that follows is a 200. ResolvesShareTargets draws that
line on the sharing path through canAssignGroup(); nobody drew it on the
membership path, and canAssignGroup() is *derived from membership*, so
whoever may edit the list also decides what the list entitles them to.
StaffLibraryScope::allowsGroupMembership answers it directly instead of
through the derived predicate, which is the wrong tool here twice over.
Membership asks about reach, so it checks reach: the client must be one
this staff member holds, and the group must not already reach past their
library -- no file assigned to it, and no folder shared with it, outside
StaffLibraryScope. A group nothing has been shared with passes trivially,
which matters, because canAssignGroup() would have said no to a group
that has no members yet and left a scoped staff member unable to put the
first client into one they had just created.
The same write has a second door. MembershipRequestsController::approve
joins a client to a group with identical consequences, under
`approve_groups_memberships_requests`, and deny() decides about somebody
else's client and emails them about it. Both go through the same
boundary, answering 404 to match the guard already above approve().
The queue and its sidebar badge are narrowed to the clients the viewer
holds, through one scope on the model that both read -- the rule the
comment badge in HandleInertiaRequests already states two branches down
("a client-scoped staff member is not shown a number they cannot act
on"), and the reason VisibleCommentScope owns its own pendingTotal()
rather than leaving the middleware to count for itself. Each row carries
the client's name and email, so an unnarrowed queue was also handing
those over for clients outside the roster. Unscoped staff still see every
pending request.
That narrowing is on the client, not on the group: whether a group is
reachable depends on what is shared with it, which is not a question to
ask row by row in a listing. A scoped viewer may therefore still be
shown a request they would be refused on -- one of their own clients
asking to join a group out of their reach. The names were the part that
leaked.
Unscoped staff are unaffected throughout -- both halves of the predicate
are true for them by construction. No seeded role reaches this: Client
Manager is the only client-scoped role that ships, and it holds no group
permissions, so a custom role is needed to get here at all.
The published API document gains a 403 on both member routes.
Regenerated with php artisan scramble:export; Scramble reads abort_unless
out of the method body but not out of a private helper, which is why the
guard is written out at each of the four call sites rather than shared.
StaffAccounts opens with the rule for roles: "Nobody hands out authority
they do not hold … that turns one permission into every permission and
makes the rest of the matrix decorative." mayGrant enforces it, and
guardTarget applies the same test to an existing account.
The client roster never got the same treatment. `assigned_clients` was
validated as `exists:users,id where type = client` and passed straight to
syncAssignedClients, with nothing anywhere asking whether the actor holds
the clients they are handing out. Assigning a client is not a label: it
is that client's whole library, given to whoever is on the other end.
The case that matters is the actor's own account. guardTarget returns
immediately when the target is the actor — editing your own name and
email is not a question of authority — so a client-scoped staff member
with edit_users could PATCH their own id with every client on the
installation and read the whole library from then on.
assignableClientIds() answers the roster question the way
assignableRoleIds() answers the role one, and the five places that accept
`assigned_clients` — users store and update on both surfaces, and the
account conversion — validate against it. It returns the full roster for
an unrestricted actor, so every caller validates against one list instead
of composing a conditional rule; that list is already client-typed, which
is why one rule replaces the exists() and the type filter together.
The two pickers that offer the roster are narrowed to the same list, so a
form no longer offers a client the request behind it will refuse.
syncAssignedClients is untouched: which ids stick to which role is its
decision and it was never the problem.
FileCommentPolicy::moderate() asked only whether somebody is staff and
holds moderate_comments. It never weighed the file the comment sits on,
and delete() returns true the moment moderate() does — so a client-scoped
moderator could delete any comment on the installation by naming its id.
Three call sites already knew this and wrote the boundary out by hand,
each with its own abort_unless($library->allowsFile(...), 403) after the
gate. The two that did not are FileCommentsController::destroy(), web and
API: both bind a comment directly, so nothing earlier in the request
establishes that the viewer may see its file.
The intent was documented in three places and enforced in none of them by
the policy — StaffLibraryScope says "the policies consult allowsFile() so
direct access respects the same boundary", VisibleCommentScope says "a
moderation screen is not a way around the visibility model". Put the rule
where those docblocks already say it lives.
moderate() now takes the comment when there is one. Named against the
class it still answers the coarser "does this user moderate at all",
which is what the queue's gate and the affordances ask. Membership is
tested by file id, so a file soft-deleted out from under its comments is
not in a scoped moderator's library either.
The author branch of delete() is deliberately untouched: deleting your
own words inside the edit window is not moderation, and a client is not
client-scoped in StaffLibraryScope's sense.
Approving through the API now derives its 403 from Gate::authorize rather
than the removed abort_unless, so the committed OpenAPI document gains
the shared AuthorizationException ref in place of an inline "An error"
schema — the shape nine of the other twelve documented 403s already use.
I said they could not, in the guide, the Zapier page and the changelog.
That was carried over from the limitation of polling a list, where a
deleted row stops being returned and nothing marks the moment it went. It
was never true of the activity endpoint: file.deleted is recorded like
any other action, so ?action[]=file.deleted works today.
Checked with a test rather than the enum, which turned up the shape a
caller needs: a deletion entry has no subject, because the row is gone by
the time the entry is written, so the name is snapshotted into
context.name instead. Reading subject.name there gets you null.
Every list in /api/v1 answers "what is there now". Nothing answered
"what happened", and for the two events people most want to act on there
was nowhere to look at all.
Sharing a file writes an assignment row and never touches the file, so no
amount of polling /files?updated_since= will ever show a share. A
download is recorded only in the activity log. So the most requested
automations for a file-sharing product — tell me when a client gets a
file, tell me when they open it — were not possible to build.
GET /api/v1/activity is one feed rather than one endpoint per event,
because the log already records every one of them and a caller filtering
by action gets whatever the application grows later without waiting for
us to expose it.
It reuses what already exists: view_actions_log is the permission the
activity screen uses, and ActivityLogScope narrows the rows the same way,
so a staff member limited to their assigned clients cannot read the whole
installation's log through a token when the screen would not show it.
Two deliberate limits. Class names never reach the wire — subject.type is
a stable public string, or moving a model between namespaces would be a
breaking change to a frozen contract. And no ip_address, though the
column exists and the screen shows it: a person looking at a log has
decided to look, where an integration streams every row to somebody
else's servers by default.
PollingQuery grew an optional column so it can walk a table that is
appended to rather than edited. The parameter stays updated_since
everywhere, because on an append-only log the two timestamps are the same
thing and one shape learned once is worth more than a second name.
Two things the guide got wrong, both of which would waste somebody's
afternoon.
It said tokens max out at a year and left it there. There is a "Never
expires" option on the token screen, and for an unattended integration
that is usually the right choice — the guide now says so, along with why
it is off by default and what to do instead if you keep an expiry.
And everything in it uses Webhooks by Zapier, which Zapier includes only
from its Professional plan up. Somebody on the free plan would follow the
whole page and then find they cannot add the step. That belongs at the
top, not in a support ticket.
The API has had everything Zapier needs since it shipped — a bearer
token, an auth-test endpoint at /me, and list endpoints that return
newest-first with a stable id, which is exactly the shape a polling
trigger wants. What was missing was anyone saying so.
This is written for somebody wiring up a Zap, not for somebody writing
code, which is why it is a second document rather than a section of the
guide. Same reason it renders in-app rather than linking to GitHub: the
installations most likely to need it are the ones least likely to have
outbound internet access.
It says out loud the two things that will otherwise be discovered the
hard way — a token expires within a year and nothing renews it, and a
deletion cannot start a Zap because polling cannot see one.
Discussion feedback: in ProjectSend "sending" can just as well mean
files. Outgoing email is unambiguous and the established name for this
screen elsewhere (GitLab, Jira, Moodle all call it that).
A step-by-step guide (docs/email-oauth.md, same shape as the API
guide, whitelisted alongside it) through both vendor consoles: the
Entra app registration with its three account-type choices and what
each means for the tenant field, and the Google Cloud client with its
consent screen, test users and the testing-status 7-day refresh-token
expiry. The troubleshooting entries are errors actually hit while
building this — including Graph's ErrorQuotaExceeded, whose message
text hides that the mailbox may simply be full.
The Sending tab links to the guide right where an admin picks an
OAuth provider, via the shared links.source origin.
Client file sharing, rebuilt from the ground up: a private area per
client, resumable uploads, folders, groups and categories, sharing with
expiry dates and download limits, comments, file versions, an activity
log, a REST API, and sixteen languages.
This repository begins here. ProjectSend 2 was developed privately, and
that development history is not published — the previous generation
remains available, with its own history, at projectsend/legacy.
Free software under the GNU General Public License v2, or (at your
option) any later version.
ignacionelson