feat: client 0.38.0 migration — drop vendored stack-auth + EQL v2.3 mapper support (CIP-3233)#409
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates proxy encryption and SQL rewriting to use ChangesProxy EQL and index-aware rewrite migration
Vendored stack-auth cleanup
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…tack-auth patch (CIP-3233) Moves Proxy off the `vendor/stack-auth` `[patch.crates-io]` workaround and onto the current released cipherstash-client group, built against the fixed stack-auth. Background: 2.2.4 (PR #408) shipped the CIP-3233 access-key token-refresh fix via a vendored stack-auth patched on top of the 0.34.1-alpha.4 source. cipherstash-client 0.38.0 links stack-auth 0.38.0, which carries the same fix from crates.io, so the vendored copy and patch are no longer needed. Changes: - cipherstash-client / cipherstash-config / cts-common: 0.34.1-alpha.4 -> 0.38.0 (carries the API migration from PR #406's 0.37.0 upgrade; 0.37 -> 0.38 needed no further source changes) - Remove `[patch.crates-io] stack-auth = { path = "vendor/stack-auth" }`, the `exclude = ["vendor/stack-auth"]` workspace entry, and the vendor/stack-auth tree - stack-auth now resolves from crates.io (0.38.0); single version of the cipherstash-client group in the lock (zerokms-protocol 0.12.19) Verified: `cargo check --workspace`, `cargo clippy --workspace --all-targets`, and `cargo test --workspace --lib` (111 proxy unit tests) all pass. Integration tests need a live DB/ZeroKMS and were not run here.
freshtonic
force-pushed
the
james/cip-3233-proxy-drop-vendor-patch
branch
from
256ad08 to
2e11f69
Compare
freshtonic
changed the title
cipherstash-client 0.38.0 emits structured JSONB (SteVec) encrypted
values as {"k":"sv",...} without a top-level `c` field. The pinned EQL
2.3.0-pre.3 enforced a top-level `c` on every encrypted value via
eql_v2._encrypted_check_c, rejecting these payloads with EP0001
("Encrypted column missing ciphertext (c) field") and failing all
JSONB/SteVec integration tests.
EQL 2.3.1 relaxes the check to `(val ? 'c') OR (val ? 'sv')`, accepting
the new SteVec format.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cipherstash-proxy/src/postgresql/messages/bind.rs (1)
84-96: 🗄️ Data Integrity & Integration | 🟠 MajorConfirm EqlOutput serialization format mismatch for EQL 2.3.1
The workspace is pinned to
cipherstash-clientversion 0.38.0, whereEqlOutputserializes viaserde_json::to_valueinto a JSON object containing a top-levelc(ciphertext) field. The required EQL 2.3.1 format expects a structure without this top-levelcwrapper. As written,rewrite()writes the legacy payload, which will fail or corrupt data ineql_v2_encryptedcolumns expecting the new format. Update the serialization logic to produce the schema-free format expected by the EQL 2.3.1 backend, likely requiring manual construction of the JSON payload from theEqlOutputcomponents or updating the dependency if a fix is available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cipherstash-proxy/src/postgresql/messages/bind.rs` around lines 84 - 96, The bind::rewrite method is serializing EqlOutput with serde_json::to_value, which preserves the legacy top-level c wrapper and does not match the EQL 2.3.1 payload shape. Update rewrite() to emit the schema-free JSON expected by eql_v2_encrypted columns, either by manually building the payload from EqlOutput’s fields or by switching to a dependency/version that already produces the new format. Keep the change localized to bind::rewrite and the EqlOutput serialization path it uses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 46-48: The Cargo.toml dependency update points cipherstash-client,
cipherstash-config, and cts-common at unpublished 0.38.0 versions, which will
break dependency resolution. Revert these entries to a published crates.io
version or restore the [patch.crates-io] override to a valid local/git source
for stack-auth until those crates are available. Keep the existing dependency
names and patch configuration aligned so Cargo can resolve them successfully.
---
Outside diff comments:
In `@packages/cipherstash-proxy/src/postgresql/messages/bind.rs`:
- Around line 84-96: The bind::rewrite method is serializing EqlOutput with
serde_json::to_value, which preserves the legacy top-level c wrapper and does
not match the EQL 2.3.1 payload shape. Update rewrite() to emit the schema-free
JSON expected by eql_v2_encrypted columns, either by manually building the
payload from EqlOutput’s fields or by switching to a dependency/version that
already produces the new format. Keep the change localized to bind::rewrite and
the EqlOutput serialization path it uses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25c7c19a-7daf-43a1-9720-a75b783fae42
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockvendor/stack-auth/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
Cargo.tomlmise.local.example.tomlmise.tomlpackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/lib.rspackages/cipherstash-proxy/src/postgresql/backend.rspackages/cipherstash-proxy/src/postgresql/context/mod.rspackages/cipherstash-proxy/src/postgresql/frontend.rspackages/cipherstash-proxy/src/postgresql/messages/bind.rspackages/cipherstash-proxy/src/postgresql/messages/data_row.rspackages/cipherstash-proxy/src/proxy/encrypt_config/manager.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rsvendor/stack-auth/.gitignorevendor/stack-auth/Cargo.tomlvendor/stack-auth/LICENSEvendor/stack-auth/README.mdvendor/stack-auth/examples/auto_strategy.rsvendor/stack-auth/examples/device_code.rsvendor/stack-auth/src/access_key.rsvendor/stack-auth/src/access_key_refresher.rsvendor/stack-auth/src/access_key_strategy.rsvendor/stack-auth/src/auto_refresh.rsvendor/stack-auth/src/auto_strategy.rsvendor/stack-auth/src/device_client.rsvendor/stack-auth/src/device_code/mod.rsvendor/stack-auth/src/device_code/protocol.rsvendor/stack-auth/src/device_code/tests.rsvendor/stack-auth/src/lib.rsvendor/stack-auth/src/oauth_refresher.rsvendor/stack-auth/src/oauth_strategy.rsvendor/stack-auth/src/refresher.rsvendor/stack-auth/src/service_token.rsvendor/stack-auth/src/static_token_strategy.rsvendor/stack-auth/src/token.rs
💤 Files with no reviewable changes (22)
- vendor/stack-auth/src/static_token_strategy.rs
- vendor/stack-auth/src/device_code/tests.rs
- vendor/stack-auth/README.md
- vendor/stack-auth/src/auto_strategy.rs
- vendor/stack-auth/src/oauth_refresher.rs
- vendor/stack-auth/.gitignore
- vendor/stack-auth/src/access_key.rs
- vendor/stack-auth/Cargo.toml
- vendor/stack-auth/src/device_client.rs
- vendor/stack-auth/src/device_code/protocol.rs
- vendor/stack-auth/src/device_code/mod.rs
- vendor/stack-auth/src/refresher.rs
- vendor/stack-auth/src/service_token.rs
- vendor/stack-auth/src/oauth_strategy.rs
- vendor/stack-auth/src/access_key_refresher.rs
- vendor/stack-auth/src/access_key_strategy.rs
- vendor/stack-auth/examples/auto_strategy.rs
- vendor/stack-auth/LICENSE
- vendor/stack-auth/src/token.rs
- vendor/stack-auth/src/lib.rs
- vendor/stack-auth/src/auto_refresh.rs
- vendor/stack-auth/examples/device_code.rs
…nt 0.38.0 (CIP-3279)
cipherstash-client 0.38.0 stores jsonb leaves as STE-vec elements carrying
CLLW ORE (`oc`), and EQL 2.3.1's `->` returns `eql_v2.ste_vec_entry`. Bare
comparison/order on the extracted entry routed to the root Block-ORE path
(`eql_v2.compare` → `ore_block_u64_8_256`, requires `ob`) and failed with
`Expected an ore index (ob) value`. Field projection returned the raw
encrypted sv element instead of decrypting it.
Group B (range/order): new RewriteJsonbSteVecOrdering rule rewrites
`(col -> sel) <op> $param` into
`eql_v2.ore_cllw((col -> sel)::JSONB) <op> eql_v2.ore_cllw($param::JSONB)`,
with a new EqlTerm::SteVecTerm variant (bound Ord) and post-inference RHS
reclassification so the param/literal is encrypted as a CLLW STE-vec query
term (`oc`) via QueryOp::SteVecTerm.
Group C (field projection): data_row reshapes a bare ste_vec_entry jsonb
(text `{` and binary `0x01`) into a decryptable EqlCiphertext; from_sql
parses sv-entry scalars (numbers as f64 to match storage's orderable
encoding).
Fixes 20 integration tests (8 range/order + 12 field projection), verified
end-to-end against live proxy + PostgreSQL + ZeroKMS. jsonb sv equality
(select_where_jsonb_eq, jsonb_term_filter) remains a separate follow-up.
…dering (CIP-3283)
eql-mapper previously saw only abstract EqlTraits for an encrypted column,
never the concrete index type, so transformation rules could not target
index-specific SQL. Introduce a side-channel IndexResolver (TableColumn ->
{IndexKind}) consulted only by transformation rules — inference, unification,
and the EqlValue/EqlTerm output types are deliberately untouched, and the
crate takes no dependency on cipherstash-config. The proxy populates the
resolver from EncryptConfig (already keyed by table+column); an
EmptyIndexResolver reproduces prior behaviour as the safe default.
First consumer: RewriteScalarOpeOrdering rewrites scalar (non-jsonb) OPE
columns so PostgreSQL compares the order-preserving `op` ciphertext directly
with built-ins only:
col <op> $p -> decode(col->>'op','hex') <op> decode($p->>'op','hex')
ORDER BY col -> ORDER BY decode(col->>'op','hex') (ASC/DESC/NULLS kept)
ORE columns, equality, and jsonb sv accessors are left untouched.
This lets scalar OPE ordering work Proxy-side with NO EQL change (potentially
retiring CIP-3282), gated on the client storing/query-encoding `op` (CIP-3280).
Verified: cargo test -p eql-mapper (94 pass, +9), proxy lib (new tests pass;
config::tandem failures are a pre-existing parallelism flake), mise run check
clean. NOT yet code-reviewed (the review pass did not complete). e2e blocked
on the unpublished CIP-3280 client.
freshtonic
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cipherstash-proxy/src/postgresql/data/from_sql.rs (1)
325-352: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
eql_termin the binaryTEXT/VARCHARfallback.The new
SteVecTermJSON path is skipped when a driver sends the RHS as binaryTEXT/VARCHAR: this fallback still hard-codesEqlTermVariant::Full, so jsonb STE-vec ordering values get converted toPlaintext::Jsoninstead of the scalarPlaintextthatQueryOp::SteVecTermexpects.Suggested fix
- (_, _, &Type::TEXT | &Type::VARCHAR) => parse_bytes_from_sql::<String>(bytes, pg_type) - .and_then(|val| text_from_sql(&val, EqlTermVariant::Full, col_type)), + (eql_term, _, &Type::TEXT | &Type::VARCHAR) => { + parse_bytes_from_sql::<String>(bytes, pg_type) + .and_then(|val| text_from_sql(&val, eql_term, col_type)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cipherstash-proxy/src/postgresql/data/from_sql.rs` around lines 325 - 352, The TEXT/VARCHAR fallback in from_sql::from_sql is dropping the original eql_term by hard-coding EqlTermVariant::Full, which breaks the SteVecTerm JSON scalar path. Update that fallback to pass through the incoming eql_term instead of forcing Full, so QueryOp::SteVecTerm still reaches text_from_sql/json_scalar_to_plaintext with the correct variant. Keep the existing JSON/JSONB/BYTEA handling intact and make sure the fallback in the final &Type::TEXT | &Type::VARCHAR arm preserves the caller’s term for both binary and text driver inputs.
🧹 Nitpick comments (1)
packages/eql-mapper/src/lib.rs (1)
2333-2335: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact
Partialvariant here.This guard says root-scalar RHS params must remain
EqlTerm::Partial, but!matches!(SteVecTerm)would still pass forFullor another wrong EQL variant.Proposed test tightening
- assert!( - !matches!(value, Value::Eql(EqlTerm::SteVecTerm(_))), - "root-scalar ordering RHS must NOT be reclassified to SteVecTerm, got {value}" - ); + assert!( + matches!(value, Value::Eql(EqlTerm::Partial(_, _))), + "root-scalar ordering RHS must remain EqlTerm::Partial, got {value}" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/eql-mapper/src/lib.rs` around lines 2333 - 2335, The guard in the root-scalar ordering check is too loose because it only excludes Value::Eql(EqlTerm::SteVecTerm(_)) and would still allow other incorrect variants. Update the assertion in the relevant ordering/RHS validation path to match the exact expected EqlTerm::Partial shape, using the same value being checked there, so the logic fails for any non-Partial EQL term. Tighten the corresponding test around this root-scalar ordering case to verify the RHS remains Partial and does not accept Full or other variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cipherstash-proxy/src/proxy/encrypt_config/index_resolver.rs`:
- Around line 53-56: The config lookup is using a bare table name in index
resolution, which breaks matches for schema-qualified EncryptConfig keys. Update
the identifier construction in `IndexResolver` so `Identifier::new(...)` uses
the fully qualified table name for `TableColumn` lookup, either by
preserving/retrieving the schema when building `TableColumn` or by resolving it
at lookup time before matching against config entries. Ensure the fix is applied
in the `IndexResolver` path that currently reads `table_column.table.value` so
schema-qualified tables no longer fall back to the empty-set rewrite path.
In `@packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs`:
- Around line 205-214: The SteVecTerm mapping in the zero-kms EqlTermVariant
match currently falls back to Store when no SteVec index is found, which can
silently drop the STE-vec query term. Update the EqlTermVariant::SteVecTerm
branch in the zerokms mapper to fail fast or otherwise prevent this path from
returning EqlOperation::Store; keep the operation as a query only when an
IndexType::SteVec is present, and use the surrounding EqlOperation::Query /
QueryOp::SteVecTerm handling as the place to enforce the invariant.
In
`@packages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_ordering.rs`:
- Around line 191-197: The rewrite in would_edit currently gates on
self.is_eql_typed(right), which is too broad and allows non-SteVec encrypted
expressions to be rewritten incorrectly. Update the condition in
rewrite_jsonb_ste_vec_ordering::would_edit to require the RHS to be classified
as a SteVecTerm (using the existing term/classification helpers in this rule)
instead of any EQL-typed expression, so only single STE-vec comparison terms
trigger the jsonb accessor ordering rewrite.
In `@packages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rs`:
- Around line 173-185: The `would_edit_comparison` check in
`rewrite_scalar_ope_ordering.rs` is too permissive because it only verifies that
the RHS is EQL-typed, not that it is OPE-backed. Update the guard in
`would_edit_comparison` so the rewrite only applies when the right-hand
expression is confirmed to carry the needed OPE backing/`op` term, using the
same kind of resolver-based validation already used for `is_scalar_ope_column`
on the left. Keep the binary-operator and left-column checks intact, but add the
missing RHS-specific predicate before allowing the rewrite.
In `@packages/eql-mapper/src/type_checked_statement.rs`:
- Around line 167-170: Gate the JSONB STE-vec rewrite behind IndexResolver
checks in RewriteJsonbSteVecOrdering and the type_checked_statement wiring.
Update RewriteJsonbSteVecOrdering to accept the resolver alongside node_types,
and use the same resolve(table_column).contains(&IndexKind::SteVec) predicate
before rewriting JsonLike columns or classifying RHS values as SteVecTerm, so
columns without a configured STE-vec index do not get rewritten to
ore_cllw(...).
---
Outside diff comments:
In `@packages/cipherstash-proxy/src/postgresql/data/from_sql.rs`:
- Around line 325-352: The TEXT/VARCHAR fallback in from_sql::from_sql is
dropping the original eql_term by hard-coding EqlTermVariant::Full, which breaks
the SteVecTerm JSON scalar path. Update that fallback to pass through the
incoming eql_term instead of forcing Full, so QueryOp::SteVecTerm still reaches
text_from_sql/json_scalar_to_plaintext with the correct variant. Keep the
existing JSON/JSONB/BYTEA handling intact and make sure the fallback in the
final &Type::TEXT | &Type::VARCHAR arm preserves the caller’s term for both
binary and text driver inputs.
---
Nitpick comments:
In `@packages/eql-mapper/src/lib.rs`:
- Around line 2333-2335: The guard in the root-scalar ordering check is too
loose because it only excludes Value::Eql(EqlTerm::SteVecTerm(_)) and would
still allow other incorrect variants. Update the assertion in the relevant
ordering/RHS validation path to match the exact expected EqlTerm::Partial shape,
using the same value being checked there, so the logic fails for any non-Partial
EQL term. Tighten the corresponding test around this root-scalar ordering case
to verify the RHS remains Partial and does not accept Full or other variants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0ef3fd4-1fb6-44ea-aac1-a8954e8b56b6
📒 Files selected for processing (19)
packages/cipherstash-proxy/src/postgresql/context/mod.rspackages/cipherstash-proxy/src/postgresql/data/from_sql.rspackages/cipherstash-proxy/src/postgresql/frontend.rspackages/cipherstash-proxy/src/postgresql/messages/data_row.rspackages/cipherstash-proxy/src/proxy/encrypt_config/index_resolver.rspackages/cipherstash-proxy/src/proxy/encrypt_config/manager.rspackages/cipherstash-proxy/src/proxy/encrypt_config/mod.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rspackages/eql-mapper/src/eql_mapper.rspackages/eql-mapper/src/index_resolver.rspackages/eql-mapper/src/inference/unifier/eql_traits.rspackages/eql-mapper/src/inference/unifier/types.rspackages/eql-mapper/src/lib.rspackages/eql-mapper/src/ste_vec_ordering.rspackages/eql-mapper/src/transformation_rules/mod.rspackages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_ordering.rspackages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rspackages/eql-mapper/src/type_checked_statement.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs
- packages/cipherstash-proxy/src/postgresql/messages/data_row.rs
…IP-3283)
Address non-blocking review nits on RewriteScalarOpeOrdering:
- Add tests pinning behaviour for shapes around the rule's match surface:
- col BETWEEN $a AND $b (Expr::Between) is not rewritten
- MIN(col)/MAX(col) route to eql_v2 aggregates, not decode(op)
- $p < col (column on the right) IS rewritten symmetrically -- a bare
comparison param unifies to the same eql column, so the rule fires for
either operand ordering (documented in the test; not a coverage gap)
- Add a multi-key ORDER BY test asserting only the OPE sort key is
rewritten to decode(col->>'op','hex') with directions/NULLS preserved
- Tighten the rule rustdoc: mem::replace is the ownership mechanism that
moves the RHS node intact (so downstream cast rules re-encrypt it), not
the reason the op slot is preserved
- Leave a TODO(CIP-3280) for the NULL-invariant scalar-OPE e2e test, which
is blocked on the unpublished client that can carry op end-to-end
…P-3281)
Rewrites jsonb STE-vec equality comparisons so they bind to EQL 2.3.1's
XOR-aware eql_v2.eq_term extractor instead of root eql_v2_encrypted
equality. The LHS sv-element accessor (col -> sel, already an
eql_v2.ste_vec_entry) is wrapped in eql_v2.eq_term; the jsonb_path_query_first
form (an eql_v2_encrypted) is cast to ::JSONB::eql_v2.ste_vec_entry first.
The RHS query payload is a root sv payload ({k:sv, ..., hm|oc}) that lacks
the s/c fields the ste_vec_entry DOMAIN CHECK requires, so it cannot use the
documented ::eql_v2.ste_vec_entry recipe. Instead the eq_term body is inlined
on the raw jsonb: decode(coalesce(rhs ->> 'hm', rhs ->> 'oc'), 'hex'). This
coalesces whichever deterministic term the column leaf carries (oc for CLLW
ORE leaves, hm for hmac/term-filter leaves), so no proxy-side query-op change
is needed: the existing QueryOp::SteVecTerm path already emits the matching
term.
The RHS param/literal is reclassified to EqlTerm::SteVecTerm via the
generalised ste_vec_term_rhs_keys, which now collects both ordering and
equality RHS operands. Both rewrite rules and the reclassification gate on the
same shared predicates plus the shared rhs_as_jsonb operand helper, so they
cannot drift apart.
Verified: cargo test -p eql-mapper (100 passed), proxy lib (121 passed),
fmt/clippy -D warnings/check --locked clean. e2e (select_where_jsonb_eq,
jsonb_term_filter) compiles but was NOT run -- no live PG/ZeroKMS in sandbox.
Points the proxy at the published cipherstash-client 0.38.1-alpha.1 — a pre-release of the suite `fix/scalar-ope-oc-term` branch (CIP-3280) that stores + query-encodes the scalar OPE `op` term. This lets CI exercise the full integration suite, including scalar OPE ordering (map_ope_*), against a client that carries the op term. Temporary: revert to a stable published cipherstash-client before merge (once CIP-3280 / suite PR #2061 lands and a release ships).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/eql-mapper/src/ste_vec_ordering.rs`:
- Around line 97-144: The rhs_as_jsonb helper is based on an outdated cast shape
and now always falls back because RewriteJsonbSteVecOrdering runs before
CastParamsAsEncrypted and CastLiteralsAsEncrypted. Update rhs_as_jsonb to match
the actual transformer order by removing the special-case logic for stripping
eql_v2_encrypted and simplifying it to return the expression unchanged when it
is already a JSONB cast, otherwise wrap it in a JSONB cast. Keep the behavior
localized in rhs_as_jsonb so the STE-vec ordering rewrite remains aligned with
make_transformer execution.
In
`@packages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_equality.rs`:
- Around line 146-154: The RHS handling in build_eq_term_rhs is cloning the
expression before extracting hm and oc, which can break NodeKey identity and
prevent CastLiteralsAsEncrypted from rewriting encrypted literals. Update
rewrite_jsonb_ste_vec_equality::build_eq_term_rhs to follow the same
single-consumption pattern used by the ordering rule: derive the hm/oc path
without passing cloned Expr values into field_text, or extract the needed fields
before any clone so the original rhs node remains intact for the transformer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5929dbcc-b3e9-4b2f-871e-904dac54d064
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
CHANGELOG.mdCargo.tomlpackages/cipherstash-proxy-integration/src/map_ope_index_order.rspackages/cipherstash-proxy/src/postgresql/data/from_sql.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rspackages/eql-mapper/src/eql_mapper.rspackages/eql-mapper/src/lib.rspackages/eql-mapper/src/ste_vec_ordering.rspackages/eql-mapper/src/transformation_rules/mod.rspackages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_equality.rspackages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_ordering.rspackages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rspackages/eql-mapper/src/type_checked_statement.rs
✅ Files skipped from review due to trivial changes (3)
- CHANGELOG.md
- packages/cipherstash-proxy-integration/src/map_ope_index_order.rs
- packages/eql-mapper/src/transformation_rules/mod.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_ordering.rs
- packages/eql-mapper/src/type_checked_statement.rs
- packages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rs
- packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
- packages/cipherstash-proxy/src/postgresql/data/from_sql.rs
- packages/eql-mapper/src/eql_mapper.rs
- Cargo.toml
- packages/eql-mapper/src/lib.rs
…CIP-3283) The scalar OPE ordering/range rewrite emitted `decode(col ->> 'op','hex')`, but `eql_v2_encrypted` defines its own `->>` operator (a selector accessor, alias of `->`), so the bare `->>` bound to that instead of native jsonb extraction — PostgreSQL then tried to parse 'op' as a composite literal and raised `EP0001/22P02 malformed record literal: "op"` at runtime. Cast the operand to its base `jsonb` type first (`decode((col)::jsonb ->> 'op','hex')`) so native jsonb field extraction is used, mirroring the jsonb STE-vec rules. Unit tests asserted the SQL string so they passed while the SQL was invalid; only e2e (CI, PG14-17) caught it. Updated the rewrite tests to expect the `::JSONB` cast.
A jsonb STE-vec *term* comparison with a bare numeric literal on the right-hand side (e.g. `jsonb_path_query_first(pii, '$.path') > 70`) failed at runtime with a fatal `SteVecTerm only supports scalar values` error. The eql-mapper correctly reclassifies the literal to `EqlTerm::SteVecTerm`, but `literal_from_sql` converted bare numeric literals via the generic `Value::Number` arm, which ignores `eql_term` and (for a Json cast type) produced a non-scalar `Plaintext::Json`. The client's `QueryOp::SteVecTerm` rejects non-scalars. Parameters and quoted string literals were unaffected because they route through `text_from_sql`/`binary_from_sql`, which already reduce sv-term values to scalars. Add a guarded arm that maps a bare numeric sv-term literal to `Plaintext::Float` (f64), matching how cipherstash-client encodes stored jsonb numeric leaves so ordering/equality comparisons stay consistent. Verified end-to-end: the showcase Test 4 (high cardiovascular risk, `> 70`) now passes and the full showcase completes successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cipherstash-proxy/src/postgresql/data/from_sql.rs`:
- Around line 603-625: The SteVecTerm RHS collection in eql_mapper’s
reclassification loop only handles Expr::Value, so negative literals parsed as
Expr::UnaryOp are skipped. Update the matching logic to also recognize
Expr::UnaryOp with Minus and unwrap its inner Number or Placeholder before
inserting into the literals set. Keep the existing Expr::Value behavior intact,
and ensure the same path is used for both direct values and unary-negated RHS
operands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 476940ea-358a-4c14-b47c-ad8596083d00
📒 Files selected for processing (4)
CHANGELOG.mdpackages/cipherstash-proxy/src/postgresql/data/from_sql.rspackages/eql-mapper/src/lib.rspackages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rs
- packages/eql-mapper/src/lib.rs
Reverts the CI-only pin to the suite PR #2061 prerelease, restoring the stable published cipherstash-client 0.38.0. The prerelease was used to validate scalar OPE end-to-end in CI (it carries the CIP-3280 `op`-store fix); that validation is complete (full suite green on PG14-17). Until CIP-3280 (suite PR #2061) is released and this is repointed to the new stable version, the 11 scalar `map_ope_*` integration tests will fail (0.38.0 does not store the OPE `op` term). All other tests remain green.
Contributor
CodeRabbit review notes (verified)Ran CodeRabbit + manually verified each finding against current code. Notes below cover the findings in this PR's diff. (Several higher-severity CodeRabbit findings were on untracked working-tree files — Should address
Known limitation (confirm intent)
Minor
|
tobyhede
approved these changes
Jun 29, 2026
A jsonb sv comparison RHS classified as `SteVecTerm` whose column has no ste_vec index in the EncryptConfig previously fell back to `EqlOperation::Store`. Because every `eql_v2_encrypted` column is granted `JsonLike` in the eql-mapper schema regardless of its configured indexes, such a value can reach the encrypt path while its column lacks a ste_vec index. The SQL has already been rewritten to extract a STE-vec term (`oc`/`hm`), so storing a generic payload silently drops the predicate. Fail fast with `EncryptError::UnknownIndexTerm` instead. Extracts the EQL-operation selection into `eql_operation_for_column` (unit testable, behaviour-preserving for Full/Partial/Tokenized/JsonPath/JsonAccessor). Adds regression/characterization tests pinning safety invariants surfaced while reviewing CodeRabbit feedback on PR #409: - negative-literal jsonb sv comparisons are rejected at type-check (UnaryOp RHS forced to Native), never rewritten with a mismatched encryption; - a different encrypted column on the RHS is rejected by EQL term unification (jsonb sv + scalar), so the rewrite `is_eql_typed(right)` gates are sufficient; - literal-form jsonb sv equality encrypts the RHS literal (no plaintext leak).
- from_sql: add Value::Bool arm to json_scalar_to_plaintext so boolean sv leaves support hm equality (col -> 'flag' = true) instead of being rejected; add unit + integration coverage - ste_vec_ordering: add unit tests for the shared predicates/operand helpers (is_ste_vec_accessor, is_ordering/equality_operator, rhs_as_jsonb, is_eql_typed) - dedupe is_eql_typed: point the scalar OPE and containment rewrite rules at the shared ste_vec_ordering::is_eql_typed helper - eql_mapper: compute ste_vec_term_rhs_keys once in resolve() and pass the param/literal halves into param_types/literal_types (was scanned twice) - map_ope_index_order: rewrite the stale CIP-3280 TODO to reflect that NULL ORDER BY placement is already covered and the remaining gap is gated on the cipherstash-client pin bump
Points the proxy at the published cipherstash-client 0.38.1 — the stable release of the scalar-OPE `op`-store fix (CIP-3280, suite PR #2061), replacing the prior 0.38.0 pin. Cargo.lock refreshed (also pulls zerokms-protocol 0.12.19 -> 0.12.20 as required by 0.38.1). With 0.38.1, scalar OPE columns store the `op` term, so the full integration suite (incl. the scalar map_ope_* tests) is expected green. Local: cargo check --workspace + eql-mapper tests + mise check all pass.
PR #409 was retargeted from `main` to the `v3` integration branch, but the Test and Benchmark workflows only triggered on `main`, so CI stopped running. Add `v3` to the pull_request + push branch filters of both workflows.
The new select_where_jsonb_bool_eq tests bound a raw Rust `bool` as the
`$2` query param, but the encrypted jsonb column expects a jsonb value —
failing at param binding with `ToSql WrongType { postgres: Jsonb, rust: bool }`
before reaching the proxy. Bind `serde_json::Value::Bool(...)` like the other
jsonb equality tests so the boolean-leaf equality path is actually exercised.
Bool sv-leaf equality (`col -> 'flag' = $bool`) was never implemented and is not wired end-to-end: the equality rewrite reclassifies the RHS to SteVecTerm, but cipherstash-client's SteVecTerm query encoding requires an *orderable* term, and a boolean has only the `hm` equality term (no `oc`) — so it fails with "Cannot convert value of type Option<bool> to orderable term". Enabling it needs a cipherstash-client change (emit the hm term for non-orderable sv leaves). Revert the proxy-side bool work that can't function without it: the json_scalar_to_plaintext Value::Bool arm + its two unit tests, and the select_where_jsonb_bool_eq integration tests. Pre-existing scalar bool handling/tests are unaffected. Tracked as a follow-up.
coderdan
approved these changes
Jun 30, 2026
coderdan
left a comment
coderdan
left a comment
Contributor
There was a problem hiding this comment.
Looks great! Just minor things that should be addressed.
Comment on lines
+49
to
+51
| /// ```sql | ||
| /// eql_v2.ore_cllw(col -> selector) > eql_v2.ore_cllw($param::jsonb) | ||
| /// ``` |
Contributor
There was a problem hiding this comment.
This may be addressed in EQL V3 but this approach locks Proxy in to CLWW. Probably not worth addressing for 2.3 but leaving as a comment for anyone who looks at this in the future (including Claude).
| ); | ||
| } | ||
|
|
||
| /// Group B (CIP-3279): ordering comparisons on a jsonb sv-element extracted |
Contributor
There was a problem hiding this comment.
Should only reference public GH issues in public repos.
| @@ -1,3 +1,20 @@ | |||
| // TODO(CIP-3280): add an end-to-end test for `<`/`<=`/`>`/`>=` comparisons | |||
Contributor
There was a problem hiding this comment.
We should address the TODO. Also reference a public GH issue instead of Linear.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Moves Proxy off the
vendor/stack-auth[patch.crates-io]workaround onto the releasedcipherstash-clientgroup (CIP-3233 follow-up) — and carries the proxy-side work to keep the EQL/encrypted-query path working against the v2.3 payload format introduced by the 0.38.x client.The 0.38.0 bump broke 38 integration tests (v2.3 changed how encrypted values are stored/searched). All are fixed and CI is green across PostgreSQL 14–17 on the published
cipherstash-client 0.38.1. Base branch:v3.Note
Green and merge-ready (pending review). Now on the published
0.38.1(the stable release of the scalar-OPEop-store fix, CIP-3280 / suite #2061); the temporary CI-only prerelease pin was reverted and re-pointed at the real release.What's in this PR
2e11f69d,b38eeb0a,7b8500a20.34.1-alpha.4 → 0.38.1+ drop vendored stack-auth/patch/exclude; EQL pin→ eql-2.3.1.d1abdd2bRewriteJsonbSteVecOrdering→ CLLWste_vec_entryextractor form; proxy reshapes/decrypts bare sv entries.1af99682,7248515aIndexResolver(no inference/unifier change);RewriteScalarOpeOrdering→decode((col)::jsonb ->> 'op','hex')bytea compare — built-ins only, no EQL change.df28b0f8=/<>via XOR-awareeql_v2.eq_term(hm/oc); no EQL/param-encoding change.4f3c2a45,af5c963d::jsonbcast before->> 'op'; bare numeric sv literals encrypt as scalarPlaintext::Float.76a8bfc5,fd20beabSteVecTermfails fast w/o ste_vec index (CodeRabbit); dedup, module tests, perf (Toby).ed7d29e3v3to Test + Benchmark branch filters (PR retargeted tov3).1ad4c3f4→93ac7b1d,10b06cf5→717b960aWhy the extra scope
cipherstash-client 0.38.x(v2.3 EQL payload) changed encrypted storage/search: JSONB leaves became STE-vec elements (CLLWoc/ HMAChm), and scalar OPE columns no longer carry a root ordering term the bare operators read. CIP-3279/3281 rewrite jsonb comparisons to theste_vec_entryextractor forms; CIP-3283 makes the mapper concrete-index-aware so scalar OPE ordering rewrites to a built-inopbytea compare — all Proxy-side, no EQL change (so CIP-3282 / the EQL U-005 reversal is cancelled as unnecessary).Verification
CI green on PG 14–17 (
717b960a, against published0.38.1): integrationwithout_multitenant348 +multitenant69 +showcase; eql-mapper 122 unit; proxy lib 128;mise run checkclean.Deferred (not in scope, tracked)
jsonb sv boolean-leaf equality (
col -> 'flag' = <bool>) — CIP-3293. It was never implemented (only bool containment worked) and can't be wired Proxy-side: the SteVecTerm query encoding requires an orderable term, but a boolean has only thehmequality term — needs acipherstash-clientchange. The proxy-side bool work was reverted; pre-existing scalar bool equality is unaffected.Review
All inline threads resolved — 9 CodeRabbit + 5 @tobyhede. Real fixes from review: 2 (
SteVecTermfail-fast; the rest false-positives or quality/test/perf).Notes
CIP-3282(cancelled, superseded by CIP-3283).