§9 test cleanup (todo §9); dropped the one genuine §9-era test overlap. app.test.ts had 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 "first-party deep link wrapped through /auth/complete; absolute target passes through as-is". The §9 test subsumes it: its middle assertion already proves an absolute /oauth2/login?login_challenge= target is handed to initBrowserFlow unchanged (the exact §6 OAuth-bounce contract, labeled as such in the test name + inline comment), plus the new host-relative-wrap + protocol-relative cases. Removed the redundant standalone §6 test, zero coverage lost. The §9 unit files (security-headers/denylist/logger/safe-url + gen-jwks rotateJwks) and the per-field config toggles (SERVICE_NAME/LOG_*/OTLP_*/REVOCATION_*/JWT_CLOCK_SKEW/ORY_TIMEOUT) are one-concern matrices following the file's per-field pattern — no fat (§3 don't-merge-across-distinct-concerns rule). Tests-only, no production code (per the §6/§7/§8 precedent, no stability reviewer). 339 → 338 units; typecheck + tests green.
This commit is contained in:
2
todo.md
2
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=<host-relative path>` via a new `loginRedirect(ctx)` (GET/HEAD only, skips `/`). `/login` bakes it into the Kratos flow: a **host-relative** target is wrapped through `<origin>/auth/complete?return_to=<path>` 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.
|
||||
|
||||
Reference in New Issue
Block a user