diff --git a/apps/web/playwright/deepest-route.test.ts b/apps/web/playwright/deepest-route.test.ts index d8200e98..f823ec1f 100644 --- a/apps/web/playwright/deepest-route.test.ts +++ b/apps/web/playwright/deepest-route.test.ts @@ -6,12 +6,14 @@ import { describe, expect, it } from "vitest"; import { ROUTE_TYPES_FILE, appRoutes, + authOriginMismatch, byDepthDesc, deepestAppRoute, - routeTypesInclude, - routeTypesListRoutes, + effectiveAuthOrigin, isRouteTableReady, probeUrlFor, + routeTypesInclude, + routeTypesListRoutes, urlSegments, } from "./deepest-route"; @@ -218,3 +220,130 @@ describe("isRouteTableReady", () => { expect(isRouteTableReady(302, `${AUTH}/login`, AUTH)).toBe(false); }); }); + +// ── The auth origin the probe must expect ──────────────────────────────────────────────────────────── +// The harness passed AUTH_BASE_URL to `next dev` via process.env and then demanded a 307 to that origin. +// But apps/web resolves it BINDING-FIRST (`getAuthBaseUrl`: workerEnv() ?? process.env), and under +// `next dev` the binding comes from getPlatformProxy — which reads `.dev.vars`. So on any machine that has +// run `pnpm dev:secrets`, the app redirected to http://localhost:3001 while the probe waited for +// http://127.0.0.1:3199, and the suite burned its whole 180s budget on a route that was serving correctly +// within a second. Green on CI (no `.dev.vars` there), dead on every developer machine — the inverse of +// the usual failure, and exactly the class this repo's local-parity work exists to remove. +describe("effectiveAuthOrigin", () => { + it("uses the origin the app will ACTUALLY resolve when .dev.vars sets one", () => { + expect( + effectiveAuthOrigin("AUTH_BASE_URL=http://localhost:3001\n", "http://127.0.0.1:3199"), + ).toBe("http://localhost:3001"); + }); + + it("falls back to the harness's isolated origin when there is no .dev.vars", () => { + // CI's case. Keeping the synthetic origin there preserves the property the comment relies on: an + // origin nothing else in the app points at, so nothing can wander off to a real auth server. + expect(effectiveAuthOrigin(null, "http://127.0.0.1:3199")).toBe("http://127.0.0.1:3199"); + }); + + it("ignores a blank or absent binding rather than expecting an empty origin", () => { + // `pnpm dev:secrets` writes an unconfigured key as `NAME=`; treating "" as configured would make the + // probe demand a redirect to the empty string and never match. + expect(effectiveAuthOrigin("AUTH_BASE_URL=\n", "http://127.0.0.1:3199")).toBe( + "http://127.0.0.1:3199", + ); + expect(effectiveAuthOrigin("OTHER=x\n", "http://127.0.0.1:3199")).toBe("http://127.0.0.1:3199"); + }); + + it("strips a trailing slash so the startsWith marker cannot miss", () => { + expect(effectiveAuthOrigin("AUTH_BASE_URL=http://localhost:3001/\n", "http://x")).toBe( + "http://localhost:3001", + ); + }); +}); + +// A 307 to the WRONG origin cannot be a partial route table: the catch-all rejects `org` and calls +// notFound(), so it emits 404 and never a redirect. Only the real route redirects — meaning the route is +// serving and the harness simply expects the wrong origin. Polling that for 180s and then blaming the +// scan is what cost this session an hour; it is a configuration mismatch and should say so at once. +describe("authOriginMismatch", () => { + it("recognises a 307 to a different origin as a config mismatch", () => { + expect(authOriginMismatch(307, "http://localhost:3001/login", "http://127.0.0.1:3199")).toBe( + true, + ); + }); + + it("is not triggered by the catch-all, which cannot redirect at all", () => { + expect(authOriginMismatch(404, null, "http://127.0.0.1:3199")).toBe(false); + expect(authOriginMismatch(200, null, "http://127.0.0.1:3199")).toBe(false); + }); + + it("is not triggered by the correct redirect", () => { + expect(authOriginMismatch(307, "http://127.0.0.1:3199/login", "http://127.0.0.1:3199")).toBe( + false, + ); + }); + + it("does not fire on a 307 with no Location, which is malformed rather than mismatched", () => { + expect(authOriginMismatch(307, null, "http://127.0.0.1:3199")).toBe(false); + }); +}); + +// The parser must accept everything a real `.dev.vars` can hold, because anything it mishandles silently +// reinstates the 180s misdiagnosis this file exists to remove. `scripts/dev-preflight.mjs` strips +// surrounding quotes; a hand-rolled parser that did not would read `"http://localhost:3001"` — quotes +// included — and never match the redirect. +describe("effectiveAuthOrigin parses what .dev.vars actually contains", () => { + it("strips surrounding quotes, as the repo's own .dev.vars parser does", () => { + expect(effectiveAuthOrigin('AUTH_BASE_URL="http://localhost:3001"\n', "http://x")).toBe( + "http://localhost:3001", + ); + expect(effectiveAuthOrigin("AUTH_BASE_URL='http://localhost:3001'\n", "http://x")).toBe( + "http://localhost:3001", + ); + }); + + it("ignores a COMMENTED-OUT key rather than reading it as configuration", () => { + expect(effectiveAuthOrigin("# AUTH_BASE_URL=http://commented\n", "http://x")).toBe("http://x"); + }); + + it("is not fooled by a key that merely ends with the name", () => { + expect(effectiveAuthOrigin("NEXT_AUTH_BASE_URL=http://other\n", "http://x")).toBe("http://x"); + }); + + it("finds the key wherever it sits in the file", () => { + expect(effectiveAuthOrigin("A=1\nAUTH_BASE_URL=http://found\nB=2\n", "http://x")).toBe( + "http://found", + ); + }); + + it("strips a trailing slash from the FALLBACK too, not just the value", () => { + expect(effectiveAuthOrigin(null, "http://127.0.0.1:3199/")).toBe("http://127.0.0.1:3199"); + }); +}); + +// `startsWith` cannot distinguish http://localhost:3001 from http://localhost:30010, so a genuinely +// mismatched origin sharing a prefix would go unflagged — and fall back to the 180s timeout this PR is +// removing. Compare ORIGINS. +describe("origin comparison is exact, not prefix-based", () => { + it("does not treat a longer port as the expected origin", () => { + expect(isRouteTableReady(307, "http://localhost:30010/login", "http://localhost:3001")).toBe( + false, + ); + expect(authOriginMismatch(307, "http://localhost:30010/login", "http://localhost:3001")).toBe( + true, + ); + }); + + it("still accepts the genuine redirect", () => { + expect(isRouteTableReady(307, "http://localhost:3001/login", "http://localhost:3001")).toBe( + true, + ); + expect(authOriginMismatch(307, "http://localhost:3001/login", "http://localhost:3001")).toBe( + false, + ); + }); + + it("treats an unparseable or relative Location as 'keep polling', never as a mismatch", () => { + // Conservative on purpose: a false fast-fail would replace a slow correct answer with a fast wrong + // one, which is a worse trade than the timeout it avoids. + expect(authOriginMismatch(307, "/login", "http://localhost:3001")).toBe(false); + expect(isRouteTableReady(307, "/login", "http://localhost:3001")).toBe(false); + }); +}); diff --git a/apps/web/playwright/deepest-route.ts b/apps/web/playwright/deepest-route.ts index 1f5d9f7f..4fb5aece 100644 --- a/apps/web/playwright/deepest-route.ts +++ b/apps/web/playwright/deepest-route.ts @@ -122,7 +122,7 @@ export function isRouteTableReady( location: string | null, authOrigin: string, ): boolean { - return status === 307 && (location ?? "").startsWith(authOrigin); + return status === 307 && locationHasOrigin(location, authOrigin) === true; } // ── Waiting for the SCAN, not for a request ────────────────────────────────────────────────────────── @@ -159,3 +159,76 @@ export function routeTypesInclude(text: string, route: readonly string[]): boole const want = `/${urlSegments(route).join("/")}`; return routeTypesListRoutes(text).includes(want); } + +/** + * The auth origin the app will ACTUALLY redirect to, given the `.dev.vars` this machine has. + * + * The probe's whole discriminator is "a 307 to the auth origin, which the catch-all cannot produce". That + * only works if the harness expects the origin the app really uses — and the harness does not get to + * choose. `getAuthBaseUrl()` resolves BINDING-FIRST (`workerEnv() ?? process.env`), and under `next dev` + * the binding comes from `getPlatformProxy`, which reads `apps/web/.dev.vars`. So a machine that has run + * `pnpm dev:secrets` redirects to http://localhost:3001 no matter what the harness puts in process.env. + * + * The symptom was maximally misleading: a route serving correctly within a second, reported after 180 + * seconds as "never entered its route table". Green on CI, where there is no `.dev.vars` at all, and dead + * on every developer machine. + * + * Falling back to the harness's own origin keeps CI isolated exactly as before — an origin nothing else in + * the app points at — so this widens nothing where the old assumption already held. + */ +export function effectiveAuthOrigin(devVars: string | null, fallback: string): string { + // Parsed the way `scripts/dev-preflight.mjs` parses the same file, because anything this mishandles + // silently reinstates the 180-second misdiagnosis above. In particular a QUOTED value — which that + // parser strips and a naive one does not — would be read with its quotes and never match the redirect. + let value = ""; + for (const raw of (devVars ?? "").split("\n")) { + const line = raw.trim(); + if (line === "" || line.startsWith("#")) continue; // a commented key is not configuration + const at = line.indexOf("="); + if (at < 0 || line.slice(0, at).trim() !== "AUTH_BASE_URL") continue; // exact key, not a suffix match + value = line.slice(at + 1).trim(); + if (value.length >= 2 && /^(".*"|'.*')$/s.test(value)) value = value.slice(1, -1); + break; + } + // A blank is UNSET — `pnpm dev:secrets` writes an unconfigured key as `NAME=`, and demanding a redirect + // to the empty string would never match. + return stripTrailingSlash(value === "" ? fallback : value); +} + +const stripTrailingSlash = (s: string): string => s.replace(/\/+$/, ""); + +/** + * Does `location` point at `origin`? Compared as ORIGINS, never as a string prefix. + * + * `startsWith` cannot tell http://localhost:3001 from http://localhost:30010, so a genuinely mismatched + * origin that happens to share a prefix would read as correct — the readiness predicate would accept the + * wrong server, and the fast-fail would miss the case it exists for. + * + * A relative or unparseable Location is NOT a match and NOT a mismatch: it means "keep polling". A false + * fast-fail would trade a slow correct answer for a quick wrong one, which is the worse bargain. + */ +export function locationHasOrigin(location: string | null, origin: string): boolean | null { + if (location === null) return null; + try { + return new URL(location).origin === new URL(origin).origin; + } catch { + return null; // relative or malformed — undecidable, so decide nothing + } +} + +/** + * Is this response a CONFIGURATION mismatch rather than a route table still filling in? + * + * The catch-all `(app)/[...legacy]` rejects a first segment of `org` and calls `notFound()`, so a partial + * table yields 404 — never a redirect. A 307 therefore proves the real route is serving; if its Location + * points somewhere other than the origin the harness expects, the route is fine and the EXPECTATION is + * wrong. Continuing to poll that for the full boot budget and then reporting "never entered its route + * table" sends the reader through the wrong subsystem entirely, which is precisely what happened. + */ +export function authOriginMismatch( + status: number, + location: string | null, + authOrigin: string, +): boolean { + return status === 307 && locationHasOrigin(location, authOrigin) === false; +} diff --git a/apps/web/playwright/global-setup.ts b/apps/web/playwright/global-setup.ts index babcfcd7..869937c9 100644 --- a/apps/web/playwright/global-setup.ts +++ b/apps/web/playwright/global-setup.ts @@ -1,4 +1,5 @@ import { spawn, type ChildProcess } from "node:child_process"; +import { readFileSync } from "node:fs"; import { readFile } from "node:fs/promises"; import { resolve } from "node:path"; import { setTimeout as sleep } from "node:timers/promises"; @@ -11,7 +12,9 @@ import { setupSchema } from "../../../packages/db/test/migrate"; import { startEphemeralPostgres, type EphemeralPostgres } from "../../../packages/db/test/pg"; import { ROUTE_TYPES_FILE, + authOriginMismatch, deepestAppRoute, + effectiveAuthOrigin, isRouteTableReady, probeUrlFor, routeTypesInclude, @@ -124,7 +127,27 @@ async function seed(appUrl: string, ownerUrl: string): Promise { } /** Where the gate sends an unauthenticated request — see `AUTH_BASE_URL` below. */ -const AUTH_ORIGIN = "http://127.0.0.1:3199"; +const HARNESS_AUTH_ORIGIN = "http://127.0.0.1:3199"; + +/** + * The origin the probe must expect — read from this machine's `.dev.vars`, not assumed. + * + * apps/web resolves AUTH_BASE_URL binding-first, and under `next dev` the binding comes from + * getPlatformProxy reading `.dev.vars`. So the harness cannot impose its own origin via process.env, and + * assuming it could made the suite unrunnable on any machine that had run `pnpm dev:secrets` — see + * `effectiveAuthOrigin`. Resolved ONCE at module load: the file cannot change mid-run, and re-reading it + * per poll would be a second way for the loop to fail. + */ +const AUTH_ORIGIN = effectiveAuthOrigin( + (() => { + try { + return readFileSync(resolve(process.cwd(), ".dev.vars"), "utf8"); + } catch { + return null; // CI has none, which is the isolated case the fallback preserves + } + })(), + HARNESS_AUTH_ORIGIN, +); /** What the probe saw, so a timeout can report it instead of guessing. */ type ProbeResult = { ready: boolean; status: number; location: string | null }; @@ -273,6 +296,17 @@ async function startDevServer(appConnectionString: string): Promise