fix(security): client-vitals — whitelist route/attr на анонимном эндпоинте
POST /api/telemetry/vitals анонимный, а route/attr не сверялись со словарём: любой мог писать свободный текст в client_metrics (высокая кардинальность, инъекция текста/PII/разметки). - route: экспортировал полный словарь шаблонов из клиентского route-template.ts (KNOWN_ROUTE_TEMPLATES — канонический источник), сервер валидирует по зеркалу ALLOWED_ROUTE_TEMPLATES: нет в словаре → drop (null), событие остаётся. - attr: это web-vitals attribution target (CSS-селектор), а не enum — ограничил консервативным charset CSS-селектора; всё вне набора → drop. Клиентский self-consistency тест: templateRoute выдаёт ТОЛЬКО значения из словаря (иначе легитимные метрики отбрасывались бы). Серверные тесты: raw-путь и инъекция в route отброшены, PII/разметка в attr отброшены. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -44,6 +44,22 @@ const STATIC_ROUTES = new Set<string>([
|
||||
'/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<string> = new Set<string>([
|
||||
'/',
|
||||
'other',
|
||||
...STATIC_ROUTES,
|
||||
...ROUTE_PATTERNS.map((p) => p.template),
|
||||
]);
|
||||
|
||||
export function templateRoute(pathname: string): string {
|
||||
// Normalise a trailing slash (except root).
|
||||
const path =
|
||||
|
||||
@@ -24,6 +24,54 @@ export const ALLOWED_RATINGS = new Set<string>([
|
||||
'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<string>([
|
||||
'/',
|
||||
'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;
|
||||
|
||||
@@ -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: '<script>alert(1)</script>' }, // 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);
|
||||
|
||||
Reference in New Issue
Block a user