Repository navigation
Check on the server who may read what, and lock down the REST routes (reads) - #560
Merged
doomlab merged 3 commits intoOct 9, 2026
Merged
Conversation
…r's filter Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…uild-viewer, viewer-downloads) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage 2 of closing the server-side access gap found while tracing #539. Stacked on #559, which is stacked on #558: this targets #559's branch; retarget as each merges. Please review the rules below: this is a behaviour change on the server.
Problem
Most read queries took a filter (
where/include) straight from the browser, so any signed-in user could read any project's data, and byinclude-ing task logs could read other people's form responses and task chat. Two classes of thing were worse than a plain read:getUser/getUsersreturned the full user row, includinghashedPassword, for any user, to any signed-in user. Nothing in the app calls them (leftover scaffolding), so they are deleted./api/send-emailsent any message from STAPLE's address to anyone (an open mail relay);/api/build-viewerqueued build jobs for anyone;/api/viewer-downloadsput a URL parameter straight into a file path (a crafted id could read any.zipfile on the server).Rules
Read queries (38 files). A new
scopeReadQuerywrapper combines the browser's filter with the caller's own limit, so the filter can only narrow, andhideOthersResponsesblanks form responses (metadata) and task chat on any task log the caller may not read, wherever it was included from (a task's logs, a member's logs, a milestone's tasks, tags, the dashboard). Logs the caller may read are: managers' project logs, and logs assigned to the caller or their team. Status, approval and dates stay, so counts and summaries keep working.getProject,getProjectStats,getProjectData,getContributors, project-manager lookups,getUserProjectMemberIds, the three tag-by-project queriesgetMilestone,getTeamgetCommentsgetRoles/getRolegetForms/getForm/getFormDeploymentsgetInvitesgetUserWidgets/getProjectWidgetsgetUser/getUsersREST routes
send-email: must be signed in; sender must be STAPLE's address; one recipient who is the caller (profile/password notices), the helpdesk (feedback) or someone the caller invited to a project they manage (invitations).build-viewer: signed in and a manager of at least one project (the Summary page is manager-only in the UI).viewer-downloads(+head): signed in, and the job id may only contain letters, digits,_and-, and the path is checked to stay inside the builds folder.Not covered (follow-ups)
sendNotificationcan still notify arbitrary users (other mutations call it internally, so it needs splitting first).viewerWorker) trusts the body it is given; builds are not tied to a specific project or requester, so any manager could fetch another manager's finished summary if they knew its (random) job id.Testing
queryAccess.test.ts(51 tests): id-based queries refuse outsiders (and plain contributors where it isn't theirs), members are not refused, every filter-based query's database call carries the caller's limit alongside the browser's filter, and responses/chat pulled in throughincludeare blanked (manager sees all). 37 of them fail against the old queries.apiRoutesAccess.test.ts(9 tests): sign-in, allowed and refused emails, manager-only build, traversal ids. 6 fail against the old routes.getJsonSchema.test.tserrors), lint and the whole suite (269 tests) pass.🤖 Generated with Claude Code