From c52c084384b546cad8e1bce5fb5a2512ff13bb6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesv=C3=A4rd?= Date: Wed, 26 Aug 2026 12:01:56 +0000 Subject: [PATCH] fix(db): resolve profile.* filter fields to known profiles columns The profile.* branch of buildFilterWhere took the caller-supplied field name and put it straight into the generated predicate, so a name that was not a profiles column became whatever text the caller sent. The group.* and session.* branches already resolve their names and drop the filter when they cannot; profile.* now does the same, against the column set that getProfilePropertySelect lists for the SELECT side. Fragments are also parenthesized on the way out. Callers join them with AND and no grouping, so a fragment with a top-level OR (contains, gt with several values) changed how the surrounding conditions bound. Co-Authored-By: Claude Opus 5 --- packages/db/src/services/chart.service.ts | 2 + .../db/src/services/filter-where.service.ts | 46 +++- packages/db/src/services/filter-where.test.ts | 247 ++++++++++++++++++ 3 files changed, 283 insertions(+), 12 deletions(-) create mode 100644 packages/db/src/services/filter-where.test.ts diff --git a/packages/db/src/services/chart.service.ts b/packages/db/src/services/chart.service.ts index ab00f319b..9e3badc04 100644 --- a/packages/db/src/services/chart.service.ts +++ b/packages/db/src/services/chart.service.ts @@ -284,6 +284,8 @@ export function getGroupPropertySelect(property: string): string { // Returns the SELECT expression when querying the profiles table directly (no join alias). // Use for fetching distinct values for profile.* properties. +// Lists the same profiles columns as PROFILE_COLUMNS in filter-where.service.ts, +// which resolves profile.* on the filter side; keep the two in sync. export function getProfilePropertySelect(property: string): string { const withoutPrefix = property.replace(/^profile\./, ''); if (withoutPrefix === 'id') { diff --git a/packages/db/src/services/filter-where.service.ts b/packages/db/src/services/filter-where.service.ts index e51872f84..5920667ea 100644 --- a/packages/db/src/services/filter-where.service.ts +++ b/packages/db/src/services/filter-where.service.ts @@ -163,18 +163,38 @@ function compileScalarClause( return null; } +/** + * Profiles columns a `profile.` filter may resolve to. Anything else is + * not a column name and must not reach the SQL text. Kept in sync with + * `getProfilePropertySelect` in chart.service.ts, which lists the same set for + * the SELECT side. + */ +const PROFILE_COLUMNS = new Set([ + 'id', + 'first_name', + 'last_name', + 'email', + 'avatar', + 'created_at', + 'last_seen_at', +]); + /** * Translate `profile.` into the SQL accessor used when querying the * profiles table directly. Bare fields (email, first_name, …) map to columns; * `profile.properties.` maps to the JSON `properties[key]` lookup. + * Returns null for a field that is neither, so the caller can drop the filter. */ -function profileColumnSql(name: string): string { +function profileColumnSql(name: string): string | null { const withoutPrefix = name.replace(/^profile\./, ''); if (withoutPrefix.startsWith('properties.')) { const key = withoutPrefix.replace(/^properties\./, ''); return `properties[${sqlstring.escape(key)}]`; } - return withoutPrefix; + if (PROFILE_COLUMNS.has(withoutPrefix)) { + return withoutPrefix; + } + return null; } /** @@ -243,6 +263,7 @@ function buildProfileClause( ctx: FilterTableContext, ): string | null { const column = profileColumnSql(filter.name); + if (!column) return null; const numeric = column === 'created_at' || column === 'last_seen_at'; const inner = compileScalarClause(column, filter.operator, filter.value, { numeric, @@ -312,34 +333,35 @@ export function buildFilterWhere( const where: Record = {}; filters.forEach((filter, index) => { const id = `f${index}`; + // Callers concatenate these fragments with AND and no grouping, so each + // fragment is parenthesized here to keep a top-level OR inside it from + // rebinding the surrounding conditions. + const set = (clause: string | null) => { + if (clause) where[id] = `(${clause})`; + }; if (filter.operator === 'inCohort' || filter.operator === 'notInCohort') { - const clause = buildCohortClause(filter, projectId, ctx); - if (clause) where[id] = clause; + set(buildCohortClause(filter, projectId, ctx)); return; } if (filter.name.startsWith('cohort:')) { - const clause = buildCohortClause(filter, projectId, ctx); - if (clause) where[id] = clause; + set(buildCohortClause(filter, projectId, ctx)); return; } if (filter.name.startsWith('group.')) { - const clause = buildGroupClause(filter, projectId, ctx); - if (clause) where[id] = clause; + set(buildGroupClause(filter, projectId, ctx)); return; } if (filter.name.startsWith('profile.')) { - const clause = buildProfileClause(filter, projectId, ctx); - if (clause) where[id] = clause; + set(buildProfileClause(filter, projectId, ctx)); return; } if (filter.name.startsWith('session.')) { - const clause = buildSessionClause(filter, projectId, ctx); - if (clause) where[id] = clause; + set(buildSessionClause(filter, projectId, ctx)); return; } diff --git a/packages/db/src/services/filter-where.test.ts b/packages/db/src/services/filter-where.test.ts new file mode 100644 index 000000000..07427c3ac --- /dev/null +++ b/packages/db/src/services/filter-where.test.ts @@ -0,0 +1,247 @@ +/** + * Unit tests for `buildFilterWhere`. + * + * These are pure string tests — no ClickHouse needed. The interesting part is + * the `profile.*` branch: the field name arrives from the caller (saved + * report, URL state, raw API call) and used to be concatenated into the SQL + * text as-is, so a name that was not a column produced whatever SQL the caller + * wrote. It now resolves against the known profiles columns and the filter is + * dropped when it does not, which is what the `group.*` and `session.*` + * branches already did. + */ +import type { IChartEventFilter } from '@openpanel/validation'; +import { describe, expect, it } from 'vitest'; +import { TABLE_NAMES } from '../clickhouse/client'; +import { + buildFilterWhere, + type FilterTableContext, +} from './filter-where.service'; + +const PROJECT_ID = 'p'; + +const CONTEXTS: Record = { + events: { + selfTable: 'events', + profileIdExpr: 'profile_id', + groupsExpr: 'groups', + }, + sessions: { + selfTable: 'sessions', + profileIdExpr: 'profile_id', + groupsExpr: 'groups', + }, + profiles: { + selfTable: 'profiles', + profileIdExpr: 'id', + groupsExpr: 'groups', + }, +}; + +const TABLES = Object.keys(CONTEXTS) as Array; + +const filter = ( + overrides: Partial & { name: string } +): IChartEventFilter => + ({ + id: overrides.name, + operator: 'is', + value: ['x'], + ...overrides, + }) as IChartEventFilter; + +const build = ( + table: keyof typeof CONTEXTS, + filters: IChartEventFilter[] +): Record => + buildFilterWhere(filters, PROJECT_ID, CONTEXTS[table]!); + +/** What `buildProfileClause` wraps `inner` in for a given table. */ +const profileWrap = (table: keyof typeof CONTEXTS, inner: string): string => + table === 'profiles' + ? `(${inner})` + : `(profile_id IN (SELECT id FROM ${TABLE_NAMES.profiles} FINAL WHERE project_id = 'p' AND ${inner}))`; + +const ALLOWED_STRING_COLUMNS = [ + 'id', + 'first_name', + 'last_name', + 'email', + 'avatar', +]; + +/** + * Names that are not profiles columns. Each exercises a different shape of the + * problem: a space, an operator, parentheses, a quote, a nested SELECT. + */ +const REJECTED_NAMES = [ + 'profile.not a column', + 'profile.id = id', + 'profile.count(id)', + "profile.id' OR '1'='1", + 'profile.id IN (SELECT id FROM profiles)', +]; + +describe.each(TABLES)('buildFilterWhere on %s', (table) => { + it('keeps every allowed profile. predicate', () => { + for (const column of ALLOWED_STRING_COLUMNS) { + const where = build(table, [ + filter({ name: `profile.${column}`, value: ['a'] }), + ]); + expect(where.f0).toBe(profileWrap(table, `${column} = 'a'`)); + } + }); + + it('keeps the numeric handling for created_at and last_seen_at', () => { + for (const column of ['created_at', 'last_seen_at']) { + const where = build(table, [ + filter({ name: `profile.${column}`, operator: 'gt', value: ['123'] }), + ]); + expect(where.f0).toBe( + profileWrap(table, `(toFloat64(${column}) > toFloat64('123'))`) + ); + } + }); + + it('keeps the escaped map lookup for profile.properties.', () => { + const where = build(table, [ + filter({ name: 'profile.properties.plan', value: ['pro'] }), + ]); + expect(where.f0).toBe(profileWrap(table, "properties['plan'] = 'pro'")); + }); + + it('escapes a quote in a properties key', () => { + const where = build(table, [ + filter({ name: "profile.properties.pl'an", value: ['pro'] }), + ]); + expect(where.f0).toBe(profileWrap(table, "properties['pl\\'an'] = 'pro'")); + }); + + it.each(REJECTED_NAMES)('drops the unknown profile field %j', (name) => { + const where = build(table, [filter({ name, value: ['a'] })]); + expect(where).toEqual({}); + expect(where.f0).toBeUndefined(); + // The supplied name must not survive anywhere in the generated SQL. + const sql = Object.values(where).join(' AND '); + expect(sql).not.toContain(name.replace(/^profile\./, '')); + }); + + it('keeps the allowed filter and drops the rejected one', () => { + const where = build(table, [ + filter({ name: 'profile.email', value: ['a@b.com'] }), + filter({ name: 'profile.not a column', value: ['a'] }), + ]); + expect(Object.keys(where)).toEqual(['f0']); + expect(where.f0).toBe(profileWrap(table, "email = 'a@b.com'")); + }); + + it('keys surviving filters by their original index', () => { + const where = build(table, [ + filter({ name: 'profile.not a column', value: ['a'] }), + filter({ name: 'profile.email', value: ['a@b.com'] }), + ]); + expect(Object.keys(where)).toEqual(['f1']); + }); + + it('parenthesizes every fragment it returns', () => { + const where = build(table, [ + filter({ + name: 'profile.email', + operator: 'contains', + value: ['a', 'b'], + }), + filter({ name: 'profile.id', value: ['1', '2'] }), + filter({ name: 'profile.created_at', operator: 'gt', value: ['1'] }), + ]); + const fragments = Object.values(where); + expect(fragments).toHaveLength(3); + for (const fragment of fragments) { + expect(fragment.startsWith('(')).toBe(true); + expect(fragment.endsWith(')')).toBe(true); + } + }); + + it('leaves no OR outside parentheses when a caller joins with AND', () => { + const where = build(table, [ + filter({ + name: 'profile.email', + operator: 'contains', + value: ['a', 'b'], + }), + filter({ name: 'profile.created_at', operator: 'gt', value: ['1', '2'] }), + ]); + const sql = [`project_id = 'p'`, ...Object.values(where)].join(' AND '); + expect(topLevel(sql)).not.toContain(' OR '); + expect(topLevel(sql)).toContain(' AND '); + }); +}); + +describe('buildFilterWhere project scoping', () => { + it('still scopes the profiles subselect by project_id on non-profiles tables', () => { + for (const table of ['events', 'sessions'] as const) { + const where = build(table, [ + filter({ name: 'profile.email', value: ['a@b.com'] }), + ]); + expect(where.f0).toContain(`project_id = 'p'`); + expect(where.f0).toContain(` AND email = 'a@b.com'`); + } + }); + + it('has no subselect on the profiles table', () => { + const where = build('profiles', [ + filter({ name: 'profile.email', value: ['a@b.com'] }), + ]); + expect(where.f0).toBe("(email = 'a@b.com')"); + }); +}); + +describe('buildFilterWhere sibling branches', () => { + it('still falls back to id for a group field outside its set', () => { + const where = build('events', [ + filter({ name: 'group.not a column', value: ['a'] }), + ]); + expect(where.f0).toContain("id = 'a'"); + expect(where.f0).not.toContain('not a column'); + }); + + it('still resolves the known group fields', () => { + const where = build('events', [ + filter({ name: 'group.name', value: ['acme'] }), + ]); + expect(where.f0).toContain("name = 'acme'"); + }); + + it('still drops a session field outside its set', () => { + expect( + build('sessions', [ + filter({ name: 'session.not a column', value: ['a'] }), + ]) + ).toEqual({}); + }); + + it('still resolves the known session fields', () => { + const where = build('sessions', [ + filter({ name: 'session.duration', operator: 'gt', value: ['10'] }), + ]); + expect(where.f0).toBe("((toFloat64(duration) > toFloat64('10')))"); + }); +}); + +/** Blank out everything inside parentheses so only top-level text is left. */ +function topLevel(sql: string): string { + let depth = 0; + let out = ''; + for (const char of sql) { + if (char === '(') { + depth += 1; + continue; + } + if (char === ')') { + depth -= 1; + continue; + } + if (depth === 0) { + out += char; + } + } + return out; +}