From 1829bd8ee493ffd76bfabb1692222c3d47dce5d9 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Tue, 6 Oct 2026 23:26:27 +0200 Subject: [PATCH] fix: stop a previous account's settings leaking into the next sign-in #1185 --- lib/sign-out-cleanup.ts | 3 +- .../settings-store-account-leak.test.ts | 184 ++++++++++++++++++ stores/auth-store.ts | 7 + stores/settings-store.ts | 106 +++++++++- 4 files changed, 294 insertions(+), 6 deletions(-) create mode 100644 stores/__tests__/settings-store-account-leak.test.ts diff --git a/lib/sign-out-cleanup.ts b/lib/sign-out-cleanup.ts index 6c70a84b7..da2212907 100644 --- a/lib/sign-out-cleanup.ts +++ b/lib/sign-out-cleanup.ts @@ -8,7 +8,8 @@ import { fileStorage } from '@/lib/plugin-storage'; * Kept on purpose: device preferences (`settings-storage`, `theme-storage`, * `locale-storage`, the Files view settings) and `template-storage`. With * settings sync off they exist only here, so deleting them would destroy - * the user's own work rather than a cache. + * the user's own work rather than a cache. With sync on, the settings store + * resets settings and templates itself (forgetSyncedSettings). */ const SIGNED_OUT_STORAGE_KEYS = [ 'email-snapshot', diff --git a/stores/__tests__/settings-store-account-leak.test.ts b/stores/__tests__/settings-store-account-leak.test.ts new file mode 100644 index 000000000..d8702b317 --- /dev/null +++ b/stores/__tests__/settings-store-account-leak.test.ts @@ -0,0 +1,184 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const apiFetch = vi.hoisted(() => vi.fn()); + +vi.mock('@/lib/browser-navigation', () => ({ apiFetch })); + +import { useSettingsStore } from '../settings-store'; +import { useTemplateStore } from '../template-store'; + +// #1185: settings-storage is one store for the whole browser. After signing +// out of A, an account B without server settings showed A's tags, templates +// and preferences, and the next push wrote them into B's server file. + +const A = { username: 'a@example.com', serverUrl: 'https://mail.example.com' }; +const B = { username: 'b@example.com', serverUrl: 'https://mail.example.com' }; +const SECRET_TAG = { id: 'customer-x', label: 'Customer X', color: 'red' }; + +const json = (body: unknown, status = 200) => + new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }); + +/** Server files by username; a missing entry means "no settings yet". */ +let serverFiles: Record = {}; +let failLoads = false; + +function posted(): Array<{ username: string; settings: Record }> { + return apiFetch.mock.calls + .filter(([, init]) => (init as RequestInit | undefined)?.method === 'POST') + .map(([, init]) => JSON.parse((init as RequestInit).body as string)); +} + +async function signIn(account: typeof A) { + const settings = useSettingsStore.getState(); + await settings.loadFromServer(account.username, account.serverUrl).finally(() => { + useSettingsStore.getState().enableSync(account.username, account.serverUrl); + }); +} + +async function signOut() { + await useSettingsStore.getState().flushSync(); + useSettingsStore.getState().disableSync(); + useSettingsStore.getState().forgetSyncedSettings(); +} + +/** Signed in as A, with a custom tag and a template that reached A's file. */ +async function setUpAccountA() { + serverFiles[A.username] = { emailKeywords: [SECRET_TAG], fontSize: 'large' }; + await signIn(A); + useTemplateStore.getState().addTemplate({ + name: 'A only', subject: '', body: '', category: '', isFavorite: false, + }); + await useSettingsStore.getState().flushSync(); + expect(useSettingsStore.getState().emailKeywords).toEqual([SECRET_TAG]); +} + +beforeEach(async () => { + localStorage.clear(); + serverFiles = {}; + failLoads = false; + apiFetch.mockReset(); + apiFetch.mockImplementation(async (_url: string, init?: RequestInit) => { + if (init?.method === 'POST') return json({ ok: true }); + if (failLoads) return json({ error: 'boom' }, 500); + const username = (init?.headers as Record)['x-settings-username']; + return json({ settings: serverFiles[username] ?? null }); + }); + useSettingsStore.getState().disableSync(); + useSettingsStore.getState().resetToDefaults(); + useTemplateStore.setState({ templates: [], recentTemplateIds: [], deletedTemplateIds: {} }); +}); + +describe('switching people in one browser (#1185)', () => { + it('starts the next account from defaults after a sign-out', async () => { + await setUpAccountA(); + await signOut(); + + expect(useSettingsStore.getState().emailKeywords).not.toContainEqual(SECRET_TAG); + expect(useTemplateStore.getState().templates).toEqual([]); + + apiFetch.mockClear(); + await signIn(B); + useSettingsStore.getState().updateSetting('sendDelaySeconds', 10); + await useSettingsStore.getState().flushSync(); + + const pushes = posted(); + expect(pushes.map((p) => p.username)).toEqual([B.username]); + expect(pushes[0].settings.emailKeywords).not.toContainEqual(SECRET_TAG); + expect(pushes[0].settings.templates).toEqual([]); + expect(pushes[0].settings.fontSize).toBe('medium'); + }); + + it("drops A's state when B signs in without a sign-out in between", async () => { + await setUpAccountA(); + useSettingsStore.getState().disableSync(); // e.g. the session expired + + apiFetch.mockClear(); + await signIn(B); + useSettingsStore.getState().updateSetting('sendDelaySeconds', 10); + await useSettingsStore.getState().flushSync(); + + expect(useSettingsStore.getState().emailKeywords).not.toContainEqual(SECRET_TAG); + const pushes = posted(); + expect(pushes.map((p) => p.username)).toEqual([B.username]); + expect(pushes[0].settings.emailKeywords).not.toContainEqual(SECRET_TAG); + expect(pushes[0].settings.templates).toEqual([]); + }); + + it("does not merge A's templates into B's server file", async () => { + await setUpAccountA(); + useSettingsStore.getState().disableSync(); + serverFiles[B.username] = { fontSize: 'small', templates: [], deletedTemplateIds: {} }; + + apiFetch.mockClear(); + await signIn(B); + await useSettingsStore.getState().flushSync(); + + expect(useTemplateStore.getState().templates).toEqual([]); + expect(useSettingsStore.getState().emailKeywords).not.toContainEqual(SECRET_TAG); + expect(useSettingsStore.getState().fontSize).toBe('small'); + expect(posted()).toEqual([]); + }); + + it("keeps sync off when B's settings cannot be loaded over A's", async () => { + await setUpAccountA(); + useSettingsStore.getState().disableSync(); + failLoads = true; + + apiFetch.mockClear(); + await signIn(B); + useSettingsStore.getState().updateSetting('sendDelaySeconds', 10); + await useSettingsStore.getState().flushSync(); + + expect(useSettingsStore.getState().emailKeywords).not.toContainEqual(SECRET_TAG); + expect(posted()).toEqual([]); + }); + + it('lets the same account pick up where it left off', async () => { + await setUpAccountA(); + useSettingsStore.getState().disableSync(); + failLoads = true; + + await signIn(A); + useSettingsStore.getState().updateSetting('sendDelaySeconds', 10); + await useSettingsStore.getState().flushSync(); + + expect(useSettingsStore.getState().emailKeywords).toEqual([SECRET_TAG]); + expect(posted().at(-1)?.username).toBe(A.username); + }); +}); + +describe('settings that never reached a server', () => { + it('lets the first synced sign-in adopt the device settings', async () => { + useSettingsStore.getState().addKeyword(SECRET_TAG); + + await signIn(A); + expect(useSettingsStore.getState().emailKeywords).toContainEqual(SECRET_TAG); + useSettingsStore.getState().updateSetting('sendDelaySeconds', 10); + await useSettingsStore.getState().flushSync(); + expect(posted()[0].settings.emailKeywords).toContainEqual(SECRET_TAG); + + // From here on they live in A's file, so a sign-out may drop them. + await signOut(); + expect(useSettingsStore.getState().emailKeywords).not.toContainEqual(SECRET_TAG); + }); + + it('keeps them on sign-out when sync was never used', () => { + useSettingsStore.getState().addKeyword(SECRET_TAG); + useSettingsStore.getState().forgetSyncedSettings(); + expect(useSettingsStore.getState().emailKeywords).toContainEqual(SECRET_TAG); + }); + + it('keeps them on sign-out when the user opted out of sync', async () => { + await setUpAccountA(); + useSettingsStore.setState({ settingsSyncDisabled: true }); + await signOut(); + expect(useSettingsStore.getState().emailKeywords).toEqual([SECRET_TAG]); + }); + + it('keeps device-only settings through the reset', async () => { + useSettingsStore.setState({ proInterface: true }); + await setUpAccountA(); + await signOut(); + expect(useSettingsStore.getState().proInterface).toBe(true); + }); +}); diff --git a/stores/auth-store.ts b/stores/auth-store.ts index 3d9fde2e6..788cf7b76 100644 --- a/stores/auth-store.ts +++ b/stores/auth-store.ts @@ -1196,6 +1196,10 @@ function forgetCalendarSubscriptions(client: IJMAPClient): void { function performFullLogout(set: (state: Partial) => void): void { useSettingsStore.getState().disableSync(); + // With settings sync on, the local settings and templates are a copy of the + // signed-out account's server file. Left in place, the next account to sign + // in here would show them and push them to its own file (#1185). + useSettingsStore.getState().forgetSyncedSettings(); set({ isAuthenticated: false, @@ -2235,6 +2239,9 @@ export const useAuthStore = create()( // while the clients are torn down. const cleanup = clearAllCredentials(endSessionSlot); + // The settings are reset below; save the last edits first. + await useSettingsStore.getState().flushSync(); + // Disconnect all clients set({ client: null }); for (const c of clients.values()) { diff --git a/stores/settings-store.ts b/stores/settings-store.ts index 07e49541f..ac068bdf6 100644 --- a/stores/settings-store.ts +++ b/stores/settings-store.ts @@ -36,6 +36,35 @@ let isLoadingFromServer = false; let pushTemplatesAfterLoad = false; // Wired up in the window-only init block below; null during SSR. let requestSync: (() => void) | null = null; +// Account whose settings could not be loaded while this browser still held +// another account's. enableSync leaves sync off for it, or the next change +// would push the other account's settings under its name (#1185). +let blockedSyncAccountId: string | null = null; + +// settings-storage is one store for the whole browser. This key names the +// account whose server copy it currently mirrors, so a different account +// signing in can tell the local settings are not its own (#1185). Unset in +// browsers that never synced, where the local settings are the user's own +// device settings and the first sign-in adopts them. +const SETTINGS_OWNER_KEY = 'settings-sync-owner'; + +function readSettingsOwner(): string | null { + try { + return typeof window === 'undefined' ? null : window.localStorage.getItem(SETTINGS_OWNER_KEY); + } catch { + return null; + } +} + +function writeSettingsOwner(accountId: string | null): void { + try { + if (typeof window === 'undefined') return; + if (accountId) window.localStorage.setItem(SETTINGS_OWNER_KEY, accountId); + else window.localStorage.removeItem(SETTINGS_OWNER_KEY); + } catch { + /* storage unavailable */ + } +} const SYNC_DEBOUNCE_MS = 2000; @@ -590,6 +619,12 @@ interface SettingsState { flushSync: () => Promise; disableSync: () => void; loadFromServer: (username: string, serverUrl: string) => Promise; + /** + * Called on a full sign-out: drops the settings and templates this browser + * mirrored from an account's server copy, so the next person to sign in + * does not start from them (#1185). Device-only settings are kept. + */ + forgetSyncedSettings: () => void; } const DEFAULT_SETTINGS = { @@ -804,7 +839,29 @@ const DEFAULT_SETTINGS = { export const useSettingsStore = create()( persist( - (set, get) => ({ + (set, get) => { + /** + * Back to defaults for everything an account's server copy holds: + * settings (device-only ones kept) and templates. Never pushed, since + * sync may still point at the account the state came from. + */ + const resetSyncedState = () => { + const defaults: Record = { ...DEFAULT_SETTINGS }; + for (const key of DEVICE_LOCAL_SETTING_KEYS) delete defaults[key]; + const wasLoading = isLoadingFromServer; + isLoadingFromServer = true; + try { + set(defaults as Partial); + templateSyncBridge?.applySyncedState([], {}, { merge: false }); + } finally { + isLoadingFromServer = wasLoading; + } + applyFontSize(DEFAULT_SETTINGS.fontSize); + applyDensity(DEFAULT_SETTINGS.density); + applyAnimations(DEFAULT_SETTINGS.animationsEnabled); + }; + + return { ...DEFAULT_SETTINGS, updateSetting: (key, value) => { @@ -1175,6 +1232,12 @@ export const useSettingsStore = create()( // Settings sync methods enableSync: (username: string, serverUrl: string) => { + const accountId = generateAccountId(username, serverUrl); + if (blockedSyncAccountId === accountId) { + syncWarn('Settings sync stays off for', username, '- its settings could not be loaded'); + return; + } + writeSettingsOwner(accountId); syncUsername = username; syncServerUrl = serverUrl; syncEnabled = true; @@ -1208,6 +1271,21 @@ export const useSettingsStore = create()( }, loadFromServer: async (username: string, serverUrl: string) => { + const accountId = generateAccountId(username, serverUrl); + const owner = readSettingsOwner(); + const heldByOther = owner !== null && owner !== accountId; + // The local settings belong to another account and this one's could + // not be read: show defaults and keep sync off, rather than showing + // the other account's settings or pushing them under this name. + const failWithoutForeignState = () => { + if (heldByOther) { + resetSyncedState(); + writeSettingsOwner(null); + blockedSyncAccountId = accountId; + } + return false; + }; + blockedSyncAccountId = null; try { syncLog('Loading settings from server for', username); pushTemplatesAfterLoad = false; @@ -1220,22 +1298,31 @@ export const useSettingsStore = create()( if (!res.ok) { const body = await res.json().catch(() => ({})); syncLog('Settings fetch failed:', body.error || `status ${res.status}`); - return false; + return failWithoutForeignState(); } const { settings } = await res.json(); if (!settings) { syncLog('No server settings found yet'); + // A new account starts from defaults, not from whatever the + // previous account left in this browser (#1185). + if (heldByOther) resetSyncedState(); + writeSettingsOwner(accountId); pushTemplatesAfterLoad = localTemplatesAhead(null); return false; } if (settings && typeof settings === 'object') { + // Importing merges templates and leaves keys the blob lacks as + // they are, so another account's state would carry over into + // this one and be pushed back to its server copy. + if (heldByOther) resetSyncedState(); isLoadingFromServer = true; // Merge (not replace) per-account maps for the account being loaded, // so multi-account logins don't clobber each other by login order. get().importSettings(JSON.stringify(settings), { - serverAccountId: generateAccountId(username, serverUrl), + serverAccountId: accountId, }); isLoadingFromServer = false; + writeSettingsOwner(accountId); pushTemplatesAfterLoad = localTemplatesAhead(settings); syncLog('Settings loaded from server successfully'); // The per-account preferred sender identity (#507) is re-applied by @@ -1248,10 +1335,19 @@ export const useSettingsStore = create()( } catch (error) { syncError('Failed to load settings from server:', error); isLoadingFromServer = false; - return false; + return failWithoutForeignState(); } }, - }), + + forgetSyncedSettings: () => { + // Never synced: the local settings are this device's own and exist + // nowhere else. Synced but opted out: they were never uploaded either. + if (readSettingsOwner() === null || get().settingsSyncDisabled) return; + resetSyncedState(); + writeSettingsOwner(null); + }, + }; + }, { name: 'settings-storage', version: 7,