From 6df1452be1e900d681b4f997d7be6124bf29bac9 Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 17 Aug 2026 19:24:21 -0400 Subject: [PATCH 1/2] fix(frontend): /api-docs is public, and the allowlist is now derived from app/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The API reference page shipped correct in every respect except one: `PUBLIC_ROUTES` in `auth-provider.tsx` did not list it, so the provider mounted in the root layout redirected every visitor to `/login` — the opposite of the page's whole purpose, which is that an integrator can read the contract before they have credentials. Nothing caught it, and the reason is worth recording. The page's own tests render the component directly, so they never mount the provider. The route-coverage test asserts the backend serves the document. And the deployment probe was HTTP-level, where the redirect is invisible: the server returns 200 and the bounce happens in the browser. Three guards, none of them positioned to see it. So the fix is not only the missing entry. `auth-provider.test.tsx` now DERIVES the expected set by reading `app/` — a top-level route directory that is neither the dashboard group nor `/login` is public by construction and must appear in `PUBLIC_ROUTES` — plus a second test asserting that every route the list names does not in fact redirect. Both directions, so neither a new public page nor a stale entry can pass silently. Mutation-checked: with `/api-docs` removed the first test fails with `app/api-docs/ renders outside the dashboard group but is not in PUBLIC_ROUTES`. `fonts/` is excluded from the scan as an asset directory, not a route. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/auth-provider.test.tsx | 43 ++++++++++++++++++- frontend/components/auth-provider.tsx | 12 +++++- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/frontend/components/__tests__/auth-provider.test.tsx b/frontend/components/__tests__/auth-provider.test.tsx index 9c780b3e..1e39b9a2 100644 --- a/frontend/components/__tests__/auth-provider.test.tsx +++ b/frontend/components/__tests__/auth-provider.test.tsx @@ -15,7 +15,7 @@ import { act, render, waitFor } from "@testing-library/react"; import { http, HttpResponse } from "msw"; import { describe, it, expect, beforeEach, vi } from "vitest"; import { server } from "../../test/msw/server"; -import { AuthProvider, useAuth } from "../auth-provider"; +import { AuthProvider, PUBLIC_ROUTES, useAuth } from "../auth-provider"; // ── Next.js navigation mocks ────────────────────────────────────────────────── const mockReplace = vi.fn(); @@ -262,3 +262,44 @@ describe("AuthProvider — silent refresh on page load", () => { expect(localStorage.getItem(TOKEN_KEY)).toBe(JSON.stringify(sameAcctToken)); }); }); + +/** + * The public-route allowlist, derived rather than enumerated. + * + * `/api-docs` shipped outside `app/(dashboard)/`, fetching without a token, and was still unreachable — + * absent from `PUBLIC_ROUTES`, the provider redirected it to `/login`. Nothing caught it: the page's own + * tests render the component directly, and an HTTP probe returns 200 because the redirect is client-side. + * + * So this reads `app/` instead of restating the list. A top-level route that is neither the authenticated + * dashboard group nor `/login` is public by construction, and must appear in `PUBLIC_ROUTES`. + */ +describe("PUBLIC_ROUTES", () => { + const AUTHENTICATED = new Set(["(dashboard)", "login"]); + + it("covers every top-level route outside the dashboard group", async () => { + const { readdirSync } = await import("node:fs"); + const { join } = await import("node:path"); + const discovered = readdirSync(join(process.cwd(), "app"), { withFileTypes: true }) + .filter((e) => e.isDirectory() && !AUTHENTICATED.has(e.name) && e.name !== "fonts") + .map((e) => `/${e.name}`); + + // The fixture is only meaningful if it found the directories at all. + expect(discovered).toContain("/api-docs"); + for (const route of discovered) { + expect(PUBLIC_ROUTES, `app${route}/ renders outside the dashboard group but is not in PUBLIC_ROUTES`) + .toContain(route); + } + // `/` is the root page, which has no directory of its own. + expect(PUBLIC_ROUTES).toContain("/"); + }); + + it("does not redirect any route it lists", async () => { + for (const route of PUBLIC_ROUTES) { + mockReplace.mockClear(); + mockPathname.mockReturnValue(route); + renderProvider(); + await new Promise((r) => setTimeout(r, 20)); + expect(mockReplace, `${route} is listed as public but redirected`).not.toHaveBeenCalled(); + } + }); +}); diff --git a/frontend/components/auth-provider.tsx b/frontend/components/auth-provider.tsx index b355037e..d32c49a9 100644 --- a/frontend/components/auth-provider.tsx +++ b/frontend/components/auth-provider.tsx @@ -6,7 +6,17 @@ import { usePathname, useRouter } from "next/navigation"; const TOKEN_KEY = "ww_token"; const USER_KEY = "ww_user"; const API_BASE = (process.env.NEXT_PUBLIC_API_BASE_URL ?? "").trim().replace(/\/+$/, ""); -const PUBLIC_ROUTES = ["/", "/sandbox"]; +/** + * Routes that render without a session, and therefore must not trigger a silent refresh or a redirect + * to `/login`. + * + * This list is the authority for the whole app, and it is easy to forget: a page can be correct in every + * other respect — outside `app/(dashboard)/`, fetching without a bearer token — and still be unreachable + * because it is absent here. `/api-docs` shipped that way (ADR-068), and an HTTP probe could not see it, + * since the redirect is client-side and the server still returns 200. `auth-provider.test.tsx` derives + * the expected set from `app/` so a new public route cannot be added without landing here. + */ +export const PUBLIC_ROUTES = ["/", "/sandbox", "/api-docs"]; type AuthUser = { email: string; From da3af091e637621d76fd39e6f629cae1259cafa2 Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 17 Aug 2026 21:38:27 -0400 Subject: [PATCH 2/2] fix(frontend): resolve real App Router URLs before checking the public allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (Codex P2) caught that the guard treated every top-level directory name as a URL segment, which the App Router does not. A public page at `app/(public)/help/page.tsx` would have made the test demand a nonexistent `/(public)` entry while never checking `/help` — so the bogus entry satisfies the guard and `/help` still redirects to login. The regression test would have passed over the very defect it exists to catch, one layer removed from the original bug. The walk now recurses for real `page` files and resolves URLs the way the router does: a route group contributes no segment, `(dashboard)` marks everything beneath it authenticated however deeply nested, and a dynamic segment is skipped because prefix matching means its static ancestor is what must be listed. Two further changes the fix made worth making. `isPublicRoute` is extracted and exported, so the test asserts against the provider's own matching instead of a reimplementation that would agree with itself. And a second assertion covers the converse — nothing in the allowlist may exempt the authenticated group — because a single `/` entry would satisfy the coverage direction while unlocking the whole app. Mutation-checked both ways, and the route-group case explicitly: with `app/(public)/help/page.tsx` present the guard fails with `app serves /help outside (dashboard) but it is not public`, naming the real URL; adding `/programs` to the allowlist fails with `/programs is inside (dashboard) but PUBLIC_ROUTES exempts it`. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/auth-provider.test.tsx | 82 ++++++++++++++----- frontend/components/auth-provider.tsx | 21 +++-- 2 files changed, 79 insertions(+), 24 deletions(-) diff --git a/frontend/components/__tests__/auth-provider.test.tsx b/frontend/components/__tests__/auth-provider.test.tsx index 1e39b9a2..176b0b90 100644 --- a/frontend/components/__tests__/auth-provider.test.tsx +++ b/frontend/components/__tests__/auth-provider.test.tsx @@ -11,11 +11,13 @@ */ import React from "react"; +import { readdirSync } from "node:fs"; +import { join } from "node:path"; import { act, render, waitFor } from "@testing-library/react"; import { http, HttpResponse } from "msw"; import { describe, it, expect, beforeEach, vi } from "vitest"; import { server } from "../../test/msw/server"; -import { AuthProvider, PUBLIC_ROUTES, useAuth } from "../auth-provider"; +import { AuthProvider, PUBLIC_ROUTES, isPublicRoute, useAuth } from "../auth-provider"; // ── Next.js navigation mocks ────────────────────────────────────────────────── const mockReplace = vi.fn(); @@ -270,27 +272,69 @@ describe("AuthProvider — silent refresh on page load", () => { * absent from `PUBLIC_ROUTES`, the provider redirected it to `/login`. Nothing caught it: the page's own * tests render the component directly, and an HTTP probe returns 200 because the redirect is client-side. * - * So this reads `app/` instead of restating the list. A top-level route that is neither the authenticated - * dashboard group nor `/login` is public by construction, and must appear in `PUBLIC_ROUTES`. + * So this walks `app/` for real `page` files instead of restating the list. Directory names are NOT URL + * segments (review): a route group `(public)` contributes nothing to the URL, so scanning top-level + * directory names would demand a nonexistent `/(public)` entry while never checking the `/help` a reader + * would actually visit. Routes are resolved the way the App Router resolves them, and then checked + * against the provider's own `isPublicRoute` rather than a reimplementation of its matching. */ describe("PUBLIC_ROUTES", () => { - const AUTHENTICATED = new Set(["(dashboard)", "login"]); - - it("covers every top-level route outside the dashboard group", async () => { - const { readdirSync } = await import("node:fs"); - const { join } = await import("node:path"); - const discovered = readdirSync(join(process.cwd(), "app"), { withFileTypes: true }) - .filter((e) => e.isDirectory() && !AUTHENTICATED.has(e.name) && e.name !== "fonts") - .map((e) => `/${e.name}`); - - // The fixture is only meaningful if it found the directories at all. - expect(discovered).toContain("/api-docs"); - for (const route of discovered) { - expect(PUBLIC_ROUTES, `app${route}/ renders outside the dashboard group but is not in PUBLIC_ROUTES`) - .toContain(route); + /** A route group — `(dashboard)` — wraps pages without contributing a URL segment. */ + const isRouteGroup = (name: string) => name.startsWith("(") && name.endsWith(")"); + /** `[id]` / `[...slug]`. Matching is prefix-based, so the static ancestor is what must be listed. */ + const isDynamic = (name: string) => name.startsWith("["); + /** The one authenticated group. A route inside it needs a session by design. */ + const AUTHENTICATED_GROUP = "(dashboard)"; + + /** Every URL `app/` actually serves, paired with whether it sits inside the authenticated group. */ + function appRoutes(): Array<{ url: string; authenticated: boolean }> { + const found: Array<{ url: string; authenticated: boolean }> = []; + + const walk = (dir: string, segments: string[], authenticated: boolean) => { + const entries = readdirSync(dir, { withFileTypes: true }); + if (entries.some((e) => e.isFile() && /^page\.(tsx|ts|jsx|js)$/.test(e.name))) { + found.push({ url: `/${segments.join("/")}`.replace(/\/+$/, "") || "/", authenticated }); + } + for (const entry of entries) { + if (!entry.isDirectory()) continue; + if (isRouteGroup(entry.name)) { + // Contributes no URL segment — but `(dashboard)` marks everything beneath it as authenticated. + walk(join(dir, entry.name), segments, authenticated || entry.name === AUTHENTICATED_GROUP); + continue; + } + // A dynamic segment is covered by its static ancestor, since matching is prefix-based. + if (isDynamic(entry.name)) continue; + walk(join(dir, entry.name), [...segments, entry.name], authenticated); + } + }; + + walk(join(process.cwd(), "app"), [], false); + return found; + } + + it("covers every route app/ serves outside the authenticated group", () => { + const routes = appRoutes(); + // The walk is only meaningful if it resolved real URLs — including one inside a route group, which + // is the case that a directory-name scan gets wrong. + expect(routes.map((r) => r.url)).toEqual(expect.arrayContaining(["/", "/api-docs", "/sandbox", "/login"])); + expect(routes.filter((r) => r.authenticated).length).toBeGreaterThan(0); + expect(routes.some((r) => r.url.includes("("))).toBe(false); + + for (const { url, authenticated } of routes) { + if (authenticated || url === "/login") continue; + expect(isPublicRoute(url), `app serves ${url} outside ${AUTHENTICATED_GROUP} but it is not public`) + .toBe(true); + } + }); + + it("keeps the authenticated group gated", () => { + // The converse: the allowlist must not accidentally open the dashboard. `/` covering everything + // by prefix would satisfy the test above while unlocking the whole app. + for (const { url, authenticated } of appRoutes()) { + if (!authenticated) continue; + expect(isPublicRoute(url), `${url} is inside ${AUTHENTICATED_GROUP} but PUBLIC_ROUTES exempts it`) + .toBe(false); } - // `/` is the root page, which has no directory of its own. - expect(PUBLIC_ROUTES).toContain("/"); }); it("does not redirect any route it lists", async () => { diff --git a/frontend/components/auth-provider.tsx b/frontend/components/auth-provider.tsx index d32c49a9..2fd6e133 100644 --- a/frontend/components/auth-provider.tsx +++ b/frontend/components/auth-provider.tsx @@ -13,11 +13,24 @@ const API_BASE = (process.env.NEXT_PUBLIC_API_BASE_URL ?? "").trim().replace(/\/ * This list is the authority for the whole app, and it is easy to forget: a page can be correct in every * other respect — outside `app/(dashboard)/`, fetching without a bearer token — and still be unreachable * because it is absent here. `/api-docs` shipped that way (ADR-068), and an HTTP probe could not see it, - * since the redirect is client-side and the server still returns 200. `auth-provider.test.tsx` derives - * the expected set from `app/` so a new public route cannot be added without landing here. + * since the redirect is client-side and the server still returns 200. `auth-provider.test.tsx` walks + * `app/` for the URLs it really serves, so a new public route cannot be added without landing here — and + * asserts the converse too, that nothing here exempts the authenticated group. */ export const PUBLIC_ROUTES = ["/", "/sandbox", "/api-docs"]; +/** + * Whether `pathname` renders without a session. An entry covers itself and everything nested under it, + * so a public section needs one entry rather than one per page. + * + * Exported because `auth-provider.test.tsx` checks the real routes of `app/` against this predicate. A + * test that reimplemented the matching would agree with itself rather than with the provider. + */ +export function isPublicRoute(pathname: string | null | undefined): boolean { + if (!pathname) return false; + return PUBLIC_ROUTES.some((route) => pathname === route || pathname.startsWith(`${route}/`)); +} + type AuthUser = { email: string; role: string; @@ -136,9 +149,7 @@ export function AuthProvider({ children }: { children: React.ReactNode }) { }, [token]); useEffect(() => { - if (pathname && PUBLIC_ROUTES.some((route) => pathname === route || pathname.startsWith(`${route}/`))) { - return; - } + if (isPublicRoute(pathname)) return; if (pathname?.startsWith("/login")) return; if (token) return;