Repository navigation
Group and user routes return 500 for a malformed or unknown id instead of 400 or 404 #1759
Description
The group and user routes return 500 for a bad or stale id, and log it as a server error. GroupsController and UsersController pass :id straight to Prisma, as do the groupId query parameters of the users, sessions, subjects and summary routes. These fields are @db.ObjectId, so a value that is not 24 hex characters makes Prisma throw a malformed-ObjectID PrismaClientKnownRequestError. Separately, the write handlers that call update or delete directly throw P2025 when no row matches. Neither error is an HttpException, so libnest's global filter answers 500 and logs it at error level. The same happens when a group manager's DELETE /v1/groups/<foreign group id> is refused. apps/api/AGENTS.md (Validation) says params and query strings must be validated explicitly, and SubjectsController.findCustomIds already uses ValidObjectIdPipe for this.
Where
apps/api/src/groups/groups.controller.ts:25-30, and the same @Param('id') id: string on the get and patch handlers:
@Delete(':id')
@RouteAccess({ action: 'delete', subject: 'Group' })
deleteById(@Param('id') id: string, @CurrentUser('ability') ability?: AppAbility) {
return this.groupsService.deleteById(id, { ability });
}apps/api/src/groups/groups.service.ts:54-58, which deletes without first checking that a row exists:
async deleteById(id: string, { ability }: EntityOperationOptions = {}) {
return this.groupModel.delete({
where: { AND: [accessibleQuery(ability, 'delete', 'Group')], id }
});
}The same pattern appears in:
apps/api/src/users/users.controller.ts:35-111: every:idhandler.apps/api/src/users/users.service.ts:37-43,:175-181and:204-217:archiveById,unarchiveByIdandupdateByIdcalluserModel.updatedirectly.- Unvalidated
groupIdquery parameters:users.controller.ts:77,sessions/sessions.controller.ts:28,subjects/subjects.controller.ts:32,summary/summary.controller.ts:22. - The audit log page's URL filters
groupIdanduserId(packages/schemas/src/audit/audit.ts:31-32).
Reproduce
Verified with the api integration harness, signed in as admin:
GET /v1/groups/abc,PATCH /v1/groups/abcandGET /v1/users/abcreturn 500.GET /v1/users?groupId=abc,GET /v1/sessions?groupId=abc,GET /v1/subjects?groupId=abcandGET /v1/summary?groupId=abcreturn 500.DELETE /v1/groups/000000000000000000000000,PATCH /v1/users/000000000000000000000000andPATCH /v1/users/000000000000000000000000/archivereturn 500.
Actual: 500 Internal Server Error, logged at error level.
Expected: 400 for a malformed id and 404 for an id that matches no row the caller can access. GET /v1/groups/<unknown but well-formed id> already returns 404.
Tests
apps/api/src/groups/__tests__/groups.service.test.ts:it('should throw NotFoundException when the group to delete does not exist or is not accessible, rather than a server error'), with matching cases inusers.service.test.tsfor archive, unarchive and update.apps/api/test/suites/(integration, which runs the real pipes):it('should answer 400 for a malformed group or user id, rather than a server error').testing/src/specs/authorization.spec.ts: tighten'should not let a group manager delete a group they do not belong to'to expect 404 instead of any non-OK status.
Suggested fix
Add ValidObjectIdPipe to every :id param in GroupsController and UsersController except self-update/:id, which already returns 403 before Prisma sees the id. Validate the groupId query parameters with new ParseSchemaPipe({ isOptional: true, schema }) using an ObjectId schema, and do the same for the audit query's groupId and userId. In GroupsService.deleteById and the three UsersService writes, look the row up first, as GroupsService.updateById does, and throw NotFoundException when it is missing.
Activity
joshunrau commentedon Oct 26, 2026
Still reproduces on
main(d86740e). No commit since this was filed touches these controllers or services.Other routes with the same hole
apps/api/src/assignments/assignments.controller.ts:100:GET /v1/assignments?groupId=abcreaches thefindManyfilter atassignments.service.ts:183.apps/api/src/instrument-records/instrument-records.controller.ts:63,:92and:100(find, linear-model, summary/by-subject): the service callsgroupsService.findById(groupId)first (instrument-records.service.ts:259,:313,:368).:82(export) puts the value into$toObjectIdinqueryRecordsRaw(instrument-records.service.ts:630-632), so the conversion fails inside MongoDB.apps/api/src/instruments/instruments.controller.ts:71:GET /v1/instruments/info?groupId=abcas an admin.resolveGroupIdsreturns the id unchecked formanage all(instruments.service.ts:765-773), and it reachesseriesGroupId: { in: groupIds }. Non-admins get 403 first.apps/api/src/instrument-repos/instrument-repos.controller.ts:24and:38: bare@Param('id')on delete and sync. The fix for Instrument repo sync and re-add ignore a group-scoped grant, letting it re-sync or replace the token of any repository #1728 changes the samesynchandler, so whichever lands second adds the pipe.summary/by-instrumentandsummary/by-series, both with a bare@Query('groupId'). Land this after feat: add the instrument hub and record provenance #1807, or cover those two on rebase. feat: add the instrument hub and record provenance #1807's edit toauthorization.spec.tsis one line inGROUP_MANAGER_ONLY_ROUTES, so it does not collide with the delete test.Leave the
:idparams of assignments, instruments and subjects alone. Those ids are plain strings, not ObjectIds (apps/api/prisma/schema.prisma:72,:215,:269), so the pipe would reject valid ids.On the suggested fix
$ObjectIdschema topackages/schemas/src/core/core.ts, matchingValidObjectIdPipe's/^[a-fA-F0-9]{24}$/, and use it in every optional query pipe and in the audit params.ValidObjectIdPipecannot go on an optional query: it rejectsundefinedwith 400, which would break every call that omitsgroupId. Keep it for the required:idparams.$AuditLogsQuerySearchParamsis also the web route'svalidateSearch(apps/web/src/routes/_app/admin/audit/logs.tsx:318). Tightening it makes a hand-edited?groupId=abcfail on the client before any request is sent, as an unknownactionalready does. Add that case beside theaction: 'TELEPORT'one atapps/web/src/routes/_app/admin/audit/__tests__/logs.test.tsx:202. fix: resolve 26 low-difficulty issues, one commit each #1825 also edits both files, so expect a small rebase.model.exists(where)with the same scopedwherethe write uses, as inAssignmentsService.updateById(assignments.service.ts:198-202).deleteById,archiveByIdandunarchiveByIdnever use the row.UsersService.updateByIdcan already read the user twice: at:200for the username, and inpermissionsWithinGroups(:290). Replace both with one lookup at the top, scoped by theupdaterule the write uses rather thanfindById'sreadrule. Throw the 404 there and pass itsusernameandadditionalPermissionsdown.Tests
Beyond the issue's list:
packages/schemas/src/core/__tests__/core.test.tsfor$ObjectId. Put the integration cases in a newapps/api/test/suites/04-id-validation.suite.ts. Suites run in filename order against one app, so it can sign in as the admin that02-user-permissionssets up. Intesting/src/specs/authorization.spec.ts, add oneapiRequestContextcase as admin expecting 400 forGET /api/v1/users?groupId=abcnext to the tightened delete test.Related
#1642 still needs its client guard. After this fix
PATCH /v1/groups/undefinedreturns 400 instead of 500, but the request is still sent. Request bodies have the same gap:POST /v1/userswithgroupIds: ['abc']fails in thefindByIdloop atusers.service.ts:100-102. Typing the write schemas' id fields as$ObjectIdwould let the globalValidationPipeanswer 400. That belongs in a follow-up.Triage note written with Claude Code. Code references point at
main@d86740e.