197 Commits

Author	SHA1	Message	Date
ignacionelson	81bb136e9e	Merge pull request #1750 from denkfabrik-li/fix/mail-oauth-alarm-fires-once ... RefreshMailOAuthTokensCommand is the daily refresh and, by its own docblock, the health check that goes with it: a delegated grant can die silently, and for a portal whose password-reset mails ride on this connection that must surface as a warning rather than as a support ticket weeks later. It decided whether to warn from last_error -- but last_error has a second writer. OAuthCodeFlowBroker::refresh() records a dead grant and notifies nobody, and freshAccessToken() reaches it from every send. So on an installation that is actually sending mail the send got there first, the command read the column as "already told them", and the warning never went out. last_error is cleared only by a successful refresh, which a dead grant never has, so it never went out later either. The alarm worked on installations that were not using the mailbox and failed on the ones that were. The anti-nag rule is not the problem and does not change: one notification per broken state is still all anybody gets. The problem is that one column was answering two questions, which the table's own comment describes -- "what the settings page's warning and the admin notification read". The warning wants "is this connection broken", and any writer may answer it, which is why the settings page turning red on a failed send is correct and stays. The notification wants "have the admins been told", and only the notifier can answer that. broken_notified_at is stamped when the command notifies, and the command asks that instead. It is cleared wherever last_error is cleared -- a successful refresh, a disconnect, a changed client id -- and those three sites now call clearFailure() rather than nulling two columns each, because a connection left healthy but still marked "already told them" would go quiet the next time it died, and a fourth caller is exactly how the first one happened. The send path still records the failure and still notifies nobody: a transport is not a place to decide who gets alarmed. Verified before merging: 27 passed on the merged tree, 2 failed / 25 passed with app/ reset and the migration and tests kept. The recovery test is green either way by design. This touches the same command and broker as #1739 and the follow-up to it, so the merged result was read rather than trusted: the refresh reporting sits in the try and the notify guard in the catch, they do not interact, and refreshSerially() re-reads the row before refreshing so the broken_notified_at the catch reads is the stored one -- while a stand-aside throws nothing and never reaches the catch at all. Note for the next release's upgrade notes: this adds a migration, so "nothing to do beyond dropping in the files" no longer holds. Reported and fixed by @denkfabrik-li.	2026-08-28 18:04:37 -03:00
ignacionelson	a2bc3fa163	Merge pull request #1749 from denkfabrik-li/fix/deleted-comment-author-type ... file_comments.author_id is cascadeOnDelete and the cascade never fires, because a user is soft-deleted. The row behind a deleted commenter is still there and the column still points at it -- the relation just would not hand it over, and every caller then had to invent a meaning for the absence. They invented different ones: the author type became "guest" on the moderation screen and on the file's own thread, and "client" in the API, each printed beside a name that stayed correct, so one row said "Dana Staff" and "guest" at the same time. The author filter and the name search stopped matching the comment altogether, which is the worse half: a moderator filtering for staff comments did not see a staff comment sitting in front of them, and nothing about that looks like a missing row. This is the author half of #1717, and DeletedClientThreadTest's docblock already described both columns. FileComment::authorName() was the one place that reached past the relation by hand, which is why the names were right while everything beside them was wrong. The relation is fixed rather than the five call sites: author() reads a deleted account, and the API resource, the author filter and the name search then need no change at all, because they were already asking the right question of a relation that would not answer it. The two authorType() copies now ask author_id, which after the relation fix answers the same either way -- written that way because "no author row means guest" is exactly the reading that produced the bug. Verified before merging: tests/Feature/Comments at 186 passed on the trial-merge, 5 failed / 1 passed with app/ reset. The survivor is the guest guard, green either way, which is what says this did not simply relabel everything as staff. scramble:export reproduces the spec byte for byte. No visibility widens, and that was checked rather than taken on trust: every decision point in VisibleCommentScope and FileCommentPolicy compares author_id directly, five sites, none through the relation. No new field is exposed either -- the API resource reads only id, name and type from the author, and name already went through authorName()'s withTrashed lookup. Deleting an account still takes its comments with it when the grace period ends, since author_id is cascadeOnDelete. Reported and fixed by @denkfabrik-li.	2026-08-28 18:03:03 -03:00
ignacionelson	ef6f8fea56	Merge pull request #1748 from denkfabrik-li/fix/password-reset-credential-source ... Two accounts reach the same reset with opposite needs, and it answered both by writing a hash and hoping. A provider account is asked for something it cannot do. The Connected accounts screen refuses to release an account's last provider -- "Set a password first, then disconnect Google" -- and nothing set auth_source back to Local, so the screen went on asking for what had just been done, with no way out from inside the application. AuthSource already states the rule that closes it, for this case by name: a social account may later set a real password, and social only means the account came into existence without anybody choosing one. A reset by emailed token is where somebody chooses one, and the prop the screen reads is literally auth_source === Local under the name has_local_password. A directory account is told something untrue. isDirectoryAccount() means the local hash is not consulted at all, so the same reset wrote a password that could never sign anybody in and reported success -- including when the directory it points at is gone, which is exactly the situation that sends somebody to a reset. The reset now asks where the account's credentials live. social becomes Local, because the new password is the credential now. A directory account is refused, with the reason, and nothing about it moves -- writing Local there would not record something that had happened, it would take the account off its directory as a side effect of a password reset, which is an administrator's decision and already lives in AccountConversion with the password requirement and activity entry that belong to it. Everything else is byte for byte as before. Verified before merging: 20 passed on the trial-merge, 2 failed / 18 passed with app/ reset, and the wider suites green -- tests/Feature/Auth 96 passed, tests/Feature/Identity 302 passed. Four properties were checked in the framework rather than argued. PasswordBroker::reset() calls validateReset() before the callback, so the refusal only reaches somebody holding a token emailed to that address and nothing is enumerable. It deletes the token after the callback, so a throw leaves the link usable. Every use of AuthSource::Local is in ConnectedAccountsController -- the has_local_password prop and the last-provider guard -- so the social-to-Local flip grants exactly the ability the screen instructs the user to obtain and nothing else, and no new login capability at all, since password login already worked for social accounts. And the check is isDirectoryAccount() rather than an auth_source comparison because LDAP is client-only, so staff are not refused; the test for that is green either way. One new string is English only for now: "This account signs in through your directory, so its password is not set here." Reported and fixed by @denkfabrik-li.	2026-08-28 18:01:47 -03:00
ignacionelson	144f5fc578	Merge pull request #1747 from denkfabrik-li/fix/bulk-edit-skip-reason ... Two different things stop a selected file being changed in a bulk edit, and bulkUpdate() reported both as the first one. Files dropped by the Gate::allows('update') filter are ones this staff member may not edit at all. A file that survives the filter and still changes nothing is a different case: it was editable, and every field they asked to change is one their role does not let them set -- expiry, download limit and categories each sit behind their own permission here, exactly as they do in the single-file editor. So a staff member with edit_files but without set_file_expiration_date, editing three files they own, was told "0 of 3 selected files were updated. The rest were skipped because you don't have permission to edit them." They own all three, and editing is precisely what they may do: the sentence was both wrong and unactionable, since nothing in it points at the permission that actually stopped the edit. The two cases get their own sentences now. Every skip being a file they may not edit keeps the existing string, unchanged, so its sixteen translations stay in use. Anything else gets a new one, "because you don't have permission to make those changes", which is also true when both reasons are in play, so a mixed selection is described correctly rather than approximately. Which files get changed is untouched, as is the silent-skip convention and the 422 when nothing at all is authorised. Verified before merging: 14 passed on the trial-merge, 2 failed / 12 passed with app/ reset -- the field-permission case and the mixture. The pure edit-permission case is green either way, which is what says the existing message was not disturbed. FilesController overlaps #1728, already merged, and its expiryDateFor work is intact in the merged tree. The new string arrived English-only; the sixteen catalogs are filled in the commit that follows. Reported and fixed by @denkfabrik-li.	2026-08-28 17:58:14 -03:00
ignacionelson	383c3b2ff5	Merge pull request #1746 from denkfabrik-li/fix/expired-file-staff-access-comment ... File::isExpired() documented the rule the whole application is supposed to follow: once past, the file is hidden from clients and the public site but staff keep full access. The second half is not true of a client-scoped staff member. StaffLibraryScope::buildFiles() builds their library as own uploads plus what each assigned client may see, and that second half runs through File::scopeVisibleToClient, which ends in notExpired() -- a client-side rule. So an expired file they held only through a client leaves their library and answers 403 on download, while their own expired upload stays and an unscoped administrator is unaffected. Api\FilesController stated it the same way, "Only the client branch of the visibility rules drops them", which reads as though a staff caller is unaffected when a client-scoped one is reached through that very branch. This does not change that behaviour. c8078f65 weighed widening it and decided against, because scopeVisibleToClient is the single source of truth for client file access and the highest-stakes function to go changing for a dashboard widget, and relabelled the widget instead. That decision lived in a commit message and one widget's label; nothing in the code said it, and the docblock nearest the rule went on promising the opposite -- which is how the next person re-derives "staff keep full access" and widens the scope to match. Documentation and characterisation only. isExpired() now states the boundary and why it is where it is, the API comment is corrected, and ExpiredFileStaffAccessTest pins all three cases. Verified before merging: 3 passed on the trial-merge. The counter-check has to be inverted for a characterisation test -- these pass on unmodified main by construction, so the question is whether they fail when the boundary moves. Deleting the closing notExpired() from scopeVisibleToClient gives 1 failed / 2 passed, and it is the third case, the one carrying the decision, that falls. File.php overlaps #1726 and Api/FilesController.php overlaps #1727, both already merged, and both are intact in the merged tree. scramble:export reproduces the spec unchanged. Reported and fixed by @denkfabrik-li.	2026-08-28 17:56:11 -03:00
ignacionelson	d91cf97bcb	Merge pull request #1745 from denkfabrik-li/fix/moderation-view-read-permission ... FilePolicy::view() has two halves for a staff member: one of the three file keys (upload / edit_files / edit_others_files), AND StaffLibraryScope. Every comment surface that spans files narrowed by the library half alone -- VisibleCommentScope::across(), pendingTotal(), and the API's GET /comments/pending. A role holding moderate_comments and no file key at all therefore read, on /comments, every comment in the installation: the text, staff-only notes, the client's name in conversation, and a visitor's IP, while getting a 403 on every file those comments were about. POST /api/v1/comments/{id}/approve was the same door on the write side, and its response carries the comment body, so an id was enough to read one. The project already states the rule this breaks in four places, including across()'s own docblock -- "a moderation screen is not a way around the visibility model: moderating means deciding about comments you can already see" -- and only the cross-file queries did not ask it. The cross-file queries now take their files from ViewableFileScope, which is FilePolicy::view() expressed as a query and already in the codebase for exactly this, instead of from StaffLibraryScope, which is only its second half. The permission half becomes a named method there, permitsAnyFile(), because three modules now ask it, and FileCommentPolicy::moderate() asks it in both of its forms. This is the other half of #1698, which library-scoped the same screen: library is not readability. Verified before merging: tests/Feature/Comments at 180 passed on the trial-merge; with app/ reset and the new test file kept, 5 failed / 2 passed. The two green either way are the right two -- the premise, that the file itself 403s for this viewer, and the guard that a moderator who does hold a file key still moderates the whole installation. Compatibility was the question worth asking, and it is clean: the only shipped roles holding moderate_comments are Account Manager, which also holds Upload, EditFiles and EditOthersFiles, and System Administrator, which holds everything. No shipped role loses moderation. The only configuration whose behaviour changes is a custom role granting moderate_comments with no file key, which is precisely the leaking one. This PR also edits docs/api/openapi.json, which #1727 edited too, so the merged result was checked rather than trusted: scramble:export on the merged tree reproduces the committed file byte for byte, with both endpoints' descriptions present. Reported and fixed by @denkfabrik-li.	2026-08-28 17:43:30 -03:00
ignacionelson	89b3d34c8f	Merge pull request #1744 from denkfabrik-li/fix/version-link-duplicate-share-notice ... FileVersions::link() resolves its audience before the merge, and its own comment says the ordering is the whole dedupe: these are the people who could already see both files, so anyone the merge is about to reach for the first time is excluded and gets file_shared from FileSharing::assign() instead. The merge then undid it. moveAssignmentsToRoot() handed every one of the revision's targets to assign() under the comment "firstOrCreate inside, so a target the root already has is a no-op rather than a duplicate notification" -- but firstOrCreate makes the assignment row idempotent, not the three side effects below it. The activity entry, the in-app notification and the digest all ran unconditionally, so a client who already held both files was told a file had been shared with them about a file they had had all along, on top of the file_new_version they were owed. Two notifications for one action, for exactly the people the early resolve exists to protect. A target the root already holds is now skipped rather than handed to assign(). Nobody is gaining access in that case, so the activity entry would have been as untrue as the notification -- which is the rule copyAssignmentsFrom() states outright for its own case, and why it inserts directly instead of going through FileSharing. The two stale comments are corrected with it. Deliberately not changed: assign() itself, and so the behaviour ShareNotificationsTest pins, where re-posting an existing assignment through the share endpoint still notifies again. That test says the condition for changing it -- it should stop for files and folders at once, which is the point of them sharing one implementation -- and a version merge is not somebody choosing to share again. Verified before merging: 10 passed on the trial-merge, 2 failed / 8 passed with app/ reset, and the whole tests/Feature/Files directory at 548 passed. The case where somebody genuinely gains the root still gets file_shared is green either way, which guards against skipping too much. The method was read whole rather than just the hunk: $file->assignments()->delete() still runs for a skipped target, so no row is left dangling and nobody loses reach. Reported and fixed by @denkfabrik-li.	2026-08-28 17:41:39 -03:00
ignacionelson	d09cb602c1	Merge pull request #1743 from denkfabrik-li/fix/read-redirect-covers-every-door ... Three middleware answer before HandleInertiaRequests and so repeat its 302-to-303 upgrade themselves: EnsureSetupIsComplete, EnsureUserIsActive and EnforceTwoFactor. This file has a write case for each. The rule has a second half -- a read still gets a plain 302, because a 303 there is an upgrade nobody asked for -- and that half was checked once, on the deactivation door, under the name "leaves a read alone in every one of those cases". So a change that upgraded reads at the setup door or the two-factor door would have gone through with the suite green and this test still claiming it would not. One case per door now, as a dataset. The setup case reads a guest-reachable GET for the same reason the write case posts to /timezone: anything behind auth is answered by the guest redirect before EnsureSetupIsComplete ever sees it. No production code changes -- all three doors answer a read with 302 today, which is what the new cases assert. Verified before merging: 9 passed on the trial-merge, and the mutation counter-check was run here rather than taken from the PR. With EnsureSetupIsComplete answering 303 to everything, this branch's file goes 1 failed / 8 passed and main's version goes 7 passed. The write case for that door stays green under the mutation, which is right: 303 is what a write should get. The mutation itself was confirmed live first, by making the middleware throw and watching the response become a 500 -- a first attempt at it bound no argument and was a silent no-op, which would have looked exactly like the new test failing to notice. Reported and fixed by @denkfabrik-li.	2026-08-28 17:38:47 -03:00
ignacionelson	b7a94d4479	Merge pull request #1742 from denkfabrik-li/fix/file-permissions-test-reads-config ... FILES_WEB_SERVER_READABLE exists so a web server running as a different user can traverse the directories a download lives in. It asked for 0755 from a key that is never consulted: FilesystemManager::createLocalDriver() passes directory_visibility ?? visibility ?? private as the default visibility for directories, and this disk sets visibility to public two lines above with no directory_visibility, so Flysystem reads dir.public and never looks at dir.private. The mode came out 0755 anyway, because 0755 is Flysystem's default for a public directory -- the right answer from the wrong place, which is the kind that stops being right quietly. Adding a directory_visibility to this disk, an ordinary hardening move, or a change to that Flysystem default would have been enough to break the flag silently on exactly the hosts that need it. Both directory keys are now named, so the intent survives whichever branch Flysystem takes. Nothing widens: the flag-off path is still literally the old configuration, spread rather than ternary, and under the flag 0755 was already the effective mode. And the test could not have caught it, because it was not testing this configuration: filesDiskWith() restated the shipped branch inline, verbatim down to the 0755, so it kept passing against its own copy however the real one changed. It now requires config/filesystems.php and replaces only the root. Two housekeeping fixes ride along: the scratch root is per parallel worker, the way Tests\TestCase already does it for upload parts, because eight workers sharing one real directory means one worker's afterEach deletes another's tree mid-test; and the tree is cleared before each test as well as after, so a killed run does not poison the next one. Verified before merging: 3 passed on the trial-merge, and the mutation counter-check was run here rather than taken from the PR. With the shipped dir.public changed to 0750, this branch's test goes 1 failed / 2 passed and main's version of the same file goes 3 passed -- the old one genuinely could not see a change to the shipped configuration. Reported and fixed by @denkfabrik-li.	2026-08-28 17:34:33 -03:00
ignacionelson	ff26fac9c5	Say when the scheduled mail refresh stood aside ... #1739 put the nightly OAuth refresh under the same lock a send holds, which is right -- but standing aside for the lock holder still printed "Refreshed <provider> (<account>)". No token request was made, so the line describes something that did not happen, and scheduler output is read precisely by somebody trying to work out what did. refreshSerially() now answers whether it refreshed, and the command says which of the two happened. Standing aside is a healthy outcome: somebody else is refreshing this very connection, which slides the token window just as well as doing it again would. It is just not a refresh, and it should not claim to be one. The existing test for the stand-aside now asserts the output too, and it fails against the old message. Same reasoning as d8ef21b, which said when the worker check was skipped rather than skipping it quietly.	2026-08-28 17:27:30 -03:00
ignacionelson	a7e883ef70	Merge pull request #1739 from denkfabrik-li/fix/scheduled-mail-refresh-lock ... OAuthCodeFlowBroker::freshAccessToken() serialises refreshes per connection, and its comment says why: both providers rotate the refresh token as they hand out a new access token, so a refresh token is good for exactly one use, and "a worker racing the nightly refresh command" means the slower one spends a token the faster one has already replaced. The provider answers that with invalid_grant, which is the same thing it says about a genuinely revoked grant -- last_error gets written, the settings page turns red, and every admin is told to re-consent a connection that was never broken. RefreshMailOAuthTokensCommand called refresh() directly, outside that lock: it was the racer the comment names rather than a party to the arrangement it describes, and the false alarm landed on the connection the daily run exists to protect. The command now goes through refreshSerially(), which takes the same lock -- named once, in one place, for both callers -- re-reads the row inside it, and refreshes. Unlike freshAccessToken() it refreshes a token that is still usable, which is the point of the daily run: a delegated refresh token dies of disuse and this keeps the window sliding. The lock is taken rather than waited for, unlike the send path: nobody is standing at a screen for a scheduled job, and a held lock means somebody is refreshing this very connection right now, which slides the window and establishes its health just as well. refresh() stays lock-free, because making it self-locking would deadlock the send path that already holds the lock. Verified before merging: 24 passed on the trial-merge, 1 failed / 23 passed with app/ reset. PHPStan level 8 clean across app/Modules/Platform/Mail. Adding a method to the MailOAuthBroker interface breaks nothing: OAuthCodeFlowBroker is its only implementer, and MailOAuthBrokers is a registry rather than an implementation. Known nit, fixed in a follow-up rather than here: when refreshSerially() stands aside because the lock is held, the command still prints "Refreshed <provider> (<account>)". Reported and fixed by @denkfabrik-li.	2026-08-28 17:26:24 -03:00
ignacionelson	90009b7029	Merge pull request #1738 from denkfabrik-li/fix/totp-replay-claim-atomically ... TwoFactorService::verify() asked Cache::has(), verified the code, then Cache::put(). Between the read and the write the key is free, so two requests carrying the same code could both be told yes -- which is precisely what the replay guard exists to prevent, and the window an intercepted code has is the whole of its validity either side. Cache::add() writes only if the key is absent, so of two requests carrying the same valid code exactly one gets true back, and has() is gone: a failed claim is "already used". Verification still runs first, so a wrong code never touches the cache and cannot burn the window for the code the person is about to type correctly. The 90-second claim, the key's shape, and the recovery codes are all unchanged. Verified before merging: 11 passed on the trial-merge, 1 failed / 10 passed with app/ reset. The existing "a totp code cannot be replayed" test is green either way, because it covers the sequential case, which was never the problem. Being on the authentication path, the wider suites were run too: tests/Feature/Identity and tests/Feature/Auth together, 393 passed. Cache::add() is only as atomic as the store under it, so every store an installation could realistically run was checked in the vendored framework rather than assumed: database (the default when CACHE_STORE is unset) decides on insertOrIgnore(...) > 0 against the cache table's primary key; redis and memcached have native atomic adds; and the file store takes an exclusive flock before it reads and writes. Reported and fixed by @denkfabrik-li.	2026-08-28 17:24:28 -03:00
ignacionelson	9508750c60	Merge pull request #1737 from denkfabrik-li/fix/transfer-range-utc-bounds ... resolveTransferRange() builds every boundary in the viewer's zone, deliberately: "last week" should end when their evening does, not at whatever hour UTC midnight falls on for them. Its docblock then claimed the instants "compare against the UTC column directly". They did not -- the query builder formats a Carbon in whatever zone the object carries and discards the offset, so the viewer's midnight reached the database as a UTC string. For Asia/Tokyo the window really began at 2026-08-21T15:00:00Z while the query asked for 2026-08-22 00:00:00: nine hours at each end, both in the same direction, so the first nine hours of the viewer's window were missing from the chart and the last nine hours of somebody else's day were counted into it. The comparison now converts to UTC, one ->copy()->utc() per boundary. The copy matters: the originals keep the viewer's zone, so the day cursor and the grouping below still put an evening upload on the right bar, which is the half that really is about the viewer's calendar. Every other date filter already goes through LocalDay::start()/end(), which return UTC, which is why the activity log and the download history never had this. Verified before merging: 20 passed on the trial-merge, 1 failed / 19 passed with app/ reset. Shares DashboardController and its test file with #1722, already merged, so the merged tree was checked -- that PR's visibleToClient change is intact. Reported and fixed by @denkfabrik-li.	2026-08-28 17:22:37 -03:00
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.	2026-08-28 17:20:44 -03:00
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.	2026-08-28 17:18:38 -03:00
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.	2026-08-28 17:17:17 -03:00
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.	2026-08-28 17:13:05 -03:00
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.	2026-08-28 17:05:30 -03:00
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.	2026-08-28 17:03:34 -03:00
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.	2026-08-28 17:02:05 -03:00
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.	2026-08-28 17:00:16 -03:00
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.	2026-08-28 16:58:34 -03:00
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.	2026-08-28 16:56:47 -03:00
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.	2026-08-28 16:55:29 -03:00
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.	2026-08-28 16:47:40 -03:00
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.	2026-08-28 16:16:49 -03:00
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.	2026-08-28 16:14:57 -03:00
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. f1b35cc9 settled that answer for deleted files: start from the live row, because a deleted file is not reach, because nobody can reach it. An expired file is the same case and was not covered. File::scopeVisibleToClient ends in notExpired(), so the moment a file expires it leaves every member's /my-files and the download answers 403 -- but it also leaves files(), where its absence reads as "outside my library". The group then became unmanageable for good: the rep could not add anyone, and could not undo their own membership change either. The reach query now skips expired files as it already skips deleted ones. File::scopeVisibleToClient is unchanged -- what expiry does to a scoped viewer's library was settled deliberately in c8078f65, and this is about what counts as reach, not about what anyone may open. The folder half needs nothing, because folders do not expire. The limit this leaves open, stated rather than implied: a membership added while a file was expired outlives the expiry, so if somebody later clears expires_at the client reaches a file that was outside the actor's library when the decision was made. f1b35cc9 leaves exactly the same opening for a file restored from the trash, and closing either would mean the guard weighing rows nobody can currently reach. Verified before merging: this changes the file half of the same method #1719 changed the folder half of, so the merged tree was read rather than trusted -- both halves now skip expired files consistently. 29 passed on the merged tree, 1 failed / 28 passed with app/ reset. The "an expired file does not excuse a live one that is still out of reach" test is green either way. Reported and fixed by @denkfabrik-li.	2026-08-28 15:59:24 -03:00
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.	2026-08-28 15:51:33 -03:00
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.	2026-08-28 15:46:17 -03:00
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.	2026-08-28 13:27:10 -03:00
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.	2026-08-28 13:11:57 -03:00
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.	2026-08-28 11:57:43 -03:00
denkfabrik-li	7be81d3586	Tell the admins the mailbox is dead, even when a send noticed first ... The daily refresh doubles as the health check for a connected OAuth mailbox, and its own docblock says why that matters: a grant can die silently, "which for a portal whose password-reset mails ride on this connection must surface as a warning, not as a support ticket weeks later". It decided whether to warn by reading last_error -- but the send path writes that column too. OAuthCodeFlowBroker::refresh() records the failure and notifies nobody, and freshAccessToken() reaches it from every send. So on an installation that actually sends mail, the send lands first, the command reads the column as "already told them", and the warning never goes out. last_error is cleared only by a successful refresh, which a dead grant never has, so it never goes out again either. Measured on main, one dead grant, two orders: nobody sends, command first 1 notification, then quiet correct a password-reset mail first 0 ... 0 ... 0 never The alarm worked on installations that were not using the mailbox and failed on the ones that were. The anti-nag rule is not the problem and does not change. The problem is that last_error answers "is this broken", which any writer may set, while the command needs "have the admins been told", which only the notifier can. The table's own comment shows the conflation -- one column described as "what the settings page's warning and the admin notification read". So the notification gets its own column. broken_notified_at is stamped when the command notifies, and cleared wherever last_error is cleared: a successful refresh, a disconnect, a changed client id. The three call sites go through MailOAuthConnection::clearFailure() rather than nulling two columns each, because a connection left marked "already told them" while healthy would go quiet the next time it died -- the same bug in a new place.	2026-08-28 14:41:38 +02:00
denkfabrik-li	92f50fdb85	Read a comment's author even after the account is deleted ... `author_id` is cascadeOnDelete and the cascade never fires, because users are soft-deleted: the row behind a deleted commenter is still there and the column still points at it. The plain relation handed back null anyway, and every caller invented its own meaning for that absence. Measured on main, one staff member's staff-only comment, before and after the account is deleted: /comments screen Dana Staff / staff -> Dana Staff / guest the file's own thread Dana Staff / staff -> Dana Staff / guest GET /api/v1/.../comments type staff -> type client filter author_type=staff 1 row -> 0 rows search "Dana" 1 row -> 0 rows unfiltered 1 row -> 1 row Three surfaces, three different wrong answers, each next to a name that stayed correct -- so a row can read "Dana Staff" and "guest" at once. A moderator filtering for staff comments does not see a staff comment that is sitting in the list in front of them. This is the author half of what #1717 fixed for client_context_id, and DeletedClientThreadTest's docblock already describes both columns. The fix is the relation, not the five call sites: author() reads a deleted account, which is what authorName() already reached for by hand. The resource, the filter and the search then need no change at all. The two authorType() copies now ask author_id rather than the relation -- which after this answers the same either way, and is the rule isFromGuest() and authorName() already follow. Nothing that decides who may read a comment goes through this relation. VisibleCommentScope and FileCommentPolicy both compare author_id directly, so no visibility widens.	2026-08-28 14:24:03 +02:00
denkfabrik-li	27c289a4d6	Let a password reset know where the account's credentials live ... Two accounts reach the same reset with opposite needs, and it treated both as "write a hash and hope". A provider-created account is told, on the Connected accounts screen, to "set a password first, then disconnect Google" -- and doing it changed nothing, because nothing ever set auth_source back to Local. AccountConversion is the only writer, and that is an administrator. So the screen went on asking for something that had already been done, and the person could not release their last provider without help. AuthSource states the rule that closes this: `social` means the account came into existence without anybody choosing a password, and, in as many words, "a social account may later set a real password". A reset by emailed token is where somebody does. The screen's has_local_password prop is literally auth_source === Local, so the write is what completes the sentence it prints. A directory account is the opposite case and gets the opposite answer. isDirectoryAccount() means the local hash is not consulted at all, so the reset reported success and left the person with a password that cannot sign them in -- including when the directory it points at is gone, which is exactly when somebody reaches for a reset. It is refused now, with the reason, and nothing about the account moves: taking one off its directory is an administrator's decision through AccountConversion, not a side effect of a reset. The refusal sits where the token has already been validated, not where the link is asked for. That endpoint answers "A reset link will be sent if the account exists" to everybody on purpose, and refusing there would tell a stranger both that an address is an account and how it signs in. Throwing before the write also leaves the token unspent, since PasswordBroker deletes it after the callback returns.	2026-08-28 14:01:24 +02:00
denkfabrik-li	5e60d2ef88	Say what expiry does to a client-scoped staff member's library ... File::isExpired() documents the rule the application is supposed to follow: once past, the file is hidden from clients and the public site "but staff keep full access to view, download, and manage it". The second half is not true of a client-scoped staff member. StaffLibraryScope::buildFiles() builds their library as their own uploads union what each assigned client may see, and that second half runs through File::scopeVisibleToClient, which ends in notExpired() -- a client-side rule. Measured on main, with a rep holding one client and a file the administrator uploaded and shared with that client: before expiry in_library true GET .../download -> 200 after expiry in_library false GET .../download -> 403 the rep's own expired upload in_library true an unscoped administrator, same expired file in_library true Api\FilesController says it the same way -- "Only the client branch of the visibility rules drops them" -- which reads as though a staff caller is unaffected, when a client-scoped one is reached through that very branch. This does not change that behaviour. c8078f65 weighed exactly this and decided against it: widening it would mean a library query that keeps expired rows, and scopeVisibleToClient is the single source of truth for client file access, the highest-stakes function to go changing for a dashboard widget. The widget was relabelled instead. That decision lives in a commit message and in one widget's label. Nothing in the code said it, and the docblock nearest the rule went on promising the opposite -- which is how the next person re-derives "staff keep full access" and widens something. So both comments now state the boundary and why it is where it is, and ExpiredFileStaffAccessTest makes it executable: an unscoped staff member keeps an expired file, a client-scoped one keeps their own expired upload, a client-scoped one loses a client's file when it expires. Not changed: scopeVisibleToClient, StaffLibraryScope, and the expired-files widget. If the boundary should move, that is a separate conversation and a separate change. Counter-check inverted, since these pass on unmodified main by construction -- there is no behaviour fix for them to prove. What they have to do is fail if the boundary moves, so the mutation is the widening itself. Deleting the closing notExpired() call from scopeVisibleToClient turns the file red, 1 failed / 2 passed, and it is the third case, the one carrying the decision, that falls. Suite 2108 passed / 2 skipped, 11416 assertions, PHPStan level 8 clean. Measured on base 06c364d2, where main itself is 2105 / 2.	2026-08-28 06:56:09 +02:00
denkfabrik-li	21cae2acb1	Stop a version link telling people about a file they already had ... FileVersions::link() resolves its notification audience before the merge, and says why: RESOLVED BEFORE THE MERGE, and the ordering is the whole dedupe: these are the people who could already see both files, so anyone the merge below is about to reach for the first time is excluded here and gets file_shared from FileSharing::assign() instead. Resolve it afterwards and every newly-added client receives two emails about one action. The merge then undoes it. moveAssignmentsToRoot() hands every one of the revision's targets to FileSharing::assign(), under a comment claiming that firstOrCreate makes a target the root already has a no-op. It makes the assignment row idempotent; the three side effects under it -- activity entry, in-app notification, digest -- run unconditionally. Measured on main: client already holds the root and the revision, then both are linked file_shared (Report) <- wrong, they have had it all along file_new_version (Report v2) <- right assignment rows on the root: 1 client holds only the revision, then both are linked file_shared (Report) <- right, the merge does hand it over Two notifications for one action, for exactly the people the early resolve was meant to protect. So a target the root already holds is skipped rather than handed to assign(). Nobody is gaining access in that case, and the activity entry would be as untrue as the notification. copyAssignmentsFrom() directly below already states that rule for its own case, which is why it inserts directly instead of going through FileSharing. Both stale comments are corrected with it. Not changed: FileSharing::assign() itself, and so the behaviour ShareNotificationsTest pins -- re-posting an existing assignment through the share endpoint still notifies again. That test names the condition for ever changing it, "it should stop being sent for both at once", and that is a decision about files and folders together. This is narrower: a version merge is not somebody choosing to share again, and it already had a stated intent to send exactly one notification. Three cases in ShareNotificationsTest -- the target already on the root, the target gaining it, and a group already on the root. Reverting FileVersions alone leaves 2 failed / 8 passed in that file; the middle case passes without the fix, because it guards against skipping too much rather than against the duplicate notice. Suite 2108 passed / 2 skipped, 11415 assertions, PHPStan level 8 clean. Measured on base 06c364d2, where main itself is 2105 / 2.	2026-08-28 06:56:09 +02:00
denkfabrik-li	fc5651faad	Check the read half of the redirect rule at every door, not one ... Three middleware answer before HandleInertiaRequests and so have to repeat its 302→303 upgrade themselves: EnsureSetupIsComplete, EnsureUserIsActive and EnforceTwoFactor. This file has a write case for each, and the rule has a second half -- a read still gets a plain 302, because a 303 there would be an upgrade nobody asked for. That half was checked once, on the deactivation door, under a name that said otherwise: "leaves a read alone in every one of those cases". The setup door and the two-factor door were not covered at all, so a change that upgraded reads at either of them would have gone through with the suite green and this test's name still claiming it would not. Both are covered now, as a dataset with one case per door. The setup case reads a guest-reachable GET for the same reason the write case posts to /timezone: anything behind `auth` is answered by the guest redirect before EnsureSetupIsComplete sees it. No production code changes; today all three doors answer a read with 302, which is what the new cases assert. Demonstrated by mutation rather than reversion: making EnsureSetupIsComplete upgrade every redirect to 303 fails this file (1 failed / 8 passed) and passes the old one (7 passed).	2026-08-28 06:40:54 +02:00
denkfabrik-li	b838036a9a	Set the directory permission Flysystem actually reads ... FILES_WEB_SERVER_READABLE asks for 0755 on the directories a download has to be traversed through, and asks for it from a key that is never consulted. FilesystemManager::createLocalDriver passes `directory_visibility ?? visibility ?? private` to PortableVisibilityConverter::fromArray() as the default visibility for directories. This disk sets `visibility` to public two lines above, and no `directory_visibility`, so directories are public and the converter reads `dir.public`. The configuration names only `dir.private`. The mode is 0755 regardless, because 0755 is Flysystem's default for a public directory -- the right answer from the wrong place. Adding `directory_visibility` to this disk, or a change to that default, is all it would take for the flag to stop doing what it says. Measured on main, with the flag on: dir.private 0755 → 0750 directory stays 0755 (nothing reads it) dir.public 0755 → 0750 directory becomes 0750 (this is the key) Both are named now, so the intent survives either way round. FilePermissionsTest could not have caught this, because it was not testing this configuration. filesDiskWith() restated the shipped branch inline, verbatim down to the 0755, so it went on passing against its own copy however the real one changed. It now requires config/filesystems.php and replaces only the root, which is what makes the mutation above visible to it. Two more things in the same helper, both about the suite rather than the subject: the scratch root is per worker now (Tests\TestCase does the same for upload parts, and eight workers sharing one directory means one worker's afterEach deletes another's tree mid-test), and it is cleared before each test as well as after, so a killed run does not poison the next one.	2026-08-28 06:40:54 +02:00
denkfabrik-li	02eafb473b	Refresh a mailbox on the schedule under the lock a send would hold ... freshAccessToken() serialises refreshes per connection, and its comment says why: both providers rotate the refresh token as they hand out an access token, so the token is good for exactly one use, and "a worker racing the nightly refresh command means the slower one spends a token the faster one has already replaced. The provider answers that with invalid_grant, which is the same thing it says about a genuinely revoked grant: last_error gets written, the settings page turns red, and every admin is told to go and re-consent a connection that was never broken." The nightly refresh command called refresh() directly, outside that lock. It was the racer the comment names, not a party to the arrangement it describes. It now goes through refreshSerially(), which takes the same lock -- named once, in one place, for both callers -- re-reads the row inside it, and refreshes. Unlike freshAccessToken() it refreshes a token that is still usable, which is the point of the daily run: a delegated refresh token dies of disuse and this keeps the window sliding. The lock is taken rather than waited for, unlike the send path. Nobody is standing at a screen for a scheduled job, and a held lock means somebody is refreshing this very connection right now -- which slides the window and establishes its health just as well as doing it again would. One test: with the lock held, the command sends no token request and leaves the connection untouched. Without the fix it spends the refresh token the holder is already spending.	2026-08-28 06:40:53 +02:00
denkfabrik-li	674781e57a	Claim a TOTP code atomically instead of checking then writing ... verify() asked Cache::has(), verified, then Cache::put(). Between the read and the write the key is free, so two requests carrying the same code could both be told yes -- which is exactly what the replay guard exists to prevent, and the window an intercepted code has is the whole of its validity either side. The claim is now the answer: Cache::add() writes only if the key is absent, so of two requests carrying the same valid code exactly one gets true back. That is the same mechanism, for the same reason, as the preview log's debounce -- "Cache::add is the whole mechanism: it writes only if the key is absent ... without a read-then-write race between two of them". Verification still happens first, so a wrong code never touches the cache and cannot burn the window for the code the person is about to type correctly. One test, modelling the interleaving it is about: the winner's claim has landed, and the loser's has() answers from before that write. Without the fix the loser is signed in.	2026-08-28 06:40:52 +02:00
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.	2026-08-28 06:40:51 +02:00
denkfabrik-li	17fc9ff4cb	Compare the transfers window against the column's own timezone ... resolveTransferRange() builds every boundary in the viewer's zone, which is right and deliberate: "last week" should end when their evening does. Its docblock then claims the instants "compare against the UTC column directly". They do not. The query builder formats a Carbon in whatever zone the object carries and drops the offset, so the viewer's midnight arrives at the database as a UTC string. For Asia/Tokyo, measured: the instant the window really starts 2026-08-21T15:00:00+00:00 what the query asked for 2026-08-22 00:00:00 Nine hours at each end, in the same direction: the first nine hours of the viewer's window are missing from the chart, and the last nine hours of somebody else's day are counted into it. Every zone east or west of UTC gets a chart that is quietly wrong at both edges, which is worse than one that is obviously wrong. The comparison now converts; the day cursor a few lines below does not, because that half genuinely is about the viewer's calendar and is what puts an evening upload on the right bar. One test, in Asia/Tokyo, with an upload in the first hour of the viewer's window. Without the fix it is missing from the chart.	2026-08-28 06:40:50 +02:00
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.	2026-08-28 06:40:50 +02:00
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.	2026-08-28 06:40:50 +02:00
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.	2026-08-28 06:40:49 +02:00
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.	2026-08-28 06:40:49 +02:00
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.	2026-08-28 06:40:48 +02:00
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.	2026-08-28 06:40:48 +02:00