Skip to content

fix: close the upload and playground findings from the security audit, release v2.5.0 - #1556

Merged
joshunrau merged 10 commits into
DouglasNeuroInformatics:mainfrom
joshunrau:fixes
Sep 22, 2026
Merged

joshunrau merged 10 commits into
DouglasNeuroInformatics:mainfrom
joshunrau:fixes

Conversation

@joshunrau

@joshunrau joshunrau commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Closes three high-severity findings from the security audit, and cuts v2.5.0. The third landed after the release commit, so it ships in the next release rather than in v2.5.0.

POST /v1/instrument-records/upload returned every group's records (49fd5a9)

The upload service re-read its result by instrument id with the request's optional groupId as the only other filter. Prisma drops an undefined filter, so a body without groupId answered with every record for that instrument across every group, to any role holding create InstrumentRecord, including STANDARD, which cannot read records at all.

The response is now the rows keyed on the sessions the call itself created, which are the caller's own writes by construction. accessibleQuery is deliberately not AND-ed in: a STANDARD uploader holds no read InstrumentRecord rule, so CASL would throw rather than filter. The instrument lookup also forwards the caller's ability, as the single-record create path already does. The groupId field stays optional, since admins importing without a group is existing, tested behaviour.

A playground share link ran attacker JavaScript on the origin holding the API token (66c1425, 35e5ef1)

The playground evaluated every instrument, including one arriving in a share link, in its own window, the same origin whose IndexedDB persists the token the upload dialog obtains.

Previews now run in an iframe of a second Vite entry, preview.html, served from a different origin, and the editor and the frame talk only through postMessage payloads parsed against zod schemas. A hosted build names the preview origin with PLAYGROUND_PREVIEW_ORIGIN; a dev server uses the other loopback name, which is why the server now binds 127.0.0.1 so both answer. When no origin but the editor's own is available, the viewer refuses to render rather than fall back. Token persistence and share-link auto-load are unchanged by decision: the frame is the control.

The frame is a real second origin with allow-same-origin kept, not an opaque sandbox: interactive instruments render in a nested frame that reads parent.document, static assets need a service worker, and libui's theme hook reads localStorage, none of which an opaque origin permits. The follow-up commit collapses Tailwind's breakpoints in the frame's stylesheet so the preview keeps the desktop layout it had when the editor page's width satisfied them.

Deploy step outstanding for the hosted playground: point a second hostname at the same static build and build with PLAYGROUND_PREVIEW_ORIGIN set to it. Until then the hosted preview panel shows "Preview Unavailable" instead of running code on the editor's origin.

A User write grant let its holder make themselves an administrator (031e9e4, 6cc9ec3, 14d5bdc, d4be83f)

Creating, editing and deleting a user were gated on the matching User action. No base level grants one, but an admin can, and the holder could then set their own basePermissionLevel to ADMIN, add themselves to every group (updateById never checked groupIds), create an admin, or set an admin's password and log in as them. Scoping the grant to a group did not help: the scope is checked against the stored row, before the write.

  • POST, PATCH and DELETE /v1/users are now ADMIN_ONLY, a new name for { action: 'manage', subject: 'all' } that the seven existing admin-only routes now use too. A non-admin previously granted a User write loses it.
  • An admin can no longer delete, disable or demote their own account, so the last one cannot lock every admin-only route.
  • User writes are no longer grantable: PUT /v1/users/:id/permissions refuses them and the permissions editor stops offering them. Grants stored before this still load; the editor marks them "No effect" and drops them on its next save.
  • Choosing Manage (All) on All in the editor now warns that the grant makes the user an administrator.
  • The security reference no longer claims group managers can modify users.

Also on the branch

  • chore: release v2.5.0, whose changelog lists the first two fixes above.
  • The odc-open-pr and odc-commit skill docs.

