From c28b3056caddca6f3603c69483a453d0eaad880a Mon Sep 17 00:00:00 2001 From: lilleman Date: Sat, 20 Jun 2026 16:45:58 +0200 Subject: [PATCH] =?UTF-8?q?=C2=A79=20comment-density=20pass=20over=20the?= =?UTF-8?q?=20=C2=A79=20accretion;=20two=20targeted=20dedupes=20(the=20res?= =?UTF-8?q?t=20authored=20dense,=20per=20the=20=C2=A76/=C2=A77/=C2=A78=20p?= =?UTF-8?q?recedent).=20(1)=20logger.ts:=20the=20requestStore=20declaratio?= =?UTF-8?q?n=20comment=20restated=20the=20file=20header=20(ambient=20per-r?= =?UTF-8?q?equest=20Log=20=E2=86=92=20deep=20modules=20join=20the=20trace?= =?UTF-8?q?=20without=20threading)=20and=20the=20accessor=20docs=20below?= =?UTF-8?q?=20it=20=E2=80=94=20collapsed=20to=20a=20one-line=20label.=20(2?= =?UTF-8?q?)=20security-headers.ts:=20dropped=20the=20redundant=20'applied?= =?UTF-8?q?=20to=20every=20response'=20note=20above=20securityHeaders()=20?= =?UTF-8?q?(the=20file=20header=20+=20function=20name=20already=20say=20it?= =?UTF-8?q?).=20No=20stale=20forward-refs=20remain;=20README=20=C2=A79=20s?= =?UTF-8?q?ections=20left=20untouched=20(concise,=20=5F(planned)=5F=20alre?= =?UTF-8?q?ady=20swept=20in=20=C2=A78).=20Docs/comments-only;=20typecheck?= =?UTF-8?q?=20+=20339=20units=20green.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/logger.ts | 3 +-- src/security-headers.ts | 1 - todo.md | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/logger.ts b/src/logger.ts index 9312239..0e28cbe 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -37,8 +37,7 @@ export function createLogger(opts: LoggerOptions = {}): Log { }); } -// The current request's Log, made ambient so deep modules (the Ory clients via tracedFetch, login, -// jwks) join its trace + correlation without threading a logger through every signature. +// The ambient per-request Log (see the header); set by runWithLog, read by currentLog/tracedFetch. const requestStore = new AsyncLocalStorage(); // Run `fn` with `log` as the ambient request logger (app.ts wraps each request). currentLog() reads diff --git a/src/security-headers.ts b/src/security-headers.ts index e00e185..e16b160 100644 --- a/src/security-headers.ts +++ b/src/security-headers.ts @@ -24,7 +24,6 @@ export interface SecurityHeaderOptions { secure?: boolean; // https deployment (mirrors SECURE_COOKIES) → also emit HSTS } -// The header set applied to every response. export function securityHeaders(options: SecurityHeaderOptions = {}): Record { const headers: Record = { "content-security-policy": CSP, diff --git a/todo.md b/todo.md index 3ce7d27..14363bb 100644 --- a/todo.md +++ b/todo.md @@ -132,7 +132,7 @@ everything via Docker. - [x] JWT signing-key rotation runbook. → Expanded the README **JWT signing key & rotation** section from a 3-line note into an operational runbook, and closed the tooling gap that made the documented steps unrunnable: the old "prepend a key / drop it later" required hand-editing a JSON file holding a private signing key. Tests-first: new pure `rotateJwks(current, {prune})` in `gen-jwks.ts` — `--prepend` puts a fresh ES256 key first (Kratos signs with `keys[0]`, the old keys still verify in-flight JWTs) and keeps the rest in order; `--prune` keeps only the newest (drop superseded post-TTL). CLI reads the existing set from a path arg and writes the new set to stdout (header documents the temp-file redirect so the shell's `>` can't truncate the input). `gen-jwks.test.ts` covers prepend (length+1, fresh kid first, old set preserved) + prune (→ 1 key). Runbook documents: the two-sided install (Kratos signer env/mount + web `JWKS_URL`, `file://` hot-reloads / `base64://` immutable), why it's zero-downtime (sign-with-first + verify-by-`kid`), the **scheduled** path (prepend → `restart kratos` → verify new kid → wait ~12 min = 10m TTL + skew → prune; rollback before prune) and the **emergency** path (replace with a single key → every leaked-key token fails signature → forced re-login; the §9 denylist is moot since the signature is already invalid). Verified the CLI live against the committed dev JWKS (bare→1 key, `--prepend`→2 with the old kid second, `--prune`→1). Docs/CLI-only behaviour, covered by units (per the §9 precedent, no new browser E2E). README Status + Layout updated. typecheck + **335 units** green (333 → 335). - [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. -- [ ] 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 §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. ## 10. User added stuff