From 029f079094ea5fc44f21a714dd586094fc7195ee Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 4 Sep 2026 08:10:28 -0700 Subject: [PATCH] Ask the folder, not just the identity list, whether a message was mine Replying to a thread whose last message I sent addressed the reply to me: Reply put my own address in To, and Reply all put me in To with everyone I had actually written to demoted to Cc. Following up on your own last message is an ordinary thing to do, and this made it useless. There was already a guard for exactly this, and the guard was sound. What it rested on was not. It asked whether an address was in the identity list, and that question has a wrong answer in more situations than it has a right one: - the list is empty until identities load; - an alias or a shared mailbox is not in it at all; - it compared lowercased strings with `includes` where the rest of the codebase uses `sameAddress`, so an identity address stored with whitespace was enough to break it; - the check ran on the address the reply was about to go to rather than on the sender, so a message of mine carrying a Reply-To skipped it entirely and my reply went to my own desk; - and the Reply all branch never filtered my own address out of To, though the Reply branch did. Every one of those failed silently, which is why five of them accumulated. So the folder is asked first: a message in Sent is mine whatever address it went out as, and `mailboxIds` is already fetched in LIST_PROPS with roleId("sent") on the mail store, so this costs no request. The identity list stays as a second opinion, now compared with `sameAddress`, and the whole test keys off the sender rather than off the computed recipient. Two cases remain unanswerable and are commented rather than papered over: a message from an unlisted alias that is not in Sent either, and any message at all when identities failed to load and it is not in Sent. Neither signal exists. Both are far narrower than what was broken. Reply addressing had no tests at all, which is how a guard this load-bearing came to be wrong five ways at once. Fifteen now, seven of which fail against the old code. --- .../store/__tests__/reply-addressing.test.ts | 165 ++++++++++++++++++ web/src/store/compose.ts | 52 ++++-- 2 files changed, 203 insertions(+), 14 deletions(-) create mode 100644 web/src/store/__tests__/reply-addressing.test.ts diff --git a/web/src/store/__tests__/reply-addressing.test.ts b/web/src/store/__tests__/reply-addressing.test.ts new file mode 100644 index 0000000..d754e5d --- /dev/null +++ b/web/src/store/__tests__/reply-addressing.test.ts @@ -0,0 +1,165 @@ +import { beforeEach, afterEach, describe, expect, it } from "vitest"; +import { useCompose } from "@/store/compose"; +import { useMail } from "@/store/mail"; +import type { Email, Identity } from "@/jmap/types"; + +/* + * Who a reply is addressed to. + * + * The hard half is replying to something *I* sent, which is what following up + * on your own last message is. The conversation is with the people I wrote to; + * addressing the reply to myself, or to my own Reply-To, sends it nowhere + * useful -- and on a Reply all it quietly demotes everyone I was talking to + * into Cc. + * + * There was a guard for this and it was sound. What it rested on was not: it + * asked whether an address was in the identity list, which is empty before + * identities load, misses an alias or a shared mailbox the server does not list + * as an identity, and compared strings where the rest of the codebase uses + * `sameAddress`. Each miss was silent. So the folder is asked first -- a + * message in Sent is mine whatever address it went out as -- and the identity + * list is the second opinion rather than the only one (#275). + */ + +const body = { + messageId: [""], subject: "Numbers", references: [], inReplyTo: [], + keywords: {}, htmlBody: [{ partId: "1", type: "text/html" }], textBody: [{ partId: "1", type: "text/html" }], + bodyValues: { "1": { value: "

hi

", isEncodingProblem: false, isTruncated: false } }, + attachments: [], receivedAt: "2026-09-04T10:00:00Z", mailboxIds: {}, +}; + +const ME = { name: "John", email: "john@example.org" }; +const ANN = { name: "Ann", email: "ann@example.com" }; +const BOB = { name: "Bob", email: "bob@example.com" }; + +/** A message I sent: me in From, Ann in To, Bob in Cc. */ +const MINE = { ...body, id: "m1", from: [ME], to: [ANN], cc: [BOB] } as unknown as Email; +/** The same conversation, but Ann's message to me. */ +const HERS = { ...body, id: "m2", from: [ANN], to: [ME], cc: [BOB] } as unknown as Email; + +const IDENTITIES = [{ id: "i1", name: "John", email: "john@example.org", replyTo: null }] as unknown as Identity[]; + +/** In Sent, which is the signal that survives an unlisted alias. */ +const inSent = (e: Email) => ({ ...e, mailboxIds: { sent1: true } }) as Email; + +function draftFor(email: Email, mode: "reply" | "replyAll" | "forward", opts: { identities?: Identity[] } = {}) { + const identities = opts.identities ?? IDENTITIES; + useMail.setState({ + accountId: "a1", + identities: identities as never, + getEmails: (async () => [email]) as never, + defaultIdentity: (() => identities[0]) as never, + loadIdentities: (async () => identities) as never, + roleId: ((role: string) => (role === "sent" ? "sent1" : null)) as never, + }); + return useCompose.getState().reply(email, mode).then((key) => useCompose.getState().drafts.find((d) => d.key === key)!); +} + +const addrs = (list: { email: string }[]) => list.map((a) => a.email); + +beforeEach(() => useCompose.setState({ drafts: [], activeKey: null })); +afterEach(() => useCompose.setState({ drafts: [], activeKey: null })); + +describe("replying to a message somebody sent me", () => { + it("replies to the sender", async () => { + const d = await draftFor(HERS, "reply"); + expect(addrs(d.to)).toEqual([ANN.email]); + expect(d.cc).toEqual([]); + }); + + it("reply all keeps the others and leaves me off", async () => { + const d = await draftFor(HERS, "replyAll"); + expect(addrs(d.to)).toEqual([ANN.email]); + expect(addrs(d.cc)).toEqual([BOB.email]); + }); + + it("honours the sender's Reply-To, which is what it is for", async () => { + const d = await draftFor({ ...HERS, replyTo: [{ name: null, email: "desk@example.com" }] } as Email, "reply"); + expect(addrs(d.to)).toEqual(["desk@example.com"]); + }); +}); + +describe("replying to a message I sent", () => { + it("writes to the people I wrote to, not to me", async () => { + const d = await draftFor(MINE, "reply"); + expect(addrs(d.to)).toEqual([ANN.email]); + expect(d.cc).toEqual([]); + }); + + it("reply all keeps my Cc as Cc, rather than promoting me into To", async () => { + const d = await draftFor(MINE, "replyAll"); + expect(addrs(d.to)).toEqual([ANN.email]); + expect(addrs(d.cc)).toEqual([BOB.email]); + }); + + it("does not follow my own Reply-To back to my own desk", async () => { + // The address replies to *me* belong at. My reply is not one of them. + const d = await draftFor({ ...MINE, replyTo: [{ name: null, email: "desk@example.org" }] } as Email, "replyAll"); + expect(addrs(d.to)).toEqual([ANN.email]); + expect(addrs(d.cc)).toEqual([BOB.email]); + }); + + it("leaves me out even when I was a recipient of my own message", async () => { + const d = await draftFor({ ...MINE, to: [ME, ANN] } as Email, "replyAll"); + expect(addrs(d.to)).toEqual([ANN.email]); + expect(addrs(d.cc)).toEqual([BOB.email]); + }); + + it("recognises my address however the identity stored it", async () => { + // A hand-typed identity address can carry whitespace, and comparing + // strings rather than addresses made that enough to break the reply. + const padded = [{ id: "i1", name: "John", email: " John@Example.ORG " }] as unknown as Identity[]; + const d = await draftFor(MINE, "replyAll", { identities: padded }); + expect(addrs(d.to)).toEqual([ANN.email]); + }); +}); + +describe("when the identity list cannot answer", () => { + it("takes a message in Sent as mine, whatever address it went out as", async () => { + // An alias or a shared mailbox the server does not list as an identity. + const alias = inSent({ ...MINE, from: [{ name: "Sales", email: "sales@example.org" }] } as Email); + const d = await draftFor(alias, "replyAll"); + expect(addrs(d.to)).toEqual([ANN.email]); + expect(addrs(d.cc)).toEqual([BOB.email]); + }); + + it("takes a message in Sent as mine before the identities have loaded", async () => { + const d = await draftFor(inSent(MINE), "replyAll", { identities: [] }); + expect(addrs(d.to)).toEqual([ANN.email]); + }); + + it("still replies to the sender of a message that is not mine and not in Sent", async () => { + // The folder signal must not swallow the ordinary case when it is absent. + const d = await draftFor(HERS, "replyAll", { identities: [] }); + expect(addrs(d.to)).toEqual([ANN.email]); + }); +}); + +describe("a message of mine with nobody obvious to reply to", () => { + it("uses the Cc when I addressed it to nobody else", async () => { + const d = await draftFor({ ...MINE, to: [ME] } as Email, "replyAll"); + expect(addrs(d.to)).toEqual([BOB.email]); + expect(d.cc).toEqual([]); + }); + + it("uses the Cc on a plain reply too, rather than leaving To empty", async () => { + const d = await draftFor({ ...MINE, to: [] } as Email, "reply"); + expect(addrs(d.to)).toEqual([BOB.email]); + }); + + it("falls back to my own address rather than a draft addressed to nobody", async () => { + // A note I sent only to myself. Replying to it is odd, and an empty To is + // worse than the only address there was. + const d = await draftFor({ ...MINE, to: [ME], cc: [] } as Email, "reply"); + expect(addrs(d.to)).toEqual([ME.email]); + }); +}); + +describe("forwarding", () => { + it("addresses nobody, whoever sent the message", async () => { + for (const m of [MINE, HERS]) { + const d = await draftFor(m, "forward"); + expect([d.to, d.cc]).toEqual([[], []]); + } + }); +}); diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index 5688713..7776145 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -299,26 +299,50 @@ export const useCompose = create((set, get) => ({ const full = (await mail.getEmails([email.id], true))[0] ?? email; const identities = mail.identities.length ? mail.identities : await mail.loadIdentities(); const ident = defaultIdentity(identities, full); - const ownEmails = identities.map((i) => i.email.toLowerCase()); - const isOwn = (a: EmailAddress) => ownEmails.includes(a.email.toLowerCase()); + const ownEmails = identities.map((i) => i.email); + /* `sameAddress` rather than a lowercased `includes`, because an identity + address can carry whitespace and a hand-typed one does. */ + const isOwn = (a: EmailAddress) => ownEmails.some((e) => sameAddress(e, a.email)); + const withoutOwn = (list: EmailAddress[]) => uniqueAddresses(list).filter((a) => !isOwn(a)); const s = settings(); + /* + * Was this message mine? + * + * The folder answers it before the addresses do, and has to: the address + * test fails in exactly the cases where the mistake is least visible. A + * message sent from an alias or a shared mailbox that `Identity/get` does + * not list is not recognisably mine, and neither is anything at all if the + * identities have not loaded yet -- and the failure is silent, addressing + * the reply back to me with everyone I actually wrote to moved to Cc. + * + * A message in Sent is mine whatever address it went out as. + */ + const sentId = mail.roleId("sent"); + const sentByMe = (Boolean(full.from?.length) && (full.from ?? []).every(isOwn)) + || Boolean(sentId && full.mailboxIds?.[sentId]); + let to: EmailAddress[] = []; let cc: EmailAddress[] = []; if (mode === "reply" || mode === "replyAll") { - const replyTo = full.replyTo?.length ? full.replyTo : (full.from ?? []); - to = uniqueAddresses(replyTo); - if (mode === "replyAll") { - const others = uniqueAddresses([...(full.to ?? []), ...(full.cc ?? [])]).filter((a) => !isOwn(a) && !to.some((t) => sameAddress(t.email, a.email))); - cc = others; - // If the message was sent by me, reply to original recipients instead. - if (to.every(isOwn) && full.to?.length) { - to = uniqueAddresses(full.to); - cc = uniqueAddresses(full.cc ?? []).filter((a) => !isOwn(a)); + if (sentByMe && (full.to?.length || full.cc?.length)) { + /* + * Replying to something I sent continues the conversation with the + * people I wrote to. Not with myself, and not with my own Reply-To + * either -- that address is where replies *to me* belong, and following + * it here would send my own reply to my own desk. + */ + to = withoutOwn(full.to ?? []); + cc = mode === "replyAll" ? withoutOwn(full.cc ?? []) : []; + // Addressed only to myself, or only in Cc: there is still somebody this + // is a reply to, and an empty To is not it. + if (!to.length) { to = cc.length ? cc : withoutOwn(full.cc ?? []); cc = []; } + if (!to.length) to = uniqueAddresses([...(full.to ?? []), ...(full.cc ?? [])]); + } else { + to = uniqueAddresses(full.replyTo?.length ? full.replyTo : (full.from ?? [])); + if (mode === "replyAll") { + cc = uniqueAddresses([...(full.to ?? []), ...(full.cc ?? [])]).filter((a) => !isOwn(a) && !to.some((t) => sameAddress(t.email, a.email))); } - } else if (to.every(isOwn) && full.to?.length) { - to = uniqueAddresses(full.to.filter((a) => !isOwn(a))); - if (!to.length) to = uniqueAddresses(full.to); } }