Merge pull request #72 from LINUXexpert-org/fix-emailpush-filter

Send a filter the server can read

server/src/mock/index.ts

@@ -520,6 +520,18 @@ const handlers: Record<string, Handler> = {
         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);

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<string, any>;
+      const filter = body.emailPush.a1.filter as Record<string, unknown>;
+      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<string, any>;
+    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<string, any>).emailPush.a1.filter;
+    expect(filter).not.toHaveProperty("inMailbox");
+    // Still narrowed to unread: notifying more widely beats not notifying.
+    expect(filter.notKeyword).toBe("$seen");
+  });
+});

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<string, unknown> {
+/**
+ * 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<string, unknown> {
   const json = sub.toJSON();
   const body: Record<string, unknown> = {
     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",
       },

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) {