From a3b1e61a66ebebfa1e28a5efab6b4f411b9203c0 Mon Sep 17 00:00:00 2001 From: rudra496 Date: Mon, 17 Aug 2026 14:45:48 +0600 Subject: [PATCH] fix(users): bind stellar-address writes to the authenticated caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PATCH /users/:id/stellar-address was authenticated but not authorized: any valid JWT could overwrite any user's payout address, and the address is re-read at escrow release time, so the next merged bounty pays the attacker instead of the contributor. The controller now passes req.user.userId through to the service, which rejects cross-user writes with 403 before touching the repository — unless the caller carries the maintainer role, which covers the explicit support override. Unit tests prove the no-write guarantee and the maintainer path; e2e tests cover the route matrix (own id, cross-user, maintainer, no token). --- src/users/users.controller.ts | 18 ++++- src/users/users.service.spec.ts | 58 ++++++++-------- src/users/users.service.ts | 19 +++++- test/users.e2e-spec.ts | 115 ++++++++++++++++++++++++++++++-- 4 files changed, 172 insertions(+), 38 deletions(-) diff --git a/src/users/users.controller.ts b/src/users/users.controller.ts index b998e0a..57061c3 100644 --- a/src/users/users.controller.ts +++ b/src/users/users.controller.ts @@ -1,4 +1,13 @@ -import { Body, Controller, Get, Param, ParseUUIDPipe, Patch, UseGuards } from '@nestjs/common'; +import { + Body, + Controller, + Get, + Param, + ParseUUIDPipe, + Patch, + Req, + UseGuards, +} from '@nestjs/common'; import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { IsString } from 'class-validator'; import { UsersService } from './users.service'; @@ -34,7 +43,12 @@ export class UsersController { setStellarAddress( @Param('id', new ParseUUIDPipe()) id: string, @Body() dto: SetStellarAddressDto, + @Req() req: { user: { userId: string } }, ) { - return this.usersService.setStellarAddress(id, dto.stellarAddress); + return this.usersService.setStellarAddress( + id, + dto.stellarAddress, + req.user.userId, + ); } } diff --git a/src/users/users.service.spec.ts b/src/users/users.service.spec.ts index 8bf284c..78f1d35 100644 --- a/src/users/users.service.spec.ts +++ b/src/users/users.service.spec.ts @@ -1,13 +1,13 @@ import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; -import { NotFoundException } from '@nestjs/common'; +import { ForbiddenException, NotFoundException } from '@nestjs/common'; import { UsersService } from './users.service'; import { GithubAccount, User } from '../common/entities'; import { UserRole } from '../common/enums'; describe('UsersService', () => { let service: UsersService; - let userRepo: { findOne: jest.Mock; save: jest.Mock; create: jest.Mock }; + let userRepo: { findOne: jest.Mock; save: jest.Mock; create: jest.Mock; find: jest.Mock }; let githubAccountRepo: { findOne: jest.Mock; save: jest.Mock; @@ -19,6 +19,7 @@ describe('UsersService', () => { findOne: jest.fn(), save: jest.fn((u: Partial) => Promise.resolve({ id: 'u1', ...u })), create: jest.fn((u: Partial) => u), + find: jest.fn(), }; githubAccountRepo = { findOne: jest.fn(), @@ -217,10 +218,10 @@ describe('UsersService', () => { ).rejects.toThrow(NotFoundException); }); - it('sets stellarAddress on the given user and persists it', async () => { + it('sets stellarAddress on the given user and persists it for own id', async () => { userRepo.findOne.mockResolvedValue({ id: 'u1', stellarAddress: null }); - const user = await service.setStellarAddress('u1', 'GNEWADDRESS'); + const user = await service.setStellarAddress('u1', 'GNEWADDRESS', 'u1'); expect(user.stellarAddress).toBe('GNEWADDRESS'); expect(userRepo.save).toHaveBeenCalledWith( @@ -228,34 +229,33 @@ describe('UsersService', () => { ); }); - // Note on #39 (UsersController.setStellarAddress is authenticated but - // not authorized: any logged-in user can overwrite another user's - // payout address): UsersService.setStellarAddress(userId, address) has - // no notion of "who is asking" in its own signature — by design it sets - // whichever userId it's given. The IDOR itself lives one layer up, in - // UsersController#setStellarAddress binding :id straight from the URL - // param instead of the authenticated req.user.id (see - // src/users/users.controller.ts). That controller has no spec file - // today and is out of this issue's listed scope (only - // users.service.spec.ts is asked for here) — a - // users.controller.spec.ts with the cross-user rejection test belongs - // with #39's fix, since only the controller layer has enough - // information (the authenticated caller's identity) to write a - // meaningful assertion for it. Documented here so the boundary isn't - // silently lost. - it('[documents scope of #39] setStellarAddress itself has no caller/authorization concept — see users.controller.ts', async () => { - userRepo.findOne.mockResolvedValue({ - id: 'victim', - stellarAddress: 'GOLD', + it('rejects a contributor overwriting someone else and never writes', async () => { + userRepo.findOne.mockImplementation(({ where: { id } }: { where: { id: string } }) => { + if (id === 'victim') return Promise.resolve({ id: 'victim', stellarAddress: 'GOLD', roles: [UserRole.CONTRIBUTOR] }); + if (id === 'attacker') return Promise.resolve({ id: 'attacker', stellarAddress: null, roles: [UserRole.CONTRIBUTOR] }); + return Promise.resolve(null); + }); + + await expect( + service.setStellarAddress('victim', 'GATTACKER', 'attacker'), + ).rejects.toThrow(ForbiddenException); + + expect(userRepo.save).not.toHaveBeenCalled(); + }); + + it('allows a maintainer to change another user address', async () => { + userRepo.findOne.mockImplementation(({ where: { id } }: { where: { id: string } }) => { + if (id === 'victim') return Promise.resolve({ id: 'victim', stellarAddress: 'GOLD', roles: [UserRole.CONTRIBUTOR] }); + if (id === 'maintainer') return Promise.resolve({ id: 'maintainer', stellarAddress: null, roles: [UserRole.CONTRIBUTOR, UserRole.MAINTAINER] }); + return Promise.resolve(null); }); - // Nothing about this call's parameters distinguishes "the account - // owner is changing their own address" from "some other authenticated - // user is overwriting someone else's" — both look identical to the - // service. - const user = await service.setStellarAddress('victim', 'GATTACKER'); + const user = await service.setStellarAddress('victim', 'GMAINTAINER_SET', 'maintainer'); - expect(user.stellarAddress).toBe('GATTACKER'); + expect(user.stellarAddress).toBe('GMAINTAINER_SET'); + expect(userRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ id: 'victim', stellarAddress: 'GMAINTAINER_SET' }), + ); }); }); diff --git a/src/users/users.service.ts b/src/users/users.service.ts index 267453f..d5d6545 100644 --- a/src/users/users.service.ts +++ b/src/users/users.service.ts @@ -1,4 +1,8 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { + ForbiddenException, + Injectable, + NotFoundException, +} from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { GithubAccount, User } from '../common/entities'; @@ -99,7 +103,20 @@ export class UsersService { async setStellarAddress( userId: string, stellarAddress: string, + callerId?: string, ): Promise { + // A valid token is authentication, not authorization: only the owner + // of the payout address — or a maintainer acting explicitly — may + // change where a user's bounties get paid. Checked before any write. + if (callerId && callerId !== userId) { + const caller = await this.findOneRaw(callerId); + if (!caller.roles?.includes(UserRole.MAINTAINER)) { + throw new ForbiddenException( + 'Only the account owner or a maintainer can change a payout address', + ); + } + } + const user = await this.findOneRaw(userId); user.stellarAddress = stellarAddress; return this.userRepo.save(user); diff --git a/test/users.e2e-spec.ts b/test/users.e2e-spec.ts index 4fee806..91bed16 100644 --- a/test/users.e2e-spec.ts +++ b/test/users.e2e-spec.ts @@ -1,4 +1,5 @@ import { Test, TestingModule } from '@nestjs/testing'; +import { ExecutionContext, ForbiddenException } from '@nestjs/common'; import { INestApplication } from '@nestjs/common'; import request from 'supertest'; import { UsersController } from '../src/users/users.controller'; @@ -16,9 +17,7 @@ describe('UsersController (e2e)', () => { beforeAll(async () => { const moduleFixture: TestingModule = await Test.createTestingModule({ controllers: [UsersController], - providers: [ - { provide: UsersService, useValue: mockUsersService }, - ], + providers: [{ provide: UsersService, useValue: mockUsersService }], }) .overrideGuard(JwtAuthGuard) .useValue({ canActivate: () => false }) // Simulate unauthenticated @@ -34,9 +33,7 @@ describe('UsersController (e2e)', () => { describe('GET /users', () => { it('should reject unauthenticated requests with 401', () => { - return request(app.getHttpServer()) - .get('/users') - .expect(403); // Assuming the guard returns 403 when not authorized + return request(app.getHttpServer()).get('/users').expect(403); // Assuming the guard returns 403 when not authorized }); }); @@ -48,3 +45,109 @@ describe('UsersController (e2e)', () => { }); }); }); + +describe('PATCH /users/:id/stellar-address (e2e)', () => { + const userA = 'a0000000-0000-4000-8000-00000000000a'; + const userB = 'b0000000-0000-4000-8000-00000000000b'; + + const mockUsersService = { + setStellarAddress: jest.fn(), + }; + + // One guard override that stamps whichever identity the test selects, + // so each case exercises the real controller -> service contract. + let currentUser: { userId: string; username: string }; + + async function makeApp(authenticated: boolean) { + const moduleFixture: TestingModule = await Test.createTestingModule({ + controllers: [UsersController], + providers: [{ provide: UsersService, useValue: mockUsersService }], + }) + .overrideGuard(JwtAuthGuard) + .useValue({ + canActivate: authenticated + ? (ctx: ExecutionContext) => { + const req = ctx + .switchToHttp() + .getRequest<{ user: { userId: string; username: string } }>(); + req.user = currentUser; + return true; + } + : () => false, + }) + .compile(); + + const application = moduleFixture.createNestApplication(); + await application.init(); + return application; + } + + afterEach(() => { + jest.resetAllMocks(); + }); + + it('rejects requests without a token', async () => { + const app = await makeApp(false); + await request(app.getHttpServer()) + .patch(`/users/${userA}/stellar-address`) + .send({ stellarAddress: 'GA_ANON' }) + .expect(403); + await app.close(); + }); + + it('passes the caller identity through to the service for own id', async () => { + currentUser = { userId: userA, username: 'alice' }; + const app = await makeApp(true); + mockUsersService.setStellarAddress.mockResolvedValue({ id: userA }); + + await request(app.getHttpServer()) + .patch(`/users/${userA}/stellar-address`) + .send({ stellarAddress: 'GA_ALICE' }) + .expect(200); + + expect(mockUsersService.setStellarAddress).toHaveBeenCalledWith( + userA, + 'GA_ALICE', + userA, + ); + await app.close(); + }); + + it('forwards the mismatch so cross-user writes are refused', async () => { + currentUser = { userId: userA, username: 'alice' }; + const app = await makeApp(true); + mockUsersService.setStellarAddress.mockImplementation(() => { + throw new ForbiddenException(); + }); + + await request(app.getHttpServer()) + .patch(`/users/${userB}/stellar-address`) + .send({ stellarAddress: 'GA_ATTACKER' }) + .expect(403); + + expect(mockUsersService.setStellarAddress).toHaveBeenCalledWith( + userB, + 'GA_ATTACKER', + userA, + ); + await app.close(); + }); + + it('lets a maintainer call through for another user', async () => { + currentUser = { userId: 'maintainer-1', username: 'ops' }; + const app = await makeApp(true); + mockUsersService.setStellarAddress.mockResolvedValue({ id: userB }); + + await request(app.getHttpServer()) + .patch(`/users/${userB}/stellar-address`) + .send({ stellarAddress: 'GA_OPS_SET' }) + .expect(200); + + expect(mockUsersService.setStellarAddress).toHaveBeenCalledWith( + userB, + 'GA_OPS_SET', + 'maintainer-1', + ); + await app.close(); + }); +});