diff --git a/.gitignore b/.gitignore index e3f55a90f..a29130a58 100644 --- a/.gitignore +++ b/.gitignore @@ -20,6 +20,7 @@ /playwright/.cache /e2e/setup/storageState.json /e2e/setup/storageStateOwner.json +/e2e/setup/storageStateDelegate.json # next.js /.next/ diff --git a/e2e/account-sharing.spec.ts b/e2e/account-sharing.spec.ts index 4f98006e3..a7d966212 100644 --- a/e2e/account-sharing.spec.ts +++ b/e2e/account-sharing.spec.ts @@ -66,11 +66,11 @@ import type { BrowserContext, Page } from "@playwright/test"; import { expect, test } from "./setup/test"; import { + DELEGATE_STORAGE_STATE_PATH, E2E_DELEGATE_MARKER_KG, E2E_OWNER, E2E_USER, OWNER_STORAGE_STATE_PATH, - STORAGE_STATE_PATH, } from "./setup/test-helpers"; const DELEGATE_MARKER = String(E2E_DELEGATE_MARKER_KG); @@ -131,7 +131,17 @@ test.describe("account sharing", () => { // The `page` fixture throughout is the DELEGATE. The owner gets an explicit // context below, because a grant needs two signed-in sides at once and the // login endpoint is IP-rate-limited. - test.use({ storageState: STORAGE_STATE_PATH }); + // + // The delegate's jar is this journey's OWN, not the shared one every other + // authenticated spec reads, and that is load-bearing rather than tidy. The + // switch is stamped on the session row (see "the nav offers only what + // sharing covers" below, which relies on exactly that), so switching the + // shared jar would switch it for every spec holding the same cookie — and + // the suite runs fully parallel. The damage lands somewhere else entirely: + // a settings surface painting "Not part of shared access", an axe scan + // reading a banner its spec never asked for. Same account, different + // session, no reach outside this file. + test.use({ storageState: DELEGATE_STORAGE_STATE_PATH }); // Desktop only, and for a reason that is about the fixture rather than the // layout: this journey mutates shared rows — one grant between two seeded diff --git a/e2e/setup/global-setup.ts b/e2e/setup/global-setup.ts index 12e1c1edc..80d3765d3 100644 --- a/e2e/setup/global-setup.ts +++ b/e2e/setup/global-setup.ts @@ -83,6 +83,28 @@ export const OWNER_STORAGE_STATE_PATH = resolve( "e2e/setup/storageStateOwner.json", ); +/** + * A SECOND cookie jar for the same delegate account, and the reason it exists + * is the one property that makes account switching work at all: the switch is + * stamped on the session row, not held in the tab. Every session opened from + * `STORAGE_STATE_PATH` is therefore the same session — so a spec that switches + * that jar into somebody else's record switches it for every spec that + * authenticates with it, whichever of them happens to be mid-navigation on the + * other worker. + * + * The symptom is a wandering one: `/settings/ai` painting "Not part of shared + * access", an axe scan reading a banner nobody asked for, a `waitForResponse` + * on a request the refused surface never sends. Nothing in the failing spec is + * wrong and nothing in its file explains it. + * + * So the journey that switches gets its own login, and the shared jar is never + * pointed anywhere. Same account, different session row, no overlap. + */ +export const DELEGATE_STORAGE_STATE_PATH = resolve( + process.cwd(), + "e2e/setup/storageStateDelegate.json", +); + async function hashPassword(password: string): Promise { return hash(password, { memoryCost: 19456, @@ -259,11 +281,12 @@ export default async function globalSetup(config: FullConfig): Promise { WHERE key LIKE 'sharing:%'`, ); - // The login bucket, for the same reason. This setup signs in TWICE now - // (delegate and owner), and the ceiling is five attempts per IP per - // quarter-hour — so three local runs in a row used to end with a 429 from - // the fixture rather than a failure from the product. Only the auth - // surfaces' own buckets are cleared; nothing else in the table is touched. + // The login bucket, for the same reason. This setup signs in THREE times + // now (the shared jar, the owner, and the sharing journey's own delegate + // jar), and the ceiling is five attempts per IP per quarter-hour — so two + // local runs in a row would otherwise end with a 429 from the fixture + // rather than a failure from the product. Only the auth surfaces' own + // buckets are cleared; nothing else in the table is touched. await pool.query(`DELETE FROM rate_limits WHERE key LIKE 'auth:%'`); // One marker weight per record. Re-seeded by delete-then-insert so a @@ -301,96 +324,82 @@ export default async function globalSetup(config: FullConfig): Promise { await pool.end(); } - // Login once and persist the resulting cookie jar so every spec can - // reuse it via `playwright.config.ts → use.storageState`. This is the - // documented Playwright pattern (`docs/auth.md`) and the only way to - // avoid the per-spec login + rate-limit dance. + // Log the fixture accounts in here and persist their cookie jars so specs + // can reuse them via `test.use({ storageState })`. This is the documented + // Playwright pattern (`docs/auth.md`) and the only way to avoid the per-spec + // login + rate-limit dance. const baseURL = config.projects[0]?.use.baseURL ?? process.env.E2E_BASE_URL ?? "http://localhost:3000"; - const ctx = await playwrightRequest.newContext({ baseURL }); - const res = await ctx.post("/api/auth/login", { - data: { - email: E2E_USER.username, - password: E2E_USER.password, - }, - }); - if (res.status() !== 200) { - const body = await res.text(); - throw new Error( - `[e2e/global-setup] login failed: HTTP ${res.status()} — ${body.slice(0, 200)}`, - ); - } - const state = await ctx.storageState(); - // Strip any cookies we don't recognise so storageState only carries - // what the auth shell needs. v1.4.22 C4 added `hl_onboarding` — the - // login route sets it to "pending" for new users and DELETES it for - // already-onboarded ones, so for our pre-seeded completed e2e user - // the cookie is absent and the proxy passes the dashboard through. - state.cookies = state.cookies.filter((c) => - ["healthlog_session", "healthlog-locale", "hl_onboarding"].includes(c.name), - ); - // v1.27.11 — pin the shared auth state to English via an explicit locale - // cookie. The server now honours `User.locale` when the cookie is absent - // (the ITP fix), and the locale-switch spec mirrors its German pick into - // the SHARED e2e user's profile — without this cookie every spec running - // after it would render German and the English-string assertions fail. - // The cookie sits at the top of the resolution ladder, so each spec - // context stays deterministically English; locale-switch overrides it - // inside its own context only. - const base = new URL(baseURL); - state.cookies = state.cookies.filter((c) => c.name !== "healthlog-locale"); - state.cookies.push({ - name: "healthlog-locale", - value: "en", - domain: base.hostname, - path: "/", - expires: Math.floor(Date.now() / 1000) + 60 * 60 * 24 * 365, - httpOnly: false, - secure: false, - sameSite: "Lax", - }); - await writeFile(STORAGE_STATE_PATH, JSON.stringify(state, null, 2)); - await ctx.dispose(); - console.log( - `[e2e/global-setup] auth state captured to ${STORAGE_STATE_PATH}`, - ); + // The shared jar every authenticated spec reads. + await captureAuthState(baseURL, E2E_USER, STORAGE_STATE_PATH); - // v1.36.0 — the owner's cookie jar, captured the same way and for the same - // reason: the login endpoint is IP-rate-limited and the sharing spec needs - // both sides of a grant signed in at once. - const ownerCtx = await playwrightRequest.newContext({ baseURL }); - const ownerRes = await ownerCtx.post("/api/auth/login", { - data: { email: E2E_OWNER.username, password: E2E_OWNER.password }, - }); - if (ownerRes.status() !== 200) { - const body = await ownerRes.text(); - throw new Error( - `[e2e/global-setup] owner login failed: HTTP ${ownerRes.status()} — ${body.slice(0, 200)}`, + // v1.36.0 — the owner's jar. The sharing journey needs both sides of a grant + // signed in at once, and the login endpoint is IP-rate-limited, so the owner + // is logged in here rather than inside the spec. + await captureAuthState(baseURL, E2E_OWNER, OWNER_STORAGE_STATE_PATH); + + // The delegate's own jar for that journey. Same account as the shared one, + // deliberately a different session row — see DELEGATE_STORAGE_STATE_PATH for + // what happens when the journey switches the shared row instead. + await captureAuthState(baseURL, E2E_USER, DELEGATE_STORAGE_STATE_PATH); +} + +/** + * Log one account in and write its cookie jar to `path`. + * + * Each call mints a fresh session row, so two jars for the same account are + * two independent browser sessions — which is the whole point of the delegate + * jar above. + * + * Only the cookies the auth shell needs survive the filter. v1.4.22 C4 added + * `hl_onboarding`: the login route sets it to "pending" for a new account and + * DELETES it for an already-onboarded one, so for these pre-seeded accounts it + * is absent and the proxy passes the dashboard through. + * + * v1.27.11 — the locale cookie is pinned to English rather than inherited. The + * server honours `User.locale` when the cookie is absent (the ITP fix), and + * the locale-switch spec mirrors its German pick into the SHARED account's + * profile — without this cookie every spec running after it would render + * German and the English-string assertions would fail. The cookie sits at the + * top of the resolution ladder, so each spec context stays deterministically + * English; locale-switch overrides it inside its own context only. + */ +async function captureAuthState( + baseURL: string, + account: { username: string; password: string }, + path: string, +): Promise { + const ctx = await playwrightRequest.newContext({ baseURL }); + try { + const res = await ctx.post("/api/auth/login", { + data: { email: account.username, password: account.password }, + }); + if (res.status() !== 200) { + const body = await res.text(); + throw new Error( + `[e2e/global-setup] login failed for ${account.username}: HTTP ${res.status()} — ${body.slice(0, 200)}`, + ); + } + const state = await ctx.storageState(); + state.cookies = state.cookies.filter((c) => + ["healthlog_session", "hl_onboarding"].includes(c.name), ); + state.cookies.push({ + name: "healthlog-locale", + value: "en", + domain: new URL(baseURL).hostname, + path: "/", + expires: Math.floor(Date.now() / 1000) + 60 * 60 * 24 * 365, + httpOnly: false, + secure: false, + sameSite: "Lax", + }); + await writeFile(path, JSON.stringify(state, null, 2)); + console.log(`[e2e/global-setup] auth state captured to ${path}`); + } finally { + await ctx.dispose(); } - const ownerState = await ownerCtx.storageState(); - ownerState.cookies = ownerState.cookies.filter( - (c) => c.name === "healthlog_session" || c.name === "hl_onboarding", - ); - ownerState.cookies.push({ - name: "healthlog-locale", - value: "en", - domain: base.hostname, - path: "/", - expires: Math.floor(Date.now() / 1000) + 60 * 60 * 24 * 365, - httpOnly: false, - secure: false, - sameSite: "Lax", - }); - await writeFile( - OWNER_STORAGE_STATE_PATH, - JSON.stringify(ownerState, null, 2), - ); - await ownerCtx.dispose(); - console.log( - `[e2e/global-setup] owner auth state captured to ${OWNER_STORAGE_STATE_PATH}`, - ); } diff --git a/e2e/setup/test-helpers.ts b/e2e/setup/test-helpers.ts index 35425aa10..9ae2f7a18 100644 --- a/e2e/setup/test-helpers.ts +++ b/e2e/setup/test-helpers.ts @@ -13,4 +13,5 @@ export { E2E_OWNER_MARKER_KG, STORAGE_STATE_PATH, OWNER_STORAGE_STATE_PATH, + DELEGATE_STORAGE_STATE_PATH, } from "./global-setup"; diff --git a/src/__tests__/e2e-shared-session-guard.test.ts b/src/__tests__/e2e-shared-session-guard.test.ts new file mode 100644 index 000000000..037c8980e --- /dev/null +++ b/src/__tests__/e2e-shared-session-guard.test.ts @@ -0,0 +1,95 @@ +/** + * Structural guard on the E2E suite's session boundary. + * + * Account switching is stamped on the session row, not held in the tab. Every + * Playwright context opened from `STORAGE_STATE_PATH` carries the SAME session + * cookie, so a spec that switches that jar into another account's record + * switches it for every spec authenticating with it — and the suite runs + * `fullyParallel`. The failure lands in an unrelated file: a settings surface + * painting "Not part of shared access", an axe scan reading a banner its spec + * never asked for, a `waitForResponse` on a request the refused surface never + * sends. Nothing in the spec that fails is wrong, and nothing in its file + * explains it. + * + * So: a spec that switches accounts uses its own jar. This file holds that + * shape by matching the two things a switching spec cannot avoid naming — the + * menu entry it clicks and the endpoint that entry calls. + * + * It is a tripwire, not a proof. A future spec could switch through a control + * neither marker names, and this guard would not see it; the counter-check + * below is what stops the matchers from quietly matching nothing. + */ +import { describe, it, expect } from "vitest"; +import { readFileSync, globSync } from "node:fs"; +import { join, sep } from "node:path"; + +const E2E = join(process.cwd(), "e2e"); + +/** + * Markers that mean "this spec drives an account switch": the switcher entry + * a person clicks, and the endpoint that click posts to. + */ +const SWITCH_MARKERS = ["account-switcher-entry", "/api/account/switch"]; + +/** The jar shared by every ordinary authenticated spec. */ +const SHARED_JAR = "STORAGE_STATE_PATH"; + +function specFiles(): string[] { + return globSync("*.spec.ts", { cwd: E2E }) + .map((p) => p.split(sep).join("/")) + .sort(); +} + +function read(rel: string): string { + return readFileSync(join(E2E, rel), "utf8"); +} + +/** + * The file with comments removed. This guard's own subject matter is worth + * explaining in prose inside the specs, and a naive substring match would be + * satisfied by that prose alone. + */ +function stripComments(src: string): string { + return src.replace(/\/\*[\s\S]*?\*\//g, "").replace(/^\s*\/\/.*$/gm, ""); +} + +function switchingSpecs(): string[] { + return specFiles().filter((rel) => { + const src = stripComments(read(rel)); + return SWITCH_MARKERS.some((marker) => src.includes(marker)); + }); +} + +/** + * `STORAGE_STATE_PATH` as a whole word, so `OWNER_STORAGE_STATE_PATH` and + * `DELEGATE_STORAGE_STATE_PATH` — the isolated jars, which are the fix rather + * than the problem — do not read as a hit. + */ +function usesSharedJar(src: string): boolean { + return new RegExp(`(^|[^A-Z_])${SHARED_JAR}\\b`).test(src); +} + +describe("e2e specs that switch accounts do not share a session", () => { + it("finds the specs that switch at all", () => { + // An empty match set would make every assertion below vacuous. The suite + // has a switching journey; if it ever has none, this guard has no subject + // and should be deleted rather than left passing. + expect(switchingSpecs().length).toBeGreaterThan(0); + }); + + it("none of them authenticates with the shared jar", () => { + const offenders = switchingSpecs().filter((rel) => + usesSharedJar(stripComments(read(rel))), + ); + expect(offenders).toEqual([]); + }); + + it("the shared jar is still what the ordinary specs read", () => { + // The other way this guard could go quietly green: the shared jar renamed + // or retired, leaving the matcher above looking for a string nothing uses. + const sharing = specFiles().filter((rel) => + usesSharedJar(stripComments(read(rel))), + ); + expect(sharing.length).toBeGreaterThan(10); + }); +});