From 368b8399bb78453260bd6dea1016aa5d468caf1b Mon Sep 17 00:00:00 2001 From: Jeff Larson Date: Sun, 26 Jul 2026 18:45:10 -0700 Subject: [PATCH] fix(ui): encode trace/span ids in cross-signal deep-links (JEF-534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit trace_id/span_id are ingested OTLP data — attacker-shapeable — but several components interpolated them raw into react-router `to=`/`navigate()` targets while the neighboring `service` was already `encodeURIComponent`-ed (TraceWaterfall's span-detail "logs for this span" link and its header "logs" link, LogView's trace/span drill-in links, MetricChart's exemplar click, and TraceList's row link via App.tsx). react-router keeps these same-origin/client-side, so no XSS and no open-redirect — worst case an id containing `?`/`#`/`&` lands on an unexpected in-app route or injects a stray query param. Pure hardening for consistency, not an active exploit. Extracted the fix into one shared `ui/src/links.ts`: `traceHref` builds `/traces/:id` (path segment `encodeURIComponent`-ed, optional span/service via URLSearchParams) and `logsHref` builds `/logs` the same way. All five call sites now import from there instead of each re-deriving its own raw-interpolated string, which also collapses three near-identical `/traces/:id?service=` builders the first pass of this fix had introduced per-file. Test: ui/src/links.test.ts pins the well-formed-hex-id shape byte-identical to the pre-fix output (no behavior change for the common case) and locks that a `&`/`#` in an id is encoded rather than landing as a stray param or route. `npm run build` (tsc + vite) and `npx vitest run` both clean (6 test files, 33 tests); `npm run lint` clean. Co-authored-by: Claude Sonnet 5 --- ui/src/App.tsx | 3 +- ui/src/components/LogView.tsx | 12 +++---- ui/src/components/MetricChart.tsx | 5 ++- ui/src/components/TraceWaterfall.tsx | 8 ++--- ui/src/links.test.ts | 51 ++++++++++++++++++++++++++++ ui/src/links.ts | 26 ++++++++++++++ 6 files changed, 89 insertions(+), 16 deletions(-) create mode 100644 ui/src/links.test.ts create mode 100644 ui/src/links.ts diff --git a/ui/src/App.tsx b/ui/src/App.tsx index 79211b1..2dd8287 100644 --- a/ui/src/App.tsx +++ b/ui/src/App.tsx @@ -18,6 +18,7 @@ import Services from "./components/Services"; import Alerts from "./components/Alerts"; import { listAlerts, listServices } from "./api"; import { useControls, rangeParams, RANGES, INTERVALS } from "./timerange"; +import { traceHref } from "./links"; const TABS: { to: string; label: string }[] = [ { to: "/traces", label: "Traces" }, @@ -57,7 +58,7 @@ function TracesRoute() { const { service } = useControls(); // Keep the focus in the URL through the drill-in so the header + tabs still show it. // A real per row (primary cell) makes rows keyboard-operable + open-in-new-tab. - return `/traces/${id}${serviceSearch(service)}`} />; + return traceHref(id, { service })} />; } function TraceRoute() { diff --git a/ui/src/components/LogView.tsx b/ui/src/components/LogView.tsx index 5cfe746..446d58e 100644 --- a/ui/src/components/LogView.tsx +++ b/ui/src/components/LogView.tsx @@ -2,6 +2,7 @@ import { Fragment, useEffect, useState } from "react"; import { Link, useSearchParams } from "react-router-dom"; import { listLogs, type LogRow } from "../api"; import { firstRunHint } from "../empty"; +import { traceHref } from "../links"; import { useControls, rangeParams } from "../timerange"; function sevClass(n: number | null): string { @@ -70,12 +71,9 @@ export default function LogView() { }); // Preserve the service focus when drilling into a trace/span from a log row. - const svc = service ? `service=${encodeURIComponent(service)}` : ""; - const traceLink = (l: LogRow) => `/traces/${l.trace_id}${svc ? `?${svc}` : ""}`; - const spanInTraceLink = (l: LogRow) => { - const parts = [l.span_id ? `span=${l.span_id}` : "", svc].filter(Boolean); - return `/traces/${l.trace_id}${parts.length ? `?${parts.join("&")}` : ""}`; - }; + const traceLink = (l: LogRow) => traceHref(l.trace_id ?? "", { service: service || undefined }); + const spanInTraceLink = (l: LogRow) => + traceHref(l.trace_id ?? "", { spanId: l.span_id, service: service || undefined }); const emptyMessage = spanFilter ? "No logs recorded for this span." @@ -172,7 +170,7 @@ export default function LogView() { {l.service ?? "—"} {l.trace_id ? ( - + {l.trace_id.slice(0, 8)} ) : ( diff --git a/ui/src/components/MetricChart.tsx b/ui/src/components/MetricChart.tsx index 3c44886..685f513 100644 --- a/ui/src/components/MetricChart.tsx +++ b/ui/src/components/MetricChart.tsx @@ -10,6 +10,7 @@ import { } from "../api"; import { formatValue } from "../format"; import { facetLabels } from "../metricLabels"; +import { traceHref } from "../links"; const RANGES: { label: string; hours: number }[] = [ { label: "1h", hours: 1 }, @@ -358,9 +359,7 @@ function FacetView({ firingFrom={firingFrom} desc={`${name} ${data.rated ? "per-second rate" : "value"}, ${rangeLabel}`} exemplars={exemplars.map((e) => ({ t: e.t, v: e.v, traceId: e.trace_id }))} - onExemplarClick={(traceId) => - navigate(`/traces/${traceId}${service ? `?service=${encodeURIComponent(service)}` : ""}`) - } + onExemplarClick={(traceId) => navigate(traceHref(traceId, { service }))} /> ); diff --git a/ui/src/components/TraceWaterfall.tsx b/ui/src/components/TraceWaterfall.tsx index a566787..ec41828 100644 --- a/ui/src/components/TraceWaterfall.tsx +++ b/ui/src/components/TraceWaterfall.tsx @@ -2,6 +2,7 @@ import { useEffect, useRef, useState } from "react"; import { Link, useSearchParams } from "react-router-dom"; import { getTrace, type SpanRow } from "../api"; import { rowKeyActivate } from "../a11y"; +import { logsHref } from "../links"; import { fmtDuration } from "./TraceList"; export interface Row { @@ -145,10 +146,7 @@ function SpanDetail({ span, onClose }: { span: SpanRow; onClose: () => void }) { {span.name} {/* Cross-signal drill: this span's logs (trace_id + span_id), or the whole service's logs (which also sets the global service focus). */} - + logs for this span ↗ {span.service && ( @@ -379,7 +377,7 @@ export default function TraceWaterfall({

{spans[0].service ?? "trace"} · {traceId.slice(0, 16)}… ·{" "} {fmtDuration(totalMs)} ·{" "} - + logs ↗ {errorSpans.length > 0 && ( diff --git a/ui/src/links.test.ts b/ui/src/links.test.ts new file mode 100644 index 0000000..942d474 --- /dev/null +++ b/ui/src/links.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "vitest"; +import { logsHref, traceHref } from "./links"; + +// JEF-534: trace_id/span_id are ingested OTLP data (attacker-shapeable) and were +// interpolated raw into these deep-links, unlike the neighboring `service` focus +// which was already `encodeURIComponent`-ed. These tests pin the well-formed-id +// shape (no behavior change vs. the pre-fix raw interpolation) and lock the +// encoding for ids with special characters. +describe("traceHref", () => { + it("links to just the trace for a well-formed hex id (no behavior change)", () => { + expect(traceHref("abcdef0123456789")).toBe("/traces/abcdef0123456789"); + }); + + it("carries the service focus", () => { + expect(traceHref("abcdef0123456789", { service: "api" })).toBe( + "/traces/abcdef0123456789?service=api", + ); + }); + + it("carries span + service, span first (matches the pre-fix param order)", () => { + expect(traceHref("abcdef0123456789", { spanId: "0123456789abcdef", service: "api" })).toBe( + "/traces/abcdef0123456789?span=0123456789abcdef&service=api", + ); + }); + + it("encodes a trace id with special characters into the path segment", () => { + expect(traceHref("dead&beef#1")).toBe("/traces/dead%26beef%231"); + }); + + it("encodes a span id with special characters into the query string", () => { + expect(traceHref("t1", { spanId: "span&1" })).toBe("/traces/t1?span=span%261"); + }); +}); + +describe("logsHref", () => { + it("carries just the trace id when no span is given", () => { + expect(logsHref("abcdef0123456789")).toBe("/logs?trace_id=abcdef0123456789"); + }); + + it("carries trace_id and span_id for a well-formed hex id (no behavior change)", () => { + expect(logsHref("abcdef0123456789", "0123456789abcdef")).toBe( + "/logs?trace_id=abcdef0123456789&span_id=0123456789abcdef", + ); + }); + + it("encodes special characters instead of injecting an extra query param", () => { + expect(logsHref("dead&beef", "span#1")).toBe( + "/logs?trace_id=dead%26beef&span_id=span%231", + ); + }); +}); diff --git a/ui/src/links.ts b/ui/src/links.ts new file mode 100644 index 0000000..773bad3 --- /dev/null +++ b/ui/src/links.ts @@ -0,0 +1,26 @@ +// Deep-link builders shared by every cross-signal drill-in (trace ↔ logs ↔ +// metrics). Trace/span ids come from ingested OTLP data — attacker-shapeable — +// so every id here is either `encodeURIComponent`-ed as a path segment or set +// via URLSearchParams: a stray `&`/`#`/`?` must stay inside its own param, +// never land on an unexpected route or inject an extra one (JEF-534). + +// `/traces/:traceId`, optionally scoped to a span and/or the global service +// focus. Used by the trace list row link, a log row's drill-in, and a metric +// chart exemplar click. +export function traceHref( + traceId: string, + opts: { spanId?: string | null; service?: string | null } = {}, +): string { + const p = new URLSearchParams(); + if (opts.spanId) p.set("span", opts.spanId); + if (opts.service) p.set("service", opts.service); + const q = p.toString(); + return `/traces/${encodeURIComponent(traceId)}${q ? `?${q}` : ""}`; +} + +// `/logs`, scoped to a trace and (optionally) a span. +export function logsHref(traceId: string, spanId?: string): string { + const p = new URLSearchParams({ trace_id: traceId }); + if (spanId) p.set("span_id", spanId); + return `/logs?${p.toString()}`; +}