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

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

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.