security(query): RLS predicate is spliced into rendered SQL by substring match — a crafted alias deletes it (and max_rows with it)

[EricAndrechek]

Area: query · policy — security (predicate-injection fragility) · found via pre-launch audit

Expected: the RLS predicate is emitted as part of the query's structure, not spliced into rendered SQL.

Actual: InjectPermissionFilters splices the predicate into already-rendered SQL by first-substring match (on WHERE, with the insert point located via GROUP BY/ORDER BY/LIMIT). A caller-controlled identifier containing one of those keywords captures the splice.

Impact — corrected 2026-08-12 (see the comment below): not "misplaces the predicate" and not latent. A crafted aggregation alias deletes the row filter, and the resulting query is valid SQL that returns the whole table. Verified end-to-end against ClickHouse. The original "identifiers are gated elsewhere on the structured path" premise is false — aggregation aliases and non-schema ORDER BY references reach the SQL with only a ? check.

Scope

The fix is one refactor with one principle: Build already receives perms — let Build use it, and delete everything that edits its output afterward.

internal/api/structured_query.go:117-147 currently reads:

result, err := query.Build(table, &sq, schema, perms, h.BucketSecs, h.defaultMaxRows)  // perms goes in
query.InjectPermissionFilters(result, perms.WhereClause, perms.WhereParams)            // reads perms, edits text
if perms.MaxRows > 0 { query.ApplyMaxRows(result, perms.MaxRows) }                     // reads perms, edits text

Both post-processors pull fields off the same perms that Build was already handed, then text-edit the SQL it just produced. Both then hit the same class of bug.

• Emit the policy predicate from Build. Append perms.WhereClause to whereParts in the WHERE assembly at internal/query/builder.go:95-102. Mind param ordering: the predicate's placeholders must line up relative to SELECT-list placeholders (time bucketing) and the caller's own filters.
• Emit the policy row cap from Build. Fold perms.MaxRows into the maxRows computation at builder.go:130-138 before the LIMIT is written. This is behavior-preserving: today you get min(q.Limit, defaultMaxRows) from Build and then ApplyMaxRows lowers it to perms.MaxRows if smaller — identical to min(q.Limit, defaultMaxRows, perms.MaxRows).
• Delete InjectPermissionFilters, findInsertPoint, ApplyMaxRows, and spliceKeywordRe (plus the two guard call sites at builder.go:259 and :283).
• Restore unrestricted identifiers and revert the identifier-contract caveat that the guard forced into the docs.

What this closes

Defect	Where
Full-table RLS bypass via crafted alias	InjectPermissionFilters / findInsertPoint
U+0131 bypass of the interim guard	spliceKeywordRe vs strings.ToUpper disagreement
Guard false positives ("Total order by region" → 400)	spliceKeywordRe over-inclusive
Unguarded identifier sources (schema columns, table name; ORDER BY check sits only in the validateColumn-failure branch)	guard is at the wrong layer
Malformed SQL / caller-triggerable 500 from byte-offset drift	findInsertPoint:534
Role max_rows cap silently no-ops	ApplyMaxRows:164

The max_rows defect, since it's easy to miss

ApplyMaxRows uppercases the SQL to locate " LIMIT ", then indexes into the original string with that offset. strings.ToUpper is not length-preserving in UTF-8 (31 runes change byte length), so a column named ıı makes strconv.Atoi receive "T 10000", the parse fails, and the policy's row cap is silently not applied. A policy control failing open, unrelated to the splice — and the one defect here that would survive a fix scoped only to InjectPermissionFilters. That is the reason it is folded into this issue rather than filed separately: splitting them invites a fix that lands the splice refactor and leaves ApplyMaxRows behind.

max_result_rows (internal/clickhouse/ch_settings.go:49) is the surviving backstop, and it does not stop a groupArray exfiltration, which returns a single row.

Related

