§6 review checkpoint (todo §6); ran the architecture + product reviewers on the whole project (weighted to the Hydra OAuth2 surfaces) and addressed their findings — no Critical from either. Fixed tests-first: (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; added hydra-admin.acceptLogoutRequest (PUT logout/accept via the shared reqUrl(kind…)) + a GET /oauth2/logout branch that accepts the RP-initiated logout_challenge → 303 to Hydra's post-logout redirect (missing→400, stale 4xx→recoverable 400, 5xx→500, byte-identical degrade to the login/consent siblings; GET-accept is safe since the challenge is Hydra-minted+single-use; the first-party POST /logout still owns ending the Kratos session + JWT cookie). (HIGH, arch) added oauth2 to RESERVED_PLUGIN_IDS so a plugins/oauth2/ folder can't silently shadow the provider routes (the route surface the §4 reserved-id fix missed; discovery now refuses it loud). (Product Blocker) the third-party consent screen now names the signed-in account — "Signed in as <email>" (ConsentView.account from whoami) — plus a CSRF-guarded "Not you? Sign out" form, so consent is informed on shared devices. (MEDIUM, arch) consent accept() now projects id_token claims only when the live Kratos session subject === the challenge subject Hydra bound at login, never leaking a mismatched session's email/name into the issued token (guards the auto-accept path too). (Product nits) register-form confidential-vs-public guidance + a client-detail "delete and re-register / secret shown once" note (no-edit friction + lost-secret). New tests across discovery (reserved oauth2), hydra-admin (acceptLogoutRequest contract), oauth-consent (subject-match + account-in-view), app.test (logout 303/400/500 matrix, consent identity+sign-out, client form/detail copy); e2e/oauth-login.spec asserts the consent screen names the account. Stability-reviewer run as a local PR: APPROVE, no Critical/High — addressed its doc/comment follow-ups (README §6 documents the logout handler + consent identity line; a comment notes the GET-accept is Hydra-validated). Deferred (reviewer-scoped): the host internal route-table (arch M1, now a pure dedup once H1/H2 are point-fixed) → §9; the RP-initiated-logout browser/live E2E → §8; redirect-URI scheme allowlist + safeUrl() → §7; full client edit / empty-list state / success-flash → §8/polish. typecheck + 279 units green; full-stack OAuth2 login+consent E2E verified live against real Hydra v26.2.0 then torn down.

This commit is contained in:
2026-06-19 11:47:06 +02:00
parent 1c324b18e3
commit 521c09fa2d
17 changed files with 138 additions and 22 deletions

View File

@@ -498,6 +498,7 @@ test("logout (CSRF-guarded POST): valid token revokes the Kratos session + clear
const stubHydra = (over: Partial<HydraAdmin> = {}): 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<void>((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<void>((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<void>((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> = {}): 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.

View File

@@ -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).

View File

@@ -41,6 +41,7 @@ const badCases: Array<{ name: string; files: Record<string, string>; 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 },

View File

@@ -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));

View File

@@ -90,6 +90,7 @@ export class HydraError extends Error {
export interface HydraAdmin {
acceptConsentRequest(challenge: string, body: AcceptConsent): Promise<Completed>;
acceptLoginRequest(challenge: string, body: AcceptLogin): Promise<Completed>;
acceptLogoutRequest(challenge: string): Promise<Completed>; // RP-initiated logout (§6): confirm + resume
createClient(client: OAuth2Client): Promise<OAuth2Client>;
deleteClient(id: string): Promise<void>;
getClient(id: string): Promise<OAuth2Client | null>;
@@ -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 ?<kind>_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) {

View File

@@ -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; });

View File

@@ -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<string, unknown>): Record<string, unknown
// the challenge, never client-submitted) plus id_token claims from the current Kratos session.
async function accept(deps: OAuthConsentDeps, consent: ConsentRequest, cookie: string | undefined): Promise<string> {
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.

View File

@@ -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,

View File

@@ -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<string> = new Set([
"admin", "auth", "login", "logout", "public", "recovery", "registration", "settings", "verification",
"admin", "auth", "login", "logout", "oauth2", "public", "recovery", "registration", "settings", "verification",
]);
export interface Semver {