mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-17 00:55:07 +00:00
v2.4.1
70 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ignacionelson
|
50f8b578df |
Ask the publication question wherever content lands, not just on upload
Reported by @skeletonsec as GHSA-rxf8-wh8v-jm9j. A file in a public folder is public: isEffectivelyPublic() is "my own flag, or my folder's", read up the whole ancestry. GHSA-237r-jx85-j3hr settled that three days ago, put the rule in Folder::uploadableBy(), and wired it into the upload paths. Content arrives in a folder four other ways. move() drags one file in, bulkUpdate() moves a selection, update() reparents through the edit form, and FoldersController::move() drags a whole folder — every file in its subtree — under a public parent. Each of them asked whether the destination was *visible* to the mover and then wrote folder_id. Visible is not the same question as publishable, and the difference is the entire permission: a staff member given editing rights and deliberately not given upload_public could publish confidential files to the anonymous site by choosing where they landed. The API twin of update() had the same gap. Both earlier advisories named these paths in their own "suggested fix" sections. Neither demonstrated them, so neither was followed. The fix to a report wants the scrutiny the report got, and this one did not get it. The predicate did not need changing — it needed calling. Four sinks now ask it, plus the API twin. The check stays split in two deliberately: the destination is resolved through StaffLibraryScope as before, so a folder somebody cannot see is still a 404 and not an existence oracle, and the publication clause is a separate 403 on top. They agree by construction — allowsFolder() is folders()->whereKey()->exists() — so nothing that used to resolve can now fail the first half. On the file paths the check fires only when folder_id actually changes, which is the convention already there: re-saving a file that sits in a folder out of the saver's scope must keep working. bulkUpdate() checks its destination once instead, before the loop, because there is one destination for the batch and if it publishes then no file in the batch may go. Folder::uploadableBy()'s docblock now says to read the name as "may place into", with why: the name is what made this easy to miss, and the next folder_id or parent_id write will be written by somebody reading it. Ten tests, one per sink with a private-destination control beside it, plus an editor who *can* publish to show the boundary is about publishing and not about moving. The last one follows the advisory's own chain to the end and asserts the thing actually claimed — a stranger with no session, no token and no assignment fetching the anonymous download URL. It returns 200 on the code before this commit and 404 after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx |
||
|
ignacionelson
|
6ad26bb61e |
Hold an upload to the size it said it was sending
Reported by @ry2811 as GHSA-6jh6-gvj5-pv8v. A resumable upload declares its size, and that declaration is what store() weighs against the maximum file size and the client's storage quota. Only the assembled file was ever held to it. The parts in between were bounded one request at a time and never added up, so a client could declare one byte and then stream parts: ten thousand part numbers at twice a 20 MB part is about 400 GB, per session, and the number of sessions was not bounded either. None of it counted against anything, because nothing becomes a File row until the upload completes and ClientStorageUsage sums File rows. A client with a 1 MB quota could fill the volume and repeat. putPart()'s own comment described this defect and treated the per-part cap as the answer to it: "without a cap here the exposure is a day's worth of disk". A cap on one request bounds one request. The exposure was a day's worth of disk multiplied by however many requests somebody cared to make. Three limits, and each one exists because the other two do not cover it. A session may not stage more than it declared. The room for a part is claimed before the body is read — a body's length is not known until it has arrived, and by then it is on the disk being protected — and the write is then capped at exactly what was claimed, so an over-long body is cut off mid-stream as it always was, against a smaller number. The claim is a read and a conditional update under a per-session lock, the same shape complete() already uses: the protocol sends parts in parallel and how many is the client's choice, so an unlocked read lets every part in flight claim the same room, while an atomic claim alone refuses the honest parallel upload instead. Whatever the part really weighs is settled back afterwards, in a finally, or a client's own retries would exhaust a session with room to spare. Open sessions count against the quota at the size they declared. A quota measured against finished files alone is spent twice by opening sessions one after another — each is told there is room, because the ones before it have not finished. The cost is that an abandoned transfer holds its share until it is cancelled or swept, so the sweeper now runs hourly rather than daily: that gap is now somebody unable to upload, which it was not before. And a cap on open sessions, because for anyone with no quota to spend — staff, and clients on an installation that sets none — the session count is the only thing between a declared size and any multiple of it. Four tests fail on the unfixed code, and three existing ones had to change: they declared a tiny size and sent a large part deliberately, to reach the re-checks at complete(). That route is now closed at putPart(), so they reach those re-checks the way a real install would instead — the file-size limit or the quota moving while a long transfer is running, which is the reason complete() re-asks rather than trusting what store() decided. The staged-byte total is BIGINT UNSIGNED, and the suite runs SQLite, which has no unsigned integers. The first version of the bounds read `staged_bytes + :delta BETWEEN 0 AND size` and raised SQLSTATE 22003 on MySQL for any refund — in the comparison, so the bound written to prevent the underflow was the statement that underflowed. Every SQLite test passed on it. Both bounds are now arranged so the column is never inside a subtraction, and UploadSessionStagedBytesMysqlTest skips loudly unless the connection is MySQL. Verified against 8.4, as was the report itself: three sessions declaring one byte each put 6 MB on the volume of a client with a 1 MB quota before, and nothing at all after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx |
||
|
ignacionelson
|
bc559ade3f |
Prove a client's upload announces itself, not just a staff one
The test above this said "whichever path stored it" and only exercised the plain staff POST. The path the hosted free tier hangs on is the other one: a client, through the resumable flow, whose upload is what cloud-modules listens for to mint the public link. Worth its own test rather than trusting the shared StoreUploadedFile, because the package's suite structurally cannot tell us. It fakes both the event and the link-minting, so a chunked path that stopped dispatching would leave all 140 of its tests green and the free tier silently inert on a real instance. That gap is why the listener was checked against sim-cloud by hand rather than believed; this is the half of it that belongs in core and runs on every commit. The counter-check is worth a note. The first attempt at it changed nothing — `\$file` inside a sed pattern is a literal, so the substitution never matched and all six tests passed, which reads exactly like a fix that is not load-bearing. Confirmed the mutation landed by counting the line before re-running: three tests fail without the dispatch, the new one among them. |
||
|
ignacionelson
|
1149df277b |
Ask for the public-folder key before publishing through a folder
Reported as GHSA-237r-jx85-j3hr.
A file is public if its own flag is on or its folder's is, so the upload
destination reaches the property `upload_public` guards without touching
the switch. A staff member allowed to upload but deliberately not allowed
to publish could publish to the anonymous public site by choosing where
the file landed.
No new key. `upload_to_public_folders` already exists, already appears on
every role's checkboxes, and already means exactly this on the client
branch of the same method — MyFilesController's picker calls it the
established meaning of the two keys. It was never asked of staff, so on a
staff role that checkbox did nothing at all: an unenforced permission, the
class this project audited and closed once already.
Effectively public rather than the folder's own flag, because the flag is
inherited down a subtree: a private folder inside a public one publishes
just the same, and a check on the folder's own column walks past it. There
is a test for that case specifically.
One place, because every upload path — the plain POST, the chunked flow,
the API and the client portal — already asks Folder::uploadableBy(). The
sibling report about the target folder not being scope-checked at all
(GHSA-56qr-cq56-qg66) was fixed in
|
||
|
ignacionelson
|
896675d631 |
Tell a client whether their own file arrived
"Did it arrive?" is the question somebody asks about a file they sent, and on a hosted free account — where a link is the whole of the sharing — the count is the only evidence either way. Every file a client uploaded now shows how often it has gone out and when it last did, in every render mode of every theme. Only their own. A download entry says somebody fetched the file, so a count on a file shared with several clients tells each of them about the others' activity, and nobody is entitled to that but the person who put the file there. A file shared *with* this client carries null, not zero: a zero would itself be a claim, and the two have to be distinguishable because zero is an answer the owner came looking for and is shown as words. Counted from the activity log through the same three actions DownloadAllowance uses, so a file leaving by the public site counts as much as one leaving by its link. One query for a listing, none at all for a client with no files of their own. Both filters have a test that fails when only that filter is removed. Two things a render check caught that types and a green build did not. `t()` does no plural selection — the catalogues are flat key/value — so a "one|many" string reached the screen with its pipe intact; the strings are whole sentences now, with the singular spelled out. And the gallery card was already laying its text out beside the action icons in a 200px column, truncating the filename to "Q…" and the size to "75 …" on main today; stacking them gives every line its full width. |
||
|
ignacionelson
|
92bb807849 |
Show a client the public link to their own file
A client's portal lists two kinds of file side by side: what they uploaded, and what somebody shared with them. Where a link exists on one of their own, they can now copy it from the row — which is what makes the hosted free plan a product rather than a place to put files, since a customer there has no staff screen on which to make one. The rule is narrow, and both halves are load-bearing: a link this client created, on a file this client uploaded. Not "a link on a file shared with them" — that link is the sharer's decision about who may reach the file, and handing the recipient the URL would quietly turn "you may download this" into "you may pass this on to anyone". And not "any link on their own file" either — a link staff minted on a file a client uploaded exists for a reason the client may be no part of, and on the shared instance it would sit beside the one link they were promised. Ordering is by id, so an unfiltered lookup would hand them whichever was minted first. Both halves have a test that fails when only that half is removed. The first draft did not: every case was carried by the ownership filter alone, so the creator check was green for the wrong reason. Links that no longer work are left out rather than shown greyed. The only thing a client can do here is copy it, and a URL that answers "this link has expired" is worse than no URL at all. One query per listing, not one per row, and none at all for a client with no files of their own. |
||
|
ignacionelson
|
757fba19ca |
Give uploads a seam, and link-minting one home
Two pieces of groundwork, no behaviour change.
FileWasStored is dispatched from StoreUploadedFile, which every upload
path converges on — the chunked flow staff and clients share, and the
synchronous POST beside it. A listener therefore sees each upload once
without knowing which route produced it, which is the property that makes
it usable from outside this repository. A notification, not a filter:
nothing on it is mutable, and anything that needs to influence an upload
has to do so before the bytes land, which is what ResolvingUploadDisk is
already for.
CreateShareLink is the other half. Minting a link was a ShareLinksController
private concern, and the controller is an HTTP handler behind `staff`
middleware — so a link now needs making from outside a request as well.
Two copies of "make a token, write the row, log it" would drift, and the
half most likely to drift is the token, which is the entire authorization
for /s/{token}: there is no session behind it and no second factor, so
being unguessable is its only defence. Anything minted through the action
gets Str::random(32) — about 190 bits, more than a UUID's 122 — and never
a chosen value. The chosen-token path stays in the controller, where a
person is typing one into a form and its minimum length can be argued
about in a validation rule.
The permission questions stay in the controller too. Whether somebody may
set an expiry or a download cap is a fact about them, and the action has
no viewer to ask; it takes both already resolved, including the expiry,
because "the end of the 12th" depends on whose timezone you are in.
Five tests, including that the file a listener receives is complete and
readable rather than half-built, and that the staff form still refuses an
expiry to somebody without the permission after the extraction.
|
||
|
ignacionelson
|
763e7b0e2e |
Render one image once, however many requests ask at the same time
Renditions are generated on demand and cached by existence, and nothing between the callers stopped two requests decoding the same image at once. The atomic rename settled which file survived; it never stopped both from doing the work. So N concurrent requests for one cold rendition were N full-size decodes, each holding four bytes per source pixel — up to 160 MB at the 40-megapixel ceiling. That is not an attack. A public listing emits a thumbnail URL per file, a browser opens six or more connections at once, and the first visit to a gallery of ordinary camera images was six simultaneous decodes on a container sized for one. PublicGroupsController reaches the generator with no account at all, so nothing about it required a customer to be signed in, and the 240/min throttle bounds rate rather than concurrency. Worse than a crash, it did not resolve itself: a render killed mid-flight renames nothing, so the cache warmed only by whatever finished before the kill and the page died again on the next visit. A lock keyed on the destination path — which already encodes the file, the audience and the rendition, so two requests collide exactly when they would have written the same path. The waiter re-reads after acquiring, which is what turns a wait into a cache hit rather than a second decode of the same image. Waiting rather than refusing, because the arithmetic says so: a waiting request holds an idle worker at about 35 MB, a rendering one holds that plus the whole source bitmap. Six waiters cost what one renderer costs. On timeout it refuses instead of rendering anyway. Falling through would reinstate the pile-on at the moment the system is already struggling, and one failed thumbnail is a better outcome than a container that dies and takes the warm cache with it. The wait is configurable because the right number is a property of the machine — a small VPS reading a large source off a slow disk wants longer — and clamped to at least a second, since a stray empty variable would otherwise make every concurrent request fail instantly, which is the opposite of the point. Eight tests. Two go red without the lock, and the clamp is asserted on the resolved value rather than the clock, because block() measures in whole seconds and a timing assertion there would be flaky rather than wrong. Found by the session sizing free-tier containers, from the outside. |
||
|
ignacionelson
|
a5496d24cd |
Stop describe() vouching for a detection it could not make
`FileDelivery::describe()` from a console returned
`{"method":"php","detected":true}` on every installation, whatever its web
server. detect() reads SERVER_SOFTWARE, which only exists inside a
request, so a console process has nothing to look at and falls to the
`php` default — and `detected: true` then vouched for it.
The value is right for that process and wrong as a statement about the
installation, which is how anybody running it from `artisan tinker` will
read it. Somebody verifying a healthy nginx tenant hit exactly that, spent
an afternoon on it, and only recognised it as an artefact of *where* the
question was asked after reading `nginx -T` in the container.
There is now a third field. `observed` is false only outside a request,
where `method` is a default rather than a finding. Both screens that read
this run in a request and always see true; it exists for whoever asks from
a shell, which is the one place the answer could mislead.
The two web paths are unchanged and were never wrong — `projectsend:status`
does not report delivery at all, so no fleet ever reported this
incorrectly. What was wrong was a confident answer to a question that
could not be answered from where it was asked.
Three tests: a console reading says not observed and still says php,
because php is what that process would actually do; a reading during a
request observes nginx; and a stated method is observed wherever it is
read, since a decision needs nothing detected to be true.
Found by the session verifying the 2.4.0 canary, not by me.
|
||
|
ignacionelson
|
b758fca19c |
Merge pull request #1724 from fix/assemble-keeps-parts-for-retry
Keep an upload's parts until its bytes are stored |
||
|
ignacionelson
|
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
|
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
|
b7a94d4479 |
Merge pull request #1742 from denkfabrik-li/fix/file-permissions-test-reads-config
FILES_WEB_SERVER_READABLE exists so a web server running as a different user can traverse the directories a download lives in. It asked for 0755 from a key that is never consulted: FilesystemManager::createLocalDriver() passes directory_visibility ?? visibility ?? private as the default visibility for directories, and this disk sets visibility to public two lines above with no directory_visibility, so Flysystem reads dir.public and never looks at dir.private. The mode came out 0755 anyway, because 0755 is Flysystem's default for a public directory -- the right answer from the wrong place, which is the kind that stops being right quietly. Adding a directory_visibility to this disk, an ordinary hardening move, or a change to that Flysystem default would have been enough to break the flag silently on exactly the hosts that need it. Both directory keys are now named, so the intent survives whichever branch Flysystem takes. Nothing widens: the flag-off path is still literally the old configuration, spread rather than ternary, and under the flag 0755 was already the effective mode. And the test could not have caught it, because it was not testing this configuration: filesDiskWith() restated the shipped branch inline, verbatim down to the 0755, so it kept passing against its own copy however the real one changed. It now requires config/filesystems.php and replaces only the root. Two housekeeping fixes ride along: the scratch root is per parallel worker, the way Tests\TestCase already does it for upload parts, because eight workers sharing one real directory means one worker's afterEach deletes another's tree mid-test; and the tree is cleared before each test as well as after, so a killed run does not poison the next one. Verified before merging: 3 passed on the trial-merge, and the mutation counter-check was run here rather than taken from the PR. With the shipped dir.public changed to 0750, this branch's test goes 1 failed / 2 passed and main's version of the same file goes 3 passed -- the old one genuinely could not see a change to the shipped configuration. 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
|
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. |
||
|
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
|
b838036a9a |
Set the directory permission Flysystem actually reads
FILES_WEB_SERVER_READABLE asks for 0755 on the directories a download has to be traversed through, and asks for it from a key that is never consulted. FilesystemManager::createLocalDriver passes `directory_visibility ?? visibility ?? private` to PortableVisibilityConverter::fromArray() as the default visibility for directories. This disk sets `visibility` to public two lines above, and no `directory_visibility`, so directories are public and the converter reads `dir.public`. The configuration names only `dir.private`. The mode is 0755 regardless, because 0755 is Flysystem's default for a public directory -- the right answer from the wrong place. Adding `directory_visibility` to this disk, or a change to that default, is all it would take for the flag to stop doing what it says. Measured on main, with the flag on: dir.private 0755 → 0750 directory stays 0755 (nothing reads it) dir.public 0755 → 0750 directory becomes 0750 (this is the key) Both are named now, so the intent survives either way round. FilePermissionsTest could not have caught this, because it was not testing this configuration. filesDiskWith() restated the shipped branch inline, verbatim down to the 0755, so it went on passing against its own copy however the real one changed. It now requires config/filesystems.php and replaces only the root, which is what makes the mutation above visible to it. Two more things in the same helper, both about the suite rather than the subject: the scratch root is per worker now (Tests\TestCase does the same for upload parts, and eight workers sharing one directory means one worker's afterEach deletes another's tree mid-test), and it is cleared before each test as well as after, so a killed run does not poison the next one. |
||
|
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
|
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
|
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.
|
||
|
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
|
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
|
41b4e477b5 |
Give each parallel test worker its own directory for upload parts
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. |
||
|
ignacionelson
|
d7e639b7af |
Close the two-request version of the deleted-folder target, and say why it failed
Follow-up to #1703, which made `exists:folders,id` mean what its ten readers already assumed. Two things it named and deliberately left. **The chunked upload is two requests.** store()'s rule only ever sees the first: POST /uploads records the resolved folder on the UploadSession and complete() reads it back from the session rather than from the caller, so deleting the folder while the bytes are in flight still files the assembled file into it -- the same orphan state #1703 removes, reached by a door a validation rule cannot watch. complete() now re-resolves through Folder::query() and files at the root when the folder has gone. Root rather than a refusal, because the two moments cost different things. At store() nothing has been sent, so refusing is free and honest, which is the call #1703 made. Here the bytes are already uploaded, and discarding somebody's finished transfer over a folder that vanished underneath them is the harsher of the two surprises. The file lands somewhere they can see it and move it. **The refusal now explains itself.** "The selected folder id is invalid" says nothing when the answer is that the folder has been deleted -- and that is the usual way to meet this rule, since a live id picked from a list is how anybody gets here. It matters most on the chunked path, the one place #1703 makes a previously-working request fail. A small ValidationRule object carries the message, which keeps the single definition Rules::folderId() exists for: a messages() array would have to be repeated at all ten call sites, and rules meaning different things in ten places is what went wrong in the first place. One note for whoever writes the next test here. Upload parts live in storage_path('app/uploads-tmp/{session_id}'), which is a real shared directory rather than a faked disk, and each parallel worker's database restarts session ids at 1 -- so two files writing parts on two workers collide, and ChunkedUploadsTest's afterEach deletes the whole tree for everybody. Six test files write parts today. These two cases live in ChunkedUploadsTest rather than beside the rest of their subject so this change does not add a seventh racer; the underlying isolation problem predates it and is worth its own fix. |
||
|
Ignacio Nelson
|
8d896191e4 |
Merge pull request #1703 from denkfabrik-li/fix/deleted-folder-upload-target
Say what `exists:folders,id` was already being read as |
||
|
Ignacio Nelson
|
18e4e014e6 |
Merge pull request #1683 from denkfabrik-li/fix/orphan-scanner-preview-renditions
Keep preview renditions out of the orphan-file scan |
||
|
ignacionelson
|
0a8b609e8b |
Build a scoped staff member's library query once per request, not once per row
#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. |
||
|
denkfabrik-li
|
7727ad7616 |
Say what exists:folders,id was already being read as
Folder uses SoftDeletes. The `exists` rule runs against the table, so a folder in the trash passes it -- while every resolution that follows goes through Folder::query(), which honours the soft delete and finds nothing. Ten rules across five controllers rely on that check, and each one reads it as "this folder exists". Two of them then wrote the id anyway. Api\FilesController::store() resolves the folder, hands the null to Folder::uploadableBy(), is told yes -- correctly, that is the rule for a root upload -- and passes $validated['folder_id'] to the write. FilesController::store() is the same shape once #1694 gives it the guard. FilesController::update() and its API twin write it straight through with nothing in between. The result is a live file inside a deleted folder, which is a state nothing else in the application produces: FolderService::delete() deletes every file in the subtree along with it. The row is reachable by id, in search and over the API, and missing from the listing its uploader would look in. Rules::folderId() makes the check mean what its readers assume, once, where the reasoning can be written down -- the same argument slug() makes for itself one method above. Every site takes it, so the file cannot end up with two spellings of the same rule and no way to tell which is the safe one. What changes, path by path: - POST /files, POST /api/v1/files, PATCH /files/{file} and PATCH /api/v1/files/{file} refuse a folder in the trash instead of writing its id. This is the fix. - POST /uploads used to accept it and quietly file the upload at the root -- its guard and its write already agreed, on null. It now says so instead, which is what the other upload paths do. - files/{file}/move, files/bulk-edit, folders, folders/{folder}/move and the portal's my-folders already refused, through StaffLibraryScope::folders() or Folder::scopeVisibleToClient(), both of which drop trashed rows. They still refuse; the answer is now 422 naming folder_id rather than a bare 404. Those two guards are asking a different question -- "is this folder yours" -- and they keep asking it. No live folder id behaves differently anywhere, and the root (a null folder_id) is untouched. The published API document is unchanged: `exists` renders the same either way. Regenerated with php artisan scramble:export and byte-identical. |