UsersController.setStellarAddress is authenticated but not authorized: any logged-in user can overwrite another user's payout address
Maintainer thường phản hồi trong vòng 1 ngày
@rudra496 đang làm issue này rồi.
Từ ngày 17/8/2026.
Đánh giá
Issue này chưa được đánh giá.
Mô tả
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
setStellarAddressmust verify the authenticated caller (req.user.userId) equals the:idbeing modified, and reject with403 Forbiddenotherwise — 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
:idfrom 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-addressrejects 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/UsersServiceno longer trust the URL-suppliedidas the target for identity-sensitive writes without validating it againstreq.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—setStellarAddresstakesuserIdpurely as a lookup key, never as something to compare against a caller.src/auth/strategies/jwt.strategy.ts:22-24— confirmsreq.usershape 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:stellarAddressis 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:
- Register two users (A, B) via the OAuth flow (or directly via
UsersService.upsertFromGithubin a test). - Sign a JWT for A.
- Call
PATCH /users/<B.id>/stellar-addresswith A's token and an attacker-controlled address. - Pre-fix: 200,
B.stellarAddressoverwritten. Post-fix: 403,B.stellarAddressunchanged. - 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).
- Ngôn ngữ chính
- TypeScript
- Star
- 2
- Fork
- 90
- Merge trung bình
- 31 phút
- Pull request đã merge (30 ngày)
- 20
Chuẩn bị môi trường
- Có Dockerfile hoặc tệp Docker Compose
- Không có mẫu pull request
- Không có hướng dẫn đóng góp
Bắt đầu từ đâu
- Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
- Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
- Fork repository và làm thay đổi trên một nhánh.
- Mở pull request có tham chiếu số hiệu của issue.
Issue khác của MergeFi/backend
-
bug
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 85/100
Maintainer thường phản hồi trong vòng 1 ngày
-
bug security
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 74/100
Maintainer thường phản hồi trong vòng 1 ngày
-
bug
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 82/100
Maintainer thường phản hồi trong vòng 1 ngày
-
bug testing
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 85/100
Maintainer thường phản hồi trong vòng 1 ngày
-
bug security
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 76/100
Maintainer thường phản hồi trong vòng 1 ngày
Tất cả issue của MergeFi/backend
Issue tương tự
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 76/100
zapcooking/frontend#946 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
enhancement good first issue
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 88/100
WiliamRosa/cosmos-semantic-platform#40 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 77/100
liberation-data/drivine#733 · 1 bình luận ·
Maintainer thường phản hồi trong vòng 1 ngày
-
bug
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 64/100
makecindy/cindy#10062 · 1 bình luận ·
Maintainer thường phản hồi trong vòng 1 ngày
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 78/100
Maintainer thường phản hồi trong vòng 1 ngày