From a81656bde1ba114a7cd8ffea81c41114cebd3bf3 Mon Sep 17 00:00:00 2001 From: GenWave Radio Date: Wed, 23 Sep 2026 16:52:29 -0600 Subject: [PATCH] feat(admin-ui): T566 one apiFetch wrapper owns 401 (STORY-475) lib/api-fetch.ts: a 401 posts /api/auth/logout (the cookie is HttpOnly), hard-navigates to /login?expired=1 and never settles; other non-ok statuses reject with the Response. The four lib 401 branches are gone. The About server component redirects to /session-expired, a route that clears genwave-auth and 307s to a relative /login?expired=1. The login page reads the flag. middleware.ts is unchanged (F208.2). --- admin-ui/__specs__/about-page.spec.tsx | 95 +++++- admin-ui/__specs__/api-fetch-401.spec.ts | 293 ++++++++++++++++++ admin-ui/__specs__/api-fetch-401.spec.tsx | 38 --- admin-ui/__specs__/catalog-rating.spec.tsx | 7 +- admin-ui/__specs__/shared-patch-hook.spec.tsx | 9 + .../__specs__/station-thumb-controls.spec.tsx | 9 +- admin-ui/app/(authed)/about/page.tsx | 11 +- admin-ui/app/login/page.tsx | 6 + admin-ui/app/session-expired/route.ts | 23 ++ admin-ui/lib/api-fetch.ts | 107 +++++++ admin-ui/lib/broadcast-api.ts | 21 +- admin-ui/lib/persona-taste-api.ts | 29 +- admin-ui/lib/station-thumb-api.ts | 27 +- admin-ui/lib/use-row-patch.ts | 36 +-- 14 files changed, 598 insertions(+), 113 deletions(-) create mode 100644 admin-ui/__specs__/api-fetch-401.spec.ts delete mode 100644 admin-ui/__specs__/api-fetch-401.spec.tsx create mode 100644 admin-ui/app/session-expired/route.ts create mode 100644 admin-ui/lib/api-fetch.ts diff --git a/admin-ui/__specs__/about-page.spec.tsx b/admin-ui/__specs__/about-page.spec.tsx index b7540c60..6f448e8e 100644 --- a/admin-ui/__specs__/about-page.spec.tsx +++ b/admin-ui/__specs__/about-page.spec.tsx @@ -5,27 +5,42 @@ // task that makes it green (T561). Each Given comment names the arrange the scenario needs. // // next/jest's SWC transform (unlike babel-jest) does not hoist jest.mock() calls above import -// statements (mirrors app-shell.spec.tsx's own header comment), so Sidebar — which calls the -// mocked next/navigation hook — is loaded via a dynamic `await import()` inside the test. +// statements (mirrors app-shell.spec.tsx's own header comment), so Sidebar and the page's own +// server component (app/(authed)/about/page.tsx) — both of which call a mocked next/navigation or +// next/headers export — are loaded via a dynamic `await import()` inside their tests or before* hooks. jest.mock("next/navigation", () => ({ usePathname: jest.fn(), + redirect: jest.fn(), +})); + +jest.mock("next/headers", () => ({ + cookies: jest.fn(), })); jest.mock("@/app/login/actions", () => ({ logout: jest.fn(), })); -import { describe, it, jest, expect } from "@jest/globals"; +import { describe, it, jest, expect, beforeAll, afterAll } from "@jest/globals"; import { render, screen } from "@testing-library/react"; import "@testing-library/jest-dom/jest-globals"; -import type { usePathname } from "next/navigation"; +import type { ReactNode } from "react"; +import type { redirect, usePathname } from "next/navigation"; +import type { cookies } from "next/headers"; import { AboutView } from "../app/(authed)/about/AboutView"; import type { AboutResponseDto } from "../lib/about-api"; -const mockedUsePathname = jest - .requireMock<{ usePathname: typeof usePathname }>("next/navigation") - .usePathname as jest.MockedFunction; +const mockedNextNavigation = jest.requireMock<{ + usePathname: typeof usePathname; + redirect: typeof redirect; +}>("next/navigation"); +const mockedUsePathname = mockedNextNavigation.usePathname as jest.MockedFunction; +const mockedRedirect = mockedNextNavigation.redirect as jest.MockedFunction; + +const mockedCookies = jest + .requireMock<{ cookies: typeof cookies }>("next/headers") + .cookies as jest.MockedFunction; // Given: a single fixture GET /api/about response — two attribution kinds, two packs, so AC4's // "shows every attribution name" fact can't pass vacuously on a one-item list. @@ -184,4 +199,70 @@ describe("Feature: About page", () => { }); }); + // The page's own 401/403 handling (SPEC F208.1, STORY-475, PLAN T566) — AboutView's fixture + // scenarios above don't exercise the server component (app/(authed)/about/page.tsx) itself, so + // GET /api/about's failure statuses need their own arrange. next/headers' cookies() is loaded + // via the mock registered above; the page module is loaded via a dynamic `await import()` + // (same reason as the Sidebar scenario: next/jest's SWC transform does not hoist jest.mock() + // above import statements). + describe("Scenario: a stale session (401)", () => { + let originalFetch: typeof fetch; + + beforeAll(async () => { + originalFetch = global.fetch; + mockedCookies.mockResolvedValue( + { toString: () => "genwave-auth=stale" } as unknown as Awaited> + ); + global.fetch = jest + .fn() + .mockResolvedValue({ + ok: false, + status: 401, + json: () => Promise.resolve({}), + } as unknown as Response) as unknown as typeof fetch; + + const { default: AboutPage } = await import("../app/(authed)/about/page"); + await AboutPage(); + }); + + afterAll(() => { + global.fetch = originalFetch; + }); + + it("apiGet 401 redirects to /session-expired", () => { + expect(mockedRedirect).toHaveBeenCalledWith("/session-expired"); + }); + }); + + describe("Scenario: a permission error (403)", () => { + let originalFetch: typeof fetch; + let node: ReactNode; + + beforeAll(async () => { + originalFetch = global.fetch; + mockedCookies.mockResolvedValue( + { toString: () => "genwave-auth=ok" } as unknown as Awaited> + ); + global.fetch = jest + .fn() + .mockResolvedValue({ + ok: false, + status: 403, + json: () => Promise.resolve({}), + } as unknown as Response) as unknown as typeof fetch; + + const { default: AboutPage } = await import("../app/(authed)/about/page"); + node = await AboutPage(); + }); + + afterAll(() => { + global.fetch = originalFetch; + }); + + it("apiGet 403 still renders the permission copy", () => { + render(<>{node}); + expect(screen.getByText("You do not have permission to view this page.")).toBeInTheDocument(); + }); + }); + }); diff --git a/admin-ui/__specs__/api-fetch-401.spec.ts b/admin-ui/__specs__/api-fetch-401.spec.ts new file mode 100644 index 00000000..edc92c10 --- /dev/null +++ b/admin-ui/__specs__/api-fetch-401.spec.ts @@ -0,0 +1,293 @@ +// STORY-475 — A stale cookie sends you to sign in (gh-#729 · SPEC F208 · PLAN T566) +// +// Runner: Jest (node environment — .ts extension, catalog-pages.spec.ts's own convention). +// Node, not jsdom, is deliberate: this suite exercises real `NextRequest`/`NextResponse` +// instances (the middleware + session-expired route-handler scenarios) — next/server's classes +// extend the platform `Request`/`Response`, which Node provides natively but +// jest-environment-jsdom's realm does not (jestjs/jest#13037). The login page's server-rendered +// output is inspected via the same recursive tree-walker catalog-pages.spec.ts/ +// settings-server.spec.ts use instead of RTL's render/screen — no DOM is needed to read string +// leaves out of a returned React element tree. +// +// Every specification below was RED (it.todo) at plan time; T566 turns each into a real `it`. +// +// next/jest's SWC transform (unlike babel-jest) does not hoist jest.mock() calls above import +// statements — ES import declarations are always evaluated first regardless of source position +// (mirrors app-shell.spec.tsx / settings-server.spec.ts's own header comments) — so +// app/login/page.tsx (which transitively calls next/headers' cookies()) is loaded via a dynamic +// `await import()` inside each "the login page" Given-describe below, after the mock has already +// registered. middleware.ts and app/session-expired/route.ts touch neither next/headers nor +// next/navigation, so both are safe to import statically. + +jest.mock("next/headers", () => ({ + cookies: jest.fn(), +})); + +import { describe, it, expect, jest, beforeEach, beforeAll } from "@jest/globals"; +import type { ReactNode } from "react"; +import { readFileSync, readdirSync, existsSync } from "node:fs"; +import path from "node:path"; +import { NextRequest } from "next/server"; +import type { NextResponse } from "next/server"; +import type { cookies } from "next/headers"; +import { apiFetch } from "@/lib/api-fetch"; +import type { NavigateFn } from "@/lib/api-fetch"; +import { middleware } from "../middleware"; +import { GET as sessionExpiredGet } from "../app/session-expired/route"; + +const mockedCookies = jest + .requireMock<{ cookies: typeof cookies }>("next/headers") + .cookies as jest.MockedFunction; + +// --------------------------------------------------------------------------- +// Tree walker (mirrors catalog-pages.spec.ts / settings-server.spec.ts) +// --------------------------------------------------------------------------- + +function collectStrings(node: ReactNode, out: string[] = []): string[] { + if (node === null || node === undefined || typeof node === "boolean") { + return out; + } + if (typeof node === "string" || typeof node === "number") { + out.push(String(node)); + return out; + } + if (Array.isArray(node)) { + for (const child of node) collectStrings(child, out); + return out; + } + const el = node as { type?: unknown; props?: Record }; + if (el && typeof el === "object" && el.props) { + if (el.props["children"] !== undefined) { + collectStrings(el.props["children"] as ReactNode, out); + } + } + return out; +} + +function treeContains(node: ReactNode, text: string): boolean { + return collectStrings(node).some((s) => s.includes(text)); +} + +// --------------------------------------------------------------------------- +// Other helpers +// --------------------------------------------------------------------------- + +/** A duck-typed fetch Response (this suite's `global.fetch` mocks follow the same shape as + * every other __specs__ fetch mock in this directory — a plain object, not a real `Response` + * instance — station-thumb-controls.spec.tsx's `installBoothLogFetchMock` is the house + * precedent). */ +function jsonResponse(status: number, body: unknown = {}): Response { + return { + ok: status >= 200 && status < 300, + status, + json: () => Promise.resolve(body), + } as unknown as Response; +} + +/** Drains the microtask queue enough for apiFetch's internal `await fetch(...)` (the request + * itself) and, on a 401, its second `await fetch(...)` (the logout POST) to both settle before a + * test inspects the mocks apiFetch drove. No real waiting/timers — apiFetch's 401 promise never + * settles (ruling #3), so there is nothing to `await` on the call itself. */ +async function flushMicrotasks(): Promise { + for (let i = 0; i < 10; i += 1) { + await Promise.resolve(); + } +} + +// --------------------------------------------------------------------------- +// Feature: A stale cookie sends you to sign in +// --------------------------------------------------------------------------- + +describe("Feature: A stale cookie sends you to sign in", () => { + describe("Scenario: a 401 through apiFetch", () => { + let fetchMock: jest.MockedFunction; + let navigate: jest.MockedFunction; + + // Given: fetch mocked 401 (then 200 for the follow-up logout POST), navigate injected + beforeEach(async () => { + fetchMock = jest + .fn() + .mockResolvedValueOnce(jsonResponse(401)) + .mockResolvedValueOnce(jsonResponse(200)); + global.fetch = fetchMock as unknown as typeof fetch; + navigate = jest.fn(); + + void apiFetch("/api/media/1/vote", { method: "POST", navigate }); + await flushMicrotasks(); + }); + + it("AC1 — the session cookie is cleared", () => { + expect(fetchMock.mock.calls[1]).toEqual([ + "/api/auth/logout", + expect.objectContaining({ method: "POST" }), + ]); + }); + + it("AC2 — the router navigates to /login?expired=1", () => { + expect(navigate).toHaveBeenCalledWith("/login?expired=1"); + }); + }); + + describe("Scenario: the login page with the flag", () => { + describe("Given expired=1 in the search params", () => { + let node: ReactNode; + + beforeAll(async () => { + mockedCookies.mockResolvedValue( + { get: () => undefined } as unknown as Awaited> + ); + const { default: LoginPage } = await import("../app/login/page"); + node = await LoginPage({ searchParams: Promise.resolve({ expired: "1" }) }); + }); + + it('AC3 — shows "Your session ended. Sign in again."', () => { + expect(treeContains(node, "Your session ended. Sign in again.")).toBe(true); + }); + }); + + describe("Given no expired flag in the search params", () => { + let node: ReactNode; + + beforeAll(async () => { + mockedCookies.mockResolvedValue( + { get: () => undefined } as unknown as Awaited> + ); + const { default: LoginPage } = await import("../app/login/page"); + node = await LoginPage({ searchParams: Promise.resolve({}) }); + }); + + it("the flag's text is absent", () => { + expect(treeContains(node, "Your session ended. Sign in again.")).toBe(false); + }); + }); + }); + + describe("Scenario: the former 401 sites", () => { + const ADMIN_UI_ROOT = path.resolve(__dirname, ".."); + const SCAN_DIRS = ["app", "lib", "components"]; + const SKIP_DIRS = new Set(["node_modules", ".next", "__specs__"]); + // Catches `=== 401`, `== 401`, `!== 401`, `!= 401`, `case 401`, and the reversed forms + // (`401 === x`, `401 == x`, `401 !== x`, `401 != x`) — any comparison shape a former 401 + // branch could have been written in (review finding, T566 round 1). + const STATUS_401_PATTERN = + /(?:={2,3}|!={1,2})\s*401\b|\b401\s*(?:={2,3}|!={1,2})|case\s+401\b/; + + function collectSourceFiles(dir: string, out: string[]): void { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + if (SKIP_DIRS.has(entry.name)) continue; + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + collectSourceFiles(full, out); + } else if (entry.name.endsWith(".ts") || entry.name.endsWith(".tsx")) { + out.push(full); + } + } + } + + let scannedFiles: string[]; + let offenders: string[]; + + // Given: admin-ui source scanned (app/, lib/, components/, middleware.ts) + beforeAll(() => { + scannedFiles = []; + for (const dir of SCAN_DIRS) { + const full = path.join(ADMIN_UI_ROOT, dir); + if (existsSync(full)) collectSourceFiles(full, scannedFiles); + } + const middlewarePath = path.join(ADMIN_UI_ROOT, "middleware.ts"); + if (existsSync(middlewarePath)) scannedFiles.push(middlewarePath); + + offenders = scannedFiles + .filter((file) => STATUS_401_PATTERN.test(readFileSync(file, "utf-8"))) + .map((file) => path.relative(ADMIN_UI_ROOT, file)) + .sort(); + }); + + it("AC4 — no `status === 401` branch outside api-fetch.ts", () => { + expect(offenders).toEqual(["lib/api-fetch.ts"]); + }); + + it("the scan is non-vacuous", () => { + expect(scannedFiles.length).toBeGreaterThan(50); + }); + }); + + describe("Scenario: the middleware", () => { + function makeRequest(pathname: string, cookieHeader?: string): NextRequest { + return new NextRequest(new URL(pathname, "http://localhost:3000"), { + headers: cookieHeader === undefined ? {} : { cookie: cookieHeader }, + }); + } + + describe("Given a request carrying the session cookie", () => { + let response: NextResponse; + + beforeAll(() => { + response = middleware(makeRequest("/dashboard", "genwave-auth=any-value")); + }); + + it("AC5 — still checks cookie presence only (a cookie passes through)", () => { + expect(response.status).toBe(200); + }); + }); + + describe("Given a request with no session cookie", () => { + let response: NextResponse; + + beforeAll(() => { + response = middleware(makeRequest("/dashboard")); + }); + + it("AC5 — still checks cookie presence only (no cookie redirects to /login)", () => { + expect(new URL(response.headers.get("location") ?? "").pathname).toBe("/login"); + }); + + it("the redirect is a 307", () => { + expect(response.status).toBe(307); + }); + }); + }); + + // ---- sad path ---- + describe("Scenario: a 500 through apiFetch", () => { + let navigate: jest.MockedFunction; + let rejection: unknown; + + // Given: fetch mocked 500 + beforeEach(async () => { + global.fetch = jest + .fn() + .mockResolvedValue(jsonResponse(500)) as unknown as typeof fetch; + navigate = jest.fn(); + + rejection = await apiFetch("/api/media/1/vote", { method: "POST", navigate }).then( + () => undefined, + (err: unknown) => err + ); + }); + + it("AC6 — rejects with the response", () => { + expect(rejection).toMatchObject({ status: 500 }); + }); + + it("AC6 — does not navigate", () => { + expect(navigate).not.toHaveBeenCalled(); + }); + }); + + describe("Scenario: the session-expired route handler", () => { + let response: NextResponse; + + beforeEach(() => { + response = sessionExpiredGet(); + }); + + it("GET deletes the cookie", () => { + expect(response.headers.get("set-cookie")).toBe("genwave-auth=; Path=/; Max-Age=0"); + }); + + it("GET redirects with a relative Location (not the container's bind address)", () => { + expect(response.headers.get("location")).toBe("/login?expired=1"); + }); + }); +}); diff --git a/admin-ui/__specs__/api-fetch-401.spec.tsx b/admin-ui/__specs__/api-fetch-401.spec.tsx deleted file mode 100644 index c414c831..00000000 --- a/admin-ui/__specs__/api-fetch-401.spec.tsx +++ /dev/null @@ -1,38 +0,0 @@ -// @jest-environment jsdom -// STORY-475 — A stale cookie sends you to sign in (gh-#729 · SPEC F208 · PLAN T566) -// -// BDD specification — Jest. RED at plan time: every specification is it.todo — turn it into an `it` only in the -// task that makes it green (T566). Each Given comment names the arrange the scenario needs. - -import { describe, it } from "@jest/globals"; - -describe("Feature: A stale cookie sends you to sign in", () => { - describe("Scenario: a 401 through apiFetch", () => { - // Given: fetch mocked 401, useRouter mocked, document.cookie seeded - it.todo("AC1 — the session cookie is cleared"); - it.todo("AC2 — the router navigates to /login?expired=1"); - }); - - describe("Scenario: the login page with the flag", () => { - // Given: /login?expired=1 rendered - it.todo("AC3 — shows \"Your session ended. Sign in again.\""); - }); - - describe("Scenario: the former 401 sites", () => { - // Given: admin-ui source scanned - it.todo("AC4 — no `status === 401` branch outside api-fetch.ts"); - }); - - describe("Scenario: the middleware", () => { - // Given: middleware.ts read - it.todo("AC5 — still checks cookie presence only"); - }); - - // ---- sad path ---- - describe("Scenario: a 500 through apiFetch", () => { - // Given: fetch mocked 500 - it.todo("AC6 — rejects with the response"); - it.todo("AC6 — does not navigate"); - }); - -}); diff --git a/admin-ui/__specs__/catalog-rating.spec.tsx b/admin-ui/__specs__/catalog-rating.spec.tsx index 7d1af2dc..31671d21 100644 --- a/admin-ui/__specs__/catalog-rating.spec.tsx +++ b/admin-ui/__specs__/catalog-rating.spec.tsx @@ -339,14 +339,17 @@ describe("Feature: Rating state in the Catalog page", () => { }); describe("Scenario: failures surface (sad path)", () => { + // 403, not 401 (STORY-475/gh-#729): a 401 is now apiFetch's own concern (clear the cookie, + // hand off to /login) — it never reaches this component as a toastable failure outcome, so + // this scenario proves the "still-surfaces" shape with a status apiFetch still lets through. it("a failed restore toasts the outcome and keeps the badge (F31.3)", async () => { - makeFetchMock({}, 401); + makeFetchMock({}, 403); await renderCatalogTable({ media: [makeRow({ mediaId: "7", neverPlay: true })] }); fireEvent.click(screen.getByRole("button", { name: "Restore to rotation" })); await waitFor(() => { - expect(screen.getByText("Your session has expired — sign in again.")).toBeInTheDocument(); + expect(screen.getByText("You don't have permission to make this change.")).toBeInTheDocument(); }); expect(screen.getByText("Never play")).toBeInTheDocument(); expect(screen.getByRole("button", { name: "Restore to rotation" })).toBeInTheDocument(); diff --git a/admin-ui/__specs__/shared-patch-hook.spec.tsx b/admin-ui/__specs__/shared-patch-hook.spec.tsx index 28c6cd13..5521ddd7 100644 --- a/admin-ui/__specs__/shared-patch-hook.spec.tsx +++ b/admin-ui/__specs__/shared-patch-hook.spec.tsx @@ -40,6 +40,7 @@ import { render, screen, fireEvent, act, waitFor, within } from "@testing-librar import "@testing-library/jest-dom/jest-globals"; import type { ComponentProps } from "react"; import type { useRouter } from "next/navigation"; +import { toast as sonnerToast } from "sonner"; import { ConfirmDialogProvider } from "@/components/ui/confirm-dialog"; import { Toaster } from "@/components/ui/toast"; import type { LibraryDto } from "@/lib/library"; @@ -551,6 +552,12 @@ describe("Feature: One shared PATCH hook with real feedback", () => { expect(screen.getByText("You don't have permission to make this change.")).toBeInTheDocument(); }); safeContent.unmount(); + // sonner's toast store is module-global and outlives a component's unmount (gh-#516, + // documented at jest.setup.ts) — jest.setup.ts's own afterEach only reaches between `it`s, + // not between the four sites this single `it` walks through in sequence, so each site + // dismisses its own toast before the next site's mounts and would otherwise + // replay it alongside the next site's own. + sonnerToast.dismiss(); // catalog detail form. makeFetchMock(403); @@ -571,6 +578,7 @@ describe("Feature: One shared PATCH hook with real feedback", () => { expect(screen.getByText("You don't have permission to make this change.")).toBeInTheDocument(); }); editForm.unmount(); + sonnerToast.dismiss(); // move-to-library. makeFetchMock(403); @@ -586,6 +594,7 @@ describe("Feature: One shared PATCH hook with real feedback", () => { expect(screen.getByText("You don't have permission to make this change.")).toBeInTheDocument(); }); moveAction.unmount(); + sonnerToast.dismiss(); // selection-toolbar: aggregates per-row outcomes into one summary toast instead of the // hook's own per-row toast (Q7 review) — still a visible, never-silent failure signal. diff --git a/admin-ui/__specs__/station-thumb-controls.spec.tsx b/admin-ui/__specs__/station-thumb-controls.spec.tsx index de8e129b..5707e187 100644 --- a/admin-ui/__specs__/station-thumb-controls.spec.tsx +++ b/admin-ui/__specs__/station-thumb-controls.spec.tsx @@ -354,9 +354,12 @@ describe("Feature: Station-thumb controls", () => { expect(stationDown).toHaveAttribute("aria-pressed", "false"); }); - it("401: toasts the house session-expired copy and marks nothing pressed", async () => { + // 403, not 401 (STORY-475/gh-#729): a 401 is now apiFetch's own concern (clear the cookie, + // hand off to /login) — it never reaches this component as a toastable failure outcome, so + // this scenario proves the "still-surfaces" shape with a status apiFetch still lets through. + it("403: toasts the forbidden copy and marks nothing pressed", async () => { installBoothLogFetchMock( - defaultBoothLogState({ stationThumb: ok(undefined, 401) }) + defaultBoothLogState({ stationThumb: ok(undefined, 403) }) ); renderBoothLog(); @@ -365,7 +368,7 @@ describe("Feature: Station-thumb controls", () => { const stationUp = screen.getByRole("button", { name: "Station thumbs up" }); await clickAndSettle(stationUp); - expect(screen.getByText("Your session has expired — sign in again.")).toBeInTheDocument(); + expect(screen.getByText("You don't have permission to make this change.")).toBeInTheDocument(); expect(stationUp).toBeEnabled(); expect(stationUp).toHaveAttribute("aria-pressed", "false"); }); diff --git a/admin-ui/app/(authed)/about/page.tsx b/admin-ui/app/(authed)/about/page.tsx index ba38daa2..98ddf17f 100644 --- a/admin-ui/app/(authed)/about/page.tsx +++ b/admin-ui/app/(authed)/about/page.tsx @@ -1,7 +1,9 @@ import type { ReactNode } from "react"; import { cookies } from "next/headers"; +import { redirect } from "next/navigation"; import { apiGet } from "@/lib/api"; import { isAboutResponseDto } from "@/lib/about-api"; +import { isUnauthorizedStatus, SESSION_EXPIRED_PATH } from "@/lib/api-fetch"; import { AboutView } from "./AboutView"; // The build/station/library/uptime/attribution facts (SPEC F207.1, F207.2; STORY-474; PLAN T561) @@ -29,7 +31,14 @@ export default async function AboutPage(): Promise { const response = await apiGet("/api/about", { cookies: cookieStr }); - if (response.status === 401 || response.status === 403) { + // A stale cookie: one owner for the 401 -> sign-out contract (SPEC F208.1, STORY-475). A + // Server Component render can't clear the cookie itself, so this hands off to a route handler + // that can (app/session-expired/route.ts). + if (isUnauthorizedStatus(response.status)) { + redirect(SESSION_EXPIRED_PATH); + } + + if (response.status === 403) { return ; } diff --git a/admin-ui/app/login/page.tsx b/admin-ui/app/login/page.tsx index 2435a27d..89890677 100644 --- a/admin-ui/app/login/page.tsx +++ b/admin-ui/app/login/page.tsx @@ -21,12 +21,18 @@ export default async function LoginPage({ const params = await searchParams; const returnTo = safeReturnTo(params["return"]); + // The one place `?expired=1` is read (STORY-475 ruling #6) — apiFetch (client-side 401) and + // app/session-expired/route.ts (server-side 401) both land here with it set. + const expired = params["expired"] === "1"; return (

