From e014521fb6ab81001ba3f559ed2abab3e0a075dc Mon Sep 17 00:00:00 2001 From: John Coffey Date: Thu, 27 Aug 2026 09:58:48 -0700 Subject: [PATCH] Keep your own settings out of someone else's account Switching to an account somebody shared pointed the whole app at it. The rule was "use the selected account if it can do this", and a shared file account can, by definition, do files. ihasmail keeps its settings in the account's Files -- that is what makes them follow you between devices -- so changing any setting while looking at somebody's shared folder wrote `settings.json` into *their* storage, creating the `ihasmail` folder there to do it. Signature images went the same way, and push registration would have gone to whichever account was on screen. Reading someone else's data by mistake is bad; writing yours into theirs is worse, and one line was doing both. There are two questions, and they had one answer: - what am I looking at -- follows the switcher, because switching to a shared account is how you read what was shared - what is mine -- never does So `accountFor` keeps the first meaning and `ownAccountFor` is the second, used by settings sync, signature images and push. A `?? accountId` fallback in `loadStoredSignature` went with it: the reader's own signature, reached through whoever happened to be selected. A third rule was hiding in the first. A capability the selected account does not advertise fell back to the selected account anyway, so a session naming no primary for something aimed it at whoever was selected -- somebody else. It now answers with nothing, which is honest: the feature is unavailable, rather than pointed at a stranger. What this does not settle is whether the mail, calendar and contacts the switcher appeared to offer were ever really reachable, or only asked for and refused. That depends on what Stalwart advertises on a shared account, which needs a look at a sharee's session; if it advertises capabilities nobody shared, more is needed here than routing. --- web/src/lib/__tests__/accountRouting.test.ts | 85 ++++++++++++++++++++ web/src/lib/accountRouting.ts | 78 ++++++++++++++++++ web/src/lib/settingsSync.ts | 6 +- web/src/lib/signatureImages.ts | 8 +- web/src/lib/webpushEnable.ts | 2 +- web/src/store/session.ts | 15 ++-- 6 files changed, 181 insertions(+), 13 deletions(-) create mode 100644 web/src/lib/__tests__/accountRouting.test.ts create mode 100644 web/src/lib/accountRouting.ts diff --git a/web/src/lib/__tests__/accountRouting.test.ts b/web/src/lib/__tests__/accountRouting.test.ts new file mode 100644 index 0000000..56dec1e --- /dev/null +++ b/web/src/lib/__tests__/accountRouting.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, it } from "vitest"; +import { accountForCapability, ownAccountForCapability, type SessionLike } from "@/lib/accountRouting"; + +/** + * Found by sharing a folder between two real accounts. + * + * Switching to the account somebody shared pointed everything at it, because + * the rule was "use the selected account if it can do this" and a shared file + * account can, by definition, do files. ihasmail keeps its own settings in the + * account's Files, so changing any setting while looking at somebody's shared + * folder wrote `settings.json` into *their* storage, creating the `ihasmail` + * folder there to do it. Reading someone else's data by mistake is bad; writing + * yours into it is worse, and it was the same one-line rule doing both. + */ + +const CAL = "urn:ietf:params:jmap:calendars"; +const FILES = "urn:ietf:params:jmap:filenode"; +const MAIL = "urn:ietf:params:jmap:mail"; + +/** Mine does everything; theirs is a shared account with only files on it. */ +const shared = (): SessionLike => ({ + accounts: { + mine: { isPersonal: true, accountCapabilities: { [MAIL]: {}, [FILES]: {}, [CAL]: {} } }, + theirs: { isPersonal: false, accountCapabilities: { [FILES]: {} } }, + }, + primaryAccounts: { [MAIL]: "mine", [FILES]: "mine", [CAL]: "mine" }, +}); + +describe("what the reader is looking at", () => { + it("follows the switch into a shared account for what was shared", () => { + expect(accountForCapability(shared(), "theirs", FILES)).toBe("theirs"); + }); + + it("leaves everything else on the reader's own account", () => { + expect(accountForCapability(shared(), "theirs", MAIL)).toBe("mine"); + expect(accountForCapability(shared(), "theirs", CAL)).toBe("mine"); + }); + + it("still follows a switch between the reader's own accounts", () => { + const s = shared(); + s.accounts.second = { isPersonal: true, accountCapabilities: { [MAIL]: {} } }; + expect(accountForCapability(s, "second", MAIL)).toBe("second"); + }); + + it("gives up rather than aim at a shared account for something unshared", () => { + // No primary for calendars, and theirs does not offer them. The old rule + // fell back to the selection, which is somebody else's account. + const s = shared(); + delete s.primaryAccounts[CAL]; + expect(accountForCapability(s, "theirs", CAL)).toBeNull(); + }); + + it("lets one of the reader's own accounts stand in when there is no primary", () => { + const s = shared(); + delete s.primaryAccounts[CAL]; + expect(accountForCapability(s, "mine", CAL)).toBe("mine"); + }); +}); + +describe("what belongs to the reader", () => { + it("stays on their own account while they look at a shared one", () => { + // The one that matters: settings are written through this. + expect(ownAccountForCapability(shared(), FILES)).toBe("mine"); + }); + + it("ignores a primary account the server says is not the reader's", () => { + const s = shared(); + s.primaryAccounts[FILES] = "theirs"; + expect(ownAccountForCapability(s, FILES)).toBe("mine"); + }); + + it("finds a personal account when no primary is named", () => { + const s = shared(); + delete s.primaryAccounts[FILES]; + expect(ownAccountForCapability(s, FILES)).toBe("mine"); + }); + + it("answers nothing rather than a shared account", () => { + const s: SessionLike = { + accounts: { theirs: { isPersonal: false, accountCapabilities: { [FILES]: {} } } }, + primaryAccounts: {}, + }; + expect(ownAccountForCapability(s, FILES)).toBeNull(); + }); +}); diff --git a/web/src/lib/accountRouting.ts b/web/src/lib/accountRouting.ts new file mode 100644 index 0000000..c020c50 --- /dev/null +++ b/web/src/lib/accountRouting.ts @@ -0,0 +1,78 @@ +/** + * Which account a request goes to. + * + * A JMAP session lists more than one account whenever anything is shared with + * you: the sharer's account appears alongside your own, carrying whichever + * capabilities they shared. Switching to one is how you read their files, so + * some requests have to follow that selection. + * + * Others must never follow it, and telling the two apart is the whole point of + * this file. ihasmail keeps its own settings in the account's Files — that is + * what makes them travel between devices — and a shared file account advertises + * the file capability by definition. So the obvious rule, "use whichever + * account is selected if it can do this", writes your settings into the other + * person's storage the moment you change one while looking at their folder. It + * would create the `ihasmail` folder there to do it. + * + * Two questions, then, and they have different answers: + * + * - what am I *looking at* -> `accountForCapability`, follows the selection + * - what is *mine* -> `ownAccountForCapability`, never does + * + * There is a third rule hiding in the first. A capability the selected account + * does not advertise used to fall back to that account anyway, so a session + * with no primary account for something would aim it at whoever was selected — + * someone else. Falling back to nothing is the honest answer: the feature is + * unavailable, which is true, rather than pointed at a stranger's data. + */ +import type { Id } from "@/jmap/types"; + +export interface AccountLike { + /** JMAP: true when the account belongs to the authenticated user. */ + isPersonal: boolean; + accountCapabilities?: Record; +} + +export interface SessionLike { + accounts: Record; + primaryAccounts: Record; +} + +const advertises = (account: AccountLike | undefined, cap: string): boolean => + Boolean(account && cap in (account.accountCapabilities ?? {})); + +/** + * The account to read and write for this capability, honouring the switcher. + * + * Use for anything the reader is looking at: their mail, a shared calendar, + * somebody's files. Not for anything of the reader's own — see below. + */ +export function accountForCapability(session: SessionLike | null, selectedId: Id | null, cap: string): Id | null { + if (!session) return null; + const selected = selectedId ? session.accounts[selectedId] : undefined; + if (selected && advertises(selected, cap)) return selectedId; + const primary = session.primaryAccounts[cap]; + if (primary) return primary; + // No primary, and the selection cannot serve this. Falling back to the + // selection would aim the request at a shared account for something nobody + // shared; only one of the reader's own accounts may stand in. + if (selected && selected.isPersonal) return selectedId; + return null; +} + +/** + * The reader's own account for this capability, whatever they are looking at. + * + * Use for the reader's own state -- synced settings, signature images, push + * registration. These belong to them and follow them, and must not land in an + * account somebody shared just because it happens to be on screen. + */ +export function ownAccountForCapability(session: SessionLike | null, cap: string): Id | null { + if (!session) return null; + const primary = session.primaryAccounts[cap]; + // A primary account is the reader's own by definition, but check rather than + // assume: a server that named a shared one here would otherwise be trusted. + if (primary && session.accounts[primary]?.isPersonal !== false) return primary; + const own = Object.entries(session.accounts).find(([, a]) => a.isPersonal && advertises(a, cap)); + return own?.[0] ?? null; +} diff --git a/web/src/lib/settingsSync.ts b/web/src/lib/settingsSync.ts index 62023e4..fe5066b 100644 --- a/web/src/lib/settingsSync.ts +++ b/web/src/lib/settingsSync.ts @@ -36,7 +36,7 @@ let armed = false; let listenersBound = false; export function settingsSyncAvailable(): boolean { - return client.hasCapability(CAP.filenode) && Boolean(useSession.getState().accountFor(CAP.filenode)); + return client.hasCapability(CAP.filenode) && Boolean(useSession.getState().ownAccountFor(CAP.filenode)); } /** @@ -46,7 +46,7 @@ export function settingsSyncAvailable(): boolean { */ export async function loadRemoteSettings(): Promise | null> { if (!settingsSyncAvailable()) return null; - const accountId = useSession.getState().accountFor(CAP.filenode)!; + const accountId = useSession.getState().ownAccountFor(CAP.filenode)!; try { const folderId = await ensureFolder(accountId); const node = await findInFolder(accountId, folderId, FILE); @@ -109,7 +109,7 @@ export async function flushSettingsPush(): Promise { async function writeSettings(body: Record): Promise { if (!settingsSyncAvailable()) return; - const accountId = useSession.getState().accountFor(CAP.filenode)!; + const accountId = useSession.getState().ownAccountFor(CAP.filenode)!; const json = JSON.stringify(body, null, 2); // Byte length, not character count: a template or a signature with any // non-ASCII in it would otherwise be reported shorter than it is. diff --git a/web/src/lib/signatureImages.ts b/web/src/lib/signatureImages.ts index 7b7a574..8a656c1 100644 --- a/web/src/lib/signatureImages.ts +++ b/web/src/lib/signatureImages.ts @@ -13,7 +13,7 @@ import { toast } from "@/ui/toast"; /** Upload an image for use in a signature; returns a same-origin blob URL. */ export async function uploadSignatureImage(file: File): Promise { - const accountId = useSession.getState().accountFor(CAP.filenode); + const accountId = useSession.getState().ownAccountFor(CAP.filenode); if (!accountId || !client.hasCapability(CAP.filenode)) { toast.error("Images in signatures need the Files feature, which this account doesn't have."); throw new Error("filenode unavailable"); @@ -42,7 +42,7 @@ export async function uploadSignatureImage(file: File): Promise { /** Store the full HTML of an over-sized signature in Files; returns the blob id. */ export async function storeSignatureHtml(html: string): Promise { - const accountId = useSession.getState().accountFor(CAP.filenode); + const accountId = useSession.getState().ownAccountFor(CAP.filenode); if (!accountId || !client.hasCapability(CAP.filenode)) throw new Error("This signature is too long for the server and the Files feature (needed to store long signatures) is not available."); const up = await client.upload(accountId, new Blob([html], { type: "text/html" }), { type: "text/html" }); const folderId = await ensureFolder(accountId); @@ -77,7 +77,9 @@ export async function externalizeDataImages(html: string): Promise { /** Load the full HTML of a marker signature. */ export async function loadStoredSignature(blobId: string, type = "text/html"): Promise { - const accountId = useSession.getState().accountFor(CAP.filenode) ?? useSession.getState().accountId; + // No `?? accountId` fallback: a signature is the reader's own, and the + // selected account may be somebody else's shared one. + const accountId = useSession.getState().ownAccountFor(CAP.filenode); if (!accountId) throw new Error("no account"); return client.fetchBlobText(accountId, blobId, type); } diff --git a/web/src/lib/webpushEnable.ts b/web/src/lib/webpushEnable.ts index d2f8155..8017977 100644 --- a/web/src/lib/webpushEnable.ts +++ b/web/src/lib/webpushEnable.ts @@ -78,7 +78,7 @@ export async function enableWebPush(): Promise<{ ok: true } | { ok: false; reaso userVisibleOnly: true, applicationServerKey: decodeApplicationServerKey(key), })); - const accountId = useSession.getState().accountFor(CAP.mail); + const accountId = useSession.getState().ownAccountFor(CAP.mail); const inboxId = useMail.getState().roleId("inbox"); await createSubscription(subscriptionPayload(sub, accountId, inboxId)); listenForVerification(); diff --git a/web/src/store/session.ts b/web/src/store/session.ts index 1eb9f5d..2e7a562 100644 --- a/web/src/store/session.ts +++ b/web/src/store/session.ts @@ -2,6 +2,7 @@ import { create } from "zustand"; import { apiFetch, ApiError, CAP, client } from "@/jmap/client"; import type { Id, JmapSession } from "@/jmap/types"; import { push, type PushState } from "@/jmap/push"; +import { accountForCapability, ownAccountForCapability } from "@/lib/accountRouting"; import { setServerLocale } from "@/lib/datetime"; import { flushSettingsPush, stopSettingsSync } from "@/lib/settingsSync"; import { unsubscribeThisDevice } from "@/lib/webpush"; @@ -22,8 +23,10 @@ interface SessionState { logout(): Promise; refresh(): Promise; setAccount(id: Id): void; - /** Returns the accountId for a capability (primary), falling back to the selected mail account. */ + /** The account to read and write for a capability, honouring the account switcher. */ accountFor(cap: string): Id | null; + /** The user's own account for a capability, whatever they are looking at. */ + ownAccountFor(cap: string): Id | null; } export const useSession = create((set, get) => ({ @@ -97,11 +100,11 @@ export const useSession = create((set, get) => ({ }, accountFor(cap) { - const s = get().session; - if (!s) return null; - const selected = get().accountId; - if (selected && s.accounts[selected] && cap in (s.accounts[selected]?.accountCapabilities ?? {})) return selected; - return s.primaryAccounts[cap] ?? selected ?? null; + return accountForCapability(get().session, get().accountId, cap); + }, + + ownAccountFor(cap) { + return ownAccountForCapability(get().session, cap); }, }));