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

@@ -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();
+  });
+});

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<string, unknown>;
+}
+
+export interface SessionLike {
+  accounts: Record<Id, AccountLike>;
+  primaryAccounts: Record<string, Id>;
+}
+
+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;
+}

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<Record<string, unknown> | 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<void> {
 
 async function writeSettings(body: Record<string, unknown>): Promise<void> {
   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.

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<string> {
-  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<string> {
 
 /** Store the full HTML of an over-sized signature in Files; returns the blob id. */
 export async function storeSignatureHtml(html: string): Promise<string> {
-  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<string> {
 
 /** Load the full HTML of a marker signature. */
 export async function loadStoredSignature(blobId: string, type = "text/html"): Promise<string> {
-  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);
 }

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();

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<void>;
   refresh(): Promise<void>;
   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<SessionState>((set, get) => ({
@@ -97,11 +100,11 @@ export const useSession = create<SessionState>((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);
   },
 }));