From 352e41af57a4b5a8a18f3e9f4896a41abb7abc6b Mon Sep 17 00:00:00 2001 From: Chidubemkingsley Date: Wed, 26 Aug 2026 20:49:19 +0100 Subject: [PATCH] fix(security): guard sponsors/reputation routes, drop unreachable addRole - SponsorsController: add JwtAuthGuard + ownership check on /sponsors/:id/dashboard and /sponsors/:id/milestones/progress so a sponsor's financials can only be read by that sponsor. - ReputationController: guard all routes with JwtAuthGuard and restrict GET/history and POST recompute to the authenticated owner, preventing unauthenticated reputation-history leaks and arbitrary snapshot flooding. - Add CurrentUser param decorator to safely extract the authenticated caller. - Remove UsersService.addRole (unreachable via any endpoint) and document that role assignment is out of scope for this version. - Rename misleading users e2e tests to describe the 403 behavior they actually assert against the mocked guard. --- .../decorators/current-user.decorator.ts | 23 +++++++++ src/reputation/reputation.controller.ts | 47 +++++++++++++++++-- src/sponsors/sponsors.controller.ts | 38 +++++++++++++-- src/users/users.service.spec.ts | 30 ------------ src/users/users.service.ts | 15 +++--- test/users.e2e-spec.ts | 8 ++-- 6 files changed, 110 insertions(+), 51 deletions(-) create mode 100644 src/common/decorators/current-user.decorator.ts diff --git a/src/common/decorators/current-user.decorator.ts b/src/common/decorators/current-user.decorator.ts new file mode 100644 index 0000000..b99e7de --- /dev/null +++ b/src/common/decorators/current-user.decorator.ts @@ -0,0 +1,23 @@ +import { createParamDecorator, ExecutionContext } from '@nestjs/common'; + +export interface AuthenticatedUser { + userId: string; + username: string; +} + +/** + * Extracts the authenticated caller from the request, as populated by + * JwtAuthGuard/JwtStrategy. Throws if used without an auth guard so callers + * don't silently read an undefined user. + */ +export const CurrentUser = createParamDecorator( + (_data: unknown, ctx: ExecutionContext): AuthenticatedUser => { + const request = ctx + .switchToHttp() + .getRequest<{ user?: AuthenticatedUser }>(); + if (!request.user) { + throw new Error('CurrentUser can only be used behind an auth guard'); + } + return request.user; + }, +); diff --git a/src/reputation/reputation.controller.ts b/src/reputation/reputation.controller.ts index 2028680..a9e9582 100644 --- a/src/reputation/reputation.controller.ts +++ b/src/reputation/reputation.controller.ts @@ -1,24 +1,61 @@ -import { Controller, Get, Param, ParseUUIDPipe, Post } from '@nestjs/common'; -import { ApiTags } from '@nestjs/swagger'; +import { + Controller, + ForbiddenException, + Get, + Param, + ParseUUIDPipe, + Post, + UseGuards, +} from '@nestjs/common'; +import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { ReputationService } from './reputation.service'; +import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; +import { CurrentUser } from '../common/decorators/current-user.decorator'; +import type { AuthenticatedUser } from '../common/decorators/current-user.decorator'; @ApiTags('reputation') @Controller('reputation') export class ReputationController { constructor(private readonly reputationService: ReputationService) {} + private assertOwner(user: AuthenticatedUser, userId: string) { + if (user.userId !== userId) { + throw new ForbiddenException( + 'You may only access or recompute your own reputation data', + ); + } + } + + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Post(':userId/recompute') - recompute(@Param('userId', new ParseUUIDPipe()) userId: string) { + recompute( + @Param('userId', new ParseUUIDPipe()) userId: string, + @CurrentUser() user: AuthenticatedUser, + ) { + this.assertOwner(user, userId); return this.reputationService.computeAndSave(userId); } + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Get(':userId') - latest(@Param('userId', new ParseUUIDPipe()) userId: string) { + latest( + @Param('userId', new ParseUUIDPipe()) userId: string, + @CurrentUser() user: AuthenticatedUser, + ) { + this.assertOwner(user, userId); return this.reputationService.getLatest(userId); } + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Get(':userId/history') - history(@Param('userId', new ParseUUIDPipe()) userId: string) { + history( + @Param('userId', new ParseUUIDPipe()) userId: string, + @CurrentUser() user: AuthenticatedUser, + ) { + this.assertOwner(user, userId); return this.reputationService.history(userId); } } diff --git a/src/sponsors/sponsors.controller.ts b/src/sponsors/sponsors.controller.ts index 407d7e2..c94dd77 100644 --- a/src/sponsors/sponsors.controller.ts +++ b/src/sponsors/sponsors.controller.ts @@ -1,19 +1,49 @@ -import { Controller, Get, Param, ParseUUIDPipe } from '@nestjs/common'; -import { ApiTags } from '@nestjs/swagger'; +import { + Controller, + ForbiddenException, + Get, + Param, + ParseUUIDPipe, + UseGuards, +} from '@nestjs/common'; +import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { SponsorsService } from './sponsors.service'; +import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; +import { CurrentUser } from '../common/decorators/current-user.decorator'; +import type { AuthenticatedUser } from '../common/decorators/current-user.decorator'; @ApiTags('sponsors') @Controller('sponsors') export class SponsorsController { constructor(private readonly sponsorsService: SponsorsService) {} + private assertOwnsSponsor(user: AuthenticatedUser, sponsorId: string) { + if (user.userId !== sponsorId) { + throw new ForbiddenException( + 'You may only view your own sponsor dashboard and progress', + ); + } + } + + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Get(':id/dashboard') - dashboard(@Param('id', new ParseUUIDPipe()) id: string) { + dashboard( + @Param('id', new ParseUUIDPipe()) id: string, + @CurrentUser() user: AuthenticatedUser, + ) { + this.assertOwnsSponsor(user, id); return this.sponsorsService.dashboard(id); } + @ApiBearerAuth() + @UseGuards(JwtAuthGuard) @Get(':id/milestones/progress') - milestoneProgress(@Param('id', new ParseUUIDPipe()) id: string) { + milestoneProgress( + @Param('id', new ParseUUIDPipe()) id: string, + @CurrentUser() user: AuthenticatedUser, + ) { + this.assertOwnsSponsor(user, id); return this.sponsorsService.milestoneProgress(id); } } diff --git a/src/users/users.service.spec.ts b/src/users/users.service.spec.ts index 8bf284c..d427c5c 100644 --- a/src/users/users.service.spec.ts +++ b/src/users/users.service.spec.ts @@ -179,36 +179,6 @@ describe('UsersService', () => { }); }); - describe('addRole', () => { - it('adds the role when the user does not already have it', async () => { - userRepo.findOne.mockResolvedValue({ - id: 'u1', - roles: [UserRole.CONTRIBUTOR], - }); - - const user = await service.addRole('u1', UserRole.MAINTAINER); - - expect(user.roles).toEqual([UserRole.CONTRIBUTOR, UserRole.MAINTAINER]); - expect(userRepo.save).toHaveBeenCalledWith( - expect.objectContaining({ - roles: [UserRole.CONTRIBUTOR, UserRole.MAINTAINER], - }), - ); - }); - - it('is a no-op when the user already has the role', async () => { - userRepo.findOne.mockResolvedValue({ - id: 'u1', - roles: [UserRole.CONTRIBUTOR], - }); - - const user = await service.addRole('u1', UserRole.CONTRIBUTOR); - - expect(user.roles).toEqual([UserRole.CONTRIBUTOR]); - expect(userRepo.save).not.toHaveBeenCalled(); - }); - }); - describe('setStellarAddress', () => { it('throws NotFoundException when the user does not exist', async () => { userRepo.findOne.mockResolvedValue(null); diff --git a/src/users/users.service.ts b/src/users/users.service.ts index 267453f..338589e 100644 --- a/src/users/users.service.ts +++ b/src/users/users.service.ts @@ -87,14 +87,13 @@ export class UsersService { return this.findOneRaw(user.id); } - async addRole(userId: string, role: UserRole): Promise { - const user = await this.findOneRaw(userId); - if (!user.roles.includes(role)) { - user.roles = [...user.roles, role]; - await this.userRepo.save(user); - } - return user; - } + // Role assignment is intentionally out of scope for this version: a User's + // `roles` are seeded to [UserRole.CONTRIBUTOR] at creation (upsertFromGithub) + // and are not mutated by any API endpoint or admin flow today. The + // MAINTAINER / SPONSOR roles remain declared but unreachable until a + // guarded role-assignment endpoint (or sponsor-verification flow) is added. + // Keeping role mutation out of the public service surface prevents a reader + // from mistaking an unused method for a working feature. async setStellarAddress( userId: string, diff --git a/test/users.e2e-spec.ts b/test/users.e2e-spec.ts index 4fee806..de717f6 100644 --- a/test/users.e2e-spec.ts +++ b/test/users.e2e-spec.ts @@ -21,7 +21,7 @@ describe('UsersController (e2e)', () => { ], }) .overrideGuard(JwtAuthGuard) - .useValue({ canActivate: () => false }) // Simulate unauthenticated + .useValue({ canActivate: () => false }) // Simulate a guard-denied request (always 403) .compile(); app = moduleFixture.createNestApplication(); @@ -33,15 +33,15 @@ describe('UsersController (e2e)', () => { }); describe('GET /users', () => { - it('should reject unauthenticated requests with 401', () => { + it('should return 403 when the guard denies the request', () => { return request(app.getHttpServer()) .get('/users') - .expect(403); // Assuming the guard returns 403 when not authorized + .expect(403); }); }); describe('GET /users/:id', () => { - it('should reject unauthenticated requests with 401', () => { + it('should return 403 when the guard denies the request', () => { return request(app.getHttpServer()) .get('/users/00000000-0000-0000-0000-000000000000') .expect(403);