Stability fixes: plugin-scoped contexts for owned pages, absent-href guard, checked locale mounts

2026-08-03 23:51:40 +02:00
parent 2b20497785
commit be3bc2bdbb
28 changed files with 176 additions and 58 deletions
@@ -1459,3 +1459,30 @@ test("the error pages speak the visitor's language too", async (t) => {
  assert.match(html, /<html lang="sv-SE"/);
  assert.match(html, /Sidan hittades inte/);
 });
+
+test("a plugin that owns a landing page, or short-circuits a hook, translates from its own catalog", async (t) => {
+  const dir = mkdtempSync(join(tmpdir(), "pp-i18n-plugin-"));
+  mkdirSync(join(dir, "demo", "i18n"), { recursive: true });
+  t.after(() => rmSync(dir, { force: true, recursive: true }));
+  writeFileSync(join(dir, "demo", "i18n", "en-US.ts"), 'const m = { "demo.hello": "Hello from the plugin" };\nexport default m;\n');
+
+  // Every plugin-owned render path: the public landing, the gated dashboard, and a hook short-circuit.
+  const demo: Plugin = {
+    apiVersion: "1.0.0",
+    dashboard: (ctx) => ({ html: ctx.t("demo.hello") }),
+    home: (ctx) => ({ html: ctx.t("demo.hello") }),
+    hooks: { onRequest: (ctx) => (ctx.url.pathname === "/hooked" ? { html: ctx.t("demo.hello") } : undefined) },
+    id: "demo",
+  };
+  const i18n = createI18n(await loadI18n({ pluginIds: ["demo"], pluginsDir: dir }));
+  const app = createApp({ i18n, jwks: staticJwks([ecJwk]), plugins: [demo], pluginsDir: dir });
+  await new Promise<void>((r) => app.listen(0, r));
+  t.after(() => app.close());
+  const url = `http://localhost:${(app.address() as AddressInfo).port}`;
+  const nowSec = Math.floor(Date.now() / 1000);
+  const cookie = `${SESSION_COOKIE}=${mintJwt({ email: "a@b", exp: nowSec + 600, permissions: [], sub: "u1" })}`;
+
+  assert.equal(await (await fetch(`${url}/`)).text(), "Hello from the plugin");
+  assert.equal(await (await fetch(`${url}/hooked`)).text(), "Hello from the plugin");
+  assert.equal(await (await fetch(`${url}/dashboard`, { headers: { cookie } })).text(), "Hello from the plugin");
+});
@@ -3,7 +3,7 @@ import { createServer, type IncomingMessage, type Server, type ServerResponse }
 import { dirname, join } from "node:path";
 import { fileURLToPath } from "node:url";
 import ejs from "ejs";
-import { type BuiltinRoute, matchBuiltinRoute, type RequestCsrf } from "./builtin-routes.ts";
+import { type BuiltinRoute, matchBuiltinRoute, type PluginContextFactory, type RequestCsrf } from "./builtin-routes.ts";
 import { buildPluginChrome, type PageChrome } from "../ui/chrome.ts";
 import { buildContext, type RequestContext, type User } from "./context.ts";
 import { csrfCookie, ensureCsrfToken, verifyCsrfRequest } from "../auth/csrf.ts";
