fix(v3): supabase short-needle guard, withLockContext input widening, error-union discrimination#659
Merged
Conversation
… error union discrimination
Three code-level correctness gaps found while fact-checking the v3 docs (the
docs described behaviour the code didn't provide):
1. Supabase matches() had no short-needle guard — matchNeedleError was wired
into Drizzle only. A sub-token_length needle blooms empty and `bf @> {}`
matches every row (fail-open, whole-table decrypt). Applied the same guard
at assertTermQueryable, the single term-resolution choke point (the class
doc already CLAIMED this guard existed). FFI-level backstop for the
encryptQuery paths filed as protectjs-ffi#138.
2. Supabase builder withLockContext only accepted a LockContext instance, so
the canonical `.withLockContext({ identityClaim: [...] })` example was a
TS2353. Widened the field + param to LockContextInput (the stack ops
already accept it and forward it through).
3. EncryptionErrorTypes lacked `as const`, collapsing every StackError member's
`type` to `string` — the union didn't discriminate and the documented
exhaustive `switch (error.type)` (with `.code` access) failed to compile.
Tests: supabase needle-rejection unit test + a matrix fix (its text_match
sample is the empty string, which the guard now correctly rejects — matches/
like cases use a real needle); withLockContext plain-object type test; a
StackError discriminated-union type test that is the documented handler and
goes red if `as const` is dropped. 478 supabase unit + 816 stack + 368 drizzle,
all type suites green.
Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
🦋 Changeset detectedLatest commit: 3e040fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes reject short Supabase v3 free-text needles, allow plain lock-context inputs, preserve literal encryption error discriminants, and add regression and compile-time tests. A changeset documents patch releases for both affected packages. Changesv3 Correctness Fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
… tsconfig The new error-discriminated-union.test-d.ts imports from `@cipherstash/stack/errors`, which the typecheck tsconfig didn't map to source (unlike schema/v3/eql-v3), so it resolved to the built `dist/errors`. turbo's `test:types` doesn't depend on `build`, so CI typechecked against a stale dist lacking the `as const` — the exhaustive switch's `never` check failed there while passing locally (fresh dist). Map the subpath to `./src/errors/index.ts` like the other public subpaths, so the type test always sees current source. Verified: passes with dist deleted entirely. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Contributor
There was a problem hiding this comment.
Pull request overview
This PR closes three correctness gaps in the EQL v3 Supabase + Stack TypeScript surfaces that were discovered during docs verification work: it prevents Supabase free-text .matches() from “fail-open” behavior on short needles, widens Supabase builder lock-context input to match the documented { identityClaim } shape, and restores proper discriminated-union narrowing for StackError by making EncryptionErrorTypes a literal-typed constant.
Changes:
- Add a Supabase v3 free-text needle minimum-length guard (via
matchNeedleError) at the shared term validation choke point. - Widen Supabase builder
.withLockContext(...)and stored state to acceptLockContextInput(eitherLockContextor{ identityClaim }). - Make
EncryptionErrorTypesas constand add type-level regression tests; ensure@cipherstash/stack/errorsresolves to source during type tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/stack/tsconfig.json | Maps @cipherstash/stack/errors to source so .test-d.ts type tests don’t accidentally compile against stale dist/ output. |
| packages/stack/src/errors/index.ts | Adds as const to EncryptionErrorTypes to preserve literal types and enable discriminated-union narrowing. |
| packages/stack/tests/error-discriminated-union.test-d.ts | Adds a type-level regression test proving StackError narrows by type and supports exhaustive switching. |
| packages/stack-supabase/src/types.ts | Updates exported/consumed types so the public builder surface accepts LockContextInput. |
| packages/stack-supabase/src/query-builder.ts | Widens stored lock-context state and .withLockContext(...) implementation to LockContextInput, matching stack operations. |
| packages/stack-supabase/src/query-builder-v3.ts | Adds the short-needle guard for free-text search terms (matches/like), preventing fail-open table-wide matches. |
| packages/stack-supabase/tests/supabase-v3.test-d.ts | Adds type-level coverage for .withLockContext({ identityClaim }) acceptance and unknown-key rejection. |
| packages/stack-supabase/tests/supabase-v3-matrix.test.ts | Updates matrix tests to use a valid tokenizable match needle now that short needles are correctly rejected. |
| packages/stack-supabase/tests/supabase-v3-builder.test.ts | Adds a runtime regression test that short .matches() needles are rejected up front. |
| .changeset/v3-supabase-needle-lockcontext-errors.md | Adds a patch changeset for @cipherstash/stack-supabase and @cipherstash/stack describing the three fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three code-level correctness gaps surfaced while fact-checking the EQL-v3 docs restructure (#658) — each a case where the docs (and in two cases the code's own comments/signatures) described behaviour the implementation didn't actually provide.
1. Supabase
matches()had no short-needle guard 🔴 (fail-open)A free-text needle shorter than the tokenizer's
token_lengthblooms to zero tokens, sobloom @> {}is true for every row — the query silently returns and the caller decrypts the whole table.matchNeedleErrorwas wired into the Drizzle adapter only; the Supabase adapter's own class doc claimed the guard existed. Now applied atassertTermQueryable(the single choke point every term spelling funnels through, same place the JSON-containment guard lives), so both first-party surfaces reject identically.Architectural note: the authoritative fix for the
encryptQuerypaths (Drizzle/core/prisma-next) belongs in protect-ffi (reject a query term that tokenizes to nothing) — filed as cipherstash/protectjs-ffi#138. It can't cover Supabase, whose operand goes through the storage-encrypt path (PostgREST can't cast to the narrowed query domain), so this client-side guard stays.2.
.withLockContext({ identityClaim })didn't typecheck on the Supabase builderThe builder accepted only a
LockContextinstance (TS2353on the canonical identity example that #642/#658 document as the primary form). Widened the stored field + method toLockContextInput(LockContext | { identityClaim }) — the type the stack-level operations already accept and that the builder forwards through unchanged.3.
EncryptionErrorTypeslackedas constEvery member's value widened to
string, so theStackErrorunion'stypefields were allstringand the union never discriminated:switch (error.type)couldn't narrow anderror.codeon the relevant branch wasTS2339. The documented exhaustive error-handling pattern didn't compile. One-line fix.Tests
matches()rejected (500 + message); a matrix-test fix — itstext_matchsample is the empty string, which the guard now correctly rejects, so the matches/like cases use a real ≥3-char needle.withLockContext({ identityClaim })accepted / unknown key rejected; aStackErrordiscriminated-union test that IS the documented exhaustive handler (goes red ifas constis dropped).test:typesclean.Found during the #658 code-review pass. Changeset:
@cipherstash/stack-supabase+@cipherstash/stackpatch.https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Summary by CodeRabbit
.matches().{ identityClaim }input shape (in addition to the prior form).typefields remain properly narrow.