From df53106a5ad34f917b6c503d2397d4ec84c3fa69 Mon Sep 17 00:00:00 2001 From: lilleman Date: Sat, 20 Jun 2026 16:50:26 +0200 Subject: [PATCH] =?UTF-8?q?=C2=A79=20test=20cleanup=20(todo=20=C2=A79);=20?= =?UTF-8?q?dropped=20the=20one=20genuine=20=C2=A79-era=20test=20overlap.?= =?UTF-8?q?=20app.test.ts=20had=20two=20`/login=3Freturn=5Fto=3D`=20tests?= =?UTF-8?q?=20for=20the=20same=20surface=20=E2=80=94=20the=20=C2=A76=20"ba?= =?UTF-8?q?kes=20the=20return=20target=20into=20the=20Kratos=20flow=20init?= =?UTF-8?q?=20(OAuth=20bounce)"=20and=20the=20=C2=A79=20"first-party=20dee?= =?UTF-8?q?p=20link=20wrapped=20through=20/auth/complete;=20absolute=20tar?= =?UTF-8?q?get=20passes=20through=20as-is".=20The=20=C2=A79=20test=20subsu?= =?UTF-8?q?mes=20it:=20its=20middle=20assertion=20already=20proves=20an=20?= =?UTF-8?q?absolute=20`/oauth2/login=3Flogin=5Fchallenge=3D`=20target=20is?= =?UTF-8?q?=20handed=20to=20initBrowserFlow=20unchanged=20(the=20exact=20?= =?UTF-8?q?=C2=A76=20OAuth-bounce=20contract,=20labeled=20as=20such=20in?= =?UTF-8?q?=20the=20test=20name=20+=20inline=20comment),=20plus=20the=20ne?= =?UTF-8?q?w=20host-relative-wrap=20+=20protocol-relative=20cases.=20Remov?= =?UTF-8?q?ed=20the=20redundant=20standalone=20=C2=A76=20test,=20zero=20co?= =?UTF-8?q?verage=20lost.=20The=20=C2=A79=20unit=20files=20(security-heade?= =?UTF-8?q?rs/denylist/logger/safe-url=20+=20gen-jwks=20rotateJwks)=20and?= =?UTF-8?q?=20the=20per-field=20config=20toggles=20(SERVICE=5FNAME/LOG=5F*?= =?UTF-8?q?/OTLP=5F*/REVOCATION=5F*/JWT=5FCLOCK=5FSKEW/ORY=5FTIMEOUT)=20ar?= =?UTF-8?q?e=20one-concern=20matrices=20following=20the=20file's=20per-fie?= =?UTF-8?q?ld=20pattern=20=E2=80=94=20no=20fat=20(=C2=A73=20don't-merge-ac?= =?UTF-8?q?ross-distinct-concerns=20rule).=20Tests-only,=20no=20production?= =?UTF-8?q?=20code=20(per=20the=20=C2=A76/=C2=A77/=C2=A78=20precedent,=20n?= =?UTF-8?q?o=20stability=20reviewer).=20339=20=E2=86=92=20338=20units;=20t?= =?UTF-8?q?ypecheck=20+=20tests=20green.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/app.test.ts | 14 -------------- todo.md | 2 +- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/src/app.test.ts b/src/app.test.ts index 78f5e1e..91c025c 100644 --- a/src/app.test.ts +++ b/src/app.test.ts @@ -828,20 +828,6 @@ test("OAuth2 login challenge (/oauth2/login): a Kratos session accepts via Hydra assert.match(decodeURIComponent(loc.split("return_to=")[1]!), /^http:\/\/[^/]+\/oauth2\/login\?login_challenge=chal1$/); }); -test("/login?return_to=… bakes the return target into the Kratos flow init (§6 OAuth bounce)", async (t) => { - let seenReturnTo: string | undefined; - const kratos: KratosPublic = { - ...mockKratos(async () => { throw new Error("unused"); }), - initBrowserFlow: async (_t, opts) => { seenReturnTo = opts?.returnTo; return { flow: { id: "f1", ui: { action: "", method: "post", nodes: [] } }, setCookie: [] }; }, - }; - const app = createApp({ kratos }); - await new Promise((r) => app.listen(0, r)); - t.after(() => app.close()); - const returnTo = "http://127.0.0.1:3000/oauth2/login?login_challenge=c"; - await fetch(`http://localhost:${(app.address() as AddressInfo).port}/login?return_to=${encodeURIComponent(returnTo)}`, { redirect: "manual" }); - assert.equal(seenReturnTo, returnTo); -}); - test("OAuth2 consent challenge (/oauth2/consent): skip auto-accepts; a third-party shows the screen; allow/deny POST; CSRF-guarded; missing challenge", async (t) => { const csrfSecret = "consent-secret"; let granted: { grant_scope?: string[]; session?: unknown } | undefined; diff --git a/todo.md b/todo.md index 14363bb..a97540b 100644 --- a/todo.md +++ b/todo.md @@ -133,7 +133,7 @@ everything via Docker. - [x] Refresh README `Layout` + drop `_(planned)_` markers as pieces land. → The `_(planned)_` markers were already dropped as each piece landed (swept the whole README — none remain, and the **Status** paragraph already reflects the built state). Refreshed the `Layout` block, which had drifted: added the three source modules it was missing — `fetch-timeout.ts` (`withTimeout()`, the Ory outbound-call deadline wrapper, §8), `guards.ts` (`requireSession()/can()/check()`, in-handler authz + `GuardError`, §4), `hooks.ts` (`runBoot/Request/ResponseHooks()`, plugin lifecycle, §2) — plus the `scripts/ci.sh` CI gate (§8). Cross-checked mechanically: every non-test `src/*.ts` and every top-level dir (bar `node_modules`) now has a line; `public/`/`plugins/`/`examples/` descriptions still match their contents. Docs-only. - [x] Run the architecture and the product reviewer agents on the _whole_ project, not just the latest changes, and address their issues. → Ran both (systems-architect + product-owner) on the whole project: **no Critical/High** — both report a converged, fit-for-purpose scaffold. **Fixed now (tests-first), the in-scope §9 customer-facing/security items:** (1) **`return_to` deep-link preservation** (product 🟡, explicitly deferred *to* §9) — a gated request hit while signed out (the plugin-route gate in `app.ts`, `requireSession`, `requireAdmin`) now bounces to `/login?return_to=` via a new `loginRedirect(ctx)` (GET/HEAD only, skips `/`). `/login` bakes it into the Kratos flow: a **host-relative** target is wrapped through `/auth/complete?return_to=` so the session JWT is minted *before* the user lands there, while an **absolute** target (the §6 OAuth2 login challenge) passes to Kratos unchanged (Kratos allow-lists it). `/auth/complete` redirects to the requested page; the OAuth2 path is untouched. (2) **`safeUrl()` + `localPath()`** — the doc-promised §9 URL-safety helpers, new pure `src/safe-url.ts`: `safeUrl` sanitises an untrusted href/src to relative-or-`http(s)` (else `"#"`), exported to plugins via `plugin-api.ts` and documented in the contract with a usage snippet (closing the "planned for §9" vapor pointer); `localPath` is the host-relative redirect-allowlist guard backing `return_to` (rejects `//evil.com`/`https:`/control chars, double-checked at both `/login` and `/auth/complete` so a crafted `?return_to=` can't open-redirect). (3) **Honest 503 on Ory-unreachable sign-in** (product 🟡) — the auth-flow block renders a new `views/503.ejs` ("sign-in temporarily unavailable") on a Kratos 5xx/network throw instead of the misattributed catch-all 500; an expired-flow 4xx still restarts, other 4xx still rethrow. Tests-first throughout: `safe-url.test.ts` (scheme/obfuscation + host-relative matrix); `guards`/`admin-nav`/`app` assertions updated to the new `return_to` contract + the `/login` wrap + the 503 (incl. a network-throw case) + `/auth/complete` honour/reject; `plugin-api` export guard; a full-flow **E2E** (deep link → login → lands on the page) and the visual-suite gated-bounce assertion. Stability-reviewer on the diff: **APPROVE** (no Critical/High) — open-redirect double-layered + the OAuth2 path verified intact; addressed its one Medium (scoped the 503 catch to genuine connectivity errors — the success render moved out of the `try` so a template bug hits the 500 with a stack, not a misleading 503) and its NBSP-strip-set note (documenting test). README (Production return_to/503 note, Layout) + `docs/plugin-contract.md` (return_to framing, `safeUrl` usage) updated. typecheck + **339 units** + the full `scripts/ci.sh` gate green (visual 9 · auth 1 · oauth 2 · full 7). **Deferred, with justification:** the **`app.ts` route-table** refactor (arch Medium — fold the admin/oauth if-ladder into a `{method,prefix,handler}` table, fixing the built-in-route 405/`allowedMethods` blindness + the dep-absent-404 honesty) → the architect explicitly recommends a **standalone change** (not bundled into review-fixes) and it's a **§10 prerequisite**; the **mock dashboard + dead `#` affordances** (arch/product) → **§10 line 139** (gate `/` + make it plugin-replaceable); **public-page blessing/docs** (product 🟢) → **§10 line 140**; success-flash after writes (stateless, known) and plugin-upstream-config visibility (no change recommended) → left as-is. - [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 §9 accretion (`security-headers.ts`, `denylist.ts`, `logger.ts`+`tracedFetch`/`ctx.log`, `gen-jwks.ts` `rotateJwks`, `safe-url.ts`/`return_to`/503, and their README sections — Production, Observability, Instant revoke, JWT rotation runbook). As the §6/§7/§8 precedent found, these were authored dense — each comment carries a real non-obvious "why" (the CSP `no-unsafe-inline` warning at the array a maintainer would edit, the denylist single-instance/group-lag bounds, the `end()`-coordination abort race, the `return_to` open-redirect guard) — so the wins are targeted dedupes, not a rewrite: (1) `logger.ts` — the `requestStore` declaration comment restated the file header verbatim (ambient per-request Log → deep modules join the trace without threading) *and* the accessor-function docs right below it; collapsed to a one-line label pointing at both. (2) `security-headers.ts` — dropped `// The header set applied to every response.` above `securityHeaders()` (the file header already says "set once per request … so every response carries them"; the function name is self-documenting). No stale forward-refs remain (prior passes cleared the "next §X item" markers; `(todo §N)` is the documented convention). README §9 sections were authored concise and the §8 pass already swept out the `_(planned)_` markers — left untouched. Docs/comments-only (per AGENTS.md, no stability reviewer); typecheck + 339 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. +- [x] 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. → Pass over the §9 test accretion (`security-headers`/`denylist`/`logger`/`safe-url` units, `gen-jwks` `rotateJwks`, + the §9 additions across `config`/`app`/`jwt-middleware`/`context`/`plugin-api`/`guards`). The new unit files are one-concern-per-test matrices (logger severity/level/format/trace-continuation, denylist iat-cutoff/TTL-evict, safe-url scheme/host-relative, security-headers strict-CSP) and the per-field `config` toggles (`SERVICE_NAME`/`LOG_*`/`OTLP_*`/`REVOCATION_*`/`JWT_CLOCK_SKEW`/`ORY_TIMEOUT`) follow the file's existing per-field validation pattern — no fat, and force-merging distinct fields/surfaces would only hurt readability (the §3 "don't merge across distinct concerns" rule). **One genuine §9-era overlap:** `app.test.ts` carried two `/login?return_to=…` tests for the *same* surface — the §6 "bakes the return target into the Kratos flow init (OAuth bounce)" and the §9 "a first-party deep link is wrapped through `/auth/complete`; an absolute target passes through as-is". The §9 test subsumes the §6 one: its middle assertion already proves an absolute `/oauth2/login?login_challenge=` target is passed to `initBrowserFlow` **unchanged** (the exact §6 OAuth-bounce contract — and it's labeled as such in the test name + inline comment), plus the new host-relative-wrap + protocol-relative cases the §6 test never had. Removed the now-redundant standalone §6 test (zero coverage lost). Pure test refactor, no production code (per the §6/§7/§8 precedent, no stability reviewer). 339 → 338 units; typecheck + tests green. ## 10. User added stuff - [ ] The dashboard, the first landing page after logging in, should be gated to only logged in users. It should also be replaceable fully from a plugin. It is important that the ergonomics for the plugin writer is great.