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.
Eliana Bracciaforte