Escape timeframe and project id values in ClickHouse queries

The cohort query builder escapes every value it puts into SQL except the
two absolute-timeframe dates, which were interpolated raw into
toDate('...'). A date carrying a quote produced a query that no longer
parsed the way the builder intended. Route both through sqlstring.escape
like the rest of the file, and constrain the timeframe schema to the
YYYY-MM-DD shape toDate actually expects, so a bad value is rejected at
the edge instead of reaching the query.

getActiveVisitorCount had the same raw interpolation for project_id. Its
callers all pass an id that came back from the database, so this changes
no behaviour, but it makes the query consistent with the others.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: lindesvard

packages/db/src/buffers/event-buffer.test.ts

@@ -254,6 +254,20 @@ describe('EventBuffer', () => {
     querySpy.mockRestore();
   });
 
+  it('escapes the project id in the active visitor query', async () => {
+    const querySpy = vi
+      .spyOn(chClient, 'chQuery')
+      .mockResolvedValueOnce([{ count: 0 }] as any);
+
+    await eventBuffer.getActiveVisitorCount("p9' OR 1=1 --");
+
+    const sql = querySpy.mock.calls[0]![0];
+    expect(sql).toContain("project_id = 'p9\\' OR 1=1 --'");
+    expect(sql).not.toContain("project_id = 'p9' OR");
+
+    querySpy.mockRestore();
+  });
+
   it('handles multiple sessions independently — all events go to buffer', async () => {
     const t0 = Date.now();
     const count1 = await eventBuffer.getBufferSize();

packages/db/src/buffers/event-buffer.ts

@@ -1,4 +1,5 @@
 import { getRedisCache, publishEvent } from '@openpanel/redis';
+import sqlstring from 'sqlstring';
 import { ch, chQuery } from '../clickhouse/client';
 import type { IClickhouseEvent } from '../services/event.service';
 import { BaseBuffer } from './base-buffer';
@@ -255,7 +256,7 @@ export class EventBuffer extends BaseBuffer {
     const rows = await chQuery<{ count: number }>(
       `SELECT uniq(profile_id) AS count
        FROM events
-       WHERE project_id = '${projectId}'
+       WHERE project_id = ${sqlstring.escape(projectId)}
          AND profile_id != ''
          AND created_at >= now() - INTERVAL 5 MINUTE`
     );

packages/db/src/services/cohort.service.test.ts

@@ -0,0 +1,90 @@
+import type { EventCriteria } from '@openpanel/validation';
+import { describe, expect, it } from 'vitest';
+import { buildEventCriteriaQuery } from './cohort.service';
+
+const PROJECT_ID = 'test-cohort-timeframe';
+
+function criteria(timeframe: EventCriteria['timeframe']): EventCriteria {
+  return {
+    name: 'screen_view',
+    filters: [],
+    timeframe,
+  } as EventCriteria;
+}
+
+/**
+ * Remove every quoted string literal from the SQL, leaving the skeleton the
+ * ClickHouse parser would act on. Anything a caller smuggled in stays inside a
+ * literal if it was escaped properly, so it disappears here; if it broke out,
+ * it shows up in the skeleton.
+ */
+function skeleton(sql: string): string {
+  return sql.replace(/'(?:\\.|[^'\\])*'/g, "''");
+}
+
+// Values that break out of a naive `toDate('${value}')` interpolation.
+const HOSTILE = [
+  "2024-01-01') OR 1=1 --",
+  "2024-01-01'",
+  "2024-01-01') UNION ALL SELECT id FROM profiles --",
+];
+
+describe('buildEventCriteriaQuery timeframe escaping', () => {
+  it.each(HOSTILE)('escapes a hostile start (%s)', (start) => {
+    const sql = buildEventCriteriaQuery(
+      PROJECT_ID,
+      criteria({ type: 'absolute', start })
+    );
+
+    // The value survives as exactly one quoted literal inside toDate(...).
+    expect(sql).toContain(
+      `event_date >= toDate('${start.replace(/'/g, "\\'")}')`
+    );
+    // The parser sees the same shape as a well-formed date: no extra clause,
+    // no early close of the toDate() call, no trailing comment.
+    expect(skeleton(sql)).toContain("event_date >= toDate('')");
+    expect(skeleton(sql)).not.toMatch(/OR|UNION|--/);
+  });
+
+  it.each(HOSTILE)('escapes a hostile end (%s)', (end) => {
+    const sql = buildEventCriteriaQuery(
+      PROJECT_ID,
+      criteria({ type: 'absolute', start: '2024-01-01', end })
+    );
+
+    expect(sql).toContain(
+      `event_date BETWEEN toDate('2024-01-01') AND toDate('${end.replace(/'/g, "\\'")}')`
+    );
+    expect(skeleton(sql)).toContain(
+      "event_date BETWEEN toDate('') AND toDate('')"
+    );
+    expect(skeleton(sql)).not.toMatch(/OR|UNION|--/);
+  });
+
+  it('leaves well-formed dates readable', () => {
+    expect(
+      buildEventCriteriaQuery(
+        PROJECT_ID,
+        criteria({ type: 'absolute', start: '2024-01-01' })
+      )
+    ).toContain("event_date >= toDate('2024-01-01')");
+
+    expect(
+      buildEventCriteriaQuery(
+        PROJECT_ID,
+        criteria({ type: 'absolute', start: '2024-01-01', end: '2024-02-01' })
+      )
+    ).toContain(
+      "event_date BETWEEN toDate('2024-01-01') AND toDate('2024-02-01')"
+    );
+  });
+
+  it('still builds relative timeframes', () => {
+    expect(
+      buildEventCriteriaQuery(
+        PROJECT_ID,
+        criteria({ type: 'relative', value: '30d' })
+      )
+    ).toContain('event_date >= toDate(now() - INTERVAL 30 DAY)');
+  });
+});

packages/db/src/services/cohort.service.ts

@@ -36,11 +36,11 @@ function buildTimeConstraint(timeframe: Timeframe): string {
     return `created_at >= toDate(now() - INTERVAL ${days} DAY)`;
   }
 
-  const start = timeframe.start;
+  const start = sqlstring.escape(timeframe.start);
   if (timeframe.end) {
-    return `created_at BETWEEN toDate('${start}') AND toDate('${timeframe.end}')`;
+    return `created_at BETWEEN toDate(${start}) AND toDate(${sqlstring.escape(timeframe.end)})`;
   }
-  return `created_at >= toDate('${start}')`;
+  return `created_at >= toDate(${start})`;
 }
 
 function getFrequencyOperator(frequency: Frequency): string {

packages/validation/src/cohort.validation.test.ts

@@ -0,0 +1,42 @@
+import { describe, expect, it } from 'vitest';
+import { zAbsoluteTimeframe } from './cohort.validation';
+
+describe('zAbsoluteTimeframe', () => {
+  it('accepts a date without an end', () => {
+    expect(
+      zAbsoluteTimeframe.safeParse({ type: 'absolute', start: '2024-01-01' })
+        .success
+    ).toBe(true);
+  });
+
+  it('accepts a date range', () => {
+    expect(
+      zAbsoluteTimeframe.safeParse({
+        type: 'absolute',
+        start: '2024-01-01',
+        end: '2024-02-01',
+      }).success
+    ).toBe(true);
+  });
+
+  it.each([
+    "2024-01-01') OR 1=1 --",
+    '2024-1-1',
+    'yesterday',
+    '',
+  ])('rejects a non-date start (%s)', (start) => {
+    expect(
+      zAbsoluteTimeframe.safeParse({ type: 'absolute', start }).success
+    ).toBe(false);
+  });
+
+  it('rejects a non-date end', () => {
+    expect(
+      zAbsoluteTimeframe.safeParse({
+        type: 'absolute',
+        start: '2024-01-01',
+        end: "2024-01-01') --",
+      }).success
+    ).toBe(false);
+  });
+});

packages/validation/src/cohort.validation.ts

@@ -29,10 +29,15 @@ export const zRelativeTimeframe = z.object({
   value: z.enum(['7d', '30d', '90d', '180d', '365d']),
 });
 
+// toDate() in the cohort query builder only understands plain calendar dates.
+const zDate = z
+  .string()
+  .regex(/^\d{4}-\d{2}-\d{2}$/, 'Expected a date in YYYY-MM-DD format');
+
 export const zAbsoluteTimeframe = z.object({
   type: z.literal('absolute'),
-  start: z.string(),
-  end: z.string().optional(),
+  start: zDate,
+  end: zDate.optional(),
 });
 
 export const zTimeframe = z.discriminatedUnion('type', [

packages/validation/vitest.config.ts

@@ -0,0 +1,3 @@
+import { getSharedVitestConfig } from '../../vitest.shared';
+
+export default getSharedVitestConfig({ __dirname });