Repository navigation
test(eql): hold the planner to the json_tree indexes on every supported PostgreSQL, and document what the tree reveals - #1336
Conversation
…ed PostgreSQL, and document what the tree reveals Two of the three items that gate a release of EQL v4's json_tree. The planner. `eql_v4.json_tree_query` is plpgsql with `COST 1000` so that a bitmap scan over the GIN index beats a sequential scan even when the documents are TOASTed and the heap is a few pages. The benches showed that on one PostgreSQL version and nothing held it: the oracle test checks the plan with sequential scans turned off, which is not what a customer's database runs. The new test seals a flat table (1,000 four-node documents, every row inline) and a nested one (300 documents of about 220 nodes, every row TOASTed), pins both regimes by `pg_column_size`, runs `ANALYZE`, and with every setting at its default asserts a bitmap scan over the GIN index for containment and a scan of the functional btree for a range at an ordered path. A control prices the function at 1 for one transaction and shows the sequential scan the cost guards against, so the assertion is known to be able to fail. CI now runs the Postgres half of the encryption suite in its own job, a matrix over the supported PostgreSQL versions (17 on a pull request, 14 to 17 on a push, like the SQLx jobs), where it was one step of the single-version `rust-crates` job. Passed locally on 14, 15, 16 and 17. The page. `docs/reference/json-tree-leakage.md` is the one page a customer reads before putting a column on the type: what a stored row shows at rest, field by field; what each query type hands the database, tokens above all; what is authenticated; the side-by-side with v3; and the two options that change the picture. Refs #1324 Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
🦋 Changeset detectedLatest commit: fcf8f0c The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…to feat/json-tree-gates
407a9f5 to
e7f782d
Compare
fdf1eb9 to
9657508
Compare
Brings up the review fixes to the json_tree design record and its SQL prototypes from docs/json-index-stack-encrypt-spike. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Brings up the #1288 review fixes as #1329's branch carries them: the 127-level depth bound, the keyed member rank, the walk and rebuild bounds, the JsonTerms Debug, AtPath<Eq> as equality, the entry-versus-jsonb blockers and the changelog. No conflicts. With the rank keyed, the leakage reference's statement that nothing it describes reveals a key name holds for a token's holder too: the member order a token exposes is an order under the index key. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Brings up the array case of the rebuild-bound test from #1288, which the mutants gate asked for. No conflicts. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Carries the #1325 review fixes up through #1329. One conflict: both branches appended a PostgreSQL test at the end of the json_tree suite (this branch's planner test, the hostile-document test from below), and both are kept. Since this branch runs the suite in CI on every supported PostgreSQL, the hostile-document test now runs there too. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Brings in the #1329 review fixes (the selector-bound chain step and position gates, the refusal of positions in ordered paths, the JSON wire fuzz target), the key-built subtree wrapper, and the json plaintext held under Protected. One conflict, in packages/eql/tests/encryption/tests/json_tree.rs: both sides added a PostgreSQL test after postgres_extracts_walks_and_orders_by_path (this branch's postgres_the_planner_picks_the_indexes_without_help with its flat_document and nested_document helpers, and #1329's postgres_a_position_reaches_array_elements_only). Both are kept, the planner test first, each with its own body; git had interleaved their shared opening lines. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Brings in the JsonPath::covers test the #1329 mutants gate asked for. No conflicts. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Carries origin/next up from #1250 through #1329, so this PR reads against one merge base. Content-empty. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Review of what #1336 adds over #1329 (git diff origin/feat/json-tree-ordinal-chain...origin/feat/json-tree-gates), at d3a47b1. I checked every claim on the leakage page against stack-encrypt/src/json/{derive,mod,query}.rs and the v4 SQL (types.sql, functions.sql, operators.sql), confirmed the stored entry order on a live table, and ran the planner test on PostgreSQL 14, 15, 16 and 17.
The planner test and the CI job do what they say, with one gap in each. The leakage page is the problem. It is the page a customer's security reviewer reads, and several of its "not revealed" claims are false for the format as built. The common cause: sv (and q) are stored in depth-first pre-order, an object's members in sorted key order, under selectors that are the same in every row. Neither the page nor section 6 of the design takes that order into account.
High
- "Not revealed at rest" is wrong on four counts. The stored order gives away each row's exact shape, every node's parent (inferable across rows, and exact for top-level fields under the sealed root), every array's length (the count of its element selector), and the alphabetical order of sibling key names. The last is the open sorted-key-order leak from the #1329 review.
- The v3 comparison table claims v4 hides shape, parent relations and subtree sizes. It does not hide any of them: v4 shows shape equality just as v3 does, the parent relation is inferable in both, and a subtree's node count can be read off the order.
- The subtree-read bullet says the query sends "a token for the node." It sends the token of the top-level field the path starts with (or of the array element reached through positions right after it), so reading
$.profile.address.cityhands over$.profile's whole subtree in every row. A#ngate carries the position in the clear. Withsealed_rootoff, every such read sends the root token, which covers the whole column's shape. - "Tampering … never [changes] what a value decrypts to" is not true of extraction.
JsonNodeandJsonSubtreeopen each entry under thesandjthe server returns beside it, so->on$.salarycan be answered with the same row's$.bonus, or with the sealed root. Each value is genuine, but nothing checks it is the one at the path asked for. The fix is the owner's open decision. The page now says what holds today.
Medium
prow. "Direct members of the root carry nop" is true only with the sealed root on. With it on, it also delimits each top-level field's subtree, so that subtree's node count is readable exactly from one row.- Ordering. A bound's OPE term shows its exact place among the stored values, and whether it equals one, not "roughly where" it sits. A shared prefix of the encoding, not nearness in order, is what makes terms share leading bytes.
- CI:
rust-encryption-postgresis missing fromci-required'sneeds. The step it replaces sat insiderust-crates, which is listed. As things stand, a failing oracle or plan-shape test leaves the aggregate check green.
Low
- The lengths row leaves out the key-name length that container entries show, and the page never mentions
h(keyset id, data-key id, sealed-root flag). - The
validatejob's comment now sits above the new job. - The planner test's "TOASTed" pin is a size threshold (
pg_column_size > 2000), not a check of where the value is stored. The test also never covers a prepared statement past its fifth execution, which is whenplan_cache_mode = automay switch to a generic plan. That is the path a pooled driver takes. - The PR body summarises the page's at-rest claims as they were.
Checked and fine. COST 1000 on json_tree_query costs nothing at run time: the bitmap recheck calls it once per candidate row, and a projection or document-to-document @> only sees inflated estimates. The planner test depends on nothing that varies. ANALYZE reads every row, and the stats are over deterministic terms. Flat documents are about 1,150 bytes and every nested one is TOASTed (verified by chunk count). Under plan_cache_mode = auto the cache keeps custom plans for the nested table and uses a GIN-backed generic plan for the flat one. The control is meaningful. The matrix expression, service health check, EQL_TEST_DATABASE_URL form and the save-if: false restore of the rust-crates key are right. The removed rust-crates step is not duplicated. The changeset, the README link and the v3 "ordering terms" row are accurate (cipherstash-client 0.42.3 emits op on every scalar leaf, OPE or ORE by mode, with no per-path choice).
Left for the owner. Two things here are format decisions, so I have not changed the format for either. The first is the stored entry order: pre-order, with sorted keys. The second is that extracted entries open under the server's selector. Both are now stated on the page and recorded as open in the Phase 4 notes of the design document.
Fixes follow on this branch; each inline thread gets a reply with its commit.
The leakage page said a stored row hides its shape, its parent relations, its array lengths and its key names, and that tampering never changes what a value decrypts to. Read against the code, none of that holds. `sv` and `q` are stored in depth-first pre-order, an object's members in sorted key order, under column-deterministic selectors. The order therefore fixes each row's shape, makes the parent relation inferable (exact for top-level fields under the sealed root), counts every array, and shows the alphabetical order of sibling keys. A subtree read sends the token of the top-level field the path starts with, not of the node asked for. With the sealed root off it sends the root's token. A position gate carries its position in the clear. An extracted entry opens under the selector the server returns with it, so a row's fields can be swapped. A bound's OPE term places it exactly among the stored values, not roughly. The page now states the format as built, field by field, including the header. The v3 comparison and the sealed-root option are corrected. The design document notes where section 6 was wrong, and records the entry order and the extraction selector as open decisions for the owner. The format is unchanged. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Moving the PostgreSQL half of the Rust encryption suite out of `rust-crates` into `rust-encryption-postgres` also took it out of `ci-required`. That job reads only the results of the jobs it needs, so a failing oracle or plan-shape test would have left the aggregate check green. The job is now listed. eql-suite-ci.test.mjs fails when any job in the workflow is missing from `ci-required`'s needs, which the old list would have tripped. The `validate` job's comment, which the new job had been inserted under, moves back above `validate`. The paths comment now names both jobs that compile the stack-* crates by path. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
The planner test inferred "every row TOASTed" from pg_column_size, which measures a value's stored size, not where it is stored. It now counts the distinct chunk ids in the table's TOAST relation. That works on PostgreSQL 14 to 17, where pg_column_toast_chunk_id is 17 only. It also asked only one-shot statements, which are planned with their operand in view. A pooled driver prepares once and executes many times, and after five custom plans `plan_cache_mode = auto` may plan generically, with the operand as a parameter. Containment and the range at an ordered path are now also asserted from a prepared statement past its fifth execution, with EXPLAIN EXECUTE showing the plan the cache would run. Verified on PostgreSQL 14, 15, 16 and 17. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…isted under An extracted entry carries the header of the document it was sealed in, and the client opens it under that header. A database can therefore list another row's entry, from the same column, in a row's place, and it opens. The page said a decrypted value "comes from the row the result says". It now says the value is genuine and from this column, and that both its path and its row are the database's word. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17ac84fa6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
The planner test must also check a forced generic plan before merging (see the inline comment). The rest of the PR can merge as it is.
Other findings not posted as comments
- Optional: the six
EXECUTEruns of each prepared statement discard their rows. A generic plan that returns wrong rows would pass. Compare the row count with the one-shotcount(*).packages/eql/tests/encryption/tests/json_tree.rs:1135 - Optional: the control block covers only containment on the nested table. No control shows that the
_ordbtree assertion can fail.packages/eql/tests/encryption/tests/json_tree.rs:1158 - Optional: no script test checks that
test:encryption:postgresruns once for each version insetup'spg-versions. If the step moves back into a job with onepostgres:17service, the existing guards inscripts/__tests__/eql-suite-ci.test.mjsstill pass. Add a case that finds the job running the task and checks its matrix and service image..github/workflows/test-eql.yml:463
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 2 found, 1 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 1 posted |
Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 1 posted finding(s) were raised by two or more models.
Plain language: claude-sonnet-5-5 read every comment as a new reader would. 1 comment(s) had a problem that stopped the reader acting; it rewrote 1.
Stack: 5 earlier and 0 later pull request(s) (#1229, #1250, #1288, #1325, #1329, *️⃣ #1336). *️⃣ marks this pull request.
Context loaded: the description, 1 linked issue(s) and 25 discussion entries.
The Phase 4 note said a decrypted value is always one sealed in that row. It is not: the entry's AAD binds the keyset, the column's context, the data key's tag, the selector and the ordinal, and nothing that names a row. An entry the database returns together with its own row's header opens, so `->` can be answered with another row's value at the same path. That is the cross-row interchangeability ADR 0010 accepts for every value sealed under one context, so the fix is to the wording. The note now says what is bound, which substitutions open, and that a selector check would close the path substitution but not the row one. The leakage page's "What is authenticated" paragraph said "under this row's data key"; it now says the key the row's header names and points at the paragraph below it, which already described the row case. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
The prepared-statement check ran six EXECUTEs and read EXPLAIN EXECUTE under plan_cache_mode = auto. auto keeps a custom plan whenever it costs less than the generic one, so for the nested table the cache could go on showing a custom plan, and a generic plan that fell back to a sequential scan would fail no test. A pooled driver can reach that generic plan. After the auto assertion, each statement (containment and the range at an ordered path, on both tables) is explained again under force_generic_plan, asserted to carry its operand as $1 (so the plan read is the generic one) and to use the same index, and the setting is reset. v4_range.rs does the same for a range domain. Passes on PostgreSQL 17 (the whole test:encryption:postgres task) and 14 (this test). Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Summary
EQL (Encrypt Query Language) is the SQL bundle that stores and queries encrypted values in PostgreSQL; EQL v4 is its version for values written by Stack Encrypt, the Rust encryption engine in
packages/stack-encrypt. Itsjson_treetype (unreleased) stores an encrypted JSON document as one entry per JSON node and answers containment, path extraction, ordering and subtree reads without the key. Three items gate its release: the stored document's size, the query planner's reliability, and a leakage write-up. The size work landed in #1329. This PR closes the other two.The planner. Containment over a
json_treecolumn runs througheql_v4.json_tree_query, a plpgsql function that returns the document's stored projection and is priced atCOST 1000so PostgreSQL prefers the GIN index over scanning every row. Large documents are stored out of line (TOAST), which makes the table's main heap look tiny and a sequential scan look cheap; the cost is what keeps the index in play. The benches had shown the right plan on one PostgreSQL version and nothing held it. A new test now does, with every planner setting at its default, on both storage shapes, and CI runs it on every supported PostgreSQL version.The page.
packages/eql/docs/reference/json-tree-leakage.mdsays what a stored row shows the database at rest. That covers: which paths, how many nodes at each, and so every array's length; which (path, value) pairs; which nodes share an array element; lengths; the order of values, only at paths the index orders; and the row's shape. The shape comes from the stored depth-first entry order, which also shows parent relations and the alphabetical order of sibling keys. The page also covers what each query hands the database: a subtree read hands it the token of the top-level field the path starts with, which is column-wide and does not expire. It says what is authenticated (only the ciphertext; an extracted entry opens under the selector the server returns with it). And it compares all this with EQL v3'sste_vec. Two of these, the entry order and the extraction selector, are open decisions for the owner, recorded in the design document.Changes
packages/eql/tests/encryption/tests/json_tree.rs):postgres_the_planner_picks_the_indexes_without_help. Flat table of 1,000 four-node documents (every row inline) and nested table of 300 documents of about 220 nodes (every row TOASTed), both regimes pinned; afterANALYZE, containment plans asBitmap Index Scanon the GIN index and a range at an ordered path as a scan of the functional btree oneql_v4.ord_term(doc -> selector). Both are asserted as one-shot statements and from a prepared statement past its fifth execution. The TOASTed regime is pinned by counting distinct chunk ids in the TOAST relation. A control sets the function's cost to 1 inside a transaction and asserts the sequential scan, so the test is known to be sensitive to the cost it guards..github/workflows/test-eql.yml): newrust-encryption-postgresjob, a matrix oversetup's PostgreSQL versions (17 on a pull request or manual dispatch, 14 to 17 on a push), runningmise run test:encryption:postgresagainst apostgres:<version>service; the step and the service are removed fromrust-crates, which keeps the DB-freetest:encryptionand the WASI check. Restores therust-cratescache key withsave-if: false. Listed inci-required'sneeds, whicheql-suite-ci.test.mjsnow holds for every job in the workflow.packages/eql/docs/reference/json-tree-leakage.md, linked frompackages/eql/docs/README.md; a Phase 4 note in the design document.@cipherstash/eqlpatch (a docs page in the bundle).Verification
mise run test:encryption:postgres(installs the v4 bundle built from this tree) passed against PostgreSQL 17.10 (Homebrew) and against thepostgres:14,postgres:15andpostgres:16images frompackages/eql/tests/docker-compose.yml; the new test runs in about 9 to 11 seconds, sealing included.cargo clippy -p eql-encryption-tests --testsandcargo fmt --checkclean;mise run test:doc-anchorspassed (117 links).pnpm run test:scripts: the workflow guards (eql-suite-ci,eql-workflow-filters,workflow-working-directory,workflow-mise-cache-image) pass with the new job.tests-crates.ymlandtest-eql.ymldispatched by hand on this branch, since GitHub has not been firingpull_requestruns for this repository since 2026-10-22; results will be noted in a comment.Review fixes (1f366a7, d01d547, 491b09c):
pnpm run test:scripts(78 files, 1,371 tests), the fullmise run test:encryption:postgreson PostgreSQL 17.10, the json_tree suite onpostgres:14,:15and:16, and the EQL static checks (docs:validate:*,test:lint,test:schema,test:doc-anchorsand the rest) all pass.Related
feat/json-tree-ordinal-chain.Review notes
ste_vecbetter than the document does.https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu