From 64ad9b8e8152483ed1f1eb5f7364c3ba820c7cff Mon Sep 17 00:00:00 2001 From: Yggdrasil Date: Wed, 14 Oct 2026 02:49:39 -0400 Subject: [PATCH 1/3] fix(users): self-check on PATCH stellar-address + controller spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PATCH /users/:id/stellar-address took the URL id with no self-check — any authenticated user could set ANY user's stellar-address (the on-chain payout recipient), a payout-hijack hole. Enforce id === the JWT caller. Also adds the missing users.controller spec (delegation + the 403). Resolves #597 + the missing #39 self-check --- src/users/users.controller.spec.ts | 65 ++++++++++++++++++++++++++++++ src/users/users.controller.ts | 11 +++++ 2 files changed, 76 insertions(+) create mode 100644 src/users/users.controller.spec.ts diff --git a/src/users/users.controller.spec.ts b/src/users/users.controller.spec.ts new file mode 100644 index 00000000..4c9ecc8f --- /dev/null +++ b/src/users/users.controller.spec.ts @@ -0,0 +1,65 @@ +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 () => { + const req = { user: { userId: 'attacker' } } as never; + await expect( + controller.setStellarAddress( + 'victim-id', + { stellarAddress: 'GABC' } as never, + req, + ), + ).rejects.toBeInstanceOf(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); } } From d603286d235a85ce338c849a43be62e29737ffcd Mon Sep 17 00:00:00 2001 From: yggdrasilworld Date: Sun, 18 Oct 2026 21:31:29 -0400 Subject: [PATCH 2/3] =?UTF-8?q?test(users):=20self-check=20spec=20?= =?UTF-8?q?=E2=80=94=20sync=20throw=20+=20fresh=20mock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setStellarAddress is sync — the ForbiddenException escapes expect() as a throw, not a rejection; and the prior test's jest.fn() leaked a recorded call into the not.toHaveBeenCalled assertion. --- src/users/users.controller.spec.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/users/users.controller.spec.ts b/src/users/users.controller.spec.ts index 4c9ecc8f..5a613e11 100644 --- a/src/users/users.controller.spec.ts +++ b/src/users/users.controller.spec.ts @@ -41,14 +41,15 @@ describe('UsersController (#597)', () => { // 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; - await expect( + expect(() => controller.setStellarAddress( 'victim-id', { stellarAddress: 'GABC' } as never, req, ), - ).rejects.toBeInstanceOf(ForbiddenException); + ).toThrow(ForbiddenException); expect(usersService.setStellarAddress).not.toHaveBeenCalled(); }); From cd8fe061f6a0715b477750656713119ed1ecf1d1 Mon Sep 17 00:00:00 2001 From: yggdrasilworld Date: Sun, 18 Oct 2026 21:50:21 -0400 Subject: [PATCH 3/3] fix: repair merge glue that left the branch uncompilable The shared base carried half-written merge artifacts: an orphaned invokeOnLockedEscrow block in escrow.service.ts, duplicated returns plus a dead parsePagination in sponsors.controller.ts, and missing describe/it closers in github-webhooks.service.spec.ts. Aligned each with the intended form so the branch typechecks and its specs load. --- src/escrow/escrow.service.ts | 10 ++-------- src/github/github-webhooks.service.spec.ts | 3 +++ src/sponsors/sponsors.controller.ts | 21 --------------------- 3 files changed, 5 insertions(+), 29 deletions(-) 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)); } }