From 3f01e94319a392a6d60b76d6e3c2e45fa2d3ba34 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 26 Sep 2026 14:59:55 +0200 Subject: [PATCH] fix: key synced settings on the account a bearer token belongs to A Bearer login may claim a bare local part, which is accepted when the server's `Session.username` has the same local part (TOTP users who type `john` log in this way). Settings were keyed on that claim, so on a multi-domain server `john@b.example` could claim `john` and read and overwrite the synced settings of the `john@a.example` who signs in as `john` with a password. `resolveJmapIdentity` now returns the canonical account name for a bare Bearer claim, the context cookie carries it as `accountName`, and the settings route keys on it. Basic logins and fully-qualified claims keep their existing key. --- app/api/auth/stalwart-context/route.ts | 5 +- app/api/settings/route.ts | 28 +++--- .../settings-route-account-key.test.ts | 97 +++++++++++++++++++ lib/__tests__/verify-jmap-identity.test.ts | 34 +++++++ lib/auth/verify-jmap-auth.ts | 32 +++++- lib/stalwart/auth-context.ts | 13 ++- 6 files changed, 190 insertions(+), 19 deletions(-) create mode 100644 lib/__tests__/settings-route-account-key.test.ts diff --git a/app/api/auth/stalwart-context/route.ts b/app/api/auth/stalwart-context/route.ts index b79407b42..01a40a673 100644 --- a/app/api/auth/stalwart-context/route.ts +++ b/app/api/auth/stalwart-context/route.ts @@ -1,6 +1,6 @@ import { NextRequest, NextResponse } from 'next/server'; import { logger } from '@/lib/logger'; -import { JmapAuthVerificationError, verifyJmapIdentity } from '@/lib/auth/verify-jmap-auth'; +import { JmapAuthVerificationError, resolveJmapIdentity } from '@/lib/auth/verify-jmap-auth'; import { setStalwartAuthContext } from '@/lib/stalwart/auth-context'; import { configManager } from '@/lib/admin/config-manager'; import { isPublicHttpUrl } from '@/lib/security/url-guard'; @@ -71,7 +71,7 @@ export async function POST(request: NextRequest) { // any user's synced settings. Admin-configured servers are `trusted`, // which only relaxes the public-address requirement (they may live on a // private network) - never the credential check. - const normalizedServerUrl = await verifyJmapIdentity(upstreamUrl, authHeader, username, { + const { serverUrl: normalizedServerUrl, accountName } = await resolveJmapIdentity(upstreamUrl, authHeader, username, { trusted: upstreamTrusted, }); @@ -79,6 +79,7 @@ export async function POST(request: NextRequest) { serverUrl: normalizedServerUrl, username, authHeader, + ...(accountName !== username ? { accountName } : {}), }); void recordLogin(username, normalizedServerUrl); diff --git a/app/api/settings/route.ts b/app/api/settings/route.ts index e456d5ed9..d3ff0a0e5 100644 --- a/app/api/settings/route.ts +++ b/app/api/settings/route.ts @@ -67,9 +67,12 @@ function normalizeUrl(url: string): string { * With multi-account, the requesting account may be on any slot. * Checks both basic-auth session cookies and stalwart auth context cookies * (used by OAuth/SSO and TOTP-upgraded sessions). - * Returns true only if a matching cookie is found. + * Returns the account name to key the settings on, or null when no cookie + * matches. That is the cookie's `accountName` when it has one: a Bearer + * login claimed with a bare local part must not share `john`'s settings + * with every other `john` on a multi-domain server. */ -async function verifyIdentity(username: string, serverUrl: string): Promise { +async function verifyIdentity(username: string, serverUrl: string): Promise { const cookieStore = await cookies(); const normalizedServerUrl = normalizeUrl(serverUrl); @@ -79,19 +82,19 @@ async function verifyIdentity(username: string, serverUrl: string): Promise ({ + getSessionSecret: () => 's'.repeat(64), + hasSessionSecret: () => true, +})); +vi.mock('@/lib/logger', () => ({ + logger: { warn: () => {}, error: () => {}, info: () => {}, debug: () => {} }, +})); +vi.mock('@/lib/admin/config-manager', () => ({ + configManager: { + ensureLoaded: async () => {}, + get: (key: string, fallback: unknown) => (key === 'settingsSyncEnabled' ? true : fallback), + getPolicy: () => ({ restrictions: {} }), + }, +})); + +const jar = new Map(); +vi.mock('next/headers', () => ({ + cookies: async () => ({ + get: (name: string) => (jar.has(name) ? { name, value: jar.get(name)! } : undefined), + set: (name: string, value: string) => { jar.set(name, value); }, + delete: (name: string) => { jar.delete(name); }, + getAll: () => [...jar].map(([name, value]) => ({ name, value })), + }), +})); + +const store = new Map>(); +vi.mock('@/lib/settings-sync', () => ({ + loadUserSettings: async (username: string, serverUrl: string) => store.get(`${username}|${serverUrl}`) ?? null, + saveUserSettings: async (username: string, serverUrl: string, settings: Record) => { + store.set(`${username}|${serverUrl}`, settings); + }, + deleteUserSettings: async (username: string, serverUrl: string) => { store.delete(`${username}|${serverUrl}`); }, +})); + +const SERVER = 'https://mail.example.org'; +const SAME_ORIGIN = { 'sec-fetch-site': 'same-origin', 'content-type': 'application/json' }; + +async function signIn(context: { username: string; accountName?: string }) { + jar.clear(); + const { setStalwartAuthContextInStore } = await import('@/lib/stalwart/auth-context'); + const cookieStore = { + get: (n: string) => (jar.has(n) ? { name: n, value: jar.get(n)! } : undefined), + set: (n: string, v: string) => { jar.set(n, v); }, + delete: (n: string) => { jar.delete(n); }, + }; + setStalwartAuthContextInStore(cookieStore as never, 0, { serverUrl: SERVER, authHeader: 'Bearer t', ...context }); +} + +beforeEach(() => { + store.clear(); + process.env.SETTINGS_SYNC_ENABLED = 'true'; +}); + +describe('settings route keys on the account the credential belongs to', () => { + it('saves a bare Bearer claim under the canonical account name', async () => { + await signIn({ username: 'john', accountName: 'john@b.example' }); + const { POST } = await import('@/app/api/settings/route'); + const res = await POST(new NextRequest('https://webmail.example/api/settings', { + method: 'POST', + headers: SAME_ORIGIN, + body: JSON.stringify({ username: 'john', serverUrl: SERVER, settings: { theme: 'dark' } }), + })); + expect(res.status).toBe(200); + expect([...store.keys()]).toEqual([`john@b.example|${SERVER}`]); + }); + + it("does not read the settings of the john who signs in with a password", async () => { + store.set(`john|${SERVER}`, { signature: 'john@a.example private' }); + await signIn({ username: 'john', accountName: 'john@b.example' }); + const { GET } = await import('@/app/api/settings/route'); + const res = await GET(new NextRequest('https://webmail.example/api/settings', { + headers: { 'x-settings-username': 'john', 'x-settings-server': SERVER }, + })); + expect(await res.json()).toEqual({ settings: null }); + }); + + it('keeps keying on the login name when the context names no other account', async () => { + await signIn({ username: 'alice@example.org' }); + const { POST } = await import('@/app/api/settings/route'); + await POST(new NextRequest('https://webmail.example/api/settings', { + method: 'POST', + headers: SAME_ORIGIN, + body: JSON.stringify({ username: 'alice@example.org', serverUrl: SERVER, settings: { theme: 'light' } }), + })); + expect([...store.keys()]).toEqual([`alice@example.org|${SERVER}`]); + }); +}); diff --git a/lib/__tests__/verify-jmap-identity.test.ts b/lib/__tests__/verify-jmap-identity.test.ts index 2487ed6bb..0c2e3d6c4 100644 --- a/lib/__tests__/verify-jmap-identity.test.ts +++ b/lib/__tests__/verify-jmap-identity.test.ts @@ -179,3 +179,37 @@ describe('verifyJmapIdentity', () => { ).rejects.toMatchObject({ status: 502 }); }); }); + +describe('resolveJmapIdentity account name', () => { + let fetchSpy: ReturnType; + + beforeEach(() => { + vi.resetModules(); + fetchSpy = vi.spyOn(globalThis, 'fetch'); + }); + + afterEach(() => { + fetchSpy.mockRestore(); + }); + + it('keys a bare Bearer claim on the canonical login, so john@b cannot become john', async () => { + fetchSpy.mockResolvedValueOnce(json({ apiUrl: 'https://mail.internal/jmap/', username: 'john@b.example', accounts: {} })); + const { resolveJmapIdentity } = await load(); + await expect(resolveJmapIdentity('https://mail.internal', 'Bearer tok', 'john', { trusted: true })) + .resolves.toEqual({ serverUrl: 'https://mail.internal', accountName: 'john@b.example' }); + }); + + it('keeps a fully-qualified Bearer claim as it is', async () => { + fetchSpy.mockResolvedValueOnce(json({ apiUrl: 'https://mail.internal/jmap/', username: 'john@b.example', accounts: {} })); + const { resolveJmapIdentity } = await load(); + await expect(resolveJmapIdentity('https://mail.internal', 'Bearer tok', 'John@B.example', { trusted: true })) + .resolves.toEqual({ serverUrl: 'https://mail.internal', accountName: 'John@B.example' }); + }); + + it('keeps a Basic claim as it is: the server accepted exactly that login', async () => { + fetchSpy.mockResolvedValueOnce(json({ apiUrl: 'https://mail.internal/jmap/', username: 'john@a.example', accounts: {} })); + const { resolveJmapIdentity } = await load(); + await expect(resolveJmapIdentity('https://mail.internal', basic('john', 'pw'), 'john', { trusted: true })) + .resolves.toEqual({ serverUrl: 'https://mail.internal', accountName: 'john' }); + }); +}); diff --git a/lib/auth/verify-jmap-auth.ts b/lib/auth/verify-jmap-auth.ts index 86059c2f3..238310c6e 100644 --- a/lib/auth/verify-jmap-auth.ts +++ b/lib/auth/verify-jmap-auth.ts @@ -220,6 +220,29 @@ export async function verifyJmapIdentity( claimedUsername: string, options: VerifyJmapAuthOptions = {}, ): Promise { + return (await resolveJmapIdentity(serverUrl, authHeader, claimedUsername, options)).serverUrl; +} + +export interface ResolvedJmapIdentity { + /** Normalized server URL. */ + serverUrl: string; + /** + * The account the credential belongs to. Equal to the claim, except for a + * Bearer credential claimed with a bare local part: that claim names no + * domain, so two users (`john@a.example`, `john@b.example`) would share it, + * and the server's canonical `Session.username` is used instead. Anything + * keyed per user (synced settings) must key on this, not on the claim. + */ + accountName: string; +} + +/** {@link verifyJmapIdentity}, also returning the account name to key per-user data on. */ +export async function resolveJmapIdentity( + serverUrl: string, + authHeader: string, + claimedUsername: string, + options: VerifyJmapAuthOptions = {}, +): Promise { if (!claimedUsername) { throw new JmapAuthVerificationError('Missing username', 400); } @@ -231,16 +254,19 @@ export async function verifyJmapIdentity( const { serverUrl: normalizedServerUrl, session } = await fetchVerifiedSession(serverUrl, authHeader, options); if (isBasic) { - return normalizedServerUrl; + // The server just accepted exactly this user:password, and a bare name + // resolves to one account on it. + return { serverUrl: normalizedServerUrl, accountName: claimedUsername }; } if (usernameMatchesSession(claimedUsername, session.username)) { - return normalizedServerUrl; + const bareClaim = !claimedUsername.includes('@') && session.username!.includes('@'); + return { serverUrl: normalizedServerUrl, accountName: bareClaim ? session.username! : claimedUsername }; } const identityEmails = await fetchIdentityEmails(normalizedServerUrl, authHeader, session, options); if (identityEmails.some((email) => usernamesEqual(email, claimedUsername))) { - return normalizedServerUrl; + return { serverUrl: normalizedServerUrl, accountName: claimedUsername }; } throw new JmapAuthVerificationError('Username does not match credentials', 403); diff --git a/lib/stalwart/auth-context.ts b/lib/stalwart/auth-context.ts index a58759b5e..38505b6b3 100644 --- a/lib/stalwart/auth-context.ts +++ b/lib/stalwart/auth-context.ts @@ -8,6 +8,12 @@ export interface StalwartAuthContext { serverUrl: string; username: string; authHeader: string; + /** + * The account the credential belongs to, when it differs from the login + * name the client claimed (see ResolvedJmapIdentity.accountName). Per-user + * data such as synced settings is keyed on this. + */ + accountName?: string; } type CookieStore = Awaited>; @@ -32,7 +38,8 @@ function isValidContext(payload: unknown): payload is StalwartAuthContext { } return typeof candidate.serverUrl === 'string' && typeof candidate.username === 'string' - && typeof candidate.authHeader === 'string'; + && typeof candidate.authHeader === 'string' + && (candidate.accountName === undefined || typeof candidate.accountName === 'string'); } function getSessionCookieOptions() { @@ -52,8 +59,8 @@ export function readStalwartAuthContextFromStore( // Hand back exactly the declared shape: the envelope's own bookkeeping // stays in the envelope rather than riding along into the credentials. - const { serverUrl, username, authHeader } = payload; - return { serverUrl, username, authHeader }; + const { serverUrl, username, authHeader, accountName } = payload; + return accountName ? { serverUrl, username, authHeader, accountName } : { serverUrl, username, authHeader }; } export async function readStalwartAuthContext(slot: number): Promise {