fix(stack): close v3 drizzle fail-open paths, fix false-negative tests

[tobyhede]

Audit of the EQL v3 Drizzle adapter's test coverage, and the fixes it turned up.

The index/query-type wiring is correct — but several tests could not detect the bug they exist to guard, and two source paths failed open.

Two hypotheses the audit refuted

Recorded so nobody re-derives them:

• Operators pass no per-operator query type to encryption. encryptOperand/encryptOperands call client.encrypt(value, { table, column }) with no operator argument. The full term envelope is emitted; the operator selects a term server-side via the eql_v3.* function it renders. Guards are sound: contains→match, ordering→ore, eq/ne reject match-only columns. eq on an ore-only column is intentional — numeric/date domains answer equality through the ORE term.
• No declarable-but-unemitted index type exists. schema-extraction.test.ts already asserts extracted.build() equals authored.build() across all 38 domains.

Source fixes

contains fail-open. A needle shorter than the tokenizer's token_length (3) builds an empty bloom filter, and stored_bf @> '{}' is true for every row. Measured live against 3 seeded rows, with the guard removed:

needle "ad" (2 chars) -> 3/3 rows
needle "a"  (1 char)  -> 3/3 rows
needle "x"  (1 char)  -> 3/3 rows   <- 'x' occurs in none of them
needle "qqqzzz" (6)   -> 0/3 rows

A user searching "ad" silently got the whole table. The length floor lives in schema/match-defaults and is shared with v2, which builds byte-identical blooms.

v3FromDriver fail-open. Malformed JSON surfaced a raw SyntaxError; a wrong-shape payload passed through unchecked (v3FromDriver('5') returned 5 typed as Encrypted, reaching decrypt as garbage). Now throws EqlV3CodecError, exported so callers can catch it. The shape guard accepts SteVec documents, whose ciphertext is at sv[0].c with no top-level c.

Dead code. Removed the unreachable sql\false`fallback ininArray/notInArray`.

The false-negative tests

Each fix was verified by mutating the source, confirming the new test goes RED, and reverting.

test	why it could not fail	mutation proving the fix
and combines predicates	both predicates true for ROW_B alone, so AND and OR both returned [ROW_B] — swapping the operator still passed	and→or: fails
bigint filters	ran against a one-row table; gt returning every row passed	comparison→true: fails
between	only ever called with identical bounds against a constant encrypt stub, so a min/max transposition in range was invisible	transpose min/max: fails
contains	no needle that must not match, none longer than token_length. The unit test searched TEXT_S[0] — the empty string — i.e. it asserted the fail-open	contains→true: all 8 cases fail

Third matrix row. With two rows every predicate could only return [A], [B], [A,B] or []: ordering ties were untestable and eq over-matching undetectable (no near-miss row). Row i now takes samples[min(i, len-1)] — the scheme v3-matrix/matrix-live-pg already uses. This also inserts 'Ada Lovelace', the only uppercase sample, so the downcase token filter finally runs.

Lock context

Every test supplied the same context on seed and query. Because lock context alters only the FFI ciphertext term and never the SQL, a regression dropping it from the index term would drop it identically on both sides and stay green. The file header documented this as intentional.

Added the negatives obtainable with one JWT: an identity-bound row must not match an eq issued with no lock context, and must not decrypt without it.

Separately, lock-context.test.ts wrapped decryptModel in try/catch — but it reports denial as a Result and never throws, so the test ran zero assertions and would also have passed had decryption wrongly succeeded.

Known-failing locally, expected green in CI

The live asc/desc proofs fail on a non-superuser Postgres: ORDER BY eql_v3.ord_term(col)'s ORE-aware btree opclass is superuser-gated and silently falls back to raw-byte order. This is pre-existing and already root-caused in docs/eql-v3-ord-term-ordering-defect.md.

run	failures	ordering defect	other
HEAD, unmodified	25	25	0
this branch	37	37	0

Count rises because the third row exposes more domains to the same defect. CI runs superuser, so these should pass there — that is the main thing to watch on this PR.

Not included

Both deliberately out of scope:

