From 5643ce257a57cde4eb59377c5c83625abd639ed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ullrich=20Sch=C3=A4fer?= Date: Fri, 19 Jun 2026 11:00:03 +0200 Subject: [PATCH] fix(journal): match metric route label against known templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the route-label cardinality fix. The first cut collapsed dynamic segments via regex (:id/:username/:provider) and capped the distinct-route set at 200 with a /other overflow. In production that cap filled almost entirely with vulnerability-scanner junk (`/.ssh/id_rsa`, `/%00.aws/credentials`, …) on a first-come-first-served basis: only 3 real templates made it in before the cap saturated, so legitimate routes hit afterward were misbucketed into /other — the metric became useless for per-route latency even though memory was bounded. Replace the regex+cap approach with explicit matching against the journal's known route templates (mirroring app/routes.ts). A `:param` segment matches any single path segment; literals are preferred over params (so /routes/new beats /routes/:id); anything matching no template collapses to /other immediately — no per-path tracking, no cap race. Cardinality is hard-bounded to (templates + 1) regardless of traffic, and scanner noise can never crowd out real routes. A co-located drift guard flattens the real route config and fails if ROUTE_TEMPLATES and app/routes.ts ever diverge. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/journal/app/lib/metrics.server.test.ts | 110 ++++++++----- apps/journal/app/lib/metrics.server.ts | 167 ++++++++++++++------ 2 files changed, 190 insertions(+), 87 deletions(-) diff --git a/apps/journal/app/lib/metrics.server.test.ts b/apps/journal/app/lib/metrics.server.test.ts index 5237d30..40681de 100644 --- a/apps/journal/app/lib/metrics.server.test.ts +++ b/apps/journal/app/lib/metrics.server.test.ts @@ -1,11 +1,8 @@ -import { describe, it, expect, beforeEach } from "vitest"; -import { normalizeRoute, _resetRouteNormalizationForTest } from "./metrics.server.ts"; +import { describe, it, expect } from "vitest"; +import { normalizeRoute, ROUTE_TEMPLATES } from "./metrics.server.ts"; +import routesConfig from "../routes.ts"; describe("normalizeRoute", () => { - beforeEach(() => { - _resetRouteNormalizationForTest(); - }); - it("passes static routes through unchanged", () => { expect(normalizeRoute("/")).toBe("/"); expect(normalizeRoute("/feed")).toBe("/feed"); @@ -15,25 +12,19 @@ describe("normalizeRoute", () => { expect(normalizeRoute("/legal/terms")).toBe("/legal/terms"); }); - it("collapses UUID segments to :id", () => { + it("maps dynamic id segments to the :id template", () => { expect( normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293"), ).toBe("/activities/:id"); expect( normalizeRoute("/routes/0257f241-d880-4ca6-b073-c8706f07a293/edit"), ).toBe("/routes/:id/edit"); - expect( - normalizeRoute("/api/v1/activities/0257F241-D880-4CA6-B073-C8706F07A293"), - ).toBe("/api/v1/activities/:id"); - }); - - it("collapses purely-numeric segments to :id", () => { expect(normalizeRoute("/api/notifications/12345/read")).toBe( "/api/notifications/:id/read", ); }); - it("collapses usernames to :username after a `users` segment", () => { + it("maps usernames to the :username template", () => { expect(normalizeRoute("/users/alice")).toBe("/users/:username"); expect(normalizeRoute("/users/bob/followers")).toBe( "/users/:username/followers", @@ -43,47 +34,84 @@ describe("normalizeRoute", () => { ); }); - it("collapses sync providers to :provider", () => { + it("maps sync providers to the :provider template", () => { expect(normalizeRoute("/api/sync/connect/wahoo")).toBe( "/api/sync/connect/:provider", ); - expect(normalizeRoute("/api/sync/callback/garmin")).toBe( - "/api/sync/callback/:provider", - ); expect( normalizeRoute( "/api/sync/push/komoot/0257f241-d880-4ca6-b073-c8706f07a293", ), - ).toBe("/api/sync/push/:provider/:id"); + ).toBe("/api/sync/push/:provider/:routeId"); }); - it("strips query strings and fragments before normalizing", () => { + it("prefers a literal template over a parametric one", () => { + // /routes/new must NOT be swallowed by /routes/:id + expect(normalizeRoute("/routes/new")).toBe("/routes/new"); + expect(normalizeRoute("/activities/new")).toBe("/activities/new"); + // a known provider literal wins over /sync/import/:provider + expect(normalizeRoute("/sync/import/komoot")).toBe("/sync/import/komoot"); + expect(normalizeRoute("/sync/import/strava")).toBe("/sync/import/:provider"); + }); + + it("collapses anything matching no template to /other", () => { + expect(normalizeRoute("/.ssh/id_rsa")).toBe("/other"); + expect(normalizeRoute("/wp-login.php")).toBe("/other"); + expect(normalizeRoute("/.aws/credentials")).toBe("/other"); + // right prefix, wrong arity also falls through + expect(normalizeRoute("/routes/abc/def/ghi")).toBe("/other"); + // a username-shaped path under an unknown collection + expect(normalizeRoute("/profile/alice")).toBe("/other"); + }); + + it("strips query strings and fragments and trailing slashes", () => { expect( - normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293?foo=bar"), + normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293?foo=1"), ).toBe("/activities/:id"); expect(normalizeRoute("/feed#section")).toBe("/feed"); + expect(normalizeRoute("/feed/")).toBe("/feed"); }); - it("preserves a dotted version segment (does not treat 2.1 as numeric)", () => { - expect(normalizeRoute("/nodeinfo/2.1")).toBe("/nodeinfo/2.1"); - }); - - it("returns the same template for two different ids (bounded cardinality)", () => { - const a = normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293"); - const b = normalizeRoute("/activities/ffffffff-d880-4ca6-b073-c8706f07a293"); - expect(a).toBe(b); - expect(a).toBe("/activities/:id"); - }); - - it("collapses unbounded junk to /other once the cap is exceeded", () => { - // Fill the tracking set with distinct unmatched paths up to the cap. - for (let i = 0; i < 200; i++) { - expect(normalizeRoute(`/scan-${i}`)).toBe(`/scan-${i}`); + it("bounds cardinality: distinct outputs are limited to templates + /other", () => { + const outputs = new Set(); + for (let i = 0; i < 500; i++) { + outputs.add(normalizeRoute(`/activities/id-${i}`)); + outputs.add(normalizeRoute(`/scanner-junk-${i}`)); + outputs.add(normalizeRoute(`/users/user${i}`)); } - // The next novel path can't be tracked, so it collapses to /other. - expect(normalizeRoute("/scan-200")).toBe("/other"); - expect(normalizeRoute("/another-novel-path")).toBe("/other"); - // Already-tracked paths are still returned verbatim. - expect(normalizeRoute("/scan-0")).toBe("/scan-0"); + // All 1500 distinct paths collapse to just 3 labels. + expect([...outputs].sort()).toEqual([ + "/activities/:id", + "/other", + "/users/:username", + ]); + }); +}); + +describe("ROUTE_TEMPLATES drift guard", () => { + // Flatten the real route config to full path templates. If a route is + // added/removed/renamed in app/routes.ts without updating ROUTE_TEMPLATES, + // this fails — keeping the metric label set honest. + type Entry = { path?: string; index?: boolean; children?: Entry[] }; + function flatten(entries: Entry[], prefix: string): string[] { + const out: string[] = []; + for (const e of entries) { + if (e.index) { + out.push(prefix === "" ? "/" : `/${prefix}`); + continue; + } + const full = prefix === "" ? e.path ?? "" : `${prefix}/${e.path ?? ""}`; + if (e.children && e.children.length > 0) { + out.push(...flatten(e.children, full)); + } else { + out.push(`/${full}`); + } + } + return out; + } + + it("matches every full path declared in app/routes.ts", () => { + const fromConfig = flatten(routesConfig as unknown as Entry[], "").sort(); + expect(fromConfig).toEqual([...ROUTE_TEMPLATES].sort()); }); }); diff --git a/apps/journal/app/lib/metrics.server.ts b/apps/journal/app/lib/metrics.server.ts index d256f07..468693a 100644 --- a/apps/journal/app/lib/metrics.server.ts +++ b/apps/journal/app/lib/metrics.server.ts @@ -55,59 +55,134 @@ export const registry = client.register; // The `route` label on httpRequestDuration MUST be bounded. prom-client // stores one label-set per distinct combination and NEVER evicts, so // feeding it raw request paths (`/activities/`, probed usernames, -// crawler junk) leaks memory linearly for the life of the process — it -// only resets on restart. `normalizeRoute` collapses dynamic path -// segments back to the `:param` templates declared in app/routes.ts so -// cardinality stays proportional to the route table, not to traffic. +// crawler junk) leaks memory linearly for the life of the process. +// +// We match each request path against the journal's known route templates +// (mirroring app/routes.ts) and label it with the matched template; a +// `:param` segment matches any single path segment. Anything matching no +// template — vulnerability scanners, typos, unmounted paths — collapses to +// a single `/other` bucket. Cardinality is therefore hard-bounded to +// (number of templates + 1), independent of traffic, and real routes can +// never be crowded out by scanner noise (the failure mode of the earlier +// first-come-first-served cap). +// +// IMPORTANT: ROUTE_TEMPLATES must stay in sync with app/routes.ts. The +// co-located test imports the route config, flattens it to full paths, and +// fails if the two ever drift — so adding a route there forces an update +// here. +const ROUTE_TEMPLATES: readonly string[] = [ + "/", + "/.well-known/trails-cool", + "/.well-known/webfinger", + "/.well-known/nodeinfo", + "/nodeinfo/2.1", + "/users/:username/inbox", + "/users/:username/outbox", + "/oauth/authorize", + "/oauth/token", + "/auth/register", + "/auth/login", + "/auth/verify", + "/auth/logout", + "/auth/accept-terms", + "/api/auth/register", + "/api/auth/login", + "/routes", + "/routes/new", + "/routes/:id", + "/routes/:id/edit", + "/api/e2e/seed", + "/api/e2e/route/:id", + "/api/e2e/komoot", + "/api/routes/:id/callback", + "/api/routes/:id/edit-in-planner", + "/api/routes/:id/gpx", + "/activities", + "/activities/new", + "/activities/:id", + "/users/:username", + "/users/:username/followers", + "/users/:username/following", + "/api/users/:username/follow", + "/api/users/:username/unfollow", + "/follows/requests", + "/follows/outgoing", + "/api/follows/:id/approve", + "/api/follows/:id/reject", + "/feed", + "/explore", + "/api/events", + "/notifications", + "/api/notifications/:id/read", + "/api/notifications/read-all", + "/settings", + "/settings/profile", + "/settings/account", + "/settings/security", + "/settings/connections", + "/settings/connections/komoot", + "/api/settings/profile", + "/api/settings/email", + "/api/settings/passkey/delete", + "/api/settings/delete-account", + "/sync/import/komoot", + "/sync/import/garmin", + "/sync/import/:provider", + "/api/sync/komoot/verify", + "/api/sync/komoot/connect", + "/api/sync/komoot/import", + "/api/sync/komoot/import-status", + "/api/sync/connect/:provider", + "/api/sync/callback/:provider", + "/api/sync/disconnect/:provider", + "/api/sync/webhook/:provider", + "/api/sync/push/:provider/:routeId", + "/privacy", + "/legal/imprint", + "/legal/terms", + "/legal/privacy", + "/api/v1/routes", + "/api/v1/routes/compute", + "/api/v1/routes/:id", + "/api/v1/activities", + "/api/v1/activities/:id", + "/api/v1/auth/devices", + "/api/v1/auth/devices/:id", + "/api/v1/uploads", +]; -const UUID_RE = - /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; +export { ROUTE_TEMPLATES }; -// A dynamic segment is keyed by the static segment that precedes it, -// mirroring the non-id `:param` slots in app/routes.ts. (`:id` params are -// all UUIDs and get caught by UUID_RE on their own.) -const DYNAMIC_AFTER = new Map([ - ["users", ":username"], // /users/:username, /api/users/:username/... - ["connect", ":provider"], // /api/sync/connect/:provider - ["callback", ":provider"], // /api/sync/callback/:provider - ["disconnect", ":provider"], // /api/sync/disconnect/:provider - ["webhook", ":provider"], // /api/sync/webhook/:provider - ["push", ":provider"], // /api/sync/push/:provider/:routeId -]); +const isParam = (seg: string): boolean => seg.startsWith(":"); -// Absolute backstop. Real route templates number well under this; once we -// have seen this many distinct normalized routes (e.g. an aggressive -// scanner hitting paths that match no `:param` rule), everything new -// collapses to a single `/other` bucket so cardinality can never run away. -const MAX_ROUTES = 200; -const seenRoutes = new Set(); +// Pre-split templates into segments, ordered most-specific first (fewest +// `:param` segments) so a literal like `/routes/new` is matched before the +// parametric `/routes/:id`. +const TEMPLATE_SEGMENTS = ROUTE_TEMPLATES.map((template) => ({ + template, + segments: template === "/" ? [] : template.slice(1).split("/"), +})).sort( + (a, b) => + a.segments.filter(isParam).length - b.segments.filter(isParam).length, +); -/** Collapse a request path's dynamic segments to a bounded route template. */ +/** Map a request path to one of the known route templates, or `/other`. */ export function normalizeRoute(pathname: string): string { const path = (pathname.split("?")[0] ?? "/").split("#")[0] || "/"; - if (path === "/") return "/"; + const segs = path.replace(/\/+$/, "").split("/").filter((s) => s !== ""); + if (segs.length === 0) return "/"; - const out: string[] = []; - for (const seg of path.split("/")) { - if (seg === "") continue; - const prev = out.length > 0 ? out[out.length - 1]! : ""; - if (UUID_RE.test(seg) || /^\d+$/.test(seg)) { - out.push(":id"); - } else if (DYNAMIC_AFTER.has(prev)) { - out.push(DYNAMIC_AFTER.get(prev)!); - } else { - out.push(seg); + for (const { template, segments } of TEMPLATE_SEGMENTS) { + if (segments.length !== segs.length) continue; + let matched = true; + for (let i = 0; i < segments.length; i++) { + const t = segments[i]!; + if (!isParam(t) && t !== segs[i]) { + matched = false; + break; + } } + if (matched) return template; } - const normalized = "/" + out.join("/"); - - if (seenRoutes.has(normalized)) return normalized; - if (seenRoutes.size >= MAX_ROUTES) return "/other"; - seenRoutes.add(normalized); - return normalized; -} - -/** Test-only: clear the distinct-route tracking set. */ -export function _resetRouteNormalizationForTest(): void { - seenRoutes.clear(); + return "/other"; }