From 510ce3128d12a19e581458e1b404cb904b49d593 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 12 Nov 2026 14:02:13 +0100 Subject: [PATCH] fix(auth): end the session only on HealthLog's own 401 or 403 A refetch of the account that answered 401 or 403 now signs the person out and clears the offline copies, which is right for HealthLog's own answer. But a self-hosted instance behind a forward-auth proxy or a firewall can get a 401 or 403 that HealthLog never wrote, and that signed a person out whose session was fine. HealthLog's error envelope always names a code, so only a 401 or 403 that carries one is read as the session's end; one without is treated like a network failure and keeps the last known state. The tests now give the account route the answer the server really sends. --- .../auth-shell-session-end.browser.test.tsx | 34 +++++++++++++----- .../__tests__/use-auth-offline-gate.test.ts | 36 ++++++++++++++++--- src/hooks/use-auth.ts | 27 ++++++++------ 3 files changed, 72 insertions(+), 25 deletions(-) diff --git a/src/components/layout/__tests__/auth-shell-session-end.browser.test.tsx b/src/components/layout/__tests__/auth-shell-session-end.browser.test.tsx index 5a5e5ebd0..84e1ec086 100644 --- a/src/components/layout/__tests__/auth-shell-session-end.browser.test.tsx +++ b/src/components/layout/__tests__/auth-shell-session-end.browser.test.tsx @@ -72,6 +72,13 @@ function shellTree(queryClient: QueryClient) { ); } +/** HealthLog's answer once there is no session; it always names a code. */ +const SIGNED_OUT = { + status: 401, + error: "Unauthorized", + meta: { errorCode: "auth.missing" }, +}; + async function mountShell(user: Parameters[0] | null) { return renderWithApp( @@ -91,10 +98,7 @@ afterEach(() => { describe("AuthShell on a sign-in page", () => { it("clears the offline copies once the server says there is no session", async () => { - const fetchStub = stubFetch().on("GET", ME, { - status: 401, - error: "Unauthorized", - }); + const fetchStub = stubFetch().on("GET", ME, SIGNED_OUT); openAt("/auth/login"); const { queryClient } = await mountShell(null); @@ -108,7 +112,7 @@ describe("AuthShell on a sign-in page", () => { }); it("does the same on the profile take-over page", async () => { - stubFetch().on("GET", ME, { status: 401, error: "Unauthorized" }); + stubFetch().on("GET", ME, SIGNED_OUT); openAt("/auth/claim"); await mountShell(null); await expect.poll(() => clearOffline.mock.calls.length).toBe(1); @@ -139,8 +143,22 @@ describe("AuthShell on a sign-in page", () => { expect(clearOffline).not.toHaveBeenCalled(); }); + it("leaves them alone for a 401 from something in front of the server", async () => { + // A forward-auth proxy or a firewall answers with its own page, which + // names no HealthLog code and says nothing about this session. + const fetchStub = stubFetch().on("GET", ME, { + status: 401, + error: "Unauthorized", + }); + openAt("/auth/login"); + await mountShell(null); + await expect.poll(() => fetchStub.calls("GET", ME).length).toBe(1); + await settle(); + expect(clearOffline).not.toHaveBeenCalled(); + }); + it("does not clear on a public page outside the sign-in flow", async () => { - stubFetch().on("GET", ME, { status: 401, error: "Unauthorized" }); + stubFetch().on("GET", ME, SIGNED_OUT); openAt("/about"); await mountShell(null); await settle(); @@ -153,9 +171,7 @@ describe("AuthShell on a protected page", () => { async function signedInAt(path: string) { let session = true; stubFetch().on("GET", ME, () => - session - ? { data: authUser({ id: "signed-in" }) } - : { status: 401, error: "Unauthorized" }, + session ? { data: authUser({ id: "signed-in" }) } : SIGNED_OUT, ); openAt(path); const view = await mountShell(null); diff --git a/src/hooks/__tests__/use-auth-offline-gate.test.ts b/src/hooks/__tests__/use-auth-offline-gate.test.ts index 2d7f9f180..376feeb1b 100644 --- a/src/hooks/__tests__/use-auth-offline-gate.test.ts +++ b/src/hooks/__tests__/use-auth-offline-gate.test.ts @@ -55,6 +55,11 @@ import { useAuth, } from "../use-auth"; +/** What `/api/auth/me` answers once the session is gone. */ +const SESSION_GONE = new ApiError("Unauthorized", 401, { + errorCode: "auth.missing", +}); + // Node test env has no localStorage — back it with a Map. const store = new Map(); beforeEach(() => { @@ -97,9 +102,20 @@ describe("isAuthVerdictUnknown — failure classification", () => { expect(isAuthVerdictUnknown(new ApiError("boom", 500))).toBe(true); }); - it("treats 401 and 403 as the real session-end verdict", () => { - expect(isAuthVerdictUnknown(new ApiError("unauthorized", 401))).toBe(false); - expect(isAuthVerdictUnknown(new ApiError("forbidden", 403))).toBe(false); + it("treats HealthLog's 401 and 403 as the real session-end verdict", () => { + expect(isAuthVerdictUnknown(SESSION_GONE)).toBe(false); + expect( + isAuthVerdictUnknown( + new ApiError("forbidden", 403, { errorCode: "http.forbidden" }), + ), + ).toBe(false); + }); + + it("treats a 401 or 403 without HealthLog's envelope as no verdict", () => { + // A forward-auth proxy or a firewall in front of the instance answers + // with its own page; the session behind it may be fine. + expect(isAuthVerdictUnknown(new ApiError("Unauthorized", 401))).toBe(true); + expect(isAuthVerdictUnknown(new ApiError("Forbidden", 403))).toBe(true); }); }); @@ -128,7 +144,7 @@ describe("useAuth — last-known state on a non-verdict failure", () => { it("reads a true 401 as session end — wipe and redirect may proceed", () => { store.set("healthlog-was-authenticated", "1"); queryState.isError = true; - queryState.error = new ApiError("unauthorized", 401); + queryState.error = SESSION_GONE; const auth = useAuth(); expect(auth.isAuthenticated).toBe(false); @@ -141,7 +157,7 @@ describe("useAuth — a verdict on a refetch", () => { store.set("healthlog-was-authenticated", "1"); queryState.data = { id: "u1", username: "someone" }; queryState.isError = true; - queryState.error = new ApiError("unauthorized", 401); + queryState.error = SESSION_GONE; const auth = useAuth(); expect(auth.isAuthenticated).toBe(false); @@ -149,6 +165,16 @@ describe("useAuth — a verdict on a refetch", () => { expect(auth.isAuthUnknown).toBe(false); }); + it("keeps the cached account through a 401 from something in front of the server", () => { + queryState.data = { id: "u1", username: "someone" }; + queryState.isError = true; + queryState.error = new ApiError("Unauthorized", 401); + + const auth = useAuth(); + expect(auth.isAuthenticated).toBe(true); + expect(auth.user).toEqual({ id: "u1", username: "someone" }); + }); + it("keeps the cached account through a network failure on a refetch", () => { queryState.data = { id: "u1", username: "someone" }; queryState.isError = true; diff --git a/src/hooks/use-auth.ts b/src/hooks/use-auth.ts index 9f8b58829..98d242366 100644 --- a/src/hooks/use-auth.ts +++ b/src/hooks/use-auth.ts @@ -338,18 +338,22 @@ function clearWasAuthenticated(): void { * True when an `/api/auth/me` failure carries NO verdict on the session — * a network-level failure (fetch `TypeError` offline, abort/timeout), the * service worker's synthetic 503 offline envelope (`meta.offline`), the - * browser reporting itself offline, or a server-side 5xx. None of these mean - * "logged out"; only an explicit 401/403 does. Treating them as session end - * used to redirect an offline relaunch to /auth/login and wipe the offline - * caches — destroying exactly the installed-PWA-opened-offline scenario the - * SW + query persister exist for. + * browser reporting itself offline, a server-side 5xx, or a 401/403 that + * HealthLog did not write. None of these mean "logged out"; only HealthLog's + * own 401/403 does. Treating them as session end used to redirect an offline + * relaunch to /auth/login and wipe the offline caches — destroying exactly + * the installed-PWA-opened-offline scenario the SW + query persister exist + * for. */ export function isAuthVerdictUnknown(error: unknown): boolean { if (error instanceof ApiError) { // 5xx (incl. the SW's synthetic 503 with `meta.offline: true`) and any - // other non-auth status: the server never ruled on the session. Only an - // explicit 401/403 is a verdict. - return error.status !== 401 && error.status !== 403; + // other non-auth status: the server never ruled on the session. + if (error.status !== 401 && error.status !== 403) return true; + // HealthLog's error envelope always names a code. A 401/403 without one + // comes from something in front of the instance (a forward-auth proxy, + // a firewall), which says nothing about this session. + return typeof error.meta?.errorCode !== "string"; } // Everything below the HTTP layer is transport-level by construction: // fetch `TypeError` (offline / DNS / connection refused), `AbortError` / @@ -528,7 +532,7 @@ export function useAuth() { }); // v1.27.x offline fix — classify a failed `/api/auth/me` before acting on - // it. Only an explicit 401/403 means "session ended"; a network-level + // it. Only HealthLog's own 401/403 means "session ended"; a network-level // failure (offline relaunch, SW 503 offline envelope, timeout) or a 5xx // carries no verdict, so the hook holds the LAST-KNOWN auth state (the // localStorage marker) instead of flipping to unauthenticated. Pre-fix, an @@ -536,8 +540,9 @@ export function useAuth() { // /auth/login and wiped every offline cache (data 7→0 entries, pages 1→0 // in the audit measurement), making the whole offline story unreachable. const verdictUnknown = query.isError && isAuthVerdictUnknown(query.error); - // A 401/403 on a refetch ends the session although the cache still holds - // the last good payload: React Query keeps `data` when a refetch fails. + // HealthLog's 401/403 on a refetch ends the session although the cache + // still holds the last good payload: React Query keeps `data` when a + // refetch fails. const sessionEnded = query.isError && !verdictUnknown; const user = sessionEnded ? null : (query.data ?? null); const isAuthenticated = !!user || (verdictUnknown && readWasAuthenticated());