Group and user routes return 500 for a malformed or unknown id instead of 400 or 404

Abierto
  • ¿Algo no está bien?
#1,759 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 2 días

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
4/5
Tiempo estimado
3-5 días
Aptitud para principiantes
55/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
typescript
Área
api, backend-api-design, databases

Línea de trabajo

Read apps/api/AGENTS.md (Validation), then inspect the named group and user controllers and services, the listed controllers with groupId filters, and packages/schemas/src/audit/audit.ts. Start with the existing ValidObjectIdPipe use in SubjectsController.findCustomIds and the lookup pattern in GroupsService.updateById; run the named service, integration, and authorization tests. Done means malformed IDs return 400 and missing or inaccessible records return 404 rather than 500.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

Area: API Bug Difficulty: Low Good First Issue Priority: Low

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 :id handler.
  • apps/api/src/users/users.service.ts:37-43, :175-181 and :204-217: archiveById, unarchiveById and updateById call userModel.update directly.
  • Unvalidated groupId query 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 groupId and userId (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/abc and GET /v1/users/abc return 500.
  • GET /v1/users?groupId=abc, GET /v1/sessions?groupId=abc, GET /v1/subjects?groupId=abc and GET /v1/summary?groupId=abc return 500.
  • DELETE /v1/groups/000000000000000000000000, PATCH /v1/users/000000000000000000000000 and PATCH /v1/users/000000000000000000000000/archive return 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 in users.service.test.ts for 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.

Lenguaje dominante
TypeScript
Estrellas
119
Forks
19
Merge medio
5 d 10 h
PR fusionados (30 d)
38

Preparar el entorno

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de DouglasNeuroInformatics/OpenDataCapture

Todos los issues de DouglasNeuroInformatics/OpenDataCapture

Issues similares

Más issues de TypeScript

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.