Repository navigation
feat(stack-encrypt): store short TextMatch text as an empty filter, refuse only short queries - #1206
Conversation
|
|
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 ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 63.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 14 files. (2 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Issue #1185requires different stored and query behavior for token-less match text.match_termnow returns an emptyMatchTermswithout a PRF call.Supports::checkand chain preparation reject to…Out of Scope Changes check The Rust implementation, dynamic path, Go behavior and diagnostic handling directly implement issue #1185. The added tests verify the required behavior. ADR, API documentation, changelog, and failure-…Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main change: short stored TextMatch text uses an empty filter, while short queries remain rejected. Full details: Linked Issues check
-
Explanation
Issue
#1185requires different stored and query behavior for token-less match text.match_termnow returns an emptyMatchTermswithout a PRF call.Supports::checkand chain preparation reject token-less queries withTermError::EmptyTermTextbefore term derivation and keyset loading. The dynamic query path applies the same refusal. Tests cover empty, separator-only, and short text; storage, round-trip, no match, equality, query refusal, batch paths, and keyset loading. The implementation uses separate stored and query paths, and the Go tests cover short match values.
Full details: Out of Scope Changes check
-
Explanation
The Rust implementation, dynamic path, Go behavior and diagnostic handling directly implement issue
#1185. The added tests verify the required behavior. ADR, API documentation, changelog, and failure-location updates document or support that behavior. No unrelated change is established. The requested release is a non-coding task and does not affect this coding assessment.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 63.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 14 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
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. |
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 134 | 0 | 53 | 0 |
Every mutant in the changed lines was caught by a test.
![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: b05caf67fd
ℹ️ 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".
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Review of this PR's diff against #1192.
What I checked. A short stored value cannot cause a false negative. A query must cut at least one n-gram, so its text has at least n characters, and it cannot be a substring of shorter stored text. Case folding runs before the tokenizer on both sides, so the two sides measure length the same way. verify on a short row returns false, which is correct. The Go refusal of a short match query (#1164/#1165) stays, through dynamic::term. The EQL payload shape does not change: the stored domain checks only that bf exists, so bf: [] is valid. Mutants: 8 caught, 0 missed. Codex already raised the Ngram { length: 0 } panic in the new pre-checks; I do not repeat it.
Blocking
- A caller who builds a query with
KeysetCipher::match_termsnow gets a match-all query in SQL with no error. See the inline comment atsrc/sem/mod.rs:1297.
Should fix (not blocking)
- The Go
locateTermFailurenow tests a stored value with the query rule, so it can name the wrong row. See the inline comment atrecords.go:485.
Suggested follow-up PR
- Length leak. A stored
bf: []is visible to a database reader. That reader can now select every row whose value is empty, only separators, or shorter than the n-gram length. Before this PR, such values could not be stored. The Bloom fill already leaks approximate length, but this is an exact class. Add one sentence to ADR-0009 (theverifyparagraph, line 284) and tolanguages/golang/encrypt/doc.go:107. - Server guard. The EQL
query_text_*domains check only thatbfexists (packages/eql/src/v3/scalars/text/query_text_types.sql:45). An emptybfis "contained by everything" (packages/eql/src/v3/sem/bloom_filter/functions.sql:45). A CHECK for a non-emptybfon the query domains closes the match-all case at the server for every client. File an issue if it does not go in this stack. - CI. The Test EQL PG17 shards fail on SteVec selector drift (
v3_doc_integer $.field op-selector drifted from the pinned constant). The base #1192 fails the same way, and this PR touches no EQL file. It is not caused by this PR.
b05caf6 to
9dbb6b5
Compare
57476bf to
dbe3661
Compare
9dbb6b5 to
a9f42c1
Compare
dbe3661 to
8aed9a6
Compare
|
Review fixes are in a9f42c1; each inline thread has a reply. The suggested follow-up on the length leak is in the same commit: ADR-0009's |
a9f42c1 to
c8a3934
Compare
8aed9a6 to
a794e7d
Compare
|
Restacked onto |
Generated by Claude Code |
…er, refuse only the query A match value that cuts no token under the index's tokenizer (empty or separator-only text, or text shorter than the n-gram length) used to fail term derivation on both the stored and the query side, so one two-letter name failed a whole row. ADR-0009 splits the two: the stored term is the empty filter, which no query matches, and only a query of such text is refused, because its filter would match every row. `match_term` now derives the empty filter for such text, with no PRF call, so `MatchTerms` as a target, `KeysetCipher::match_terms` and the `dynamic` record path all store it, and the value seals and opens as any other. The refusal moves to the query paths: `op::Supports::check` takes the query's value, the `Match` index refuses token-less text there with `TermError::EmptyTermText` when the query is awaited, before any term is derived or keyset loaded, and `dynamic::term` refuses it for a match kind. The error keeps its code and is reworded for the query. The Go module builds the stack-encrypt guest by path, so its round-trip test of the old refusal is rewritten to the new behaviour: a short `TextMatch` value seals, opens and matches no query, and a short match query is still `ErrTerm`. The package docs and the term-failure message follow. Two tests that used token-less text as a leaf that fails before any I/O now use a match configuration with invalid options, which still fails during the synchronous build. Closes #1185 Claude-Session: https://claude.ai/code/session_01VYKAWbd361xDLmqBdLoUcB
match_terms is no longer a query. In 0.2.0 the same call served write time and query time and refused text that cuts no token; it now returns the empty filter for that text, and EQL treats an empty filter as contained by every stored filter. A caller who still sends its output as a query gets every row and no error, so the CHANGELOG entry moves to Breaking and says so plainly, naming cipher.query(op::Matches, ..) as the query. The query side validated the match options only when it derived the term, after the new no-token check had tokenised the text, so an n-gram length of zero panicked in the tokenizer instead of returning InvalidOptions. Both query prechecks, op::Supports::check and dynamic::term, now validate first, as the stored path always has. Go's term-failure locator asks the query path for each term, and the query path refuses short match text that a record stores as the empty filter. It could stop at an earlier row with a short email and name the wrong row. It now skips that one refusal by its code, and a test pins it with a short email in row 0. ADR-0009 and the Go package doc say that a stored empty filter tells a reader of the column the value is empty, separator-only or shorter than the n-gram length. The tokenizer's doc no longer says match_terms rejects that text.
…tch, stored or queried Rebased onto next, the EQL v4 TextMatch filter met this PR's change to match_terms. eql-bindings builds the stored TextMatch filter and the TextMatchQuery operand from one type, BloomFilter, through the stored-side match_terms. Before this PR that refused text that cuts no token, so both refused it. After it, match_terms returns the empty filter for that text, so a TextMatchQuery of "" or "al" became `bf: []`, which EQL treats as contained by every stored filter: a query matching every row, with no error, through Rust's encrypt_as and through the Go SDK's generated Query. BloomFilter's visitor now refuses an empty filter with TermError::EmptyTermText, for the stored value and the query alike, which is what EQL v4 TextMatch did before and what its docs, the Rust tests and the Go test already say. Storing a short value as the empty filter while refusing it as a query needs the query twin to have a filter type of its own, which is a codegen change left for later. The Go SDK README and the stashgen README still said a plain `match` index refuses short text; they now say it stores the empty filter and refuses only the query, and that an EQL TextMatch field is stricter.
- One rule for a match query, in one place. `QueryTokens::of_query` validates the options, tokenises the text and refuses text that cuts no token. The query chain and `dynamic::term` both call it, so the Rust chain and the Go guest cannot drift on what they refuse; `dynamic::term` no longer repeats the validate, tokenise and refuse steps by hand. - An awaited `op::Matches` query tokenises its text once fewer. `Supports` gains a provided `admit`, which checks and takes the carry together, and a match index overrides it to tokenise once for both. `prepare` calls `admit` instead of a second `check` and then `carry`. The term itself is still derived through the index's own operation, which tokenises again: that is the path a write takes, and sharing it is what makes the query's filter the stored filter's.
c8a3934 to
c816046
Compare
a794e7d to
2ec5306
Compare
|
Re the triple tokenising and the copied refusal logic (
A unit test pins On the restack: #1191 now calls |
3 participants
Summary
Stacked on #1192. This PR implements the stored/query split of ADR-0009 for short
TextMatchtext: a value with no tokens is stored as the empty filter, and only a query with none is refused.match_termderives the empty filter (bf: []) for text that cuts no token under the index's tokenizer: empty or separator-only text, or text shorter than the n-gram length. No PRF call is made.MatchTermsas a target,KeysetCipher::match_termsand thedynamicrecord path all store it, so a two-letter name no longer fails a whole row, and the value seals and opens as any other. No query matches the empty filter, because a query always sets at least one bit.op::Supports::checknow takes the query's value. TheMatchindex refuses token-less text there withTermError::EmptyTermText, when the query is awaited, before any term is derived and before any keyset is loaded. The chain runs the same check inprepare, so a chain prepared on its own is held to it too.dynamic::termrefuses it for a match kind, so the Go guest'stermexport behaves as before.TermError::EmptyTermTextkeeps its code,stack_encrypt::empty_term_text, so a binding's mapping holds; its message and help now describe the query.What this reaches outside the crate
The Go module builds the stack-encrypt guest by path, so this lands there at once rather than with a release. Its round-trip test of the old refusal is rewritten to the new behaviour: a short
TextMatchvalue seals, opens, and matches no query, and a short match query is stillErrTerm. The package docs and the term-failure message follow. The guest's own leak test, which expectstermto refuse separator-only match text, passes unchanged.eql-bindings builds no
TextMatchtype through stack-encrypt yet, so nothing changes there.Two tests that used token-less text as the leaf that fails before any I/O now use a match configuration with invalid options, which still fails during the synchronous build.
Tests
tests/op.rs: a short value stores as the empty filter, opens as written, is found by its equality term and by no match query; a match query with no tokens is refused throughon, throughviaand in anallbatch, with the named keyset never loaded; the same text is a fine equality query and a fine stored value.tests/sem_terms.rs: the empty filter for every token-less input, under both tokenizers, holds no query's positions and round-trips through its bytes.dynamic::termunit test: the same scalar is refused as a query and stored empty throughIndex<Value>.TestAMatchIndexStoresShortText.Test plan
cargo nextest run -p stack-encrypt --all-features: 578 passed.-D warningswith and without--all-features, rustdoc with warnings as errors,cargo fmt --check.go test ./encrypt/against a rebuilt deterministic guest, where the one failure is the EQL guest not being built locally.The issue's last checkbox, a stack-encrypt release so the Go SDK can stop refusing short text, turned out not to apply: the Go module builds the guest by path, so the Go side changes in this PR.
Closes #1185
https://claude.ai/code/session_01VYKAWbd361xDLmqBdLoUcB
Summary by CodeRabbit