Verification

  • Unit: apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts (result query keyed on created sessions; ability forwarded) and the new playground vitest project, apps/playground/src/preview/__tests__/protocol.test.ts (message schemas, error round trip, origin resolution). Both fail against the old code.
  • E2E: testing/src/specs/authorization.spec.ts seeds a record in a foreign group and asserts a STANDARD upload without groupId returns only its own row; red against the old service with the foreign row present. New testing/src/specs/playground.spec.ts starts the playground alongside the other servers and checks the origin split, that share-link code runs but cannot reach the editor page (red when the frame is pointed at the editor's origin), that an interactive instrument renders, and that submissions come back.
  • pnpm lint 34/34, pnpm test 152 files / 1337 tests, pnpm test:e2e 194/194, all from the repo root. Lint was re-run after the release commit; the full e2e run predates it.
  • User-escalation fix: unit tests in apps/api/src/users/__tests__/users.controller.spec.ts (every write route refuses a group manager granted manage User), users.service.spec.ts (the self-guard), the schemas and the editor; apps/api/test/suites/02-user-permissions.suite.ts writes a manage User grant directly and checks that all five escalation requests get 403. E2E in authorization.spec.ts (the grant is refused; an admin cannot delete or disable themselves) and admin-management.spec.ts (only Read is offered on User; the warning appears). Each went red against the code it guards. pnpm lint 34/34, pnpm test 152 files / 1371 tests, pnpm test:e2e 197/198: the failure was a gateway ECONNRESET in gateway-assignment.spec.ts, which passed 10/10 when re-run alone. The warning's final wording was checked by web lint and the editor's unit tests only.

Not verified: the hosted playground with a real second hostname (dev only exercises the loopback pair), and the static-assets interactive example inside the frame (the e2e covers the React one).

Co-Authored-By: Claude Fable 5.1
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Co-Authored-By: Claude Opus 5.5 (1M context)

joshunrau and others added 10 commits September 22, 2026 11:37
Records how a pull request is filed here: based on upstream main rather
than the fork's, and ending in one Co-Authored-By line per distinct
model across the branch's commits, so the models that touched a branch
are visible at review time.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
POST /v1/instrument-records/upload re-read its result by instrument with the
request's optional groupId as the only other filter. Prisma drops an undefined
filter, so a body without groupId answered with every record for that
instrument across every group, to any role holding create InstrumentRecord.

The response is now the rows keyed on the sessions this call created, which
are the caller's own writes by construction. accessibleQuery cannot scope
this read: a STANDARD uploader holds no read InstrumentRecord rule, so CASL
would throw rather than filter. The instrument lookup also forwards the
caller's ability, as the single-record create path already does.

Co-Authored-By: Claude Fable 5.1
The playground evaluated every instrument, including one arriving in a
share link, in its own window: the same origin whose IndexedDB holds the
API token the upload dialog obtained. Opening a crafted link ran
attacker-chosen code with that token in reach.

The preview is now an iframe of a second Vite entry, preview.html, served
from a different origin, and the editor and the frame speak only through
postMessage payloads parsed against zod schemas. A hosted build names the
preview origin with PLAYGROUND_PREVIEW_ORIGIN; a dev server uses the other
loopback name, which is why the server now binds 127.0.0.1 so both answer.
When no origin but the editor's own is available the viewer refuses to
render instead of falling back.

The frame keeps allow-same-origin on a real second origin rather than an
opaque sandbox: interactive instruments render in a nested frame that
reads parent.document, static assets need a service worker, and libui's
theme hook reads localStorage, none of which an opaque origin permits.

Adds a playground vitest project for the protocol, and a Playwright spec
that starts the playground and checks the origin split, that share-link
code runs but cannot reach the editor page, that an interactive
instrument still renders, and that submissions come back to the editor.

Co-Authored-By: Claude Fable 5.1
Tailwind's sm/md/lg variants are viewport media queries. Before the
preview moved into a frame, the editor page's width satisfied them even
in the narrow split-view panel; the frame's viewport is that panel, so
the same instrument switched to its phone layout, and the summary lost
its grid and its copy, download and print actions.

The frame now loads its own stylesheet, which imports react-core's and
collapses every breakpoint to 1px, so the preview renders an
instrument's desktop layout as it did before.

Co-Authored-By: Claude Fable 5.1
`{ action: 'manage', subject: 'all' }` was written out at every route only an administrator may
reach. `ADMIN_ONLY` names that intent, and its doc comment says why nothing narrower will do: the
guard checks the subject type alone, so a conditional rule or a granted additional permission
satisfies any other declaration.

Co-Authored-By: Claude Opus 5.5 (1M context)
Creating, editing and deleting a user were gated on the matching `User` action. No base level grants
one, but an administrator can, and the holder could then promote themselves to ADMIN, add themselves
to every group, create an administrator, or set an administrator's password and log in as them. The
three routes are now `ADMIN_ONLY`, like the permissions route beside them, so no grant reaches a
user's level, groups or password. A non-administrator who was granted a `User` write loses it.

Only administrators reach these routes now, so an administrator may no longer delete, disable or
demote their own account: the last one would otherwise lock every admin-only route for good.

Co-Authored-By: Claude Opus 5.5 (1M context)
Every route that writes a user is admin-only, so a grant of any `User` action but `read` reaches
nothing, yet the permissions editor still offered all four and the route stored them. The
permissions route now refuses such a grant, and the editor offers only Read once User is chosen
and leaves User out once a write is chosen.

Grants stored before this still load, since the read model is unchanged. The editor marks them
"No effect" and leaves them out of its next save, which the route would otherwise refuse. That a
stored one still reaches none of the routes it names is now checked in the api's integration
suite, which can write one directly; the e2e suite can no longer create one.

Co-Authored-By: Claude Opus 5.5 (1M context)
Choosing Manage (All) on All in the permissions editor now shows what that grant amounts to: every
group's data, every user's account and permissions, and instrument creation, which can run code on
the server. It is the one grant that passes every admin-only route.

Co-Authored-By: Claude Opus 5.5 (1M context)
@joshunrau
joshunrau merged commit fe09850 into DouglasNeuroInformatics:main Sep 22, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant