vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

fix(auth): handle null-password (SSO/LDAP-only) accounts without bcrypt throw (#70)

Browse Source 
A user with password=NULL passed the missing/disabled guard and reached
comparePasswordHash(pw, null), which native bcrypt rejects -> 500 on
/api/auth/login and, on /mcp, a leaky 401 that the brute-force limiter ignored
(enumeration oracle + limiter evasion). Treat a null/empty password like a
missing user in verifyUserCredentials (dummy compare for timing parity + unified
CREDENTIALS_MISMATCH_MESSAGE) and reject early in changePassword before bcrypt.
Contract spec asserts the null-password guard.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-06-21 03:07:53 +03:00
parent d79f709742
commit 5344a9bdde
2 changed files with 43 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
apps/server/src/core/auth/services/auth.service.ts
View File

@@ -95,12 +95,19 @@ export class AuthService {
// Single source of truth (see auth.constants): the /mcp brute-force limiter
// recognises this exact message via isCredentialsFailure.
const errorMessage = CREDENTIALS_MISMATCH_MESSAGE;
if (!user || isUserDisabled(user)) {
if (!user || isUserDisabled(user) || !user.password) {
// SSO/LDAP-only accounts have no local password hash (user.password is
// null): feeding null to native bcrypt makes it REJECT with
// "data and hash arguments required", which surfaces as a 500 on
// /api/auth/login and as a leaky 401 (not recognised by the /mcp
// brute-force limiter) on /mcp. Treat such accounts like a missing user.
//
// Constant-time intent: run ONE bcrypt comparison (against a dummy hash)
// even when the user is missing/disabled, so this path takes about the
// same time as the real-user wrong-password path below. This closes the
// user-enumeration timing oracle (registered vs. not). The result is
// intentionally discarded — we always throw the same credentials error.
// even when the user is missing/disabled/password-less, so this path takes
// about the same time as the real-user wrong-password path below. This
// closes the user-enumeration timing oracle (registered vs. not). The
// result is intentionally discarded — we always throw the same
// credentials error (recognised by isCredentialsFailure on /mcp).
await comparePasswordHash(loginDto.password, DUMMY_PASSWORD_HASH);
throw new UnauthorizedException(errorMessage);
}
@@ -168,6 +175,15 @@ export class AuthService {
throw new NotFoundException('User not found');
}
// SSO/LDAP-only accounts have no local password hash (user.password is
// null). Passing null to native bcrypt makes it REJECT with
// "data and hash arguments required" (an unhandled 500), so never call
// comparePasswordHash on null. There is no current local password to verify,
// so reject the same way a wrong current password is rejected.
if (!user.password) {
throw new BadRequestException('Current password is incorrect');
}
const comparePasswords = await comparePasswordHash(
dto.oldPassword,
user.password,

24
apps/server/src/core/auth/services/verify-user-credentials.contract.spec.ts
View File

@@ -108,9 +108,10 @@ describe('AuthService no-side-effect contract (item 4)', () => {
// source level for the same reason as the rest of this file: AuthService cannot
// be imported under this jest config to spy on comparePasswordHash live.
describe('constant-time missing/disabled branch (item 4)', () => {
// Isolate the body of the `if (!user || isUserDisabled(user)) { ... }` guard.
// Isolate the body of the
// `if (!user || isUserDisabled(user) || !user.password) { ... }` guard.
const guardMatch = verifyBody.match(
/if \(!user \|\| isUserDisabled\(user\)\) \{([\s\S]*?)\n {4}\}/,
/if \(!user \|\| isUserDisabled\(user\) \|\| !user\.password\) \{([\s\S]*?)\n {4}\}/,
);
it('the missing/disabled guard runs a bcrypt compare before throwing', () => {
@@ -126,6 +127,25 @@ describe('AuthService no-side-effect contract (item 4)', () => {
expect(throwIdx).toBeGreaterThan(compareIdx);
});
// null-password (SSO/LDAP-only) accounts have user.password === null. The
// missing/disabled guard MUST also short-circuit on a null/empty password,
// otherwise comparePasswordHash(loginDto.password, null) feeds null to native
// bcrypt, which REJECTS ("data and hash arguments required") — a 500 on
// /api/auth/login and a leaky, limiter-evading 401 on /mcp. A regression that
// drops this null check fails here.
it('the guard also short-circuits null-password (SSO/LDAP-only) accounts', () => {
expect(guardMatch).not.toBeNull();
// The guard CONDITION includes a null/empty password check...
expect(verifyBody).toMatch(
/if \(!user \|\| isUserDisabled\(user\) \|\| !user\.password\)/,
);
// ...and the password-less branch reuses the same dummy-compare-then-throw
// body, so it never reaches the real `comparePasswordHash(..., user.password)`.
const guardBody = guardMatch![1];
expect(guardBody).toContain('comparePasswordHash');
expect(guardBody).toContain('throw new');
});
it('uses a module-level dummy hash constant (never a real credential)', () => {
// The dummy hash is a module-level constant referenced in the guard, not an
// inline literal recomputed per call.