diff --git a/apps/server/src/core/auth/services/auth.service.ts b/apps/server/src/core/auth/services/auth.service.ts index 1c952f6e..986084b9 100644 --- a/apps/server/src/core/auth/services/auth.service.ts +++ b/apps/server/src/core/auth/services/auth.service.ts @@ -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, diff --git a/apps/server/src/core/auth/services/verify-user-credentials.contract.spec.ts b/apps/server/src/core/auth/services/verify-user-credentials.contract.spec.ts index e7b37e08..4b84ac50 100644 --- a/apps/server/src/core/auth/services/verify-user-credentials.contract.spec.ts +++ b/apps/server/src/core/auth/services/verify-user-credentials.contract.spec.ts @@ -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.