mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-24 12:22:01 +00:00
9c6f4df5bc2bc4bb0a0530cb5fa26c85317e5798
288 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ignacionelson
|
9c6f4df5bc |
Merge pull request #1736 from denkfabrik-li/fix/scoped-creator-keeps-client
A client-scoped staff member with create_clients created a client and lost it in the same request. guardTarget() answers 404 for anything off their roster, and StaffLibraryScope::clients() leaves it out of their list -- so the record existed, was logged, was welcomed by email, and was invisible to the person who made it. store() redirects to the edit page, which is exactly where they landed on a 404. The API twin had the same shape: a scoped token got a 404 from every route that binds the client it had just created. The new client is now attached to the creator's roster when the creator is client-scoped, on both sides. That is where a client they created belongs -- the roster is the same list assignedClients already uses for everything else they may reach. Unscoped creators gain nothing: they see every client already, and a roster entry would change what assignedClients means for them. Nothing is attached retroactively. The widening this involves is self-limited: the only thing added is an account the creator just made, which starts with no files, no folders and no group memberships, so assignableClientIds gains nothing to reach. Seats do not move either, since they are counted from active and account_requested. Verified before merging: 34 passed across both suites on the trial-merge, 2 failed / 32 passed with app/ reset. This is the busiest file set of the series -- it shares ClientsController with #1718 and Api/ClientsController plus the API test file with #1723 -- so the merged result was read rather than trusted: #1718's reassign_candidates gating and #1723's patchCustomFieldValues are both intact alongside it. scramble:export reproduces the committed docs/api/openapi.json byte for byte. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
2903a1da6d |
Merge pull request #1735 from denkfabrik-li/fix/editable-once-checkbox
ClientPortalCustomFields::save() writes '0' for an unticked checkbox, and filled('0') is true in Laravel. isLocked() asked whether anything is stored, so an editable_once checkbox locked itself the first time the client saved the page it sits on, whatever they had chosen. A box they never ticked could then never be ticked, and the one edit the setting promises was spent on a decision they had not made. A text field left empty stores null and stays open; that asymmetry was the bug, and '0' is the absence of a decision in exactly the way null is for every other type.
A checkbox now locks on a stored '1' and nothing else. Every other type keeps filled(). What save() stores is unchanged -- '0' remains a recorded "no", as the API's client create also writes it -- and the behaviour after a real tick is unchanged too: the client still cannot untick it, and the test pinning that is untouched.
Verified before merging: 6 passed on the trial-merge, 1 failed / 5 passed with app/ reset. The editable-once text field test is green either way, which confines the change to checkboxes. The relaxation is safe because the lock is enforced on the write path and not only rendered: isLocked() gates rules(), which drops the field from validation, and save(), which skips it, so the ticked-to-unticked direction stays closed server-side.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
c11cb3cc63 |
Merge pull request #1734 from denkfabrik-li/fix/quota-message-inherited-default
ClientStorageUsage::quotaMb() exists because a client's own storage_quota_mb of 0 does not mean "unlimited" -- it means "no quota of their own", and the site default is what is then enforced. Both chunked-upload quota checks enforced the resolved limit through quotaBytes() and then printed the raw column in the rejection, so a client with no quota of their own and a site default of 1 MB was told "This upload would exceed your storage quota of 0 MB." That is every client who was never given a quota, including every self-registered one, and the sentence appears at the one moment somebody is trying to find out what their limit is. Both now print quotaMb(), which is what the check enforced. The API's single-request upload already did exactly this for the same sentence, so the three copies agree. The enforcement itself is untouched -- only the number in the message changes -- and the unlimited case never reaches these branches, because quotaBytes() > 0 guards them. Verified before merging: 16 passed on the trial-merge, 2 failed / 14 passed with app/ reset. The "a client with a quota of their own still sees their own number" test is green either way. The string itself is unchanged, so no locale file needs anything. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
5117511946 |
Merge pull request #1732 from denkfabrik-li/fix/public-preview-log-debounce
FileThumbnailController::preview() writes at most one FilePreviewed row per viewer per file per five minutes, because watching a video is a single deliberate act that the browser turns into dozens of Range requests. Its docblock ended by naming the route where the same act happens without an account -- PublicGroupsController::preview -- and that route logged unconditionally. Five requests for the same public file wrote five rows where the signed-in twin wrote one, so one visitor watching one clip buried the public half of the activity log, which is the half an operator reads to see what the outside world is doing. The window moves into a shared PreviewLog, next to PreviewKind, which those two routes already share for the same reason. Keying is unchanged for a signed-in viewer. An anonymous visitor has no account to key on, so the request IP stands in -- the same substitute ApiServiceProvider's rate limiter makes for an unauthenticated caller. It is a cache key with a five-minute life and never reaches the log, which keeps its own decision about recording an IP. Downloads are deliberately untouched and stay one row per download: each is a transfer, and DownloadAllowance::used() counts those rows to enforce a per-file cap, so swallowing one would hand out free downloads. The limit this leaves open, since the IP is a stand-in and not an identity: two anonymous visitors behind one address share a key, so within five minutes the second one's view of the same file is not recorded. That is the same trade the signed-in side has always made per account, and the alternative is the row-per-Range-request this fixes. Verified before merging: 24 passed across the public-preview and thumbnail suites on the trial-merge, which also confirms this co-exists with #1725 -- the two share both controllers and change different methods in each. With app/ reset and PreviewLog deleted, 1 failed / 9 passed. The signed-in route's existing debounce tests pass unchanged, which is what says the shared class did not move that side. request()->ip() honours the trusted-proxy configuration, so a forged X-Forwarded-For cannot defeat the window from outside. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
b6f4770795 |
Merge pull request #1731 from denkfabrik-li/fix/zip-build-failure-hygiene
BuildZipDownloadJob already draws this line in its write-failure branch: "What the requester sees stays generic: a libzip string means nothing to them and can name a server path. An operator needs the opposite, so the reason goes to the log instead." Thirty-seven lines below it, the catch-all around the whole build stored $e->getMessage() in the row the requester polls -- and ZipDownloadsController hands that column straight back to whoever asked, clients included. A client asking for an archive of a file whose disk is no longer configured read "Disk [a-disk-that-is-not-configured] does not have a configured driver." verbatim. The reason now goes to the log with the exception class, and the row carries the same kind of sentence fail() already uses. Two more in the same method. tempnam() creates the file, and $tempFiles[] was appended only after the copy finished, so every throw in between left a zip-src- file in the system temp directory that nothing ever removed; it is now registered the moment it exists. And the copy itself was unchecked -- a copy that stops early is a truncated member added to the archive as though it were the file, so the build reports ready and the recipient gets something that opens and is wrong. stream_copy_to_stream and the flushing fclose are both checked now, and both handles close on every path. Deliberately not changed: comparing the copied byte count against files.size, which would fail perfectly good archives whenever that column is stale; the write-failure branch and its wording; and the skipped-files reporting, which still says which files and why, so only the catch-all went generic. Verified before merging: 37 passed on the trial-merge, 2 failed / 35 passed with app/ reset. The leak was confirmed at the consuming end rather than inferred -- ZipDownloadsController:169 returns the error column to the requester. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
037439e1f2 |
Merge pull request #1730 from denkfabrik-li/fix/provisioning-over-deleted-address
The unique index on users.email spans soft-deleted rows -- AvailableEmailRule is built on exactly that -- so a deleted account keeps its address until erasure removes the row. The registration form learns this from validation. The machine paths have no form to validate: a directory or an identity provider hands over an address and ClientProvisioning::provision() inserts it, so a client deleted earlier signing in through a provider that may auto-provision got a QueryException, and what the person met was a 500 in the middle of their sign-in. Same shape through LDAP at POST /login. Both provisioners now ask ClientProvisioning::addressIsFree() first and refuse. The social flow reuses the refusal it already gives every other identity it cannot provision -- "There is no account here for that address." -- which is also all a stranger should learn: whether an address was once an account here is not the provider's to publish. The LDAP flow falls through to the ordinary failed sign-in. The deleted account is deliberately not resurrected and not linked. Restoring one because a directory still lists the address is a decision for a person, not a side effect of somebody signing in -- and a linking shortcut here would be an account takeover. Everything about an address belonging to a live account is untouched. Verified before merging: 47 passed on the trial-merge, 2 failed / 45 passed with app/ reset. addressIsFree() queries withTrashed(), the same span as the unique index it protects, so the check and the constraint agree. Worth noting that both new warning lines record the email address, which is consistent with what these paths already log but is PII in the application log. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
6b99e37d01 |
Merge pull request #1729 from denkfabrik-li/fix/api-surface-by-route
Two places asked "is this the API?", and each got it wrong in the opposite direction.
EnsureCapability asked $request->expectsJson(). Whether a feature exists in this installation's edition is a property of the installation, not of what the caller is willing to parse: the same capability-gated API route answered 403 capability_unavailable to Accept: application/json and a bare 404 to Accept: */*, which is curl's default, while routes/api.php promises the 403 in as many words. The mirror image was worse -- an Inertia visit to a capability-gated web screen accepts JSON, so it took the API branch and announced the feature by name, where the whole point of the 404 is that an unavailable feature is absent rather than teased.
ProblemDetails asked $request->is('api/*'). Two staff pages live under that prefix -- the API dashboard at /api and the OpenAPI reference at /api/docs, both from routes/web.php -- so a signed-out visitor to /api/docs got a 401 problem+json telling them to send a Bearer token instead of the login redirect every other page gives.
One question now, asked once, in App\Support\ApiSurface: under the API prefix, and not part of the web middleware group. The group is what actually separates the two surfaces -- sessions, cookies and CSRF on one side, tokens on the other -- and it keeps answering correctly for a future /api/v2 without being edited. An unmatched path has no route to ask and stays the API's answer, which is what the existing "a missing API route is a problem+json 404" test pins. EnsureStaff keeps its expectsJson() check: there the question really is about the caller.
Verified before merging: the discriminator was checked in the running application rather than assumed -- /api/docs and /api resolve to [web, auth, staff], /api/v1/files to [api, auth:sanctum, api-active, staff-token, token-can:...]. 12 passed on the trial-merge; with app/ reset and ApiSurface deleted, 3 failed / 9 passed, every new test and no old one. Wider suites green: tests/Feature/Api 239 passed, tests/Feature/Platform 485 passed. scramble:export reproduces main's docs/api/openapi.json byte for byte. Worth recording that the blast radius here is the shape of a refusal and never whether one happens: both call sites run after authentication and authorization.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
f676e09bb2 |
Merge pull request #1728 from denkfabrik-li/fix/expiry-timezone-drift
The edit screen is given a file's expiry as a calendar date read back in the viewer's own zone -- deliberately, or "a file set to expire on the 12th reopens showing the 11th". Every save posts that date back, touched or not, and update() derived a fresh instant from it every time. So the expiry drifted by the difference between two people's zones on any other edit: a file set from Pacific/Auckland moved 19 hours later the moment somebody in Buenos Aires renamed it, and moved again on the next save from a third zone. A file could quietly outlive the expiry somebody set for it, through an edit that had nothing to do with expiry. The instant is now re-derived only when the posted date differs from the one the form was given, compared against the same string through a named pair: expiryDateFor() renders it, expiryInstant() reads it back, and the edit screen calls the render half so the two cannot drift apart. What a changed date means is unchanged -- still the end of that day in the zone of whoever changed it. bulkUpdate() needs nothing: its expiry is an explicit set / clear / no_change action, so an untouched expiry is never posted at all. Verified before merging: 22 passed on the trial-merge, 1 failed / 21 passed with app/ reset. The "a real change still lands in the editor's zone" and "clearing still clears" tests are green either way. Edge cases walked: a posted date against no stored expiry still sets it, and a posted null against a stored null leaves the column alone rather than writing. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
eb3d6e321d |
Merge pull request #1727 from denkfabrik-li/fix/api-expiry-end-of-day
FilesController::expiryInstant() exists because a calendar day ends where the person naming it lives: the web form posts a bare YYYY-MM-DD, which Eloquent would otherwise store as midnight UTC, so "expires on the 12th" would cut the file off partway through the 11th for anyone in the Americas. PATCH /api/v1/files/{id} took the same field, validated it as a date, and stored it exactly as it arrived -- so the same value that meant end-of-the-12th on the web meant start-of-the-12th over the API, and earlier still for a caller west of Greenwich.
A bare YYYY-MM-DD now means the end of that day in the caller's timezone, through the same LocalDay::end() the web path uses. A value carrying a time is unchanged: that is an instant the caller named on purpose, the API can express one where a date input cannot, and it is stored as it arrives. null still clears the expiry, and the validation rule and permission gate are untouched.
Note for the release notes: this lengthens the life of a file whose expiry an existing integration sets with a bare date, by up to a day. That is the correct meaning and the one the web has always had, but it is a behaviour change for callers who were relying on the old one.
Verified before merging: 19 passed on the trial-merge, 1 failed / 18 passed with app/ reset. The timestamp and clearing tests are green either way. The bare-date branch is gated on a strict ^\d{4}-\d{2}-\d{2}$ match, so nothing else takes it. scramble:export on the merged tree reproduces the committed docs/api/openapi.json byte for byte.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
d89807b237 |
Merge pull request #1726 from denkfabrik-li/fix/rendition-cleanup-independent
FileDiskCleanup::delete() wrapped two deletions in one try: the original upload, on whatever disk the row names, and every cached rendition, which is always on the local files disk. Storage::disk() throws outright for a name with no configured driver -- precisely the state the original's disk is in whenever this fails at all -- so the catch swallowed it and the renditions were never reached. Nothing looks for them afterwards: OrphanFileScanner skips the rendition directories on purpose, as derived artifacts rather than orphaned uploads. A file whose external disk had been removed or renamed therefore kept every cached copy of itself indefinitely on the disk that still worked, including the client-facing ones, which for a shared image may be the only copies anyone ever generated. The two attempts are now separate, each with the tolerance the class was written for: a storage failure still never turns a delete click into a 500, and the warning is still the whole report. Also corrected: File::booted() justified deferring the byte removal with "the worst case is bytes left on disk with no row, which OrphanFileScanner already finds and reports". That is not this path -- the row is soft-deleted, and knownPaths() counts a trashed row's path as claimed, deliberately, so a scan never offers to double-adopt a file still inside its erasure grace period. The comment now says what actually happens, which is that FileDiskCleanup's warning is the only record. Verified before merging: 8 passed on the trial-merge, 1 failed / 7 passed with app/ reset. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
7ff2674e4f |
Merge pull request #1725 from denkfabrik-li/fix/rendition-written-atomically
Both thumbnail routes treat "the file exists" as "the rendition is cached", and nothing ever invalidates one: RenderedImageCache::flush() runs on ImageRenderingChanged, which no code in core raises. Whatever sits at the path is what every later viewer gets. ThumbnailGenerator::generate() encoded straight onto that path, so a render that died partway -- a full volume, a killed worker -- left a half-written file that was then served as the rendition indefinitely, and two requests rendering the same file at once encoded into the same path together. Write side: the image is written beside its destination and renamed into place. rename() within a directory is atomic and replaces what is there, so the path holds either the previous rendition or a complete new one, and the loser of a race leaves a whole image rather than a mixture of two. Renditions always cache on the local files disk and the generator is handed $disk->path(), so both files are on the same filesystem and the atomicity is real. Read side: an empty file is not a rendition, so both routes replace one rather than serve it -- writing through a temporary file means core can no longer create that state, but an installation that ran an older version can already have it on disk and nothing else will ever clear it. The cache itself is unchanged: a non-empty rendition is still reused without further checks, because decoding every cached image on every request to prove it is intact would cost the cache its point. The RenderingImage seam still fires before the encode. Verified before merging: 14 passed on the trial-merge, 2 failed / 12 passed with app/ reset. The third test, about the generator's own temporary file, passes either way and the PR says so rather than leaving it to be found. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
262cb2457a |
Merge pull request #1723 from denkfabrik-li/fix/api-patch-custom-fields
Api\ClientsController::update() states the rule eighteen lines above the bug: "PATCH semantics, unlike the web form which always submits every field: an absent key means 'leave alone', not 'clear'." Every column obeyed it. The custom fields did not -- they went through saveCustomFieldValues(), which is create()'s pass: it walks every field there is and writes null for the ones the request did not carry. A PATCH naming one field emptied all the others, with nothing in the response to say so and no second copy of the value anywhere. The write pass is still shared but is now entered two ways: create() keeps writing every field, and update() writes only the fields the request named. Creating a client is deliberately unchanged -- it is not a partial update, and a checkbox nobody ticked is a recorded "no" rather than an absent row. Clearing a field by naming it with an empty value still clears it, and the validation rules are untouched. Verified before merging: 23 passed on the trial-merge, 1 failed / 22 passed with app/ reset. The two guard tests -- a named empty value still clears, create still records every field -- are green either way, so the write path was not simply switched off. The keys reaching whereIn() are stripped to real field ids by validateCustomFieldValues() before they get there. scramble:export re-run on the merged tree produces a docs/api/openapi.json identical to main's, so the published spec does not move. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
a285f86b93 |
Merge pull request #1722 from denkfabrik-li/fix/portal-dashboard-visible-files
DashboardController::clientDashboard() built its own whereHas('assignments') query instead of using File::scopeVisibleToClient -- "the single source of truth for client file access", as that scope's own docblock puts it. The copy reproduced the assignment half and stopped there, so the page disagreed with the portal it introduces, in both directions. Over: the scope ends in notExpired(), so an expired file was gone from /my-files and refused on download while the dashboard went on counting it and printing its name. Under: a file in a folder shared with the client, a file the client uploaded through the portal themselves, and a revision -- which owns no assignment row and inherits its original's recipients through SharingIdentity -- were all missing from the count and the list.
The hand-rolled query is gone and the scope is used, one query object cloned for the count exactly as before. groups_count and the storage figures are untouched: they answer different questions and have their own tests.
Verified before merging: 19 passed on the trial-merge, 2 failed / 17 passed with app/ reset. The existing "clients get the portal dashboard with their own numbers" test is unchanged and green either way, so a directly assigned live file counts as it always did. PHPStan level 8 clean on the changed file.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
bc68a24ef5 |
Merge pull request #1721 from denkfabrik-li/fix/api-dashboard-activity-log-scope
ApiUsage::recentActions() read the activity log without ActivityLogScope::apply(). It was the only ActivityLog::query() outside ActivityLogger and AccountEraser that skipped it. Its only boundary was view_actions_log -- the permission ActivityLogScope's own docblock says "is not the whole answer for a client-scoped staff member", because a log row carries the subject's name. The Client Manager system role is client_scoped and ships with that permission, so this was the default configuration and not an exotic one: the same person who gets a 403 on a file and an empty /activity read that file's name off /api?all=1. ApiUsage now takes ActivityLogScope and applies it to the recent-actions query, on both sides of the install-wide branch rather than only in the install-wide arm -- the own-actor filter already stays inside what the scope allows, and a boundary that exists in only one arm of an if is one refactor away from not existing. The token inventory, request counts and endpoint table keep ApiUsageScope alone: those rows are about the viewer's own credentials rather than library content. Verified before merging: 17 passed on the trial-merge and 1 failed / 16 passed with app/ reset. The two tests guarding against narrowing further than /activity does -- a viewer's own actions stay whole, an unscoped viewer's feed is unchanged -- are green either way. ActivityLogScope::apply() wraps its conditions in a single where(Closure), so it composes with the origin and actor_id filters around it without a precedence trap, and ApiUsage is never constructed with new, so the added dependency is wired by the container everywhere. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
1644d634d5 |
Merge pull request #1720 from denkfabrik-li/fix/group-reach-expired-file
groupReachesNoFurther() decides whether a client-scoped staff member may edit a group, by asking whether anything shared with it sits outside their library. |
||
|
ignacionelson
|
abbe9a3acc |
Merge pull request #1719 from denkfabrik-li/fix/group-reach-subtree
StaffLibraryScope::groupReachesNoFurther() asks whether anything shared with a group sits outside the viewer's library, and its docblock says the folder half covers "the folders whose subtrees it can browse". It compared the folder ids the assignment names and stopped there. But 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. A scoped rep could add their own client to a group holding a folder they own, and a stranger's file inside it went to that client, and then into the rep's own library, because files() is "own uploads plus everything my clients can see". That is exactly the widening the first test in the file exists to refuse. The folder half now walks each assigned folder's subtree via subtreeFolderIds(), 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 deleted ones are -- membership grants nobody access to one, and something nobody can reach is not reach. Verified before merging: 27 passed on the trial-merge, and 2 failed / 25 passed with app/ reset to main. The "subtree wholly inside the library stays manageable" test is green either way, which is what says the guard was tightened rather than closed. subtreeFolderIds() walks a materialised path prefix, so it is one query per assigned folder with no recursion. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
3dc407a777 |
Merge pull request #1718 from denkfabrik-li/fix/reassign-candidates-scope
reassign_candidates is the delete dialog's picker -- every active account in the installation, by name and role label -- and it was narrowed by nothing. Two lines above it on the clients index sits the listing itself, narrowed through StaffLibraryScope with a comment saying why. A client-scoped rep with manage_clients therefore read the name and role of every client in the installation, including the ones they can reach nothing of. The can('delete_clients') filter meant to hide the picker runs in React, which decides what is rendered, not what is sent.
The client half of the candidate list now goes through the same StaffLibraryScope as the listing beside it, and each screen sends the picker only to a viewer holding the delete permission it exists for. Staff accounts are not narrowed, here or anywhere else in the application. Privacy settings keeps the whole installation deliberately: that picker sets the erasure default stored once for everybody, behind edit_settings, so narrowing it by whoever happens to be editing would store the wrong answer.
Verified before merging: the four new tests pass on the trial-merge and go 3 failed / 16 passed with app/ reset to main, so they are testing the fix and not something else. Every call site of the changed candidates() signature was checked.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
d8ef21bb6a |
Say when the worker check was skipped rather than skipping it quietly
ensure_worker_watches_zips reads the unit file with `systemctl show -p FragmentPath`, and an empty answer meant an immediate, silent return. The common cause is a mistyped --worker: systemd does not know the unit, the check never runs, and the operator finishes the update believing their worker was inspected. Which produces precisely the outcome the function exists to prevent. Its own comment says a worker that does not watch the zips queue finishes no zip downloads while cheerfully sending every email, and that nothing says why. Skipping the check in silence is a quieter way to arrive there. It now warns, names the consequence, and says what to check. The read-only case is separated out too: a unit file somebody else owns cannot be repaired, but it can still be read, so a worker that is missing the queue is diagnosed rather than passed over. Worth recording why this was looked at. The portal session found a deploy script that had printed `next run` followed by nothing for its whole life, because `systemctl show` answers an unknown property with an empty value and a zero exit -- a line always blank is worse than no line, since somebody believes a check is being performed. FragmentPath here is correct, verified against a real unit; the failure was the same shape one step further on, in what an empty answer was taken to mean. |
||
|
ignacionelson
|
479dc61d2d |
Move branding into core, and leave white-labelling behind
Logo and watermark belonged in the private package for one reason: that is where they were written. Nothing about them needs a hosted platform, and an installation wanting its own mark on the pages it serves is the ordinary case rather than the exotic one. They are core's now, and every installation has them. Hiding "Powered by ProjectSend" did not come. That is what a hosted customer pays for, and its gate is not a capability key but the absence of the code: cloud-modules keeps the listener, so an installation without that package holds the column and has nothing able to read it. Flipping an edition variable buys nothing, which was true before and stays true. Core renders the switch where Capability::AttributionHide is held and has no route that can save it -- there is a test asserting exactly that, which fails the day white-labelling quietly becomes free. The migrations move with their original filenames on purpose. A Cloud tenant already ran them under those names, so Laravel skips them there and the table and its data are untouched; a fresh install or a community one runs them from here for the first time. What got better on the way rather than merely moving: The watermark listeners take core's real RenderingImage and ResolvingImageRendering instead of duck-typed `object` payloads, and the tests construct the genuine events rather than anonymous stand-ins that imitated their shape. The package had to do it that way -- it builds with no host present -- so three PHPStan ignore entries existed to describe what the type system could not see. They are gone. ModuleBoundaryTest asserted "branding is cloud-only, and the suite runs as community", which was never what it was testing. It now reads the capability off the route and subtracts it, so the invariant holds for whichever module is installed. The 43 branding strings arrived in all sixteen locales from the package's own catalogues rather than being retranslated, and the package's are pruned to the one string it still uses. A hosted plan without branding subtracts branding.customize and attribution.hide from the instance's environment. The row is never deleted by that: a downgrade is usually an expired card rather than a decision, and wiping somebody's artwork over a billing event is a loss they would find weeks later with no way to know what it used to be. Hiding reverses; deleting does not. |
||
|
ignacionelson
|
530f30606d |
Let a plan take a capability away, and split branding from white-labelling
Groundwork for moving Branding out of the private package. Two changes, both about who decides what an installation may do. An edition grants capabilities; an operator may now take some away, via PROJECTSEND_CAPABILITIES_DISABLED. Subtractive only, and that asymmetry is the whole design: a variable that could *add* would put the hosted edition's proprietary screens one line of .env away on every self-hosted install, which is not a gate at all. So the list is intersected with what the edition already allows and can only make the answer smaller. This is not the plan tier core has always refused to invent. There are still no billing tiers here to key off -- the objection config/api.php makes about rate limits stands. It is the operator stating a fact about this installation, exactly as PROJECTSEND_PLATFORM_MAX_STAFF_USERS does for seats: the platform knows what it sold, the installation is told and enforces. Unknown keys are ignored rather than fatal, because the variable outlives both the plan that wrote it and the release that named the key, and refusing to boot over a stale one would be an outage on upgrade day. The registry takes the list as a constructor argument rather than reading config itself, which keeps it a value object testable without an application -- the failure that surfaced it was a unit test with no container. And branding.customize is now both editions, with the white-label half split into attribution.hide, which stays Cloud-only. Dressing an installation in its own logo is not a hosted concern; taking ProjectSend's name off somebody's public pages is what a hosted customer pays for. The gate on the second is not the key but that the only code able to answer "hide it" ships in the private package, so flipping an edition variable buys nothing. EnsureCapabilityMiddlewareTest had to pick a new Cloud-only example for the second time -- branding after users.manage. It now uses storage.managed, and records what to ask if it ever needs a third. The code move itself is the next commit; nothing user-visible changes yet, because the screens still live in cloud-modules. |
||
|
ignacionelson
|
afc2c74617 |
Say who depends on the activity log never being pruned
last_staff_login_at is a MAX() over activity_log, and the docblock already said the log is never pruned. It did not say that anything depends on it. Something does now: the hosted platform warns, pauses and finally removes a free instance nobody has signed in to, counting from this field. So retention or pruning added to activity_log would break nothing here -- every test would pass, the field would keep answering, and old installations would quietly start looking dormant to the process that deletes them. That is the shape of failure worth naming in advance, because the person adding a retention policy would have no reason to look at this file. Same note as the one on SeatAllowance's counting rules and on ManagedStorageBackend::describe(): an assumption with a reader outside this repository is a contract, and the place to record it is where somebody would otherwise change it. |
||
|
ignacionelson
|
d62c62f788 |
Let an installation say which build it is
A version string is a decision somebody made. A commit is a fact, and the two come apart exactly when it matters: an image built from the tag and one built from the branch that tag sits on carry the same version and different code. The fleet spent a day reporting 2.2.0 from images that were not the released 2.2.0, and nothing inside any of them could have said so -- which is why 2.2.1 was cut for a control plane rather than for users. So every artifact now carries config/build.php, written by build-release.sh and never committed, and projectsend:status reports it as `build`: the commit, the ref it describes to, the channel and the build time. All four are null on a source checkout, because there is no such file there. That is the honest answer rather than a missing one -- "I was not built" and "I will not say" are different facts, and this file's whole null discipline exists because a reader that cannot tell them apart eventually acts on the wrong one. An empty string is treated as no answer for the same reason: a build step that ran and produced nothing must not read as "answered" to anything checking presence. |
||
|
ignacionelson
|
2029309126 | Release 2.2.1 v2.2.1 | ||
|
ignacionelson
|
d83d2d9acb |
Translate the three strings the last release cycle added
Two seat counters and one folder-delete refusal, in all sixteen locales. Additive only: nothing already in a catalogue was reordered or reworded, so the diff is three lines per file. Polish, Czech and Russian get three plural forms where the English has two. Those languages inflect a noun by the number in front of it -- one case for 2-4, another for 5 and up -- and the framework's selector picks between three segments for them, so writing only the English pair would have produced "5 pliki" where it has to be "5 plikow". Verified through trans_choice at 1, 3 and 7. |
||
|
denkfabrik-li
|
a1773cad5e |
Count a shared folder's contents as reach, not just the folder
groupReachesNoFurther() asks whether anything shared with a group sits outside the viewer's library. Its docblock says the folder half covers "the folders whose subtrees it can browse". It compares the folder ids the assignment names and stops there. A folder shared with a group hands its members the whole subtree -- File::scopeVisibleToClient matches on folder placement, and a folder is visible to a client when it or an ancestor is shared with them. So the guard passed on a subtree it had never looked into. Measured on main: a scoped rep's own folder, a subfolder somebody else created inside it, and that person's file in the subfolder. parent in the rep's library true subfolder in it false the file in it false add their own client to a group holding the parent 302, allowed the client can then reach the file true And because files() is "own uploads plus everything my clients can see", the file lands in the rep's own library on the next request. That is the widening this guard exists to refuse -- the first test in the file is called "a scoped staff member cannot widen their own library through a group". The folder half now walks each assigned folder's subtree, and the files inside it are checked too: a folder can be in the library while a file in it is not, since somebody else's upload into a folder this rep owns is neither their own nor their clients'. Expired files are skipped for the reason the deleted ones are -- membership grants nobody access to one. Three tests: the subfolder case, the stranger-file case, and a subtree wholly inside the library, which stays manageable. The first two go red without the fix. |
||
|
denkfabrik-li
|
776d3d99f4 |
Put a client on the roster of the scoped staff member who created them
A client-scoped staff member with create_clients creates a client and loses it immediately. guardTarget() answers 404 for anything off their roster, and StaffLibraryScope::clients() leaves it out of their list -- so the record exists, is logged, is welcomed by email, and is invisible to the person who made it. store() redirects to the edit page, which is where they land: POST /clients → 302 → /clients/4 on_creator_roster → false GET /clients/4/edit → 404 clients listed → ["Mine"] the new client is not there Their own roster is where a client they created belongs, so it is attached there. An unscoped creator gains nothing: they see every client already, and a roster entry would change what assignedClients means for them. The API twin does the same, for the same reason -- a scoped token gets a 404 from every route that binds the client it just created. Three tests: the scoped creator can open and list the client, an unscoped creator gains no roster entry, and the API twin behaves like the web. The first and third go red without the fix. |
||
|
denkfabrik-li
|
9ddd39c41d |
Refuse to provision over a deleted account's address instead of crashing
The unique index on `email` spans soft-deleted rows -- AvailableEmailRule is built on exactly that, so a deleted account keeps its address until erasure removes the row. The registration form learns this from validation. The machine paths have no form: a directory or an identity provider hands over an address and provision() inserts it. Measured on main, a client deleted last week signing in through a provider that may auto-provision: GET /auth/google/callback → 500 (QueryException, unique constraint) Same shape through LDAP at POST /login. Nothing is created, nothing is signed in, and what the person meets is a server error. Both provisioners now ask ClientProvisioning::addressIsFree() first and refuse. The social flow already has a refusal for an identity it cannot provision -- "There is no account here for that address." -- which is also all a stranger should learn: whether an address was once an account here is not the provider's to publish. The LDAP flow falls through to the ordinary failed sign-in. Deliberately not resurrecting the deleted account. Restoring one because a directory says the address exists is a decision for a person, not a side effect of somebody logging in. Two tests, one per path: the sign-in is refused, nothing is created, and the trashed row is still trashed. Both go red without the fix. |
||
|
denkfabrik-li
|
19c449ee20 |
Stop an editable-once checkbox locking before anybody ticks it
save() writes '0' for an unticked checkbox, and filled('0') is true in
Laravel -- so isLocked(), which asks whether anything is stored, locked
the field the first time the client saved the page it sits on, whatever
they had chosen. A box they never ticked could then never be ticked, and
the one edit the setting promises was spent on a decision they had not
made.
A text field left empty stores null and stays open. That asymmetry is the
bug: '0' is the absence of a decision, which is what null means for every
other type.
So a checkbox locks on a stored '1' and nothing else. Everything else is
unchanged, including the existing case of a client ticking the box and
then being unable to untick it.
Two tests: an unrelated save leaves the box open and the tick that follows
still lands and locks it; and an editable-once text field behaves exactly
as before. Without the fix the first goes red.
|
||
|
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
|
250e8664d3 |
Stop a client PATCH clearing custom fields it never mentioned
update() states the rule eighteen lines above the bug: "PATCH semantics, unlike the web form which always submits every field: an absent key means 'leave alone', not 'clear'." Every column obeys it. The custom fields did not, because they went through create()'s pass, which walks every field there is and writes null for the ones the request did not carry. Two fields filled, a PATCH naming one: status → 200 named field → "Robin" the other one → null (was "ATU12345678") Nothing says so in the response, and there is no other copy of the value. The write pass is now shared but entered two ways: create() keeps writing every field, since a new client has no values and a checkbox nobody ticked is a recorded "no"; update() writes only the fields the request named. Three tests: the untouched field survives, a named empty value still clears, and create still records every field. Without the fix the first goes red. |
||
|
denkfabrik-li
|
640c5db591 |
Stop an expiry moving because somebody else saved the file
The edit form is given a file's expiry as a calendar date, read back in the viewer's own zone -- deliberately, so a file set to expire on the 12th does not reopen showing the 11th. Every save posts that date back, whether or not anybody touched it, and update() derived a fresh instant from it every time. So the expiry drifts by the difference between two people's zones on any other edit. A date set from Pacific/Auckland stores 2026-09-12T11:59:59Z; a colleague in UTC-3 opens the file, sees the same 12th, renames it, and the file now expires at 2026-09-13T06:59:59Z -- 19 hours later, with nobody having gone near the date. The instant is now re-derived only when the posted date differs from the one the form was given, compared against the same string through a named pair: expiryDateFor() renders it, expiryInstant() reads it back. The edit screen uses the same method it is compared against, so the two cannot drift apart. bulkUpdate() needs nothing: its expiry is an explicit set/clear/no_change action, so an untouched expiry is never posted in the first place. Three tests: the rename leaves the instant alone, a real change still lands in the editor's own zone, and clearing still clears. Without the fix the first goes red. |
||
|
denkfabrik-li
|
e1cd010f9d |
Give an API expiry date the same meaning the web gives it
FilesController::expiryInstant exists because a calendar day ends where the person naming it lives: the web form posts a bare YYYY-MM-DD, and storing that as it arrives would cut a file off at midnight UTC -- "expires on the 12th" ending partway through the 11th for anyone in the Americas. The API takes the same field, validates it as a date, and stores it raw: web → 2026-09-12T23:59:59+00:00 (end of the day, as the docblock means) API → 2026-09-12T00:00:00+00:00 (raw) Same value, same field, same file, two meanings -- and the earlier of the two is a file that dies at the start of the day it was promised. A bare date now means the end of that day in the caller's timezone, as it does on the web. A value carrying a time is unchanged: it is an instant the caller named on purpose, the API can express one and a date input cannot. The endpoint's docblock says both, so the OpenAPI document does too. Three tests: the day, the timestamp, and clearing. Without the fix the first goes red. |
||
|
denkfabrik-li
|
c2dd2c758a |
Debounce the public preview log the way the signed-in one already is
FileThumbnailController::preview() writes at most one FilePreviewed row per viewer per file per five minutes, because a browser turns one video into a long tail of Range requests against the same URL. Its docblock names the anonymous route as the place the same act happens without an account -- and that route logs unconditionally. Measured: five requests for the same public file, five PublicFilePreviewed rows, against one for the signed-in twin. One visitor watching one clip buries the public half of the activity log, which is also the half an operator reads to see what the outside world is doing. The window is now a shared PreviewLog, next to PreviewKind, which the two preview routes already share for the same reason. Keying is unchanged for a signed-in viewer; an anonymous one has no account to key on, so the request IP stands in -- the same substitute the API's rate limiter makes for an unauthenticated caller. It is a cache key with a five-minute life and never reaches the log, which keeps its own decision about recording an IP (ActivityLogger::shouldRecordIp, Setting::DownloadIpLogging). Three tests: the replay is one row, two visitors are two rows, and the window is per file. Without the fix the first goes red. |
||
|
denkfabrik-li
|
9d4b096c19 |
Narrow the reassignment picker to what a viewer may see
`reassign_candidates` is the delete dialog's picker: every active account
in the installation, by name and by role label. The same list is shared
on the clients index, the users index, both edit screens and privacy
settings, and it was narrowed by nothing.
Two lines above it on the clients index sits the listing itself, narrowed
through `scope->clients($viewer)` with a comment saying why: "a
client-scoped staff member is not shown the name and email of somebody
they can reach nothing of". The picker beside it handed over every client
in the installation, plus every staff account and its role name. The
filter by `can('delete_clients')` happens in React, which decides what is
rendered, not what is sent.
So the client half of the candidate list goes through the same
StaffLibraryScope as the listing, and each screen sends the picker only to
a viewer holding the delete permission it exists for. Staff accounts are
not narrowed -- they are not narrowed anywhere else either -- and an
unscoped viewer's list is unchanged, because StaffLibraryScope::clients()
returns every client for them.
Privacy settings keeps the whole installation on purpose: that picker sets
the erasure default stored once for everybody, behind edit_settings, so
narrowing it by whoever happens to be editing would store the wrong
answer. The parameter is nullable for that one caller, and the docblock
says so.
Four tests. Without the fix three go red; the fourth is the guard that an
administrator still sees every active account.
|
||
|
denkfabrik-li
|
f424fe5365 |
Decide what is an API request from the route, not from the caller's headers
Two places asked "is this the API?" and got it wrong in opposite ways.
EnsureCapability asked $request->expectsJson(). Whether a feature exists
in this installation's edition is a property of the installation, not of
what the caller is willing to parse, so the same route answered
differently per header: `Accept: application/json` got the 403
`capability_unavailable` routes/api.php promises, `Accept: */*` -- curl's
default -- got a bare 404 `not_found`. The mirror image is worse: an
Inertia visit to a capability-gated *web* screen accepts JSON, so it got
403 with Laravel's default error body, naming the exception class, where
the point of the 404 is that an unavailable feature is absent rather than
teased.
ProblemDetails asked $request->is('api/*'). Two staff pages live under
that prefix -- the API dashboard at /api and the OpenAPI reference at
/api/docs, both registered in routes/web.php -- so a signed-out visitor to
either got 401 problem+json, "Send a valid API token in the Authorization
header as \"Bearer <token>\"", instead of the login redirect every other
page gives them.
Both now ask App\Support\ApiSurface: under the API prefix, and not part of
the `web` middleware group. The group is what actually separates the two
-- sessions and CSRF on one side, tokens on the other -- and it keeps
answering correctly for a future /api/v2 without being edited. An
unmatched path has no route to ask, which is the API's answer anyway: a
404 under its prefix is one it should describe in its own format, and the
existing test for that stays green.
Three tests, in the two files that already own these rules. Without the
fix all three go red.
|
||
|
denkfabrik-li
|
cd8da6a117 |
Name the quota a client is actually held to when an upload is refused
Both chunked-upload quota checks resolve the limit through ClientStorageUsage::quotaBytes(), which falls back to the site default when a client has no quota of their own -- and then print `$user->storage_quota_mb` in the rejection. For every client who was never given an explicit quota that column is 0, so the message reads "This upload would exceed your storage quota of 0 MB." at the one moment somebody is trying to find out what their limit is. The API's single-request upload already prints `$this->storageUsage->quotaMb($user)` for the same sentence (Api/FilesController.php:208). The two chunked copies now do the same. Three tests: the inherited default is named at session creation and again at completion, and a client with a quota of their own still sees their own number. Without the fix the first two go red, the third stays green. |
||
|
denkfabrik-li
|
db1dd71f3c |
Stop an expired file locking a group shut for a scoped staff member
groupReachesNoFurther() asks whether anything shared with a group sits
outside the viewer's library. `f1b35cc9` established the shape of the
answer for deleted files: start from the live row, because "a deleted file
is not reach, because nobody can reach it".
An expired file is the same case. Membership grants nobody access to it --
File::scopeVisibleToClient ends in notExpired(), so it has left every
member's /my-files and the download answers 403 -- but it is equally gone
from files(), where its absence reads as "outside my library". The group
then locks for a scoped staff member: they cannot add a member, cannot
rename it, and cannot remove their own client again.
So the reach query skips expired files as it already skips deleted ones.
Expiry is reversible where deletion is not, and that needs no special
handling: the guard asks what is reachable at the moment somebody is added
or removed, and the file counts again the moment it stops being expired.
Not changed: File::scopeVisibleToClient, whose treatment of expiry was
settled deliberately in
|
||
|
denkfabrik-li
|
cb53120779 |
Show the portal dashboard the files a client can actually open
clientDashboard() restates the assignment half of File::scopeVisibleToClient in a whereHas of its own. The scope is the single source of truth for client file access and ends in notExpired(), which the copy leaves off, so the two disagree in both directions. Over: an expired file stays counted and keeps its name on the dashboard after /my-files has stopped listing it and the download answers 403. Under: everything that reaches a client another way is missing -- a file inside a folder shared with them, a file they uploaded through the portal themselves, and a revision, which owns no assignment row at all and inherits its original's recipients through SharingIdentity. Replaced by the scope itself, which is what /my-files runs. The existing test for the page is unchanged and still passes: a directly assigned, unexpired file counts exactly as before. Two tests, one for each direction. Without the fix both go red. |
||
|
denkfabrik-li
|
84e9f6e2fe |
Scope the API dashboard's recent actions to what the viewer may read
ApiUsage::recentActions() is the only ActivityLog query outside ActivityLogger and AccountEraser that does not run through ActivityLogScope::apply(). Its whole boundary is view_actions_log -- the permission whose own scope class says, in as many words, that it "is not the whole answer for a client-scoped staff member". The Client Manager system role is client_scoped and ships with that permission, so this is the default configuration. Such a viewer opening /api?all=1 reads the fifteen most recent API log rows for the entire installation, each with its subject_name: the names of files and clients they get a 403 on. /activity, the download history and the dashboard's recent-activity widget all narrow the same rows; the API dashboard was missed. The scope is applied on both sides of the install-wide branch. The own-actor filter for the narrow view already stays inside what the scope allows, and a boundary that exists in only one arm of an `if` is one refactor away from not existing. Three tests: the scoped viewer sees only the entry about a file in their library, their own actions stay whole even when the subject is outside it, and an unscoped viewer's feed is unchanged. Without the fix the first goes red; the other two are green either way and guard against narrowing too far. |
||
|
ignacionelson
|
06c364d29a |
Report storage, health and what packages loaded in projectsend:status
Five more facts for whatever watches an installation from outside the container, and one seam so a package can add its own. Storage is the one that was about to be wrong. It is summed from the rows that record it, not measured on the volume: measuring the directory was correct until external storage went live and silently stopped being, since an upload that resolves to a bucket leaves nothing on disk to measure. A figure taken from the filesystem freezes while the account keeps filling, and on a managed installation that figure is what a customer is shown and billed against. `by_disk` splits the same sum by where the bytes went, which is the only way to see what is still sitting locally from before a cutover. Trashed files are excluded because they hold no bytes -- File's deleted hook takes them. Health is what a container cannot show from outside. A queue worker dying is invisible to anything watching the process: it is still up, and zips quietly stop building while mail stops going out. Same for a deploy whose migrations failed -- the application answers every request and is a schema behind. An unreachable queue reports null rather than zero, because an unreachable Redis is not an empty queue and reading the second as the first is how a dead worker looks healthy. The two-factor enforcement setting is echoed back the way EnforceTwoFactor reads it, fallback included: reporting a stricter rule than the middleware actually applies would be worse than reporting none. And ResolvingInstallationStatus, so a package can report what core cannot know. The managed storage backend and the version of the package providing it live in cloud-modules, which this repository must not reference, and a platform that writes eight environment variables only ever knows what it asked for. Those came apart once: a bucket provisioned, a token minted, every variable correct, and an image whose copy of the package predated the module that reads them. Files went to local disk with the configuration sitting perfectly right beside them. Two shapes are cast to objects deliberately. An empty PHP array encodes as [], so an installation with no packages -- or holding no files -- would answer a map-shaped field with a list, and a reader unmarshalling it breaks on the day it happens to be empty rather than the day it is written. There is a test for each. Requested by the ProjectSend Cloud control plane, whose storage figure stops growing the moment a tenant's uploads start reaching the bucket. |
||
|
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. |
||
|
Ignacio Nelson
|
4a35c25894 |
Merge pull request #1717 from denkfabrik-li/fix/deleted-client-comment-context
file_comments.client_context_id is cascadeOnDelete, but users are soft-deleted, so the cascade never fires: the column goes on pointing at a row that is still there while the relation resolves to null. resolveClientContext() branched on the relation, so "this is Alice's conversation" read as "this has no conversation" -- and a null context on a clients comment is the branch every client on the file reads. A staff reply into a departed client's private thread became a circular, and canAssignClient() was skipped on the way. That is the invariant docs/feature-comments.md calls the rule everything hangs off: a clients comment carrying client_context_id = C is never returned to any non-staff viewer other than C, because one customer learning another exists is worse than leaking a comment's text. Verified before merging: both new tests are red on main and green here, and the three that must not move stay green either way. Suite at 2093, PHPStan clean. The second half is the same root cause through the other column. authorName() read a deleted client's comment as "Anonymous", which is what a visitor's comment looks like -- and a visitor's comment is governed by different rules, so the two must not be able to look the same. Whether the author is a visitor is now decided by author_id alone, the question isFromGuest() already asks. Accepted consequence: a soft-deleted client's name is visible on their old comments during the erasure grace period, where it previously read as Anonymous. It goes for good when erasure removes the row. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
58497ef776 |
Merge pull request #1716 from denkfabrik-li/fix/sole-administrator-self-deletion
ProfileController::destroy() validated the current password and soft-deleted, without asking guardLastAdministrator() -- the rule the other four doors ask, at the one door where the account being removed is certainly signed in. The sole administrator could empty their own installation, and EnsureSetupIsComplete, which asks exists() and so skips trashed rows, then handed the first-run setup form to whoever loaded the page next. That form creates an active System Administrator, unauthenticated. Verified before merging: on main the sole administrator's self-deletion succeeds and setup reopens; both new tests are red there and green here. Suite at 2088, PHPStan clean. Two locks, because one of these questions is asked at five doors and the other at one. The guard closes the door. And "has this installation been set up" stops meaning "does it have a working administrator right now" -- a trashed staff row is still evidence that setup happened, counted now in both the middleware and SetupController::setupIsComplete(), which have to agree or the result is a redirect loop or an open form. Worth recording: erasure force-deletes a self-deleted account after its grace period, so the second lock would expire on its own. It does not matter because the first lock stops the installation reaching that state, but a future change to either should know the other is not permanent. An installation that has already lost its last administrator now finds setup shut. That is the point: recovery is php artisan projectsend:admin, which is also how every unattended container installs itself. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
d751314196 |
Merge pull request #1715 from denkfabrik-li/fix/zip-duplicate-entries
The job walked the loose file ids and then every selected folder's subtree, adding whatever each pass found. A selection reaching the same file both ways got it twice: two copies of the same bytes, a total_size inflated by the repeat -- which is what the size cap is checked against -- and a file limited to a single download handed over in three copies while the log recorded one, because delivery logs per contained file and DownloadAllowance counts those records. Verified before merging: the three new tests fail on main and pass here. Suite at 2082, PHPStan clean. Two halves, because one fix does not cover both shapes. The added-ids list becomes a map keyed by id and the folder pass skips what is already in, before the per-file re-checks, so a duplicate does not spend an allowance twice either. And a folder sitting inside another selected folder is dropped before either is walked, which also settles which path the surviving entry keeps rather than leaving it to row order. One measured cost, accepted: the pruning compares every selected folder with every other. The pathological case -- ten thousand sibling folders, the selection cap -- benchmarks at around twenty seconds of CPU, in a background worker, on a selection that would take far longer to compress. A sort-by-path-length version would be cheaper if it ever matters. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
00d118559d |
Merge pull request #1714 from denkfabrik-li/fix/group-edit-library-scope
Every group route asked StaffLibraryScope whether this viewer may act on this group except the two that read it. So a client-scoped staff member could open the edit screen of a group they cannot change, read its membership with addresses, and get the whole client roster in available_clients besides. The API twin returned the same membership. Verified before merging: the three new tests fail on main and pass here. Two things checked beyond the report -- group membership is edited through separate, already-guarded routes, so narrowing the displayed list cannot remove anybody on save; and scramble:export regenerates byte-identical, as claimed. Suite at 2078, PHPStan clean. The fix has two halves because one guard does not cover both shapes. Reading the group now asks the same reach question the write half asks. And both lists narrow through StaffLibraryScope::clients(), because a group nobody has shared anything with reaches nowhere, stays open to everybody, and can still hold a stranger's client. Unscoped viewers are unaffected: clients() returns the whole roster for them and allowsGroupChange() is true by construction. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
abaca20261 |
Merge pull request #1713 from denkfabrik-li/fix/api-self-deactivation-boolean
The validation rule accepts 0 and "0" as well as false and does not cast, so a strict comparison against the validated array let two of the three spellings past the self-deactivation guard -- and the model's own boolean cast then stored exactly the value the guard had just decided was not a deactivation.
Reproduced on main before merging: {"active": false} is refused, {"active": 0} and {"active": "0"} both return 200 and switch the account off. Green on the branch, suite at 2074, PHPStan clean.
The fix reads the flag once with Request::boolean() and gives that same value to the guard and to the write -- the rule RolesController::guardScopeRemoval already documents for the same reason. Validation is unchanged, so the accepted inputs are the same; one of them just stops meaning two different things on its way through the method.
Follow-up for the release: this is a caller-visible change (200 to 422) and wants a line in api-changelog.md.
Reported and fixed by @denkfabrik-li.
|
||
|
Ignacio Nelson
|
b16d780ebe |
Merge pull request #1712 from denkfabrik-li/fix/storage-durability-dashboard-assertion
The test named for carrying the durability verdict to the system widget asserted only has('system'), and system is an unconditional key of the render array -- the controller's own comment beside storage_durability says as much. So the assertion could not fail.
Confirmed here by deleting the line that supplies the verdict: the new assertion fails with "Property [system.storage_durability] does not exist", where the old one stayed green.
Test-only, no application code.
Reported and fixed by @denkfabrik-li.
|
||
|
Ignacio Nelson
|
602c7bed94 |
Merge pull request #1708 from denkfabrik-li/fix/confirm-password-under-enforcement
EnforceTwoFactor exempts by route name, and only the GET half of the confirm-password screen had one -- Route::named() answers false for a null name, so the submission was never exempt. Enrolling requires password confirmation, so with enforcement on nobody could enrol at all: the form rendered, its POST was redirected to two-factor.show, auth.password_confirmed_at was never written, and every account on the installation was left with logout as its only working route. Including the administrator who turned the setting on. Reproduced on main before merging: POST /confirm-password redirects to /settings/two-factor and the session flag stays unset. The widened pattern was checked against the route table -- password.confirm* reaches password.confirm and the newly named password.confirm.store and nothing else; password.reset, password.store and the rest are not under that prefix. Exempting the submission grants nothing further, since every other route stays bounced and store() still validates the password. Reported and fixed by @denkfabrik-li. |