• fix(policy): fail closed on unresolvable row-filter claims #457 — made 1 = 0 reachable for every unresolvable _eq/_neq/_gt/_lt (previously only _in), which is what turned this from theoretical into exploitable; it also carries the interim spliceKeywordRe guard this issue removes.
• refactor(api): split internal/api/ into focused subpackages #146 (api refactor), perf(cache): structured-query cache key is non-deterministic for multi-column policy filters #261 (cache-key determinism for multi-column policy filters).

Implementation notes

Everything below was established while reviewing #457. It is here so this issue can be picked up cold.

Param ordering is not a risk — params is empty when the WHERE is assembled

The scope box above says "mind param ordering". It was traced, and the answer is that there is nothing to be careful about:

var params []any is declared at the top of Build, and nothing appends to it before builder.go:99 (params = append(params, whereParams...)). Building the row projection and the aggregations only appends to selectParts. Every bound value in the query — including the time-range and bucket params at builder.go:348/:358/:366 — is produced inside buildWhere.

So params is empty at the point the WHERE clause is assembled. Put the policy predicate first in whereParts and its params first in params, which reproduces exactly the order InjectPermissionFilters produces today (result.Params = append(whereParams, result.Params...)). No SELECT-list placeholder can be shifted, because there aren't any.

Blast radius

InjectPermissionFilters and ApplyMaxRows have exactly one call site each, both in internal/api/structured_query.go (:141 and :145). findInsertPoint and spliceKeywordRe are used only inside internal/query/builder.go. The deletion is contained to those two files plus tests.

Why the interim guard is being deleted rather than fixed

