diff --git a/apps/client/src/lib/telemetry/route-template.test.ts b/apps/client/src/lib/telemetry/route-template.test.ts index aa798a3d..4b16883e 100644 --- a/apps/client/src/lib/telemetry/route-template.test.ts +++ b/apps/client/src/lib/telemetry/route-template.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { templateRoute } from "./route-template"; +import { templateRoute, KNOWN_ROUTE_TEMPLATES } from "./route-template"; describe("templateRoute", () => { it("templates a space page path (never leaks slugs)", () => { @@ -32,4 +32,30 @@ describe("templateRoute", () => { expect(templateRoute("/weird/unknown/thing")).toBe("other"); expect(templateRoute("/s/team/p/slug/extra/segments")).toBe("other"); }); + + // The server's /api/telemetry/vitals mirror (ALLOWED_ROUTE_TEMPLATES) drops any + // route outside KNOWN_ROUTE_TEMPLATES, so templateRoute must NEVER emit a label + // that is not in that dictionary — otherwise legit client metrics get dropped. + it("only ever emits labels contained in KNOWN_ROUTE_TEMPLATES (#495)", () => { + const samples = [ + "/", + "/home", + "/settings/members", + "/settings/groups/g-1", + "/s/team", + "/s/team/trash", + "/s/team/p/slug", + "/p/slug", + "/share/abc", + "/share/abc/p/slug", + "/share/p/slug", + "/labels/urgent", + "/invites/inv-1", + "/weird/unknown/thing", // -> "other" + "/deep/unmatched/x/y/z", // -> "other" + ]; + for (const path of samples) { + expect(KNOWN_ROUTE_TEMPLATES.has(templateRoute(path))).toBe(true); + } + }); }); diff --git a/apps/client/src/lib/telemetry/route-template.ts b/apps/client/src/lib/telemetry/route-template.ts index 45fe4855..b3a29c9d 100644 --- a/apps/client/src/lib/telemetry/route-template.ts +++ b/apps/client/src/lib/telemetry/route-template.ts @@ -44,6 +44,22 @@ const STATIC_ROUTES = new Set([ '/settings/sharing', ]); +/** + * The COMPLETE, finite vocabulary `templateRoute` can ever emit: the two + * synthetic labels (`/` and `other`), the static routes, and the dynamic + * templates. Exported so the public `/api/telemetry/vitals` endpoint can reject + * any `route` outside this dictionary server-side (the endpoint is anonymous, so + * an un-checked `route` is a free-text write surface). The server keeps a mirror + * (`ALLOWED_ROUTE_TEMPLATES` in client-metrics.constants.ts) — this is the + * canonical source; keep them in lockstep. + */ +export const KNOWN_ROUTE_TEMPLATES: ReadonlySet = new Set([ + '/', + 'other', + ...STATIC_ROUTES, + ...ROUTE_PATTERNS.map((p) => p.template), +]); + export function templateRoute(pathname: string): string { // Normalise a trailing slash (except root). const path = diff --git a/apps/server/src/core/telemetry/client-metrics.constants.ts b/apps/server/src/core/telemetry/client-metrics.constants.ts index a23fd06f..a3a0e469 100644 --- a/apps/server/src/core/telemetry/client-metrics.constants.ts +++ b/apps/server/src/core/telemetry/client-metrics.constants.ts @@ -24,6 +24,54 @@ export const ALLOWED_RATINGS = new Set([ 'poor', ]); +// The ONLY route labels accepted. The endpoint is anonymous, so an un-checked +// `route` is a free-text write surface (arbitrary high-cardinality strings / +// injected text into the metrics table). The client only ever sends a label from +// a finite template dictionary (`templateRoute`), so we drop anything not in it. +// +// PARITY: this MUST mirror `KNOWN_ROUTE_TEMPLATES` in the client's +// `apps/client/src/lib/telemetry/route-template.ts` (the canonical source). A +// drift means legit client routes get dropped — keep the two in lockstep; the +// client self-consistency test asserts `templateRoute` only emits these values. +export const ALLOWED_ROUTE_TEMPLATES = new Set([ + '/', + 'other', + // Static routes. + '/home', + '/spaces', + '/favorites', + '/login', + '/forgot-password', + '/password-reset', + '/setup/register', + '/settings/account/profile', + '/settings/account/preferences', + '/settings/workspace', + '/settings/ai', + '/settings/members', + '/settings/groups', + '/settings/spaces', + '/settings/sharing', + // Dynamic templates (slugs/ids are already collapsed to `:param`). + '/share/:shareId/p/:slug', + '/share/p/:slug', + '/share/:shareId', + '/p/:slug', + '/s/:space/p/:slug', + '/s/:space/trash', + '/s/:space', + '/labels/:label', + '/invites/:invitationId', + '/settings/groups/:groupId', +]); + +// `attr` is a web-vitals attribution TARGET: a CSS-selector-ish string (an +// element path like `html>body>div#app>button.cta`), never free prose. Constrain +// it to a conservative CSS-selector charset so the anonymous endpoint cannot be +// used to write arbitrary text / PII / markup into the metrics table. A value +// containing anything outside this set is DROPPED (-> null); the event is kept. +export const ATTR_ALLOWED_CHARSET = /^[A-Za-z0-9#.\-_> :()[\]="'*+~,]+$/; + // Max events accepted per batch; the rest are ignored. export const MAX_EVENTS_PER_BATCH = 50; @@ -77,14 +125,20 @@ export function sanitizeVitalEvent( ? e.rating : null; + // route: accept ONLY a known template label (dictionary check), else drop to + // null. The length cap stays as a cheap pre-guard before the Set lookup. let route: string | null = null; if (typeof e.route === 'string' && e.route.length > 0) { - route = e.route.slice(0, MAX_ROUTE_LENGTH); + const candidate = e.route.slice(0, MAX_ROUTE_LENGTH); + route = ALLOWED_ROUTE_TEMPLATES.has(candidate) ? candidate : null; } + // attr: truncate, then accept ONLY if it is a CSS-selector-shaped string + // (charset whitelist); anything with characters outside the set is dropped. let attr: string | null = null; if (typeof e.attr === 'string' && e.attr.length > 0) { - attr = e.attr.slice(0, MAX_ATTR_LENGTH); + const candidate = e.attr.slice(0, MAX_ATTR_LENGTH); + attr = ATTR_ALLOWED_CHARSET.test(candidate) ? candidate : null; } let docSize: number | null = null; diff --git a/apps/server/src/core/telemetry/vitals.service.spec.ts b/apps/server/src/core/telemetry/vitals.service.spec.ts index 12fb78b9..62da97ee 100644 --- a/apps/server/src/core/telemetry/vitals.service.spec.ts +++ b/apps/server/src/core/telemetry/vitals.service.spec.ts @@ -90,6 +90,44 @@ describe('VitalsService.buildRows', () => { expect(rows[0].attr).toHaveLength(MAX_ATTR_LENGTH); }); + it('keeps a known route template but DROPS an unknown/free-text route (#495)', () => { + const rows = svc.buildRows( + { + events: [ + { name: 'INP', value: 1, route: '/s/:space/p/:slug' }, // known + { name: 'INP', value: 2, route: '/s/acme-corp/p/secret-slug' }, // raw path (slugs) — not a template + { name: 'INP', value: 3, route: 'DROP TABLE client_metrics;--' }, // injected free text + { name: 'INP', value: 4, route: '/home' }, // known static + ], + }, + WS, + ); + expect(rows.map((r) => r.route)).toEqual([ + '/s/:space/p/:slug', + null, // raw path dropped + null, // free text dropped + '/home', + ]); + }); + + it('DROPS an attr that is not a CSS-selector-shaped string (#495)', () => { + const rows = svc.buildRows( + { + events: [ + { name: 'INP', value: 1, attr: 'div#app>button.cta' }, // valid selector + { name: 'INP', value: 2, attr: 'user@example.com wrote a note' }, // free text / PII + { name: 'INP', value: 3, attr: '' }, // markup + ], + }, + WS, + ); + expect(rows.map((r) => r.attr)).toEqual([ + 'div#app>button.cta', + null, + null, + ]); + }); + it('caps the batch at 50 events', () => { const events = Array.from({ length: 200 }, () => ({ name: 'CLS', value: 1 })); const rows = svc.buildRows({ events }, WS);