From 8be51d9beb4b93faa1ebe5326a6ed2cf959aba0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 05:41:17 +0200 Subject: [PATCH 1/4] test(e2e): cover notification preferences through to the dispatch decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One browser journey for the web arm of notification delivery: an account configures its channels on the settings surface, a due medication reminder runs through the product's own sweep, and the dispatch decision is read back off that account's `push_attempts` ledger via the admin diagnostic. What it pins: - the enabled channel delivers (`result: ok`) and the message really leaves the process, asserted against a local SMTP responder rather than against the ledger row alone; - a channel that is configured and switched OFF is never attempted, and its own test control is inert on the surface; - the APNs arm is attempted and skipped, and the ledger's reason distinguishes the transport refusing (`apns_not_configured`) from the client-managed opt-in refusing (`client_managed`) — with email still delivering under the opt-in, which is the published "APNs leg only" contract; - an ntfy relay URL inside a private range is refused with 422, leaves the stored config untouched, and writes no attempt. Email is the channel exercised for real because it is the only one that can be: every account-supplied delivery target crosses the SSRF floor (`isPublicUrl` plus `safeFetch({ requirePublicHost: true })` with a connect-time address pin), so ntfy, the generic webhook and Web Push refuse a local stub by design, and Telegram and APNs address hard-coded third parties. Email's transport is operator configuration, so `e2e/setup/smtp-stub.ts` can stand in for it end to end. The config points the e2e server at that stub. The journey carries one skipped test at full strength: the medication detail surface decides between the reminder switch and the client-managed chip on `notificationPrefs.medication.clientManaged` read out of `/api/auth/me`, and that payload does not carry the field. The flag therefore reads as false for every account and the chip never renders, so a person whose phone owns the reminders is still shown a server-side switch that no longer decides anything. The write path and the dispatch decision are both correct; only the web reflection is missing its end. The account and its jar are seeded in global setup, and its channels, devices, ledger and preferences are reset before every test so the counts are about the run in front of them. --- e2e/notification-dispatch-journey.spec.ts | 617 ++++++++++++++++++++++ e2e/setup/global-setup.ts | 60 ++- e2e/setup/notification-fixture.ts | 102 ++++ e2e/setup/smtp-stub.ts | 133 +++++ playwright.config.ts | 22 + 5 files changed, 932 insertions(+), 2 deletions(-) create mode 100644 e2e/notification-dispatch-journey.spec.ts create mode 100644 e2e/setup/notification-fixture.ts create mode 100644 e2e/setup/smtp-stub.ts diff --git a/e2e/notification-dispatch-journey.spec.ts b/e2e/notification-dispatch-journey.spec.ts new file mode 100644 index 000000000..aceff5319 --- /dev/null +++ b/e2e/notification-dispatch-journey.spec.ts @@ -0,0 +1,617 @@ +/** + * Notification preferences → dispatch decision, the web arm — one journey. + * + * The flow this file is checked against, in one script: + * + * 1. an account configures two delivery channels on the settings surface — + * email switched ON with a recipient, ntfy saved with a public relay and + * left OFF; + * 2. it pairs a phone, so the APNs arm exists in the cascade; + * 3. a medication whose dose window closed hours ago is created through the + * page's own fetch, and the reminder sweep the admin panel drives runs; + * 4. the dispatch decision is read back off the account's own delivery + * ledger: one email attempt, delivered; nothing for the channel that is + * switched off; the APNs arm attempted and skipped with its reason; + * 5. the account then opts into client-managed medication reminders — the + * iOS contract — and the same sweep skips the APNs arm for THAT reason + * while email still delivers, which is the whole claim of the opt-in. + * + * Refusing controls: a channel switched off is never attempted and its own + * test control is inert; and an ntfy relay URL inside a private range is + * refused by the save, leaves the stored config untouched, and writes no + * delivery attempt. + * + * Why email is the channel that gets exercised for real, and why nothing else + * could be: every account-supplied delivery target crosses the SSRF floor + * (`isPublicUrl` at save time, `safeFetch({ requirePublicHost: true })` with a + * connect-time address pin at send time), so ntfy, the generic webhook and Web + * Push all refuse a local stub by design; Telegram and APNs address hard-coded + * third parties. Email's transport is OPERATOR configuration, so it never + * crosses that floor — see `e2e/setup/smtp-stub.ts`. The APNs arm therefore + * cannot deliver here, and the journey does not pretend it can: what it proves + * about APNs is the DECISION, read from the reason the ledger carries. + * + * Every assertion addresses a stable attribute — `data-slot`, `data-state`, + * a form-control id — or the account's own API responses, never rendered copy: + * the labels are i18n-driven and this account renders them in English only by + * cookie. + */ +import type { Page } from "@playwright/test"; + +import { NOTIFY_STORAGE_STATE_PATH } from "./setup/global-setup"; +import { resetNotificationFixture } from "./setup/notification-fixture"; +import { startSmtpStub, type SmtpStub } from "./setup/smtp-stub"; +import { expect, test } from "./setup/test"; + +/** The panel has finished its read and the channel cards are mounted. */ +const CHANNELS_PANEL = '[data-slot="notification-channels-panel"]'; + +/** Where the account's email notifications are addressed. */ +const EMAIL_RECIPIENT = "notification-journey@healthlog.test"; + +/** + * A public relay hostname for the channel that stays switched OFF. It is + * never dialled — the dispatcher drops a disabled channel before any I/O, and + * that is precisely the verdict this journey wants — but it has to pass + * `isPublicUrl` for the save to succeed at all. + */ +const NTFY_SERVER = "https://ntfy.example.com"; +const NTFY_TOPIC = "healthlog-e2e-notify"; + +/** RFC1918. The save refuses it; the refusal is the point. */ +const PRIVATE_NTFY_SERVER = "http://192.168.31.7"; + +/** + * A 64-character hex APNs token. Hex only — the registration route refuses + * anything else — and this spec's alone, because a token belongs to exactly + * one account and re-registering it under another is a 409. + */ +const APNS_TOKEN = `e2e0d15c0de${"0".repeat(53)}`; + +/** The medication whose window has closed, so the sweep has something to send. */ +const MEDICATION_NAME = "E2E Notification Journey"; + +interface PushAttempt { + channel: string; + eventType: string; + result: string; + reason: string | null; +} + +interface DiagnosticPayload { + notificationChannels: Array<{ + type: string; + enabled: boolean; + configPresent: boolean; + }>; + recentPushAttempts: PushAttempt[]; +} + +/** + * Every fetch below rides the PAGE's own `fetch` rather than a shared + * Playwright request context: a pooled context can hand a POST to a keep-alive + * socket the server has already closed, and the retry lands as a second write. + * It also means the cookie, the origin and the CSRF posture are the browser's, + * which is the arm this journey is about. + */ +async function fromPage( + page: Page, + path: string, + init: { method?: string; body?: unknown } = {}, +): Promise<{ status: number; data: T | null }> { + return page.evaluate( + async (call: { path: string; method: string; body: string | null }) => { + const res = await fetch(call.path, { + method: call.method, + credentials: "include", + ...(call.body === null + ? {} + : { + headers: { "Content-Type": "application/json" }, + body: call.body, + }), + }); + const envelope = (await res.json()) as { data: unknown }; + return { status: res.status, data: envelope.data }; + }, + { + path, + method: init.method ?? "GET", + body: init.body === undefined ? null : JSON.stringify(init.body), + }, + ) as Promise<{ status: number; data: T | null }>; +} + +/** The admin notification diagnostic — channels and the trailing ledger. */ +async function readDiagnostic(page: Page): Promise { + const res = await fromPage( + page, + "/api/admin/notifications/diagnostic", + ); + expect(res.status, "the diagnostic answered the admin cookie").toBe(200); + if (!res.data) throw new Error("diagnostic returned no data"); + return res.data; +} + +/** + * The delivery attempts written since `before` rows existed. + * + * The ledger only ever grows and the diagnostic returns it newest-first, so + * the head of the list is exactly what this dispatch produced. The wait is a + * poll rather than a sleep because the ledger write is fire-and-forget: the + * trigger's response can return before the row lands. + * + * `expected` is asserted, not merely awaited — an EXTRA attempt is the failure + * this journey exists to catch (a channel that was switched off delivering + * anyway), and a poll that stops at "at least" would never see it. + */ +async function attemptsSince( + page: Page, + before: number, + expected: number, +): Promise { + await expect + .poll(async () => (await readDiagnostic(page)).recentPushAttempts.length, { + message: `the dispatch wrote ${expected} delivery attempt(s)`, + timeout: 20_000, + }) + .toBe(before + expected); + + const all = (await readDiagnostic(page)).recentPushAttempts; + expect( + all.length, + "no further attempt arrived after the ledger settled", + ).toBe(before + expected); + return all.slice(0, expected); +} + +/** How many attempts the ledger already holds. */ +async function attemptCount(page: Page): Promise { + return (await readDiagnostic(page)).recentPushAttempts.length; +} + +/** + * Open the delivery-channels surface and wait until each card has taken its + * seed from the server. + * + * The wait is not cosmetic. Both cards mount BEFORE their query resolves and + * then copy the server's value into the input once it arrives — the React + * sync-from-server shape, no effect involved. A value typed into the input + * before that copy runs is silently replaced by the server's, so the save that + * follows persists an empty field and the next request is refused for a reason + * that points nowhere near the test. Waiting for the two reads is what makes + * the copy already done rather than pending. + */ +async function openChannels(page: Page): Promise { + const seeded = ["/api/settings/email", "/api/settings/ntfy"].map((endpoint) => + page.waitForResponse( + (res) => + new URL(res.url()).pathname === endpoint && + res.request().method() === "GET", + ), + ); + await page.goto("/settings/integrations", { waitUntil: "domcontentloaded" }); + await expect(page.locator(CHANNELS_PANEL)).toBeVisible({ timeout: 30_000 }); + await Promise.all(seeded); +} + +/** + * Type into a channel card's field and confirm the field kept it. The + * confirmation is the second half of the seed race described on + * `openChannels`: the read has landed, so nothing is left to overwrite it. + */ +async function fillField( + page: Page, + selector: string, + value: string, +): Promise { + await page.locator(selector).fill(value); + await expect(page.locator(selector)).toHaveValue(value); +} + +/** + * Save one channel card's form and hand back the status the settings route + * answered with. Gated on the card's own PUT rather than on a repaint, so a + * refusal is read from the wire instead of inferred from what did not change. + */ +async function saveCard( + page: Page, + anchor: string, + endpoint: string, +): Promise { + const settled = page.waitForResponse( + (res) => + new URL(res.url()).pathname === endpoint && + res.request().method() === "PUT", + ); + await page + .locator( + `${anchor} [data-slot="settings-card-actions"] button[type=submit]`, + ) + .click(); + return (await settled).status(); +} + +/** Flip a channel card's enable switch and wait for the route to answer. */ +async function toggleCard( + page: Page, + switchId: string, + endpoint: string, +): Promise { + const settled = page.waitForResponse( + (res) => + new URL(res.url()).pathname === endpoint && + res.request().method() === "PUT", + ); + await page.locator(switchId).click(); + return (await settled).status(); +} + +/** + * Configure both channels the way a person would: email on with a recipient, + * ntfy saved and left off. Everything here happens on the settings surface. + */ +async function configureChannels(page: Page): Promise { + await openChannels(page); + + await fillField(page, "#email-recipient", EMAIL_RECIPIENT); + expect(await saveCard(page, "#email", "/api/settings/email")).toBe(200); + expect(await toggleCard(page, "#email-toggle", "/api/settings/email")).toBe( + 200, + ); + await expect(page.locator("#email-toggle")).toHaveAttribute( + "data-state", + "checked", + ); + + await fillField(page, "#ntfy-server", NTFY_SERVER); + await fillField(page, "#ntfy-topic", NTFY_TOPIC); + expect(await saveCard(page, "#ntfy", "/api/settings/ntfy")).toBe(200); + await expect(page.locator("#ntfy-toggle")).toHaveAttribute( + "data-state", + "unchecked", + ); +} + +/** + * Pair a phone through the device-registration route the native client uses. + * That registration — and nothing else in the product — is what puts an APNS + * row in the cascade, so the arm the client-managed opt-in suppresses only + * exists once this has run. + */ +async function pairPhone(page: Page): Promise { + const res = await fromPage(page, "/api/devices", { + method: "POST", + body: { + token: APNS_TOKEN, + bundleId: "dev.healthlog.e2e", + apnsToken: APNS_TOKEN, + apnsEnvironment: "sandbox", + }, + }); + expect(res.status, "the phone registered").toBe(201); +} + +/** + * Pin the account's timezone so its local clock reads midday. + * + * The sweep classifies a dose window against the account's OWN wall clock, so + * a window written as a fixed pair of times is overdue for most of the day and + * not yet open around midnight. Rather than leave a spec that is honest 1439 + * minutes out of 1440, the account sets the one preference that decides which + * clock is consulted — through its own profile route, the same PATCH the + * profile form sends — to a whole-hour zone where "now" is early afternoon. + * The morning window below is then unambiguously closed, every run. + */ +async function pinAccountClockToMidday(page: Page): Promise { + const wrapped = (12 - new Date().getUTCHours() + 24) % 24; + const offsetHours = wrapped > 12 ? wrapped - 24 : wrapped; + // `Etc/GMT-5` is UTC+5 — the POSIX sign convention is inverted, which is + // why the zone name is built rather than looked up. + const zone = + offsetHours === 0 + ? "Etc/GMT" + : offsetHours > 0 + ? `Etc/GMT-${offsetHours}` + : `Etc/GMT+${-offsetHours}`; + + const res = await fromPage<{ timezone: string }>(page, "/api/user/profile", { + method: "PATCH", + body: { timezone: zone }, + }); + expect(res.status, "the profile accepted the timezone").toBe(200); + expect(res.data?.timezone).toBe(zone); +} + +/** + * A medication with a morning window and reminders on. With the account's + * clock pinned to early afternoon the window is hours closed, which is the + * state the sweep dispatches for. + */ +async function createOverdueMedication(page: Page): Promise { + const res = await fromPage<{ id: string }>(page, "/api/medications", { + method: "POST", + body: { + name: MEDICATION_NAME, + dose: "1 tablet", + notificationsEnabled: true, + schedules: [{ windowStart: "08:00", windowEnd: "09:00" }], + }, + }); + expect(res.status, "the medication was created").toBe(201); + const id = res.data?.id; + expect(id, "the create returned an id").toBeTruthy(); + return id as string; +} + +interface SweepResult { + medications: Array<{ + name: string; + schedules: Array<{ notificationSent: boolean }>; + }>; +} + +/** + * Run the reminder sweep the admin notifications panel drives. This is the + * product's own trigger for the medication-reminder dispatch, and it goes + * through `dispatchNotification` — the cascade, the per-channel preference + * read and the client-managed gate — rather than calling a sender directly. + */ +async function runReminderSweep(page: Page): Promise { + const res = await fromPage( + page, + "/api/admin/notifications/reminder-check", + { method: "POST" }, + ); + expect(res.status, "the sweep ran").toBe(200); + const mine = res.data?.medications.find((m) => m.name === MEDICATION_NAME); + expect(mine, "the sweep saw this account's overdue medication").toBeTruthy(); + expect( + mine?.schedules.some((s) => s.notificationSent), + "the sweep dispatched for the closed window", + ).toBe(true); +} + +/** Look one channel's attempt out of a dispatch's rows. */ +function attemptFor( + attempts: PushAttempt[], + channel: string, +): PushAttempt | undefined { + return attempts.find((a) => a.channel === channel); +} + +test.describe("notification preferences drive the dispatch decision", () => { + test.use({ storageState: NOTIFY_STORAGE_STATE_PATH }); + + /** + * Serial, because every test configures the one account's channels and then + * counts rows in its ledger. Two of these in flight would be counting each + * other's deliveries. + */ + test.describe.configure({ mode: "serial" }); + + /** + * The sweep walks every active medication on the instance and the ledger + * write that follows is fire-and-forget, so the budget covers a loaded + * runner rather than the 30 s default. + */ + test.setTimeout(120_000); + + let smtp: SmtpStub; + + test.beforeAll(async () => { + smtp = await startSmtpStub(); + }); + + test.afterAll(async () => { + await smtp.close(); + }); + + test.beforeEach(async () => { + await resetNotificationFixture(); + smtp.reset(); + }); + + test("the card's own test control delivers on the enabled channel only", async ({ + page, + }) => { + await configureChannels(page); + + // The diagnostic agrees with the surface about which channel is on. Both + // reads are the account's own; neither is the switch it just clicked. + const channels = (await readDiagnostic(page)).notificationChannels; + expect( + channels.find((c) => c.type === "EMAIL"), + "the email channel is configured and on", + ).toMatchObject({ enabled: true, configPresent: true }); + expect( + channels.find((c) => c.type === "NTFY"), + "the ntfy channel is configured and off", + ).toMatchObject({ enabled: false, configPresent: true }); + + // A channel that is off offers no way to probe it — the refusal is on the + // surface, before any request is made. + await expect( + page.locator( + '#ntfy [data-slot="settings-card-actions"] button[type=button]', + ), + ).toBeDisabled(); + + const before = await attemptCount(page); + const probe = page.waitForResponse( + (res) => new URL(res.url()).pathname === "/api/settings/email/test", + ); + await page + .locator('#email [data-slot="settings-card-actions"] button[type=button]') + .click(); + expect((await probe).status(), "the probe reached the transport").toBe(200); + + const attempts = await attemptsSince(page, before, 1); + expect(attempts[0]).toMatchObject({ + channel: "EMAIL", + result: "ok", + reason: null, + }); + + // And the transport is a real one: the message left the process and was + // accepted for the address the settings surface saved. + expect(smtp.accepted()).toEqual([EMAIL_RECIPIENT]); + }); + + test("a due reminder delivers on the enabled channel and skips the rest", async ({ + page, + }) => { + await configureChannels(page); + await pairPhone(page); + await pinAccountClockToMidday(page); + await createOverdueMedication(page); + + const before = await attemptCount(page); + await runReminderSweep(page); + + // Two arms, and exactly two: the paired phone and the enabled email + // channel. A third row would mean the switched-off relay was dialled. + const attempts = await attemptsSince(page, before, 2); + expect( + attempts.map((a) => a.channel).sort(), + "only the channels in the cascade were attempted", + ).toEqual(["APNS", "EMAIL"]); + + expect(attemptFor(attempts, "EMAIL")).toMatchObject({ + eventType: "MEDICATION_REMINDER", + result: "ok", + reason: null, + }); + expect(smtp.accepted()).toEqual([EMAIL_RECIPIENT]); + + // The APNs arm was reached and refused by the TRANSPORT, not by a + // preference — this instance has no APNs credentials. That distinction is + // the baseline the next test moves. + expect(attemptFor(attempts, "APNS")).toMatchObject({ + eventType: "MEDICATION_REMINDER", + result: "skipped", + reason: "apns_not_configured", + }); + }); + + test("the client-managed opt-in suppresses the APNs arm and nothing else", async ({ + page, + }) => { + await configureChannels(page); + await pairPhone(page); + await pinAccountClockToMidday(page); + await createOverdueMedication(page); + + // The opt-in the native client sets when it owns the local reminder. + const patched = await fromPage<{ medication: { clientManaged: boolean } }>( + page, + "/api/auth/me/notification-prefs", + { method: "PATCH", body: { medication: { clientManaged: true } } }, + ); + expect(patched.status, "the preference was accepted").toBe(200); + expect(patched.data?.medication.clientManaged).toBe(true); + + // Read back through the account's own resolved preferences, not through + // the value just echoed: the deep-merge is what the dispatcher consults. + const resolved = await fromPage<{ medication: { clientManaged: boolean } }>( + page, + "/api/auth/me/notification-prefs", + ); + expect(resolved.data?.medication.clientManaged).toBe(true); + + const before = await attemptCount(page); + await runReminderSweep(page); + + const attempts = await attemptsSince(page, before, 2); + + // Suppressed BEFORE the sender: the reason is the gate's, not the + // transport's. Same instance, same missing credentials as the previous + // test — only the preference moved, and the ledger says so. + expect(attemptFor(attempts, "APNS")).toMatchObject({ + result: "skipped", + reason: "client_managed", + }); + + // "The APNs leg only" is the published contract, so email still has to + // deliver. An opt-in that silenced the whole dispatch would pass every + // APNs assertion above and be the bug. + expect(attemptFor(attempts, "EMAIL")).toMatchObject({ + eventType: "MEDICATION_REMINDER", + result: "ok", + reason: null, + }); + expect(smtp.accepted()).toEqual([EMAIL_RECIPIENT]); + }); + + test("the client-managed opt-in is visible on the medication it silences", async ({ + page, + }) => { + test.skip( + true, + "The chip exists and is unreachable. " + + "`src/components/medications/sections/notifications-section.tsx` " + + "decides between the reminder switch and the client-managed chip on " + + "`notificationPrefs.medication.clientManaged` read out of " + + "`/api/auth/me` — a field that payload does not carry (the prefs live " + + "on `/api/auth/me/notification-prefs`). So the flag reads as false for " + + "every account, the chip never renders, and a person whose phone owns " + + "the reminders is shown a server-side switch that no longer decides " + + "anything. The write path and the dispatch decision are correct — the " + + "two tests above prove both — only the web reflection is missing its " + + "end. The account-payload guard cannot see this: it asks that every " + + "field in the payload has a reader, and this is a reader with no " + + "field. Kept at full strength rather than relaxed to match today's " + + "render.", + ); + + await configureChannels(page); + await pinAccountClockToMidday(page); + const medicationId = await createOverdueMedication(page); + + const patched = await fromPage(page, "/api/auth/me/notification-prefs", { + method: "PATCH", + body: { medication: { clientManaged: true } }, + }); + expect(patched.status).toBe(200); + + await page.goto(`/medications/${medicationId}`, { + waitUntil: "domcontentloaded", + }); + await page.locator('[data-slot="medication-tab-zeitplan"]').click(); + await expect( + page.locator('[data-slot="notifications-client-managed-chip"]'), + ).toBeVisible({ timeout: 30_000 }); + await expect( + page.locator('[data-slot="notifications-switch-row"]'), + ).toHaveCount(0); + }); + + test("a relay URL inside a private range is refused and delivers nothing", async ({ + page, + }) => { + await configureChannels(page); + + const before = await attemptCount(page); + await fillField(page, "#ntfy-server", PRIVATE_NTFY_SERVER); + expect( + await saveCard(page, "#ntfy", "/api/settings/ntfy"), + "the save refused the private host", + ).toBe(422); + + // A refusal that still wrote something would be worse than no refusal: + // the stored relay is the one the account saved before. + const stored = await fromPage<{ serverUrl: string; enabled: boolean }>( + page, + "/api/settings/ntfy", + ); + expect(stored.status).toBe(200); + expect(stored.data).toMatchObject({ + serverUrl: NTFY_SERVER, + enabled: false, + }); + + // And nothing was dispatched on the way to being refused. + expect(await attemptCount(page)).toBe(before); + expect(smtp.accepted()).toEqual([]); + }); +}); diff --git a/e2e/setup/global-setup.ts b/e2e/setup/global-setup.ts index 9ee0f42ec..bd0f24e99 100644 --- a/e2e/setup/global-setup.ts +++ b/e2e/setup/global-setup.ts @@ -93,6 +93,31 @@ export const E2E_GUARDIAN = { role: "USER", } as const; +/** + * The notification-dispatch journey's own account. + * + * It has one, rather than borrowing the shared fixture, because the journey's + * verdicts are counts over ONE account's `push_attempts` ledger — "one email + * attempt, no ntfy attempt, the APNs arm skipped and for which reason". Any + * other spec that happened to trigger a delivery for the shared account would + * land inside that window and flip a count that is not about it. + * + * ADMIN because the two surfaces the journey reads the dispatch decision + * through — the reminder trigger and the notification diagnostic — are + * `requireAdmin()`, which is cookie-only by construction. + */ +export const E2E_NOTIFY = { + email: "e2e-notify@healthlog.test", + username: "e2e-notify", + password: "Xd6!Ct3pW9qBz5Fh", + role: "ADMIN", +} as const; + +export const NOTIFY_STORAGE_STATE_PATH = resolve( + process.cwd(), + "e2e/setup/storageStateNotify.json", +); + /** A separate delegate session for the eight scoped-record browser journeys. */ export const E2E_SCOPE_DELEGATE = { email: "e2e-scope-delegate@healthlog.test", @@ -408,6 +433,33 @@ export default async function globalSetup(config: FullConfig): Promise { [E2E_GUARDIAN.username], ); + // The notification-dispatch journey's account. Seeded like the others; + // its channels, devices, ledger rows and preferences are reset by + // `e2e/setup/notification-fixture.ts` before every test, because the + // journey has to be re-runnable and `--repeat-each` is exactly the case a + // once-per-suite reset does not cover. + await pool.query( + `INSERT INTO users + (id, username, email, password_hash, role, created_at, updated_at, + onboarding_completed_at, onboarding_tour_completed) + VALUES ($1, $2, $3, $4, $5, $6, $6, $6, true) + ON CONFLICT (username) DO UPDATE SET + email = EXCLUDED.email, + password_hash = EXCLUDED.password_hash, + role = EXCLUDED.role, + updated_at = EXCLUDED.updated_at, + onboarding_completed_at = EXCLUDED.onboarding_completed_at, + onboarding_tour_completed = EXCLUDED.onboarding_tour_completed`, + [ + cuid(), + E2E_NOTIFY.username, + E2E_NOTIFY.email, + await hashPassword(E2E_NOTIFY.password), + E2E_NOTIFY.role, + now, + ], + ); + // The scoped-record journey uses a dedicated delegate so its preseeded // grants cannot affect the invitation lifecycle exercised by the sharing // journey above. Its own module preferences are explicitly off: a target @@ -647,7 +699,7 @@ export default async function globalSetup(config: FullConfig): Promise { WHERE key LIKE 'sharing:%'`, ); - // The login bucket, for the same reason. This setup signs in TEN times now + // The login bucket, for the same reason. This setup signs in ELEVEN times now // (the shared jar, the owner, and one jar apiece for every spec that moves // a session's record selector), and the ceiling is five attempts per IP per // quarter-hour — so two local runs in a row would otherwise end with a 429 @@ -727,7 +779,7 @@ export default async function globalSetup(config: FullConfig): Promise { * Log one account in, clearing the login bucket first. * * The ceiling is FIVE attempts per IP per quarter-hour and this setup now - * signs in ten times, so clearing once before the batch is no longer + * signs in eleven times, so clearing once before the batch is no longer * enough — the sixth would be answered by the fixture's own 429 rather * than by the product. Only the auth surfaces' buckets are touched, and * only between logins this setup is itself performing. @@ -753,6 +805,10 @@ export default async function globalSetup(config: FullConfig): Promise { // for what happens when the journey switches the shared row instead. await capture(E2E_USER, DELEGATE_STORAGE_STATE_PATH); + // The notification-dispatch journey's jar. Its own account, so its own + // login — see `E2E_NOTIFY` for why the ledger counts need one. + await capture(E2E_NOTIFY, NOTIFY_STORAGE_STATE_PATH); + await capture(E2E_SCOPE_DELEGATE, SCOPE_DELEGATE_STORAGE_STATE_PATH); await capture(E2E_SCOPE_DELEGATE, SCOPE_A11Y_STORAGE_STATE_PATH); await capture(E2E_SCOPE_DELEGATE, SCOPE_CAPABILITIES_STORAGE_STATE_PATH); diff --git a/e2e/setup/notification-fixture.ts b/e2e/setup/notification-fixture.ts new file mode 100644 index 000000000..23363b05a --- /dev/null +++ b/e2e/setup/notification-fixture.ts @@ -0,0 +1,102 @@ +/** + * Account hygiene for the notification-dispatch journey. + * + * Every verdict in that journey is a COUNT over one account's delivery ledger + * inside a window the test opens itself — one email attempt, no ntfy attempt, + * one APNs skip carrying a named reason. A channel row, a paired device, a + * medication or a preference blob left behind by an earlier run would put a + * second dispatch decision inside that window, and the count would be reading + * the previous run rather than this one. `--repeat-each` is the case that + * makes it certain rather than likely. + * + * So the account is put back to "nothing configured" before every test: + * no channels, no devices, no ledger, no medications, default preferences and + * the default timezone. Deletes rather than disables — a disabled channel row + * is itself one of the journey's subjects, so leaving one behind would seed + * the answer to a question the test asks. + * + * Reaching for Postgres directly is the channel `global-setup.ts` already + * uses to seed and to clear buckets, and every statement here is parameter + * bound on the account's own id. + */ +import pg from "pg"; + +import { E2E_NOTIFY } from "./global-setup"; + +async function withPool(fn: (pool: pg.Pool) => Promise): Promise { + const url = process.env.DATABASE_URL; + if (!url) throw new Error("[notification-fixture] DATABASE_URL is not set"); + const pool = new pg.Pool({ connectionString: url }); + try { + return await fn(pool); + } finally { + await pool.end(); + } +} + +/** The notification account's id, or a loud failure if the seed never ran. */ +export async function notificationAccountId(): Promise { + return withPool(async (pool) => { + const res = await pool.query<{ id: string }>( + "SELECT id FROM users WHERE username = $1", + [E2E_NOTIFY.username], + ); + const id = res.rows[0]?.id; + if (!id) { + throw new Error( + "[notification-fixture] e2e-notify is not seeded — global-setup must run first", + ); + } + return id; + }); +} + +/** + * Put the notification account back to a freshly-registered state. + * + * The rate-limit buckets go too. The card's own test control allows five + * probes per five minutes and device registration twenty per quarter-hour — + * production numbers, and both below what three repeats of this journey + * spend. A run that ended on the fixture's 429 would say nothing about the + * dispatcher. + */ +export async function resetNotificationFixture(): Promise { + await withPool(async (pool) => { + const res = await pool.query<{ id: string }>( + "SELECT id FROM users WHERE username = $1", + [E2E_NOTIFY.username], + ); + const userId = res.rows[0]?.id; + if (!userId) { + throw new Error( + "[notification-fixture] e2e-notify is not seeded — global-setup must run first", + ); + } + + await pool.query("DELETE FROM notification_channels WHERE user_id = $1", [ + userId, + ]); + await pool.query("DELETE FROM devices WHERE user_id = $1", [userId]); + await pool.query( + `DELETE FROM push_attempts + WHERE user_id = $1 OR recipient_user_id = $1 OR record_user_id = $1`, + [userId], + ); + await pool.query("DELETE FROM medications WHERE user_id = $1", [userId]); + await pool.query( + `UPDATE users + SET notification_prefs = NULL, + timezone = DEFAULT + WHERE id = $1`, + [userId], + ); + await pool.query("DELETE FROM rate_limits WHERE key = ANY($1)", [ + [ + `email-test:${userId}`, + `ntfy-test:${userId}`, + `devices:register:${userId}`, + `notification-prefs:patch:${userId}`, + ], + ]); + }); +} diff --git a/e2e/setup/smtp-stub.ts b/e2e/setup/smtp-stub.ts new file mode 100644 index 000000000..d640cc492 --- /dev/null +++ b/e2e/setup/smtp-stub.ts @@ -0,0 +1,133 @@ +/** + * A local SMTP responder, and the reason the notification journey needs one. + * + * Every user-configurable DELIVERY channel in this product refuses a local + * destination on purpose. ntfy and the generic webhook take their URL from the + * account, so the URL passes `isPublicUrl()` at save time and the send goes + * through `safeFetch({ requirePublicHost: true })`, which pins the resolved + * address at connect time; loopback, RFC1918 and every alternate notation are + * refused at both ends. Web Push is the same shape one layer down — the + * subscription endpoint is checked before the dial. Telegram and APNs address + * hard-coded third parties. So none of them can be pointed at a stub, and + * faking one would prove the fixture rather than the dispatcher. + * + * Email is the exception, and not by oversight: its transport is OPERATOR + * configuration (`SMTP_HOST` / `SMTP_PORT` / `SMTP_FROM`), never account + * input, so it never crosses the SSRF floor that guards the account-supplied + * hosts. That makes it the one channel a self-hoster — and this suite — can + * exercise end to end on one machine, through the real sender, the real + * transport and the real `push_attempts` ledger write. + * + * The responder below speaks the minimum of RFC 5321 that nodemailer needs: + * a greeting, EHLO with no advertised extensions (so nodemailer neither + * pipelines nor tries STARTTLS), the envelope commands, and DATA. It stores + * nothing but the envelope recipients, which is all the journey reads back. + */ +import { createServer, type Server, type Socket } from "node:net"; + +/** + * Port the stub listens on, and the port `playwright.config.ts` hands the app + * server as `SMTP_PORT`. Named here so the config and the spec cannot drift. + */ +export const SMTP_STUB_PORT = 3925; + +export const SMTP_STUB_HOST = "127.0.0.1"; + +/** The envelope sender the app is configured with (`SMTP_FROM`). */ +export const SMTP_STUB_FROM = "healthlog-e2e@healthlog.test"; + +export interface SmtpStub { + /** Envelope recipients of every message the stub accepted, oldest first. */ + accepted(): readonly string[]; + /** Forget everything accepted so far. */ + reset(): void; + close(): Promise; +} + +const CRLF = "\r\n"; + +/** Start the responder and resolve once it is accepting connections. */ +export async function startSmtpStub(): Promise { + const accepted: string[] = []; + + const server: Server = createServer((socket: Socket) => { + let buffer = ""; + let inData = false; + let envelope: string[] = []; + + const say = (line: string): void => { + socket.write(`${line}${CRLF}`); + }; + + socket.setEncoding("utf8"); + say("220 healthlog-e2e-smtp ready"); + + socket.on("data", (chunk: string) => { + buffer += chunk; + let index = buffer.indexOf(CRLF); + while (index !== -1) { + const line = buffer.slice(0, index); + buffer = buffer.slice(index + CRLF.length); + + if (inData) { + // A bare "." on its own line ends the message body. + if (line === ".") { + inData = false; + accepted.push(...envelope); + envelope = []; + say("250 2.0.0 Ok: queued"); + } + } else { + const verb = line.slice(0, 4).toUpperCase(); + if (verb === "EHLO" || verb === "HELO") { + // Single-line, no extensions: nodemailer then sends one command + // at a time and never attempts STARTTLS or AUTH. + say("250 healthlog-e2e-smtp"); + } else if (verb === "MAIL") { + say("250 2.1.0 Ok"); + } else if (verb === "RCPT") { + const match = /<([^>]*)>/.exec(line); + if (match) envelope.push(match[1]); + say("250 2.1.5 Ok"); + } else if (verb === "DATA") { + inData = true; + say("354 End data with ."); + } else if (verb === "QUIT") { + say("221 2.0.0 Bye"); + socket.end(); + } else if (verb === "RSET") { + envelope = []; + say("250 2.0.0 Ok"); + } else { + say("250 2.0.0 Ok"); + } + } + + index = buffer.indexOf(CRLF); + } + }); + + // A client that vanishes mid-exchange is not this stub's problem, and an + // unhandled 'error' on a socket would take the whole runner down. + socket.on("error", () => socket.destroy()); + }); + + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(SMTP_STUB_PORT, SMTP_STUB_HOST, () => { + server.removeListener("error", reject); + resolve(); + }); + }); + + return { + accepted: () => accepted, + reset: () => { + accepted.length = 0; + }, + close: () => + new Promise((resolve) => { + server.close(() => resolve()); + }), + }; +} diff --git a/playwright.config.ts b/playwright.config.ts index 4bd5961d6..b2c689661 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -4,6 +4,11 @@ import { SSR_PREFETCH_BASE_URL, SSR_PREFETCH_PORT, } from "./e2e/setup/ssr-prefetch-server"; +import { + SMTP_STUB_FROM, + SMTP_STUB_HOST, + SMTP_STUB_PORT, +} from "./e2e/setup/smtp-stub"; /** * Playwright configuration for the HealthLog E2E suite. @@ -171,6 +176,14 @@ export default defineConfig({ // through stable data attributes, not a mobile layout, so it runs in // one project. "apple-health-import.spec.ts", + // The notification-dispatch journey binds a local SMTP responder on a + // fixed port and reads GLOBAL verdicts off one account's delivery + // ledger — "one email attempt, no ntfy attempt, the APNs arm skipped + // for this reason". A second project would fight it for the port and + // add attempts to the window it counts. It proves a dispatch decision + // through stable attributes and the account's own API, not a mobile + // layout, so it runs in one project. + "notification-dispatch-journey.spec.ts", ], use: { // Pixel 5 — Chromium-based mobile profile so CI only needs @@ -210,6 +223,15 @@ export default defineConfig({ // Disable the prefetch for the e2e server — the suite keeps the // deterministic client-fetch path. DASHBOARD_SSR_PREFETCH: "false", + // The one delivery channel a notification journey can exercise + // for real on one machine. Its transport is operator config, not + // account input, so it never crosses the SSRF floor that refuses + // a local host for ntfy / webhook / Web Push. The responder these + // point at is started by the journey itself; see + // `e2e/setup/smtp-stub.ts` for why no other channel can be. + SMTP_HOST: SMTP_STUB_HOST, + SMTP_PORT: String(SMTP_STUB_PORT), + SMTP_FROM: SMTP_STUB_FROM, }, }, // The SHIPPED configuration, on its own port. From f3321b803111e2918191f5b1651938b3e2b84079 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 06:00:27 +0200 Subject: [PATCH 2/4] feat(api): let the admin reminder sweep name one account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /api/admin/notifications/reminder-check` walked every active medication on the instance and dispatched a reminder for each overdue slot it found. That is the operator button's intended behaviour and it stays the default, but it left any other caller — a browser journey, a smoke check — writing into every account's delivery ledger at once with no way to say otherwise. The body now accepts an optional `userId` that narrows the sweep. It can only narrow: the route is `requireAdmin()`, cookie-only by construction, so a Bearer caller never reaches it however wide its scope, and the selector feeds the Prisma `where` rather than the authorisation. A bodyless POST — what the admin panel sends — is unchanged. An unknown key is a 422 instead of a silently instance-wide run. Published in the contract alongside the notification diagnostic, so the scope is readable rather than folded into the console-only exemption. --- docs/api/openapi.yaml | 170 ++++++++++++++++++ .../openapi-route-coverage-guard.test.ts | 4 - .../reminder-check/__tests__/route.test.ts | 120 +++++++++++++ .../notifications/reminder-check/route.ts | 55 +++++- src/lib/openapi/routes/admin.ts | 85 +++++++++ src/lib/validations/notifications.ts | 22 +++ 6 files changed, 447 insertions(+), 9 deletions(-) create mode 100644 src/app/api/admin/notifications/reminder-check/__tests__/route.test.ts diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index d5b69b924..e2aaf39bf 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -9712,6 +9712,50 @@ paths: $ref: "#/components/schemas/ErrorEnvelope" "422": *a4 "429": *a5 + /api/admin/notifications/reminder-check: + post: + tags: + - Admin + summary: Run the medication-reminder sweep + description: >- + The admin panel's manual reminder sweep: it classifies every active medication's dose windows against the OWNING + account's wall clock and dispatches a MEDICATION_REMINDER for each overdue slot that has no intake event today. + Cookie auth only — `requireAdmin()` refuses every Bearer caller, wildcard scope included. + + + Scope is the thing to get right. With no body the sweep is INSTANCE-WIDE and dispatches into every account that + has an overdue dose and a configured channel; that is the operator button's behaviour and it is deliberate. A + caller that means to sweep one account sends `userId` and stops being a second writer for every other one. The + selector can only narrow: it feeds the Prisma `where`, never the authorisation. + requestBody: + required: false + content: + application/json: + schema: + type: object + properties: + userId: + type: string + minLength: 1 + maxLength: 64 + pattern: ^[A-Za-z0-9_-]+$ + additionalProperties: false + responses: + "200": + description: The sweep's findings and how many reminders it sent. + content: + application/json: + schema: + $ref: "#/components/schemas/AdminReminderCheckResponse" + "401": *a1 + "403": + description: Caller is not an admin. + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" + "422": *a4 + "429": *a5 /api/admin/notifications/diagnostic: get: tags: @@ -48690,6 +48734,132 @@ components: - data - error additionalProperties: false + AdminReminderCheckResponse: + type: object + properties: + data: + $ref: "#/components/schemas/AdminReminderCheckResult" + error: + type: "null" + meta: + type: object + properties: + requestId: + type: string + additionalProperties: false + required: + - data + - error + additionalProperties: false + AdminReminderCheckResult: + type: object + properties: + timestamp: + type: string + format: date-time + pattern: ^(?:(?:\d\d[2468][048]|\d\d[13579][26]|\d\d0[48]|[02468][048]00|[13579][26]00)-02-29|\d{4}-(?:(?:0[13578]|1[02])-(?:0[1-9]|[12]\d|3[01])|(?:0[469]|11)-(?:0[1-9]|[12]\d|30)|(?:02)-(?:0[1-9]|1\d|2[0-8])))T(?:(?:[01]\d|2[0-3]):[0-5]\d(?::[0-5]\d(?:\.\d+)?)?(?:Z|([+-](?:[01]\d|2[0-3]):[0-5]\d)))$ + missedThresholdMinutes: + type: integer + minimum: -9007199254740991 + maximum: 9007199254740991 + scoped: + type: boolean + description: True when the request named a `userId` and the sweep therefore walked one account only. + medications: + type: array + items: + $ref: "#/components/schemas/AdminReminderMedication" + notificationsSent: + type: integer + minimum: -9007199254740991 + maximum: 9007199254740991 + message: + type: string + required: + - timestamp + - missedThresholdMinutes + - scoped + - medications + - notificationsSent + - message + additionalProperties: false + description: What the sweep found and what it sent. `notificationsSent` counts DISPATCHES, not deliveries — each one + enters the channel cascade and the per-channel verdict lands in the `push_attempts` ledger the notification + diagnostic surfaces. + AdminReminderMedication: + type: object + properties: + name: + type: string + dose: + type: string + user: + type: string + timezone: + type: string + localTime: + type: string + dayOfWeek: + type: string + notificationsEnabled: + type: boolean + schedules: + type: array + items: + $ref: "#/components/schemas/AdminReminderScheduleStatus" + eventsToday: + type: integer + minimum: -9007199254740991 + maximum: 9007199254740991 + required: + - name + - dose + - user + - timezone + - localTime + - dayOfWeek + - notificationsEnabled + - schedules + - eventsToday + additionalProperties: false + description: One active medication the sweep walked, with the account it belongs to and that account's local clock — the + sweep classifies every window against the owner's timezone, never the operator's. + AdminReminderScheduleStatus: + type: object + properties: + window: + type: string + days: + type: string + status: + type: string + enum: + - open + - late + - threshold + - missed + - skipped + label: + type: string + minutesPastEnd: + anyOf: + - type: integer + minimum: -9007199254740991 + maximum: 9007199254740991 + - type: "null" + notificationSent: + type: boolean + required: + - window + - days + - status + - label + - minutesPastEnd + - notificationSent + additionalProperties: false + description: One dose window as the sweep classified it against the OWNING account's wall clock, and whether this run + dispatched for it. `notificationSent` is false for a window that is still open, for a day the schedule does not + cover, and for a slot that already has an intake event today. AdminDiagnosticResponse: type: object properties: diff --git a/src/__tests__/openapi-route-coverage-guard.test.ts b/src/__tests__/openapi-route-coverage-guard.test.ts index 82480859c..013e83d16 100644 --- a/src/__tests__/openapi-route-coverage-guard.test.ts +++ b/src/__tests__/openapi-route-coverage-guard.test.ts @@ -260,10 +260,6 @@ const UNPUBLISHED: Readonly> = { }, "/api/admin/provider-health": { kind: "adminConsole", methods: ["GET"] }, "/api/admin/notifications/health": { kind: "adminConsole", methods: ["GET"] }, - "/api/admin/notifications/reminder-check": { - kind: "adminConsole", - methods: ["POST"], - }, "/api/admin/notifications/test": { kind: "adminConsole", methods: ["POST"] }, "/api/admin/rollups/recompute": { kind: "adminConsole", methods: ["POST"] }, "/api/admin/settings": { kind: "adminConsole", methods: ["GET", "PUT"] }, diff --git a/src/app/api/admin/notifications/reminder-check/__tests__/route.test.ts b/src/app/api/admin/notifications/reminder-check/__tests__/route.test.ts new file mode 100644 index 000000000..25d5a54c1 --- /dev/null +++ b/src/app/api/admin/notifications/reminder-check/__tests__/route.test.ts @@ -0,0 +1,120 @@ +/** + * The admin reminder sweep's SCOPE, which is the half of this route that is + * easy to state backwards. + * + * The route dispatches. Every overdue slot it finds enters the channel cascade + * and reaches whoever owns the medication, so an unscoped run is a writer into + * every account on the instance at once — not a reader that somebody else might + * disturb. These cases pin both arms: no body means the long-standing + * instance-wide sweep, and a named account means a `where` that cannot reach + * anyone else. + * + * Mutation check: drop the `userId` spread from the route's `where` and the + * second case fails on the filter it no longer carries. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +vi.mock("@/lib/api-handler", async () => { + const actual = + await vi.importActual( + "@/lib/api-handler", + ); + return { + ...actual, + apiHandler: Promise>( + h: T, + ): T => h, + requireAdmin: vi.fn(), + }; +}); + +vi.mock("@/lib/logging/context", () => ({ + annotate: vi.fn(), + getEvent: vi.fn(() => null), +})); + +vi.mock("@/lib/db", () => ({ + prisma: { + appSettings: { findUnique: vi.fn() }, + medication: { findMany: vi.fn() }, + medicationIntakeEvent: { count: vi.fn() }, + }, +})); + +vi.mock("@/lib/notifications/dispatch-localised", () => ({ + dispatchLocalisedNotification: vi.fn(async () => undefined), +})); + +import { POST } from "../route"; +import { requireAdmin } from "@/lib/api-handler"; +import { prisma } from "@/lib/db"; + +const ADMIN_CTX = { + authMethod: "cookie" as const, + session: { id: "s1", expiresAt: new Date(Date.now() + 3_600_000) }, + user: { id: "admin-1", username: "admin", role: "ADMIN" } as never, +}; + +const URL_ = "http://localhost/api/admin/notifications/reminder-check"; + +/** The admin console's own call: no body, no content-type. */ +function bare(): NextRequest { + return new NextRequest(URL_, { method: "POST" }); +} + +function withBody(body: unknown): NextRequest { + return new NextRequest(URL_, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); +} + +function whereOfLastFindMany(): Record { + const call = vi.mocked(prisma.medication.findMany).mock.calls.at(-1); + return (call?.[0] as { where: Record }).where; +} + +beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(requireAdmin).mockResolvedValue(ADMIN_CTX); + vi.mocked(prisma.appSettings.findUnique).mockResolvedValue({ + reminderMissedMinutes: 240, + } as never); + vi.mocked(prisma.medication.findMany).mockResolvedValue([] as never); + vi.mocked(prisma.medicationIntakeEvent.count).mockResolvedValue(0 as never); +}); + +describe("POST /api/admin/notifications/reminder-check", () => { + it("sweeps the whole instance when no account is named", async () => { + const res = await POST(bare()); + expect(res.status).toBe(200); + const json = (await res.json()) as { data: { scoped: boolean } }; + expect(json.data.scoped).toBe(false); + expect(whereOfLastFindMany()).toEqual({ active: true }); + }); + + it("narrows the sweep to the named account", async () => { + const res = await POST(withBody({ userId: "cku0mock0account0id" })); + expect(res.status).toBe(200); + const json = (await res.json()) as { data: { scoped: boolean } }; + expect(json.data.scoped).toBe(true); + expect(whereOfLastFindMany()).toEqual({ + active: true, + userId: "cku0mock0account0id", + }); + }); + + it("refuses an unknown body key rather than sweeping everything", async () => { + const res = await POST(withBody({ userid: "cku0mock0account0id" })); + expect(res.status).toBe(422); + expect(prisma.medication.findMany).not.toHaveBeenCalled(); + }); + + it("refuses an account selector that is not an id", async () => { + const res = await POST(withBody({ userId: "../../etc/passwd" })); + expect(res.status).toBe(422); + expect(prisma.medication.findMany).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/admin/notifications/reminder-check/route.ts b/src/app/api/admin/notifications/reminder-check/route.ts index 784b7d4dd..0124e0c89 100644 --- a/src/app/api/admin/notifications/reminder-check/route.ts +++ b/src/app/api/admin/notifications/reminder-check/route.ts @@ -1,7 +1,10 @@ +import type { NextRequest } from "next/server"; + import { prisma } from "@/lib/db"; import { apiHandler, requireAdmin } from "@/lib/api-handler"; import { annotate, getEvent } from "@/lib/logging/context"; -import { apiSuccess } from "@/lib/api-response"; +import { apiSuccess, returnAllZodIssues, safeJson } from "@/lib/api-response"; +import { adminReminderCheckSchema } from "@/lib/validations/notifications"; import { parseScheduleRecurrence } from "@/lib/medication-schedule"; import { dispatchLocalisedNotification } from "@/lib/notifications/dispatch-localised"; import { getUserTodayBounds, getDayOfWeekInTz } from "@/lib/tz/local-day"; @@ -40,13 +43,50 @@ interface MedicationResult { } /** - * POST: Execute the reminder check — analyzes all medications AND sends + * POST: Execute the reminder check — analyses medications AND sends * notifications for overdue schedules (late + missed). Returns detailed * results for display in the admin panel. + * + * ## Scope + * + * The sweep is instance-wide by default: no body, every active medication on + * the instance, one dispatch per overdue slot. That is the operator button's + * long-standing behaviour and it stays. + * + * An optional `userId` in the body narrows it to ONE account. The direction + * that matters is easy to state backwards, so it is written here: the hazard a + * caller of this route creates is not that somebody else writes into its + * window, it is that THIS call dispatches into everybody else's — a run reaches + * every account that has an overdue dose and a configured channel. A caller + * that only means to sweep one account names it and stops being a second + * writer for every other one. + * + * Naming an account cannot widen anything. `requireAdmin()` is cookie-only by + * construction, so a Bearer token — wildcard scope included — never reaches + * this handler at all, and the field only ever removes rows from the `where`. */ -export const POST = apiHandler(async () => { +export const POST = apiHandler(async (request: NextRequest) => { await requireAdmin(); - annotate({ action: { name: "admin.notifications.reminder-check" } }); + + // A bodyless POST is the admin console's own call (`apiPost(path)` sends no + // body and no content-type), so the parse only runs when a JSON body is + // actually presented. An empty object is the same instance-wide sweep. + let input: unknown = {}; + if ( + (request.headers.get("content-type") ?? "").includes("application/json") + ) { + const { data, error } = await safeJson(request, { maxBytes: 4 * 1024 }); + if (error) return error; + input = data ?? {}; + } + const parsed = adminReminderCheckSchema.safeParse(input); + if (!parsed.success) return returnAllZodIssues(parsed.error); + const scopedUserId = parsed.data.userId ?? null; + + annotate({ + action: { name: "admin.notifications.reminder-check" }, + meta: { scope: scopedUserId ? "account" : "instance" }, + }); const now = new Date(); @@ -57,7 +97,11 @@ export const POST = apiHandler(async () => { const missedMinutes = appSettings?.reminderMissedMinutes ?? 240; const medications = await prisma.medication.findMany({ - where: { active: true }, + where: { + active: true, + // Absent selector ⇒ no `userId` key ⇒ the instance-wide sweep. + ...(scopedUserId ? { userId: scopedUserId } : {}), + }, include: { schedules: true, user: { select: { id: true, username: true, timezone: true } }, @@ -211,6 +255,7 @@ export const POST = apiHandler(async () => { return apiSuccess({ timestamp: now.toISOString(), missedThresholdMinutes: missedMinutes, + scoped: scopedUserId !== null, medications: results, notificationsSent, message: diff --git a/src/lib/openapi/routes/admin.ts b/src/lib/openapi/routes/admin.ts index e2e013a03..1e764c178 100644 --- a/src/lib/openapi/routes/admin.ts +++ b/src/lib/openapi/routes/admin.ts @@ -8,6 +8,7 @@ import { z } from "zod/v4"; import type { ZodOpenApiObject } from "zod-openapi"; import { inviteCreateSchema } from "@/lib/validations/invite"; +import { adminReminderCheckSchema } from "@/lib/validations/notifications"; import { dataEnvelope, errorEnvelope, stdResponses } from "./shared"; // v1.4.48 H-APNs-1 — admin diagnostic endpoint for the notification @@ -190,7 +191,91 @@ export const adminInvitePaths: NonNullable = { }, }; +const adminReminderSchedule = z + .object({ + window: z.string(), + days: z.string(), + status: z.enum(["open", "late", "threshold", "missed", "skipped"]), + label: z.string(), + minutesPastEnd: z.number().int().nullable(), + notificationSent: z.boolean(), + }) + .meta({ + id: "AdminReminderScheduleStatus", + description: + "One dose window as the sweep classified it against the OWNING account's wall clock, and whether this run dispatched for it. `notificationSent` is false for a window that is still open, for a day the schedule does not cover, and for a slot that already has an intake event today.", + }); + +const adminReminderMedication = z + .object({ + name: z.string(), + dose: z.string(), + user: z.string(), + timezone: z.string(), + localTime: z.string(), + dayOfWeek: z.string(), + notificationsEnabled: z.boolean(), + schedules: z.array(adminReminderSchedule), + eventsToday: z.number().int(), + }) + .meta({ + id: "AdminReminderMedication", + description: + "One active medication the sweep walked, with the account it belongs to and that account's local clock — the sweep classifies every window against the owner's timezone, never the operator's.", + }); + +const adminReminderCheckResult = z + .object({ + timestamp: z.iso.datetime({ offset: true }), + missedThresholdMinutes: z.number().int(), + scoped: z + .boolean() + .describe( + "True when the request named a `userId` and the sweep therefore walked one account only.", + ), + medications: z.array(adminReminderMedication), + notificationsSent: z.number().int(), + message: z.string(), + }) + .meta({ + id: "AdminReminderCheckResult", + description: + "What the sweep found and what it sent. `notificationsSent` counts DISPATCHES, not deliveries — each one enters the channel cascade and the per-channel verdict lands in the `push_attempts` ledger the notification diagnostic surfaces.", + }); + export const adminDiagnosticPaths: NonNullable = { + "/api/admin/notifications/reminder-check": { + post: { + tags: ["Admin"], + summary: "Run the medication-reminder sweep", + description: + "The admin panel's manual reminder sweep: it classifies every active medication's dose windows against the OWNING account's wall clock and dispatches a MEDICATION_REMINDER for each overdue slot that has no intake event today. Cookie auth only — `requireAdmin()` refuses every Bearer caller, wildcard scope included.\n\nScope is the thing to get right. With no body the sweep is INSTANCE-WIDE and dispatches into every account that has an overdue dose and a configured channel; that is the operator button's behaviour and it is deliberate. A caller that means to sweep one account sends `userId` and stops being a second writer for every other one. The selector can only narrow: it feeds the Prisma `where`, never the authorisation.", + requestBody: { + required: false, + content: { + "application/json": { schema: adminReminderCheckSchema }, + }, + }, + responses: { + "200": { + description: "The sweep's findings and how many reminders it sent.", + content: { + "application/json": { + schema: dataEnvelope( + adminReminderCheckResult, + "AdminReminderCheckResponse", + ), + }, + }, + }, + "403": { + description: "Caller is not an admin.", + content: { "application/json": { schema: errorEnvelope } }, + }, + ...stdResponses, + }, + }, + }, "/api/admin/notifications/diagnostic": { get: { tags: ["Admin"], diff --git a/src/lib/validations/notifications.ts b/src/lib/validations/notifications.ts index f5aeca557..f7d1e0c36 100644 --- a/src/lib/validations/notifications.ts +++ b/src/lib/validations/notifications.ts @@ -347,3 +347,25 @@ export const webPushSubscriptionSchema = z.object({ auth: z.string().min(1), }), }); + +/** + * Body of `POST /api/admin/notifications/reminder-check` — the admin panel's + * manual reminder sweep. + * + * `userId` narrows the sweep to one account. Omitted (and a bodyless POST is + * the admin console's own call) the sweep stays instance-wide, which is what + * the operator button has always done. The field is an ACCOUNT SELECTOR, not + * an identity: the route is `requireAdmin()`, cookie-only by construction, so + * naming an account here can never widen who is allowed to run the sweep — it + * only narrows whose overdue doses it dispatches for. + * + * Strict, so a typo'd key is a 422 rather than a silently instance-wide run. + */ +export const adminReminderCheckSchema = z.strictObject({ + userId: z + .string() + .min(1) + .max(64) + .regex(/^[A-Za-z0-9_-]+$/, "Ungültige Account-ID") + .optional(), +}); From fd6fa18913f755d99bfe3596203bb48a5dbecd39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 06:04:39 +0200 Subject: [PATCH 3/4] feat(api): publish the resolved notification preferences on the account payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The medication detail's notification section decides between the server-side reminder switch and the client-managed chip on `notificationPrefs.medication.clientManaged`, read off `GET /api/auth/me`. That payload never carried the field — the preferences live on their own endpoint, which is not on the app-boot path — so the flag was `undefined` for every account: the chip never rendered, and a person whose phone owns the reminders was shown a switch that no longer decides anything. The write path and the dispatch gate were correct the whole time; only the web reflection was missing its end. The payload now carries the fully-resolved block from the same resolver the dedicated endpoint uses, so the two cannot disagree, and an account that never opted in reads `false` rather than absent. No extra query: the session already loads the row the column sits on. The pair guard over this payload gained the direction that could not see it. It asked that every published field has a reader; a reader with no field was invisible. It now also reads the response types clients declare for this endpoint and holds them against what the route publishes. A typed guard was tried first and does not work here — each consumer declares its own local interface, so the compiler checks every file against its own private claim and agrees with all of them. --- docs/api/openapi.yaml | 15 +++ e2e/notification-dispatch-journey.spec.ts | 27 ++---- .../account-payload-consumer-guard.test.ts | 95 ++++++++++++++++++- src/app/api/auth/me/route.ts | 11 +++ src/lib/openapi/routes/account-sharing.ts | 11 +++ 5 files changed, 138 insertions(+), 21 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index e2aaf39bf..165a61b47 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -65898,11 +65898,26 @@ components: anyOf: - $ref: "#/components/schemas/RecordSessionState" - type: "null" + notificationPrefs: + type: object + properties: + medication: + type: object + properties: + clientManaged: + type: boolean + required: + - clientManaged + additionalProperties: {} + required: + - medication + additionalProperties: {} required: - id - username - accountAccess - recordSession + - notificationPrefs additionalProperties: {} description: "The signed-in account: its identity, its preferences, and (since v1.36.0) what account sharing lets it do. Additional properties are the preference fields this spec does not yet enumerate. Under an active switch this diff --git a/e2e/notification-dispatch-journey.spec.ts b/e2e/notification-dispatch-journey.spec.ts index aceff5319..4e6db6c54 100644 --- a/e2e/notification-dispatch-journey.spec.ts +++ b/e2e/notification-dispatch-journey.spec.ts @@ -543,27 +543,18 @@ test.describe("notification preferences drive the dispatch decision", () => { expect(smtp.accepted()).toEqual([EMAIL_RECIPIENT]); }); + /** + * The web reflection of the opt-in, and the reason it is a test rather than a + * screenshot: the section reads `notificationPrefs.medication.clientManaged` + * off `/api/auth/me`, and that payload did not publish it until this branch. + * The flag was `undefined` for every account, so the chip never rendered and + * a person whose phone owned the reminders was shown a server-side switch + * that decided nothing. Both halves are asserted — the chip appears AND the + * switch row is gone — because either one alone passes on the wrong render. + */ test("the client-managed opt-in is visible on the medication it silences", async ({ page, }) => { - test.skip( - true, - "The chip exists and is unreachable. " + - "`src/components/medications/sections/notifications-section.tsx` " + - "decides between the reminder switch and the client-managed chip on " + - "`notificationPrefs.medication.clientManaged` read out of " + - "`/api/auth/me` — a field that payload does not carry (the prefs live " + - "on `/api/auth/me/notification-prefs`). So the flag reads as false for " + - "every account, the chip never renders, and a person whose phone owns " + - "the reminders is shown a server-side switch that no longer decides " + - "anything. The write path and the dispatch decision are correct — the " + - "two tests above prove both — only the web reflection is missing its " + - "end. The account-payload guard cannot see this: it asks that every " + - "field in the payload has a reader, and this is a reader with no " + - "field. Kept at full strength rather than relaxed to match today's " + - "render.", - ); - await configureChannels(page); await pinAccountClockToMidday(page); const medicationId = await createOverdueMedication(page); diff --git a/src/__tests__/account-payload-consumer-guard.test.ts b/src/__tests__/account-payload-consumer-guard.test.ts index dffe39795..011d62988 100644 --- a/src/__tests__/account-payload-consumer-guard.test.ts +++ b/src/__tests__/account-payload-consumer-guard.test.ts @@ -1,5 +1,6 @@ /** - * v1.32.36 — pair guard over the account payload (`GET /api/auth/me`). + * v1.32.36 — pair guard over the account payload (`GET /api/auth/me`), in both + * directions. * * The recurring failure this exists to prevent: a two-ended feature whose ends * ship in different releases. The column lands with the schema change, the @@ -33,12 +34,39 @@ * be allowlisted with that reason written down, and the reason is an * assumption about another repository until an audit crosses over. * + * ## The other direction: a reader with no field + * + * Everything above asks that a published field has a reader. It cannot see the + * inverse — a client that reads a field the payload never published — and that + * blind spot cost a shipped feature: the medication surface decided between + * the server-side reminder switch and the client-managed chip on + * `notificationPrefs.medication.clientManaged`, read off this payload, which + * did not carry it. The flag was `undefined` for every account, so the chip + * never rendered and a person whose phone owned the reminders was shown a + * switch that decided nothing. Every guard was green. + * + * The last case closes it, and the shape it takes is deliberate. A typed guard + * — one `AccountPayload` interface, `tsc` refusing an absent property — was the + * first idea and does not work here: the consumers declare their own local + * response interfaces (`interface UserPrefsResponse { … }`), so `tsc` checks + * each file against its own private claim about the payload and agrees with + * every one of them. So the check reads those local claims instead, out of the + * file that makes them, and holds them against what the route publishes. It + * asserts a non-zero match count, because a textual guard that silently + * matches nothing is the failure mode this repository already has on record. + * + * Its limit, written down: it sees the response type a `/api/auth/me` read + * names, so a consumer that types the read as `unknown` and indexes into it, or + * imports its response type from another module, is invisible to it. + * * Mutation check: append a field to the response literal in * `src/app/api/auth/me/route.ts` and this goes red; delete the practice-name - * read in `health-record-export-panel.tsx` and it goes red too. + * read in `health-record-export-panel.tsx` and it goes red too; delete + * `notificationPrefs` from the response literal and the last case goes red + * naming the medication notification section. */ import { readFileSync, readdirSync, statSync } from "node:fs"; -import { join } from "node:path"; +import { join, relative } from "node:path"; import { describe, it, expect } from "vitest"; @@ -189,6 +217,31 @@ function readsField(text: string, field: string): boolean { return re.test(text); } +/** + * Top-level member names of a locally declared `interface Name { … }`, or null + * when the file does not declare one under that name (the type is imported, or + * is a shared transport type this file only references). + * + * Members are read at one indent level, the same way the route's response + * literal is read above — the tree is prettier-formatted, so a nested object's + * keys sit deeper and are not mistaken for top-level fields. + */ +function declaredMembers(text: string, name: string): string[] | null { + const open = new RegExp(`\\binterface\\s+${name}\\s*\\{`).exec(text); + if (!open) return null; + const start = open.index + open[0].length; + const end = text.indexOf("\n}", start); + if (end === -1) return null; + const body = text.slice(start, end); + return [ + ...new Set( + [...body.matchAll(/^ {2}([a-zA-Z_$][\w$]*)\??\s*[?:]/gm)].map( + (m) => m[1], + ), + ), + ]; +} + describe("account payload consumer guard", () => { const source = readFileSync(ME_ROUTE, "utf8"); const fields = payloadFields(source); @@ -294,4 +347,40 @@ describe("account payload consumer guard", () => { ).toMatch(new RegExp(`active\\.${field}(?![\\w$])`)); } }); + + it("every field a client types onto the account payload is published", () => { + const published = new Set(fields); + const offenders: string[] = []; + let claimsChecked = 0; + + for (const file of consumerFiles) { + const text = stripComments(readFileSync(file, "utf8")); + // The type argument of a read of THIS endpoint. `/api/auth/me/...` + // siblings are different payloads, so the literal is anchored closed. + for (const call of text.matchAll( + /<\s*([A-Za-z_$][\w$]*)\s*>\s*\(\s*"\/api\/auth\/me"\s*\)/g, + )) { + const declared = declaredMembers(text, call[1]); + if (declared === null) continue; // an imported / shared type + claimsChecked += 1; + for (const member of declared) { + if (!published.has(member)) { + offenders.push( + `${relative(process.cwd(), file)} reads \`${member}\``, + ); + } + } + } + } + + expect( + claimsChecked, + "no client names a response type for GET /api/auth/me, so this case matched nothing rather than proving anything", + ).toBeGreaterThan(0); + + expect( + offenders, + "these clients type a field onto the account payload that the route does not publish, so it is `undefined` for every account and whatever it decides never happens", + ).toEqual([]); + }); }); diff --git a/src/app/api/auth/me/route.ts b/src/app/api/auth/me/route.ts index 425bfa33a..ddc985ca9 100644 --- a/src/app/api/auth/me/route.ts +++ b/src/app/api/auth/me/route.ts @@ -30,6 +30,7 @@ import { getOperatorModuleAvailability, } from "@/lib/modules/gate"; import { parseTourProgress } from "@/lib/onboarding/tour-progress"; +import { parseNotificationPrefs } from "@/lib/validations/notification-prefs"; import { resolveAccountAccess } from "@/lib/sharing/account-access"; import { recordSessionForPayload } from "@/lib/sharing/record-session-fence"; @@ -145,6 +146,16 @@ export const GET = apiHandler(async () => { // the account has never saved one, which is what makes the panel show the // named template on the first run rather than a silent server default. reportSelection: user.reportSelectionJson ?? null, + // The account's resolved notification preferences — the same resolver + // `GET /api/auth/me/notification-prefs` answers with, so the two surfaces + // cannot disagree. Published here because the medication detail's + // notification section decides between the server-side reminder switch and + // the client-managed chip on `medication.clientManaged`, and it reads that + // decision off THIS payload: the dedicated endpoint is not on the app-boot + // path. An account that has never opted in resolves to the documented + // defaults rather than to `undefined`, so the switch is what renders. + // No extra query — the session already loads the row this column is on. + notificationPrefs: parseNotificationPrefs(user.notificationPrefs ?? null), // v1.4.47 W3 — per-user Coach opt-out. Default `false` if the // column is absent (partial-deploy rollback safety, see migration // 0078 commentary). Every Coach mount point on the client checks diff --git a/src/lib/openapi/routes/account-sharing.ts b/src/lib/openapi/routes/account-sharing.ts index 76826de99..5dbc0f124 100644 --- a/src/lib/openapi/routes/account-sharing.ts +++ b/src/lib/openapi/routes/account-sharing.ts @@ -426,6 +426,17 @@ const accountPayload = z username: z.string(), accountAccess: accountAccessBlock, recordSession: recordSessionState.nullable(), + // Enumerated because a client DECIDES on it rather than displays it: the + // medication surface hides the server-side reminder switch when + // `medication.clientManaged` is true, because the phone owns those + // reminders and a switch that no longer decides anything is worse than no + // switch. Always fully resolved — an account that never opted in reads + // `false`, not absent. `PATCH /api/auth/me/notification-prefs` writes it; + // the categories this object does not name are the rest of the open + // preference envelope. + notificationPrefs: z.looseObject({ + medication: z.looseObject({ clientManaged: z.boolean() }), + }), }) .meta({ id: "AccountPayload", From 74dbb1a2e44c9303c8d62a293be0ec2b474f24da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 06:35:40 +0200 Subject: [PATCH 4/4] test(e2e): give the notification journey a server and a scope of its own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The journey's isolation went one way only. `test.describe.configure` orders the tests inside the file; the suite is fullyParallel with two workers, so everything else runs beside it — and its own trigger, the admin reminder sweep, walked the whole instance and dispatched for every account that had an overdue dose. With the mail env pointed at the local responder, that would have started sending real messages for accounts this file has never heard of, and the exact-equality assertion on the accepted recipients would have been reading them. The sweep now names the account it then counts, and the comments that defended the opposite direction say what the exposure actually is. Its own server, too, and for two reasons. The `SMTP_*` vars decide whether the Email card renders at all, so on the shared server they put a card the axe scan and the card-anatomy sweep had never measured onto `/settings/integrations` for every spec that opens the route. And unset, `HEALTHLOG_PROCESS_TYPE` starts the pg-boss reminder scheduler inside the process serving requests, whose every-fifteen-minutes tick either mints the missed-dose row that makes the sweep skip its own dispatch or dispatches beside it. A third server carries both: mail configured, `web`, and only this spec talks to it. The shared server keeps `all` on measured grounds rather than taste. Worker status is per-process and in-memory, so a `web` process reports the worker stopped, `/admin` renders that row destructive-red, and axe fails it at 3.97:1 against the muted tile — a real contrast defect any operator running the documented web/worker split already sees, and not this branch's to fix. That limit is written where the server is defined. Also here: the stub draws its port per run instead of pinning one, and says so by name when the port is taken; teardown ends its sockets and survives a setup that never got as far as starting it; the ledger reset stops deleting rows whose subject is this account but whose recipient is somebody else, and clears intake events explicitly rather than through a cascade; the attempt poll refuses a window larger than the twenty rows the diagnostic returns; and the header names the precondition the file cannot enforce and the internal reasons it pins on purpose. Verified against a production build and a throwaway Postgres: the journey three times over at one worker, then the a11y and settings geometry specs at two. Both assertions broken once to see them fail. --- e2e/notification-dispatch-journey.spec.ts | 125 +++++++++++++++++++--- e2e/setup/global-setup.ts | 12 ++- e2e/setup/notification-fixture.ts | 16 ++- e2e/setup/notification-server.ts | 43 ++++++++ e2e/setup/smtp-stub.ts | 61 ++++++++++- playwright.config.ts | 66 +++++++++--- 6 files changed, 283 insertions(+), 40 deletions(-) create mode 100644 e2e/setup/notification-server.ts diff --git a/e2e/notification-dispatch-journey.spec.ts b/e2e/notification-dispatch-journey.spec.ts index 4e6db6c54..ea8b68a90 100644 --- a/e2e/notification-dispatch-journey.spec.ts +++ b/e2e/notification-dispatch-journey.spec.ts @@ -35,11 +35,37 @@ * a form-control id — or the account's own API responses, never rendered copy: * the labels are i18n-driven and this account renders them in English only by * cookie. + * + * What isolates this file, and what does not. `test.describe.configure({ mode: + * "serial" })` orders the tests INSIDE it; the suite runs `fullyParallel` with + * two workers, so every other spec file is live against the same database + * throughout. Three things carry the isolation instead: the account is this + * file's own (`E2E_NOTIFY`), the server is this file's own (mail configured, no + * reminder scheduler — `e2e/setup/notification-server.ts`), and the reminder + * sweep is told which account to walk, so the one trigger here that could + * dispatch for the whole instance no longer does. + * + * What is left standing is a precondition rather than a mechanism: nothing else + * delivers email in this run, because no other e2e account configures an EMAIL + * channel and no other server is pointed at the responder. `smtp.accepted()` is + * asserted by exact equality on the strength of that, and a spec that seeds an + * email channel for another account has to say so here. + * + * `reason: "apns_not_configured"` and `reason: "client_managed"` are internal + * sender / dispatcher constants rather than published contract, and they are + * pinned on purpose: the ledger reason is the only place the DECISION is + * legible, and the whole claim of the client-managed opt-in is that the reason + * changes while everything else does not. A rename that reaches here should + * read as a decision to make, not as a mystery break. */ import type { Page } from "@playwright/test"; -import { NOTIFY_STORAGE_STATE_PATH } from "./setup/global-setup"; -import { resetNotificationFixture } from "./setup/notification-fixture"; +import { E2E_NOTIFY, NOTIFY_STORAGE_STATE_PATH } from "./setup/global-setup"; +import { NOTIFICATION_BASE_URL } from "./setup/notification-server"; +import { + notificationAccountId, + resetNotificationFixture, +} from "./setup/notification-fixture"; import { startSmtpStub, type SmtpStub } from "./setup/smtp-stub"; import { expect, test } from "./setup/test"; @@ -144,12 +170,24 @@ async function readDiagnostic(page: Page): Promise { * `expected` is asserted, not merely awaited — an EXTRA attempt is the failure * this journey exists to catch (a channel that was switched off delivering * anyway), and a poll that stops at "at least" would never see it. + * + * The diagnostic returns the trailing TWENTY rows and no more + * (`src/app/api/admin/notifications/diagnostic/route.ts`). The fixture resets + * the ledger to zero before every test and the largest cascade here is two, so + * the ceiling is far away — but a poll that ran past it could never converge + * and would report "the dispatch wrote N delivery attempt(s)" while the real + * answer was "the diagnostic stopped counting". The ceiling is asserted up + * front so that failure names itself. */ async function attemptsSince( page: Page, before: number, expected: number, ): Promise { + expect( + before + expected, + "the diagnostic returns at most 20 attempts, so a larger window could never converge", + ).toBeLessThanOrEqual(20); await expect .poll(async () => (await readDiagnostic(page)).recentPushAttempts.length, { message: `the dispatch wrote ${expected} delivery attempt(s)`, @@ -345,25 +383,43 @@ async function createOverdueMedication(page: Page): Promise { } interface SweepResult { + scoped: boolean; medications: Array<{ name: string; + user: string; schedules: Array<{ notificationSent: boolean }>; }>; } /** - * Run the reminder sweep the admin notifications panel drives. This is the - * product's own trigger for the medication-reminder dispatch, and it goes - * through `dispatchNotification` — the cascade, the per-channel preference - * read and the client-managed gate — rather than calling a sender directly. + * Run the reminder sweep the admin notifications panel drives, NAMING this + * account. This is the product's own trigger for the medication-reminder + * dispatch, and it goes through `dispatchNotification` — the cascade, the + * per-channel preference read and the client-managed gate — rather than + * calling a sender directly. + * + * The selector is not a convenience. Without it the sweep walks every active + * medication on the instance and dispatches for each overdue slot, which makes + * this file a writer into every other account's delivery ledger — the exact + * inverse of the interference it is trying to avoid, and one that would start + * sending real mail the moment another account had a channel and an overdue + * dose. Scoped, the run reaches one account: the one it then counts. */ -async function runReminderSweep(page: Page): Promise { +async function runReminderSweep(page: Page, userId: string): Promise { const res = await fromPage( page, "/api/admin/notifications/reminder-check", - { method: "POST" }, + { method: "POST", body: { userId } }, ); expect(res.status, "the sweep ran").toBe(200); + expect( + res.data?.scoped, + "the sweep walked one account, not the instance", + ).toBe(true); + expect( + [...new Set(res.data?.medications.map((m) => m.user))], + "every medication the sweep touched belongs to this account", + ).toEqual([E2E_NOTIFY.username]); const mine = res.data?.medications.find((m) => m.name === MEDICATION_NAME); expect(mine, "the sweep saw this account's overdue medication").toBeTruthy(); expect( @@ -381,7 +437,16 @@ function attemptFor( } test.describe("notification preferences drive the dispatch decision", () => { - test.use({ storageState: NOTIFY_STORAGE_STATE_PATH }); + /** + * Its own server as well as its own account. That process is the one carrying + * the mail configuration, and the one with no reminder scheduler inside it — + * `e2e/setup/notification-server.ts` says why each matters and what the + * shared server keeping the scheduler still leaves standing. + */ + test.use({ + storageState: NOTIFY_STORAGE_STATE_PATH, + baseURL: NOTIFICATION_BASE_URL, + }); /** * Serial, because every test configures the one account's channels and then @@ -391,20 +456,48 @@ test.describe("notification preferences drive the dispatch decision", () => { test.describe.configure({ mode: "serial" }); /** - * The sweep walks every active medication on the instance and the ledger - * write that follows is fire-and-forget, so the budget covers a loaded - * runner rather than the 30 s default. + * The ledger write that follows a dispatch is fire-and-forget and the tests + * poll for it, so the budget covers a loaded runner rather than the 30 s + * default. */ test.setTimeout(120_000); let smtp: SmtpStub; + let accountId: string; - test.beforeAll(async () => { + test.beforeAll(async ({ playwright }) => { smtp = await startSmtpStub(); + accountId = await notificationAccountId(); + + // The one precondition this file cannot arrange for itself: the server it + // talks to has to have been started with `SMTP_*`. `playwright.config.ts` + // hands them to the server it starts, but `reuseExistingServer` (the local + // default) and `E2E_SKIP_WEB_SERVER=1` both attach to a server started + // without them — and the symptom is a missing `#email-recipient` field + // thirty lines into the first test, which points nowhere near the cause. + const api = await playwright.request.newContext({ + baseURL: NOTIFICATION_BASE_URL, + storageState: NOTIFY_STORAGE_STATE_PATH, + }); + try { + const res = await api.get("/api/settings/email"); + const body = (await res.json()) as { data: { smtpConfigured: boolean } }; + expect( + body.data?.smtpConfigured, + `the server on ${NOTIFICATION_BASE_URL} was started without SMTP_* — a reused ` + + `dev server or E2E_SKIP_WEB_SERVER=1 does not carry them. Start it with ` + + `SMTP_HOST=127.0.0.1 SMTP_PORT=${smtp.port} SMTP_FROM=.`, + ).toBe(true); + } finally { + await api.dispose(); + } }); test.afterAll(async () => { - await smtp.close(); + // Optional-chained: a `beforeAll` that threw before the stub resolved + // leaves this undefined, and a teardown TypeError buries the real cause + // under its own. + await smtp?.close(); }); test.beforeEach(async () => { @@ -467,7 +560,7 @@ test.describe("notification preferences drive the dispatch decision", () => { await createOverdueMedication(page); const before = await attemptCount(page); - await runReminderSweep(page); + await runReminderSweep(page, accountId); // Two arms, and exactly two: the paired phone and the enabled email // channel. A third row would mean the switched-off relay was dialled. @@ -520,7 +613,7 @@ test.describe("notification preferences drive the dispatch decision", () => { expect(resolved.data?.medication.clientManaged).toBe(true); const before = await attemptCount(page); - await runReminderSweep(page); + await runReminderSweep(page, accountId); const attempts = await attemptsSince(page, before, 2); diff --git a/e2e/setup/global-setup.ts b/e2e/setup/global-setup.ts index bd0f24e99..2cc432cc0 100644 --- a/e2e/setup/global-setup.ts +++ b/e2e/setup/global-setup.ts @@ -98,9 +98,15 @@ export const E2E_GUARDIAN = { * * It has one, rather than borrowing the shared fixture, because the journey's * verdicts are counts over ONE account's `push_attempts` ledger — "one email - * attempt, no ntfy attempt, the APNs arm skipped and for which reason". Any - * other spec that happened to trigger a delivery for the shared account would - * land inside that window and flip a count that is not about it. + * attempt, no ntfy attempt, the APNs arm skipped and for which reason". + * + * The direction that matters is the one that is easy to state backwards. The + * exposure is not mainly that another spec writes into this account's window; + * it is that this journey's own trigger — the admin reminder sweep — dispatches + * for whatever accounts it is pointed at, and pointed at the instance it + * reaches all of them. A dedicated account gives the sweep something to name, + * so the run stays a writer for exactly one ledger and a reader of exactly the + * same one. * * ADMIN because the two surfaces the journey reads the dispatch decision * through — the reminder trigger and the notification diagnostic — are diff --git a/e2e/setup/notification-fixture.ts b/e2e/setup/notification-fixture.ts index 23363b05a..cd48bae00 100644 --- a/e2e/setup/notification-fixture.ts +++ b/e2e/setup/notification-fixture.ts @@ -77,9 +77,23 @@ export async function resetNotificationFixture(): Promise { userId, ]); await pool.query("DELETE FROM devices WHERE user_id = $1", [userId]); + // `user_id` and `recipient_user_id`, and deliberately NOT + // `record_user_id`: that column is the health-record SUBJECT, so a + // delivery ABOUT this account sent TO a guardian carries it while the + // recipient is somebody else. Deleting on it would take a row out of + // another account's ledger. The diagnostic this journey reads filters on + // the recipient, so the two agree. await pool.query( `DELETE FROM push_attempts - WHERE user_id = $1 OR recipient_user_id = $1 OR record_user_id = $1`, + WHERE user_id = $1 OR recipient_user_id = $1`, + [userId], + ); + // The medication delete cascades to its intake events, but a stale event + // is what makes the sweep decide a dose was already taken and skip its + // dispatch, so the statement is written out rather than inherited from a + // foreign key a future fixture might stop relying on. + await pool.query( + "DELETE FROM medication_intake_events WHERE user_id = $1", [userId], ); await pool.query("DELETE FROM medications WHERE user_id = $1", [userId]); diff --git a/e2e/setup/notification-server.ts b/e2e/setup/notification-server.ts new file mode 100644 index 000000000..053d30205 --- /dev/null +++ b/e2e/setup/notification-server.ts @@ -0,0 +1,43 @@ +/** + * The third E2E server: the one configured for mail, and the one with no + * scheduler. + * + * Two things the notification-dispatch journey needs are wrong for every other + * spec, so it gets its own process rather than bending the shared one. + * + * **Mail.** `SMTP_HOST` / `SMTP_PORT` / `SMTP_FROM` are operator config, and + * `isEmailConfigured()` is what decides whether the Email card renders at all + * (`src/components/settings/email-card.tsx`). Putting them on the shared server + * would put that card on `/settings/integrations` for every spec that opens the + * route — the axe scan and the card-anatomy geometry sweep included — and make + * this branch responsible for a surface it does not touch. Here, the shipped + * "no SMTP, no card" state stays what every other spec observes. + * + * **No scheduler.** Unset, `HEALTHLOG_PROCESS_TYPE` reads "all" and + * `src/instrumentation.ts` starts the pg-boss reminder worker inside the + * process serving requests, which schedules `medication-reminder-check` every + * fifteen minutes. The journey keeps a medication overdue for the length of a + * test, and a tick inside that window either mints the missed-dose row that + * makes the sweep skip its own dispatch or dispatches beside it and adds a + * ledger row the count was not expecting. `web` is the shipped, documented + * value for HTTP-without-scheduler (docs/self-hosting/scaling.md); the pg-boss + * PRODUCER still starts, so a route that enqueues work keeps working, and the + * journey's own trigger is a route rather than a job. + * + * The shared server keeps "all" deliberately, and the reason is measured + * rather than assumed: `getWorkerStatus()` is per-process in-memory, so a `web` + * process reports the worker stopped, `/admin` renders that row + * `text-destructive`, and axe fails it at 3.97:1 against the muted tile. That + * is a real contrast defect, visible to any operator running the documented + * web/worker split, and it is not this branch's to fix. What it means here is + * that the scheduler still exists on the suite's shared process: it is off in + * the process the journey drives, not off in the run. + * + * With `E2E_SKIP_WEB_SERVER=1` no server is started for you and all three have + * to be running — the same contract the other two already carry. + */ +export const NOTIFICATION_PORT = 3200; + +export const NOTIFICATION_BASE_URL = + process.env.E2E_NOTIFICATION_BASE_URL ?? + `http://localhost:${NOTIFICATION_PORT}`; diff --git a/e2e/setup/smtp-stub.ts b/e2e/setup/smtp-stub.ts index d640cc492..d63ea8e5a 100644 --- a/e2e/setup/smtp-stub.ts +++ b/e2e/setup/smtp-stub.ts @@ -27,9 +27,31 @@ import { createServer, type Server, type Socket } from "node:net"; /** * Port the stub listens on, and the port `playwright.config.ts` hands the app - * server as `SMTP_PORT`. Named here so the config and the spec cannot drift. + * server as `SMTP_PORT`. + * + * It cannot be an OS-assigned port-0 binding: the config has to put `SMTP_PORT` + * into the web server's environment before any worker process exists, and the + * stub itself does not start until the spec's `beforeAll` — so by the time a + * kernel-assigned port would be known, the server it has to reach is already + * running. The port is therefore CHOSEN once per run, at config load, from the + * IANA dynamic range, and published through `SMTP_STUB_PORT` in the + * environment so the config, the server and the spec all read the one value. + * Two suites on one machine draw different numbers instead of colliding on a + * constant, and a collision that does happen fails by name in `listen` below + * rather than as a missing form field thirty lines into a test. + * + * An operator pinning it by hand (`SMTP_STUB_PORT=3925 pnpm e2e`) wins, which + * is what makes an externally started server — `E2E_SKIP_WEB_SERVER=1` — able + * to carry the same value. */ -export const SMTP_STUB_PORT = 3925; +export function resolveSmtpStubPort(): number { + const pinned = Number(process.env.SMTP_STUB_PORT); + if (Number.isInteger(pinned) && pinned > 0 && pinned < 65_536) return pinned; + // 49152–65535, the dynamic/private range no registered service claims. + const chosen = 49_152 + Math.floor(Math.random() * (65_536 - 49_152)); + process.env.SMTP_STUB_PORT = String(chosen); + return chosen; +} export const SMTP_STUB_HOST = "127.0.0.1"; @@ -37,6 +59,8 @@ export const SMTP_STUB_HOST = "127.0.0.1"; export const SMTP_STUB_FROM = "healthlog-e2e@healthlog.test"; export interface SmtpStub { + /** The port this responder bound, which is the one the app was handed. */ + readonly port: number; /** Envelope recipients of every message the stub accepted, oldest first. */ accepted(): readonly string[]; /** Forget everything accepted so far. */ @@ -49,8 +73,16 @@ const CRLF = "\r\n"; /** Start the responder and resolve once it is accepting connections. */ export async function startSmtpStub(): Promise { const accepted: string[] = []; + const port = resolveSmtpStubPort(); + // Every established socket, so `close()` can end them. `server.close()` + // resolves only once the last connection is gone and never ends one itself, + // so a socket abandoned mid-DATA by a timed-out test would otherwise hold + // teardown open until Playwright's own timeout fires over it. + const open = new Set(); const server: Server = createServer((socket: Socket) => { + open.add(socket); + socket.on("close", () => open.delete(socket)); let buffer = ""; let inData = false; let envelope: string[] = []; @@ -113,20 +145,39 @@ export async function startSmtpStub(): Promise { }); await new Promise((resolve, reject) => { - server.once("error", reject); - server.listen(SMTP_STUB_PORT, SMTP_STUB_HOST, () => { - server.removeListener("error", reject); + server.once("error", (err: NodeJS.ErrnoException) => { + // The one failure worth naming. A crashed prior run, a second suite, or + // a hand-pinned port already in use all arrive here, and the bare + // `EADDRINUSE` that used to propagate took the whole `beforeAll` down + // with a message that named neither the port nor the stub. + reject( + err.code === "EADDRINUSE" + ? new Error( + `[smtp-stub] port ${port} on ${SMTP_STUB_HOST} is already in use — ` + + "another run's responder is still listening, or SMTP_STUB_PORT " + + "was pinned to a port something else holds. Free it, or unset " + + "SMTP_STUB_PORT to draw a fresh one.", + { cause: err }, + ) + : err, + ); + }); + server.listen(port, SMTP_STUB_HOST, () => { + server.removeAllListeners("error"); resolve(); }); }); return { + port, accepted: () => accepted, reset: () => { accepted.length = 0; }, close: () => new Promise((resolve) => { + for (const socket of open) socket.destroy(); + open.clear(); server.close(() => resolve()); }), }; diff --git a/playwright.config.ts b/playwright.config.ts index b2c689661..6f9ecee3e 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -1,13 +1,17 @@ import { defineConfig, devices } from "@playwright/test"; +import { + NOTIFICATION_BASE_URL, + NOTIFICATION_PORT, +} from "./e2e/setup/notification-server"; import { SSR_PREFETCH_BASE_URL, SSR_PREFETCH_PORT, } from "./e2e/setup/ssr-prefetch-server"; import { + resolveSmtpStubPort, SMTP_STUB_FROM, SMTP_STUB_HOST, - SMTP_STUB_PORT, } from "./e2e/setup/smtp-stub"; /** @@ -24,6 +28,14 @@ import { * To run locally: `pnpm dlx playwright install --with-deps chromium` * once, then `pnpm e2e`. */ +/** + * Drawn once here, before any server or worker exists, and published through + * `process.env.SMTP_STUB_PORT` — which the web server inherits and which every + * worker process re-reads when it loads this config, so all three agree on one + * number without a constant to collide on. + */ +const smtpStubPort = resolveSmtpStubPort(); + export default defineConfig({ testDir: "./e2e", testIgnore: ["setup/**"], @@ -176,11 +188,12 @@ export default defineConfig({ // through stable data attributes, not a mobile layout, so it runs in // one project. "apple-health-import.spec.ts", - // The notification-dispatch journey binds a local SMTP responder on a - // fixed port and reads GLOBAL verdicts off one account's delivery - // ledger — "one email attempt, no ntfy attempt, the APNs arm skipped - // for this reason". A second project would fight it for the port and - // add attempts to the window it counts. It proves a dispatch decision + // The notification-dispatch journey binds a local SMTP responder and + // reads verdicts off one account's delivery ledger — "one email + // attempt, no ntfy attempt, the APNs arm skipped for this reason". A + // second project would run the file in a second worker process, whose + // stub would find the port already bound, and would add attempts to + // the window the first one counts. It proves a dispatch decision // through stable attributes and the account's own API, not a mobile // layout, so it runs in one project. "notification-dispatch-journey.spec.ts", @@ -223,15 +236,6 @@ export default defineConfig({ // Disable the prefetch for the e2e server — the suite keeps the // deterministic client-fetch path. DASHBOARD_SSR_PREFETCH: "false", - // The one delivery channel a notification journey can exercise - // for real on one machine. Its transport is operator config, not - // account input, so it never crosses the SSRF floor that refuses - // a local host for ntfy / webhook / Web Push. The responder these - // point at is started by the journey itself; see - // `e2e/setup/smtp-stub.ts` for why no other channel can be. - SMTP_HOST: SMTP_STUB_HOST, - SMTP_PORT: String(SMTP_STUB_PORT), - SMTP_FROM: SMTP_STUB_FROM, }, }, // The SHIPPED configuration, on its own port. @@ -262,5 +266,37 @@ export default defineConfig({ DASHBOARD_SSR_PREFETCH: "true", }, }, + // The mail-configured, scheduler-free server, on its own port. + // + // Only `notification-dispatch-journey.spec.ts` talks to it, and + // `e2e/setup/notification-server.ts` carries the whole reasoning: why + // the SMTP env must not reach the shared server (it would render the + // Email card for every spec that opens `/settings/integrations`), why + // this process must not run the reminder scheduler (a tick inside the + // journey's window is a writer no spec can see), and what the shared + // server keeping "all" still leaves standing. + { + command: `PORT=${NOTIFICATION_PORT} HOSTNAME=127.0.0.1 ${JSON.stringify(process.execPath)} .next/standalone/server.js`, + url: `${NOTIFICATION_BASE_URL}/api/version`, + timeout: 60_000, + reuseExistingServer: !process.env.CI, + stdout: "ignore", + stderr: "pipe", + env: { + ...process.env, + NATIVE_CANVAS: "off", + DASHBOARD_SSR_PREFETCH: "false", + HEALTHLOG_PROCESS_TYPE: "web", + // The one delivery channel a notification journey can exercise for + // real on one machine. Its transport is operator config, not + // account input, so it never crosses the SSRF floor that refuses a + // local host for ntfy / webhook / Web Push. The responder these + // point at is started by the journey itself; see + // `e2e/setup/smtp-stub.ts` for why no other channel can be. + SMTP_HOST: SMTP_STUB_HOST, + SMTP_PORT: String(smtpStubPort), + SMTP_FROM: SMTP_STUB_FROM, + }, + }, ], });