From d1fbf8fa1f291dc785bcc9df1a12c5e0e23b0945 Mon Sep 17 00:00:00 2001 From: lilleman Date: Thu, 18 Jun 2026 11:52:49 +0200 Subject: [PATCH] =?UTF-8?q?Comment/README=20cleanup=20(todo=20=C2=A74);=20?= =?UTF-8?q?tighten=20the=20kratos/keto=20client=20module-headers=20(drop?= =?UTF-8?q?=20forward-refs=20+=20caller-listings,=20keep=20rationale),=20r?= =?UTF-8?q?etarget=20the=20stale=20safeUrl()=20ref=20in=20plugin-contract.?= =?UTF-8?q?md=20to=20=C2=A75/=C2=A77?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/plugin-contract.md | 3 ++- src/keto-client.ts | 9 ++++----- src/kratos-admin.ts | 9 ++++----- src/kratos-public.ts | 5 ++--- todo.md | 2 +- 5 files changed, 13 insertions(+), 15 deletions(-) diff --git a/docs/plugin-contract.md b/docs/plugin-contract.md index 7b24ac8..f4c33a8 100644 --- a/docs/plugin-contract.md +++ b/docs/plugin-contract.md @@ -158,7 +158,8 @@ safety of the data it renders**: item, a breadcrumb, `brand.logo` — is emitted as-is inside the attribute: a `javascript:` or `data:` URL from upstream/user data becomes live XSS. When a URL comes from data you don't control, restrict it to a relative (`/`, `?`, `#`) or `http(s):` URL before handing it to a - partial. (A shared `safeUrl()` helper lands with the data-driven plugins in §4.) + partial. (A shared `safeUrl()` helper will land with the first plugin that renders untrusted + URL data, §5/§7.) ## RequestContext diff --git a/src/keto-client.ts b/src/keto-client.ts index cd0fffa..592967e 100644 --- a/src/keto-client.ts +++ b/src/keto-client.ts @@ -1,9 +1,8 @@ // Keto client (todo §4): typed `fetch` wrappers over Ory Keto's relation-tuple APIs — -// `check` a permission, `listRelations`/`expand` to inspect them (read API), and -// `writeTuple`/`deleteTuple` to grant/revoke them (write API). Built-in `fetch` only, no -// SDK dep (AGENTS.md); `fetchImpl`-injectable like the kratos clients. read/write split -// onto the two ports config.ts targets (ketoReadUrl 4466 / ketoWriteUrl 4467). The login -// role projection (§4) reads roles via this; guards' live `check` (§4) calls `check`. +// `check` a permission, `listRelations`/`expand` to inspect them (read API), `writeTuple`/ +// `deleteTuple` to grant/revoke them (write API). Built-in `fetch` only, no SDK dep (AGENTS.md); +// `fetchImpl`-injectable like the kratos clients. Read/write split onto the two ports config.ts +// targets (ketoReadUrl 4466 / ketoWriteUrl 4467). // A subject set: a relation on another object (e.g. Group:eng#members), resolved // transitively. The other Keto subject form is a direct `subject_id` string. diff --git a/src/kratos-admin.ts b/src/kratos-admin.ts index 3c8ca4d..346dc14 100644 --- a/src/kratos-admin.ts +++ b/src/kratos-admin.ts @@ -1,8 +1,7 @@ -// Kratos admin-API client (todo §4): typed `fetch` wrappers over Ory Kratos' admin -// endpoints — identity CRUD and the surgical `metadata_public` update login completion -// projects Keto roles into (README). Built-in `fetch` only, no SDK dep (AGENTS.md); -// `fetchImpl`-injectable like kratos-public.ts. Reuses that module's `KratosError` so a -// caller can branch on `.status`. Admin endpoints listen on the internal-only admin port. +// Kratos admin-API client (todo §4): typed `fetch` wrappers over Ory Kratos' admin endpoints +// (internal-only admin port) — identity CRUD + the surgical `metadata_public` update login +// completion projects Keto roles into (README). Built-in `fetch` only, no SDK dep (AGENTS.md); +// `fetchImpl`-injectable, reuses kratos-public.ts's `KratosError` (branch on `.status`). import { KratosError } from "./kratos-public.ts"; export interface Identity { diff --git a/src/kratos-public.ts b/src/kratos-public.ts index c3f9d1d..a22eb3e 100644 --- a/src/kratos-public.ts +++ b/src/kratos-public.ts @@ -1,8 +1,7 @@ // Kratos public-API client (todo §4): typed `fetch` wrappers over Ory Kratos' public // endpoints — self-service flow init/get/submit, browser logout, session `whoami`, and the -// session→JWT tokenizer (`whoami?tokenize_as`). Built-in `fetch` only, no SDK dep (AGENTS.md). The -// themed flow pages and login completion (§4) build on this; rendering flow `ui.nodes` -// and mapping field errors is the renderer's job (§4), so we keep those types loose. +// session→JWT tokenizer (`whoami?tokenize_as`). Built-in `fetch` only, no SDK dep (AGENTS.md). +// Flow `ui.nodes` types stay loose — rendering + field-error mapping is flow-view.ts's job. export type FlowType = "login" | "recovery" | "registration" | "settings" | "verification"; diff --git a/todo.md b/todo.md index a3cdc32..87ffc63 100644 --- a/todo.md +++ b/todo.md @@ -89,7 +89,7 @@ everything via Docker. - [x] Secure cookie flags; CSRF for our own POST forms. → **Secure flag:** new explicit `SECURE_COOKIES` toggle (`config.ts`, default off — dev is http; `compose.yml` sets it `true`, `compose.override.yml`/`compose.e2e.yml` `false`), threaded through every first-party Set-Cookie (session JWT, clear, re-mint, CSRF). **CSRF:** `src/csrf.ts` — stateless **signed double-submit** token `.` (node:crypto, no dep): `issueCsrfToken`/`verifyCsrfToken` (self-validating, timing-safe), `ensureCsrfToken` (reuse a genuine `plainpages_csrf` cookie, else mint — one token across tabs), `csrfCookie` (HttpOnly+Lax, secure opt-in), `verifyCsrfRequest` (cookie genuine **and** field echoes it). `src/body.ts` `readFormBody` (size-capped urlencoded reader; §5 forms reuse it). Applied to our one first-party form: **logout is now a CSRF-guarded `POST`** — `shell.ejs`'s Sign-out is a `
` with a hidden `_csrf` (semantic win: a state change is a form, not a GET link), `app.ts` issues the token cookie on `GET /` and verifies it on `POST /logout` (bad/missing → 403, before any Kratos call); `dashboard.ts`→`index.ejs`→shell thread the token. Kratos' own flows keep Kratos' CSRF; the host does **not** auto-gate plugin routes (they own their body/safety per the contract). Switched the cookie-setting sites to `appendHeader` so the CSRF cookie coexists with others. Tests-first: `csrf.test.ts`/`body.test.ts` + extended `config`/`dashboard`/`shell`/`app` tests (logout POST: valid→Kratos logout + cleared JWT, no-session→/login, missing/forged→403) + an Ory-free E2E (GET / issues the cookie + matching form token; tokenless POST→403). typecheck + 217 units + 8 E2E green. Boot-verified live on the full stack: GET / double-submit token matches; admin login → `POST /logout` 303s to the real `…/self-service/logout?token=ory_lo_…` with the JWT cleared; no-session→/login; forged/missing→403; torn down. - [x] Make sure we have E2E tests for token timeouts and refresh (maybe by shorten the token lifetime to very low or something). → New full-stack Playwright suite `e2e/auth-refresh.spec.ts` (run via `compose.e2e-auth.yml`): boots the **real** Ory stack (Postgres + Kratos + Keto + bootstrap + web), logs in the seeded admin, completes login on web → session JWT, then proves the §4 "stay signed in" hot path end-to-end — once the token lapses the next request is silently **re-minted** from the live Kratos session (fresh JWT, later `exp`, roles re-read from Keto = `["admin"]`); revoking the Kratos session (admin API) then makes the next lapsed request **clear** the stale cookie (→ anonymous). To make timeout/refresh observable in seconds not ~10m: `ory/kratos/e2e.yml` (merged via a second `-c`) shortens the tokenizer `ttl` to **8s** and points `serve.public.base_url` at `kratos:4433` (so the runner drives self-service over the compose network), and a new explicit **`JWT_CLOCK_SKEW_SEC`** config (default 60, the E2E sets `0`) makes web treat the JWT as expired the instant its ttl lapses instead of +60s. The flow is driven over HTTP (fetch + manual cookie relay) because Kratos/web sit on different hosts here — it exercises web's own server-side relay; the browser-UI login stays §8. Scoped the existing visual suite to `visual.spec.ts` (stays Ory-free/fast) so the two suites don't cross-run. Tests-first for the config knob (`config.test.ts`). Verified live: auth suite green (re-mint + clear), visual suite still 8/8 green; typecheck + 218 units green; both stacks torn down. - [x] Run the architecture _and_ the stability reviewer agents on the _whole_ project, not just the latest changes, and address their issues. → Ran both on the whole project (weighted to the §4 auth hot path). Verdict: no Critical/High; both confirmed the auth core (alg-allowlist JWS verify, fail-closed `resolveSession`, key-by-`kid` cache, timing-safe CSRF, traversal guards) is sound, and that a tampered/garbage cookie **can't** drive the Ory re-mint round-trip (only a validly-signed, time-expired token sets `expired`). **Fixed now (tests-first):** (1) MEDIUM (stability) — the re-mint hot path turned an Ory *outage* into a 500 on **every** lapsed request (a dead Kratos session returns `null` and clears cleanly, but a 5xx/refused/timeout *throws* and escaped to the 500 handler). Wrapped the `remintSession` call in `app.ts` in try/catch → degrade to **anonymous** (route renders signed-out / guard bounces to `/login`), and leave the cookie untouched so it re-mints once Ory recovers; `app.test.ts` re-mint test now also asserts outage→403-not-500 + no cleared cookie. (2) MEDIUM (architecture) — a plugin folder named after a host route (`login`/`logout`/`auth`/`public`/`recovery`/`registration`/`settings`/`verification`) would **silently shadow** it (plugin routes resolve first), the one collision `findConflicts` didn't catch. Added `RESERVED_PLUGIN_IDS` (`plugin.ts`) checked in `discovery.ts` → fails boot loud, like every other conflict; documented in `docs/plugin-contract.md` Identity; `discovery.test.ts` covers it. **Deferred (reviewer-scoped, not §4):** extract `buildShellContext` out of `dashboard.ts` + thread the real `ctx.user` into the shell (kills the hardcoded "Sam Rivers" demo profile) **and** give the host its own internal route table via `matchRoute`/`isAuthorized` → **§5** (the 2nd/3rd built-in screen is the forcing function; the hardcoded user is the one user-visible §4 gap, so §5 opens with it); `/auth/complete` login-CSRF hardening + the `POST /logout` oversized-body→500 papercut → **§9** (security headers/CSRF/cookies); retarget the stale `safeUrl()` §4 reference in the contract doc → the next §4 comment-cleanup item (line 92), helper itself deferred to §5/§7 when untrusted URL data first flows. No action: forwarding the full cookie header to Kratos on re-mint (works, mild over-coupling), the deliberately-opt-in `iss`/`aud` claim checks, the `serializeCookie` length bound. typecheck + 219 units green. -- [ ] 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. +- [x] 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. → Pass over the §4 auth accretion (the §3 cleanup at line 74 stands). The §4 comments were authored dense, so the wins are targeted: tightened the verbose client module-headers — `kratos-public.ts` (dropped the "themed flow pages build on this" forward-ref, kept the loose-`ui.nodes`-types rationale), `kratos-admin.ts` (folded the admin-port note up, trimmed the `KratosError` restatement), `keto-client.ts` (dropped the caller-listing tail). Retargeted the stale `safeUrl()` ref in `docs/plugin-contract.md` (the §4 reviewer flag at line 91): the helper was deferred to §5/§7, not §4. Left intact: app.ts's per-branch *why* comments (right altitude for scanning the request flow), config.ts's dense field notes, and the §4 README **Auth, sessions & permissions** sections (the canonical design rationale, authored concise in §4). `_(planned)_` markers stay for §9 (line 133 owns dropping them). typecheck + 219 units green. - [ ] 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. ## 5. Built-in admin screens (writes go only to Keto/Kratos)