Skip to content

feat(stack-encrypt): answer a document field's JSON operators and open its reads from a data plan - #1373

Open
coderdan wants to merge 14 commits into
feat/json-tree-data-planfrom
feat/json-tree-queries-reads
Open

coderdan wants to merge 14 commits into
feat/json-tree-data-planfrom
feat/json-tree-queries-reads

Conversation

@coderdan

@coderdan coderdan commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This pull request lets a language SDK query and read an encrypted JSON document (JsonSearch, EQL's json_tree type) through a data plan, the declaration a non-Rust SDK sends Stack Encrypt (the Rust engine) as data. It is slice C of the plan in #1353: the JSON operators, selector and token preparation, and reads opened against the request that prepared them, in the engine and in the WebAssembly guest (the engine compiled for Go). The Go runtime learns to carry the new answers; generated Go methods follow in the next pull request.

Every operand is the typed Rust chain's, bit for bit. A read's rows open only against the trusted request (operator, path, bound, field and keyset), which the engine re-derives from, so the database never says which path a row is for.

Changes

Operators (packages/stack-encrypt/src/dynamic)

  • OpSpec gains the document operators. Each takes an object of named inputs, with JSON values as JSON text:

    operator input operand, bound as
    contains, contained_by {"json"} needle, eql_v4.query_json
    eq_at {"path", "json"} needle holding the scalar there
    lt_at … gte_at {"path", "json"} at an ordered path leaf operand, eql_v4.query_json_leaf
    between_at {"path", "low", "high"} two leaf operands
    selector {"path"} text
    subtree {"path", "wildcard_bound"?} text[]
    children, child_count {"path"}, no wildcard text (the node's token)
  • field_operators lists them from the field's resolved options (the orderings only when the field orders a path), and only those whose SQL type the target names, so it lists exactly what query_term derives. between_at's arity is one: its input is one object.

  • The query term's wire form gains encoding (bytes, eql, text, text_array), and Operand gains Text and TextList, so a selector or a token list is a named value, not anonymous bytes. An operator entry gains input (its input's shape), so a generator can tell inputs apart without a copy of the rules.

  • Refusals before anything is derived: a position in a path where the operand is a selector (json_path_position; a positional read is a subtree read), a wildcard where the operator names one node (json_path_wildcard), malformed inputs (json_input), and the engine's existing ones (container at a path, unordered path, non-ordered leaf).

  • json_open(cipher, plan, field, request, rows, resolver) / check_json_open: open the rows of a selector or subtree read, one group per database row, in one key request. The engine derives the selector or the subtree key again from the request and the field's options under the caller's keyset, runs the index's own row checks, and opens through that keyset alone.

EQL (packages/eql)

  • Target::leaf_query_sql_domain, so the engine knows what an ordering at a path binds as.
  • tests/json_tree_data_plan.rs: every operand equals the typed chain's; the operator list; the refusals; reads of -> and of a wildcard subtree across two documents, absence as no value, rows refused against another path's request, and a truncated wildcard refused.

Guest (languages/golang/encrypt/guest)

  • New export se_json_open; JSON read refusals (JsonWrongEntry, JsonWildcardTruncated) are STATUS_ENCODING with their own diagnostic codes, not internal errors.

Go runtime (languages/golang)

  • QueryTerm.Encoding and QueryTerm.Text; record.Operator.Input; record.JSONIndex; Cipher.JSONOpen.
  • json_tree_test.go: every operator through the guest with its encoding and domain, a refused positional selector, and a selector read opened against its request (and refused against another's).

Docs: CHANGELOGs (stack-encrypt, eql-bindings, Go) and a changeset for @cipherstash/eql.

Assumptions

The five owner decisions in #1347 are open; this stack takes the plan's recommended answer to each (the Go-facing ones land in the next pull request):

  1. Go field types: json.RawMessage / *json.RawMessage.
  2. JSON crosses the guest as text: yes; every JSON input here is JSON text.
  3. Containment needle: an explicit JSON value ({"json": …}), not the field's struct type.
  4. Ordered-path accessors: generated from the validated options; the engine lists orderings only for a field that orders a path.
  5. First release scope: the full inventory.

Also assumed:

  • Operator names contains, contained_by, eq_at, lt_at, lte_at, gt_at, gte_at, between_at, selector, subtree, children, child_count; input objects follow the plan's examples ({"json"}, {"path", "json"}, {"path", "wildcard_bound"}).
  • Read descriptor, not a guest handle (plan section 4): the host keeps the request and hands it back with the rows; the guest re-derives. The read is opened through the keyset that prepared it, never the client-wide scope, so a document from another keyset is refused before any key request.
  • Children: the guest prepares the node token for json_tree_children / json_tree_child_count. The plaintext child-opening helper the plan lists (parent-bound child validation) is not in this pull request; children are exposed as encrypted entries for SQL. See "Not done" below.
  • Text-valued operands bind as text / text[], named in the term's domain.

Not done here

  • A request-bound opener for json_tree_children rows (child scalars without a subtree read). A subtree read of the node returns the same values today.
  • A typed struct adapter for a needle (plan section 2): the needle is JSON text.

Verification

After merging the base's rename to EQL v3's names (97b04ed, 2b5cb48) and renaming this branch's own uses (97cf926), on Linux x86-64 with Rust 1.97.0, Go 1.26.10 and PostgreSQL 16:

  • Root cargo fmt --all --check and cargo clippy --locked --no-deps --workspace --all-targets --all-features -- -D warnings: clean. cargo nextest run -p stack-encrypt --all-features without the trybuild ui binary (its snapshots record Rust 1.94.1's diagnostics): 927 passed.
  • packages/eql: fmt and clippy clean; mise run types:generate left the tree unchanged; cargo nextest run --workspace --exclude eql_tests 625 of 627 passed. The two failures are the eql-codegen tests that make a directory read-only and expect a write to fail, which cannot happen when running as root; they fail the same way before this change.
  • The four guest builds, wasm:guest:test, go:test, go:lint (0 issues), go generate ./... (no diff) and go:test:postgres (all three tests by name): passed.

Before the merge: macOS arm64, Rust 1.94.1, Go 1.26.10, on this branch. After the review fixes (abdc767, merged with the base in 978f0cd): root clippy and cargo nextest run --workspace --all-features (1,493 passed, including the new in-crate tests/json_operators.rs and the heap-spy test for an operator's input), rustdoc with warnings as errors, packages/eql fmt, clippy and nextest (627 passed), types:check, the four guest builds and wasm:guest:test, go:test (including TestParseQueryTermRefusesAMixedShape), go:lint, go:test:postgres against PostgreSQL 17 with the tree's EQL v4 bundle, crap:stack-encrypt (none over 30), and a local cargo mutants --in-diff run (96 mutants; the six survivors are fixed or, for the visitor's expecting, excluded as equivalent). The original verification:

  • cargo fmt --all --check (root, packages/eql, guest): clean. cargo clippy --locked --no-deps --workspace --all-targets --all-features -- -D warnings: clean. EQL cargo clippy --workspace --all-targets --all-features --exclude eql_tests -- -D warnings: clean.
  • Root mise x --env test -- cargo nextest run --workspace --all-features --locked: 1,470 passed.
  • packages/eql cargo nextest run --workspace --exclude eql_tests: 624 passed, including the 7 tests of tests/json_tree_data_plan.rs. The SQLx crate needs generated fixtures and a database and was not built.
  • Guests rebuilt (wasm:guest:build, :deterministic, :eql, :eql:deterministic); mise run go:test passed, including the new TestJSONOperatorsDeriveTaggedTerms and TestJSONSelectorReadsOpenAgainstTheirRequest.
  • mise run wasm:guest:test (clippy in every feature set, docs, nextest with and without eql): passed, with the catalog test now pinning JsonSearch's operators.
  • The pinned wire bytes test (dynamic::query::wire_bytes) is re-pinned for the new keys and gains the JSON index's entries and a text and text_array term; the two trybuild snapshots that now print stack_encrypt::op::Contains in full (because OpSpec::Contains exists) are re-recorded.
  • Not run: live ZeroKMS tests (no credentials here). PostgreSQL: the next pull request runs the Go data path against PostgreSQL; nothing here changes SQL.

Related

Stack

Merge order is bottom to top; each pull request is based on the one below it.

  1. chore(deps): take vitaminc's Go bindings from cipherstash/vitaminc#526 by git reference #1369: vitaminc's Go bindings from the unreleased feat(vcffi): read and write the storage profile in Go vitaminc#526 by git reference (temporary)
  2. feat(go): seal an opaque struct as a vitaminc storage blob #1370: Go: an opaque struct sealed as a vitaminc storage blob
  3. feat(stack-encrypt,eql): store a JSON document through a data plan as JsonSearch #1372: JsonSearch stored through a data plan (engine, EQL catalog, guest, Go runtime)
  4. feat(stack-encrypt): answer a document field's JSON operators and open its reads from a data plan #1373: JSON operators and request-bound reads through a data plan (this pull request)
  5. feat(go): generate encrypted JSON document fields #1374: Go: generated encrypted JSON document fields

https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu

…n its reads from a data plan

A data plan could store an EncryptedJson document but not query or read
one: the operators were reachable only from typed Rust, and a read's rows
opened only against a JsonSubtreeKey or JsonSelector a binding cannot
hold.

OpSpec gains the document operators (contains, contained_by, eq_at, the
orderings at a path, between_at, selector, subtree, children,
child_count), each taking an object of named inputs with JSON values as
JSON text. query_term derives the typed chain's operands bit for bit and
tags them: the term's wire form gains `encoding`, and Operand gains Text
and TextList, so a selector or a token list is named, not anonymous
bytes; an operator entry gains `input`. A position is refused where the
operand is a selector, which reads it as the wildcard, and a wildcard
where the operator names one node.

json_open opens a selector or subtree read's rows against the request
that prepared it: the engine derives the selector or the key again from
the request, the field's resolved options and the keyset, runs the
index's own row checks on each database row's group, and confines
opening to that keyset. Nothing in the rows says which path they are
for. The guest exports it as se_json_open, and maps a wrong entry and a
truncated wildcard to STATUS_ENCODING. The Go runtime reads the new keys
(QueryTerm.Encoding/Text, record.Operator.Input) and adds
Cipher.JSONOpen.

tests/json_tree_data_plan.rs holds every operand to the typed chain's and
opens reads of `->` and of a wildcard subtree across documents.

Refs #1347, #1350, #1352

Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
@changeset-bot

changeset-bot Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7246df1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@cipherstash/eql Minor
stash Patch
@cipherstash/stack-prisma Patch
@cipherstash/basic-example Patch
@cipherstash/e2e Patch
@cipherstash/prisma-example Patch
@cipherstash/stack-drizzle Patch
@cipherstash/stack-supabase Patch
@cipherstash/stack Patch
@cipherstash/wizard Patch
@cipherstash/bench Patch
@cipherstash/test-kit Patch

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

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI; Enforced overrides: Organization
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 261f7fc9-1613-43ec-95cf-0cb9bdfa4f50
Commits

Reviewing files that changed from the base of the PR and between 15f5aea and 7246df1.

Files selected for processing: 33
  • .cargo/mutants.toml
  • .changeset/eql-encrypted-json-operations.md
  • languages/golang/CHANGELOG.md
  • languages/golang/encrypt/checker.go
  • languages/golang/encrypt/guest.go
  • languages/golang/encrypt/guest/src/abi.rs
  • languages/golang/encrypt/guest/src/ops.rs
  • languages/golang/encrypt/guest/src/status.rs
  • languages/golang/encrypt/guest/src/targets.rs
  • languages/golang/encrypt/guest/tests/native_ops.rs
  • languages/golang/encrypt/json.go
  • languages/golang/encrypt/json_tree_test.go
  • languages/golang/encrypt/query.go
  • languages/golang/encrypt/query_test.go
  • languages/golang/encrypt/targets_test.go
  • languages/golang/internal/record/record.go
  • packages/eql/crates/eql-bindings/CHANGELOG.md
  • packages/eql/crates/eql-bindings/src/encryption/targets.rs
  • packages/eql/tests/encryption/tests/json_tree_data_plan.rs
  • packages/eql/tests/encryption/tests/typed_queries.rs
  • packages/stack-encrypt/CHANGELOG.md
  • packages/stack-encrypt/src/dynamic/json_query.rs
  • packages/stack-encrypt/src/dynamic/mod.rs
  • packages/stack-encrypt/src/dynamic/query.rs
  • packages/stack-encrypt/src/dynamic/target.rs
  • packages/stack-encrypt/src/json/derive.rs
  • packages/stack-encrypt/src/json/query.rs
  • packages/stack-encrypt/tests/heap_hygiene.rs
  • packages/stack-encrypt/tests/json_operators.rs
  • packages/stack-encrypt/tests/query_operators.rs
  • packages/stack-encrypt/tests/ui/op_at_path_not_a_leaf.stderr
  • packages/stack-encrypt/tests/ui/op_json_not_answered.stderr
  • packages/stack-encrypt/tests/value_kinds.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@blacksmith-sh

This comment has been minimized.

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Mutation testing (cargo-mutants, --in-diff, stack-auth + stack-encrypt)

caught missed unviable timeout
81 0 39 0

cargo-mutants exited with no surviving mutants recorded. The run may not have completed cleanly; check the workflow run logs.

CI's rustdoc, CRAP and coverage gates failed on this branch: two public
docs linked the private json_query module, and the JSON operators were
exercised only from the EQL workspace, so stack-encrypt's own coverage of
read_input, derive, open and read_open was zero.

- OpSpec's docs link OpSpec::input, query_term and json_open instead.
- tests/json_operators.rs runs every operator, the input refusals, the
  read refusals and reads through a resolver backed by the engine's own
  JSON index, so the crate measures its own code.
- read_input is split per operator; its complexity alone (31) passed the
  CRAP threshold at full coverage.

Refs #1347

Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…nst a selector

The mutants gate's last survivors in derive: eq_at's needle and an
ordering's leaf operand were only checked to be objects, and children's
token only against child_count's. The test now holds eq_at to
contains' shape and an ordering to another, and a node's token apart
from its selector. Parsed's Drop and the JsonText visitor's expecting
are excluded with their reasons, as for the PR below.

Refs #1347

Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
@coderdan

Copy link
Copy Markdown
Contributor Author

Automated code review of this pull request at 8485fe9fa (the review skill did not post inline comments, so the findings are listed here):

  1. packages/stack-encrypt/src/dynamic/query.rs:619: answers() lists a document field's operators from its options alone, without checking that the target names the domain each binds as, so field_operators can list an operator query_term then refuses.
  2. packages/stack-encrypt/src/dynamic/json_query.rs:516: json_open's scope has no effect: Pending::all(keyset, …) scopes the batch to the deriving keyset whatever scope is passed.
  3. packages/stack-encrypt/src/dynamic/query.rs:268: takes_pair() is true for between_at, so the catalog says pair while the input is one object {path, low, high}.
  4. packages/stack-encrypt/src/dynamic/json_query.rs:336: derive() clones the parsed plaintext into an owned index operation that never wipes it.
  5. packages/stack-encrypt/src/dynamic/json_query.rs:418: open()'s docs say a non-read operator is operator_not_answered; the code says json_input.
  6. languages/golang/encrypt/query.go:210: parseQueryTerm accepts term shapes the engine never writes (encoding vs. text/list/domain mismatches); its doc lists five keys.
  7. languages/golang/encrypt/guest/src/ops.rs:376: parse_field took parse_query's doc comment, and parse_query repeats parse_field's parsing.
  8. packages/stack-encrypt/src/dynamic/json_query.rs:133: json_reason matches error codes as strings with a catch-all, not the enum.
  9. packages/stack-encrypt/src/dynamic/json_query.rs:85: binds() and answered_by end in catch-all arms, so a new operator is silently mis-bound or unanswered.
  10. languages/golang/encrypt/query.go:114: public docs not updated for the document operators (Cipher.QueryTerm, parseOperatorName, the guest's query_term, Rust query_term, QueryTerm::domain).
  11. .cargo/mutants.toml:134: excluding Parsed's Drop exempts a reachable, behaviour-changing mutant, against AGENTS.md's rule.
  12. packages/stack-encrypt/src/dynamic/query.rs:620: every answer() for a document field clones the options twice to look up one operator.

https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu

- A document field lists an operator only when its target names the SQL
  type the operand binds as, so field_operators lists exactly what
  query_term derives.
- json_open drops its scope parameter, which had no effect: a read opens
  through the keyset that derived it, and that keyset alone.
- between_at's arity is one (its input is one object); takes_pair is
  between alone. Pinned wire bytes re-pinned.
- json_query::binds is exhaustive over OpSpec and defines is_json;
  answered_by lists the document operators in their own arm; json_reason
  matches JsonTextError's variants.
- Go's parseQueryTerm refuses a term whose encoding, shape and domain do
  not agree as the engine writes them.
- Parsed's Drop is no longer excluded from mutation: heap_hygiene's
  a_json_operators_input_leaves_no_plaintext_on_the_heap fails without it.
- Docs: the guest's parse helpers and query_term (with encoding), Rust
  query_term's inputs and refusals, QueryTerm::domain, Cipher.QueryTerm,
  parseOperatorName, open()'s refusal reasons.

Refs #1347

Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
@coderdan

Copy link
Copy Markdown
Contributor Author

Review response to the findings listed above (fixed in abdc767 unless noted; merged with the base in accb055 and 978f0cd):

# Finding Outcome
1 query.rs:619: operators listed without their domain Fixed: a document field lists an operator only when the target names the SQL type its operand binds as (json_query::domain, now shared with query_term), so field_operators lists exactly what query_term derives.
2 json_query.rs:516: json_open's scope has no effect Fixed: the parameter is gone (json_open(cipher, plan, field, request, rows, resolver)); a read opens through the keyset that derived it and that keyset alone, as documented. Guest and tests updated.
3 query.rs:268: between_at reported as a pair Fixed: takes_pair is between alone; between_at's arity is one and its input path_pair. Pinned wire bytes re-pinned for the seven between_at entries.
4 json_query.rs:336: cloned plaintext handed to the index Not changed: the JSON index takes its input by value and its derivation does not wipe it yet. That is the same gap reported on #1372 for the seal path (json::derive frees key and leaf plaintext unwiped); moving instead of cloning would not change what the index frees.
5 json_query.rs:418: wrong reason in open()'s docs Fixed: the docs list json_input for every request refusal, as the code does.
6 query.go:210: parseQueryTerm too lenient Fixed: encoding, term shape and domain must agree as the engine writes them (bytes: bytes, no domain; eql: bytes with a domain; text: one string; text_array: a list of strings). New TestParseQueryTermRefusesAMixedShape (seven accepted shapes, seven refused). Doc updated.
7 ops.rs:376: misplaced doc, duplicated parsing Fixed: each function has its own doc, and parse_query calls parse_field.
8 json_query.rs:133: reason by string code Fixed: json_reason matches JsonTextError's variants.
9 json_query.rs:85: catch-all arms Fixed: binds is exhaustive over OpSpec and returns None for a scalar operator (is_json is defined by it); answered_by lists the document operators in its own arm.
10 query.go:114: stale docs Fixed: Cipher.QueryTerm, parseOperatorName, the guest's query_term (with encoding), Rust query_term (inputs and refusals) and QueryTerm::domain.
11 mutants.toml:134: Parsed's Drop excluded Fixed: the exclusion is removed. heap_hygiene's new a_json_operators_input_leaves_no_plaintext_on_the_heap checks a document operator's input and fails when Parsed's Drop is a no-op (checked by making it one). The same was done for JsonPlaintext on #1372.
12 query.rs:620: options cloned per lookup Not changed: field_operators returns an owned IndexSpec per operator, so a copy per listed operator is part of its contract; the cost is a small path set per call.

CI failures on the previous push of this branch (rustdoc's private links, the CRAP gate, which needed in-crate coverage of the JSON operators, and the mutants gate's survivors) were fixed in e1b21ce and b2b5e1d.

Verified on this branch after the fixes: root cargo clippy and cargo nextest run --workspace --all-features (1,493 passed), rustdoc with warnings as errors, packages/eql fmt, clippy and nextest (627 passed), types:check, the four guest builds and wasm:guest:test, go:test, go:lint, go:test:postgres against PostgreSQL 17 with the tree's EQL v4 bundle, and the CRAP gate (crap:stack-encrypt: none over 30).

https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu

…est writer

CI's Linux run flagged a freed hash table in vitaminc's frame writer that
held the marker in bytes left by the test's own input building
(serde_json and a growing format! string), not by the engine. The inputs
are now joined into strings sized before they are written and framed by
the suite's object writer. The test still fails when Parsed's Drop is a
no-op.

Refs #1347

Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
@coderdan
coderdan added this pull request to stack #1377 October 25, 2026 22:47
Brings in the v4 JSON document's v3 names (`JsonSearch` over
`public.eql_v4_json_search`, `eql_v4.query_json`, `eql_v4.query_json_leaf`,
`public.eql_v4_json_entry`) and the SteVec SQL confined to EQL v3.

Claude-Session: https://claude.ai/code/session_01HLuZxys1NDRmAkv3uhnuoJ
… v3 names

The JSON operators and reads this branch adds named the document target
`EncryptedJson` and bound their operands as `eql_v4.query_json_tree` and
`eql_v4.query_json_tree_leaf`. With the base's rename those are
`JsonSearch`, `eql_v4.query_json` and `eql_v4.query_json_leaf`, and a read
opens `public.eql_v4_json_entry` rows. The resolvers the tests build, the
pinned operand domains, the guest's and Go's operator tests, the
operations changeset and the CHANGELOGs now say so.

Refs #1350, #1352

Claude-Session: https://claude.ai/code/session_01HLuZxys1NDRmAkv3uhnuoJ
Brings in the realigned JSON tree domain list and the corrected json
index note in the target descriptor's docs.

Claude-Session: https://claude.ai/code/session_01HLuZxys1NDRmAkv3uhnuoJ
@coderdan

Copy link
Copy Markdown
Contributor Author

History rewritten on this branch, authorship only. Twelve commits pushed here this morning by a cloud session were unsigned and authored Claude <noreply@anthropic.com> with a Co-Authored-By line, against this repository's rule that commits are signed and human-authored. Each was recreated with the same tree, parents, message and dates, authored and signed by the human owner, with the co-author line removed; the commits above them were re-signed because their parents moved. git diff between the old and new tips is empty on every branch. No code changed.

https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu

@coderdan
coderdan marked this pull request as ready for review October 26, 2026 05:10
@coderdan
coderdan requested a review from a team as a code owner October 26, 2026 05:10
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-26T05:12:35.092109Z e91856e Draft marked ready
🔒 Security Review 🔄 Running since 2026-10-26T05:10:40.206274Z e91856e Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cipherstash-agent cipherstash-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review attestation: Claude Code review (/code-review medium), then cross-checked with cross-check-review-findings against head e91856e. Three reviewers covered the engine, the Go guest and runtime, and the tests and release hygiene. Findings were checked against head and against the existing PR feedback. The earlier 12-item review was already fixed and none of it is repeated here.

Blocking / major (1)

  • json_query.rs:361: eq_at with a very long path builds a nested Value as deep as the path, and its recursive drop can overflow the stack and abort the host process. Nothing caps the segment count. See the inline comment.

Minor (3)

  • json_query.rs:321: check_query and check_json_open don't refuse everything derive and open refuse, despite their docs.
  • eql-bindings/CHANGELOG.md:18: duplicated line.
  • json_operators.rs:363: the test doc promises a check the test doesn't make.

Checked, no issue found

  • Row binding in json_open: I found no way to open a row against another path, field or keyset.
  • The se_json_open ABI buffer handling and status mapping.
  • Wire parsing.
  • The changeset and crate changelogs.
  • The earlier Go EncryptedJson Postgres failure: no such subtest exists at head.

Not verified: I ran no tests or builds. The stack-overflow claim rests on code reading plus a standalone serde_json reproduction, not on running the engine.


Generated by Claude Code

Input::Needle(needle) => terms(keyset, options, label, needle.0.clone())
.try_map(|terms| eql(&JsonQuery::of_terms(&terms)).map(Operand::One)),
Input::At(path, value) if op == OpSpec::EqAt => {
terms(keyset, options, label, path.needle(value.0.clone()))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major: path.needle(...) folds every path segment into a nested serde_json::Value before any depth check, and JsonPath::parse (json/path.rs:79) has no segment cap. eq_at (no ordered-path gate) with {"path": "$" + ".a" * 50_000, "json": "1"} passes check_query/read_eq_at, builds a ~50k-deep value, and its recursive drop can overflow the stack. A stack overflow aborts the host process and cannot be caught. A standalone serde_json program aborted at 50k and 200k levels on a 2 MB stack. I did not run the engine itself.

Suggested fix: refuse path.segments().len() over the existing depth limit in path(), so every operator is covered and check refuses it before a keyset is resolved.

Source: Claude Code review (medium), verified against head e91856e.


Generated by Claude Code

}
}

/// Check a JSON operator's input without a cipher: what [`derive`]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the docs on check (and on check_open, ~line 460) say it refuses what derive/open refuse before deriving anything. read_input/read_open only parse the path. JsonTooDeep, JsonWildcardTooWide and JsonTooWide are raised later in subtree_key_for. For example {"operation":"subtree","path":"$.a[*][*][*]"} with a wide wildcard_bound passes the check and fails only after a keyset is resolved. Either move those checks into read_* or narrow the docs. Not a security issue. The reviewer found this by reading the code and did not run it.

Source: Claude Code review (medium), verified against head e91856e.


Generated by Claude Code

other type, which a resolver hands Stack Encrypt
(`TargetDescriptor::with_leaf_query_sql_domain`).

- **`JsonSearch`, the `json_tree` document, by name** (`targets`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this line is duplicated on line 19 (- **JsonSearch, the json_tree document, by name** (targets`), leaving a dangling bullet with a broken header. Delete one copy.

Source: Claude Code review (medium), verified against head e91856e.


Generated by Claude Code

}

/// A field with no ordered path answers no ordering; a target given
/// options it does not take is refused when the plan is built.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the doc says "a target given options it does not take is refused when the plan is built", but the test body only checks lt_at is absent and refused with OperatorNotAnswered on an unordered plan. TargetError::Options is not asserted here. Add the assertion or drop the sentence. Related: the reason helper in malformed_inputs_are_refused_with_their_reason maps reason() == None to "engine", so any reason-less failure passes those rows.

Source: Claude Code review (medium), verified against head e91856e.


Generated by Claude Code

@freshtonic freshtonic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design is good. The read descriptor is a request that the host keeps, and the guest derives the selector or the subtree key from it again, under the field's label and the caller's keyset. It opens through JsonSelector::open / JsonSubtreeKey::open, and it is held to the request with scoped_to (json_query.rs:503-533). So a server-supplied entry cannot open under a different selector, column or keyset than the one the request names. The tests show this for another path's selector rows, another path's subtree rows and a truncated wildcard. They also show that every operand is equal to the typed chain's operand.

Rust and Go agree on the wire names, the operator names and the refusal reasons. The new terms (needle, leaf operand, selector, node token, subtree tokens and probes) leak only what json-tree-leakage.md already gives. Error payloads carry reasons, expected or bound only, and no input.

The main point is plaintext that the new query path leaves in freed memory (the inline comments on json_query.rs:358 and heap_hygiene.rs:378). A heap-hygiene probe, not committed, that calls query_term with the PR's marked needle fails: for Contains alone, 14 freed heap blocks still hold the plaintext marker. The existing test does not find this, because it calls only check_query, which derives nothing. The other items are gaps in the tests and the docs.

Other points with no line in the diff:

  • Nit: query.rs:245 gives exact as the fidelity() of selector, subtree, children and child_count, but these are not predicates. Also, eq_at and the orderings through a wildcard compare "any" or "first" element (json_query.rs:26-29). A note in the OperatorEntry docs would help.
  • Nit: the PR body says "rename to EQL v3's names", but the code uses eql_v4.

Checks that ran: the stack-encrypt tests json_operators, heap_hygiene, query_operators and json_index (pass) and the dynamic:: lib tests (228 pass); the EQL encryption tests json_tree_data_plan and typed_queries (pass); the guest crate's native tests (pass); and go test ./encrypt/... against the deterministic and EQL guests (pass).

) -> Result<Pending<'a, Operand, K>, Error> {
let column = || NonEmpty::from(label.clone());
Ok(match read_input(op, options, input)? {
Input::Needle(needle) => terms(keyset, options, label, needle.0.clone())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. needle.0.clone() and path.needle(value.0.clone()) give an unwiped copy of the plaintext to keyset.run(... Owned ...). The same pattern is at lines 361, 364 and 367-368. json_terms reads the copy only through M::view, and then it drops the copy without wipe_value (operations.rs:580). Parsed wipes the original, but not this copy.

A heap-hygiene probe confirms the leak: the freed BTreeMap<String, Value> frames come from terms at line 335, called from line 358. Please use the borrowing form, crate::json::terms_pending(&needle.0, …), or give the operation a wrapper that wipes the value when it is dropped. Then add a query_term variant of a_json_operators_input_leaves_no_plaintext_on_the_heap.

}

/// A document operator's JSON input is parsed and checked, and the parsed
/// needle wiped when the check drops it: a check derives nothing, so what

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. This test calls only check_query, which derives nothing ("a check derives nothing"). So it cannot see what query_term leaves behind, or what json_open leaves behind (it returns plaintext through opened_frame). Please add watched runs of query_term (Contains, EqAt, GtAt, BetweenAt) and of json_open (selector and subtree).

Note: the same probe also finds plaintext in json::derive::Walker::visit, value_selector and JsonPath::selector_bytes (key names in path segments, and selector bytes). That code is not in this diff, and record sealing and the typed path share it. If you do not fix it here, please open an issue and add a named temporary exclusion, as for the aws-lc exclusion, so that the new test can pass.

/// no value, a wildcard subtree per row, and rows refused against another
/// read's request or past the bound, each group opening on its own.
#[tokio::test]
async fn reads_open_against_the_request_that_prepared_them() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking. The wrong-entry tests use rows of a different path in the same column. Please add a negative test for each of these:

  • A document sealed under another keyset. The selector changes, and scoped_to refuses it. The docs say "refused before any key is retrieved" (json_query.rs:476 and the json_open doc), so assert that the key source sees no request.
  • An entry from another field of the plan (another label).
  • A request with operation: "subtree" and selector rows, and the inverse.

// as the SQL grafts it (the document's header and the entry's ordinal),
// opens to the scalar; no entry opens to nothing; and another path's entry
// is refused, since the request names the path and the rows do not.
func TestJSONSelectorReadsOpenAgainstTheirRequest(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking. The Go test opens only a selector read. Please add a subtree read with wildcard_bound. This proves that Go's integer encoding of wildcard_bound deserializes as Rust's Option<u32>. Please also add a truncated wildcard that maps to ErrEncoding. That mapping is the new status.rs arm (status.rs:144-150), and no test covers it.

/// ([`Error::JsonWrongEntry`](crate::Error::JsonWrongEntry)), as does a
/// wildcard read of an array longer than its bound
/// ([`Error::JsonWildcardTruncated`](crate::Error::JsonWildcardTruncated)).
pub fn json_open<'a, K>(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking. json_open and check_json_open call has_context (line 978 and the matching line in json_open), so a plan with a context field gets Reason::QueryContextField. The # Errors section does not say this. Please add it, and say in the CHANGELOG that the data plan cannot read plans with row context yet.

(`TargetDescriptor::with_leaf_query_sql_domain`).

- **`JsonSearch`, the `json_tree` document, by name** (`targets`
- **`JsonSearch`, the `json_tree` document, by name** (`targets`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking. The bullet header "JsonSearch, the json_tree document, by name (targets" occurs two times (lines 18 and 19). The merge left a broken line. Please delete one.

fn ordered_path(options: &JsonOptions, text: &str) -> Result<JsonPath, Error> {
let path = path(text)?;
if path.names_a_position() {
Err(term(TermError::JsonOrderingAtPosition))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. A position on an ordering path gives TermError::JsonOrderingAtPosition (an engine error). A position on an eq_at or selector path gives Reason::JsonPathPosition (line 224). The condition is the same, but the refusal is different, so a binding must handle both. If this is deliberate parity with the typed path, please add a comment.

/// A path that names one node: no wildcard.
fn node_path(text: &str) -> Result<JsonPath, Error> {
let path = path(text)?;
if path.is_positional() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. node_path uses is_positional() to mean "no wildcard". The name reads as the opposite of names_a_position(). Please add a short comment at the call site.

Brings in the vitaminc d7de535c4 pin for vcffi/vcvalue and the go-release
guard that refuses a final version while go.mod names a pseudo-version,
with the opaque storage profile's two fixes beneath them.

Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants