diff --git a/server/src/mock/index.ts b/server/src/mock/index.ts index 8489058..1ad8454 100644 --- a/server/src/mock/index.ts +++ b/server/src/mock/index.ts @@ -520,6 +520,18 @@ const handlers: Record = { notCreated[cid] = { type: "invalidProperties", properties: ["url"], description: "Push endpoint must be https." }; continue; } + // A filter condition with a null value is not a filter -- the real server + // answers "Invalid filter" and refuses the whole subscription. ihasmail + // shipped `inMailbox: null` meaning "the inbox", which meant nothing at + // all here, and the mock accepted it happily. It does not any more. + const badFilter = Object.entries((o.emailPush ?? {}) as Obj).find(([, cfg]) => { + const f = ((cfg as Obj)?.filter ?? {}) as Obj; + return Object.values(f).some((v) => v === null || v === undefined); + }); + if (badFilter) { + notCreated[cid] = { type: "invalidArguments", properties: ["emailPush"], description: "Invalid filter." }; + continue; + } // One per device: re-subscribing replaces rather than accumulates. const deviceId = String(o.deviceClientId ?? ""); const clash = pushSubscriptions.findIndex((s) => s.deviceClientId === deviceId); diff --git a/web/src/lib/__tests__/webpush.test.ts b/web/src/lib/__tests__/webpush.test.ts index 780977f..40b50f4 100644 --- a/web/src/lib/__tests__/webpush.test.ts +++ b/web/src/lib/__tests__/webpush.test.ts @@ -128,3 +128,50 @@ describe("availability", () => { expect(webPushAvailable()).toBe(false); }); }); + +describe("the emailPush filter", () => { + /** + * This is the bug that reached production: `inMailbox: null` read as "the + * inbox" and meant nothing to the server, which answered "Invalid filter" + * and refused the subscription outright. The original tests checked the + * property ordering and never looked at the filter at all. + */ + const fakeSub = { + endpoint: "https://push.example/abc", + toJSON: () => ({ keys: { p256dh: "cGRoLWtleQ", auth: "YXV0aA" } }), + getKey: () => null, + } as unknown as PushSubscription; + + const withEmailPush = () => { + client.session = session({ + "urn:ietf:params:jmap:webpush-vapid": { applicationServerKey: LIVE_KEY }, + "urn:ietf:params:jmap:emailpush": {}, + }); + }; + + it("never sends a condition with a null or undefined value", () => { + withEmailPush(); + for (const inbox of ["mb1", null]) { + const body = subscriptionPayload(fakeSub, "a1", inbox) as Record; + const filter = body.emailPush.a1.filter as Record; + for (const [k, v] of Object.entries(filter)) { + expect(v, `${k} was ${String(v)} with inbox=${String(inbox)}`).not.toBeNull(); + expect(v, k).not.toBeUndefined(); + } + } + }); + + it("uses the real mailbox id when it knows one", () => { + withEmailPush(); + const body = subscriptionPayload(fakeSub, "a1", "mbInbox") as Record; + expect(body.emailPush.a1.filter.inMailbox).toBe("mbInbox"); + }); + + it("leaves inMailbox out entirely when it does not, rather than sending null", () => { + withEmailPush(); + const filter = (subscriptionPayload(fakeSub, "a1", null) as Record).emailPush.a1.filter; + expect(filter).not.toHaveProperty("inMailbox"); + // Still narrowed to unread: notifying more widely beats not notifying. + expect(filter.notKeyword).toBe("$seen"); + }); +}); diff --git a/web/src/lib/webpush.ts b/web/src/lib/webpush.ts index 11700e5..0432d03 100644 --- a/web/src/lib/webpush.ts +++ b/web/src/lib/webpush.ts @@ -105,8 +105,17 @@ export function deviceClientId(): string { } } -/** What to send Stalwart for a browser subscription. */ -export function subscriptionPayload(sub: PushSubscription, accountId: Id | null): Record { +/** + * What to send Stalwart for a browser subscription. + * + * `inboxId` is the Inbox's mailbox id. It is a parameter rather than something + * looked up here because an `inMailbox` condition needs a real id: the first + * version of this passed `null`, meaning "the inbox" in the author's head and + * nothing at all to the server, which answered "Invalid filter" and refused the + * whole subscription. Without an id the filter simply leaves `inMailbox` out + * and notifies more widely, which is a worse default but a working one. + */ +export function subscriptionPayload(sub: PushSubscription, accountId: Id | null, inboxId: Id | null = null): Record { const json = sub.toJSON(); const body: Record = { deviceClientId: deviceClientId(), @@ -121,7 +130,10 @@ export function subscriptionPayload(sub: PushSubscription, accountId: Id | null) [accountId]: { // Only mail that actually lands in the inbox. Filtering here rather // than in the service worker means spam never leaves the server. - filter: { inMailbox: null, notKeyword: "$seen" }, + // Unread mail only, and only in the Inbox when we know which it is. + // Filtering here rather than in the service worker means spam and + // filed mail never leave the server at all. + filter: { ...(inboxId ? { inMailbox: inboxId } : {}), notKeyword: "$seen" }, properties: PAYLOAD_PROPS, urgency: "normal", }, diff --git a/web/src/lib/webpushEnable.ts b/web/src/lib/webpushEnable.ts index 7285a68..d2f8155 100644 --- a/web/src/lib/webpushEnable.ts +++ b/web/src/lib/webpushEnable.ts @@ -7,6 +7,7 @@ */ import { CAP } from "@/jmap/client"; import { useSession } from "@/store/session"; +import { useMail } from "@/store/mail"; import { applicationServerKey, createSubscription, @@ -78,7 +79,8 @@ export async function enableWebPush(): Promise<{ ok: true } | { ok: false; reaso applicationServerKey: decodeApplicationServerKey(key), })); const accountId = useSession.getState().accountFor(CAP.mail); - await createSubscription(subscriptionPayload(sub, accountId)); + const inboxId = useMail.getState().roleId("inbox"); + await createSubscription(subscriptionPayload(sub, accountId, inboxId)); listenForVerification(); return { ok: true }; } catch (err) {