Each entry was three paragraphs explaining itself. Somebody deciding
whether to upgrade reads a list, and a list that takes ten minutes is one
they skim — so the reasoning is gone and the fact is what is left.
What stayed long is Upgrade notes, deliberately: those are the two things
somebody has to *do*, and a one-liner that says "allow headroom" without
saying how much or when is a note they have to come back and ask about.
This makes the section shorter than 2.3.0 and 2.2.1 above it. Those are
published and stay as they are; the style changes from here.
I shipped both daily calls as the same kind of thing — an operator's
preference — and only one of them is. That was wrong in the direction that
matters, because it handed a decision over rather than keeping it.
An update notice on a hosted tenant is useless: they cannot act on it, the
image is ours, and the screen that would show it is closed by capability.
So that check does not run there at all, which is right and unchanged.
News is the reverse. Announcements about the product are exactly what a
hosted customer should be told, and a Cloud client with view_news sees
that card today. One administrator switching it off for everybody on that
instance is not a decision the platform meant to hand over — so on a
managed instance the news now runs whatever any setting says, including a
row left behind by an instance that used to be self-hosted.
Capability::NewsConfigure, Community-only, and the thing it gates is the
*choice* rather than the news. A self-hosted operator keeps the switch,
because there nobody else decides what their installation reaches out for.
An edition difference through the capability registry rather than an
edition check, as everything here is.
Gated in all three places rather than only the screen: the command ignores
the setting without the capability, the controller neither sends nor reads
the field, and the checkbox is absent. There is a test that a hand-crafted
PATCH cannot do what the missing checkbox could not, and the guard is
proved load-bearing — remove it and the managed-instance test goes red.
The changelog and product highlights said "two switches" and now say what
is actually true, including that neither appears on Cloud and why they are
absent for opposite reasons.
A message worth showing was only on the dashboard, which means somebody
who works in Files and Clients all day never meets it. It now also sits
behind an icon next to the notification bell, and that is on every page.
**One shared prop, not two.** "The same message in both places" is the
requirement, and two props would have drifted the first time anybody
edited one — so the hook moved out of DashboardController into
HandleInertiaRequests, and the dashboard reads the same shared value the
header does. The band and the dropdown also share the component that
renders the words, for the same reason: the reliable way to keep two
renderings identical is not to have two.
Renamed with it. ResolvingDashboardCallout was accurate for about an hour
and became a lie the moment it appeared somewhere else; it is
ResolvingAnnouncement now, and the prop is `announcement`. Free to rename
because nothing has shipped yet — the only other reference was
cloud-modules', by string, updated alongside.
The icon follows UpdateAvailableIcon beside it: absent entirely when there
is nothing to say rather than a dead control, and a plain dot instead of a
count, because there is only ever one of these and a "1" would invite
somebody to look for the second.
Two tests worth naming. One asserts the message reaches a page that is not
the dashboard, which is the whole point of the addition. The other asserts
a client is shown nothing even from a listener that sets it
unconditionally — a client's header carries the bell too, and staff
messages must not reach it however careless the listener.
Two seams, in the shape docs/extension-points-architecture.md settles on:
a Laravel event with a mutable payload, dispatched unconditionally, and
with nothing listening the documented default holds. A community
installation gets an empty list and a null callout, which is exactly what
it had before.
ResolvingNavigationLinks exists because the sidebar is a hardcoded array
in app-sidebar.tsx, so a package could not contribute to it at all — the
nav entry was a separate manual edit every time a package grew a screen,
and being manual it was forgotten more than once. Staff-only, decided in
HandleInertiaRequests rather than trusted to each listener: these render
in the administration area, and a client's portal shows their own files
and nothing about the installation. There is a test that a listener adding
unconditionally still reaches no client.
ResolvingDashboardCallout is one band above the widget grid rather than a
widget in it. The grid is a closed list of keys that dashboard.tsx renders
one by one and each viewer arranges, so a message that mattered would sit
wherever somebody dragged it, or under a fold, or switched off. One at a
time, first listener wins: a dashboard that can accumulate banners
accumulates them, and the second is what teaches people to skip the first.
Core learns nothing about what either seam carries. Titles, URLs and copy
all arrive from the listener, and that is not fastidiousness — the first
caller is the hosted edition's link to its own customer portal and its
pitch to free instances, which is commercial copy belonging to one
offering and has no business sitting in the public repository because the
sidebar happens to live here.
An external link renders as a plain anchor opening in a new tab, never an
Inertia <Link>: Link expects a page component back and another origin will
not give it one, so it fails without saying so. It is also never marked
active — nothing outside this app is the page you are on.
Every instance reached projectsend.org twice a day and an operator could
stop neither. The news feed had no switch of any kind — FetchNewsCommand
went straight to the request, touching Settings only to write results back.
The update check had one, but its default is on, and a managed fleet had
been setting PROJECTSEND_CHECK_FOR_UPDATES=false for months against code
that reads no such variable: check_for_updates is a database setting, so
the environment never touched it and updates were enabled fleet-wide the
whole time.
They look like one problem and are two, which is why they are fixed
differently.
**The news feed gets a Setting**, its own key, default on. A Cloud client
with view_news sees that card today — DashboardController gates it on the
permission alone, with a comment saying in as many words that it is both
editions and carries no capability. So switching it off is an operator's
choice rather than an edition's, and it must stay reachable everywhere.
Its own key rather than riding on check_for_updates because they are two
different wants: "do not tell me about releases" and "do not show me the
project's news" are asked separately, and an installation with no outbound
access at all wants both.
**The update check gets a capability guard**, ahead of the setting it
already had, and deliberately not a Setting of its own. On a managed
installation the result is unreachable rather than unwanted: the
dashboard's System card and the update UI are both gated on
Capability::SystemUpdates, which is Community-only, and the image is
chosen by whoever provisioned the instance. A Setting would encode a fact
about the edition as a preference — leaving it switchable back on per
tenant, buying a nightly call for a number no screen can draw, and putting
the reason in a provisioning script rather than beside the code. A
self-hosted install holds the capability and loses nothing: its own
setting still decides.
Both guards return success rather than failure. A scheduled task that was
asked not to run has not failed, and reporting it as one would put a red
line in the scheduler history every night for an installation behaving
exactly as configured.
The news switch is on the General settings screen, outside the
can_manage_updates block that hides the update toggle where the capability
is absent — a setting only reachable by editing a database row is a row,
not a switch. Seven tests, and the two that matter go red when either
guard is removed. Sixteen locales translated in the same commit rather
than left for the pass, since a release is close.
Two entries under Unreleased, both for the same reason: an operator would
otherwise be surprised.
The shorter download link is a behaviour change with a cost attached — a
resumed download more than a minute old is refused where an hour tolerated
it — so it says that plainly rather than only advertising the benefit. It
also says who is not affected, since installations on local disk never used
one of these links at all, and neither do zip bundles.
The upload fix is an ordinary bug fix and would normally need no entry, but
it moves peak temporary disk from "the file plus one part" to "the file
twice over" while assembling. That is a sizing question somebody with a
small temp volume has to answer, so it gets an upgrade note. Nothing to
configure — just headroom.
#1733 explains its two lifetimes by contrasting a presigned URL with
X-Accel-Redirect, "nginx serves these bytes, now, to this request". That
was true when the branch was written and stopped being true on 1 September,
when FileDelivery gave the local path four methods — auto, nginx, xsendfile
and PHP streaming.
The argument survives intact: every one of those authorises exactly one
response and nothing that outlives it, which is the property the contrast
rests on. Only the naming was stale, and a docblock that says "nginx" to
an operator running Apache reads as "this does not apply to me".
Found resolving the merge, not by the author — the branch predates the
change it collided with.
Give a download's presigned URL a minute rather than an hour
Conflicted against FileDelivery, which landed on main after this branch
was written: main added a constructor where the branch added two
constants. Both belong; the resolution keeps each.
The eight strings the portal file editor added, which had been sitting in
English since the feature landed — the deliberate trade, but the pass is
due now that the English has settled.
Nothing else came up. The scan reports eight missing per locale and they
are all from this feature, so no unrelated drift crept in alongside it.
Written against each catalogue's own established voice rather than
translated fresh: Spanish stays informal, and "Expires on" takes the verb
its neighbouring "Leave empty for a file that never expires" already uses
in each language. Quoting follows each locale too — Russian keeps its
guillemets, Japanese its corner brackets, Chinese and Vietnamese their
curly quotes — matching how the sibling folder-deletion warning already
reads there.
Every :name placeholder survives verbatim, checked rather than assumed,
and the diff is additive: the one deleted line per file is the previous
last entry re-emitted with a comma.
Checked on the screen, not only in the file, which is the part a parsing
JSON cannot show: the editor rendered in Spanish with every label and hint
in place, "Vence el" agreeing with the hint below it, and no console
errors. The dev instance's Client role was snapshotted, granted the keys
for the run, and restored; the throwaway file it needed is gone.
Locale suite green, 13 tests. The 249 orphans each catalogue reports are
older than this work and left alone deliberately — scan.php cannot see a
key held as data or supplied by a package, and a wrongly deleted entry
reverts a screen to English in silence.
Found reviewing the client file editor rather than building it.
File::isEffectivelyPublic() is "my own flag OR my folder's", and
Folder::uploadableBy() admits a client to a public folder on
upload_to_public_folders — a different key from upload_public. So a client
can make a file world-readable without touching the public switch, and
without holding the key that switch is behind.
That is what those two keys have always meant and what uploading into such
a folder has always done, so this does not refuse it. What was new is
where the choice is made. The upload page is entered from a folder the
client has already navigated to, where the list shows a Globe badge on a
public folder. The editor's picker is a flat list of names, and it is the
first place a destination is chosen with none of that context — so the
consequence was invisible exactly where it mattered most.
Public folders now carry the badge in the picker, and choosing one says in
words that anyone will be able to open the file without signing in. Two
tests: that the side door genuinely publishes and is labelled, and that a
private folder is not labelled — a warning on everything is a warning on
nothing.
The rest of the review found no defect. Ownership, the per-field keys, the
staff-scope trap and mass assignment were already covered; a client
deleting a file that staff later revised was checked directly and moves
the chain's recipients onto the successor without widening them, which is
what it is supposed to do. The write path was driven in a real browser —
rename, publish and delete through the actual form and dialog — because a
green suite over a write that 419s in every browser is a mistake this
repository has made before. Bytes gone, audit trail complete, and
file.made_public records the slug.
The authorization landed last commit; this is the way in. A client with
edit_files now gets an Edit action on the files they uploaded, opening a
form with every field their role actually grants, and a Delete beside it.
One page for every theme, not one per theme. portal/edit-file.tsx picks
its shell from the `theme` prop exactly as portal/upload.tsx does, because
a form with eight fields behind five separate permissions, rebuilt four
times, is four places for a field to go quietly missing. What *is*
per-theme is only the entry point: one <FileRowActions /> in each theme's
row actions group, the file twin of the FolderRowActions that was already
there.
Row actions gate on can_update/can_delete, sent per file by
MyFilesController and answered by FilePolicy — never on is_mine, which is
half the question. Holding the file is one half and the role's keys are
the other, and a theme that reads is_mine offers an Edit button that
403s. Written into docs/theming-files-checklist.md so the next theme does
not have to rediscover it.
The folder picker offers only folders the client could have uploaded to,
so it cannot present a destination the save would refuse. Publishing says
in plain words that anyone with the link will be able to open the file
without signing in, and says so differently when the installation has no
public page configured, because there the switch would do nothing visible.
Hiding a control is a courtesy, never the enforcement. Every can_* prop
here is the same question ApplyFileEdits asks when the form posts, and the
tests assert both ends.
Verified in a real browser over CDP rather than only by types and tests,
which say nothing about whether a page mounts: 23 edit actions on the
client's 23 own files and none on the file shared with them, the editor
mounting with its real values, every gated field present, no console
errors. The dev instance's Client role was snapshotted before the run and
restored to exactly what it was.
Refs #1771
A client could upload a file and then never touch it again. No rename, no
description, no expiry, no categories, no delete — the portal has three
file routes and all three are GET. Meanwhile the Roles screen happily
grants the Client role edit_files, delete_files, set_file_categories,
set_file_expiration_date and upload_public, and every one of them was
inert, because the routes that honour them are `staff`-gated rather than
permission-gated. That is what #1771 hit: a permission granted, saved, and
silently doing nothing.
A client owns what they uploaded. Ownership is now what lets them edit and
delete it, subject to the same per-field keys staff are subject to.
The obvious implementation is a trap, and it is worth writing down. Both
policy methods began `if (! $user->isStaff()) return false;` and both end
in StaffLibraryScope, whose allowsFile() reads `if (! isClientScoped())
return true` — and isClientScoped() is `isStaff() && role->client_scoped`,
so it is false for every client. Delete the early return and a client
falls into the branch meaning "this staff member is unrestricted" and is
handed the whole library. Same for folders(), which returns an unfiltered
query: a client could move their file into any folder on the installation.
So clients get their own branch, reaching neither. The portal asks
Folder::uploadableBy() instead — a file cannot be moved somewhere it could
not have been uploaded.
edit_others_files and delete_others_files stay inert for clients by
construction. A client has no others' files, only files somebody showed
them, and being shown a file is not being given it.
Which fields an editor may write moved into ApplyFileEdits, shared by the
staff editor, /api/v1 and the portal. There were two copies of the same
eight permission checks and this would have been the third; the checks are
easy, which is exactly why the drift would have been invisible. Callers
normalise their own request shape, this gates and writes and logs. Expiry
reading and writing came along too, as FileExpiry — three copies, of which
only the API's could read a timestamp.
Clients do not choose the public slug. It is derived from the name they
already picked, because an installation-wide unique slug a client sets is
a name to squat and an existence oracle to probe with.
One consequence for later, written up in docs/api-todo.md: the policy now
says yes to a client for file writes, so `staff-token` is the only thing
holding the API boundary where there used to be two independent refusals.
ActorBoundaryTest pins it, and asserts the policy passes first so the test
cannot quietly stop testing the middleware.
Also corrects a stale comment that claimed a deleted file's bytes stay on
disk. They have not since File::booted() grew a `deleted` hook; nothing
ever forceDelete()s a File row, so "until a purge lands" would have meant
never — which is why a client's delete frees their quota by exactly what
it frees on disk.
The UI comes next; this is the authorization, the routes and the tests.
Fixes#1771
The command writes Setting::CaptchaProvider = 'none'. On an installation
using the platform's managed keys, Captcha::resolve() returns
managedConfig() — read from config — before it ever looks at that setting,
so the write lands somewhere nothing reads and every form stays protected.
The command then printed "CAPTCHA is off". That is false in the worst
direction: the person running this is locked out and debugging, and the
message sends them away from the one thing that would have explained why
they are still being challenged.
It now says it changed nothing, and names PROJECTSEND_CAPTCHA_DISABLED,
which is checked ahead of the key source and is therefore the only one of
the two escape hatches that works on a managed installation. The docblock
said those two were equivalent; they never were.
Deliberately not gated behind captcha.configure. Gating it would take a
self-hosted operator's way back in — the alternative being a hand-edited
database row — to close something that on a managed installation does
nothing anyway. Reaching it needs a shell in the container, which needs an
RCE, at which point the CAPTCHA is not the problem.
The command had no test at all. It has three now, including one that pins
the ordering inside resolve(): if the environment check ever moves below
the key source, a locked-out operator loses their last way in.
The screen is open in both editions and stays that way by default, so a
self-hosted installation loses nothing: nobody else supplies its keys, and
nobody else is affected by what it decides.
What the key buys is the ability to take it away. A hosted fleet puts every
tenant on one parent domain and one sending reputation, so an administrator
who turns their own CAPTCHA off is spending everybody else's deliverability
rather than only their own. That is not the shape LDAP and social login
have, which is why those two stay ungated and this one does not.
Gated all-or-nothing on the route, read included, exactly as Storage and
Branding are. Per-field gating in the controller would not have closed it:
switching the CAPTCHA off needs none of the gated fields — `provider: none`
does it, and so does unticking the four per-form switches while leaving good
keys in place — so the PATCH had to be closed too, and the middleware closes
both verbs at once. Which keys the screen may offer is still the separate,
narrower question Capability::CaptchaManagedKeys answers per field.
An operator withdraws it by naming captcha.configure in
PROJECTSEND_CAPABILITIES_DISABLED. Note that the key also joins the list
`projectsend:status` and GET /api/v1/me report, which is additive — the
OpenAPI document types capabilities as an untyped array, so nothing there
needed regenerating.
ProjectSend Cloud already appears in LICENSING.md and CONTRIBUTING.md,
but not in the README — the first thing people and search engines read.
One paragraph after the intro names it, says who runs it, and points to
LICENSING.md for where the line between the free core and Cloud sits.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A staff member limited to their own assigned clients could read the names
and ids of clients on nobody's roster but their own, out of ordinary file
metadata.
The file boundary was never wrong. Sharing means a file can legitimately
reach a scoped viewer through client A while client B uploaded it, or
while B also receives it -- StaffLibraryScope::buildFiles is right to
permit that, and a B-only file is still a 403. What was wrong is that
every response then went on to name B. FileResource serialised the loaded
uploader and each assignment unfiltered; ShareTargets::assigned took no
viewer at all, so the details panel published the recipient list as it
stands and forSubject narrowed available_clients while handing
assigned_clients straight through. FoldersController::fileRow,
FilesController::edit, FileDetailsController and ClientFilesController
each named the uploader the same way. The API's uploaded_by filter asked
the question without any name attached: it answered "does this client of
yours put files in front of a client of mine" for any id a caller cared
to try.
12a8ebe3 said the rule out loud while fixing topClientsByStorage -- "the
file was theirs to read and the uploader's name was not theirs to see" --
and then the rule stayed in that widget. So it is a class now.
ClientIdentityScope is the one decision, asked by every surface that
names a client, and it deliberately answers about clients only: a
colleague's name is not a client identity, and hiding it would hide who
uploaded most of the library from the people who work in it. Groups go
through it too, on the same argument -- a group is a list of clients
wearing one name -- which the report did not cover but is the same leak.
Two judgement calls worth naming. assigned() keeps returning the whole
truth and gains a warning, because VisibleCommentScope resolves
notification recipients from it and a recipient filtered out of that list
is one who never hears about a message addressed to them; assignedFor()
is the display half. And FileResource asks at serialisation rather than
in its callers' eager loads, which is the opposite of how the version
counterparts next door are narrowed: that one is set-shaped and folds
into a query, this one is a per-row roster check across eight call sites
in four controllers, two of them re-loading assignments after a write.
The tests assert on whole response bodies rather than on named keys. The
leak was never in one field -- the same name arrived through the
uploader, through the recipient list and through four screens -- so a
body that does not contain the name anywhere is the only assertion that
would have caught all of it. Ten of the eighteen fail without this
change; the rest are the negative controls, including that an unscoped
administrator still sees every name and that the uploaded_by filter still
works for a client on the roster and for staff.
Reported by @Noorkhalel, GHSA-whmp-p9hv-r7j7. Their write-up named every
affected surface and the root cause in each, which is most of why this
took one pass.
Branding moved into the application on 2026-08-28 and its two read-only
endpoints came with it unchanged -- same paths under
/api/v1/modules/branding, same capability, same ability. Their
documentation did not: the guide still sent readers to
packages/cloud-modules/docs/api.md, so endpoints that every installation
now carries were described in a private repository almost none of their
callers can open.
The OpenAPI document is still not the place for them. OpenApiContractTest
skips api/v1/modules/* on purpose: that document is served
unauthenticated and has to be identical on every installation, while a
module's paths exist only where the module does. So this is a plain
markdown file beside the guide, and published like it -- ignored docs are
maintainer notes, and this one is for integrators.
It is the cloud-modules file moved across, minus the attribution switch:
that half stayed Cloud-only and has no API surface at all. The gate is
described as every edition holding branding.customize, with a hosted plan
able to subtract it, because that is what the enum now says.
The comment said a reader unmarshalling a map breaks on an empty array.
Checked against the reader since, and it is worse than that: the hosted
platform decodes the block into a typed struct and discards a block it
cannot read, and Go refuses a JSON list into a map outright. A [] here
loses the whole usage block -- downloads and uploads with it -- on the
day a tenant happens to have no counted activity, with nothing logging a
fault. The quietest installations would be the ones that went quiet.
Comment only. The cast was already right; what was missing was the
reason it is load-bearing, which is exactly the kind of condition this
week kept proving nobody had written down.
Turns the Unreleased section into a numbered entry and adds this
cycle's work to it: downloads on Apache and LiteSpeed, branding in
core, the build fact, the scheduler check, and the run of boundary
fixes.
The three security entries already in Unreleased are carried across
word for word rather than summarised. Their "Who this affected"
paragraphs are the part a reader decides on, and a one-line retelling
would have thrown that away. Their two upgrade notes move across whole
for the same reason, joined by the two this release adds.
Leaves an empty Unreleased scaffold for the next cycle. The version is
not stamped anywhere else yet -- config/projectsend.php and the tag are
still on 2.2.1, so this is the entry waiting for a release rather than
a released one.
The 29 English strings added by the file-delivery work: the System
widget row and its explanation dialog, and the settings panel that
repeats it.
Product nouns left alone throughout -- PHP, nginx, Apache, LiteSpeed,
X-Sendfile, mod_xsendfile, XSendFilePath, PROJECTSEND_FILE_DELIVERY,
INSTALL.md, S3, Google Cloud. "PHP" as a whole string stays "PHP",
which is why every locale gains one entry counted as untranslated, the
same way API and OK are.
Purely additive: appended rather than merged in sorted position, so each
diff is 29 new lines plus a comma on the line that used to be last.
Verified: scan reports 0 missing in all sixteen, 13 locale tests pass,
and the Spanish settings screen was read out of the running app rather
than out of the file.
Found reviewing the delivery work. The path is written into
X-Accel-Redirect or X-Sendfile, and a CR or LF in a header value is
header injection. PHP's header() refuses to emit one, so the real effect
is a 500 on every download, preview and thumbnail of that file rather
than a split response -- a file permanently broken by its own name.
Paths are generated here as Y/m/{uuid}.{ext}, so this should be
unreachable. The extension is not generated: it comes from the
uploader's filename, and on a migrated installation from a v1 database.
The upload routes all check the extension against an allowlist, which no
control character can match -- but upload_type_restriction can be set to
none, and the importer does not consult that policy at all.
assertRelative() was documented as the backstop for what a path may be
and only covered traversal, which is the half that cannot happen here.
Low severity, and the guard should have covered it either way.
Same amber, same underline, same warning triangle as the System widget
row. The two say the same thing about the same installation, so looking
different made the settings one read as an ordinary footnote rather than
the thing to go and read.
The warning triangle alone did not read as clickable. The label is now
an underlined button and the value and icon are a second one, so either
half opens the dialog and the row looks like the one thing on this card
you are meant to act on.
Two buttons rather than one wrapping the row: a dt/dd pair cannot be
nested inside a single button without losing the description-list
semantics that tie the value to its name. The value button carries an
aria-label because "PHP, button" describes nothing on its own.
Only when PHP is sending the files. On the fast path the row stays plain
text, since there is nothing to go and read.
It was 2975 characters and scrolled. Same four questions answered and
nothing dropped -- what is happening, what it costs, why it is set that
way, the three ways out -- in 1696, which fits the dialog without
scrolling. The three fixes are a list rather than three headed
paragraphs, since each is one instruction.
A wall of text explaining a performance trade-off is self-defeating: the
person who most needs to read it is the one who opened the dashboard for
something else.
Uploads live outside the web root, so PHP authorizes every download and
then hands the file to the web server with a header naming it. Four
routes decided that for themselves and all four hard-coded nginx's
spelling. On Apache or LiteSpeed nothing acts on the header, so the
empty body PHP sent goes to the visitor: files upload fine, thumbnails
are broken images, and downloads arrive as 0 bytes, with every other
page working. Reported as #1765 from an Apache 2.4 install, and before
that as #1266, #1215, #870 and #1271.
It is also a regression from v1, which had a download_method setting --
php, apache_xsendfile, litespeed, nginx_xaccel -- defaulting to php. v1
therefore worked on any server out of the box and v2 did not, and a v1
Apache user migrating lost every download with nothing to tell them why.
So the four sites now go through one FileDelivery, and it picks:
auto (default) nginx when SERVER_SOFTWARE says nginx, else php
nginx X-Accel-Redirect, a URL path via the internal location
xsendfile X-Sendfile, an absolute path (Apache mod_xsendfile,
LiteSpeed)
php BinaryFileResponse
Defaulting to auto rather than nginx is the point of the change: a
default that assumes nginx leaves an Apache install exactly as broken as
it is today until somebody reads INSTALL.md. Slow beats empty.
Auto never picks xsendfile, even where the module is loaded.
mod_xsendfile also needs XSendFilePath to allow the storage directory,
which cannot be seen from here, and choosing it on the strength of the
module being present would trade a silent failure an administrator can
diagnose from the dashboard for one nobody can.
BinaryFileResponse rather than a readfile loop because it answers Range
requests. nginx does that itself on the fast path, so hand-rolling it
would have broken seeking through a video on exactly the installations
this fallback exists for. Verified end to end: 206 with the right
Content-Range through the live stack.
Two guards. Every method checks the path cannot climb out of the storage
area -- nginx resolves `..` in the URL it is handed as happily as PHP
would -- and the two methods that hand over a filesystem path resolve it
and prove it lands inside the root. Callers pass paths from rows they
just authorized, so this is a backstop; it is here because the cost of
being wrong once is handing over any file the web server can read.
The dashboard's System panel names the method, with a warning icon and a
dialog when PHP is doing the sending: what is happening, what it costs
(one worker held for the whole of each download, so a few large
simultaneous ones can occupy every worker while the processor sits
idle), why it is set that way, and the three ways out. Written to be
accurate rather than reassuring -- nothing is broken, it does not scale
-- and the notice stays even when php was chosen deliberately, because
the trade-off is the same either way. /system/settings/downloads repeats
it, which is where somebody coming from v1 goes looking for the
dropdown.
An environment variable rather than a stored setting: it describes the
server this installation runs on, not a preference, and a value in the
database travels to a different server in a restore and is wrong there.
Read only in config/projectsend.php, so config:cache cannot blank it.
The suite pins itself to nginx. Left at auto it would detect no server
at all, fall back to php, and quietly retire the coverage of the
mechanism most installations actually use.
projectsend:status could say what an installation holds and how many
accounts it has, but nothing about whether anybody was using it. Adds a
`usage` block -- downloads split staff/clients/anonymous, uploads, and
five allowlisted action counts -- plus `activity.last_client_login_at`,
`health.scheduler` and `health.failed_jobs_latest_at`.
The scheduler is the one worth having on its own. `health.queues`
catches a dead worker; nothing caught a dead scheduler, and its first
symptom is not a stalled feature but an expired file that is still
downloadable, because the job that was going to remove it stopped
running weeks ago. Nothing about the installation looks wrong while that
is true.
`failed_jobs_latest_at` exists because the count beside it cannot say
whether anything is wrong *now*, and reading it as though it could is a
category error rather than a threshold wanting tuning. The table is
swept daily, so the count spans a retention window -- one the
installation chooses, and one that can be set to keep-forever by
somebody who treats a failed job as evidence rather than debris. Two
identical installations therefore report different numbers, and on a
keep-forever one the count grows until any fixed threshold trips. A
timestamp is independent of how long rows are kept: 27 failures whose
newest is three weeks old is an installation that has been healthy for
three weeks and has not been swept yet.
`usage` is a rolling window with no lifetime totals, and that is a
correctness decision rather than a presentational one: activity_log is
never pruned, so a lifetime count over it gets slower every day of the
installation's life while a windowed one stays flat. The window is
emitted as `window_days` rather than left for the reader to assume.
The actions are an allowlist, not a `group by action`. This document
leaves the installation and Action gains cases most weeks, so an open
group-by would ship new action names outward with nobody having decided
they should go -- and some of them (account.erased, two_factor.reset)
are somebody's compliance event, not a business metric. It is also ~30x
cheaper: five keyed counts ride (action, created_at) while a group-by
starts from created_at and reads rows. The scheduler's failure message
and the queue exception text are omitted for the same reason; they are
the fields here that can carry a path or a stack trace, and a count with
a timestamp says "go and look", which is all a watcher is owed.
The two indexes ship as a pair and the migration explains at length why.
Measured at 2.1M rows: adding (action, created_at) alone fixes the
windowed counts and takes last_staff_login_at -- already running hourly
on every tenant -- from 0.63s to 7.7s, because the planner switches to
it, still needs actor_type, and does a scattered primary-key lookup per
row. With both, that query is answered from the index without reading a
row at all (0.0004s) and the whole new usage block costs ~70ms.
Also documents the keys as a contract, the way `capabilities` already
is. This one fails worse: a renamed capability key breaks a comparison
somebody is watching, a renamed usage key produces a chart that is
silently empty, and nobody gets paged for a flat line.
#1762 put six screens through the translator and deliberately left the
new keys for a focused pass; this is that pass, plus the one older gap
the scan turned up -- the directory-account notice on the password form.
Twenty-seven keys each, written to match what the catalogue beside them
already says: de and tr formal, es, nl, pl and zh_CN informal, and each
locale's own established vocabulary rather than a fresh choice per file
(nl keeps wachtwoord, tr keeps parola, ru keeps "адрес эл. почты").
"Or, return to" and "log in" are rendered as one sentence with a space
between them, so each pair was chosen to read as a phrase in that
language rather than translated word by word -- Turkish reorders it to
"Ya da giriş sayfasına dön", Japanese to "または ログインに戻る".
Additive only: 432 insertions, nothing reordered or reformatted. The
scan now reports zero missing across all sixteen.
use-translation.ts states the rule: every user-facing string in a
component must go through t(). Five screens never called it at all --
forgot-password, reset-password, confirm-password, verify-email and
settings/password had zero occurrences of useTranslation -- so a client
who had chosen Spanish reset their password in English, from the browser
tab down to the submit button. settings/profile had the hook but used it
for two strings, leaving its heading, labels and the whole
email-verification notice hardcoded around them.
The password page also carried a second, smaller mistake the miss was
hiding: its <Head> title said "Profile settings", copied from the
profile page, so the tab named the wrong screen in every language.
It says "Password settings" now, the wording its own breadcrumb and
the sibling "Notification settings" title already use.
Every string on the six screens goes through t() now. The two
module-level breadcrumb arrays moved inside their components to reach
the hook -- the shape two-factor, notifications and the other settings
pages already have. Where a key already exists in the catalogs (Email
address, Password, Confirm password, New password, Log out and friends,
shared with the login screen) the existing translations light up
immediately; the keys new to the catalogs fall back to their English
text, exactly what those lines rendered before, until the locales pick
them up.
TranslationUsageTest is the guard, a source scan like
DateFormattingUsageTest and for the same reason: no JavaScript test
runner gates this class of miss. It fails on any page under pages/auth
or pages/settings that never uses the hook -- those screens always carry
copy of their own, so a page there without it is a page somebody forgot
-- and on any literal <Head title="..."> anywhere, which is both a
user-facing string and where the copy-paste title above lived. Both
scans go red on the tree without this change: five pages and six
literal titles.
The frontend translator replaced placeholders by exact match only:
`:${name}`, nothing else. But the catalogs it consumes are Laravel JSON
catalogs, and Laravel's convention has always been three forms — :name
receives the value as-is, :Name capitalized, :NAME upper-cased. The
backend translator honours all three; fifteen values in lang/nl.json
and one in lang/tr.json already rely on it. Dutch writes "Add :name" as
":Name toevoegen" because the noun opens the phrase there and gets the
capital; Turkish does the same with "Go to page :page" as ":Page
sayfasına git". Through this hook, those sixteen values rendered the
literal ":Name" and ":Page" instead of the replacement — the value was
right for the language and wrong only for the half of the app that
reads it with an exact-match replace.
t() builds the three variants per replacement now, longest placeholder
first — strtr's implicit rule made explicit, so with :name and :names
both in play, :name cannot eat the front half of :names. Ties keep
insertion order, which resolves a fully-colliding key to the as-is
value, the same answer the backend's assignment order produces.
No test accompanies this: there is no JavaScript test runner in the
project, and the PHP suite exercises the backend translator, which was
never wrong. Counter-checked by running both implementations over the
affected catalog values in node — the old replace leaves ":Name
toevoegen" and ":Page sayfasına git" literal, the new one renders
"Bestand toevoegen" and "2 sayfasına git" — plus the existing in-repo
call shapes (":used of :limit", ":name — files"), which come out
byte-identical to before.
useZipDownload sets an interval that polls zip-downloads/{id} every two
seconds until the build reports ready or failed. The only paths that
ever cleared it were those two answers and close() — there was no
unmount cleanup at all, no useEffect in the file. But the pages that
hold the hook are Inertia pages: navigating away unmounts them without
close(), and the interval keeps hitting the endpoint every two seconds
for as long as the tab lives, polling for a download nobody can receive
any more. A zip stuck in pending — the exact case the polling exists
for — polls forever.
Three holes, one leak:
- No cleanup on unmount. A useEffect returning stopPolling closes the
main path.
- An unmount while the store POST is still in flight: its then() runs
after the cleanup already did, and would set a fresh interval on the
dead component. The unmounted flag makes that then() a no-op.
- A second start() while a poll is running overwrote pollRef and
orphaned the first interval the same way. start() stops the previous
poll first now.
No test accompanies this: there is no JavaScript test runner in the
project, and the PHP suite never mounts a component. Verified with
tsc, eslint and prettier, and by reading the two consumers —
files/index.tsx and use-portal-files.ts — both of which only ever
clear the interval through the dialog's onClose today.
#1760 changes how an existing installation behaves on the next pull, and
the part worth saying out loud is the one nobody could see: "reject
known-breached passwords" was reporting itself as on while doing nothing.
Filed under Security with who was actually affected -- not anyone
following the documentation -- and under Upgrade notes with the one thing
that stops working: APP_ENV and APP_DEBUG edited inside storage/.env.
#1759 changes a file people copy verbatim, so the change has to reach
them somewhere other than a diff: anyone who copied the old example and
reaches the app on <server-ip>:8080 will find it stops answering.
Filed under Security with the reason it was a finding at all -- a
published Docker port is not covered by a host firewall, so the port was
often open without anyone intending it -- and under Upgrade notes with
what to do when the proxy lives on another machine.
app.blade.php is the root template for all three interfaces, and it opened
with two lines pointing at a third party:
<link rel="preconnect" href="https://fonts.bunny.net">
<link href="https://fonts.bunny.net/css?family=instrument-sans:400,500,600" rel="stylesheet" />
Every visitor to /login, /register, /forgot-password, /s/{token} and every
public listing page therefore made a request to a host the operator did
not choose and could not switch off, before they had done anything at all
-- handing it their IP address, their user agent, and through Origin the
hostname of the installation they were visiting. On the signed-out pages
that is a visitor who has agreed to nothing, and an operator who often has
told their own users that this server is where their files live.
There was no self-hosted copy in the repository, no setting, no mention in
INSTALL.md, DOCKER.md or SECURITY.md, and no SRI on the tag.
The font now ships with the application, through @fontsource/instrument-sans
-- the same font, the same three weights the URL asked for, from a
versioned dependency rather than binaries pasted into the repository.
Vite fingerprints and emits them like any other asset.
Cost, measured on this build: twelve files, 192 KB on disk. A browser
fetches only woff2 and only the subsets it needs, which is 73 KB for all
six woff2 files together and typically 41 KB (latin, three weights) for a
page in English. Against that, every page load loses a DNS lookup, a TLS
handshake and a round trip to another origin, so signed-out pages get
faster rather than slower.
This is a privacy change rather than a vulnerability fix, and worth saying
plainly: the share token does not leak this way. Referrer-Policy:
strict-origin-when-cross-origin is set in both nginx configs and in the
INSTALL.md snippet, so the path never travelled in the Referer. What
travelled was the visit itself.
Not changed: public/.htaccess still sets no security headers at all, so an
Apache installation has no Referrer-Policy. That is a real gap and a
separate change.
Verified: `npm run build` succeeds and emits the faces; no reference to
the CDN survives anywhere in public/build; `tsc --noEmit` and prettier are
clean. No test asserts on the font, before or after.
ConfirmablePasswordController checked the local hash and nothing else:
Auth::guard('web')->validate(['email' => ..., 'password' => ...])
An account provisioned from a directory has no local password. It holds a
Str::password(64) generated at provisioning time that nobody has ever
seen, and the application knows this -- LdapAuthenticator::isDirectoryAccount()
is the question, and the sign-in form asks it before deciding what to
check. This screen did not, so it refused those accounts the only password
they have.
That is not a cosmetic refusal. `password.confirm` stands in front of
enrolling in two-factor, so a directory-provisioned client could not enrol
at all. Set TwoFactorEnforcement to `clients` or `all` and EnforceTwoFactor
redirects every request they make to two-factor.show -- a screen whose
"enable" button leads to a door they cannot open. PR #1708 fixed the
routing half of that ("Let an enforced user reach the far side of the
confirm-password screen"); this is the credential half.
The rule now lives in one place. PasswordVerification is the sibling of
SignIn on the other side of the line SignIn draws -- SignIn is everything
after a credential checks out, this is the one question asked before it --
and it exists for the reason SignIn gives for existing: "the way they get
broken is by being written twice". LoginRequest keeps its ordering, its
provisioning and its rate limiting, and delegates the check itself.
Behaviour preserved exactly on the sign-in path: local hash first so an
account that answers locally generates no directory traffic, directory
only for accounts whose credentials live there, the stale-hash re-hash on
the local branch only, and the ldap_dn stamp on the directory branch. All
23 existing LDAP sign-in tests pass unchanged.
One thing this closes on the way past. Because the old check went straight
to the local hash, a directory account's placeholder *would* have confirmed
if anybody ever learned it -- a door the sign-in form does not have, since
it skips the local branch for those accounts. It now behaves the same on
both screens; there is a test.
**What this does not fix, and should be read as a limitation.** Accounts
provisioned by a social provider are in the same position -- a random local
password nobody holds -- and they are not directory accounts, so this
changes nothing for them. Their route to a local password is the password
reset, which #1748 made work end to end by moving auth_source to Local when
the reset completes. A social account that has never done that still cannot
confirm a password, and so still cannot enrol in two-factor.
Tests: three fail against the unfixed pair, including the placeholder case
above. Two more pin what must not change -- a wrong directory password is
still refused, and a local account with LDAP switched on still confirms
against its own hash.
The entrypoint seeds the .env on the storage volume when there is none:
if [ ! -f storage/.env ]; then
cp .env.example storage/.env
.env.example is the development template. It carries APP_ENV=local and
APP_DEBUG=true, and the Dockerfile set no defaults of its own -- its only
ENV was PROJECTSEND_IMAGE=1.
Real environment variables win over that file, so compose.example.yaml
(APP_ENV: production, APP_DEBUG: "false") was never affected, and neither
was anybody following the documentation. Everybody else was: `docker run`
with nothing but a database address, the Portainer / unRAID / TrueNAS
templates people actually use, a Kubernetes manifest naming only
DB/Redis/APP_URL. All of them booted a debug build and nothing said so.
Two things follow, and neither is visible from inside the application:
1. **Every 500 hands its stack trace to whoever caused it**, signed in or
not -- Laravel's exception page, with the file, the line and the
surrounding source. docker/production/php.ini sets display_errors=Off
and that does not help, because Laravel renders the page itself rather
than letting PHP print it.
2. **"Reject known-breached passwords" never ran.** PasswordPolicy::rule()
appends ->uncompromised() only when app()->isProduction(). On
APP_ENV=local an administrator could switch the setting on, watch
descriptor() advertise it on every password form and the security
settings screen report it as active, and have it do nothing.
The image now states its own environment.
Set in the Dockerfile rather than in the seeded .env on purpose: the copy
only happens when no .env exists, so seeding would fix a fresh install and
leave every installation already running on a stale one exactly as it is.
As an ENV it takes effect on the next pull.
What it does not outrank: Laravel builds its env repository immutable
(Illuminate\Support\Env), so a real environment variable beats the .env
file. `docker run -e`, compose `environment:` and Kubernetes `env:` all
set real environment variables, so an operator who asks for something
explicitly still gets it -- verified against this image, where a .env
saying local/true is overridden to production/false by the variables.
The trade-off, stated because it is a behaviour change: editing APP_ENV or
APP_DEBUG inside storage/.env no longer has any effect, since these are
real environment variables and that file is not. Turning debug on
deliberately is `-e APP_DEBUG=true`, which still works. That is written
into the Dockerfile comment so the next person finds it there.
Not changed: PasswordPolicy's isProduction() test itself. Tying an
administrator's setting to the environment rather than to the setting is
arguably wrong on its own, but it is a separate question with its own
blast radius, and this change makes the shipped image behave the way that
code already assumes.
No test: the environment an image ships is not observable from the suite.
`docker build --check` reports no warnings on the edited file.
compose.example.yaml does two things that are each fine alone and unsafe
together:
ports:
- "8080:80" # Docker binds 0.0.0.0 unless told otherwise
environment:
TRUSTED_PROXIES: "*" # believe the X-Forwarded-For of whoever connects
Behind a proxy that appends the header, "*" is correct and harmless --
Symfony strips the peer and takes the real client the proxy appended. The
example never gets there. It publishes the container on every interface,
so a visitor can reach port 8080 themselves, and then *they* are the peer
the application has been told to trust. `X-Forwarded-For: 203.0.113.9`
makes request()->ip() return exactly that.
What that costs, all of it on the signed-out surface:
- the login lockout, keyed on `email|ip` in LoginRequest::throttleKey()
- throttle:6,1 on register, password-email, password-reset, two-factor
- throttle:30,1 on share-link, public-browse, public-comment
- the download log, the activity log, and the `ip_address` recorded on
guest comments -- which FileComments::post calls "the one handle that
makes spam actionable"
Rotate the header and every one of them counts a different attacker.
The project's own test states the primitive: TrustedProxiesTest sets
trustedproxy.proxies = '*', sends X-Forwarded-For from a *direct* client,
and asserts the address is taken.
The documentation has always qualified "*" correctly -- .env.example says
it is "only safe when nothing but the proxy can reach the app", and
dockerhub-overview.md repeats it. The example file is what did not meet
its own precondition, and it is the file the Docker Hub description tells
a first-time reader to copy.
Publishing on 127.0.0.1 restores the precondition: a proxy on the host, or
in this compose file, still reaches it; nothing off the machine does. This
repository's own compose.yaml already publishes Adminer that way, for the
same reason.
The two settings are now documented as a pair in all three places that
carry them, including what to do when the proxy is on another host: bind
to the interface it arrives from and name that address in TRUSTED_PROXIES
instead of "*".
DOCKER.md's health-check command changes with it -- it told the reader to
curl <host-ip>:8080 from the same machine, which the new binding does not
answer. It now says 127.0.0.1:8080.
No test: this is packaging and prose. `docker compose config` parses the
edited file.
When validation fails, Laravel flashes the request's input into the
session so the form can be repopulated. Its exclusion list is
current_password, password and password_confirmation -- written for the
login and password screens, and covering none of the credentials the
system settings screens take. `dontFlash` did not appear anywhere in this
repository.
So every one of these went into the session in clear the moment its form
was rejected:
secret ExternalStorageSettingsController (S3 secret access key)
key_file ExternalStorageSettingsController (GCS service account JSON)
bind_password LdapSettingsController
client_secret SocialLoginSettingsController, EmailSettingsController
secret_key CaptchaSettingsController
Each is stored with an `encrypted` cast, and config/session.php puts
sessions in the database with `encrypt => false` -- so the rejected save
wrote in clear into the same database the cast exists to protect.
The sharpest one is key_file. serviceAccountKeyRule() exists to catch a
paste that lost its last line, which makes "the request carrying a
service account private key" and "the request that fails validation" the
same request more often than not.
dontFlash() merges rather than replaces, so the framework's three stay.
The cost is that these five come back blank after a failed save. That is
already what they do after a successful one -- every screen here treats
them as write-only, and a blank means "keep what is stored" -- so the
behaviour is now the same either way instead of only on success.
Tests: one per field, each submitting a form that fails validation while
carrying a secret, then reading the old input back the way the form
would. All five fail against the unmodified bootstrap/app.php. A sixth
pins that the framework's own three are still excluded, and each
assertion checks a neighbouring non-secret field still comes back, so
this cannot pass by flashing nothing at all.
Note for the record: this is testable in the existing harness after all.
phpunit.xml sets SESSION_DRIVER=array, but old input is written to the
session whatever the driver backs it, so getOldInput() sees exactly what
a database session would have stored.
Connecting a provider needed nothing but the session. Anyone holding one
could POST /settings/connected-accounts/google, follow the returned
Inertia::location(), sign in at the provider as *themselves*, and
completeLink() would bind their identity to the victim's account.
SocialAccount says what that row is:
This row *is* the authorization to sign in as that account.
So it is not a preference -- it is a credential, and one that outlives
every way the victim has of ending the session that created it. It
survives a password change, it survives Auth::logoutOtherDevices(), it
survives invalidating every session. Where a stolen session gives an
attacker access until it is noticed, this gives them an account.
routes/settings.php already makes exactly this argument, twenty lines
down, for the two-factor block and the API token routes:
a token outlives the session that minted it, so a stolen session must
not be enough to mint one
The link has that property too, and was the one thing on this screen
without the gate. Now it has it.
The gate goes on `connect`, not on the callback: starting the flow is what
writes the intent the callback completes, and the callback deliberately
sits outside every group so a provider sign-in works without a session.
Not changed, deliberately: `connected-accounts.destroy`. Disconnecting
removes a way in rather than adding one, and destroy() already refuses to
remove the last one ("This is the only way you can sign in. Set a password
first"). Putting it behind password.confirm would fall hardest on the
accounts a provider provisioned -- they hold a Str::password(64) nobody
has ever seen -- and leave them unable to disconnect anything at all.
There is a test pinning that it stays reachable.
Also not changed: the account owner still is not told. SocialLoginController
writes an activity log entry, and that sits behind `staff` +
can:view_actions_log, so a client never sees it. Notifying them is a real
gap and a separate change; this one closes the door rather than adding a
bell to it.
Tests: two that fail against the ungated route -- the redirect, and the
whole attack end to end with a stranger identity never binding. The
existing connect() helper now confirms the password, the way
enableTwoFactor() already did, so the rest of the file keeps exercising
the real gate rather than asserting around it.
POST /confirm-password verified the account's password and counted
nothing. Forty wrong guesses, forty identical refusals, no lockout, no
Retry-After, no log line.
routes/auth.php opens by requiring the opposite:
**Every `throttle:` below names its own bucket, and must.**
and every other route in the file has one. POST login is the deliberate
exception, and the file says why -- LoginRequest limits it per email *and*
IP, which is a stronger boundary than a per-IP count. confirm-password had
neither of those things.
It is the wrong door to leave unlatched. Re-proving the password is what
stands between a stolen session and disabling two-factor, regenerating
recovery codes, or minting an API token -- credentials that outlive the
session, which is the reason routes/settings.php gives for putting those
routes behind it. An attacker who already holds the session can sit on
this endpoint until the password falls out of it, and then has the
password for everything else too.
Two more with the same shape, in routes/settings.php:
- PUT /settings/password -- update() validates `current_password`.
- DELETE /settings/profile -- destroy() validates `current_password`.
Both were equally uncounted, and both answer the same question in the same
way, so an attacker refused at one door simply used the next. Fixing one
of three would have been cosmetic.
All three get named buckets at 6/1, matching the credential-facing routes
already in auth.php. Named rather than bare: a bare `throttle:` keys on
sha1(domain|ip) or sha1(user_id) with no route in it, which is how six
share links once locked a visitor out of the two-factor challenge.
Not changed: POST /logout has no bucket either and does not need one -- it
checks no credential and reveals nothing by being repeated. PATCH
/settings/profile likewise.
Tests: three that fail against the unthrottled routes, and two that pin
what the buckets must not do -- exhausting one must not spend another's,
and one account's guesses must not lock a different account out.
Both preference writers validated their array as ['required', 'array']
and looped updateOrCreate over it:
'widgets' => ['required', 'array'],
'widgets.*.widget_key' => ['required', 'string', Rule::in(WIDGET_KEYS)],
Rule::in answers "is this a key I know", once per element. It says
nothing about how many elements there are, and nothing about whether they
repeat -- so a request could name the same valid key any number of times
and buy a SELECT and an UPDATE for each one.
Measured on this base, sent as JSON (a form-encoded array that size is
truncated by max_input_vars long before it reaches the controller):
widgets 10 entries 37 queries 1 row
500 entries 1044 queries 1 row
3000 entries 7051 queries 1 row
notifications 10 entries 23 queries 1 row
2000 entries 2025 queries 1 row
One row, every time. The work is not even data growth -- 3000 entries
write the same single row 3000 times, because updateOrCreate matches on
(user_id, widget_key) and every element after the first is an update of
what the one before it just wrote.
Neither route is behind a throttle: bootstrap/app.php applies
throttleApi() to the API group only, /dashboard/widgets is behind `auth`
alone and /settings/notifications is deliberately outside the `staff`
group, since every account manages its own. So the weakest account on the
installation -- a client with no permission at all -- can reach both, and
the only ceiling is post_max_size.
Both are bounded by the list they already validate against, not by a
number:
- widgets by count(self::WIDGET_KEYS), the same constant Rule::in reads.
- preferences by count($this->emailableKeys()), because
NotificationTypeRegistry is deliberately open -- "never a closed enum,
since core must not need to know a package's notification type keys at
compile time" -- so a literal would be wrong the day a module
registers one.
`distinct` on the key does the other half: a layout has at most one entry
per widget, which is what the screen sends and what the loop assumes.
After: 3000 entries cost 30 queries and write nothing, refused with a 422
instead of half-applied.
Two findings, one cause, one change -- they are the same three words in
two modules, and splitting them would leave the rule stated once and
broken once. Tests live with each controller: two refusals each, both
failing against the unfixed controllers, plus one for the largest
legitimate submission -- a full nine-widget layout, and every emailable
type at once -- so the bound can never be tighter than the screen.
MailConfigApplier and ExternalStorageConfigApplier read their settings
through the `encrypted` casts -- decrypted -- and wrote the result into
the cache store with rememberForever(). The SMTP password, the S3 secret
access key and the whole GCS service account key file, private key
included, went in as plain text under a key that never expires.
The cache store encrypts nothing. On the store INSTALL.md documents for a
manual install (CACHE_STORE=database) and config/cache.php defaults to,
that is the `cache` table of the same database whose dump the `encrypted`
cast exists to survive. On redis it is the redis dump.
The rule already exists, two files away. MailOAuthConnection states it:
Transports read this row fresh at send time -- tokens must never travel
through the boot-config cache (see MailConfigApplier, which caches only
readiness and the account address).
MailConfigApplier's own cache-key comment says the same thing about the
same array: what is deliberately NOT in the cached shape is tokens,
because neither readiness nor an address is a credential. The SMTP
password was in it anyway. SocialSettings::available() names both classes
outright as making the mistake.
So the credentials are read the way the tokens already are: from the row,
at the point that uses them. The cached array keeps everything that is
not a credential, and each applier reads its secret inside the branch
that configures a transport -- an installation on OAuth, on cloud, or one
that has never opened the Email or Storage screen reads nothing extra.
BootSettingsCache grows a second entry point rather than the callers
restating its rule. The cached read already survives a database with no
tables, because booting must not require this application's own database;
an uncached credential read on the same path needs exactly that guarantee
and nothing else, since resolve() can hand back a warm "configured" from
a database that has since stopped answering.
Both cache keys are bumped, as their comments require on a shape change.
Tests: five for the absence, two of them against the database cache store
read as the raw rows an operator would find in a dump, since phpunit.xml
runs the suite on the array store and the cache path was structurally
invisible -- which is why GoogleCloudStorageTest could assert that the private
key is not in the column while it sat in the cache. All five were run
against the unfixed appliers and fail there. The three "still configures
what it no longer caches" tests deliberately pass either way: they pin the
behaviour the fix must not break.
Adding or removing a group member answered with the group, and loaded the
relation whole:
return new GroupResource($group->loadCount('members')->load('members'));
GroupResource gives each member an id, a name and an email. So a
client-scoped staff member who added one of their own clients to a group
was handed, in the same response, the name and address of every other
client in it -- people they may not read anywhere else in the application,
and whom the group edit screen refuses to name for exactly that reason.
syncWithoutDetaching() makes the call idempotent, so the same request
returns the same list as often as it is sent.
The boundary is already written down. GroupResource's docblock:
both narrow the list to the clients the viewer may act on, and the
controller loading this relation is where that narrowing is applied
and Api\GroupsController::show() does it for the read of the same group,
noting that "it hands back the membership with addresses". Changing the
membership is not a reason to be told more than reading it is, so both
halves now narrow by the same query, through one private helper rather
than a third copy of it.
members_count is deliberately left whole, matching show(): a size is not
an identity, and it is the number the group listing already reports.
Nothing about who may perform the write changes -- StaffLibraryScope
::allowsGroupMembership() already decided that, and still does. This is
only what the answer is allowed to say.
Tests: added beside the existing "the API twin narrows the membership it
hands back", which covered the read half only. Both write tests fail
against the unfixed controller; the third pins that an unscoped token
still gets every member.
/users/convert lists the accounts a conversion can be started from. For
the promotion direction those are clients, and the query asked only for
the type:
User::query()->where('type', UserType::Client)
The write beside it does not. AccountConversion::guardToStaff() ends with
abort_unless($this->library->canAssignClient($actor, $target), 404);
and says why: a promotion is the most far-reaching thing that can be done
to a client, so reaching one outside the actor's roster "through this door
and no other is not a rule, it is a gap".
The gap was on the way in. A client-scoped staff member holding
manage_users and edit_users was refused the promotion with a 404 -- the
refusal that is careful not to distinguish a stranger from an account that
is not there -- and then shown that same person's name, email, role,
status and consequence counts in the list the refusal came from,
searchable by name or address and paginated to the end.
StaffLibraryScope::clients() is canAssignClient()'s listing half, written
for this: "so a screen narrows by the same rule its buttons are guarded
with rather than restating it -- which is how ClientsController came to
list every client on the installation, name and email, to a viewer who
could reach nothing of theirs." The picker twenty lines below already went
through the same boundary via assignableClientIds().
Only the client direction is narrowed. The staff direction is left exactly
as it was: whoever may demote a staff member may see the staff roster, and
what limits a demotion is guardTarget() on the write, not the listing.
Tests: the listing half added to AccountConversionScopeTest, which until
now covered only the refusals. Two of the five fail against the unfixed
controller -- the stranger's address in the list, and reaching it by exact
search. The other three pin what must not change: the actor still sees
their own client, unscoped staff still see everybody, and the demotion
list still lists staff.
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.
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.
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.
#1747 split the bulk edit's skip message in two, because "you don't have permission to edit them" was being said to somebody about files they own. The new sentence arrived English only, so every non-English installation would have read the correct reason in the wrong language.
Translated from its near-twin rather than from scratch: the two messages differ in one clause, so each locale keeps the first sentence it already had, its own register -- de and tr formal, es, nl, pl and zh_CN informal -- and only the reason changes. That way the pair reads as one voice on the same screen, which is where a staff member meets both.
Added immediately after the sibling key in each file, which is also its sorted position, so the diff is one line per locale and nothing else moved.
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.
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.
#1745 closes an unauthorised read, so it belongs in the changelog rather than only in the merge log: CHANGELOG.md ships in the zip and renders in the application, and it is where somebody running an installation finds out whether a release is about them.
Written for that reader rather than for the codebase. Permission to moderate comments was letting somebody read them, which is not the same thing, and the entry says what was exposed -- the text, staff-only notes, the client each conversation belongs to, and a visitor's IP -- because "a scoping issue" tells an operator nothing about whether to worry.
It also says who was affected and what to do, which matters more than usual here: no role ProjectSend ships is affected, and the one configuration that is -- a custom role that moderates comments but may open no file -- stops being able to moderate at all. Somebody meeting that on a Monday morning should find the answer in the release notes rather than in a bug report.
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.
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.
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.
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.
ensure_worker_watches_zips() exists because a worker unit written before zip downloads had their own queue watches default only, and a zip enqueued to zips then waits forever with nothing to say why. It was called from exactly one place: inside the branch that reloads PHP-FPM, nested inside the branch that found a worker unit -- so it ran only when a PHP-FPM unit had been detected. A worker is a different unit from PHP-FPM, and not finding one says nothing about the other: a host running mod_php, or one whose FPM unit is named in a way this script does not recognise, still has a systemd worker that may predate the zips queue, and it got no check and no mention.
The check now also runs in the else branch, where it costs nothing -- its own first line returns immediately unless systemd and a worker unit are both present -- and the worker is restarted after it, paired exactly as the FPM branch pairs them. That pairing is the point: the new --queue argument reaches the worker only when systemd next starts it from ExecStart, and the queue:restart projectsend:update signals cannot deliver it, because that makes a worker pick up new code and it has already run by the time this block is reached. Editing the unit without a restart would leave the operator told that zip downloads were fixed while they still could not finish, which is worse than the silence it replaces: silence sends somebody looking and a success message does not.
Under --no-restart it is said rather than done. Editing a unit file is exactly what that flag asks us not to do, but a worker that cannot finish a zip is broken whether or not we may touch it, and this is the only place that knows to mention it.
Verified before merging: bash -n parses, and the restart block was driven through four host shapes with say, warn, systemctl and the check itself stubbed, on both this branch and main, rather than relying on the transcript in the PR. Before: fpm+worker reached; worker without fpm silent; --no-restart silent; no systemd silent. After: the first two both reached and restarted, --no-restart not reached but said so, no systemd reached and a no-op. That no-op rests on update.sh:330, which returns unless systemd and a worker unit are both present, so it was read rather than assumed.
Reported and fixed by @denkfabrik-li.
#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.
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.
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.
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.
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.
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.
ClientStorageUsage::quotaMb() exists because a client's own storage_quota_mb of 0 does not mean "unlimited" -- it means "no quota of their own", and the site default is what is then enforced. Both chunked-upload quota checks enforced the resolved limit through quotaBytes() and then printed the raw column in the rejection, so a client with no quota of their own and a site default of 1 MB was told "This upload would exceed your storage quota of 0 MB." That is every client who was never given a quota, including every self-registered one, and the sentence appears at the one moment somebody is trying to find out what their limit is.
Both now print quotaMb(), which is what the check enforced. The API's single-request upload already did exactly this for the same sentence, so the three copies agree. The enforcement itself is untouched -- only the number in the message changes -- and the unlimited case never reaches these branches, because quotaBytes() > 0 guards them.
Verified before merging: 16 passed on the trial-merge, 2 failed / 14 passed with app/ reset. The "a client with a quota of their own still sees their own number" test is green either way. The string itself is unchanged, so no locale file needs anything.
Reported and fixed by @denkfabrik-li.
FileThumbnailController::preview() writes at most one FilePreviewed row per viewer per file per five minutes, because watching a video is a single deliberate act that the browser turns into dozens of Range requests. Its docblock ended by naming the route where the same act happens without an account -- PublicGroupsController::preview -- and that route logged unconditionally. Five requests for the same public file wrote five rows where the signed-in twin wrote one, so one visitor watching one clip buried the public half of the activity log, which is the half an operator reads to see what the outside world is doing.
The window moves into a shared PreviewLog, next to PreviewKind, which those two routes already share for the same reason. Keying is unchanged for a signed-in viewer. An anonymous visitor has no account to key on, so the request IP stands in -- the same substitute ApiServiceProvider's rate limiter makes for an unauthenticated caller. It is a cache key with a five-minute life and never reaches the log, which keeps its own decision about recording an IP.
Downloads are deliberately untouched and stay one row per download: each is a transfer, and DownloadAllowance::used() counts those rows to enforce a per-file cap, so swallowing one would hand out free downloads.
The limit this leaves open, since the IP is a stand-in and not an identity: two anonymous visitors behind one address share a key, so within five minutes the second one's view of the same file is not recorded. That is the same trade the signed-in side has always made per account, and the alternative is the row-per-Range-request this fixes.
Verified before merging: 24 passed across the public-preview and thumbnail suites on the trial-merge, which also confirms this co-exists with #1725 -- the two share both controllers and change different methods in each. With app/ reset and PreviewLog deleted, 1 failed / 9 passed. The signed-in route's existing debounce tests pass unchanged, which is what says the shared class did not move that side. request()->ip() honours the trusted-proxy configuration, so a forged X-Forwarded-For cannot defeat the window from outside.
Reported and fixed by @denkfabrik-li.
BuildZipDownloadJob already draws this line in its write-failure branch: "What the requester sees stays generic: a libzip string means nothing to them and can name a server path. An operator needs the opposite, so the reason goes to the log instead." Thirty-seven lines below it, the catch-all around the whole build stored $e->getMessage() in the row the requester polls -- and ZipDownloadsController hands that column straight back to whoever asked, clients included. A client asking for an archive of a file whose disk is no longer configured read "Disk [a-disk-that-is-not-configured] does not have a configured driver." verbatim. The reason now goes to the log with the exception class, and the row carries the same kind of sentence fail() already uses.
Two more in the same method. tempnam() creates the file, and $tempFiles[] was appended only after the copy finished, so every throw in between left a zip-src- file in the system temp directory that nothing ever removed; it is now registered the moment it exists. And the copy itself was unchecked -- a copy that stops early is a truncated member added to the archive as though it were the file, so the build reports ready and the recipient gets something that opens and is wrong. stream_copy_to_stream and the flushing fclose are both checked now, and both handles close on every path.
Deliberately not changed: comparing the copied byte count against files.size, which would fail perfectly good archives whenever that column is stale; the write-failure branch and its wording; and the skipped-files reporting, which still says which files and why, so only the catch-all went generic.
Verified before merging: 37 passed on the trial-merge, 2 failed / 35 passed with app/ reset. The leak was confirmed at the consuming end rather than inferred -- ZipDownloadsController:169 returns the error column to the requester.
Reported and fixed by @denkfabrik-li.
The 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.
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.
The edit screen is given a file's expiry as a calendar date read back in the viewer's own zone -- deliberately, or "a file set to expire on the 12th reopens showing the 11th". Every save posts that date back, touched or not, and update() derived a fresh instant from it every time. So the expiry drifted by the difference between two people's zones on any other edit: a file set from Pacific/Auckland moved 19 hours later the moment somebody in Buenos Aires renamed it, and moved again on the next save from a third zone. A file could quietly outlive the expiry somebody set for it, through an edit that had nothing to do with expiry.
The instant is now re-derived only when the posted date differs from the one the form was given, compared against the same string through a named pair: expiryDateFor() renders it, expiryInstant() reads it back, and the edit screen calls the render half so the two cannot drift apart. What a changed date means is unchanged -- still the end of that day in the zone of whoever changed it. bulkUpdate() needs nothing: its expiry is an explicit set / clear / no_change action, so an untouched expiry is never posted at all.
Verified before merging: 22 passed on the trial-merge, 1 failed / 21 passed with app/ reset. The "a real change still lands in the editor's zone" and "clearing still clears" tests are green either way. Edge cases walked: a posted date against no stored expiry still sets it, and a posted null against a stored null leaves the column alone rather than writing.
Reported and fixed by @denkfabrik-li.
FilesController::expiryInstant() exists because a calendar day ends where the person naming it lives: the web form posts a bare YYYY-MM-DD, which Eloquent would otherwise store as midnight UTC, so "expires on the 12th" would cut the file off partway through the 11th for anyone in the Americas. PATCH /api/v1/files/{id} took the same field, validated it as a date, and stored it exactly as it arrived -- so the same value that meant end-of-the-12th on the web meant start-of-the-12th over the API, and earlier still for a caller west of Greenwich.
A bare YYYY-MM-DD now means the end of that day in the caller's timezone, through the same LocalDay::end() the web path uses. A value carrying a time is unchanged: that is an instant the caller named on purpose, the API can express one where a date input cannot, and it is stored as it arrives. null still clears the expiry, and the validation rule and permission gate are untouched.
Note for the release notes: this lengthens the life of a file whose expiry an existing integration sets with a bare date, by up to a day. That is the correct meaning and the one the web has always had, but it is a behaviour change for callers who were relying on the old one.
Verified before merging: 19 passed on the trial-merge, 1 failed / 18 passed with app/ reset. The timestamp and clearing tests are green either way. The bare-date branch is gated on a strict ^\d{4}-\d{2}-\d{2}$ match, so nothing else takes it. scramble:export on the merged tree reproduces the committed docs/api/openapi.json byte for byte.
Reported and fixed by @denkfabrik-li.
FileDiskCleanup::delete() wrapped two deletions in one try: the original upload, on whatever disk the row names, and every cached rendition, which is always on the local files disk. Storage::disk() throws outright for a name with no configured driver -- precisely the state the original's disk is in whenever this fails at all -- so the catch swallowed it and the renditions were never reached. Nothing looks for them afterwards: OrphanFileScanner skips the rendition directories on purpose, as derived artifacts rather than orphaned uploads. A file whose external disk had been removed or renamed therefore kept every cached copy of itself indefinitely on the disk that still worked, including the client-facing ones, which for a shared image may be the only copies anyone ever generated.
The two attempts are now separate, each with the tolerance the class was written for: a storage failure still never turns a delete click into a 500, and the warning is still the whole report.
Also corrected: File::booted() justified deferring the byte removal with "the worst case is bytes left on disk with no row, which OrphanFileScanner already finds and reports". That is not this path -- the row is soft-deleted, and knownPaths() counts a trashed row's path as claimed, deliberately, so a scan never offers to double-adopt a file still inside its erasure grace period. The comment now says what actually happens, which is that FileDiskCleanup's warning is the only record.
Verified before merging: 8 passed on the trial-merge, 1 failed / 7 passed with app/ reset.
Reported and fixed by @denkfabrik-li.
Both thumbnail routes treat "the file exists" as "the rendition is cached", and nothing ever invalidates one: RenderedImageCache::flush() runs on ImageRenderingChanged, which no code in core raises. Whatever sits at the path is what every later viewer gets. ThumbnailGenerator::generate() encoded straight onto that path, so a render that died partway -- a full volume, a killed worker -- left a half-written file that was then served as the rendition indefinitely, and two requests rendering the same file at once encoded into the same path together.
Write side: the image is written beside its destination and renamed into place. rename() within a directory is atomic and replaces what is there, so the path holds either the previous rendition or a complete new one, and the loser of a race leaves a whole image rather than a mixture of two. Renditions always cache on the local files disk and the generator is handed $disk->path(), so both files are on the same filesystem and the atomicity is real. Read side: an empty file is not a rendition, so both routes replace one rather than serve it -- writing through a temporary file means core can no longer create that state, but an installation that ran an older version can already have it on disk and nothing else will ever clear it.
The cache itself is unchanged: a non-empty rendition is still reused without further checks, because decoding every cached image on every request to prove it is intact would cost the cache its point. The RenderingImage seam still fires before the encode.
Verified before merging: 14 passed on the trial-merge, 2 failed / 12 passed with app/ reset. The third test, about the generator's own temporary file, passes either way and the PR says so rather than leaving it to be found.
Reported and fixed by @denkfabrik-li.
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.
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.
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.
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.
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.
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.
ensure_worker_watches_zips reads the unit file with `systemctl show -p
FragmentPath`, and an empty answer meant an immediate, silent return. The
common cause is a mistyped --worker: systemd does not know the unit, the
check never runs, and the operator finishes the update believing their
worker was inspected.
Which produces precisely the outcome the function exists to prevent. Its
own comment says a worker that does not watch the zips queue finishes no
zip downloads while cheerfully sending every email, and that nothing says
why. Skipping the check in silence is a quieter way to arrive there.
It now warns, names the consequence, and says what to check. The
read-only case is separated out too: a unit file somebody else owns
cannot be repaired, but it can still be read, so a worker that is missing
the queue is diagnosed rather than passed over.
Worth recording why this was looked at. The portal session found a deploy
script that had printed `next run` followed by nothing for its whole
life, because `systemctl show` answers an unknown property with an empty
value and a zero exit -- a line always blank is worse than no line, since
somebody believes a check is being performed. FragmentPath here is
correct, verified against a real unit; the failure was the same shape one
step further on, in what an empty answer was taken to mean.
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.
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.
last_staff_login_at is a MAX() over activity_log, and the docblock
already said the log is never pruned. It did not say that anything
depends on it. Something does now: the hosted platform warns, pauses and
finally removes a free instance nobody has signed in to, counting from
this field.
So retention or pruning added to activity_log would break nothing here --
every test would pass, the field would keep answering, and old
installations would quietly start looking dormant to the process that
deletes them. That is the shape of failure worth naming in advance,
because the person adding a retention policy would have no reason to look
at this file.
Same note as the one on SeatAllowance's counting rules and on
ManagedStorageBackend::describe(): an assumption with a reader outside
this repository is a contract, and the place to record it is where
somebody would otherwise change it.
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.
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.
`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.
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.
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.
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.
ensure_worker_watches_zips() exists because a worker unit written before
zip downloads had their own queue watches 'default' only, and a zip
enqueued to 'zips' then waits forever with nothing saying why. It is
called from exactly one place: inside the branch that reloads PHP-FPM,
nested inside the branch that found a worker unit.
So it runs only when a PHP-FPM unit was detected. Driving the restart
block through four host shapes, with everything it touches stubbed:
systemd + fpm + worker reached, restarted
systemd + worker, no fpm silent
--no-restart silent
no systemd at all silent
The second line is the one that matters. A worker unit is a different
service from PHP-FPM, and not finding one says nothing about the other: a
host running mod_php, or one whose FPM unit is named in a way this script
does not recognise, can still have a systemd worker that predates the
zips queue. That host gets no check and no mention.
The check now runs in the else branch too, where it costs nothing -- its
own first line returns immediately unless systemd and a worker unit are
both present -- and the worker is restarted after it, paired exactly as
the FPM branch pairs them. That pairing is the point rather than a
flourish: the new --queue argument reaches the worker only when systemd
next starts it from ExecStart. The queue:restart that projectsend:update
signals cannot deliver it, because that makes a worker pick up new *code*
and it has already run by the time this block is reached, so the worker
came back on the old command line. Editing the unit without the restart
would leave the operator told that zip downloads were fixed while they
still could not finish -- worse than the silence it replaces, since
silence sends somebody looking.
Under --no-restart it is said rather than done: editing a unit file is
exactly what that flag asks us not to do, but a worker that cannot finish
a zip is broken whether or not we are allowed to touch it, and this is the
only place that knows to mention it.
Same four shapes afterwards:
systemd + fpm + worker reached, restarted
systemd + worker, no fpm reached, restarted
--no-restart not reached, but said so
no systemd at all reached, no restart (returns immediately)
No test: the suite cannot drive a shell script that restarts services.
bash -n parses, and the harness above is the evidence.
Two seat counters and one folder-delete refusal, in all sixteen locales.
Additive only: nothing already in a catalogue was reordered or reworded,
so the diff is three lines per file.
Polish, Czech and Russian get three plural forms where the English has
two. Those languages inflect a noun by the number in front of it -- one
case for 2-4, another for 5 and up -- and the framework's selector picks
between three segments for them, so writing only the English pair would
have produced "5 pliki" where it has to be "5 plikow". Verified through
trans_choice at 1, 3 and 7.
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).
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.
StoredFileResponse hands external storage a presigned URL for an hour,
whatever the delivery is for. That URL is a bearer credential: whoever
holds it fetches the file without passing any of the caller's checks
again, and it outlives them. A download cap spent in the meantime, an
expires_at that falls inside the hour, an assignment withdrawn -- none of
them reach it, and nothing here can revoke one. It is also forwardable,
which the local path is not: X-Accel-Redirect authorises one response to
one request.
The two deliveries do not need the same window, so they no longer share
one.
A download has to survive being followed -- a redirect and a request --
which a minute covers with room to spare. An object store checks the
signature when the request arrives rather than while it runs, so a
transfer that starts inside the window finishes however long it takes.
A preview keeps the hour, because it is watched rather than fetched: the
player holds the URL and issues a Range request every time somebody seeks
past the buffer, so a minute would break playback of anything longer than
a minute. The class docblock now says that this is the trade being made,
instead of leaving it in a single number.
Two tests, one per window. Without the fix the download link is an hour
long.
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.
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.
INSTALL.md tells an operator to cache routes, views and events once, and
promises: "You only run these once: projectsend:update notices they are in
place and rebuilds them for you after every update."
It cannot, for anybody who updates with update.sh. The script wipes
bootstrap/cache/*.php while replacing the application files, and
UpdateInstallation::warmCaches() decides what to rebuild by asking
file_exists() on those very paths -- forty lines later. Every installation
looks like one that never cached anything.
Measured in a copy of the tree, the two orderings:
wiping everything, as the script does it
→ "Cleared the compiled configuration, events, routes and views."
→ bootstrap/cache is empty afterwards
keeping the route and event caches
→ "Rebuilt the route, event and view caches — they were in place before."
→ routes-v7.php and events.php are back
So the site quietly loses route, event and view caching on every update,
and the operator is never told.
The wipe now names what it removes rather than taking the directory:
- packages.php and services.php, because they must not survive the swap:
they name the old release's package providers, and the first artisan
run after the copy would try to load classes this version no longer
ships.
- config.php, for a sharper reason. It is read at every boot, so leaving
it means projectsend:update reads the *previous* release's version out
of it. Measured with a doctored version inside a cached config: the run
said "Re-applied 2.2.0" while the release on disk was 9.9.9, recorded
that old version as the one applied, and ran the migrations under the
old configuration. With it removed: "Updated from 2.2.0 to 9.9.9".
The route and event caches stay, since neither is read at boot -- both are
arrays of class names, consulted when a route is matched or an event
dispatched -- and projectsend:update clears them itself moments later.
Removing config.php here would have taken the command's "a cached
configuration was found" warning with it, since it warns about what it
finds. The script now says it, in the same words, at the moment it removes
the file.
No test: the suite covers the command's decision (UpdateCommandTest pins
the whole rewarm matrix) and cannot run a shell script that replaces an
installation. The measurements above are the evidence; bash -n parses.
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.
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.
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.
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.
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.
The write-failure branch already draws the line and says why: "What the
requester sees stays generic: a libzip string means nothing to them and
can name a server path. An operator needs the opposite ... so the reason
goes to the log instead." Thirty-seven lines below it, the catch-all
around the whole build stored $e->getMessage() in the row the requester
polls. Measured, a client asking for an archive of a file on a disk that
is no longer configured was told:
"Disk [a-disk-that-is-not-configured] does not have a configured driver."
The reason now goes to the log with the exception class, and the row
carries the same kind of sentence fail() already uses.
Second, the temp files. tempnam() creates the file, and $tempFiles[] was
appended only after the copy had finished -- so every throw in between (a
disk that will not resolve, a stream that will not open) left a zip-src-
file in the system temp directory that nothing ever removes. It is now
registered the moment it exists.
Third, in the same method: the copy itself was unchecked. A copy that
stops early is a truncated member added to the archive as though it were
the file, so the build reports ready and the recipient gets something that
opens and is wrong. Both the copy and the fclose that flushes it are
checked now, and both handles close on every path.
Two tests: the failure message names nothing about the server, and a build
that throws mid-copy leaves no temp file behind. Both go red without the
fix.
FileDiskCleanup wraps both deletions in one try. The first is the original
upload, on whatever disk the row names; the second is every cached
rendition, always on the local files disk. Storage::disk() throws outright
for a name with no configured driver -- which is the state the original's
disk is in whenever this fails at all -- so the catch swallowed it and the
renditions were never reached.
Nothing looks for them afterwards. OrphanFileScanner skips the rendition
directories on purpose (they are derived artifacts, never orphaned
uploads), so a file whose external disk had been removed or renamed kept
every cached copy of itself, indefinitely, on the disk that was working.
The two attempts are now separate, each with the same tolerance the class
was written for: a storage failure still never turns a delete click into a
500, and the warning is still the report.
While here, the comment in File::booted() that justifies deferring the
byte removal claimed "the worst case is bytes left on disk with no row,
which OrphanFileScanner already finds and reports". Not on this path: the
row is soft-deleted, and knownPaths() counts a trashed row's path as
claimed -- deliberately, so a scan never offers to double-adopt a file
still inside its erasure grace period. The comment now says what actually
happens.
One test: a file whose disk cannot be resolved loses its renditions. It
goes red without the fix, next to the existing test that the delete itself
still succeeds.
Both thumbnail routes treat "the file exists" as "the rendition is
cached", and nothing ever invalidates one: RenderedImageCache::flush()
runs on ImageRenderingChanged, which no core code raises. Whatever is at
the path is what every later viewer gets.
ThumbnailGenerator encoded straight onto that path. A render that died
partway -- a full volume, a killed worker -- left a half-written file
that was then served as the rendition for good, and two requests
rendering the same file at once encoded into one path together.
It now writes beside the destination and renames into place. rename()
within a directory is atomic and replaces what is there, so the path is
either the previous rendition or a complete new one, and the loser of a
race leaves a whole image rather than a mixture of two. The temporary
file is removed on the way out either way.
The read side gets the other half: an empty file is not a rendition, so
both routes replace one rather than serve it. Writing through a temporary
file means this state can no longer be created here, but an installation
that ran an older version can already have it on disk, and nothing else
will ever clear it.
Three tests: an empty rendition is replaced on the signed-in route and on
the public one, and a successful render leaves nothing half-written
behind. Without the fix the first two go red; the third is about the fix's
own temporary file and passes either way.
complete() holds a lock whose comment promises "the lock's TTL releases
the claim if a completion dies mid-flight, so a later retry still works".
A retry has nothing to work from but the parts, and assemble() unlinked
each one inside the loop that read it -- so everything that can fail
afterwards took the retry with it.
Measured on main, with a disk refusing the write (the case the guard forty
lines further down was written for, found against a real GCS bucket):
first complete → 422, 0 parts left, the half-written copy left behind
retry → 422 "Upload is incomplete: missing parts."
For good: listParts() is empty, so no later attempt can ever succeed, and
the client has to send the whole file again. The abandoned copy sat in the
session directory until the sweeper came round.
The parts now go when abort() clears the session directory -- which
already ran on success -- and a failure deletes only the half-written copy
it made. The cost is temp space: peak usage during assembly is the whole
file twice over rather than the file plus one part. The docblock says so.
Also checked while here: every read and every write in the concatenation.
A failing fwrite is loud in practice, since Laravel's error handler turns
the warning into an ErrorException, but loud there is a 500 carrying a PHP
message where this method's other storage failure is a sentence the person
uploading can act on. A short write arriving without a warning would be
worse: the byte count and the checksum describe the buffer that was read,
so an unchecked one records a truncated file with a checksum matching
bytes that were never stored.
Two tests: the retry after a refused write now succeeds, and a temporary
directory that refuses writes (/dev/full, skipped where it does not exist)
fails the upload with this method's own message. Without the fix both go
red.
update() states the rule eighteen lines above the bug: "PATCH semantics,
unlike the web form which always submits every field: an absent key means
'leave alone', not 'clear'." Every column obeys it. The custom fields did
not, because they went through create()'s pass, which walks every field
there is and writes null for the ones the request did not carry.
Two fields filled, a PATCH naming one:
status → 200
named field → "Robin"
the other one → null (was "ATU12345678")
Nothing says so in the response, and there is no other copy of the value.
The write pass is now shared but entered two ways: create() keeps writing
every field, since a new client has no values and a checkbox nobody ticked
is a recorded "no"; update() writes only the fields the request named.
Three tests: the untouched field survives, a named empty value still
clears, and create still records every field. Without the fix the first
goes red.
The edit form is given a file's expiry as a calendar date, read back in
the viewer's own zone -- deliberately, so a file set to expire on the 12th
does not reopen showing the 11th. Every save posts that date back, whether
or not anybody touched it, and update() derived a fresh instant from it
every time.
So the expiry drifts by the difference between two people's zones on any
other edit. A date set from Pacific/Auckland stores 2026-09-12T11:59:59Z;
a colleague in UTC-3 opens the file, sees the same 12th, renames it, and
the file now expires at 2026-09-13T06:59:59Z -- 19 hours later, with
nobody having gone near the date.
The instant is now re-derived only when the posted date differs from the
one the form was given, compared against the same string through a named
pair: expiryDateFor() renders it, expiryInstant() reads it back. The edit
screen uses the same method it is compared against, so the two cannot
drift apart.
bulkUpdate() needs nothing: its expiry is an explicit set/clear/no_change
action, so an untouched expiry is never posted in the first place.
Three tests: the rename leaves the instant alone, a real change still
lands in the editor's own zone, and clearing still clears. Without the fix
the first goes red.
FilesController::expiryInstant exists because a calendar day ends where
the person naming it lives: the web form posts a bare YYYY-MM-DD, and
storing that as it arrives would cut a file off at midnight UTC -- "expires
on the 12th" ending partway through the 11th for anyone in the Americas.
The API takes the same field, validates it as a date, and stores it raw:
web → 2026-09-12T23:59:59+00:00 (end of the day, as the docblock means)
API → 2026-09-12T00:00:00+00:00 (raw)
Same value, same field, same file, two meanings -- and the earlier of the
two is a file that dies at the start of the day it was promised.
A bare date now means the end of that day in the caller's timezone, as it
does on the web. A value carrying a time is unchanged: it is an instant
the caller named on purpose, the API can express one and a date input
cannot. The endpoint's docblock says both, so the OpenAPI document does
too.
Three tests: the day, the timestamp, and clearing. Without the fix the
first goes red.
FileThumbnailController::preview() writes at most one FilePreviewed row
per viewer per file per five minutes, because a browser turns one video
into a long tail of Range requests against the same URL. Its docblock
names the anonymous route as the place the same act happens without an
account -- and that route logs unconditionally.
Measured: five requests for the same public file, five
PublicFilePreviewed rows, against one for the signed-in twin. One visitor
watching one clip buries the public half of the activity log, which is
also the half an operator reads to see what the outside world is doing.
The window is now a shared PreviewLog, next to PreviewKind, which the two
preview routes already share for the same reason. Keying is unchanged for
a signed-in viewer; an anonymous one has no account to key on, so the
request IP stands in -- the same substitute the API's rate limiter makes
for an unauthenticated caller. It is a cache key with a five-minute life
and never reaches the log, which keeps its own decision about recording an
IP (ActivityLogger::shouldRecordIp, Setting::DownloadIpLogging).
Three tests: the replay is one row, two visitors are two rows, and the
window is per file. Without the fix the first goes red.
`reassign_candidates` is the delete dialog's picker: every active account
in the installation, by name and by role label. The same list is shared
on the clients index, the users index, both edit screens and privacy
settings, and it was narrowed by nothing.
Two lines above it on the clients index sits the listing itself, narrowed
through `scope->clients($viewer)` with a comment saying why: "a
client-scoped staff member is not shown the name and email of somebody
they can reach nothing of". The picker beside it handed over every client
in the installation, plus every staff account and its role name. The
filter by `can('delete_clients')` happens in React, which decides what is
rendered, not what is sent.
So the client half of the candidate list goes through the same
StaffLibraryScope as the listing, and each screen sends the picker only to
a viewer holding the delete permission it exists for. Staff accounts are
not narrowed -- they are not narrowed anywhere else either -- and an
unscoped viewer's list is unchanged, because StaffLibraryScope::clients()
returns every client for them.
Privacy settings keeps the whole installation on purpose: that picker sets
the erasure default stored once for everybody, behind edit_settings, so
narrowing it by whoever happens to be editing would store the wrong
answer. The parameter is nullable for that one caller, and the docblock
says so.
Four tests. Without the fix three go red; the fourth is the guard that an
administrator still sees every active account.
Two different things stop a selected file being changed, and bulkUpdate()
reported both as the first one.
Files dropped by the Gate::allows('update') filter are ones the staff
member may not edit at all. A file that survives the filter and still
changes nothing is a different case: it was editable, and every field they
asked to change was one their role does not let them set -- expiry,
download limit, categories, each behind its own permission, exactly as the
single-file editor treats them.
Measured with edit_files but without set_file_expiration_date, three files
they own, expiry the only change: "0 of 3 selected files were updated. The
rest were skipped because you don't have permission to edit them." They
own all three and editing is precisely what they may do, so the sentence is
both wrong and unactionable.
The two cases now have their own sentences. The existing string is kept
for the case it describes -- every skip a file they may not edit -- so its
sixteen translations stay in use. The new one covers a field permission,
and covers a mixture of both reasons, since "permission to make those
changes" is true either way.
The new key is English only; a locale without it falls back to English,
which is a translated-but-wrong sentence traded for an untranslated
correct one.
Three tests: each reason on its own, and the mixture. Without the fix the
first and third go red.
Two places asked "is this the API?" and got it wrong in opposite ways.
EnsureCapability asked $request->expectsJson(). Whether a feature exists
in this installation's edition is a property of the installation, not of
what the caller is willing to parse, so the same route answered
differently per header: `Accept: application/json` got the 403
`capability_unavailable` routes/api.php promises, `Accept: */*` -- curl's
default -- got a bare 404 `not_found`. The mirror image is worse: an
Inertia visit to a capability-gated *web* screen accepts JSON, so it got
403 with Laravel's default error body, naming the exception class, where
the point of the 404 is that an unavailable feature is absent rather than
teased.
ProblemDetails asked $request->is('api/*'). Two staff pages live under
that prefix -- the API dashboard at /api and the OpenAPI reference at
/api/docs, both registered in routes/web.php -- so a signed-out visitor to
either got 401 problem+json, "Send a valid API token in the Authorization
header as \"Bearer <token>\"", instead of the login redirect every other
page gives them.
Both now ask App\Support\ApiSurface: under the API prefix, and not part of
the `web` middleware group. The group is what actually separates the two
-- sessions and CSRF on one side, tokens on the other -- and it keeps
answering correctly for a future /api/v2 without being edited. An
unmatched path has no route to ask, which is the API's answer anyway: a
404 under its prefix is one it should describe in its own format, and the
existing test for that stays green.
Three tests, in the two files that already own these rules. Without the
fix all three go red.
Both chunked-upload quota checks resolve the limit through
ClientStorageUsage::quotaBytes(), which falls back to the site default
when a client has no quota of their own -- and then print
`$user->storage_quota_mb` in the rejection. For every client who was never
given an explicit quota that column is 0, so the message reads "This
upload would exceed your storage quota of 0 MB." at the one moment
somebody is trying to find out what their limit is.
The API's single-request upload already prints
`$this->storageUsage->quotaMb($user)` for the same sentence
(Api/FilesController.php:208). The two chunked copies now do the same.
Three tests: the inherited default is named at session creation and again
at completion, and a client with a quota of their own still sees their own
number. Without the fix the first two go red, the third stays green.
groupReachesNoFurther() asks whether anything shared with a group sits
outside the viewer's library. `f1b35cc9` established the shape of the
answer for deleted files: start from the live row, because "a deleted file
is not reach, because nobody can reach it".
An expired file is the same case. Membership grants nobody access to it --
File::scopeVisibleToClient ends in notExpired(), so it has left every
member's /my-files and the download answers 403 -- but it is equally gone
from files(), where its absence reads as "outside my library". The group
then locks for a scoped staff member: they cannot add a member, cannot
rename it, and cannot remove their own client again.
So the reach query skips expired files as it already skips deleted ones.
Expiry is reversible where deletion is not, and that needs no special
handling: the guard asks what is reachable at the moment somebody is added
or removed, and the file counts again the moment it stops being expired.
Not changed: File::scopeVisibleToClient, whose treatment of expiry was
settled deliberately in c8078f65. This is about what counts as reach, not
about what a scoped viewer may open.
Two tests, next to the deleted-file pair they mirror: the lockout, and the
half that must not soften -- a live out-of-reach file is still reach with
an expired sibling next to it. Without the fix the first goes red.
FilePolicy::view() has two halves for staff: 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.
A role holding moderate_comments and no file key therefore got a 403 on
every file in the installation while reading every comment written about
them on /comments: the text, staff-only notes, the client name a
Clients-visibility comment carries, and a visitor's IP address. The API
queue answered the same way, and approving through it hands the body back
in the response, so it was a reading door as well as a writing one.
The class says this is not supposed to happen -- across()'s own docblock
("a moderation screen is not a way around the visibility model"), the
route comment on /comments ("the list itself is still narrowed by
VisibleCommentScope, so holding the permission does not widen what a
viewer may read"), and routes/api.php ("reading and writing a comment is
gated by 'may see this file', the same three keys the file endpoints
use"). FileCommentPolicy::view() enforces it for a single comment, by
running the file's own gate first. Only the cross-file queries did not.
So they now take their files from ViewableFileScope, which is
FilePolicy::view() expressed as a query, instead of from StaffLibraryScope,
which is only its second half: across(), pendingTotal() and the API's
pending list. The permission half moves into a named method on that class,
since three modules now ask the same question.
FileCommentPolicy::moderate() gets it too, in both forms. Its row form is
otherwise unchanged -- the library check still runs by file id, so a
comment on a soft-deleted file behaves exactly as before.
No system role changes behaviour: Account Manager and System Administrator
are the two that ship with moderate_comments, and both hold upload. What
changes is a hand-built role that holds moderation and nothing else.
Seven tests. Without the fix, five go red; the other two are the premise
(that the viewer really is refused the file itself) and the guard that a
moderator who may read files still moderates the whole installation.
docs/api/openapi.json regenerated for the one changed description.
clientDashboard() restates the assignment half of
File::scopeVisibleToClient in a whereHas of its own. The scope is the
single source of truth for client file access and ends in notExpired(),
which the copy leaves off, so the two disagree in both directions.
Over: an expired file stays counted and keeps its name on the dashboard
after /my-files has stopped listing it and the download answers 403. Under:
everything that reaches a client another way is missing -- a file inside a
folder shared with them, a file they uploaded through the portal
themselves, and a revision, which owns no assignment row at all and
inherits its original's recipients through SharingIdentity.
Replaced by the scope itself, which is what /my-files runs. The existing
test for the page is unchanged and still passes: a directly assigned,
unexpired file counts exactly as before.
Two tests, one for each direction. Without the fix both go red.
ApiUsage::recentActions() is the only ActivityLog query outside
ActivityLogger and AccountEraser that does not run through
ActivityLogScope::apply(). Its whole boundary is view_actions_log -- the
permission whose own scope class says, in as many words, that it "is not
the whole answer for a client-scoped staff member".
The Client Manager system role is client_scoped and ships with that
permission, so this is the default configuration. Such a viewer opening
/api?all=1 reads the fifteen most recent API log rows for the entire
installation, each with its subject_name: the names of files and clients
they get a 403 on. /activity, the download history and the dashboard's
recent-activity widget all narrow the same rows; the API dashboard was
missed.
The scope is applied on both sides of the install-wide branch. The
own-actor filter for the narrow view already stays inside what the scope
allows, and a boundary that exists in only one arm of an `if` is one
refactor away from not existing.
Three tests: the scoped viewer sees only the entry about a file in their
library, their own actions stay whole even when the subject is outside it,
and an unscoped viewer's feed is unchanged. Without the fix the first goes
red; the other two are green either way and guard against narrowing too far.
Five more facts for whatever watches an installation from outside the
container, and one seam so a package can add its own.
Storage is the one that was about to be wrong. It is summed from the rows
that record it, not measured on the volume: measuring the directory was
correct until external storage went live and silently stopped being, since
an upload that resolves to a bucket leaves nothing on disk to measure. A
figure taken from the filesystem freezes while the account keeps filling,
and on a managed installation that figure is what a customer is shown and
billed against. `by_disk` splits the same sum by where the bytes went,
which is the only way to see what is still sitting locally from before a
cutover. Trashed files are excluded because they hold no bytes -- File's
deleted hook takes them.
Health is what a container cannot show from outside. A queue worker dying
is invisible to anything watching the process: it is still up, and zips
quietly stop building while mail stops going out. Same for a deploy whose
migrations failed -- the application answers every request and is a schema
behind. An unreachable queue reports null rather than zero, because an
unreachable Redis is not an empty queue and reading the second as the
first is how a dead worker looks healthy.
The two-factor enforcement setting is echoed back the way EnforceTwoFactor
reads it, fallback included: reporting a stricter rule than the middleware
actually applies would be worse than reporting none.
And ResolvingInstallationStatus, so a package can report what core cannot
know. The managed storage backend and the version of the package providing
it live in cloud-modules, which this repository must not reference, and a
platform that writes eight environment variables only ever knows what it
asked for. Those came apart once: a bucket provisioned, a token minted,
every variable correct, and an image whose copy of the package predated
the module that reads them. Files went to local disk with the
configuration sitting perfectly right beside them.
Two shapes are cast to objects deliberately. An empty PHP array encodes as
[], so an installation with no packages -- or holding no files -- would
answer a map-shaped field with a list, and a reader unmarshalling it
breaks on the day it happens to be empty rather than the day it is
written. There is a test for each.
Requested by the ProjectSend Cloud control plane, whose storage figure
stops growing the moment a tenant's uploads start reaching the bucket.
FoldersController::destroy() authorized delete on the folder and nothing else, while FolderService::delete() soft-deletes every file in the subtree and File's deleted hook takes the bytes off disk. So a staff member refused a file one route over could destroy it by deleting the folder around it -- permission and library boundary both unasked.
MyFoldersController::destroy() already draws this line for the client half of the same cascade, and says why: owning the folder is not authority over content someone else put in it. This is the staff half of that sentence.
Verified before merging: the four bug tests fail on main and pass here, and the SQL predicate was read line by line against FilePolicy::delete -- it is a faithful negation, including the null-uploader case and the short-circuit for an unscoped viewer holding both delete permissions. Membership of the check is one COUNT, not a policy call per file. Suite at 2099, PHPStan clean.
Behaviour change, deliberately accepted: a folder delete that used to succeed now refuses, naming how many files are in the way. The likely case is somebody who owns a folder another account uploaded into. The alternative is irreversible loss of files the same person is refused individually.
Not taken: deleting what the actor may and keeping the rest. Half a tree is worse than either answer. Naming the blocking files would be friendlier than counting them and is worth doing later -- the list has to hide any file the viewer cannot see, which is its own small design question.
Reported and fixed by @denkfabrik-li.
file_comments.client_context_id is cascadeOnDelete, but users are soft-deleted, so the cascade never fires: the column goes on pointing at a row that is still there while the relation resolves to null. resolveClientContext() branched on the relation, so "this is Alice's conversation" read as "this has no conversation" -- and a null context on a clients comment is the branch every client on the file reads. A staff reply into a departed client's private thread became a circular, and canAssignClient() was skipped on the way.
That is the invariant docs/feature-comments.md calls the rule everything hangs off: a clients comment carrying client_context_id = C is never returned to any non-staff viewer other than C, because one customer learning another exists is worse than leaking a comment's text.
Verified before merging: both new tests are red on main and green here, and the three that must not move stay green either way. Suite at 2093, PHPStan clean.
The second half is the same root cause through the other column. authorName() read a deleted client's comment as "Anonymous", which is what a visitor's comment looks like -- and a visitor's comment is governed by different rules, so the two must not be able to look the same. Whether the author is a visitor is now decided by author_id alone, the question isFromGuest() already asks.
Accepted consequence: a soft-deleted client's name is visible on their old comments during the erasure grace period, where it previously read as Anonymous. It goes for good when erasure removes the row.
Reported and fixed by @denkfabrik-li.
ProfileController::destroy() validated the current password and soft-deleted, without asking guardLastAdministrator() -- the rule the other four doors ask, at the one door where the account being removed is certainly signed in. The sole administrator could empty their own installation, and EnsureSetupIsComplete, which asks exists() and so skips trashed rows, then handed the first-run setup form to whoever loaded the page next. That form creates an active System Administrator, unauthenticated.
Verified before merging: on main the sole administrator's self-deletion succeeds and setup reopens; both new tests are red there and green here. Suite at 2088, PHPStan clean.
Two locks, because one of these questions is asked at five doors and the other at one. The guard closes the door. And "has this installation been set up" stops meaning "does it have a working administrator right now" -- a trashed staff row is still evidence that setup happened, counted now in both the middleware and SetupController::setupIsComplete(), which have to agree or the result is a redirect loop or an open form.
Worth recording: erasure force-deletes a self-deleted account after its grace period, so the second lock would expire on its own. It does not matter because the first lock stops the installation reaching that state, but a future change to either should know the other is not permanent.
An installation that has already lost its last administrator now finds setup shut. That is the point: recovery is php artisan projectsend:admin, which is also how every unattended container installs itself.
Reported and fixed by @denkfabrik-li.
The job walked the loose file ids and then every selected folder's subtree, adding whatever each pass found. A selection reaching the same file both ways got it twice: two copies of the same bytes, a total_size inflated by the repeat -- which is what the size cap is checked against -- and a file limited to a single download handed over in three copies while the log recorded one, because delivery logs per contained file and DownloadAllowance counts those records.
Verified before merging: the three new tests fail on main and pass here. Suite at 2082, PHPStan clean.
Two halves, because one fix does not cover both shapes. The added-ids list becomes a map keyed by id and the folder pass skips what is already in, before the per-file re-checks, so a duplicate does not spend an allowance twice either. And a folder sitting inside another selected folder is dropped before either is walked, which also settles which path the surviving entry keeps rather than leaving it to row order.
One measured cost, accepted: the pruning compares every selected folder with every other. The pathological case -- ten thousand sibling folders, the selection cap -- benchmarks at around twenty seconds of CPU, in a background worker, on a selection that would take far longer to compress. A sort-by-path-length version would be cheaper if it ever matters.
Reported and fixed by @denkfabrik-li.
Every group route asked StaffLibraryScope whether this viewer may act on this group except the two that read it. So a client-scoped staff member could open the edit screen of a group they cannot change, read its membership with addresses, and get the whole client roster in available_clients besides. The API twin returned the same membership.
Verified before merging: the three new tests fail on main and pass here. Two things checked beyond the report -- group membership is edited through separate, already-guarded routes, so narrowing the displayed list cannot remove anybody on save; and scramble:export regenerates byte-identical, as claimed. Suite at 2078, PHPStan clean.
The fix has two halves because one guard does not cover both shapes. Reading the group now asks the same reach question the write half asks. And both lists narrow through StaffLibraryScope::clients(), because a group nobody has shared anything with reaches nowhere, stays open to everybody, and can still hold a stranger's client.
Unscoped viewers are unaffected: clients() returns the whole roster for them and allowsGroupChange() is true by construction.
Reported and fixed by @denkfabrik-li.
The validation rule accepts 0 and "0" as well as false and does not cast, so a strict comparison against the validated array let two of the three spellings past the self-deactivation guard -- and the model's own boolean cast then stored exactly the value the guard had just decided was not a deactivation.
Reproduced on main before merging: {"active": false} is refused, {"active": 0} and {"active": "0"} both return 200 and switch the account off. Green on the branch, suite at 2074, PHPStan clean.
The fix reads the flag once with Request::boolean() and gives that same value to the guard and to the write -- the rule RolesController::guardScopeRemoval already documents for the same reason. Validation is unchanged, so the accepted inputs are the same; one of them just stops meaning two different things on its way through the method.
Follow-up for the release: this is a caller-visible change (200 to 422) and wants a line in api-changelog.md.
Reported and fixed by @denkfabrik-li.
The test named for carrying the durability verdict to the system widget asserted only has('system'), and system is an unconditional key of the render array -- the controller's own comment beside storage_durability says as much. So the assertion could not fail.
Confirmed here by deleting the line that supplies the verdict: the new assertion fails with "Property [system.storage_durability] does not exist", where the old one stayed green.
Test-only, no application code.
Reported and fixed by @denkfabrik-li.
EnforceTwoFactor exempts by route name, and only the GET half of the confirm-password screen had one -- Route::named() answers false for a null name, so the submission was never exempt. Enrolling requires password confirmation, so with enforcement on nobody could enrol at all: the form rendered, its POST was redirected to two-factor.show, auth.password_confirmed_at was never written, and every account on the installation was left with logout as its only working route. Including the administrator who turned the setting on.
Reproduced on main before merging: POST /confirm-password redirects to /settings/two-factor and the session flag stays unset. The widened pattern was checked against the route table -- password.confirm* reaches password.confirm and the newly named password.confirm.store and nothing else; password.reset, password.store and the rest are not under that prefix. Exempting the submission grants nothing further, since every other route stays bounced and store() still validates the password.
Reported and fixed by @denkfabrik-li.
Ten tests ran the real projectsend:update, which runs clear-compiled, which deletes bootstrap/cache/packages.php and services.php -- one copy for the whole checkout, shared by all eight workers of a parallel run. A worker booting in the window between that delete and its own rebuild reads an empty package manifest, registers no package service providers, and dies rendering the next page with "Target [Inertia\Ssr\Gateway] is not instantiable", in a file that has nothing to do with updates.
Verified here rather than taken on trust: a probe running the real update inside a test on main deletes the manifests, exactly as described. The branch is green at 2066 with PHPStan clean, and touches no application code.
The file already owned a double and explained why the artisan call is a seam; this extends it to the whole file and adds a test asserting the compiled caches survive.
Reported and fixed by @denkfabrik-li.
Two doors onto the client seat cap did not ask it. Both update()
methods -- the edit screen and PATCH /api/v1/clients/{id} -- clear
account_requested when a pending client is activated, under a comment
saying that counts as approval, and approval is the moment a seat is
spent. So a managed installation sitting at its cap kept taking clients
on for as long as registrations arrived, and self-registration is open
to strangers, so the supply of pending rows is not the operator's to
control.
Verified rather than taken on trust: the two new door tests were run
against the unguarded controllers and fail there, and every place in
app/ that clears the flag was enumerated to check no third door was
missed. There is none -- the other six already ask, and a conversion
refuses a pending account outright rather than approving it sideways.
The guard sits inside the approval branch, so an installation at its cap
can still rename a client it already holds. That is pinned by a test of
its own.
Conflicted with tonight's seat work in SeatAllowanceTest, which had
added an import beside the one this adds. Resolved by keeping both;
suite green at 2065 and PHPStan clean after resolution.
Reported and fixed by @denkfabrik-li.
The tests workflow has not parsed since c05927c1 added a second top-level `concurrency:` key four lines below the one that was already there. YAML refuses a duplicate key, so GitHub created a run and scheduled no jobs -- verified here with symfony/yaml ("Duplicate key concurrency detected at line 66") and against the run list: every run since is zero-job, including the commit v2.2.0 is tagged at and all five pushed tonight.
The linter workflow carries one block and kept running, which is why the tree read as checked when the suite had not run at all.
Reported and fixed by @denkfabrik-li.
The docblock argued for one definition by describing a control plane
showing "2 of 3 seats used" next to an application refusing the fourth,
and the two disagreeing. That was written as a thing to avoid. As of
today it is a screen: the hosted fleet console reads these numbers per
tenant out of projectsend:status --json.
Which makes two rules here load-bearing somewhere nobody editing this
file would think to look -- a deactivated staff account still holds a
seat, a client awaiting approval does not. Changing either changes what
a support person is told before it changes what a customer hits, and
the note is here so that is a decision rather than a surprise.
The quick-start list gates its "Add the rest of your team" step on
Capability::UsersManage, which is right and unchanged: it is the seam an
edition difference would travel through. The comment above it still gave
the old reason -- that a managed installation has no staff accounts of
its own to hand out -- which the capability opening on both editions
made false. The step has appeared on a managed installation's list since
623ad68, and GettingStartedTest already says so.
Found by sweeping every repo for the same claim after four others turned
up: core, both module packages, the migration tool, the customer portal
and the private docs. The remaining ones are in the portal's own
planning documents, which are its to correct.
A platform can see that an installation is running. It cannot see
whether anybody is still using it, and the difference is what separates
a customer from an abandoned free instance holding a database.
So the status probe gains one field:
"activity": { "last_staff_login_at": "2026-08-24T21:13:32+00:00" }
Null means no staff account has ever signed in, and the key is emitted
either way. That is the whole care in this change: "they said never" and
"we got no answer" have to stay distinguishable, because collapsing them
is how a broken probe reads as a dormant fleet.
Only interactive sign-ins count. Laravel's Login event does not fire for
token authentication, so an integration polling every hour cannot make
an empty installation look busy -- which matters when the reading is
used to decide something.
Derived from the activity log rather than denormalised onto users. A
column would cost a migration, a listener change and a backfill to save
one indexed MAX() over a table with a handful of rows on exactly the
installations anybody asks this about. Nothing prunes the log, and
erasure anonymises entries rather than removing them -- actor_type
survives on purpose -- so the answer does not change when the person who
gave it is forgotten.
Requested by the ProjectSend Cloud control plane, which has no other way
to learn the date. Recorded in docs/api-todo.md as deliberately a
command rather than an endpoint, for the reason the command exists at
all: it observes, it does not accept instructions.
It stopped being true in 623ad68, when users.manage opened on both
editions. The code moved and these did not, which is the worst kind of
comment: confidently wrong, and about the very rule a reader comes to
them to learn.
PlatformManaged claimed the tenant's own /users screens stay closed,
directly contradicting the UsersManage comment eleven lines above it.
routes/web.php said the same about the group it gates. The API
controller's docblock opened with "**Community only.**", and the
conversion screen's said a managed installation creates staff accounts
elsewhere.
Each now says what is actually true, and says the division the change
turned on: a platform sells the seats, the tenant decides who sits in
them. What limits a managed plan is the seat cap, not a shut door -- so
the API answers 422 at the limit rather than 403, which is a different
sentence to whoever is reading it.
On a managed installation with its staff seats full, /users/create opened
as though there were room. You typed a name, an address and a password
you had to invent, pressed Save, and the plan limit came back as a
validation error under the email field -- which reads as a complaint
about the address rather than a fact about the plan.
A full installation is an ordinary state on a plan sold by the seat, so
it is now stated up front. The list carries the seat position, the
button goes dead once the last seat is taken and says why, and the
create screen turns away anyone who reaches it by link or bookmark. The
guard in store() is untouched: that is still the rule, this is only the
door.
The refusal is worded once, in SeatAllowance, and the screen is handed
that sentence rather than writing its own -- two wordings of one limit
is how somebody ends up believing there are two limits. `full` is
derived there too, from the same comparison the guard refuses on, so a
screen cannot disagree with it about the edge (used > limit, after an
operator lowers a limit) and offer a button for a form that cannot be
submitted.
Clients get the same treatment: the cap exists there too, and reached it
the same way. Self-hosted installations have no limit, so they are shown
nothing about one.
file_comments.client_context_id is cascadeOnDelete, but users are
soft-deleted, so the cascade never fires: the column keeps pointing at a
row that is still there while the Eloquent relation resolves to null.
resolveClientContext branched on the relation, and a null context on a
Clients comment is the branch every client on the file reads -- so a
staff reply into one client's private thread became a circular to all of
them, with the canAssignClient check skipped on the way.
VisibleCommentScope says so in its own docblock: "A Clients comment
carrying client_context_id = C is never returned to any non-staff viewer
other than C ... A Clients comment with a null context is a staff message
to everyone on the file, and every client with access reads it."
Measured on main, with one file shared with two clients and the first of
them deleted after commenting:
column client_context_id 3
relation clientContext null
POST reply into her thread 201, stored with client_context_id null
read by the other client yes
Ask the column, and refuse when the account behind it is gone. There is
nobody left to answer, and the one outcome that must not follow from a
filled column is the broadcast, so this throws rather than falling
through to it.
authorName() had the same root cause from the other column: its docblock
claimed author_id cascades so there is no deleted author, and a deleted
client's comment was going out as "Anonymous" -- which is what a guest
comment looks like, and a guest comment is read by different rules. Guest
is now decided by author_id alone, the same question isFromGuest() asks,
and a trashed author is read with withTrashed(). Nothing comes back only
once the grace-period erasure has removed the row for real.
That read costs one query per comment whose author is trashed. Measured
on a ten-comment thread: 11 queries before, 21 after, against 20 for the
same thread with every author alive. Left as a lazy read rather than
eager-loading with withTrashed() at every call site, because the callers
would each have to remember it and the cost only applies to comments
whose author is gone.
Five tests, two measured red against the unfixed code (2 failed / 3
passed) -- one per column. The three that stay green either way are the
branches that must not move: a staff message with no context still
reaches everybody, a reply into a live client's thread still lands in
that thread alone, and a genuine guest comment is still anonymous.
Full suite passes (2053 passed / 2 skipped), PHPStan level 8 clean.
ProfileController::destroy() validates current_password and soft-deletes.
It never asks StaffAccounts::guardLastAdministrator(), and every other
door does: Staff update(), guardDeletable(), and both directions of the
role conversion. This is the one door where the account being removed is
certainly signed in.
An installation with a single administrator therefore had a button that
emptied it. Measured on main:
DELETE /settings/profile 302, the account is gone
live staff rows 0 (the row is trashed, not removed)
anonymous GET / 302 -> /setup
anonymous POST /setup a new active System Administrator
EnsureSetupIsComplete asks ->exists(), which excludes trashed rows, and
routes/web.php registers GET and POST setup with no auth and no guest
middleware -- correctly, since a fresh installation has nobody to
authenticate. SetupController::store() re-checks the same condition, so
both halves agreed with each other and both were wrong once the last
staff row was trashed.
Two locks, because one of them is asked at five doors and the other at
one.
First: destroy() now asks guardLastAdministrator(), the same call with
the same message as everywhere else. An administrator with a colleague
still goes, a non-administrator staff member still goes, and a client
still closes their own account.
Second: "has this installation been set up" is not the same question as
"does it have a working administrator right now", and only the first one
belongs in EnsureSetupIsComplete. A trashed staff row is still evidence
that setup happened, so it now counts -- in the middleware and in
SetupController::setupIsComplete(), which have to agree or the result is
either a redirect loop or an open form.
That second lock holds even if a future door forgets the first one.
Measured with the guard bypassed entirely and the row trashed directly:
GET / answers with the login screen and POST /setup creates nothing.
Worth stating plainly: an installation that has already lost its last
administrator will now find setup shut rather than open. That is the
point -- the recovery path for it is `php artisan projectsend:admin`,
which is also how every unattended container installs itself, not a form
that anybody on the internet can reach.
Six tests, two measured red against the unfixed code (2 failed / 4
passed) -- one per lock. The other four are the boundaries: a colleague
present, a staff member who is not an administrator, a client, and a
genuinely fresh installation that must still reach setup.
Two existing tests needed saying more clearly rather than changing:
ProfileUpdateTest's deletion cases now create a second administrator, so
that what they assert is self-deletion and not this new refusal; and
GettingStartedTest's "fresh installation" cases forceDelete rather than
delete, because a soft-deleted staff row is no longer a fresh
installation -- which is the whole of the second lock.
Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean.
BuildZipDownloadJob walks the loose file ids and then every selected
folder's subtree, and adds whatever each pass finds. A selection can
reach the same file from more than one of them, and nothing noticed:
file_ids [f], folder_ids [Reports]
-> ['report.pdf', 'Reports/report.pdf']
file_ids [f], folder_ids [Reports, Reports/Q1]
-> three entries, file_count 3, total_size three times the file
Two copies of the same bytes in one archive, and total_size is what the
size cap is checked against, so a selection could also be refused for a
weight it does not have.
The one that costs more than bandwidth is delivery. It logs one
FileDownloaded per contained file, and DownloadAllowance counts those
records -- so a file limited to a single download left in three copies
while the log recorded one. Measured: three entries, one record.
Two causes, so two halves.
`$added` is now keyed by id instead of being appended to a list, and the
folder pass skips a file already in the archive. A lookup rather than a
scan because the selection cap is 10000 sources. The loose pass runs
first, so a file picked both ways sits under its loose name; either
answer is defensible, but it has to be the same one every run.
And a selected folder inside another selected folder is dropped before
either is walked. Zipping both would reach every file in the inner one
twice, and which path the surviving entry ended up under would be decided
by the order the rows came back in. Keeping the outer folder keeps the
fuller path -- Reports/Q1/report.pdf rather than Q1/report.pdf.
Containment is decided on the materialized path, so it is one comparison
per pair with no queries: a folder's path starts with an ancestor's
subtreePathPrefix(), and both end in '/', so /5/ cannot match /50/.
Not changed: the per-file re-checks inside the folder pass. Visibility
and the download allowance are still re-derived per file, and the skip
happens before them, so a duplicate never spends an allowance twice
either. Nor the selection endpoint -- a caller may send whatever
selection they like, and the job is where it is resolved.
Four tests. Three measured red against the unfixed job (3 failed / 32
passed): the loose-plus-folder case, the nested-folder case, and the
three-way case asserted through delivery rather than through the archive.
The fourth -- two selected folders that merely share a name are both
zipped -- is green either way and guards the pruning against being about
names rather than containment.
Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
Every other group route asks StaffLibraryScope whether this viewer may
act on this group. GroupsController::update() and ::destroy() do, and so
do their API twins -- all four with abort_unless(allowsGroupChange, 404).
The two that read do not: edit() and Api\GroupsController::show() had no
boundary at all.
What they hand over is the membership, name and email per member, plus
the whole client roster of the installation as available_clients. So a
client-scoped staff member could open a group whose contents they cannot
see, read off every client on the installation, and only be refused when
they pressed save.
Two halves, because the leak has two shapes:
- The group itself. Reading it now asks the same reach question the write
half asks, one step earlier, with the same 404 -- a group that reaches
past the viewer's library is not theirs to open either.
- The lists inside it. Both narrow through StaffLibraryScope::clients(),
the listing half of the rule this screen's buttons are already guarded
with: allowsGroupMembership refuses removing a member outside the
roster, and refuses adding a client outside it. Naming them anyway,
with their address, is the mistake ClientsController made before
clients() existed -- that method's own docblock says so.
The reach guard alone would not have been enough. A group nobody has
shared anything with reaches nowhere, so it stays open to everybody --
and it can still hold a stranger's client. That case is why the lists
narrow separately, and there is a test for it.
members_count is left whole on purpose: a size is not an identity, and it
is the same number the group listing already reports.
GroupResource's docblock claimed members are safe to expose because "the
group edit screen already shows [them] to anyone holding edit_groups".
That was a claim about a screen, and it stopped being true the moment the
screen narrowed. Reworded to say what now holds it up, and where.
Not changed: the group listing. It reports names and member counts, not
identities, and every button on it is guarded. Nor Api\GroupsController::
index(), for the same reason. Nor the API document -- scramble:export is
byte-identical, because GET /groups/{group} already documented a 404.
Four tests. Three measured red against the unguarded controllers (3
failed / 21 passed): the group cannot be opened at all, the edit screen
stops naming strangers, and the API twin narrows what it hands back. The
fourth -- an unscoped viewer keeps the whole roster and every member -- is
green either way and guards against the fix over-refusing.
Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
Api\UsersController::update() compares the validated value strictly:
if ($user->is($actor) && ($validated['active'] ?? true) === false) {
The `boolean` rule accepts 0 and "0" as well as false, and it does not
cast. `0 === false` is false, so the refusal never fires -- and the
model's own `boolean` cast then stores as false exactly the value the
guard had just decided was not a deactivation.
Measured against main, with a second administrator present so that
guardLastAdministrator is not what answers:
{"active": false} -> 422, still active
{"active": 0} -> 200, active is now false
{"active": "0"} -> 200, active is now false
The method's own docblock says it is "Refused with a 422 if the change
would leave the installation with no active administrator, or if you
would be deactivating yourself", and the web screen does refuse. This is
the API half of that sentence.
RolesController::guardScopeRemoval documents the rule this breaks, in the
same words: callers resolve the flag with Request::boolean() and hand the
same value to the guard and to the write, deliberately, because reading
the validated array and comparing it strictly "would let a request
through here that the model's `boolean` cast then stores as false anyway
-- the guard and the write disagreeing about one value is exactly the
shape this guard exists to prevent".
So read it once, with Request::boolean(), and give that one value to both.
Not changed: the validation rule. It stays `boolean`, so the accepted
inputs are the same as before -- what changes is that one of them stops
meaning two different things on its way through. Nor anything about
deactivating somebody else: all three forms still work, and there are
tests saying so.
Six cases from two datasets. Two measured red against the unfixed
controller (2 failed / 4 passed): 0 and "0" on yourself. `false` was
already refused, and the three "somebody else" cases are green either way
-- they guard against the fix over-refusing, not against the bug.
Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean.
The test named for carrying the verdict to the system widget only
asserted that the 'system' key exists. It is an unconditional key of the
Inertia::render array and is allowed to be null, and Inertia's has() is a
key check, so the assertion held whether or not the verdict was in there.
Deleting 'storage_durability' from DashboardController::systemInfo() left
the file green.
Substitute the class the way the rest of the file already does and assert
the payload, as InstallationKindTest does for install_kind next door.
`UpdateWelcomeTest > staff who may not read system information are not
interrupted` fails on a parallel run roughly one time in six, with
BindingResolutionException: Target [Inertia\Ssr\Gateway] is not
instantiable
in a file that has nothing to do with updates. Run alone it is green
every time. The cause is not in that file.
`clear-compiled` deletes bootstrap/cache/packages.php and
bootstrap/cache/services.php. There is one of each for the whole
checkout, and `pest --parallel` gives eight worker processes the same
one. Instrumented over three full runs, the real command ran 12 times per
run -- 11 from UpdateCommandTest, 1 from StaleCodeNoticeTest -- and the
other workers observed the package manifest missing at boot 46 times.
What that costs is in PackageManifest::getManifest():
if (! is_file($this->manifestPath)) {
$this->build();
}
return $this->manifest = is_file($this->manifestPath) ?
$this->files->getRequire($this->manifestPath) : [];
A worker that loses the second is_file() to another worker's unlink gets
`[]`: no discovered packages, so no package service providers, so
Inertia's is never registered and `Inertia\Ssr\Gateway` is never bound.
The next page it renders dies in the compiled root view, where
`@inertia` resolves that interface. Any test in any file, whichever one
happened to be booting.
Both halves measured. Building the manifest with inertia-laravel in
`dont-discover` reproduces the reported failure exactly -- same test,
same exception, same frame (`app('Inertia\Ssr\Gateway')` from the
compiled app.blade.php). And 12 real `clear-compiled` calls per run is
the count above.
UpdateCommandTest already owns a double for this, and says why in its own
docblock: the artisan call is a seam. Nine of its tests and one in
StaleCodeNoticeTest simply do not use it. None of them asserts that a
command ran -- they assert EnsureSystemRoles, the settings writes, the
activity log and the welcome marker, and the double touches none of
those. So the seam now covers the file, through a beforeEach rather than
per test, because the next test added here should not have to know any of
this.
The double moves to tests/Support and its helper to tests/Helpers.php,
for the reason that file documents: Pest hands whole files to workers, so
a class declared in one test file does not exist for another.
Not changed: UpdateInstallation. `clear-compiled` belongs in a real
update. Also not changed: giving each worker its own bootstrap/cache
through APP_PACKAGES_CACHE and friends. That would make the destruction
cheap rather than remove it, and nothing in the suite needs those
commands to run at all.
One new test, on the files rather than on the recorded call list -- a
future double that forgot to intercept one command would still satisfy a
call-list assertion. Counter-checked: with the beforeEach removed it goes
red on both manifests being gone (1 failed / 22 passed).
Eight consecutive parallel runs green after the change; the manifests'
mtimes are untouched by a full run, where before they were rewritten
every time. Full suite passes (2049 passed / 2 skipped). PHPStan level 8
clean -- it analyses `app` only, so it does not cover this change.
Pre-existing and left alone: pint reports `ordered_imports` on
UpdateCommandTest.php. Its import block is misordered on main too.
FoldersController::destroy() authorizes `delete` on the folder and nothing
else. FolderService::delete() then soft-deletes every file in the subtree,
and File::booted()'s `deleted` hook takes the bytes off disk. There is no
restore.
FilePolicy::delete asks two questions the folder route never reaches:
`delete_others_files` for somebody else's upload, and
StaffLibraryScope::allowsFile on top of it. Measured with a role holding
create_own_folders, delete_files, upload and edit_files -- the shape the
Client Manager system role already has, minus delete_others_files:
DELETE /files/{someone-elses} 403, the file is still there
DELETE /folders/{their-folder} 302, the file and its bytes are gone
MyFoldersController::destroy already refuses the client half of this exact
cascade, and says why: "Owning the folder is not authority over content
someone else put in it... Refuse rather than silently destroy them." This
is the staff half of the same sentence.
Counted rather than asked per file. A folder can hold thousands, Gate
resolves a fresh policy for every check, and a per-row policy check on a
listing is the cost 0a8b609e went to some trouble to remove. Both halves
of FilePolicy::delete are expressible in SQL: the permission half is
constant for the viewer, and the library half is the query
StaffLibraryScope already memoises per request. Somebody holding both
delete permissions with no library scope short-circuits before the query
runs at all, so the common case pays nothing.
Not changed, deliberately:
- The service. FolderService::delete stays dumb. Its other caller applies
the client rule ("files you did not upload"), which is a different
predicate, and putting both in one place is the drift this codebase
keeps refactoring away from.
- The client half. MyFoldersController is already correct.
- Nothing partial. A blocked folder is left whole rather than emptied of
what the actor may delete -- half a tree is worse than either answer.
Worth saying plainly: this is a behaviour change. A folder delete that
used to succeed now refuses, and somebody will notice. The alternative is
irreversible loss of files the same person is refused one route over.
Six tests. Four measured red against the unguarded controller (4 failed /
2 passed), one per half of the predicate: the permission half, its
message, a nested file, and the library half -- that last one with both
delete permissions held, so only StaffLibraryScope can refuse. The two
that stay green either way are the other side of the question -- that a
folder holding only your own files still goes, and that an administrator
holding both permissions is unaffected. They guard against the fix
over-refusing, not against the bug.
Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean.
The new string is English only, per CONTRIBUTING.md -- translations are
their own pass.
SeatAllowance says a cap is only a cap if every door asks, and has a test
per door for that reason. Two doors do not ask.
The moment a seat is spent is the moment `account_requested` is cleared.
Five places do that. approve(), both store()s and ClientProvisioning ask
guardClient(); AccountConversion asks it through guardToClient(). The two
update()s -- web and API -- clear the flag with no guard at all, under a
comment that names exactly what they are doing:
// Activating a pending account through the edit screen counts as
// approval and clears the request flag.
Measured with clients: 0, one pending registration:
POST /account-requests/{id}/approve refused, flag still set
PATCH /clients/{id} active=true approved, clientUsed() 0 -> 1
PATCH /api/v1/clients/{id} active=true approved, clientUsed() 0 -> 1
A managed installation at its cap therefore keeps taking clients on, from
the edit screen or a PATCH, for as long as registrations keep arriving --
and self-registration is open to strangers, so the supply is not the
operator's to control.
Inside the branch, not above it. Above it, an installation sitting at its
cap could not rename a client it already holds, which would trade one
wrong refusal for another. There is a test pinning that.
The field is `active` rather than the default `email`: on this screen the
administrator is toggling `active`, and an error under the email field
would point at the wrong thing. approve() has no form of its own, so it
keeps the default.
Three tests, per door as the file's other eight are. The two door tests
were measured red against the unguarded controllers (2 failed / 18
passed). The third -- that editing an existing client still works at the
cap -- is green either way: it guards against the fix being written a
line too high, not against the bug.
Full suite passes (2051 passed / 2 skipped), PHPStan level 8 clean.
One thing worth knowing that this branch does not touch: on a parallel
run, `UpdateWelcomeTest > staff who may not read...` fails roughly one run
in six on untouched main, with `BindingResolutionException: Target
[Inertia\Ssr\Gateway] is not instantiable`. Measured over 24 baseline runs
before this change existed. It is not this fix, and it is not in scope
here, but it will start being visible as soon as the workflow parses
again.
EnforceTwoFactor exempts by route name, and only the GET half of
confirm-password has one. routes/auth.php:95 names the form
`password.confirm`; :98 registers its submission with no name at all, and
Route::named() answers false for a null name.
So the loop the exemption exists to prevent is still there, one step
further along. With Setting::TwoFactorEnforcement set to staff, clients
or all, an un-enrolled account walks:
GET /dashboard -> two-factor.show
GET /system/settings/security -> two-factor.show
PATCH /system/settings/security -> two-factor.show
POST /settings/two-factor -> /confirm-password (RequirePassword)
GET /confirm-password -> 200, the form renders
POST /confirm-password -> two-factor.show <- not exempt
`auth.password_confirmed_at` is never written, so enrolling can never
start, and every route that is not on the exemption list stays shut --
including Settings -> Security, the one screen that could turn
enforcement back off. Logout is the only door left; recovery is CLI or
database access. It takes one administrator turning the setting on to
reach it, and it reaches every account on the installation at once,
including their own.
The fix is the name. `password.confirm*` then covers both halves of one
screen, matching `two-factor.*` in the same expression; the namespace
belongs entirely to a flow enrolment already depends on being reachable,
and the route table has nothing else under it -- `password.confirm` (GET)
and `password.confirm.store` (POST) are the two it reaches.
Exempting the submission grants nothing further. store() validates the
password, writes a session flag and redirects; the redirect it issues
enters this middleware like any other request, so Settings -> Security is
still answered with two-factor.show after confirming. What changes is
that enrolment can now be started.
Two tests, both measured red against the unfixed middleware: the password
confirmation sticks, and enrolment can be started afterwards (the secret
is written and the screen reports `pending`).
Also named the redirect the existing test settles for. `->assertRedirect()`
with no target passes on this middleware bouncing the request back to
two-factor.show, which is the shape that file exists to refuse. It is a
clarification rather than a guard -- that assertion is green either way,
since the redirect it sees comes from RequirePassword.
Full suite passes (2050 passed / 2 skipped), PHPStan level 8 clean.
c05927c1 added a `concurrency:` block on the premise that the suite never
got one. It already had one, four lines above -- the hunk header of that
diff reads `@@ -50,6 +50,23 @@ concurrency:`, which is the existing block
it was appended below.
A YAML mapping cannot carry the same key twice, so the file has not
loaded since. GitHub still creates a run and then schedules nothing:
553f5fd2 (last green) run 33036453748 jobs=1 ci -> success
d58e4830 (main) run 33114046849 jobs=0 failure
Every run since has that shape, and the run list names it in passing:
those runs appear as `.github/workflows/tests.yml` where the green ones
appear as `tests`, because the `name:` key sits inside the file that did
not parse. `linter` is unaffected -- it carries one block -- which is why
351da21e shows a green linter beside a failed tests run, and the tree
reads as half-checked rather than unchecked.
Reproduced with a parser rather than inferred from the job count:
before -> THREW: Duplicate key "concurrency" detected at line 66.
after -> parsed ok, top-level keys: name,on,concurrency,jobs
Kept the second block, verbatim, because it is the one c05927c1 meant to
end up with and its comment carries the reasoning -- including the
tradeoff that an intermediate commit on `main` can end up with no run of
its own. The two group keys are interchangeable: `github.workflow` is
constant within a workflow, so `tests-${{ github.workflow }}-${{ github.ref }}`
and `tests-${{ github.ref }}` produce the same grouping. Worth knowing
that lint.yml still uses the first shape, if you would rather the two
files read alike.
No test. The failure is loud on the next push, and a test that parses a
workflow file would be a second place to keep the same rule.
It read "limited to 1 staff accounts" -- the number sat directly in front
of a countable noun, which is the message a free-tier customer meets the
first time they try to add anybody.
Adding plural forms would fix English and not much else. Polish, Czech and
Russian inflect the noun by the number in front of it, on a three-way split
that a two-form string cannot express, so ':count kont' cannot be right for
every value however many variants it carries. Ending the sentence on the
number means no language has to agree with it -- the same shape the other
counted strings here already use.
Both strings rewritten in all sixteen locales rather than left to the next
translation pass, since the old key would otherwise go missing and block a
build. Checked at 1 and at 25 in English, Spanish, German, Polish and
Russian.
The summary reads well and says nothing a reader can chase. The numbers and
titles are the way back to the original report, so they go at the end where
they are available without being in the way -- summary at the top for
whoever is deciding whether to upgrade, paper trail at the bottom for
whoever is looking for their own bug.
Generated from the closed-since date rather than hand-picked, and titled
'closed since' rather than 'fixed in' so no per-issue judgement is needed
about how each one was resolved.
Laravel's notification view writes the subcopy URL as [$url]($url).
The HTML half parses that into an anchor; the text half parses nothing,
so it arrives as literal brackets around a duplicated address. With the
button line above it the URL appeared three times in one message.
It reads as broken, and it reads broken in a specific direction: a long
opaque token, the recipient's address in the query string, and a
duplicated link in brackets is the shape of a phishing template. On a
password reset, which is often the first mail an installation ever sends
somebody, from a domain with no reputation yet.
Fixed the way every other component in that message already handles the
same split -- one name, two files, Laravel picks per half. Which meant
publishing the framework's view for a one-line change, so there is a note
in it saying to re-copy on upgrade.
Seen in a real reset mail, not in a test.
The detail underneath was 400 lines of two-and-three-sentence entries.
Written to be complete, and complete is not the same as read: the list at
the top already says what changed, and the long version mostly restated it
at length for somebody who had stopped reading.
The credits do not go with it. Most of the boundary work in this release
came from outside, and dropping the names to save space would be taking
somebody's contribution off the record to tidy a file. One line at the end
instead of twenty inline.
TRUSTED_PROXIES said 'see the fix below' and there is no longer a below.
2026-08-27 14:54:24 -03:00
252 changed files with 16199 additions and 1304 deletions
- **A limited staff role can no longer rename or delete a group it has no part in.** Group
membership was already held to that boundary; the group itself was not, which was the sharper half
— sharing a file with a group is how its members reach that file, so deleting the group takes the
access away from every one of them, including clients outside the person's own list. A role
limited to its own clients can still manage any group that shares nothing beyond what it can
already see, so a group it created, or one holding its own clients, stays fully editable. Nothing
changes for an administrator or any unrestricted role.
- [#1627](https://github.com/projectsend/projectsend/issues/1627) — Errors while installing via Docker
- [#1648](https://github.com/projectsend/projectsend/issues/1648) — A deleted account's email address can never be used again
- [#1661](https://github.com/projectsend/projectsend/issues/1661) — Docker update instructions do not update ProjectSend when using official Compose setup
- [#1662](https://github.com/projectsend/projectsend/issues/1662) — Preview files not available on v2.1.0
@@ -504,9 +485,23 @@ class FilesController extends Controller
});
$requested=count($validated['file_ids']);
$message=$updated<$requested
?__(':updated of :requested selected files were updated. The rest were skipped because you don\'t have permission to edit them.',['updated'=>$updated,'requested'=>$requested])
$updated+$unreachable===$requested=>__(':updated of :requested selected files were updated. The rest were skipped because you don\'t have permission to edit them.',['updated'=>$updated,'requested'=>$requested]),
default=>__(':updated of :requested selected files were updated. The rest were skipped because you don\'t have permission to make those changes.',['updated'=>$updated,'requested'=>$requested]),
};
returnback()->with('success',$message);
}
@@ -516,28 +511,17 @@ class FilesController extends Controller
Gate::authorize('delete',$file);
$name=$file->name;
// Soft delete; the bytes stay on disk until a purge policy
// lands with the retention work.
// Soft delete of the row — but not of the bytes. File::booted()'s
// `deleted` hook runs FileDiskCleanup on commit, so the upload and
// every cached rendition of it are gone from disk by the time this
// returns. The row is kept because version chains, the activity
// log and the erasure grace period all still point at it; nothing
// serves it (route-model binding 404s), and nothing ever
@@ -38,6 +42,24 @@ class DisableCaptchaCommand extends Command
Captcha::forgetDisplayCache();
CaptchaVerifier::forgetOutage();
// Managed keys are not this setting. Captcha::resolve() reaches
// them from config and returns before it ever looks at
// Setting::CaptchaProvider, so on an installation using them the
// write above changed a value nothing reads. Saying "CAPTCHA is
// off" there would be false, and false in the worst direction: an
// operator who is still being challenged would stop looking,
// having just been told the thing challenging them is gone.
//
// Read after the write rather than before it, because the write is
// what makes the answer meaningful — if this still resolves to
// something, the something is not ours to switch off.
if($captcha->managedKeysSelected()){
$this->warn('Nothing changed. This installation uses CAPTCHA keys supplied by the platform, and those do not come from the setting this command writes.');
$this->line('Set PROJECTSEND_CAPTCHA_DISABLED=true in the environment and restart to switch it off.');
returnself::SUCCESS;
}
$this->info('CAPTCHA is off. Your keys are still stored — switch it back on at /system/settings/captcha.');
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
ignacionelson