diff --git a/e2e/src/specs/server/api/oauth.e2e-spec.ts b/e2e/src/specs/server/api/oauth.e2e-spec.ts index 157fdfc84c..8851356c9e 100644 --- a/e2e/src/specs/server/api/oauth.e2e-spec.ts +++ b/e2e/src/specs/server/api/oauth.e2e-spec.ts @@ -351,7 +351,7 @@ describe(`/oauth`, () => { const callbackParams = await loginWithOAuth('oauth-no-auto-register'); const { status, body } = await request(app).post('/oauth/callback').send(callbackParams); expect(status).toBe(400); - expect(body).toEqual(errorDto.badRequest('User does not exist and auto registering is disabled.')); + expect(body).toEqual(errorDto.badRequest('OAuth authentication failed')); }); it('should link to an existing user by email', async () => { diff --git a/e2e/src/specs/server/api/user.e2e-spec.ts b/e2e/src/specs/server/api/user.e2e-spec.ts index ee13a29c1b..7623cb5a63 100644 --- a/e2e/src/specs/server/api/user.e2e-spec.ts +++ b/e2e/src/specs/server/api/user.e2e-spec.ts @@ -120,7 +120,7 @@ describe('/users', () => { .set('Authorization', `Bearer ${nonAdmin.accessToken}`); expect(status).toBe(400); - expect(body).toMatchObject(errorDto.badRequest('Email already in use by another account')); + expect(body).toMatchObject(errorDto.badRequest('Email is not available')); }); it('should update my email', async () => { diff --git a/server/src/constants.ts b/server/src/constants.ts index 5af37ef797..c771c1a995 100644 --- a/server/src/constants.ts +++ b/server/src/constants.ts @@ -36,6 +36,8 @@ export const VECTOR_INDEX_TABLES = { export const VECTORCHORD_LIST_SLACK_FACTOR = 1.2; export const SALT_ROUNDS = 10; +// Syntactically valid bcrypt hash used in login() preventing timing-based user enumeration. +export const LOGIN_DUMMY_HASH = '$2b$10$abcdefghijklmnopqrstuuABCDEFGHIJKLMNOPQRSTUVWXYZabcde'; export const IWorker = 'IWorker'; diff --git a/server/src/services/album.service.ts b/server/src/services/album.service.ts index 24b0668eb2..7bfc0bdcc2 100644 --- a/server/src/services/album.service.ts +++ b/server/src/services/album.service.ts @@ -107,7 +107,8 @@ export class AlbumService extends BaseService { for (const { userId } of albumUsers) { const exists = await this.userRepository.get(userId, {}); if (!exists) { - throw new BadRequestException('User not found'); + this.logger.debug('Album creation failed: user not found'); + throw new BadRequestException('Invalid user'); } if (userId == auth.user.id) { @@ -302,7 +303,8 @@ export class AlbumService extends BaseService { const user = await this.userRepository.get(userId, {}); if (!user) { - throw new BadRequestException('User not found'); + this.logger.debug('Adding user to album failed: user not found'); + throw new BadRequestException('Invalid user'); } await this.albumUserRepository.create({ userId, albumId: id, role }); diff --git a/server/src/services/auth.service.ts b/server/src/services/auth.service.ts index 628e863712..5323252738 100644 --- a/server/src/services/auth.service.ts +++ b/server/src/services/auth.service.ts @@ -2,7 +2,7 @@ import { BadRequestException, ForbiddenException, Injectable, UnauthorizedExcept import { parse } from 'cookie'; import { DateTime } from 'luxon'; import { IncomingHttpHeaders } from 'node:http'; -import { LOGIN_URL, MOBILE_REDIRECT, SALT_ROUNDS } from 'src/constants'; +import { LOGIN_DUMMY_HASH, LOGIN_URL, MOBILE_REDIRECT, SALT_ROUNDS } from 'src/constants'; import { AuthSharedLink, AuthUser, UserAdmin } from 'src/database'; import { AuthDto, @@ -62,15 +62,12 @@ export class AuthService extends BaseService { throw new UnauthorizedException('Password login has been disabled'); } - let user = await this.userRepository.getByEmail(dto.email, { withPassword: true }); - if (user) { - const isAuthenticated = this.validateSecret(dto.password, user.password); - if (!isAuthenticated) { - user = undefined; - } - } + const user = await this.userRepository.getByEmail(dto.email, { withPassword: true }); + // Always run bcrypt so response time is constant regardless of whether the email + // is registered, preventing timing-based user enumeration. + const authenticated = this.cryptoRepository.compareBcrypt(dto.password, user?.password ?? LOGIN_DUMMY_HASH); - if (!user) { + if (!user || !user.password || !authenticated) { this.logger.warn(`Failed login attempt for user ${dto.email} from ip address ${details.clientIp}`); throw new UnauthorizedException('Incorrect email or password'); } @@ -325,7 +322,8 @@ export class AuthService extends BaseService { const emailUser = await this.userRepository.getByEmail(normalizedEmail); if (emailUser) { if (emailUser.oauthId) { - throw new BadRequestException('User already exists, but is linked to another account.'); + this.logger.debug('OAuth login conflict: email already linked to different account'); + throw new BadRequestException('OAuth authentication failed'); } user = await this.userRepository.update(emailUser.id, { oauthId: profile.sub }); } @@ -335,9 +333,9 @@ export class AuthService extends BaseService { if (!user) { if (!autoRegister) { this.logger.warn( - `Unable to register ${profile.sub}/${normalizedEmail || '(no email)'}. To enable set OAuth Auto Register to true in admin settings.`, + `Unable to register ${profile.sub}/${normalizedEmail || '(no email)'}. User does not exist and auto registering is disabled. To enable set OAuth Auto Register to true in admin settings.`, ); - throw new BadRequestException(`User does not exist and auto registering is disabled.`); + throw new BadRequestException('OAuth authentication failed'); } if (!normalizedEmail) { diff --git a/server/src/services/base.service.ts b/server/src/services/base.service.ts index dc402592cd..d930dd0a31 100644 --- a/server/src/services/base.service.ts +++ b/server/src/services/base.service.ts @@ -218,7 +218,8 @@ export class BaseService { async createUser(dto: Insertable & { email: string }): Promise { const exists = await this.userRepository.getByEmail(dto.email); if (exists) { - throw new BadRequestException('User exists'); + this.logger.debug('User creation rejected: user already exists'); + throw new BadRequestException('Email is not available'); } if (!dto.isAdmin) { diff --git a/server/src/services/shared-link.service.ts b/server/src/services/shared-link.service.ts index 31c50b7c2c..0643a432b8 100644 --- a/server/src/services/shared-link.service.ts +++ b/server/src/services/shared-link.service.ts @@ -110,7 +110,8 @@ export class SharedLinkService extends BaseService { private handleError(error: unknown): never { if ((error as PostgresError).constraint_name === 'shared_link_slug_uq') { - throw new BadRequestException('Shared link with this slug already exists'); + this.logger.debug('Shared link with this slug already exists'); + throw new BadRequestException('Failed to save shared link'); } throw error; } diff --git a/server/src/services/user-admin.service.ts b/server/src/services/user-admin.service.ts index 58b4221cc9..2a57fdd299 100644 --- a/server/src/services/user-admin.service.ts +++ b/server/src/services/user-admin.service.ts @@ -64,7 +64,8 @@ export class UserAdminService extends BaseService { if (dto.email) { const duplicate = await this.userRepository.getByEmail(dto.email); if (duplicate && duplicate.id !== id) { - throw new BadRequestException('Email already in use by another account'); + this.logger.debug('Email already in use by another account'); + throw new BadRequestException('Email is not available'); } } diff --git a/server/src/services/user.service.spec.ts b/server/src/services/user.service.spec.ts index 847f96cfc6..a00efe82fd 100644 --- a/server/src/services/user.service.spec.ts +++ b/server/src/services/user.service.spec.ts @@ -179,7 +179,7 @@ describe(UserService.name, () => { it('should throw an error if the user does not exist', async () => { mocks.user.get.mockResolvedValue(void 0); - await expect(sut.getProfileImage(userStub.admin.id)).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.getProfileImage(userStub.admin.id)).rejects.toBeInstanceOf(NotFoundException); expect(mocks.user.get).toHaveBeenCalledWith(userStub.admin.id, {}); }); diff --git a/server/src/services/user.service.ts b/server/src/services/user.service.ts index 8e1f74bcf4..82ab90a590 100644 --- a/server/src/services/user.service.ts +++ b/server/src/services/user.service.ts @@ -49,7 +49,8 @@ export class UserService extends BaseService { if (dto.email) { const duplicate = await this.userRepository.getByEmail(dto.email); if (duplicate && duplicate.id !== user.id) { - throw new BadRequestException('Email already in use by another account'); + this.logger.warn('Email already in use by another account'); + throw new BadRequestException('Email is not available'); } } @@ -134,9 +135,10 @@ export class UserService extends BaseService { } async getProfileImage(id: string): Promise { - const user = await this.findOrFail(id, {}); - if (!user.profileImagePath) { - throw new NotFoundException('User does not have a profile image'); + const user = await this.userRepository.get(id, {}); + if (!user || !user.profileImagePath) { + this.logger.debug('User or profile image not found'); + throw new NotFoundException(); } return new ImmichFileResponse({ diff --git a/server/test/medium/specs/services/user.service.spec.ts b/server/test/medium/specs/services/user.service.spec.ts index 2250034eea..c8c990a8da 100644 --- a/server/test/medium/specs/services/user.service.spec.ts +++ b/server/test/medium/specs/services/user.service.spec.ts @@ -48,7 +48,7 @@ describe(UserService.name, () => { ctx.getMock(EventRepository).emit.mockResolvedValue(); const user = mediumFactory.userInsert(); await expect(sut.createUser({ name: 'Test', email: user.email })).resolves.toMatchObject({ email: user.email }); - await expect(sut.createUser({ name: 'Test', email: user.email })).rejects.toThrow('User exists'); + await expect(sut.createUser({ name: 'Test', email: user.email })).rejects.toThrow('Email is not available'); }); it('should not return password', async () => {