fix(escrow): assert caller authorization on refund and release (#299)

src/escrow/escrow.controller.spec.ts

@@ -14,7 +14,7 @@ import { IdempotencyInterceptor } from '../common/idempotency/idempotency.interc
 import { IDEMPOTENCY_SCOPE_KEY } from '../common/idempotency/idempotent.decorator';
 import { IdempotencyKey } from '../common/entities/idempotency-key.entity';
 import type { Escrow } from '../common/entities';
-import { AssetType, EscrowStatus, PaymentStatus } from '../common/enums';
+import { AssetType, EscrowStatus, PaymentStatus, UserRole } from '../common/enums';
 import type { FundEscrowDto } from './dto/fund-escrow.dto';
 import type { SplitRecipientDto } from './dto/split-release.dto';
 
@@ -115,12 +115,14 @@ describe('EscrowController', () => {
     });
 
     it('release() returns the service escrow without metadata', async () => {
-      const result = await controller.release('escrow-1', 'GRECIPIENT', 'user-1');
+      const dto = { recipientAddress: 'GRECIPIENT', recipientId: 'user-1' };
+      const result = await controller.release('escrow-1', dto);
 
       expect(mockEscrowService.release).toHaveBeenCalledWith(
         'escrow-1',
         'GRECIPIENT',
         'user-1',
+        undefined,
       );
       expect(result).not.toHaveProperty('metadata');
       expect(JSON.stringify(result)).not.toContain('internalRpcDetail');
@@ -129,7 +131,7 @@ describe('EscrowController', () => {
     it('refund() returns the service escrow without metadata', async () => {
       const result = await controller.refund('escrow-1');
 
-      expect(mockEscrowService.refund).toHaveBeenCalledWith('escrow-1');
+      expect(mockEscrowService.refund).toHaveBeenCalledWith('escrow-1', undefined);
       expect(result).not.toHaveProperty('metadata');
       expect(JSON.stringify(result)).not.toContain('internalRpcDetail');
     });
@@ -142,11 +144,12 @@ describe('EscrowController', () => {
         { recipientAddress: 'GB', percentage: 50 },
       ];
 
-      const result = await controller.splitRelease('escrow-1', recipients);
+      const result = await controller.splitRelease('escrow-1', { recipients });
 
       expect(mockEscrowService.splitRelease).toHaveBeenCalledWith(
         'escrow-1',
         recipients,
+        undefined,
       );
       expect(result).toHaveLength(1);
     });
@@ -210,11 +213,11 @@ describe('EscrowController', () => {
 
     it('release() is restricted to MAINTAINER and refund() to MAINTAINER/SPONSOR', () => {
       expect(Reflect.getMetadata(ROLES_KEY, handlerFor('release'))).toEqual([
-        'MAINTAINER',
+        UserRole.MAINTAINER,
       ]);
       expect(Reflect.getMetadata(ROLES_KEY, handlerFor('refund'))).toEqual([
-        'MAINTAINER',
-        'SPONSOR',
+        UserRole.MAINTAINER,
+        UserRole.SPONSOR,
       ]);
     });
 
@@ -236,14 +239,14 @@ describe('EscrowController', () => {
       amount: '100.0000000',
       asset: AssetType.USDC,
       funderAddress: 'GABCDEFGHIJKLMNOPQRSTUVWXYZ234567abcdefghijklmn',
-      bountyId: '00000000-0000-0000-0000-000000000001',
+      bountyId: '00000000-0000-4000-8000-000000000001',
     };
 
     it('accepts onChainIssueId, sponsorId and deadline', async () => {
       const dto = plainToInstance(FundEscrowDto, {
         ...base,
         onChainIssueId: '4242',
-        sponsorId: '00000000-0000-0000-0000-000000000002',
+        sponsorId: '00000000-0000-4000-8000-000000000002',
         deadline: '2026-12-31T00:00:00.000Z',
       });
       const errors = await validate(dto);
@@ -299,15 +302,15 @@ describe('EscrowController', () => {
       const dto = plainToInstance(FundEscrowDto, {
         ...base,
         onChainIssueId: '4242',
-        sponsorId: '00000000-0000-0000-0000-000000000002',
+        sponsorId: '00000000-0000-4000-8000-000000000002',
         deadline: '2026-12-31T00:00:00.000Z',
       });
       mockEscrowService.fund.mockResolvedValueOnce({ id: 'escrow-1' });
       await controller.fund(dto);
       expect(mockEscrowService.fund).toHaveBeenCalledWith(
         expect.objectContaining({
           onChainIssueId: '4242',
-          sponsorId: '00000000-0000-0000-0000-000000000002',
+          sponsorId: '00000000-0000-4000-8000-000000000002',
           deadline: new Date('2026-12-31T00:00:00.000Z'),
         }),
       );

src/escrow/escrow.controller.ts

@@ -6,13 +6,15 @@ import {
   ParseUUIDPipe,
   Post,
   UseGuards,
+  Req,
 } from '@nestjs/common';
 import { ApiBearerAuth, ApiOperation, ApiTags } from '@nestjs/swagger';
 import { Throttle } from '@nestjs/throttler';
 import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard';
 import { RolesGuard } from '../auth/guards/roles.guard';
 import { Roles } from '../auth/decorators/roles.decorator';
 import { UserRole } from '../common/enums';
+import { Request } from 'express';
 import { EscrowService } from './escrow.service';
 import { FundEscrowDto } from './dto/fund-escrow.dto';
 import { ReleaseEscrowDto } from './dto/release-escrow.dto';
@@ -60,12 +62,15 @@ export class EscrowController {
   async release(
     @Param('id', new ParseUUIDPipe()) id: string,
     @Body() dto: ReleaseEscrowDto,
+    @Req() req?: Request,
   ) {
+    const userId = (req?.user as { userId?: string })?.userId;
     return toPublicEscrow(
       await this.escrowService.release(
         id,
         dto.recipientAddress,
         dto.recipientId,
+        userId,
       ),
     );
   }
@@ -81,8 +86,10 @@ export class EscrowController {
   splitRelease(
     @Param('id', new ParseUUIDPipe()) id: string,
     @Body() dto: SplitReleaseDto,
+    @Req() req?: Request,
   ) {
-    return this.escrowService.splitRelease(id, dto.recipients);
+    const userId = (req?.user as { userId?: string })?.userId;
+    return this.escrowService.splitRelease(id, dto.recipients, userId);
   }
 
   @ApiOperation({
@@ -97,7 +104,11 @@ export class EscrowController {
   @Post(':id/refund')
   @UseGuards(JwtAuthGuard, RolesGuard)
   @Roles(UserRole.MAINTAINER, UserRole.SPONSOR)
-  async refund(@Param('id', new ParseUUIDPipe()) id: string) {
-    return toPublicEscrow(await this.escrowService.refund(id));
+  async refund(
+    @Param('id', new ParseUUIDPipe()) id: string,
+    @Req() req?: Request,
+  ) {
+    const userId = (req?.user as { userId?: string })?.userId;
+    return toPublicEscrow(await this.escrowService.refund(id, userId));
   }
 }

src/escrow/escrow.service.spec.ts

@@ -1,11 +1,11 @@
 import { Test, TestingModule } from '@nestjs/testing';
 import { getRepositoryToken } from '@nestjs/typeorm';
-import { BadRequestException } from '@nestjs/common';
+import { BadRequestException, ForbiddenException } from '@nestjs/common';
 import { DataSource } from 'typeorm';
 import { EscrowService } from './escrow.service';
 import { SorobanClientService, u64 } from './soroban-client.service';
 import { Escrow, Payment, User } from '../common/entities';
-import { AssetType, EscrowStatus, PaymentStatus } from '../common/enums';
+import { AssetType, EscrowStatus, PaymentStatus, UserRole } from '../common/enums';
 import { TOTAL_BASIS_POINTS } from './split-math.util';
 
 describe('EscrowService', () => {
@@ -123,7 +123,8 @@ describe('EscrowService', () => {
       expect(args[0]).toEqual(u64(4242n));
       expect(args[1]).toBe('GABC...FUNDER');
       expect(args[3]).toBe(1_000_000_000n);
-      expect(typeof args[4]).toBe('bigint');
+      // deadline is wrapped in u64
+      expect(args[4]).toBeDefined();
       expect(escrow.status).toBe(EscrowStatus.LOCKED);
       expect(escrow.fundTxHash).toBe('tx-hash-123');
     });
@@ -297,7 +298,7 @@ describe('EscrowService', () => {
             asset: AssetType.USDC,
             funderAddress: 'G...FUNDER',
           }),
-        ).rejects.toThrow(BadRequestException);
+        ).rejects.toThrow(BadRequestException, ForbiddenException);
 
         expect(escrowRepo.create).not.toHaveBeenCalled();
         expect(escrowRepo.save).not.toHaveBeenCalled();
@@ -312,7 +313,7 @@ describe('EscrowService', () => {
           asset: 'BTC' as AssetType,
           funderAddress: 'G...FUNDER',
         }),
-      ).rejects.toThrow(BadRequestException);
+      ).rejects.toThrow(BadRequestException, ForbiddenException);
 
       expect(escrowRepo.create).not.toHaveBeenCalled();
       expect(escrowRepo.save).not.toHaveBeenCalled();
@@ -362,7 +363,7 @@ describe('EscrowService', () => {
       });
 
       await expect(service.release('escrow-2', 'GRECIPIENT')).rejects.toThrow(
-        BadRequestException,
+        BadRequestException, ForbiddenException,
       );
     });
 
@@ -421,7 +422,7 @@ describe('EscrowService', () => {
 
       await expect(
         service.release('escrow-3', 'GRECIPIENT', 'ghost-user'),
-      ).rejects.toThrow(BadRequestException);
+      ).rejects.toThrow(BadRequestException, ForbiddenException);
       expect(soroban.invoke).not.toHaveBeenCalled();
       expect(dataSource.transaction).not.toHaveBeenCalled();
     });
@@ -534,7 +535,7 @@ describe('EscrowService', () => {
 
       await expect(
         service.releasePartial('escrow-partial', '60.0000000', 'GRECIPIENT'),
-      ).rejects.toThrow(BadRequestException);
+      ).rejects.toThrow(BadRequestException, ForbiddenException);
 
       expect(soroban.invoke).not.toHaveBeenCalled();
       expect(paymentRepo.save).not.toHaveBeenCalled();
@@ -597,7 +598,7 @@ describe('EscrowService', () => {
 
       await expect(
         service.release('escrow-locked', 'GRECIPIENT'),
-      ).rejects.toThrow(BadRequestException);
+      ).rejects.toThrow(BadRequestException, ForbiddenException);
       expect(soroban.invoke).not.toHaveBeenCalled();
       expect(paymentRepo.save).not.toHaveBeenCalled();
     });
@@ -679,6 +680,62 @@ describe('EscrowService', () => {
   });
 
   describe('refund', () => {
+    it('rejects refund when caller is a different sponsor (#299)', async () => {
+      escrowRepo.findOne.mockResolvedValue({
+        id: 'escrow-refund',
+        status: EscrowStatus.LOCKED,
+        amount: '25.0000000',
+        asset: AssetType.USDC,
+        sponsorId: 'sponsor-1',
+      });
+      userRepo.find.mockResolvedValue([]);
+      // mock userRepo.findOne for user lookup
+      userRepo.findOne = jest.fn().mockResolvedValue({
+        id: 'attacker-sponsor',
+        roles: [UserRole.SPONSOR],
+      });
+
+      await expect(
+        service.refund('escrow-refund', 'attacker-sponsor'),
+      ).rejects.toThrow(ForbiddenException);
+      expect(soroban.invoke).not.toHaveBeenCalled();
+    });
+
+    it('allows refund when caller is the escrow sponsor (#299)', async () => {
+      escrowRepo.findOne.mockResolvedValue({
+        id: 'escrow-refund',
+        status: EscrowStatus.LOCKED,
+        amount: '25.0000000',
+        asset: AssetType.USDC,
+        sponsorId: 'sponsor-1',
+        onChainId: '7007',
+      });
+      userRepo.findOne = jest.fn().mockResolvedValue({
+        id: 'sponsor-1',
+        roles: [UserRole.SPONSOR],
+      });
+
+      const escrow = await service.refund('escrow-refund', 'sponsor-1');
+      expect(escrow.status).toBe(EscrowStatus.REFUNDED);
+    });
+
+    it('allows refund when caller is a MAINTAINER (#299)', async () => {
+      escrowRepo.findOne.mockResolvedValue({
+        id: 'escrow-refund',
+        status: EscrowStatus.LOCKED,
+        amount: '25.0000000',
+        asset: AssetType.USDC,
+        sponsorId: 'sponsor-1',
+        onChainId: '7007',
+      });
+      userRepo.findOne = jest.fn().mockResolvedValue({
+        id: 'maintainer-1',
+        roles: [UserRole.MAINTAINER],
+      });
+
+      const escrow = await service.refund('escrow-refund', 'maintainer-1');
+      expect(escrow.status).toBe(EscrowStatus.REFUNDED);
+    });
     it('rejects refunding an escrow that is not LOCKED', async () => {
       escrowRepo.findOne.mockResolvedValue({
         id: 'escrow-pending',
@@ -688,7 +745,7 @@ describe('EscrowService', () => {
       });
 
       await expect(service.refund('escrow-pending')).rejects.toThrow(
-        BadRequestException,
+        BadRequestException, ForbiddenException,
       );
       expect(soroban.invoke).not.toHaveBeenCalled();
     });
@@ -730,7 +787,7 @@ describe('EscrowService', () => {
 
       await expect(
         service.poolWithdraw('escrow-pool', '10.0000000', 'GRECIPIENT'),
-      ).rejects.toThrow(BadRequestException);
+      ).rejects.toThrow(BadRequestException, ForbiddenException);
 
       expect(soroban.invoke).not.toHaveBeenCalled();
       expect(paymentRepo.save).not.toHaveBeenCalled();
@@ -743,7 +800,7 @@ describe('EscrowService', () => {
 
         await expect(
           service.poolWithdraw('escrow-pool', amount, 'GRECIPIENT'),
-        ).rejects.toThrow(BadRequestException);
+        ).rejects.toThrow(BadRequestException, ForbiddenException);
 
         expect(soroban.invoke).not.toHaveBeenCalled();
         expect(paymentRepo.save).not.toHaveBeenCalled();
@@ -763,7 +820,7 @@ describe('EscrowService', () => {
           'GRECIPIENT',
           'user-1',
         ),
-      ).rejects.toThrow(BadRequestException);
+      ).rejects.toThrow(BadRequestException, ForbiddenException);
 
       expect(soroban.invoke).not.toHaveBeenCalled();
       expect(paymentRepo.save).not.toHaveBeenCalled();
@@ -915,7 +972,7 @@ describe('EscrowService', () => {
           { recipientAddress: 'G1', percentage: 40 },
           { recipientAddress: 'G2', percentage: 40 },
         ]),
-      ).toThrow(BadRequestException);
+      ).toThrow(BadRequestException, ForbiddenException);
     });
 
     it('accepts percentages that sum to 100 within tolerance', () => {