fix(webapp): validate chart colors and isolate style identifiers

Mono-RevId: 613a15b5658e00dd4d0bd7a4fa018dfe14158545

Author: pullfrog[bot]

apps/webapp/app/components/alerts/AlertThresholdChart.render.test.ts

@@ -47,7 +47,7 @@ describe("AlertMultiSeriesChart hostile queue names", () => {
     expect(html).not.toContain("<script>");
     expect(styles).not.toContain("alert(1)");
     expect(styles).not.toContain('q"}{x:y');
-    expect(styles).toContain("--color-ok-queue");
+    expect(html).toContain("ok-queue");
     expect(html).toContain("a&lt;/style&gt;&lt;script&gt;alert(1)&lt;/script&gt;");
   });
 });

apps/webapp/app/components/code/QueryResultsChart.dom.test.ts

@@ -0,0 +1,87 @@
+// @vitest-environment jsdom
+import { createElement } from "react";
+import { renderToStaticMarkup } from "react-dom/server";
+import { describe, expect, it } from "vitest";
+import type { ChartConfiguration } from "../metrics/QueryWidget";
+import { QueryResultsChart } from "./QueryResultsChart";
+
+const hostileNames = [
+  "</style><script>alert(1)</script>",
+  'series" onload="alert(1)',
+  "series; } body { color: red; } /*",
+  "series], body [data-chart",
+  "url(https://example.com)",
+  "__proto__",
+  "constructor",
+];
+
+describe("query chart series rendering", () => {
+  it.each(["bar", "line"] as const)(
+    "preserves grouped %s labels without using them in CSS",
+    (chartType) => {
+      const config: ChartConfiguration = {
+        chartType,
+        xAxisColumn: "bucket",
+        yAxisColumns: ["count"],
+        groupByColumn: "group",
+        stacked: false,
+        sortByColumn: null,
+        sortDirection: "asc",
+        aggregation: "sum",
+        seriesColors: Object.fromEntries(hostileNames.map((name) => [name, name])),
+      };
+      const container = document.createElement("div");
+      container.innerHTML = renderToStaticMarkup(
+        createElement(QueryResultsChart, {
+          rows: hostileNames.map((group) => ({ bucket: "category", group, count: 1 })),
+          columns: [],
+          config,
+          fullLegend: true,
+        })
+      );
+      const style = container.querySelector("style")!;
+
+      for (const name of hostileNames) {
+        expect(container.textContent).toContain(name);
+        expect(style.textContent).not.toContain(name);
+      }
+      expect(container.querySelectorAll("style")).toHaveLength(1);
+      expect(container.querySelector("script, [onload]")).toBeNull();
+    }
+  );
+
+  it.each([false, true])(
+    "aggregates hostile names with numeric values (grouped: %s)",
+    (grouped) => {
+      const name = 'series.[0]</style>"{}';
+      const config: ChartConfiguration = {
+        chartType: "bar",
+        xAxisColumn: "bucket",
+        yAxisColumns: grouped ? ["count"] : [name],
+        groupByColumn: grouped ? "group" : null,
+        stacked: false,
+        sortByColumn: null,
+        sortDirection: "asc",
+        aggregation: "sum",
+        seriesColors: { [name]: "url(https://example.com)" },
+      };
+      const container = document.createElement("div");
+      container.innerHTML = renderToStaticMarkup(
+        createElement(QueryResultsChart, {
+          rows: grouped
+            ? [{ bucket: "category", group: name, count: 7 }]
+            : [{ bucket: "category", [name]: 7 }],
+          columns: [],
+          config,
+          fullLegend: true,
+        })
+      );
+
+      expect(container.querySelector("style")?.textContent).not.toContain(name);
+      expect(container.querySelector("script, [onload]")).toBeNull();
+      container.querySelector("style")?.remove();
+      expect(container.textContent).toContain(name);
+      expect(container.textContent).toContain("7");
+    }
+  );
+});

apps/webapp/app/components/code/QueryResultsChart.tsx

@@ -15,6 +15,7 @@ import { aggregateValues } from "../primitives/charts/aggregation";
 import { MAX_SERIES } from "~/components/primitives/charts/seriesFromRows";
 import { getRunStatusChartColor } from "~/components/runs/v3/TaskRunStatus";
 import { getSeriesColor } from "./chartColors";
+import { safeChartColor } from "../primitives/charts/chartColors";
 
 const MAX_SVG_ELEMENT_BUDGET = 6_000;
 const MIN_DATA_POINTS = 100;
