mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-17 00:55:07 +00:00
main
8 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 denkfabrik-li
|
19ee9d9833 |
Narrow the membership an API member write hands back
Adding or removing a group member answered with the group, and loaded the
relation whole:
return new GroupResource($group->loadCount('members')->load('members'));
GroupResource gives each member an id, a name and an email. So a
client-scoped staff member who added one of their own clients to a group
was handed, in the same response, the name and address of every other
client in it -- people they may not read anywhere else in the application,
and whom the group edit screen refuses to name for exactly that reason.
syncWithoutDetaching() makes the call idempotent, so the same request
returns the same list as often as it is sent.
The boundary is already written down. GroupResource's docblock:
both narrow the list to the clients the viewer may act on, and the
controller loading this relation is where that narrowing is applied
and Api\GroupsController::show() does it for the read of the same group,
noting that "it hands back the membership with addresses". Changing the
membership is not a reason to be told more than reading it is, so both
halves now narrow by the same query, through one private helper rather
than a third copy of it.
members_count is deliberately left whole, matching show(): a size is not
an identity, and it is the number the group listing already reports.
Nothing about who may perform the write changes -- StaffLibraryScope
::allowsGroupMembership() already decided that, and still does. This is
only what the answer is allowed to say.
Tests: added beside the existing "the API twin narrows the membership it
hands back", which covered the read half only. Both write tests fail
against the unfixed controller; the third pins that an unscoped token
still gets every member.
|
||
|
ignacionelson
|
1644d634d5 |
Merge pull request #1720 from denkfabrik-li/fix/group-reach-expired-file
groupReachesNoFurther() decides whether a client-scoped staff member may edit a group, by asking whether anything shared with it sits outside their library. |
||
|
denkfabrik-li
|
a1773cad5e |
Count a shared folder's contents as reach, not just the folder
groupReachesNoFurther() asks whether anything shared with a group sits outside the viewer's library. Its docblock says the folder half covers "the folders whose subtrees it can browse". It compares the folder ids the assignment names and stops there. A folder shared with a group hands its members the whole subtree -- File::scopeVisibleToClient matches on folder placement, and a folder is visible to a client when it or an ancestor is shared with them. So the guard passed on a subtree it had never looked into. Measured on main: a scoped rep's own folder, a subfolder somebody else created inside it, and that person's file in the subfolder. parent in the rep's library true subfolder in it false the file in it false add their own client to a group holding the parent 302, allowed the client can then reach the file true And because files() is "own uploads plus everything my clients can see", the file lands in the rep's own library on the next request. That is the widening this guard exists to refuse -- the first test in the file is called "a scoped staff member cannot widen their own library through a group". The folder half now walks each assigned folder's subtree, and the files inside it are checked too: a folder can be in the library while a file in it is not, since somebody else's upload into a folder this rep owns is neither their own nor their clients'. Expired files are skipped for the reason the deleted ones are -- membership grants nobody access to one. Three tests: the subfolder case, the stranger-file case, and a subtree wholly inside the library, which stays manageable. The first two go red without the fix. |
||
|
denkfabrik-li
|
db1dd71f3c |
Stop an expired file locking a group shut for a scoped staff member
groupReachesNoFurther() asks whether anything shared with a group sits
outside the viewer's library. `f1b35cc9` established the shape of the
answer for deleted files: start from the live row, because "a deleted file
is not reach, because nobody can reach it".
An expired file is the same case. Membership grants nobody access to it --
File::scopeVisibleToClient ends in notExpired(), so it has left every
member's /my-files and the download answers 403 -- but it is equally gone
from files(), where its absence reads as "outside my library". The group
then locks for a scoped staff member: they cannot add a member, cannot
rename it, and cannot remove their own client again.
So the reach query skips expired files as it already skips deleted ones.
Expiry is reversible where deletion is not, and that needs no special
handling: the guard asks what is reachable at the moment somebody is added
or removed, and the file counts again the moment it stops being expired.
Not changed: File::scopeVisibleToClient, whose treatment of expiry was
settled deliberately in
|
||
|
denkfabrik-li
|
eade690f73 |
Hold the group edit screen to the same library boundary as the rest
Every other group route asks StaffLibraryScope whether this viewer may
act on this group. GroupsController::update() and ::destroy() do, and so
do their API twins -- all four with abort_unless(allowsGroupChange, 404).
The two that read do not: edit() and Api\GroupsController::show() had no
boundary at all.
What they hand over is the membership, name and email per member, plus
the whole client roster of the installation as available_clients. So a
client-scoped staff member could open a group whose contents they cannot
see, read off every client on the installation, and only be refused when
they pressed save.
Two halves, because the leak has two shapes:
- The group itself. Reading it now asks the same reach question the write
half asks, one step earlier, with the same 404 -- a group that reaches
past the viewer's library is not theirs to open either.
- The lists inside it. Both narrow through StaffLibraryScope::clients(),
the listing half of the rule this screen's buttons are already guarded
with: allowsGroupMembership refuses removing a member outside the
roster, and refuses adding a client outside it. Naming them anyway,
with their address, is the mistake ClientsController made before
clients() existed -- that method's own docblock says so.
The reach guard alone would not have been enough. A group nobody has
shared anything with reaches nowhere, so it stays open to everybody --
and it can still hold a stranger's client. That case is why the lists
narrow separately, and there is a test for it.
members_count is left whole on purpose: a size is not an identity, and it
is the same number the group listing already reports.
GroupResource's docblock claimed members are safe to expose because "the
group edit screen already shows [them] to anyone holding edit_groups".
That was a claim about a screen, and it stopped being true the moment the
screen narrowed. Reworded to say what now holds it up, and where.
Not changed: the group listing. It reports names and member counts, not
identities, and every button on it is guarded. Nor Api\GroupsController::
index(), for the same reason. Nor the API document -- scramble:export is
byte-identical, because GET /groups/{group} already documented a 404.
Four tests. Three measured red against the unguarded controllers (3
failed / 21 passed): the group cannot be opened at all, the edit screen
stops naming strangers, and the API twin narrows what it hands back. The
fourth -- an unscoped viewer keeps the whole roster and every member -- is
green either way and guards against the fix over-refusing.
Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
|
||
|
ignacionelson
|
eb2917f5ff |
Hold a group object to the same boundary its membership already has
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. |
||
|
ignacionelson
|
f1b35cc9f6 |
Stop a deleted file locking a scoped staff member out of a group for good
#1701 closed a real hole: group membership decides what a client reaches, and through File::scopeVisibleToClient it decides what the staff member holding that client reaches, so `edit_groups` alone was never a boundary. The predicate it added asks whether everything shared with a group is already inside the actor's library. It asked by counting: pluck the group's assignment rows, count how many of those ids the library query returns, and require the two to match. An assignment row outlives the thing it points at — nothing clears them when a file or folder is deleted — while files() and folders() exclude trashed rows by construction. So one deleted file left a count that could never balance again, and the group closed permanently: the scoped staff member could no longer add their own client to it, or remove anybody from it, with a 403 and nothing to explain it. Every group accumulates dead assignments over time, so groups would have gone quiet one at a time. Asked the other way round — is there anything live, shared with this group, that is outside my library — the dead rows drop out by construction, because the query starts from File/Folder rather than from the assignment. That is also the truer question: a deleted file is not reach, since nobody can reach it. Three tests. A group stays usable after a file shared with it is deleted, including removing a member; the same for a deleted folder assignment; and the half that must not soften — a live file still out of reach is still refused, deleted siblings or not. |
||
|
denkfabrik-li
|
8a6543073b |
Group membership is a library boundary, not just a list
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.
|