mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-19 01:55:08 +00:00
d445b01dd458f6ed60634135d6a910a0dd67f4f0
450 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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. |
||
|
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.
|
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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." |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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`. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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.
|
||
|
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. |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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
|
||
|
ignacionelson
|
b8050b36ca |
Release 2.4.1
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRxv2.4.1 |
||
|
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
|
||
|
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 (
|
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
ignacionelson
|
83a8fe2288 |
Claim the installation instead of checking whether it is free
Reported by @ry2811 as GHSA-w3w9-prpw-qx77, with a working two-worker reproducer. Setup asked the database whether any staff user existed, and created one some time later, in a separate statement with nothing joining the two. So two POSTs arriving together both read "no staff" and both inserted a System Administrator. Different addresses do not collide; `users.email` is the only unique key and it has nothing to say about there being one first administrator. The gap is not narrow. Between the check and the insert sits password hashing at BCRYPT_ROUNDS=12, which is slow on purpose, so the window is hundreds of milliseconds wide and observable without trying. What makes this worth fixing is not that a stranger can set up an unconfigured installation — first-run setup is open to whoever reaches it first, and always was. It is that racing the operator is *quiet*. The operator's own request also succeeds, also redirects to /setup/success, and the installation they get looks exactly like the one they expected. The second administrator is discovered later or not at all, and closing setup afterwards does not revoke it. FirstAdministrator::claim() makes it one operation. The row it locks is the System Administrator role, because the obvious candidate cannot work: there are no staff rows on a fresh install and a lock over an empty result serialises nothing. That role row is written by the roles migration and rewritten on every boot, so it is always there to be locked. The second caller waits on it, and by the time it has the lock the first caller's user is committed and visible to the re-check it then makes. Everything the request writes moved inside the claim, including the site name. A request that loses now writes nothing at all, rather than renaming the installation on its way to the login screen. `projectsend:admin --if-none` had the same shape and is fixed the same way — two containers coming up against one database is the version of this that needs no attacker. The early check stays where it is so an unattended boot does not prompt for a password it is about to discard; it is simply asked again under the lock. Both tests fail on the unfixed code. They stage the interleaving rather than attempting real concurrency, creating the winning administrator from a query listener after the request has made its first check — which is exactly the window, and the re-check is the only thing that closes it. The lock itself is invisible to them: the suite runs SQLite, where lockForUpdate() compiles to nothing. That half was verified against MySQL 8.4 by running the reporter's race for real, two processes through the full HTTP kernel: two administrators before, one after, repeatably. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx |
||
|
ignacionelson
|
62c763d04e |
Put a floor under a client quota nobody set
Setting::DefaultClientStorageQuotaMb defaults to 0, and 0 means
unlimited. That is the right default for somebody setting up their own
installation and the wrong one for an installation a platform operates
on other people's behalf: an account that arrived without an explicit
quota has no ceiling at all, and it does not have to be an account the
platform created.
So a platform may set a floor in the environment
(PROJECTSEND_PLATFORM_DEFAULT_CLIENT_QUOTA_MB), exactly as it sets the
seat caps, and for the same reason those are not settings: it is the
shape of what was sold rather than a preference the installation's
administrator is expressing. It applies only where the setting says
nothing, so an administrator who chose a number keeps it, and an install
with no platform behind it is unaffected.
ClientStorageUsage::defaultQuotaMb() is where the three sources resolve,
and every screen that presents the answer now reads it there:
- The client create and edit screens. The edit screen mirrors that
resolution client-side to draw the usage bar, so handed the raw
setting on a floored installation it computed an effective quota of
zero, printed "unlimited" and hid the bar entirely -- for a client
whose next upload was about to be rejected for exceeding a limit the
screen said did not exist.
- projectsend:status, which gains clients_can_register and
default_client_storage_quota_mb. Both defaults are the permissive
ones, both are invisible from outside, and a document reporting the
setting while uploads obeyed the floor would say the ceiling was
missing on an installation that has one.
The Client settings form deliberately still reads the raw setting: that
field is read and written back on save, so prefilling it with the floor
would write the platform's number into the setting as the
administrator's own choice, where it would outlive the floor.
|
||
|
ignacionelson
|
0671848bfa |
Read settings written before the columns they name existed
Reported by @apps3000 in #1770. Upgrading a container from 2.0 or 2.1 with external storage configured restart-loops, and says the database is unreachable while the database is fine. A row hydrated from the database does not get the model's column defaults — only a new model does. So a row written before external_storage_settings.provider existed reads that column as null, and the enum match in isConfigured() throws UnhandledMatchError. That would be a small bug anywhere else. It is not here, because PlatformServiceProvider::boot() reads these settings on every process boot, and boot happens before `artisan migrate` runs. During an upgrade the code is new and the schema is still old, so every artisan command in that window dies — including `projectsend:update`, the one that would have added the column. Reordering the entrypoint or using a lighter readiness probe does not help for that reason; the crash is in the bootstrap, not in the probe. current() now applies the model's declared defaults to any column the hydrated row does not have. That closes the window for every column with a default rather than for the one where it was found, and goes inert the moment the schema is current. The match in isConfigured() is left total on purpose: a default arm would swallow a real unhandled case, and the invariant it needs now holds at the one place the row is read. The probe's message is the other half. It boots the whole application, so it fails both when the database is absent and when the application cannot start, and it reported the second as the first — sending an operator off checking credentials that were never wrong. It now prints the error it actually hit and says which of the two it looks like. Verified end to end against a 2.1-shaped database: `artisan migrate` dies with UnhandledMatchError before the change and completes after it, leaving the row reading as S3 with its bucket intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmyH342d8MuW3pDuE9mbtS |
||
|
Ignacio Nelson
|
469297893b |
Merge pull request #1775 from projectsend/s3-instance-role
Support AWS IAM roles for S3 storage |
||
|
ignacionelson
|
eaba7ff633 |
Let an AWS-hosted install authenticate as its own IAM role
Requested by @ToMMy86 in #1773: an install running on ECS, EC2 or EKS already has a role attached, and making it also create an IAM user with a long-lived access key is both extra work and a worse security posture than the one AWS offers. The AWS SDK resolves credentials from its default provider chain whenever none is supplied, and Laravel's FilesystemManager already omits the `credentials` entry when the key and secret are empty — so the upload path needed almost nothing. What blocked it was ours: - `isConfigured()` demanded a key and a secret for S3, so a credential-less row was never "configured" and every upload silently stayed on the local disk. - `access_key` was `required_if:provider,s3` on both the save and the connection test. - `probeS3()` built an explicit `credentials` array, so Test connection would have failed even once uploads worked. An explicit `use_instance_role` column rather than "the key was left blank", because blank already means "keep the credential you have" on this form — neither the secret nor the GCS key file is ever sent back to the browser. Ticking it deletes the stored key and secret rather than leaving them in the row for the next database dump. Unchanged for everyone else: MinIO, Backblaze, Wasabi and any other S3-compatible service still authenticate with a key and secret, and the region is still required — the chain resolves credentials, not regions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmyH342d8MuW3pDuE9mbtS |
||
|
ignacionelson
|
6339ae1514 |
Answer a failed reset the same way whatever failed
The screen leaked account existence a second way, through the write, and this one is older than last night's: it is the scaffolding. Laravel answers a failed reset with passwords.user for an address it cannot find and passwords.token for a real one whose token is dead, and the controller surfaced __($status) straight through. Two sentences, one difference, and the difference is whether the account is here. passwords.throttled is the third and the sharpest. The broker throttles per user, so an address nobody holds can never be throttled — being told to wait is being told the account exists. All of them collapse to one sentence now. Nothing is lost: the action is the same in every case, and /forgot-password one step earlier already refuses to say whether an address has an account. Keeping three messages was only ever more precise about a thing we had decided not to say. Found because the portal session went looking for the GET oracle I had just fixed, found theirs, and also found a POST variant I had not thought to check. I had it too. The test asserts the two refusals are identical rather than naming the sentence, so it survives the wording changing. |
||
|
ignacionelson
|
7c4b582d25 |
Stop the expired-link notice saying whether an account exists
I built the oracle in the commit whose docblock describes preventing it. The comment said a page answering "expired" for a real address and something else for an unknown one would tell anybody who typed a guess whether an account is here — and then the method returned false for an unknown address and true for a known one. Two branches, two answers, and the difference was the account. /forgot-password deliberately says "a link will be sent if the account exists". This undid that on the next screen along. Both branches answer the same now: anything that will not validate reads as expired, whether the address is known, unknown or absent. The message stays right in every case somebody real will meet — a mistyped address gets "ask for a new link", which is what they should do anyway — and the page reveals nothing. The test is written as "these two are the same answer" rather than "both are false", so it keeps holding if somebody later changes which answer it is. The two tests that encoded the oracle asserted `expired` was false for an unknown address; they were pinning the bug. Found by asking my own question of my own code. The portal session had checked whether their collation folded addresses, which sent me back to the reset screen to see what it does with an address it cannot place. |
||
|
ignacionelson
|
b96d060ad8 |
Compare an address ourselves, instead of asking the collation
Reported by @choewonwoo1817 as GHSA-wgxf-v8cr-37mj, with a working
end-to-end reproducer against Keycloak.
`where('email', $address)` is not an exact match. It is whatever the
database says equality means, and the collation INSTALL.md tells people to
create — utf8mb4_unicode_ci — folds accents:
administrator@example.com = administrator@éxample.com -> 1
Those are two different domains. The second is xn--xample-9ua.com, which
somebody else can register and honestly verify at an OIDC provider. So an
attacker with no account here could sign in as themselves and be handed
the first account: SocialAuthenticator found it, linked their subject to
it permanently, and started a session. No password, no interaction from
the owner, an administrator session where that account was one.
Comparison now happens in PHP, in one place, on every driver. Case is
still folded because that is a real requirement — addresses are stored
lowercased and a provider may send any case — and mb_strtolower folds case
without folding accents, which is exactly the line to draw.
Three call sites move to it and two deliberately do not. Loose matching is
right when *refusing* and wrong when *selecting*: AvailableEmailRule and
ClientProvisioning ask "is this address free", where a collation that says
no to a near-miss refuses more registrations, which is the safe direction.
The three that ask "which account is this" are the social path, the login
form (where a password still gated it, so it was confusion rather than
takeover) and the erasure command (irreversible, and the wrong row is the
wrong person).
The test story is the part worth reading. The suite runs on SQLite, whose
`=` is byte-exact, so this defect does not exist there and never did —
which is how it survived six releases with everything green. A test
written the obvious way passes on unfixed code. So the comparison is
pinned by driver-independent tests that always run, and the chain is
proved by AccountLookupCollationTest, which skips unless the connection is
MySQL and carries the command to run it. Run against real MySQL with the
real collation: it fails on the old code and passes on the new.
|
||
|
ignacionelson
|
6d7d80f62f |
Give the announcement band its own row, and put it on the dashboard too
All four themes had it as a flex child of the row holding the heading and the buttons, so it was never full width: it took part of the line and squeezed "My files" and "Upload a file" into a narrow column beside it. It now sits above that row, which is where the staff dashboard has always put it and where the comment claimed it was. Worth noting why four themes shipped it wrong. The band renders nothing for almost every viewer — it needs a hosted instance, a free plan, and a client looking — so the broken layout was invisible to the suite, to the build, and to anybody working on those pages. Seen only once somebody on the real free tier looked at it. Also adds it to the client's dashboard. That screen is about the account rather than about the files, which is the more natural place for an offer about the plan, and both read the same shared prop so they cannot disagree about what is said or to whom. Checked with real screenshots in all four themes and on the dashboard, against a temporary listener standing in for cloud-modules, since this install is community and would otherwise render nothing. The listener and the theme setting were both put back. |
||
|
ignacionelson
|
bc559ade3f |
Prove a client's upload announces itself, not just a staff one
The test above this said "whichever path stored it" and only exercised the plain staff POST. The path the hosted free tier hangs on is the other one: a client, through the resumable flow, whose upload is what cloud-modules listens for to mint the public link. Worth its own test rather than trusting the shared StoreUploadedFile, because the package's suite structurally cannot tell us. It fakes both the event and the link-minting, so a chunked path that stopped dispatching would leave all 140 of its tests green and the free tier silently inert on a real instance. That gap is why the listener was checked against sim-cloud by hand rather than believed; this is the half of it that belongs in core and runs on every commit. The counter-check is worth a note. The first attempt at it changed nothing — `\$file` inside a sed pattern is a literal, so the substitution never matched and all six tests passed, which reads exactly like a fix that is not load-bearing. Confirmed the mutation landed by counting the line before re-running: three tests fail without the dispatch, the new one among them. |
||
|
ignacionelson
|
df44c46a12 |
Say a reset link has expired before asking for the work
The page rendered the form without looking at the token, so somebody opening a link an hour late typed a password, typed it again to confirm, and was then told "this password reset token is invalid" — a word nobody outside the code knows, at the end rather than the start. Links last an hour and people open them late. That is ordinary, not an error to be scolded for. store() still validates and is still the rule; there is a test that a spent token is refused there whatever the page drew. This is only the screen being honest a minute earlier. An address that is missing, or belongs to nobody, is drawn as the form was before. Partly because an unanswerable question is not an expired link, but mostly because a page that said "expired" for a real address and something else for an unknown one would answer whether an account exists here to anybody typing guesses — the exact property /forgot-password protects by saying "a link will be sent if the account exists". Two tests pin that. Worth having now rather than later: the advisories publishing with this release will send more people than usual through this screen, in a hurry and some of them frightened. Found by the portal session's user, who opened a real link an hour and forty minutes after it was sent. |
||
|
ignacionelson
|
a1f59e133b |
Translate the strings the security fixes added
Three, sixteen locales: the Entra optional-claim instruction on the social-login screen, and the two the profile screen gained when changing an address started asking for a password. The catalogues gate the release build, so these had to land before the version could be stamped. |
||
|
ignacionelson
|
0a3410140d |
Keep an erased staff member's library away from a client
The reassignment target is one installation-wide id used for every erasure, and the picker offers clients deliberately: erasing a client and handing their files to another client is what the setting is for. Applied to a staff account the same id means something else. A staff library is usually the whole installation's, so a client named there inherits all of it — through an unattended scheduled job, with no per-account confirmation, because this is the default rather than a choice somebody makes at the moment of deleting. So a staff account's content may only go to staff. With nobody valid to hand it to, handleContent() already cascades, which keeps the existing promise that content is never orphaned — it now also never becomes a disclosure. Not in the settings validation, which is where it looks like it belongs. That runs when the target is chosen, and whose account will be erased later is not knowable then. Both halves are only in hand here. Found while checking a list from the portal session, who had it as one where() on `type`. That would have been too broad: it would also have stopped a client's files reaching another client, which is the case the setting exists to serve. The condition is on the account being erased, not on the target alone. |
||
|
ignacionelson
|
80cf99d80e |
Put what the operator must do at the top, and mark it
The upgrade notes sat at the bottom of a release entry, after every list of what changed. That is the wrong end of the page: somebody deciding whether to upgrade reads the first screen and stops, and that is exactly the reader who needs to know a permission stopped working or a value changed meaning. So the section moves to the top of the entry, gets a heading nobody skims past, and opens by saying what these items have in common — everything else in a release happens on its own, and these do not. It also says plainly that none of them stops the upgrade, because an operator who cannot tell "the installation will not start" from "one role gets a 403" plans the wrong maintenance window. Three items for this release: the CAPTCHA flag that now means the opposite for anybody who typed something other than true or 1, the public folder permission that is now asked of staff, and the Entra claim. Releases before this keep the old "Upgrade notes" heading — rewriting published entries would change what people were told at the time. The file's own header names both, and the release skill now carries the new shape so the next one does not drift back. |