Skip to content

Commit 4a2595d

Browse files
authored
Classify ClickHouse NO_COMMON_TYPE (386) as unsafe (#1380)
## 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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent cbd945e commit 4a2595d

2 files changed

Lines changed: 70 additions & 0 deletions

File tree

apps/backend/src/lib/clickhouse-errors.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ const UNSAFE_CLICKHOUSE_ERROR_CODES = [
1515
43, // ILLEGAL_TYPE_OF_ARGUMENT
1616
47, // UNKNOWN_IDENTIFIER
1717
60, // UNKNOWN_TABLE
18+
386, // NO_COMMON_TYPE
1819
497, // ACCESS_DENIED
1920
];
2021

apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,41 @@ it("handles invalid SQL query", async ({ expect }) => {
232232
`);
233233
});
234234

235+
it("does not leak data from the internal cross-project users table via type-mismatch errors", async ({ expect }) => {
236+
const response = await runQuery({
237+
query: "SELECT if(1, primary_email, 1) AS leaked FROM analytics_internal.users LIMIT 1",
238+
});
239+
240+
expect(response.status).toBe(400);
241+
const errorText = JSON.stringify(response.body);
242+
expect(errorText).not.toContain("@");
243+
expect(errorText).not.toMatch(/primary_email\s*[:=]\s*['"]/);
244+
expect(stripQueryId(response, expect)).toMatchInlineSnapshot(`
245+
NiceResponse {
246+
"status": 400,
247+
"body": {
248+
"code": "ANALYTICS_QUERY_ERROR",
249+
"details": {
250+
"error": deindent\`
251+
Error during execution of this query.
252+
253+
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.
254+
\`,
255+
},
256+
"error": deindent\`
257+
Error during execution of this query.
258+
259+
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.
260+
\`,
261+
},
262+
"headers": Headers {
263+
"x-stack-known-error": "ANALYTICS_QUERY_ERROR",
264+
<some fields may have been hidden>,
265+
},
266+
}
267+
`);
268+
});
269+
235270
it("can execute query returning multiple rows", async ({ expect }) => {
236271
const response = await runQuery({ query: "SELECT arrayJoin([0, 1, 2]) AS number" });
237272

@@ -1658,6 +1693,40 @@ it("does not leak column names from restricted tables via illegal type of argume
16581693
`);
16591694
});
16601695

1696+
it("does not leak data from restricted tables via type-mismatch errors (code 386)", async ({ expect }) => {
1697+
// ClickHouse resolves types before checking permissions, so a code 386
1698+
// referencing a restricted table column would otherwise leak its type.
1699+
// 386 is classified unsafe so the raw message must not reach prod.
1700+
const response = await runQuery({
1701+
query: "SELECT if(1, query, 1) FROM system.query_log",
1702+
});
1703+
1704+
expect(stripQueryId(response, expect)).toMatchInlineSnapshot(`
1705+
NiceResponse {
1706+
"status": 400,
1707+
"body": {
1708+
"code": "ANALYTICS_QUERY_ERROR",
1709+
"details": {
1710+
"error": deindent\`
1711+
Error during execution of this query.
1712+
1713+
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, query, 1) FROM system.query_log.
1714+
\`,
1715+
},
1716+
"error": deindent\`
1717+
Error during execution of this query.
1718+
1719+
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, query, 1) FROM system.query_log.
1720+
\`,
1721+
},
1722+
"headers": Headers {
1723+
"x-stack-known-error": "ANALYTICS_QUERY_ERROR",
1724+
<some fields may have been hidden>,
1725+
},
1726+
}
1727+
`);
1728+
});
1729+
16611730
it("does not leak column names from restricted tables via unknown identifier (code 47)", async ({ expect }) => {
16621731
// ClickHouse resolves identifiers before checking permissions, and suggests
16631732
// real column names ("Maybe you meant: ..."), so code 47 must be unsafe

0 commit comments

Comments
 (0)