GenWave

Sign in

+ {expired && ( +

Your session ended. Sign in again.

+ )}
diff --git a/admin-ui/app/session-expired/route.ts b/admin-ui/app/session-expired/route.ts new file mode 100644 index 00000000..a7d88642 --- /dev/null +++ b/admin-ui/app/session-expired/route.ts @@ -0,0 +1,23 @@ +import { NextResponse } from "next/server"; +import { LOGIN_EXPIRED_PATH, SESSION_COOKIE } from "@/lib/api-fetch"; + +// Route handler (STORY-475 ruling #5) a server-rendered 401 (about/page.tsx) redirects to — a +// Server Component render can't set/delete a cookie itself (see next/headers' `cookies()` docs: +// `.set`/`.delete` only work in a Server Function or Route Handler). Deliberately NOT under +// `/api` — that prefix is rewritten to the C# backend (next.config.ts), so a route handler there +// would never run. Deletes the stale session cookie and lands on the same `LOGIN_EXPIRED_PATH` the +// client-side `apiFetch` wrapper (`lib/api-fetch.ts`) uses (SPEC F208.1) — both constants are +// imported from there, not repeated here. +// +// A relative Location, not `new URL(path, request.url)`: in the standalone image that resolves +// against the 0.0.0.0 bind address, not the browser's Host. The browser resolves this one against +// the origin it is already on. + +export function GET(): NextResponse { + const response = new NextResponse(null, { + status: 307, + headers: { Location: LOGIN_EXPIRED_PATH }, + }); + response.cookies.set(SESSION_COOKIE, "", { path: "/", maxAge: 0 }); + return response; +} diff --git a/admin-ui/lib/api-fetch.ts b/admin-ui/lib/api-fetch.ts new file mode 100644 index 00000000..39ac32a4 --- /dev/null +++ b/admin-ui/lib/api-fetch.ts @@ -0,0 +1,107 @@ +// One wrapper that owns the 401 -> sign-out contract (SPEC F208.1, STORY-475, PLAN T566, +// gh-#729). Every client-side fetch call that carries the session cookie goes through here +// instead of a bare `fetch`: a 401 means the cookie went stale (expired session, revoked token, +// server restart), and the wrong place to relearn that is five call sites each toasting their own +// "unauthorized" message. `isUnauthorizedStatus` below is the only other exported way to test the +// `401` literal (about/page.tsx, a Server Component, uses it instead of holding the literal +// itself) — this file is the one place in admin-ui left holding it +// (__specs__/api-fetch-401.spec.ts's source scan, AC4, enforces that). +// +// Must stay importable from a Server Component: no top-level `window`/`document` access. `fetch` +// is a global in both worlds, so `clearSessionCookie` below is safe at any scope; only +// `defaultNavigate` reaches for `window`, and it does so from inside a function body — never +// evaluated at module-load time — so importing this module server-side never touches it. + +/** Injectable seam over `window.location.assign` (STORY-475 ruling #2). Production callers never + * set this — every real `apiFetch` call falls through to the default below. jsdom does not + * implement `location.assign`, so a spec MUST inject a fake here rather than let a 401 reach it. */ +export type NavigateFn = (url: string) => void; + +function defaultNavigate(url: string): void { + window.location.assign(url); +} + +export interface ApiFetchOptions extends RequestInit { + /** Test seam only (STORY-475 ruling #2) — production call sites never set this. */ + navigate?: NavigateFn; +} + +/** `/login?expired=1` — the one URL every 401 path (this module's own `navigate`, and + * app/session-expired/route.ts's redirect) lands the browser on. Exported so route.ts doesn't + * hold its own copy of the literal. */ +export const LOGIN_EXPIRED_PATH = "/login?expired=1"; + +/** `genwave-auth` — the HttpOnly session cookie's name. Exported so the one other place that + * clears it (app/session-expired/route.ts's route handler) doesn't hold its own copy. */ +export const SESSION_COOKIE = "genwave-auth"; + +/** The route a Server Component's 401 hands off to (STORY-475 ruling #5) — a route handler can + * clear the cookie; a Server Component render can't. Exported so about/page.tsx's `redirect()` + * call doesn't repeat the path as a bare string. */ +export const SESSION_EXPIRED_PATH = "/session-expired"; + +/** Best-effort session-cookie clear (STORY-475 ruling #1). `genwave-auth` is HttpOnly, so client + * JS can't delete it directly — POST /api/auth/logout (`AuthController.Logout`, `[AllowAnonymous]`) + * runs ASP.NET's `SignOutAsync`, and its expiring `Set-Cookie` reaches the browser through the + * `/api/:path*` same-origin rewrite exactly like every other `/api/*` call in this directory (a + * direct browser fetch, unlike `app/login/actions.ts`'s server-side `logout()` action, which has + * to forward `Set-Cookie` by hand because IT runs on the server). A failed POST is swallowed — the + * operator is leaving for /login either way, and a cookie the browser still holds doesn't stop the + * login page from working. */ +async function clearSessionCookie(): Promise { + try { + await fetch("/api/auth/logout", { method: "POST", credentials: "include" }); + } catch { + // best-effort — navigate regardless (ruling #1) + } +} + +/** True for the one status this module owns — the only place in admin-ui allowed to hold the + * `401` literal (SPEC F208.1). A caller that needs to special-case a 401 outside a plain + * `apiFetch` call (about/page.tsx's Server Component render, which can't itself clear a cookie or + * call `apiFetch`) tests through this instead of repeating the literal itself. */ +export function isUnauthorizedStatus(status: number): boolean { + return status === 401; +} + +/** True when `err` is the failed `Response` {@link apiFetch} rejects a non-2xx, non-401 call with + * (AC6) — as opposed to a thrown network error, which propagates unchanged from the underlying + * `fetch` and carries no `status`. Every migrated call site's catch block uses this to tell the + * two apart and classify exactly as it did before this module owned the fetch. */ +export function isApiFetchResponseError(err: unknown): err is Response { + return typeof err === "object" && err !== null && "status" in err; +} + +/** + * Fetch wrapper every session-cookie-carrying client call should use instead of bare `fetch` + * (SPEC F208.1). Resolves with `response` for a 2xx. For any other non-401 status it REJECTS with + * `response` (AC6) — a caller that used to branch on `response.ok` now catches instead, reading + * `.status` off whatever it caught (via {@link isApiFetchResponseError}) to classify the failure + * exactly as before. A network failure (no response at all) still rejects exactly as `fetch` does + * — it never satisfies {@link isApiFetchResponseError}, so a caller's classification is unchanged. + * + * A 401 is handled here and only here: clear the session cookie, then hard-navigate to + * `/login?expired=1`. The returned promise never settles in that case (ruling #3) — the page is on + * its way out, so there is nothing for a caller to do with a resolved/rejected value, and letting + * the promise settle risks an error toast flickering on screen for the instant before the + * navigation completes. + */ +export async function apiFetch( + input: RequestInfo | URL, + init: ApiFetchOptions = {} +): Promise { + const { navigate = defaultNavigate, ...requestInit } = init; + const response = await fetch(input, requestInit); + if (response.ok) return response; + + if (isUnauthorizedStatus(response.status)) { + await clearSessionCookie(); + navigate(LOGIN_EXPIRED_PATH); + // Never settles — see the doc comment above (ruling #3). + return new Promise(() => { + /* the page is navigating away; nothing ever resolves or rejects this */ + }); + } + + throw response; +} diff --git a/admin-ui/lib/broadcast-api.ts b/admin-ui/lib/broadcast-api.ts index 9ef1ade6..689d2a66 100644 --- a/admin-ui/lib/broadcast-api.ts +++ b/admin-ui/lib/broadcast-api.ts @@ -6,6 +6,7 @@ // exists in a Server Component/Route Handler request context). import type { GardenerStatusSummary } from "@/lib/gardener-api"; +import { apiFetch, isApiFetchResponseError } from "@/lib/api-fetch"; interface NowPlayingTrackWire { stationId: string; @@ -155,7 +156,7 @@ export type VoteDirection = "up" | "down"; * clamped increment and a never-play set is idempotent (F33.3/F33.4) — neither has an `If-Match` * to violate. */ -export type RatingFailureKind = "unauthorized" | "forbidden" | "not-found" | "network" | "unknown"; +export type RatingFailureKind = "forbidden" | "not-found" | "network" | "unknown"; /** * User-facing copy for a classified rating-mutation failure (SPEC F31.3) — the single source of @@ -165,8 +166,6 @@ export type RatingFailureKind = "unauthorized" | "forbidden" | "not-found" | "ne */ export function describeRatingFailure(kind: RatingFailureKind, status: number | null): string { switch (kind) { - case "unauthorized": - return "Your session has expired — sign in again."; case "forbidden": return "You don't have permission to make this change."; case "not-found": @@ -212,8 +211,6 @@ export type ExplicitOverrideOutcome = ExplicitOverrideSuccess | RatingFailure; function classifyRatingStatus(status: number): RatingFailureKind { switch (status) { - case 401: - return "unauthorized"; case 403: return "forbidden"; case 404: @@ -228,7 +225,9 @@ function classifyRatingStatus(status: number): RatingFailureKind { * `setNeverPlay`, `setExplicitOverride`) follows: POST/PUT a JSON `body`, a network failure or a * non-2xx response both resolve to a classified {@link RatingFailure} — never throws, the whole * point of the idiom — and a 2xx response's JSON is handed to `parseSuccess` to build the caller's - * specific success shape. One definition so the three call sites can't drift apart on it. + * specific success shape. One definition so the three call sites can't drift apart on it. Goes + * through `apiFetch` (STORY-475, SPEC F208.1) rather than a bare `fetch`: a 401 is that module's + * sole concern (clear the cookie, hand off to /login), never reaches this function's classifier. */ async function writeRatingMutation( path: string, @@ -238,18 +237,18 @@ async function writeRatingMutation( ): Promise<({ ok: true } & T) | RatingFailure> { let response: Response; try { - response = await fetch(path, { + response = await apiFetch(path, { method, credentials: "include", headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), }); - } catch { + } catch (err) { + if (isApiFetchResponseError(err)) { + return { ok: false, kind: classifyRatingStatus(err.status), status: err.status }; + } return { ok: false, kind: "network", status: null }; } - if (!response.ok) { - return { ok: false, kind: classifyRatingStatus(response.status), status: response.status }; - } return { ok: true, ...parseSuccess(await response.json()) }; } diff --git a/admin-ui/lib/persona-taste-api.ts b/admin-ui/lib/persona-taste-api.ts index 760fe1ae..c8a1a96c 100644 --- a/admin-ui/lib/persona-taste-api.ts +++ b/admin-ui/lib/persona-taste-api.ts @@ -7,6 +7,8 @@ // convention as lib/broadcast-api.ts and lib/booth-log-api.ts — never lib/api.ts's apiGet, which // is server-only. +import { apiFetch, isApiFetchResponseError } from "@/lib/api-fetch"; + export type TasteThumbDirection = "up" | "down"; /** Failure buckets a taste-thumb POST classifies a non-2xx/network outcome into (SPEC F31.3 @@ -15,7 +17,6 @@ export type TasteThumbDirection = "up" | "down"; * `detail` rather than fixed copy, since the three cases read very differently to an operator. */ export type TasteThumbFailureKind = | "not-thumbable" - | "unauthorized" | "forbidden" | "not-found" | "network" @@ -48,8 +49,6 @@ function classifyTasteThumbStatus(status: number): TasteThumbFailureKind { switch (status) { case 400: return "not-thumbable"; - case 401: - return "unauthorized"; case 403: return "forbidden"; case 404: @@ -82,8 +81,6 @@ export function describeTasteThumbFailure(outcome: TasteThumbFailure): string { switch (outcome.kind) { case "not-thumbable": return outcome.detail ?? "This row can't be thumbed for taste."; - case "unauthorized": - return "Your session has expired — sign in again."; case "forbidden": return "You don't have permission to make this change."; case "not-found": @@ -108,23 +105,25 @@ export async function postTasteThumb( ): Promise { let response: Response; try { - response = await fetch(`/api/booth-log/${boothLogRowId}/taste-thumb`, { + // apiFetch (STORY-475, SPEC F208.1) owns 401 — a stale cookie never reaches + // classifyTasteThumbStatus below. + response = await apiFetch(`/api/booth-log/${boothLogRowId}/taste-thumb`, { method: "POST", credentials: "include", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ direction }), }); - } catch { + } catch (err) { + if (isApiFetchResponseError(err)) { + return { + ok: false, + kind: classifyTasteThumbStatus(err.status), + status: err.status, + detail: await readDetail(err), + }; + } return { ok: false, kind: "network", status: null, detail: null }; } - if (!response.ok) { - return { - ok: false, - kind: classifyTasteThumbStatus(response.status), - status: response.status, - detail: await readDetail(response), - }; - } const body = (await response.json()) as { alreadyRecorded: boolean; weight: number | null }; return { ok: true, alreadyRecorded: body.alreadyRecorded, weight: body.weight }; } diff --git a/admin-ui/lib/station-thumb-api.ts b/admin-ui/lib/station-thumb-api.ts index 3878cb46..9e3bbfb5 100644 --- a/admin-ui/lib/station-thumb-api.ts +++ b/admin-ui/lib/station-thumb-api.ts @@ -7,6 +7,7 @@ // same convention as lib/persona-taste-api.ts. import { readErrorMessage } from "@/lib/problem-details"; +import { apiFetch, isApiFetchResponseError } from "@/lib/api-fetch"; export type StationThumbDirection = "up" | "down"; @@ -49,18 +50,16 @@ export type StationThumbOutcome = StationThumbSuccess | StationThumbFailure; /** User-facing copy for a classified station-thumb failure (SPEC F31.3 posture, mirrors * lib/persona-taste-api.ts's describeTasteThumbFailure — same wording for the buckets the two - * share). A 401 always reads as session-expiry regardless of whatever body the framework's own - * auth challenge attached; a network failure gets the house network copy; 403/404 get the same - * fixed copy every other mutation module in this directory uses; everything else (400 and - * anything unclassified) prefers the server's own `detail` — for the 400 case it already names - * the row's own kind (F150.8), app-authored vocabulary the operator can act on directly — falling - * back to a generic message only when none arrived. */ + * share). A 401 never reaches this function — apiFetch (SPEC F208.1) owns it and navigates away + * before a caller ever sees a failure outcome. A network failure gets the house network copy; + * 403/404 get the same fixed copy every other mutation module in this directory uses; everything + * else (400 and anything unclassified) prefers the server's own `detail` — for the 400 case it + * already names the row's own kind (F150.8), app-authored vocabulary the operator can act on + * directly — falling back to a generic message only when none arrived. */ export function describeStationThumbFailure(outcome: StationThumbFailure): string { switch (outcome.status) { case 0: return "Network error — check your connection."; - case 401: - return "Your session has expired — sign in again."; case 403: return "You don't have permission to make this change."; case 404: @@ -89,18 +88,20 @@ export async function postStationThumb( ): Promise { let response: Response; try { - response = await fetch(`/api/booth-log/${boothLogId}/station-thumb`, { + // apiFetch (STORY-475, SPEC F208.1) owns 401 — a stale cookie never reaches the status + // checks below. + response = await apiFetch(`/api/booth-log/${boothLogId}/station-thumb`, { method: "POST", credentials: "include", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ direction }), }); - } catch { + } catch (err) { + if (isApiFetchResponseError(err)) { + return { ok: false, status: err.status, detail: await readErrorMessage(err) }; + } return { ok: false, status: 0, detail: null }; } - if (!response.ok) { - return { ok: false, status: response.status, detail: await readErrorMessage(response) }; - } try { const raw = (await response.json()) as unknown; const result = typeof raw === "object" && raw !== null ? (raw as Record)["result"] : undefined; diff --git a/admin-ui/lib/use-row-patch.ts b/admin-ui/lib/use-row-patch.ts index 5c205f80..925d8a52 100644 --- a/admin-ui/lib/use-row-patch.ts +++ b/admin-ui/lib/use-row-patch.ts @@ -2,6 +2,7 @@ import { useCallback } from "react"; import { toast } from "@/components/ui/toast"; +import { apiFetch, isApiFetchResponseError } from "@/lib/api-fetch"; /** A row this hook can PATCH — the id plus the bare version (Postgres xmin, no `W/"…"` wrapper) * used to build the `If-Match` header. */ @@ -15,7 +16,6 @@ export interface RowPatchTarget { * `"unknown"` — callers with a site-specific case for a code outside this set handle it via * `describeFailure`. */ export type RowPatchFailureKind = - | "unauthorized" | "forbidden" | "not-found" | "conflict" @@ -46,8 +46,6 @@ export type RowPatchOutcome = RowPatchSuccess | RowPatchFailure; function classifyStatus(status: number): RowPatchFailureKind { switch (status) { - case 401: - return "unauthorized"; case 403: return "forbidden"; case 404: @@ -64,8 +62,6 @@ function classifyStatus(status: number): RowPatchFailureKind { function defaultFailureMessage(failure: RowPatchFailure): string { switch (failure.kind) { - case "unauthorized": - return "Your session has expired — sign in again."; case "forbidden": return "You don't have permission to make this change."; case "not-found": @@ -149,7 +145,9 @@ export function useRowPatch(options: UseRowPatchOptions = {}): UseRowPatchResult async (target: RowPatchTarget, body: Record): Promise => { let response: Response; try { - response = await fetch(`/api/media/${target.mediaId}`, { + // apiFetch (STORY-475, SPEC F208.1) owns 401 — a stale cookie never reaches the classify + // step below, since apiFetch clears it and hands off to /login itself. + response = await apiFetch(`/api/media/${target.mediaId}`, { method: "PATCH", headers: { "Content-Type": "application/json", @@ -157,27 +155,19 @@ export function useRowPatch(options: UseRowPatchOptions = {}): UseRowPatchResult }, body: JSON.stringify(body), }); - } catch { - const failure: RowPatchFailure = { ok: false, kind: "network", status: null }; + } catch (err) { + const failure: RowPatchFailure = isApiFetchResponseError(err) + ? { ok: false, kind: classifyStatus(err.status), status: err.status } + : { ok: false, kind: "network", status: null }; if (notify) toast.error(describeFailure?.(failure) ?? defaultFailureMessage(failure)); + if (failure.kind === "conflict") onConflict?.(target); return failure; } - if (response.ok) { - const etagHeader = response.headers.get("etag"); - const version = etagHeader !== null ? stripWeakETag(etagHeader) : target.version; - const responseBody: unknown = await response.json().catch(() => null); - return { ok: true, version, body: responseBody }; - } - - const failure: RowPatchFailure = { - ok: false, - kind: classifyStatus(response.status), - status: response.status, - }; - if (notify) toast.error(describeFailure?.(failure) ?? defaultFailureMessage(failure)); - if (failure.kind === "conflict") onConflict?.(target); - return failure; + const etagHeader = response.headers.get("etag"); + const version = etagHeader !== null ? stripWeakETag(etagHeader) : target.version; + const responseBody: unknown = await response.json().catch(() => null); + return { ok: true, version, body: responseBody }; }, [notify, onConflict, describeFailure] );