From 367ba65bf667e8c013e80bffa7bf8c75d4e3306a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 12 Nov 2026 13:51:44 +0100 Subject: [PATCH] fix(auth): take the push subscription off on every sign-out button Only the menu's sign-out removed this browser's push subscription. The sign-out buttons on the registration page and the profile take-over page, which exist so someone else can use the browser next, left it behind, and the next person kept seeing the last account's reminders. All three now go through one signOutRequest. The browser's own subscription is now removed only once the server has confirmed the sign-out; the server row still goes first, while the session can name it. A refused sign-out stores the row again, so the person who is still signed in keeps push. The logout test pins the new order and the refusal. The helper file is renamed web-push-subscription.ts, since it now also saves a subscription for the settings card. --- .../destructive-control-guard.test.ts | 2 +- .../register/__tests__/page.browser.test.tsx | 41 +++++++++- src/app/auth/register/page.tsx | 10 ++- .../__tests__/claim-profile.browser.test.tsx | 36 +++++++++ src/components/auth/claim-profile.tsx | 10 ++- src/components/settings/web-push-card.tsx | 34 +++----- src/hooks/__tests__/use-logout.test.ts | 44 +++++++++-- src/hooks/use-auth.ts | 23 +++--- .../notifications/web-push-subscription.ts | 77 +++++++++++++++++++ src/lib/notifications/web-push-unsubscribe.ts | 23 ------ 10 files changed, 231 insertions(+), 69 deletions(-) create mode 100644 src/lib/notifications/web-push-subscription.ts delete mode 100644 src/lib/notifications/web-push-unsubscribe.ts diff --git a/src/__tests__/destructive-control-guard.test.ts b/src/__tests__/destructive-control-guard.test.ts index a9a1bc80d..39bf621d0 100644 --- a/src/__tests__/destructive-control-guard.test.ts +++ b/src/__tests__/destructive-control-guard.test.ts @@ -348,7 +348,7 @@ const REGISTRY: DestructiveEntry[] = [ }, ], undetected: - "No detector sees it. The DELETE goes out from `unsubscribeWebPush` in `lib/notifications/web-push-unsubscribe.ts`, which sign-out calls too, and `lib/` is an excluded root. Registered by hand.", + "No detector sees it. The DELETE goes out from `lib/notifications/web-push-subscription.ts` (`unsubscribeWebPush`, and `signOutWithoutWebPush` for sign-out), and `lib/` is an excluded root. Registered by hand.", }, // ── Measurements, mood, labs, illness — the undo-and-restore surfaces ──── diff --git a/src/app/auth/register/__tests__/page.browser.test.tsx b/src/app/auth/register/__tests__/page.browser.test.tsx index 4a66bbd99..1a67ca9e4 100644 --- a/src/app/auth/register/__tests__/page.browser.test.tsx +++ b/src/app/auth/register/__tests__/page.browser.test.tsx @@ -6,7 +6,7 @@ * existed belongs to it. The app replaces the page in the history, so Back * does not show the form again. */ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { page } from "vitest/browser"; import { queryKeys } from "@/lib/query-keys"; @@ -48,6 +48,45 @@ describe("registration page", () => { }); describe("registration page, already signed in", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("takes this browser's push subscription with it on sign-out", async () => { + // The button exists so someone else can register on this browser; a + // subscription left behind would show them the last account's reminders. + const unsubscribe = vi.fn(async () => true); + vi.spyOn(navigator.serviceWorker, "getRegistration").mockResolvedValue({ + pushManager: { + getSubscription: async () => ({ + endpoint: "https://push.example.test/send/abc", + unsubscribe, + }), + }, + } as unknown as ServiceWorkerRegistration); + const api = stubFetch() + .on("DELETE", "/api/notifications/web-push", { data: {} }) + .on("POST", "/api/auth/logout", { data: { loggedOut: true } }); + window.history.replaceState(null, "", "/auth/register?invite=hlv_x"); + const { screen } = await renderWithApp(, { + user: { username: "signed-in" }, + }); + + await screen.getByTestId("register-sign-out").click(); + + await expect.poll(() => unsubscribe.mock.calls.length).toBe(1); + const order = api.requests + .map((r) => `${r.method} ${r.path.split("?")[0]}`) + .filter((call) => !call.startsWith("GET ")); + expect(order).toEqual([ + "DELETE /api/notifications/web-push", + "POST /api/auth/logout", + ]); + expect(api.calls("DELETE", "/api/notifications/web-push")[0].body).toEqual({ + endpoint: "https://push.example.test/send/abc", + }); + }); + it("keeps the page and says so when the server refuses the sign-out", async () => { const api = stubFetch().on("POST", "/api/auth/logout", { status: 403, diff --git a/src/app/auth/register/page.tsx b/src/app/auth/register/page.tsx index 8832b0f50..99b4a9b98 100644 --- a/src/app/auth/register/page.tsx +++ b/src/app/auth/register/page.tsx @@ -12,9 +12,13 @@ import { Label } from "@/components/ui/label"; import { PasswordStrength } from "@/components/ui/password-strength"; import { useTranslations } from "@/lib/i18n/context"; import { detectBrowserTimezone } from "@/lib/tz/format"; -import { ApiError, apiFetch, apiPost } from "@/lib/api/api-fetch"; +import { ApiError, apiPost } from "@/lib/api/api-fetch"; import { localizedApiError } from "@/lib/api/localized-error"; -import { clearCachesForSessionEnd, useAuth } from "@/hooks/use-auth"; +import { + clearCachesForSessionEnd, + signOutRequest, + useAuth, +} from "@/hooks/use-auth"; export default function RegisterPage() { const router = useRouter(); @@ -89,7 +93,7 @@ export default function RegisterPage() { setError(null); setSigningOut(true); try { - await apiFetch("/api/auth/logout", { method: "POST" }); + await signOutRequest(); // Same session-end wipe the shell and the menu do: the QueryClient // outlives the navigation on this SPA and the health-data families are // not user-scoped, so nothing may survive into the next account. diff --git a/src/components/auth/__tests__/claim-profile.browser.test.tsx b/src/components/auth/__tests__/claim-profile.browser.test.tsx index b4a80a369..d4a0f7080 100644 --- a/src/components/auth/__tests__/claim-profile.browser.test.tsx +++ b/src/components/auth/__tests__/claim-profile.browser.test.tsx @@ -117,6 +117,42 @@ describe("ClaimProfile", () => { }); describe("ClaimProfile, already signed in", () => { + it("takes this browser's push subscription with it on sign-out", async () => { + // The button exists so the profile's person can take over on this + // browser; a subscription left behind would show them the last + // account's reminders. + const unsubscribe = vi.fn(async () => true); + const registration = vi + .spyOn(navigator.serviceWorker, "getRegistration") + .mockResolvedValue({ + pushManager: { + getSubscription: async () => ({ + endpoint: "https://push.example.test/send/abc", + unsubscribe, + }), + }, + } as unknown as ServiceWorkerRegistration); + api + .on("DELETE", "/api/notifications/web-push", { data: {} }) + .on("POST", "/api/auth/logout", { data: { loggedOut: true } }); + await renderWithApp(, { + user: { username: "signed-in" }, + }); + + await click("claim-sign-out"); + + await expect.poll(() => unsubscribe.mock.calls.length).toBe(1); + const order = api.requests + .map((r) => `${r.method} ${r.path.split("?")[0]}`) + .filter((call) => !call.startsWith("GET ")); + // The signed-out page then loads the link's preview. + expect(order.slice(0, 2)).toEqual([ + "DELETE /api/notifications/web-push", + "POST /api/auth/logout", + ]); + registration.mockRestore(); + }); + it("keeps the session's page and says so when the server refuses the sign-out", async () => { api.on("POST", "/api/auth/logout", { status: 403, diff --git a/src/components/auth/claim-profile.tsx b/src/components/auth/claim-profile.tsx index 54120d064..0a3b60c9e 100644 --- a/src/components/auth/claim-profile.tsx +++ b/src/components/auth/claim-profile.tsx @@ -13,8 +13,12 @@ import { Input } from "@/components/ui/input"; import { Label } from "@/components/ui/label"; import { Logo } from "@/components/ui/logo"; import { PasswordStrength } from "@/components/ui/password-strength"; -import { clearCachesForSessionEnd, useAuth } from "@/hooks/use-auth"; -import { ApiError, apiFetch } from "@/lib/api/api-fetch"; +import { + clearCachesForSessionEnd, + signOutRequest, + useAuth, +} from "@/hooks/use-auth"; +import { ApiError } from "@/lib/api/api-fetch"; import { showErrorToast } from "@/lib/errors/error-toast"; import { useFormatters, useTranslations } from "@/lib/i18n/context"; import { loadDocument } from "@/lib/navigation/load-document"; @@ -79,7 +83,7 @@ export function ClaimProfile({ async function signOut() { setSigningOut(true); try { - await apiFetch("/api/auth/logout", { method: "POST" }); + await signOutRequest(); clearCachesForSessionEnd(queryClient); // Back to this page with the link intact: the token only rides the URL. router.replace( diff --git a/src/components/settings/web-push-card.tsx b/src/components/settings/web-push-card.tsx index e7c19b0d7..9a73f0a9b 100644 --- a/src/components/settings/web-push-card.tsx +++ b/src/components/settings/web-push-card.tsx @@ -10,7 +10,10 @@ import { SettingsCardActions } from "@/components/settings/_card-actions"; import { SettingsCardHeader } from "@/components/settings/_card-header"; import { useTranslations } from "@/lib/i18n/context"; import { apiFetchRaw } from "@/lib/api/api-fetch"; -import { unsubscribeWebPush } from "@/lib/notifications/web-push-unsubscribe"; +import { + saveWebPushSubscription, + unsubscribeWebPush, +} from "@/lib/notifications/web-push-subscription"; /** Convert a URL-safe base64 string to an ArrayBuffer (for VAPID key). */ function urlBase64ToArrayBuffer(base64String: string): ArrayBuffer { @@ -38,27 +41,6 @@ export function needsHomeScreen(): boolean { return appleTouch && !window.matchMedia("(display-mode: standalone)").matches; } -/** - * Store this browser's subscription for the signed-in account. Also run for a - * subscription the browser already holds, so a row the server lost or gave - * to another account on this browser comes back, and a channel switched off - * by failed sends is turned on again. - */ -function saveSubscription(subscription: PushSubscription): Promise { - const subJson = subscription.toJSON(); - return apiFetchRaw("/api/notifications/web-push", { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ - endpoint: subJson.endpoint, - keys: { - p256dh: subJson.keys?.p256dh ?? "", - auth: subJson.keys?.auth ?? "", - }, - }), - }); -} - type Unsupported = "browser" | "install" | null; export function WebPushCard() { @@ -94,7 +76,11 @@ export function WebPushCard() { if (registration) { const subscription = await registration.pushManager.getSubscription(); setIsSubscribed(!!subscription); - if (subscription) void saveSubscription(subscription).catch(() => {}); + // Sent again on every open: the server may have lost the row or + // switched the channel off, and the browser cannot tell. + if (subscription) { + void saveWebPushSubscription(subscription).catch(() => null); + } } } catch { // Ignore errors during check @@ -133,7 +119,7 @@ export function WebPushCard() { applicationServerKey: urlBase64ToArrayBuffer(vapidPublicKey), }); - const res = await saveSubscription(subscription); + const res = await saveWebPushSubscription(subscription); if (res.ok) { setIsSubscribed(true); setMsg(t("settings.webPushSubscribed")); diff --git a/src/hooks/__tests__/use-logout.test.ts b/src/hooks/__tests__/use-logout.test.ts index 592b28e79..45fb6b415 100644 --- a/src/hooks/__tests__/use-logout.test.ts +++ b/src/hooks/__tests__/use-logout.test.ts @@ -88,16 +88,18 @@ describe("useLogout", () => { expect(showErrorToast).not.toHaveBeenCalled(); }); - it("removes this browser's push subscription before the session ends", async () => { - // Left in place, the browser kept showing the signed-out account's - // medication reminders, also after someone else signed in. - const order: string[] = []; + /** A browser holding a push subscription; returns the call order log. */ + function holdSubscription(order: string[]) { const unsubscribe = vi.fn(async () => { order.push("browser unsubscribe"); return true; }); const subscription = { endpoint: "https://push.example.com/send/abc", + toJSON: () => ({ + endpoint: "https://push.example.com/send/abc", + keys: { p256dh: "p256dh", auth: "auth" }, + }), unsubscribe, }; vi.stubGlobal("window", { PushManager: class {} }); @@ -108,6 +110,16 @@ describe("useLogout", () => { }), }, }); + return { subscription, unsubscribe }; + } + + it("takes this browser's push subscription off the account and out of the browser", async () => { + // Left in place, the browser kept showing the signed-out account's + // medication reminders, also after someone else signed in. The server + // row goes while the session can still name it; the browser's own only + // once the sign-out is confirmed. + const order: string[] = []; + const { subscription } = holdSubscription(order); fetchMock.mockImplementation(async (url, init) => { order.push(`${init?.method} ${url}`); return json(200, { data: {} }); @@ -118,8 +130,8 @@ describe("useLogout", () => { expect(order).toEqual([ "DELETE /api/notifications/web-push", - "browser unsubscribe", "POST /api/auth/logout", + "browser unsubscribe", ]); const deleteCall = fetchMock.mock.calls[0]; expect(JSON.parse(String(deleteCall[1]?.body))).toEqual({ @@ -128,6 +140,28 @@ describe("useLogout", () => { expect(push).toHaveBeenCalledWith("/auth/login"); }); + it("keeps push working when the server refuses the sign-out", async () => { + const order: string[] = []; + const { unsubscribe } = holdSubscription(order); + fetchMock.mockImplementation(async (url, init) => { + order.push(`${init?.method} ${url}`); + return url === "/api/auth/logout" + ? json(503, { data: null, error: "Unavailable" }) + : json(200, { data: {} }); + }); + + useLogout(); + await settle(captured.options!); + + expect(order).toEqual([ + "DELETE /api/notifications/web-push", + "POST /api/auth/logout", + "POST /api/notifications/web-push", + ]); + expect(unsubscribe).not.toHaveBeenCalled(); + expect(showErrorToast).toHaveBeenCalledTimes(1); + }); + it("still signs out when removing the subscription fails", async () => { vi.stubGlobal("window", { PushManager: class {} }); vi.stubGlobal("navigator", { diff --git a/src/hooks/use-auth.ts b/src/hooks/use-auth.ts index 7a162487a..9f8b58829 100644 --- a/src/hooks/use-auth.ts +++ b/src/hooks/use-auth.ts @@ -30,7 +30,7 @@ import type { TourProgress } from "@/lib/onboarding/tour-progress"; import { clearLastDay } from "@/lib/day/last-day"; import { clearCoachDeleteJournal } from "@/lib/ai/coach/delete-journal"; import { clearOfflineCachesForSessionEnd } from "@/lib/pwa/query-persister"; -import { unsubscribeWebPush } from "@/lib/notifications/web-push-unsubscribe"; +import { signOutWithoutWebPush } from "@/lib/notifications/web-push-subscription"; import { setRecordScope, setRefusedRecordScope, @@ -646,20 +646,25 @@ export function clearCachesForSessionEnd(queryClient: QueryClient): void { void clearOfflineCachesForSessionEnd(); } +/** + * End the session on the server, for every sign-out button. Takes this + * browser's push subscription with it. Throws on any non-2xx answer: a + * refused sign-out leaves the session alive, and showing the sign-in page + * then would claim otherwise. + */ +export function signOutRequest(): Promise { + return signOutWithoutWebPush(() => + apiFetch("/api/auth/logout", { method: "POST" }), + ); +} + export function useLogout() { const queryClient = useQueryClient(); const router = useRouter(); const { t } = useTranslations(); return useMutation({ - mutationFn: async () => { - // Before the session ends: the server row can only be removed while - // it is still valid. A failure here does not block signing out. - await unsubscribeWebPush().catch(() => undefined); - // Throws on any non-2xx answer: a refused sign-out leaves the session - // alive, and showing the sign-in page then would claim otherwise. - await apiFetch("/api/auth/logout", { method: "POST" }); - }, + mutationFn: signOutRequest, onSuccess: () => { clearCachesForSessionEnd(queryClient); router.push("/auth/login"); diff --git a/src/lib/notifications/web-push-subscription.ts b/src/lib/notifications/web-push-subscription.ts new file mode 100644 index 000000000..ace66218e --- /dev/null +++ b/src/lib/notifications/web-push-subscription.ts @@ -0,0 +1,77 @@ +import { apiFetchRaw } from "@/lib/api/api-fetch"; + +/** This browser's push subscription, or null when it has none. */ +async function browserSubscription(): Promise { + if (!("serviceWorker" in navigator) || !("PushManager" in window)) { + return null; + } + const registration = await navigator.serviceWorker.getRegistration("/sw.js"); + return (await registration?.pushManager.getSubscription()) ?? null; +} + +function deleteServerRow(subscription: PushSubscription): Promise { + return apiFetchRaw("/api/notifications/web-push", { + method: "DELETE", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ endpoint: subscription.endpoint }), + }); +} + +/** + * Store this browser's subscription for the signed-in account. Also run for a + * subscription the browser already holds, so a row the server lost comes + * back and a channel switched off by failed sends is turned on again. + */ +export function saveWebPushSubscription( + subscription: PushSubscription, +): Promise { + const subJson = subscription.toJSON(); + return apiFetchRaw("/api/notifications/web-push", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + endpoint: subJson.endpoint, + keys: { + p256dh: subJson.keys?.p256dh ?? "", + auth: subJson.keys?.auth ?? "", + }, + }), + }); +} + +/** + * "Disable push": remove the server row, then the browser's own + * subscription. Does nothing when the browser has none. + */ +export async function unsubscribeWebPush(): Promise { + const subscription = await browserSubscription(); + if (!subscription) return; + await deleteServerRow(subscription); + await subscription.unsubscribe(); +} + +/** + * Run a sign-out with this browser's push subscription taken off the account. + * A subscription left behind keeps showing that account's reminders on this + * browser, to whoever uses it next. + * + * The server row goes first, while the session can still name it; the + * browser's subscription only once `signOut` resolved. A refused sign-out + * stores the row again, so the person who is still signed in keeps push. + * Failures around the subscription never block the sign-out. + */ +export async function signOutWithoutWebPush( + signOut: () => Promise, +): Promise { + const subscription = await browserSubscription().catch(() => null); + if (subscription) await deleteServerRow(subscription).catch(() => null); + try { + await signOut(); + } catch (failure) { + if (subscription) { + await saveWebPushSubscription(subscription).catch(() => null); + } + throw failure; + } + await subscription?.unsubscribe().catch(() => false); +} diff --git a/src/lib/notifications/web-push-unsubscribe.ts b/src/lib/notifications/web-push-unsubscribe.ts deleted file mode 100644 index ca2ba162a..000000000 --- a/src/lib/notifications/web-push-unsubscribe.ts +++ /dev/null @@ -1,23 +0,0 @@ -import { apiFetchRaw } from "@/lib/api/api-fetch"; - -/** - * Remove this browser's Web Push subscription: the server row first, while - * the session can still name it, then the browser's own. Does nothing when - * the browser has none. - * - * Used by "Disable push" and by sign-out. A subscription left behind at - * sign-out keeps showing that account's reminders on this browser, to - * whoever uses it next. - */ -export async function unsubscribeWebPush(): Promise { - if (!("serviceWorker" in navigator) || !("PushManager" in window)) return; - const registration = await navigator.serviceWorker.getRegistration("/sw.js"); - const subscription = await registration?.pushManager.getSubscription(); - if (!subscription) return; - await apiFetchRaw("/api/notifications/web-push", { - method: "DELETE", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ endpoint: subscription.endpoint }), - }); - await subscription.unsubscribe(); -}