From 521c09fa2d86ee7c5453cff2cf8cdf21d1c61565 Mon Sep 17 00:00:00 2001 From: lilleman Date: Fri, 19 Jun 2026 11:47:06 +0200 Subject: [PATCH] =?UTF-8?q?=C2=A76=20review=20checkpoint=20(todo=20=C2=A76?= =?UTF-8?q?);=20ran=20the=20architecture=20+=20product=20reviewers=20on=20?= =?UTF-8?q?the=20whole=20project=20(weighted=20to=20the=20Hydra=20OAuth2?= =?UTF-8?q?=20surfaces)=20and=20addressed=20their=20findings=20=E2=80=94?= =?UTF-8?q?=20no=20Critical=20from=20either.=20Fixed=20tests-first:=20(HIG?= =?UTF-8?q?H,=20arch)=20/oauth2/logout=20was=20published=20to=20Hydra=20(h?= =?UTF-8?q?ydra.yml=20urls.logout)=20and=20asserted=20in=20hydra.test.ts?= =?UTF-8?q?=20but=20had=20no=20handler=20=E2=80=94=20a=20dead/published=20?= =?UTF-8?q?contract;=20added=20hydra-admin.acceptLogoutRequest=20(PUT=20lo?= =?UTF-8?q?gout/accept=20via=20the=20shared=20reqUrl(kind=E2=80=A6))=20+?= =?UTF-8?q?=20a=20GET=20/oauth2/logout=20branch=20that=20accepts=20the=20R?= =?UTF-8?q?P-initiated=20logout=5Fchallenge=20=E2=86=92=20303=20to=20Hydra?= =?UTF-8?q?'s=20post-logout=20redirect=20(missing=E2=86=92400,=20stale=204?= =?UTF-8?q?xx=E2=86=92recoverable=20400,=205xx=E2=86=92500,=20byte-identic?= =?UTF-8?q?al=20degrade=20to=20the=20login/consent=20siblings;=20GET-accep?= =?UTF-8?q?t=20is=20safe=20since=20the=20challenge=20is=20Hydra-minted+sin?= =?UTF-8?q?gle-use;=20the=20first-party=20POST=20/logout=20still=20owns=20?= =?UTF-8?q?ending=20the=20Kratos=20session=20+=20JWT=20cookie).=20(HIGH,?= =?UTF-8?q?=20arch)=20added=20`oauth2`=20to=20RESERVED=5FPLUGIN=5FIDS=20so?= =?UTF-8?q?=20a=20plugins/oauth2/=20folder=20can't=20silently=20shadow=20t?= =?UTF-8?q?he=20provider=20routes=20(the=20route=20surface=20the=20=C2=A74?= =?UTF-8?q?=20reserved-id=20fix=20missed;=20discovery=20now=20refuses=20it?= =?UTF-8?q?=20loud).=20(Product=20Blocker)=20the=20third-party=20consent?= =?UTF-8?q?=20screen=20now=20names=20the=20signed-in=20account=20=E2=80=94?= =?UTF-8?q?=20"Signed=20in=20as=20"=20(ConsentView.account=20from?= =?UTF-8?q?=20whoami)=20=E2=80=94=20plus=20a=20CSRF-guarded=20"Not=20you?= =?UTF-8?q?=3F=20Sign=20out"=20form,=20so=20consent=20is=20informed=20on?= =?UTF-8?q?=20shared=20devices.=20(MEDIUM,=20arch)=20consent=20accept()=20?= =?UTF-8?q?now=20projects=20id=5Ftoken=20claims=20only=20when=20the=20live?= =?UTF-8?q?=20Kratos=20session=20subject=20=3D=3D=3D=20the=20challenge=20s?= =?UTF-8?q?ubject=20Hydra=20bound=20at=20login,=20never=20leaking=20a=20mi?= =?UTF-8?q?smatched=20session's=20email/name=20into=20the=20issued=20token?= =?UTF-8?q?=20(guards=20the=20auto-accept=20path=20too).=20(Product=20nits?= =?UTF-8?q?)=20register-form=20confidential-vs-public=20guidance=20+=20a?= =?UTF-8?q?=20client-detail=20"delete=20and=20re-register=20/=20secret=20s?= =?UTF-8?q?hown=20once"=20note=20(no-edit=20friction=20+=20lost-secret).?= =?UTF-8?q?=20New=20tests=20across=20discovery=20(reserved=20oauth2),=20hy?= =?UTF-8?q?dra-admin=20(acceptLogoutRequest=20contract),=20oauth-consent?= =?UTF-8?q?=20(subject-match=20+=20account-in-view),=20app.test=20(logout?= =?UTF-8?q?=20303/400/500=20matrix,=20consent=20identity+sign-out,=20clien?= =?UTF-8?q?t=20form/detail=20copy);=20e2e/oauth-login.spec=20asserts=20the?= =?UTF-8?q?=20consent=20screen=20names=20the=20account.=20Stability-review?= =?UTF-8?q?er=20run=20as=20a=20local=20PR:=20APPROVE,=20no=20Critical/High?= =?UTF-8?q?=20=E2=80=94=20addressed=20its=20doc/comment=20follow-ups=20(RE?= =?UTF-8?q?ADME=20=C2=A76=20documents=20the=20logout=20handler=20+=20conse?= =?UTF-8?q?nt=20identity=20line;=20a=20comment=20notes=20the=20GET-accept?= =?UTF-8?q?=20is=20Hydra-validated).=20Deferred=20(reviewer-scoped):=20the?= =?UTF-8?q?=20host=20internal=20route-table=20(arch=20M1,=20now=20a=20pure?= =?UTF-8?q?=20dedup=20once=20H1/H2=20are=20point-fixed)=20=E2=86=92=20?= =?UTF-8?q?=C2=A79;=20the=20RP-initiated-logout=20browser/live=20E2E=20?= =?UTF-8?q?=E2=86=92=20=C2=A78;=20redirect-URI=20scheme=20allowlist=20+=20?= =?UTF-8?q?safeUrl()=20=E2=86=92=20=C2=A77;=20full=20client=20edit=20/=20e?= =?UTF-8?q?mpty-list=20state=20/=20success-flash=20=E2=86=92=20=C2=A78/pol?= =?UTF-8?q?ish.=20typecheck=20+=20279=20units=20green;=20full-stack=20OAut?= =?UTF-8?q?h2=20login+consent=20E2E=20verified=20live=20against=20real=20H?= =?UTF-8?q?ydra=20v26.2.0=20then=20torn=20down.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 7 +++-- e2e/oauth-login.spec.ts | 1 + public/css/auth.css | 5 ++-- src/app.test.ts | 38 +++++++++++++++++++++++++-- src/app.ts | 23 ++++++++++++++++ src/discovery.test.ts | 1 + src/hydra-admin.test.ts | 8 ++++++ src/hydra-admin.ts | 12 +++++++-- src/oauth-consent.test.ts | 23 +++++++++++++--- src/oauth-consent.ts | 11 ++++++-- src/oauth-login.test.ts | 1 + src/plugin.ts | 8 +++--- todo.md | 2 +- views/oauth-consent.ejs | 8 +++++- views/partials/client-detail-body.ejs | 1 + views/partials/client-form-body.ejs | 1 + views/partials/consent-body.ejs | 10 ++++--- 17 files changed, 138 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index a8a12c0..00bc42c 100644 --- a/README.md +++ b/README.md @@ -501,8 +501,11 @@ the browser here, the app resolves it against the Kratos session and accepts (or an unauthenticated user to the themed login, returning here once signed in). The **consent challenge** is wired too (`src/oauth-consent.ts` at `/oauth2/consent`): a first-party client (its Hydra `metadata.first_party: true`) — or one Hydra already skipped — is auto-granted the -requested scopes; any other client gets a themed consent screen whose CSRF-guarded Allow/Deny -accepts or rejects. id_token claims (email, name) come from the Kratos identity. +requested scopes; any other client gets a themed consent screen (naming the signed-in account, with +a sign-out escape) whose CSRF-guarded Allow/Deny accepts or rejects. id_token claims (email, name) +come from the Kratos identity. RP-initiated **logout** is wired too (`/oauth2/logout`): Hydra hands +the browser here, the app accepts the `logout_challenge` and resumes to Hydra's post-logout redirect +— the first-party `POST /logout` still owns ending the Kratos session + our JWT cookie. Those clients are registered from the admin **OAuth2 clients** screen (`/admin/clients`, `src/admin-clients.ts`): register (Hydra shows the generated `client_secret` **once**, on the diff --git a/e2e/oauth-login.spec.ts b/e2e/oauth-login.spec.ts index 7f17f4e..70b95ed 100644 --- a/e2e/oauth-login.spec.ts +++ b/e2e/oauth-login.spec.ts @@ -141,6 +141,7 @@ test("Hydra consent challenge: web shows the third-party consent screen; Allow const html = await screen.text(); expect(html).toContain("Authorize e2e-login"); expect(html).toContain("openid"); + expect(html, "names the signed-in account so consent is informed").toContain(`Signed in as ${ADMIN_EMAIL}`); const csrf = html.match(/name="_csrf" value="([^"]+)"/)?.[1]; expect(csrf, "consent form carries a CSRF token").toBeTruthy(); diff --git a/public/css/auth.css b/public/css/auth.css index 0165968..a9f2fbd 100644 --- a/public/css/auth.css +++ b/public/css/auth.css @@ -119,8 +119,9 @@ body:has(#forgot:target) #login { display: none; } /* alternate action line */ .auth-alt { margin: 0; text-align: center; font-size: var(--fz-sm); color: var(--text-muted); } -.auth-alt a { color: var(--accent); font-weight: 550; } -.auth-alt a:hover { text-decoration: underline; } +.auth-alt a, .auth-alt button { color: var(--accent); font-weight: 550; } +.auth-alt button { background: none; border: 0; padding: 0; font: inherit; cursor: pointer; } +.auth-alt a:hover, .auth-alt button:hover { text-decoration: underline; } /* legal footnote */ .auth-foot { margin: 0; text-align: center; font-size: var(--fz-xs); color: var(--text-faint); } diff --git a/src/app.test.ts b/src/app.test.ts index f0c442d..6f51e68 100644 --- a/src/app.test.ts +++ b/src/app.test.ts @@ -498,6 +498,7 @@ test("logout (CSRF-guarded POST): valid token revokes the Kratos session + clear const stubHydra = (over: Partial = {}): HydraAdmin => ({ acceptConsentRequest: async () => ({ redirect: "http://127.0.0.1:4444/oauth2/auth?consent_verifier=v" }), acceptLoginRequest: async () => ({ redirect: "http://127.0.0.1:4444/oauth2/auth?login_verifier=v" }), + acceptLogoutRequest: async () => ({ redirect: "http://acme.example/post-logout" }), createClient: async (c) => ({ ...c, client_id: "c1", client_secret: "s3cr3t" }), deleteClient: async () => {}, getClient: async () => null, @@ -593,6 +594,9 @@ test("OAuth2 consent challenge (/oauth2/consent): skip auto-accepts; a third-par assert.match(html, /openid/); assert.match(html, /profile/); assert.match(html, /action="\/oauth2\/consent"/); + // Informed consent: the screen names the account being authorized + offers a sign-out escape. + assert.match(html, /Signed in as.*ada@x\.io/s); + assert.match(html, /action="\/logout".*Sign out/s); assert.match(page.headers.get("set-cookie") ?? "", /plainpages_csrf=/); // Allow → 303 to Hydra, granting the scopes re-read from the challenge (never form-supplied) + @@ -631,6 +635,32 @@ test("OAuth2 consent challenge (/oauth2/consent): skip auto-accepts; a third-par assert.equal((await fetch(`http://localhost:${(down.address() as AddressInfo).port}/oauth2/consent?consent_challenge=x`, { redirect: "manual" })).status, 500); }); +test("OAuth2 RP-initiated logout (/oauth2/logout): accepts the logout challenge → 303 to Hydra; missing → 400; stale 4xx → 400, outage 5xx → 500", async (t) => { + let acceptedChallenge: string | undefined; + const hydra = stubHydra({ acceptLogoutRequest: async (c) => { acceptedChallenge = c; return { redirect: "http://acme.example/post-logout" }; } }); + const app = createApp({ hydra, kratos: withWhoami(async () => null) }); + await new Promise((r) => app.listen(0, r)); + t.after(() => app.close()); + const base = `http://localhost:${(app.address() as AddressInfo).port}`; + + const ok = await fetch(base + "/oauth2/logout?logout_challenge=lc1", { redirect: "manual" }); + assert.equal(ok.status, 303); + assert.equal(ok.headers.get("location"), "http://acme.example/post-logout"); + assert.equal(acceptedChallenge, "lc1"); + + assert.equal((await fetch(base + "/oauth2/logout", { redirect: "manual" })).status, 400); + + const stale = createApp({ hydra: stubHydra({ acceptLogoutRequest: async () => { throw new HydraError("gone", 404, ""); } }), kratos: withWhoami(async () => null) }); + await new Promise((r) => stale.listen(0, r)); + t.after(() => stale.close()); + assert.equal((await fetch(`http://localhost:${(stale.address() as AddressInfo).port}/oauth2/logout?logout_challenge=gone`, { redirect: "manual" })).status, 400); + + const down = createApp({ hydra: stubHydra({ acceptLogoutRequest: async () => { throw new HydraError("down", 503, ""); } }), kratos: withWhoami(async () => null) }); + await new Promise((r) => down.listen(0, r)); + t.after(() => down.close()); + assert.equal((await fetch(`http://localhost:${(down.address() as AddressInfo).port}/oauth2/logout?logout_challenge=x`, { redirect: "manual" })).status, 500); +}); + // Built-in Users admin screen (§5): gate + every CRUD action over HTTP against a mock Kratos admin. test("admin Users screen: gate, list/filter, create, edit, deactivate, delete, recovery (CSRF-guarded)", async (t) => { const mk = (email: string, over: Partial = {}): Identity => @@ -866,8 +896,11 @@ test("admin OAuth2 clients screen: gate, list, register (one-time secret), detai assert.match(listHtml, /href="\/admin\/clients\/new"/); assert.match(listHtml, /Reporting/); - // Register: the form renders; a valid post creates the client and shows the one-time secret + id. - assert.match(await (await get("/admin/clients/new")).text(), /Register client/); + // Register: the form renders (with confidential-vs-public guidance); a valid post creates the + // client and shows the one-time secret + id. + const formHtml = await (await get("/admin/clients/new")).text(); + assert.match(formHtml, /Register client/); + assert.match(formHtml, /can't keep a secret/i); // guidance on the public-vs-confidential choice const created = await post("/admin/clients", `_csrf=${token}&name=Grafana&redirectUris=${encodeURIComponent("https://graf/cb")}&scope=openid+offline_access`); assert.equal(created.status, 200); // not a redirect — the secret is shown once const createdHtml = await created.text(); @@ -886,6 +919,7 @@ test("admin OAuth2 clients screen: gate, list, register (one-time secret), detai const detail = await (await get("/admin/clients/existing")).text(); assert.match(detail, /reporting\.example\/cb/); assert.doesNotMatch(detail, /Client secret/i); // the secret is shown only once, at creation + assert.match(detail, /delete and re-register/i); // no edit: the lifecycle guidance is surfaced assert.match(detail, /href="\/admin\/clients\/existing\/delete"/); // Delete: a confirm step (GET) then the POST removes the client, back to the list. diff --git a/src/app.ts b/src/app.ts index 4a0ac68..cd6eb45 100644 --- a/src/app.ts +++ b/src/app.ts @@ -298,6 +298,29 @@ export function createApp(options: AppOptions = {}): Server { } } + // OAuth2 RP-initiated logout (§6): Hydra hands the browser here to end the OAuth2 session + // (hydra.yml urls.logout). Accept the challenge and resume to Hydra's post-logout redirect; + // the first-party POST /logout (below) owns the Kratos session + our JWT cookie. Provider-only. + // GET-accept is safe (like the login/consent handlers): the challenge is Hydra-minted + + // single-use, so a forged GET can't fabricate one — we skip only the optional "confirm logout?". + if (hydra && pathname === "/oauth2/logout" && (method === "GET" || method === "HEAD")) { + const challenge = ctx.url.searchParams.get("logout_challenge"); + if (!challenge) { + res.writeHead(400, { "content-type": "text/plain; charset=utf-8" }).end("Missing logout_challenge"); + return; + } + try { + const { redirect } = await hydra.acceptLogoutRequest(challenge); + res.writeHead(303, { location: redirect }).end(); + } catch (err) { + // Stale/consumed challenge (Hydra 4xx) → recoverable 400; a genuine outage (5xx) → 500. + if (err instanceof HydraError && err.status < 500) { + res.writeHead(400, { "content-type": "text/plain; charset=utf-8" }).end("This logout request has expired. Please start again from the application you were signing out of."); + } else throw err; + } + return; + } + // Login completion: where Kratos lands the browser after authenticating (kratos.yml). // Mint our session JWT — read roles from Keto, project onto the identity, tokenize — // and store it as the cookie; no active session bounces back to sign in (§4). diff --git a/src/discovery.test.ts b/src/discovery.test.ts index 98782a2..72f33b3 100644 --- a/src/discovery.test.ts +++ b/src/discovery.test.ts @@ -41,6 +41,7 @@ const badCases: Array<{ name: string; files: Record; match: RegE { name: "invalid folder name", files: { "Bad_Name/plugin.ts": full("x") }, match: /Bad_Name/ }, { name: "reserved id shadows a host route", files: { "login/plugin.ts": full("login") }, match: /login.*reserved/s }, { name: "reserved admin id shadows the admin screens", files: { "admin/plugin.ts": full("admin") }, match: /admin.*reserved/s }, + { name: "reserved oauth2 id shadows the provider routes", files: { "oauth2/plugin.ts": full("oauth2") }, match: /oauth2.*reserved/s }, { name: "missing plugin.ts", files: { "broken/readme.txt": "x" }, match: /broken.*plugin\.ts/s }, { name: "no default export", files: { "named-only/plugin.ts": "export const x = 1;" }, match: /named-only.*default/s }, { name: "import throws", files: { "explodes/plugin.ts": "throw new Error('boom');" }, match: /explodes.*boom/s }, diff --git a/src/hydra-admin.test.ts b/src/hydra-admin.test.ts index 6a83dc9..f9c2c18 100644 --- a/src/hydra-admin.test.ts +++ b/src/hydra-admin.test.ts @@ -51,6 +51,14 @@ test("rejectLoginRequest PUTs the error and returns Hydra's redirect_to", async assert.deepEqual(JSON.parse(calls[0]!.body!), { error: "access_denied", error_description: "no" }); }); +test("acceptLogoutRequest PUTs the logout challenge and returns Hydra's redirect_to", async () => { + const { calls, fetchImpl } = recorder(() => res(200, { redirect_to: "http://client/post-logout" })); + const out = await createHydraAdmin({ baseUrl: BASE, fetchImpl }).acceptLogoutRequest(CHALLENGE); + assert.equal(out.redirect, "http://client/post-logout"); + assert.equal(calls[0]!.method, "PUT"); + assert.match(calls[0]!.url, /\/admin\/oauth2\/auth\/requests\/logout\/accept\?logout_challenge=a1b2c3d4e5f6$/); +}); + test("getConsentRequest GETs the consent challenge and returns the request", async () => { const request = { challenge: CHALLENGE, client: { client_name: "Acme" }, requested_scope: ["openid", "email"], skip: false, subject: SUBJECT }; const { calls, fetchImpl } = recorder(() => res(200, request)); diff --git a/src/hydra-admin.ts b/src/hydra-admin.ts index 14572c7..53fab4d 100644 --- a/src/hydra-admin.ts +++ b/src/hydra-admin.ts @@ -90,6 +90,7 @@ export class HydraError extends Error { export interface HydraAdmin { acceptConsentRequest(challenge: string, body: AcceptConsent): Promise; acceptLoginRequest(challenge: string, body: AcceptLogin): Promise; + acceptLogoutRequest(challenge: string): Promise; // RP-initiated logout (§6): confirm + resume createClient(client: OAuth2Client): Promise; deleteClient(id: string): Promise; getClient(id: string): Promise; @@ -111,8 +112,8 @@ export function createHydraAdmin(config: { baseUrl: string; fetchImpl?: typeof f const base = config.baseUrl.replace(/\/+$/, ""); const http = config.fetchImpl ?? fetch; const json = { "content-type": "application/json" }; - // Hydra keys the login/consent handshake off a ?login_challenge=/?consent_challenge= query. - const reqUrl = (kind: "consent" | "login", challenge: string, action = "") => + // Hydra keys each handshake off a ?_challenge= query (login/consent/logout). + const reqUrl = (kind: "consent" | "login" | "logout", challenge: string, action = "") => `${base}/admin/oauth2/auth/requests/${kind}${action}?${kind}_challenge=${encodeURIComponent(challenge)}`; const clientsUrl = `${base}/admin/clients`; const clientUrl = (id: string) => `${clientsUrl}/${encodeURIComponent(id)}`; @@ -137,6 +138,13 @@ export function createHydraAdmin(config: { baseUrl: string; fetchImpl?: typeof f return put("accept login", reqUrl("login", challenge, "/accept"), body); }, + // RP-initiated logout: Hydra hands the browser to /oauth2/logout?logout_challenge=…; accept to + // end its OAuth2 session and get the post-logout redirect (no body / no first-party teardown — + // /logout owns the Kratos session). A stale/consumed challenge → HydraError 4xx (app degrades). + async acceptLogoutRequest(challenge) { + return put("accept logout", reqUrl("logout", challenge, "/accept"), {}); + }, + // OAuth2 client registration (§6, admin screen). Hydra generates the client_id/secret when // omitted; the secret rides the 201 body and is never retrievable afterwards. async createClient(client) { diff --git a/src/oauth-consent.test.ts b/src/oauth-consent.test.ts index 80df9e8..b3dd918 100644 --- a/src/oauth-consent.test.ts +++ b/src/oauth-consent.test.ts @@ -17,6 +17,7 @@ function stubHydra(consent: ConsentRequest, capture?: (b: AcceptConsent) => void return { acceptConsentRequest: async (_c, body) => { capture?.(body); return { redirect: REDIRECT }; }, acceptLoginRequest: unused, + acceptLogoutRequest: unused, createClient: unused, deleteClient: unused, getClient: unused, @@ -59,15 +60,31 @@ test("a first-party client (metadata.first_party) auto-accepts even without skip assert.equal(granted?.session, undefined); }); -test("a third-party client shows the consent screen (no auto-accept)", async () => { +test("a third-party client shows the consent screen carrying the signed-in account (no auto-accept)", async () => { let accepted = false; const hydra = stubHydra(consent(), () => { accepted = true; }); - const out = await resolveConsentChallenge({ hydra, kratos: stubKratos(async () => null) }, CHALLENGE, undefined); + const kratos = stubKratos(async () => sessionWith({ email: "ada@x.io" })); + const out = await resolveConsentChallenge({ hydra, kratos }, CHALLENGE, "plainpages_session=s"); assert.equal(out.redirect, undefined); - assert.deepEqual(out.view, { challenge: CHALLENGE, client: "Acme Reports", scopes: ["openid", "profile"] }); + assert.deepEqual(out.view, { account: "ada@x.io", challenge: CHALLENGE, client: "Acme Reports", scopes: ["openid", "profile"] }); assert.equal(accepted, false); }); +test("the consent screen omits the account when there's no session", async () => { + const out = await resolveConsentChallenge({ hydra: stubHydra(consent()), kratos: stubKratos(async () => null) }, CHALLENGE, undefined); + assert.equal(out.view?.account, undefined); +}); + +test("id_token claims are only projected when the session subject matches the challenge (else omitted)", async () => { + let granted: AcceptConsent | undefined; + const hydra = stubHydra(consent(), (b) => { granted = b; }); + // A session whose identity differs from the challenge subject must not leak its claims into the grant. + const other: Session = { active: true, identity: { id: "01902d5e-0000-7e3a-9f21-3c8d1e0a4b55", traits: { email: "mallory@x.io" } } }; + const redirect = await acceptConsent({ hydra, kratos: stubKratos(async () => other) }, CHALLENGE, "plainpages_session=s"); + assert.equal(redirect, REDIRECT); + assert.equal(granted?.session, undefined); +}); + test("acceptConsent re-fetches the challenge and grants its scopes (never client-supplied)", async () => { let granted: AcceptConsent | undefined; const hydra = stubHydra(consent(), (b) => { granted = b; }); diff --git a/src/oauth-consent.ts b/src/oauth-consent.ts index 71c29c4..716b7fb 100644 --- a/src/oauth-consent.ts +++ b/src/oauth-consent.ts @@ -17,6 +17,7 @@ export interface OAuthConsentDeps { // What to show on the consent screen for a third-party client. export interface ConsentView { + account?: string; // the signed-in user's email — shown so consent is informed (whose account) challenge: string; client: string; // display name scopes: string[]; @@ -47,7 +48,9 @@ function idTokenClaims(traits?: Record): Record { const session = await deps.kratos.whoami(cookie ? { cookie } : {}); - const idToken = idTokenClaims(session?.identity?.traits); + // Only project id_token claims when the session's identity matches the subject Hydra bound at + // login — never leak a mismatched session's email/name into the issued token (defensive). + const idToken = session?.identity?.id === consent.subject ? idTokenClaims(session?.identity?.traits) : undefined; const body: AcceptConsent = { grant_access_token_audience: consent.requested_access_token_audience ?? [], grant_scope: consent.requested_scope ?? [], @@ -64,7 +67,11 @@ export async function resolveConsentChallenge(deps: OAuthConsentDeps, challenge: if (consent.skip || isFirstParty(consent.client)) { return { redirect: await accept(deps, consent, cookie) }; } - return { view: { challenge, client: clientName(consent.client), scopes: consent.requested_scope ?? [] } }; + // Third party: name the signed-in account on the screen so the user sees whose access they grant. + const session = await deps.kratos.whoami(cookie ? { cookie } : {}); + const email = session?.identity?.traits?.email; + const account = typeof email === "string" ? email : undefined; + return { view: { challenge, client: clientName(consent.client), scopes: consent.requested_scope ?? [], ...(account ? { account } : {}) } }; } // The user allowed: re-fetch the challenge (don't trust the form for scopes) and accept. diff --git a/src/oauth-login.test.ts b/src/oauth-login.test.ts index 782fb0d..5edfdd6 100644 --- a/src/oauth-login.test.ts +++ b/src/oauth-login.test.ts @@ -15,6 +15,7 @@ function stubHydra(login: LoginRequest, capture?: (b: AcceptLogin) => void): Hyd return { acceptConsentRequest: unused, acceptLoginRequest: async (_c, body) => { capture?.(body); return { redirect: "http://hydra/oauth2/auth?login_verifier=v" }; }, + acceptLogoutRequest: unused, createClient: unused, deleteClient: unused, getClient: unused, diff --git a/src/plugin.ts b/src/plugin.ts index e94e0b1..bc688ef 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -78,11 +78,11 @@ export function isValidPluginId(id: string): boolean { } // Ids the host reserves for its own first-party mount segments (the auth flows, /auth/complete, -// /logout, the /admin screens, the dashboard's /public/ static). Plugin routes resolve before -// these, so a folder named one of them would silently shadow a built-in route — discovery refuses -// it, loud like any conflict. +// /logout, the /admin screens, the /oauth2 provider routes, the dashboard's /public/ static). +// Plugin routes resolve before these, so a folder named one of them would silently shadow a +// built-in route — discovery refuses it, loud like any conflict. export const RESERVED_PLUGIN_IDS: ReadonlySet = new Set([ - "admin", "auth", "login", "logout", "public", "recovery", "registration", "settings", "verification", + "admin", "auth", "login", "logout", "oauth2", "public", "recovery", "registration", "settings", "verification", ]); export interface Semver { diff --git a/todo.md b/todo.md index 4d05c9b..cf141e1 100644 --- a/todo.md +++ b/todo.md @@ -105,7 +105,7 @@ everything via Docker. - [x] Login-challenge handler: authenticate via Kratos session, accept/reject. → `src/hydra-admin.ts` (`createHydraAdmin`): typed `fetch` wrappers over Hydra's OAuth2 admin API (port 4445, no SDK, `fetchImpl`-injectable like the kratos/keto clients) — `getLoginRequest`/`acceptLoginRequest`/`rejectLoginRequest` + a `HydraError` carrying `.status`. `src/oauth-login.ts` (`resolveLoginChallenge`, pure): `getLoginRequest` → **skip** (Hydra already authenticated the subject) ⇒ accept it without touching Kratos; a live **Kratos session** (`whoami`) ⇒ accept with that identity as the subject (`remember`, browser-session lifetime); **no session** ⇒ bounce to our themed `/login?return_to=`, so Kratos lands back on the challenge once signed in. Wired into `app.ts` at `GET /oauth2/login` (gated on `hydra`+`kratos` present; missing `login_challenge`→400; the absolute return target derives from the request Host + the SECURE_COOKIES scheme — a spoofed Host can't escape, Kratos validates `return_to` against its allow-list); `/login` now bakes a `return_to` into the Kratos flow init so the round-trip works. `config.ts` gains `hydraAdminUrl` (default `http://hydra:4445`); `server.ts` builds the client; `compose.yml` `web` now gates on `hydra` healthy (the app consumes it). Tests-first: `hydra-admin.test.ts` (request contracts + error mapping), `oauth-login.test.ts` (skip/session/no-session matrix), `app.test.ts` (HTTP: accept→Hydra redirect / no-session→/login bounce / missing-challenge→400 / `/login` return_to forwarding), `config.test.ts` + `compose.test.ts` (web↦hydra dep). Full-stack E2E `e2e/oauth-login.spec.ts` (`compose.e2e-oauth.yml`): boots the real stack incl. Hydra, registers an OAuth2 client, starts an authorization flow, asserts the unauthenticated bounce **and** the authenticated accept (→ Hydra `/oauth2/auth?…login_verifier=…`) — green, then torn down. Stability-reviewer run as a local PR: APPROVE, no Critical/High; addressed its one stability warning — a stale/invalid/consumed challenge (Hydra 4xx, user-reachable via back button/slow login) now degrades to a recoverable 400 instead of a 500, while a genuine Hydra 5xx outage still surfaces as 500 (mirrors the themed-flow + §4 re-mint hardening). Deferred (reviewer-scoped, §9): document that prod `allowed_return_urls` entries must be exact origins with a trailing `/` (the return_to safety leans on Kratos' allow-list). typecheck + 253 units + 8 visual E2E green. Consent handler + client registration are the next §6 items. - [x] Consent-challenge handler: show / auto-accept first-party, grant scopes, accept/reject. → `src/hydra-admin.ts` gains the consent half of the handshake (`getConsentRequest`/`acceptConsentRequest`/`rejectConsentRequest` + `ConsentRequest`/`AcceptConsent`/`ConsentSession` types; the login/consent URL builder folded into one `reqUrl(kind,…)` + a shared `put()`). `src/oauth-consent.ts` (pure, sibling of `oauth-login.ts`): `resolveConsentChallenge` → **skip** (Hydra already consented / a skip-consent client) or **first-party** (the client's Hydra `metadata.first_party === true`) ⇒ auto-accept, else return a `view` to show the themed consent screen; `acceptConsent` (re-reads the challenge so scopes/audience are **never** client-supplied) + `rejectConsent` (access_denied). The grant carries an OIDC `session.id_token` with `email`/`name` projected from the Kratos identity (`whoami` traits; absent ⇒ omitted). Wired in `app.ts` at `GET|POST /oauth2/consent` (gated on `hydra`+`kratos`): GET shows/auto-accepts (sets the page CSRF cookie when fresh), POST is **CSRF-guarded** (same signed double-submit as `/logout`) and dispatches `decision=allow`→accept / else→reject → 303 to Hydra; a stale/consumed challenge (Hydra 4xx) degrades to a recoverable 400, a genuine outage (5xx) → 500 (mirrors `/oauth2/login`). `views/oauth-consent.ejs` + `partials/consent-body.ejs` reuse the auth-card: the consent screen lists the requested scopes (friendly labels for the standard OIDC ones) with Allow/Deny submit buttons. Tests-first: `hydra-admin.test.ts` (consent request contracts), `oauth-consent.test.ts` (skip/first-party/third-party/audience/id_token/accept-refetch/reject matrix), `app.test.ts` HTTP integration (auto-accept / screen render+CSRF cookie / allow+deny / forged-CSRF→403 / missing→400 / stale→400 / outage→500). Stability-reviewer run as a local PR: APPROVE, no Critical/High. Extended the full-stack E2E `e2e/oauth-login.spec.ts` to drive the **whole** authorization-code flow against real Hydra — login accept → follow the login_verifier through Hydra → web's consent screen (third-party client `e2e-login`, scopes listed) → Allow → consent_verifier → the client callback with a real `code` (per-host cookie jars; Hydra resume URLs rebased onto the compose host). typecheck + 262 units + 8 visual + OAuth login+consent E2E green; stack torn down. OAuth2 client registration is the next §6 item. - [x] OAuth2 client registration (admin UI or CLI). → Built-in **OAuth2 clients** admin screen (`src/admin-clients.ts`, `/admin/clients`) — the §6 client side of Hydra (apps that log in *through* us). `src/hydra-admin.ts` gains the client half of the admin API: `createClient`/`listClients`/`getClient`/`deleteClient` over `/admin/clients` (+ a `nextPageToken` Link parser, mirrors kratos-admin) and the registration fields on `OAuth2Client`. The screen mirrors the §5 Users/Roles pattern — pure builders (`toClientView`, `clientPayload`, `validateClientInput`, `parseRedirectUris`, `buildClients{List,Form,Detail}Model`) + `handleAdminClients` (the imperative shell app.ts dispatches `/admin/clients*` to). Routes: `GET /admin/clients` (list — search/paginate over one Hydra page), `GET|POST /admin/clients/new`+`/` (register), `GET /admin/clients/:id` (read-only detail), `GET|POST …/:id/delete` (confirm + delete). Register builds a standard authorization-code client (+ refresh_token), confidential (`client_secret_basic`) or public (PKCE, `none`), with an optional first-party (auto-consent) flag; **Hydra returns the `client_secret` once**, so the register POST renders the new client's detail page with the one-time secret directly (no PRG) — never re-shown (`getClient` carries no secret; detail asserts it). Writes go **only to Hydra**; gated admin-only (anon→/login, non-admin→403) + every mutation CSRF-guarded, like §5; a Hydra 4xx (bad redirect/scope) re-renders the form (400), a 5xx → 500 (mirrors `oauth-login.ts`); `:id` via `safeDecode` (malformed→404). Wired into the shared `adminSection` (Users·Groups·Roles·**OAuth2 clients**, `i-globe`) so it shows for admins, invisible otherwise. New views (`admin/clients`,`client-form`,`client-detail` + `partials/client-{form,detail}-body`) reuse the shell/filter-bar/data-table/field blocks; one `.detail-list` CSS rule. Tests-first: `hydra-admin.test.ts` (client CRUD contracts incl. Link pagination/404→null/204), `admin-clients.test.ts` (builder/validation/payload matrix), `app.test.ts` HTTP integration (gate/list/register-shows-secret-once/invalid+CSRF-reject/detail-hides-secret/delete + malformed-`%`→404). Stability-reviewer run as a local PR: APPROVE, no Critical/High; addressed its one nit (dropped a dead `URL.protocol` check in `validateClientInput`). Boot-verified the client CRUD live against real Hydra v26.2.0 (create→201 w/ one-time secret → list finds it → get → delete → get null); torn down. typecheck + 274 units green. Review/comment/test-cleanup are the next §6 items. -- [ ] Run the architecture and the product reviewer agents on the _whole_ project, not just the latest changes, and address their issues. +- [x] Run the architecture and the product reviewer agents on the _whole_ project, not just the latest changes, and address their issues. → Ran both on the whole project (weighted to the §6 Hydra OAuth2 surfaces). Architecture: **no Critical**; Product: **no Critical** this checkpoint. **Fixed now (tests-first):** (1) HIGH (arch) — `/oauth2/logout` was published to Hydra (`hydra.yml` `urls.logout`) and asserted in `hydra.test.ts`, but had **no handler** (a dead/published contract — Hydra's RP-initiated logout would 404). Added `hydra-admin.acceptLogoutRequest` (PUT logout/accept, folded into the shared `reqUrl(kind…)`) + a `GET /oauth2/logout` branch: accept the `logout_challenge` → 303 to Hydra's post-logout redirect; missing challenge → 400; a stale/consumed 4xx → recoverable 400, a 5xx outage → 500 (byte-identical degrade to the login/consent siblings). GET-accept is safe — the challenge is Hydra-minted + single-use; the first-party `POST /logout` still owns ending the Kratos session + our JWT cookie. (2) HIGH (arch) — added `oauth2` to `RESERVED_PLUGIN_IDS` (a `plugins/oauth2/` folder would silently shadow the provider routes — the one route surface the §4 reserved-id fix didn't cover; discovery now refuses it loud). (3) Product **Blocker** — the consent screen never told the user *whose* account they were authorizing (informed-consent gap on shared devices). It now renders "Signed in as ``" (`ConsentView.account` from `whoami`) + a "Not you? Sign out" form (CSRF-guarded by the same signed double-submit). (4) MEDIUM (arch) — consent `accept()` now projects id_token claims **only when** the live Kratos session subject `===` the challenge subject Hydra bound at login (never leak a mismatched session's email/name into the issued token; guards the auto-accept path too). (5) Product nits — register-form confidential-vs-public guidance ("Browser/mobile apps can't keep a secret — choose Public…") and a client-detail "to change a client, delete and re-register — the secret is shown only once" note (covers the no-edit friction + lost-secret-on-reload). Stability-reviewer run as a local PR: **APPROVE, no Critical/High**; addressed its actionable follow-ups (README §6 now documents the logout handler + the consent identity line; a comment notes the GET-accept is Hydra-validated). Extended `e2e/oauth-login.spec.ts` to assert the consent screen names the signed-in account; **boot-verified the full OAuth2 login+consent flow live against real Hydra v26.2.0** (E2E green) then torn down. typecheck + 279 units green. **Deferred (reviewer-scoped, not the §6 checkpoint):** the host **internal route-table** (fold the admin/oauth if-ladder into one `{method,prefix,handler}` table, derive `RESERVED_PLUGIN_IDS`/`allowedMethods` from it — arch M1, the long-deferred §2/§5 item) → **§9**: H1/H2 are now point-fixed, so M1 is reduced to a pure dedup/structural improvement (Medium) best done as a focused standalone change to the central dispatcher, not bundled into a review-fix; the **RP-initiated-logout browser/live E2E** (needs token-exchange + `id_token_hint` + `post_logout_redirect_uris`) → **§8** (owns full E2E — the handler is unit+HTTP covered and reuses the `complete()` path already live-verified by the login/consent accepts); the redirect-URI **scheme allowlist** + the `safeUrl()` href helper (arch L1) → **§7** (first untrusted-URL flow); full **client edit** (registration-only by design — the detail page now says delete+re-register), the blank **empty-list** state (known §5 deferral), and success-flash after writes → §8/polish; unconditional `refresh_token` + raw custom-scope labels (product 🟢) → future. - [ ] Go over all comments in the code and the README and try to make it shorter and more information dense. Remove not strictly needed stuff. - [ ] Go over all tests and combine/unify ones that cover the same stuff or are very related and could be combined in a good way. Remove tests that aren't helping, we only want tests that are actually helpful to us. diff --git a/views/oauth-consent.ejs b/views/oauth-consent.ejs index 1ff144e..5c26c4f 100644 --- a/views/oauth-consent.ejs +++ b/views/oauth-consent.ejs @@ -4,7 +4,7 @@ /oauth2/consent route, CSRF-guarded (consent-body carries the token). Auto theme (styles.css). %><% const brand = locals.brand || "Plainpages"; - const body = include("partials/consent-body", { challenge: consent.challenge, csrfField, csrfToken, scopes: consent.scopes }); + const body = include("partials/consent-body", { account: consent.account, challenge: consent.challenge, csrfField, csrfToken, scopes: consent.scopes }); %> @@ -30,6 +30,12 @@ sub: `${consent.client} wants access to your account.`, title: `Authorize ${consent.client}`, }) %> + <% if (consent.account) { %> +
+ + Not you? +
+ <% } %> diff --git a/views/partials/client-detail-body.ejs b/views/partials/client-detail-body.ejs index 0123c17..b669889 100644 --- a/views/partials/client-detail-body.ejs +++ b/views/partials/client-detail-body.ejs @@ -32,6 +32,7 @@
+

To change a client, delete and re-register — this issues a new client ID and secret. The secret is shown only once, at registration.

Delete client
diff --git a/views/partials/client-form-body.ejs b/views/partials/client-form-body.ejs index 73a71f2..2c740e4 100644 --- a/views/partials/client-form-body.ejs +++ b/views/partials/client-form-body.ejs @@ -20,6 +20,7 @@ <%- include("field", form.scopeField) %> + Browser and mobile apps can't keep a secret — choose Public. Server-side apps that can store one — leave it Confidential.
Cancel diff --git a/views/partials/consent-body.ejs b/views/partials/consent-body.ejs index bd7f70b..854c198 100644 --- a/views/partials/consent-body.ejs +++ b/views/partials/consent-body.ejs @@ -1,9 +1,13 @@ <%# - OAuth2 consent form body (todo §6): the inner of the auth-card form — the CSRF + challenge - hidden inputs, the requested scopes, then Allow / Deny submit buttons (one `decision` field). - Locals: challenge, csrfField, csrfToken, scopes (string[]). Captured by views/oauth-consent.ejs. + OAuth2 consent form body (todo §6): the inner of the auth-card form — the signed-in account, the + CSRF + challenge hidden inputs, the requested scopes, then Allow / Deny submit buttons (one + `decision` field). Locals: account?, challenge, csrfField, csrfToken, scopes (string[]). Captured + by views/oauth-consent.ejs. -%> <% const labels = { email: "Your email address", offline_access: "Stay signed in (offline access)", openid: "Verify your identity", profile: "Your basic profile (name)" }; -%> +<% if (locals.account) { -%> +

Signed in as <%= account %>

+<% } -%> <% if (scopes.length) { -%>