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().
This commit is contained in:
@@ -61,6 +61,9 @@ export class RateLimitError extends Error {
|
|||||||
|
|
||||||
// JMAP protocol types - these are intentionally flexible due to server variations
|
// JMAP protocol types - these are intentionally flexible due to server variations
|
||||||
interface JMAPSession {
|
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;
|
apiUrl: string;
|
||||||
downloadUrl: string;
|
downloadUrl: string;
|
||||||
uploadUrl?: string;
|
uploadUrl?: string;
|
||||||
@@ -3601,6 +3604,14 @@ export class JMAPClient implements IJMAPClient {
|
|||||||
return this.username || this.session?.accounts?.[this.accountId]?.name || '';
|
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 {
|
supportsEmailSubmission(): boolean {
|
||||||
return this.hasCapability("urn:ietf:params:jmap:submission");
|
return this.hasCapability("urn:ietf:params:jmap:submission");
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,52 +1,68 @@
|
|||||||
import { describe, it, expect, beforeEach } from 'vitest';
|
import { describe, it, expect } from 'vitest';
|
||||||
import { connectedAccountId } from '../auth-store';
|
import { connectedAccountCandidates } from '../auth-store';
|
||||||
import { useIdentityStore } from '../identity-store';
|
|
||||||
import type { Identity } from '@/lib/jmap/types';
|
import type { Identity } from '@/lib/jmap/types';
|
||||||
|
|
||||||
// Covers the make-or-break bit of the account-switch identity guard: the
|
// The account-switch guard force-re-auths only when the connected session
|
||||||
// connected session's accountId must be derived with the SAME canonicalisation
|
// matches NONE of its server-confirmed identifiers. accountId is generated
|
||||||
// as login (primary-identity email for OAuth, where the JMAP session username
|
// from the primary-identity email (OAuth) OR the login username (basic), so
|
||||||
// is often a preferred_username claim, not the address). Comparing the raw
|
// the candidate set must cover both: the JMAP Session.username (authenticated
|
||||||
// JMAP username would both false-positive on OAuth and miss real desyncs.
|
// login) and the primary sending-identity email.
|
||||||
|
|
||||||
const SERVER = 'https://mail.example.com';
|
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 () => {
|
getIdentities: async () => {
|
||||||
if (identities instanceof Error) throw identities;
|
if (opts.identities instanceof Error) throw opts.identities;
|
||||||
return identities;
|
return opts.identities ?? [];
|
||||||
},
|
},
|
||||||
}) as never;
|
}) as never;
|
||||||
|
|
||||||
const id = (over: Partial<Identity> = {}): Identity => ({
|
const id = (over: Partial<Identity> = {}): Identity => ({
|
||||||
id: 'id-1',
|
id: 'id-1', name: 'Real User', email: 'real@example.com', mayDelete: true, ...over,
|
||||||
name: 'Real User',
|
|
||||||
email: 'real@example.com',
|
|
||||||
mayDelete: true,
|
|
||||||
...over,
|
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('connectedAccountId (account-switch guard)', () => {
|
describe('connectedAccountCandidates (account-switch guard)', () => {
|
||||||
beforeEach(() => {
|
it('includes the primary-identity email (OAuth registers by email)', async () => {
|
||||||
useIdentityStore.setState({ identities: [], preferredPrimaryId: null } as never);
|
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 () => {
|
it('includes the session login username (basic auth registers by login)', async () => {
|
||||||
// OAuth: JMAP username is a preferred_username claim, the real address lives
|
// support@ case: login is the email, but the primary sending identity is a
|
||||||
// on the identity. The id must be built from the email.
|
// different address. The login must still be accepted.
|
||||||
const result = await connectedAccountId(fakeClient('preferred_user', [id()]), SERVER);
|
const out = await connectedAccountCandidates(
|
||||||
expect(result).toBe('real@example.com@mail.example.com');
|
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 () => {
|
it('excludes the constructor username (cannot mask a desync)', async () => {
|
||||||
const result = await connectedAccountId(fakeClient('basic@example.com', new Error('no idents')), SERVER);
|
// A desynced slot: client built for support@ but the token resolves to
|
||||||
expect(result).toBe('basic@example.com@mail.example.com');
|
// 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 () => {
|
it('returns empty when nothing can be confirmed (caller must not bounce)', async () => {
|
||||||
const broken = { getUsername: () => { throw new Error('disconnected'); } } as never;
|
const out = await connectedAccountCandidates(
|
||||||
expect(await connectedAccountId(broken, SERVER)).toBeNull();
|
fakeClient({ sessionUsername: undefined, identities: new Error('no idents') }), SERVER,
|
||||||
|
);
|
||||||
|
expect(out).toEqual([]);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
+22
-15
@@ -422,20 +422,27 @@ function scheduleRefresh(expiresIn: number, refreshFn: () => Promise<string | nu
|
|||||||
* resolves to the wrong account before it surfaces as the wrong mailbox.
|
* 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").
|
* Returns null when it can't determine the identity (treated as "don't block").
|
||||||
*/
|
*/
|
||||||
export async function connectedAccountId(client: JMAPClient, serverUrl: string): Promise<string | null> {
|
export async function connectedAccountCandidates(client: JMAPClient, serverUrl: string): Promise<string[]> {
|
||||||
|
// 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<string>();
|
||||||
try {
|
try {
|
||||||
const jmapUsername = client.getUsername();
|
const sessionUser = client.getSessionUsername();
|
||||||
let canonical = jmapUsername;
|
if (sessionUser) ids.add(generateAccountId(sessionUser, serverUrl));
|
||||||
|
} catch { /* session unavailable */ }
|
||||||
try {
|
try {
|
||||||
const { primaryIdentity } = loadIdentities(await client.getIdentities(), jmapUsername);
|
const { primaryIdentity } = loadIdentities(await client.getIdentities(), client.getUsername());
|
||||||
canonical = primaryIdentity?.email || jmapUsername;
|
if (primaryIdentity?.email) ids.add(generateAccountId(primaryIdentity.email, serverUrl));
|
||||||
} catch {
|
} catch { /* identities unavailable */ }
|
||||||
/* identities unavailable — fall back to the JMAP session username */
|
return [...ids];
|
||||||
}
|
|
||||||
return generateAccountId(canonical, serverUrl);
|
|
||||||
} catch {
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function clearRefreshTimer(accountId?: string): void {
|
function clearRefreshTimer(accountId?: string): void {
|
||||||
@@ -1393,9 +1400,9 @@ export const useAuthStore = create<AuthState>()(
|
|||||||
// connection then succeeds and we would silently show the wrong
|
// connection then succeeds and we would silently show the wrong
|
||||||
// mailbox. On mismatch, drop the poisoned cookies for this slot and
|
// mailbox. On mismatch, drop the poisoned cookies for this slot and
|
||||||
// force a clean re-auth instead of surfacing someone else's mail.
|
// force a clean re-auth instead of surfacing someone else's mail.
|
||||||
const connectedId = await connectedAccountId(targetClient, targetAccount.serverUrl);
|
const connectedCandidates = await connectedAccountCandidates(targetClient, targetAccount.serverUrl);
|
||||||
if (connectedId && connectedId !== accountId) {
|
if (connectedCandidates.length > 0 && !connectedCandidates.includes(accountId)) {
|
||||||
debug.error(`switchAccount: slot ${targetAccount.cookieSlot} for ${accountId} resolved to ${connectedId} — forcing re-auth`);
|
debug.error(`switchAccount: slot ${targetAccount.cookieSlot} for ${accountId} resolved to [${connectedCandidates.join(", ")}] — forcing re-auth`);
|
||||||
clients.delete(accountId);
|
clients.delete(accountId);
|
||||||
try { targetClient.disconnect(); } catch { /* noop */ }
|
try { targetClient.disconnect(); } catch { /* noop */ }
|
||||||
apiFetch(`/api/auth/token?slot=${targetAccount.cookieSlot}`, { method: 'DELETE' }).catch(() => {});
|
apiFetch(`/api/auth/token?slot=${targetAccount.cookieSlot}`, { method: 'DELETE' }).catch(() => {});
|
||||||
|
|||||||
Reference in New Issue
Block a user