Escape property keys in ClickHouse Map access expressions

[lindesvard]

What changed

A property path (properties.foo, profile.properties.plan) reaches the SQL builders as free text: a filter name, a breakdown name, or a math-metric property, all of which come from a saved report or an API call. getSelectPropertyKey turned that path into a Map access by concatenating the key between literal quotes:

return `${aliasPrefix}${match}['${property.replace(new RegExp(`^${match}.`), '')}']`;

A key containing a single quote therefore stopped being one Map access. It closed the string literal, and the remainder of the key was parsed as SQL, sitting in the same WHERE list as the project_id predicate that scopes the query to one project. A key like x'] = '' OR 1 = 1 OR properties['y produced a valid query with an extra boolean operator in it.

Three changes:

1. getSelectPropertyKey builds the non-wildcard Map access with sqlstring.escape, the same way the wildcard branch and the group/profile helpers next to it already do. Ordinary keys render byte-for-byte as before; keys with a quote or backslash now stay inside the literal.
2. rewriteProfilePropertyRefs searches for the reference text through a shared profilePropertyRef helper, so it keeps matching what getSelectPropertyKey emits. Without this, a narrowed key with a quote would have kept a reference to a Map the profile CTE no longer selects.
3. getWhere in the SQL builder parenthesises each clause before joining with AND. Previously a clause holding a top-level OR changed how its neighbours grouped.

Evidence

• packages/db/src/services/chart.service.ts:382 — the unescaped render (before this change).
• packages/db/src/services/chart.service.ts:260, :280, :314, :377 — every other Map render in the same file goes through sqlstring.escape; so does the filter side at packages/db/src/services/filter-where.service.ts:192 and :211.
• Callers that inline a caller-supplied name through that render: breakdowns at packages/db/src/services/chart.service.ts:756 and :1098, math metrics at :786 and :1135, and filters at :1189 (getEventFiltersWhereClause), which the event list and event count queries use at packages/db/src/services/event.service.ts:682 and :766.
• packages/db/src/services/chart.service.ts:445 — rewriteProfilePropertyRefs matched the exact text profile.properties['<key>'].
• packages/db/src/sql-builder.ts:36 — getWhere joined clauses with AND and no parentheses.

Tests

New packages/db/src/services/property-key-escaping.test.ts (string assertions only, no ClickHouse needed):

• getSelectPropertyKey renders a key containing a quote, a backslash and a ] as one escaped literal, and renders ordinary keys (properties.foo, the e.-qualified form, profile.properties.plan, the properties.a.* wildcard, plain columns) exactly as before.
• Chart SQL for a hostile filter name, breakdown name and math-metric property, plus the aggregate chart, keeps exactly one project_id = predicate and the payload stays inside the key literal. Same assertion for the event list and event count queries, captured through a mocked chQuery.
• Profile narrowing with a quoted key yields a CTE column and a matching rewritten reference; a key with a backslash still falls back to the full Map; ordinary keys narrow unchanged.
• getWhere parenthesises its clauses.

All ten of the injection assertions fail on the current code and pass with the change.

One existing assertion moved: packages/db/src/services/funnel-sql.test.ts pinned the funnel step pre-filter text, which now carries the extra parentheses from getWhere. The SQL means the same thing; the neighbouring assertion that counts each complete step condition twice is untouched.

Deliberately left out

• getAggregateChartSql and getChartSql build the one_event_per_user subquery with join(sb.where, ' AND ') directly (chart.service.ts:806, :1155), which still has no parentheses. No builder emits an unparenthesised top-level OR today: contains, regex, isNull and the group branch all wrap their own clauses. Changing those two call sites is a wider edit than this needed.
• transformPropertyKey's non-wildcard return (chart.service.ts:245) still concatenates quotes, but its only caller reaches it on the wildcard path and wraps the result in sqlstring.escape. Left alone.
• The narrowing guard that skips keys with a backtick or backslash is unchanged; those keys keep the full-Map fallback.
• No reformatting. biome check reports pre-existing complaints in these files (12 in chart.service.ts, 2 in sql-builder.ts before and after this change); the new test file follows the style of its neighbours rather than the formatter's output.

Checks

• vitest run over packages/db/src/services — 168 passed, 25 skipped. retention.service.test.ts fails in this environment because it needs a reachable ClickHouse; it fails identically without this change.
• Full workspace run produces the same set of failing files with and without the change (all of them need ClickHouse, Redis or Postgres, none of which run here).
• tsc --noEmit in packages/db — 26 errors, all pre-existing, in notification.service.ts, organization.service.ts and insights/store.ts. None in the touched files.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of property keys containing quotes or backslashes in generated queries.
  • Prevented special characters in property keys from affecting project-scoping filters.
  • Corrected filter grouping to ensure conditions containing OR do not broaden results unexpectedly.
  • Updated funnel filtering to preserve correct condition grouping.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: f2f33be4-b62a-4607-bd7b-ac514cde15f0

📥 Commits

Reviewing files that changed from the base of the PR and between 2d4f21e and b36753f.

📒 Files selected for processing (4)

• packages/db/src/services/chart.service.ts
• packages/db/src/services/funnel-sql.test.ts
• packages/db/src/services/property-key-escaping.test.ts
• packages/db/src/sql-builder.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review.

---

📝 Walkthrough

Walkthrough

The change escapes property keys in ClickHouse Map-access expressions, centralizes profile-property references, parenthesizes individual WHERE clauses, and adds coverage for chart, event, aggregate, profile, and funnel SQL generation.

Changes

SQL safety changes

Layer / File(s)	Summary
SQL generation updates packages/db/src/services/chart.service.ts, packages/db/src/sql-builder.ts	Property keys use sqlstring.escape in Map-access expressions. Profile-property rewriting uses the same escaped reference format. WHERE clauses are parenthesized before they are joined with AND.
SQL generation coverage packages/db/src/services/property-key-escaping.test.ts, packages/db/src/services/funnel-sql.test.ts	Tests cover escaped keys, grouped predicates, chart and event SQL, profile-property narrowing, and the updated funnel pre-filter SQL.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to b3675

This change keeps quoted and backslash-containing property keys inside SQL Map literals and preserves predicate grouping, with coverage for affected chart, event, profile, and funnel queries. No merge-blocking risk remains.

Suggested reviewers: niajkitir

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: escaping property keys in ClickHouse Map access expressions.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch agent/escape-property-map-keys

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[lindesvard]

Thanks for flagging this — the property-key handling that let a query cross a project's boundary is fixed. That's in PR #483, which just merged and will go out with the next release, probably within a day or so.