From 18dc4f3136f8909572d429d8e172a8c2376f22f6 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Wed, 2 Sep 2026 18:24:56 +0200 Subject: [PATCH] Refuse an emailless identity where the session is minted, and rows the upstream should not have sent --- AGENTS.md | 7 ++++--- e2e-tests/full-flow.spec.ts | 5 +---- e2e-tests/visual.spec.ts | 5 ++--- examples/plugins/scheduling/README.md | 2 +- examples/plugins/scheduling/shifts.test.ts | 15 +++++++++++++++ examples/plugins/scheduling/shifts.ts | 8 ++++---- examples/plugins/scheduling/views/mine.ejs | 2 +- examples/shifts-upstream/server.ts | 4 ++-- src/auth/login.test.ts | 16 ++++++++++++++-- src/auth/login.ts | 17 +++++++++-------- src/auth/routes.test.ts | 7 ++++++- src/http/builtin-routes.ts | 2 -- src/plugin-host/plugin.ts | 1 - src/ui/nav.ts | 1 - 14 files changed, 59 insertions(+), 33 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 960e72f..ca83fd3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -186,9 +186,10 @@ Revisit only if the stated reason stops holding. node names exactly one of `public`, `session`, `permission` — discovery refuses none, two, and a flag spelled anything but `true`, so a forgotten gate fails the boot rather than publishing a page. `src/auth/gate.ts` is the one home of the rule the plugin router, the host's own route table and - the menu all read. Exactly-one-gate is a discovery-time rule on manifests, not a runtime invariant: - `allows({}, user)` stays open **by design**, because the central override's `groups` builds header - nodes that carry no gate. Making `allows` fail closed would hide every operator-grouped section. `session` exists because a plugin whose data is + the menu all read. Exactly-one-gate is a discovery-time rule on manifests, not a runtime + invariant: `allows({}, user)` stays open **by design**, because the central override's `groups` + builds header nodes that carry no gate. Making `allows` fail closed would hide every + operator-grouped section. `session` exists because a plugin whose data is the visitor's own — their upstream account, their own tokens — has no distinction a permission could name; the alternative, granting every newly registered user a permission, couples the identity lifecycle to a Keto write that nothing retries when it fails. A page scoped to "mine" joins on diff --git a/e2e-tests/full-flow.spec.ts b/e2e-tests/full-flow.spec.ts index a66831e..b043e7d 100644 --- a/e2e-tests/full-flow.spec.ts +++ b/e2e-tests/full-flow.spec.ts @@ -194,10 +194,7 @@ test.describe.serial("authenticated admin journey", () => { await expect(page.locator("h1")).toHaveText("Shifts"); await expect(page.locator("table")).toContainText("Morning — Front desk"); // seeded by the mock upstream - // The session-gated page scopes the upstream read by the visitor's identity id. The demo - // upstream's rows belong to three made-up people, so the admin's own page is empty — which is - // the assertion that matters: nobody else's shifts come back. (A matching row rendering is - // covered where it is exact, in the plugin's own unit test.) + // The admin owns none of the demo's rows, so an empty page is the no-leak assertion. await page.goto("/scheduling/mine"); await expect(page.locator("h1")).toHaveText("My shifts"); await expect(page.getByText("No shifts are assigned to admin@plainpages.local")).toBeVisible(); diff --git a/e2e-tests/visual.spec.ts b/e2e-tests/visual.spec.ts index 57f435a..01c2720 100644 --- a/e2e-tests/visual.spec.ts +++ b/e2e-tests/visual.spec.ts @@ -179,9 +179,8 @@ test("the reference plugin: public Overview is open to all, My shifts takes any await expect(page.locator('.sidebar a[href="/scheduling/shifts"]')).toHaveCount(0); // gated leaf filtered out await expect(page.locator('.sidebar a[href="/scheduling/mine"]')).toHaveCount(1); // session gate: a session is enough - // And the page itself renders for that same member, holding no permission at all. This stack runs - // no shifts upstream, so it also pins the degraded page: the reason, never a 500 and never a claim - // about what is assigned. The working page is asserted against a real upstream in full-flow.spec. + // No shifts upstream on this stack, so this also pins the degraded page: the reason, never a 500 + // and never a claim about what is assigned. await page.goto("/scheduling/mine"); await expect(page.getByRole("heading", { name: "My shifts" })).toBeVisible(); await expect(page.getByText("Couldn't reach the scheduling service")).toBeVisible(); diff --git a/examples/plugins/scheduling/README.md b/examples/plugins/scheduling/README.md index 5986d8d..b43d96d 100644 --- a/examples/plugins/scheduling/README.md +++ b/examples/plugins/scheduling/README.md @@ -43,7 +43,7 @@ Your backend must expose two routes; the plugin treats any non-2xx as a recovera | Route | Request | Success | Response body | | --- | --- | --- | --- | | `GET /shifts` | `Accept: application/json`, optional `?assigneeId=` | `200` | JSON array of `{ id, title, assignee, assigneeId, start, end }` (all strings; missing fields coerce to `""`). With `assigneeId`, only that person's rows | -| `POST /shifts` | JSON body `{ title, assignee, assigneeId?, start, end }` | `2xx` | ignored (the plugin POST-redirect-GETs back to the list) | +| `POST /shifts` | JSON body `{ title, assignee, start, end }` | `2xx` | ignored (the plugin POST-redirect-GETs back to the list) | Domain rules (overlap, capacity, time ordering) live in your backend — reject with a 4xx and the form re-renders. The plugin only validates that `title` and `assignee` are non-empty. diff --git a/examples/plugins/scheduling/shifts.test.ts b/examples/plugins/scheduling/shifts.test.ts index 88fe3f0..62f547f 100644 --- a/examples/plugins/scheduling/shifts.test.ts +++ b/examples/plugins/scheduling/shifts.test.ts @@ -191,3 +191,18 @@ test("my shifts scopes the upstream read by the visitor's id, and names them in // `session: true` guarantee the contract cannot state in the handler's type. await assert.rejects(async () => { await myShifts(fakeUpstream())(fakeCtx()); }, GuardError); }); + +test("my shifts degrades to the reason alone when the upstream is down, claiming nothing about what is assigned", async () => { + const user: User = { email: "Blair.Mora@example.test", id: "01a06091-baa3-71f4-a068-4879972979ff", permissions: [] }; + const down = fakeUpstream({ list: async () => { throw new UpstreamError("down", 503); } }); + const r = asView(await myShifts(down)(fakeCtx({ url: "http://localhost/scheduling/mine", user }))); + assert.match(String(r.data["error"]), /scheduling service/i); + assert.deepEqual((r.data["table"] as { rows: unknown[] }).rows, []); // mine.ejs drops the count + table while `error` is set +}); + +test("my shifts drops a row the upstream returned that is not the visitor's", async () => { + const user: User = { email: "Blair.Mora@example.test", id: "01a06091-baa3-71f4-a068-4879972979ff", permissions: [] }; + const theirs: Shift = { assignee: "Avery Kline", assigneeId: "019bdc1a-3f27-7c41-9a6e-2b1d4f8e05a3", end: "12:00", id: "9", start: "08:00", title: "Not mine" }; + const r = asView(await myShifts(fakeUpstream({ list: async () => [theirs] }))(fakeCtx({ url: "http://localhost/scheduling/mine", user }))); + assert.deepEqual((r.data["table"] as { rows: unknown[] }).rows, []); // a backend ignoring the scope must not leak through this page +}); diff --git a/examples/plugins/scheduling/shifts.ts b/examples/plugins/scheduling/shifts.ts index e278d75..e8984dc 100644 --- a/examples/plugins/scheduling/shifts.ts +++ b/examples/plugins/scheduling/shifts.ts @@ -198,8 +198,9 @@ export function myShifts(upstream: ShiftsUpstream): RouteHandler { let error: string | undefined; try { // Join on the id, never the email: an address is user-changeable and can be reassigned to - // someone else, which would hand them the previous holder's rows. - shifts = await upstream.list({ assigneeId: user.id }); + // someone else, which would hand them the previous holder's rows. The re-filter is + // defence-in-depth: a backend that ignores an unknown query param would answer with everyone. + shifts = (await upstream.list({ assigneeId: user.id })).filter((s) => s.assigneeId === user.id); } catch (err) { ctx.log.warn("scheduling upstream unreachable", { error: String(err) }); error = ctx.t("scheduling.upstream.list"); @@ -218,8 +219,7 @@ export function buildMineModel(opts: { chrome: PageChrome; email: string; error? table: { caption: t("scheduling.mine.title"), columns: [{ label: t("scheduling.table.shift") }, { label: t("scheduling.table.start") }, { label: t("scheduling.table.end") }], - // Only when the upstream answered: a failed read knows nothing about what is assigned. - ...(opts.error === undefined ? { emptyText: t("scheduling.mine.empty", { email: opts.email }) } : {}), + emptyText: t("scheduling.mine.empty", { email: opts.email }), rows: opts.shifts.map((s) => ({ cells: [{ rowHeader: { text: s.title } }, s.start, s.end], name: s.title })), }, title: t("scheduling.mine.title"), diff --git a/examples/plugins/scheduling/views/mine.ejs b/examples/plugins/scheduling/views/mine.ejs index f4bb88c..d92dc20 100644 --- a/examples/plugins/scheduling/views/mine.ejs +++ b/examples/plugins/scheduling/views/mine.ejs @@ -7,7 +7,7 @@ const alertHtml = locals.error ? include("partials/alert", { text: locals.error, tone: "neg" }) : ""; -%> <%- include("partials/shell", { - body: '
' + alertHtml + '

' + count + '

' + tableHtml + '
', + body: '
' + alertHtml + (locals.error ? '' : '

' + count + '

' + tableHtml) + '
', brand: chrome.brand, breadcrumbs, csrfToken: chrome.csrfToken, diff --git a/examples/shifts-upstream/server.ts b/examples/shifts-upstream/server.ts index 8e85c21..959f3e6 100644 --- a/examples/shifts-upstream/server.ts +++ b/examples/shifts-upstream/server.ts @@ -4,7 +4,7 @@ // at your real service in production. // // GET /shifts → 200 [ { id, title, assigneeId, assignee, start, end }, … ] (?assigneeId= → only theirs) -// POST /shifts → 201 { id, … } (body: { title, assignee, assigneeId?, start, end }) +// POST /shifts → 201 { id, … } (body: { title, assignee, start, end }) import { randomUUID } from "node:crypto"; import { createServer } from "node:http"; @@ -42,7 +42,7 @@ createServer(async (req, res) => { } if (url.pathname === "/shifts" && req.method === "POST") { const b = await readBody(req); - const shift = { id: randomUUID(), assignee: String(b.assignee ?? ""), assigneeId: String(b.assigneeId ?? ""), end: String(b.end ?? ""), start: String(b.start ?? ""), title: String(b.title ?? "") }; + const shift = { id: randomUUID(), assignee: String(b.assignee ?? ""), assigneeId: "", end: String(b.end ?? ""), start: String(b.start ?? ""), title: String(b.title ?? "") }; shifts.push(shift); return json(res, 201, shift); } diff --git a/src/auth/login.test.ts b/src/auth/login.test.ts index abbe3d7..5327bad 100644 --- a/src/auth/login.test.ts +++ b/src/auth/login.test.ts @@ -92,12 +92,24 @@ test("completeLogin returns null and touches nothing when there is no active ses assert.equal(touched, false); }); -test("completeLogin maps a missing email trait to null and throws if the tokenizer yields no JWT", async () => { - const identity: Identity = { id: ID, traits: {} }; +test("completeLogin throws if the tokenizer yields no JWT", async () => { + const identity: Identity = { id: ID, traits: { email: "admin@plainpages.local" } }; const kratosPublic = publicStub({ whoami: async () => ({ active: true, identity }) as Session }); // never returns a tokenized JWT await assert.rejects(completeLogin({ keto: ketoStub(), kratosAdmin: adminStub(), kratosPublic }, "c"), /tokenizer returned no JWT/); }); +// An identity with no email is no session, decided here so /auth/complete and remintSession cannot +// disagree: `claimsToUser` reads a token carrying none as anonymous, so minting one would hand the +// browser a cookie every later request refuses. +test("completeLogin refuses an identity carrying no email, before it mints anything", async () => { + const identity: Identity = { id: ID, traits: {} }; + let touched = false; + const kratosAdmin = adminStub({ updateMetadataPublic: async () => { touched = true; return { id: ID }; } }); + const kratosPublic = publicStub({ whoami: async () => ({ active: true, identity, tokenized: "h.p.s" }) as Session }); + assert.equal(await completeLogin({ keto: ketoStub(), kratosAdmin, kratosPublic }, "c"), null); + assert.equal(touched, false); // no Keto read, no metadata write, no JWT +}); + test("remintSession: a live Kratos session → fresh cookie + refreshed user; a dead session → a clearing cookie + null", async () => { const identity: Identity = { id: ID, traits: { email: "admin@plainpages.local" } }; const kratosPublic = publicStub({ whoami: async (o) => (o?.tokenizeAs ? { active: true, identity, tokenized: "h.p.s" } : { active: true, identity }) as Session }); diff --git a/src/auth/login.ts b/src/auth/login.ts index 9f15624..621c24d 100644 --- a/src/auth/login.ts +++ b/src/auth/login.ts @@ -31,7 +31,7 @@ export interface LoginDeps { } export interface CompletedLogin { - email: string | null; + email: string; userId: string; jwt: string; permissions: string[]; @@ -61,7 +61,13 @@ export async function completeLogin(deps: LoginDeps, cookie: string | undefined) if (!session?.identity) return null; const userId = session.identity.id; const emailTrait = session.identity.traits?.["email"]; - const email = typeof emailTrait === "string" ? emailTrait : null; + const email = typeof emailTrait === "string" ? emailTrait : ""; + // No email is no session: `claimsToUser` reads a token carrying none as anonymous, so minting one + // would hand the browser a cookie every later request refuses. + if (!email) { + currentLog()?.warn("session dropped: identity has no email", { sub: userId }); + return null; + } const permissions = await readPermissions(deps.keto, userId); await deps.kratosAdmin.updateMetadataPublic(userId, { permissions }); @@ -86,12 +92,7 @@ export interface Reminted { // anonymous instead of re-hitting Ory on every one. export async function remintSession(deps: LoginDeps, cookie: string | undefined, options: { secure?: boolean } = {}): Promise { const completed = await completeLogin(deps, cookie); - // No email is no session, exactly as `claimsToUser` reads a token carrying none: a User with an - // empty email reads as anonymous in the shell, and is a blank key to whatever scopes on it. - if (!completed?.email) { - if (completed) currentLog()?.warn("session dropped: identity has no email", { sub: completed.userId }); - return { setCookie: clearSessionCookie(options), user: null }; - } + if (!completed) return { setCookie: clearSessionCookie(options), user: null }; return { setCookie: sessionCookie(completed.jwt, options), user: { email: completed.email, id: completed.userId, permissions: completed.permissions } }; } diff --git a/src/auth/routes.test.ts b/src/auth/routes.test.ts index fa9ed2d..89bf278 100644 --- a/src/auth/routes.test.ts +++ b/src/auth/routes.test.ts @@ -4,6 +4,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { AUTH_FLOWS } from "./flow-view.ts"; +import { gatesSet } from "./gate.ts"; import type { HydraAdmin } from "./hydra-admin.ts"; import type { KetoClient } from "./keto-client.ts"; import type { KratosAdmin } from "./kratos-admin.ts"; @@ -39,8 +40,12 @@ test("hydra alone ⇒ only RP-initiated logout of the OAuth2 group (login/consen }); test("everything wired ⇒ the full group: OAuth2 challenges, consent GET+POST, /auth/complete", () => { - const got = keys(buildAuthRoutes(deps({ hydra, keto, kratos, kratosAdmin }))); + const routes = buildAuthRoutes(deps({ hydra, keto, kratos, kratosAdmin })); + const got = keys(routes); for (const key of ["GET /auth/complete", "GET /login", "GET /oauth2/consent", "GET /oauth2/login", "GET /oauth2/logout", "POST /logout", "POST /oauth2/consent"]) { assert.ok(got.includes(key), key); } + // Discovery enforces exactly one gate per plugin declaration; nothing checks the host's own table + // at boot, so a route added here without a gate would be silently public. + for (const route of routes) assert.deepEqual(gatesSet(route), ["public"], `${route.method} ${route.path}`); }); diff --git a/src/http/builtin-routes.ts b/src/http/builtin-routes.ts index 055ec8e..1c24997 100644 --- a/src/http/builtin-routes.ts +++ b/src/http/builtin-routes.ts @@ -20,8 +20,6 @@ export interface RequestCsrf { // own context — otherwise the plugin's keys render as bare keys on the pages it owns. export type PluginContextFactory = (pluginId: string) => RequestContext; -// `Gate` carries `permission`/`public`/`session`, checked before the handler runs — the same rule -// the plugin router and the menu read. export interface BuiltinRoute extends Gate { // Returns a RouteResult, or null when the handler wrote to ctx.res itself // (the landing slots dispatch a plugin's own result against that plugin's views). diff --git a/src/plugin-host/plugin.ts b/src/plugin-host/plugin.ts index 23547b8..4c4716f 100644 --- a/src/plugin-host/plugin.ts +++ b/src/plugin-host/plugin.ts @@ -25,7 +25,6 @@ export type RouteResult = export type RouteHandler = (ctx: RequestContext) => Promise | RouteResult | void; -// `Gate` carries `permission`/`public`/`session`, checked before the handler runs. export interface Route extends Gate { handler: RouteHandler; method: HttpMethod; diff --git a/src/ui/nav.ts b/src/ui/nav.ts index 4526fc4..6ee6885 100644 --- a/src/ui/nav.ts +++ b/src/ui/nav.ts @@ -8,7 +8,6 @@ import type { User } from "../http/context.ts"; import { ENGLISH } from "../i18n/english.ts"; import type { Translate } from "../i18n/translate.ts"; -// `Gate` carries `permission`/`public`/`session` — consumed by the filter, never rendered. export interface NavNode extends Gate { id?: string; // stable key for override targeting; stripped from the rendered tree children?: NavNode[];