Repository navigation
feat(stack-encrypt): store short TextMatch text as an empty filter, refuse only short queries - #1206
Conversation
…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
|
|
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:
Comment |
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 |
|---|---|---|---|
| 8 | 0 | 6 | 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".
| // A query with no tokens would match every row. The stored side | ||
| // keeps such a value as the empty filter; the query side refuses it | ||
| // here, before any term is derived. | ||
| if QueryTokens::of::<O>(value.as_ref()).is_empty() { |
There was a problem hiding this comment.
When a custom MatchConfig supplies Tokenizer::Ngram { length: 0 }, this new preflight reaches slice::windows(0) before MatchOptions::validate, causing cipher.query(...) to panic instead of returning Error::Term(TermError::InvalidOptions) as term derivation does. Validate the options before QueryTokens::of (and before the analogous dynamic::term precheck) so malformed configurations remain recoverable errors.
Useful? React with 👍 / 👎.
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.
| /// yields no tokens: empty or separator-only text, or an n-gram probe | ||
| /// shorter than the gram length (which could never match; see | ||
| /// [`Tokenizer::Ngram`]). | ||
| /// This derives the *stored* filter. Text that yields no tokens (empty |
There was a problem hiding this comment.
Blocking. On main (0.2.0), this doc says the same call serves write time and query time, and that short text fails with EmptyTermText. After this PR, short text returns bf: [] and no error.
A caller who follows the 0.2.0 doc sends this filter as a query. EQL treats an empty bf as contained by every stored filter (packages/eql/src/v3/sem/bloom_filter/functions.sql:45), and the query domain does not refuse it. So the query returns every row. MatchTerms::contains guards the Rust side, but SQL has no guard. The caller gets no compile error and no runtime error.
The CHANGELOG puts this under "Changed" (CHANGELOG.md:98). Please:
- Move the entry to "Breaking", and say plainly that the output of
match_termsmust not be used as a query now; a query comes fromcipher.query(op::Matches, ..). - Either add the EQL query-domain CHECK for a non-empty
bf, or file an issue for it and link it here.
| @@ -483,7 +483,7 @@ func (cph *Cipher) locateTermFailure(ctx context.Context, p *record.Plan, rows [ | |||
| continue | |||
| } | |||
| if _, derr := cph.Derive(ctx, p, f.Name, o, row[f.Name]); errors.Is(derr, ErrTerm) { | |||
There was a problem hiding this comment.
Derive calls the guest term export, which is the query path (guest/src/ops.rs:102 calls dynamic::term). After this PR, dynamic::term refuses short match text, but the record path stores it. So if a record call still fails with ErrTerm for another reason, this loop can stop at an earlier row whose email is "ab". The error then names the wrong row and field.
The new comment at line 474 says a stored value is never refused for its text, but this loop tests each value with the query rule. Skip a match output whose value cuts no token, or derive with the stored rule here. A test for this: a batch with a short email in row 0 and a term failure in row 1.
This branch has not been deployed
2 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