From a526ee526f553df2164d966d378fd44ab828ea47 Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Sat, 10 Oct 2026 18:54:08 -0400 Subject: [PATCH] refactor(auth): one confirmEmail for the code and link confirmation routes Both routes now re-run the email checks, refuse an address another account confirmed, strip it from other rows, promote the group and push user.email_confirmed to the account's sockets (with the originating socket id on the code route). The link route's full-table UPDATE clearing other accounts' pending change-email requests is dropped: /change_email/confirm already refuses an address a confirmed account owns. --- .../controllers/auth/AuthController.ts | 56 +------------ .../static/StaticPagesController.test.ts | 32 +++++++ .../static/StaticPagesController.ts | 83 ++----------------- .../services/user/SignupService.test.ts | 74 ++++++++++++++++- src/backend/services/user/SignupService.ts | 66 ++++++++++++++- 5 files changed, 176 insertions(+), 135 deletions(-) diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index 0aabb4cbb..5c5b48b67 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -1086,62 +1086,10 @@ export class AuthController extends PuterController { return; } - // Re-validate the email at confirmation time — the address may - // have been added to the blocklist (or flagged by an extension) - // after signup but before confirmation. - await this.services.signup.validateEmail(user.email!); - - // An account that already confirmed this address proved access to the - // inbox, and revoking it below would hand the address to whoever - // confirmed second. Refuse instead — a duplicate this old is data to - // repair, not a race to resolve. - const canonical = cleanEmail(user.email!); - const confirmedRival = await this.stores.user.findConfirmedOtherByEmail( - user.id, - user.email!, - canonical, - ); - if (confirmedRival) { - throw new HttpError( - 400, - 'This email was confirmed on a different account.', - { legacyCode: 'email_already_in_use' as never }, - ); - } - - // Revoke the address from every remaining (unconfirmed) account holding - // it, THEN confirm this one. Only one row may own an address, so - // confirming first would momentarily create a second owner — which the - // unique index rejects, turning a legitimate confirmation into a 500. - await this.stores.user.unconfirmOthersByEmail( - user.id, - user.email!, - canonical, - ); - - await this.stores.user.update(user.id, { - email_confirmed: 1, - requires_email_confirmation: 0, - email_confirm_code: null, - email_confirm_token: null, + await this.services.signup.confirmEmail(user, { + originalClientSocketId: original_client_socket_id, }); - await promoteToVerifiedGroup(this.stores.group, this.config, user); - - try { - this.clients.event?.emit( - 'user.email-confirmed' as never, - { - user_id: user.id, - user_uid: user.uuid, - email: user.email, - } as never, - {}, - ); - } catch { - // ignore — event is a side-channel signal, not load-bearing - } - res.json({ email_confirmed: true, original_client_socket_id }); } diff --git a/src/backend/controllers/static/StaticPagesController.test.ts b/src/backend/controllers/static/StaticPagesController.test.ts index 447aafa8b..d2f937183 100644 --- a/src/backend/controllers/static/StaticPagesController.test.ts +++ b/src/backend/controllers/static/StaticPagesController.test.ts @@ -375,6 +375,38 @@ describe('StaticPagesController GET /confirm-email-by-token', () => { expect(Boolean(refreshed?.requires_email_confirmation)).toBe(false); }); + it('re-runs the email checks, as the code route does', async () => { + const user = await makeUser({ email_confirmed: 0 }); + const deny = (_k: string, data: unknown) => { + Object.assign(data as object, { + allow: false, + message: 'Address no longer accepted.', + }); + }; + server.clients.event.on('email.validate', deny); + const { res, captured } = makeRes(); + try { + await callRoute( + 'get', + '/confirm-email-by-token', + makeReq({ + query: { + user_uuid: user.uuid, + token: user.email_confirm_token, + }, + }), + res, + ); + } finally { + server.clients.event.off('email.validate', deny); + } + expect(String(captured.body)).toContain('Address no longer accepted.'); + const refreshed = await server.stores.user.getById(user.id, { + force: true, + }); + expect(Boolean(refreshed?.email_confirmed)).toBe(false); + }); + it('rejects when the email is already confirmed on a different account', async () => { // Duplicate-email gate fires only when the existing row is // (a) email_confirmed=1 and (b) has a non-null password — the diff --git a/src/backend/controllers/static/StaticPagesController.ts b/src/backend/controllers/static/StaticPagesController.ts index 8565990ee..e26684b1e 100644 --- a/src/backend/controllers/static/StaticPagesController.ts +++ b/src/backend/controllers/static/StaticPagesController.ts @@ -19,7 +19,7 @@ import { PuterController } from '../types.js'; import type { PuterRouter } from '../../core/http/PuterRouter'; -import { promoteToVerifiedGroup } from '../../util/userProvisioning.js'; +import { HttpError } from '../../core/http/HttpError.js'; /** * One-off user-facing pages. @@ -274,85 +274,14 @@ export class StaticPagesController extends PuterController { return; } - // v2 writes `clean_email` at signup (lowercased email). Older rows - // that predate that may be null — fall back to email.lower(). - const cleanEmail = - (user.clean_email as string | null | undefined) ?? - String(user.email ?? '').toLowerCase(); - - // An account that already confirmed this address proved access - // to the inbox. The strip below would take it away from them, - // so refuse here instead. Password-less accounts count: an - // identity provider verified the address for those, and they - // are exactly what the old `password IS NOT NULL` clause let - // through. - const confirmedRival = - await this.stores.user.findConfirmedOtherByEmail( - user.id as number, - user.email as string, - cleanEmail, - ); - if (confirmedRival) { - res.send( - err('This email was confirmed on a different account.'), - ); + try { + await this.services.signup.confirmEmail(user); + } catch (e) { + if (!(e instanceof HttpError)) throw e; + res.send(err(e.message)); return; } - // Revoke any other accounts' pending change-email slots targeting - // this address — they're no longer valid once someone confirms it. - await this.clients.db.write( - 'UPDATE `user` SET `unconfirmed_change_email` = NULL, `change_email_confirm_token` = NULL WHERE `unconfirmed_change_email` = ?', - [user.email], - ); - - // Take the address off every remaining row before confirming - // this one. The check above leaves only unconfirmed rows, and - // only one row may own an address once this one is confirmed. - await this.stores.user.unconfirmOthersByEmail( - user.id, - user.email as string, - cleanEmail, - ); - - await this.stores.user.update(user.id, { - email_confirmed: 1, - requires_email_confirmation: 0, - email_confirm_code: null, - email_confirm_token: null, - }); - - await promoteToVerifiedGroup( - this.stores.group, - this.config, - user, - ); - - // Best-effort side-channels — don't fail the user-visible response - // if sockets or the event bus are unavailable. - try { - await this.services.socket.send( - { room: user.id }, - 'user.email_confirmed', - {}, - ); - } catch { - /* ignore */ - } - try { - this.clients.event?.emit( - 'user.email-confirmed', - { - user_id: user.id, - user_uid: user.uuid, - email: user.email, - }, - {}, - ); - } catch { - /* ignore */ - } - res.send(ok('Your email has been successfully confirmed.')); }, ); diff --git a/src/backend/services/user/SignupService.test.ts b/src/backend/services/user/SignupService.test.ts index cee88c939..0250aee02 100644 --- a/src/backend/services/user/SignupService.test.ts +++ b/src/backend/services/user/SignupService.test.ts @@ -18,7 +18,7 @@ */ import type { Request } from 'express'; -import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; import { PuterServer } from '../../server.js'; import { setupTestServer } from '../../testUtil.js'; @@ -201,3 +201,75 @@ describe('SignupService.announceSignup', () => { ]); }); }); + +describe('SignupService.confirmEmail', () => { + const makeUnconfirmed = async () => { + const username = `ce_${slug()}`; + const created = await server.stores.user.create({ + username, + uuid: crypto.randomUUID(), + password: null, + email: `${username}@example.com`, + requires_email_confirmation: true, + }); + return (await server.stores.user.getById(created.id, { + force: true, + }))!; + }; + + it("tells the account's other tabs, naming the one that confirmed", async () => { + const user = await makeUnconfirmed(); + const send = vi.spyOn(server.services.socket, 'send'); + try { + await server.services.signup.confirmEmail(user, { + originalClientSocketId: 'sock-1', + }); + expect(send).toHaveBeenCalledWith( + { room: user.id }, + 'user.email_confirmed', + { original_client_socket_id: 'sock-1' }, + ); + } finally { + send.mockRestore(); + } + const after = await server.stores.user.getById(user.id, { + force: true, + }); + expect(after!.email_confirmed).toBeTruthy(); + expect(after!.email_confirm_code).toBeNull(); + }); + + it('leaves another account no way to take the address through a pending change', async () => { + const owner = await makeUnconfirmed(); + const other = await makeUnconfirmed(); + await server.stores.user.update(other.id, { + unconfirmed_change_email: owner.email, + change_email_confirm_token: 'pending-tok', + }); + + await server.services.signup.confirmEmail(owner); + + const token = server.services.token.sign( + 'otp', + { + token: 'pending-tok', + user_id: other.id, + purpose: 'change-email', + }, + { expiresIn: '1h' }, + ); + await expect( + server.controllers.auth.handleChangeEmailConfirm( + { query: { token } } as unknown as Request, + { send: vi.fn() } as never, + ), + ).rejects.toMatchObject({ + statusCode: 400, + legacyCode: 'email_already_in_use', + }); + const after = await server.stores.user.getById(other.id, { + force: true, + }); + expect(after!.email).toBe(other.email); + }); +}); diff --git a/src/backend/services/user/SignupService.ts b/src/backend/services/user/SignupService.ts index b0c154748..7f1c3af0c 100644 --- a/src/backend/services/user/SignupService.ts +++ b/src/backend/services/user/SignupService.ts @@ -86,9 +86,9 @@ export const signupClientIp = (req: Request): string | null => req.ip || req.socket?.remoteAddress || null; /** - * Account creation for every signup route (form, temp, OIDC). Callers parse - * requests and set cookies; the steps and the events listeners rely on live - * here. + * Account creation for every signup route (form, temp, OIDC) and email + * confirmation for every confirmation route. Callers parse requests and set + * cookies; the steps and the events listeners rely on live here. */ export class SignupService extends PuterService { /** @@ -614,6 +614,66 @@ export class SignupService extends PuterService { } } + /** + * Confirm the account's current address, for the code and the link routes + * alike. Re-runs the email checks (the address may have been blocked since + * signup), refuses an address another account already confirmed, and takes + * it off every other row. Another account's pending change to this address + * needs no cleanup: `/change_email/confirm` refuses an address a confirmed + * account owns. + */ + async confirmEmail( + user: UserRow, + { originalClientSocketId }: { originalClientSocketId?: unknown } = {}, + ): Promise { + const email = user.email as string; + await this.validateEmail(email); + + const canonical = cleanEmail(email); + const confirmedRival = await this.stores.user.findConfirmedOtherByEmail( + user.id, + email, + canonical, + ); + if (confirmedRival) { + throw new HttpError( + 400, + 'This email was confirmed on a different account.', + { legacyCode: 'email_already_in_use' as never }, + ); + } + + // Only one row may own an address once this one is confirmed, so the + // others lose it first. + await this.stores.user.unconfirmOthersByEmail( + user.id, + email, + canonical, + ); + await this.stores.user.update(user.id, { + email_confirmed: 1, + requires_email_confirmation: 0, + email_confirm_code: null, + email_confirm_token: null, + }); + await promoteToVerifiedGroup(this.stores.group, this.config, user); + + // Best-effort side channels. Other tabs refresh; the one that sent + // the code (named by its socket id) already knows. + try { + await this.services.socket.send( + { room: user.id }, + 'user.email_confirmed', + originalClientSocketId === undefined + ? {} + : { original_client_socket_id: originalClientSocketId }, + ); + } catch { + // ignore + } + this.#emitEmailConfirmed(user); + } + /** * The account holding `email` that a signup has to contend with. A seat * whose address its team typed and nobody confirmed doesn't count; with