From 4a2595d9f7aa2e3eb42cd4f6723d13cdb2546fb5 Mon Sep 17 00:00:00 2001 From: BilalG1 Date: Fri, 24 Apr 2026 12:07:16 -0700 Subject: [PATCH] Classify ClickHouse NO_COMMON_TYPE (386) as unsafe (#1380) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Add ClickHouse error code `386` (`NO_COMMON_TYPE`) to `UNSAFE_CLICKHOUSE_ERROR_CODES` in `apps/backend/src/lib/clickhouse-errors.ts`. This stops the Sentry `StackAssertionError` (`Unknown Clickhouse error: code 386 not in safe or unsafe codes`) that was firing whenever an admin wrote a query like `SELECT [1, 'a']` or `SELECT if(1, 'a', 1)`, while keeping the raw error message out of prod responses. - Add two e2e regression tests: one against the cross-project `analytics_internal.users` table, and one against `system.query_log`, to pin that 386 is wrapped with the generic `Error during execution of this query.` message in prod (full detail only surfaces in dev/test). ## Why unsafe, not safe Both callers of `getSafeClickhouseErrorMessage` (`apps/backend/src/app/api/latest/internal/analytics/query/route.ts:59` and `apps/backend/src/lib/ai/tools/sql-query.ts:80`) execute caller-authored SQL under `readonly: "1"` with `SQL_project_id`/`SQL_branch_id` scoping. The ClickHouse client runs under a `limited_user` whose grants restrict most tables — but ClickHouse resolves types **before** enforcing ACL. That means a query like `SELECT if(1, query, 1) FROM system.query_log` surfaces code 386 with a message like `There is no supertype for types String, UInt8 ...`, leaking that `system.query_log.query` is a `String` — schema info from a table the caller can't actually read. This is the same type-before-ACL class as code 43 (`ILLEGAL_TYPE_OF_ARGUMENT`), which is already classified unsafe. Classifying 386 as unsafe keeps the defense-in-depth consistent: if per-customer tables are ever introduced and grants don't block reference-resolution in time, 386 won't leak their schema. Cost: in prod, an admin writing a malformed type-mismatch query sees only `Error during execution of this query.` instead of the supertype hint. Dev and test environments still show the full error via the existing `getNodeEnvironment()` branch, so local iteration is unaffected. ## Test plan - [x] `pnpm test run apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts` — all 64 tests pass, including the two 386 regression tests. - [ ] Monitor Sentry after deploy to confirm the `unknown-clickhouse-error-for-query` events for code 386 stop firing. ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of a ClickHouse type-mismatch error to prevent exposure of sensitive data and ensure sanitized error responses. * **Tests** * Added regression tests that verify error responses are sanitized, return consistent error codes, and include expected headers without leaking internal details. --- apps/backend/src/lib/clickhouse-errors.ts | 1 + .../endpoints/api/v1/analytics-query.test.ts | 69 +++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/apps/backend/src/lib/clickhouse-errors.ts b/apps/backend/src/lib/clickhouse-errors.ts index 60e15398b..dbac2f286 100644 --- a/apps/backend/src/lib/clickhouse-errors.ts +++ b/apps/backend/src/lib/clickhouse-errors.ts @@ -15,6 +15,7 @@ const UNSAFE_CLICKHOUSE_ERROR_CODES = [ 43, // ILLEGAL_TYPE_OF_ARGUMENT 47, // UNKNOWN_IDENTIFIER 60, // UNKNOWN_TABLE + 386, // NO_COMMON_TYPE 497, // ACCESS_DENIED ]; diff --git a/apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts b/apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts index afcdc9324..7776140d4 100644 --- a/apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts +++ b/apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts @@ -232,6 +232,41 @@ it("handles invalid SQL query", async ({ expect }) => { `); }); +it("does not leak data from the internal cross-project users table via type-mismatch errors", async ({ expect }) => { + const response = await runQuery({ + query: "SELECT if(1, primary_email, 1) AS leaked FROM analytics_internal.users LIMIT 1", + }); + + expect(response.status).toBe(400); + const errorText = JSON.stringify(response.body); + expect(errorText).not.toContain("@"); + expect(errorText).not.toMatch(/primary_email\s*[:=]\s*['"]/); + expect(stripQueryId(response, expect)).toMatchInlineSnapshot(` + NiceResponse { + "status": 400, + "body": { + "code": "ANALYTICS_QUERY_ERROR", + "details": { + "error": deindent\` + Error during execution of this query. + + As you are in development mode, you can see the full error: 386 There is no supertype for types String, UInt8 because some of them are String\\\\/FixedString\\\\/Enum and some of them are not: In scope SELECT if(1, primary_email, 1) AS leaked FROM analytics_internal.users LIMIT 1. + \`, + }, + "error": deindent\` + Error during execution of this query. + + As you are in development mode, you can see the full error: 386 There is no supertype for types String, UInt8 because some of them are String\\\\/FixedString\\\\/Enum and some of them are not: In scope SELECT if(1, primary_email, 1) AS leaked FROM analytics_internal.users LIMIT 1. + \`, + }, + "headers": Headers { + "x-stack-known-error": "ANALYTICS_QUERY_ERROR", +