diff --git a/docs/plugin-contract.md b/docs/plugin-contract.md index 6639cd5..79d1618 100644 --- a/docs/plugin-contract.md +++ b/docs/plugin-contract.md @@ -134,6 +134,21 @@ export async function listShifts(ctx: RequestContext) { (see the README's *Stateless* section). The partials only need rows. - `default` status: `200` for `view`/`html`/`json`, `303` for `redirect`. +### Escaping & the trust boundary + +The host does not sandbox plugin output (crash-isolation is a non-goal), so a handler **owns the +safety of the data it renders**: + +- **Raw HTML is raw.** An `{ html }` result and the `*.html` partial fields (`cell.html`, + `error.html`, a menu `trigger.html`) are emitted **unescaped** — that's their purpose (slot + composition). Escape any untrusted content yourself before putting it there. +- **Text is auto-escaped; URLs are not scheme-checked.** Partials escape text fields (labels, + names), so those are injection-safe. But a URL field — nav `href`, a table cell link, a menu + 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.) + ## RequestContext Every handler receives one argument, the `RequestContext` (`src/context.ts`), built once per @@ -223,9 +238,14 @@ Optional, for reacting to system actions. A plugin's `hooks` may implement: | `onRequest(ctx)` | before route matching | inspect, or **short-circuit** by returning a `RouteResult` | | `onResponse(ctx, result)` | after the handler | observe/log; cannot change the response | -Hooks run with no sandbox — a throwing hook fails loud (boot for `onBoot`, the request for the -others). Keep them cheap; `onRequest` is on the hot path. This surface is intentionally small and -may grow additively within the major version. +Hooks run in **discovery order** (plugins sorted by id). `onRequest` fires on every request that +reaches routing (static assets bypass it); the **first** hook to return a `RouteResult` wins and +short-circuits — later `onRequest` hooks and the route handler are skipped, and that result renders +against its own plugin's views. `onResponse` runs for a matched route after its handler, with the +handler's result; its return value is ignored. Hooks run with no sandbox — a throwing hook fails +loud (boot for `onBoot`, the request for the others). Keep them cheap; `onRequest` is on the hot +path (the host skips the pipeline entirely when no plugin declares a hook). This surface is +intentionally small and may grow additively within the major version. ## Local dev & test story diff --git a/src/app.test.ts b/src/app.test.ts index c06319f..0fbc46b 100644 --- a/src/app.test.ts +++ b/src/app.test.ts @@ -176,6 +176,29 @@ test("mounts plugin routes: params, html/json/redirect/view results, and the per assert.equal((await fetch(url + "/demo/nope")).status, 404); }); +test("plugin hooks: onRequest can short-circuit a request and onResponse observes the handler result", async (t) => { + const seen: string[] = []; + const hooked: Plugin = { + apiVersion: "1.0.0", + hooks: { + onRequest: (c) => (c.url.pathname === "/hooked/blocked" ? { html: "blocked by hook", status: 403 } : undefined), + onResponse: (c, r) => void seen.push(`${c.url.pathname}:${r && "html" in r ? r.html : "?"}`), + }, + id: "hooked", + routes: [{ handler: () => ({ html: "handler ran" }), method: "GET", path: "/ok" }], + }; + const url = await startApp(t, [hooked]); + + // onRequest short-circuits before routing — handler never runs. + const blocked = await fetch(url + "/hooked/blocked"); + assert.equal(blocked.status, 403); + assert.match(await blocked.text(), /blocked by hook/); + + // A normal route runs the handler; onResponse observed its result. + assert.match(await (await fetch(url + "/hooked/ok")).text(), /handler ran/); + assert.ok(seen.includes("/hooked/ok:handler ran")); +}); + test("rejects unsafe static request paths (encoded traversal, NUL) with 403", async () => { assert.equal((await fetch(base + "/public/..%2f..%2fapp.ts")).status, 403); assert.equal((await fetch(base + "/public/%00")).status, 403); diff --git a/src/app.ts b/src/app.ts index 451002b..64b6568 100644 --- a/src/app.ts +++ b/src/app.ts @@ -5,6 +5,7 @@ import * as ejs from "ejs"; import { buildContext } from "./context.ts"; import { buildDashboardModel } from "./dashboard.ts"; import { PLUGINS_DIR } from "./discovery.ts"; +import { runRequestHooks, runResponseHooks } from "./hooks.ts"; import { DEFAULT_MENU, type MenuConfig } from "./menu-config.ts"; import type { Plugin, RouteResult } from "./plugin.ts"; import { allowedMethods, isAuthorized, matchRoute } from "./router.ts"; @@ -29,6 +30,9 @@ export function createApp(options: AppOptions = {}): Server { const menu = options.menu ?? DEFAULT_MENU; const plugins = options.plugins ?? []; const pluginIds = new Set(plugins.map((p) => p.id)); + // Skip the hook pipeline entirely unless a plugin declares the hook (keeps the hot path free). + const anyRequestHooks = plugins.some((p) => p.hooks?.onRequest); + const anyResponseHooks = plugins.some((p) => p.hooks?.onResponse); const pluginsDir = options.pluginsDir ?? PLUGINS_DIR; const publicDir = options.publicDir ?? join(rootDir, "public"); const viewsDir = options.viewsDir ?? join(rootDir, "views"); @@ -48,8 +52,8 @@ export function createApp(options: AppOptions = {}): Server { return createServer(async (req, res) => { try { const method = req.method ?? "GET"; - const { url } = buildContext(req, res); - const pathname = url.pathname; + const ctx = buildContext(req, res); // base context (no route params yet); reused for onRequest + const pathname = ctx.url.pathname; if (pathname.startsWith("/public/") && (method === "GET" || method === "HEAD")) { // /public//… serves a plugin's public/; everything else the core public/. @@ -58,22 +62,32 @@ export function createApp(options: AppOptions = {}): Server { return; } + // Plugin onRequest hooks run before routing and may short-circuit the request. + if (anyRequestHooks) { + const short = await runRequestHooks(plugins, ctx); + if (short) { + await sendResult(res, short.result, (view, data) => renderView(short.plugin.id, view, data)); + return; + } + } + // Plugin routes (any method): gate on the route's permission, then run the handler. const match = matchRoute(plugins, method, pathname); if (match) { - const ctx = buildContext(req, res, { params: match.params }); - if (!isAuthorized(match.route, ctx.roles)) { + const routeCtx = buildContext(req, res, { params: match.params }); + if (!isAuthorized(match.route, routeCtx.roles)) { sendHtml(res, 403, await render("403", { title: "Forbidden" })); return; } - const result = await match.route.handler(ctx); - await sendResult(res, result ?? null, (view, data) => renderView(match.plugin.id, view, data)); + const result = (await match.route.handler(routeCtx)) ?? null; + if (anyResponseHooks) await runResponseHooks(plugins, routeCtx, result); // observers; a throw → 500 + await sendResult(res, result, (view, data) => renderView(match.plugin.id, view, data)); return; } if (pathname === "/" && (method === "GET" || method === "HEAD")) { // Mock data + no roles until auth (§4) lands; branding/override come from config/menu.ts. - sendHtml(res, 200, await render("index", { model: buildDashboardModel(url, [], menu) })); + sendHtml(res, 200, await render("index", { model: buildDashboardModel(ctx.url, [], menu) })); return; } diff --git a/src/discovery.ts b/src/discovery.ts index 8285a4c..870b13e 100644 --- a/src/discovery.ts +++ b/src/discovery.ts @@ -29,7 +29,7 @@ export async function discoverPlugins(options: DiscoverOptions = {}): Promise errors.push(`plugins/${id}: ${msg}`); + const fail = (msg: string): void => void errors.push(`plugins/${id}: ${msg}`); if (!isValidPluginId(id)) { errors.push(`"${id}" is not a valid plugin folder name (lowercase a–z, digits, dashes)`); diff --git a/src/hooks.test.ts b/src/hooks.test.ts new file mode 100644 index 0000000..41b62a8 --- /dev/null +++ b/src/hooks.test.ts @@ -0,0 +1,50 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import type { RequestContext } from "./context.ts"; +import type { Plugin, PluginHooks } from "./plugin.ts"; +import { runBootHooks, runRequestHooks, runResponseHooks } from "./hooks.ts"; + +const ctx = {} as RequestContext; // the hooks only thread ctx through; they never read it + +function plugin(id: string, hooks: PluginHooks): Plugin { + return { apiVersion: "1.0.0", hooks, id }; +} + +test("runBootHooks runs each onBoot in order, skips plugins without one, and a throw aborts", async () => { + const calls: string[] = []; + await runBootHooks([ + plugin("a", { onBoot: () => void calls.push("a") }), + plugin("b", {}), // no onBoot → skipped + plugin("c", { onBoot: async () => void calls.push("c") }), + ]); + assert.deepEqual(calls, ["a", "c"]); + + await assert.rejects(runBootHooks([plugin("x", { onBoot: () => { throw new Error("boom"); } })]), /boom/); +}); + +test("runRequestHooks short-circuits on the first RouteResult (with its plugin); later hooks skipped", async () => { + const calls: string[] = []; + const short = await runRequestHooks([ + plugin("a", { onRequest: () => void calls.push("a") }), // returns void → continue + plugin("b", { onRequest: () => { calls.push("b"); return { html: "stop" }; } }), + plugin("c", { onRequest: () => void calls.push("c") }), // never reached + ], ctx); + + assert.deepEqual(short?.result, { html: "stop" }); + assert.equal(short?.plugin.id, "b"); // the owning plugin (so a `view` result resolves correctly) + assert.deepEqual(calls, ["a", "b"]); + + // No hook short-circuits → null (proceed with normal routing). + assert.equal(await runRequestHooks([plugin("a", { onRequest: () => {} })], ctx), null); +}); + +test("runResponseHooks runs every onResponse as an observer with the result; a throw fails", async () => { + const seen: unknown[] = []; + await runResponseHooks([ + plugin("a", { onResponse: (_c, r) => void seen.push(r) }), + plugin("b", {}), // no onResponse → skipped + ], ctx, { html: "ok" }); + assert.deepEqual(seen, [{ html: "ok" }]); + + await assert.rejects(runResponseHooks([plugin("x", { onResponse: () => { throw new Error("boom"); } })], ctx, null), /boom/); +}); diff --git a/src/hooks.ts b/src/hooks.ts new file mode 100644 index 0000000..6564fe2 --- /dev/null +++ b/src/hooks.ts @@ -0,0 +1,29 @@ +// Plugin lifecycle hooks (todo §2): the host invokes the optional PluginHooks a plugin may declare +// (docs/plugin-contract.md → Hooks). No sandbox — a throwing hook fails loud (boot for onBoot, the +// request for the others). Hooks run in discovery order (plugins sorted by id). app.ts skips these +// entirely when no plugin declares the hook, so the no-hooks hot path stays free. + +import type { RequestContext } from "./context.ts"; +import type { Plugin, RouteResult } from "./plugin.ts"; + +// After discovery, before the server listens. A throw aborts boot. +export async function runBootHooks(plugins: Plugin[]): Promise { + for (const plugin of plugins) await plugin.hooks?.onBoot?.(); +} + +// Before route matching. The first hook to return a RouteResult short-circuits the request — its +// result becomes the response and later hooks + the route handler are skipped. Returns that result +// with its owning plugin (so a `view` result resolves against that plugin's views), or null to proceed. +export async function runRequestHooks(plugins: Plugin[], ctx: RequestContext): Promise<{ plugin: Plugin; result: RouteResult } | null> { + for (const plugin of plugins) { + const result = await plugin.hooks?.onRequest?.(ctx); + if (result != null) return { plugin, result }; + } + return null; +} + +// After a route handler produces its result. Observers only — the return value is ignored, so a +// hook cannot change the response; a throw fails the request. +export async function runResponseHooks(plugins: Plugin[], ctx: RequestContext, result: RouteResult | null): Promise { + for (const plugin of plugins) await plugin.hooks?.onResponse?.(ctx, result); +} diff --git a/src/server.ts b/src/server.ts index d603b43..4d88587 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1,6 +1,7 @@ import { createApp } from "./app.ts"; import { loadConfig } from "./config.ts"; import { discoverPlugins } from "./discovery.ts"; +import { runBootHooks } from "./hooks.ts"; import { loadMenuConfig } from "./menu-config.ts"; const config = loadConfig(); // validates the env (incl. enforced secrets) — fails loud at boot @@ -8,6 +9,7 @@ const menu = await loadMenuConfig(); // config/menu.ts override + branding — f const plugins = await discoverPlugins(); // scans plugins/, validates — fails loud on a bad plugin console.log(`Discovered ${plugins.length} plugin(s)${plugins.length ? `: ${plugins.map((p) => p.id).join(", ")}` : ""}`); +await runBootHooks(plugins); // plugin onBoot — after discovery, before listen; a throw aborts boot const server = createApp({ cache: config.cacheTemplates, menu, plugins }).listen(config.port, () => { console.log(`Listening on http://localhost:${config.port}`); diff --git a/todo.md b/todo.md index 7a667a2..59c855b 100644 --- a/todo.md +++ b/todo.md @@ -52,7 +52,7 @@ everything via Docker. - [x] Per-plugin static serving: `plugins//public/` → `/public//`. → `routePublic` (pure, in `src/static.ts`), wired into `app.ts`'s existing `/public/` branch. A request `/public/` whose leading segment names a discovered plugin serves from `plugins//public/`; anything else (e.g. `css/styles.css`) stays on the core `public/`. Disambiguates by the discovered plugin-id set, so only mounted plugins expose assets and core paths are unaffected; plugin ids are URL-safe so the raw segment compares directly (no decode needed). Reuses `serveStatic` unchanged, so the sub-path keeps its decode + traversal/control-char guard (encoded `..` ⇒ 403) and HEAD support; a missing `public/` or file ⇒ 404. Tests-first: a `routePublic` unit (plugin/core split, nested asset, bare `/public/`) + the `app.test.ts` plugin integration now serves a real `demo/public/app.css` (200 + `text/css`) and still 403s a traversal; typecheck + 103 units green. `config/menu.ts` central override is the next §2 item. - [x] `config/menu.ts` central override: reorder/rename/hide/group + branding (app name, logo, default theme). → `src/menu-config.ts` (`MenuConfig`/`Branding`/`MenuConfigInput`, `defineMenu()` identity helper, `DEFAULT_MENU`, `loadMenuConfig()`) + the operator file `config/menu.ts`. The override is `composeNav`'s existing `NavOverride` (reorder/rename/group/hide by node id, applied before the per-user filter); branding = `{ name, logo?, sub?, theme? }`. `loadMenuConfig` (imperative shell) dynamically imports `config/menu.ts` if present, validates the authored shape fail-loud (branding field types + `theme` enum, override `hide`/`order` string-arrays / `groups` array / `rename` object), merges branding over defaults; **absent file ⇒ `DEFAULT_MENU`** (clean clone). Wired: `server.ts` loads it at boot → `createApp({ menu })` → `buildDashboardModel(url, roles, menu)` feeds `menu.override` into `composeNav` and `menu.branding` (name/sub) into the shell brand. `config/menu.ts` ships defaults matching prior behaviour (name "Plainpages"/sub "Console", empty override), so a clean clone is unchanged. Added `config` to tsconfig `include` so the authored file is type-checked (Dockerfile `COPY . .` already bakes it). Tests-first: `menu-config.test.ts` (absent⇒defaults / read+merge / malformed⇒throws) + a `dashboard.test.ts` case asserting rename+hide+branding take effect; typecheck (incl. `config/`) + 107 units green; smoke-loaded the real file at boot. **Rendering branding (logo, default theme) into the app shell is the next §2 item.** - [x] Wire branding into the app shell. → Completes the §2 branding chain (name/sub already flowed). `shell.ejs` now renders `brand.logo` as `` when set, else the default `#i-box` brand-mark; the `theme` local (already forwarded to the theme-switch) is now supplied. `buildDashboardModel` puts `menu.branding.logo` into `shell.brand` and `menu.branding.theme` into `shell.theme` (both omitted when unset, so a clean clone is unchanged → brand-mark + auto theme); `views/index.ejs` forwards `theme` to the shell. Added a `.brand-logo` CSS rule (22px, matches `.brand-mark` sizing). Tests-first: `shell.test.ts` (logo replaces the mark + default theme checked; no-logo ⇒ mark + auto) + extended `dashboard.test.ts` (logo→brand, theme→shell.theme) + an `app.test.ts` integration rendering `createApp({ menu })` end-to-end (logo `` + `theme-dark` checked on `/`). Default-app shell rendering is byte-equivalent, so the visual E2E is unaffected; typecheck + 109 units green. The §2 plugin host is feature-complete (remaining §2 items are the project-wide review + comment/test cleanup). -- [ ] Run the architecture _and_ the stability reviewer agents on the _whole_ project, not just the latest changes, and address their issues. +- [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 all of `src/`, `views/`, `config/`, Docker/tsconfig. Verdict: architecture sound + disciplined, no crash/security defect in the current path (fail-loud, traversal guards, JWT/cookie defenses all confirmed). **Fixed now:** (1) HIGH — `PluginHooks` was typed+documented but never invoked; wired it (`src/hooks.ts`: `runBootHooks`/`runRequestHooks`/`runResponseHooks`) — `server.ts` runs `onBoot` after discovery before listen, `app.ts` runs `onRequest` (before routing, first non-void short-circuits, renders against its plugin) + `onResponse` (after handler, observer, throw→500); skipped entirely when no plugin declares a hook (hot path free); `hooks.test.ts` + an `app.test.ts` integration. (2) `discovery.ts` `fail` helper retyped `: void`. (3) Documented the template trust boundary in `docs/plugin-contract.md` (raw `html`/`*.html` fields; URL sinks escaped but not scheme-checked) + tightened the Hooks prose to the wired semantics. **Deferred (reviewer-scoped, not §2):** extract a shared `buildShellContext` out of `dashboard.ts` and route the built-in screens through `matchRoute`/`isAuthorized` → §5 (premature at one call site); a `safeUrl()` helper for href sinks → §4 (no untrusted URLs until upstream data flows); doc/type-duplication + non-local `§N` refs → the §2 comment-cleanup item; HEAD-render cost + dev empty-secret fallback → negligible. typecheck + 113 units green; boot smoke-tested. - [ ] 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. - [ ] 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.