From 2263aa494f02cfc432b119b19c867d2eadab5168 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Wed, 26 Aug 2026 14:01:56 -0700 Subject: [PATCH] Send a filter the server can read Enabling background notifications failed with "Invalid filter". The subscription asked to be notified about mail matching: filter: { inMailbox: null, notKeyword: "$seen" } `inMailbox: null` meant "the inbox" in my head and nothing at all to Stalwart, which needs a mailbox id there. It refused the whole subscription, so the feature did not work at all for anyone who tried it. The Inbox's id is now passed in and used. Where it is not known the condition is left out rather than sent empty: notifying more widely is a worse default than filtering to the Inbox, but it is a working one, and sending a malformed filter is not a fallback. Two reasons this got out, both worth fixing rather than just the bug: - The tests checked the properties list and its ordering, and never looked at the filter. There is now one that walks every condition and fails on a null or undefined value, for both the known-inbox and unknown-inbox cases. - The mock accepted it happily, so nothing local disagreed with the code. It now refuses a filter condition with a null value and answers "Invalid filter.", which is what the live server said. Reproduced: the old payload is rejected, the new one accepted. --- server/src/mock/index.ts | 12 +++++++ web/src/lib/__tests__/webpush.test.ts | 47 +++++++++++++++++++++++++++ web/src/lib/webpush.ts | 18 ++++++++-- web/src/lib/webpushEnable.ts | 4 ++- 4 files changed, 77 insertions(+), 4 deletions(-) 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) {