diff --git a/src/escrow/escrow.service.ts b/src/escrow/escrow.service.ts index 6d2dd289..c252993f 100644 --- a/src/escrow/escrow.service.ts +++ b/src/escrow/escrow.service.ts @@ -584,20 +584,14 @@ export class EscrowService { recipients: Array<[string, number]>, manager?: EntityManager, ): Promise { - return this.invokeOnLockedEscrow(escrow, operation, () => - this.soroban.invoke( - 'release', - // `release(issue_id: u64, recipients)` — u64-typed on-chain (#301). - [u64(this.onChainKeyFor(escrow)), recipients], - this.contractOpts(escrow), - ), return this.invokeOnLockedEscrow( escrow, operation, () => this.soroban.invoke( 'release', - [this.onChainKeyFor(escrow), recipients], + // `release(issue_id: u64, recipients)` — u64-typed on-chain (#301). + [u64(this.onChainKeyFor(escrow)), recipients], this.contractOpts(escrow), ), manager, diff --git a/src/github/github-webhooks.service.spec.ts b/src/github/github-webhooks.service.spec.ts index 45bb6fe8..a35ac3be 100644 --- a/src/github/github-webhooks.service.spec.ts +++ b/src/github/github-webhooks.service.spec.ts @@ -869,6 +869,9 @@ describe('GithubWebhooksService', () => { expect(bountiesService.markInReview).not.toHaveBeenCalled(); expect(bountiesService.markMergedAndRelease).not.toHaveBeenCalled(); + }); + }); + // #308: a redelivered X-GitHub-Delivery used to hit the unique constraint // on webhook_events.deliveryId and escape handleEvent as a 500, so every // redelivery of that event failed forever. diff --git a/src/sponsors/sponsors.controller.ts b/src/sponsors/sponsors.controller.ts index b7a48cca..c097bf1e 100644 --- a/src/sponsors/sponsors.controller.ts +++ b/src/sponsors/sponsors.controller.ts @@ -26,25 +26,6 @@ function validatePage(limit?: number, offset?: number) { throw new BadRequestException('offset must be >= 0'); } return { limit, offset }; -/** - * Parse optional limit/offset query params, rejecting NaN, negatives, - * non-integers, and limits above MAX_PAGE_LIMIT with a 400 (#280). - */ -function parsePagination(limit?: string, offset?: string) { - const parse = (name: string, raw: string | undefined, min: number, max: number) => { - if (raw === undefined || raw === '') return undefined; - const n = Number(raw); - if (!Number.isInteger(n) || n < min || n > max) { - throw new BadRequestException( - `${name} must be an integer between ${min} and ${max}`, - ); - } - return n; - }; - return { - limit: parse('limit', limit, 1, MAX_PAGE_LIMIT), - offset: parse('offset', offset, 0, Number.MAX_SAFE_INTEGER), - }; } @ApiTags('sponsors') @@ -73,7 +54,6 @@ export class SponsorsController { ) { this.assertOwnsSponsor(user, id); return this.sponsorsService.dashboard(id, validatePage(limit, offset)); - return this.sponsorsService.dashboard(id, parsePagination(limit, offset)); } @ApiBearerAuth() @@ -91,6 +71,5 @@ export class SponsorsController { id, validatePage(limit, offset), ); - return this.sponsorsService.milestoneProgress(id, parsePagination(limit, offset)); } } diff --git a/src/users/users.controller.spec.ts b/src/users/users.controller.spec.ts new file mode 100644 index 00000000..5a613e11 --- /dev/null +++ b/src/users/users.controller.spec.ts @@ -0,0 +1,66 @@ +import { ForbiddenException } from '@nestjs/common'; +import { UsersController } from './users.controller'; +import { UsersService } from './users.service'; +import { Reflector } from '@nestjs/core'; +import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; +import { GUARDS_METADATA } from '@nestjs/common/constants'; + +describe('UsersController (#597)', () => { + const usersService = { + list: jest.fn(), + findById: jest.fn(), + setStellarAddress: jest.fn(), + } as unknown as UsersService; + const controller = new UsersController(usersService); + + it('list delegates to usersService.list', async () => { + usersService.list = jest.fn().mockResolvedValue([{ id: 'u1' }]); + await expect(controller.list()).resolves.toEqual([{ id: 'u1' }]); + expect(usersService.list).toHaveBeenCalled(); + }); + + it('findOne delegates with the parsed id', async () => { + usersService.findById = jest.fn().mockResolvedValue({ id: 'u1' }); + await controller.findOne('u1'); + expect(usersService.findById).toHaveBeenCalledWith('u1'); + }); + + it('setStellarAddress delegates for the authenticated self', async () => { + usersService.setStellarAddress = jest + .fn() + .mockResolvedValue({ stellarAddress: 'G...' }); + const req = { user: { userId: 'u1' } } as never; + await controller.setStellarAddress( + 'u1', + { stellarAddress: 'GABC' } as never, + req, + ); + expect(usersService.setStellarAddress).toHaveBeenCalledWith('u1', 'GABC'); + }); + + // The self-only check (#39) — anyone setting ANOTHER user's stellar-address + // (the on-chain payout recipient) is a payout-hijack. Must 403. + it("setStellarAddress rejects setting another user's address", async () => { + usersService.setStellarAddress = jest.fn(); + const req = { user: { userId: 'attacker' } } as never; + expect(() => + controller.setStellarAddress( + 'victim-id', + { stellarAddress: 'GABC' } as never, + req, + ), + ).toThrow(ForbiddenException); + expect(usersService.setStellarAddress).not.toHaveBeenCalled(); + }); + + it('all routes are behind JwtAuthGuard', () => { + const reflector = new Reflector(); + for (const method of ['list', 'findOne', 'setStellarAddress'] as const) { + const guards = reflector.get( + GUARDS_METADATA, + UsersController.prototype[method], + ); + expect(guards).toContain(JwtAuthGuard); + } + }); +}); diff --git a/src/users/users.controller.ts b/src/users/users.controller.ts index 21559de8..fac78368 100644 --- a/src/users/users.controller.ts +++ b/src/users/users.controller.ts @@ -1,12 +1,15 @@ import { Body, Controller, + ForbiddenException, Get, Param, ParseUUIDPipe, Patch, + Req, UseGuards, } from '@nestjs/common'; +import { Request } from 'express'; import { ApiBearerAuth, ApiProperty, ApiTags } from '@nestjs/swagger'; import { UsersService } from './users.service'; import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; @@ -48,7 +51,15 @@ export class UsersController { setStellarAddress( @Param('id', new ParseUUIDPipe()) id: string, @Body() dto: SetStellarAddressDto, + @Req() req: Request, ) { + // Self-only (#39): the URL :id must be the authenticated caller — the + // stellar-address is the on-chain payout recipient, so letting anyone set + // anyone's address would misdirect escrow releases. Reject non-self. + const callerId = (req.user as { userId?: string }).userId; + if (callerId !== id) { + throw new ForbiddenException('Can only set your own stellar address'); + } return this.usersService.setStellarAddress(id, dto.stellarAddress); } }