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.
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.
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.
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.
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.
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.
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.
groupReachesNoFurther() decides whether a client-scoped staff member may edit a group, by asking whether anything shared with it sits outside their library. f1b35cc9 settled that 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 and was not covered. File::scopeVisibleToClient ends in notExpired(), so the moment a file expires it leaves every member's /my-files and the download answers 403 -- but it also leaves files(), where its absence reads as "outside my library". The group then became unmanageable for good: the rep could not add anyone, and could not undo their own membership change either.
The reach query now skips expired files as it already skips deleted ones. File::scopeVisibleToClient is unchanged -- what expiry does to a scoped viewer's library was settled deliberately in c8078f65, and this is about what counts as reach, not about what anyone may open. The folder half needs nothing, because folders do not expire.
The limit this leaves open, stated rather than implied: a membership added while a file was expired outlives the expiry, so if somebody later clears expires_at the client reaches a file that was outside the actor's library when the decision was made. f1b35cc9 leaves exactly the same opening for a file restored from the trash, and closing either would mean the guard weighing rows nobody can currently reach.
Verified before merging: this changes the file half of the same method #1719 changed the folder half of, so the merged tree was read rather than trusted -- both halves now skip expired files consistently. 29 passed on the merged tree, 1 failed / 28 passed with app/ reset. The "an expired file does not excuse a live one that is still out of reach" test is green either way.
Reported and fixed by @denkfabrik-li.
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.
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.
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.
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.
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.
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.
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.
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.
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 c8078f65. This is about what counts as reach, not
about what a scoped viewer may open.
Two tests, next to the deleted-file pair they mirror: the lockout, and the
half that must not soften -- a live out-of-reach file is still reach with
an expired sibling next to it. Without the fix the first goes red.
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.
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.
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 0a8b609e went to some trouble to remove. Both halves
of FilePolicy::delete are expressible in SQL: the permission half is
constant for the viewer, and the library half is the query
StaffLibraryScope already memoises per request. Somebody holding both
delete permissions with no library scope short-circuits before the query
runs at all, so the common case pays nothing.
Not changed, deliberately:
- The service. FolderService::delete stays dumb. Its other caller applies
the client rule ("files you did not upload"), which is a different
predicate, and putting both in one place is the drift this codebase
keeps refactoring away from.
- The client half. MyFoldersController is already correct.
- Nothing partial. A blocked folder is left whole rather than emptied of
what the actor may delete -- half a tree is worse than either answer.
Worth saying plainly: this is a behaviour change. A folder delete that
used to succeed now refuses, and somebody will notice. The alternative is
irreversible loss of files the same person is refused one route over.
Six tests. Four measured red against the unguarded controller (4 failed /
2 passed), one per half of the predicate: the permission half, its
message, a nested file, and the library half -- that last one with both
delete permissions held, so only StaffLibraryScope can refuse. The two
that stay green either way are the other side of the question -- that a
folder holding only your own files still goes, and that an administrator
holding both permissions is unaffected. They guard against the fix
over-refusing, not against the bug.
Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean.
The new string is English only, per CONTRIBUTING.md -- translations are
their own pass.
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.
Finalise each chunked upload once, under a per-session lock
Resolved a trivial conflict in ChunkedUploadsTest: this branch and
d7e639b both append tests to the end of the file, so both are kept.
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.
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.
A full parallel run failed once and passed on retry while I was doing the
#1703 follow-up. A flake is worse than a steady failure: it trains you to
re-run rather than look, and it quietly weakens every green run reported
beside it.
Upload parts are real files under storage_path('app/uploads-tmp/{session_id}'),
not a faked disk. Every parallel worker gets its own database, so session
ids restart at 1 in each of them, and two workers writing parts land in
the same directory. On top of that ChunkedUploadsTest's afterEach deleted
the whole tree rather than its own share, for everybody. Six test files
write parts, so this was reachable without anything I added.
The same collision exists inside one worker: RefreshDatabase rolls back,
so ids restart at 1 for every test, and a run that died before its
cleanup leaves parts sitting under the id the next test is about to
claim.
LocalPartStore now reads its root from config, defaulting to exactly
where it always was -- an installation with UPLOAD_PARTS_PATH unset
behaves identically. Tests\TestCase points it at a per-worker directory
and empties that directory per test, which closes the cross-worker, the
cross-run and the intra-worker versions together. ChunkedUploadsTest's
cleanup and its two directory assertions read the configured root rather
than the hardcoded path, so they can no longer reach into a neighbour.
Verified with eight consecutive parallel runs, green, and by watching the
per-worker directories appear separately (w1, w2, w4 … w14) rather than
one shared tree. The isolation itself cannot be asserted from inside a
single test; what a test can pin is the mechanism it rests on, so one
does: parts go where the configured root says.
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.
The other half of the sweep. ClientsController and its API twin checked
`abort_unless($client->isClient(), 404)` and nothing else -- a type
check, not a boundary, which is the phrase #1701 used about the group
membership routes for exactly the same reason.
Measured before the fix, with a client-scoped role holding the client
permissions:
GET /clients every client on the installation, name + email
GET /clients/{stranger} 200
PATCH /clients/{stranger} 302, name actually changed
DELETE /clients/{stranger} 302, client gone
The tell was one route over. ClientFilesController::index already draws
this line with StaffLibraryScope::canAssignClient and calls it "the same
boundary StaffLibraryScope enforces everywhere else in the library". Its
neighbours in the same family did not.
So the predicate is not new here. What is new is StaffLibraryScope::clients(),
the listing half of canAssignClient, so a screen narrows by the rule its
own buttons are guarded with instead of restating it -- restating it is
how this went wrong, and how the last four of these went wrong.
Eight actions take it: edit, update, destroy and the two-factor reset on
both surfaces, plus both listings. Answering 404 rather than 403, since a
client outside the roster should not be distinguishable from one that is
not there -- matching the isClient() guard already above it.
Account requests stay installation-wide on purpose: a self-registered
client who has not been approved belongs to nobody yet, so there is no
roster to narrow by and narrowing would empty the screen.
The published API document is unchanged -- both routes already documented
the 404 that the type check produced.
#1701 closed a real hole: group membership decides what a client reaches,
and through File::scopeVisibleToClient it decides what the staff member
holding that client reaches, so `edit_groups` alone was never a boundary.
The predicate it added asks whether everything shared with a group is
already inside the actor's library.
It asked by counting: pluck the group's assignment rows, count how many
of those ids the library query returns, and require the two to match. An
assignment row outlives the thing it points at — nothing clears them when
a file or folder is deleted — while files() and folders() exclude trashed
rows by construction. So one deleted file left a count that could never
balance again, and the group closed permanently: the scoped staff member
could no longer add their own client to it, or remove anybody from it,
with a 403 and nothing to explain it. Every group accumulates dead
assignments over time, so groups would have gone quiet one at a time.
Asked the other way round — is there anything live, shared with this
group, that is outside my library — the dead rows drop out by
construction, because the query starts from File/Folder rather than from
the assignment. That is also the truer question: a deleted file is not
reach, since nobody can reach it.
Three tests. A group stays usable after a file shared with it is deleted,
including removing a member; the same for a deleted folder assignment;
and the half that must not soften — a live file still out of reach is
still refused, deleted siblings or not.
#1698 moved the library boundary into FileCommentPolicy, where it
belongs, and said plainly what that cost: the moderation screen went
from 65 queries to 465 for a client-scoped moderator with five assigned
clients. Measured here, those numbers are exactly right.
The cost is not in asking. It is that StaffLibraryScope::files() rebuilds
its query every time, and building one runs four immediate lookups per
assigned client — the client's group ids, the same ids again inside
Folder::sharedFolderIds(), that method's own assignment lookup, and the
shared-folder get() in Folder::scopeVisibleToClient(). None of them
depend on the query being built. Gate resolves a fresh policy for every
check, so a listing paid for all of it once per row.
The built query is now memoised per user and handed back as a clone,
since every caller adds to it, and the scope is registered as `scoped`
rather than transient so the memo survives a request. Scoped rather than
a singleton on purpose: a long-lived queue worker keeps singletons
between jobs, and a library query built from one job's data has no
business answering the next one's question.
That is 465 queries down to 60 on the same page — below the 65 it cost
before #1698, because the memo also helps the callers that were already
asking repeatedly. FileVersions::sharedAudience(), which runs the same
helper twice per candidate while resolving notification recipients, gets
it for free.
So the answer to the question #1698 left open is neither of the two it
offered. can_delete stays a real question asked of the policy; nothing
restates the boundary; and the page is faster than it was before the
fix. Three tests: one user's query never answers another's, one caller's
constraints never follow the next, and the moderation screen does not
ask once per row.
The four routes that edit a group's membership -- add and remove, web
and API -- contain no authorization call of any kind. `can:edit_groups`
in front of them is the whole of it, and a permission is not a boundary.
The authorization sweep looked at these and let them stand, on the
grounds that groups are installation-wide by design: GroupsController
::index lists every group unfiltered, so list and single-object access
agree, and there is no listing/direct-access mismatch to fix. That is
true, and it is the answer to the question of who may *see* a group.
This is a different question: what a write to one *does*.
Joining a group hands the new member everything shared with it. When
that member is one of a client-scoped staff member's own clients,
File::scopeVisibleToClient hands the same content straight back to them
-- that scope is what StaffLibraryScope::files() is built out of. So the
one write turns a file they get a 403 on into a file in their library,
and the download that follows is a 200. ResolvesShareTargets draws that
line on the sharing path through canAssignGroup(); nobody drew it on the
membership path, and canAssignGroup() is *derived from membership*, so
whoever may edit the list also decides what the list entitles them to.
StaffLibraryScope::allowsGroupMembership answers it directly instead of
through the derived predicate, which is the wrong tool here twice over.
Membership asks about reach, so it checks reach: the client must be one
this staff member holds, and the group must not already reach past their
library -- no file assigned to it, and no folder shared with it, outside
StaffLibraryScope. A group nothing has been shared with passes trivially,
which matters, because canAssignGroup() would have said no to a group
that has no members yet and left a scoped staff member unable to put the
first client into one they had just created.
The same write has a second door. MembershipRequestsController::approve
joins a client to a group with identical consequences, under
`approve_groups_memberships_requests`, and deny() decides about somebody
else's client and emails them about it. Both go through the same
boundary, answering 404 to match the guard already above approve().
The queue and its sidebar badge are narrowed to the clients the viewer
holds, through one scope on the model that both read -- the rule the
comment badge in HandleInertiaRequests already states two branches down
("a client-scoped staff member is not shown a number they cannot act
on"), and the reason VisibleCommentScope owns its own pendingTotal()
rather than leaving the middleware to count for itself. Each row carries
the client's name and email, so an unnarrowed queue was also handing
those over for clients outside the roster. Unscoped staff still see every
pending request.
That narrowing is on the client, not on the group: whether a group is
reachable depends on what is shared with it, which is not a question to
ask row by row in a listing. A scoped viewer may therefore still be
shown a request they would be refused on -- one of their own clients
asking to join a group out of their reach. The names were the part that
leaked.
Unscoped staff are unaffected throughout -- both halves of the predicate
are true for them by construction. No seeded role reaches this: Client
Manager is the only client-scoped role that ships, and it holds no group
permissions, so a custom role is needed to get here at all.
The published API document gains a 403 on both member routes.
Regenerated with php artisan scramble:export; Scramble reads abort_unless
out of the method body but not out of a private helper, which is why the
guard is written out at each of the four call sites rather than shared.
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.
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.
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.
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.
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.
File::booted() removed the bytes the moment a row was deleted. For a
single file that is right. Two paths delete files inside a transaction,
though, and both delete many at once: FolderService::delete() takes a
folder's whole subtree, and DeletedAccountContent::cascadeDelete() takes
everything an account uploaded.
Anything that rolls either transaction back puts every row back while the
bytes are already gone. A transaction exists to make a set of writes
undoable, and removing the bytes was the one write in that set that
nothing can undo. The account path is the sharper one: since content
disposal is nested inside the caller's transaction, the write that fails
need not be in this code at all.
The two failure directions are not equal. Bytes gone with the rows
restored leaves rows pointing at nothing and no way back. Rows gone with
the bytes left leaves orphans on disk, which OrphanFileScanner already
exists to find. Defer to the recoverable one.
Three properties this relies on, all of them checked rather than assumed:
without a pending transaction the callback runs immediately, so a single
delete is unchanged; a savepoint committing inside a larger transaction
does not fire it, which is exactly the account case; and the connection
is the row's own rather than whichever is default.
detachOnDelete stays inside the transaction — it repairs the version
chain's pointers, which is database work that must roll back with
everything else.
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.
BuildZipDownloadJob deferred every write to ZipArchive::close() but then
marked the row STATUS_READY regardless of the result:
- close() returns false when a source file was deleted between addFile()
and close() (a concurrent staff delete runs FileDiskCleanup at once) or
the disk filled up; the row went ready over an archive libzip never
wrote, and the download controller X-Accel-served a path that isn't there.
- An archive that ended up with no entries (every selected file removed or
its allowance spent before the queued job ran) is written as no file at
all by libzip, yet close() still returns true — again marked ready.
Check both the close() return and the added-entry count, and fail the row
(deleting any partial archive) when either says nothing was written.
The job also had no $tries/$timeout/failed(): a build of up to MAX_FILES
sources runs past the worker's default 60s timeout, and the kill skips the
catch, stranding the row as PENDING while the frontend polls forever. Give
it room, run it once, and add a failed() backstop that fails a row still
pending (leaving an already-resolved one alone).
Finally, purge leftover zips/{id}.zip* by row id: a killed build leaves a
partial archive and libzip temp file with no path recorded, so the path
field alone never cleaned them up.
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.
Four create flows redirected to the new record's edit page on success,
but store is gated by create_* while the edit page is gated by edit_*,
and PermissionChecker has no create-implies-edit rule. A role holding
create_* without edit_* would create the record -- write, activity log
and notifications all run -- and then meet a 403 on the success
redirect, with no way to tell the action worked and every reason to
submit a duplicate. Categories is reachable with plain UI clicks, since
the sidebar shows it from create_categories alone.
Keep landing on the edit page for anyone who may edit, and divert only
those who can't -- to the create form, which shares store's own gate
and is therefore reachable by exactly whoever just created the record;
the success toast shows there. The index would not do: Clients/Groups
lists are gated by manage_*, which store itself does not require.
Implying edit_* from create_* would not do either -- edit has no
own/others split here, so it would silently hand a deliberately narrow
create-only role edit (two-factor reset included) on every existing
record.
ignacionelson