9 Commits

Author	SHA1	Message	Date
ignacionelson	5f414c7a4a	Give the API the same file filters the library screen has ... The staff library grew filters for uploader, role, public/private, download count and version. Two of those already existed on /api/v1/files (`uploaded_by`, `public`); the other four did not, so an integration could not ask what the screen asks. Adds `role_id`, `downloads=none|any`, `version=current|outdated` and `visibility=public|private`. `visibility` rather than changing `public`, and that is the decision worth explaining. `public` has always tested the file's own column, and callers depend on that answer; changing what an existing filter means is breaking for everyone already sending it, however much better the new meaning is. So `public` is untouched and `visibility` is added beside it with the application's own definition -- File::isEffectivelyPublic(), the flag or a public folder anywhere above the file -- which is what the badge on a staff row means. The guide says in a sentence which to reach for. Point the visibility filter at the column instead and the test that separates them fails, which is the whole point of having both. That predicate now lives once, as File::scopeEffectivelyPublic(), beside the isEffectivelyPublic() it has to agree with. It was a private helper on FoldersController until a second surface wanted it. `role_id` deliberately carries no identity guard, unlike `uploaded_by` beside it. A role names nobody: the files in the result are ones the caller may already read, and learning one came from somebody holding the Client role narrows to a set they could have guessed. `uploaded_by` is different in kind -- a non-empty answer confirms exactly the identity the response is redacting -- which is why only it is guarded. The reasoning is in the code, because an absent guard sitting next to a present one is the kind of thing a reader should not have to re-derive. Tests cover each filter, the public/visibility split, and the client-scoped negative: every new filter still returns nothing outside the token's own library, because a filter narrows a library and never widens one.	2026-09-16 14:11:06 -03:00
ignacionelson	5966d22f50	Add five ways to narrow the file library ... Search and the category dropdown were the whole filter bar, which is thin for a library of any size. It now also narrows by: - who uploaded the file, and separately by the role they hold - public or private - never downloaded, or downloaded at least once - current version, or outdated Each one forces the same flat, whole-library view search already used, and they combine. Two decisions worth naming. "Public" means what the badge on the row means -- File::isEffectivelyPublic(), the file's own flag or a public folder anywhere above it. Filtering on the `public` column alone would have hidden files this very screen labels Public, which is a filter arguing with the list it filters. The private half needs its own null branch, because `folder_id NOT IN (...)` is never true for a NULL folder_id: without it a file at the library root belonged to neither half and vanished from both. Removing that branch turns the private filter from one row to zero, which is the test. The uploader filter carries the same guard /api/v1/files puts on `uploaded_by`. fileRow() already withholds an uploader's name from a viewer who may not identify them, so answering this filter plainly would have handed the same identity straight back as a row count. An id the caller may not identify now matches nothing, which is indistinguishable from someone who uploaded nothing, and the dropdown is built through filterClientPairs so it never offers the name either. Without the guard the scoped-staff test gets its stranger's file back. "Outdated" rather than "superseded" throughout, because that is the word the version badge already uses and the two should not disagree. A file nothing has replaced counts as current, including one never versioned at all. The ids are cast out of the validated input: `integer` validates "5" without converting it, and permitsClientId() takes a strict ?int.	2026-09-16 13:04:10 -03:00
ignacionelson	50f8b578df	Ask the publication question wherever content lands, not just on upload ... Reported by @skeletonsec as GHSA-rxf8-wh8v-jm9j. A file in a public folder is public: isEffectivelyPublic() is "my own flag, or my folder's", read up the whole ancestry. GHSA-237r-jx85-j3hr settled that three days ago, put the rule in Folder::uploadableBy(), and wired it into the upload paths. Content arrives in a folder four other ways. move() drags one file in, bulkUpdate() moves a selection, update() reparents through the edit form, and FoldersController::move() drags a whole folder — every file in its subtree — under a public parent. Each of them asked whether the destination was *visible* to the mover and then wrote folder_id. Visible is not the same question as publishable, and the difference is the entire permission: a staff member given editing rights and deliberately not given upload_public could publish confidential files to the anonymous site by choosing where they landed. The API twin of update() had the same gap. Both earlier advisories named these paths in their own "suggested fix" sections. Neither demonstrated them, so neither was followed. The fix to a report wants the scrutiny the report got, and this one did not get it. The predicate did not need changing — it needed calling. Four sinks now ask it, plus the API twin. The check stays split in two deliberately: the destination is resolved through StaffLibraryScope as before, so a folder somebody cannot see is still a 404 and not an existence oracle, and the publication clause is a separate 403 on top. They agree by construction — allowsFolder() is folders()->whereKey()->exists() — so nothing that used to resolve can now fail the first half. On the file paths the check fires only when folder_id actually changes, which is the convention already there: re-saving a file that sits in a folder out of the saver's scope must keep working. bulkUpdate() checks its destination once instead, before the loop, because there is one destination for the batch and if it publishes then no file in the batch may go. Folder::uploadableBy()'s docblock now says to read the name as "may place into", with why: the name is what made this easy to miss, and the next folder_id or parent_id write will be written by somebody reading it. Ten tests, one per sink with a private-destination control beside it, plus an editor who *can* publish to show the boundary is about publishing and not about moving. The last one follows the advisory's own chain to the end and asserts the thing actually claimed — a stranger with no session, no token and no assignment fetching the anonymous download URL. It returns 200 on the code before this commit and 404 after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx	2026-09-11 12:16:23 -03:00
ignacionelson	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. 12a8ebe3 said the rule out loud while fixing topClientsByStorage -- "the file was theirs to read and the uploader's name was not theirs to see" -- and then the rule stayed in that widget. So it is a class now. ClientIdentityScope is the one decision, asked by every surface that names a client, and it deliberately answers about clients only: a colleague's name is not a client identity, and hiding it would hide who uploaded most of the library from the people who work in it. Groups go through it too, on the same argument -- a group is a list of clients wearing one name -- which the report did not cover but is the same leak. Two judgement calls worth naming. assigned() keeps returning the whole truth and gains a warning, because VisibleCommentScope resolves notification recipients from it and a recipient filtered out of that list is one who never hears about a message addressed to them; assignedFor() is the display half. And FileResource asks at serialisation rather than in its callers' eager loads, which is the opposite of how the version counterparts next door are narrowed: that one is set-shaped and folds into a query, this one is a per-row roster check across eight call sites in four controllers, two of them re-loading assignments after a write. The tests assert on whole response bodies rather than on named keys. The leak was never in one field -- the same name arrived through the uploader, through the recipient list and through four screens -- so a body that does not contain the name anywhere is the only assertion that would have caught all of it. Ten of the eighteen fail without this change; the rest are the negative controls, including that an unscoped administrator still sees every name and that the uploaded_by filter still works for a client on the roster and for staff. Reported by @Noorkhalel, GHSA-whmp-p9hv-r7j7. Their write-up named every affected surface and the root cause in each, which is most of why this took one pass.	2026-09-03 00:56:41 -03:00
denkfabrik-li	26205082c2	Stop a folder deleting the files inside it that its owner may not delete ... FoldersController::destroy() authorizes `delete` on the folder and nothing else. FolderService::delete() then soft-deletes every file in the subtree, and File::booted()'s `deleted` hook takes the bytes off disk. There is no restore. FilePolicy::delete asks two questions the folder route never reaches: `delete_others_files` for somebody else's upload, and StaffLibraryScope::allowsFile on top of it. Measured with a role holding create_own_folders, delete_files, upload and edit_files -- the shape the Client Manager system role already has, minus delete_others_files: DELETE /files/{someone-elses} 403, the file is still there DELETE /folders/{their-folder} 302, the file and its bytes are gone MyFoldersController::destroy already refuses the client half of this exact cascade, and says why: "Owning the folder is not authority over content someone else put in it... Refuse rather than silently destroy them." This is the staff half of the same sentence. Counted rather than asked per file. A folder can hold thousands, Gate resolves a fresh policy for every check, and a per-row policy check on a listing is the cost 0a8b609e went to some trouble to remove. Both halves of FilePolicy::delete are expressible in SQL: the permission half is constant for the viewer, and the library half is the query StaffLibraryScope already memoises per request. Somebody holding both delete permissions with no library scope short-circuits before the query runs at all, so the common case pays nothing. Not changed, deliberately: - The service. FolderService::delete stays dumb. Its other caller applies the client rule ("files you did not upload"), which is a different predicate, and putting both in one place is the drift this codebase keeps refactoring away from. - The client half. MyFoldersController is already correct. - Nothing partial. A blocked folder is left whole rather than emptied of what the actor may delete -- half a tree is worse than either answer. Worth saying plainly: this is a behaviour change. A folder delete that used to succeed now refuses, and somebody will notice. The alternative is irreversible loss of files the same person is refused one route over. Six tests. Four measured red against the unguarded controller (4 failed / 2 passed), one per half of the predicate: the permission half, its message, a nested file, and the library half -- that last one with both delete permissions held, so only StaffLibraryScope can refuse. The two that stay green either way are the other side of the question -- that a folder holding only your own files still goes, and that an administrator holding both permissions is unaffected. They guard against the fix over-refusing, not against the bug. Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean. The new string is English only, per CONTRIBUTING.md -- translations are their own pass.	2026-08-28 00:32:33 +02:00
Ignacio Nelson	f12692520a	Merge pull request #1694 from denkfabrik-li/fix/upload-folder-library-scope ... Hold the folder an upload names to the same library boundary as everything else	2026-08-26 22:34:14 -03:00
denkfabrik-li	7727ad7616	Say what exists:folders,id was already being read as ... Folder uses SoftDeletes. The `exists` rule runs against the table, so a folder in the trash passes it -- while every resolution that follows goes through Folder::query(), which honours the soft delete and finds nothing. Ten rules across five controllers rely on that check, and each one reads it as "this folder exists". Two of them then wrote the id anyway. Api\FilesController::store() resolves the folder, hands the null to Folder::uploadableBy(), is told yes -- correctly, that is the rule for a root upload -- and passes $validated['folder_id'] to the write. FilesController::store() is the same shape once #1694 gives it the guard. FilesController::update() and its API twin write it straight through with nothing in between. The result is a live file inside a deleted folder, which is a state nothing else in the application produces: FolderService::delete() deletes every file in the subtree along with it. The row is reachable by id, in search and over the API, and missing from the listing its uploader would look in. Rules::folderId() makes the check mean what its readers assume, once, where the reasoning can be written down -- the same argument slug() makes for itself one method above. Every site takes it, so the file cannot end up with two spellings of the same rule and no way to tell which is the safe one. What changes, path by path: - POST /files, POST /api/v1/files, PATCH /files/{file} and PATCH /api/v1/files/{file} refuse a folder in the trash instead of writing its id. This is the fix. - POST /uploads used to accept it and quietly file the upload at the root -- its guard and its write already agreed, on null. It now says so instead, which is what the other upload paths do. - files/{file}/move, files/bulk-edit, folders, folders/{folder}/move and the portal's my-folders already refused, through StaffLibraryScope::folders() or Folder::scopeVisibleToClient(), both of which drop trashed rows. They still refuse; the answer is now 422 naming folder_id rather than a bare 404. Those two guards are asking a different question -- "is this folder yours" -- and they keep asking it. No live folder id behaves differently anywhere, and the root (a null folder_id) is untouched. The published API document is unchanged: `exists` renders the same either way. Regenerated with php artisan scramble:export and byte-identical.	2026-08-26 06:01:06 +02:00
denkfabrik-li	2c2b86ffa1	Hold the folder an upload names to the same library boundary as everything else ... 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.	2026-08-26 04:01:25 +02:00
ignacionelson	6e47d76ba6	ProjectSend 2.0.0 ... 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.	2026-08-14 01:38:12 -03:00