mirror of
https://github.com/projectsend/projectsend.git
synced 2026-10-03 12:54:18 +00:00
f676e09bb2eafd789f0702c081c7d9a295eeb2d7
151 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
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. |
||
|
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
|
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
|
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
|
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
|
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. |
||
|
ignacionelson
|
3f81dd5eab |
Merge pull request #1709 from denkfabrik-li/fix/seat-cap-approval-doors
Two doors onto the client seat cap did not ask it. Both update()
methods -- the edit screen and PATCH /api/v1/clients/{id} -- clear
account_requested when a pending client is activated, under a comment
saying that counts as approval, and approval is the moment a seat is
spent. So a managed installation sitting at its cap kept taking clients
on for as long as registrations arrived, and self-registration is open
to strangers, so the supply of pending rows is not the operator's to
control.
Verified rather than taken on trust: the two new door tests were run
against the unguarded controllers and fail there, and every place in
app/ that clears the flag was enumerated to check no third door was
missed. There is none -- the other six already ask, and a conversion
refuses a pending account outright rather than approving it sideways.
The guard sits inside the approval branch, so an installation at its cap
can still rename a client it already holds. That is pinned by a test of
its own.
Conflicted with tonight's seat work in SeatAllowanceTest, which had
added an import beside the one this adds. Resolved by keeping both;
suite green at 2065 and PHPStan clean after resolution.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
f2e7820f5c |
Say that the seat counts now have a reader outside this application
The docblock argued for one definition by describing a control plane showing "2 of 3 seats used" next to an application refusing the fourth, and the two disagreeing. That was written as a thing to avoid. As of today it is a screen: the hosted fleet console reads these numbers per tenant out of projectsend:status --json. Which makes two rules here load-bearing somewhere nobody editing this file would think to look -- a deactivated staff account still holds a seat, a client awaiting approval does not. Changing either changes what a support person is told before it changes what a customer hits, and the note is here so that is a decision rather than a surprise. |
||
|
ignacionelson
|
a92feed3ad |
Correct the fifth stale Community-only comment, in QuickStart
The quick-start list gates its "Add the rest of your team" step on
Capability::UsersManage, which is right and unchanged: it is the seam an
edition difference would travel through. The comment above it still gave
the old reason -- that a managed installation has no staff accounts of
its own to hand out -- which the capability opening on both editions
made false. The step has appeared on a managed installation's list since
|
||
|
ignacionelson
|
73d93495c9 |
Report the last staff sign-in in projectsend:status
A platform can see that an installation is running. It cannot see
whether anybody is still using it, and the difference is what separates
a customer from an abandoned free instance holding a database.
So the status probe gains one field:
"activity": { "last_staff_login_at": "2026-08-24T21:13:32+00:00" }
Null means no staff account has ever signed in, and the key is emitted
either way. That is the whole care in this change: "they said never" and
"we got no answer" have to stay distinguishable, because collapsing them
is how a broken probe reads as a dormant fleet.
Only interactive sign-ins count. Laravel's Login event does not fire for
token authentication, so an integration polling every hour cannot make
an empty installation look busy -- which matters when the reading is
used to decide something.
Derived from the activity log rather than denormalised onto users. A
column would cost a migration, a listener change and a backfill to save
one indexed MAX() over a table with a handful of rows on exactly the
installations anybody asks this about. Nothing prunes the log, and
erasure anonymises entries rather than removing them -- actor_type
survives on purpose -- so the answer does not change when the person who
gave it is forgotten.
Requested by the ProjectSend Cloud control plane, which has no other way
to learn the date. Recorded in docs/api-todo.md as deliberately a
command rather than an endpoint, for the reason the command exists at
all: it observes, it does not accept instructions.
|
||
|
ignacionelson
|
2eb23dbc07 |
Stop four comments saying user management is Community-only
It stopped being true in
|
||
|
ignacionelson
|
13b56186f4 |
Say the seat limit before the form, not after it
On a managed installation with its staff seats full, /users/create opened as though there were room. You typed a name, an address and a password you had to invent, pressed Save, and the plan limit came back as a validation error under the email field -- which reads as a complaint about the address rather than a fact about the plan. A full installation is an ordinary state on a plan sold by the seat, so it is now stated up front. The list carries the seat position, the button goes dead once the last seat is taken and says why, and the create screen turns away anyone who reaches it by link or bookmark. The guard in store() is untouched: that is still the rule, this is only the door. The refusal is worded once, in SeatAllowance, and the screen is handed that sentence rather than writing its own -- two wordings of one limit is how somebody ends up believing there are two limits. `full` is derived there too, from the same comparison the guard refuses on, so a screen cannot disagree with it about the edge (used > limit, after an operator lowers a limit) and offer a button for a form that cannot be submitted. Clients get the same treatment: the cap exists there too, and reached it the same way. Self-hosted installations have no limit, so they are shown nothing about one. |
||
|
denkfabrik-li
|
e272f19045 |
Keep a private reply private after the client is deleted
file_comments.client_context_id is cascadeOnDelete, but users are soft-deleted, so the cascade never fires: the column keeps pointing at a row that is still there while the Eloquent relation resolves to null. resolveClientContext branched on the relation, and a null context on a Clients comment is the branch every client on the file reads -- so a staff reply into one client's private thread became a circular to all of them, with the canAssignClient check skipped on the way. VisibleCommentScope says so in its own docblock: "A Clients comment carrying client_context_id = C is never returned to any non-staff viewer other than C ... A Clients comment with a null context is a staff message to everyone on the file, and every client with access reads it." Measured on main, with one file shared with two clients and the first of them deleted after commenting: column client_context_id 3 relation clientContext null POST reply into her thread 201, stored with client_context_id null read by the other client yes Ask the column, and refuse when the account behind it is gone. There is nobody left to answer, and the one outcome that must not follow from a filled column is the broadcast, so this throws rather than falling through to it. authorName() had the same root cause from the other column: its docblock claimed author_id cascades so there is no deleted author, and a deleted client's comment was going out as "Anonymous" -- which is what a guest comment looks like, and a guest comment is read by different rules. Guest is now decided by author_id alone, the same question isFromGuest() asks, and a trashed author is read with withTrashed(). Nothing comes back only once the grace-period erasure has removed the row for real. That read costs one query per comment whose author is trashed. Measured on a ten-comment thread: 11 queries before, 21 after, against 20 for the same thread with every author alive. Left as a lazy read rather than eager-loading with withTrashed() at every call site, because the callers would each have to remember it and the cost only applies to comments whose author is gone. Five tests, two measured red against the unfixed code (2 failed / 3 passed) -- one per column. The three that stay green either way are the branches that must not move: a staff message with no context still reaches everybody, a reply into a live client's thread still lands in that thread alone, and a genuine guest comment is still anonymous. Full suite passes (2053 passed / 2 skipped), PHPStan level 8 clean. |
||
|
denkfabrik-li
|
28e18497b5 |
Refuse the last administrator deleting themselves, and keep setup shut
ProfileController::destroy() validates current_password and soft-deletes. It never asks StaffAccounts::guardLastAdministrator(), and every other door does: Staff update(), guardDeletable(), and both directions of the role conversion. This is the one door where the account being removed is certainly signed in. An installation with a single administrator therefore had a button that emptied it. Measured on main: DELETE /settings/profile 302, the account is gone live staff rows 0 (the row is trashed, not removed) anonymous GET / 302 -> /setup anonymous POST /setup a new active System Administrator EnsureSetupIsComplete asks ->exists(), which excludes trashed rows, and routes/web.php registers GET and POST setup with no auth and no guest middleware -- correctly, since a fresh installation has nobody to authenticate. SetupController::store() re-checks the same condition, so both halves agreed with each other and both were wrong once the last staff row was trashed. Two locks, because one of them is asked at five doors and the other at one. First: destroy() now asks guardLastAdministrator(), the same call with the same message as everywhere else. An administrator with a colleague still goes, a non-administrator staff member still goes, and a client still closes their own account. Second: "has this installation been set up" is not the same question as "does it have a working administrator right now", and only the first one belongs in EnsureSetupIsComplete. A trashed staff row is still evidence that setup happened, so it now counts -- in the middleware and in SetupController::setupIsComplete(), which have to agree or the result is either a redirect loop or an open form. That second lock holds even if a future door forgets the first one. Measured with the guard bypassed entirely and the row trashed directly: GET / answers with the login screen and POST /setup creates nothing. Worth stating plainly: an installation that has already lost its last administrator will now find setup shut rather than open. That is the point -- the recovery path for it is `php artisan projectsend:admin`, which is also how every unattended container installs itself, not a form that anybody on the internet can reach. Six tests, two measured red against the unfixed code (2 failed / 4 passed) -- one per lock. The other four are the boundaries: a colleague present, a staff member who is not an administrator, a client, and a genuinely fresh installation that must still reach setup. Two existing tests needed saying more clearly rather than changing: ProfileUpdateTest's deletion cases now create a second administrator, so that what they assert is self-deletion and not this new refusal; and GettingStartedTest's "fresh installation" cases forceDelete rather than delete, because a soft-deleted staff row is no longer a fresh installation -- which is the whole of the second lock. Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean. |
||
|
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
|
eade690f73 |
Hold the group edit screen to the same library boundary as the rest
Every other group route asks StaffLibraryScope whether this viewer may
act on this group. GroupsController::update() and ::destroy() do, and so
do their API twins -- all four with abort_unless(allowsGroupChange, 404).
The two that read do not: edit() and Api\GroupsController::show() had no
boundary at all.
What they hand over is the membership, name and email per member, plus
the whole client roster of the installation as available_clients. So a
client-scoped staff member could open a group whose contents they cannot
see, read off every client on the installation, and only be refused when
they pressed save.
Two halves, because the leak has two shapes:
- The group itself. Reading it now asks the same reach question the write
half asks, one step earlier, with the same 404 -- a group that reaches
past the viewer's library is not theirs to open either.
- The lists inside it. Both narrow through StaffLibraryScope::clients(),
the listing half of the rule this screen's buttons are already guarded
with: allowsGroupMembership refuses removing a member outside the
roster, and refuses adding a client outside it. Naming them anyway,
with their address, is the mistake ClientsController made before
clients() existed -- that method's own docblock says so.
The reach guard alone would not have been enough. A group nobody has
shared anything with reaches nowhere, so it stays open to everybody --
and it can still hold a stranger's client. That case is why the lists
narrow separately, and there is a test for it.
members_count is left whole on purpose: a size is not an identity, and it
is the same number the group listing already reports.
GroupResource's docblock claimed members are safe to expose because "the
group edit screen already shows [them] to anyone holding edit_groups".
That was a claim about a screen, and it stopped being true the moment the
screen narrowed. Reworded to say what now holds it up, and where.
Not changed: the group listing. It reports names and member counts, not
identities, and every button on it is guarded. Nor Api\GroupsController::
index(), for the same reason. Nor the API document -- scramble:export is
byte-identical, because GET /groups/{group} already documented a 404.
Four tests. Three measured red against the unguarded controllers (3
failed / 21 passed): the group cannot be opened at all, the edit screen
stops naming strangers, and the API twin narrows what it hands back. The
fourth -- an unscoped viewer keeps the whole roster and every member -- is
green either way and guards against the fix over-refusing.
Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
|
||
|
denkfabrik-li
|
3e15237f90 |
Refuse self-deactivation over the API however the boolean is written
Api\UsersController::update() compares the validated value strictly:
if ($user->is($actor) && ($validated['active'] ?? true) === false) {
The `boolean` rule accepts 0 and "0" as well as false, and it does not
cast. `0 === false` is false, so the refusal never fires -- and the
model's own `boolean` cast then stores as false exactly the value the
guard had just decided was not a deactivation.
Measured against main, with a second administrator present so that
guardLastAdministrator is not what answers:
{"active": false} -> 422, still active
{"active": 0} -> 200, active is now false
{"active": "0"} -> 200, active is now false
The method's own docblock says it is "Refused with a 422 if the change
would leave the installation with no active administrator, or if you
would be deactivating yourself", and the web screen does refuse. This is
the API half of that sentence.
RolesController::guardScopeRemoval documents the rule this breaks, in the
same words: callers resolve the flag with Request::boolean() and hand the
same value to the guard and to the write, deliberately, because reading
the validated array and comparing it strictly "would let a request
through here that the model's `boolean` cast then stores as false anyway
-- the guard and the write disagreeing about one value is exactly the
shape this guard exists to prevent".
So read it once, with Request::boolean(), and give that one value to both.
Not changed: the validation rule. It stays `boolean`, so the accepted
inputs are the same as before -- what changes is that one of them stops
meaning two different things on its way through. Nor anything about
deactivating somebody else: all three forms still work, and there are
tests saying so.
Six cases from two datasets. Two measured red against the unfixed
controller (2 failed / 4 passed): 0 and "0" on yourself. `false` was
already refused, and the three "somebody else" cases are green either way
-- they guard against the fix over-refusing, not against the bug.
Full suite passes (2054 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
|
||
|
denkfabrik-li
|
ab6e9eecf3 |
Ask the seat cap where a pending client is approved through edit()
SeatAllowance says a cap is only a cap if every door asks, and has a test
per door for that reason. Two doors do not ask.
The moment a seat is spent is the moment `account_requested` is cleared.
Five places do that. approve(), both store()s and ClientProvisioning ask
guardClient(); AccountConversion asks it through guardToClient(). The two
update()s -- web and API -- clear the flag with no guard at all, under a
comment that names exactly what they are doing:
// Activating a pending account through the edit screen counts as
// approval and clears the request flag.
Measured with clients: 0, one pending registration:
POST /account-requests/{id}/approve refused, flag still set
PATCH /clients/{id} active=true approved, clientUsed() 0 -> 1
PATCH /api/v1/clients/{id} active=true approved, clientUsed() 0 -> 1
A managed installation at its cap therefore keeps taking clients on, from
the edit screen or a PATCH, for as long as registrations keep arriving --
and self-registration is open to strangers, so the supply is not the
operator's to control.
Inside the branch, not above it. Above it, an installation sitting at its
cap could not rename a client it already holds, which would trade one
wrong refusal for another. There is a test pinning that.
The field is `active` rather than the default `email`: on this screen the
administrator is toggling `active`, and an error under the email field
would point at the wrong thing. approve() has no form of its own, so it
keeps the default.
Three tests, per door as the file's other eight are. The two door tests
were measured red against the unguarded controllers (2 failed / 18
passed). The third -- that editing an existing client still works at the
cap -- is green either way: it guards against the fix being written a
line too high, not against the bug.
Full suite passes (2051 passed / 2 skipped), PHPStan level 8 clean.
One thing worth knowing that this branch does not touch: on a parallel
run, `UpdateWelcomeTest > staff who may not read...` fails roughly one run
in six on untouched main, with `BindingResolutionException: Target
[Inertia\Ssr\Gateway] is not instantiable`. Measured over 24 baseline runs
before this change existed. It is not this fix, and it is not in scope
here, but it will start being visible as soon as the workflow parses
again.
|
||
|
denkfabrik-li
|
1dc274e896 |
Let an enforced user reach the far side of the confirm-password screen
EnforceTwoFactor exempts by route name, and only the GET half of confirm-password has one. routes/auth.php:95 names the form `password.confirm`; :98 registers its submission with no name at all, and Route::named() answers false for a null name. So the loop the exemption exists to prevent is still there, one step further along. With Setting::TwoFactorEnforcement set to staff, clients or all, an un-enrolled account walks: GET /dashboard -> two-factor.show GET /system/settings/security -> two-factor.show PATCH /system/settings/security -> two-factor.show POST /settings/two-factor -> /confirm-password (RequirePassword) GET /confirm-password -> 200, the form renders POST /confirm-password -> two-factor.show <- not exempt `auth.password_confirmed_at` is never written, so enrolling can never start, and every route that is not on the exemption list stays shut -- including Settings -> Security, the one screen that could turn enforcement back off. Logout is the only door left; recovery is CLI or database access. It takes one administrator turning the setting on to reach it, and it reaches every account on the installation at once, including their own. The fix is the name. `password.confirm*` then covers both halves of one screen, matching `two-factor.*` in the same expression; the namespace belongs entirely to a flow enrolment already depends on being reachable, and the route table has nothing else under it -- `password.confirm` (GET) and `password.confirm.store` (POST) are the two it reaches. Exempting the submission grants nothing further. store() validates the password, writes a session flag and redirects; the redirect it issues enters this middleware like any other request, so Settings -> Security is still answered with two-factor.show after confirming. What changes is that enrolment can now be started. Two tests, both measured red against the unfixed middleware: the password confirmation sticks, and enrolment can be started afterwards (the secret is written and the screen reports `pending`). Also named the redirect the existing test settles for. `->assertRedirect()` with no target passes on this middleware bouncing the request back to two-factor.show, which is the shape that file exists to refuse. It is a clarification rather than a guard -- that assertion is green either way, since the redirect it sees comes from RequirePassword. Full suite passes (2050 passed / 2 skipped), PHPStan level 8 clean. |
||
|
ignacionelson
|
d58e48301f |
Move the seat number to the end of the sentence
It read "limited to 1 staff accounts" -- the number sat directly in front of a countable noun, which is the message a free-tier customer meets the first time they try to add anybody. Adding plural forms would fix English and not much else. Polish, Czech and Russian inflect the noun by the number in front of it, on a three-way split that a two-form string cannot express, so ':count kont' cannot be right for every value however many variants it carries. Ending the sentence on the number means no language has to agree with it -- the same shape the other counted strings here already use. Both strings rewritten in all sixteen locales rather than left to the next translation pass, since the old key would otherwise go missing and block a build. Checked at 1 and at 25 in English, Spanish, German, Polish and Russian. |
||
|
ignacionelson
|
787e9ec189 |
Report version, edition, capabilities and seat usage as one probe
Asked for by the platform side, and the reason is better than convenience. Their reconciler's rule is that it observes an end state and never sends an instruction. `docker exec … php -r '…'` to reach a public method is an instruction with the caller's argv in it, however harmless the argv, and it would have been the first crack in that rule. A named command is an observation, the same kind of thing as reading a directory size. `--json` for a machine, plain lines for a person. Nothing here is a secret or a credential: every field is already visible to any signed-in administrator, which is what makes it safe to read from outside the container. The counts come from SeatAllowance — the code that refuses the account past the limit — rather than from a second query that agrees with it today. Two counts that merely agree diverge eventually, over an inactive account or a soft-deleted one, and the divergence reads as a billing fault rather than a counting one. Unlimited is emitted as null, with a test saying so, because the failure if a reader takes it for zero is a customer on the most expensive plan whose instance refuses to create a single client. The platform side independently landed the same care on the emitting end, omitting the variable rather than sending it empty. It also answers the question that started all of this. Diagnosing why a tenant ignored its bucket meant reaching into a container and calling app() by hand; `projectsend:status` now says which capabilities the edition grants, which is where that hunt began. |
||
|
ignacionelson
|
ac691387e8 |
Seed two-factor enforcement at provision, before the first account exists
The last of the three. Enforcement is a database setting defaulting to 'none', and on a managed installation the only writers are whoever administers it and the boot that creates them — so a policy meant to be on from the start had nowhere to be written. A control plane calling in afterwards leaves a window between the first account existing and the policy covering it, and the first account is the one with every permission. The entrypoint already seeds an account from the environment. This seeds the policy one line above it, so the administrator is born under the rule rather than ahead of it. There is a test for exactly that ordering, because the ordering is the whole point. Seeded, never overridden. A value that won on every boot would take the setting away from the person it belongs to — somebody who tightened it would find it loosened again by a restart. So it writes only when nothing has ever been stored, the same shape as `projectsend:admin --if-none`. Two things that would have been easy to get wrong, both pinned: 'none' is the enum's own default, so Settings::get() cannot tell "stored as none" from "never stored". Asking the accessor would have overwritten an administrator who deliberately chose it. The command asks the table. And it reads config rather than env() directly. `config:cache` stops .env being read at all, which is how TRUSTED_PROXIES came to have no effect on any web request while looking correct in the file. Deliberately not a general PROJECTSEND_SETTING_<KEY> mechanism. Every setting reachable from outside is one whose value depends on where you look, and the blast radius of getting that wrong is the settings table. One named key per setting that needs it. The three new variables are documented in config/projectsend.php and not in .env.example or the Docker Hub overview. Those two are written for somebody running one installation for themselves, and a seat cap is not a thing they have — FILES_WEB_SERVER_READABLE is in .env.example because a self-hoster on cPanel genuinely meets that problem. |
||
|
ignacionelson
|
463e86f82b |
Refuse an account past the seat count an operator sold
Opening user management on cloud (
|