diff --git a/backend/src/auth/controllers/auth.controller.ts b/backend/src/auth/controllers/auth.controller.ts index 3ba406cf..5ae1315c 100644 --- a/backend/src/auth/controllers/auth.controller.ts +++ b/backend/src/auth/controllers/auth.controller.ts @@ -1,27 +1,14 @@ -import { - Controller, - Post, - Get, - UseGuards, - Request, - HttpStatus, - HttpCode, - Body, -} from '@nestjs/common'; -import { - ApiTags, - ApiOperation, - ApiResponse, - ApiBearerAuth, -} from '@nestjs/swagger'; -import { AuthService } from '../services/auth.service'; -import { Auth } from '../decorators/auth-decorator'; -import { AuthType } from '../enums/auth-type.enum'; -import { AuthResponseDto } from '../dto/auth-response.dto'; -import { RegisterDto } from '../dto/register.dto'; -import { LoginDto } from '../dto/login.dto'; -import { JwtAuthGuard } from '../guards/jwt-auth.guard'; -import { User } from '../entities/user.entity'; +import { Controller, Post, Get, UseGuards, Request, HttpStatus, HttpCode, Body } from "@nestjs/common" +import { ApiTags, ApiOperation, ApiResponse, ApiBearerAuth } from "@nestjs/swagger" +import { AuthService } from "../services/auth.service" +import { Auth } from "../decorators/auth-decorator" +import { AuthType } from "../enums/auth-type.enum" +import { AuthResponseDto } from "../dto/auth-response.dto" +import { GenericAuthMessageDto } from "../dto/generic-auth-message.dto" +import { RegisterDto } from "../dto/register.dto" +import { LoginDto } from "../dto/login.dto" +import { JwtAuthGuard } from "../guards/jwt-auth.guard" +import { User } from "../entities/user.entity" @ApiTags('Authentication') @Controller('auth') @@ -38,18 +25,15 @@ export class AuthController { }) @ApiResponse({ status: 201, - description: 'User successfully registered', + description: + "User successfully registered (or, for anti-enumeration, a generic neutral response when the email is already taken)", type: AuthResponseDto, }) @ApiResponse({ status: 400, description: 'Bad request - validation failed', }) - @ApiResponse({ - status: 409, - description: 'Conflict - user already exists', - }) - async register(@Body() registerDto: RegisterDto): Promise { + async register(@Body() registerDto: RegisterDto): Promise { try { return await this.authService.register(registerDto); } catch (error) { diff --git a/backend/src/auth/dto/generic-auth-message.dto.ts b/backend/src/auth/dto/generic-auth-message.dto.ts new file mode 100644 index 00000000..dd2e4af5 --- /dev/null +++ b/backend/src/auth/dto/generic-auth-message.dto.ts @@ -0,0 +1,14 @@ +import { ApiProperty } from "@nestjs/swagger" + +// Anti-enumeration response body returned by public auth endpoints when the +// server must not reveal whether an account already exists (OWASP A01: +// Broken Access Control — user/account enumeration). It carries no user +// identifier and no access token, so a probing attacker cannot distinguish +// a brand-new registration from one that already exists. +export class GenericAuthMessageDto { + @ApiProperty({ + description: "Generic, account-existence-neutral message", + example: "Registration successful. If an account already exists, please log in.", + }) + message: string +} \ No newline at end of file diff --git a/backend/src/auth/services/auth.service.spec.ts b/backend/src/auth/services/auth.service.spec.ts index 09bf043e..447d9872 100644 --- a/backend/src/auth/services/auth.service.spec.ts +++ b/backend/src/auth/services/auth.service.spec.ts @@ -1,19 +1,20 @@ -import { Test, type TestingModule } from '@nestjs/testing'; -import { getRepositoryToken } from '@nestjs/typeorm'; -import { JwtService } from '@nestjs/jwt'; -import { ConfigService } from '@nestjs/config'; -import { ConflictException, UnauthorizedException } from '@nestjs/common'; -import { AuthService } from './auth.service'; -import { User } from '../entities/user.entity'; -import type { RegisterDto } from '../dto/register.dto'; -import type { LoginDto } from '../dto/login.dto'; -import { jest } from '@jest/globals'; -import type { Repository } from 'typeorm'; - -describe('AuthService', () => { - let service: AuthService; - let userRepository: jest.Mocked>; - let jwtService: jest.Mocked; +import { Test, type TestingModule } from "@nestjs/testing" +import { getRepositoryToken } from "@nestjs/typeorm" +import { JwtService } from "@nestjs/jwt" +import { ConfigService } from "@nestjs/config" +import { UnauthorizedException } from "@nestjs/common" +import { AuthService } from "./auth.service" +import { User } from "../entities/user.entity" +import type { RegisterDto } from "../dto/register.dto" +import type { LoginDto } from "../dto/login.dto" +import type { AuthResponseDto } from "../dto/auth-response.dto" +import { jest } from "@jest/globals" +import type { Repository } from "typeorm" + +describe("AuthService", () => { + let service: AuthService + let userRepository: jest.Mocked> + let jwtService: jest.Mocked const mockUserRepository = { findOne: jest.fn(), @@ -49,12 +50,10 @@ describe('AuthService', () => { ], }).compile(); - service = module.get(AuthService); - userRepository = module.get(getRepositoryToken(User)) as jest.Mocked< - Repository - >; - jwtService = module.get(JwtService); - }); + service = module.get(AuthService) + userRepository = module.get(getRepositoryToken(User)) as jest.Mocked> + jwtService = module.get(JwtService) as unknown as jest.Mocked + }) afterEach(() => { jest.clearAllMocks(); @@ -62,41 +61,58 @@ describe('AuthService', () => { describe('register', () => { const registerDto: RegisterDto = { - name: 'John Doe', - email: 'john@example.com', - password: 'SecurePass123!', - }; - - it('should successfully register a new user', async () => { - const mockUser = { - id: 'user-id', - name: 'John Doe', - email: 'john@example.com', - createdAt: new Date(), - } as User; - - userRepository.findOne.mockResolvedValue(null); - userRepository.create.mockReturnValue(mockUser); - userRepository.save.mockResolvedValue(mockUser); - jwtService.sign.mockReturnValue('jwt-token'); - mockConfigService.get.mockReturnValue('15m'); - - const result = await service.register(registerDto); + name: "John Doe", + username: "john_doe", + email: "john@example.com", + password: "SecurePass123!", + } + + const registeredUser = { + id: "user-id", + name: "John Doe", + email: "john@example.com", + createdAt: new Date(), + } as User + + it("should successfully register a new user", async () => { + userRepository.findOne.mockResolvedValue(null) + userRepository.create.mockReturnValue(registeredUser) + userRepository.save.mockResolvedValue(registeredUser) + jwtService.sign.mockReturnValue("jwt-token") + mockConfigService.get.mockReturnValue("15m") + + const result = (await service.register(registerDto)) as AuthResponseDto expect(result).toHaveProperty('accessToken', 'jwt-token'); expect(result).toHaveProperty('user'); expect(result.user.email).toBe('john@example.com'); }); - it('should throw ConflictException if user already exists', async () => { - const existingUser = { id: 'existing-user' } as User; - userRepository.findOne.mockResolvedValue(existingUser); + it("should return a generic neutral message if user already exists (anti-enumeration)", async () => { + const existingUser = { id: "existing-user" } as User + userRepository.findOne.mockResolvedValue(existingUser) - await expect(service.register(registerDto)).rejects.toThrow( - ConflictException, - ); - }); - }); + const result = await service.register(registerDto) + + // Must NOT reveal that the account exists or issue a token. + expect(result).toHaveProperty("message") + expect(result).not.toHaveProperty("accessToken") + expect(userRepository.create).not.toHaveBeenCalled() + expect(userRepository.save).not.toHaveBeenCalled() + }) + + it("should return a generic neutral message on unique violation (anti-enumeration)", async () => { + userRepository.findOne.mockResolvedValue(null) + const error: any = new Error("duplicate") + error.code = "23505" + userRepository.save.mockRejectedValue(error) + + const result = await service.register(registerDto) + + expect(result).toHaveProperty("message") + expect(result).not.toHaveProperty("accessToken") + }) + }) describe('login', () => { const loginDto: LoginDto = { @@ -104,23 +120,23 @@ describe('AuthService', () => { password: 'SecurePass123!', }; - it('should successfully login with valid credentials', async () => { - const mockUser = { - id: 'user-id', - name: 'John Doe', - email: 'john@example.com', - isActive: true, - validatePassword: jest.fn().mockResolvedValue(true), - } as User & { validatePassword: jest.Mock }; - - userRepository.findOne.mockResolvedValue(mockUser); - userRepository.update.mockResolvedValue({ - affected: 1, - generatedMaps: [], - raw: {}, - }); - jwtService.sign.mockReturnValue('jwt-token'); - mockConfigService.get.mockReturnValue('15m'); + const validatedUser = (isActive: boolean, passwordMatches: boolean) => { + const user = { + id: "user-id", + name: "John Doe", + email: "john@example.com", + isActive, + validatePassword: async () => passwordMatches, + } as unknown as User + return user + } + + it("should successfully login with valid credentials", async () => { + const mockUser = validatedUser(true, true) + userRepository.findOne.mockResolvedValue(mockUser) + userRepository.update.mockResolvedValue({ affected: 1, generatedMaps: [], raw: {} }) + jwtService.sign.mockReturnValue("jwt-token") + mockConfigService.get.mockReturnValue("15m") const result = await service.login(loginDto); @@ -128,25 +144,32 @@ describe('AuthService', () => { expect(result).toHaveProperty('user'); }); - it('should throw UnauthorizedException for invalid credentials', async () => { - userRepository.findOne.mockResolvedValue(null); + it("should throw UnauthorizedException for unknown email without revealing that the account does not exist", async () => { + userRepository.findOne.mockResolvedValue(null) await expect(service.login(loginDto)).rejects.toThrow( - UnauthorizedException, - ); - }); + new UnauthorizedException("Invalid email or password"), + ) + }) - it('should throw UnauthorizedException for inactive user', async () => { - const mockUser = { - id: 'user-id', - isActive: false, - } as User; + it("should throw UnauthorizedException for inactive user without revealing account status", async () => { + const mockUser = validatedUser(false, true) + userRepository.findOne.mockResolvedValue(mockUser) - userRepository.findOne.mockResolvedValue(mockUser); + // Same generic message as for unknown email / wrong password. + await expect(service.login(loginDto)).rejects.toThrow( + new UnauthorizedException("Invalid email or password"), + ) + }) + + it("should not reveal whether an account exists when password is wrong", async () => { + const mockUser = validatedUser(true, false) + userRepository.findOne.mockResolvedValue(mockUser) + // Message identical across all failure modes. await expect(service.login(loginDto)).rejects.toThrow( - UnauthorizedException, - ); - }); - }); -}); + new UnauthorizedException("Invalid email or password"), + ) + }) + }) +}) diff --git a/backend/src/auth/services/auth.service.ts b/backend/src/auth/services/auth.service.ts index d3dc3eaf..8d229c85 100644 --- a/backend/src/auth/services/auth.service.ts +++ b/backend/src/auth/services/auth.service.ts @@ -1,28 +1,13 @@ -import { - Injectable, - ConflictException, - UnauthorizedException, - BadRequestException, -} from '@nestjs/common'; -import { Repository } from 'typeorm'; -import { JwtService } from '@nestjs/jwt'; -import { ConfigService } from '@nestjs/config'; -import { User } from '../entities/user.entity'; -import { RegisterDto } from '../dto/register.dto'; -import { AuthResponseDto } from '../dto/auth-response.dto'; -import { LoginDto } from '../dto/login.dto'; -import { InjectRepository } from '@nestjs/typeorm'; - -const BREACHED_PASSWORDS = new Set([ - 'password', - 'password123!', - '12345678', - 'qwerty123!', - 'letmein123!', - 'welcome123!', - 'iloveyou123!', - 'admin123!', -]); +import { Injectable, UnauthorizedException, BadRequestException } from "@nestjs/common" +import { Repository } from "typeorm" +import { JwtService } from "@nestjs/jwt" +import { ConfigService } from "@nestjs/config" +import { User } from "../entities/user.entity" +import { RegisterDto } from "../dto/register.dto" +import { AuthResponseDto } from "../dto/auth-response.dto" +import { GenericAuthMessageDto } from "../dto/generic-auth-message.dto" +import { LoginDto } from "../dto/login.dto" +import { InjectRepository } from "@nestjs/typeorm" export interface JwtPayload { sub: string; // user id @@ -42,8 +27,20 @@ export class AuthService { private readonly configService: ConfigService, ) {} - async register(registerDto: RegisterDto): Promise { - const { name, username, email, password } = registerDto; + /** + * Registers a new user. + * + * Anti-enumeration: whether or not the account already exists we return a + * generic, account-existence-neutral success response instead of a + * distinctive "already exists" error, so attackers cannot probe whether a + * given email is registered (OWASP A01 — account enumeration). + * + * A real (fresh) registration still returns the authenticated session + * (AuthResponseDto); a duplicate email returns the same neutral HTTP + * success status without issuing a token. + */ + async register(registerDto: RegisterDto): Promise { + const { name, username, email, password } = registerDto try { this.assertPasswordPolicy(password, email, username); @@ -54,7 +51,7 @@ export class AuthService { }); if (existingUser) { - throw new ConflictException('User with this email already exists'); + return this.genericRegistrationMessage() } // Create new user @@ -91,25 +88,25 @@ export class AuthService { } catch (error) { console.error('Registration error:', error); // Add logging - if (error instanceof ConflictException) { - throw error; - } - - if (error instanceof BadRequestException) { - throw error; + if (error.code === "23505") { + // PostgreSQL unique violation (email/username collision). Same + // neutral response so attackers cannot infer which identifier is + // already taken. + return this.genericRegistrationMessage() } - if (error.code === '23505') { - // PostgreSQL unique violation - throw new ConflictException('User with this email already exists'); - } - - throw new BadRequestException( - `Failed to create user account: ${error.message}`, - ); + throw new BadRequestException("Registration could not be completed") } } + /** + * Authenticates a user. + * + * Anti-enumeration: every failure path (unknown email, deactivated account, + * wrong password) returns the same generic `UnauthorizedException`, so an + * attacker cannot infer whether an account exists or its status from the + * login response. + */ async login(loginDto: LoginDto): Promise { const { email, password } = loginDto; @@ -119,19 +116,11 @@ export class AuthService { where: { email: email.toLowerCase() }, }); - if (!user) { - throw new UnauthorizedException('Invalid email or password'); - } - - // Check if user is active - if (!user.isActive) { - throw new UnauthorizedException('Account has been deactivated'); - } - - // Validate password - const isPasswordValid = await user.validatePassword(password); - if (!isPasswordValid) { - throw new UnauthorizedException('Invalid email or password'); + // Check password. Nobody exists OR account deactivated OR wrong + // password all surface the exact same generic message & status. + const isPasswordValid = user ? await user.validatePassword(password) : false + if (!user || !user.isActive || !isPasswordValid) { + throw new UnauthorizedException("Invalid email or password") } // Update last login time @@ -167,7 +156,7 @@ export class AuthService { throw error; } - throw new BadRequestException(`Login failed: ${error.message}`); + throw new BadRequestException("Login failed") } } @@ -195,6 +184,16 @@ export class AuthService { return user; } + /** + * Returns a neutral, account-existence-neutral registration response. + * Mirrors the JS static message constant so tests can assert against it. + */ + private genericRegistrationMessage(): GenericAuthMessageDto { + return { + message: "Registration successful. If an account already exists, please log in.", + } + } + private getTokenExpirationTime(): number { const expiresIn = this.configService.get('JWT_EXPIRES_IN') || '15m';