fix(security): bind money-moving DTOs to authenticated caller identity

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.
 

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;
+}

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 };
   }
 }

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();
+    });
+  });
+});

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')

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],