mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
main
18 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ignacionelson
|
5fb17388cd |
Stop scoped staff reaching groups that are not theirs
Reported by @Drescargot as GHSA-r3hg-3fxw-rcmr, in two halves. The groups listing never narrowed at all. Every other action in that controller is guarded with allowsGroupChange(), and index() — web and API alike — built a bare Group::query(), so a client-scoped staff member was shown every group on the installation with its name, description and member count. StaffLibraryScope::groups() is that narrowing, and assignableGroupIds() now reads from it rather than restating the same rule a second time, which is how the two drifted apart to begin with. The second half is the one that mattered. allowsGroupChange() asked only groupReachesNoFurther() — "is anything shared with this group outside my library" — which a group with nothing shared with it yet passes vacuously. So a scoped staff member could rename, delete or publish a group whose every member was somebody else's client. Publishing is the sharp end: whatever is shared with the group afterwards is reachable without signing in. The reporter suggested putting the membership check inside groupReachesNoFurther(). Tried, and it breaks two things. That predicate is shared with allowsGroupMembership(), where a group nobody has joined must stay usable so its creator can add the first member. And "every member must be mine" is the obvious reading of the rule and is wrong: it turns GHSA-whmp-p9hv-r7j7's narrowing — a mixed group's edit screen loads and simply does not name the stranger — back into a 404, undoing that fix. Four tests from it fail that way. So the check sits in allowsGroupChange() alone, and asks whether the group is wholly somebody else's rather than whether it is wholly theirs. A mixed group stays workable and is still covered by the reach check; an empty one stays nameable by whoever just made it; a group with members and none of them theirs is refused. |
||
|
ignacionelson
|
d6fd5a917d |
Send downloads the way the web server in front of us understands
Uploads live outside the web root, so PHP authorizes every download and then hands the file to the web server with a header naming it. Four routes decided that for themselves and all four hard-coded nginx's spelling. On Apache or LiteSpeed nothing acts on the header, so the empty body PHP sent goes to the visitor: files upload fine, thumbnails are broken images, and downloads arrive as 0 bytes, with every other page working. Reported as #1765 from an Apache 2.4 install, and before that as #1266, #1215, #870 and #1271. It is also a regression from v1, which had a download_method setting -- php, apache_xsendfile, litespeed, nginx_xaccel -- defaulting to php. v1 therefore worked on any server out of the box and v2 did not, and a v1 Apache user migrating lost every download with nothing to tell them why. So the four sites now go through one FileDelivery, and it picks: auto (default) nginx when SERVER_SOFTWARE says nginx, else php nginx X-Accel-Redirect, a URL path via the internal location xsendfile X-Sendfile, an absolute path (Apache mod_xsendfile, LiteSpeed) php BinaryFileResponse Defaulting to auto rather than nginx is the point of the change: a default that assumes nginx leaves an Apache install exactly as broken as it is today until somebody reads INSTALL.md. Slow beats empty. Auto never picks xsendfile, even where the module is loaded. mod_xsendfile also needs XSendFilePath to allow the storage directory, which cannot be seen from here, and choosing it on the strength of the module being present would trade a silent failure an administrator can diagnose from the dashboard for one nobody can. BinaryFileResponse rather than a readfile loop because it answers Range requests. nginx does that itself on the fast path, so hand-rolling it would have broken seeking through a video on exactly the installations this fallback exists for. Verified end to end: 206 with the right Content-Range through the live stack. Two guards. Every method checks the path cannot climb out of the storage area -- nginx resolves `..` in the URL it is handed as happily as PHP would -- and the two methods that hand over a filesystem path resolve it and prove it lands inside the root. Callers pass paths from rows they just authorized, so this is a backstop; it is here because the cost of being wrong once is handing over any file the web server can read. The dashboard's System panel names the method, with a warning icon and a dialog when PHP is doing the sending: what is happening, what it costs (one worker held for the whole of each download, so a few large simultaneous ones can occupy every worker while the processor sits idle), why it is set that way, and the three ways out. Written to be accurate rather than reassuring -- nothing is broken, it does not scale -- and the notice stays even when php was chosen deliberately, because the trade-off is the same either way. /system/settings/downloads repeats it, which is where somebody coming from v1 goes looking for the dropdown. An environment variable rather than a stored setting: it describes the server this installation runs on, not a preference, and a value in the database travels to a different server in a restore and is wrong there. Read only in config/projectsend.php, so config:cache cannot blank it. The suite pins itself to nginx. Left at auto it would detect no server at all, fall back to php, and quietly retire the coverage of the mechanism most installations actually use. |
||
|
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
|
5117511946 |
Merge pull request #1732 from denkfabrik-li/fix/public-preview-log-debounce
FileThumbnailController::preview() writes at most one FilePreviewed row per viewer per file per five minutes, because watching a video is a single deliberate act that the browser turns into dozens of Range requests. Its docblock ended by naming the route where the same act happens without an account -- PublicGroupsController::preview -- and that route logged unconditionally. Five requests for the same public file wrote five rows where the signed-in twin wrote one, so one visitor watching one clip buried the public half of the activity log, which is the half an operator reads to see what the outside world is doing. The window moves into a shared PreviewLog, next to PreviewKind, which those two routes already share for the same reason. Keying is unchanged for a signed-in viewer. An anonymous visitor has no account to key on, so the request IP stands in -- the same substitute ApiServiceProvider's rate limiter makes for an unauthenticated caller. It is a cache key with a five-minute life and never reaches the log, which keeps its own decision about recording an IP. Downloads are deliberately untouched and stay one row per download: each is a transfer, and DownloadAllowance::used() counts those rows to enforce a per-file cap, so swallowing one would hand out free downloads. The limit this leaves open, since the IP is a stand-in and not an identity: two anonymous visitors behind one address share a key, so within five minutes the second one's view of the same file is not recorded. That is the same trade the signed-in side has always made per account, and the alternative is the row-per-Range-request this fixes. Verified before merging: 24 passed across the public-preview and thumbnail suites on the trial-merge, which also confirms this co-exists with #1725 -- the two share both controllers and change different methods in each. With app/ reset and PreviewLog deleted, 1 failed / 9 passed. The signed-in route's existing debounce tests pass unchanged, which is what says the shared class did not move that side. request()->ip() honours the trusted-proxy configuration, so a forged X-Forwarded-For cannot defeat the window from outside. Reported and fixed by @denkfabrik-li. |
||
|
denkfabrik-li
|
fc758c701a |
Write a rendition through a temporary file, and never serve an empty one
Both thumbnail routes treat "the file exists" as "the rendition is cached", and nothing ever invalidates one: RenderedImageCache::flush() runs on ImageRenderingChanged, which no core code raises. Whatever is at the path is what every later viewer gets. ThumbnailGenerator encoded straight onto that path. A render that died partway -- a full volume, a killed worker -- left a half-written file that was then served as the rendition for good, and two requests rendering the same file at once encoded into one path together. It now writes beside the destination and renames into place. rename() within a directory is atomic and replaces what is there, so the path is either the previous rendition or a complete new one, and the loser of a race leaves a whole image rather than a mixture of two. The temporary file is removed on the way out either way. The read side gets the other half: an empty file is not a rendition, so both routes replace one rather than serve it. Writing through a temporary file means this state can no longer be created here, but an installation that ran an older version can already have it on disk, and nothing else will ever clear it. Three tests: an empty rendition is replaced on the signed-in route and on the public one, and a successful render leaves nothing half-written behind. Without the fix the first two go red; the third is about the fix's own temporary file and passes either way. |
||
|
denkfabrik-li
|
c2dd2c758a |
Debounce the public preview log the way the signed-in one already is
FileThumbnailController::preview() writes at most one FilePreviewed row per viewer per file per five minutes, because a browser turns one video into a long tail of Range requests against the same URL. Its docblock names the anonymous route as the place the same act happens without an account -- and that route logs unconditionally. Measured: five requests for the same public file, five PublicFilePreviewed rows, against one for the signed-in twin. One visitor watching one clip buries the public half of the activity log, which is also the half an operator reads to see what the outside world is doing. The window is now a shared PreviewLog, next to PreviewKind, which the two preview routes already share for the same reason. Keying is unchanged for a signed-in viewer; an anonymous one has no account to key on, so the request IP stands in -- the same substitute the API's rate limiter makes for an unauthenticated caller. It is a cache key with a five-minute life and never reaches the log, which keeps its own decision about recording an IP (ActivityLogger::shouldRecordIp, Setting::DownloadIpLogging). Three tests: the replay is one row, two visitors are two rows, and the window is per file. Without the fix the first goes red. |
||
|
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.
|
||
|
Ignacio Nelson
|
dacf2b3eda |
Merge pull request #1693 from denkfabrik-li/fix/public-download-external-disk
Hand over a public download from the disk the file is on |
||
|
Ignacio Nelson
|
c5d32c06f6 |
Merge pull request #1684 from denkfabrik-li/fix/create-only-redirect-403
Land a successful create where a create-only role can actually go |
||
|
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. |
||
|
Ignacio Nelson
|
350a7b3073 |
Merge pull request #1705 from denkfabrik-li/fix/deny-membership-request-once
Deny a membership request once, as approve() already does |
||
|
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.
|
||
|
denkfabrik-li
|
5242169bb0 |
Deny a membership request once, as approve() already does
approve() refuses a request that is not pending:
abort_unless($group !== null && $client !== null
&& $membershipRequest->status === MembershipRequest::STATUS_PENDING, 404);
deny(), one method below, checks nothing. Denying is not idempotent, so
repeating it is not a no-op:
- denied_at is stamped again, and that is what the client's re-request
cooldown counts from (MyGroupsController::inDenyCooldown). Repeating
the request keeps one client out of one group for as long as somebody
cares to keep asking, without a single new decision being made.
- a second GroupMembershipDenied entry goes into the activity log, for
a denial that did not happen.
- a second "your request was declined" mail goes to the client.
The queue lists only pending requests, so nothing on the screen offers
this; it takes asking for the route directly. It needs
approve_groups_memberships_requests, so it is not a stranger's move.
The guard is the same one, answering the same 404, placed where deny()
can reach it. deny() keeps tolerating a vanished group or client -- that
tolerance is deliberate and separate: the denied row persists for the
cooldown even when the group it named is gone, and index() already
filters those rows out with whereHas.
Not in this change: deny() writes the status, the log entry and the
notification without a shared transaction. approve() has exactly the same
shape, so fixing one alone would replace a symmetry with a difference,
and doing both means also deciding where the mail sits relative to the
commit -- which is the question #1691 answers for file bytes, and worth
answering on its own rather than inside a state-machine fix.
|
||
|
denkfabrik-li
|
a8b1987e2c |
Hand over a public download from the disk the file is on
|
||
|
denkfabrik-li
|
e6dc271f27 |
Land a successful create where a create-only role can actually go
Four create flows redirected to the new record's edit page on success, but store is gated by create_* while the edit page is gated by edit_*, and PermissionChecker has no create-implies-edit rule. A role holding create_* without edit_* would create the record -- write, activity log and notifications all run -- and then meet a 403 on the success redirect, with no way to tell the action worked and every reason to submit a duplicate. Categories is reachable with plain UI clicks, since the sidebar shows it from create_categories alone. Keep landing on the edit page for anyone who may edit, and divert only those who can't -- to the create form, which shares store's own gate and is therefore reachable by exactly whoever just created the record; the success toast shows there. The index would not do: Clients/Groups lists are gated by manage_*, which store itself does not require. Implying edit_* from create_* would not do either -- edit has no own/others split here, so it would silently hand a deliberately narrow create-only role edit (two-factor reset included) on every existing record. |
||
|
ignacionelson
|
57540164fa |
Read a file from the disk it is actually on, everywhere
Two routes still assumed every file sits on local disk, which stopped
being true the moment external storage was switched on. A share link
answered with X-Accel-Redirect whatever the file's disk said, pointing
nginx at a path it has nothing behind; a public listing built a
thumbnail from Storage::disk('files')->path(), which for an externally
stored file is a path nobody ever wrote. Both fail only for installs
using S3, and only on those two routes, so the same file downloading
correctly from the file manager made the share link look like the
broken thing rather than where the file lives.
Neither is a new rule. FileDownloadController and
FileThumbnailController already did it right, which is the actual
finding: the knowledge was sitting in a private method on one class and
inline in another, so the next caller could not inherit it and did not.
Both are now objects with one job.
StoredFileResponse replaces InlineFileResponse and grows an
attachment() alongside inline(), since the two differ only by
disposition. LocalSourceFile takes a closure rather than returning a
path: the version that returned one also left the caller to unlink it,
and both of those are exactly the mistakes made here.
The regression tests fail against the previous controllers — checked in
both directions rather than assumed.
|
||
|
ignacionelson
|
88c182cf3b |
Preview video, audio and PDF, not only images
v1 could preview four kinds of file in a modal — images, video, audio and PDF. v2 previewed only images, and not by decision: preview shipped as part of the image *thumbnail* work (1c68aa1), so "previewable" quietly became a synonym for "GD can decode it". FileThumbnailController::preview() gated on ThumbnailGenerator::SUPPORTED_MIME_TYPES, the frontend mirrored the same four types, and the dialog was a hardcoded <img>. Rather than widen that list — it drives pathFor(), extensionFor(), generate() and FileDiskCleanup, and a video reaching getimagesize() is a 500 — this separates the two questions. PreviewKind now answers "may these bytes be served inline, and what element renders them?", while ThumbnailGenerator keeps answering the narrower "can this app decode it itself?", which is what renditions, the cache and the watermark hook actually depend on. Image delegates to it so the two cannot drift. The allowlist stays a security boundary: mime_type is sniffed from the bytes, so text/html and image/svg+xml remain excluded, and PreviewKind is deliberately narrower than "formats a browser might cope with" — no quicktime, avi or matroska, because an embedded player for those shows a black rectangle. Those still download exactly as before. docs/security-audit-2026-08-05.md finding 1 recorded that adding application/pdf "should be a conscious decision". This is that decision, and three things were measured rather than assumed: - An <iframe sandbox> cannot be used. Chrome refuses to run its PDF viewer in a sandboxed frame at all (ERR_BLOCKED_BY_CLIENT, with or without allow-same-origin) — the attribute removes the feature, it does not harden it. - nginx's `Content-Security-Policy: sandbox; default-src 'none'` on /protected-files/ does work (a <video> frame lands in an opaque origin), but Chrome exempts its PDF viewer from it, so it is not what protects the PDF case. - What does is the allowlist plus the browser's own PDF sandbox, where PDF JavaScript has no DOM and no cookies. Range requests were verified end to end: 206 with a correct Content-Range, a byte-perfect file reassembled from three ranges, and a real browser seeking to 10s of a 20s clip. nginx drops the upstream Content-Length on the X-Accel path, so there is no collision. Two settings, both defaulting on so no installation loses what it has: clients_can_preview_files and public_listing_preview_enabled. Staff are never gated. The anonymous side needed a route of its own — there was no public preview endpoint — with its own throttle bucket, since a bare throttle: shares one counter across that whole block. A preview now logs at most one FilePreviewed per viewer per file per five minutes: a <video> turns one deliberate act into a long tail of Range requests, and a row each would bury the log. Also fixes a layout bug the tests could never catch. A portal file row was flex justify-between with three children — name, comment trigger, download — so the middle one settled wherever the name happened to end and the comment icon sat at a different place on every row. The name block now takes the slack and every action lives in one trailing group, with the comment trigger in a fixed-width slot so the icons form a column. And because half the previewable files have no thumbnail to click — a PDF, an mp3 and an mp4 all render as a generic icon — every row gains an explicit PreviewAction beside DownloadAction, matching whatever style that theme gives its download control. |
||
|
ignacionelson
|
6e47d76ba6 |
ProjectSend 2.0.0
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. |