mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
main
92 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ignacionelson
|
3917cb2af3 |
Stop a file expiry date sent as a number from 500ing
Laravel's `date` rule accepts a JSON number when it reads as a real day
(20301231 passes) and hands it on unconverted. Every file expiry field
then passes it to a method that only takes a string, so the request
failed with a 500 instead of a validation error.
Affected: PATCH /api/v1/files/{file}, the staff file editor, the bulk
editor, new share links and the client file editor. Each now also
requires `string`, so a number is a 422 on `expires_at`. Dates sent as
text behave exactly as before. A form never sent a number, so this was
only reachable with a hand-written JSON body.
|
||
|
ignacionelson
|
7c7ba7cd53 |
Stop a typed-in storage quota from 500ing when a client is created
Filling the "Storage quota (MB)" field on the new-client form raised a TypeError and the request died with a 500. Leaving it blank worked, which is why it reached a release: that path goes through `null ?? 0`, and the 0 is an int. The `integer` validation rule checks that a value looks like an integer. It does not convert it. `$request->validate()` returns the raw input, so the form field arrives as the string "2048" -- and the create form types that field as a string in React, so it is a string even over JSON. Both controllers declare strict_types, so handing it to `ClientAccounts::create()`'s `int $storageQuotaMb` is a TypeError. Fixed on both surfaces that call create(): the staff screen and /api/v1/clients. The API twin had the same defect, reachable by sending the quota as a quoted JSON value or a form-encoded body -- its own create test only ever sent a JSON number. Two more call sites had the same shape and are cast too, though nothing sends them a string today: the share-link download cap and a comment's reply_to. Both are safe only because a frontend file happens to call Number() first, which is a fact about that file rather than anything the signature guarantees. The null in each is preserved rather than collapsed to 0 -- "no cap" is not a cap of zero. `storage_quota_mb` is also cast on User and Invitation. The column is an unsignedInteger and both docblocks already promise int; it is read straight into provision()'s typed parameter when an invitation is redeemed, and which type a driver hands back is not something that call site should depend on. Found on the new files-test rehearsal instance, on its first real use, against the same build the whole fleet is running. |
||
|
ignacionelson
|
50f8b578df |
Ask the publication question wherever content lands, not just on upload
Reported by @skeletonsec as GHSA-rxf8-wh8v-jm9j. A file in a public folder is public: isEffectivelyPublic() is "my own flag, or my folder's", read up the whole ancestry. GHSA-237r-jx85-j3hr settled that three days ago, put the rule in Folder::uploadableBy(), and wired it into the upload paths. Content arrives in a folder four other ways. move() drags one file in, bulkUpdate() moves a selection, update() reparents through the edit form, and FoldersController::move() drags a whole folder — every file in its subtree — under a public parent. Each of them asked whether the destination was *visible* to the mover and then wrote folder_id. Visible is not the same question as publishable, and the difference is the entire permission: a staff member given editing rights and deliberately not given upload_public could publish confidential files to the anonymous site by choosing where they landed. The API twin of update() had the same gap. Both earlier advisories named these paths in their own "suggested fix" sections. Neither demonstrated them, so neither was followed. The fix to a report wants the scrutiny the report got, and this one did not get it. The predicate did not need changing — it needed calling. Four sinks now ask it, plus the API twin. The check stays split in two deliberately: the destination is resolved through StaffLibraryScope as before, so a folder somebody cannot see is still a 404 and not an existence oracle, and the publication clause is a separate 403 on top. They agree by construction — allowsFolder() is folders()->whereKey()->exists() — so nothing that used to resolve can now fail the first half. On the file paths the check fires only when folder_id actually changes, which is the convention already there: re-saving a file that sits in a folder out of the saver's scope must keep working. bulkUpdate() checks its destination once instead, before the loop, because there is one destination for the batch and if it publishes then no file in the batch may go. Folder::uploadableBy()'s docblock now says to read the name as "may place into", with why: the name is what made this easy to miss, and the next folder_id or parent_id write will be written by somebody reading it. Ten tests, one per sink with a private-destination control beside it, plus an editor who *can* publish to show the boundary is about publishing and not about moving. The last one follows the advisory's own chain to the end and asserts the thing actually claimed — a stranger with no session, no token and no assignment fetching the anonymous download URL. It returns 200 on the code before this commit and 404 after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx |
||
|
ignacionelson
|
6ad26bb61e |
Hold an upload to the size it said it was sending
Reported by @ry2811 as GHSA-6jh6-gvj5-pv8v. A resumable upload declares its size, and that declaration is what store() weighs against the maximum file size and the client's storage quota. Only the assembled file was ever held to it. The parts in between were bounded one request at a time and never added up, so a client could declare one byte and then stream parts: ten thousand part numbers at twice a 20 MB part is about 400 GB, per session, and the number of sessions was not bounded either. None of it counted against anything, because nothing becomes a File row until the upload completes and ClientStorageUsage sums File rows. A client with a 1 MB quota could fill the volume and repeat. putPart()'s own comment described this defect and treated the per-part cap as the answer to it: "without a cap here the exposure is a day's worth of disk". A cap on one request bounds one request. The exposure was a day's worth of disk multiplied by however many requests somebody cared to make. Three limits, and each one exists because the other two do not cover it. A session may not stage more than it declared. The room for a part is claimed before the body is read — a body's length is not known until it has arrived, and by then it is on the disk being protected — and the write is then capped at exactly what was claimed, so an over-long body is cut off mid-stream as it always was, against a smaller number. The claim is a read and a conditional update under a per-session lock, the same shape complete() already uses: the protocol sends parts in parallel and how many is the client's choice, so an unlocked read lets every part in flight claim the same room, while an atomic claim alone refuses the honest parallel upload instead. Whatever the part really weighs is settled back afterwards, in a finally, or a client's own retries would exhaust a session with room to spare. Open sessions count against the quota at the size they declared. A quota measured against finished files alone is spent twice by opening sessions one after another — each is told there is room, because the ones before it have not finished. The cost is that an abandoned transfer holds its share until it is cancelled or swept, so the sweeper now runs hourly rather than daily: that gap is now somebody unable to upload, which it was not before. And a cap on open sessions, because for anyone with no quota to spend — staff, and clients on an installation that sets none — the session count is the only thing between a declared size and any multiple of it. Four tests fail on the unfixed code, and three existing ones had to change: they declared a tiny size and sent a large part deliberately, to reach the re-checks at complete(). That route is now closed at putPart(), so they reach those re-checks the way a real install would instead — the file-size limit or the quota moving while a long transfer is running, which is the reason complete() re-asks rather than trusting what store() decided. The staged-byte total is BIGINT UNSIGNED, and the suite runs SQLite, which has no unsigned integers. The first version of the bounds read `staged_bytes + :delta BETWEEN 0 AND size` and raised SQLSTATE 22003 on MySQL for any refund — in the comparison, so the bound written to prevent the underflow was the statement that underflowed. Every SQLite test passed on it. Both bounds are now arranged so the column is never inside a subtraction, and UploadSessionStagedBytesMysqlTest skips loudly unless the connection is MySQL. Verified against 8.4, as was the report itself: three sessions declaring one byte each put 6 MB on the volume of a client with a 1 MB quota before, and nothing at all after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx |
||
|
ignacionelson
|
1149df277b |
Ask for the public-folder key before publishing through a folder
Reported as GHSA-237r-jx85-j3hr.
A file is public if its own flag is on or its folder's is, so the upload
destination reaches the property `upload_public` guards without touching
the switch. A staff member allowed to upload but deliberately not allowed
to publish could publish to the anonymous public site by choosing where
the file landed.
No new key. `upload_to_public_folders` already exists, already appears on
every role's checkboxes, and already means exactly this on the client
branch of the same method — MyFilesController's picker calls it the
established meaning of the two keys. It was never asked of staff, so on a
staff role that checkbox did nothing at all: an unenforced permission, the
class this project audited and closed once already.
Effectively public rather than the folder's own flag, because the flag is
inherited down a subtree: a private folder inside a public one publishes
just the same, and a check on the folder's own column walks past it. There
is a test for that case specifically.
One place, because every upload path — the plain POST, the chunked flow,
the API and the client portal — already asks Folder::uploadableBy(). The
sibling report about the target folder not being scope-checked at all
(GHSA-56qr-cq56-qg66) was fixed in
|
||
|
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
|
896675d631 |
Tell a client whether their own file arrived
"Did it arrive?" is the question somebody asks about a file they sent, and on a hosted free account — where a link is the whole of the sharing — the count is the only evidence either way. Every file a client uploaded now shows how often it has gone out and when it last did, in every render mode of every theme. Only their own. A download entry says somebody fetched the file, so a count on a file shared with several clients tells each of them about the others' activity, and nobody is entitled to that but the person who put the file there. A file shared *with* this client carries null, not zero: a zero would itself be a claim, and the two have to be distinguishable because zero is an answer the owner came looking for and is shown as words. Counted from the activity log through the same three actions DownloadAllowance uses, so a file leaving by the public site counts as much as one leaving by its link. One query for a listing, none at all for a client with no files of their own. Both filters have a test that fails when only that filter is removed. Two things a render check caught that types and a green build did not. `t()` does no plural selection — the catalogues are flat key/value — so a "one|many" string reached the screen with its pipe intact; the strings are whole sentences now, with the singular spelled out. And the gallery card was already laying its text out beside the action icons in a 200px column, truncating the filename to "Q…" and the size to "75 …" on main today; stacking them gives every line its full width. |
||
|
ignacionelson
|
92bb807849 |
Show a client the public link to their own file
A client's portal lists two kinds of file side by side: what they uploaded, and what somebody shared with them. Where a link exists on one of their own, they can now copy it from the row — which is what makes the hosted free plan a product rather than a place to put files, since a customer there has no staff screen on which to make one. The rule is narrow, and both halves are load-bearing: a link this client created, on a file this client uploaded. Not "a link on a file shared with them" — that link is the sharer's decision about who may reach the file, and handing the recipient the URL would quietly turn "you may download this" into "you may pass this on to anyone". And not "any link on their own file" either — a link staff minted on a file a client uploaded exists for a reason the client may be no part of, and on the shared instance it would sit beside the one link they were promised. Ordering is by id, so an unfiltered lookup would hand them whichever was minted first. Both halves have a test that fails when only that half is removed. The first draft did not: every case was carried by the ownership filter alone, so the creator check was green for the wrong reason. Links that no longer work are left out rather than shown greyed. The only thing a client can do here is copy it, and a URL that answers "this link has expired" is worse than no URL at all. One query per listing, not one per row, and none at all for a client with no files of their own. |
||
|
ignacionelson
|
757fba19ca |
Give uploads a seam, and link-minting one home
Two pieces of groundwork, no behaviour change.
FileWasStored is dispatched from StoreUploadedFile, which every upload
path converges on — the chunked flow staff and clients share, and the
synchronous POST beside it. A listener therefore sees each upload once
without knowing which route produced it, which is the property that makes
it usable from outside this repository. A notification, not a filter:
nothing on it is mutable, and anything that needs to influence an upload
has to do so before the bytes land, which is what ResolvingUploadDisk is
already for.
CreateShareLink is the other half. Minting a link was a ShareLinksController
private concern, and the controller is an HTTP handler behind `staff`
middleware — so a link now needs making from outside a request as well.
Two copies of "make a token, write the row, log it" would drift, and the
half most likely to drift is the token, which is the entire authorization
for /s/{token}: there is no session behind it and no second factor, so
being unguessable is its only defence. Anything minted through the action
gets Str::random(32) — about 190 bits, more than a UUID's 122 — and never
a chosen value. The chosen-token path stays in the controller, where a
person is typing one into a form and its minimum length can be argued
about in a validation rule.
The permission questions stay in the controller too. Whether somebody may
set an expiry or a download cap is a fact about them, and the action has
no viewer to ask; it takes both already resolved, including the expiry,
because "the end of the 12th" depends on whose timezone you are in.
Five tests, including that the file a listener receives is complete and
readable rather than half-built, and that the staff form still refuses an
expiry to somebody without the permission after the extraction.
|
||
|
ignacionelson
|
763e7b0e2e |
Render one image once, however many requests ask at the same time
Renditions are generated on demand and cached by existence, and nothing between the callers stopped two requests decoding the same image at once. The atomic rename settled which file survived; it never stopped both from doing the work. So N concurrent requests for one cold rendition were N full-size decodes, each holding four bytes per source pixel — up to 160 MB at the 40-megapixel ceiling. That is not an attack. A public listing emits a thumbnail URL per file, a browser opens six or more connections at once, and the first visit to a gallery of ordinary camera images was six simultaneous decodes on a container sized for one. PublicGroupsController reaches the generator with no account at all, so nothing about it required a customer to be signed in, and the 240/min throttle bounds rate rather than concurrency. Worse than a crash, it did not resolve itself: a render killed mid-flight renames nothing, so the cache warmed only by whatever finished before the kill and the page died again on the next visit. A lock keyed on the destination path — which already encodes the file, the audience and the rendition, so two requests collide exactly when they would have written the same path. The waiter re-reads after acquiring, which is what turns a wait into a cache hit rather than a second decode of the same image. Waiting rather than refusing, because the arithmetic says so: a waiting request holds an idle worker at about 35 MB, a rendering one holds that plus the whole source bitmap. Six waiters cost what one renderer costs. On timeout it refuses instead of rendering anyway. Falling through would reinstate the pile-on at the moment the system is already struggling, and one failed thumbnail is a better outcome than a container that dies and takes the warm cache with it. The wait is configurable because the right number is a property of the machine — a small VPS reading a large source off a slow disk wants longer — and clamped to at least a second, since a stray empty variable would otherwise make every concurrent request fail instantly, which is the opposite of the point. Eight tests. Two go red without the lock, and the clamp is asserted on the resolved value rather than the clock, because block() measures in whole seconds and a timing assertion there would be flaky rather than wrong. Found by the session sizing free-tier containers, from the outside. |
||
|
ignacionelson
|
a5496d24cd |
Stop describe() vouching for a detection it could not make
`FileDelivery::describe()` from a console returned
`{"method":"php","detected":true}` on every installation, whatever its web
server. detect() reads SERVER_SOFTWARE, which only exists inside a
request, so a console process has nothing to look at and falls to the
`php` default — and `detected: true` then vouched for it.
The value is right for that process and wrong as a statement about the
installation, which is how anybody running it from `artisan tinker` will
read it. Somebody verifying a healthy nginx tenant hit exactly that, spent
an afternoon on it, and only recognised it as an artefact of *where* the
question was asked after reading `nginx -T` in the container.
There is now a third field. `observed` is false only outside a request,
where `method` is a default rather than a finding. Both screens that read
this run in a request and always see true; it exists for whoever asks from
a shell, which is the one place the answer could mislead.
The two web paths are unchanged and were never wrong — `projectsend:status`
does not report delivery at all, so no fleet ever reported this
incorrectly. What was wrong was a confident answer to a question that
could not be answered from where it was asked.
Three tests: a console reading says not observed and still says php,
because php is what that process would actually do; a reading during a
request observes nginx; and a stated method is observed wherever it is
read, since a decision needs nothing detected to be true.
Found by the session verifying the 2.4.0 canary, not by me.
|
||
|
ignacionelson
|
b758fca19c |
Merge pull request #1724 from fix/assemble-keeps-parts-for-retry
Keep an upload's parts until its bytes are stored |
||
|
ignacionelson
|
02946abf85 |
Stop the delivery docblock naming nginx as the only local path
#1733 explains its two lifetimes by contrasting a presigned URL with X-Accel-Redirect, "nginx serves these bytes, now, to this request". That was true when the branch was written and stopped being true on 1 September, when FileDelivery gave the local path four methods — auto, nginx, xsendfile and PHP streaming. The argument survives intact: every one of those authorises exactly one response and nothing that outlives it, which is the property the contrast rests on. Only the naming was stale, and a docblock that says "nginx" to an operator running Apache reads as "this does not apply to me". Found resolving the merge, not by the author — the branch predates the change it collided with. |
||
|
ignacionelson
|
b7ac44e77b |
Merge pull request #1733 from fix/presigned-download-window
Give a download's presigned URL a minute rather than an hour Conflicted against FileDelivery, which landed on main after this branch was written: main added a constructor where the branch added two constants. Both belong; the resolution keeps each. |
||
|
ignacionelson
|
8de28059db |
Say when a folder choice publishes the file
Found reviewing the client file editor rather than building it. File::isEffectivelyPublic() is "my own flag OR my folder's", and Folder::uploadableBy() admits a client to a public folder on upload_to_public_folders — a different key from upload_public. So a client can make a file world-readable without touching the public switch, and without holding the key that switch is behind. That is what those two keys have always meant and what uploading into such a folder has always done, so this does not refuse it. What was new is where the choice is made. The upload page is entered from a folder the client has already navigated to, where the list shows a Globe badge on a public folder. The editor's picker is a flat list of names, and it is the first place a destination is chosen with none of that context — so the consequence was invisible exactly where it mattered most. Public folders now carry the badge in the picker, and choosing one says in words that anyone will be able to open the file without signing in. Two tests: that the side door genuinely publishes and is labelled, and that a private folder is not labelled — a warning on everything is a warning on nothing. The rest of the review found no defect. Ownership, the per-field keys, the staff-scope trap and mass assignment were already covered; a client deleting a file that staff later revised was checked directly and moves the chain's recipients onto the successor without widening them, which is what it is supposed to do. The write path was driven in a real browser — rename, publish and delete through the actual form and dialog — because a green suite over a write that 419s in every browser is a mistake this repository has made before. Bytes gone, audit trail complete, and file.made_public records the slug. |
||
|
ignacionelson
|
ea214fc27e |
Give the client portal a file editor
The authorization landed last commit; this is the way in. A client with edit_files now gets an Edit action on the files they uploaded, opening a form with every field their role actually grants, and a Delete beside it. One page for every theme, not one per theme. portal/edit-file.tsx picks its shell from the `theme` prop exactly as portal/upload.tsx does, because a form with eight fields behind five separate permissions, rebuilt four times, is four places for a field to go quietly missing. What *is* per-theme is only the entry point: one <FileRowActions /> in each theme's row actions group, the file twin of the FolderRowActions that was already there. Row actions gate on can_update/can_delete, sent per file by MyFilesController and answered by FilePolicy — never on is_mine, which is half the question. Holding the file is one half and the role's keys are the other, and a theme that reads is_mine offers an Edit button that 403s. Written into docs/theming-files-checklist.md so the next theme does not have to rediscover it. The folder picker offers only folders the client could have uploaded to, so it cannot present a destination the save would refuse. Publishing says in plain words that anyone with the link will be able to open the file without signing in, and says so differently when the installation has no public page configured, because there the switch would do nothing visible. Hiding a control is a courtesy, never the enforcement. Every can_* prop here is the same question ApplyFileEdits asks when the form posts, and the tests assert both ends. Verified in a real browser over CDP rather than only by types and tests, which say nothing about whether a page mounts: 23 edit actions on the client's 23 own files and none on the file shared with them, the editor mounting with its real values, every gated field present, no console errors. The dev instance's Client role was snapshotted before the run and restored to exactly what it was. Refs #1771 |
||
|
ignacionelson
|
922be7226c |
Let a client edit and delete the files they uploaded
A client could upload a file and then never touch it again. No rename, no description, no expiry, no categories, no delete — the portal has three file routes and all three are GET. Meanwhile the Roles screen happily grants the Client role edit_files, delete_files, set_file_categories, set_file_expiration_date and upload_public, and every one of them was inert, because the routes that honour them are `staff`-gated rather than permission-gated. That is what #1771 hit: a permission granted, saved, and silently doing nothing. A client owns what they uploaded. Ownership is now what lets them edit and delete it, subject to the same per-field keys staff are subject to. The obvious implementation is a trap, and it is worth writing down. Both policy methods began `if (! $user->isStaff()) return false;` and both end in StaffLibraryScope, whose allowsFile() reads `if (! isClientScoped()) return true` — and isClientScoped() is `isStaff() && role->client_scoped`, so it is false for every client. Delete the early return and a client falls into the branch meaning "this staff member is unrestricted" and is handed the whole library. Same for folders(), which returns an unfiltered query: a client could move their file into any folder on the installation. So clients get their own branch, reaching neither. The portal asks Folder::uploadableBy() instead — a file cannot be moved somewhere it could not have been uploaded. edit_others_files and delete_others_files stay inert for clients by construction. A client has no others' files, only files somebody showed them, and being shown a file is not being given it. Which fields an editor may write moved into ApplyFileEdits, shared by the staff editor, /api/v1 and the portal. There were two copies of the same eight permission checks and this would have been the third; the checks are easy, which is exactly why the drift would have been invisible. Callers normalise their own request shape, this gates and writes and logs. Expiry reading and writing came along too, as FileExpiry — three copies, of which only the API's could read a timestamp. Clients do not choose the public slug. It is derived from the name they already picked, because an installation-wide unique slug a client sets is a name to squat and an existence oracle to probe with. One consequence for later, written up in docs/api-todo.md: the policy now says yes to a client for file writes, so `staff-token` is the only thing holding the API boundary where there used to be two independent refusals. ActorBoundaryTest pins it, and asserts the policy passes first so the test cannot quietly stop testing the middleware. Also corrects a stale comment that claimed a deleted file's bytes stay on disk. They have not since File::booted() grew a `deleted` hook; nothing ever forceDelete()s a File row, so "until a purge lands" would have meant never — which is why a client's delete frees their quota by exactly what it frees on disk. The UI comes next; this is the authorization, the routes and the tests. Fixes #1771 |
||
|
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.
|
||
|
ignacionelson
|
97596da7d0 |
Refuse a stored path carrying a control character
Found reviewing the delivery work. The path is written into
X-Accel-Redirect or X-Sendfile, and a CR or LF in a header value is
header injection. PHP's header() refuses to emit one, so the real effect
is a 500 on every download, preview and thumbnail of that file rather
than a split response -- a file permanently broken by its own name.
Paths are generated here as Y/m/{uuid}.{ext}, so this should be
unreachable. The extension is not generated: it comes from the
uploader's filename, and on a migrated installation from a v1 database.
The upload routes all check the extension against an allowlist, which no
control character can match -- but upload_type_restriction can be set to
none, and the importer does not consult that policy at all.
assertRelative() was documented as the backstop for what a path may be
and only covered traversal, which is the half that cannot happen here.
Low severity, and the guard should have covered it either way.
|
||
|
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. |
||
|
ignacionelson
|
144f5fc578 |
Merge pull request #1747 from denkfabrik-li/fix/bulk-edit-skip-reason
Two different things stop a selected file being changed in a bulk edit, and bulkUpdate() reported both as the first one. Files dropped by the Gate::allows('update') filter are ones this staff member may not edit at all. A file that survives the filter and still changes nothing is a different case: it was editable, and every field they asked to change is one their role does not let them set -- expiry, download limit and categories each sit behind their own permission here, exactly as they do in the single-file editor. So a staff member with edit_files but without set_file_expiration_date, editing three files they own, was told "0 of 3 selected files were updated. The rest were skipped because you don't have permission to edit them." They own all three, and editing is precisely what they may do: the sentence was both wrong and unactionable, since nothing in it points at the permission that actually stopped the edit.
The two cases get their own sentences now. Every skip being a file they may not edit keeps the existing string, unchanged, so its sixteen translations stay in use. Anything else gets a new one, "because you don't have permission to make those changes", which is also true when both reasons are in play, so a mixed selection is described correctly rather than approximately. Which files get changed is untouched, as is the silent-skip convention and the 422 when nothing at all is authorised.
Verified before merging: 14 passed on the trial-merge, 2 failed / 12 passed with app/ reset -- the field-permission case and the mixture. The pure edit-permission case is green either way, which is what says the existing message was not disturbed. FilesController overlaps #1728, already merged, and its expiryDateFor work is intact in the merged tree.
The new string arrived English-only; the sixteen catalogs are filled in the commit that follows.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
383c3b2ff5 |
Merge pull request #1746 from denkfabrik-li/fix/expired-file-staff-access-comment
File::isExpired() documented the rule the whole application is supposed to follow: once past, the file is hidden from clients and the public site but staff keep full access. The second half is not true of a client-scoped staff member. StaffLibraryScope::buildFiles() builds their library as own uploads plus what each assigned client may see, and that second half runs through File::scopeVisibleToClient, which ends in notExpired() -- a client-side rule. So an expired file they held only through a client leaves their library and answers 403 on download, while their own expired upload stays and an unscoped administrator is unaffected. Api\FilesController stated it the same way, "Only the client branch of the visibility rules drops them", which reads as though a staff caller is unaffected when a client-scoped one is reached through that very branch.
This does not change that behaviour.
|
||
|
ignacionelson
|
d91cf97bcb |
Merge pull request #1745 from denkfabrik-li/fix/moderation-view-read-permission
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.
|
||
|
ignacionelson
|
89b3d34c8f |
Merge pull request #1744 from denkfabrik-li/fix/version-link-duplicate-share-notice
FileVersions::link() resolves its audience before the merge, and its own comment says the ordering is the whole dedupe: these are the people who could already see both files, so anyone the merge is about to reach for the first time is excluded and gets file_shared from FileSharing::assign() instead. The merge then undid it. moveAssignmentsToRoot() handed every one of the revision's targets to assign() under the comment "firstOrCreate inside, so a target the root already has is a no-op rather than a duplicate notification" -- but firstOrCreate makes the assignment row idempotent, not the three side effects below it. The activity entry, the in-app notification and the digest all ran unconditionally, so a client who already held both files was told a file had been shared with them about a file they had had all along, on top of the file_new_version they were owed. Two notifications for one action, for exactly the people the early resolve exists to protect. A target the root already holds is now skipped rather than handed to assign(). Nobody is gaining access in that case, so the activity entry would have been as untrue as the notification -- which is the rule copyAssignmentsFrom() states outright for its own case, and why it inserts directly instead of going through FileSharing. The two stale comments are corrected with it. Deliberately not changed: assign() itself, and so the behaviour ShareNotificationsTest pins, where re-posting an existing assignment through the share endpoint still notifies again. That test says the condition for changing it -- it should stop for files and folders at once, which is the point of them sharing one implementation -- and a version merge is not somebody choosing to share again. Verified before merging: 10 passed on the trial-merge, 2 failed / 8 passed with app/ reset, and the whole tests/Feature/Files directory at 548 passed. The case where somebody genuinely gains the root still gets file_shared is green either way, which guards against skipping too much. The method was read whole rather than just the hunk: $file->assignments()->delete() still runs for a skipped target, so no row is left dangling and nobody loses reach. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
c11cb3cc63 |
Merge pull request #1734 from denkfabrik-li/fix/quota-message-inherited-default
ClientStorageUsage::quotaMb() exists because a client's own storage_quota_mb of 0 does not mean "unlimited" -- it means "no quota of their own", and the site default is what is then enforced. Both chunked-upload quota checks enforced the resolved limit through quotaBytes() and then printed the raw column in the rejection, so a client with no quota of their own and a site default of 1 MB was told "This upload would exceed your storage quota of 0 MB." That is every client who was never given a quota, including every self-registered one, and the sentence appears at the one moment somebody is trying to find out what their limit is. Both now print quotaMb(), which is what the check enforced. The API's single-request upload already did exactly this for the same sentence, so the three copies agree. The enforcement itself is untouched -- only the number in the message changes -- and the unlimited case never reaches these branches, because quotaBytes() > 0 guards them. Verified before merging: 16 passed on the trial-merge, 2 failed / 14 passed with app/ reset. The "a client with a quota of their own still sees their own number" test is green either way. The string itself is unchanged, so no locale file needs anything. Reported and fixed by @denkfabrik-li. |
||
|
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. |
||
|
ignacionelson
|
b6f4770795 |
Merge pull request #1731 from denkfabrik-li/fix/zip-build-failure-hygiene
BuildZipDownloadJob already draws this line in its write-failure branch: "What the requester sees stays generic: a libzip string means nothing to them and can name a server path. An operator needs the opposite, so the reason goes to the log instead." Thirty-seven lines below it, the catch-all around the whole build stored $e->getMessage() in the row the requester polls -- and ZipDownloadsController hands that column straight back to whoever asked, clients included. A client asking for an archive of a file whose disk is no longer configured read "Disk [a-disk-that-is-not-configured] does not have a configured driver." verbatim. The reason now goes to the log with the exception class, and the row carries the same kind of sentence fail() already uses. Two more in the same method. tempnam() creates the file, and $tempFiles[] was appended only after the copy finished, so every throw in between left a zip-src- file in the system temp directory that nothing ever removed; it is now registered the moment it exists. And the copy itself was unchecked -- a copy that stops early is a truncated member added to the archive as though it were the file, so the build reports ready and the recipient gets something that opens and is wrong. stream_copy_to_stream and the flushing fclose are both checked now, and both handles close on every path. Deliberately not changed: comparing the copied byte count against files.size, which would fail perfectly good archives whenever that column is stale; the write-failure branch and its wording; and the skipped-files reporting, which still says which files and why, so only the catch-all went generic. Verified before merging: 37 passed on the trial-merge, 2 failed / 35 passed with app/ reset. The leak was confirmed at the consuming end rather than inferred -- ZipDownloadsController:169 returns the error column to the requester. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
f676e09bb2 |
Merge pull request #1728 from denkfabrik-li/fix/expiry-timezone-drift
The edit screen is given a file's expiry as a calendar date read back in the viewer's own zone -- deliberately, or "a file set to expire on the 12th reopens showing the 11th". Every save posts that date back, touched or not, and update() derived a fresh instant from it every time. So the expiry drifted by the difference between two people's zones on any other edit: a file set from Pacific/Auckland moved 19 hours later the moment somebody in Buenos Aires renamed it, and moved again on the next save from a third zone. A file could quietly outlive the expiry somebody set for it, through an edit that had nothing to do with expiry. The instant is now re-derived only when the posted date differs from the one the form was given, compared against the same string through a named pair: expiryDateFor() renders it, expiryInstant() reads it back, and the edit screen calls the render half so the two cannot drift apart. What a changed date means is unchanged -- still the end of that day in the zone of whoever changed it. bulkUpdate() needs nothing: its expiry is an explicit set / clear / no_change action, so an untouched expiry is never posted at all. Verified before merging: 22 passed on the trial-merge, 1 failed / 21 passed with app/ reset. The "a real change still lands in the editor's zone" and "clearing still clears" tests are green either way. Edge cases walked: a posted date against no stored expiry still sets it, and a posted null against a stored null leaves the column alone rather than writing. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
eb3d6e321d |
Merge pull request #1727 from denkfabrik-li/fix/api-expiry-end-of-day
FilesController::expiryInstant() exists because a calendar day ends where the person naming it lives: the web form posts a bare YYYY-MM-DD, which Eloquent would otherwise store as midnight UTC, so "expires on the 12th" would cut the file off partway through the 11th for anyone in the Americas. PATCH /api/v1/files/{id} took the same field, validated it as a date, and stored it exactly as it arrived -- so the same value that meant end-of-the-12th on the web meant start-of-the-12th over the API, and earlier still for a caller west of Greenwich.
A bare YYYY-MM-DD now means the end of that day in the caller's timezone, through the same LocalDay::end() the web path uses. A value carrying a time is unchanged: that is an instant the caller named on purpose, the API can express one where a date input cannot, and it is stored as it arrives. null still clears the expiry, and the validation rule and permission gate are untouched.
Note for the release notes: this lengthens the life of a file whose expiry an existing integration sets with a bare date, by up to a day. That is the correct meaning and the one the web has always had, but it is a behaviour change for callers who were relying on the old one.
Verified before merging: 19 passed on the trial-merge, 1 failed / 18 passed with app/ reset. The timestamp and clearing tests are green either way. The bare-date branch is gated on a strict ^\d{4}-\d{2}-\d{2}$ match, so nothing else takes it. scramble:export on the merged tree reproduces the committed docs/api/openapi.json byte for byte.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
d89807b237 |
Merge pull request #1726 from denkfabrik-li/fix/rendition-cleanup-independent
FileDiskCleanup::delete() wrapped two deletions in one try: the original upload, on whatever disk the row names, and every cached rendition, which is always on the local files disk. Storage::disk() throws outright for a name with no configured driver -- precisely the state the original's disk is in whenever this fails at all -- so the catch swallowed it and the renditions were never reached. Nothing looks for them afterwards: OrphanFileScanner skips the rendition directories on purpose, as derived artifacts rather than orphaned uploads. A file whose external disk had been removed or renamed therefore kept every cached copy of itself indefinitely on the disk that still worked, including the client-facing ones, which for a shared image may be the only copies anyone ever generated. The two attempts are now separate, each with the tolerance the class was written for: a storage failure still never turns a delete click into a 500, and the warning is still the whole report. Also corrected: File::booted() justified deferring the byte removal with "the worst case is bytes left on disk with no row, which OrphanFileScanner already finds and reports". That is not this path -- the row is soft-deleted, and knownPaths() counts a trashed row's path as claimed, deliberately, so a scan never offers to double-adopt a file still inside its erasure grace period. The comment now says what actually happens, which is that FileDiskCleanup's warning is the only record. Verified before merging: 8 passed on the trial-merge, 1 failed / 7 passed with app/ reset. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
7ff2674e4f |
Merge pull request #1725 from denkfabrik-li/fix/rendition-written-atomically
Both thumbnail routes treat "the file exists" as "the rendition is cached", and nothing ever invalidates one: RenderedImageCache::flush() runs on ImageRenderingChanged, which no code in core raises. Whatever sits at the path is what every later viewer gets. ThumbnailGenerator::generate() encoded straight onto that path, so a render that died partway -- a full volume, a killed worker -- left a half-written file that was then served as the rendition indefinitely, and two requests rendering the same file at once encoded into the same path together. Write side: the image is written beside its destination and renamed into place. rename() within a directory is atomic and replaces what is there, so the path holds 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. Renditions always cache on the local files disk and the generator is handed $disk->path(), so both files are on the same filesystem and the atomicity is real. Read side: an empty file is not a rendition, so both routes replace one rather than serve it -- writing through a temporary file means core can no longer create that state, but an installation that ran an older version can already have it on disk and nothing else will ever clear it. The cache itself is unchanged: a non-empty rendition is still reused without further checks, because decoding every cached image on every request to prove it is intact would cost the cache its point. The RenderingImage seam still fires before the encode. Verified before merging: 14 passed on the trial-merge, 2 failed / 12 passed with app/ reset. The third test, about the generator's own temporary file, passes either way and the PR says so rather than leaving it to be found. Reported and fixed by @denkfabrik-li. |
||
|
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
|
5e60d2ef88 |
Say what expiry does to a client-scoped staff member's library
File::isExpired() documents the rule the application is supposed to
follow: once past, the file is hidden from clients and the public site
"but staff keep full access to view, download, and manage it".
The second half is not true of a client-scoped staff member.
StaffLibraryScope::buildFiles() builds their library as their own uploads
union what each assigned client may see, and that second half runs
through File::scopeVisibleToClient, which ends in notExpired() -- a
client-side rule. Measured on main, with a rep holding one client and a
file the administrator uploaded and shared with that client:
before expiry in_library true GET .../download -> 200
after expiry in_library false GET .../download -> 403
the rep's own expired upload in_library true
an unscoped administrator, same expired file in_library true
Api\FilesController says it the same way -- "Only the client branch of
the visibility rules drops them" -- which reads as though a staff caller
is unaffected, when a client-scoped one is reached through that very
branch.
This does not change that behaviour.
|
||
|
denkfabrik-li
|
21cae2acb1 |
Stop a version link telling people about a file they already had
FileVersions::link() resolves its notification audience before the merge,
and says why:
RESOLVED BEFORE THE MERGE, and the ordering is the whole dedupe:
these are the people who could already see both files, so anyone the
merge below is about to reach for the first time is excluded here and
gets file_shared from FileSharing::assign() instead. Resolve it
afterwards and every newly-added client receives two emails about one
action.
The merge then undoes it. moveAssignmentsToRoot() hands every one of the
revision's targets to FileSharing::assign(), under a comment claiming
that firstOrCreate makes a target the root already has a no-op. It makes
the assignment row idempotent; the three side effects under it --
activity entry, in-app notification, digest -- run unconditionally.
Measured on main:
client already holds the root and the revision, then both are linked
file_shared (Report) <- wrong, they have had it all along
file_new_version (Report v2) <- right
assignment rows on the root: 1
client holds only the revision, then both are linked
file_shared (Report) <- right, the merge does hand it over
Two notifications for one action, for exactly the people the early
resolve was meant to protect.
So a target the root already holds is skipped rather than handed to
assign(). Nobody is gaining access in that case, and the activity entry
would be as untrue as the notification. copyAssignmentsFrom() directly
below already states that rule for its own case, which is why it inserts
directly instead of going through FileSharing. Both stale comments are
corrected with it.
Not changed: FileSharing::assign() itself, and so the behaviour
ShareNotificationsTest pins -- re-posting an existing assignment through
the share endpoint still notifies again. That test names the condition
for ever changing it, "it should stop being sent for both at once", and
that is a decision about files and folders together. This is narrower: a
version merge is not somebody choosing to share again, and it already
had a stated intent to send exactly one notification.
Three cases in ShareNotificationsTest -- the target already on the root,
the target gaining it, and a group already on the root. Reverting
FileVersions alone leaves 2 failed / 8 passed in that file; the middle
case passes without the fix, because it guards against skipping too much
rather than against the duplicate notice.
Suite 2108 passed / 2 skipped, 11415 assertions, PHPStan level 8 clean.
Measured on base
|
||
|
denkfabrik-li
|
5a9133bb07 |
Give a download's presigned URL a minute rather than an hour
StoredFileResponse hands external storage a presigned URL for an hour, whatever the delivery is for. That URL is a bearer credential: whoever holds it fetches the file without passing any of the caller's checks again, and it outlives them. A download cap spent in the meantime, an expires_at that falls inside the hour, an assignment withdrawn -- none of them reach it, and nothing here can revoke one. It is also forwardable, which the local path is not: X-Accel-Redirect authorises one response to one request. The two deliveries do not need the same window, so they no longer share one. A download has to survive being followed -- a redirect and a request -- which a minute covers with room to spare. An object store checks the signature when the request arrives rather than while it runs, so a transfer that starts inside the window finishes however long it takes. A preview keeps the hour, because it is watched rather than fetched: the player holds the URL and issues a Range request every time somebody seeks past the buffer, so a minute would break playback of anything longer than a minute. The class docblock now says that this is the trade being made, instead of leaving it in a single number. Two tests, one per window. Without the fix the download link is an hour long. |
||
|
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
|
4b998cda92 |
Fail a zip build without handing the requester the server's reason
The write-failure branch already draws the line and says why: "What the requester sees stays generic: a libzip string means nothing to them and can name a server path. An operator needs the opposite ... so the reason goes to the log instead." Thirty-seven lines below it, the catch-all around the whole build stored $e->getMessage() in the row the requester polls. Measured, a client asking for an archive of a file on a disk that is no longer configured was told: "Disk [a-disk-that-is-not-configured] does not have a configured driver." The reason now goes to the log with the exception class, and the row carries the same kind of sentence fail() already uses. Second, the temp files. tempnam() creates the file, and $tempFiles[] was appended only after the copy had finished -- so every throw in between (a disk that will not resolve, a stream that will not open) left a zip-src- file in the system temp directory that nothing ever removes. It is now registered the moment it exists. Third, in the same method: the copy itself was unchecked. A copy that stops early is a truncated member added to the archive as though it were the file, so the build reports ready and the recipient gets something that opens and is wrong. Both the copy and the fclose that flushes it are checked now, and both handles close on every path. Two tests: the failure message names nothing about the server, and a build that throws mid-copy leaves no temp file behind. Both go red without the fix. |
||
|
denkfabrik-li
|
4164678ebc |
Delete a file's renditions even when its own disk cannot be resolved
FileDiskCleanup wraps both deletions in one try. The first is the original upload, on whatever disk the row names; the second is every cached rendition, always on the local files disk. Storage::disk() throws outright for a name with no configured driver -- which is the state the original's disk is in whenever this fails at all -- so the catch swallowed it and the renditions were never reached. Nothing looks for them afterwards. OrphanFileScanner skips the rendition directories on purpose (they are derived artifacts, never orphaned uploads), so a file whose external disk had been removed or renamed kept every cached copy of itself, indefinitely, on the disk that was working. The two attempts are now separate, each with the same tolerance the class was written for: a storage failure still never turns a delete click into a 500, and the warning is still the report. While here, the comment in File::booted() that justifies deferring the byte removal claimed "the worst case is bytes left on disk with no row, which OrphanFileScanner already finds and reports". Not on this path: the row is soft-deleted, and knownPaths() counts a trashed row's path as claimed -- deliberately, so a scan never offers to double-adopt a file still inside its erasure grace period. The comment now says what actually happens. One test: a file whose disk cannot be resolved loses its renditions. It goes red without the fix, next to the existing test that the delete itself still succeeds. |
||
|
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
|
f2b705beee |
Keep an upload's parts until its bytes are stored
complete() holds a lock whose comment promises "the lock's TTL releases the claim if a completion dies mid-flight, so a later retry still works". A retry has nothing to work from but the parts, and assemble() unlinked each one inside the loop that read it -- so everything that can fail afterwards took the retry with it. Measured on main, with a disk refusing the write (the case the guard forty lines further down was written for, found against a real GCS bucket): first complete → 422, 0 parts left, the half-written copy left behind retry → 422 "Upload is incomplete: missing parts." For good: listParts() is empty, so no later attempt can ever succeed, and the client has to send the whole file again. The abandoned copy sat in the session directory until the sweeper came round. The parts now go when abort() clears the session directory -- which already ran on success -- and a failure deletes only the half-written copy it made. The cost is temp space: peak usage during assembly is the whole file twice over rather than the file plus one part. The docblock says so. Also checked while here: every read and every write in the concatenation. A failing fwrite is loud in practice, since Laravel's error handler turns the warning into an ErrorException, but loud there is a 500 carrying a PHP message where this method's other storage failure is a sentence the person uploading can act on. A short write arriving without a warning would be worse: the byte count and the checksum describe the buffer that was read, so an unchecked one records a truncated file with a checksum matching bytes that were never stored. Two tests: the retry after a refused write now succeeds, and a temporary directory that refuses writes (/dev/full, skipped where it does not exist) fails the upload with this method's own message. Without the fix both go red. |
||
|
denkfabrik-li
|
640c5db591 |
Stop an expiry moving because somebody else saved the file
The edit form is given a file's expiry as a calendar date, read back in the viewer's own zone -- deliberately, so a file set to expire on the 12th does not reopen showing the 11th. Every save posts that date back, whether or not anybody touched it, and update() derived a fresh instant from it every time. So the expiry drifts by the difference between two people's zones on any other edit. A date set from Pacific/Auckland stores 2026-09-12T11:59:59Z; a colleague in UTC-3 opens the file, sees the same 12th, renames it, and the file now expires at 2026-09-13T06:59:59Z -- 19 hours later, with nobody having gone near the date. The instant is now re-derived only when the posted date differs from the one the form was given, compared against the same string through a named pair: expiryDateFor() renders it, expiryInstant() reads it back. The edit screen uses the same method it is compared against, so the two cannot drift apart. bulkUpdate() needs nothing: its expiry is an explicit set/clear/no_change action, so an untouched expiry is never posted in the first place. Three tests: the rename leaves the instant alone, a real change still lands in the editor's own zone, and clearing still clears. Without the fix the first goes red. |
||
|
denkfabrik-li
|
e1cd010f9d |
Give an API expiry date the same meaning the web gives it
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. |
||
|
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
|
763777d282 |
Say which permission a bulk edit was actually missing
Two different things stop a selected file being changed, and bulkUpdate()
reported both as the first one.
Files dropped by the Gate::allows('update') filter are ones the staff
member may not edit at all. A file that survives the filter and still
changes nothing is a different case: it was editable, and every field they
asked to change was one their role does not let them set -- expiry,
download limit, categories, each behind its own permission, exactly as the
single-file editor treats them.
Measured with edit_files but without set_file_expiration_date, three files
they own, expiry the only change: "0 of 3 selected files were updated. The
rest were skipped because you don't have permission to edit them." They
own all three and editing is precisely what they may do, so the sentence is
both wrong and unactionable.
The two cases now have their own sentences. The existing string is kept
for the case it describes -- every skip a file they may not edit -- so its
sixteen translations stay in use. The new one covers a field permission,
and covers a mixture of both reasons, since "permission to make those
changes" is true either way.
The new key is English only; a locale without it falls back to English,
which is a translated-but-wrong sentence traded for an untranslated
correct one.
Three tests: each reason on its own, and the mixture. Without the fix the
first and third go red.
|
||
|
denkfabrik-li
|
cd8da6a117 |
Name the quota a client is actually held to when an upload is refused
Both chunked-upload quota checks resolve the limit through ClientStorageUsage::quotaBytes(), which falls back to the site default when a client has no quota of their own -- and then print `$user->storage_quota_mb` in the rejection. For every client who was never given an explicit quota that column is 0, so the message reads "This upload would exceed your storage quota of 0 MB." at the one moment somebody is trying to find out what their limit is. The API's single-request upload already prints `$this->storageUsage->quotaMb($user)` for the same sentence (Api/FilesController.php:208). The two chunked copies now do the same. Three tests: the inherited default is named at session creation and again at completion, and a client with a quota of their own still sees their own number. Without the fix the first two go red, the third stays green. |
||
|
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
|
c8de16101f |
Gate the comment moderation surfaces on reading, not just on the library
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.
|
||
|
Ignacio Nelson
|
046be36861 |
Merge pull request #1710 from denkfabrik-li/fix/folder-delete-file-authority
FoldersController::destroy() authorized delete on the folder and nothing else, while FolderService::delete() soft-deletes every file in the subtree and File's deleted hook takes the bytes off disk. So a staff member refused a file one route over could destroy it by deleting the folder around it -- permission and library boundary both unasked. MyFoldersController::destroy() already draws this line for the client half of the same cascade, and says why: owning the folder is not authority over content someone else put in it. This is the staff half of that sentence. Verified before merging: the four bug tests fail on main and pass here, and the SQL predicate was read line by line against FilePolicy::delete -- it is a faithful negation, including the null-uploader case and the short-circuit for an unscoped viewer holding both delete permissions. Membership of the check is one COUNT, not a policy call per file. Suite at 2099, PHPStan clean. Behaviour change, deliberately accepted: a folder delete that used to succeed now refuses, naming how many files are in the way. The likely case is somebody who owns a folder another account uploaded into. The alternative is irreversible loss of files the same person is refused individually. Not taken: deleting what the actor may and keeping the rest. Half a tree is worse than either answer. Naming the blocking files would be friendlier than counting them and is worth doing later -- the list has to hide any file the viewer cannot see, which is its own small design question. Reported and fixed by @denkfabrik-li. |
||
|
denkfabrik-li
|
b44c6bf098 |
Add a file to a zip once, however many ways the selection reaches it
BuildZipDownloadJob walks the loose file ids and then every selected
folder's subtree, and adds whatever each pass finds. A selection can
reach the same file from more than one of them, and nothing noticed:
file_ids [f], folder_ids [Reports]
-> ['report.pdf', 'Reports/report.pdf']
file_ids [f], folder_ids [Reports, Reports/Q1]
-> three entries, file_count 3, total_size three times the file
Two copies of the same bytes in one archive, and total_size is what the
size cap is checked against, so a selection could also be refused for a
weight it does not have.
The one that costs more than bandwidth is delivery. It logs one
FileDownloaded per contained file, and DownloadAllowance counts those
records -- so a file limited to a single download left in three copies
while the log recorded one. Measured: three entries, one record.
Two causes, so two halves.
`$added` is now keyed by id instead of being appended to a list, and the
folder pass skips a file already in the archive. A lookup rather than a
scan because the selection cap is 10000 sources. The loose pass runs
first, so a file picked both ways sits under its loose name; either
answer is defensible, but it has to be the same one every run.
And a selected folder inside another selected folder is dropped before
either is walked. Zipping both would reach every file in the inner one
twice, and which path the surviving entry ended up under would be decided
by the order the rows came back in. Keeping the outer folder keeps the
fuller path -- Reports/Q1/report.pdf rather than Q1/report.pdf.
Containment is decided on the materialized path, so it is one comparison
per pair with no queries: a folder's path starts with an ancestor's
subtreePathPrefix(), and both end in '/', so /5/ cannot match /50/.
Not changed: the per-file re-checks inside the folder pass. Visibility
and the download allowance are still re-derived per file, and the skip
happens before them, so a duplicate never spends an allowance twice
either. Nor the selection endpoint -- a caller may send whatever
selection they like, and the job is where it is resolved.
Four tests. Three measured red against the unfixed job (3 failed / 32
passed): the loose-plus-folder case, the nested-folder case, and the
three-way case asserted through delivery rather than through the archive.
The fourth -- two selected folders that merely share a name are both
zipped -- is green either way and guards the pruning against being about
names rather than containment.
Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
|
||
|
denkfabrik-li
|
26205082c2 |
Stop a folder deleting the files inside it that its owner may not delete
FoldersController::destroy() authorizes `delete` on the folder and nothing
else. FolderService::delete() then soft-deletes every file in the subtree,
and File::booted()'s `deleted` hook takes the bytes off disk. There is no
restore.
FilePolicy::delete asks two questions the folder route never reaches:
`delete_others_files` for somebody else's upload, and
StaffLibraryScope::allowsFile on top of it. Measured with a role holding
create_own_folders, delete_files, upload and edit_files -- the shape the
Client Manager system role already has, minus delete_others_files:
DELETE /files/{someone-elses} 403, the file is still there
DELETE /folders/{their-folder} 302, the file and its bytes are gone
MyFoldersController::destroy already refuses the client half of this exact
cascade, and says why: "Owning the folder is not authority over content
someone else put in it... Refuse rather than silently destroy them." This
is the staff half of the same sentence.
Counted rather than asked per file. A folder can hold thousands, Gate
resolves a fresh policy for every check, and a per-row policy check on a
listing is the cost
|