mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-17 00:55:07 +00:00
main
54 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ignacio Nelson
|
7944eccaf1 |
Merge pull request #1782 from projectsend/client-expiry-and-start-page
Client accounts that expire, a start page per role and person, and five more file filters |
||
|
ignacionelson
|
5f414c7a4a |
Give the API the same file filters the library screen has
The staff library grew filters for uploader, role, public/private, download count and version. Two of those already existed on /api/v1/files (`uploaded_by`, `public`); the other four did not, so an integration could not ask what the screen asks. Adds `role_id`, `downloads=none|any`, `version=current|outdated` and `visibility=public|private`. `visibility` rather than changing `public`, and that is the decision worth explaining. `public` has always tested the file's own column, and callers depend on that answer; changing what an existing filter means is breaking for everyone already sending it, however much better the new meaning is. So `public` is untouched and `visibility` is added beside it with the application's own definition -- File::isEffectivelyPublic(), the flag or a public folder anywhere above the file -- which is what the badge on a staff row means. The guide says in a sentence which to reach for. Point the visibility filter at the column instead and the test that separates them fails, which is the whole point of having both. That predicate now lives once, as File::scopeEffectivelyPublic(), beside the isEffectivelyPublic() it has to agree with. It was a private helper on FoldersController until a second surface wanted it. `role_id` deliberately carries no identity guard, unlike `uploaded_by` beside it. A role names nobody: the files in the result are ones the caller may already read, and learning one came from somebody holding the Client role narrows to a set they could have guessed. `uploaded_by` is different in kind -- a non-empty answer confirms exactly the identity the response is redacting -- which is why only it is guarded. The reasoning is in the code, because an absent guard sitting next to a present one is the kind of thing a reader should not have to re-derive. Tests cover each filter, the public/visibility split, and the client-scoped negative: every new filter still returns nothing outside the token's own library, because a filter narrows a library and never widens one. |
||
|
ignacionelson
|
5966d22f50 |
Add five ways to narrow the file library
Search and the category dropdown were the whole filter bar, which is thin for a library of any size. It now also narrows by: - who uploaded the file, and separately by the role they hold - public or private - never downloaded, or downloaded at least once - current version, or outdated Each one forces the same flat, whole-library view search already used, and they combine. Two decisions worth naming. "Public" means what the badge on the row means -- File::isEffectivelyPublic(), the file's own flag or a public folder anywhere above it. Filtering on the `public` column alone would have hidden files this very screen labels Public, which is a filter arguing with the list it filters. The private half needs its own null branch, because `folder_id NOT IN (...)` is never true for a NULL folder_id: without it a file at the library root belonged to neither half and vanished from both. Removing that branch turns the private filter from one row to zero, which is the test. The uploader filter carries the same guard /api/v1/files puts on `uploaded_by`. fileRow() already withholds an uploader's name from a viewer who may not identify them, so answering this filter plainly would have handed the same identity straight back as a row count. An id the caller may not identify now matches nothing, which is indistinguishable from someone who uploaded nothing, and the dropdown is built through filterClientPairs so it never offers the name either. Without the guard the scoped-staff test gets its stranger's file back. "Outdated" rather than "superseded" throughout, because that is the word the version badge already uses and the two should not disagree. A file nothing has replaced counts as current, including one never versioned at all. The ids are cast out of the validated input: `integer` validates "5" without converting it, and permitsClientId() takes a strict ?int. |
||
|
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
|
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
|
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
|
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
|
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
|
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.
|
||
|
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
|
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
|
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
|
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
|
||
|
Ignacio Nelson
|
f12692520a |
Merge pull request #1694 from denkfabrik-li/fix/upload-folder-library-scope
Hold the folder an upload names to the same library boundary as everything else |
||
|
Ignacio Nelson
|
e4cd56f5d6 |
Merge pull request #1692 from denkfabrik-li/fix/zip-download-limit-at-delivery
Enforce the download limit when a zip is delivered |
||
|
Ignacio Nelson
|
09efad2d8c |
Merge pull request #1690 from denkfabrik-li/fix/client-portal-subfolder-names
Don't name a subfolder to a client who cannot open it |
||
|
ignacionelson
|
835943e1b6 |
Merge pull request #1686 from denkfabrik-li/fix/chunked-upload-complete-lock
Finalise each chunked upload once, under a per-session lock
Resolved a trivial conflict in ChunkedUploadsTest: this branch and
|
||
|
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 |
||
|
Ignacio Nelson
|
d36abd73ba |
Merge pull request #1682 from denkfabrik-li/fix/chunked-upload-max-size
Enforce the max file size against the bytes a chunked upload assembles |
||
|
Ignacio Nelson
|
e815ac8be5 |
Merge pull request #1681 from denkfabrik-li/fix/file-update-folder-scope
Scope a file's destination folder on update(), as move() already does |
||
|
ignacionelson
|
d7e639b7af |
Close the two-request version of the deleted-folder target, and say why it failed
Follow-up to #1703, which made `exists:folders,id` mean what its ten readers already assumed. Two things it named and deliberately left. **The chunked upload is two requests.** store()'s rule only ever sees the first: POST /uploads records the resolved folder on the UploadSession and complete() reads it back from the session rather than from the caller, so deleting the folder while the bytes are in flight still files the assembled file into it -- the same orphan state #1703 removes, reached by a door a validation rule cannot watch. complete() now re-resolves through Folder::query() and files at the root when the folder has gone. Root rather than a refusal, because the two moments cost different things. At store() nothing has been sent, so refusing is free and honest, which is the call #1703 made. Here the bytes are already uploaded, and discarding somebody's finished transfer over a folder that vanished underneath them is the harsher of the two surprises. The file lands somewhere they can see it and move it. **The refusal now explains itself.** "The selected folder id is invalid" says nothing when the answer is that the folder has been deleted -- and that is the usual way to meet this rule, since a live id picked from a list is how anybody gets here. It matters most on the chunked path, the one place #1703 makes a previously-working request fail. A small ValidationRule object carries the message, which keeps the single definition Rules::folderId() exists for: a messages() array would have to be repeated at all ten call sites, and rules meaning different things in ten places is what went wrong in the first place. One note for whoever writes the next test here. Upload parts live in storage_path('app/uploads-tmp/{session_id}'), which is a real shared directory rather than a faked disk, and each parallel worker's database restarts session ids at 1 -- so two files writing parts on two workers collide, and ChunkedUploadsTest's afterEach deletes the whole tree for everybody. Six test files write parts today. These two cases live in ChunkedUploadsTest rather than beside the rest of their subject so this change does not add a seventh racer; the underlying isolation problem predates it and is worth its own fix. |
||
|
denkfabrik-li
|
7727ad7616 |
Say what exists:folders,id was already being read as
Folder uses SoftDeletes. The `exists` rule runs against the table, so a folder in the trash passes it -- while every resolution that follows goes through Folder::query(), which honours the soft delete and finds nothing. Ten rules across five controllers rely on that check, and each one reads it as "this folder exists". Two of them then wrote the id anyway. Api\FilesController::store() resolves the folder, hands the null to Folder::uploadableBy(), is told yes -- correctly, that is the rule for a root upload -- and passes $validated['folder_id'] to the write. FilesController::store() is the same shape once #1694 gives it the guard. FilesController::update() and its API twin write it straight through with nothing in between. The result is a live file inside a deleted folder, which is a state nothing else in the application produces: FolderService::delete() deletes every file in the subtree along with it. The row is reachable by id, in search and over the API, and missing from the listing its uploader would look in. Rules::folderId() makes the check mean what its readers assume, once, where the reasoning can be written down -- the same argument slug() makes for itself one method above. Every site takes it, so the file cannot end up with two spellings of the same rule and no way to tell which is the safe one. What changes, path by path: - POST /files, POST /api/v1/files, PATCH /files/{file} and PATCH /api/v1/files/{file} refuse a folder in the trash instead of writing its id. This is the fix. - POST /uploads used to accept it and quietly file the upload at the root -- its guard and its write already agreed, on null. It now says so instead, which is what the other upload paths do. - files/{file}/move, files/bulk-edit, folders, folders/{folder}/move and the portal's my-folders already refused, through StaffLibraryScope::folders() or Folder::scopeVisibleToClient(), both of which drop trashed rows. They still refuse; the answer is now 422 naming folder_id rather than a bare 404. Those two guards are asking a different question -- "is this folder yours" -- and they keep asking it. No live folder id behaves differently anywhere, and the root (a null folder_id) is untouched. The published API document is unchanged: `exists` renders the same either way. Regenerated with php artisan scramble:export and byte-identical. |
||
|
denkfabrik-li
|
2c2b86ffa1 |
Hold the folder an upload names to the same library boundary as everything else
Folder::uploadableBy() returned true for any staff member without looking at the folder, on the strength of a comment saying staff had already validated folder_id through FilesController's own flow. No upload path did. FilesController::store() did not check the folder at all; the two that called uploadableBy() — the API upload and the chunked upload the browser actually posts to — called a guard that could only ever say yes. A client-scoped staff member could therefore name any folder id and put the file inside a subtree shared with somebody else's client, where File::scopeVisibleToClient hands it over without an assignment row ever being written. That is the boundary StaffLibraryScope's own docblock claims to hold everywhere. The staff branch now asks StaffLibraryScope::allowsFolder, which returns true for unscoped staff, so nothing changes for them. The client branch is untouched: a client is never client-scoped, and ownership or a public folder opting into client uploads remains the whole of their rule. The two folder pickers that fed those ids are narrowed the same way the listings around them already are. |
||
|
denkfabrik-li
|
acda732ff4 |
Enforce the download limit when a zip is delivered
A download limit is checked when an archive is ordered and again while it is built, but it is only spent when the archive is collected. Nothing about ordering or building moves the count, so every check along the way sees an allowance that is still untouched. That turns a prepared archive into a voucher. Order the same limited file into ten archives and all ten pass, because at the point each one is checked nothing has been taken yet. Collect them all and the file has been downloaded ten times against a limit of one. The three endpoints are independent of the interface that normally drives them, so this needs nothing more than calling store() in a loop — and no timing luck at all, since the archives can be collected minutes apart. DownloadAllowance says of itself that six routes put a file's bytes on the wire and that every one of them asks, precisely because there is no choke point to put the rule in. The zip pair asked in the two places that do not count and not in the one that does. So the delivery re-checks what the archive holds, where the count actually moves. Refusing is 403, matching the single-file download route for the same situation. It is also the only one of the two candidates that reaches the person: an archive is fetched by navigating to it, and there is no error view for 422, so the message would be replaced by the framework's generic "something is broken" page. One refused file refuses the whole delivery, because nothing can be taken out of a finished archive without building it again. Ordering the same selection afresh is the way through — the build leaves the spent file out and names it in skipped_files, which the poll already reports. This is stricter than store(), which drops spent files from a selection and refuses only when nothing survives: there, a selection can still be narrowed, and here it cannot. Checking costs nothing where nothing is limited. An unlimited file is answered from its own column and never reaches a count. Claiming the delivery is a conditional update now rather than a read followed by a write. Two fetches of one archive arriving together both saw delivered_at unset and both wrote a full set of downloads, counting a single delivery twice — the same shape as the conditional increment that guards a share link's max_downloads. Only the fetch that moves the column logs anything; the other still receives the archive, which is the existing rule that re-fetching one prepared zip is one delivery. Two things this deliberately leaves alone. Simultaneous downloads of one file can still both pass before either is logged: that race is documented in DownloadAllowance, and closing it needs the counter column it explains why it does not have. And an archive already delivered stays fetchable for its 24 hours even once the limit is spent — one delivery, re-fetched, which is what that rule is for. An archive built before the job recorded its contents is handed over the way it always was, without this check. What it holds can only be guessed at by resolving the selection a second time, and guessing is exactly what must not decide a refusal: the same reconstruction refuses over files the archive does not hold and misses files it does. Those rows stop existing within a day or two of an upgrade, and until then they behave as they did before this change rather than worse. |
||
|
denkfabrik-li
|
16787cf697 |
Record which files a zip actually contains
A zip download's row stores what was asked for — some file ids, some folder ids — and the download action resolved that selection a second time, when the archive was collected, to decide what to log as downloaded. The two are not the same thing. Folder contents are resolved against the scope as it stands at that moment, and an archive is written some time before it is fetched. Add a file to the folder in between and it was logged as downloaded without ever having been in the zip. Move one out of the folder and it was handed over without being logged at all. The same goes for a file that expired or otherwise left the requester's scope after the build: its bytes are in the archive either way. Nothing about this is visible to anyone — the download count on the file is simply wrong. The job already walks exactly the set that goes in, and already counted it for file_count. It now keeps the ids rather than a tally, and the download action logs those. count() gives back the number it was keeping before. Rows written before this column existed fall back to resolving the selection, which is what they were built for; the purge command clears them within a day. |
||
|
ignacionelson
|
073101d184 |
Put a ceiling on a zip download, and clean up after the ones that fail
Follow-up to #1687, which made a zip build report failure honestly. Four things it passed near, none of them regressions it introduced. A zip has never had a size limit — only a cap of 10,000 files, which bounds nothing that costs anything. Ten thousand spreadsheets zip in seconds; two hundred videos is an hour of stream-copying and an archive that fills the disk. Bytes are what a build actually costs, so the new Settings → Downloads screen caps the total size instead, at 2 GB out of the box. It is a setting rather than a constant because the safe figure depends on free disk, on whether sources live on a remote disk, and on the plan a hosted tenant is on — the file count stays fixed, since it is a foot-gun rail and not a knob anybody needs. The controller measures the selection at request time and names both numbers when it refuses; the job measures again, because it re-derives the selection at run time and a folder can grow while the job waits in the queue. Every shipped topology runs exactly one queue worker, and everything shares the default queue, so raising the job timeout to an hour handed any signed-in person an hour of everyone else's notification mail. There is now one build in progress per requester and a named throttle bucket on the endpoint, which had neither. A pending row older than an hour is treated as abandoned rather than in progress, so a worker killed hard enough to skip failed() cannot lock somebody out for good. Giving zip builds their own queue is the structural fix and wants its own change: it touches compose, supervisord and the systemd unit in INSTALL.md, and an install that upgrades without changing its worker command would stop building zips silently. zip_downloads.requested_by cascades on delete, so removing a user takes their rows with it and strands every archive they built — invisible to a purge that walks rows, and to OrphanFileScanner, which skips zips/ on purpose. The purge now also sweeps files in zips/ that no row explains, after a day's grace so a build in progress is never taken out from under itself. Two smaller things while in here. A build that failed because every file had already hit its download limit said only that nothing was available, and dropped the skipped list — the same distinction the store guard goes out of its way to draw at request time. And a failed close() now logs libzip's reason, which the @ silencing had been discarding: "the disk is full" and "the source vanished" are different problems for whoever has to fix one, while the requester still sees a message with no server paths in it. |
||
|
denkfabrik-li
|
862765643b |
Don't name a subfolder to a client who cannot open it
MyFilesController::index() builds the portal's folder list in two branches. At the root it narrows to $visibleIds; one level in it listed every direct child of the folder being browsed, visible or not. Opening one was still refused — $current is resolved through visibleToClient() and 404s otherwise — so what escaped was the name, not the contents. A name is worth protecting here for the same reason VisibleCommentScope gives about its own boundary: it is what stops one customer learning that another exists. $visibleIds is computed once and wanted in three places. The root branch narrows by it, the breadcrumb narrows by it, and the nested branch did not. That is a gap rather than a distinction, and the class docblock had already promised the opposite: "Group and internal folder names never leak." Reachable only where a folder is visible for the created_by reason rather than by sharing. Inside a shared subtree every child matches by path prefix anyway, so nothing leaks there. A client with create_own_folders makes such a folder through POST /my-folders, and staff can file anything inside it — FoldersController::store resolves its parent through StaffLibraryScope, which is unfiltered for unscoped staff. Files were never affected: that branch's query already starts from File::query()->visibleToClient(). The breadcrumb already trims to the first visible ancestor. Neither is touched. |
||
|
denkfabrik-li
|
329aef98e0 |
Finalise each chunked upload once, under a per-session lock
complete() assembled the received parts into the one target file and created the File row with no guard against a second complete() for the same session running at the same time -- an Uppy retry, a double submit, a resend after a lost connection. Two of them would interleave writes into the session's single `assembled` file (the stored bytes then no longer match the checksum computed from the in-memory buffers) and could each create a File row. Take a per-session lock around the finalisation and fail a second caller fast; the lock's TTL releases the claim if a completion dies mid-flight, so a genuine retry still works. The body moves to a finalise() helper so complete() reads as auth + lock + finalise. |
||
|
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. |
||
|
denkfabrik-li
|
21fdf98981 |
Scope a file's destination folder on update(), as move() already does
Setting folder_id through update() is the same privileged reparent as move() and bulkUpdate(), but only those two verified the target folder was inside the caller's library (StaffLibraryScope::folders). update() validated it only for existence, so a client-scoped staff member could reparent an in-scope file into a folder shared with a client they are not assigned to -- which File::scopeVisibleToClient then exposes to that client, sidestepping the boundary the sharing endpoints enforce (guardAssignable), and likewise into a public folder without upload_public. Apply the same scope->folders()->findOrFail() guard on both the web and API update(), but only when folder_id actually changes, so re-saving a file that already sits in an out-of-scope folder (reachable via a direct client share) still works. |
||
|
denkfabrik-li
|
71d6b8937e |
Enforce the max file size against the bytes a chunked upload assembles
store() checks Setting::MaxFileSizeMb against the size the client declares when it opens the session, and complete() re-checks the storage quota against the real assembled byte count -- but nothing re-checked the size limit itself. A client that declared a one-byte upload and then streamed gigabytes of parts passed store()'s check and was never stopped, so the configured limit (which store() applies to everyone, staff included) did not hold for the resumable path that real uploads use. Re-check the assembled byte count against MaxFileSizeMb in complete(), cleaning up the assembled bytes and the session exactly as the quota branch already does. |
||
|
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
|
51eea30dda |
Answer "did they ever actually get it?" from the file itself
The two things staff most often want to know about a file — who downloaded it, who looked at it — were answerable only by reading the whole activity log past everything else that had happened to it, or by going back to the library list for the details panel. The file's own page now has a Downloads & previews tab: the twenty most recent times it was taken or looked at, each with who did it and the address it went to, over a running count of both. Below them, two buttons open the file's full history already filtered — one to every download, one to every preview — so the narrow question is one click and the whole log is still one click further. Which filter value stands for "every download" is decided server-side and travels with the payload, because it is a fact about the log's vocabulary: downloads are three actions and share a group, previews are one action and are filtered by name. The history page now also keeps whatever filter it was sent with visible in its dropdown even at a count of zero, so a button cannot land somebody on an empty table above a select that has gone blank. |
||
|
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. |