Answer stability round 1: 2 🔴, 3 🟡 (0 misread); refuse a trailing-slash range, full plugin route, merged tests, compose passes TRUSTED_PROXIES

src/config.ts

@@ -185,7 +185,7 @@ export function loadConfig(env: Env = process.env): Config {
     ketoWriteUrl: readUrl(env, "KETO_WRITE_URL", "http://keto:4467"),
     kratosAdminUrl: readUrl(env, "KRATOS_ADMIN_URL", "http://kratos:4434"),
     kratosPublicUrl: readUrl(env, "KRATOS_PUBLIC_URL", "http://kratos:4433"),
-    // Console-only by default; OTLP_ENDPOINT exports logs + per-request spans to an OpenTelemetry Collector.
+    // Console-only unless OTLP_ENDPOINT names a collector.
     logFormat: readEnum(env, "LOG_FORMAT", ["json", "text"] as const, "text"),
     logLevel: readEnum(env, "LOG_LEVEL", LOG_LEVELS, "info"),
     oryTimeoutSec: readPosInt(env, "ORY_TIMEOUT_SEC", 5),

src/http/app-request-log.test.ts

@@ -2,6 +2,7 @@ import assert from "node:assert/strict";
 import type { AddressInfo } from "node:net";
 import { test, type TestContext } from "node:test";
 import { createLogger } from "../logger.ts";
+import type { Plugin } from "../plugin-host/plugin.ts";
 import { createApp } from "./app.ts";
 
 // The `request` line, parsed, once the server has logged it (it logs on the response's close).
@@ -14,10 +15,10 @@ async function requestLineOf(lines: string[]): Promise<Record<string, unknown>>
   throw new Error("no request line was logged");
 }
 
