Commit Graph

464 Commits

Author SHA1 Message Date
Ignacio Nelson 044afe5fcb Merge pull request #1783 from projectsend/virus-scanning
Scan uploaded files for viruses
2026-09-17 00:01:47 -03:00
ignacionelson a255a883a8 Merge remote-tracking branch 'origin/main' into virus-scanning
# Conflicts:
#	tests/Feature/Platform/SchedulerMonitoringTest.php
2026-09-16 23:57:19 -03:00
ignacionelson 5f7e3089eb Stop offering a file nobody can have
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.
2026-09-16 23:51:33 -03:00
ignacionelson 7119435c3c Make "New scan" mean a new scan, and colour a result by what it is
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.
2026-09-16 23:20:34 -03:00
ignacionelson 25b92c086b Refuse a scanner address that only looks like one
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".
2026-09-16 23:09:24 -03:00
ignacionelson afb4c2c6d4 Test the address on screen, and count the quarantine in the sidebar
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.
2026-09-16 22:57:58 -03:00
ignacionelson 0b36cf2c38 Put the connection test under the address it tests
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.
2026-09-16 22:54:24 -03:00
ignacionelson d445b01dd4 Make starting a scan a button in the header, and drop the box it lived in
"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.
2026-09-16 22:52:08 -03:00
ignacionelson f937b4398d Tell a missing file apart from a missing scanner, and do something about it
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.
2026-09-16 22:46:21 -03:00
ignacionelson dc0937fda1 Add an Activity tab that shows a scan as it happens
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.
2026-09-16 20:18:37 -03:00
ignacionelson c2039d9608 Find the files a real upgrade leaves behind
"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.
2026-09-16 20:11:08 -03:00
ignacionelson da79969435 Put virus scanning on the System card as a line, not only as a warning
"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.
2026-09-16 15:41:47 -03:00
ignacionelson 85b1650ef0 Tell a self-hosted installation when nothing is checking its uploads
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.
2026-09-16 15:34:49 -03:00
ignacionelson f2a7bbb182 Split the virus scanning screen in two, and make the Test button answer
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."
2026-09-16 15:08:41 -03:00
ignacionelson 73d5a5f8e4 Record which engine and definitions reached each verdict
`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.
2026-09-16 15:02:54 -03:00
ignacionelson c0494c6b4b Explain virus scanning in both install guides
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.
2026-09-16 14:46:19 -03:00
ignacionelson 5493955bea Show staff where a file stands, and say the same through the API
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.
2026-09-16 14:45:13 -03:00
ignacionelson 0ae3f3d0f0 Hold the "shared with you" email until the file can actually be had
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.
2026-09-16 14:41:59 -03:00
ignacionelson d11bda094b Say out loud when scanning has quietly stopped protecting anything
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`.
2026-09-16 14:39:06 -03:00
ignacionelson b6b777e42f Add the virus scanning settings screen, with a button that proves it works
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.
2026-09-16 14:34:09 -03:00
Ignacio Nelson 7944eccaf1 Merge pull request #1782 from projectsend/client-expiry-and-start-page
Client accounts that expire, a start page per role and person, and five more file filters
2026-09-16 14:30:14 -03:00
ignacionelson e9496dc357 Give quarantined files a screen, an owner, and somebody to tell
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.
2026-09-16 14:29:41 -03:00
ignacionelson bab90c0ad8 Scan uploaded files for viruses, and withhold them until they are checked
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.
2026-09-16 14:23:56 -03:00
ignacionelson 5f414c7a4a Give the API the same file filters the library screen has
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.
2026-09-16 14:11:06 -03:00
ignacionelson 5966d22f50 Add five ways to narrow the file library
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.
2026-09-16 13:04:10 -03:00
ignacionelson 9b99972a1a Update js-yaml to 4.3.2 to close a Dependabot alert
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.
2026-09-13 16:25:41 -03:00
ignacionelson 2768c87b27 Widen the start page picker so a translated default fits
"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.
2026-09-13 15:38:11 -03:00
ignacionelson b0a95f953d Translate the fourteen strings client expiry and start pages added
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.
2026-09-13 15:37:21 -03:00
ignacionelson 3917cb2af3 Stop a file expiry date sent as a number from 500ing
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.
2026-09-13 15:34:05 -03:00
ignacionelson 495f3ae471 Let each role, and each person, choose where they land after signing in
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.
2026-09-13 15:05:40 -03:00
ignacionelson c21658f6f7 Let a client account expire on a date
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.
2026-09-13 14:57:16 -03:00
ignacionelson 7c7ba7cd53 Stop a typed-in storage quota from 500ing when a client is created
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.
2026-09-12 21:16:49 -03:00
ignacionelson f06a3c7ab3 Put a new client account in front of the staff who administer clients
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
2026-09-12 17:34:47 -03:00
ignacionelson 123ae68972 Give invitations their own place in the navigation
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
2026-09-12 17:22:31 -03:00
ignacionelson 1577862099 Translate the forty-one strings the invitation work added
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
2026-09-12 16:47:29 -03:00
ignacionelson fb3fd1766c Turn the second tab into a history of every invitation, and open on it
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
2026-09-12 16:23:50 -03:00
ignacionelson 073bf8b853 Put the invite form and the pending list on their own tabs
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
2026-09-12 16:16:35 -03:00
ignacionelson 38187dcf1a Stop an expired invitation being renewed for ever
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
2026-09-12 15:57:27 -03:00
ignacionelson a502a26075 Let staff cancel an invitation nobody has used
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
2026-09-12 15:55:17 -03:00
ignacionelson 74d1de2d6e Say why an invitation was refused when the installation is full
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
2026-09-12 15:50:21 -03:00
ignacionelson cd2ce960d3 Refuse an invitation whose address was taken while the link was live
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
2026-09-12 14:30:42 -03:00
mash2k3 856c13b09c Invite a client to register instead of handing them a password (#1780)
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
2026-09-12 14:28:47 -03:00
ignacionelson b8050b36ca Release 2.4.1
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
v2.4.1
2026-09-11 13:59:02 -03:00
ignacionelson da5aadd1f3 Cut the 2.4.1 entry down to what changed, and add what was missing
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
2026-09-11 13:55:06 -03:00
ignacionelson 386cb32ebb Translate the four strings that accumulated since 2.4.0
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
2026-09-11 13:26:08 -03:00
ignacionelson 38400956bc Put the installation's own logo on the pages people sign in through
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
2026-09-11 13:00:38 -03:00
ignacionelson 8aef6e5b5a Tell an Apache install what it needs, and where its 500 is written
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
2026-09-11 12:45:26 -03:00
ignacionelson 8372f42525 Ask the picker's own question of what comes back from it
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
2026-09-11 12:19:28 -03:00
ignacionelson 50f8b578df Ask the publication question wherever content lands, not just on upload
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
2026-09-11 12:16:23 -03:00
ignacionelson 6ad26bb61e Hold an upload to the size it said it was sending
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
2026-09-11 12:05:41 -03:00