From c6e3afd938904dbfcfe38bccc3208c38ee0a7d9b Mon Sep 17 00:00:00 2001 From: Shuki Vaknin Date: Fri, 26 Jun 2026 23:47:53 +0300 Subject: [PATCH 1/2] =?UTF-8?q?fix(auth):=20guard=20account=20switch=20aga?= =?UTF-8?q?inst=20slot=E2=86=92token=20desync?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When switching accounts, the target client connects with the token at the account's stored cookieSlot. If that slot→token mapping is ever wrong — e.g. corrupted client state persisted by an older build, or any future slot desync — the connection succeeds as a *different* account and the UI silently shows the wrong mailbox. Add a post-connect identity guard: derive the connected session's accountId (primary-identity email for OAuth, else the JMAP session username) and compare it to the account being switched to. On mismatch, drop the poisoned slot cookies and force a clean re-auth instead of binding the wrong session. This is belt-and-suspenders on top of 8b164c5, which fixed the slot allocation that caused such a desync: that prevents new corruption, this catches any residual/leftover mapping at switch time. Adds a unit test for the canonicalisation (email vs JMAP username), the make-or-break detail that avoids OAuth false-positives. --- .../__tests__/auth-store-switch-guard.test.ts | 52 +++++++++++++++++++ stores/auth-store.ts | 43 +++++++++++++++ 2 files changed, 95 insertions(+) create mode 100644 stores/__tests__/auth-store-switch-guard.test.ts diff --git a/stores/__tests__/auth-store-switch-guard.test.ts b/stores/__tests__/auth-store-switch-guard.test.ts new file mode 100644 index 00000000..2bef56f0 --- /dev/null +++ b/stores/__tests__/auth-store-switch-guard.test.ts @@ -0,0 +1,52 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { connectedAccountId } from '../auth-store'; +import { useIdentityStore } from '../identity-store'; +import type { Identity } from '@/lib/jmap/types'; + +// Covers the make-or-break bit of the account-switch identity guard: the +// connected session's accountId must be derived with the SAME canonicalisation +// as login (primary-identity email for OAuth, where the JMAP session username +// is often a preferred_username claim, not the address). Comparing the raw +// JMAP username would both false-positive on OAuth and miss real desyncs. + +const SERVER = 'https://mail.example.com'; + +const fakeClient = (jmapUsername: string, identities: Identity[] | Error) => + ({ + getUsername: () => jmapUsername, + getIdentities: async () => { + if (identities instanceof Error) throw identities; + return identities; + }, + }) as never; + +const id = (over: Partial = {}): Identity => ({ + id: 'id-1', + name: 'Real User', + email: 'real@example.com', + mayDelete: true, + ...over, +}); + +describe('connectedAccountId (account-switch guard)', () => { + beforeEach(() => { + useIdentityStore.setState({ identities: [], preferredPrimaryId: null } as never); + }); + + it('derives from the primary-identity EMAIL, not the JMAP session username', async () => { + // OAuth: JMAP username is a preferred_username claim, the real address lives + // on the identity. The id must be built from the email. + const result = await connectedAccountId(fakeClient('preferred_user', [id()]), SERVER); + expect(result).toBe('real@example.com@mail.example.com'); + }); + + it('falls back to the JMAP username when identities cannot be fetched', async () => { + const result = await connectedAccountId(fakeClient('basic@example.com', new Error('no idents')), SERVER); + expect(result).toBe('basic@example.com@mail.example.com'); + }); + + it('returns null when the session identity cannot be determined at all', async () => { + const broken = { getUsername: () => { throw new Error('disconnected'); } } as never; + expect(await connectedAccountId(broken, SERVER)).toBeNull(); + }); +}); diff --git a/stores/auth-store.ts b/stores/auth-store.ts index 4aa67f60..d198ac12 100644 --- a/stores/auth-store.ts +++ b/stores/auth-store.ts @@ -296,6 +296,29 @@ function scheduleRefresh(expiresIn: number, refreshFn: () => Promisetoken mapping that + * resolves to the wrong account before it surfaces as the wrong mailbox. + * Returns null when it can't determine the identity (treated as "don't block"). + */ +export async function connectedAccountId(client: JMAPClient, serverUrl: string): Promise { + try { + const jmapUsername = client.getUsername(); + let canonical = jmapUsername; + try { + const { primaryIdentity } = loadIdentities(await client.getIdentities(), jmapUsername); + canonical = primaryIdentity?.email || jmapUsername; + } catch { + /* identities unavailable — fall back to the JMAP session username */ + } + return generateAccountId(canonical, serverUrl); + } catch { + return null; + } +} + function clearRefreshTimer(accountId?: string): void { if (accountId) { const timer = refreshTimers.get(accountId); @@ -1207,6 +1230,26 @@ export const useAuthStore = create()( return; } + // GUARD: verify the connected session actually belongs to the target + // account before we bind it. A corrupted slot->token mapping (e.g. + // persisted client state left over from an older build, or any future + // slot desync) can hand back a *different* account's token; the + // connection then succeeds and we would silently show the wrong + // mailbox. On mismatch, drop the poisoned cookies for this slot and + // force a clean re-auth instead of surfacing someone else's mail. + const connectedId = await connectedAccountId(targetClient, targetAccount.serverUrl); + if (connectedId && connectedId !== accountId) { + debug.error(`switchAccount: slot ${targetAccount.cookieSlot} for ${accountId} resolved to ${connectedId} — forcing re-auth`); + clients.delete(accountId); + try { targetClient.disconnect(); } catch { /* noop */ } + apiFetch(`/api/auth/token?slot=${targetAccount.cookieSlot}`, { method: 'DELETE' }).catch(() => {}); + apiFetch(`/api/auth/session?slot=${targetAccount.cookieSlot}`, { method: 'DELETE' }).catch(() => {}); + accountStore.updateAccount(accountId, { isConnected: false, hasError: true, errorMessage: 'session_mismatch' }); + set({ isLoading: false, error: 'connection_failed', activeAccountId: state.activeAccountId }); + replaceWindowLocation(getLocaleLoginPath()); + return; + } + // Restore cached state or fetch fresh const restored = restoreAccount(accountId); accountStore.setActiveAccount(accountId); From 732e7f0f7869b8ee122f40443db503e05175b042 Mon Sep 17 00:00:00 2001 From: Shuki Vaknin Date: Tue, 30 Jun 2026 18:05:40 +0300 Subject: [PATCH 2/2] fix: guard false-positive on basic-auth accounts (identity != login) Accounts whose primary sending identity differs from their login (basic auth registers accountId from the typed login; OAuth from the identity email) were force-re-authed on switch because the guard derived the connected id only from the primary-identity email. Collect every server-confirmed identifier (JMAP Session.username + primary-identity email) and only re-auth when the target matches none. Excludes the constructor username so a real desync still trips. Adds JMAPClient.getSessionUsername(). --- lib/jmap/client.ts | 11 +++ .../__tests__/auth-store-switch-guard.test.ts | 78 +++++++++++-------- stores/auth-store.ts | 39 ++++++---- 3 files changed, 81 insertions(+), 47 deletions(-) diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 0f3ff10e..489788d0 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -26,6 +26,9 @@ export class RateLimitError extends Error { // JMAP protocol types - these are intentionally flexible due to server variations interface JMAPSession { + // The authenticated login (JMAP spec Session.username) — server-confirmed, + // unlike the client-side constructor username or the sending identity. + username?: string; apiUrl: string; downloadUrl: string; uploadUrl?: string; @@ -3412,6 +3415,14 @@ export class JMAPClient implements IJMAPClient { return this.username || this.session?.accounts?.[this.accountId]?.name || ''; } + // Server-confirmed authenticated login from the JMAP Session object. Use + // this (not getUsername(), which echoes the constructor arg, nor the + // sending identity) to verify a slot's token resolved to the expected + // account. + getSessionUsername(): string | undefined { + return this.session?.username; + } + supportsEmailSubmission(): boolean { return this.hasCapability("urn:ietf:params:jmap:submission"); } diff --git a/stores/__tests__/auth-store-switch-guard.test.ts b/stores/__tests__/auth-store-switch-guard.test.ts index 2bef56f0..edf8b0da 100644 --- a/stores/__tests__/auth-store-switch-guard.test.ts +++ b/stores/__tests__/auth-store-switch-guard.test.ts @@ -1,52 +1,68 @@ -import { describe, it, expect, beforeEach } from 'vitest'; -import { connectedAccountId } from '../auth-store'; -import { useIdentityStore } from '../identity-store'; +import { describe, it, expect } from 'vitest'; +import { connectedAccountCandidates } from '../auth-store'; import type { Identity } from '@/lib/jmap/types'; -// Covers the make-or-break bit of the account-switch identity guard: the -// connected session's accountId must be derived with the SAME canonicalisation -// as login (primary-identity email for OAuth, where the JMAP session username -// is often a preferred_username claim, not the address). Comparing the raw -// JMAP username would both false-positive on OAuth and miss real desyncs. +// The account-switch guard force-re-auths only when the connected session +// matches NONE of its server-confirmed identifiers. accountId is generated +// from the primary-identity email (OAuth) OR the login username (basic), so +// the candidate set must cover both: the JMAP Session.username (authenticated +// login) and the primary sending-identity email. const SERVER = 'https://mail.example.com'; -const fakeClient = (jmapUsername: string, identities: Identity[] | Error) => +const fakeClient = (opts: { + sessionUsername?: string; + constructorUsername?: string; + identities?: Identity[] | Error; +}) => ({ - getUsername: () => jmapUsername, + getSessionUsername: () => opts.sessionUsername, + getUsername: () => opts.constructorUsername ?? '', getIdentities: async () => { - if (identities instanceof Error) throw identities; - return identities; + if (opts.identities instanceof Error) throw opts.identities; + return opts.identities ?? []; }, }) as never; const id = (over: Partial = {}): Identity => ({ - id: 'id-1', - name: 'Real User', - email: 'real@example.com', - mayDelete: true, - ...over, + id: 'id-1', name: 'Real User', email: 'real@example.com', mayDelete: true, ...over, }); -describe('connectedAccountId (account-switch guard)', () => { - beforeEach(() => { - useIdentityStore.setState({ identities: [], preferredPrimaryId: null } as never); +describe('connectedAccountCandidates (account-switch guard)', () => { + it('includes the primary-identity email (OAuth registers by email)', async () => { + const out = await connectedAccountCandidates( + fakeClient({ sessionUsername: 'preferred_user', identities: [id()] }), SERVER, + ); + expect(out).toContain('real@example.com@mail.example.com'); }); - it('derives from the primary-identity EMAIL, not the JMAP session username', async () => { - // OAuth: JMAP username is a preferred_username claim, the real address lives - // on the identity. The id must be built from the email. - const result = await connectedAccountId(fakeClient('preferred_user', [id()]), SERVER); - expect(result).toBe('real@example.com@mail.example.com'); + it('includes the session login username (basic auth registers by login)', async () => { + // support@ case: login is the email, but the primary sending identity is a + // different address. The login must still be accepted. + const out = await connectedAccountCandidates( + fakeClient({ sessionUsername: 'support@linux-hosting.co.il', identities: [id({ email: 'alias@elsewhere.com' })] }), + SERVER, + ); + expect(out).toContain('support@linux-hosting.co.il@mail.example.com'); + expect(out).toContain('alias@elsewhere.com@mail.example.com'); }); - it('falls back to the JMAP username when identities cannot be fetched', async () => { - const result = await connectedAccountId(fakeClient('basic@example.com', new Error('no idents')), SERVER); - expect(result).toBe('basic@example.com@mail.example.com'); + it('excludes the constructor username (cannot mask a desync)', async () => { + // A desynced slot: client built for support@ but the token resolves to + // shuki@. Candidates come only from the server (session + identities), + // never the constructor echo, so support@ is NOT among them. + const out = await connectedAccountCandidates( + fakeClient({ sessionUsername: 'shuki@linux-hosting.co.il', constructorUsername: 'support@linux-hosting.co.il', identities: [id({ email: 'shuki@linux-hosting.co.il' })] }), + SERVER, + ); + expect(out).not.toContain('support@linux-hosting.co.il@mail.example.com'); + expect(out).toContain('shuki@linux-hosting.co.il@mail.example.com'); }); - it('returns null when the session identity cannot be determined at all', async () => { - const broken = { getUsername: () => { throw new Error('disconnected'); } } as never; - expect(await connectedAccountId(broken, SERVER)).toBeNull(); + it('returns empty when nothing can be confirmed (caller must not bounce)', async () => { + const out = await connectedAccountCandidates( + fakeClient({ sessionUsername: undefined, identities: new Error('no idents') }), SERVER, + ); + expect(out).toEqual([]); }); }); diff --git a/stores/auth-store.ts b/stores/auth-store.ts index d198ac12..6e1ee7b4 100644 --- a/stores/auth-store.ts +++ b/stores/auth-store.ts @@ -303,20 +303,27 @@ function scheduleRefresh(expiresIn: number, refreshFn: () => Promise { +export async function connectedAccountCandidates(client: JMAPClient, serverUrl: string): Promise { + // accountId is generated differently per auth mode: OAuth/SSO registers from + // the primary-identity EMAIL, basic auth from the typed login username. A + // single derivation can't match both, so collect every server-confirmed + // identifier the connected session exposes — the JMAP Session.username + // (authenticated login) and the primary sending-identity email — and let the + // caller accept the session if the target accountId matches ANY of them. + // Deliberately excludes client.getUsername(), which echoes the constructor + // username (always the target) and would defeat the desync check. An empty + // result means nothing could be confirmed → the caller should NOT force a + // re-auth. + const ids = new Set(); try { - const jmapUsername = client.getUsername(); - let canonical = jmapUsername; - try { - const { primaryIdentity } = loadIdentities(await client.getIdentities(), jmapUsername); - canonical = primaryIdentity?.email || jmapUsername; - } catch { - /* identities unavailable — fall back to the JMAP session username */ - } - return generateAccountId(canonical, serverUrl); - } catch { - return null; - } + const sessionUser = client.getSessionUsername(); + if (sessionUser) ids.add(generateAccountId(sessionUser, serverUrl)); + } catch { /* session unavailable */ } + try { + const { primaryIdentity } = loadIdentities(await client.getIdentities(), client.getUsername()); + if (primaryIdentity?.email) ids.add(generateAccountId(primaryIdentity.email, serverUrl)); + } catch { /* identities unavailable */ } + return [...ids]; } function clearRefreshTimer(accountId?: string): void { @@ -1237,9 +1244,9 @@ export const useAuthStore = create()( // connection then succeeds and we would silently show the wrong // mailbox. On mismatch, drop the poisoned cookies for this slot and // force a clean re-auth instead of surfacing someone else's mail. - const connectedId = await connectedAccountId(targetClient, targetAccount.serverUrl); - if (connectedId && connectedId !== accountId) { - debug.error(`switchAccount: slot ${targetAccount.cookieSlot} for ${accountId} resolved to ${connectedId} — forcing re-auth`); + const connectedCandidates = await connectedAccountCandidates(targetClient, targetAccount.serverUrl); + if (connectedCandidates.length > 0 && !connectedCandidates.includes(accountId)) { + debug.error(`switchAccount: slot ${targetAccount.cookieSlot} for ${accountId} resolved to [${connectedCandidates.join(", ")}] — forcing re-auth`); clients.delete(accountId); try { targetClient.disconnect(); } catch { /* noop */ } apiFetch(`/api/auth/token?slot=${targetAccount.cookieSlot}`, { method: 'DELETE' }).catch(() => {});