Merge pull request #94 from LINUXexpert-org/fix-account-routing

Keep your own settings out of someone else's account

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);
   },
 }));