From eb53c842dd4acb7799ad8323f03f4e696d2c1a7a Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Wed, 5 Aug 2026 17:05:31 -0700 Subject: [PATCH] fix: oidc issues with cache (#3512) closes #3497 closes #3502 --- .../controllers/auth/AuthController.test.ts | 58 ++++++++++++ .../controllers/auth/AuthController.ts | 10 ++- .../controllers/oidc/OIDCController.test.ts | 33 +++++++ .../controllers/oidc/OIDCController.ts | 22 ++++- src/backend/stores/user/UserStore.test.ts | 89 +++++++++++++++++++ src/backend/stores/user/UserStore.ts | 37 ++++++++ 6 files changed, 243 insertions(+), 6 deletions(-) diff --git a/src/backend/controllers/auth/AuthController.test.ts b/src/backend/controllers/auth/AuthController.test.ts index 79a133e6e..1637f868a 100644 --- a/src/backend/controllers/auth/AuthController.test.ts +++ b/src/backend/controllers/auth/AuthController.test.ts @@ -955,6 +955,40 @@ describe('AuthController.handleLogin', () => { expect(isCompleteLoginResponse(res.body)).toBe(true); }); + it('refuses an email the account has moved off', async () => { + const moverName = `lm_${Math.random().toString(36).slice(2, 10)}`; + const oldEmail = `${moverName}-old@test.local`; + const newEmail = `${moverName}-new@test.local`; + await controller.handleSignup( + makeReq({ username: moverName, email: oldEmail, password }), + makeRes(), + ); + // Warm the by-email lookup the way a real login would. + await controller.handleLogin( + makeReq({ email: oldEmail, password }), + makeRes(), + ); + + const mover = await server.stores.user.getByUsername(moverName); + await server.stores.user.update(mover!.id, { + email: newEmail, + clean_email: newEmail, + }); + + await expect( + controller.handleLogin( + makeReq({ email: oldEmail, password }), + makeRes(), + ), + ).rejects.toMatchObject({ statusCode: 404 }); + const res = makeRes(); + await controller.handleLogin( + makeReq({ email: newEmail, password }), + res, + ); + expect(isCompleteLoginResponse(res.body)).toBe(true); + }); + it('returns 400 when neither username nor email is supplied', async () => { await expect( controller.handleLogin(makeReq({ password }), makeRes()), @@ -4130,6 +4164,30 @@ describe('AuthController user-protected mutations (validation paths)', () => { ).rejects.toMatchObject({ statusCode: 400 }); }); + it('change-email: accepts an address that resolves back to the caller', async () => { + // `foo+tag@gmail.com` canonicalizes to the caller's own row, so the + // collision check has to exclude them — otherwise Puter reports your + // own address as already in use and there's no way to set it. + const local = `ch_${uniq()}`; + const { user, actor } = await makeUserAndActor(); + await server.stores.user.update(user.id, { + email: `${local}@gmail.com`, + clean_email: `${local}@gmail.com`, + email_confirmed: 1, + }); + + const res = makeRes(); + await controller.handleChangeEmail( + makeReq({ new_email: `${local}+work@gmail.com` }, { actor }), + res, + ); + expect(res.body).toEqual({}); + const after = await server.stores.user.getById(user.id, { + force: true, + }); + expect(after!.unconfirmed_change_email).toBe(`${local}+work@gmail.com`); + }); + it('change-email: stages the new email + token on success', async () => { const { user, actor } = await makeUserAndActor(); const newEmail = `ch_${uniq()}@test.local`; diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index 616ffb75e..7630b11f9 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -1004,8 +1004,7 @@ export class AuthController extends PuterController { is_temp: user!.password === null && user!.email === null, ip: (req?.headers?.['x-forwarded-for'] as - | string - | undefined) || + string | undefined) || ( req as unknown as { connection?: { remoteAddress?: string }; @@ -2339,15 +2338,18 @@ export class AuthController extends PuterController { } await this.#validateEmail(new_email); - // Block if any confirmed account (password or OIDC) already + // Block if any OTHER confirmed account (password or OIDC) already // owns that email. Match raw + canonical to collapse gmail - // aliases. + // aliases — which is also why the caller has to be excluded: an + // alias of your own current address resolves back to you, and + // "already in use" about yourself is nonsense. const canonical = cleanEmail(new_email); const existing = (await this.stores.user.getByEmail(new_email)) ?? (await this.stores.user.getByCleanEmail(canonical)); if ( existing && + existing.id !== req.actor!.user.id && (existing.email_confirmed || existing.password !== null) ) { throw new HttpError(400, 'This email is already in use.', { diff --git a/src/backend/controllers/oidc/OIDCController.test.ts b/src/backend/controllers/oidc/OIDCController.test.ts index 8a6295d27..f5c768c35 100644 --- a/src/backend/controllers/oidc/OIDCController.test.ts +++ b/src/backend/controllers/oidc/OIDCController.test.ts @@ -1045,6 +1045,39 @@ describe('OIDCController signup veto (abuse harness)', () => { expect(captured.redirectUrl).toContain('message=signup_blocked'); expect(captured.redirectUrl).not.toContain('request_code'); }); + + it('keeps a veto legible when the listener stamps its own code', async () => { + const email = `veto-${Math.random().toString(36).slice(2, 8)}@test.local`; + signupValidateOverride = (data) => { + if (data.email !== email) return; + data.allow = false; + data.code = 'email_reputation_too_low'; + data.trail_id = 'trail-custom-code'; + }; + stubIdp(`sub-${Math.random().toString(36).slice(2, 8)}`, email); + + const state = oidc().signState({ + provider: 'custom', + redirect_uri: TEST_ORIGIN + '/', + }); + const { res, captured } = makeRes(); + await callRoute( + 'get', + '/auth/oidc/callback/signup', + makeReq({ query: { code: 'c', state } }), + res, + ); + + // A listener code isn't one of the display codes, but the failure + // is still a blocked signup — collapsing it to `unauthorized` + // would tell the user sign-in broke and bury the Request Code + // support needs. + expect(captured.redirectUrl).toContain('message=signup_blocked'); + expect(captured.redirectUrl).not.toContain('message=unauthorized'); + expect(captured.redirectUrl).toContain( + 'request_code=trail-custom-code', + ); + }); }); describe('OIDCController browser binding', () => { diff --git a/src/backend/controllers/oidc/OIDCController.ts b/src/backend/controllers/oidc/OIDCController.ts index 7c56f5777..ae194265c 100644 --- a/src/backend/controllers/oidc/OIDCController.ts +++ b/src/backend/controllers/oidc/OIDCController.ts @@ -47,6 +47,23 @@ const ALLOWED_ERRORS = [ 'signup_blocked', ] as const; +/** + * Pick the display code for a failed user resolution. + * + * A `code` is only ever set when the signup-validate harness vetoed the signup, + * but the code it stamps comes from an abuse listener and is not drawn from + * {@link ALLOWED_ERRORS} — so clamping it directly turns every vetoed OIDC + * signup into a bare `unauthorized`, which reads as "sign-in broke" and hides + * both the real cause and the Request Code that support looks the decision up + * by. Anything unrecognized falls back to the veto's own category instead. + */ +function resolutionErrorCode(code: string | undefined): string { + if (!code) return 'unauthorized'; + return (ALLOWED_ERRORS as readonly string[]).includes(code) + ? code + : 'signup_blocked'; +} + // GUI pages an OIDC flow may return to: /desktop, /dashboard, and direct app // landings (/app/ and its desktop-booted twin /desktop/app/, // mirroring APP_NAME_REGEX in AppDriver). Strict whitelist — never a @@ -404,7 +421,7 @@ export class OIDCController extends PuterController { origin, 'login', 'other', - resolved.code ?? 'unauthorized', + resolutionErrorCode(resolved.code), stateDecoded, resolved.requestCode, (p) => this.services.oidc.signPopupReturn(p), @@ -442,6 +459,7 @@ export class OIDCController extends PuterController { const origin = this.config.origin ?? ''; const result = await this.#processCallback(req, res, 'signup'); if ('error' in result) { + console.warn(`OIDC signup callback error: ${result.error}`); return res.redirect( 302, buildErrorRedirectUrl( @@ -470,7 +488,7 @@ export class OIDCController extends PuterController { origin, 'signup', 'other', - resolved.code ?? 'unauthorized', + resolutionErrorCode(resolved.code), stateDecoded, resolved.requestCode, (p) => this.services.oidc.signPopupReturn(p), diff --git a/src/backend/stores/user/UserStore.test.ts b/src/backend/stores/user/UserStore.test.ts index f72551403..4e5e4436f 100644 --- a/src/backend/stores/user/UserStore.test.ts +++ b/src/backend/stores/user/UserStore.test.ts @@ -55,6 +55,95 @@ describe('UserStore', () => { expect(typeof cachedUser?.requires_email_confirmation).toBe('boolean'); }); + it('stops resolving an email the account no longer holds', async () => { + const username = `us-${Math.random().toString(36).slice(2, 10)}`; + const oldEmail = `${username}-old@test.local`; + const newEmail = `${username}-new@test.local`; + const user = await server.stores.user.create({ + username, + uuid: uuidv4(), + password: null, + email: oldEmail, + }); + + // Warm the by-email key, then move the account to a new address. + expect((await server.stores.user.getByEmail(oldEmail))?.id).toBe( + user.id, + ); + await server.stores.user.update(user.id, { + email: newEmail, + clean_email: newEmail, + }); + + // The old address is no longer this account's — a cached copy of the + // pre-update row answering for it is what let a replaced email keep + // working as a login. + expect(await server.stores.user.getByEmail(oldEmail)).toBeNull(); + expect((await server.stores.user.getByEmail(newEmail))?.id).toBe( + user.id, + ); + }); + + it('stops resolving a username the account no longer holds', async () => { + const oldUsername = `us-${Math.random().toString(36).slice(2, 10)}`; + const newUsername = `${oldUsername}-renamed`; + const user = await server.stores.user.create({ + username: oldUsername, + uuid: uuidv4(), + password: null, + email: `${oldUsername}@test.local`, + }); + + expect((await server.stores.user.getByUsername(oldUsername))?.id).toBe( + user.id, + ); + await server.stores.user.update(user.id, { username: newUsername }); + + expect(await server.stores.user.getByUsername(oldUsername)).toBeNull(); + expect((await server.stores.user.getByUsername(newUsername))?.id).toBe( + user.id, + ); + }); + + it('stops resolving an email revoked from a competing account', async () => { + const shared = `shared-${Math.random().toString(36).slice(2, 10)}@test.local`; + const makeClaimant = async () => { + const username = `uc-${Math.random().toString(36).slice(2, 10)}`; + const claimant = await server.stores.user.create({ + username, + uuid: uuidv4(), + password: null, + email: shared, + clean_email: shared, + }); + await server.stores.user.update(claimant.id, { + email_confirmed: true, + }); + return claimant; + }; + + const loser = await makeClaimant(); + const winner = await makeClaimant(); + + // Warm the loser's cache entries before its claim is revoked. + expect((await server.stores.user.getById(loser.id))?.email).toBe( + shared, + ); + + await server.stores.user.unconfirmOthersByEmail( + winner.id, + shared, + shared, + ); + + const strippedLoser = await server.stores.user.getById(loser.id); + expect(strippedLoser?.email).toBeNull(); + expect(strippedLoser?.email_confirmed).toBe(false); + expect((await server.stores.user.getByEmail(shared))?.id).toBe( + winner.id, + ); + }); + it('counts other accounts holding the same phone number', async () => { const phone = `+1415555${Math.floor(1000 + Math.random() * 9000)}`; const makeUser = async () => { diff --git a/src/backend/stores/user/UserStore.ts b/src/backend/stores/user/UserStore.ts index 019c6591e..6529ca571 100644 --- a/src/backend/stores/user/UserStore.ts +++ b/src/backend/stores/user/UserStore.ts @@ -465,12 +465,36 @@ export class UserStore extends PuterStore { const setClause = keys.map((k) => `\`${k}\` = ?`).join(', '); const values = keys.map((k) => dbPatch[k]); + // Identifying columns are themselves cache keys, so changing one + // leaves the old key holding a full copy of the pre-update row. + // Snapshot the row first so the keys this write retires can be + // dropped: otherwise a replaced email keeps resolving to the account + // for the rest of the TTL, and login and password recovery accept it + // as if it were still the account's address. + const touchesIdentity = keys.some((k) => + (USER_ID_PROPERTIES as readonly string[]).includes(k), + ); + const before = touchesIdentity + ? await this.getByProperty('id', userId, { force: true }) + : null; + await this.clients.db.write( `UPDATE \`user\` SET ${setClause} WHERE \`id\` = ?`, [...values, userId], ); const fresh = await this.getByProperty('id', userId, { force: true }); + + if (before) { + const live = new Set(fresh ? this.#cacheKeysForUser(fresh) : []); + const retired = this.#cacheKeysForUser(before).filter( + (key) => !live.has(key), + ); + if (retired.length > 0) { + await this.publishCacheKeys({ keys: retired, broadcast: true }); + } + } + if (fresh) { await this.#refreshCache(fresh); } else { @@ -501,6 +525,15 @@ export class UserStore extends PuterStore { email: string, cleanEmailValue: string, ): Promise { + // Read the rows this strips before stripping them: the update goes + // straight to SQL, so without their pre-image we don't know which + // cache keys it retires — and a cached copy still carries the address + // that was just revoked, which would keep answering lookups for it. + const stripped = (await this.clients.db.pread( + 'SELECT * FROM `user` WHERE `id` != ? AND (`email` = ? OR `clean_email` = ?)', + [userId, email, cleanEmailValue], + )) as Array>; + await this.clients.db.write( `UPDATE \`user\` SET \`email\` = NULL, @@ -519,6 +552,10 @@ export class UserStore extends PuterStore { cleanEmailValue, ], ); + + for (const row of stripped) { + await this.invalidate(this.#normalizeRow(row)); + } } async invalidate(user: UserRow): Promise {