The OpenAPI document gains the seven folder operations; `ancestors` gets
an explicit type so the schema says what it holds rather than Scramble's
guess. The guide gets a Folders section, the folder abilities, the
idempotent create under "Retries", and public folders under "Not in v1".
The abilities table was split in two by a blank line, with the groups row
left under the paragraph after it; both are back in the table.
An integration could put a file into a folder by id but could not see,
make or arrange the folders themselves, so mirroring a directory tree
into ProjectSend was impossible over the API. The hosted AI connector
already creates, lists and shares folders.
GET /folders polls like every list (updated_since, cursor) and filters
on parent_id, top_level and search. Each folder carries its ancestors
and a display path, trimmed for a client-scoped token to the folders it
may see (BreadcrumbBuilder::visible's rule), worked out for a whole page
in two queries by FolderTrails.
POST /folders returns an existing folder of the same name in the same
place with a 200 rather than making a second one, so a retried request
is safe. PATCH renames and moves. DELETE refuses a non-empty folder with
409 unless content_action=cascade_delete is sent, and then asks
UndeletableFiles exactly as the web does. Sharing goes through
FolderSharing.
Every write uses the web's policy, scope and FolderService, and asks
Folder::uploadableBy for every parent it writes, creation included.
Public state stays web-only: the resource reports `public`, nothing here
changes it. A file's `folder` now carries `parent_id` as well.
Sharing a folder was four steps written in the web controller: the
assignment row, the activity entry, the in-app notification and the
digest email. The hosted edition's AI connector repeated them, because
there was nothing in the core to call, and the two copies had already
drifted (one re-notifies on a repeated share, the other does not). The
folder API about to land would have been a third copy.
FolderSharing is the folder twin of FileSharing, and the web controller
now calls it. Behaviour on the web is unchanged.
The count of files a staff member may not delete inside a folder's
subtree moves out of FoldersController into UndeletableFiles, for the
same reason: deleting a folder over the API has to ask exactly the
question the web screen asks before the cascade takes files with it.
Two edges of the staff folder screens, found while the folder API was
built to answer the same questions.
A client-scoped staff member can hold one of their clients' folders that
sits inside somebody else's tree. The breadcrumb above it named every
folder on the way up, including ones their library does not show them.
It now starts at the first folder they can reach, as the client portal's
already does (BreadcrumbBuilder::visible). Unscoped staff see the whole
trail as before.
Creating a folder did not ask Folder::uploadableBy for its parent, though
every other write of a parent_id does: a folder inside a public one is
public. Files were already refused there by the upload check, so what
this closes is an empty folder's name appearing on a public page without
upload_public. Staff holding upload_public, or creating inside a private
folder, are unaffected.
The sign-in, password reset, setup and share-link pages drew a custom logo
in a box 80 pixels tall, up from 48 in 2.6.0. Tested on 2.6.0, a square
logo still read as small on both phone and desktop. The box is now 128
pixels tall and up to 320 wide. The card is 384 wide, so a wide logo still
fits a phone. ProjectSend's own logo is unchanged.
The Branding → Logo hint states the new size. Its existing translations
are carried over with only the numbers changed, rather than left to fall
back to English.
Reported by @jiits (#1798)
#1804 dropped every /up request from the nginx access log, so the
container's health checks stopped flooding `docker logs`. That also hid
the failing ones: when the container goes unhealthy, the 5xx from /up is
the line someone looks for, and Docker's health status alone does not say
why.
Key the map on the status as well as the path, so only a 2xx /up is
dropped. Verified against the 2.6.0 image: a 503 /up logs, a 200 /up does
not, and a 200 /upload still logs.
Core imports any stylesheet a package ships under resources/css after its
own app.css, and marks the pieces worth restyling with data attributes:
the staff shell (data-surface="staff"), the header, cards, buttons with
their variant, list toolbars, table frames and the default logo marks.
The layout takes its icons from projectsend.icons when a package names
some, replacing the defaults as a set.
Core names no package and no style. With nothing installed that ships a
stylesheet or icons, nothing renders differently.
Eight strings, in all sixteen locales: bulk delete's confirmation and its
error, the upload page's "Uploading into", and the Branding page's site-name
switch and logo size hint.
"Delete files" is already the label of the delete_files permission, and
several locales translate it as a noun ("deletion of files"), which reads
wrongly on a button. "Delete" is translated as a verb everywhere.
The selection bar could zip and bulk-edit the ticked files, including
moving them to a folder, but deleting was one file at a time.
A Delete button now appears when at least one ticked file is one this
person may delete. It asks for confirmation and sends only those files.
The server asks each file the same question a single delete asks, through
FilePolicy, and gives each the same soft delete and the same FileDeleted
activity entry. A file the person may not delete is dropped from the
batch rather than failing it, as bulk edit already does. A batch with
nothing left to delete is a 422. The route sits before files/{file},
which would otherwise read "bulk-delete" as a file id.
Reported by @lolgufdHD (#1800)
Inside a folder, Upload opened the upload page with no folder, so every
file landed at the top of the library and had to be moved. The client
portal already carried the folder; the staff page now does the same.
The upload page takes ?folder=, says "Uploading into <folder>", passes
it to the upload, and sends a multi-file upload back to that folder. The
same two checks as the portal's upload page: a folder outside the staff
member's library is a 404, so the page never confirms it exists, and one
they may not upload into is a 403. ChunkedUploadsController still checks
the destination again when the upload starts. While searching, the
button keeps uploading to the top, since results span folders.
Reported by @lolgufdHD (#1801)
Saving a settings form could open Inertia's error dialog showing
{"count":0}. back() prefers the Referer and falls back to the URL the
session recorded last. Laravel records every GET not marked as Ajax, and
the notification bell's plain fetch() of its unread count is not marked,
so the poll became "the previous page". Where the Referer did not arrive,
because a proxy or a browser stripped it, the save redirected to
/notifications/unread-count, and Inertia rendered the JSON as an error.
The session middleware is swapped for a subclass that skips recording
when the request asked for JSON. The rule is about the request, not about
that one route: nothing that asked for JSON is a page anybody goes back
to. Inertia visits ask for HTML and are recorded as before, and the
middleware order is unchanged (the sorter matches the subclass by its
parent).
A test reproduces it: open the privacy form, poll the count the way the
bell does, and save with no Referer. It failed with a redirect to
/notifications/unread-count before this change.
Reported by @0xVavaldi (#1799)
The sign-in, password reset, setup and share-link pages drew a custom logo
48 pixels tall, so a square logo was a 48x48 stamp. It now gets a box 80
pixels tall and up to 240 wide, so a square logo reads and a wide one still
fits a phone. ProjectSend's own logo is unchanged.
The site name appeared nowhere on those pages except as the image's alt
text. A new switch on Branding → Logo prints it under the logo. It is off
by default, because many logos already say the name and would then say it
twice. It is stored on the branding row, gated like the rest of the
screen (staff, edit_settings, branding.customize), and a withheld
capability takes it off the pages along with the logo. The branding API's
logo endpoint reports it as show_site_name.
The Logo tab now says what size to use and which formats are accepted.
SVG stays refused: it can carry script, and the logo is served from the
site's own origin. The crop tool the issue also asks for is not part of
this.
Reported by @jiits (#1798)
PHP 8.5 deprecated PDO::MYSQL_ATTR_SSL_CA, so loading config/database.php
printed two "Deprecated" warnings, one for each of the MySQL and MariaDB
connections. On a server that displays warnings, they appeared on every
page.
Pdo\Mysql::ATTR_SSL_CA exists since PHP 8.4, which is our minimum, so no
version check is needed. Checked by loading the real config file under
PHP 8.5 with pdo_mysql: the old file prints both warnings, and the new one
prints none and sets the same option.
Reported by @jiits (#1796)
The README now opens its instructions before the screenshots, and says
what the quick start assumed: Docker Engine with Compose installed, your
own user in the docker group (so nobody reaches for sudo), and which
directory the commands run from.
The quick start also saved the file as compose.example.yaml and started
it with -f. Every later command in DOCKER.md, UPDATE.md and the migration
guide is a plain `docker compose ...`, which only finds a file called
compose.yaml, so each of them failed with "no configuration file
provided". It is now saved as compose.yaml, as the Docker Hub page
already said, with one line for people who kept the old name.
The migration guide covered "Legacy on this machine, ProjectSend in
Docker" in one sentence. It now has a worked route through a bundle. The
exporter runs with the host's own PHP, where Legacy's `localhost`
database really is local, so no container networking is needed. The
guide also says why Direct is harder there: the database, and hardlinks
that cannot cross a mount. Step 2 was run against a real v1 install on
the host (60 files, 63 MB).
Reported by @lukatong (#1635), with the install and migration gaps
pointed out by @jjoelc.
Downloads, previews and public links from managed storage have failed in
every browser since late August with the bucket's AccessDenied XML. The
platform pins each instance's R2 key to our servers' IP (portal 0125be7,
2026-08-26). Core started redirecting to signed URLs at about the same
time (57540164, d6fd5a91). A signed URL carries every restriction of the
key that signed it, so it worked from the server and nowhere else.
A disk can now name another disk in `signing_disk`, and links are signed
with that one. The platform gives it a read-only key without the IP pin.
The read-write key stays pinned. A name that points at no configured disk
is ignored, and the file's own disk signs as before. Uploads are
unaffected: they go through the server, and nothing else signs.
Reported from the shared free instance. A client deleted their own account
on 2026-09-18. Their five files kept serving through share links with no
expiry for the whole 30-day grace period. Somebody who asked to leave
stayed published.
Rule 1: once an account is soft-deleted, its uploads are served to nobody
but staff. That covers the client scope (assignment, group, shared
folder), share links, the public listing, public comments and zips.
Staff keep them, because the grace period exists to undo a mistake.
Nothing is deleted and share links are kept, so a restored account is
served again. A withdrawn share link answers like a token that never
existed. In practice this only meets self-deleted accounts: an
administrator deleting an account that owns anything must already choose
to delete or reassign it.
Rule 2, new in Privacy settings: when someone deletes their own account,
their files are deleted "when the grace period ends" (the default, and
today's behaviour) or "right away". "Right away" uses
DeletedAccountContent's cascade: their own uploads, and their folders only
if nothing else is left inside. It runs in the same transaction as the
account delete. A platform can force "right away" through the new
ResolvingSelfDeletion hook. The screen then shows the choice as set by
the hosting plan instead of offering a switch.
A second setting decides whose deletion both rules apply to: any account
(the default) or clients only. A staff member's uploads are often the
organization's work for its clients.
The delete-account screen now says what happens to the files before the
person confirms. New strings are in all sixteen locales.
A new capability, granted by both editions. Self-hosted installs keep zip
downloads as they are. A platform removes it through
PROJECTSEND_CAPABILITIES_DISABLED. The free shared instances do this,
because building an archive holds the zips worker, the disk and a CPU on
a server that thousands of accounts share.
- The three zip routes sit behind capability:downloads.zip, so a
hand-made request gets a 404, not just a missing button.
- BuildZipDownloadJob refuses a build that was queued before the key went
away. The row ends failed and is never stamped as started.
StalledZipBuilds stays quiet when the key is off, so leftover rows
raise no worker banner.
- The zip buttons are hidden. In the portal, the checkboxes and the
selection bar are hidden too, since they exist only to pick files for a
zip. Staff /files keeps its checkboxes, which also drive bulk edit.
- Archives already built are not touched. They expire on the normal
purge schedule.
- A guard test walks the router. It fails if any route that reaches
ZipDownloadsController, or dispatches the build job, lacks the
middleware. No API route builds zips today.
The case goes last in the enum, because the control plane reads keys in
enum order.
Eight strings, in all sixteen locales: the seven the System card gained
in #1794 (where files are stored, what is free, the temporary upload
space and why it exists), and the password dialog's "Too many attempts",
which until now was only in Spanish.
c9a4b552 added custom-pr-sign-comment to close a hole that was not
there: the key was already set further down the same block, with the
same value, and has been since 2.0.0. The action was already comparing
the whole comment, so a comment wrapping the phrase in other text was
never recorded as a signature, and loosening the job filter in
0a7330d5 opened nothing. The second copy made the file invalid, and
GitHub stopped running the CLA check at all.
This removes the copy and says at the original why it is set, since it
repeats the action's default phrase and looks removable.
The action, left to its default, searches a comment for the signing
phrase, so a single line such as "I LIE, I have read the CLA Document
and I hereby sign the CLA. I do not sign it" was recorded as a
signature. The exact match in the job filter used to block that, but
only by accident, and it also blocked @JensS's real signature, which
had line breaks after it. Loosening that filter in 0a7330d5 let both
through.
custom-pr-sign-comment makes the action compare the whole comment,
trimmed and lowercased, against the phrase. Trailing line breaks still
sign; anything else around the phrase does not. The job filter stays
loose, since it only decides whether a runner starts.
The CLA job only ran for a comment exactly equal to the signing phrase.
@JensS signed on #1792 with the phrase followed by line breaks
("...sign the CLA\r\n\r\n\n"), the comparison failed, the job was
skipped, and the signature was never recorded, so all three of his pull
requests still show the CLA as unsigned.
The action itself matches the phrase loosely. The filter in front of it
now does too: contains() for the signature, startsWith() for recheck,
both case-insensitive. It still keeps the job off ordinary comments,
which is what it is there for.
The confirm-password screen hid its field from every account that was
not Local, and told it to set a password instead. That is right for an
account a provider created, which has no password anybody has seen. It
is wrong for a directory account: its password is the directory's,
PasswordVerification accepts it, and /settings/password refuses to let
it set another. So an LDAP account could not get past the confirmation
at all, and everything behind it, turning on two-factor included, was
out of reach. The new dialog copied the same question.
Both now ask whether the account came from a provider, and the prop is
called has_password, which is what it means. The dialog also clears
the typed password when it closes or once it has been used, instead of
keeping it in component state.
password.confirm redirected every write to the confirm-password screen.
A redirect cannot carry a POST body, and Redirector::guest() only
remembers the exact URL of a GET, so after confirming, the user landed
back on an empty form and the action never ran. On the API token forms
that meant typing the name, the scopes and the expiry again.
An Inertia request now gets a 423 marked X-Password-Confirmation. A
dialog mounted around every page catches it, asks for the password over
the current page, and sends the refused request again with the same data
and callbacks, so the form finishes as if nothing happened. The check
itself is still the framework's. Plain form posts and JSON clients are
answered as before, and accounts with no local password are offered a
way to set one, as the confirm screen does.
Reported by binghuo. With per-client folders switched on, a client editing
one of their own files could choose "No folder" and the file left their
home for the root of the library — beside the staff folders, where the
administrator's own things are. The feature exists precisely to stop that
mess, and the editor was the one door still open to it.
Uploading resolves an absent folder to the client's home, and so does
creating a folder without naming a parent. The portal's file editor did
not, so the rule held on two paths out of three.
Now it holds on all three: update() resolves a null folder to the home
where the installation gives them one, and the editor stops offering "No
folder" at all in that case — there is no such place for this client — and
preselects their home for a file that has none.
An installation with the setting off is unchanged: no folder still means
no folder, because there every client's file sits at the root.
Reported by @veenone (#1788). Setting "folder inside the bucket" on S3 or
an S3-compatible backend made every page that touches storage answer 500,
including the orphan-files screen. Reproduced against MinIO: the disk
resolved into
Class "League\Flysystem\PathPrefixing\PathPrefixedAdapter" not found
One folder on the screen, two names underneath: Laravel's own drivers
read `root`, and this application's GCS driver reads `prefix` and builds
its adapter with it. Setting both looked like a harmless way to serve
both, and was not — FilesystemManager wraps any disk carrying a non-empty
`prefix` in that adapter, which lives in an optional package nobody
installs. So the key meant for GCS broke S3, and only once somebody set a
folder.
`prefix` now goes to the GCS driver alone, and both keys are written on
every apply rather than only when there is a folder — a process that
switched provider or cleared the field kept a stale one otherwise.
Verified against MinIO with a folder set: the orphans screen loads, and an
upload lands inside the folder rather than at the root of the bucket.
Reported by Ricardo Cazati, who ticked "create an account on first
sign-in" and "approve those accounts automatically", named his domain,
and then found every colleague's account sitting in the approval queue
with nothing anywhere saying why.
The rule is right and stays: an address the provider has not confirmed
goes to the queue whatever the box says, because inside one Entra tenant
a colleague can present somebody else's address. What was missing is that
Microsoft confirms nothing until `xms_edov` is added to the app
registration — an administrator configuring their own company directory
reads "verified" as already true of it.
The tenant-ID field mentioned that claim, but only for the half about
never attaching to an existing account. The other half — that this is
also why auto-approval does nothing — now sits under the auto-approve
checkbox itself, where the promise is made, and only when the three
settings that produce the surprise are all on.
Reported by Ricardo Cazati. A client with every file permission could
mark their own file public — and was then shown nothing. The screen said
"anyone with the link will be able to open and download it" while the
only route that makes a link was staff-only, so the link existed for
nobody. Version 1 could do this.
Making one now asks `upload_public`, the same key that lets them mark the
file public, and `update` on the file, which for a client means one they
uploaded and nothing else. Staff are not asked for the key, as they never
have been: `update` is their boundary and asking now would be a new
refusal on every installation that upgrades.
Revoking deliberately does not ask for the publishing key. It takes
access away, and somebody whose permission to publish was withdrawn must
still be able to undo what they published.
The portal's file editor grows the section the staff screen has, minus
what a client has no business setting: the link, a copy button, and
revoke.
Reported by Ricardo Cazati, who had to turn compulsory two-factor off to
get his colleagues working.
An account provisioned by a provider carries a generated password nobody
has ever seen. The password screen asked for the current one before it
would set a new one, so those accounts could never have a password of
their own — and enrolling in two-factor is behind a password
confirmation, so they could not enrol either. With
`TwoFactorEnforcement` set, the enforcement middleware sent them to
enrol, enrolling sent them to confirm a password they do not have, and
every other screen — including the one that would have given them one —
redirected back. No way in and no way out.
- The password screen asks for the current one only where there is one,
and says "Set a password" otherwise. Setting it moves the account to
`local`, the line NewPasswordController already writes when such an
account resets its password: the hash is now what signs it in, and the
settings screens read that off this column.
- An LDAP account is refused outright rather than handed a password that
signs nothing in — its password lives in the directory.
- The enforcement middleware lets the password screen through, the way it
already lets the confirm-password screen through, so the loop has an
exit.
- The confirm-password screen offers to set one instead of asking for a
password that does not exist.
Reported by @hackchang (GHSA-c6h9-hcm7-j3x9). GHSA-r3hg-3fxw-rcmr scoped
the group controllers; invitations were written afterwards and were not,
so the same reach was open through a different door.
A client-scoped staff member with `create_clients` could read every
group's id and name off the invitation form, name any of them on an
invitation, and have the invited client added to it at redemption — a
group whose files they cannot see and whose members are not theirs. The
ordinary way to do that, adding a member to a group, refuses on
StaffLibraryScope::allowsGroupMembership(); the invitation path never
asked.
Three places, because the hole had three halves:
- the form lists `$this->scope->groups($viewer)`, as GroupsController
already does;
- the request is validated against those groups rather than every group
there is, since a request need not come from the form;
- redemption asks allowsGroupMembership() of the invitation's sender
before writing the membership.
The last one is the one that matters. An invitation is a grant that lands
days later, when the sender is not present to be checked, and the ones
written before today are still outstanding. A refused membership is
dropped and logged rather than failing the redemption: the account is
what the person holding the link came for, and it is theirs either way.
An invitation whose sender has since been deleted keeps its group — there
is no longer a reach to exceed.
An installation that scans can still let files through: too large for the
scanner, an archive it could not open, or an upload that arrived while the
scanner was down. Both policies default to letting those through, and the
count of them is on the settings screen and the dashboard.
Everyone could see that except the one person it matters to. The uploader
sees the state on their own file and staff see it in the library; whoever
follows a public link saw the page a file that passed gets, having neither
chosen the policy nor any way to see the setting. On the hosted free plan,
where every upload is published behind a link, that is the whole audience.
The share page and the public file page in all four themes now carry one
line: "This file was not checked for viruses." Said plainly and without
alarm — nothing is known to be wrong with the file; what is known is that
nothing looked.
Only where this installation scans, and only for the three reasons that
mean a scanner let something past. A file from before scanning was
switched on says nothing: on an installation that has only just switched
it on that is every file, and saying it about all of them says it about
none of them.
Seven features since 2.4.1, so the middle number moves: virus scanning,
client invitations, client account expiry, start pages, per-client folders,
the library filters and the missing-from-storage report. Three fixes to
behaviour that shipped in 2.4.1, plus a dependency advisory.
One line per item, which is the 2.4.1 shape rather than the paragraphs
older entries carry. Virus scanning is six lines rather than one long one --
splitting is how a big feature fits the format.
No "Important -- do these yourself" section, and that was checked rather
than assumed: virus scanning and per-client folders both default to off,
the three new scheduled commands run on their own, and the six migrations
need nothing from anybody. Upgrading leaves nothing working differently
from how you expect.
The date is today's. Whoever cuts the release restamps it if it lands on
another day.
Client file rows in My files now carry expires_at, and all four portal
themes show "Available until <date>" beside the size, date and download
count. Before, the date was only on the file's edit screen, so a file
about to disappear gave no sign of it where the client looks for it.
The portal upload page dispatches ResolvingUploadNotice, handing the
uploader to listeners, and shows any lines they add above the uploader.
The upload page is one page for every theme, so this is the only place a
rule about uploads can be read before one happens; the announcement band
is drawn by one theme's shell and would miss the other three. With
nothing listening it shows nothing. First caller: cloud-modules, telling
free-plan customers how long uploads are kept.
composer require and composer remove both update the rest of the
dependencies, and without --update-no-dev they install ProjectSend's
development and test tools too. The Docker section already said so; the
release-zip install and the removal step did not. The removal step also
now says how to run it on the Docker image.
The dashboard warned that 62 files had been let through unscanned in the
last day. They were 62 activity log entries for 16 files, and none of
those files could be downloaded: 50 entries were for files now missing
from storage, and 12 for files since deleted. The count read the log, so
it counted a file once per attempt and kept counting it after it was
deleted, went missing or was scanned clean.
It now counts files in their current state: not scanned, let through
while the scanner was down or because it could not open them, with that
verdict in the last day. The settings screen already counted this way
without the time limit; the rule is one File scope used by the
dashboard, the settings screen and projectsend:status. The status key
keeps its name and meaning, and is now accurate.
"Detected the test file" left it open what the file was, which reads
like ProjectSend carrying something malicious. The Test button's
description and both of its results now name EICAR and say it is a
harmless file made only for testing, as do DOCKER.md and INSTALL.md.
Run against the dev stack with real ClamAV and queue workers, and a code
review looking for ways around the scanner.
Quarantine now stays quarantined until somebody releases the file. A
rescan only touches files people can download, and changes nothing when
the scanner cannot answer or scanning is off. Before, an old infected file
rescanned while clamd restarted went through the "allow" policy and became
downloadable. The daily missing-files check leaves quarantined files alone,
so a storage outage no longer brings one back as a fresh upload.
A file longer than clamd's StreamMaxLength is "too large" again. clamd
answers and hangs up; the next write raised a warning that became an
exception before the answer was read, so the file was recorded as
"scanner down" and retried past the unscannable policy.
The production compose example gives clamd the settings it needs. On its
own defaults an encrypted zip comes back clean. The Test button now sends a
password-protected zip and fails when it is called clean, and says when an
address answers but is not ClamAV.
Saving the settings restarts the queue workers, which kept the old values
in memory. New scan runs --all, as its name says, and is refused while
scans are queued. A retry scheduled for later no longer counts as a scan
in progress.
Also: quarantine respects client scope for listing, release and
notifications; a zip built before a file was quarantined is refused;
public comments and version links skip unavailable files; a client no
longer sees their own quarantined or missing upload; a file whose bytes
return is scanned at once; clamd listens on IPv6 too, so its container
health check passes.
Virus scanning and the missing-from-storage report account for most of
them; the rest are the library filters and the per-client folder setting.
Every locale was at 0 missing before this and is at 0 missing again.
Notes for whoever reads this next:
- The Slavic locales keep the `Label: :count` shape rather than `:count
plików`, because Polish, Czech and Russian inflect the noun according to
the number in front of it and no single form is right for every value.
That is the existing convention in those files, not a new one.
- "Scanner" is now counted as untranslated in German and Dutch. It is the
correct word in both, the same way API and OK are elsewhere.
- Russian stays on the formal вы/ваш the rest of that catalogue uses --
152 entries to 0 before this, and unchanged after.
Checked in a browser with the locale switched, not only in the file: the
client settings screen reads correctly in Spanish and the long
folder-setting description fits its column. A catalogue that parses is not
evidence that a sentence reads well.
A client who may create folders creates them at the top of the library,
beside the ones staff made, and their uploads land at the root too. An
administrator opening /files gets one flat pile with nothing saying which
parts belong to whom.
With the new "Give each client a folder of their own" setting, every new
client gets a folder named after them and it acts as their root: what they
upload and any folder they create goes inside it. /files becomes a list of
clients rather than a pile.
The sentence this feature has to keep true: **the home is a default
location, not a boundary.** Folder::scopeVisibleToClient is untouched, so a
folder staff shared with a client still reaches them and sits beside their
own. Making the home a jail would have silently revoked every share that
already exists -- a data-access change wearing the clothes of a tidying-up
feature. There is a test named after that rule.
What the client sees is the *inside* of their folder, not a folder wearing
their own name, which is not information to them. The breadcrumb is trimmed
of it for the same reason: "Invoices", not "Acme Ltd / Invoices".
Some decisions worth naming:
- **A column, not a convention.** `folders.home_for_user_id`, unique.
Matching on the name breaks the moment two clients share one, and
`created_by` plus a null parent catches every root folder a client ever
made themselves. The question is asked on each upload and each portal
listing and the answer has to be exact.
- **created_by is the client**, because that is how scopeVisibleToClient
already grants somebody their own folder -- no assignment row to keep in
step with it. That is also why this writes the row rather than calling
FolderService::create(), which takes created_by from auth()->id().
- **On model events**, not in the services that make and rename clients.
There are nine of those (ClientAccounts, ClientProvisioning, the profile
screen, two update endpoints, AccountConversion, invitations, LDAP,
social) and a rule repeated in nine places is missing from the tenth.
- **Turning the setting on creates nothing.** Existing clients get a folder
when an administrator presses a button that says how many are waiting,
and it reports created/total/already-had afterwards. Somebody should be
able to switch this on, look, and switch it off without having
reorganised a library. It moves no files either.
- **Nobody deletes a home from a folder screen**, staff included, and the
client cannot rename theirs -- they own it, so ownership alone would have
let them, and its name follows the account anyway.
- **The name always follows the client**, over a hand-typed one. A folder
still called "Acme Ltd" under an account now called something else
misleads the administrator the feature exists for.
Verified in a real browser as well as in tests: the screen mounts, the
panel reads "24 of your existing clients have no folder yet", and pressing
the button answers "24 of 24 clients got a folder. 0 already had one."
A quarantined file was listed in the library with every button a working
file has, and Download answered with an error page. Three changes, all
the same idea: do not offer what cannot be done.
The library no longer lists a file that is quarantined or missing from
storage. Those two live on the screens that exist to act on them —
Quarantine, and Files missing from storage — and both now link each row
to the file itself, which is where somebody deciding needs to look.
That page says why, at the top, in the colour the state deserves: red
for a threat, amber for bytes that are gone. And it stops offering the
download and the preview, because a button that answers 423 is not an
affordance.
A file still being checked stays in the library. It is about to be
usable, and its uploader should be able to see where it went.
The button was disabled on a library that had already been scanned
once, which is most of the time and exactly when somebody would press
it — after updating definitions, say. It now re-checks everything
rather than only what was never looked at, which is what its name says.
A rescan keeps each file available until its new verdict arrives, so a
full pass takes nothing offline. The two states it skips are a file
already waiting for its first verdict and one whose bytes are gone.
It is disabled for two honest reasons now — scanning is off, or a scan
is already running — and says which.
Results are green, amber and red: checked and fine, checked and could
not be read, checked and something was found. The badge gained a
warning variant to say the middle one, matching the amber the warning
alert already uses; before this a missing file wore the same red as a
virus.
A file found missing is also stamped with the time it was checked, so
it appears in the Activity list. It is a verdict like any other, and
without the stamp it was decided somewhere nobody could see.
tcp://clamav:3310djlkasjdlk connected happily. PHP reads a port the way
atoi does — the digits at the front, the rest ignored — so an address
with a typo on the end was saved, tested, and reported as working, while
tcp://clamav:33101 went somewhere else and failed. The feedback an
operator got had nothing to do with the mistake they made.
ScannerAddress says what an address is: tcp:// with a host and a port of
1 to 65535 and nothing after it, or unix:// with an absolute path. It is
asked in all three places an address arrives — saving, testing, and
connecting. The third matters because a managed address comes from the
environment and never passes the screen.
The answer names the problem rather than reporting "no answer", which
would be true of any unreachable scanner and would send somebody to look
at their network for a typo.
Checked in a browser with both addresses from the report: each is now
refused on Test and on Save, with the same sentence, and
tcp://clamav:3310 still comes back "Working. ClamAV 1.5.4 detected the
test file".
The Test button asked the scanner on file, which makes it useless at
the moment it is most needed: the first attempt, before anything has
been saved. It now tries what is typed, falling back to the stored
address when the field is empty so the button still answers on a screen
nobody has touched. Nothing is written either way — testing is not
saving.
The address travels as a request field and is applied to the request's
own ScanningConfig, which is scoped so the screen and the scanner it
resolves share one. A preview address beats even a managed one, and is
set in exactly one place.
Quarantine now carries a count in the sidebar, like Comments — in amber
rather than the usual colour, because the others count work waiting and
this one counts something that went wrong. Shown only to whoever holds
the permission to act on it.
Both checked in a browser: an address typed and not saved came back
"No answer from tcp://escrito-a-mano.invalid:3310", the stored one
untouched, and the badge renders amber with the real count.
It sat above the form, which read as a box about the screen rather than
about the field. It belongs under "Scanner address" and above Save,
where it is plainly the answer to "is this address right?".
The box is now shown only where there is something to test — an
installation that connects its own scanner. On a hosted one the endpoint
answers 403, and a button that leads to a refusal is worse than no
button. That is a separate prop from `managed`, which covers two
different reasons the address is not editable: a scanner named in the
environment still has a connection worth testing.
"New scan" sits beside Quarantine as the screen's one action, in the
primary colour, on every tab. The "Files already here" box it replaces
is gone: it held an action under a Save button, a counter that the
Activity tab now shows live, and a field that belongs with the other
settings, which is where it is now.
The button says why when it cannot be pressed — scanning is off, every
file has already been checked, or a scan is already running — rather
than sitting grey with no explanation. It refuses a second scan while
one is working through the queue, which is a thing somebody would
otherwise do by clicking twice.
Starting one lands on the Activity tab. A button whose screen looks
unchanged afterwards reads as a button that did nothing, and this one
has somewhere worth looking.
Checked against a real ClamAV: 42 files queued, 31 came back clean, the
rest were the ones whose bytes are missing. The button then disabled
itself, because there was nothing left to scan.
A row whose bytes are gone was recorded as "the scanner could not be
reached". Wrong on screen, and wrong underneath: that is the one reason
the hourly sweep re-queues, so every orphaned row would have been
rescanned hourly forever.
It is its own state now, `missing`, and withheld rather than offered:
a client who sees a file listed and gets an error on the download is
worse off than one who never saw it. Staff still see it, marked, which
is the point — somebody has to decide what to do about it. The refusal
says what it is ("no longer on the server") instead of sending somebody
looking for a permission that would let them through.
A daily `projectsend:check-missing-files` finds them, whether or not
this installation scans for viruses: it is not a virus question, and an
installation with no scanner has exactly the same problem. It compares
one disk listing against the rows rather than asking "does this exist?"
per file, which on object storage would be a request per file per day.
Files that come back — a remount, a restored backup — are picked up on
the next run and re-checked rather than left for dead.
They are listed beside the orphans, which is the same fault seen from
the other end: bytes with no row, rows with no bytes. The tab carries
the count, each row says where the file should be, and removing one
takes the record with it through the deletion that already exists.
The dashboard says how many there are, and so does
`projectsend:status`, because a fleet-wide jump in this is a storage
fault nothing else in that document would show.
A backfill runs for minutes or hours inside a queue worker, where none
of it is visible. The third tab polls every four seconds and says what
is happening: whether anything is running, how many uploads are held,
how deep the queue is, how many files were checked in the last hour,
and the last twenty verdicts with what each one was. When nothing is
running, that same list is the record of the last run, which is what
somebody opening the tab after the fact came for.
Two things the live screen found that the tests had not:
**A backfill read as "nothing is being scanned."** Re-scanning a file
that already went out unchecked deliberately leaves it available, so it
is never "pending" — and the screen counted only pending files. It
counts the scans queue too, and the two are shown separately, because
"an upload nobody can download yet" and "work the scanner has not
reached" are different facts.
**A file whose bytes are missing was recorded as "the scanner could not
be reached."** Wrong on screen, and worse than wrong in behaviour: that
is the one reason the hourly sweep re-queues, so every orphaned row
would have been rescanned every hour forever. It has its own reason
now, and goes through the same policy as a file the scanner could not
open.
Both tabs also gained the header shortcut to Quarantine, and Quarantine
one back to the settings, each shown only to somebody the destination
will actually let in.
"Scan existing files" sat disabled on an installation with a library of
144 of them, and the hourly backfill would have found none either. Both
looked for `scan_note = 'before_scanning'`, and no file on any upgraded
installation carries it: the migration gives `scan_status` its default
and writes no note, and the v1 import inserts rows the same way. The
feature was inert on exactly the libraries it exists for.
A file with no reason beside its "not scanned" is now what it plainly
is — one nothing has ever looked at — through File::neverScanned(),
which the backfill, the counts and the badge all ask. Reported as
"Uploaded before virus scanning was switched on" rather than as a bare
"Not scanned", which is the one badge somebody would have had to come
and ask about.
Found on the real screen, not by a test. The tests now cover the shape
an upgrade actually produces.
"Uploads checked by: ClamAV 1.5.4" now sits beside "Downloads sent by"
and "Files stored on", and is always there. Same reasoning those two
already carry: being able to confirm at a glance that uploads are
checked is worth as much as being told when they are not.
Four states in one row. A working scanner is named. One that is not
answering says so. One letting files through is amber. No scanner at
all reads "Nothing", amber, and links to the screen that sets it up —
which is where the "turn it on" link now lives, so the big alert above
is left to the cases where a configured scanner is misbehaving. Absent
entirely where the scanner is not this installation's to connect.
Also fixes a line the dashboard itself exposed: the activity log read
'The file "" was quarantined'. The scan job has no actor and attaches no
subject, so those two templates have to take the name from their
context, not from :subject. There is a test now, which there was not
before, because a real screen caught it and a green suite did not.
The dashboard's System card now says so when no scanner is configured
at all, not only when a configured one is failing: "Anything uploaded
here — by staff, by clients, or through an upload link — is passed on
unchecked", with a link to set it up.
Said only where somebody can act on it. Connecting a scanner is a new
capability, scanning.connect, community only — on a hosted installation
the scanner is infrastructure the platform runs, so its address is not a
tenant's to set and its absence is not a tenant's to fix. The two
policies stay on both editions, because what to do with a file nobody
could scan is a decision about somebody's own files. An edition
difference through the registry, never an edition check.
Also: PROJECTSEND_SCANNER_DEFAULT_ADDRESS, seeded into the settings on
first boot by the command that already does this for two-factor
enforcement. It is the opposite of PROJECTSEND_SCANNER_ADDRESS — a
starting value rather than a policy, so a Docker install that brings up
the optional scanner container arrives configured while the address and
the switch stay on the settings screen. Both are seeded together or
neither: an address with scanning off would look configured and check
nothing.
Nothing changes for an existing installation on upgrade: scanning stays
off, existing files are marked "never scanned", and the scanner
container is still opt-in.
The Test button did nothing visible. The page read `scanner_test_result`
off the shared props, and HandleInertiaRequests shares `success` and
`error` and nothing else — so the answer was set on the session and
never arrived. It is read in the controller and handed over as a prop
now, the way the CAPTCHA screen does it.
The screen is two tabs, Scanner and Options, following the scheduler's
`?tab=` links. Scanner holds the connection and the Test button; Options
holds the policies and the backfill. Both end with their Save, and
nothing sits below it — before this, "Files already here" and its button
were stranded under the Save button of a form they had nothing to do
with.
Verified by clicking the real button in a browser against a real ClamAV:
"Working. ClamAV 1.5.4 detected the test file as Eicar-Test-Signature."
`scan_engine` was always null: the column existed, the client never
filled it. Found by running a real ClamAV against a real upload rather
than by a test, since the fake scanner reports whatever it is told.
Asked once per scanner instance — so once per queue job, and the worker
is recycled hourly — rather than on every scan, which would double the
connections to answer a question that changes daily.
Docker gets the profile command, the memory it needs, and why the first
start is slow. A manual install gets the packages, the three clamd
settings without which an unopenable file comes back clean, and where
the socket usually lives. Both point at the Test button, because
"connected" and "detecting" are different answers.
Staff keep seeing every file they always saw — withholding is about
recipients, not about the library — so the library now carries the state
on the row: Checking, Quarantined, Released, or Not scanned with the
reason behind it. Nothing at all for a clean file, which is the common
case.
The API says the same in a `scan` object on every file, with an
`available` flag so a caller need not learn which of six states mean
"you can have it", and `scan_status` is a filter, so an integration can
wait for the file it just uploaded or collect what is in quarantine.
The download endpoint answers 423 for a file that is not available,
which it already did through the shared controller.
Re-exported the OpenAPI document.
Sharing a file that is still being checked writes the assignment and
says nothing. The announcement goes out when the file becomes
available — a clean scan, a file let through while the scanner was
down, or an administrator releasing it from quarantine — so nobody is
ever sent to a page that refuses them, and a file about to be
quarantined is not announced to everyone before anybody knows.
Recipients are derived from the assignments as they stand at that
moment, not remembered from the moment of sharing: a share taken back
in the meantime produces no email, and one added does. New-version
notices ride the same path, which they had to anyway — the audience
rule re-checks visibility, and a file being scanned is not visible.
Two bugs found while writing the tests, both in the hourly command:
Re-queuing a file marked it pending first. Pending means withheld, so
running --existing over a library that predates scanning would have
hidden every file in it from every client for as long as the backfill
ran, and then announced each one to its recipients a second time when
it came back. The job now knows which state it expects instead, and a
rescan leaves the file downloadable until a verdict actually arrives.
The defaults let files through when the scanner cannot answer, so an
installation whose scanner died looks, from every screen anybody uses,
exactly like one that is working. Three places now say otherwise.
`projectsend:status` gains a `scanning` block: whether it is on, whether
it is managed, whether the scanner answers right now, the engine and
how old its definitions are, what is waiting, what is quarantined, and
how many files went out unscanned in the last 24 hours. Absent, null and
zero stay distinct — `reachable: null` means there is nothing to reach,
`false` means it should be answering and is not. The scans queue is
reported beside the other two.
The dashboard's System card carries the same warning for whoever is
actually looking at a screen, and says nothing at all while scanning is
healthy or switched off.
Docker gets the scanner as an opt-in profile — `--profile scanner` — in
both the development compose file and the published example, with a
clamd.conf whose Alert* options are what make an encrypted archive come
back as "could not scan" instead of "OK". No published ports: clamd has
no authentication and the file crosses that socket in the clear. Both
images also run a worker for the scans queue.
The dashboard test caught a 500 before it shipped: a nullable return
written as `array`.
Settings → Virus scanning: switch it on, point it at a ClamAV daemon,
choose the two policies, and see how many files are waiting, in
quarantine, or were let through unscanned. Switching it on with no
address is refused rather than saved and left inert.
The Test button is three answers, not one. Unreachable is obvious.
Reachable but detecting nothing is the failure that looks like success
— empty or broken virus definitions — so the test sends the EICAR
string and reports "it found the test file", never "it did not
complain". The string is assembled at runtime so no checkout contains
it: antivirus software on a developer's machine quarantines files that
do.
"Scan existing files" queues the library that predates scanning,
through the hourly command so no request is held open, paced by a
setting so it does not starve today's uploads.
Where the environment names a scanner, the connection and the on/off
switch leave the screen and scanning cannot be turned off — the same
managed shape the CAPTCHA screen has. The two policies stay editable,
because what to do with a file nobody could scan is a decision about
somebody's own files.
An infected file now goes somewhere rather than nowhere. Staff holding
the new release_quarantined_files permission get a Quarantine screen
listing what was refused, who uploaded it, and what the scanner called
it. They can delete it as they always could, or release it — which
needs a written reason, a password confirmation on top of the
permission, and lands in the activity log under their name.
Only the administrator role holds that permission by default. Deciding
a threat report is wrong is a different judgement from deciding a file
is no longer needed, which is why it is not delete_files.
Two notifications, two audiences: staff who can act on it, and the
person who uploaded it — for whom this is how they learn their own
machine has something on it. The people the file was shared with are
deliberately not told about a file they never received.
`projectsend:scan-files` runs hourly: it re-queues files still waiting,
and re-scans the ones that went out unscanned while the scanner was
unreachable, since it may be back. With --existing it also works
through a library uploaded before scanning was switched on, paced by a
setting so it does not starve today's uploads.
A file that was downloadable before it was caught says so on the
screen, with its download count, because that is the case where
somebody may already have a copy.
Every upload now starts as "being checked" and is not served to anyone
until a scanner has looked at it. Infected files are quarantined: kept
on disk, unreachable, waiting for an administrator.
The scanner is ClamAV, reached over a socket, streaming the file
wherever it is stored — no temporary copy for an S3 or GCS disk. What
the scanner answers is a fact; what it means for the file is this
installation's setting, so ClamAvScanner knows nothing about settings
and ScanPolicy knows nothing about sockets. Three of clamd's own alert
options are what make a file it could not open come back as an answer
rather than as "OK"; the client maps those to "too large" and
"encrypted" instead of to a threat.
Both policies default to letting files through, marked "not scanned",
which is the product owner's decision: a scanner that cannot answer must
not stop people working. Every such file is logged, and the screens that
say so come with the rest of this work.
Withholding is two rules. A file that is not available drops out of the
scopes that answer "what may this person see" — recipients and the
public listings, never the uploader's own copy. And every route that
puts bytes on the wire asks FileAvailability first: download, thumbnail,
preview, share link, the four public routes and both ends of a zip
build. A share link minted before the scan finishes says the file is
still being checked rather than 404ing.
Not yet here, and coming next: the quarantine screen and its permission,
the notifications, the settings screen, the hourly retry, the backfill
for existing libraries, and the Docker service.
The staff library grew filters for uploader, role, public/private, download
count and version. Two of those already existed on /api/v1/files
(`uploaded_by`, `public`); the other four did not, so an integration could
not ask what the screen asks.
Adds `role_id`, `downloads=none|any`, `version=current|outdated` and
`visibility=public|private`.
`visibility` rather than changing `public`, and that is the decision worth
explaining. `public` has always tested the file's own column, and callers
depend on that answer; changing what an existing filter means is breaking
for everyone already sending it, however much better the new meaning is. So
`public` is untouched and `visibility` is added beside it with the
application's own definition -- File::isEffectivelyPublic(), the flag or a
public folder anywhere above the file -- which is what the badge on a staff
row means. The guide says in a sentence which to reach for. Point the
visibility filter at the column instead and the test that separates them
fails, which is the whole point of having both.
That predicate now lives once, as File::scopeEffectivelyPublic(), beside the
isEffectivelyPublic() it has to agree with. It was a private helper on
FoldersController until a second surface wanted it.
`role_id` deliberately carries no identity guard, unlike `uploaded_by` beside
it. A role names nobody: the files in the result are ones the caller may
already read, and learning one came from somebody holding the Client role
narrows to a set they could have guessed. `uploaded_by` is different in kind
-- a non-empty answer confirms exactly the identity the response is
redacting -- which is why only it is guarded. The reasoning is in the code,
because an absent guard sitting next to a present one is the kind of thing a
reader should not have to re-derive.
Tests cover each filter, the public/visibility split, and the client-scoped
negative: every new filter still returns nothing outside the token's own
library, because a filter narrows a library and never widens one.
Search and the category dropdown were the whole filter bar, which is thin
for a library of any size. It now also narrows by:
- who uploaded the file, and separately by the role they hold
- public or private
- never downloaded, or downloaded at least once
- current version, or outdated
Each one forces the same flat, whole-library view search already used, and
they combine.
Two decisions worth naming.
"Public" means what the badge on the row means -- File::isEffectivelyPublic(),
the file's own flag or a public folder anywhere above it. Filtering on the
`public` column alone would have hidden files this very screen labels
Public, which is a filter arguing with the list it filters. The private half
needs its own null branch, because `folder_id NOT IN (...)` is never true for
a NULL folder_id: without it a file at the library root belonged to neither
half and vanished from both. Removing that branch turns the private filter
from one row to zero, which is the test.
The uploader filter carries the same guard /api/v1/files puts on
`uploaded_by`. fileRow() already withholds an uploader's name from a viewer
who may not identify them, so answering this filter plainly would have handed
the same identity straight back as a row count. An id the caller may not
identify now matches nothing, which is indistinguishable from someone who
uploaded nothing, and the dropdown is built through filterClientPairs so it
never offers the name either. Without the guard the scoped-staff test gets
its stranger's file back.
"Outdated" rather than "superseded" throughout, because that is the word the
version badge already uses and the two should not disagree. A file nothing
has replaced counts as current, including one never versioned at all.
The ids are cast out of the validated input: `integer` validates "5" without
converting it, and permitsClientId() takes a strict ?int.
js-yaml 4.3.1 had a high-severity advisory: its limit on YAML merge
keys did not bound CPU use when the merge sources were empty. It is
only here as a dependency of ESLint's config loader, so it was never
part of a build or a release. ESLint's range (^4.3.0) already allowed
the patched version, so only the lockfile changes.
"Predeterminado (Panel de control)" was cut off in the old fixed width.
The picker now takes the column's width, up to a readable maximum, and
still fits a phone screen.
Sixteen locales, fourteen strings each: the expiry field and its two
hints, the expired badge text, the sign-in refusal, the scheduler label,
two activity-log entries, and the start page picker with its three hints
and the role permission error.
Wording follows each catalogue rather than being chosen fresh: every
locale already had its own words for expired, deactivated, sign in and
default, and these reuse them. Formality follows each file too. Every
:placeholder was checked to survive in every locale.
Laravel's `date` rule accepts a JSON number when it reads as a real day
(20301231 passes) and hands it on unconverted. Every file expiry field
then passes it to a method that only takes a string, so the request
failed with a 500 instead of a validation error.
Affected: PATCH /api/v1/files/{file}, the staff file editor, the bulk
editor, new share links and the client file editor. Each now also
requires `string`, so a number is a 422 on `expires_at`. Dates sent as
text behave exactly as before. A form never sent a number, so this was
only reachable with a hand-written JSON body.
A role now has a start page: the dashboard, files, upload, groups,
clients or the activity log (the last two for staff only). Anyone can
override their role's choice in their profile. The administrator role
takes a start page too, while everything else about it stays locked.
A choice is only used if the account can open that page now. Otherwise
the next one down is tried, ending at the dashboard, so a permission
removed later never lands somebody on a 403. A role cannot be saved
with a start page its own permissions block. A link followed before
signing in still wins, and a waiting getting-started or what's-new page
still goes first.
Applies to password, two-factor and provider sign-ins, and to the site
root for someone already signed in. StartPageTest opens every page for
real, with and without its permission, so the enum cannot drift from
the routes.
Requested by @Zodiac1978 in #1777.
Staff can give a client an expiry date on the create and edit screens,
and through /api/v1/clients. When the date passes, the client is refused
at sign-in and on their next request, and their API access ends too.
Files and history stay, and a later date (or none) brings them back.
Access is checked through one predicate, User::maySignIn(), at every
door: sign-in, the web session, API tokens and the two-factor
challenge. An hourly sweep also switches `active` off, so the list,
its filter and seat counts agree. The sweep is not what enforces it,
so a scheduler that is not running cannot keep an account open.
An account cannot be active with a date that has passed. Reactivating
an expired client needs a new date in the same save.
The day-means-end-of-day-where-you-are rule moved out of FileExpiry
into a shared DateInput, so file and account expiry read dates the
same way.
Requested by @Drardollan in #1310.
Filling the "Storage quota (MB)" field on the new-client form raised a
TypeError and the request died with a 500. Leaving it blank worked, which
is why it reached a release: that path goes through `null ?? 0`, and the
0 is an int.
The `integer` validation rule checks that a value looks like an integer.
It does not convert it. `$request->validate()` returns the raw input, so
the form field arrives as the string "2048" -- and the create form types
that field as a string in React, so it is a string even over JSON. Both
controllers declare strict_types, so handing it to
`ClientAccounts::create()`'s `int $storageQuotaMb` is a TypeError.
Fixed on both surfaces that call create(): the staff screen and
/api/v1/clients. The API twin had the same defect, reachable by sending
the quota as a quoted JSON value or a form-encoded body -- its own create
test only ever sent a JSON number.
Two more call sites had the same shape and are cast too, though nothing
sends them a string today: the share-link download cap and a comment's
reply_to. Both are safe only because a frontend file happens to call
Number() first, which is a fact about that file rather than anything the
signature guarantees. The null in each is preserved rather than collapsed
to 0 -- "no cap" is not a cap of zero.
`storage_quota_mb` is also cast on User and Invitation. The column is an
unsignedInteger and both docblocks already promise int; it is read
straight into provision()'s typed parameter when an invitation is
redeemed, and which type a driver hands back is not something that call
site should depend on.
Found on the new files-test rehearsal instance, on its first real use,
against the same build the whole fleet is running.
Invitations produced no in-app notification at all, and neither did
self-registration: the whole Clients module raised none. The only
admin-facing signal when an account appeared was an email to whatever raw
addresses an operator typed into a setting -- addresses that need not
correspond to any account in this installation, and that plenty of
installations never fill in. An invitation could be accepted and nobody
signed in would ever be told.
So: one new type, client_registered, reaching the bell and /notifications.
One type for both doors on purpose. A client arriving through the public
form and one arriving through an invitation are the same event to the
person being told -- an account now exists that did not -- and a second
type would buy nothing, because preferences here govern email only, so it
could not have been switched off separately anyway. Which door it came
through is one click away in the activity log and on the invitations
screen.
In-app only, the reasoning client_uploaded already states: email for this
event is sent separately to that address list, and routing it through
Notifier's mail dispatch too would risk double-emailing any staff member
who is also on it.
Two things worth stating about who gets it. Recipients are resolved at the
call site, because Notifier authorizes nothing by design -- its security
contract is explicit that a broad query must never be handed to it. And a
client-scoped staff member is deliberately not told: their whole view is
the clients assigned to them, and a brand-new account is assigned to
nobody, so it would link them to a screen they are refused.
Which is also why the notification links to the clients list filtered to
the address, and not to clients.edit: that route is gated by edit_clients
while these recipients are chosen by manage_clients. A notification that
refuses the person it was sent to is worse than one that lands a click
short.
Translated in all sixteen locales, and the redemption was driven through a
real browser to see the row arrive.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
The "Invite client" button led to a history, which is not what it says. The
tabs were a way of housing two things that had nowhere else to live, and
now they do: Invitations is a sidebar entry between Custom fields and
Groups, and the button goes to the form.
That is also the shape every other list in this application already has --
Clients, Groups, Categories, Roles all sit in the sidebar with a "New X"
button leading to their own create screen -- so the tabs were the odd one
out rather than the pattern. Two URLs, each meaning one thing:
/clients/invitations is the history, /clients/invitations/create is the
form. Sending now returns to the history, where the invitation just sent
is the first row.
No badge on the sidebar entry, deliberately, unlike the two queues below
it. Account requests and Membership requests count things waiting on
somebody here; an outstanding invitation is waiting on the person who was
invited. A number there would say "you have three things to do" about
three things nobody in this installation can act on.
Translations move with it: "History (:count pending)" was the tab label and
is gone from all sixteen, and "Invite a client to share files with" comes
back -- it was the form's description before the tabs took the heading, and
had never been translated because it left the code in the same commit that
would have reported it missing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
Sixteen locales, forty-one strings each: the invite screen and its
history, the redemption and expired-link pages, the invitation email, the
expiry setting, and the four new activity-log entries.
Two English strings were fixed before translating rather than after.
"Waiting" was a second word for what this application has always called
Pending -- account requests and membership requests both use it -- and a
synonym is much harder to take back once it exists in sixteen files. And
the submit button said "Send invitation" while the tab beside it said
"Send an invitation", which is a distinction a translator has to stop and
puzzle over to discover there isn't one. Both now reuse what was already
there, which is also why this pass is forty-one strings rather than
forty-three.
Terminology was read off each catalogue rather than chosen. Every locale
already had its own words for client, group, expired, revoke and status,
and these strings reuse them exactly, so a badge in the new table reads in
the same vocabulary as the filter above it. Formality follows each file
too -- informal in ca, es, it, nl, pl and zh_CN, formal in cs, de, fr, id,
pt_BR, ru and tr -- which for Polish meant following the catalogue rather
than the note in the skill, since its existing strings say "Cześć" and
"Twoje konto".
The numeral trap caught one for real, and only because the screen was
looked at: "Historial (1 pendientes)". Spanish, Catalan and Portuguese
inflect that adjective with the number, so those three now use the
"label: :count" shape the Slavic locales already use, and Swahili follows
for the same reason. French, German, Dutch, Italian, Turkish, Indonesian
and Vietnamese keep the natural word order because their word does not
move.
Verified by rendering the screen in Spanish, German, Japanese and Polish
against real rows in all five states -- nothing overflows, and the German
column headers wrap rather than collide. Enabling every locale to do that
meant changing a live setting: it was ["en","es"] and is ["en","es"] again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
The tab listed only what was still live, which cannot answer the question
somebody actually arrives with: did we ever invite this person, and what
happened? An invitation that was accepted, revoked or replaced by a newer
one simply vanished from the screen, and the activity log was the only
place left to look.
So the tab is a history now -- every invitation ever sent, newest first,
each carrying the state it ended in -- with a status filter for reading one
slice of it. And it opens first, because arriving here the question is
usually about what has already been sent, including to the person you were
about to invite again. ?tab=send still goes straight to the form.
Five states, and two of them needed deciding:
- "Expired" is not a stored status and deliberately is not one: nothing
writes it, a row becomes expired by the clock passing rather than by
anybody acting, and storing it would need a scheduled task to stay true.
Invitation::state() derives it, once, and both the badge and the filter
read that -- two copies of the rule is how they start disagreeing about a
row whose expiry passed a second ago.
- "Replaced" is what superseded says to somebody who is not reading the
source. It is a different fact from expired, and worth telling apart: one
ran out, the other was retired by a newer invitation to the same address.
The count in the tab label stays a count of live invitations rather than of
the rows below. The history is mostly settled, and the number worth
carrying in a label is the one that says whether anybody is still waiting --
which is also why it is counted over the table rather than the filtered
page, so narrowing the list cannot change it.
Revoke appears only on a row that still has something to revoke, and the
expiry column is blank on a settled one: the date is still stored and still
true, and printing it invites somebody to wonder what expires about an
invitation that was accepted.
The screen is called Invitations now, in the heading and the breadcrumb.
With the history first it is no longer a form with a list under it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
They were stacked: the form, then the list under it. Two different jobs on
one scroll, and the list -- the half somebody opens this screen to act on
rather than to fill in -- sat below the fold on any installation with a few
invitations out.
Two tabs now, the same plain border-bottom nav the staff account screen and
the theming settings already use. The count rides in the tab label, because
the reason to open that half is that something is waiting in it.
Three details worth stating:
- The form is hidden rather than unmounted, exactly as the staff account
form is, so switching to the list and back does not throw away a
half-typed invitation.
- Revoking passes preserveState, so the page comes back on the tab the
person was working in. Acting on a row and landing on the other half
reads as having lost the list.
- ?tab=pending opens on the list, so something elsewhere can link at the
half it means rather than at the screen plus a sentence telling the
reader which tab to find.
Verified in a browser, since none of this is visible to the suite: both
tabs render, the form is present on one and absent on the other, the query
parameter opens the right one, and revoking leaves the page on the pending
tab with the count down by one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
The expired page offers a "send me a new one" button that re-issues the
invitation with no staff member involved. On its own that is reasonable --
the new link goes to the address on the invitation, never to whoever
clicked, so holding a leaked URL gets nobody a working one, and it is the
same shape as a password reset.
What it spent was the operator's expiry window. A link could be renewed
from a dead link, indefinitely, so a window set to 72 hours was only ever
as short as the longest anybody bothered to wait. That matters in the case
expiry is actually for: a link sitting somewhere it should not be -- a
forwarded thread, a shared inbox, a mailbox that changed hands.
So the chain gets a limit: three renewals, then a staff member has to send
a new invitation. The count is carried forward on each renewal rather than
stored per row, which is what makes it apply to the chain; a staff-sent
invitation starts at zero, because sending one is somebody deciding to.
A renewal beyond the limit answers in exactly the same words as a spent,
unknown or revoked token, and sends nothing. Four situations, one sentence:
telling them apart is how this door would become a way to learn which
addresses an installation has invited.
Renewals are now logged, which they were not -- sending and redeeming
already were, and renewing was the one step that moved an invitation along
with nobody behind it and left no trace. The limit bounds how many rows an
anonymous door can write.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
An invitation could be sent and never taken back. There was no list of
outstanding ones and no revoke, so the only way to withdraw a link sent to
the wrong address was to let it expire -- and the expired page's own "send
me a new one" button undoes exactly that, silently, for anybody still
holding the link. The one cancel the feature had could be reversed by the
person it was aimed at.
So: a new STATUS_REVOKED, outside the pending() scope that both the
redemption and the resend doors look through. A revoked link is dead to all
three things a live one can do -- opening the form, redeeming it, and
asking for a replacement -- and nothing but sending a fresh invitation
brings it back.
The list sits under the invite form rather than on a screen of its own,
because the person who wants to cancel an invitation is the person who just
sent one. It shows outstanding invitations only: pending, expired ones
included. An expired invitation is not inert until it is revoked, so hiding
it would hide the rows most worth a decision -- which is why they sort to
the top, soonest expiry first.
Revoking is gated by create_clients, the same authority as sending:
whoever may invite somebody may take it back. It is logged, like sending
and redeeming already were. The row is kept rather than deleted, for the
reason a superseded one is kept -- the activity log names who invited this
address and when, and that trail should still lead somewhere.
Verified in a browser, not only in tests: the screen mounts, both rows
render, and the expired one carries its badge.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
Two halves of the same gap. Redemption always provisions with
autoApprove: true, so it always meets SeatAllowance::guardClient(), which
refuses on the `email` field -- and the redemption form's email input is
read-only, never submitted, and had nowhere to render an error. The account
was correctly not created and the person was told nothing at all: the form
simply came back. The field now renders errors.email, which is where every
other account form's refusal already lands.
The other half is the button. "New client" has been seat-limited since the
limit existed -- it goes dead with the reason beside it, rather than
offering a form that cannot be submitted. "Invite client" sat next to it,
live, on a full installation. Worse than the original complaint, because
the refusal is met by the invited person rather than by the staff member
who caused it.
So the invite button is seat-limited too, and sending guards as well as
redeeming. An outstanding invitation is still not a client and is still not
counted as one -- the rule a pending account request follows, for the
reason SeatAllowance spells out -- so this reserves nothing. It refuses to
send a link a full installation could not honour, and redemption keeps its
own guard, because the seat can be taken by somebody else in the days
between.
SeatLimitedAction's usage caption is now optional, and the invite button
omits it. Two buttons governed by one limit, each captioned with the same
sentence, reads as two limits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
An invitation stays live for days -- 72 hours by default -- and the address
it names can be claimed in that window: staff got impatient and created the
account by hand, or the person used the public registration form instead.
Redemption never asked, so User::create() met the unique index on
users.email and raised a QueryException. A 500, on the screen of somebody
who had just chosen a password, having done nothing wrong.
ClientProvisioning::addressIsFree() exists for exactly this, and its
docblock says who must call it: the paths with no form to validate. LDAP
asks. Redemption is the third such path and did not.
It now refuses with a message that says what happened, rather than the
generic "this invitation is no longer valid" the expired case uses. There
is nothing to withhold here -- whoever holds the link already knows the
address, because it is the one the invitation was sent to -- and being told
to sign in instead is the only useful thing to say.
The invitation stays pending rather than being retired. It is the account
that resolved the situation, not the link, and a retired row would only
make the second attempt read as expired.
The address rule spans soft-deleted accounts, the same as it does
everywhere else, so a deleted account still holds its address until erasure
takes the row away. Both cases are tested.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
Staff can now invite a specific address to register instead of typing a
password for somebody and finding a way to get it to them. The invited
person sets their own, the link is locked to the address it was sent to,
and an invitation always activates the account regardless of the
auto-approve setting -- naming an address is already the decision the
approval queue exists to make for one nobody named.
Two fixes ride along: outgoing mail now reads the installation's own site
name in its title, header and signature rather than the one baked into
config('app.name') at install time, and the CSRF cookie name is read per
request rather than captured once at load.
Follow-up work, tracked separately: an invitation cannot be cancelled --
there is no pending-invitations screen and no revoke, so letting one expire
is the only way to take it back, which the self-service resend button then
undoes. Redemption also needs the address-availability check every other
non-form caller of ClientProvisioning makes.
Thanks @mash2k3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
Three entries were absent. A sweep of every commit since v2.4.0 for code
changes with no changelog line returned thirteen; ten were correctly absent
— the announcement work is a seam with no content on a self-hosted install,
the client share link is always null unless a platform module mints one, the
quota floor is a platform environment variable, and the email_verified_at
change is inert while MustVerifyEmail is off. The other three were real:
- the IAM-role feature, a visible control on the storage settings screen
that every self-hosted administrator can reach, with no entry at all;
- #1770, a Docker upgrade that fails outright when external storage is
already configured, which is exactly what a changelog is for;
- download counts on a client's own files, which OwnFileDownloads gates
on nothing, so it is live everywhere.
Every entry is now one line. The explanatory paragraph, the "who this
affected" note and the upgrade advice are gone from the change list; what an
operator must actually do was already collected at the top and stays there,
because a title alone cannot be acted on.
Reordered so the account takeovers lead rather than sitting ninth and
eleventh behind a thumbnail-rendering fix, and the two public-folder entries
sit together — they are one boundary reported in two halves, and had six
entries between them.
All seven reporters keep their credit, moved inline.
Version is the user's call, recorded here rather than argued: 2.4.1. Note
that the file's own rule above says the last number moves when there are
only fixes, and this entry has an Added section.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
Sixteen locales, four strings each, which is the whole of what had drifted:
three from the IAM-role work (the toggle, its explainer, and the warning
that saving clears a stored key and secret) and one from this week's upload
limits ("Too many uploads are already in progress").
Terminology was taken from each catalogue rather than chosen: every locale
already had "Access key" and "Secret key", and the new strings reuse those
words exactly, so the explainer reads in the same vocabulary as the two
fields directly beneath it. Product nouns stay as they are — AWS, IAM, ECS,
EC2, EKS/IRSA, MinIO, Backblaze, Wasabi, ProjectSend.
Formality was read off each file instead of assumed: informal in ca, es, it,
nl and zh_CN, formal in cs, de, fr, id, pl, pt_BR, ru and tr, matching what
the neighbouring sentences already do. Spanish has three voseo entries among
thirty-three tuteo ones — they arrived with the password-reset work on
2026-09-08 (df44c46a, 6339ae15) and are the outliers, so these follow the
tuteo the rest of the file uses. Worth settling one way or the other by
somebody who speaks it, which is not a job for this commit.
The edits are additive: no entry reordered or rewritten, so the diff is the
four new lines per file and the comma the previous last line grew.
Verified past the point where a JSON file merely parses. The scanner reports
0 missing across all sixteen, the Locale suite passes, and the Spanish
strings were read off a rendered page in a browser — signed in, interface
switched to Spanish through the app's own control, with the explainer
wrapping to four lines inside its column and reading in the same words as
the "Clave de acceso" and "Clave secreta" labels under it. That screen only
draws its S3 half when the provider is S3 and the dev instance is on GCS, so
the provider was flipped for the reading and put back; it was snapshotted
first and restored to what it was, not to a default.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
Requested by @Zodiac1978 in #1777.
The logo already replaced ours in the staff sidebar, on the public listing
and in the client portal. The sign-in screen still wore the ProjectSend
wordmark — and that is the first page of yours most people ever see, and
often the only one a client sees, because it is where the link in a
notification email lands them.
One layout serves every screen reached before signing in, so this covers
login, registration, both password-reset pages, the two-factor challenge,
first-run setup and the page a share link opens. That breadth is the reason
to change the layout rather than the login page: the same visitor moves
between several of them in one sitting, and a logo that appeared on one and
not the next would read as a different site.
Nothing needed gating. `branding.logo_url` is already shared on every
request and is already null wherever the Branding capability is absent, so
an installation that has withheld branding, or never uploaded anything,
renders exactly what it rendered before.
Three tests cover the server's half — the prop reaching a page nobody has
signed in to see, the null fallback, and the capability being taken away.
None of them can say whether the component mounted or the image resolved,
so that was checked in a real browser: headless Chrome against the dev
instance with a logo installed reports the <img> present, naturalWidth 360
(so it decoded rather than sitting broken) and a rendered height of 48px,
and with the logo removed reports no <img> and the fallback SVG in its
place. The branding row was snapshotted before and restored after.
One thing worth knowing, unchanged by this and not introduced by it: a logo
drawn for a white background is hard to read on the dark theme, here and on
every other surface that shows it, because none of them filter the artwork.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
Reported by @Zodiac1978 in #1778, on IONOS.
Step 6 was nginx and only nginx, while "What you need" says Apache is fine.
It is fine, but not without being told two things — document root at
public/, and AllowOverride All with mod_rewrite on, or the .htaccess we ship
does nothing and every address but the home page is a 404. There is now an
Apache vhost beside the nginx one.
The 500 in the report is its own troubleshooting entry, because the entry we
had sends people to storage/logs/ and for this class of failure that
directory is empty — Apache never reached PHP, so ProjectSend had nothing to
write, and an empty log reads as a dead end rather than as the clue it is.
The error is in Apache's log. Two causes cover nearly all of them: Options
refused by AllowOverride, and the internal-redirect loop this reporter hit,
where Apache cannot derive the per-directory base and the front-controller
rule rewrites to a path that is not there, repeatedly.
public/.htaccess now carries a commented-out RewriteBase with the
explanation next to it, which is where somebody debugging a 500 is already
looking.
Only RewriteBase is documented, not the report's second change — making the
substitution absolute (`/index.php`). With the base set correctly the
relative form resolves to the same place, and the absolute one would send a
subdirectory install to the domain root's index.php instead.
The trap underneath all of this is worth its own paragraph, and nothing said
it before: update.sh merge-copies the release over the install, so an edit
to public/.htaccess is reverted on the next update and the site 500s again.
Put the directives in the vhost if it is yours to edit, since an update
cannot reach there — and on shared hosting, where it is not, keep a note.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
Reported by @skeletonsec as GHSA-w29w-pj29-x7ww.
Deleting an account that owns files makes the admin choose who inherits
them. The picker narrows that list for a client-scoped staff member to their
own roster, and says why two methods up: "a client-scoped staff member is
not shown the name of somebody they can reach nothing of, and a picker is no
more a reason to hand one over than a listing is."
The write asked something else entirely — exists, active, and not the
account being deleted. All three are true of every account on the
installation. So a scoped staffer could name an id the picker had
deliberately kept off the list, and a roster client's files and folders
landed with a client on somebody else's roster: readable, editable and
deletable there, because a client owns what they uploaded and
visibleToClient() includes uploaded_by.
The entry doors scope the source account and always did — guardTarget goes
through canAssignClient. It is the destination nobody scoped.
candidates() and validate() now run one predicate, reachableTargets(),
rather than two that happened to agree. Two that agree by inspection is what
this was: the narrowing existed, was correct, and was only ever applied to
the list.
The refusal deliberately reads as "no such account". An out-of-roster id and
an id belonging to nobody now produce the same message, because a refusal
that distinguishes them lets a scoped staffer walk the id space and learn
which accounts exist outside their roster. That is why Rule::exists is gone
rather than kept alongside: one code path, one answer. A test pins the two
messages as identical instead of naming either.
Both the web screen and the API twin come through this one validate(), so
both are fixed by it — and the test file proves each separately rather than
assuming the sharing holds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
Reported by @skeletonsec as GHSA-rxf8-wh8v-jm9j.
A file in a public folder is public: isEffectivelyPublic() is "my own flag,
or my folder's", read up the whole ancestry. GHSA-237r-jx85-j3hr settled
that three days ago, put the rule in Folder::uploadableBy(), and wired it
into the upload paths.
Content arrives in a folder four other ways. move() drags one file in,
bulkUpdate() moves a selection, update() reparents through the edit form,
and FoldersController::move() drags a whole folder — every file in its
subtree — under a public parent. Each of them asked whether the destination
was *visible* to the mover and then wrote folder_id. Visible is not the same
question as publishable, and the difference is the entire permission: a
staff member given editing rights and deliberately not given upload_public
could publish confidential files to the anonymous site by choosing where
they landed. The API twin of update() had the same gap.
Both earlier advisories named these paths in their own "suggested fix"
sections. Neither demonstrated them, so neither was followed. The fix to a
report wants the scrutiny the report got, and this one did not get it.
The predicate did not need changing — it needed calling. Four sinks now ask
it, plus the API twin. The check stays split in two deliberately: the
destination is resolved through StaffLibraryScope as before, so a folder
somebody cannot see is still a 404 and not an existence oracle, and the
publication clause is a separate 403 on top. They agree by construction —
allowsFolder() is folders()->whereKey()->exists() — so nothing that used to
resolve can now fail the first half.
On the file paths the check fires only when folder_id actually changes,
which is the convention already there: re-saving a file that sits in a
folder out of the saver's scope must keep working. bulkUpdate() checks its
destination once instead, before the loop, because there is one destination
for the batch and if it publishes then no file in the batch may go.
Folder::uploadableBy()'s docblock now says to read the name as "may place
into", with why: the name is what made this easy to miss, and the next
folder_id or parent_id write will be written by somebody reading it.
Ten tests, one per sink with a private-destination control beside it, plus
an editor who *can* publish to show the boundary is about publishing and not
about moving. The last one follows the advisory's own chain to the end and
asserts the thing actually claimed — a stranger with no session, no token
and no assignment fetching the anonymous download URL. It returns 200 on the
code before this commit and 404 after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
Reported by @ry2811 as GHSA-6jh6-gvj5-pv8v.
A resumable upload declares its size, and that declaration is what store()
weighs against the maximum file size and the client's storage quota. Only
the assembled file was ever held to it. The parts in between were bounded
one request at a time and never added up, so a client could declare one
byte and then stream parts: ten thousand part numbers at twice a 20 MB
part is about 400 GB, per session, and the number of sessions was not
bounded either. None of it counted against anything, because nothing
becomes a File row until the upload completes and ClientStorageUsage sums
File rows. A client with a 1 MB quota could fill the volume and repeat.
putPart()'s own comment described this defect and treated the per-part cap
as the answer to it: "without a cap here the exposure is a day's worth of
disk". A cap on one request bounds one request. The exposure was a day's
worth of disk multiplied by however many requests somebody cared to make.
Three limits, and each one exists because the other two do not cover it.
A session may not stage more than it declared. The room for a part is
claimed before the body is read — a body's length is not known until it
has arrived, and by then it is on the disk being protected — and the write
is then capped at exactly what was claimed, so an over-long body is cut
off mid-stream as it always was, against a smaller number. The claim is a
read and a conditional update under a per-session lock, the same shape
complete() already uses: the protocol sends parts in parallel and how many
is the client's choice, so an unlocked read lets every part in flight
claim the same room, while an atomic claim alone refuses the honest
parallel upload instead. Whatever the part really weighs is settled back
afterwards, in a finally, or a client's own retries would exhaust a
session with room to spare.
Open sessions count against the quota at the size they declared. A quota
measured against finished files alone is spent twice by opening sessions
one after another — each is told there is room, because the ones before it
have not finished. The cost is that an abandoned transfer holds its share
until it is cancelled or swept, so the sweeper now runs hourly rather than
daily: that gap is now somebody unable to upload, which it was not before.
And a cap on open sessions, because for anyone with no quota to spend —
staff, and clients on an installation that sets none — the session count
is the only thing between a declared size and any multiple of it.
Four tests fail on the unfixed code, and three existing ones had to change:
they declared a tiny size and sent a large part deliberately, to reach the
re-checks at complete(). That route is now closed at putPart(), so they
reach those re-checks the way a real install would instead — the file-size
limit or the quota moving while a long transfer is running, which is the
reason complete() re-asks rather than trusting what store() decided.
The staged-byte total is BIGINT UNSIGNED, and the suite runs SQLite, which
has no unsigned integers. The first version of the bounds read
`staged_bytes + :delta BETWEEN 0 AND size` and raised SQLSTATE 22003 on
MySQL for any refund — in the comparison, so the bound written to prevent
the underflow was the statement that underflowed. Every SQLite test passed
on it. Both bounds are now arranged so the column is never inside a
subtraction, and UploadSessionStagedBytesMysqlTest skips loudly unless the
connection is MySQL. Verified against 8.4, as was the report itself: three
sessions declaring one byte each put 6 MB on the volume of a client with a
1 MB quota before, and nothing at all after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
Reported by @ry2811 as GHSA-w3w9-prpw-qx77, with a working two-worker
reproducer.
Setup asked the database whether any staff user existed, and created one
some time later, in a separate statement with nothing joining the two. So
two POSTs arriving together both read "no staff" and both inserted a System
Administrator. Different addresses do not collide; `users.email` is the only
unique key and it has nothing to say about there being one first
administrator.
The gap is not narrow. Between the check and the insert sits password
hashing at BCRYPT_ROUNDS=12, which is slow on purpose, so the window is
hundreds of milliseconds wide and observable without trying.
What makes this worth fixing is not that a stranger can set up an
unconfigured installation — first-run setup is open to whoever reaches it
first, and always was. It is that racing the operator is *quiet*. The
operator's own request also succeeds, also redirects to /setup/success, and
the installation they get looks exactly like the one they expected. The
second administrator is discovered later or not at all, and closing setup
afterwards does not revoke it.
FirstAdministrator::claim() makes it one operation. The row it locks is the
System Administrator role, because the obvious candidate cannot work: there
are no staff rows on a fresh install and a lock over an empty result
serialises nothing. That role row is written by the roles migration and
rewritten on every boot, so it is always there to be locked. The second
caller waits on it, and by the time it has the lock the first caller's user
is committed and visible to the re-check it then makes.
Everything the request writes moved inside the claim, including the site
name. A request that loses now writes nothing at all, rather than renaming
the installation on its way to the login screen.
`projectsend:admin --if-none` had the same shape and is fixed the same way
— two containers coming up against one database is the version of this that
needs no attacker. The early check stays where it is so an unattended boot
does not prompt for a password it is about to discard; it is simply asked
again under the lock.
Both tests fail on the unfixed code. They stage the interleaving rather than
attempting real concurrency, creating the winning administrator from a query
listener after the request has made its first check — which is exactly the
window, and the re-check is the only thing that closes it. The lock itself
is invisible to them: the suite runs SQLite, where lockForUpdate() compiles
to nothing. That half was verified against MySQL 8.4 by running the
reporter's race for real, two processes through the full HTTP kernel: two
administrators before, one after, repeatably.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
Setting::DefaultClientStorageQuotaMb defaults to 0, and 0 means
unlimited. That is the right default for somebody setting up their own
installation and the wrong one for an installation a platform operates
on other people's behalf: an account that arrived without an explicit
quota has no ceiling at all, and it does not have to be an account the
platform created.
So a platform may set a floor in the environment
(PROJECTSEND_PLATFORM_DEFAULT_CLIENT_QUOTA_MB), exactly as it sets the
seat caps, and for the same reason those are not settings: it is the
shape of what was sold rather than a preference the installation's
administrator is expressing. It applies only where the setting says
nothing, so an administrator who chose a number keeps it, and an install
with no platform behind it is unaffected.
ClientStorageUsage::defaultQuotaMb() is where the three sources resolve,
and every screen that presents the answer now reads it there:
- The client create and edit screens. The edit screen mirrors that
resolution client-side to draw the usage bar, so handed the raw
setting on a floored installation it computed an effective quota of
zero, printed "unlimited" and hid the bar entirely -- for a client
whose next upload was about to be rejected for exceeding a limit the
screen said did not exist.
- projectsend:status, which gains clients_can_register and
default_client_storage_quota_mb. Both defaults are the permissive
ones, both are invisible from outside, and a document reporting the
setting while uploads obeyed the floor would say the ceiling was
missing on an installation that has one.
The Client settings form deliberately still reads the raw setting: that
field is read and written back on save, so prefilling it with the floor
would write the platform's number into the setting as the
administrator's own choice, where it would outlive the floor.
Reported by @apps3000 in #1770. Upgrading a container from 2.0 or 2.1
with external storage configured restart-loops, and says the database is
unreachable while the database is fine.
A row hydrated from the database does not get the model's column
defaults — only a new model does. So a row written before
external_storage_settings.provider existed reads that column as null,
and the enum match in isConfigured() throws UnhandledMatchError.
That would be a small bug anywhere else. It is not here, because
PlatformServiceProvider::boot() reads these settings on every process
boot, and boot happens before `artisan migrate` runs. During an upgrade
the code is new and the schema is still old, so every artisan command in
that window dies — including `projectsend:update`, the one that would
have added the column. Reordering the entrypoint or using a lighter
readiness probe does not help for that reason; the crash is in the
bootstrap, not in the probe.
current() now applies the model's declared defaults to any column the
hydrated row does not have. That closes the window for every column with
a default rather than for the one where it was found, and goes inert the
moment the schema is current. The match in isConfigured() is left total
on purpose: a default arm would swallow a real unhandled case, and the
invariant it needs now holds at the one place the row is read.
The probe's message is the other half. It boots the whole application,
so it fails both when the database is absent and when the application
cannot start, and it reported the second as the first — sending an
operator off checking credentials that were never wrong. It now prints
the error it actually hit and says which of the two it looks like.
Verified end to end against a 2.1-shaped database: `artisan migrate`
dies with UnhandledMatchError before the change and completes after it,
leaving the row reading as S3 with its bucket intact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QmyH342d8MuW3pDuE9mbtS
Requested by @ToMMy86 in #1773: an install running on ECS, EC2 or EKS
already has a role attached, and making it also create an IAM user with
a long-lived access key is both extra work and a worse security posture
than the one AWS offers.
The AWS SDK resolves credentials from its default provider chain
whenever none is supplied, and Laravel's FilesystemManager already omits
the `credentials` entry when the key and secret are empty — so the
upload path needed almost nothing. What blocked it was ours:
- `isConfigured()` demanded a key and a secret for S3, so a
credential-less row was never "configured" and every upload silently
stayed on the local disk.
- `access_key` was `required_if:provider,s3` on both the save and the
connection test.
- `probeS3()` built an explicit `credentials` array, so Test connection
would have failed even once uploads worked.
An explicit `use_instance_role` column rather than "the key was left
blank", because blank already means "keep the credential you have" on
this form — neither the secret nor the GCS key file is ever sent back to
the browser. Ticking it deletes the stored key and secret rather than
leaving them in the row for the next database dump.
Unchanged for everyone else: MinIO, Backblaze, Wasabi and any other
S3-compatible service still authenticate with a key and secret, and the
region is still required — the chain resolves credentials, not regions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QmyH342d8MuW3pDuE9mbtS
The screen leaked account existence a second way, through the write, and
this one is older than last night's: it is the scaffolding. Laravel
answers a failed reset with passwords.user for an address it cannot find
and passwords.token for a real one whose token is dead, and the controller
surfaced __($status) straight through. Two sentences, one difference, and
the difference is whether the account is here.
passwords.throttled is the third and the sharpest. The broker throttles
per user, so an address nobody holds can never be throttled — being told
to wait is being told the account exists.
All of them collapse to one sentence now. Nothing is lost: the action is
the same in every case, and /forgot-password one step earlier already
refuses to say whether an address has an account. Keeping three messages
was only ever more precise about a thing we had decided not to say.
Found because the portal session went looking for the GET oracle I had
just fixed, found theirs, and also found a POST variant I had not thought
to check. I had it too.
The test asserts the two refusals are identical rather than naming the
sentence, so it survives the wording changing.
I built the oracle in the commit whose docblock describes preventing it.
The comment said a page answering "expired" for a real address and
something else for an unknown one would tell anybody who typed a guess
whether an account is here — and then the method returned false for an
unknown address and true for a known one. Two branches, two answers, and
the difference was the account.
/forgot-password deliberately says "a link will be sent if the account
exists". This undid that on the next screen along.
Both branches answer the same now: anything that will not validate reads
as expired, whether the address is known, unknown or absent. The message
stays right in every case somebody real will meet — a mistyped address
gets "ask for a new link", which is what they should do anyway — and the
page reveals nothing.
The test is written as "these two are the same answer" rather than "both
are false", so it keeps holding if somebody later changes which answer it
is. The two tests that encoded the oracle asserted `expired` was false for
an unknown address; they were pinning the bug.
Found by asking my own question of my own code. The portal session had
checked whether their collation folded addresses, which sent me back to
the reset screen to see what it does with an address it cannot place.
Reported by @choewonwoo1817 as GHSA-wgxf-v8cr-37mj, with a working
end-to-end reproducer against Keycloak.
`where('email', $address)` is not an exact match. It is whatever the
database says equality means, and the collation INSTALL.md tells people to
create — utf8mb4_unicode_ci — folds accents:
administrator@example.com = administrator@éxample.com -> 1
Those are two different domains. The second is xn--xample-9ua.com, which
somebody else can register and honestly verify at an OIDC provider. So an
attacker with no account here could sign in as themselves and be handed
the first account: SocialAuthenticator found it, linked their subject to
it permanently, and started a session. No password, no interaction from
the owner, an administrator session where that account was one.
Comparison now happens in PHP, in one place, on every driver. Case is
still folded because that is a real requirement — addresses are stored
lowercased and a provider may send any case — and mb_strtolower folds case
without folding accents, which is exactly the line to draw.
Three call sites move to it and two deliberately do not. Loose matching is
right when *refusing* and wrong when *selecting*: AvailableEmailRule and
ClientProvisioning ask "is this address free", where a collation that says
no to a near-miss refuses more registrations, which is the safe direction.
The three that ask "which account is this" are the social path, the login
form (where a password still gated it, so it was confusion rather than
takeover) and the erasure command (irreversible, and the wrong row is the
wrong person).
The test story is the part worth reading. The suite runs on SQLite, whose
`=` is byte-exact, so this defect does not exist there and never did —
which is how it survived six releases with everything green. A test
written the obvious way passes on unfixed code. So the comparison is
pinned by driver-independent tests that always run, and the chain is
proved by AccountLookupCollationTest, which skips unless the connection is
MySQL and carries the command to run it. Run against real MySQL with the
real collation: it fails on the old code and passes on the new.
All four themes had it as a flex child of the row holding the heading and
the buttons, so it was never full width: it took part of the line and
squeezed "My files" and "Upload a file" into a narrow column beside it. It
now sits above that row, which is where the staff dashboard has always put
it and where the comment claimed it was.
Worth noting why four themes shipped it wrong. The band renders nothing
for almost every viewer — it needs a hosted instance, a free plan, and a
client looking — so the broken layout was invisible to the suite, to the
build, and to anybody working on those pages. Seen only once somebody on
the real free tier looked at it.
Also adds it to the client's dashboard. That screen is about the account
rather than about the files, which is the more natural place for an offer
about the plan, and both read the same shared prop so they cannot disagree
about what is said or to whom.
Checked with real screenshots in all four themes and on the dashboard,
against a temporary listener standing in for cloud-modules, since this
install is community and would otherwise render nothing. The listener and
the theme setting were both put back.
The test above this said "whichever path stored it" and only exercised
the plain staff POST. The path the hosted free tier hangs on is the other
one: a client, through the resumable flow, whose upload is what
cloud-modules listens for to mint the public link.
Worth its own test rather than trusting the shared StoreUploadedFile,
because the package's suite structurally cannot tell us. It fakes both the
event and the link-minting, so a chunked path that stopped dispatching
would leave all 140 of its tests green and the free tier silently inert on
a real instance. That gap is why the listener was checked against
sim-cloud by hand rather than believed; this is the half of it that
belongs in core and runs on every commit.
The counter-check is worth a note. The first attempt at it changed
nothing — `\$file` inside a sed pattern is a literal, so the substitution
never matched and all six tests passed, which reads exactly like a fix
that is not load-bearing. Confirmed the mutation landed by counting the
line before re-running: three tests fail without the dispatch, the new one
among them.
The page rendered the form without looking at the token, so somebody
opening a link an hour late typed a password, typed it again to confirm,
and was then told "this password reset token is invalid" — a word nobody
outside the code knows, at the end rather than the start. Links last an
hour and people open them late. That is ordinary, not an error to be
scolded for.
store() still validates and is still the rule; there is a test that a
spent token is refused there whatever the page drew. This is only the
screen being honest a minute earlier.
An address that is missing, or belongs to nobody, is drawn as the form was
before. Partly because an unanswerable question is not an expired link,
but mostly because a page that said "expired" for a real address and
something else for an unknown one would answer whether an account exists
here to anybody typing guesses — the exact property /forgot-password
protects by saying "a link will be sent if the account exists". Two tests
pin that.
Worth having now rather than later: the advisories publishing with this
release will send more people than usual through this screen, in a hurry
and some of them frightened.
Found by the portal session's user, who opened a real link an hour and
forty minutes after it was sent.
Three, sixteen locales: the Entra optional-claim instruction on the
social-login screen, and the two the profile screen gained when changing
an address started asking for a password.
The catalogues gate the release build, so these had to land before the
version could be stamped.
The reassignment target is one installation-wide id used for every
erasure, and the picker offers clients deliberately: erasing a client and
handing their files to another client is what the setting is for.
Applied to a staff account the same id means something else. A staff
library is usually the whole installation's, so a client named there
inherits all of it — through an unattended scheduled job, with no
per-account confirmation, because this is the default rather than a choice
somebody makes at the moment of deleting.
So a staff account's content may only go to staff. With nobody valid to
hand it to, handleContent() already cascades, which keeps the existing
promise that content is never orphaned — it now also never becomes a
disclosure.
Not in the settings validation, which is where it looks like it belongs.
That runs when the target is chosen, and whose account will be erased
later is not knowable then. Both halves are only in hand here.
Found while checking a list from the portal session, who had it as one
where() on `type`. That would have been too broad: it would also have
stopped a client's files reaching another client, which is the case the
setting exists to serve. The condition is on the account being erased,
not on the target alone.
The upgrade notes sat at the bottom of a release entry, after every list
of what changed. That is the wrong end of the page: somebody deciding
whether to upgrade reads the first screen and stops, and that is exactly
the reader who needs to know a permission stopped working or a value
changed meaning.
So the section moves to the top of the entry, gets a heading nobody
skims past, and opens by saying what these items have in common —
everything else in a release happens on its own, and these do not. It also
says plainly that none of them stops the upgrade, because an operator who
cannot tell "the installation will not start" from "one role gets a 403"
plans the wrong maintenance window.
Three items for this release: the CAPTCHA flag that now means the
opposite for anybody who typed something other than true or 1, the public
folder permission that is now asked of staff, and the Entra claim.
Releases before this keep the old "Upgrade notes" heading — rewriting
published entries would change what people were told at the time. The
file's own header names both, and the release skill now carries the new
shape so the next one does not drift back.
The fix reads only true and 1 as "disabled". An operator who wrote
"off" and has been running without a CAPTCHA gets it back on this
upgrade, which is correct and is still a surprise if nobody says so.
One of the four said 'the same researcher as the group finding above',
which was wrong: @skeletonsec reported the public-folder and upload-target
findings, @Drescargot the group one. The other two were credited by name
rather than by handle, which is not how anybody finds them.
Reported as GHSA-237r-jx85-j3hr.
A file is public if its own flag is on or its folder's is, so the upload
destination reaches the property `upload_public` guards without touching
the switch. A staff member allowed to upload but deliberately not allowed
to publish could publish to the anonymous public site by choosing where
the file landed.
No new key. `upload_to_public_folders` already exists, already appears on
every role's checkboxes, and already means exactly this on the client
branch of the same method — MyFilesController's picker calls it the
established meaning of the two keys. It was never asked of staff, so on a
staff role that checkbox did nothing at all: an unenforced permission, the
class this project audited and closed once already.
Effectively public rather than the folder's own flag, because the flag is
inherited down a subtree: a private folder inside a public one publishes
just the same, and a check on the folder's own column walks past it. There
is a test for that case specifically.
One place, because every upload path — the plain POST, the chunked flow,
the API and the client portal — already asks Folder::uploadableBy(). The
sibling report about the target folder not being scope-checked at all
(GHSA-56qr-cq56-qg66) was fixed in 2c2b86ff and is what put the scope
check on the line above this one.
Reported by Dickson Massawe as GHSA-2rfh-v3j2-2jg7.
Pinning the tenant was half an answer. It defeats the classic
cross-tenant nOAuth, where a stranger's own directory asserts your
address, because a foreign tenant carries a different tid. It does
nothing about the same attack from inside the pinned tenant: Entra's
email claim is user-mutable — a B2B guest's otherMails among its sources
— so a colleague or an invited guest could present an administrator's
address and have their subject bound to that account.
Tenant-pinning answers "which directory said this". It never answered
"does this person own that address". xms_edov is Microsoft's own answer
to the second, and their guidance says to require it wherever email
identifies an account. Absent counts as unverified, which is the only
safe reading given it is absent by default.
Nobody is locked out by this, which is worth saying because it looked
like a breaking change until I read SocialAuthenticator::resolve in
order. An account already linked resolves by subject at step 3, before
trust is consulted at all — those keep working untouched. A first-time
link to an existing account is refused with the message that already
exists for exactly this case, which names the way through: sign in with
your password and connect the provider from your settings. A brand-new
account is still created; it goes to the approval queue rather than
auto-approving.
The settings screen and docs/testing-social-login.md now tell an
operator to add the claim, and there is an upgrade note.
The tests exercise fromSocialite() on raw claims, which nothing did
before: tests/Feature/Auth/SocialLoginTest.php builds a SocialIdentity by
hand and so never reaches this mapping. That is how the branch could
trust a tenant match alone with a full suite passing.
Reported by Nooraldden Khalel as GHSA-f32x-fgmp-q353.
The profile screen let a signed-in session change its own email address
with nothing else, and that address is where a password reset is sent. So
a stolen session was enough: point the account at your own inbox, ask for
a reset, set a password, and temporary access is permanent ownership.
Clearing email_verified_at did not stand in the way, because the model
does not implement MustVerifyEmail and the reset broker never asks.
destroy(), thirty lines further down the same controller, has always
required the current password, and its comment says why: "the rule every
other door into this already asks". This door leads to the same place and
was not asking.
Only a *different* address asks. A name, a timezone or a custom field is
not a credential, so the rest of the screen saves with nothing extra —
which is why the rule is excluded rather than flat, and why the comparison
is trimmed and lowercased: re-saving a profile with the address typed in a
different case must not demand a password for nothing.
An account whose credentials live in a directory or at an identity
provider is refused outright and told why, rather than being asked for a
password it does not have. LdapProvisioner stores Str::password(64)
exactly so that local password can never be used, so asking would be a
dead end dressed as a form error — and the address is not theirs to change
here anyway: it is what the directory says it is.
The test walks the whole chain rather than checking the field is
validated, because the chain is what made this high: change the address,
ask for a reset there, and confirm nothing is sent and no such account
exists.
Reported by @Drescargot as GHSA-r3hg-3fxw-rcmr, in two halves.
The groups listing never narrowed at all. Every other action in that
controller is guarded with allowsGroupChange(), and index() — web and API
alike — built a bare Group::query(), so a client-scoped staff member was
shown every group on the installation with its name, description and
member count. StaffLibraryScope::groups() is that narrowing, and
assignableGroupIds() now reads from it rather than restating the same
rule a second time, which is how the two drifted apart to begin with.
The second half is the one that mattered. allowsGroupChange() asked only
groupReachesNoFurther() — "is anything shared with this group outside my
library" — which a group with nothing shared with it yet passes
vacuously. So a scoped staff member could rename, delete or publish a
group whose every member was somebody else's client. Publishing is the
sharp end: whatever is shared with the group afterwards is reachable
without signing in.
The reporter suggested putting the membership check inside
groupReachesNoFurther(). Tried, and it breaks two things. That predicate
is shared with allowsGroupMembership(), where a group nobody has joined
must stay usable so its creator can add the first member. And "every
member must be mine" is the obvious reading of the rule and is wrong: it
turns GHSA-whmp-p9hv-r7j7's narrowing — a mixed group's edit screen
loads and simply does not name the stranger — back into a 404, undoing
that fix. Four tests from it fail that way.
So the check sits in allowsGroupChange() alone, and asks whether the
group is wholly somebody else's rather than whether it is wholly theirs.
A mixed group stays workable and is still covered by the reach check; an
empty one stays nameable by whoever just made it; a group with members
and none of them theirs is refused.
Four strings, sixteen locales. Everything else was already complete, so
this is the whole of the debt from the client-portal work.
Czech, Polish and Russian do not get "downloaded :count times": those
languages inflect the noun according to the number in front of it, so no
single string can be right for every value. They read as a labelled count
instead — "Pobrania: :count — ostatnie :date" — which is the shape the
other numeric strings in those catalogues already use.
German and Turkish stay formal, Spanish, Dutch, Polish and Chinese stay
informal, as the rest of each catalogue does.
The last two paths that passed email_verified_at into User::create() and
lost it: the setup screen, and projectsend:admin for a container that
comes up from environment variables. It is deliberately absent from
$fillable, so mass assignment drops it without a word, and both meant to
set it.
The intent is plain in both cases — the first administrator typed their
own address into the form in front of them, and whoever provisioned the
container supplied it themselves. There is nobody to confirm it to.
Inert today, since MustVerifyEmail is not enabled on the model, but the
column is what a later switch would read: turning verification on would
have locked out the one account that cannot be helped by another
administrator.
Both are now pinned by a test that fails when the forceFill is removed.
StaffAccounts had already fixed this for staff and named the rest; with
client accounts done earlier today, that list is empty.
Also says on User::$fillable what absence from it buys and what it does
not. It stops a request smuggling a value in; it does not tell code that
meant to set the value that it failed. Four separate paths made the same
mistake against the same comment.
`env()` recognises the words "true" and "false" and returns everything
else as the string it was — and every non-empty string is truthy in PHP.
So `(bool) env('PROJECTSEND_CAPTCHA_DISABLED')` read all of these as "yes,
disabled":
PROJECTSEND_CAPTCHA_DISABLED=no
PROJECTSEND_CAPTCHA_DISABLED=off
PROJECTSEND_CAPTCHA_DISABLED=fasle
An operator who meant to say no took the bot protection off their login
and registration forms and had nothing to tell them so — the setting
screen still shows the CAPTCHA configured, because this is the escape
hatch that runs ahead of it.
For most settings the cast is a shrug: somebody notices the feature is on
and fixes the line. It stops being a shrug when the wrong answer is the
unsafe one, and this is one of those. EnvFlag lists what counts as yes —
`true` and `1`, either case, either type — and reads everything else,
recognised or not, as no. A value typed as `disabled` turns nothing off:
a configuration mistake to be found rather than guessed at.
Found while fixing the same bug in a new cloud-modules flag, where the
unsafe direction was publishing a customer's files rather than dropping a
CAPTCHA. Two of the four remaining `(bool) env()` casts are left alone on
purpose: a wrong S3 path-style value breaks storage loudly, and the
migration tool's direct mode defaults to true anyway, so neither fails
into an unsafe state.
"Did it arrive?" is the question somebody asks about a file they sent, and
on a hosted free account — where a link is the whole of the sharing — the
count is the only evidence either way. Every file a client uploaded now
shows how often it has gone out and when it last did, in every render mode
of every theme.
Only their own. A download entry says somebody fetched the file, so a
count on a file shared with several clients tells each of them about the
others' activity, and nobody is entitled to that but the person who put
the file there. A file shared *with* this client carries null, not zero:
a zero would itself be a claim, and the two have to be distinguishable
because zero is an answer the owner came looking for and is shown as
words.
Counted from the activity log through the same three actions
DownloadAllowance uses, so a file leaving by the public site counts as
much as one leaving by its link. One query for a listing, none at all for
a client with no files of their own. Both filters have a test that fails
when only that filter is removed.
Two things a render check caught that types and a green build did not.
`t()` does no plural selection — the catalogues are flat key/value — so a
"one|many" string reached the screen with its pipe intact; the strings are
whole sentences now, with the singular spelled out. And the gallery card
was already laying its text out beside the action icons in a 200px column,
truncating the filename to "Q…" and the size to "75 …" on main today;
stacking them gives every line its full width.
A client's portal lists two kinds of file side by side: what they
uploaded, and what somebody shared with them. Where a link exists on one
of their own, they can now copy it from the row — which is what makes the
hosted free plan a product rather than a place to put files, since a
customer there has no staff screen on which to make one.
The rule is narrow, and both halves are load-bearing: a link this client
created, on a file this client uploaded.
Not "a link on a file shared with them" — that link is the sharer's
decision about who may reach the file, and handing the recipient the URL
would quietly turn "you may download this" into "you may pass this on to
anyone".
And not "any link on their own file" either — a link staff minted on a
file a client uploaded exists for a reason the client may be no part of,
and on the shared instance it would sit beside the one link they were
promised. Ordering is by id, so an unfiltered lookup would hand them
whichever was minted first.
Both halves have a test that fails when only that half is removed. The
first draft did not: every case was carried by the ownership filter
alone, so the creator check was green for the wrong reason.
Links that no longer work are left out rather than shown greyed. The only
thing a client can do here is copy it, and a URL that answers "this link
has expired" is worse than no URL at all.
One query per listing, not one per row, and none at all for a client with
no files of their own.
Three surfaces create client accounts now: the staff screens,
/api/v1/clients, and the platform control plane in the private package.
Two of them held their own copy of the type, the role, the active flag,
the "0 means inherit the site default" quota, the verified stamp, the
activity entry and the seat guard — and the third could not have a copy
at all, because a package cannot import a host class.
ClientAccounts is that one definition, reached by name from outside.
What stays with each caller is what genuinely differs: its validation,
its response, its custom fields, and who is asking.
Two things changed rather than moved:
The seat cap is now checked inside create(), before anything is written,
instead of at the top of each controller. That is what makes a leaked
platform token an incident rather than an unbounded one — a guard that
ran only where somebody remembered it is not a guard.
email_verified_at is written with forceFill. It is deliberately absent
from User::$fillable, so every client-creation path passed it into a mass
assignment and lost it in silence. StaffAccounts already noted this and
named the other paths; this closes the client half.
The band existed only on the staff dashboard, which is a screen a client
never opens. On a shared instance the customer *is* a client account —
they sign in, upload, and share by link, and the administrator is the
operator — so a message for them has to reach the page they actually use.
ViewerAnnouncement reads the shared prop itself rather than taking one,
so each theme adds it in a single line without threading a prop through a
page that has no other reason to know about it. All four portal themes
render it, because a message that only appears in the theme somebody
remembered to wire is a message that quietly does not exist.
Above the heading, not inside the list: it is not one of the things the
client came to do, and burying it under the files would defeat the point
of having it at all.
No theme decides who sees it. Core drops anything not aimed at the
viewer, so a theme renders whatever it is handed and cannot leak a staff
message to a client by being careless.
The first version refused clients outright. That was right for the only
message that existed — a hosted instance telling its administrator about
their plan — and it stopped being right the moment a message needed to
reach the *clients* of a shared instance, where the administrator is the
operator and the customers are client accounts.
The unsafe fix would have been to drop the guard and let each listener
check `isStaff`. The safe one is to make every caller say who it is
talking to and have core enforce it: `show()` now takes a required
`audience` with no default, and a message aimed elsewhere is dropped
before it reaches the props. A listener that forgets therefore reaches
nobody rather than everybody, which is the direction a mistake should
fall.
An unrecognised audience reaches nobody either, and is ignored rather
than thrown — a listener aimed at the wrong people should show nothing,
not break the page it was decorating.
The old "a client is never shown one" test became "a message for staff
reaches no client, even from a listener that never checks", which is the
property that actually matters and the one the enforcement provides. Two
more pin the other directions: a client message reaches clients and no
staff, and an unknown audience reaches neither.
cloud-modules declares `staff` for the free-plan band, and its test fake
enforces the same rule, so a listener aimed at the wrong audience fails
in the package's own suite rather than passing there and misbehaving in
the host.
Two pieces of groundwork, no behaviour change.
FileWasStored is dispatched from StoreUploadedFile, which every upload
path converges on — the chunked flow staff and clients share, and the
synchronous POST beside it. A listener therefore sees each upload once
without knowing which route produced it, which is the property that makes
it usable from outside this repository. A notification, not a filter:
nothing on it is mutable, and anything that needs to influence an upload
has to do so before the bytes land, which is what ResolvingUploadDisk is
already for.
CreateShareLink is the other half. Minting a link was a ShareLinksController
private concern, and the controller is an HTTP handler behind `staff`
middleware — so a link now needs making from outside a request as well.
Two copies of "make a token, write the row, log it" would drift, and the
half most likely to drift is the token, which is the entire authorization
for /s/{token}: there is no session behind it and no second factor, so
being unguessable is its only defence. Anything minted through the action
gets Str::random(32) — about 190 bits, more than a UUID's 122 — and never
a chosen value. The chosen-token path stays in the controller, where a
person is typing one into a form and its minimum length can be argued
about in a validation rule.
The permission questions stay in the controller too. Whether somebody may
set an expiry or a download cap is a fact about them, and the action has
no viewer to ask; it takes both already resolved, including the expiry,
because "the end of the 12th" depends on whose timezone you are in.
Five tests, including that the file a listener receives is complete and
readable rather than half-built, and that the staff form still refuses an
expiry to somebody without the permission after the extraction.
Renditions are generated on demand and cached by existence, and nothing
between the callers stopped two requests decoding the same image at once.
The atomic rename settled which file survived; it never stopped both from
doing the work. So N concurrent requests for one cold rendition were N
full-size decodes, each holding four bytes per source pixel — up to 160 MB
at the 40-megapixel ceiling.
That is not an attack. A public listing emits a thumbnail URL per file, a
browser opens six or more connections at once, and the first visit to a
gallery of ordinary camera images was six simultaneous decodes on a
container sized for one. PublicGroupsController reaches the generator with
no account at all, so nothing about it required a customer to be signed
in, and the 240/min throttle bounds rate rather than concurrency.
Worse than a crash, it did not resolve itself: a render killed mid-flight
renames nothing, so the cache warmed only by whatever finished before the
kill and the page died again on the next visit.
A lock keyed on the destination path — which already encodes the file, the
audience and the rendition, so two requests collide exactly when they
would have written the same path. The waiter re-reads after acquiring,
which is what turns a wait into a cache hit rather than a second decode of
the same image.
Waiting rather than refusing, because the arithmetic says so: a waiting
request holds an idle worker at about 35 MB, a rendering one holds that
plus the whole source bitmap. Six waiters cost what one renderer costs.
On timeout it refuses instead of rendering anyway. Falling through would
reinstate the pile-on at the moment the system is already struggling, and
one failed thumbnail is a better outcome than a container that dies and
takes the warm cache with it.
The wait is configurable because the right number is a property of the
machine — a small VPS reading a large source off a slow disk wants longer
— and clamped to at least a second, since a stray empty variable would
otherwise make every concurrent request fail instantly, which is the
opposite of the point.
Eight tests. Two go red without the lock, and the clamp is asserted on the
resolved value rather than the clock, because block() measures in whole
seconds and a timing assertion there would be flaky rather than wrong.
Found by the session sizing free-tier containers, from the outside.
`FileDelivery::describe()` from a console returned
`{"method":"php","detected":true}` on every installation, whatever its web
server. detect() reads SERVER_SOFTWARE, which only exists inside a
request, so a console process has nothing to look at and falls to the
`php` default — and `detected: true` then vouched for it.
The value is right for that process and wrong as a statement about the
installation, which is how anybody running it from `artisan tinker` will
read it. Somebody verifying a healthy nginx tenant hit exactly that, spent
an afternoon on it, and only recognised it as an artefact of *where* the
question was asked after reading `nginx -T` in the container.
There is now a third field. `observed` is false only outside a request,
where `method` is a default rather than a finding. Both screens that read
this run in a request and always see true; it exists for whoever asks from
a shell, which is the one place the answer could mislead.
The two web paths are unchanged and were never wrong — `projectsend:status`
does not report delivery at all, so no fleet ever reported this
incorrectly. What was wrong was a confident answer to a question that
could not be answered from where it was asked.
Three tests: a console reading says not observed and still says php,
because php is what that process would actually do; a reading during a
request observes nginx; and a stated method is observed wherever it is
read, since a decision needs nothing detected to be true.
Found by the session verifying the 2.4.0 canary, not by me.
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.
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.
2026-08-28 06:40:41 +02:00
439 changed files with 41100 additions and 1332 deletions
php artisan migrate # creates the tool's two tables
```
`--update-no-dev` keeps Composer from also installing the tools ProjectSend is developed and tested
with. A release zip leaves them out, and a server has no use for them.
That is the whole installation — there is no `npm run build` to run. The zip ships its assets
already compiled and deliberately without the toolchain that compiled them, so there is no
`package.json` to build from.
@@ -177,6 +180,7 @@ are listed at each step.
|---|---|
| Legacy and ProjectSend are on the **same machine** | [**Direct**](#step-3a--direct-same-machine) |
| Legacy is on **another server**, or on hosting you cannot reach from the new box | [**Bundle**](#step-3b--bundle-different-machines) |
| Legacy runs on this machine's own web server, and ProjectSend runs **in Docker** | **Bundle** — see [below](#legacy-on-this-machine-projectsend-in-docker) |
Direct is faster and simpler. It copies your files by default, and it can also *hardlink* them
instead when you ask it to — on a single filesystem that writes no bytes at all, so 400 GB migrates
@@ -205,8 +209,63 @@ file bytes:
| `move` | Takes the bytes out of Legacy. Fast and frees disk — and **cannot be undone** |
| `defer` | Writes no bytes at all. For importing the database now and moving half a terabyte overnight |
If ProjectSend runs in Docker, the Legacy directory has to be visible **inside the app container**
— bind-mount it there, and use the container's path, not the host's.
If ProjectSend runs in Docker, Direct needs two things the container does not have by default: the
Legacy directory mounted inside it, and a way to reach Legacy's database. That database is usually
at `localhost` in Legacy's config, and inside a container `localhost` is the container itself.
Hardlinks also cannot cross into a mount, so `--files=hardlink` quietly becomes a copy. When
Legacy runs on the same machine's own web server, the bundle route below is simpler and needs
none of that.
### Legacy on this machine, ProjectSend in Docker
The usual shape of an upgrade: Legacy was copied into a LAMP server, and the new install follows
[Getting started](README.md#getting-started). Use a bundle. The exporter runs with the PHP your
Legacy site already uses, on the host, where `localhost` really is Legacy's database. Nothing has
to reach across into the container except one directory at the end.
Every command below runs from the directory that holds your `compose.yaml`, after the tool is
installed as described in [Step 1](#if-you-are-running-the-official-docker-image).
No public link passed around by email, no third-party service holding your clients' documents, no
per-seat pricing. It runs on your server, and the files stay there.
Prefer not to run the server yourself? [ProjectSend Cloud](https://projectsend.cloud) is the
official hosted version of ProjectSend, run by the same team — every subscription funds this free
software. The line between the free core and Cloud, and the commitments that go with it, are set
out in [LICENSING.md](LICENSING.md).
## What it does
**For the people you send to**
@@ -49,34 +54,29 @@ per-seat pricing. It runs on your server, and the files stay there.
- Privacy controls, including GDPR-grade account erasure with a grace period
- Local disk, S3-compatible storage, or Google Cloud Storage
## Screenshots
<p align="center">
<img src=".github/screenshots/dashboard.png" alt="The dashboard: counters for files, clients and groups, the clients using the most storage against their quotas, a month of uploads and downloads as a line chart, and recent activity" width="900">
</p>
<p align="center"><em>The dashboard — what is in the installation, and what has been happening in it.</em></p>
<p align="center">
<img src=".github/screenshots/files.png" alt="The file library, showing folders and files with thumbnails, sharing status and download counts" width="900">
</p>
<p align="center"><em>Your library — folders, categories, and who each file is shared with.</em></p>
<p align="center">
<img src=".github/screenshots/portal.png" alt="A client's own page, listing the files shared with them with download buttons" width="900">
</p>
<p align="center"><em>What your client sees — only their files, nothing else.</em></p>
## Getting started
**With Docker** — the quickest path, and the one we recommend. Nothing to build: the published
image ships with its dependencies and its frontend already compiled.
You need Docker Engine with the Compose plugin. If the machine does not have it yet,
[install it](https://docs.docker.com/engine/install/) and then
[let your own user run it](https://docs.docker.com/engine/install/linux-postinstall/): add yourself
to the `docker` group, then log out and back in. Without that step every command below fails with
"permission denied", and putting `sudo` in front of it is not the fix.
Then, in an empty directory of your choosing — `/srv/projectsend` is a good one — run:
Open `APP_URL` and the first thing you see is a setup screen that creates your administrator
account — or uncomment `ADMIN_EMAIL` and `ADMIN_PASSWORD` in the file first, with a password of
your own, and it is created for you.
@@ -98,6 +98,23 @@ installation: the dependencies and the compiled frontend are deliberately not in
needs Composer and npm before it runs. **[CONTRIBUTING.md](CONTRIBUTING.md)** has the sequence, and
it is short.
## Screenshots
<p align="center">
<img src=".github/screenshots/dashboard.png" alt="The dashboard: counters for files, clients and groups, the clients using the most storage against their quotas, a month of uploads and downloads as a line chart, and recent activity" width="900">
</p>
<p align="center"><em>The dashboard — what is in the installation, and what has been happening in it.</em></p>
<p align="center">
<img src=".github/screenshots/files.png" alt="The file library, showing folders and files with thumbnails, sharing status and download counts" width="900">
</p>
<p align="center"><em>Your library — folders, categories, and who each file is shared with.</em></p>
<p align="center">
<img src=".github/screenshots/portal.png" alt="A client's own page, listing the files shared with them with download buttons" width="900">
</p>
<p align="center"><em>What your client sees — only their files, nothing else.</em></p>
## Coming from ProjectSend Legacy?
The previous generation of ProjectSend lives on at
@@ -504,9 +531,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,8 +557,13 @@ 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
// Detecting is half of it. A clamd left on its own defaults
// answers "OK" for an archive it could not open, and every
// encrypted zip would be recorded as clean — while this test
// passed. So ask it about one.
if($this->passesEncryptedArchives($scanner)){
returnback()->with('scanner_test_result',[
'ok'=>false,
'message'=>__(':engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.',[
'engine'=>$status->engine??__('The scanner'),
]),
]);
}
returnback()->with('scanner_test_result',[
'ok'=>true,
'message'=>__('Working. :engine detected the EICAR test file as ":threat". EICAR is a harmless file made only for testing, and every antivirus recognises it.',[
'engine'=>$status->engine??__('The scanner'),
'threat'=>$verdict->detail??'',
]),
]);
}
// Reachable, and did not recognise a file every engine is supposed
// to. Almost always empty or broken virus definitions, which is
// exactly the failure nothing else would show.
returnback()->with('scanner_test_result',[
'ok'=>false,
'message'=>__(':engine answered but did not detect the EICAR test file, a harmless file made only for testing. Check that its virus definitions are installed and up to date.',[
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