Merge pull request #549 from trails-cool/fix-journal-metrics-template-match
fix(journal): match metric route label against known route templates
This commit is contained in:
Ullrich Schäfer
GitHub
commit
ce00376f3b
2 changed files with 190 additions and 87 deletions
|
|
@ -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<string>();
|
||||
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());
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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/<uuid>`, 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<string, string>([
|
||||
["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<string>();
|
||||
// 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";
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue