From 83da7c57c8b609a55e9f1b4d6156f0572eb2c340 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 19 Jul 2026 14:31:25 +0200 Subject: [PATCH] fix(notifications): dial Web Push through the pinned dispatcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The push endpoint host is user-controlled — the browser supplies it at subscribe time. `isPublicUrl` guards it at subscribe time and again before each dial, but it is a literal input-time check and cannot see a DNS rebind between the accept and the connect; with a short-TTL record the dial reaches the pod's private network. `web-push`'s `sendNotification` dials with its own internal `https.request`, which bypassed `safeFetch` and therefore the connect-time pin entirely. Both senders now sign and encrypt through `generateRequestDetails` — identical VAPID handling and payload encryption — and dial through `safeFetch` with `requirePublicHost`. Non-2xx is re-shaped as the `{ statusCode }` rejection the existing classification already understands, so the 410/404 expiry reap and the transient/permanent split are unchanged. This is the last user-controlled-host egress that was not behind the pinned dispatcher (issue #217). --- .../__tests__/web-push-dose-clear.test.ts | 73 +++++++-- .../__tests__/web-push-egress-pin.test.ts | 147 ++++++++++++++++++ .../__tests__/push-attempt-record.test.ts | 30 +++- .../senders/__tests__/urgency.test.ts | 32 ++-- src/lib/notifications/senders/web-push.ts | 38 ++++- src/lib/notifications/web-push-clear.ts | 28 +++- 6 files changed, 316 insertions(+), 32 deletions(-) create mode 100644 src/lib/notifications/__tests__/web-push-egress-pin.test.ts diff --git a/src/lib/notifications/__tests__/web-push-dose-clear.test.ts b/src/lib/notifications/__tests__/web-push-dose-clear.test.ts index 330d30e10..5fd17e260 100644 --- a/src/lib/notifications/__tests__/web-push-dose-clear.test.ts +++ b/src/lib/notifications/__tests__/web-push-dose-clear.test.ts @@ -11,9 +11,24 @@ */ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -const { sendNotificationMock, findManyMock, deleteManyMock } = vi.hoisted( +// safeFetch's requirePublicHost path drives undici's OWN fetch (version-locked +// with its dispatcher). Delegate it to the global fetch these tests stub so the +// existing interception still applies. +vi.mock("undici", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + fetch: (input: unknown, init?: unknown) => + (globalThis.fetch as unknown as (i: unknown, n?: unknown) => unknown)( + input, + init, + ), + }; +}); + +const { generateRequestDetailsMock, findManyMock, deleteManyMock } = vi.hoisted( () => ({ - sendNotificationMock: vi.fn(), + generateRequestDetailsMock: vi.fn(), findManyMock: vi.fn(), deleteManyMock: vi.fn(), }), @@ -61,7 +76,17 @@ vi.mock("@/lib/validations/notifications", () => ({ vi.mock("web-push", () => ({ setVapidDetails: vi.fn(), - sendNotification: (...args: unknown[]) => sendNotificationMock(...args), + // The sender signs + encrypts here and dials through `safeFetch`; the + // payload assertions below read the same second argument as before. + generateRequestDetails: (...args: unknown[]) => { + generateRequestDetailsMock(...args); + return { + endpoint: "https://push.example.com/x", + method: "POST", + headers: {}, + body: null, + }; + }, })); import { sendViaWebPush } from "../senders/web-push"; @@ -76,7 +101,7 @@ const SUB = { }; beforeEach(() => { - sendNotificationMock.mockReset(); + generateRequestDetailsMock.mockReset(); findManyMock.mockReset(); deleteManyMock.mockReset(); deleteManyMock.mockResolvedValue({ count: 0 }); @@ -96,7 +121,9 @@ describe("medicationDoseTag", () => { describe("web-push reminder push — stable tag + badge", () => { it("uses metadata.webPushTag as the notification tag and carries the badge", async () => { findManyMock.mockResolvedValue([SUB]); - sendNotificationMock.mockResolvedValue(undefined); + vi.spyOn(global, "fetch").mockResolvedValue( + new Response(null, { status: 201 }), + ); const tag = medicationDoseTag("med-1", "2026-06-18T07:00:00.000Z"); const res = await sendViaWebPush("user-1", { @@ -108,7 +135,9 @@ describe("web-push reminder push — stable tag + badge", () => { }); expect(res.ok).toBe(true); - const body = JSON.parse(sendNotificationMock.mock.calls[0][1] as string); + const body = JSON.parse( + generateRequestDetailsMock.mock.calls[0][1] as string, + ); expect(body.tag).toBe(tag); expect(body.badge).toBe(2); expect(body.url).toBe("/medications/med-1"); @@ -116,7 +145,9 @@ describe("web-push reminder push — stable tag + badge", () => { it("discreet mode still wins over a stable tag (no event name leak)", async () => { findManyMock.mockResolvedValue([SUB]); - sendNotificationMock.mockResolvedValue(undefined); + vi.spyOn(global, "fetch").mockResolvedValue( + new Response(null, { status: 201 }), + ); await sendViaWebPush("user-1", { eventType: "CYCLE_PERIOD_SOON", @@ -127,7 +158,9 @@ describe("web-push reminder push — stable tag + badge", () => { metadata: { webPushTag: "med:should-not-leak" }, }); - const body = JSON.parse(sendNotificationMock.mock.calls[0][1] as string); + const body = JSON.parse( + generateRequestDetailsMock.mock.calls[0][1] as string, + ); expect(body.tag).toBe("REMINDER"); }); }); @@ -135,7 +168,9 @@ describe("web-push reminder push — stable tag + badge", () => { describe("clear-on-taken push", () => { it("emits type:clear with the SAME slot tag as the reminder + badge", async () => { findManyMock.mockResolvedValue([SUB]); - sendNotificationMock.mockResolvedValue(undefined); + vi.spyOn(global, "fetch").mockResolvedValue( + new Response(null, { status: 201 }), + ); await dispatchMedicationIntakeWebClear({ userId: "user-1", @@ -144,8 +179,10 @@ describe("clear-on-taken push", () => { badgeCount: 1, }); - expect(sendNotificationMock).toHaveBeenCalledTimes(1); - const body = JSON.parse(sendNotificationMock.mock.calls[0][1] as string); + expect(generateRequestDetailsMock).toHaveBeenCalledTimes(1); + const body = JSON.parse( + generateRequestDetailsMock.mock.calls[0][1] as string, + ); expect(body.type).toBe("clear"); expect(body.tag).toBe( medicationDoseTag("med-1", "2026-06-18T07:00:00.000Z"), @@ -155,7 +192,9 @@ describe("clear-on-taken push", () => { it("a count of 0 clears the badge (badge:0 on the wire)", async () => { findManyMock.mockResolvedValue([SUB]); - sendNotificationMock.mockResolvedValue(undefined); + vi.spyOn(global, "fetch").mockResolvedValue( + new Response(null, { status: 201 }), + ); await dispatchMedicationIntakeWebClear({ userId: "user-1", @@ -164,7 +203,9 @@ describe("clear-on-taken push", () => { badgeCount: 0, }); - const body = JSON.parse(sendNotificationMock.mock.calls[0][1] as string); + const body = JSON.parse( + generateRequestDetailsMock.mock.calls[0][1] as string, + ); expect(body.badge).toBe(0); }); @@ -177,12 +218,14 @@ describe("clear-on-taken push", () => { scheduledFor: "2026-06-18T07:00:00.000Z", }), ).resolves.toBeUndefined(); - expect(sendNotificationMock).not.toHaveBeenCalled(); + expect(generateRequestDetailsMock).not.toHaveBeenCalled(); }); it("reaps expired (410) subscriptions", async () => { findManyMock.mockResolvedValue([SUB]); - sendNotificationMock.mockRejectedValue({ statusCode: 410 }); + vi.spyOn(global, "fetch").mockResolvedValue( + new Response(null, { status: 410 }), + ); await dispatchMedicationIntakeWebClear({ userId: "user-1", diff --git a/src/lib/notifications/__tests__/web-push-egress-pin.test.ts b/src/lib/notifications/__tests__/web-push-egress-pin.test.ts new file mode 100644 index 000000000..00885c993 --- /dev/null +++ b/src/lib/notifications/__tests__/web-push-egress-pin.test.ts @@ -0,0 +1,147 @@ +/** + * Web Push egress must run through the pinned dispatcher. + * + * The push endpoint host is USER-CONTROLLED — the browser hands it over at + * subscribe time and the server stores it. `isPublicUrl` is checked both at + * subscribe time and again before each dial, but it is a literal, input-time + * check: it cannot see a DNS rebind between the accept and the connect. With a + * short-TTL record an attacker flips the host to `169.254.169.254` or an + * internal address after the check passes, and the dial reaches the pod's + * private network (issue #217). + * + * `web-push`'s `sendNotification` dials with its own internal `https.request`, + * which bypasses `safeFetch` entirely and therefore bypasses the pinned + * dispatcher. Both senders now sign and encrypt via `generateRequestDetails` + * — identical VAPID + payload handling — and dial through `safeFetch` with + * `requirePublicHost: true`. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const { safeFetchMock, findManyMock, deleteManyMock } = vi.hoisted(() => ({ + safeFetchMock: vi.fn(), + findManyMock: vi.fn(), + deleteManyMock: vi.fn(), +})); + +vi.mock("@/lib/safe-fetch", () => ({ + safeFetch: (...args: unknown[]) => safeFetchMock(...args), +})); + +vi.mock("@/lib/logging/context", () => ({ + getEvent: () => ({ + addWarning: vi.fn(), + addExternalCall: vi.fn(), + setError: vi.fn(), + addMeta: vi.fn(), + }), +})); + +vi.mock("@/lib/notifications/senders/push-attempt-record", () => ({ + recordPushAttempt: vi.fn(), +})); + +vi.mock("@/lib/db", () => ({ + prisma: { + pushSubscription: { findMany: findManyMock, deleteMany: deleteManyMock }, + }, +})); + +vi.mock("@/lib/crypto", () => ({ + decrypt: (v: string) => v, + encrypt: (v: string) => v, +})); + +vi.mock("@/lib/notifications/vapid-config", () => ({ + getVapidConfig: () => + Promise.resolve({ + subject: "mailto:a@b.c", + publicKey: "pub", + privateKey: "priv", + }), +})); + +vi.mock("@/lib/validations/notifications", () => ({ + isPublicUrl: () => true, +})); + +vi.mock("web-push", () => ({ + setVapidDetails: vi.fn(), + generateRequestDetails: () => ({ + endpoint: "https://push.example.com/x", + method: "POST", + headers: { TTL: "60" }, + body: null, + }), +})); + +import { sendViaWebPush } from "../senders/web-push"; +import { dispatchMedicationIntakeWebClear } from "../web-push-clear"; + +const SUB = { + id: "s1", + endpoint: "https://push.example.com/x", + p256dh: "a", + auth: "b", +}; + +beforeEach(() => { + safeFetchMock.mockReset(); + findManyMock.mockReset(); + deleteManyMock.mockReset(); + deleteManyMock.mockResolvedValue({ count: 0 }); + findManyMock.mockResolvedValue([SUB]); + safeFetchMock.mockResolvedValue({ ok: true, status: 201 }); +}); + +function optsOfFirstCall() { + return safeFetchMock.mock.calls[0]?.[2] as + { requirePublicHost?: boolean } | undefined; +} + +describe("sendViaWebPush egress", () => { + it("dials through safeFetch, not web-push's own transport", async () => { + const res = await sendViaWebPush("user-1", { + eventType: "MEDICATION_REMINDER", + userId: "user-1", + title: "T", + message: "M", + }); + expect(res.ok).toBe(true); + expect(safeFetchMock).toHaveBeenCalledTimes(1); + }); + + it("pins the connect-time host check with requirePublicHost", async () => { + await sendViaWebPush("user-1", { + eventType: "MEDICATION_REMINDER", + userId: "user-1", + title: "T", + message: "M", + }); + expect(optsOfFirstCall()?.requirePublicHost).toBe(true); + }); + + it("maps a 410 onto the expiry-reap path", async () => { + safeFetchMock.mockResolvedValue({ ok: false, status: 410 }); + await sendViaWebPush("user-1", { + eventType: "MEDICATION_REMINDER", + userId: "user-1", + title: "T", + message: "M", + }); + expect(deleteManyMock).toHaveBeenCalledWith({ + where: { id: { in: ["s1"] } }, + }); + }); +}); + +describe("dispatchMedicationIntakeWebClear egress", () => { + it("dials through safeFetch with requirePublicHost too", async () => { + await dispatchMedicationIntakeWebClear({ + userId: "user-1", + medicationId: "med-1", + scheduledFor: "2026-06-18T07:00:00.000Z", + }); + expect(safeFetchMock).toHaveBeenCalledTimes(1); + expect(optsOfFirstCall()?.requirePublicHost).toBe(true); + }); +}); diff --git a/src/lib/notifications/senders/__tests__/push-attempt-record.test.ts b/src/lib/notifications/senders/__tests__/push-attempt-record.test.ts index b2e5b7cb8..eda7a445d 100644 --- a/src/lib/notifications/senders/__tests__/push-attempt-record.test.ts +++ b/src/lib/notifications/senders/__tests__/push-attempt-record.test.ts @@ -38,7 +38,7 @@ const hoisted = vi.hoisted(() => ({ apnsSendMock: vi.fn(), apnsShutdownMock: vi.fn(), webPushSetVapidDetailsMock: vi.fn(), - webPushSendNotificationMock: vi.fn(), + webPushGenerateRequestDetailsMock: vi.fn(), sendTelegramMessageMock: vi.fn(), deleteTelegramMessageMock: vi.fn(), })); @@ -111,13 +111,23 @@ vi.mock("@/lib/notifications/vapid-config", () => ({ }), })); +// The sender signs + encrypts via `generateRequestDetails` and dials through +// `safeFetch` (pinned dispatcher) rather than letting web-push run its own +// `https.request`, so the dial is interceptable by the global fetch stub. +const webPushRequestDetails = () => ({ + endpoint: "https://example.test/push", + method: "POST", + headers: {}, + body: null, +}); + vi.mock("web-push", () => ({ default: { setVapidDetails: hoisted.webPushSetVapidDetailsMock, - sendNotification: hoisted.webPushSendNotificationMock, + generateRequestDetails: hoisted.webPushGenerateRequestDetailsMock, }, setVapidDetails: hoisted.webPushSetVapidDetailsMock, - sendNotification: hoisted.webPushSendNotificationMock, + generateRequestDetails: hoisted.webPushGenerateRequestDetailsMock, })); vi.mock("@/lib/telegram", () => ({ @@ -330,7 +340,12 @@ describe("recordPushAttempt — WEB_PUSH sender", () => { auth: "enc(a)", }, ] as never); - hoisted.webPushSendNotificationMock.mockResolvedValue(undefined); + hoisted.webPushGenerateRequestDetailsMock.mockReturnValue( + webPushRequestDetails(), + ); + vi.spyOn(global, "fetch").mockResolvedValue( + new Response(null, { status: 201 }), + ); const outcome = await sendViaWebPush("user-1", { userId: "user-1", @@ -379,7 +394,12 @@ describe("recordPushAttempt — WEB_PUSH sender", () => { auth: "enc(a)", }, ] as never); - hoisted.webPushSendNotificationMock.mockResolvedValue(undefined); + hoisted.webPushGenerateRequestDetailsMock.mockReturnValue( + webPushRequestDetails(), + ); + vi.spyOn(global, "fetch").mockResolvedValue( + new Response(null, { status: 201 }), + ); vi.mocked(prisma.pushAttempt.create).mockRejectedValue( new Error("connection refused"), ); diff --git a/src/lib/notifications/senders/__tests__/urgency.test.ts b/src/lib/notifications/senders/__tests__/urgency.test.ts index 4ca535299..e23a15f44 100644 --- a/src/lib/notifications/senders/__tests__/urgency.test.ts +++ b/src/lib/notifications/senders/__tests__/urgency.test.ts @@ -11,12 +11,12 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const { safeFetchMock, recordPushAttemptMock, - sendNotificationMock, + generateRequestDetailsMock, findManyMock, } = vi.hoisted(() => ({ safeFetchMock: vi.fn(), recordPushAttemptMock: vi.fn(), - sendNotificationMock: vi.fn(), + generateRequestDetailsMock: vi.fn(), findManyMock: vi.fn(), })); @@ -61,7 +61,19 @@ vi.mock("@/lib/validations/notifications", () => ({ vi.mock("web-push", () => ({ setVapidDetails: vi.fn(), - sendNotification: (...args: unknown[]) => sendNotificationMock(...args), + // The sender now signs + encrypts via `generateRequestDetails` and dials + // through `safeFetch` (pinned dispatcher) instead of letting web-push run + // its own `https.request`. Same three arguments, so the urgency assertions + // below are unchanged. + generateRequestDetails: (...args: unknown[]) => { + generateRequestDetailsMock(...args); + return { + endpoint: "https://push.example.com/x", + method: "POST", + headers: {}, + body: null, + }; + }, })); import { sendViaNtfy } from "../ntfy"; @@ -81,7 +93,7 @@ function payload(over?: Record) { beforeEach(() => { safeFetchMock.mockReset(); recordPushAttemptMock.mockReset(); - sendNotificationMock.mockReset(); + generateRequestDetailsMock.mockReset(); findManyMock.mockReset(); }); @@ -164,9 +176,9 @@ describe("web-push urgent mapping", () => { auth: "b", }, ]); - sendNotificationMock.mockResolvedValue(undefined); + safeFetchMock.mockResolvedValue({ ok: true, status: 201 }); await sendViaWebPush("user-1", payload({ urgent: true })); - const [, body, options] = sendNotificationMock.mock.calls[0]; + const [, body, options] = generateRequestDetailsMock.mock.calls[0]; expect((options as { urgency?: string }).urgency).toBe("high"); expect(JSON.parse(body as string).requireInteraction).toBe(true); }); @@ -180,9 +192,9 @@ describe("web-push urgent mapping", () => { auth: "b", }, ]); - sendNotificationMock.mockResolvedValue(undefined); + safeFetchMock.mockResolvedValue({ ok: true, status: 201 }); await sendViaWebPush("user-1", payload()); - const [, body, options] = sendNotificationMock.mock.calls[0]; + const [, body, options] = generateRequestDetailsMock.mock.calls[0]; expect(options).toBeUndefined(); expect(JSON.parse(body as string).requireInteraction).toBe(false); }); @@ -201,7 +213,6 @@ describe("APNs-less instance degrades gracefully", () => { auth: "b", }, ]); - sendNotificationMock.mockResolvedValue(undefined); const ntfy = await sendViaNtfy( { serverUrl: "https://ntfy.example.com", topic: "t" }, @@ -216,7 +227,8 @@ describe("APNs-less instance degrades gracefully", () => { .headers.Priority, ).toBe("5"); expect( - (sendNotificationMock.mock.calls[0][2] as { urgency?: string }).urgency, + (generateRequestDetailsMock.mock.calls[0][2] as { urgency?: string }) + .urgency, ).toBe("high"); }); }); diff --git a/src/lib/notifications/senders/web-push.ts b/src/lib/notifications/senders/web-push.ts index 8a63a4e04..84b6a25fa 100644 --- a/src/lib/notifications/senders/web-push.ts +++ b/src/lib/notifications/senders/web-push.ts @@ -7,6 +7,7 @@ import { getVapidConfig } from "@/lib/notifications/vapid-config"; import { getEvent } from "@/lib/logging/context"; import { recordPushAttempt } from "@/lib/notifications/senders/push-attempt-record"; import { isPublicUrl } from "@/lib/validations/notifications"; +import { safeFetch } from "@/lib/safe-fetch"; import { plainPushText } from "@/lib/notifications/strip-emoji"; /** @@ -154,14 +155,49 @@ export async function sendViaWebPush( const p256dh = decrypt(sub.p256dh); const auth = decrypt(sub.auth); - await webpush.sendNotification( + // The dial itself goes through `safeFetch` with `requirePublicHost`, + // not through `web-push`'s own internal `https.request`. The literal + // `isPublicUrl` check above is an input-time floor and cannot see a + // DNS rebind between the check and the connect; the pinned dispatcher + // vets every address the resolver actually returns (issue #217). + // `generateRequestDetails` performs the identical VAPID signing and + // payload encryption `sendNotification` would have — only the + // transport changes. + const details = webpush.generateRequestDetails( { endpoint: sub.endpoint, keys: { p256dh, auth }, }, pushPayload, sendOptions, + ) as { + endpoint: string; + method: string; + headers: Record; + body: Buffer | string | null; + }; + + const pushRes = await safeFetch( + details.endpoint, + { + method: details.method, + headers: details.headers, + // `generateRequestDetails` hands back a Node Buffer for an + // encrypted payload; widen it to the BodyInit the fetch API takes. + body: (details.body ?? undefined) as BodyInit | undefined, + }, + { requirePublicHost: true }, ); + + if (pushRes.status < 200 || pushRes.status >= 300) { + // Re-shape as the `{ statusCode }` rejection the classification + // below already understands, so the 410/404 expiry path and the + // transient/permanent split stay byte-for-byte unchanged. + throw Object.assign( + new Error(`Web Push responded ${pushRes.status}`), + { statusCode: pushRes.status }, + ); + } anySuccess = true; allPermanentReject = false; } catch (err: unknown) { diff --git a/src/lib/notifications/web-push-clear.ts b/src/lib/notifications/web-push-clear.ts index 7a3d4539d..5936cc8ce 100644 --- a/src/lib/notifications/web-push-clear.ts +++ b/src/lib/notifications/web-push-clear.ts @@ -22,6 +22,7 @@ import { decrypt } from "@/lib/crypto"; import { getVapidConfig } from "@/lib/notifications/vapid-config"; import { getEvent } from "@/lib/logging/context"; import { isPublicUrl } from "@/lib/validations/notifications"; +import { safeFetch } from "@/lib/safe-fetch"; import { medicationDoseTag } from "@/lib/notifications/dose-tag"; export interface WebClearInput { @@ -80,10 +81,35 @@ export async function dispatchMedicationIntakeWebClear( } const p256dh = decrypt(sub.p256dh); const auth = decrypt(sub.auth); - await webpush.sendNotification( + // Same transport swap as the main sender: sign + encrypt via + // `generateRequestDetails`, then dial through `safeFetch` with the + // pinned dispatcher so a DNS rebind between the literal + // `isPublicUrl` check above and the connect cannot reach a private + // address (issue #217). + const details = webpush.generateRequestDetails( { endpoint: sub.endpoint, keys: { p256dh, auth } }, clearPayload, + ) as { + endpoint: string; + method: string; + headers: Record; + body: Buffer | string | null; + }; + const clearRes = await safeFetch( + details.endpoint, + { + method: details.method, + headers: details.headers, + body: (details.body ?? undefined) as BodyInit | undefined, + }, + { requirePublicHost: true }, ); + if (clearRes.status < 200 || clearRes.status >= 300) { + throw Object.assign( + new Error(`Web Push clear responded ${clearRes.status}`), + { statusCode: clearRes.status }, + ); + } } catch (err: unknown) { const status = (err as { statusCode?: number }).statusCode; if (status === 410 || status === 404) expiredIds.push(sub.id);