fix(mapper): close inference gaps on AST fields of type-checked statements (CIP-3699) - #440
Conversation
…ments (CIP-3699) Several clauses on statements that pass requires_type_check escaped inference entirely, so encrypted columns in them were silently wrong (plaintext stored unencrypted, or operations running on raw ciphertext) instead of loudly rejected. Each gap is now either fully supported (bound + rewrite) or explicitly rejected: - ON CONFLICT DO UPDATE SET (supported): assignments are resolved against the target table like a plain UPDATE, so plaintext values on the conflict path are encrypted; the `excluded` pseudo-table is brought into scope so `excluded.<col>` carries the column's EQL type; comparisons in DO UPDATE ... WHERE are rewritten to terms. A conflict target naming an encrypted column is rejected (uniqueness would be judged on randomised ciphertext, so the conflict would never fire). The insert target relation is now named, so `t.col` resolves in RETURNING and DO UPDATE. - Window specifications (supported): WindowSpec now has its own InferType impl covering both inline OVER (...) and named WINDOW w AS (...) definitions — PARTITION BY keys get an Eq bound, ORDER BY keys an Ord bound, and RewriteEqlPartitionBy matches the WindowSpec node so named windows are rewritten too. RANGE frames with an offset over an encrypted sort key are rejected (no term supports the arithmetic); ROWS/GROUPS frames work. - Aggregate clauses: DISTINCT arguments get an Eq bound and count(DISTINCT enc) is rewritten to count the equality term (deterministic per plaintext); other aggregates with a DISTINCT encrypted argument are rejected, since the substitution would change their result. Argument-list ORDER BY keys get an Ord bound (the OrderByExpr rewrite already fires there). WITHIN GROUP (ORDER BY enc) is rejected — the aggregate's result is computed from the sort key, so a term rewrite would hand the client the term itself. - SELECT ... INTO with an encrypted column is rejected: the projection lands in a table the schema has never seen. Native-only projections pass through. - ORDER BY ALL (never produced by the Postgres dialect) is rejected rather than left unconstrained. Also rejected explicitly rather than ignored: MySQL INSERT ... SET and ON DUPLICATE KEY UPDATE, and unknown (non-exhaustive) OnInsert variants. CIP-3699
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Comment-only review.\n\nI did not find a blocking issue in the diff. The newly covered AST fields are either given explicit capability/type constraints and rewrites or rejected, and the tests cover the supported and rejected cases. The full PostgreSQL 14–17 matrix and performance check are green.
2 participants
Closes the fail-open gaps from CIP-3699: statements that pass
requires_type_checkhad clauses that escaped inference entirely, so encrypted columns in them were silently wrong — plaintext stored unencrypted, or operations executing on raw ciphertext — instead of loudly rejected. Every gap is now either fully supported (inference bound + transformation rewrite) or explicitly rejected.Gap by gap
1.
INSERT … ON CONFLICT DO UPDATE SET— supportedThe common upsert path. Previously the
DO UPDATEassignments were never resolved, soON CONFLICT (id) DO UPDATE SET enc = 'plaintext'stored plaintext in the encrypted column whenever the conflict fired.UPDATE … SET), so literals and params on the conflict path get the column's EQL type and are encrypted, with the existingAssignmentcast rule applying the full-payload domain cast.excludedpseudo-table is brought into scope with the target's projection, soSET enc = excluded.encandexcluded.<col>in theDO UPDATE … WHEREpredicate carry the column's encrypted type; predicate comparisons are rewritten to search terms by the existing rules.t.colresolves inRETURNING/DO UPDATE. Unqualified column references inDO UPDATEexpressions become ambiguous (target vsexcluded) — mirroring PostgreSQL's owncolumn reference is ambiguouserror there.ON CONFLICT (enc)) — a conflict only fires off a unique index, and uniqueness of the randomised ciphertext never collides, so the upsert would silently always insert.ON CONFLICT ON CONSTRAINT <name>is let through: the mapper has no constraint catalog to resolve the name against, and a constraint over an encrypted column is a schema-design error independent of any statement.Where:
packages/eql-mapper/src/inference/infer_type_impls/insert_statement.rs,packages/eql-mapper/src/importer.rs.2. Window
ORDER BYand frames — supported (frames partially rejected)row_number() OVER (ORDER BY enc)previously ordered on ciphertext. The window'sORDER BYkeys now get anOrdbound (theOrderByExprrewrite already fired inside window specs, so the missing piece was the capability check). Frame offsets are unified as native.RANGEframes with an offset over an encrypted sort key are rejected — offsetRANGEneedskey ± offsetarithmetic, which no term supports;ROWSandGROUPSframes work (the ordering term is deterministic, so peer groups are preserved).Where: new
packages/eql-mapper/src/inference/infer_type_impls/window_spec.rs.3. Named windows (
OVER w/WINDOW w AS (…)) — supportedInference and rewriting moved from the
Functionnode to theWindowSpecnode itself, which exists in both the inlineOVER (…)form and the namedWINDOW w AS (…)definition. Named windows now get the sameEqbound +eq_termrewrite onPARTITION BYandOrdbound onORDER BYas inline specs, checked once at the definition site;OVER wneeds nothing of its own. Therewrite_eql_partition_by.rsskip ofNamedWindowis gone (the rule now matchesWindowSpecnodes directly), so the transform no longer contradicts inference.Where:
window_spec.rs,packages/eql-mapper/src/transformation_rules/rewrite_eql_partition_by.rs,function.rs.4. Aggregate argument clauses — mixed
count(DISTINCT enc)— supported:Eqbound onDISTINCTarguments, and a new rule rewrites the argument toeql_v3.eq_term(enc). The equality term is deterministic per plaintext, so distinct terms = distinct plaintexts; previously the dedup ran on randomised payloads and the count silently equalled the row count. Sound forcountprecisely becausecountdiscards its argument values.DISTINCTencrypted argument — rejected at transform: the same substitution would change that aggregate's result (min(DISTINCT enc)would return the term, not the payload). Note: functions outside the EQL registry already force native arguments, so in practice this bitesmin/max.array_agg(x ORDER BY enc)— supported:Ordbound; the existingOrderByExprrewrite producesORDER BY eql_v3.ord_term(enc).WITHIN GROUP (ORDER BY enc)— rejected: ordered-set aggregates compute their result from the sort key, so a term rewrite would hand the client an opaque, undecryptable term. Native sort keys are unified as native (same closure as unknown-function arguments), so nothing escapes as an unresolved variable.Where:
function.rs, newpackages/eql-mapper/src/transformation_rules/rewrite_eql_aggregate_distinct.rs.5.
SELECT … INTO— rejected when encryptedThe projection lands in a table the encryption schema has never seen — unreachable ciphertext. Rejected when any projected column (including through wildcards) is encrypted; native-only
SELECT INTOpasses through unchanged.Where:
select.rs.6.
ORDER BY ALL— rejectedThe PostgreSQL dialect never parses it (DuckDB/ClickHouse syntax), so this is defense in depth on the AST shape: the
OrderByKind::Allarm now errors instead of silently matching nothing.Where:
query_statement.rs.Also closed while in there
MySQL INSERT … SET(theInsert.assignmentsfield),ON DUPLICATE KEY UPDATE, and unknown variants of the non-exhaustiveOnInsertenum are rejected explicitly instead of ignored.Notes for review
OrdimpliesEqinEqlTraits(equality falls back to the ordering term), so theEq-bound rejection tests use a storage-only domain (eql_v3_boolean).INSERTs (qualifiedt.colnow resolves where it previously errored); no existing tests changed behaviour.window_function_with_forward_reference,common_table_expressions) used a capability-lesssalary (EQL)column underwindow w AS (… order by salary …)— they only passed because the clause escaped inference. They now grantOrd.Testing
mise run check(fmt, clippy-D warnings, compile): passcargo test -p eql-mapper: pass — 154 tests (20 new: transformed-SQL assertions for every supported shape, explicit-error assertions for every rejected shape)mise run test:unit: 277/278 pass; the one failure isproxy::tests::init_zerokms_client_with_crn, which also fails on a clean checkout oforigin/mainin this environment (env-dependent ZeroKMS init test) — pre-existing, unrelated.mise run test:integration: not run — the suite tears down and rebuilds the shared Postgres containers and needs CipherStash credentials not available in the isolated worktree this was built in. Please treat integration coverage of the upsert path as unverified.CIP-3699