fix: mask SQL comments and reject UNION/JOIN in the custom-SQL chart rewrite (#14574)

web/src/utils/alerts/aggregationPreviewQuery.spec.ts

@@ -14,7 +14,11 @@
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import { describe, expect, it } from "vitest";
-import { cleanAggregationQuery, withCompositeGroupLabel } from "./aggregationPreviewQuery";
+import {
+  buildCountChartQuery,
+  cleanAggregationQuery,
+  withCompositeGroupLabel,
+} from "./aggregationPreviewQuery";
 
 describe("cleanAggregationQuery", () => {
   it("drops the HAVING clause on a plain query", () => {
@@ -122,3 +126,63 @@ describe("withCompositeGroupLabel", () => {
     expect(out).toMatch(/GROUP BY zo_sql_key, zo_group_label/i);
   });
 });
+
+describe("buildCountChartQuery", () => {
+  it("rewrites a plain custom SQL query into a count-over-time query", () => {
+    const chartQuery = buildCountChartQuery('SELECT _timestamp FROM "bugtest"');
+
+    expect(chartQuery).toBe(
+      'SELECT histogram(_timestamp) AS zo_sql_key, count(*) AS zo_sql_num FROM "bugtest" GROUP BY zo_sql_key',
+    );
+  });
+
+  it("does not mistake a line comment's 'from' for the statement's real FROM", () => {
+    const chartQuery = buildCountChartQuery(
+      'SELECT _timestamp -- pick the from column\nFROM "bugtest"',
+    );
+
+    expect(chartQuery).toBe(
+      'SELECT histogram(_timestamp) AS zo_sql_key, count(*) AS zo_sql_num FROM "bugtest" GROUP BY zo_sql_key',
+    );
+  });
+
+  it("does not mistake a block comment's 'from' for the statement's real FROM", () => {
+    const chartQuery = buildCountChartQuery('SELECT _timestamp /* from here */ FROM "bugtest"');
+
+    expect(chartQuery).toBe(
+      'SELECT histogram(_timestamp) AS zo_sql_key, count(*) AS zo_sql_num FROM "bugtest" GROUP BY zo_sql_key',
+    );
+  });
+
+  it("returns null rather than a broken query for a query that starts with a CTE", () => {
+    const chartQuery = buildCountChartQuery(
+      'WITH x AS (SELECT _timestamp FROM "bugtest") SELECT _timestamp FROM x',
+    );
+
+    expect(chartQuery).toBeNull();
+  });
+
+  it("returns null rather than a broken query for a UNION", () => {
+    const chartQuery = buildCountChartQuery(
+      'SELECT _timestamp FROM "bugtest" UNION ALL SELECT _timestamp FROM "bugtest"',
+    );
+
+    expect(chartQuery).toBeNull();
+  });
+
+  it("returns null rather than a broken query for a JOIN", () => {
+    const chartQuery = buildCountChartQuery(
+      'SELECT a._timestamp FROM "bugtest" a JOIN "bugtest" b ON a.svc = b.svc',
+    );
+
+    expect(chartQuery).toBeNull();
+  });
+
+  it("still ignores a quoted filter value containing a real comment marker", () => {
+    const chartQuery = buildCountChartQuery(
+      "SELECT _timestamp FROM \"bugtest\" WHERE (note = '-- not a comment')",
+    );
+
+    expect(chartQuery).toContain("WHERE (note = '-- not a comment')");
+  });
+});

web/src/utils/alerts/aggregationPreviewQuery.ts

@@ -25,6 +25,57 @@ const maskStringLiterals = (sql: string): string =>
     (m) => m[0] + "x".repeat(m.length - 2) + m[m.length - 1],
   );
 
+/**
+ * Blank out SQL line and block comments, quote-aware in both directions: a
+ * comment marker inside a string literal isn't mistaken for a real comment,
+ * and a quote inside a comment isn't mistaken for the start of a string
+ * literal. Run before maskStringLiterals — a regex pass can't safely tell
+ * the two apart in one direction only.
+ */
+const maskComments = (sql: string): string => {
+  let out = "";
+  let i = 0;
+  while (i < sql.length) {
+    const ch = sql[i];
+    if (ch === "'" || ch === '"') {
+      out += ch;
+      i++;
+      while (i < sql.length) {
+        out += sql[i];
+        if (sql[i] === ch) {
+          if (sql[i + 1] === ch) {
+            out += sql[i + 1];
+            i += 2;
+            continue;
+          }
+          i++;
+          break;
+        }
+        i++;
+      }
+      continue;
+    }
+    if (ch === "-" && sql[i + 1] === "-") {
+      let j = i;
+      while (j < sql.length && sql[j] !== "\n") j++;
+      out += "x".repeat(j - i);
+      i = j;
+      continue;
+    }
+    if (ch === "/" && sql[i + 1] === "*") {
+      let j = i + 2;
+      while (j < sql.length && !(sql[j] === "*" && sql[j + 1] === "/")) j++;
+      j = Math.min(j + 2, sql.length);
+      out += "x".repeat(j - i);
+      i = j;
+      continue;
+    }
+    out += ch;
+    i++;
+  }
+  return out;
+};
+
 /**
  * Blank out everything inside parentheses, nesting-aware, so a keyword used
  * inside a function call or subquery — e.g. the FROM in EXTRACT(EPOCH FROM
@@ -186,7 +237,7 @@ export default cleanAggregationQuery;
  */
 export const buildCountChartQuery = (query: string): string | null => {
   if (!query) return null;
-  const masked = maskForKeywordSearch(query);
+  const masked = maskForKeywordSearch(maskComments(query));
   if (!/^\s*SELECT\b/i.test(masked)) return null;
   const fromMatch = masked.match(/\bFROM\b/i);
   if (!fromMatch) return null;
@@ -197,6 +248,11 @@ export const buildCountChartQuery = (query: string): string | null => {
   const maskedTail = masked.slice(fromIndex);
   const cutMatch = maskedTail.match(/\s+(?:ORDER\s+BY|LIMIT|GROUP\s+BY|HAVING)\b/i);
   const cutIndex = cutMatch ? fromIndex + (cutMatch.index as number) : undefined;
+  const maskedRowQuery = masked.slice(0, cutIndex);
+  // UNION/JOIN change what a single "count of matching rows" even means —
+  // rewriting the projection around them would either fail to parse or chart
+  // something other than the alert's own row count. No chart beats a wrong one.
+  if (/\b(?:UNION|JOIN)\b/i.test(maskedRowQuery)) return null;
   const tail = query.slice(fromIndex, cutIndex).trim();
   if (!tail) return null;
 

web/src/views/OnCall/OnCallResponses.spec.ts

@@ -13,8 +13,8 @@
 // You should have received a copy of the GNU Affero General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import { flushPromises, mount } from "@vue/test-utils";
-import { beforeEach, describe, expect, it, vi } from "vitest";
+import { flushPromises, mount, VueWrapper } from "@vue/test-utils";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 
 import i18n from "@/locales";
 import destinationService from "@/services/alert_destination";
@@ -201,11 +201,23 @@ const team = {
   updated_at: 0,
 };
 
+// Nothing here ever unmounts on its own account, so every mount from every
+// test would otherwise sit alive (watchers, listeners) through the rest of
+// the 70+ tests in this file — the accumulation is exactly what pushed the
+// last few tests in the file past the default 5s timeout under load.
+const mountedWrappers: VueWrapper[] = [];
+
 function render() {
-  return mount(OnCallResponses, { global: { plugins: [i18n, store], stubs } });
+  const wrapper = mount(OnCallResponses, { global: { plugins: [i18n, store], stubs } });
+  mountedWrappers.push(wrapper);
+  return wrapper;
 }
 
 describe("OnCallResponses", () => {
+  afterEach(() => {
+    for (const wrapper of mountedWrappers.splice(0)) wrapper.unmount();
+  });
+
   beforeEach(() => {
     vi.clearAllMocks();
     for (const key of Object.keys(routeQuery)) delete routeQuery[key];