spliceKeywordRe (added in #457 as a stopgap) is bypassable. Go's regexp (?i) uses simple case folding, which does not fold ı (U+0131) to i. findInsertPoint uppercases with strings.ToUpper, which does map ı to I. So an alias of e lımıt z passes the guard and still reads as " LIMIT " to the splice:

alias "e limit z"  -> rejected by the guard
alias "e lımıt z"  -> accepted; Build + InjectPermissionFilters emit:
  SELECT groupArray(`email`) AS `e WHERE 1 = 0 lımıt z` FROM `clicks` LIMIT 10000

Executed on clickhouse local against a three-tenant table that returns every tenant's rows, where the correctly-formed query returns none. A differential fuzz over 400k aliases found 227 evading variants, 209 of which executed with a 100% leak rate. Exactly two runes in Unicode uppercase to ASCII (ı→I, ſ→S) and only LIMIT contains an I, so it is a single-rune hole — but one is enough.

The guard is also over-inclusive: it rejects "Total order by region", lowercase " where " (which the byte-exact strings.Contains splice never matched), and keywords padded with tabs. Being wrong in both directions is the evidence it approximates the splice rather than matching it, which is the argument for removing the mechanism instead of tuning the regex.

Test design — the existing tests cannot catch this class

This is the part most likely to be got wrong. TestBuild_RejectsSpliceKeywordAlias asserts that Build returns an error. That shape of assertion can only ever test the guard, never the property the guard exists to protect — which is why the U+0131 case slipped through a green suite.

The regression test must assert the outcome: given a hostile alias and a role whose filter fails closed, the final SQL still contains the policy predicate as a real predicate. Include a ı case explicitly.

Tests to delete with the guard:

• TestBuild_RejectsSpliceKeywordAlias
• TestBuild_RejectsSpliceKeywordOrderRef

Tests that must still pass unchanged (they assert quoting containment, not rejection):

• TestBuild_AggregationAliasQuotedAndContained
• TestIntegration_AliasInjectionContained

Also add a max_rows case: a column named ıı currently makes ApplyMaxRows silently skip the cap. After the refactor the emitted LIMIT should be min(q.Limit, defaultMaxRows, perms.MaxRows) regardless of identifier content.

Verification recipe

All three defects here were confirmed this way, and it is the fastest way to prove the fix:

1. Construct a policy whose filter references an unresolvable claim, so Evaluate yields WhereClause == "1 = 0".
2. Run the real pipeline — policy.Evaluate → query.Build → (today) query.InjectPermissionFilters — and print the SQL.
3. Execute that SQL against clickhouse local on a small multi-tenant table.

A leak shows as rows from every tenant; a correct result is empty. The control case is worth keeping: with the claim present, the predicate contains backticks, which break the quoted alias and make ClickHouse return SYNTAX_ERROR — that asymmetry is why only the fail-closed predicate is exploitable.

Cache keys are unaffected

queryCacheKey(result.SQL, result.Params) is computed at internal/api/structured_query.go:149, after Build returns. Moving the predicate inside Build does not change what is hashed or when. (#261 is adjacent but independent: resolveFilters iterates a map, so a multi-column policy filter produces a non-deterministic clause order and therefore a non-deterministic cache key. That is not made better or worse by this change.)

Sequencing

This work sits on top of #457, which is where spliceKeywordRe and the extra 1 = 0 cases come from. Land #457 first, then this.

One conditional cleanup: if #457 ends up documenting the guard's identifier restriction in docs/src/content/docs/api.md:492 and docs/src/content/docs/architecture.md:153, this issue must revert that — deleting the guard restores the original contract ("any ClickHouse-legal name is accepted; the one exception is a name containing ?"). If #457 does not add that caveat, there is nothing to do in those files.

---

From WaveHouse-Stats pre-launch security audit (WAVEHOUSE-FEEDBACK.md dogfooding), audited dev 60fed15 (2026-06-10). Filed via /pm-triage. Impact corrected and scope expanded 2026-08-12, implementation notes added 2026-08-13, during the #457 review.

[EricAndrechek]

This is not latent, and the impact is understated. Please re-prioritize off Backlog. Verified end-to-end during the #457 review — policy → query.Build → InjectPermissionFilters → executed against clickhouse local.

The premise is false

Latent (identifiers are gated elsewhere on the structured path)

Identifiers are not gated on the structured path. Two caller-controlled inputs reach the emitted SQL with only a ? check:

• Aggregation alias — internal/query/builder.go:237-241 checks chsql.BindUnsafe(a.Alias) and nothing else. The comment there reasons "the alias is backtick-quoted by aggregationExpr, so any legal ClickHouse name is safe" — correct for injection, but the quoting happens before the string-splice, and strings.Replace doesn't know which bytes are inside quotes.
• ORDER BY on a non-schema column — builder.go:253-262 deliberately allows any unrecognized column through as an "alias reference," again with only a BindUnsafe check.

The impact is a complete row-filter bypass, not a misplacement

a crafted alias or column name containing WHERE or LIMIT shifts the match, misplacing the injected predicate

It doesn't misplace the predicate — it deletes it. The predicate lands inside the backtick-quoted alias, and the query executes with no WHERE clause:

POST /v1/query?table=clicks
{"columns":["user_id"],"group_by":["user_id"],
 "aggregations":[{"fn":"any","column":"email","alias":"e WHERE z"}]}

SELECT `user_id`, any(`email`) AS `e WHERE (1 = 0) AND z` FROM `clicks` GROUP BY `user_id` LIMIT 10000

Against a 3-tenant table that returns every tenant's rows; the correctly-formed query returns none. /v1/query is not admin-gated (internal/api/router.go:146-148), so any role with a select entry on the table can reach it. Column allow/deny still applies — this is a pure row bypass.

The detail that decides exploitability

Only a predicate containing no backtick survives inside a quoted alias as valid SQL. Every ordinary predicate (`tenant_id` = ?) injects backticks that terminate the identifier → SYNTAX_ERROR, no bypass. So the attack works exactly when a policy filter fails closed, because 1 = 0 is the only backtick-free WhereClause the engine emits.

Control cases:

claim present + evil alias  -> AS `x WHERE (`tenant_id` = ?) AND y`  -> SYNTAX_ERROR (500). No bypass.
claim absent  + evil alias  -> valid SQL, filter gone.               -> full table.

That coupling raises the priority beyond this issue's own merits: fail-closed predicates are becoming more common, not less. #358 introduced 1 = 0 for _in; #457 extends it to _eq/_neq/_gt/_lt on an unresolvable claim. Each such change widens this issue's reachable surface. For _eq specifically, #457 converts what was a bounded leak (tenant_id = '', empty-tenant rows only) into an unbounded one.

Suggested handling

• Short term (proposed in fix(policy): fail closed on unresolvable row-filter claims #457): reject an alias or ORDER BY alias-reference matching (?i)\s(where|group by|order by|limit)\s, next to the existing BindUnsafe call. ~5 lines, no product decision.
• Proper fix (this issue's stated scope): emit the predicate structurally from Build instead of splicing rendered SQL. That also resolves the findInsertPoint variant at builder.go:509-517, which has the same shape for the no-WHERE branch.

[EricAndrechek]

Severity correction — this issue (and my earlier comment) understates it in one specific way.

The write-up frames "Total order by region" as a false positive of the interim guard, i.e. a harmless alias getting a needless 400. On main, where there is no guard, that same alias is a full leak. findInsertPoint uppercases before matching, so an alias containing order by, group by, or limit in any casing captures the splice. Verified against current main (ea4fbf6):

alias "Total order by region"    -> SELECT groupArray(`email`) AS `Total WHERE 1 = 0 order by region` FROM `clicks` LIMIT 10000
alias "monthly count where paid" -> SELECT groupArray(`email`) AS `monthly count where paid` FROM `clicks` WHERE 1 = 0 LIMIT 10000

The first has no WHERE clause at all and returns every tenant. The second is fine, because the " WHERE " splice path uses a byte-exact strings.Contains and lowercase where does not match it — only the findInsertPoint path is case-insensitive.

Two consequences worth recording:

This is reachable by accident, not only by attack. "Total order by region" is an ordinary analyst label. Any deployment whose callers alias aggregations in natural language can trip it without anyone trying.

It is live on main today, independent of #457. 1 = 0 is already produced there by _in against an absent or empty claim (#224 behaviour), so any deployment using an _in row filter is exposed now. #457 widens the trigger from _in-only to every operator; it does not create it.

That moves this from "brittle by construction, latent" — the original framing — to a live, accidentally-reachable row-security bypass in shipped code. Worth reflecting in whatever priority this carries, and worth a line in the release notes when the fix ships with #457.

[EricAndrechek]

One more dimension, and it is the one that matters most for detection.

Both splice branches leak, and the strings.Replace(" WHERE ", …) branch is the nastier of the two. It replaces the first occurrence, and the SELECT list precedes the real WHERE — so a hostile alias captures the splice even when the caller supplies filters. Verified on main:

SELECT groupArray(`email`) AS `e WHERE (1 = 0) AND z` FROM `clicks` WHERE `page` = ? LIMIT 10000
params=[/home]

The query still carries a plausible WHERE \page` = ?. Nothing about it looks wrong in a log, in query_log`, or to anyone eyeballing slow queries — the row-security predicate is simply gone, hidden inside a column alias. The previously-recorded case at least produced a query with no WHERE clause, which is visibly missing something.

Taken with the earlier note that a benign analyst label like Total order by region triggers it, the accurate characterisation is:

• reachable without a crafted identifier — an ordinary aggregation alias is enough
• reachable with caller filters present, where the emitted SQL looks correctly scoped
• live on main today for any deployment using an _in row filter (Policy filter/check: _in accepted but never enforced; check honors only _eq #224 already emits 1 = 0 there)
• silent in both directions — no error, no anomalous SQL shape, no signal

There is also a non-leak variant with no hostile input at all: a legitimately-named column containing a length-changing rune (e.g. ıı) plus a role with max_rows drifts the byte offset into the table name, producing SYNTAX_ERROR (a caller-triggerable 500) and silently dropping the max_rows cap in the same query.

All of it is fixed by the structural change on row-filter-claim-templates; recording it because the issue body still describes the impact as "misplaces the predicate" and someone writing this up later should not have to re-derive how bad it actually was.