• SQL half of the contains fix (bloom_filter returning NULL on an empty array). It contradicts a documented semantic — "an empty bf array yields an empty filter (contains nothing, contained by everything)" — so it is a spec decision with an owner, not a drive-by edit. Shipping it alone would also turn "returns everything" into "returns nothing, silently".
• Supabase adapter needle guard, which lives on feat/eql-v3-supabase-adapter.

Follow-ups

• The lock-context suite never runs in CI. tests.yml provisions the CS_* secrets and DATABASE_URL but not USER_JWT, and the suite soft-skips — reporting "6 passed" while executing nothing. The new negatives inherit that.

• A true cross-identity test (sealed under A, queried under B) is writable today: USER_2_JWT already exists (AGENTS.md, and used in __tests__/bulk-protect.test.ts to decrypt mixed lock-context payloads). Through the operator path it needs a second client with its own OidcFederationStrategy built from that token — a lock context only names the claim, and ZeroKMS resolves its value from the authenticating JWT, so a different claim object cannot fabricate identity B.

  Correction: an earlier revision of this description claimed no second-JWT env var existed. That was wrong — it came from a keyword search for USER_JWT_B/OTHER_JWT/SECOND_JWT, which missed the actual name. The cross-identity test is not blocked on provisioning a new secret.

• Bloom saturation: at m=2048, k=6, contains degrades past ~500 chars of stored text. Separate concern, tracked in the design doc.

Summary by CodeRabbit

• New Features

  • Added stricter validation for free-text search inputs so unsupported terms are rejected instead of returning overly broad results.
  • Improved encrypted value handling to better support both standard encrypted payloads and structured document payloads.

• Bug Fixes

  • Prevented empty or too-short search terms from matching all rows.
  • Invalid encrypted payloads now fail with clearer errors instead of surfacing raw parsing issues.
  • Improved reliability of encryption-related queries, ordering, and null handling.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0ac69341-e346-4ede-bb33-1877b9064271

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch drizzle-test-coverage

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 2bc4872

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

This PR includes changesets to release 6 packages

Name	Type
@cipherstash/stack	Minor
@cipherstash/bench	Patch
@cipherstash/prisma-next	Patch
@cipherstash/basic-example	Patch
@cipherstash/prisma-next-example	Patch
@cipherstash/e2e	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]

Actionable comments posted: 1

🧹 Nitpick comments (2)

packages/stack/__tests__/drizzle-v3/sql-dialect.test.ts (1)

61-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Well-structured parameter-binding tests.

The three test groups correctly pin the security-critical property that operand values are bound as positional parameters rather than inlined into SQL text:

• Hostile operands: The params equality check, $1::jsonb presence, and not.toContain('OR 1=1') assertions comprehensively guard against injection.
• Range: The expected SQL (eql_v3.gte("users"."x", $1::jsonb) AND eql_v3.lte("users"."x", $2::jsonb)) matches the parenthesised min-first output from v3Dialect.range.
• Large ciphertext: The 16KB payload test confirms no truncation or inlining occurs.

One minor note: the it.each title '%s binds a hostile operand as $1' produces slightly redundant names (e.g., "equality binds a hostile operand as equality") since $1 substitutes the first array element. If the intent was to evoke the SQL $1 placeholder, consider a title like '%s binds a hostile operand as a positional parameter' for clarity.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/stack/__tests__/drizzle-v3/sql-dialect.test.ts` around lines 61 -
101, The parameter-binding tests are good, but the `it.each` title in
`sql-dialect.test.ts` is misleading because `%s` expands to the case name rather
than the SQL placeholder, producing redundant descriptions. Update the `it.each`
test name in the `operand values are bound, never interpolated into SQL text`
block to describe positional binding clearly, and keep the assertions in
`renderFull`, `v3Dialect.equality`, `v3Dialect.comparison`,
`v3Dialect.contains`, and `v3Dialect.range` unchanged.

packages/stack/src/eql/v3/drizzle/codec.ts (1)

69-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider preserving the original parse error as cause.

The EqlV3CodecError thrown on JSON parse failure embeds the cause's message in the string but doesn't pass it as a structured cause. If the project targets ES2022+, using super(message, { cause }) would preserve the original stack trace and improve debuggability.

♻️ Optional refactor

     throw new EqlV3CodecError(
       `Failed to parse an EQL v3 encrypted envelope from the driver: ${cause instanceof Error ? cause.message : String(cause)}`,
+      // If your tsconfig targets ES2022+, pass cause for better stack traces:
+      // { cause },
     )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/stack/src/eql/v3/drizzle/codec.ts` around lines 69 - 91, The JSON
