From 11edc3fd4a9a66cba8a0204778291f6281b3fae3 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Mon, 20 Jul 2026 14:57:55 -0400 Subject: [PATCH 1/3] fix(api): resolve the instrument subject filter without a relation join `GET /v1/instruments/info?subjectId=` returns HTTP 500, and the datahub subject view stops rendering entirely, once a single instrument accumulates roughly 182,000 records. `InstrumentsService.find` expressed "instruments this subject has records for" as a prisma relation filter: records: { some: { subjectId } } On mongodb prisma compiles that into a $lookup which materialises *every* record belonging to an instrument into one array before applying the predicate, and mongodb caps a single document's $lookup output at 100 MiB (error 4568). The limit cannot be raised and allowDiskUse does not apply to it. Because the threshold is per-instrument, the busiest instrument hits it first -- a core intake instrument administered at every visit reaches 182,000 records at 5,000 subjects x 36 visits. `useInstrumentVisualization` calls this endpoint on mount and it backs both /datahub/$subjectId/table and /datahub/$subjectId/graph, so there is no partial degradation and no workaround available to the user. Query the child collection instead, which is bounded by the subject's own records rather than the instrument's, and has no size ceiling. The record lookup carries the caller's ability, so the filter is resolved only from records they may read. Without it the subject filter would be answered from every group's records -- disclosing which instruments a subject has been administered outside the caller's groups -- which the relation filter it replaces did not do. Verified against a live instance with one instrument at 105.7 MiB: main HTTP 500 (three times), "exceeds 104857600 bytes" branch HTTP 200 in 77ms, 811 bytes And below the ceiling, where main still answers, on the same 55.3 MiB instrument: main 1.66-1.83s branch 0.04-0.10s Fixes #1416 Co-Authored-By: Claude Opus 4.8 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc --- .../__tests__/instruments.service.spec.ts | 58 +++++++++++++++++++ .../src/instruments/instruments.service.ts | 37 +++++++++--- 2 files changed, 86 insertions(+), 9 deletions(-) diff --git a/apps/api/src/instruments/__tests__/instruments.service.spec.ts b/apps/api/src/instruments/__tests__/instruments.service.spec.ts index b15da3da6..368b59273 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.spec.ts +++ b/apps/api/src/instruments/__tests__/instruments.service.spec.ts @@ -9,6 +9,8 @@ import type { SeriesInstrument } from '@opendatacapture/runtime-core'; import type { WithID } from '@opendatacapture/schemas/core'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; + import { InstrumentsService } from '../instruments.service'; import type { InstrumentVirtualizationContext } from '../instruments.service'; @@ -644,4 +646,60 @@ describe('InstrumentsService', () => { ]); }); }); + + describe('find', () => { + beforeEach(() => { + instrumentModel.findMany.mockResolvedValue([]); + instrumentRecordModel.findMany.mockResolvedValue([]); + vi.spyOn(instrumentsService as any, 'instantiate').mockResolvedValue([]); + }); + + it('should not query records when no subject is given, so the unfiltered listing costs one query', async () => { + await instrumentsService.find(); + expect(instrumentRecordModel.findMany).not.toHaveBeenCalled(); + }); + + // A `records: { some: ... }` relation filter compiles to a $lookup that materialises every + // record belonging to an instrument, which mongodb aborts past 100 MiB. The subject's own + // records are queried instead, so the work is bounded by the subject rather than the instrument. + it('should resolve the subject filter against records rather than joining from instruments', async () => { + instrumentRecordModel.findMany.mockResolvedValueOnce([{ instrumentId: 'id-1' }, { instrumentId: 'id-2' }] as any); + + await instrumentsService.find({ subjectId: 'subject-1' }); + + expect(instrumentRecordModel.findMany).toHaveBeenCalledWith({ + distinct: ['instrumentId'], + select: { instrumentId: true }, + where: { AND: [{}, { subjectId: 'subject-1' }] } + }); + expect(instrumentModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: { AND: expect.arrayContaining([{ id: { in: ['id-1', 'id-2'] } }]) } + }); + expect(JSON.stringify(instrumentModel.findMany.mock.lastCall?.[0])).not.toContain('records'); + }); + + it('should constrain the record lookup to what the caller may read, so the filter cannot be resolved from other groups records', async () => { + // Conditions are what make this meaningful: an unconditional read rule yields `{}`, which is + // indistinguishable from the ability never having been applied. + const ability = createAppAbility([ + { action: 'read', conditions: { groupId: { in: ['group-1'] } }, subject: 'InstrumentRecord' }, + { action: 'read', subject: 'Instrument' } + ]); + + await instrumentsService.find({ subjectId: 'subject-1' }, { ability }); + + const [call] = instrumentRecordModel.findMany.mock.lastCall as [{ where: { AND: unknown[] } }]; + expect(call.where.AND[0]).toStrictEqual(accessibleQuery(ability, 'read', 'InstrumentRecord')); + }); + + it('should return nothing when the subject has no records', async () => { + instrumentRecordModel.findMany.mockResolvedValueOnce([]); + + await instrumentsService.find({ subjectId: 'subject-with-no-records' }); + + expect(instrumentModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: { AND: expect.arrayContaining([{ id: { in: [] } }]) } + }); + }); + }); }); diff --git a/apps/api/src/instruments/instruments.service.ts b/apps/api/src/instruments/instruments.service.ts index d81c84423..93818d458 100644 --- a/apps/api/src/instruments/instruments.service.ts +++ b/apps/api/src/instruments/instruments.service.ts @@ -36,6 +36,7 @@ import type { import { pick } from 'lodash-es'; import { accessibleQuery } from '@/auth/ability.utils'; +import type { AppAbility } from '@/auth/auth.types'; import type { EntityOperationOptions } from '@/core/types'; import { CreateInstrumentDto } from './dto/create-instrument.dto'; @@ -261,18 +262,20 @@ export class InstrumentsService { { ability }: EntityOperationOptions = {}, groupIds?: string[] ): Promise>[]> { + // Resolved against InstrumentRecord rather than expressed as a `records: { some: ... }` relation + // filter. On mongodb prisma compiles that filter into a $lookup which materialises *every* record + // belonging to an instrument into one array before applying the predicate, and mongodb caps a + // single document's $lookup output at 100 MiB. A busy instrument therefore fails the whole query + // outright once it passes that threshold. Querying the child collection is bounded by the + // subject's own records instead of the instrument's. + const subjectInstrumentIds = query.subjectId + ? await this.findInstrumentIdsBySubject(query.subjectId, ability) + : null; + const instruments = await this.instrumentModel.findMany({ where: { AND: [ - { - records: query.subjectId - ? { - some: { - subjectId: query.subjectId - } - } - : undefined - }, + subjectInstrumentIds ? { id: { in: subjectInstrumentIds } } : {}, query.seriesGroupId ? { OR: [ @@ -520,6 +523,22 @@ export class InstrumentsService { >; } + /** + * The ids of the instruments the subject has at least one record for. + * + * Scoped to the records the caller may read, so the filter cannot be resolved from records outside + * their groups — the ids feed straight into the instrument query, so an unscoped lookup would + * disclose which instruments a subject has been administered elsewhere. + */ + private async findInstrumentIdsBySubject(subjectId: string, ability?: AppAbility): Promise { + const records = await this.instrumentRecordModel.findMany({ + distinct: ['instrumentId'], + select: { instrumentId: true }, + where: { AND: [accessibleQuery(ability, 'read', 'InstrumentRecord'), { subjectId }] } + }); + return records.map((record) => record.instrumentId); + } + /** * The title of an existing series instrument that contains the exact same ordered sequence of items as * `items`, or `null` when none exists. Used to warn before creating a redundant series. From bddd6c930cb3ffbda4364a14ac5f14a295b327d3 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 13:23:54 -0400 Subject: [PATCH 2/3] fix(api): answer the subject filter for a caller who may read no records Scoping the record lookup to the caller introduced a new failure on the endpoint this branch set out to fix. `accessibleQuery` does not return an empty filter when the ability holds no rule for the subject at all -- only an `undefined` ability does that -- it throws a CASL ForbiddenError, which is not an HttpException, so libnest's filter renders it a 500. A STANDARD user holds `create` but not `read` on InstrumentRecord, and `GET /v1/instruments/info` is gated on `read Instrument`, which they do hold. So a standard user passing `subjectId` got an Internal Server Error where main returns 200. Return an empty id list for such a caller instead: no readable records means no instruments qualify. The scoping itself stays, since resolving the filter from records the caller cannot read is the disclosure this branch is closing. Pinned at both tiers, each verified by mutation: - a unit test building the ability through `AbilityFactory` with a STANDARD payload, rather than by hand -- a hand-built ability tends to carry a `read InstrumentRecord` rule, and it is the absence of one that breaks - an e2e case in `authorization.spec.ts` asserting the endpoint answers 200, which returns 500 without the guard The at-scale reproduction cannot be expressed at the e2e tier; that gap is noted in the spec rather than left silent. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc --- .../__tests__/instruments.service.spec.ts | 20 ++++++++++ .../src/instruments/instruments.service.ts | 7 ++++ testing/src/specs/authorization.spec.ts | 38 +++++++++++++++++++ 3 files changed, 65 insertions(+) diff --git a/apps/api/src/instruments/__tests__/instruments.service.spec.ts b/apps/api/src/instruments/__tests__/instruments.service.spec.ts index 368b59273..eb6d878c5 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.spec.ts +++ b/apps/api/src/instruments/__tests__/instruments.service.spec.ts @@ -9,6 +9,7 @@ import type { SeriesInstrument } from '@opendatacapture/runtime-core'; import type { WithID } from '@opendatacapture/schemas/core'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { AbilityFactory } from '@/auth/ability.factory'; import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; import { InstrumentsService } from '../instruments.service'; @@ -692,6 +693,25 @@ describe('InstrumentsService', () => { expect(call.where.AND[0]).toStrictEqual(accessibleQuery(ability, 'read', 'InstrumentRecord')); }); + // Built through the factory rather than by hand: a hand-built ability tends to include a + // `read InstrumentRecord` rule, and it is the absence of one that breaks. A STANDARD user holds + // `create` but not `read`, and this route is gated on `read Instrument`, which they do hold. + it('should resolve for a caller who may read no records, rather than failing the request', async () => { + const abilityFactory = new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService); + const ability = abilityFactory.createForPayload({ + basePermissionLevel: 'STANDARD', + groups: [{ id: 'group-1' }], + id: 'user-1' + } as any); + + await expect(instrumentsService.find({ subjectId: 'subject-1' }, { ability })).resolves.toStrictEqual([]); + + expect(instrumentRecordModel.findMany).not.toHaveBeenCalled(); + expect(instrumentModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: { AND: expect.arrayContaining([{ id: { in: [] } }]) } + }); + }); + it('should return nothing when the subject has no records', async () => { instrumentRecordModel.findMany.mockResolvedValueOnce([]); diff --git a/apps/api/src/instruments/instruments.service.ts b/apps/api/src/instruments/instruments.service.ts index 93818d458..6be89fdcc 100644 --- a/apps/api/src/instruments/instruments.service.ts +++ b/apps/api/src/instruments/instruments.service.ts @@ -531,6 +531,13 @@ export class InstrumentsService { * disclose which instruments a subject has been administered elsewhere. */ private async findInstrumentIdsBySubject(subjectId: string, ability?: AppAbility): Promise { + // `accessibleQuery` throws rather than returning a restrictive filter when the ability holds no + // rule for the subject at all, and a STANDARD user holds `create` but not `read` on + // InstrumentRecord. This route is gated on `read Instrument`, which they do hold, so they reach + // here; no readable records means no instruments qualify. + if (ability && !ability.can('read', 'InstrumentRecord')) { + return []; + } const records = await this.instrumentRecordModel.findMany({ distinct: ['instrumentId'], select: { instrumentId: true }, diff --git a/testing/src/specs/authorization.spec.ts b/testing/src/specs/authorization.spec.ts index 90ceae4a3..1af142659 100644 --- a/testing/src/specs/authorization.spec.ts +++ b/testing/src/specs/authorization.spec.ts @@ -39,5 +39,43 @@ test.describe('authorization', () => { await page.goto('/session/remote-assignment'); await expect(page).toHaveURL('/session/start-session'); }); + + // `GET /v1/instruments/info` is gated on `read Instrument`, which a standard user holds, but + // resolving `subjectId` reads instrument records, which they do not. Scoping that lookup to the + // caller must answer with an empty list rather than failing the request. + // + // The defect this endpoint was changed for -- a mongodb $lookup exceeding its 100 MiB per-document + // ceiling once one instrument holds ~182,000 records -- cannot be reproduced at this tier. That + // gap is deliberate; the evidence for it is in the PR, measured against a live instance. + test('should answer the subject-filtered instrument list for a caller who may read no records', async ({ + apiRequestContext, + roleAccount + }) => { + const { accessToken } = await roleAccount('STANDARD'); + + const response = await apiRequestContext.get('/api/v1/instruments/info?subjectId=any-subject', { + headers: { Authorization: `Bearer ${accessToken}` } + }); + + expect(response.status()).toBe(200); + expect(await response.json()).toStrictEqual([]); + }); + }); + + // The populated case -- a group manager reaching /datahub/$subjectId/table and picking an + // instrument from the list -- is covered end to end by `instrument-completion.spec.ts`, which + // administers one first so the list has something in it. + test('should serve the subject-filtered instrument list to a group manager', async ({ + apiRequestContext, + roleAccount + }) => { + const { accessToken } = await roleAccount('GROUP_MANAGER'); + + const response = await apiRequestContext.get('/api/v1/instruments/info?subjectId=any-subject', { + headers: { Authorization: `Bearer ${accessToken}` } + }); + + expect(response.status()).toBe(200); + expect(await response.json()).toStrictEqual([]); }); }); From 061e7a3654730665e4a0f4dc6dbeae0ea14cc0de Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 18:11:11 -0400 Subject: [PATCH 3/3] docs(api): describe the no-rule ForbiddenError beside the undefined-ability rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit accessibleQuery's other failure mode was documented nowhere: a defined ability with no rule for the queried subject throws CASL's ForbiddenError, which is not an HttpException and so renders as a 500. The guard in findInstrumentIdsBySubject exists solely because of it, and the decision to keep the throw (per-call-site guards, no deny-all filter) is now recorded. The group-manager e2e case is renamed to the contract it actually asserts — an empty list for a subject with no visible records, rather than an error. Co-Authored-By: Claude Fable 5 --- .agents/docs/architecture/auth-and-permissions.md | 10 ++++++++++ apps/api/AGENTS.md | 8 ++++++++ testing/src/specs/authorization.spec.ts | 5 +++-- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/.agents/docs/architecture/auth-and-permissions.md b/.agents/docs/architecture/auth-and-permissions.md index e67b57f18..4723d5275 100644 --- a/.agents/docs/architecture/auth-and-permissions.md +++ b/.agents/docs/architecture/auth-and-permissions.md @@ -70,6 +70,16 @@ available in this codebase. Check the `where` clause of every query you add or t that the controller actually forwards `@CurrentUser('ability')` — `EntityOperationOptions.ability` is optional, so a controller that simply never passes it compiles and runs. +**A defined ability holding no rule for the queried subject does not produce a deny-all filter — +CASL's `accessibleBy` throws `ForbiddenError`.** That error is not an `HttpException`, so the +global exception filter renders it as a 500. It is reachable whenever the route guard names a +different subject than the service queries: a STANDARD user passes a `read Instrument` guard but +holds no rule on `InstrumentRecord`, so a record query made on their behalf throws. The throw is +deliberate — `accessibleQuery` is kept as-is rather than returning a deny-all filter — so where +such a caller is legitimate, guard the query with `ability.can(action, subject)` and return the +empty result instead; `findInstrumentIdsBySubject` in `src/instruments/instruments.service.ts` is +the reference example. + One call site uses `{ ...accessibleQuery(...), id: subjectId }` instead of `AND: [...]` (`src/subjects/subjects.service.ts`). Spreading merges keys and will silently lose a condition if the generated query and the literal share one. Prefer `AND`. diff --git a/apps/api/AGENTS.md b/apps/api/AGENTS.md index 1b81f53f0..f3d1ac5b0 100644 --- a/apps/api/AGENTS.md +++ b/apps/api/AGENTS.md @@ -83,6 +83,14 @@ restriction**, which reads clinical data across every group. Nothing catches thi eslint, not a passing test suite. It is the single highest-severity mistake available in this codebase, so verify the `where` clause of any query you add or edit. +The defined case has the opposite failure mode: **an ability holding no rule at all for the queried +subject makes `accessibleQuery` throw CASL's `ForbiddenError`**, which is not an `HttpException` +and so surfaces as a 500. This is reachable whenever a route's guard names one subject but the +service queries another — a STANDARD user passes `read Instrument` yet holds no `read` rule on +`InstrumentRecord`. That behaviour is deliberate and stays; where such a caller is legitimate, +short-circuit before the query with `ability.can(action, subject)` and return the empty result — +see `findInstrumentIdsBySubject` in `src/instruments/instruments.service.ts`. + Granting a new `action`/`subject` pair means editing `src/auth/ability.factory.ts` and adding tests for **both** the allow and the deny case — see `src/auth/__tests__/ability.factory.test.ts`. diff --git a/testing/src/specs/authorization.spec.ts b/testing/src/specs/authorization.spec.ts index 1af142659..ad2f0ec76 100644 --- a/testing/src/specs/authorization.spec.ts +++ b/testing/src/specs/authorization.spec.ts @@ -64,8 +64,9 @@ test.describe('authorization', () => { // The populated case -- a group manager reaching /datahub/$subjectId/table and picking an // instrument from the list -- is covered end to end by `instrument-completion.spec.ts`, which - // administers one first so the list has something in it. - test('should serve the subject-filtered instrument list to a group manager', async ({ + // administers one first so the list has something in it. This case only pins the contract for a + // subject with no visible records: an empty list, not an error. + test('should answer the subject-filtered instrument list for a group manager rather than erroring', async ({ apiRequestContext, roleAccount }) => {