mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
v2.4.0
81 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
||
|
ignacionelson
|
5d99ab94fd |
Say on screen when nothing is building zip downloads
Zip building moved onto its own queue, which a manual install's worker has to be told about. update.sh repairs the service file and Docker is unaffected, so the population left is somebody upgrading by hand who skipped the release note — and for them the failure is the worst shape available. Email keeps going out perfectly. Zip downloads never finish. Nothing in any log says why, because nothing went wrong: the jobs sit on a queue nobody is reading. The person who missed it has no reason to suspect anything, so the notice has to go looking for them. The application cannot see its own worker processes, only whether work gets done, so the question is asked from the other end: was a build requested that no worker ever picked up? That needs a record of when a build *started*, which is what the new zip_downloads.started_at column is — stamped before any of the work, so it says a worker had the row, not that the row succeeded. Two conditions, because either alone cries wolf. A build has waited past five minutes and was never started, *and* no other build is in hand. The second matters because one worker builds one archive at a time: a queue behind a large build is a healthy queue, and its waiting rows look exactly like abandoned ones until you notice something running. "In hand" is bounded by the job's own timeout, so a worker that died holding a build stops counting as alive an hour later. The banner sits beside the stale-code one, on every staff page rather than the dashboard alone, gated on view_system_info for the reason that one already argues: a background worker not picking work up is a fact about the machine, not a feature of an edition. It names the fix rather than the symptom — "your worker command needs --queue=default,zips" — because somebody reading that downloads are not being processed still has to work out what to do about it. Eight tests, covering both halves of the discrimination rather than just the happy one: a queue waiting behind a live build stays quiet, and a build held by a worker that died does not. Translated into all sixteen locales in the same commit, since a release is close and a banner nobody can read is worse than none. Checked on screen as well as in assertions, with a real stalled row on the dev stack: the banner renders, wraps, and reads correctly. |
||
|
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
|
9b3f7023d0 |
Merge pull request #1691 from denkfabrik-li/fix/file-bytes-after-commit
Delete a file's bytes when its transaction commits, not before |
||
|
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
|
92a132d74f |
Give zip builds their own queue, so one archive cannot hold up the mail
The last piece of the #1687 follow-up. BuildZipDownloadJob allows itself an hour, every shipped topology runs exactly one worker, and everything shares the default queue -- so one large archive delayed every notification email queued behind it. The size cap and the one-build-per-person rule bounded that in July; they did not remove it. onQueue('zips') in the constructor rather than at the dispatch site, so a second caller cannot forget it. Both images grow a worker for it: compose.yaml gains worker-zips, supervisord gains [program:queue-zips], and the existing worker in each narrows to --queue=default. --tries=1 there matches the job, which records its own failure rather than being retried. The part that needs care is the manual install. A worker whose command still says plain `queue:work` consumes `default` only, so it would send email happily and never finish a single zip, with nothing in any log saying why. INSTALL.md's unit now reads --queue=default,zips -- one worker watching both, which is right for most installations -- and says what happens if you leave it off, with the two-worker split offered for anyone who would rather keep the two kinds of work apart. CHANGELOG carries it as an upgrade note, since it is something to do rather than something that was done. Verified in the dev stack rather than only in a test: dispatched a build and watched worker-zips take it while the default worker stayed idle. |
||
|
ignacionelson
|
eb2917f5ff |
Hold a group object to the same boundary its membership already has
This overturns something #1701 decided, so it should say so. That PR closed the membership hole and left GroupsController::update and destroy installation-wide on purpose, on the grounds that managing the group object is a different question from managing who is in it. What decides it is a measurement that was not in front of that decision. An assignment to a group is how its members reach a file, so deleting a group revokes that access for every member. Measured before this guard, with a client-scoped role holding the group permissions: stranger client can read the shared file true PATCH /groups/{stranger group} 302, renamed DELETE /groups/{stranger group} 302, group gone stranger client can read the shared file false So a staff member who may not add somebody to a group out of their reach could delete it out from under the people already in it. That is not a gentler version of the membership rule, it is a harder one, and the two sitting on opposite sides of the same boundary was the odd part. StaffLibraryScope::allowsGroupChange is the reach half of allowsGroupMembership on its own, since no client appears in this question -- one predicate, two callers, rather than a second statement of it. Both surfaces take it, at 404, matching the membership guards. A group that shares nothing beyond the actor's library still passes, so a group they created or one holding their own clients stays theirs, and unscoped staff are unaffected by construction. The API document moves a 404 above a 422 on two paths. Both already documented the 404 -- route model binding produced one -- and Scramble orders responses by where they appear in the method, so the guard landing before the validate() call is the whole of the change. |