Repository navigation
feat: support protect-ffi 0.30 JSON queries - #711
Conversation
|
Important Review skippedToo many files! This PR contains 134 files, which is 34 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (137)
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 |
🦋 Changeset detectedLatest commit: 434975a The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
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 |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Overview
Upgrades Stack to @cipherstash/protect-ffi 0.30 and EQL 3.0.2, and lands the real feature: GIN-indexable JSON-path equality plus ciphertext-free JSON-path ordering (the new steVecValueSelector op) across Drizzle and Prisma Next, a versioned EQL 3.0.2 upgrade migration, and three fail-closed hardening changes (short free-text needles rejected at the FFI boundary; legacy v2 searchableJson() rejected at client setup; Supabase fails fast for operators PostgREST can't express).
Given the breadth (117 files, four packages, an FFI+EQL bump), I read the core boundary and changeset myself and ran four focused verifications — core stack, Supabase fail-closed, Drizzle+Prisma-Next operator routing, and migrations/deps/changeset. No confirmed bugs surfaced anywhere. Approving.
What I verified
Security posture is fail-closed throughout — the strongest part of this PR:
- Short free-text needle rejection happens in protect-ffi 0.30 on the query-encrypt path and rejects (not warns) below the ngram
token_length(3), closing thestored_bf @> '{}'match-all fail-open (#697). Pinned by a livematch-bloomtest. - Legacy v2
searchableJson()is rejected inresolveEqlVersion(stack/src/encryption/index.ts) at theEncryption()factory — before any FFI client is constructed, and even wheneqlVersion: 2is set explicitly — with a clear steer totypes.Json(). Correctly gated onv3Count === 0(v3 columns never trip it) and tested (asserts throw +newClientnot called). - Supabase fail-fast (
query-builder-v3.ts):matches(), encryptedcontains(),selectorEq()/selectorNe()throw an actionable error (naming Drizzle/Prisma-Next/scoped-RPC) rather than emitting a query PostgREST would run incorrectly. Version detection reads theeql_v3schema comment and fails closed on unknown/unparseable (→ true); the anchored^\d+\.\d+\.\d+regex is safe because v3 versions are bare (noeql-prefix hazard). A common async term-path guard catches every raw filter spelling (filter('cs'),not('matches'),or([...])). No over-triggering on equality/order/plaintextcontains.
The operator rewrite is a real improvement, not a risky correctness change. Drizzle and Prisma Next lower identically (eq/ne → @> value-selector containment over eql_v3.query_json; ordering → eql_v3.{gt,gte,lt,lte} over jsonb_path_query_first + query_text_ord/query_double_ord), the type surface exactly matches the six runtime eqlJsonPath* operators (no orphans either way), and selector-path.ts leaf validation now correctly rejects Date/bigint/non-finite numbers with actionable errors. Crucially: the old paths were functionally correct but placed a storage ciphertext c in the WHERE clause and couldn't engage a GIN index — the rewrite removes ciphertext from the predicate and makes equality index-answerable. Net: better, and no silent wrong-results regression.
Migration + release hygiene is clean. The new 20260720T0000_upgrade_eql_v3_3_0_2 edge is an invariant-only self-edge that chains onto head (head's invariant list grows, graph stays reachable, example mirror consistent). The upgrade SQL is data-safe: it drops/recreates only EQL's own eql_v3/eql_v3_internal schemas — zero DROP TABLE/TRUNCATE/DELETE/ALTER on user tables (re-encryption of SteVec rows is correctly left as a runtime concern). No protect-ffi/eql version skew among the bumped packages; the lockfile is fully updated (--frozen-lockfile-compatible); onlyBuiltDependencies unchanged (protect-ffi ships prebuilt). The single changeset covers every changed published package (stack/drizzle/prisma-next minor, supabase/stash patch) with all three breaking notes captured; minor on stack is consistent with the rc prerelease train.
Notes — non-blocking
steVecTermpublicQueryTypeNamesemantics flipped from containment → ordering. The recommended auto-inferringsearchableJsonpath handles this internally (object/array now routes to a newdefaultcontainment op), so real-world impact is low, but anyone who explicitly passedqueryType: 'steVecTerm'expecting containment would now get an ordering term and silently different results. Worth an explicit line in the upgrade notes for the explicit-queryType path (v3 is pre-GA, so acceptable).- Two test-coverage gaps worth a cheap follow-up: the short-needle rejection is pinned only by a credential-gated live test (won't run in no-creds CI) — given it closes a security fail-open, a non-live unit pin on the length guard would be valuable; and
unsupportedLeafReasonhas no direct unit test instack(exercised only via the adapter packages). - Pre-existing, out of scope:
packages/protectstill pins protect-ffi0.23.0while stack/drizzle/supabase move to0.30.0, so the repo genuinely resolves two protect-ffi majors. Not touched by this PR and@cipherstash/stackdoesn't depend onprotect, but worth tracking given the version-skew lesson from the rc.2 B1 bug. - Cosmetic: the
ne/neqSQL differs in operand order between the two adapters ((NOT contains OR col IS NULL)vs(col IS NULL OR NOT (...))) — logically identical and precedence-safe; and the supabase SKILL.md filter table still listsmatch()(PostgREST's regex operator, distinct from the removedmatches()) — pre-existing.
Verdict
Approve. A large but disciplined upgrade: the fail-closed hardening is correct and closes a real match-all fail-open, the JSON-path operators lower identically across adapters with an exact type↔runtime match and now keep ciphertext out of the WHERE clause, the upgrade migration is data-safe, and dependency/changeset hygiene is clean with no version skew. All four notes are non-blocking follow-ups; item 1 (documenting the steVecTerm semantics flip) and item 2 (a non-live unit pin for the needle guard) are the two most worth picking up.
|
Addressed the actionable follow-up feedback in signed commit
I left the two notes explicitly identified as pre-existing/out of scope or cosmetic unchanged. Validation:
|
|
Addressed the JSON integration parity feedback in verified commit
Local validation passed: Drizzle 377 tests + declaration tests + build; Prisma Next 611 tests + typecheck + build; test-kit typecheck; repository code check. The shared credentialed live suites are running in CI. |
9531a95 to
434975a
Compare
2 participants
Summary
@cipherstash/protect-ffito 0.30 and@cipherstash/eqlto 3.0.2Upgrade notes
searchableJson()schemas are rejected during client setup because protect-ffi 0.30 cannot emit the removed v2 selector envelope. Migrate these columns to EQL v3types.Json.matches(), encryptedcontains(), andselectorEq()/selectorNe(). PostgREST cannot express the requiredeql_v3.query_*casts; use Drizzle, Prisma Next, or a carefully scoped SQL/RPC path for those queries.Validation
pnpm code:checkgit diff --checkCloses #697
Closes #677