From 63f7eab4bbc31fdcde8ea46a40abaf070cdb40d9 Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 7 Oct 2026 16:07:23 +0200 Subject: [PATCH] fix(auth): match email addresses case-insensitively everywhere Login and the OIDC fallback match emails case-insensitively, but the password reset lookup and the admin create and edit checks compared exactly. An account stored as Foo@x.com never got a reset mail, and an admin could create bob@x next to Bob@x, after which login picked whichever row came first. The signup collision check and the first-user count also move inside the signup transaction. --- .../src/db/repositories/Users.repository.ts | 39 ++++++++----------- server/src/nest/admin/admin.service.ts | 4 +- server/src/nest/auth/auth.service.ts | 17 ++++---- .../db/repositories/Users.repository.test.ts | 25 ++++++------ server/tests/unit/nest/admin.service.test.ts | 13 ++++++- 5 files changed, 53 insertions(+), 45 deletions(-) diff --git a/server/src/db/repositories/Users.repository.ts b/server/src/db/repositories/Users.repository.ts index 6a37d836d..617790a60 100644 --- a/server/src/db/repositories/Users.repository.ts +++ b/server/src/db/repositories/Users.repository.ts @@ -693,15 +693,15 @@ export class UsersRepository extends TrekRepository { // --------------------------------------------------------------------- /** - * `SELECT id, email, password_hash, oidc_sub FROM users WHERE email = ? - * AND COALESCE(is_guest, 0) = 0` — case-SENSITIVE on `email`, unlike - * every other email lookup in this repository (no `LOWER()` in the legacy - * statement here); kept exactly as written, not "fixed" to match the - * others (parity is law). + * `SELECT id, email, password_hash, oidc_sub FROM users WHERE LOWER(email) + * = LOWER(?) AND COALESCE(is_guest, 0) = 0`. Case-insensitive like login: + * the caller lowercases the address it was given, and an exact match never + * found an account stored as `Foo@x.com`, which then got no reset mail. */ async findForPasswordReset(email: string): Promise { + const platform = this.getEntityManager().getPlatform(); const row = await this.findOne( - { email, is_guest: 0 }, + { [lower(platform, 'email')]: lowerParam(platform, email), is_guest: 0 }, { fields: ['id', 'email', 'password_hash', 'oidc_sub'] }, ); return row ? { id: row.id, email: row.email, password_hash: row.password_hash, oidc_sub: row.oidc_sub ?? null } : null; @@ -928,15 +928,22 @@ export class UsersRepository extends TrekRepository { // --------------------------------------------------------------------- /** - * `SELECT id FROM users WHERE LOWER(email) = LOWER(?) AND id != ? AND COALESCE(is_guest, 0) = 0` + * `SELECT id FROM users WHERE LOWER(email) = LOWER(?) [AND id != ?] AND COALESCE(is_guest, 0) = 0` * * Both sides folded by SQLite's own `LOWER()` (`lowerParam` binds the RAW - * value) — program rule 18. + * value) — program rule 18. Every email collision check goes through here + * (profile, admin create and update), because login and the OIDC fallback + * match case-insensitively: a second account that differs only in case + * would make them pick whichever row comes first. */ - async findIdByEmailCI(email: string, excludeId: number): Promise { + async findIdByEmailCI(email: string, excludeId?: number): Promise { const platform = this.getEntityManager().getPlatform(); const row = await this.findOne( - { [lower(platform, 'email')]: lowerParam(platform, email), id: { $ne: excludeId }, is_guest: 0 }, + { + [lower(platform, 'email')]: lowerParam(platform, email), + ...(excludeId === undefined ? {} : { id: { $ne: excludeId } }), + is_guest: 0, + }, { fields: ['id'] }, ); return row?.id ?? null; @@ -1513,12 +1520,6 @@ export class UsersRepository extends TrekRepository { return row?.id ?? null; } - /** AD3 (`createUser`) — `SELECT id FROM users WHERE email = ? AND COALESCE(is_guest, 0) = 0`. Case-sensitive, same reasoning as {@link findIdByUsernameExact}. */ - async findIdByEmailExact(email: string): Promise { - const row = await this.findOne({ email, is_guest: 0 }, { fields: ['id'] }); - return row?.id ?? null; - } - /** * AD4 (`createUser`'s write) — `INSERT INTO users (username, email, * password_hash, role) VALUES (?, ?, ?, ?)`. Only these four columns are @@ -1564,12 +1565,6 @@ export class UsersRepository extends TrekRepository { return row?.id ?? null; } - /** AD8 (`updateUser`) — `SELECT id FROM users WHERE email = ? AND id != ? AND COALESCE(is_guest, 0) = 0`. Case-sensitive, excludes self, same reasoning as {@link findIdByUsernameExactExcluding}. */ - async findIdByEmailExactExcluding(email: string, excludeId: number): Promise { - const row = await this.findOne({ email, id: { $ne: excludeId }, is_guest: 0 }, { fields: ['id'] }); - return row?.id ?? null; - } - /** * AD11 (`updateUser`, inside `uow.transactional` alongside AD12/AD13) — * `UPDATE users SET username=COALESCE(?,username), email=COALESCE(?,email), diff --git a/server/src/nest/admin/admin.service.ts b/server/src/nest/admin/admin.service.ts index 1f38d66a5..29bee08bd 100644 --- a/server/src/nest/admin/admin.service.ts +++ b/server/src/nest/admin/admin.service.ts @@ -170,7 +170,7 @@ export class AdminService { const existingUsername = await this.users.findIdByUsernameExact(username); if (existingUsername) return { error: 'Username already taken', status: 409 }; - const existingEmail = await this.users.findIdByEmailExact(email); + const existingEmail = await this.users.findIdByEmailCI(email); if (existingEmail) return { error: 'Email already taken', status: 409 }; const passwordHash = bcrypt.hashSync(password, BCRYPT_COST); @@ -211,7 +211,7 @@ export class AdminService { if (conflict) return { error: 'Username already taken', status: 409 }; } if (email && email !== user.email) { - const conflict = await this.users.findIdByEmailExactExcluding(email, userId); + const conflict = await this.users.findIdByEmailCI(email, userId); if (conflict) return { error: 'Email already taken', status: 409 }; } diff --git a/server/src/nest/auth/auth.service.ts b/server/src/nest/auth/auth.service.ts index 36ffe6ad8..e7231e02a 100644 --- a/server/src/nest/auth/auth.service.ts +++ b/server/src/nest/auth/auth.service.ts @@ -479,20 +479,21 @@ export class AuthService { return { error: 'Invalid email format', status: 400 }; } - // Ignore guests (#1362): their synthetic username/email must never block a real signup. - const existingUserId = await this.usersRepo.findIdByEmailOrUsernameCI(email, username); - if (existingUserId !== null) { - return { error: 'Registration failed. Please try different credentials.', status: 409 }; - } - const password_hash = bcrypt.hashSync(password, BCRYPT_COST); - const isFirstUser = userCount === 0; - const role = isFirstUser ? 'admin' : 'user'; try { // One transaction for the whole signup: a mid-sequence throw (invite // bookkeeping, trip auto-join) must not leave a half-registered user. + // The collision check and the first-user count run inside it too, so two + // signups at once cannot both pass the check or both become admin. return await this.uow.transactional(async () => { + // Ignore guests (#1362): their synthetic username/email must never block a real signup. + const existingUserId = await this.usersRepo.findIdByEmailOrUsernameCI(email, username); + if (existingUserId !== null) { + return { error: 'Registration failed. Please try different credentials.', status: 409 }; + } + const isFirstUser = (await this.usersRepo.countNonGuest()) === 0; + const role = isFirstUser ? 'admin' : 'user'; const inserted = await this.usersRepo.insertUser({ username, email, password_hash, role, first_seen_version: readEnv().app.appVersion || '0.0.0', }); diff --git a/server/tests/unit/db/repositories/Users.repository.test.ts b/server/tests/unit/db/repositories/Users.repository.test.ts index e4979500d..dea5a7dff 100644 --- a/server/tests/unit/db/repositories/Users.repository.test.ts +++ b/server/tests/unit/db/repositories/Users.repository.test.ts @@ -514,12 +514,12 @@ describe('UsersRepository', () => { }); }); - describe('findForPasswordReset (AU36) — case-SENSITIVE email, unlike every other email lookup here', () => { - it('USERSREPO-035: matches the exact case only', async () => { + describe('findForPasswordReset (AU36) — case-insensitive, like login', () => { + it('USERSREPO-035: finds an address stored with capitals from the lowercased input the service passes', async () => { const { user } = createUser(testDb, { email: 'Exact@Example.com' }); - const row = await users.findForPasswordReset('Exact@Example.com'); - expect(row?.id).toBe(user.id); - expect(await users.findForPasswordReset('exact@example.com')).toBeNull(); // no LOWER() in the legacy statement + expect((await users.findForPasswordReset('exact@example.com'))?.id).toBe(user.id); + expect((await users.findForPasswordReset('Exact@Example.com'))?.id).toBe(user.id); + expect(await users.findForPasswordReset('other@example.com')).toBeNull(); }); it('USERSREPO-036: excludes a guest', async () => { @@ -1063,13 +1063,13 @@ describe('UsersRepository — admin (AD1-AD17) read methods, full-key parity', ( expect(rows.map((r) => r.id)).toEqual(expect.arrayContaining([admin.id, plain.id])); }); - it('USERSREPO-079 (AD2/AD3): findIdByUsernameExact/findIdByEmailExact are case-sensitive and exclude guests', async () => { + it('USERSREPO-079 (AD2/AD3): findIdByUsernameExact is case-sensitive, the email check is not, and both exclude guests', async () => { expect(await users.findIdByUsernameExact('ad-admin')).toBe(admin.id); expect(await users.findIdByUsernameExact('AD-ADMIN')).toBeNull(); // case-sensitive, unlike findIdByUsernameCIAny expect(await users.findIdByUsernameExact('ad-guest')).toBeNull(); // guest excluded - expect(await users.findIdByEmailExact('ad-plain@example.com')).toBe(plain.id); - expect(await users.findIdByEmailExact('AD-PLAIN@EXAMPLE.COM')).toBeNull(); - expect(await users.findIdByEmailExact('ad-guest@example.com')).toBeNull(); + expect(await users.findIdByEmailCI('ad-plain@example.com')).toBe(plain.id); + expect(await users.findIdByEmailCI('AD-PLAIN@EXAMPLE.COM')).toBe(plain.id); + expect(await users.findIdByEmailCI('ad-guest@example.com')).toBeNull(); }); it('USERSREPO-080 (AD4/AD5): insertAdminCreatedUser stores exactly the four named columns, every other column left to the entity default, then findAdminSummary re-selects it', async () => { @@ -1097,14 +1097,15 @@ describe('UsersRepository — admin (AD1-AD17) read methods, full-key parity', ( expect(await users.findAdminSummary(999999)).toBeNull(); }); - it('USERSREPO-082 (AD7/AD8): findIdByUsernameExactExcluding/findIdByEmailExactExcluding exclude the caller\'s own row, case-sensitive, guests excluded', async () => { + it('USERSREPO-082 (AD7/AD8): findIdByUsernameExactExcluding/findIdByEmailCI exclude the caller\'s own row and guests', async () => { // Renaming plain to admin's own username, excluding plain's own id, must // still find admin's row — that's the whole point of the check. expect(await users.findIdByUsernameExactExcluding('ad-admin', plain.id)).toBe(admin.id); expect(await users.findIdByUsernameExactExcluding('ad-plain', plain.id)).toBeNull(); // excludes self expect(await users.findIdByUsernameExactExcluding('ad-guest', plain.id)).toBeNull(); // guest excluded - expect(await users.findIdByEmailExactExcluding('ad-admin@example.com', plain.id)).toBe(admin.id); - expect(await users.findIdByEmailExactExcluding('ad-plain@example.com', plain.id)).toBeNull(); + expect(await users.findIdByEmailCI('ad-admin@example.com', plain.id)).toBe(admin.id); + expect(await users.findIdByEmailCI('AD-ADMIN@example.com', plain.id)).toBe(admin.id); + expect(await users.findIdByEmailCI('ad-plain@example.com', plain.id)).toBeNull(); }); it('USERSREPO-083 (AD11): applyAdminEdit only touches the keys supplied (coalesceParam semantics) and always stamps updated_at', async () => { diff --git a/server/tests/unit/nest/admin.service.test.ts b/server/tests/unit/nest/admin.service.test.ts index 356b6ce34..b9160bbff 100644 --- a/server/tests/unit/nest/admin.service.test.ts +++ b/server/tests/unit/nest/admin.service.test.ts @@ -975,7 +975,7 @@ describe('createUser — the pre-existing uniqueness-check TOCTOU window (AD2-4, // INSERT — the two statements are two separate, un-transacted repository // calls, exactly as the legacy raw SQL was. const usersRepo = await createTestUsersRepo(testDb); - const spy = vi.spyOn(usersRepo, 'findIdByEmailExact').mockResolvedValueOnce(null); + const spy = vi.spyOn(usersRepo, 'findIdByEmailCI').mockResolvedValueOnce(null); // The pre-check said "clear," but `users.email` carries its own UNIQUE // index (`Migration20200101000000_baseline_schema.ts`) — that's what @@ -990,4 +990,15 @@ describe('createUser — the pre-existing uniqueness-check TOCTOU window (AD2-4, spy.mockRestore(); }); + + it('ADMIN-SVC-095 — an address that differs only in case is already taken, on create and on update', async () => { + const { user: existing } = createUser(testDb, { email: 'Case@Test.example.com' }); + const { user: other } = createUser(testDb, { email: 'other-case@test.example.com' }); + + await expect(svcCreateUser({ username: 'caseuser', email: 'case@test.example.com', password: 'ValidPass1!' })) + .resolves.toEqual({ error: 'Email already taken', status: 409 }); + await expect(updateUser(String(other.id), { email: 'CASE@TEST.EXAMPLE.COM' })) + .resolves.toEqual({ error: 'Email already taken', status: 409 }); + expect(existing.id).toBeGreaterThan(0); + }); });