Reported by @skeletonsec as GHSA-rxf8-wh8v-jm9j.
A file in a public folder is public: isEffectivelyPublic() is "my own flag,
or my folder's", read up the whole ancestry. GHSA-237r-jx85-j3hr settled
that three days ago, put the rule in Folder::uploadableBy(), and wired it
into the upload paths.
Content arrives in a folder four other ways. move() drags one file in,
bulkUpdate() moves a selection, update() reparents through the edit form,
and FoldersController::move() drags a whole folder — every file in its
subtree — under a public parent. Each of them asked whether the destination
was *visible* to the mover and then wrote folder_id. Visible is not the same
question as publishable, and the difference is the entire permission: a
staff member given editing rights and deliberately not given upload_public
could publish confidential files to the anonymous site by choosing where
they landed. The API twin of update() had the same gap.
Both earlier advisories named these paths in their own "suggested fix"
sections. Neither demonstrated them, so neither was followed. The fix to a
report wants the scrutiny the report got, and this one did not get it.
The predicate did not need changing — it needed calling. Four sinks now ask
it, plus the API twin. The check stays split in two deliberately: the
destination is resolved through StaffLibraryScope as before, so a folder
somebody cannot see is still a 404 and not an existence oracle, and the
publication clause is a separate 403 on top. They agree by construction —
allowsFolder() is folders()->whereKey()->exists() — so nothing that used to
resolve can now fail the first half.
On the file paths the check fires only when folder_id actually changes,
which is the convention already there: re-saving a file that sits in a
folder out of the saver's scope must keep working. bulkUpdate() checks its
destination once instead, before the loop, because there is one destination
for the batch and if it publishes then no file in the batch may go.
Folder::uploadableBy()'s docblock now says to read the name as "may place
into", with why: the name is what made this easy to miss, and the next
folder_id or parent_id write will be written by somebody reading it.
Ten tests, one per sink with a private-destination control beside it, plus
an editor who *can* publish to show the boundary is about publishing and not
about moving. The last one follows the advisory's own chain to the end and
asserts the thing actually claimed — a stranger with no session, no token
and no assignment fetching the anonymous download URL. It returns 200 on the
code before this commit and 404 after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
Reported as GHSA-237r-jx85-j3hr.
A file is public if its own flag is on or its folder's is, so the upload
destination reaches the property `upload_public` guards without touching
the switch. A staff member allowed to upload but deliberately not allowed
to publish could publish to the anonymous public site by choosing where
the file landed.
No new key. `upload_to_public_folders` already exists, already appears on
every role's checkboxes, and already means exactly this on the client
branch of the same method — MyFilesController's picker calls it the
established meaning of the two keys. It was never asked of staff, so on a
staff role that checkbox did nothing at all: an unenforced permission, the
class this project audited and closed once already.
Effectively public rather than the folder's own flag, because the flag is
inherited down a subtree: a private folder inside a public one publishes
just the same, and a check on the folder's own column walks past it. There
is a test for that case specifically.
One place, because every upload path — the plain POST, the chunked flow,
the API and the client portal — already asks Folder::uploadableBy(). The
sibling report about the target folder not being scope-checked at all
(GHSA-56qr-cq56-qg66) was fixed in 2c2b86ff and is what put the scope
check on the line above this one.
Folder::uploadableBy() returned true for any staff member without looking
at the folder, on the strength of a comment saying staff had already
validated folder_id through FilesController's own flow. No upload path
did. FilesController::store() did not check the folder at all; the two
that called uploadableBy() — the API upload and the chunked upload the
browser actually posts to — called a guard that could only ever say yes.
A client-scoped staff member could therefore name any folder id and put
the file inside a subtree shared with somebody else's client, where
File::scopeVisibleToClient hands it over without an assignment row ever
being written. That is the boundary StaffLibraryScope's own docblock
claims to hold everywhere.
The staff branch now asks StaffLibraryScope::allowsFolder, which returns
true for unscoped staff, so nothing changes for them. The client branch
is untouched: a client is never client-scoped, and ownership or a public
folder opting into client uploads remains the whole of their rule.
The two folder pickers that fed those ids are narrowed the same way the
listings around them already are.
Client file sharing, rebuilt from the ground up: a private area per
client, resumable uploads, folders, groups and categories, sharing with
expiry dates and download limits, comments, file versions, an activity
log, a REST API, and sixteen languages.
This repository begins here. ProjectSend 2 was developed privately, and
that development history is not published — the previous generation
remains available, with its own history, at projectsend/legacy.
Free software under the GNU General Public License v2, or (at your
option) any later version.
ignacionelson