@@ -132,12 +132,14 @@ export function createApp(options: AppOptions = {}): Server {
  // (rendered against its own views, native shell via ctx.chrome, with a fresh CSRF cookie for
  // any form it ships). Else the built-in intro page with prominent sign-in / register links
  // (`user` picks "go to dashboard" vs sign-in; the shell's Sign-out form needs the CSRF cookie).
-  const serveHome = async (ctx: RequestContext, csrf: RequestCsrf): Promise<RouteResult | null> => {
+  const serveHome = async (ctx: RequestContext, csrf: RequestCsrf, contextFor: PluginContextFactory): Promise<RouteResult | null> => {
    csrf.setCookie();
    if (homePlugin) {
-      const result = (await homePlugin.home(ctx)) ?? null;
-      if (anyResponseHooks) await runResponseHooks(plugins, ctx, result);
-      await sendResult(ctx.res, result, pluginViewsFor(ctx, homePlugin.id), ctx.localeHref);
+      // The plugin owns this page, so it runs on its own context — its catalog first, then core.
+      const pluginCtx = contextFor(homePlugin.id);
+      const result = (await homePlugin.home(pluginCtx)) ?? null;
+      if (anyResponseHooks) await runResponseHooks(plugins, pluginCtx, result);
+      await sendResult(ctx.res, result, pluginViewsFor(pluginCtx, homePlugin.id), pluginCtx.localeHref);
      return null;
    }
    return { data: { chrome: ctx.chrome, user: ctx.user }, view: "home" };
@@ -147,14 +149,15 @@ export function createApp(options: AppOptions = {}): Server {
  // in, remembering /dashboard as return_to. A plugin may fully own it via `dashboard` — its
  // handler renders against its own views, same path as a plugin route. Else the built-in
  // mock-data People list with the one global menu (ctx.chrome.nav) + branding from config/menu.ts.
-  const serveDashboard = async (ctx: RequestContext, csrf: RequestCsrf): Promise<RouteResult | null> => {
+  const serveDashboard = async (ctx: RequestContext, csrf: RequestCsrf, contextFor: PluginContextFactory): Promise<RouteResult | null> => {
    if (!ctx.user) return { redirect: loginRedirect(ctx), status: 303 };
    // The page carries the Sign-out form, so Set-Cookie a fresh CSRF token here when absent.
    csrf.setCookie();
    if (dashboardPlugin) {
-      const result = (await dashboardPlugin.dashboard(ctx)) ?? null;
-      if (anyResponseHooks) await runResponseHooks(plugins, ctx, result);
-      await sendResult(ctx.res, result, pluginViewsFor(ctx, dashboardPlugin.id), ctx.localeHref);
+      const pluginCtx = contextFor(dashboardPlugin.id); // as serveHome: the owner's own translator
+      const result = (await dashboardPlugin.dashboard(pluginCtx)) ?? null;
+      if (anyResponseHooks) await runResponseHooks(plugins, pluginCtx, result);
+      await sendResult(ctx.res, result, pluginViewsFor(pluginCtx, dashboardPlugin.id), pluginCtx.localeHref);
      return null;
    }
    return { data: { model: buildDashboardModel({ csrfToken: csrf.token, menu, user: ctx.user, nav: ctx.chrome.nav, t: ctx.t }) }, view: "index" };
@@ -183,6 +186,9 @@ export function createApp(options: AppOptions = {}): Server {
      // Set before any branch so every response — static/redirect/error included — inherits them
      // (writeHead merges these with its own headers; a plugin's RouteResult.headers can override).
      for (const [name, value] of secHeaderEntries) res.setHeader(name, value);
+      // The same URL renders in different languages depending on Accept-Language, so a cache in
+      // front of us must key on it — otherwise the first visitor's language is served to everyone.
+      res.setHeader("vary", "accept-language");

      if (pathname.startsWith("/public/") && (method === "GET" || method === "HEAD")) {
        // /public/<id>/… serves a plugin's public/; everything else the core public/.
@@ -261,18 +267,22 @@ export function createApp(options: AppOptions = {}): Server {
        t: pluginId === undefined ? t : i18n.translator(locale, pluginId),
      });

-      // base context (no route params yet); reused for onRequest hooks and the landing routes.
+      // base context (no route params yet); reused for the built-in routes. A plugin-owned render
+      // (a landing slot, a hook short-circuit, a plugin route) gets `contextFor(id)` instead, so its
+      // own catalog is what `ctx.t` reads.
      const ctx = buildContext(req, res, { chrome, user, ...i18nFor(), log: reqLog, verifyCsrf, ...(system ? { system } : {}) });
+      const contextFor = (pluginId: string, params?: Record<string, string>): RequestContext =>
+        buildContext(req, res, { chrome, user, ...i18nFor(pluginId), log: reqLog, ...(params ? { params } : {}), verifyCsrf, ...(system ? { system } : {}) });
      renderPage = viewsFor(ctx);

      // Plugin onRequest hooks run before routing and may short-circuit the request.
      if (anyRequestHooks) {
-        const short = await runRequestHooks(plugins, ctx);
+        const short = await runRequestHooks(plugins, contextFor);
        if (short) {
          // Set the fresh CSRF cookie like every other page-emitting path, so a form the hook
          // renders (its token is in ctx.chrome.csrfToken) has the matching double-submit cookie.
          csrfMint.setCookie();
-          await sendResult(res, short.result, pluginViewsFor(ctx, short.plugin.id), carryLocale);
+          await sendResult(res, short.result, pluginViewsFor(short.ctx, short.plugin.id), carryLocale);
          return;
        }
      }
@@ -282,7 +292,7 @@ export function createApp(options: AppOptions = {}): Server {
      // CSRF cookie is set so those forms have a valid double-submit token.
      const match = matchRoute(plugins, method, pathname);
      if (match) {
-        const routeCtx = buildContext(req, res, { chrome, user, ...i18nFor(match.plugin.id), log: reqLog, params: match.params, verifyCsrf, ...(system ? { system } : {}) });
+        const routeCtx = contextFor(match.plugin.id, match.params);
        if (!isAuthorized(match.route, routeCtx.permissions)) {
          // Anonymous → sign in (like the built-in screens' requireSession), remembering the page as
          // return_to; a signed-in user who simply lacks the permission gets the 403 page.
@@ -303,7 +313,7 @@ export function createApp(options: AppOptions = {}): Server {
      // null means the handler wrote to ctx.res itself.
      const builtin = matchBuiltinRoute(builtinRoutes, method, pathname);
      if (builtin) {
-        await sendResult(res, await builtin.handler(ctx, csrfMint), viewsFor(ctx), carryLocale);
+        await sendResult(res, await builtin.handler(ctx, csrfMint, contextFor), viewsFor(ctx), carryLocale);
        return;
      }

@@ -14,10 +14,15 @@ export interface RequestCsrf {
  token: string;
 }

+// A context scoped to a plugin: same request, but `t` reads that plugin's catalog first. The
+// landing slots run a plugin's handler, so they must hand it one of these rather than the host's
+// own context — otherwise the plugin's keys render as bare keys on the pages it owns.
+export type PluginContextFactory = (pluginId: string) => RequestContext;
+
 export interface BuiltinRoute {
  // Returns a RouteResult, or null when the handler wrote to ctx.res itself
  // (the landing slots dispatch a plugin's own result against that plugin's views).
-  handler: (ctx: RequestContext, csrf: RequestCsrf) => Promise<RouteResult | null> | RouteResult | null;
+  handler: (ctx: RequestContext, csrf: RequestCsrf, contextFor: PluginContextFactory) => Promise<RouteResult | null> | RouteResult | null;
  method: "GET" | "POST"; // a GET route also answers HEAD, like plugin routes
  path: string; // exact pathname
 }
@@ -63,7 +63,7 @@ export function checkCatalog({ baseline, baselineLocale, catalog, locale }: Pari
  }

  for (const key of Object.keys(catalog)) {
-    if (!(key in baseline)) problems.push(`unknown key "${key}" — add it to ${baselineLocale} first`);
+    if (!Object.hasOwn(baseline, key)) problems.push(`unknown key "${key}" — add it to ${baselineLocale} first`);
  }

  return problems;
@@ -21,9 +21,10 @@ const rootDir = join(dirname(fileURLToPath(import.meta.url)), "..", "..");
 export const LOCALES_DIR = join(dirname(fileURLToPath(import.meta.url)), "locales");
 export const MOUNTED_LOCALES_DIR = join(rootDir, "locales");

-// A catalog file is named for the full locale it holds — sv-SE.ts, never sv.ts. Anything else in
-// the folder is a mistake worth stopping for.
-const LOCALE_FILE = /^([a-z]{2,3}-[A-Z]{2})\.ts$/;
+// A catalog file is named for the full locale it holds — sv-SE.ts, never sv.ts — with the script
+// subtag when the language needs one (sr-Latn-RS). Anything else in the folder is a mistake worth
+// stopping for.
+const LOCALE_FILE = /^([a-z]{2,3}(?:-[A-Z][a-z]{3})?-[A-Z]{2})\.ts$/;

 export interface LoadI18nOptions {
  localesDir?: string;
@@ -46,10 +47,15 @@ export async function loadI18n(options: LoadI18nOptions = {}): Promise<LoadedI18
  const logger = options.logger ?? console;
  const errors: string[] = [];

-  const core = await readSet(localesDir, "core", errors);
+  const shipped = await readSet(localesDir, "core", errors);
+  // The SHIPPED en-US stays the baseline even when the mount replaces it — otherwise a mounted
+  // en-US would only ever be compared against itself, and a one-key rewording would boot green with
+  // the whole UI rendering bare keys.
+  const baseline = shipped.get(DEFAULT_LOCALE);
+  if (!baseline) errors.push(`core: no ${DEFAULT_LOCALE}.ts — it is the baseline every other locale is checked against`);
+  const core = new Map(shipped);
  for (const [locale, catalog] of await readSet(mountedDir, "locales", errors)) core.set(locale, catalog);
-  if (!core.has(DEFAULT_LOCALE)) errors.push(`core: no ${DEFAULT_LOCALE}.ts — it is the baseline every other locale is checked against`);
-  checkSet(core, "core", errors);
+  checkSet(core, "core", baseline, errors);
  const available = [...core.keys()].sort();

  const plugins = new Map<string, Map<string, Catalog>>();
@@ -62,7 +68,7 @@ export async function loadI18n(options: LoadI18nOptions = {}): Promise<LoadedI18
    for (const locale of set.keys()) {
      if (!available.includes(locale)) errors.push(`plugins/${id}: ${locale} is not installed — add src/i18n/locales/${locale}.ts first`);
    }
-    checkSet(set, `plugins/${id}`, errors);
+    checkSet(set, `plugins/${id}`, set.get(DEFAULT_LOCALE), errors);
    // Legitimate — the plugin's strings fall back to en-US on that page — but an operator who
    // installed a locale should hear about the gap at deploy time, not see English islands later.
    const gaps = available.filter((locale) => !set.has(locale));
@@ -103,8 +109,7 @@ async function readSet(dir: string, label: string, errors: string[]): Promise<Ma
  return set;
 }

-function checkSet(set: Map<string, Catalog>, label: string, errors: string[]): void {
-  const baseline = set.get(DEFAULT_LOCALE);
+function checkSet(set: Map<string, Catalog>, label: string, baseline: Catalog | undefined, errors: string[]): void {
  if (baseline === undefined) return; // already reported; nothing to compare against
  for (const [locale, catalog] of set) {
    for (const problem of checkCatalog({ baseline, baselineLocale: DEFAULT_LOCALE, catalog, locale })) {
@@ -59,6 +59,10 @@ test("localeHref carries the locale on host-relative links only", () => {
  assert.equal(localeHref("https://example.com/x", "sv-SE"), "https://example.com/x"); // off-site
  assert.equal(localeHref("//example.com/x", "sv-SE"), "//example.com/x"); // protocol-relative is off-site too
  assert.equal(localeHref("", "sv-SE"), "");
+  // The building blocks document href as optional (an unlinked page item, a header with no sort
+  // target) — an absent one must not throw, or the page breaks only for visitors who chose a language.
+  assert.equal(localeHref(undefined as unknown as string, "sv-SE"), undefined);
+  assert.equal(localeHref(null as unknown as string, "sv-SE"), null);
 });

 test("textDirection reads the script direction, defaulting to ltr", () => {
@@ -58,10 +58,17 @@ export function resolveLocale({ acceptLanguage, available, param }: ResolveInput
 // Carry `locale` on a host-relative link. Off-site and protocol-relative URLs are left alone — the
 // locale is ours to state, not theirs. `locale` null (the visitor never asked for one) ⇒ unchanged.
 export function localeHref(href: string, locale: string | null): string {
-  if (locale === null || href === "" || !href.startsWith("/") || href.startsWith("//")) return href;
-  const url = new URL(href, "http://localhost");
+  // An absent href is a shape the building blocks document as optional (a non-linked page item, a
+  // header with no sort target) — it must not throw here, or a page renders for every visitor
+  // except the ones who chose a language.
+  if (locale === null || !href || href.startsWith("//")) return href;
+  // A query-only href ("?" — the filter bar's documented "clear" target) keeps that shape; anything
+  // else must be host-relative, or it is someone else's URL to state a language for.
+  const queryOnly = href.startsWith("?");
+  if (!queryOnly && !href.startsWith("/")) return href;
+  const url = new URL(href, "http://localhost/");
  url.searchParams.set("locale", locale);
-  return `${url.pathname}${url.search}${url.hash}`;
+  return queryOnly ? `${url.search}${url.hash}` : `${url.pathname}${url.search}${url.hash}`;
 }

 // Both are asked for on every render (the <html> tag, the language picker) but depend only on the
@@ -45,7 +45,6 @@ const messages = {
  "common.edit": "Edit",
  "common.new": "New",
  "common.remove": "Remove",
-  "common.save": "Save",

  "consent.allow": "Allow",
  "consent.deny": "Deny",
@@ -36,7 +36,6 @@ const messages: CoreMessages = {
  "common.edit": "Redigera",
  "common.new": "Ny",
  "common.remove": "Ta bort",
-  "common.save": "Spara",

  "consent.allow": "Tillåt",
  "consent.deny": "Neka",
@@ -24,6 +24,9 @@ const pluralRules = new Map<string, Intl.PluralRules>();
 export function createTranslator({ catalogs, locale }: TranslatorOptions): Translate {
  return (key, vars) => {
    for (const catalog of catalogs) {
+      // Own keys only: `t("toString")` must fall through to the key itself like any other unknown
+      // one, not pick up Object.prototype.
+      if (!Object.hasOwn(catalog, key)) continue;
      const message = catalog[key];
      if (message === undefined) continue;
      return interpolate(isPluralMessage(message) ? selectPlural(message, locale, vars?.["count"]) : message, vars);
@@ -56,6 +59,7 @@ function rulesFor(locale: string): Intl.PluralRules {
 function interpolate(text: string, vars: TranslateVars | undefined): string {
  if (vars === undefined) return text;
  return text.replace(PLACEHOLDER, (whole, name: string) => {
+    if (!Object.hasOwn(vars, name)) return whole;
    const value = vars[name];
    return value === undefined ? whole : String(value);
  });
@@ -0,0 +1,32 @@
+import assert from "node:assert/strict";
+import { test } from "node:test";
+import { ENGLISH } from "./english.ts";
+import { localeHref } from "./locale.ts";
+import { i18nLocals, type I18nRequest } from "./view-locals.ts";
+
+const request = (overrides: Partial<I18nRequest> = {}): I18nRequest => ({
+  locale: "sv-SE",
+  localeHref: (href) => href,
+  locales: ["en-US", "sv-SE"],
+  t: ENGLISH,
+  url: new URL("http://localhost/admin/users?q=ada"),
+  ...overrides,
+});
+
+test("localeSwitch offers this same page in every installed locale, marking the current one", () => {
+  const locals = i18nLocals(request());
+  assert.deepEqual(locals.localeSwitch.map((c) => c.href), ["/admin/users?q=ada&locale=en-US", "/admin/users?q=ada&locale=sv-SE"]);
+  assert.deepEqual(locals.localeSwitch.map((c) => c.current), [false, true]);
+  assert.match(locals.localeSwitch[1]?.label ?? "", /svenska/i); // named in its own language
+});
+
+test("localeParam is the tag only when the URL asked — it is what the GET forms carry", () => {
+  // The probe asks the very function that decides, so the two can't drift apart.
+  assert.equal(i18nLocals(request()).localeParam, null); // identity localeHref ⇒ nothing was chosen
+  assert.equal(i18nLocals(request({ localeHref: (href) => localeHref(href, "sv-SE") })).localeParam, "sv-SE");
+});
+
+test("dir follows the locale's script", () => {
+  assert.equal(i18nLocals(request()).dir, "ltr");
+  assert.equal(i18nLocals(request({ locale: "ar-EG" })).dir, "rtl");
+});
@@ -24,18 +24,22 @@ test("runBootHooks runs each onBoot in order, skips plugins without one, and a t

 test("runRequestHooks short-circuits on the first RouteResult (with its plugin); later hooks skipped", async () => {
  const calls: string[] = [];
+  const scoped: string[] = []; // each hook is handed a context built for its own plugin
+  const contextFor = (pluginId: string) => { scoped.push(pluginId); return ctx; };
  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);
+  ], contextFor);

  assert.deepEqual(short?.result, { html: "stop" });
  assert.equal(short?.plugin.id, "b"); // the owning plugin (so a `view` result resolves correctly)
+  assert.equal(short?.ctx, ctx); // …and the context it ran on, for rendering its view
  assert.deepEqual(calls, ["a", "b"]);
+  assert.deepEqual(scoped, ["a", "b"]); // a plugin without the hook never builds a context

  // No hook short-circuits → null (proceed with normal routing).
-  assert.equal(await runRequestHooks([plugin("a", { onRequest: () => {} })], ctx), null);
+  assert.equal(await runRequestHooks([plugin("a", { onRequest: () => {} })], contextFor), null);
 });

 test("runResponseHooks runs every onResponse as an observer with the result; a throw fails", async () => {
@@ -13,11 +13,17 @@ export async function runBootHooks(plugins: Plugin[]): Promise<void> {

 // 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> {
+// with its owning plugin (so a `view` result resolves against that plugin's views), or null to
+// proceed. Each hook gets a context scoped to its own plugin, so `ctx.t` reads that plugin's catalog.
+export async function runRequestHooks(
+  plugins: Plugin[],
+  contextFor: (pluginId: string) => RequestContext,
+): Promise<{ ctx: RequestContext; plugin: Plugin; result: RouteResult } | null> {
  for (const plugin of plugins) {
-    const result = await plugin.hooks?.onRequest?.(ctx);
-    if (result != null) return { plugin, result };
+    if (!plugin.hooks?.onRequest) continue;
+    const ctx = contextFor(plugin.id);
+    const result = await plugin.hooks.onRequest(ctx);
+    if (result != null) return { ctx, plugin, result };
  }
  return null;
 }
@@ -41,7 +41,7 @@ export interface ShellModel {
 export function shellUser(user: User | null | undefined, t: Translate = ENGLISH): ShellUser {
  if (!user) {
    const guest = t("shell.guest");
-    return { email: "", initials: guest.slice(0, 1).toUpperCase(), name: guest };
+    return { email: "", initials: ([...guest][0] ?? "?").toUpperCase(), name: guest }; // by character: the word is translated
  }
  const local = user.email.split("@")[0] || user.email;
  return { email: user.email, initials: (local.slice(0, 2) || "U").toUpperCase(), name: local };