fix(stack): retract the include_original substring-search fiction (CIP-3483)#615
fix(stack): retract the include_original substring-search fiction (CIP-3483)#615tobyhede wants to merge 1 commit into
Conversation
CIP-3483 reports that v3 `contains()` cannot match substrings, blaming
`include_original: true` for appending a whole-value token to the needle's
bloom that the haystack's bloom cannot contain. Measured against real ffi,
none of that is true.
`include_original` is INERT in protect-ffi. Encrypting the same plaintext
with the flag on and off yields the same bloom bits (0.24 — main's pin, so
the version the issue was filed against — and 0.29, under EQL v2 and v3).
The bloom is trigram-only either way, and a value shorter than token_length
blooms to nothing under both settings. Against the live eql-3.0.0 bundle,
`eql_v3.contains(a public.eql_v3_text_search, b jsonb)` with storage
envelopes on both sides matches the issue's own failing needles ('alice',
'example') and correctly rejects an absent one.
The claim was never measured. It began as a comment on the v3 supabase
builder ("KNOWN BROKEN for real substrings ... do not paper over it here")
and propagated into types.ts, the reference doc, the skill, and this
changeset. supabase-v3-pgrest-live then "confirmed" it: that suite has no
credentials and mints its own envelopes, and its `bloomTokens` prepended
the whole-value token itself — to the seeded row AND the needle — so the
substring failure it asserted was its own invention. Nothing tied the fake
to ffi. drizzle-v3/operators-live-pg has been asserting the truth against
real ffi and real Postgres the whole time.
- bloomTokens emits trigrams only, mirroring ffi (sub-trigram values now
bloom to nothing, as they really do). The `does not match a longer
substring` test inverts: 'example' matches 'ada@example.com'. Adds a
'zzz' negative — every other assertion in that suite is satisfied by a
contains() that matches everything.
- The v3 domains emit `include_original: false`: inert today, but it is
the value a substring-search domain wants (and the zod schema's own
default) if an ffi release ever starts honouring the flag. v2 keeps its
historical `true`, so no v2 wire moves. The v3-vs-v2 parity test now
asserts both sides of that divergence rather than byte-identity, so
neither can drift unnoticed.
- match-bloom-live pins the invariant where it actually lives — on ffi's
bloom output, not through a stack builder. It also documents that the
bit ORDER is nondeterministic across encrypts of the same plaintext:
only the bit set is meaningful, so comparisons sort first.
- Drops the false claims from query-builder-v3, supabase/types,
match-defaults (include_original does NOT make sub-trigram values
matchable — that fail-open is exactly what matchNeedleError guards),
the supabase reference doc, and the stash-supabase skill.
Left alone: the columns.ts / stash-encryption note that index inference
picks `unique` before `match`, so encryptQuery without an explicit
queryType builds an equality term. That one is real and unrelated.
Not verified here: the pgrest suite needs PGRST_URL (unset locally); its
four rewritten assertions were checked against the corrected oracle.
Upstream, protect-ffi should either honour `include_original` or reject
it — silently accepting dead config is what made this plausible.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
| Name | Type |
|---|---|
| @cipherstash/stack | Minor |
| @cipherstash/bench | Patch |
| @cipherstash/prisma-next | Patch |
| @cipherstash/basic-example | Patch |
| @cipherstash/prisma-next-example | Patch |
| @cipherstash/e2e | Patch |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
|
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
| ])('emits the same bloom for %j whether include_original is on or off', async (plaintext) => { | ||
| const on = await bloomOf(withOriginal, plaintext) | ||
| const off = await bloomOf(withoutOriginal, plaintext) | ||
|
|
||
| expect(sorted(on)).toEqual(sorted(off)) |
There was a problem hiding this comment.
The test values are generated here with encrypt not encryptQuery so all this does is prove that there's a bug in protect-ffi: i.e. that include_original is ignored when it shouldn't be!
|
I don't think this is correct and Claude is confused. It isn't distinguishing between a bloom filter generated for storage and one generated for query. Queries must not set Counter exampleThis is why original values can't be included in bloom filters for query payloads. A bloom filter is just a set with collisions so the easiest way to reason about this is to look at the plaintext term set and ignore collisions. Broken caseStored term is "Danger", query includes the original. Correct caseStored term as above but the query doesn't include the original. |
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
The logic here is broken as per my comment.
2 separate issues:
include_queryis being ignored byencrypt(protectjs-ffibug)encryptis used for generating queries (instead ofencryptQuerywhich should never setinclude_query=trueeven if the storage path does)
A single 3-character needle was insufficient coverage for free-text search, and the reason is subtle: its whole-value token IS a trigram, so it matches whether or not `include_original` is honoured. That is the degenerate case, and it was the only case the suite exercised. The match index blooms downcased 3-grams; `include_original: true` additionally blooms the whole value as one token. Storage wants that. A QUERY operand must not have it, or the needle's bloom carries a whole-needle token the haystack's bloom cannot contain, and every strict-substring search returns zero rows. Both v3 adapters build match operands with `encrypt` — the full storage envelope — rather than `encryptQuery`, because PostgREST cannot cast a filter value to `eql_v3.query_*`. Both are therefore structurally exposed. It is masked today only because protect-ffi ignores `include_original` outright, so neither side carries the token: two bugs cancel. When protect-ffi is fixed, substring search breaks silently. Two layers of coverage: - The text family now searches four needles per match-capable domain: one trigram (degenerate), a strict interior substring longer than a trigram (the discriminating case), the whole stored value, and a needle absent from every value. - `match-bloom.integration.test.ts` asserts the precondition directly on the wire — bloom(needle) must be a subset of bloom(haystack) for every substring — so a failure names its cause instead of surfacing as "a query returned no rows". It also proves its own sensitivity: the honest operand is contained, and unioning in the bits of one value the haystack never saw is enough to break the subset. An `include_original` token is exactly such a foreign token. These go red the day protect-ffi honours the flag. That is the intent: the fix is to stop the operand carrying the original token, not to relax the tests.
2 participants
Closes CIP-3483. Closes #610.
TL;DR
The bug doesn't exist. v3
contains()matches substrings correctly, and always has. What existed was an unverified code comment that propagated into the docs, a changeset, and a test that manufactured its own confirmation.What CIP-3483 claims
.contains()on a v3text_search/text_matchcolumn matches only when the needle equals the stored value or is exactly 3 characters. Cause:include_original: trueappends a whole-value token to the needle's bloom filter, which the haystack's bloom cannot contain.What I measured
include_originalis inert in protect-ffi. Encrypting the same plaintext with the flag on and off yields the same bloom bits — verified against 0.24.0 (whichmainpins, so it is the version the issue was filed against) and 0.29.0, under both EQL v2 and v3. The bloom is trigram-only either way, and a value shorter thantoken_lengthblooms to nothing under both settings.The database agrees. Against the live
eql-3.0.0bundle, calling the exact overload both adapters reach —eql_v3.contains(a public.eql_v3_text_search, b jsonb)with storage envelopes on both sides:"alice"and"example"are the issue's own failing examples."zzzzz"correctly returns false, so this is not a fail-open masquerading as success. Same result foreql_v3_text_match.How a non-bug got this far
It began as a comment on
EncryptedQueryBuilderV3Impl("KNOWN BROKEN for real substrings … Tracked in EQL; do not paper over it here") and spread tosupabase/types.ts,match-defaults.ts, the reference doc, thestash-supabaseskill, and the unreleased changeset.Then
supabase-v3-pgrest-live.test.ts"confirmed" it. That suite has no CipherStash credentials and mints its own envelopes — and itsbloomTokens()helper prepended the whole-value token itself, to both the seeded row and the query needle:So the substring failure it asserted was its own invention. Nothing tied that fake to ffi. It was also internally inconsistent: under a real
include_originalthe seeded rows would carry the token too, and"example"'s trigrams would still be contained.Meanwhile
drizzle-v3/operators-live-pg.test.ts:532has been asserting the truth against real ffi and real Postgres the whole time ('lovelace','grace'match). The two tests contradicted each other and the fake won, because it was the one people read.What changed
bloomTokens()emits trigrams only, mirroring real ffi. Sub-trigram values now bloom to nothing, as they actually do. Thedoes not match a longer substringtest inverts:'example'matches'ada@example.com'. Adds a'zzz'negative — every other assertion in that suite is satisfied by acontains()that matches everything.include_original: false. Inert today, but it's the value a substring-search domain wants (and the zod schema's own default) if an ffi release ever starts honouring the flag. v2 keeps its historicaltrue, so no v2 wire moves. Verified there is no other consumer: the CLI SQL'sconfig_match_default()is a v2-only DB-side helper that never reads the client-emitted field.match-bloom-live.test.tspins the invariant where it actually lives — on ffi's bloom output, not through a stack builder. This is the test that would have caught the fiction.query-builder-v3.ts,supabase/types.ts,match-defaults.ts(include_originaldoes not make sub-trigram values matchable — that fail-open is exactly whatmatchNeedleErrorguards), the reference doc, the skill, and the changeset.Left alone: the
columns.ts/stash-encryptionnote that index inference picksuniquebeforematch, soencryptQuerywithout an explicitqueryTypebuilds an equality term. That one is real, and unrelated.Two things found while implementing
Bloom bit order is nondeterministic. Two encrypts of the same plaintext on the same client return the same bits in a different sequence. Only the bit set is meaningful (
@>issmallint[]containment), so the new test sorts before comparing. Any future assertion onbfmust do the same.schema-v3.test.ts:14was aLOAD-BEARINGtest asserting v3 builds byte-identically to v2, which the flip deliberately breaks. I didn't delete it — its real job is guarding the v2 wire from drift. It now asserts both sides of the one intentional divergence (v3 === false,v2 === true) and excludes onlyinclude_originalfrom the byte-identity check, so neither side can drift unnoticed.Verification
match-bloom-live.test.ts: 5 tests green against real ffi + credentials.packages/drizzle's one failure isMissing env.DATABASE_URL, environmental;schema-extraction.test.tspassed untouched, confirming the v3 flip didn't reach the v2-override test.Not run locally, and the highest-value thing for a reviewer to check:
supabase-v3-pgrest-live.test.tsneedsPGRST_URL(unset here). Its four rewrittencontainsassertions were verified against the corrected bloom oracle by hand (exampleandadacontained,zzznot,ad→[]).drizzle-v3/operators-live-pg.test.tsneeds staleprotect_ci_v3_*tables dropped from the dev database first, orbeforeAllfails on an older schema.Follow-ups (not in this PR)
cipherstash/docs#39, which documents the non-existent limitation.include_originalor reject it. Silently accepting dead config across at least 0.24 → 0.29, in both EQL versions, is what made this plausible enough to survive.