-async function serve(t: TestContext, trustedProxies: string[]): Promise<{ lines: string[]; url: string }> {
+async function serve(t: TestContext, trustedProxies: string[], plugins: Plugin[] = []): Promise<{ lines: string[]; url: string }> {
   const lines: string[] = [];
   const log = createLogger({ format: "json", level: "info", stderr: () => {}, stdout: (l: string) => lines.push(l) });
-  const app = createApp({ log, trustedProxies });
+  const app = createApp({ log, plugins, trustedProxies });
   await new Promise<void>((r) => app.listen(0, r));
   t.after(() => app.close());
   return { lines, url: `http://localhost:${(app.address() as AddressInfo).port}` };
@@ -35,9 +36,25 @@ test("the request line names the caller, the site and the matched route in OpenT
   assert.equal(line["server.address"], "localhost");
   assert.equal(line["user_agent.original"], "TV/1.0");
   assert.match(String(line["client.address"]), /^(127\.0\.0\.1|::1)$/); // the forged header is not believed
+  assert.equal(line["service.name"], "plainpages");
+  assert.ok(line["requestId"], "carries a requestId for log↔trace correlation");
   for (const old of ["method", "path", "status", "ms"]) assert.equal(line[old], undefined, old);
 });
 
+test("http.route is the full pattern, plugin prefix included, so two plugins' routes never merge", async (t) => {
+  const plugin: Plugin = {
+    apiVersion: "1.0.0",
+    id: "demo",
+    routes: [{ handler: () => ({ json: {} }), method: "GET", path: "/items/:id", public: true }],
+  };
+  const { lines, url } = await serve(t, [], [plugin]);
+  await (await fetch(`${url}/demo/items/7`)).text();
+  const line = await requestLineOf(lines);
+
+  assert.equal(line["http.route"], "/demo/items/:id");
+  assert.equal(line["url.path"], "/demo/items/7");
+});
+
 test("a listed proxy lets X-Forwarded-For name the caller; an unmatched path carries no route", async (t) => {
   const { lines, url } = await serve(t, ["127.0.0.0/8", "::1"]);
   await (await fetch(`${url}/no-such-page`, { headers: { "x-forwarded-for": "198.51.100.7" } })).text();

src/http/app.test.ts

@@ -115,28 +115,6 @@ test("renders branding from the menu config into the shell: logo + default theme
   assert.match(html, /id="theme-dark"\s+checked/); // config default theme reaches the switch
 });
 
-test("emits a structured access-log line per request (the injected logger)", async (t) => {
-  const lines: string[] = [];
-  const app = createApp({ log: createLogger({ format: "json", level: "info", stderr: () => {}, stdout: (m) => lines.push(m) }) });
-  await new Promise<void>((r) => app.listen(0, r));
-  t.after(() => app.close());
-  const res = await fetch(`http://localhost:${(app.address() as AddressInfo).port}/?q=zz`); // the public "/" — no auth
-  assert.equal(res.status, 200);
-  await res.text(); // consume the body so the connection closes (the access line emits on close)
-
-  // The line is emitted on connection close (after the body is sent) — poll briefly for it.
-  let line: string | undefined;
-  for (let i = 0; i < 50 && !line; i++) {
-    line = lines.find((l) => l.includes('"msg":"request"'));
-    if (!line) await new Promise((r) => setTimeout(r, 10));
-  }
-  assert.ok(line, "an access line is logged for the request");
-  const rec = JSON.parse(line!);
-  assert.equal(rec["url.path"], "/"); // pathname only — the ?q=… query is dropped (may carry tokens)
-  assert.equal(rec["service.name"], "plainpages");
-  assert.ok(rec.requestId, "carries a requestId for log↔trace correlation");
-});
-
 test("ctx.log: a handler logs in the request trace, and ctx.log.fetch continues the inbound trace", async (t) => {
   const lines: string[] = [];
   const upstream: { traceparent: string | undefined; url: string }[] = [];

src/http/app.ts

@@ -24,7 +24,7 @@ import type { KratosPublic } from "../auth/kratos-public.ts";
 import { createLogger, type Log } from "../logger.ts";
 import { remintSession } from "../auth/login.ts";
 import { DEFAULT_MENU, type MenuConfig } from "../ui/menu-config.ts";
-import { declaredPermissions, type Plugin, type RouteHandler, type RouteResult } from "../plugin-host/plugin.ts";
+import { declaredPermissions, fullPath, type Plugin, type RouteHandler, type RouteResult } from "../plugin-host/plugin.ts";
 import type { PluginSettings } from "../plugin-host/settings.ts";
 import type { SystemCapabilities } from "../plugin-host/system.ts";
 import { allows, type Gate } from "../auth/gate.ts";
@@ -295,7 +295,7 @@ export function createApp(options: AppOptions = {}): Server {
 
       const match = matchRoute(plugins, method, pathname);
       if (match) {
-        reqLog.context["http.route"] = match.route.path;
+        reqLog.context["http.route"] = fullPath(match.plugin.id, match.route.path);
         const routeCtx = contextFor(match.plugin.id, match.params);
         if (!allows(match.route, routeCtx.user)) {
           await refuse(match.route, routeCtx);

src/http/request-fields.test.ts

@@ -26,8 +26,22 @@ test("clientAddress: a malformed hop or a mapped IPv4 peer never yields garbage"
   assert.equal(clientAddress("fd00::1", "2001:db8::7", proxies), "2001:db8::7");
 });
 
+test("parseTrustedProxies: a range covers exactly its prefix, and the extremes are legal", () => {
+  const one = parseTrustedProxies(["10.1.2.3/32", "fd00::7/128", "192.168.0.0/16"]);
+  assert.equal(clientAddress("10.1.2.3", "198.51.100.1", one), "198.51.100.1");
+  assert.equal(clientAddress("10.1.2.4", "198.51.100.1", one), "10.1.2.4"); // /32 is one address
+  assert.equal(clientAddress("fd00::8", "2001:db8::7", one), "fd00::8"); // /128 is one address
+  assert.equal(clientAddress("192.168.255.1", "198.51.100.1", one), "198.51.100.1");
+  assert.equal(clientAddress("192.169.0.1", "198.51.100.1", one), "192.169.0.1");
+  assert.equal(clientAddress("203.0.113.9", "198.51.100.1", parseTrustedProxies(["0.0.0.0/0"])), "198.51.100.1"); // /0 is a choice, not a typo
+});
+
+test("clientAddress: a malformed hop left of a trusted one is not believed over the peer", () => {
+  assert.equal(clientAddress("10.0.0.1", "garbage, 10.0.0.2", proxies), "10.0.0.1");
+});
+
 test("parseTrustedProxies: refuses an entry that is not an IP or a CIDR range", () => {
-  for (const bad of ["proxy.internal", "10.0.0.0/33", "10.0.0.0/x", "::1/129", ""]) {
+  for (const bad of ["proxy.internal", "10.0.0.0/33", "10.0.0.0/x", "::1/129", "", "10.0.0.1/", "10.0.0.0/ ", "fd00::1/", "10.0.0.0//8", "10.0.0.0/8/8", "10.0.0.0/-1", "10.0.0.0/1e1"]) {
     assert.throws(() => parseTrustedProxies([bad]), /TRUSTED_PROXIES/, bad);
   }
 });

src/http/request-fields.ts

@@ -3,8 +3,8 @@ import { BlockList, isIP } from "node:net";
 
 export type TrustedProxies = BlockList;
 
-// Each entry is an IP address or a CIDR range (`10.0.0.0/8`). Fails loud: a typo here would
-// silently log a proxy's address for every caller, or trust a stranger's header.
+// Each entry is an IP address or a CIDR range (`10.0.0.0/8`). A bad entry refuses the boot: a
+// misread one could trust every caller's X-Forwarded-For.
 export function parseTrustedProxies(entries: readonly string[] = []): TrustedProxies {
   const list = new BlockList();
   for (const entry of entries) {
@@ -12,7 +12,9 @@ export function parseTrustedProxies(entries: readonly string[] = []): TrustedPro
     const family = isIP(address);
     const length = prefix === undefined ? undefined : Number(prefix);
     const maxLength = family === 4 ? 32 : 128;
-    if (family === 0 || rest.length > 0 || (length !== undefined && !(Number.isInteger(length) && length >= 0 && length <= maxLength))) {
+    // `Number("")` is 0, so a trailing slash must be refused before the range check sees it.
+    const badPrefix = prefix !== undefined && !(/^\d{1,3}$/.test(prefix) && length! <= maxLength);
+    if (family === 0 || rest.length > 0 || badPrefix) {
       throw new Error(`config: TRUSTED_PROXIES entry "${entry}" is not an IP address or CIDR range`);
     }
     const kind = family === 4 ? "ipv4" : "ipv6";

src/http/request-tracking.ts

@@ -4,7 +4,8 @@ import { type Log, requestLogger, runWithLog } from "../logger.ts";
 import { requestFields, type TrustedProxies } from "./request-fields.ts";
 
 // Gives one request its own logger and span, runs `handle` under it, and writes the `request` line
-// when the response closes. The handler adds `http.route` to the logger's context once it knows it.
+// when the response closes. The handler adds `http.route` to the logger's context once routing has
+// matched.
 export function trackRequest(
   log: Log,
   trusted: TrustedProxies,
@@ -12,8 +13,6 @@ export function trackRequest(
   res: ServerResponse,
   handle: (req: IncomingMessage, res: ServerResponse, reqLog: Log) => Promise<void>,
 ): void {
-  // "close" (not "finish") fires on both a completed response and a premature disconnect, so an
-  // aborted request is still logged and its span flushed.
   const reqLog = requestLogger(log, {
     attributes: requestFields(req, trusted),
     requestId: randomUUID(),
@@ -25,10 +24,12 @@ export function trackRequest(
   let settled = false;
   let closed = false;
   const finalize = (): void => { if (settled && closed) void reqLog.end().catch(() => {}); };
+  // "close" (not "finish") fires on both a completed response and a premature disconnect, so an
+  // aborted request is still logged and its span flushed.
   res.on("close", () => {
     closed = true;
     try {
-      // The context is what the span exports, so the status goes there; the span's duration is the latency.
+      // The context is what the span exports, so the status goes there.
       reqLog.context["http.response.status_code"] = res.statusCode;
       reqLog.info("request");
     } catch { /* never let logging crash a served request */ }