mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
v2.4.1
253 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
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 |
||
|
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
|
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
|
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
|
1149df277b |
Ask for the public-folder key before publishing through a folder
Reported as GHSA-237r-jx85-j3hr.
A file is public if its own flag is on or its folder's is, so the upload
destination reaches the property `upload_public` guards without touching
the switch. A staff member allowed to upload but deliberately not allowed
to publish could publish to the anonymous public site by choosing where
the file landed.
No new key. `upload_to_public_folders` already exists, already appears on
every role's checkboxes, and already means exactly this on the client
branch of the same method — MyFilesController's picker calls it the
established meaning of the two keys. It was never asked of staff, so on a
staff role that checkbox did nothing at all: an unenforced permission, the
class this project audited and closed once already.
Effectively public rather than the folder's own flag, because the flag is
inherited down a subtree: a private folder inside a public one publishes
just the same, and a check on the folder's own column walks past it. There
is a test for that case specifically.
One place, because every upload path — the plain POST, the chunked flow,
the API and the client portal — already asks Folder::uploadableBy(). The
sibling report about the target folder not being scope-checked at all
(GHSA-56qr-cq56-qg66) was fixed in
|
||
|
ignacionelson
|
ab5fa2da8b |
Make Entra prove the address, not just the directory
Reported by Dickson Massawe as GHSA-2rfh-v3j2-2jg7. Pinning the tenant was half an answer. It defeats the classic cross-tenant nOAuth, where a stranger's own directory asserts your address, because a foreign tenant carries a different tid. It does nothing about the same attack from inside the pinned tenant: Entra's email claim is user-mutable — a B2B guest's otherMails among its sources — so a colleague or an invited guest could present an administrator's address and have their subject bound to that account. Tenant-pinning answers "which directory said this". It never answered "does this person own that address". xms_edov is Microsoft's own answer to the second, and their guidance says to require it wherever email identifies an account. Absent counts as unverified, which is the only safe reading given it is absent by default. Nobody is locked out by this, which is worth saying because it looked like a breaking change until I read SocialAuthenticator::resolve in order. An account already linked resolves by subject at step 3, before trust is consulted at all — those keep working untouched. A first-time link to an existing account is refused with the message that already exists for exactly this case, which names the way through: sign in with your password and connect the provider from your settings. A brand-new account is still created; it goes to the approval queue rather than auto-approving. The settings screen and docs/testing-social-login.md now tell an operator to add the claim, and there is an upgrade note. The tests exercise fromSocialite() on raw claims, which nothing did before: tests/Feature/Auth/SocialLoginTest.php builds a SocialIdentity by hand and so never reaches this mapping. That is how the branch could trust a tenant match alone with a full suite passing. |
||
|
ignacionelson
|
3244be6bac |
Ask for the password before changing the address a reset goes to
Reported by Nooraldden Khalel as GHSA-f32x-fgmp-q353. The profile screen let a signed-in session change its own email address with nothing else, and that address is where a password reset is sent. So a stolen session was enough: point the account at your own inbox, ask for a reset, set a password, and temporary access is permanent ownership. Clearing email_verified_at did not stand in the way, because the model does not implement MustVerifyEmail and the reset broker never asks. destroy(), thirty lines further down the same controller, has always required the current password, and its comment says why: "the rule every other door into this already asks". This door leads to the same place and was not asking. Only a *different* address asks. A name, a timezone or a custom field is not a credential, so the rest of the screen saves with nothing extra — which is why the rule is excluded rather than flat, and why the comparison is trimmed and lowercased: re-saving a profile with the address typed in a different case must not demand a password for nothing. An account whose credentials live in a directory or at an identity provider is refused outright and told why, rather than being asked for a password it does not have. LdapProvisioner stores Str::password(64) exactly so that local password can never be used, so asking would be a dead end dressed as a form error — and the address is not theirs to change here anyway: it is what the directory says it is. The test walks the whole chain rather than checking the field is validated, because the chain is what made this high: change the address, ask for a reset there, and confirm nothing is sent and no such account exists. |
||
|
ignacionelson
|
5fb17388cd |
Stop scoped staff reaching groups that are not theirs
Reported by @Drescargot as GHSA-r3hg-3fxw-rcmr, in two halves. The groups listing never narrowed at all. Every other action in that controller is guarded with allowsGroupChange(), and index() — web and API alike — built a bare Group::query(), so a client-scoped staff member was shown every group on the installation with its name, description and member count. StaffLibraryScope::groups() is that narrowing, and assignableGroupIds() now reads from it rather than restating the same rule a second time, which is how the two drifted apart to begin with. The second half is the one that mattered. allowsGroupChange() asked only groupReachesNoFurther() — "is anything shared with this group outside my library" — which a group with nothing shared with it yet passes vacuously. So a scoped staff member could rename, delete or publish a group whose every member was somebody else's client. Publishing is the sharp end: whatever is shared with the group afterwards is reachable without signing in. The reporter suggested putting the membership check inside groupReachesNoFurther(). Tried, and it breaks two things. That predicate is shared with allowsGroupMembership(), where a group nobody has joined must stay usable so its creator can add the first member. And "every member must be mine" is the obvious reading of the rule and is wrong: it turns GHSA-whmp-p9hv-r7j7's narrowing — a mixed group's edit screen loads and simply does not name the stranger — back into a 404, undoing that fix. Four tests from it fail that way. So the check sits in allowsGroupChange() alone, and asks whether the group is wholly somebody else's rather than whether it is wholly theirs. A mixed group stays workable and is still covered by the reach check; an empty one stays nameable by whoever just made it; a group with members and none of them theirs is refused. |
||
|
ignacionelson
|
6560346280 |
Mark the first administrator's address verified, as intended
The last two paths that passed email_verified_at into User::create() and lost it: the setup screen, and projectsend:admin for a container that comes up from environment variables. It is deliberately absent from $fillable, so mass assignment drops it without a word, and both meant to set it. The intent is plain in both cases — the first administrator typed their own address into the form in front of them, and whoever provisioned the container supplied it themselves. There is nobody to confirm it to. Inert today, since MustVerifyEmail is not enabled on the model, but the column is what a later switch would read: turning verification on would have locked out the one account that cannot be helped by another administrator. Both are now pinned by a test that fails when the forceFill is removed. StaffAccounts had already fixed this for staff and named the rest; with client accounts done earlier today, that list is empty. Also says on User::$fillable what absence from it buys and what it does not. It stops a request smuggling a value in; it does not tell code that meant to set the value that it failed. Four separate paths made the same mistake against the same comment. |
||
|
ignacionelson
|
e187513cdd |
Stop a mistyped CAPTCHA flag from switching the CAPTCHA off
`env()` recognises the words "true" and "false" and returns everything
else as the string it was — and every non-empty string is truthy in PHP.
So `(bool) env('PROJECTSEND_CAPTCHA_DISABLED')` read all of these as "yes,
disabled":
PROJECTSEND_CAPTCHA_DISABLED=no
PROJECTSEND_CAPTCHA_DISABLED=off
PROJECTSEND_CAPTCHA_DISABLED=fasle
An operator who meant to say no took the bot protection off their login
and registration forms and had nothing to tell them so — the setting
screen still shows the CAPTCHA configured, because this is the escape
hatch that runs ahead of it.
For most settings the cast is a shrug: somebody notices the feature is on
and fixes the line. It stops being a shrug when the wrong answer is the
unsafe one, and this is one of those. EnvFlag lists what counts as yes —
`true` and `1`, either case, either type — and reads everything else,
recognised or not, as no. A value typed as `disabled` turns nothing off:
a configuration mistake to be found rather than guessed at.
Found while fixing the same bug in a new cloud-modules flag, where the
unsafe direction was publishing a customer's files rather than dropping a
CAPTCHA. Two of the four remaining `(bool) env()` casts are left alone on
purpose: a wrong S3 path-style value breaks storage loudly, and the
migration tool's direct mode defaults to true anyway, so neither fails
into an unsafe state.
|
||
|
ignacionelson
|
896675d631 |
Tell a client whether their own file arrived
"Did it arrive?" is the question somebody asks about a file they sent, and on a hosted free account — where a link is the whole of the sharing — the count is the only evidence either way. Every file a client uploaded now shows how often it has gone out and when it last did, in every render mode of every theme. Only their own. A download entry says somebody fetched the file, so a count on a file shared with several clients tells each of them about the others' activity, and nobody is entitled to that but the person who put the file there. A file shared *with* this client carries null, not zero: a zero would itself be a claim, and the two have to be distinguishable because zero is an answer the owner came looking for and is shown as words. Counted from the activity log through the same three actions DownloadAllowance uses, so a file leaving by the public site counts as much as one leaving by its link. One query for a listing, none at all for a client with no files of their own. Both filters have a test that fails when only that filter is removed. Two things a render check caught that types and a green build did not. `t()` does no plural selection — the catalogues are flat key/value — so a "one|many" string reached the screen with its pipe intact; the strings are whole sentences now, with the singular spelled out. And the gallery card was already laying its text out beside the action icons in a 200px column, truncating the filename to "Q…" and the size to "75 …" on main today; stacking them gives every line its full width. |
||
|
ignacionelson
|
92bb807849 |
Show a client the public link to their own file
A client's portal lists two kinds of file side by side: what they uploaded, and what somebody shared with them. Where a link exists on one of their own, they can now copy it from the row — which is what makes the hosted free plan a product rather than a place to put files, since a customer there has no staff screen on which to make one. The rule is narrow, and both halves are load-bearing: a link this client created, on a file this client uploaded. Not "a link on a file shared with them" — that link is the sharer's decision about who may reach the file, and handing the recipient the URL would quietly turn "you may download this" into "you may pass this on to anyone". And not "any link on their own file" either — a link staff minted on a file a client uploaded exists for a reason the client may be no part of, and on the shared instance it would sit beside the one link they were promised. Ordering is by id, so an unfiltered lookup would hand them whichever was minted first. Both halves have a test that fails when only that half is removed. The first draft did not: every case was carried by the ownership filter alone, so the creator check was green for the wrong reason. Links that no longer work are left out rather than shown greyed. The only thing a client can do here is copy it, and a URL that answers "this link has expired" is worse than no URL at all. One query per listing, not one per row, and none at all for a client with no files of their own. |
||
|
ignacionelson
|
0a28e239d6 |
One home for what a client account is
Three surfaces create client accounts now: the staff screens, /api/v1/clients, and the platform control plane in the private package. Two of them held their own copy of the type, the role, the active flag, the "0 means inherit the site default" quota, the verified stamp, the activity entry and the seat guard — and the third could not have a copy at all, because a package cannot import a host class. ClientAccounts is that one definition, reached by name from outside. What stays with each caller is what genuinely differs: its validation, its response, its custom fields, and who is asking. Two things changed rather than moved: The seat cap is now checked inside create(), before anything is written, instead of at the top of each controller. That is what makes a leaked platform token an incident rather than an unbounded one — a guard that ran only where somebody remembered it is not a guard. email_verified_at is written with forceFill. It is deliberately absent from User::$fillable, so every client-creation path passed it into a mass assignment and lost it in silence. StaffAccounts already noted this and named the other paths; this closes the client half. |
||
|
ignacionelson
|
b128b114b5 |
Make an announcement say who it is for
The first version refused clients outright. That was right for the only message that existed — a hosted instance telling its administrator about their plan — and it stopped being right the moment a message needed to reach the *clients* of a shared instance, where the administrator is the operator and the customers are client accounts. The unsafe fix would have been to drop the guard and let each listener check `isStaff`. The safe one is to make every caller say who it is talking to and have core enforce it: `show()` now takes a required `audience` with no default, and a message aimed elsewhere is dropped before it reaches the props. A listener that forgets therefore reaches nobody rather than everybody, which is the direction a mistake should fall. An unrecognised audience reaches nobody either, and is ignored rather than thrown — a listener aimed at the wrong people should show nothing, not break the page it was decorating. The old "a client is never shown one" test became "a message for staff reaches no client, even from a listener that never checks", which is the property that actually matters and the one the enforcement provides. Two more pin the other directions: a client message reaches clients and no staff, and an unknown audience reaches neither. cloud-modules declares `staff` for the free-plan band, and its test fake enforces the same rule, so a listener aimed at the wrong audience fails in the package's own suite rather than passing there and misbehaving in the host. |
||
|
ignacionelson
|
757fba19ca |
Give uploads a seam, and link-minting one home
Two pieces of groundwork, no behaviour change.
FileWasStored is dispatched from StoreUploadedFile, which every upload
path converges on — the chunked flow staff and clients share, and the
synchronous POST beside it. A listener therefore sees each upload once
without knowing which route produced it, which is the property that makes
it usable from outside this repository. A notification, not a filter:
nothing on it is mutable, and anything that needs to influence an upload
has to do so before the bytes land, which is what ResolvingUploadDisk is
already for.
CreateShareLink is the other half. Minting a link was a ShareLinksController
private concern, and the controller is an HTTP handler behind `staff`
middleware — so a link now needs making from outside a request as well.
Two copies of "make a token, write the row, log it" would drift, and the
half most likely to drift is the token, which is the entire authorization
for /s/{token}: there is no session behind it and no second factor, so
being unguessable is its only defence. Anything minted through the action
gets Str::random(32) — about 190 bits, more than a UUID's 122 — and never
a chosen value. The chosen-token path stays in the controller, where a
person is typing one into a form and its minimum length can be argued
about in a validation rule.
The permission questions stay in the controller too. Whether somebody may
set an expiry or a download cap is a fact about them, and the action has
no viewer to ask; it takes both already resolved, including the expiry,
because "the end of the 12th" depends on whose timezone you are in.
Five tests, including that the file a listener receives is complete and
readable rather than half-built, and that the staff form still refuses an
expiry to somebody without the permission after the extraction.
|
||
|
ignacionelson
|
763e7b0e2e |
Render one image once, however many requests ask at the same time
Renditions are generated on demand and cached by existence, and nothing between the callers stopped two requests decoding the same image at once. The atomic rename settled which file survived; it never stopped both from doing the work. So N concurrent requests for one cold rendition were N full-size decodes, each holding four bytes per source pixel — up to 160 MB at the 40-megapixel ceiling. That is not an attack. A public listing emits a thumbnail URL per file, a browser opens six or more connections at once, and the first visit to a gallery of ordinary camera images was six simultaneous decodes on a container sized for one. PublicGroupsController reaches the generator with no account at all, so nothing about it required a customer to be signed in, and the 240/min throttle bounds rate rather than concurrency. Worse than a crash, it did not resolve itself: a render killed mid-flight renames nothing, so the cache warmed only by whatever finished before the kill and the page died again on the next visit. A lock keyed on the destination path — which already encodes the file, the audience and the rendition, so two requests collide exactly when they would have written the same path. The waiter re-reads after acquiring, which is what turns a wait into a cache hit rather than a second decode of the same image. Waiting rather than refusing, because the arithmetic says so: a waiting request holds an idle worker at about 35 MB, a rendering one holds that plus the whole source bitmap. Six waiters cost what one renderer costs. On timeout it refuses instead of rendering anyway. Falling through would reinstate the pile-on at the moment the system is already struggling, and one failed thumbnail is a better outcome than a container that dies and takes the warm cache with it. The wait is configurable because the right number is a property of the machine — a small VPS reading a large source off a slow disk wants longer — and clamped to at least a second, since a stray empty variable would otherwise make every concurrent request fail instantly, which is the opposite of the point. Eight tests. Two go red without the lock, and the clamp is asserted on the resolved value rather than the clock, because block() measures in whole seconds and a timing assertion there would be flaky rather than wrong. Found by the session sizing free-tier containers, from the outside. |
||
|
ignacionelson
|
a5496d24cd |
Stop describe() vouching for a detection it could not make
`FileDelivery::describe()` from a console returned
`{"method":"php","detected":true}` on every installation, whatever its web
server. detect() reads SERVER_SOFTWARE, which only exists inside a
request, so a console process has nothing to look at and falls to the
`php` default — and `detected: true` then vouched for it.
The value is right for that process and wrong as a statement about the
installation, which is how anybody running it from `artisan tinker` will
read it. Somebody verifying a healthy nginx tenant hit exactly that, spent
an afternoon on it, and only recognised it as an artefact of *where* the
question was asked after reading `nginx -T` in the container.
There is now a third field. `observed` is false only outside a request,
where `method` is a default rather than a finding. Both screens that read
this run in a request and always see true; it exists for whoever asks from
a shell, which is the one place the answer could mislead.
The two web paths are unchanged and were never wrong — `projectsend:status`
does not report delivery at all, so no fleet ever reported this
incorrectly. What was wrong was a confident answer to a question that
could not be answered from where it was asked.
Three tests: a console reading says not observed and still says php,
because php is what that process would actually do; a reading during a
request observes nginx; and a stated method is observed wherever it is
read, since a decision needs nothing detected to be true.
Found by the session verifying the 2.4.0 canary, not by me.
|
||
|
ignacionelson
|
7c16733c16 |
Stop a managed instance being able to hide the project news
I shipped both daily calls as the same kind of thing — an operator's preference — and only one of them is. That was wrong in the direction that matters, because it handed a decision over rather than keeping it. An update notice on a hosted tenant is useless: they cannot act on it, the image is ours, and the screen that would show it is closed by capability. So that check does not run there at all, which is right and unchanged. News is the reverse. Announcements about the product are exactly what a hosted customer should be told, and a Cloud client with view_news sees that card today. One administrator switching it off for everybody on that instance is not a decision the platform meant to hand over — so on a managed instance the news now runs whatever any setting says, including a row left behind by an instance that used to be self-hosted. Capability::NewsConfigure, Community-only, and the thing it gates is the *choice* rather than the news. A self-hosted operator keeps the switch, because there nobody else decides what their installation reaches out for. An edition difference through the capability registry rather than an edition check, as everything here is. Gated in all three places rather than only the screen: the command ignores the setting without the capability, the controller neither sends nor reads the field, and the checkbox is absent. There is a test that a hand-crafted PATCH cannot do what the missing checkbox could not, and the guard is proved load-bearing — remove it and the managed-instance test goes red. The changelog and product highlights said "two switches" and now say what is actually true, including that neither appears on Cloud and why they are absent for opposite reasons. |
||
|
ignacionelson
|
d7d7acce85 |
Put the announcement behind the header icon too, from one source
A message worth showing was only on the dashboard, which means somebody who works in Files and Clients all day never meets it. It now also sits behind an icon next to the notification bell, and that is on every page. **One shared prop, not two.** "The same message in both places" is the requirement, and two props would have drifted the first time anybody edited one — so the hook moved out of DashboardController into HandleInertiaRequests, and the dashboard reads the same shared value the header does. The band and the dropdown also share the component that renders the words, for the same reason: the reliable way to keep two renderings identical is not to have two. Renamed with it. ResolvingDashboardCallout was accurate for about an hour and became a lie the moment it appeared somewhere else; it is ResolvingAnnouncement now, and the prop is `announcement`. Free to rename because nothing has shipped yet — the only other reference was cloud-modules', by string, updated alongside. The icon follows UpdateAvailableIcon beside it: absent entirely when there is nothing to say rather than a dead control, and a plain dot instead of a count, because there is only ever one of these and a "1" would invite somebody to look for the second. Two tests worth naming. One asserts the message reaches a page that is not the dashboard, which is the whole point of the addition. The other asserts a client is shown nothing even from a listener that sets it unconditionally — a client's header carries the bell too, and staff messages must not reach it however careless the listener. |
||
|
ignacionelson
|
334b11d562 |
Give packages a way into the sidebar and the top of the dashboard
Two seams, in the shape docs/extension-points-architecture.md settles on: a Laravel event with a mutable payload, dispatched unconditionally, and with nothing listening the documented default holds. A community installation gets an empty list and a null callout, which is exactly what it had before. ResolvingNavigationLinks exists because the sidebar is a hardcoded array in app-sidebar.tsx, so a package could not contribute to it at all — the nav entry was a separate manual edit every time a package grew a screen, and being manual it was forgotten more than once. Staff-only, decided in HandleInertiaRequests rather than trusted to each listener: these render in the administration area, and a client's portal shows their own files and nothing about the installation. There is a test that a listener adding unconditionally still reaches no client. ResolvingDashboardCallout is one band above the widget grid rather than a widget in it. The grid is a closed list of keys that dashboard.tsx renders one by one and each viewer arranges, so a message that mattered would sit wherever somebody dragged it, or under a fold, or switched off. One at a time, first listener wins: a dashboard that can accumulate banners accumulates them, and the second is what teaches people to skip the first. Core learns nothing about what either seam carries. Titles, URLs and copy all arrive from the listener, and that is not fastidiousness — the first caller is the hosted edition's link to its own customer portal and its pitch to free instances, which is commercial copy belonging to one offering and has no business sitting in the public repository because the sidebar happens to live here. An external link renders as a plain anchor opening in a new tab, never an Inertia <Link>: Link expects a page component back and another origin will not give it one, so it fails without saying so. It is also never marked active — nothing outside this app is the page you are on. |
||
|
ignacionelson
|
da1f432d87 |
Let an installation stop calling home, two different ways
Every instance reached projectsend.org twice a day and an operator could stop neither. The news feed had no switch of any kind — FetchNewsCommand went straight to the request, touching Settings only to write results back. The update check had one, but its default is on, and a managed fleet had been setting PROJECTSEND_CHECK_FOR_UPDATES=false for months against code that reads no such variable: check_for_updates is a database setting, so the environment never touched it and updates were enabled fleet-wide the whole time. They look like one problem and are two, which is why they are fixed differently. **The news feed gets a Setting**, its own key, default on. A Cloud client with view_news sees that card today — DashboardController gates it on the permission alone, with a comment saying in as many words that it is both editions and carries no capability. So switching it off is an operator's choice rather than an edition's, and it must stay reachable everywhere. Its own key rather than riding on check_for_updates because they are two different wants: "do not tell me about releases" and "do not show me the project's news" are asked separately, and an installation with no outbound access at all wants both. **The update check gets a capability guard**, ahead of the setting it already had, and deliberately not a Setting of its own. On a managed installation the result is unreachable rather than unwanted: the dashboard's System card and the update UI are both gated on Capability::SystemUpdates, which is Community-only, and the image is chosen by whoever provisioned the instance. A Setting would encode a fact about the edition as a preference — leaving it switchable back on per tenant, buying a nightly call for a number no screen can draw, and putting the reason in a provisioning script rather than beside the code. A self-hosted install holds the capability and loses nothing: its own setting still decides. Both guards return success rather than failure. A scheduled task that was asked not to run has not failed, and reporting it as one would put a red line in the scheduler history every night for an installation behaving exactly as configured. The news switch is on the General settings screen, outside the can_manage_updates block that hides the update toggle where the capability is absent — a setting only reachable by editing a database row is a row, not a switch. Seven tests, and the two that matter go red when either guard is removed. Sixteen locales translated in the same commit rather than left for the pass, since a release is close. |
||
|
ignacionelson
|
b758fca19c |
Merge pull request #1724 from fix/assemble-keeps-parts-for-retry
Keep an upload's parts until its bytes are stored |
||
|
ignacionelson
|
b7ac44e77b |
Merge pull request #1733 from fix/presigned-download-window
Give a download's presigned URL a minute rather than an hour Conflicted against FileDelivery, which landed on main after this branch was written: main added a constructor where the branch added two constants. Both belong; the resolution keeps each. |
||
|
ignacionelson
|
8de28059db |
Say when a folder choice publishes the file
Found reviewing the client file editor rather than building it. File::isEffectivelyPublic() is "my own flag OR my folder's", and Folder::uploadableBy() admits a client to a public folder on upload_to_public_folders — a different key from upload_public. So a client can make a file world-readable without touching the public switch, and without holding the key that switch is behind. That is what those two keys have always meant and what uploading into such a folder has always done, so this does not refuse it. What was new is where the choice is made. The upload page is entered from a folder the client has already navigated to, where the list shows a Globe badge on a public folder. The editor's picker is a flat list of names, and it is the first place a destination is chosen with none of that context — so the consequence was invisible exactly where it mattered most. Public folders now carry the badge in the picker, and choosing one says in words that anyone will be able to open the file without signing in. Two tests: that the side door genuinely publishes and is labelled, and that a private folder is not labelled — a warning on everything is a warning on nothing. The rest of the review found no defect. Ownership, the per-field keys, the staff-scope trap and mass assignment were already covered; a client deleting a file that staff later revised was checked directly and moves the chain's recipients onto the successor without widening them, which is what it is supposed to do. The write path was driven in a real browser — rename, publish and delete through the actual form and dialog — because a green suite over a write that 419s in every browser is a mistake this repository has made before. Bytes gone, audit trail complete, and file.made_public records the slug. |
||
|
ignacionelson
|
ea214fc27e |
Give the client portal a file editor
The authorization landed last commit; this is the way in. A client with edit_files now gets an Edit action on the files they uploaded, opening a form with every field their role actually grants, and a Delete beside it. One page for every theme, not one per theme. portal/edit-file.tsx picks its shell from the `theme` prop exactly as portal/upload.tsx does, because a form with eight fields behind five separate permissions, rebuilt four times, is four places for a field to go quietly missing. What *is* per-theme is only the entry point: one <FileRowActions /> in each theme's row actions group, the file twin of the FolderRowActions that was already there. Row actions gate on can_update/can_delete, sent per file by MyFilesController and answered by FilePolicy — never on is_mine, which is half the question. Holding the file is one half and the role's keys are the other, and a theme that reads is_mine offers an Edit button that 403s. Written into docs/theming-files-checklist.md so the next theme does not have to rediscover it. The folder picker offers only folders the client could have uploaded to, so it cannot present a destination the save would refuse. Publishing says in plain words that anyone with the link will be able to open the file without signing in, and says so differently when the installation has no public page configured, because there the switch would do nothing visible. Hiding a control is a courtesy, never the enforcement. Every can_* prop here is the same question ApplyFileEdits asks when the form posts, and the tests assert both ends. Verified in a real browser over CDP rather than only by types and tests, which say nothing about whether a page mounts: 23 edit actions on the client's 23 own files and none on the file shared with them, the editor mounting with its real values, every gated field present, no console errors. The dev instance's Client role was snapshotted before the run and restored to exactly what it was. Refs #1771 |
||
|
ignacionelson
|
922be7226c |
Let a client edit and delete the files they uploaded
A client could upload a file and then never touch it again. No rename, no description, no expiry, no categories, no delete — the portal has three file routes and all three are GET. Meanwhile the Roles screen happily grants the Client role edit_files, delete_files, set_file_categories, set_file_expiration_date and upload_public, and every one of them was inert, because the routes that honour them are `staff`-gated rather than permission-gated. That is what #1771 hit: a permission granted, saved, and silently doing nothing. A client owns what they uploaded. Ownership is now what lets them edit and delete it, subject to the same per-field keys staff are subject to. The obvious implementation is a trap, and it is worth writing down. Both policy methods began `if (! $user->isStaff()) return false;` and both end in StaffLibraryScope, whose allowsFile() reads `if (! isClientScoped()) return true` — and isClientScoped() is `isStaff() && role->client_scoped`, so it is false for every client. Delete the early return and a client falls into the branch meaning "this staff member is unrestricted" and is handed the whole library. Same for folders(), which returns an unfiltered query: a client could move their file into any folder on the installation. So clients get their own branch, reaching neither. The portal asks Folder::uploadableBy() instead — a file cannot be moved somewhere it could not have been uploaded. edit_others_files and delete_others_files stay inert for clients by construction. A client has no others' files, only files somebody showed them, and being shown a file is not being given it. Which fields an editor may write moved into ApplyFileEdits, shared by the staff editor, /api/v1 and the portal. There were two copies of the same eight permission checks and this would have been the third; the checks are easy, which is exactly why the drift would have been invisible. Callers normalise their own request shape, this gates and writes and logs. Expiry reading and writing came along too, as FileExpiry — three copies, of which only the API's could read a timestamp. Clients do not choose the public slug. It is derived from the name they already picked, because an installation-wide unique slug a client sets is a name to squat and an existence oracle to probe with. One consequence for later, written up in docs/api-todo.md: the policy now says yes to a client for file writes, so `staff-token` is the only thing holding the API boundary where there used to be two independent refusals. ActorBoundaryTest pins it, and asserts the policy passes first so the test cannot quietly stop testing the middleware. Also corrects a stale comment that claimed a deleted file's bytes stay on disk. They have not since File::booted() grew a `deleted` hook; nothing ever forceDelete()s a File row, so "until a purge lands" would have meant never — which is why a client's delete frees their quota by exactly what it frees on disk. The UI comes next; this is the authorization, the routes and the tests. Fixes #1771 |
||
|
ignacionelson
|
1e30e83f11 |
Stop projectsend:captcha-off claiming a success it did not have
The command writes Setting::CaptchaProvider = 'none'. On an installation using the platform's managed keys, Captcha::resolve() returns managedConfig() — read from config — before it ever looks at that setting, so the write lands somewhere nothing reads and every form stays protected. The command then printed "CAPTCHA is off". That is false in the worst direction: the person running this is locked out and debugging, and the message sends them away from the one thing that would have explained why they are still being challenged. It now says it changed nothing, and names PROJECTSEND_CAPTCHA_DISABLED, which is checked ahead of the key source and is therefore the only one of the two escape hatches that works on a managed installation. The docblock said those two were equivalent; they never were. Deliberately not gated behind captcha.configure. Gating it would take a self-hosted operator's way back in — the alternative being a hand-edited database row — to close something that on a managed installation does nothing anyway. Reaching it needs a shell in the container, which needs an RCE, at which point the CAPTCHA is not the problem. The command had no test at all. It has three now, including one that pins the ordering inside resolve(): if the environment check ever moves below the key source, a locked-out operator loses their last way in. |
||
|
ignacionelson
|
d32788e4a1 |
Put the CAPTCHA settings screen behind a capability
The screen is open in both editions and stays that way by default, so a self-hosted installation loses nothing: nobody else supplies its keys, and nobody else is affected by what it decides. What the key buys is the ability to take it away. A hosted fleet puts every tenant on one parent domain and one sending reputation, so an administrator who turns their own CAPTCHA off is spending everybody else's deliverability rather than only their own. That is not the shape LDAP and social login have, which is why those two stay ungated and this one does not. Gated all-or-nothing on the route, read included, exactly as Storage and Branding are. Per-field gating in the controller would not have closed it: switching the CAPTCHA off needs none of the gated fields — `provider: none` does it, and so does unticking the four per-form switches while leaving good keys in place — so the PATCH had to be closed too, and the middleware closes both verbs at once. Which keys the screen may offer is still the separate, narrower question Capability::CaptchaManagedKeys answers per field. An operator withdraws it by naming captcha.configure in PROJECTSEND_CAPABILITIES_DISABLED. Note that the key also joins the list `projectsend:status` and GET /api/v1/me report, which is additive — the OpenAPI document types capabilities as an untyped array, so nothing there needed regenerating. |
||
|
ignacionelson
|
7da4635f13 |
Say which clients a scoped staff member may be told about
A staff member limited to their own assigned clients could read the names
and ids of clients on nobody's roster but their own, out of ordinary file
metadata.
The file boundary was never wrong. Sharing means a file can legitimately
reach a scoped viewer through client A while client B uploaded it, or
while B also receives it -- StaffLibraryScope::buildFiles is right to
permit that, and a B-only file is still a 403. What was wrong is that
every response then went on to name B. FileResource serialised the loaded
uploader and each assignment unfiltered; ShareTargets::assigned took no
viewer at all, so the details panel published the recipient list as it
stands and forSubject narrowed available_clients while handing
assigned_clients straight through. FoldersController::fileRow,
FilesController::edit, FileDetailsController and ClientFilesController
each named the uploader the same way. The API's uploaded_by filter asked
the question without any name attached: it answered "does this client of
yours put files in front of a client of mine" for any id a caller cared
to try.
|
||
|
ignacionelson
|
97596da7d0 |
Refuse a stored path carrying a control character
Found reviewing the delivery work. The path is written into
X-Accel-Redirect or X-Sendfile, and a CR or LF in a header value is
header injection. PHP's header() refuses to emit one, so the real effect
is a 500 on every download, preview and thumbnail of that file rather
than a split response -- a file permanently broken by its own name.
Paths are generated here as Y/m/{uuid}.{ext}, so this should be
unreachable. The extension is not generated: it comes from the
uploader's filename, and on a migrated installation from a v1 database.
The upload routes all check the extension against an allowlist, which no
control character can match -- but upload_type_restriction can be set to
none, and the importer does not consult that policy at all.
assertRelative() was documented as the backstop for what a path may be
and only covered traversal, which is the half that cannot happen here.
Low severity, and the guard should have covered it either way.
|
||
|
ignacionelson
|
d6fd5a917d |
Send downloads the way the web server in front of us understands
Uploads live outside the web root, so PHP authorizes every download and then hands the file to the web server with a header naming it. Four routes decided that for themselves and all four hard-coded nginx's spelling. On Apache or LiteSpeed nothing acts on the header, so the empty body PHP sent goes to the visitor: files upload fine, thumbnails are broken images, and downloads arrive as 0 bytes, with every other page working. Reported as #1765 from an Apache 2.4 install, and before that as #1266, #1215, #870 and #1271. It is also a regression from v1, which had a download_method setting -- php, apache_xsendfile, litespeed, nginx_xaccel -- defaulting to php. v1 therefore worked on any server out of the box and v2 did not, and a v1 Apache user migrating lost every download with nothing to tell them why. So the four sites now go through one FileDelivery, and it picks: auto (default) nginx when SERVER_SOFTWARE says nginx, else php nginx X-Accel-Redirect, a URL path via the internal location xsendfile X-Sendfile, an absolute path (Apache mod_xsendfile, LiteSpeed) php BinaryFileResponse Defaulting to auto rather than nginx is the point of the change: a default that assumes nginx leaves an Apache install exactly as broken as it is today until somebody reads INSTALL.md. Slow beats empty. Auto never picks xsendfile, even where the module is loaded. mod_xsendfile also needs XSendFilePath to allow the storage directory, which cannot be seen from here, and choosing it on the strength of the module being present would trade a silent failure an administrator can diagnose from the dashboard for one nobody can. BinaryFileResponse rather than a readfile loop because it answers Range requests. nginx does that itself on the fast path, so hand-rolling it would have broken seeking through a video on exactly the installations this fallback exists for. Verified end to end: 206 with the right Content-Range through the live stack. Two guards. Every method checks the path cannot climb out of the storage area -- nginx resolves `..` in the URL it is handed as happily as PHP would -- and the two methods that hand over a filesystem path resolve it and prove it lands inside the root. Callers pass paths from rows they just authorized, so this is a backstop; it is here because the cost of being wrong once is handing over any file the web server can read. The dashboard's System panel names the method, with a warning icon and a dialog when PHP is doing the sending: what is happening, what it costs (one worker held for the whole of each download, so a few large simultaneous ones can occupy every worker while the processor sits idle), why it is set that way, and the three ways out. Written to be accurate rather than reassuring -- nothing is broken, it does not scale -- and the notice stays even when php was chosen deliberately, because the trade-off is the same either way. /system/settings/downloads repeats it, which is where somebody coming from v1 goes looking for the dropdown. An environment variable rather than a stored setting: it describes the server this installation runs on, not a preference, and a value in the database travels to a different server in a restore and is wrong there. Read only in config/projectsend.php, so config:cache cannot blank it. The suite pins itself to nginx. Left at auto it would detect no server at all, fall back to php, and quietly retire the coverage of the mechanism most installations actually use. |
||
|
ignacionelson
|
6340b71dca |
Report recent usage and whether the scheduler is alive
projectsend:status could say what an installation holds and how many accounts it has, but nothing about whether anybody was using it. Adds a `usage` block -- downloads split staff/clients/anonymous, uploads, and five allowlisted action counts -- plus `activity.last_client_login_at`, `health.scheduler` and `health.failed_jobs_latest_at`. The scheduler is the one worth having on its own. `health.queues` catches a dead worker; nothing caught a dead scheduler, and its first symptom is not a stalled feature but an expired file that is still downloadable, because the job that was going to remove it stopped running weeks ago. Nothing about the installation looks wrong while that is true. `failed_jobs_latest_at` exists because the count beside it cannot say whether anything is wrong *now*, and reading it as though it could is a category error rather than a threshold wanting tuning. The table is swept daily, so the count spans a retention window -- one the installation chooses, and one that can be set to keep-forever by somebody who treats a failed job as evidence rather than debris. Two identical installations therefore report different numbers, and on a keep-forever one the count grows until any fixed threshold trips. A timestamp is independent of how long rows are kept: 27 failures whose newest is three weeks old is an installation that has been healthy for three weeks and has not been swept yet. `usage` is a rolling window with no lifetime totals, and that is a correctness decision rather than a presentational one: activity_log is never pruned, so a lifetime count over it gets slower every day of the installation's life while a windowed one stays flat. The window is emitted as `window_days` rather than left for the reader to assume. The actions are an allowlist, not a `group by action`. This document leaves the installation and Action gains cases most weeks, so an open group-by would ship new action names outward with nobody having decided they should go -- and some of them (account.erased, two_factor.reset) are somebody's compliance event, not a business metric. It is also ~30x cheaper: five keyed counts ride (action, created_at) while a group-by starts from created_at and reads rows. The scheduler's failure message and the queue exception text are omitted for the same reason; they are the fields here that can carry a path or a stack trace, and a count with a timestamp says "go and look", which is all a watcher is owed. The two indexes ship as a pair and the migration explains at length why. Measured at 2.1M rows: adding (action, created_at) alone fixes the windowed counts and takes last_staff_login_at -- already running hourly on every tenant -- from 0.63s to 7.7s, because the planner switches to it, still needs actor_type, and does a scattered primary-key lookup per row. With both, that query is answered from the index without reading a row at all (0.0004s) and the whole new usage block costs ~70ms. Also documents the keys as a contract, the way `capabilities` already is. This one fails worse: a renamed capability key breaks a comparison somebody is watching, a renamed usage key produces a chart that is silently empty, and nobody gets paged for a flat line. |
||
|
ignacionelson
|
1a3260a397 |
Merge pull request #1758 from denkfabrik-li/fix/confirm-password-asks-the-directory
Let the confirm-password screen ask where the password lives |
||
|
ignacionelson
|
07e7132747 |
Merge pull request #1757 from denkfabrik-li/fix/dont-flash-stored-secrets
Stop a rejected settings form flashing the credential it carried |
||
|
ignacionelson
|
f4fd194991 |
Merge pull request #1756 from denkfabrik-li/fix/provider-link-password-confirm
Make linking a provider re-prove the password |
||
|
ignacionelson
|
ea45943f40 |
Merge pull request #1755 from denkfabrik-li/fix/credential-checks-rate-limited
Give every password check in front of an account its own bucket |
||
|
ignacionelson
|
ce96313710 |
Merge pull request #1754 from denkfabrik-li/fix/api-group-members-response-scope
Narrow the membership an API member write hands back |
||
|
ignacionelson
|
77dd5ff90b |
Merge pull request #1753 from denkfabrik-li/fix/account-conversion-list-scope
Narrow the conversion list to the clients its own refusal allows |
||
|
ignacionelson
|
8984aba7d8 |
Merge pull request #1752 from denkfabrik-li/fix/preference-writes-bounded
Bound the two preference endpoints by their own registries |
||
|
denkfabrik-li
|
3e24ccd42f |
Let the confirm-password screen ask where the password lives
ConfirmablePasswordController checked the local hash and nothing else:
Auth::guard('web')->validate(['email' => ..., 'password' => ...])
An account provisioned from a directory has no local password. It holds a
Str::password(64) generated at provisioning time that nobody has ever
seen, and the application knows this -- LdapAuthenticator::isDirectoryAccount()
is the question, and the sign-in form asks it before deciding what to
check. This screen did not, so it refused those accounts the only password
they have.
That is not a cosmetic refusal. `password.confirm` stands in front of
enrolling in two-factor, so a directory-provisioned client could not enrol
at all. Set TwoFactorEnforcement to `clients` or `all` and EnforceTwoFactor
redirects every request they make to two-factor.show -- a screen whose
"enable" button leads to a door they cannot open. PR #1708 fixed the
routing half of that ("Let an enforced user reach the far side of the
confirm-password screen"); this is the credential half.
The rule now lives in one place. PasswordVerification is the sibling of
SignIn on the other side of the line SignIn draws -- SignIn is everything
after a credential checks out, this is the one question asked before it --
and it exists for the reason SignIn gives for existing: "the way they get
broken is by being written twice". LoginRequest keeps its ordering, its
provisioning and its rate limiting, and delegates the check itself.
Behaviour preserved exactly on the sign-in path: local hash first so an
account that answers locally generates no directory traffic, directory
only for accounts whose credentials live there, the stale-hash re-hash on
the local branch only, and the ldap_dn stamp on the directory branch. All
23 existing LDAP sign-in tests pass unchanged.
One thing this closes on the way past. Because the old check went straight
to the local hash, a directory account's placeholder *would* have confirmed
if anybody ever learned it -- a door the sign-in form does not have, since
it skips the local branch for those accounts. It now behaves the same on
both screens; there is a test.
**What this does not fix, and should be read as a limitation.** Accounts
provisioned by a social provider are in the same position -- a random local
password nobody holds -- and they are not directory accounts, so this
changes nothing for them. Their route to a local password is the password
reset, which #1748 made work end to end by moving auth_source to Local when
the reset completes. A social account that has never done that still cannot
confirm a password, and so still cannot enrol in two-factor.
Tests: three fail against the unfixed pair, including the placeholder case
above. Two more pin what must not change -- a wrong directory password is
still refused, and a local account with LDAP switched on still confirms
against its own hash.
|