@@ -287,10 +288,10 @@ function fillTimeGaps(
         __rawDate: bucket.rawDate,
         __granularity: granularity,
         __originalX: bucket.originalX,
+        ...Object.fromEntries(
+          series.map((s) => [s, aggregateValues(bucket.values[s], aggregation)])
+        ),
       };
-      for (const s of series) {
-        point[s] = aggregateValues(bucket.values[s], aggregation);
-      }
       filledData.push(point);
     } else {
       // Create a null-filled data point so gaps appear in line/bar charts
@@ -300,10 +301,8 @@ function fillTimeGaps(
         __rawDate: new Date(t),
         __granularity: granularity,
         __originalX: new Date(t).toISOString(),
+        ...Object.fromEntries(series.map((s) => [s, null])),
       };
-      for (const s of series) {
-        gapPoint[s] = null;
-      }
       filledData.push(gapPoint);
     }
   }
@@ -591,10 +590,10 @@ function transformDataForChart(
         __rawDate: rawDate,
         __granularity: granularity,
         __originalX: originalX,
+        ...Object.fromEntries(
+          yAxisColumns.map((col) => [col, aggregateValues(yValues[col], aggregation)])
+        ),
       };
-      for (const yCol of yAxisColumns) {
-        point[yCol] = aggregateValues(yValues[yCol], aggregation);
-      }
       return point;
     });
 
