Area: query · policy — security (column-authorization bypass) · found via PR #457 review
Expected: the column allowlist is enforced on every column a query references, in every clause. docs/src/content/docs/access-control.mdx:177 states this explicitly, naming "an aggregation argument" among them, and promises 403 column "x" not allowed for a denied column named anywhere.
Actual: an aggregation whose argument is the literal * skips column authorization entirely, so a role can read — or infer — columns its policy denies.
The code
internal/query/builder.go:209-213:
for _, a := range q.Aggregations {
if a.Column != "*" { // <- exemption
if err := check(a.Column); err != nil {
return err
}
}
...
}check() is the allow/deny enforcement. The exemption exists because count(*) legitimately takes *, and * cannot be passed to check() — it isn't a schema column, so it would fail as "unknown column". The exemption is necessary; it is simply scoped to the wrong thing. It says "if the argument is *, skip authorization" where it should say "if the function is count, allow *", so it silently applies to every allowlisted function.
What it exposes
Two distinct capabilities, verified against ClickHouse 26.7.3 on a table (secret String, ts UInt64) with deny_columns: ["secret"]:
1. Direct value read — argMax(*) / argMin(*), exactly-2-column tables.
POST /v1/query {"aggregations":[{"fn":"argMax","column":"*","alias":"o"}]}
-> SELECT argMax(*) AS `o` FROM `events` LIMIT 1000 (Build accepts; no 403)
-> ClickHouse returns: SSN-222 <- the denied column's value, verbatim
argMin(*) -> SSN-111
Control: {"fn":"argMax","column":"secret"} is correctly rejected with column "secret" not allowed.
argMax takes exactly two arguments, so * expands cleanly only at exactly two columns; at three it errors with NUMBER_OF_ARGUMENTS_DOESNT_MATCH. That bounds this to narrow tables — but where it applies it is a plain read of denied data, not an inference.
2. Cardinality leak — uniq(*) / uniqExact(*), any table width. These are variadic, so they work regardless of shape, returning the distinct-row count across all columns including denied ones. This is the same inference channel the e2e suite already treats as a bug elsewhere — see the test named "filtering on a denied column is rejected (403), not an inference oracle" in tests/e2e/sdk/query.test.ts:366.
Every other allowlisted function is single-argument and errors on * (sum, avg, min, max, any, anyLast, groupArray), so the exposed set is argMax, argMin, uniq, uniqExact.
Both allow-list and deny-list are bypassed, since check() is skipped outright rather than partially applied.
Reachability
POST /v1/query is not admin-gated (internal/api/router.go:146-148), so any role with a select entry on the table can issue this. isValidAggFn allowlists all four functions, and IsAggregationAllowed only helps a deployment that has explicitly denied them.
Scope
- Gate the exemption on the function rather than the argument: only
count may take "*"; every other function's argument goes through check().
- Decide what a non-
count * should return — 400 (malformed) reads better than 403 (denied), since no specific column was named.
- Pin it with a test asserting the outcome — that a denied column's value cannot be reached — rather than only that
Build errors.
- Note this is a breaking change to the query API for any caller currently using
uniq(*)/argMax(*) legitimately. That cost is near zero today and grows after the first tagged release.
Related
Implementation notes
Added after the fact so this can be picked up cold.
Correction to the exposed set above
The original text named four functions. It is five — countDistinct is a ClickHouse alias for uniqExact and behaves identically. I had tested only 10 of the 19 allowlisted names; the complete result, verified against ClickHouse 26.7.3 on a 2-column table:
accepts * |
errors on * |
count (intended), argMax, argMin, uniq, uniqExact, countDistinct |
sum, avg, min, max, any, anyLast, groupArray, median, quantile, stddevPop, stddevSamp, varPop, varSamp |
Do not trust that table as permanent. It is ClickHouse's argument-arity behaviour, not ours — a future version, or a new entry in the allowlist, changes it silently. The fix should not depend on which functions currently tolerate *; gate on count and let everything else go through check(), which makes the arity question irrelevant.
The allowlist is isValidAggFn, internal/query/builder.go:479-500 (19 names).
Arity bound on the direct read
argMax/argMin take exactly two arguments, so * expands cleanly only when the table has exactly two columns; at three ClickHouse returns NUMBER_OF_ARGUMENTS_DOESNT_MATCH. That bounds the direct value read to two-column tables. uniq/uniqExact/countDistinct are variadic and work at any width, but leak cardinality rather than values.
Verification recipe
Both halves, and the way the finding was confirmed:
- Go side — build a
ResolvedPermissions{Allowed: true, DenyColumns: []string{"secret"}} and call query.Build with Aggregation{Fn: "argMax", Column: "*"}. It returns SQL with no error, while Column: "secret" returns column "secret" not allowed. That asymmetry is the bug in one assertion.
- ClickHouse side —
SELECT argMax(*) FROM (SELECT 'SSN-111' AS secret, 1 AS ts UNION ALL SELECT 'SSN-222', 2) returns SSN-222. Use the single-statement subquery form; a multi-statement CREATE TABLE … ; INSERT … ; SELECT … in one clickhouse local -q hangs.
Test inventory
Files touching aggregations, so you know what to update and what must not break:
internal/query/builder_test.go — the main Build coverageinternal/api/structured_query_test.go — handler levelinternal/api/pipes_test.go, internal/pipes/pipes_test.go — pipes reuse the buildertests/integration/identifier_roundtrip_test.go
count(*) must keep working — it is the reason the exemption exists. Pin the fix with a test asserting the outcome (a denied column's value cannot be reached through any aggregation) rather than only that Build returns an error; the same reasoning applied in #322, where mechanism-shaped assertions missed a whole bypass class.
SDK
clients/ts/src/types.ts:189 types the aggregation argument as column: string, so "*" is already permitted by the type and no type change is needed. If the behaviour narrows to count only, the doc comment on that field should say so.
Decision to make before coding
What a non-count * should return. 400 reads better than 403 — no specific column was named, so it is a malformed request rather than a denied one — but 403 is more consistent with the surrounding column-authorization failures. Pick one deliberately and document it in the api.md error table for POST /v1/query, which already lists the other policy 403s.
Found during the PR #457 review, by following the same thread as the isValidAggFn Unicode-fold fix ("what else reaches ClickHouse verbatim?"). Pre-existing and live on main; not introduced or widened by #457, which is why it is filed separately rather than folded in.
Area: query · policy — security (column-authorization bypass) · found via PR #457 review
Expected: the column allowlist is enforced on every column a query references, in every clause.
docs/src/content/docs/access-control.mdx:177states this explicitly, naming "an aggregation argument" among them, and promises403 column "x" not allowedfor a denied column named anywhere.Actual: an aggregation whose argument is the literal
*skips column authorization entirely, so a role can read — or infer — columns its policy denies.The code
internal/query/builder.go:209-213:check()is the allow/deny enforcement. The exemption exists becausecount(*)legitimately takes*, and*cannot be passed tocheck()— it isn't a schema column, so it would fail as "unknown column". The exemption is necessary; it is simply scoped to the wrong thing. It says "if the argument is*, skip authorization" where it should say "if the function iscount, allow*", so it silently applies to every allowlisted function.What it exposes
Two distinct capabilities, verified against ClickHouse 26.7.3 on a table
(secret String, ts UInt64)withdeny_columns: ["secret"]:1. Direct value read —
argMax(*)/argMin(*), exactly-2-column tables.Control:
{"fn":"argMax","column":"secret"}is correctly rejected withcolumn "secret" not allowed.argMaxtakes exactly two arguments, so*expands cleanly only at exactly two columns; at three it errors withNUMBER_OF_ARGUMENTS_DOESNT_MATCH. That bounds this to narrow tables — but where it applies it is a plain read of denied data, not an inference.2. Cardinality leak —
uniq(*)/uniqExact(*), any table width. These are variadic, so they work regardless of shape, returning the distinct-row count across all columns including denied ones. This is the same inference channel the e2e suite already treats as a bug elsewhere — see the test named "filtering on a denied column is rejected (403), not an inference oracle" intests/e2e/sdk/query.test.ts:366.Every other allowlisted function is single-argument and errors on
*(sum,avg,min,max,any,anyLast,groupArray), so the exposed set isargMax,argMin,uniq,uniqExact.Both allow-list and deny-list are bypassed, since
check()is skipped outright rather than partially applied.Reachability
POST /v1/queryis not admin-gated (internal/api/router.go:146-148), so any role with aselectentry on the table can issue this.isValidAggFnallowlists all four functions, andIsAggregationAllowedonly helps a deployment that has explicitly denied them.Scope
countmay take"*"; every other function's argument goes throughcheck().count*should return —400(malformed) reads better than403(denied), since no specific column was named.Builderrors.uniq(*)/argMax(*)legitimately. That cost is near zero today and grows after the first tagged release.Related
*, group_by, order_by, filters) #223 — the column allowlist hard cap this weakens.Implementation notes
Added after the fact so this can be picked up cold.
Correction to the exposed set above
The original text named four functions. It is five —
countDistinctis a ClickHouse alias foruniqExactand behaves identically. I had tested only 10 of the 19 allowlisted names; the complete result, verified against ClickHouse 26.7.3 on a 2-column table:**count(intended),argMax,argMin,uniq,uniqExact,countDistinctsum,avg,min,max,any,anyLast,groupArray,median,quantile,stddevPop,stddevSamp,varPop,varSampDo not trust that table as permanent. It is ClickHouse's argument-arity behaviour, not ours — a future version, or a new entry in the allowlist, changes it silently. The fix should not depend on which functions currently tolerate
*; gate oncountand let everything else go throughcheck(), which makes the arity question irrelevant.The allowlist is
isValidAggFn,internal/query/builder.go:479-500(19 names).Arity bound on the direct read
argMax/argMintake exactly two arguments, so*expands cleanly only when the table has exactly two columns; at three ClickHouse returnsNUMBER_OF_ARGUMENTS_DOESNT_MATCH. That bounds the direct value read to two-column tables.uniq/uniqExact/countDistinctare variadic and work at any width, but leak cardinality rather than values.Verification recipe
Both halves, and the way the finding was confirmed:
ResolvedPermissions{Allowed: true, DenyColumns: []string{"secret"}}and callquery.BuildwithAggregation{Fn: "argMax", Column: "*"}. It returns SQL with no error, whileColumn: "secret"returnscolumn "secret" not allowed. That asymmetry is the bug in one assertion.SELECT argMax(*) FROM (SELECT 'SSN-111' AS secret, 1 AS ts UNION ALL SELECT 'SSN-222', 2)returnsSSN-222. Use the single-statement subquery form; a multi-statementCREATE TABLE … ; INSERT … ; SELECT …in oneclickhouse local -qhangs.Test inventory
Files touching aggregations, so you know what to update and what must not break:
internal/query/builder_test.go— the mainBuildcoverageinternal/api/structured_query_test.go— handler levelinternal/api/pipes_test.go,internal/pipes/pipes_test.go— pipes reuse the buildertests/integration/identifier_roundtrip_test.gocount(*)must keep working — it is the reason the exemption exists. Pin the fix with a test asserting the outcome (a denied column's value cannot be reached through any aggregation) rather than only thatBuildreturns an error; the same reasoning applied in #322, where mechanism-shaped assertions missed a whole bypass class.SDK
clients/ts/src/types.ts:189types the aggregation argument ascolumn: string, so"*"is already permitted by the type and no type change is needed. If the behaviour narrows tocountonly, the doc comment on that field should say so.Decision to make before coding
What a non-
count*should return.400reads better than403— no specific column was named, so it is a malformed request rather than a denied one — but403is more consistent with the surrounding column-authorization failures. Pick one deliberately and document it in theapi.mderror table forPOST /v1/query, which already lists the other policy403s.Found during the PR #457 review, by following the same thread as the
isValidAggFnUnicode-fold fix ("what else reaches ClickHouse verbatim?"). Pre-existing and live onmain; not introduced or widened by #457, which is why it is filed separately rather than folded in.