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

Open
  • Is something wrong?
#726 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
55/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
typescript
Domain
authorization, backend-api-design, payments

Research direction

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.

Written by the indexing model from the issue text.

Description

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.
Dominant language
TypeScript
Stars
2
Forks
90
Avg merge
31m
Merged PRs (30d)
20

Getting set up

  • Ships a Dockerfile or Docker Compose file
  • No pull request template
  • No contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from MergeFi/backend

All issues in MergeFi/backend

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.