[Regression on closed #537] PATCH /teams/:id/splits still has no ownership check — any SPONSOR or MAINTAINER can rewrite any team's payout percentages

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

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

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

評価

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

調査の方向性

Start with src/teams/teams.controller.ts and src/teams/teams.service.ts, then compare updateSplits with assignToBounty to trace how caller identity and authorization are handled. Check how team ownership and bounty status are represented before implementing the requested checks. Add controller and service specs for forbidden callers, and confirm owners and maintainers can still update splits while teams past CLAIMED cannot be changed.

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

説明

bug security

Location — src/teams/teams.controller.ts:65 (updateSplits) and src/teams/teams.service.ts:72 (TeamsService.updateSplits).

Problem — The defect closed as #537 is present again on the default branch. The route is guarded only by @UseGuards(JwtAuthGuard, RolesGuard) + @Roles(UserRole.MAINTAINER, UserRole.SPONSOR), which answers "is this caller a sponsor/maintainer", not "does this caller own this team":

updateSplits(
  @Param('id', new ParseUUIDPipe()) id: string,
  @Body() dto: UpdateTeamSplitsDto,
) {
  return this.teamsService.updateSplits(id, dto.splits);
}

The caller's identity is not even passed to the service — there is no @CurrentUser() parameter — and TeamsService.updateSplits(teamId, members) takes no caller id, so no check is possible downstream either. Team.createdById exists on the entity and is set at creation time (teams.service.ts:41), but nothing ever reads it. Compare assignToBounty (teams.service.ts:108), which does receive callerId and does check sponsor-or-maintainer against the specific bounty.

Why it matters — This is a direct payout-hijack path on money. Any account holding the SPONSOR or MAINTAINER role can enumerate team UUIDs and rewrite another team's member splits to [{ userId: <attacker>, percentage: 100 }]. The next markMergedAndRelease on a bounty assigned to that team pays the whole bounty amount to the attacker instead of to the contributors who did the work. Nothing in the flow re-validates split ownership at payout time, and the existing splits are destroyed (splitRepo.delete({ teamId }), line 90), so the original percentages are not recoverable from the database.

Reproduction / failure path

  1. User A (SPONSOR) creates team T with members B 60% / C 40%. POST /teams returns T's id.
  2. User D, an unrelated account that merely holds the SPONSOR role, calls PATCH /teams/<T>/splits with body {"splits":[{"userId":"<D>","percentage":100}]}.
  3. Returns 200. T's splits are now 100% to D. No ownership error, and A is not notified.
  4. A bounty assigned to T is merged and released; the entire payout goes to D.

Suggested fix

  • Add @CurrentUser() user: AuthenticatedUser to the controller method and pass user (id and roles) into TeamsService.updateSplits.
  • In the service, after findOne(teamId), assert team.createdById === callerId or that the caller holds UserRole.MAINTAINER, mirroring the check in assignToBounty (teams.service.ts:118-128), and throw ForbiddenException otherwise.
  • Also reject the mutation when the team is already attached to a bounty whose status is past CLAIMED, so splits cannot be rewritten after work has been delivered.
  • Add a controller spec and a service spec asserting 403 for a non-owner non-maintainer caller — #537's fix had no such test, which is why it could silently regress.
主要言語
TypeScript
スター
2
フォーク
90
平均マージ
31分
マージ済み PR（30日）
20

環境構築

  • Dockerfile または Docker Compose ファイルあり
  • プルリクエストのテンプレートなし
  • コントリビューションガイドなし

はじめの一歩

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

MergeFi/backend のほかの issue

MergeFi/backend の issue をすべて見る

似ている issue

TypeScript の issue をもっと見る

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

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