parse failure path in v3FromDriver currently only copies the parse error message
into EqlV3CodecError, but drops the original error as structured context. Update
the catch block in v3FromDriver to pass the caught error through as a cause when
constructing EqlV3CodecError, so the original stack and metadata are preserved
while keeping the existing message from the parse failure.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/stack/__tests__/drizzle-v3/operators-lock-context-live-pg.test.ts`:
- Around line 224-254: The two decrypt-denial tests in
operators-lock-context-live-pg.test.ts can falsely pass when the SQL query
returns no row because `row.value` throws inside `decryptDenied`. In both test
cases that destructure `[row]` after the `sqlClient.unsafe` query, add an
explicit assertion that `row` exists before calling `client.decrypt(...)`, so
the failure is reported as a missing fixture rather than a denied decrypt. Use
the existing `decryptDenied` helper and the `client.decrypt`/`withLockContext`
calls as the location to update.

---

Nitpick comments:
In `@packages/stack/__tests__/drizzle-v3/sql-dialect.test.ts`:
- Around line 61-101: The parameter-binding tests are good, but the `it.each`
title in `sql-dialect.test.ts` is misleading because `%s` expands to the case
name rather than the SQL placeholder, producing redundant descriptions. Update
the `it.each` test name in the `operand values are bound, never interpolated
into SQL text` block to describe positional binding clearly, and keep the
assertions in `renderFull`, `v3Dialect.equality`, `v3Dialect.comparison`,
`v3Dialect.contains`, and `v3Dialect.range` unchanged.

In `@packages/stack/src/eql/v3/drizzle/codec.ts`:
- Around line 69-91: The JSON parse failure path in v3FromDriver currently only
copies the parse error message into EqlV3CodecError, but drops the original
error as structured context. Update the catch block in v3FromDriver to pass the
caught error through as a cause when constructing EqlV3CodecError, so the
original stack and metadata are preserved while keeping the existing message
from the parse failure.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c17aa09e-17cf-4925-9fc0-6875b0e02f81

📥 Commits

Reviewing files that changed from the base of the PR and between 222be16 and 0ebf57e.

📒 Files selected for processing (14)

• .changeset/eql-v3-drizzle-fail-open-guards.md
• packages/stack/__tests__/drizzle-v3/codec.test.ts
• packages/stack/__tests__/drizzle-v3/column.test.ts
• packages/stack/__tests__/drizzle-v3/exports.test.ts
• packages/stack/__tests__/drizzle-v3/operators-live-pg.test.ts
• packages/stack/__tests__/drizzle-v3/operators-lock-context-live-pg.test.ts
• packages/stack/__tests__/drizzle-v3/operators.test.ts
• packages/stack/__tests__/drizzle-v3/schema-extraction.test.ts
• packages/stack/__tests__/drizzle-v3/sql-dialect.test.ts
• packages/stack/__tests__/lock-context.test.ts
• packages/stack/src/eql/v3/drizzle/codec.ts
• packages/stack/src/eql/v3/drizzle/index.ts
• packages/stack/src/eql/v3/drizzle/operators.ts
• packages/stack/src/schema/match-defaults.ts

[freshtonic]

Reviewed against a local checkout. Unit suites (297 tests across the 7 touched files) and pnpm --filter @cipherstash/stack test:types both pass. I did not run the live-pg suites.

The two source fixes are correct and the test work is genuinely good — the mutation-verification table in the description is the right way to argue that a test can fail, and the and/between/contains false negatives it turned up were real. A few things below.

Dead-code removal is sound

I checked the claim rather than take it on faith. In inArrayOp, values.length === 0 throws, and encryptOperands rejects any bulk response whose length differs from values.length. So conditions is provably non-empty, and(...)/or(...) cannot return undefined, and the as SQL cast is not hiding a fail-open. Removing the sql\false`` fallback is safe.

Similarly, matchNeedleMinLength can't silently skip the floor: token_length is a required field of the ngram variant in tokenizerSchema (schema/index.ts:102-105), not an optional one, so the kind === 'ngram' narrowing always yields a number.

The v2 like/ilike fail-open is the same bug, and it's the one that ships

The changeset notes this and defers it, with a good reason — v2 needles carry SQL wildcards, so the floor has to be measured against what the tokenizer actually receives. That reasoning is right and I'm not asking for the fix here.

But I want to be precise about what's being deferred. In src/drizzle/operators.ts:825, a freeTextSearch column encrypts right verbatim and renders eql_v2.ilike(col, $1), which is the same bloom containment. ops.ilike(users.email, 'ad') — no wildcards, two characters — builds an empty filter and returns the whole table. The wildcard caveat doesn't cover that call; it's unguarded today, in a published 0.18.0, on the adapter that has users. notIlike fails closed (returns nothing), so only the positive operators leak.

v3 Drizzle is new surface. The severity ordering is inverted from the fix ordering here.

Concretely: could you link the tracking issue from the changeset and the matchNeedleError docblock? Both say "tracked separately" with no reference. Given this PR just built the mechanism, a v2 needle guard that only applies when the term contains no wildcard characters would close the concrete case above without needing the tokenizer archaeology.

assertEnvelope accepts an empty SteVec

sv: [] satisfies envelope.sv !== undefined and passes, but the docblock's premise is that the root ciphertext lives at sv[0].c. Array.isArray(sv) && sv.length > 0 would make the guard match what it documents. Low stakes — I can't construct a path that produces it — but the whole point of the change is that a wrong value here reaches decrypt as garbage.

needleFor counts UTF-16 code units

__tests__/drizzle-v3/operators.test.ts:

const needle = spec.samples.find(
  (sample) => typeof sample === 'string' && sample.length >= 3,
)

This is the exact check the guard exists to reject. It's harmless today because every match-domain sample is ASCII, but if someone adds an astral-plane sample to the catalog, needleFor selects a needle the guard then throws on, and the failure will point at the wrong place. [...sample].length >= 3 for consistency with matchNeedleError.

The live positive path for ops.and is gone

The old and combines encrypted predicates asserted [ROW_B]. Replacing it with the disjoint pair asserting [] is the right call — the old one couldn't distinguish and from or. But now no live test proves ops.and ever returns rows.

An and that emitted a constant false predicate would pass and → [], and or → [A,B,C] wouldn't catch it because it exercises or. The unit test and ignores undefined conditions and keeps the encrypted predicates does pin the rendered SQL contains and, so this is defended, just not live. Cheapest fix is to keep both: the disjoint pair for the operator-swap mutation, plus the old intersecting pair (eq(text_eq, 'ada@example.com') and lt(integer_ord, 0) → [ROW_B]) for the positive path.

KEY_DENIAL on the ['email'] claim test

.withLockContext({ identityClaim: ['email'] } as never)

asserted against /^Failed to retrieve key/.

If the test JWT carries no email claim, does ZeroKMS report a key-derivation denial, or does resolution fail earlier with a different message? The suite doesn't run in CI today (the USER_JWT gap you flagged), so this won't bite until it does — but that's exactly when a message-shape assumption is most expensive to debug. Worth confirming against a real token, or loosening to "denied, and not an infrastructure fault" if the claim-missing message differs.

Formatting

codec.test.ts fails biome check — the it.each in preserves the underlying SyntaxError as \cause`needs reflowing. Looks like the7a76041fixup wasn't formatted.pnpm code:fixbefore merge. Biome isn't intests.yml`, so CI won't catch it.

Minor

exports.test.ts now has exports exactly the public surface, which strictly subsumes both exports the public surface and re-exports the codec and column helpers as callables. The latter two check typeof === 'function', which the exhaustive key check doesn't, so they aren't pure duplication — but three tests over one barrel is a lot of surface for what a single toEqual on a sorted key list plus one typeof sweep would cover.

---

Nothing here blocks. The changeset is honest about the breaking change and minor is the right bump pre-1.0. The main ask is the tracking link for v2, and confirming the contains/asc/desc proofs go green on CI's superuser Postgres — as your table says, that's the thing to watch.

[freshtonic]

Approved with non-blocking feedback