UsersController.setStellarAddress is authenticated but not authorized: any logged-in user can overwrite another user's payout address

[chonilius]

Overview

PATCH /users/:id/stellar-address is the one route in the app that does sit behind JwtAuthGuard — but authentication is not authorization, and this route never checks that the :id in the URL matches the caller who owns the token.

// src/users/users.controller.ts:27-35
@ApiBearerAuth()
@UseGuards(JwtAuthGuard)
@Patch(':id/stellar-address')
setStellarAddress(
  @Param('id') id: string,
  @Body() dto: SetStellarAddressDto,
) {
  return this.usersService.setStellarAddress(id, dto.stellarAddress);
}

The handler never injects @Req()/req.user, so it has no way to compare the caller's identity to id even if it wanted to — and UsersService.setStellarAddress doesn't either:

// src/users/users.service.ts:92-99
async setStellarAddress(
  userId: string,
  stellarAddress: string,
): Promise<User> {
  const user = await this.findById(userId);
  user.stellarAddress = stellarAddress;
  return this.userRepo.save(user);
}

JwtStrategy.validate (src/auth/strategies/jwt.strategy.ts:22-24) attaches { userId, username } to req.user — that's the only place the caller's real identity lives, and this endpoint ignores it entirely.

stellarAddress is the single field that determines where bounty payouts physically go. BountiesService.markMergedAndRelease (src/bounties/bounties.service.ts:121-129) looks it up fresh at payout time, not at claim time:

const contributor = await this.userRepo.findOne({ where: { id: bounty.claimedById } });
await this.escrowService.release(bounty.escrowId, contributor?.stellarAddress ?? '', bounty.claimedById);

Put together: any user who can obtain a valid JWT for their own account (i.e. anyone who completes GitHub OAuth login — a two-minute, self-serve process) can call PATCH /users/<victim-userId>/stellar-address with their own wallet address, and the next time any bounty claimed by the victim gets merged, the payout goes to the attacker's wallet instead. The victim did the work; the attacker gets paid. This requires zero access to the victim's account, session, or GitHub credentials — only the victim's user UUID, which is exposed by the unauthenticated GET /users and GET /bounties/:id (claimedById) endpoints.

Requirements

• setStellarAddress must verify the authenticated caller (req.user.userId) equals the :id being modified, and reject with 403 Forbidden otherwise — or, if an admin/maintainer override is intended, gate that path behind an explicit role check (UserRole.MAINTAINER), not "any bearer token."
• Audit every other route in the codebase that takes a resource :id from the URL and mutates it, to confirm none of them have the same "authenticated but not authorized" shape once guards are added per the companion "no auth at all" issue — this bug class (checking a valid token instead of the right caller) is easy to reintroduce if the general fix for that issue is "just slap @UseGuards(JwtAuthGuard) on everything" without also auditing for ownership.
• Add a regression test that specifically proves cross-user modification is rejected: user A's token, user B's :id, expect 403.

Acceptance Criteria

• PATCH /users/:id/stellar-address rejects any request where the authenticated caller's ID doesn't match :id (absent an explicit, tested maintainer-override path).
• A test proves: user A can set their own address; user A cannot set user B's address; the rejection happens before any write to user.stellarAddress.
• UsersController/UsersService no longer trust the URL-supplied id as the target for identity-sensitive writes without validating it against req.user.

Additional Notes

Precise references:

• src/users/users.controller.ts:27-35 — the vulnerable handler; no @Req() parameter at all.
• src/users/users.service.ts:92-99 — setStellarAddress takes userId purely as a lookup key, never as something to compare against a caller.
• src/auth/strategies/jwt.strategy.ts:22-24 — confirms req.user shape is { userId, username }, so the fix is a one-line comparison once @Req() is added: if (req.user.userId !== id) throw new ForbiddenException().
• src/bounties/bounties.service.ts:121-129 — confirms the attack actually pays off: stellarAddress is re-read at release time, not pinned at claim time, so the attacker doesn't even need to time the write before the claim — any time before the PR merges works.
• src/common/entities/user.entity.ts:38-39 — stellarAddress's own doc comment says "Custody of the corresponding secret key always remains with the user... MergeFi never stores private keys for end users" — true, but irrelevant to this bug: the attacker never needs the victim's key, only write access to a public-key field that has no ownership check.

Why this is worse than a typical IDOR: most IDORs leak or corrupt data. This one redirects real money to an address of the attacker's choosing, is invisible to the victim until they notice they were never paid, and requires no special access beyond "has a GitHub account and completed OAuth once" — the lowest possible bar in a platform whose whole premise is open GitHub-based bounty claiming.

Test/reproduction plan:

1. Register two users (A, B) via the OAuth flow (or directly via UsersService.upsertFromGithub in a test).
2. Sign a JWT for A.
3. Call PATCH /users/<B.id>/stellar-address with A's token and an attacker-controlled address.
4. Pre-fix: 200, B.stellarAddress overwritten. Post-fix: 403, B.stellarAddress unchanged.
5. End-to-end variant: fund a bounty, have B claim it, have A overwrite B's stellarAddress, merge the PR, assert the release call would target A's address pre-fix / is rejected post-fix.

Cross-references: this is the second concrete exploit (after the general "no auth at all" issue) of the broader pattern this batch keeps surfacing — the app conflates "has a valid token" with "is allowed to do this." The companion issue on client-supplied contributorId/recipientId/funderAddress fields is the same pattern one layer further out (trusting the body instead of trusting the URL param, but the same underlying missing check against req.user).

[rudra496]

I'd like to take this one. Plan:

1. Ownership check at the controller: inject @Req() into setStellarAddress, compare req.user.userId against the :id param and throw ForbiddenException before calling UsersService — so rejection happens before any write to user.stellarAddress. The service keeps its current lookup/save responsibility; the controller owns the caller-identity decision.
2. Maintainer override: users.roles is already persisted (UserRole[], default CONTRIBUTOR in user.entity.ts:29-30), so I'll gate an explicit override on roles.includes(UserRole.MAINTAINER) and cover it with its own test rather than inventing new role infrastructure.
3. Sibling-route audit: sweep the other controllers for the same "URL :id + mutating handler + no @Req()" shape (bounties claim/release, teams assign, milestones add/resolve, maintenance-pool) and report the findings in the PR description — so the general guard rollout from the no-auth-at-all issue doesn't reintroduce this "valid token ≠ right caller" class unexamined.
4. Tests: the exact acceptance matrix — A sets own address → 200; A targets B's id → 403 and B's stellarAddress unchanged; no token → 401; maintainer override → allowed. Service-level test that no save() runs on rejection.

Build/lint/tests via the existing scripts (npm run build, npm run lint, npm test, test:e2e where it applies). Happy to adjust the plan if maintainers prefer a different shape.

[grantfox-oss]

🦊 GrantFox — @rudra496 has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #39)
2. Your PR will be reviewed by the MergeFi maintainers

Good luck! Track your progress on GrantFox.