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

オープン
  • 何か問題がありますか？
#1,759 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

メンテナーはふだん 3 日以内に返信

まだ誰も着手していません。

評価

難易度
4/5
見積もり時間
3〜5日
初心者へのやさしさ
55/100
issue の種類
バグ
明瞭さ
明確に書かれている
活発さ
活発
技術スタック
typescript
領域
api, backend-api-design, databases

調査の方向性

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.

索引モデルが issue の本文から書いたものです。

説明

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.

主要言語
TypeScript
スター
119
フォーク
19
平均マージ
6日 16分
マージ済み PR（30日）
34

環境構築

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

DouglasNeuroInformatics/OpenDataCapture のほかの issue

DouglasNeuroInformatics/OpenDataCapture の issue をすべて見る

似ている issue

TypeScript の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。