From 73b65ecf5a32e343e7fb78c16fce93ad0f37ffee Mon Sep 17 00:00:00 2001 From: davieslennox <102302431+davieslennox0@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:58:41 +0000 Subject: [PATCH 1/2] fix(security): bind money-moving DTOs to authenticated caller identity - ClaimBountyDto: removed entirely; POST /bounties/:id/claim now takes no body and derives the contributor from req.user.userId - EscrowService: added assertRecipientAddressMatchesUser() cross-check; release(), splitRelease() and releasePartial() reject a mismatched recipientId/recipientAddress pair before any Soroban call - funderAddress: the four funding routes assert the address is the caller's own linked stellarAddress. req.user carries only {userId, username}, so this is a user-record lookup (UsersService.assertOwnsStellarAddress), not a token-claim comparison - JwtAuthGuard added to exactly the five routes that now derive identity from the caller. Without a guard req.user is undefined and these checks would be decorative; the remaining mutating routes are left to the companion auth issue - BountiesService: a payout to a contributor with no linked wallet used to fall back to address '', releasing to nobody while still marking the bounty PAID; it now fails with a message naming the real problem - Tests: user-A-cannot-claim-as-B, mismatched pair rejected pre-Soroban (release/splitRelease/releasePartial), matching pair passes, unlinked and unknown recipients rejected, funder binding on all four funding routes - PR description includes the full DTO audit table Closes #40 --- README.md | 29 +++ src/auth/authenticated-request.ts | 26 +++ src/auth/strategies/jwt.strategy.ts | 3 +- src/bounties/bounties.controller.spec.ts | 161 +++++++++++++++++ src/bounties/bounties.controller.ts | 54 +++++- src/bounties/bounties.module.ts | 7 +- src/bounties/bounties.service.ts | 38 +++- src/bounties/dto/claim-bounty.dto.ts | 8 - .../idempotency/idempotency.interceptor.ts | 14 +- src/escrow/dto/fund-escrow.dto.ts | 11 +- src/escrow/dto/release-escrow.dto.ts | 12 +- src/escrow/dto/split-release.dto.ts | 10 +- src/escrow/escrow.controller.spec.ts | 71 +++++++- src/escrow/escrow.controller.ts | 32 +++- src/escrow/escrow.module.ts | 6 +- src/escrow/escrow.service.spec.ts | 168 +++++++++++++++++- src/escrow/escrow.service.ts | 51 ++++++ .../maintenance-pool.controller.ts | 44 ++++- .../maintenance-pool.module.ts | 7 +- src/milestones/milestones.controller.ts | 44 ++++- src/milestones/milestones.module.ts | 7 +- .../users-stellar-address-binding.spec.ts | 70 ++++++++ src/users/users.service.ts | 37 +++- test/escrow-idempotency.e2e-spec.ts | 8 + 24 files changed, 866 insertions(+), 52 deletions(-) create mode 100644 src/auth/authenticated-request.ts create mode 100644 src/bounties/bounties.controller.spec.ts delete mode 100644 src/bounties/dto/claim-bounty.dto.ts create mode 100644 src/users/users-stellar-address-binding.spec.ts diff --git a/README.md b/README.md index 1f871f2..85c7395 100644 --- a/README.md +++ b/README.md @@ -182,6 +182,31 @@ Route groups: `/api/auth`, `/api/users`, `/api/github`, `/api/escrow`, `/api/teams`, `/api/milestones`, `/api/maintenance-pools`, `/api/sponsors`, `/api/reputation`, `/api/analytics`. +### Caller identity on money-moving routes + +Fields that decide *who benefits* from a mutation are never taken from the +request body. Two rules enforce this (#40): + +- **The claimant is the caller.** `POST /bounties/:id/claim` takes no body at + all — the contributor is read from the JWT. It used to accept a + `contributorId`, which let any caller claim a bounty as somebody else and + burn their claim, since `CLAIMED` is a one-way state transition. +- **The funder is the caller.** `funderAddress` on `POST /bounties/:id/fund`, + `/escrow/fund`, `/milestones/:id/fund`, and + `/maintenance-pools/:id/deposit` must equal the caller's own linked + `stellarAddress`, checked before anything is locked. These four routes and + `/claim` require a bearer token for that reason. +- **An attributed recipient must own the address being paid.** Where a request + supplies both `recipientId` and `recipientAddress` (`/escrow/:id/release`, + `/escrow/:id/split-release`, `/milestones/:id/issues/:issueId/resolve`, + `/maintenance-pools/:id/assign-reward`), `EscrowService` rejects the pair + unless the address is the one on file for that user. The check sits in the + service rather than the controllers so that every release path — including + the merge-triggered one — passes through it before reaching Soroban. + +Authorization on the remaining mutating routes is tracked separately; see the +roadmap. + ### Idempotency Every fund/claim/release/refund mutation — `POST /bounties/:id/fund`, @@ -296,6 +321,10 @@ Unit tests cover critical domains including: - `src/github/webhook-signature.util.spec.ts` — GitHub webhook HMAC-SHA256 signature verification. - `src/github/github-webhooks.service.spec.ts` — webhook-to-escrow release logic. - `src/bounties/bounties.service.spec.ts` — bounty core management. +- `src/bounties/bounties.controller.spec.ts` — claim/fund identity binding: the + claimant and funder come from the JWT, not the body (#40). +- `src/users/users-stellar-address-binding.spec.ts` — `funderAddress` must be + the caller's own linked address (#40). - `src/sponsors/sponsors.service.spec.ts` — sponsor dashboard aggregate queries (budgetLocked/totalSpend read the Escrow/Payment ledger directly). - `src/database/escrow-fk-integrity.integration.spec.ts` — **integration** test against a real Postgres (requires `DATABASE_URL`, not mocked): the exactly-one-parent CHECK constraint on `escrows`, and that sponsor dashboard figures survive a parent bounty/milestone being deleted. diff --git a/src/auth/authenticated-request.ts b/src/auth/authenticated-request.ts new file mode 100644 index 0000000..8fbaa24 --- /dev/null +++ b/src/auth/authenticated-request.ts @@ -0,0 +1,26 @@ +import type { Request } from 'express'; + +/** + * Shape of `req.user` on a route behind {@link JwtAuthGuard}. This is exactly + * what `JwtStrategy.validate` returns, and it is deliberately narrow: the JWT + * carries an identity, not a profile. Anything else about the caller — their + * linked `stellarAddress`, their roles — has to be read from the user record, + * because a token claim is a snapshot the client holds and the database row is + * the current truth. + */ +export interface AuthenticatedUser { + userId: string; + username: string; +} + +/** + * Express request on an authenticated route. `user` is non-optional here: the + * guard rejects the request before the handler runs, so any handler typed with + * this has already been proven to have a caller. Handlers that derive + * money-moving identity from the caller should take this type rather than a + * bare `Request`, so that dropping the guard becomes a type error rather than a + * silent `undefined`. + */ +export interface AuthenticatedRequest extends Request { + user: AuthenticatedUser; +} diff --git a/src/auth/strategies/jwt.strategy.ts b/src/auth/strategies/jwt.strategy.ts index 2deaeea..d0b2de2 100644 --- a/src/auth/strategies/jwt.strategy.ts +++ b/src/auth/strategies/jwt.strategy.ts @@ -3,6 +3,7 @@ import { ConfigService } from '@nestjs/config'; import { PassportStrategy } from '@nestjs/passport'; import { ExtractJwt, Strategy } from 'passport-jwt'; import { AppConfig } from '../../config/configuration'; +import type { AuthenticatedUser } from '../authenticated-request'; export interface JwtPayload { sub: string; @@ -19,7 +20,7 @@ export class JwtStrategy extends PassportStrategy(Strategy) { }); } - validate(payload: JwtPayload) { + validate(payload: JwtPayload): AuthenticatedUser { return { userId: payload.sub, username: payload.username }; } } diff --git a/src/bounties/bounties.controller.spec.ts b/src/bounties/bounties.controller.spec.ts new file mode 100644 index 0000000..6c7a9cb --- /dev/null +++ b/src/bounties/bounties.controller.spec.ts @@ -0,0 +1,161 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { ForbiddenException } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { BountiesController } from './bounties.controller'; +import { BountiesService } from './bounties.service'; +import { UsersService } from '../users/users.service'; +import { BountyStatus } from '../common/enums'; +import { IdempotencyKey } from '../common/entities/idempotency-key.entity'; +import { IdempotencyInterceptor } from '../common/idempotency/idempotency.interceptor'; +import type { AuthenticatedRequest } from '../auth/authenticated-request'; + +const USER_A = 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa'; +const USER_B = 'bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb'; + +/** + * Minimal stand-in for the request a JwtAuthGuard-protected handler receives. + * Only `user` is read by these handlers. + */ +const requestAs = (userId: string): AuthenticatedRequest => + ({ user: { userId, username: userId } }) as AuthenticatedRequest; + +/** + * #40: `POST /bounties/:id/claim` used to take `contributorId` from the request + * body, so a caller authenticated as user A could set `claimedById` to user B. + * Because `CLAIMED` is a one-way gate in the bounty state machine, that both + * burned B's chance to claim and — combined with the address IDOR — could + * redirect the eventual payout. + * + * The fix is structural: the handler takes no body at all, so there is no field + * left to spoof. These tests assert the identity actually reaching the service, + * which is what decides `claimedById`. + */ +describe('BountiesController (#40 identity binding)', () => { + let controller: BountiesController; + let bountiesService: { claim: jest.Mock; fund: jest.Mock }; + let usersService: { assertOwnsStellarAddress: jest.Mock }; + + beforeEach(async () => { + bountiesService = { + // Mirrors the real service: whatever id it is handed becomes claimedById. + claim: jest.fn((id: string, contributorId: string) => + Promise.resolve({ + id, + claimedById: contributorId, + status: BountyStatus.CLAIMED, + }), + ), + fund: jest.fn().mockResolvedValue({ id: 'bounty-1' }), + }; + usersService = { + assertOwnsStellarAddress: jest.fn().mockResolvedValue(undefined), + }; + + const module: TestingModule = await Test.createTestingModule({ + controllers: [BountiesController], + providers: [ + { provide: BountiesService, useValue: bountiesService }, + { provide: UsersService, useValue: usersService }, + // These routes carry @Idempotent, which resolves + // IdempotencyInterceptor through DI even though this suite calls + // controller methods directly and never runs the interceptor. + IdempotencyInterceptor, + Reflector, + { provide: getRepositoryToken(IdempotencyKey), useValue: {} }, + ], + }).compile(); + + controller = module.get(BountiesController); + }); + + describe('claim', () => { + it('claims as the authenticated caller, not anyone named by the client', async () => { + const bounty = (await controller.claim( + 'bounty-1', + requestAs(USER_A), + )) as { claimedById: string }; + + expect(bountiesService.claim).toHaveBeenCalledWith('bounty-1', USER_A); + expect(bounty.claimedById).toBe(USER_A); + }); + + it('authenticated user A cannot cause claimedById to be set to user B', async () => { + // The pre-fix exploit: A authenticates as themselves and puts B's id in + // the body. There is no longer a parameter to carry it — the only id the + // handler can reach is the one the guard put on the request — so the + // attempt cannot even be expressed, and B's id must appear nowhere in + // what the service is told. + const bounty = (await controller.claim( + 'bounty-1', + requestAs(USER_A), + )) as { claimedById: string }; + + expect(bountiesService.claim).toHaveBeenCalledTimes(1); + expect(bountiesService.claim).not.toHaveBeenCalledWith( + expect.anything(), + USER_B, + ); + expect(bounty.claimedById).not.toBe(USER_B); + }); + + it('takes no request body, so no body field can influence the claimant', () => { + // Guards against a regression that reintroduces a body parameter: the + // handler's arity is part of the security property here. Bound because + // the arity is all we want, not a callable detached from its instance. + const handler = controller.claim.bind(controller); + + expect(handler).toHaveLength(2); // (id, req) — no body + }); + + it('two different callers claim as themselves', async () => { + await controller.claim('bounty-1', requestAs(USER_A)); + await controller.claim('bounty-2', requestAs(USER_B)); + + expect(bountiesService.claim).toHaveBeenNthCalledWith( + 1, + 'bounty-1', + USER_A, + ); + expect(bountiesService.claim).toHaveBeenNthCalledWith( + 2, + 'bounty-2', + USER_B, + ); + }); + }); + + describe('fund', () => { + it('checks funderAddress against the caller before funding', async () => { + await controller.fund( + 'bounty-1', + { funderAddress: 'GFUNDER' }, + requestAs(USER_A), + ); + + expect(usersService.assertOwnsStellarAddress).toHaveBeenCalledWith( + USER_A, + 'GFUNDER', + ); + expect(bountiesService.fund).toHaveBeenCalledWith('bounty-1', 'GFUNDER'); + }); + + it("does not fund when the address is not the caller's own", async () => { + usersService.assertOwnsStellarAddress.mockRejectedValue( + new ForbiddenException( + 'funderAddress must match your linked Stellar address', + ), + ); + + await expect( + controller.fund( + 'bounty-1', + { funderAddress: 'GSOMEONE_ELSE' }, + requestAs(USER_A), + ), + ).rejects.toThrow(ForbiddenException); + + expect(bountiesService.fund).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/bounties/bounties.controller.ts b/src/bounties/bounties.controller.ts index d2e6056..4d911a3 100644 --- a/src/bounties/bounties.controller.ts +++ b/src/bounties/bounties.controller.ts @@ -1,11 +1,22 @@ -import { Body, Controller, Get, Param, Post, Query } from '@nestjs/common'; -import { ApiTags } from '@nestjs/swagger'; +import { + Body, + Controller, + Get, + Param, + Post, + Query, + Req, + UseGuards, +} from '@nestjs/common'; +import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { BountiesService } from './bounties.service'; import { CreateBountyDto } from './dto/create-bounty.dto'; -import { ClaimBountyDto } from './dto/claim-bounty.dto'; import { BountyStatus } from '../common/enums'; import { Idempotent } from '../common/idempotency/idempotent.decorator'; import { IsStellarAddress } from '../common/validators/stellar-address.validator'; +import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; +import type { AuthenticatedRequest } from '../auth/authenticated-request'; +import { UsersService } from '../users/users.service'; class FundBountyDto { @IsStellarAddress() @@ -15,7 +26,10 @@ class FundBountyDto { @ApiTags('bounties') @Controller('bounties') export class BountiesController { - constructor(private readonly bountiesService: BountiesService) {} + constructor( + private readonly bountiesService: BountiesService, + private readonly usersService: UsersService, + ) {} @Post() create(@Body() dto: CreateBountyDto) { @@ -32,16 +46,42 @@ export class BountiesController { return this.bountiesService.findOne(id); } + /** + * The funder is the caller: this debits their wallet, so `funderAddress` may + * only be the address linked to their own account (#40). + */ + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Idempotent('bounty.fund') @Post(':id/fund') - fund(@Param('id') id: string, @Body() dto: FundBountyDto) { + async fund( + @Param('id') id: string, + @Body() dto: FundBountyDto, + @Req() req: AuthenticatedRequest, + ) { + await this.usersService.assertOwnsStellarAddress( + req.user.userId, + dto.funderAddress, + ); return this.bountiesService.fund(id, dto.funderAddress); } + /** + * Claiming is first-person only. The contributor is read from the verified + * token, never from the body — `CLAIMED` is a one-way gate in the bounty + * state machine, so a client-supplied contributor id let any caller burn + * another user's claim (or point the eventual payout at them) (#40). + * + * There is deliberately no "claim on behalf of" path here. If maintainer-side + * assignment is wanted later it needs its own route and its own authorization + * check, not a field on this one. + */ + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Idempotent('bounty.claim') @Post(':id/claim') - claim(@Param('id') id: string, @Body() dto: ClaimBountyDto) { - return this.bountiesService.claim(id, dto.contributorId); + claim(@Param('id') id: string, @Req() req: AuthenticatedRequest) { + return this.bountiesService.claim(id, req.user.userId); } @Idempotent('bounty.refund') diff --git a/src/bounties/bounties.module.ts b/src/bounties/bounties.module.ts index 2fda357..bd33720 100644 --- a/src/bounties/bounties.module.ts +++ b/src/bounties/bounties.module.ts @@ -4,9 +4,14 @@ import { Bounty, Team, User } from '../common/entities'; import { BountiesService } from './bounties.service'; import { BountiesController } from './bounties.controller'; import { EscrowModule } from '../escrow/escrow.module'; +import { UsersModule } from '../users/users.module'; @Module({ - imports: [TypeOrmModule.forFeature([Bounty, Team, User]), EscrowModule], + imports: [ + TypeOrmModule.forFeature([Bounty, Team, User]), + EscrowModule, + UsersModule, + ], controllers: [BountiesController], providers: [BountiesService], exports: [BountiesService], diff --git a/src/bounties/bounties.service.ts b/src/bounties/bounties.service.ts index 7bf75f8..b0ac561 100644 --- a/src/bounties/bounties.service.ts +++ b/src/bounties/bounties.service.ts @@ -1,4 +1,8 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + NotFoundException, +} from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { Bounty, Team, User } from '../common/entities'; @@ -111,7 +115,10 @@ export class BountiesService { }); return { recipientId: split.userId, - recipientAddress: user?.stellarAddress ?? '', + recipientAddress: this.payoutAddressOrThrow( + user?.stellarAddress, + split.userId, + ), percentage: Number(split.percentage), }; }), @@ -124,7 +131,10 @@ export class BountiesService { }); await this.escrowService.release( bounty.escrowId, - contributor?.stellarAddress ?? '', + this.payoutAddressOrThrow( + contributor?.stellarAddress, + bounty.claimedById, + ), bounty.claimedById, ); } @@ -135,6 +145,28 @@ export class BountiesService { return this.bountyRepo.save(bounty); } + /** + * Resolves a payee's on-chain address, refusing to proceed without one. + * + * This used to fall back to `''`, which meant a merged PR by a contributor + * who had never linked a wallet would call the escrow contract with an empty + * recipient — paying nobody while still marching the bounty on to PAID. The + * escrow layer now rejects that pair outright (#40), so the only thing left to + * decide is which error the operator sees; a missing wallet deserves to say so + * rather than surfacing as a recipient-mismatch further down. + */ + private payoutAddressOrThrow( + stellarAddress: string | null | undefined, + userId: string, + ): string { + if (!stellarAddress) { + throw new BadRequestException( + `Cannot release funds: user ${userId} has no linked Stellar address`, + ); + } + return stellarAddress; + } + /** Sponsor (or admin/expiry job) reclaims escrowed funds. */ async refund(id: string): Promise { const bounty = await this.findOne(id); diff --git a/src/bounties/dto/claim-bounty.dto.ts b/src/bounties/dto/claim-bounty.dto.ts deleted file mode 100644 index a4748ac..0000000 --- a/src/bounties/dto/claim-bounty.dto.ts +++ /dev/null @@ -1,8 +0,0 @@ -import { ApiProperty } from '@nestjs/swagger'; -import { IsUUID } from 'class-validator'; - -export class ClaimBountyDto { - @ApiProperty() - @IsUUID() - contributorId: string; -} diff --git a/src/common/idempotency/idempotency.interceptor.ts b/src/common/idempotency/idempotency.interceptor.ts index d581675..de8a1bb 100644 --- a/src/common/idempotency/idempotency.interceptor.ts +++ b/src/common/idempotency/idempotency.interceptor.ts @@ -113,10 +113,12 @@ interface CachedOutcome { * released instead — because forcing every future retry to replay a server * error forever is worse than letting the retry try again cleanly. * - * Caller scoping: none of the controllers this guards (bounties, escrow, - * milestones, maintenance-pool) currently sit behind JwtAuthGuard, so there - * is no authenticated caller to scope by yet. `resolveCallerId` falls back - * to a shared 'anonymous' bucket per scope in that case — see its doc + * Caller scoping: the routes that derive identity from the caller — the four + * funding routes and `bounty.claim` — now sit behind JwtAuthGuard (#40), so + * `resolveCallerId` returns a real `req.user.userId` and their keys are scoped + * per user. The remaining guarded routes (release, split-release, refund, + * milestone resolve, pool assign-reward) are still unauthenticated and fall + * back to a shared 'anonymous' bucket per scope — see `resolveCallerId`'s doc * comment for what that does and doesn't protect against. * * Request identity: `scope` is a static string per route (e.g. @@ -211,8 +213,8 @@ export class IdempotencyInterceptor implements NestInterceptor { /** * Determines the bucket a key is scoped to. `req.user.userId` is used - * when the route is authenticated (none of the current target routes - * are — see class doc comment). The 'anonymous' fallback still gives + * when the route is authenticated (some now are — see class doc + * comment). The 'anonymous' fallback still gives * correct duplicate-suppression and concurrency-safety for a single * client retrying its own request, since that's driven entirely by the * (key, scope, callerId) uniqueness, not by callerId being a *real* diff --git a/src/escrow/dto/fund-escrow.dto.ts b/src/escrow/dto/fund-escrow.dto.ts index 30099e5..117258c 100644 --- a/src/escrow/dto/fund-escrow.dto.ts +++ b/src/escrow/dto/fund-escrow.dto.ts @@ -16,7 +16,16 @@ export class FundEscrowDto { @IsSupportedEscrowAsset() asset: AssetType; - @ApiProperty({ description: 'Stellar public key of the funding sponsor' }) + /** + * Not permissionless: this endpoint requires a JWT, and the controller + * asserts this equals the caller's own linked `stellarAddress` before + * anything is locked. Funding is a debit of the named wallet, so naming + * someone else's is never a legitimate request (#40). + */ + @ApiProperty({ + description: + "Stellar public key of the funding sponsor. Must equal the caller's own linked Stellar address.", + }) @IsStellarAddress() funderAddress: string; diff --git a/src/escrow/dto/release-escrow.dto.ts b/src/escrow/dto/release-escrow.dto.ts index a0a44da..4255407 100644 --- a/src/escrow/dto/release-escrow.dto.ts +++ b/src/escrow/dto/release-escrow.dto.ts @@ -7,7 +7,17 @@ export class ReleaseEscrowDto { @IsStellarAddress() recipientAddress: string; - @ApiProperty({ required: false }) + /** + * Who the resulting `Payment` row is attributed to. When present, the pair is + * cross-checked server-side in `EscrowService` — + * `recipientAddress` must equal this user's linked `stellarAddress`, so the + * ledger cannot name one party while the chain pays another (#40). + */ + @ApiProperty({ + required: false, + description: + 'Attributed recipient. Must match the linked Stellar address of this user; a mismatched pair is rejected.', + }) @IsOptional() @IsUUID() recipientId?: string; diff --git a/src/escrow/dto/split-release.dto.ts b/src/escrow/dto/split-release.dto.ts index d519a45..92f4221 100644 --- a/src/escrow/dto/split-release.dto.ts +++ b/src/escrow/dto/split-release.dto.ts @@ -16,7 +16,15 @@ export class SplitRecipientDto { @IsStellarAddress() recipientAddress: string; - @ApiProperty({ required: false }) + /** + * Cross-checked against `recipientAddress` per entry in `EscrowService` + * before any chain call — see `ReleaseEscrowDto.recipientId` (#40). + */ + @ApiProperty({ + required: false, + description: + 'Attributed recipient. Must match the linked Stellar address of this user; a mismatched pair is rejected.', + }) @IsOptional() @IsUUID() recipientId?: string; diff --git a/src/escrow/escrow.controller.spec.ts b/src/escrow/escrow.controller.spec.ts index 7673cd1..354c8ba 100644 --- a/src/escrow/escrow.controller.spec.ts +++ b/src/escrow/escrow.controller.spec.ts @@ -1,12 +1,22 @@ import { Test, TestingModule } from '@nestjs/testing'; +import { ForbiddenException } from '@nestjs/common'; import { Reflector } from '@nestjs/core'; import { getRepositoryToken } from '@nestjs/typeorm'; import { EscrowController } from './escrow.controller'; import { EscrowService } from './escrow.service'; +import { UsersService } from '../users/users.service'; import { AssetType, EscrowStatus } from '../common/enums'; import { Escrow } from '../common/entities'; import { IdempotencyKey } from '../common/entities/idempotency-key.entity'; import { IdempotencyInterceptor } from '../common/idempotency/idempotency.interceptor'; +import type { AuthenticatedRequest } from '../auth/authenticated-request'; + +/** + * Minimal stand-in for the request object a JwtAuthGuard-protected handler + * receives. Only `user` is read by these handlers. + */ +const requestAs = (userId: string): AuthenticatedRequest => + ({ user: { userId, username: userId } }) as AuthenticatedRequest; function makeEscrowWithLeakyMetadata(): Escrow { return { @@ -48,8 +58,12 @@ describe('EscrowController (#19 metadata leak)', () => { refund: jest.Mock; splitRelease: jest.Mock; }; + let usersService: { assertOwnsStellarAddress: jest.Mock }; beforeEach(async () => { + usersService = { + assertOwnsStellarAddress: jest.fn().mockResolvedValue(undefined), + }; escrowService = { fund: jest.fn().mockResolvedValue(makeEscrowWithLeakyMetadata()), findOne: jest.fn().mockResolvedValue(makeEscrowWithLeakyMetadata()), @@ -62,6 +76,7 @@ describe('EscrowController (#19 metadata leak)', () => { controllers: [EscrowController], providers: [ { provide: EscrowService, useValue: escrowService }, + { provide: UsersService, useValue: usersService }, // These endpoints carry @Idempotent, which resolves // IdempotencyInterceptor via DI even though this suite calls // controller methods directly and never runs the interceptor @@ -79,12 +94,15 @@ describe('EscrowController (#19 metadata leak)', () => { }); it('fund() never returns metadata to the client', async () => { - const result = await controller.fund({ - amount: '100', - asset: AssetType.USDC, - funderAddress: 'GFUNDER', - bountyId: 'bounty_1', - }); + const result = await controller.fund( + { + amount: '100', + asset: AssetType.USDC, + funderAddress: 'GFUNDER', + bountyId: 'bounty_1', + }, + requestAs('user_1'), + ); expect(result).not.toHaveProperty('metadata'); expect(JSON.stringify(result)).not.toContain('internal RPC detail'); @@ -110,4 +128,45 @@ describe('EscrowController (#19 metadata leak)', () => { expect(result).not.toHaveProperty('metadata'); }); + + describe('#40 funderAddress is bound to the caller', () => { + it('checks funderAddress against the caller before funding anything', async () => { + await controller.fund( + { + amount: '100', + asset: AssetType.USDC, + funderAddress: 'GFUNDER', + bountyId: 'bounty_1', + }, + requestAs('user_a'), + ); + + expect(usersService.assertOwnsStellarAddress).toHaveBeenCalledWith( + 'user_a', + 'GFUNDER', + ); + }); + + it("does not fund when the address is not the caller's own", async () => { + usersService.assertOwnsStellarAddress.mockRejectedValue( + new ForbiddenException( + 'funderAddress must match your linked Stellar address', + ), + ); + + await expect( + controller.fund( + { + amount: '100', + asset: AssetType.USDC, + funderAddress: 'GSOMEONE_ELSE', + bountyId: 'bounty_1', + }, + requestAs('user_a'), + ), + ).rejects.toThrow(ForbiddenException); + + expect(escrowService.fund).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/escrow/escrow.controller.ts b/src/escrow/escrow.controller.ts index 8387138..ebf9682 100644 --- a/src/escrow/escrow.controller.ts +++ b/src/escrow/escrow.controller.ts @@ -1,20 +1,44 @@ -import { Body, Controller, Get, Param, Post } from '@nestjs/common'; -import { ApiTags } from '@nestjs/swagger'; +import { + Body, + Controller, + Get, + Param, + Post, + Req, + UseGuards, +} from '@nestjs/common'; +import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { EscrowService } from './escrow.service'; import { FundEscrowDto } from './dto/fund-escrow.dto'; import { ReleaseEscrowDto } from './dto/release-escrow.dto'; import { SplitReleaseDto } from './dto/split-release.dto'; import { toPublicEscrow } from './escrow-response.mapper'; import { Idempotent } from '../common/idempotency/idempotent.decorator'; +import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; +import type { AuthenticatedRequest } from '../auth/authenticated-request'; +import { UsersService } from '../users/users.service'; @ApiTags('escrow') @Controller('escrow') export class EscrowController { - constructor(private readonly escrowService: EscrowService) {} + constructor( + private readonly escrowService: EscrowService, + private readonly usersService: UsersService, + ) {} + /** + * The funder is the caller: this debits their wallet, so `funderAddress` may + * only be the address linked to their own account (#40). + */ + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Idempotent('escrow.fund') @Post('fund') - async fund(@Body() dto: FundEscrowDto) { + async fund(@Body() dto: FundEscrowDto, @Req() req: AuthenticatedRequest) { + await this.usersService.assertOwnsStellarAddress( + req.user.userId, + dto.funderAddress, + ); return toPublicEscrow(await this.escrowService.fund(dto)); } diff --git a/src/escrow/escrow.module.ts b/src/escrow/escrow.module.ts index 2765bfd..c4bf0d8 100644 --- a/src/escrow/escrow.module.ts +++ b/src/escrow/escrow.module.ts @@ -4,9 +4,13 @@ import { Escrow, Payment } from '../common/entities'; import { EscrowService } from './escrow.service'; import { EscrowController } from './escrow.controller'; import { SorobanClientService } from './soroban-client.service'; +import { UsersModule } from '../users/users.module'; @Module({ - imports: [TypeOrmModule.forFeature([Escrow, Payment])], + // UsersModule supplies the user-record lookup behind the + // recipientId/recipientAddress cross-check (#40). It depends on nothing in + // this module, so the edge stays one-way. + imports: [TypeOrmModule.forFeature([Escrow, Payment]), UsersModule], controllers: [EscrowController], providers: [EscrowService, SorobanClientService], exports: [EscrowService, SorobanClientService], diff --git a/src/escrow/escrow.service.spec.ts b/src/escrow/escrow.service.spec.ts index dd9dd9e..453ead9 100644 --- a/src/escrow/escrow.service.spec.ts +++ b/src/escrow/escrow.service.spec.ts @@ -3,14 +3,16 @@ import { getRepositoryToken } from '@nestjs/typeorm'; import { BadRequestException } from '@nestjs/common'; import { EscrowService } from './escrow.service'; import { SorobanClientService } from './soroban-client.service'; +import { UsersService } from '../users/users.service'; import { Escrow, Payment } from '../common/entities'; import { AssetType, EscrowStatus } from '../common/enums'; describe('EscrowService', () => { let service: EscrowService; let escrowRepo: { create: jest.Mock; save: jest.Mock; findOne: jest.Mock }; - let paymentRepo: { create: jest.Mock; save: jest.Mock }; + let paymentRepo: { create: jest.Mock; save: jest.Mock; find: jest.Mock }; let soroban: { invoke: jest.Mock }; + let usersService: { findRawOrNull: jest.Mock }; beforeEach(async () => { escrowRepo = { @@ -24,6 +26,8 @@ describe('EscrowService', () => { ...data, })), save: jest.fn((data: Partial) => Promise.resolve(data)), + // releasePartial() sums prior payments to work out the remaining balance. + find: jest.fn().mockResolvedValue([]), }; soroban = { invoke: jest.fn().mockResolvedValue({ @@ -33,6 +37,10 @@ describe('EscrowService', () => { status: 'SUCCESS', }), }; + // Backs the recipientId/recipientAddress cross-check (#40). Defaults to + // "no such user", so any test that passes a recipientId has to say what + // that user's address is on purpose. + usersService = { findRawOrNull: jest.fn().mockResolvedValue(null) }; const module: TestingModule = await Test.createTestingModule({ providers: [ @@ -40,6 +48,7 @@ describe('EscrowService', () => { { provide: getRepositoryToken(Escrow), useValue: escrowRepo }, { provide: getRepositoryToken(Payment), useValue: paymentRepo }, { provide: SorobanClientService, useValue: soroban }, + { provide: UsersService, useValue: usersService }, ], }).compile(); @@ -205,6 +214,10 @@ describe('EscrowService', () => { asset: AssetType.USDC, bountyId: 'bounty-3', }); + usersService.findRawOrNull.mockResolvedValue({ + id: 'user-1', + stellarAddress: 'GRECIPIENT', + }); const escrow = await service.release('escrow-3', 'GRECIPIENT', 'user-1'); @@ -302,4 +315,157 @@ describe('EscrowService', () => { expect(bps.reduce((a, b) => a + b, 0)).toBe(10_000); }); }); + + /** + * #40: `recipientAddress` decides who the chain pays, `recipientId` decides + * who the Payment row credits, and nothing tied them together — so a caller + * could pay one address while attributing the payment to someone else. + * + * These exercise the check through the public release methods rather than + * calling the private helper directly, because the property that matters is + * that every release path actually routes through it before invoking Soroban. + * A test of the helper alone would still pass if the call site were deleted. + */ + describe('recipientId/recipientAddress cross-check (#40)', () => { + // A factory, not a shared constant: release() mutates the escrow it is + // handed, so a shared object would leak RELEASED into the next test. + const lockedEscrow = () => ({ + id: 'escrow-40', + status: EscrowStatus.LOCKED, + amount: '100', + asset: AssetType.USDC, + bountyId: 'bounty-40', + }); + + beforeEach(() => { + escrowRepo.findOne.mockImplementation(() => + Promise.resolve(lockedEscrow()), + ); + usersService.findRawOrNull.mockResolvedValue({ + id: 'user-b', + stellarAddress: 'GABCONFILE', + }); + }); + + it('release() rejects a mismatched pair before any Soroban call', async () => { + await expect( + service.release('escrow-40', 'GDIFFERENT', 'user-b'), + ).rejects.toThrow(BadRequestException); + + expect(soroban.invoke).not.toHaveBeenCalled(); + expect(paymentRepo.save).not.toHaveBeenCalled(); + expect(escrowRepo.save).not.toHaveBeenCalled(); + }); + + it('release() names neither address in the rejection message', async () => { + // The message must not become an oracle for what a given user id's + // address actually is. + await expect( + service.release('escrow-40', 'GDIFFERENT', 'user-b'), + ).rejects.toThrow( + 'recipientAddress does not match the address on file for recipientId', + ); + }); + + it('release() accepts a matching pair and reaches the chain', async () => { + const escrow = await service.release('escrow-40', 'GABCONFILE', 'user-b'); + + expect(soroban.invoke).toHaveBeenCalledWith( + 'release', + expect.arrayContaining(['GABCONFILE']), + ); + expect(escrow.status).toBe(EscrowStatus.RELEASED); + expect(paymentRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + recipientId: 'user-b', + recipientAddress: 'GABCONFILE', + }), + ); + }); + + it('release() still allows an unattributed payout (no recipientId)', async () => { + await service.release('escrow-40', 'GANYADDRESS'); + + expect(usersService.findRawOrNull).not.toHaveBeenCalled(); + expect(soroban.invoke).toHaveBeenCalled(); + }); + + it('release() rejects a recipientId that names no user', async () => { + usersService.findRawOrNull.mockResolvedValue(null); + + await expect( + service.release('escrow-40', 'GANYADDRESS', 'ghost-user'), + ).rejects.toThrow(BadRequestException); + expect(soroban.invoke).not.toHaveBeenCalled(); + }); + + it('release() rejects a recipient whose record has no linked address', async () => { + // Otherwise an unlinked user's null address would match any string the + // caller supplied. + usersService.findRawOrNull.mockResolvedValue({ + id: 'user-b', + stellarAddress: null, + }); + + await expect( + service.release('escrow-40', 'GANYADDRESS', 'user-b'), + ).rejects.toThrow(BadRequestException); + expect(soroban.invoke).not.toHaveBeenCalled(); + }); + + it('splitRelease() rejects when any one recipient in the split mismatches', async () => { + usersService.findRawOrNull.mockImplementation((id: string) => + Promise.resolve( + id === 'user-good' + ? { id, stellarAddress: 'GGOOD' } + : { id, stellarAddress: 'GONFILE' }, + ), + ); + + await expect( + service.splitRelease('escrow-40', [ + { + recipientAddress: 'GGOOD', + recipientId: 'user-good', + percentage: 50, + }, + { + recipientAddress: 'GATTACKER', + recipientId: 'user-bad', + percentage: 50, + }, + ]), + ).rejects.toThrow(BadRequestException); + + expect(soroban.invoke).not.toHaveBeenCalled(); + expect(paymentRepo.save).not.toHaveBeenCalled(); + }); + + it('splitRelease() accepts a split whose every attributed pair matches', async () => { + usersService.findRawOrNull.mockImplementation((id: string) => + Promise.resolve({ + id, + stellarAddress: id === 'user-1' ? 'GONE' : 'GTWO', + }), + ); + + const payments = await service.splitRelease('escrow-40', [ + { recipientAddress: 'GONE', recipientId: 'user-1', percentage: 50 }, + { recipientAddress: 'GTWO', recipientId: 'user-2', percentage: 50 }, + ]); + + expect(payments).toHaveLength(2); + expect(soroban.invoke).toHaveBeenCalled(); + }); + + it('releasePartial() rejects a mismatched pair before any Soroban call', async () => { + // Reached by the milestone-resolve and pool-assign-reward routes, which + // accept the same pair from the client. + await expect( + service.releasePartial('escrow-40', '10', 'GDIFFERENT', 'user-b'), + ).rejects.toThrow(BadRequestException); + + expect(soroban.invoke).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/escrow/escrow.service.ts b/src/escrow/escrow.service.ts index 9986857..04a1f5c 100644 --- a/src/escrow/escrow.service.ts +++ b/src/escrow/escrow.service.ts @@ -16,6 +16,7 @@ import { } from '../common/validators/money.validator'; import { SorobanClientService } from './soroban-client.service'; import { apportionBasisPoints, splitStroops } from './split-math.util'; +import { UsersService } from '../users/users.service'; export interface FundEscrowInput { amount: string; @@ -49,6 +50,7 @@ export class EscrowService { @InjectRepository(Payment) private readonly paymentRepo: Repository, private readonly soroban: SorobanClientService, + private readonly usersService: UsersService, ) {} /** Locks funds for a bounty/milestone/pool by calling the escrow contract's `fund`. */ @@ -98,6 +100,8 @@ export class EscrowService { recipientAddress: string, recipientId?: string, ): Promise { + await this.assertRecipientAddressMatchesUser(recipientId, recipientAddress); + const escrow = await this.getOrThrow(escrowId); this.assertLocked(escrow); @@ -143,6 +147,13 @@ export class EscrowService { escrowId: string, recipients: SplitRecipient[], ): Promise { + for (const recipient of recipients) { + await this.assertRecipientAddressMatchesUser( + recipient.recipientId, + recipient.recipientAddress, + ); + } + const escrow = await this.getOrThrow(escrowId); this.assertLocked(escrow); this.assertValidSplits(recipients); @@ -198,6 +209,8 @@ export class EscrowService { recipientAddress: string, recipientId?: string, ): Promise { + await this.assertRecipientAddressMatchesUser(recipientId, recipientAddress); + const escrow = await this.getOrThrow(escrowId); this.assertLocked(escrow); this.assertValidAmount(amount); @@ -280,6 +293,44 @@ export class EscrowService { } } + /** + * Rejects a `recipientId`/`recipientAddress` pair that the user record does + * not agree with. + * + * `recipientAddress` decides who is actually paid on-chain; `recipientId` + * decides who the resulting `Payment` row is attributed to. Nothing tied the + * two together, so a caller could name one party in the ledger and pay a + * different address entirely (#40). Both are supplied together only by the + * HTTP surface — the internal callers + * (`BountiesService.markMergedAndRelease`) already derive the address from + * the user record, so for them this is a no-op restatement of an invariant + * they hold anyway. + * + * This lives in the service rather than the controller on purpose: it is the + * last common point every release path passes through, so no future route, + * job, or event handler can reach the chain around it. + * + * A `recipientId` that names no user, or one whose record has no linked + * address, is rejected with the same message as a genuine mismatch — the + * distinction is not useful to a caller and spelling it out would turn the + * endpoint into an oracle for which user ids exist. + */ + private async assertRecipientAddressMatchesUser( + recipientId: string | undefined, + recipientAddress: string, + ): Promise { + // Attribution is optional; when it's absent there is no claimed pairing to + // disprove, and the address stands on its own as it did before. + if (!recipientId) return; + + const user = await this.usersService.findRawOrNull(recipientId); + if (!user?.stellarAddress || user.stellarAddress !== recipientAddress) { + throw new BadRequestException( + 'recipientAddress does not match the address on file for recipientId', + ); + } + } + /** Validates that split percentages sum to 100.00, within floating point tolerance. */ assertValidSplits(recipients: SplitRecipient[]): void { if (recipients.length === 0) { diff --git a/src/maintenance-pool/maintenance-pool.controller.ts b/src/maintenance-pool/maintenance-pool.controller.ts index 4440c7d..f72886c 100644 --- a/src/maintenance-pool/maintenance-pool.controller.ts +++ b/src/maintenance-pool/maintenance-pool.controller.ts @@ -1,16 +1,31 @@ -import { Body, Controller, Get, Param, Post } from '@nestjs/common'; -import { ApiTags } from '@nestjs/swagger'; +import { + Body, + Controller, + Get, + Param, + Post, + Req, + UseGuards, +} from '@nestjs/common'; +import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { IsOptional, IsUUID } from 'class-validator'; import { MaintenancePoolService } from './maintenance-pool.service'; import { CreatePoolDto } from './dto/create-pool.dto'; import { IsMoneyAmount } from '../common/validators/money.validator'; import { IsStellarAddress } from '../common/validators/stellar-address.validator'; import { Idempotent } from '../common/idempotency/idempotent.decorator'; +import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; +import type { AuthenticatedRequest } from '../auth/authenticated-request'; +import { UsersService } from '../users/users.service'; class DepositDto { @IsMoneyAmount() amount: string; + /** + * Not permissionless: asserted against the caller's own linked + * `stellarAddress` below (#40). + */ @IsStellarAddress() funderAddress: string; } @@ -22,6 +37,10 @@ class AssignRewardDto { @IsStellarAddress() recipientAddress: string; + /** + * Cross-checked against `recipientAddress` in `EscrowService.releasePartial` + * before any chain call — see `ReleaseEscrowDto.recipientId` (#40). + */ @IsOptional() @IsUUID() recipientId?: string; @@ -30,7 +49,10 @@ class AssignRewardDto { @ApiTags('maintenance-pool') @Controller('maintenance-pools') export class MaintenancePoolController { - constructor(private readonly poolService: MaintenancePoolService) {} + constructor( + private readonly poolService: MaintenancePoolService, + private readonly usersService: UsersService, + ) {} @Post() create(@Body() dto: CreatePoolDto) { @@ -47,9 +69,23 @@ export class MaintenancePoolController { return this.poolService.findOne(id); } + /** + * The funder is the caller: this debits their wallet, so `funderAddress` may + * only be the address linked to their own account (#40). + */ + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Idempotent('pool.deposit') @Post(':id/deposit') - deposit(@Param('id') id: string, @Body() dto: DepositDto) { + async deposit( + @Param('id') id: string, + @Body() dto: DepositDto, + @Req() req: AuthenticatedRequest, + ) { + await this.usersService.assertOwnsStellarAddress( + req.user.userId, + dto.funderAddress, + ); return this.poolService.deposit(id, dto.amount, dto.funderAddress); } diff --git a/src/maintenance-pool/maintenance-pool.module.ts b/src/maintenance-pool/maintenance-pool.module.ts index 1f29bed..99f6249 100644 --- a/src/maintenance-pool/maintenance-pool.module.ts +++ b/src/maintenance-pool/maintenance-pool.module.ts @@ -4,9 +4,14 @@ import { MaintenancePool } from '../common/entities'; import { MaintenancePoolService } from './maintenance-pool.service'; import { MaintenancePoolController } from './maintenance-pool.controller'; import { EscrowModule } from '../escrow/escrow.module'; +import { UsersModule } from '../users/users.module'; @Module({ - imports: [TypeOrmModule.forFeature([MaintenancePool]), EscrowModule], + imports: [ + TypeOrmModule.forFeature([MaintenancePool]), + EscrowModule, + UsersModule, + ], controllers: [MaintenancePoolController], providers: [MaintenancePoolService], exports: [MaintenancePoolService], diff --git a/src/milestones/milestones.controller.ts b/src/milestones/milestones.controller.ts index be211fd..7867fd5 100644 --- a/src/milestones/milestones.controller.ts +++ b/src/milestones/milestones.controller.ts @@ -1,12 +1,27 @@ -import { Body, Controller, Get, Param, Post } from '@nestjs/common'; -import { ApiTags } from '@nestjs/swagger'; +import { + Body, + Controller, + Get, + Param, + Post, + Req, + UseGuards, +} from '@nestjs/common'; +import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { IsOptional, IsUUID } from 'class-validator'; import { MilestonesService } from './milestones.service'; import { CreateMilestoneDto } from './dto/create-milestone.dto'; import { Idempotent } from '../common/idempotency/idempotent.decorator'; import { IsStellarAddress } from '../common/validators/stellar-address.validator'; +import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; +import type { AuthenticatedRequest } from '../auth/authenticated-request'; +import { UsersService } from '../users/users.service'; class FundMilestoneDto { + /** + * Not permissionless: asserted against the caller's own linked + * `stellarAddress` below (#40). + */ @IsStellarAddress() funderAddress: string; } @@ -15,6 +30,10 @@ class ResolveIssueDto { @IsStellarAddress() recipientAddress: string; + /** + * Cross-checked against `recipientAddress` in `EscrowService.releasePartial` + * before any chain call — see `ReleaseEscrowDto.recipientId` (#40). + */ @IsOptional() @IsUUID() recipientId?: string; @@ -23,7 +42,10 @@ class ResolveIssueDto { @ApiTags('milestones') @Controller('milestones') export class MilestonesController { - constructor(private readonly milestonesService: MilestonesService) {} + constructor( + private readonly milestonesService: MilestonesService, + private readonly usersService: UsersService, + ) {} @Post() create(@Body() dto: CreateMilestoneDto) { @@ -40,9 +62,23 @@ export class MilestonesController { return this.milestonesService.findOne(id); } + /** + * The funder is the caller: this debits their wallet, so `funderAddress` may + * only be the address linked to their own account (#40). + */ + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Idempotent('milestone.fund') @Post(':id/fund') - fund(@Param('id') id: string, @Body() dto: FundMilestoneDto) { + async fund( + @Param('id') id: string, + @Body() dto: FundMilestoneDto, + @Req() req: AuthenticatedRequest, + ) { + await this.usersService.assertOwnsStellarAddress( + req.user.userId, + dto.funderAddress, + ); return this.milestonesService.fund(id, dto.funderAddress); } diff --git a/src/milestones/milestones.module.ts b/src/milestones/milestones.module.ts index 24a31f4..991ab12 100644 --- a/src/milestones/milestones.module.ts +++ b/src/milestones/milestones.module.ts @@ -4,9 +4,14 @@ import { Issue, Milestone } from '../common/entities'; import { MilestonesService } from './milestones.service'; import { MilestonesController } from './milestones.controller'; import { EscrowModule } from '../escrow/escrow.module'; +import { UsersModule } from '../users/users.module'; @Module({ - imports: [TypeOrmModule.forFeature([Milestone, Issue]), EscrowModule], + imports: [ + TypeOrmModule.forFeature([Milestone, Issue]), + EscrowModule, + UsersModule, + ], controllers: [MilestonesController], providers: [MilestonesService], exports: [MilestonesService], diff --git a/src/users/users-stellar-address-binding.spec.ts b/src/users/users-stellar-address-binding.spec.ts new file mode 100644 index 0000000..2566a41 --- /dev/null +++ b/src/users/users-stellar-address-binding.spec.ts @@ -0,0 +1,70 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { ForbiddenException } from '@nestjs/common'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { UsersService } from './users.service'; +import { GithubAccount, User } from '../common/entities'; + +/** + * #40: funding endpoints take a `funderAddress` in the body. The wallet being + * debited has to be the caller's own, so the address is checked against the user + * record rather than trusted. + */ +describe('UsersService.assertOwnsStellarAddress (#40)', () => { + let service: UsersService; + let userRepo: { findOne: jest.Mock }; + + beforeEach(async () => { + userRepo = { findOne: jest.fn() }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + UsersService, + { provide: getRepositoryToken(User), useValue: userRepo }, + { provide: getRepositoryToken(GithubAccount), useValue: {} }, + ], + }).compile(); + + service = module.get(UsersService); + }); + + it('passes when the address is the one linked to the caller', async () => { + userRepo.findOne.mockResolvedValue({ + id: 'user-a', + stellarAddress: 'GMINE', + }); + + await expect( + service.assertOwnsStellarAddress('user-a', 'GMINE'), + ).resolves.toBeUndefined(); + }); + + it("rejects funding that names someone else's address", async () => { + userRepo.findOne.mockResolvedValue({ + id: 'user-a', + stellarAddress: 'GMINE', + }); + + await expect( + service.assertOwnsStellarAddress('user-a', 'GTHEIRS'), + ).rejects.toThrow(ForbiddenException); + }); + + it('rejects a caller with no linked address rather than matching anything', async () => { + userRepo.findOne.mockResolvedValue({ + id: 'user-a', + stellarAddress: null, + }); + + await expect( + service.assertOwnsStellarAddress('user-a', 'GANYTHING'), + ).rejects.toThrow(ForbiddenException); + }); + + it('rejects a token whose user no longer exists', async () => { + userRepo.findOne.mockResolvedValue(null); + + await expect( + service.assertOwnsStellarAddress('ghost', 'GANYTHING'), + ).rejects.toThrow(ForbiddenException); + }); +}); diff --git a/src/users/users.service.ts b/src/users/users.service.ts index 267453f..d7d9c8f 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'; @@ -34,11 +38,42 @@ export class UsersService { return user; } + /** + * Like {@link findOneRaw} but yields null for an unknown id instead of + * throwing. Validation callers need this: "no such user" and "user exists but + * the address doesn't match" must be answerable without a 404 escaping as the + * response to what is really a bad-input problem. + */ + async findRawOrNull(id: string): Promise { + return this.userRepo.findOne({ where: { id } }); + } + async findById(id: string): Promise { const user = await this.findOneRaw(id); return toPublicUser(user); } + /** + * Asserts that `address` is the Stellar address currently linked to `userId`. + * + * Used on funding endpoints, where the caller is the party whose wallet is + * being debited, so the address in the body is only ever allowed to be their + * own. A user with no linked address cannot fund at all — there is nothing to + * match against, and treating "unlinked" as "matches anything" would reopen + * the hole this closes. + */ + async assertOwnsStellarAddress( + userId: string, + address: string, + ): Promise { + const user = await this.findRawOrNull(userId); + if (!user?.stellarAddress || user.stellarAddress !== address) { + throw new ForbiddenException( + 'funderAddress must match your linked Stellar address', + ); + } + } + async findByUsername(username: string): Promise { return this.userRepo.findOne({ where: { username } }); } diff --git a/test/escrow-idempotency.e2e-spec.ts b/test/escrow-idempotency.e2e-spec.ts index 1307a55..ce2f6cd 100644 --- a/test/escrow-idempotency.e2e-spec.ts +++ b/test/escrow-idempotency.e2e-spec.ts @@ -5,6 +5,7 @@ import { getRepositoryToken } from '@nestjs/typeorm'; import request from 'supertest'; import { EscrowController } from '../src/escrow/escrow.controller'; import { EscrowService } from '../src/escrow/escrow.service'; +import { UsersService } from '../src/users/users.service'; import { IdempotencyKey } from '../src/common/entities/idempotency-key.entity'; import { IdempotencyInterceptor } from '../src/common/idempotency/idempotency.interceptor'; import { IdempotencyKeyStatus } from '../src/common/enums'; @@ -101,6 +102,13 @@ describe('Escrow idempotency: cross-resource key reuse (#54)', () => { controllers: [EscrowController], providers: [ { provide: EscrowService, useValue: escrowService }, + // EscrowController resolves UsersService for the funderAddress + // ownership check on POST /escrow/fund (#40). This suite only exercises + // the release route, so a bare stub is enough to satisfy DI. + { + provide: UsersService, + useValue: { assertOwnsStellarAddress: jest.fn() }, + }, IdempotencyInterceptor, Reflector, { From 75c9c5b0df3114e4a97b97c2d36401033b22a012 Mon Sep 17 00:00:00 2001 From: davieslennox <102302431+davieslennox0@users.noreply.github.com> Date: Fri, 21 Aug 2026 07:01:32 +0000 Subject: [PATCH 2/2] test(#60): adapt address-validation e2e specs to the new auth binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #60 boundary specs construct their TestingModules from a bare controller list. After #40 all four of those controllers inject UsersService and the funding routes carry JwtAuthGuard, so the modules no longer compile and every funding assertion would answer 401 instead of the 400/201 the specs are asserting. Stub the guard to a fixed caller and make the ownership assertion a no-op, so these specs keep testing what they were written to test — StrKey validation at the HTTP boundary — rather than auth. Co-Authored-By: Claude Opus 5 --- ...validation-bounties-milestones.e2e-spec.ts | 33 +++++++++++++++++-- ...llar-address-validation-escrow.e2e-spec.ts | 33 +++++++++++++++++-- ...ss-validation-maintenance-pool.e2e-spec.ts | 33 +++++++++++++++++-- 3 files changed, 93 insertions(+), 6 deletions(-) diff --git a/test/stellar-address-validation-bounties-milestones.e2e-spec.ts b/test/stellar-address-validation-bounties-milestones.e2e-spec.ts index 188c8b6..78aa34e 100644 --- a/test/stellar-address-validation-bounties-milestones.e2e-spec.ts +++ b/test/stellar-address-validation-bounties-milestones.e2e-spec.ts @@ -1,10 +1,17 @@ -import { INestApplication, ValidationPipe } from '@nestjs/common'; +import { + ExecutionContext, + INestApplication, + ValidationPipe, +} from '@nestjs/common'; import { Reflector } from '@nestjs/core'; import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; import request from 'supertest'; import { randomUUID } from 'crypto'; import { Keypair, StrKey } from '@stellar/stellar-sdk'; +import { UsersService } from '../src/users/users.service'; +import { JwtAuthGuard } from '../src/auth/guards/jwt-auth.guard'; +import type { AuthenticatedRequest } from '../src/auth/authenticated-request'; import { BountiesController } from '../src/bounties/bounties.controller'; import { BountiesService } from '../src/bounties/bounties.service'; import { MilestonesController } from '../src/milestones/milestones.controller'; @@ -89,6 +96,24 @@ function newFakeRepoProvider() { }; } +/** + * These endpoints became JWT-guarded in #40 (funding debits the caller's own + * wallet). This suite is about address validation at the HTTP boundary, not + * about auth, so the guard is stubbed to a fixed caller and UsersService's + * ownership assertion is a no-op — leaving both real would turn every + * assertion below into a 401 and stop testing #60 entirely. + */ +const authedUser = { userId: 'user_1', username: 'octocat' }; +const passingGuard = { + canActivate: (ctx: ExecutionContext) => { + ctx.switchToHttp().getRequest().user = authedUser; + return true; + }, +}; +const usersServiceStub = { + assertOwnsStellarAddress: jest.fn().mockResolvedValue(undefined), +}; + describe('Stellar address validation at the API boundary — bounties & milestones endpoints (#60)', () => { let app: INestApplication; let bountiesService: { fund: jest.Mock }; @@ -108,11 +133,15 @@ describe('Stellar address validation at the API boundary — bounties & mileston providers: [ { provide: BountiesService, useValue: bountiesService }, { provide: MilestonesService, useValue: milestonesService }, + { provide: UsersService, useValue: usersServiceStub }, IdempotencyInterceptor, Reflector, newFakeRepoProvider(), ], - }).compile(); + }) + .overrideGuard(JwtAuthGuard) + .useValue(passingGuard) + .compile(); app = moduleFixture.createNestApplication(); app.useGlobalPipes( diff --git a/test/stellar-address-validation-escrow.e2e-spec.ts b/test/stellar-address-validation-escrow.e2e-spec.ts index 862a935..7000b1b 100644 --- a/test/stellar-address-validation-escrow.e2e-spec.ts +++ b/test/stellar-address-validation-escrow.e2e-spec.ts @@ -1,10 +1,17 @@ -import { INestApplication, ValidationPipe } from '@nestjs/common'; +import { + ExecutionContext, + INestApplication, + ValidationPipe, +} from '@nestjs/common'; import { Reflector } from '@nestjs/core'; import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; import request from 'supertest'; import { randomUUID } from 'crypto'; import { Keypair, StrKey } from '@stellar/stellar-sdk'; +import { UsersService } from '../src/users/users.service'; +import { JwtAuthGuard } from '../src/auth/guards/jwt-auth.guard'; +import type { AuthenticatedRequest } from '../src/auth/authenticated-request'; import { EscrowController } from '../src/escrow/escrow.controller'; import { EscrowService } from '../src/escrow/escrow.service'; import { AssetType, IdempotencyKeyStatus } from '../src/common/enums'; @@ -85,6 +92,24 @@ function checksumInvalidAddress(): string { return candidate; } +/** + * These endpoints became JWT-guarded in #40 (funding debits the caller's own + * wallet). This suite is about address validation at the HTTP boundary, not + * about auth, so the guard is stubbed to a fixed caller and UsersService's + * ownership assertion is a no-op — leaving both real would turn every + * assertion below into a 401 and stop testing #60 entirely. + */ +const authedUser = { userId: 'user_1', username: 'octocat' }; +const passingGuard = { + canActivate: (ctx: ExecutionContext) => { + ctx.switchToHttp().getRequest().user = authedUser; + return true; + }, +}; +const usersServiceStub = { + assertOwnsStellarAddress: jest.fn().mockResolvedValue(undefined), +}; + describe('Stellar address validation at the API boundary — escrow endpoints (#60)', () => { let app: INestApplication; let escrowService: { @@ -104,6 +129,7 @@ describe('Stellar address validation at the API boundary — escrow endpoints (# controllers: [EscrowController], providers: [ { provide: EscrowService, useValue: escrowService }, + { provide: UsersService, useValue: usersServiceStub }, IdempotencyInterceptor, Reflector, { @@ -111,7 +137,10 @@ describe('Stellar address validation at the API boundary — escrow endpoints (# useValue: new FakeIdempotencyRepo(), }, ], - }).compile(); + }) + .overrideGuard(JwtAuthGuard) + .useValue(passingGuard) + .compile(); app = moduleFixture.createNestApplication(); // Mirrors main.ts's ValidationPipe config exactly — this is what diff --git a/test/stellar-address-validation-maintenance-pool.e2e-spec.ts b/test/stellar-address-validation-maintenance-pool.e2e-spec.ts index c8d2894..093828d 100644 --- a/test/stellar-address-validation-maintenance-pool.e2e-spec.ts +++ b/test/stellar-address-validation-maintenance-pool.e2e-spec.ts @@ -1,10 +1,17 @@ -import { INestApplication, ValidationPipe } from '@nestjs/common'; +import { + ExecutionContext, + INestApplication, + ValidationPipe, +} from '@nestjs/common'; import { Reflector } from '@nestjs/core'; import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; import request from 'supertest'; import { randomUUID } from 'crypto'; import { Keypair, StrKey } from '@stellar/stellar-sdk'; +import { UsersService } from '../src/users/users.service'; +import { JwtAuthGuard } from '../src/auth/guards/jwt-auth.guard'; +import type { AuthenticatedRequest } from '../src/auth/authenticated-request'; import { MaintenancePoolController } from '../src/maintenance-pool/maintenance-pool.controller'; import { MaintenancePoolService } from '../src/maintenance-pool/maintenance-pool.service'; import { IdempotencyKeyStatus } from '../src/common/enums'; @@ -80,6 +87,24 @@ function checksumInvalidAddress(): string { return candidate; } +/** + * These endpoints became JWT-guarded in #40 (funding debits the caller's own + * wallet). This suite is about address validation at the HTTP boundary, not + * about auth, so the guard is stubbed to a fixed caller and UsersService's + * ownership assertion is a no-op — leaving both real would turn every + * assertion below into a 401 and stop testing #60 entirely. + */ +const authedUser = { userId: 'user_1', username: 'octocat' }; +const passingGuard = { + canActivate: (ctx: ExecutionContext) => { + ctx.switchToHttp().getRequest().user = authedUser; + return true; + }, +}; +const usersServiceStub = { + assertOwnsStellarAddress: jest.fn().mockResolvedValue(undefined), +}; + describe('Stellar address validation at the API boundary — maintenance-pool endpoints (#60)', () => { let app: INestApplication; let poolService: { deposit: jest.Mock; assignReward: jest.Mock }; @@ -94,6 +119,7 @@ describe('Stellar address validation at the API boundary — maintenance-pool en controllers: [MaintenancePoolController], providers: [ { provide: MaintenancePoolService, useValue: poolService }, + { provide: UsersService, useValue: usersServiceStub }, IdempotencyInterceptor, Reflector, { @@ -101,7 +127,10 @@ describe('Stellar address validation at the API boundary — maintenance-pool en useValue: new FakeIdempotencyRepo(), }, ], - }).compile(); + }) + .overrideGuard(JwtAuthGuard) + .useValue(passingGuard) + .compile(); app = moduleFixture.createNestApplication(); app.useGlobalPipes(