feat(auth): refuse non-bcrypt password hashes before calling bcrypt
compareLocalHash() returns false for any passwordHash that does not start with `$2`: '' (accounts created without a password) and the new OTP_ONLY_PASSWORD_HASH sentinel the data import writes for ~1,590 OTP-only legacy customers. It is used on both local-compare paths of verifyPassword(), IAM off and IAM fallback. bcryptjs 3 already returns false for these. The guard keeps the result from depending on that: a future version or a native binding that throws would turn a wrong password into a 500 (owner ruling 2026-10-07). Behaviour-neutral for existing data. Real hashes start with $2a$, $2b$ or PHP's $2y$, and every row that fails the prefix already failed in bcrypt. Verified live: wrong password 401, correct password 200. Tests: refusal for both values with IAM off and on, bcrypt never called for them, and legacy $2y$ hashes still verify. 21/21. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
e020e6a633
commit
884413513d
@@ -9,7 +9,18 @@ import * as bcrypt from 'bcryptjs';
|
||||
*/
|
||||
jest.mock('@nestjs/jwt', () => ({ JwtService: class JwtService {} }));
|
||||
|
||||
import { AuthService } from './auth.service';
|
||||
/**
|
||||
* The real bcryptjs, with `compare` wrapped so a test can assert it was never
|
||||
* reached. It calls straight through, so every other test still runs against
|
||||
* the real implementation. (A namespace import's properties cannot be
|
||||
* re-defined, so jest.spyOn is not an option here.)
|
||||
*/
|
||||
jest.mock('bcryptjs', () => {
|
||||
const actual = jest.requireActual('bcryptjs');
|
||||
return { ...actual, compare: jest.fn(actual.compare) };
|
||||
});
|
||||
|
||||
import { AuthService, compareLocalHash, OTP_ONLY_PASSWORD_HASH } from './auth.service';
|
||||
import type { AdminSessionService } from '../sessions/admin-session.service';
|
||||
import {
|
||||
IamInvalidCredentialsError,
|
||||
@@ -258,6 +269,58 @@ describe('AuthService login', () => {
|
||||
);
|
||||
});
|
||||
|
||||
/* ─────────────── accounts with no usable password ─────────────── */
|
||||
|
||||
describe('accounts with no usable password', () => {
|
||||
const noPassword = ['', OTP_ONLY_PASSWORD_HASH];
|
||||
|
||||
it.each(noPassword)('refuses login for hash %j, whatever is typed', async (hash) => {
|
||||
const { service } = build(disabledIam(), { ...adminRow(), passwordHash: hash });
|
||||
|
||||
// Including the empty string and the sentinel itself: neither may open it
|
||||
for (const typed of ['', 'x', OTP_ONLY_PASSWORD_HASH]) {
|
||||
await expect(
|
||||
service.login({ phone: '09120000000', password: typed }),
|
||||
).rejects.toThrow('نام کاربری یا رمز عبور اشتباه است');
|
||||
}
|
||||
});
|
||||
|
||||
it.each(noPassword)('refuses hash %j on the IAM fallback path too', async (hash) => {
|
||||
const verifyCredentials = jest.fn().mockRejectedValue(new IamUnavailableError('down'));
|
||||
const { service } = build(
|
||||
{ enabled: true, allowsLocalFallback: true, verifyCredentials },
|
||||
{ ...adminRow(), passwordHash: hash },
|
||||
);
|
||||
|
||||
await expect(
|
||||
service.login({ phone: '09120000000', password: '' }),
|
||||
).rejects.toBeInstanceOf(UnauthorizedException);
|
||||
});
|
||||
|
||||
it('never hands a malformed hash to bcrypt', async () => {
|
||||
const compare = bcrypt.compare as unknown as jest.Mock;
|
||||
compare.mockClear();
|
||||
|
||||
expect(await compareLocalHash('x', OTP_ONLY_PASSWORD_HASH)).toBe(false);
|
||||
expect(await compareLocalHash('x', '')).toBe(false);
|
||||
expect(await compareLocalHash('x', null)).toBe(false);
|
||||
expect(compare).not.toHaveBeenCalled();
|
||||
|
||||
// …while a real hash still goes through it
|
||||
await compareLocalHash(PASSWORD, passwordHash);
|
||||
expect(compare).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("still accepts the legacy server's PHP $2y$ hashes", async () => {
|
||||
// PHP's $2y$ is the same algorithm as $2b$; the legacy passwords
|
||||
// (customers and the sales experts moving to AdminUser) use it.
|
||||
const legacy = '$2y$' + (await bcrypt.hash(PASSWORD, 4)).slice(4);
|
||||
|
||||
expect(await compareLocalHash(PASSWORD, legacy)).toBe(true);
|
||||
expect(await compareLocalHash('wrong', legacy)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
/* ─────────────── two-factor ─────────────── */
|
||||
|
||||
it('explains that a two-factor account cannot log in through this path', async () => {
|
||||
|
||||
Reference in New Issue
Block a user