fix(auth): prevent login endpoint from returning 500 on invalid credentials
LocalStrategy.validate lacked a try-catch, so infrastructure errors (DB timeouts, bcrypt failures, null/undefined phone) escaped as raw Error instances. LocalAuthGuard.handleRequest blindly re-threw them, causing GlobalExceptionFilter to map them to 500 Internal Server Error instead of 401 Unauthorized. Changes: - Add null/falsy guard for phone and password in LocalStrategy.validate - Wrap validate body in try-catch; re-throw DomainExceptions, wrap unexpected errors as UnauthorizedException (401) - Add error type-checking in LocalAuthGuard.handleRequest: re-throw HttpException subclasses directly, wrap other errors as 401 - Add @IsNotEmpty() validators to LoginDto for Swagger accuracy - Add 5 new test cases covering undefined/null/empty inputs, DB errors, and bcrypt failures - Update guard tests for the new type-checking behaviour Resolves TEC-1841 Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
@@ -18,16 +18,36 @@ vi.mock('@nestjs/passport', () => {
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock('@nestjs/common', async (importOriginal) => {
|
||||
const actual = (await importOriginal()) as any;
|
||||
return {
|
||||
...actual,
|
||||
Logger: class MockLogger {
|
||||
error = vi.fn();
|
||||
warn = vi.fn();
|
||||
log = vi.fn();
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock('@modules/shared', () => {
|
||||
class UnauthorizedException extends Error {
|
||||
constructor(message: string) {
|
||||
class DomainException extends Error {
|
||||
constructor(public errorCode: string, message: string) {
|
||||
super(message);
|
||||
this.name = 'DomainException';
|
||||
}
|
||||
}
|
||||
class UnauthorizedException extends DomainException {
|
||||
constructor(message: string) {
|
||||
super('UNAUTHORIZED', message);
|
||||
this.name = 'UnauthorizedException';
|
||||
}
|
||||
}
|
||||
return {
|
||||
DomainException,
|
||||
UnauthorizedException,
|
||||
normalizeVietnamPhone: (phone: string) => {
|
||||
if (!phone) return null;
|
||||
if (phone.startsWith('+84') && phone.length === 12) return phone;
|
||||
if (phone.startsWith('0') && phone.length === 10) return '+84' + phone.slice(1);
|
||||
return null;
|
||||
@@ -121,4 +141,44 @@ describe('LocalStrategy', () => {
|
||||
role: 'BUYER',
|
||||
});
|
||||
});
|
||||
|
||||
it('throws UnauthorizedException when phone is undefined (empty body)', async () => {
|
||||
await expect(strategy.validate(undefined as any, undefined as any)).rejects.toThrow(
|
||||
'Số điện thoại hoặc mật khẩu không đúng',
|
||||
);
|
||||
});
|
||||
|
||||
it('throws UnauthorizedException when phone is null', async () => {
|
||||
await expect(strategy.validate(null as any, 'password')).rejects.toThrow(
|
||||
'Số điện thoại hoặc mật khẩu không đúng',
|
||||
);
|
||||
});
|
||||
|
||||
it('throws UnauthorizedException when password is empty string', async () => {
|
||||
await expect(strategy.validate('0912345678', '')).rejects.toThrow(
|
||||
'Số điện thoại hoặc mật khẩu không đúng',
|
||||
);
|
||||
});
|
||||
|
||||
it('wraps unexpected DB errors as UnauthorizedException', async () => {
|
||||
mockUserRepo.findByPhone.mockRejectedValue(new Error('DB connection refused'));
|
||||
|
||||
await expect(strategy.validate('0912345678', 'password')).rejects.toThrow(
|
||||
'Số điện thoại hoặc mật khẩu không đúng',
|
||||
);
|
||||
});
|
||||
|
||||
it('wraps unexpected bcrypt errors as UnauthorizedException', async () => {
|
||||
mockUserRepo.findByPhone.mockResolvedValue({
|
||||
id: 'user-1',
|
||||
passwordHash: { compare: vi.fn().mockRejectedValue(new Error('bcrypt internal error')) },
|
||||
isActive: true,
|
||||
phone: { value: '+84912345678' },
|
||||
role: 'BUYER',
|
||||
});
|
||||
|
||||
await expect(strategy.validate('0912345678', 'password')).rejects.toThrow(
|
||||
'Số điện thoại hoặc mật khẩu không đúng',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,11 +1,13 @@
|
||||
import { Inject, Injectable } from '@nestjs/common';
|
||||
import { Inject, Injectable, Logger } from '@nestjs/common';
|
||||
import { PassportStrategy } from '@nestjs/passport';
|
||||
import { Strategy } from 'passport-local';
|
||||
import { normalizeVietnamPhone, UnauthorizedException } from '@modules/shared';
|
||||
import { DomainException, normalizeVietnamPhone, UnauthorizedException } from '@modules/shared';
|
||||
import { USER_REPOSITORY, type IUserRepository } from '../../domain/repositories/user.repository';
|
||||
|
||||
@Injectable()
|
||||
export class LocalStrategy extends PassportStrategy(Strategy) {
|
||||
private readonly logger = new Logger(LocalStrategy.name);
|
||||
|
||||
constructor(
|
||||
@Inject(USER_REPOSITORY)
|
||||
private readonly userRepo: IUserRepository,
|
||||
@@ -14,25 +16,38 @@ export class LocalStrategy extends PassportStrategy(Strategy) {
|
||||
}
|
||||
|
||||
async validate(phone: string, password: string): Promise<{ id: string; phone: string; role: string }> {
|
||||
const normalizedPhone = normalizeVietnamPhone(phone);
|
||||
if (!normalizedPhone) {
|
||||
throw new UnauthorizedException('Số điện thoại không hợp lệ');
|
||||
}
|
||||
const user = await this.userRepo.findByPhone(normalizedPhone);
|
||||
try {
|
||||
if (!phone || !password) {
|
||||
throw new UnauthorizedException('Số điện thoại hoặc mật khẩu không đúng');
|
||||
}
|
||||
|
||||
if (!user || !user.passwordHash) {
|
||||
const normalizedPhone = normalizeVietnamPhone(phone);
|
||||
if (!normalizedPhone) {
|
||||
throw new UnauthorizedException('Số điện thoại không hợp lệ');
|
||||
}
|
||||
const user = await this.userRepo.findByPhone(normalizedPhone);
|
||||
|
||||
if (!user || !user.passwordHash) {
|
||||
throw new UnauthorizedException('Số điện thoại hoặc mật khẩu không đúng');
|
||||
}
|
||||
|
||||
if (!user.isActive) {
|
||||
throw new UnauthorizedException('Tài khoản đã bị vô hiệu hóa');
|
||||
}
|
||||
|
||||
const isValid = await user.passwordHash.compare(password);
|
||||
if (!isValid) {
|
||||
throw new UnauthorizedException('Số điện thoại hoặc mật khẩu không đúng');
|
||||
}
|
||||
|
||||
return { id: user.id, phone: user.phone.value, role: user.role };
|
||||
} catch (error) {
|
||||
if (error instanceof DomainException) throw error;
|
||||
this.logger.error(
|
||||
`Authentication failed: ${error instanceof Error ? error.message : error}`,
|
||||
error instanceof Error ? error.stack : undefined,
|
||||
);
|
||||
throw new UnauthorizedException('Số điện thoại hoặc mật khẩu không đúng');
|
||||
}
|
||||
|
||||
if (!user.isActive) {
|
||||
throw new UnauthorizedException('Tài khoản đã bị vô hiệu hóa');
|
||||
}
|
||||
|
||||
const isValid = await user.passwordHash.compare(password);
|
||||
if (!isValid) {
|
||||
throw new UnauthorizedException('Số điện thoại hoặc mật khẩu không đúng');
|
||||
}
|
||||
|
||||
return { id: user.id, phone: user.phone.value, role: user.role };
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,6 +1,30 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { HttpException, HttpStatus } from '@nestjs/common';
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import { LocalAuthGuard } from '../guards/local-auth.guard';
|
||||
|
||||
vi.mock('@nestjs/common', async (importOriginal) => {
|
||||
const actual = (await importOriginal()) as any;
|
||||
return {
|
||||
...actual,
|
||||
Logger: class MockLogger {
|
||||
error = vi.fn();
|
||||
warn = vi.fn();
|
||||
log = vi.fn();
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock('@modules/shared', async () => {
|
||||
const { HttpException: HE, HttpStatus: HS } = await import('@nestjs/common');
|
||||
class UnauthorizedException extends HE {
|
||||
constructor(message: string) {
|
||||
super(message, HS.UNAUTHORIZED);
|
||||
this.name = 'UnauthorizedException';
|
||||
}
|
||||
}
|
||||
return { UnauthorizedException };
|
||||
});
|
||||
|
||||
describe('LocalAuthGuard', () => {
|
||||
let guard: LocalAuthGuard;
|
||||
|
||||
@@ -14,9 +38,24 @@ describe('LocalAuthGuard', () => {
|
||||
expect(result).toEqual(user);
|
||||
});
|
||||
|
||||
it('throws error when error is provided', () => {
|
||||
const error = new Error('Strategy error');
|
||||
expect(() => guard.handleRequest(error, null, undefined, {} as any)).toThrow('Strategy error');
|
||||
it('re-throws HttpException errors directly', () => {
|
||||
const httpError = new HttpException('Bad Request', HttpStatus.BAD_REQUEST);
|
||||
expect(() => guard.handleRequest(httpError, null, undefined, {} as any)).toThrow(HttpException);
|
||||
expect(() => guard.handleRequest(httpError, null, undefined, {} as any)).toThrow('Bad Request');
|
||||
});
|
||||
|
||||
it('wraps non-HttpException errors as UnauthorizedException', () => {
|
||||
const dbError = new Error('DB connection refused');
|
||||
expect(() => guard.handleRequest(dbError, null, undefined, {} as any)).toThrow(
|
||||
'Số điện thoại hoặc mật khẩu không đúng',
|
||||
);
|
||||
});
|
||||
|
||||
it('wraps TypeError (e.g. from null/undefined) as UnauthorizedException', () => {
|
||||
const typeError = new TypeError("Cannot read properties of undefined (reading 'replace')");
|
||||
expect(() => guard.handleRequest(typeError, null, undefined, {} as any)).toThrow(
|
||||
'Số điện thoại hoặc mật khẩu không đúng',
|
||||
);
|
||||
});
|
||||
|
||||
it('throws UnauthorizedException when no user and no error', () => {
|
||||
@@ -24,9 +63,4 @@ describe('LocalAuthGuard', () => {
|
||||
'Số điện thoại hoặc mật khẩu không đúng',
|
||||
);
|
||||
});
|
||||
|
||||
it('re-throws the original error type', () => {
|
||||
const customError = new TypeError('Custom type error');
|
||||
expect(() => guard.handleRequest(customError, null, undefined, {} as any)).toThrow(TypeError);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,12 +1,14 @@
|
||||
import { ApiProperty } from '@nestjs/swagger';
|
||||
import { IsString } from 'class-validator';
|
||||
import { IsNotEmpty, IsString } from 'class-validator';
|
||||
|
||||
export class LoginDto {
|
||||
@ApiProperty({ example: '0901234567' })
|
||||
@IsString()
|
||||
@IsNotEmpty({ message: 'Số điện thoại không được để trống' })
|
||||
phone!: string;
|
||||
|
||||
@ApiProperty({ example: 'P@ssw0rd!' })
|
||||
@IsString()
|
||||
@IsNotEmpty({ message: 'Mật khẩu không được để trống' })
|
||||
password!: string;
|
||||
}
|
||||
|
||||
@@ -1,14 +1,25 @@
|
||||
import { type ExecutionContext, Injectable } from '@nestjs/common';
|
||||
import { type ExecutionContext, HttpException, Injectable, Logger } from '@nestjs/common';
|
||||
import { AuthGuard } from '@nestjs/passport';
|
||||
import { UnauthorizedException } from '@modules/shared';
|
||||
|
||||
@Injectable()
|
||||
export class LocalAuthGuard extends AuthGuard('local') {
|
||||
private readonly logger = new Logger(LocalAuthGuard.name);
|
||||
|
||||
override handleRequest<T>(err: Error | null, user: T, _info: unknown, _context: ExecutionContext): T {
|
||||
// If the strategy threw a DomainException (e.g. our custom UnauthorizedException),
|
||||
// re-throw it directly so GlobalExceptionFilter produces the structured error format.
|
||||
if (err) {
|
||||
throw err;
|
||||
// If the strategy threw a DomainException or HttpException,
|
||||
// re-throw it directly so GlobalExceptionFilter produces the structured error format.
|
||||
if (err instanceof HttpException) {
|
||||
throw err;
|
||||
}
|
||||
// Unexpected infrastructure errors (e.g. DB failure, bcrypt crash) — log and
|
||||
// wrap as 401 to avoid leaking a 500 on the login endpoint.
|
||||
this.logger.error(
|
||||
`Unexpected auth error: ${err.message}`,
|
||||
err.stack,
|
||||
);
|
||||
throw new UnauthorizedException('Số điện thoại hoặc mật khẩu không đúng');
|
||||
}
|
||||
if (!user) {
|
||||
throw new UnauthorizedException('Số điện thoại hoặc mật khẩu không đúng');
|
||||
|
||||
Reference in New Issue
Block a user