@@ -652,7 +651,7 @@ function transformDataForChart(
   const groupTotals = new Map<string, number>();
   const groupedByX = new Map<
     string | number,
-    { values: Record<string, number[]>; rawDate: Date | null; originalX: unknown }
+    { values: Map<string, number[]>; rawDate: Date | null; originalX: unknown }
   >();
 
   for (const row of rows) {
@@ -667,14 +666,14 @@ function transformDataForChart(
     groupTotals.set(groupValue, (groupTotals.get(groupValue) ?? 0) + Math.abs(yValue));
 
     if (!groupedByX.has(xKey)) {
-      groupedByX.set(xKey, { values: {}, rawDate, originalX: row[xAxisColumn] });
+      groupedByX.set(xKey, { values: new Map(), rawDate, originalX: row[xAxisColumn] });
     }
 
     const existing = groupedByX.get(xKey)!;
-    if (!existing.values[groupValue]) {
-      existing.values[groupValue] = [];
+    if (!existing.values.has(groupValue)) {
+      existing.values.set(groupValue, []);
     }
-    existing.values[groupValue].push(yValue);
+    existing.values.get(groupValue)!.push(yValue);
   }
 
   // Keep only the top MAX_SERIES groups by absolute total to avoid O(n) processing
@@ -697,10 +696,13 @@ function transformDataForChart(
       __rawDate: rawDate,
       __granularity: granularity,
       __originalX: originalX,
+      ...Object.fromEntries(
+        series.map((group) => {
+          const groupValues = values.get(group);
+          return [group, groupValues ? aggregateValues(groupValues, aggregation) : 0];
+        })
+      ),
     };
-    for (const group of series) {
-      point[group] = values[group] ? aggregateValues(values[group], aggregation) : 0;
-    }
     return point;
   });
 
@@ -936,20 +938,34 @@ export const QueryResultsChart = memo(function QueryResultsChart({
     return col?.customRenderType === "runStatus";
   }, [groupByColumn, columns]);
 
-  // Build chart config for colors/labels
-  const chartConfig = useMemo(() => {
-    const cfg: ChartConfig = {};
-    sortedSeries.forEach((s, i) => {
-      const statusColor = groupByIsRunStatus ? getRunStatusChartColor(s) : undefined;
-      const originalIndex = config.yAxisColumns.indexOf(s);
-      const colorIndex = originalIndex >= 0 ? originalIndex : i;
-      cfg[s] = {
-        label: s,
-        color: statusColor ?? config.seriesColors?.[s] ?? getSeriesColor(colorIndex),
-      };
-    });
-    return cfg;
-  }, [sortedSeries, groupByIsRunStatus, config.seriesColors, config.yAxisColumns]);
+  // Keep data-derived names in labels, never in chart identifiers or CSS variable names.
+  const { chartConfig, chartData, chartSeries } = useMemo(() => {
+    const chartSeries = sortedSeries.map((_, i) => `series-${i}`);
+    const chartConfig: ChartConfig = Object.fromEntries(
+      sortedSeries.map((s, i) => {
+        const statusColor = groupByIsRunStatus ? getRunStatusChartColor(s) : undefined;
+        const originalIndex = config.yAxisColumns.indexOf(s);
+        const colorIndex = originalIndex >= 0 ? originalIndex : i;
+        return [
+          chartSeries[i],
+          {
+            label: s,
+            color:
+              statusColor ?? safeChartColor(config.seriesColors?.[s]) ?? getSeriesColor(colorIndex),
+          },
+        ];
+      })
+    );
+    const chartData = data.map((point) => ({
+      ...Object.fromEntries(sortedSeries.map((s, i) => [chartSeries[i], point[s]])),
+      __chartX: point[xDataKey],
+      __rawDate: point.__rawDate,
+      __granularity: point.__granularity,
+      __originalX: point.__originalX,
+    }));
+    return { chartConfig, chartData, chartSeries };
+  }, [sortedSeries, data, xDataKey, groupByIsRunStatus, config.seriesColors, config.yAxisColumns]);
+  const chartVisibleSeries = chartSeries.slice(0, visibleSeries.length);
 
   // Custom tooltip label formatter for better date display
   const tooltipLabelFormatter = useMemo(() => {
@@ -1182,10 +1198,10 @@ export const QueryResultsChart = memo(function QueryResultsChart({
       <div ref={chartMeasureRef} className="h-full w-full">
         <Chart.Root
           config={chartConfig}
-          data={data}
-          dataKey={xDataKey}
-          series={sortedSeries}
-          visibleSeries={visibleSeries}
+          data={chartData}
+          dataKey="__chartX"
+          series={chartSeries}
+          visibleSeries={chartVisibleSeries}
           labelFormatter={legendLabelFormatter}
           showLegend={showLegend}
           maxLegendItems={fullLegend ? Infinity : 5}
@@ -1215,10 +1231,10 @@ export const QueryResultsChart = memo(function QueryResultsChart({
     <div ref={chartMeasureRef} className="h-full w-full">
       <Chart.Root
         config={chartConfig}
-        data={data}
-        dataKey={xDataKey}
-        series={sortedSeries}
-        visibleSeries={visibleSeries}
+        data={chartData}
+        dataKey="__chartX"
+        series={chartSeries}
+        visibleSeries={chartVisibleSeries}
         labelFormatter={legendLabelFormatter}
         showLegend={showLegend}
         maxLegendItems={fullLegend ? Infinity : 5}

apps/webapp/app/components/primitives/charts/Chart.dom.test.ts

@@ -0,0 +1,47 @@
+// @vitest-environment jsdom
+import { createElement } from "react";
+import { renderToStaticMarkup } from "react-dom/server";
+import { describe, expect, it } from "vitest";
+import { ChartContainer, type ChartConfig } from "./Chart";
+
+describe("chart stylesheet rendering", () => {
+  it.each([
+    "</style><script>alert(1)</script>",
+    'series" onload="alert(1)',
+    "series; } body { color: red; } /*",
+    "series], body [data-chart",
+    "url(https://example.com)",
+  ])("keeps configuration key %s out of CSS and selector attributes", (key) => {
+    const config: ChartConfig = {
+      [key]: { label: key, color: "#123456" },
+      invalidColor: { color: key },
+      themed: { theme: { light: "#abc", dark: key } },
+    };
+    const html = renderToStaticMarkup(
+      createElement(ChartContainer, {
+        id: key,
+        config,
+        // oxlint-disable-next-line react/no-children-prop -- Required by ChartContainer's createElement props type.
+        children: createElement("div"),
+      })
+    );
+    const container = document.createElement("div");
+    container.innerHTML = html;
+    const chart = container.querySelector("[data-chart]")!;
+    const chartId = chart.getAttribute("data-chart")!;
+    const style = chart.querySelector("style")!;
+
+    expect(chartId).toMatch(/^chart-[a-f0-9-]+$/);
+    expect(chart.id).toBe(key);
+    expect(style.childNodes).toHaveLength(1);
+    expect(style.firstChild?.nodeType).toBe(Node.TEXT_NODE);
+    expect(style.textContent).toContain(`[data-chart=${chartId}]`);
+    expect(style.textContent).toContain("--color-series-0: #123456;");
+    expect(style.textContent).toContain("--color-series-1: var(--color-text-dimmed);");
+    expect(style.textContent).toContain(":is([data-theme=dark], [data-theme=black])");
+    expect(style.textContent).not.toContain(key);
+    expect(style.textContent).not.toContain("&quot;");
+    expect(container.querySelectorAll("style")).toHaveLength(1);
+    expect(container.querySelector("script, [onload]")).toBeNull();
+  });
+});