fix(stack): escape encrypted in-list operands; widen is/contains#609
Conversation
Follow-up to #588. Two of these are broken features, not polish; both were invisible to the existing tests because `createMockSupabase` records the arguments handed to each builder method and never builds a URL. `.in()` on an encrypted column produced a request PostgREST rejects. Every encrypted operand is a serialized envelope, dense with `"` and `,`, and postgrest-js wraps a comma-bearing element as `"…"` without escaping the quotes already inside it: in.("{"v":1,"c":"…"}",…) ^ PostgREST ends the value here -> PGRST100 Encrypted lists now go through `filter(col, 'in', …)` with each element quoted and escaped, as the `.or()` path already did. This affects v2 as well as v3 — v2's `("a@b.com")` composite literal is itself quote-bearing and was equally broken — so three existing tests that asserted the old encoding are updated. `.not(col, 'in', […])` encrypted the whole list as a single ciphertext, so the filter silently matched nothing, and emitted an unparenthesized `not.in.a,b`. The not-collector now encrypts element-wise, mirroring the regular `in` and `or(… .in. …)` paths. A PostgREST list literal on an encrypted column throws rather than silently matching nothing. `is(col, null)` is widened to every row key. `is` is never encrypted and a NULL plaintext is stored as a SQL NULL, so on a storage-only column `IS NULL` is not merely legal but the only predicate available. `is(col, true)` stays a compile error on encrypted columns. `contains()` accepts native operands on plaintext columns, which fall through to PostgREST's native containment. Relatedly, `.or([{ op: 'contains' }])` now emits `cs` for plaintext conditions too — previously only encrypted ones were translated, so plaintext containment reached the wire as `.contains.` and failed to parse — and `splitOrString` tracks `{}` so an array/jsonb literal's commas no longer split the condition mid-value. `DOMAIN_REGISTRY` is derived from `types` rather than hand-listed, so the two cannot drift. The keys are an external contract (the information_schema query parameter), and because they are now derived from `getEqlType()`, no test that also derives them can detect a corrupted domain constant — the test file pins them against a hand-written literal list for exactly that reason. Tests: a `postgrest-wire` harness runs the real PostgrestClient against a capturing fetch, so operand serialization is asserted without a database. Adds biting coverage for the three null-prototype maps (`mergeDeclaredTables`, `v3Columns` pattern-filter lookup, mutation-model rebuild) and for the unreachable scalar-queryType backstop.
🦋 Changeset detectedLatest commit: 4b27b7a The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens the Supabase adapter’s PostgREST wire-format handling for encrypted filters (notably in/not(..., 'in', ...)), broadens the v3 type surface for contains() and is(col, null), and reduces drift risk in the EQL v3 domain registry by deriving it from types while still pinning the external contract via tests.
Changes:
- Fix encrypted
in()/not(..., 'in', ...)to emit parseable PostgREST operands (escape quote-bearing ciphertexts; encryptnot.inelement-wise). - Make
contains()accept native jsonb/array operands on plaintext columns (and ensure.or()always translatescontains→cs, preserving containment literals during parse/rebuild). - Derive
DOMAIN_REGISTRYfromtypesand add wire-format tests via a realPostgrestClient+ capturingfetch.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/stack/src/supabase/types.ts | Adds conditional operand typing for contains(), and widens is(col, null) beyond filterable keys. |
| packages/stack/src/supabase/query-builder.ts | Applies encrypted in via filter('in', ...) with explicit operand formatting; fixes not(..., 'in', ...) element-wise handling; simplifies or() rebuild flow. |
| packages/stack/src/supabase/query-builder-v3.ts | Removes v3-only or() operator shaping hook now that contains → cs is handled centrally. |
| packages/stack/src/supabase/helpers.ts | Centralizes contains → cs, adds containment-literal formatting, tracks {} during .or() string splitting, and introduces formatInListOperand. |
| packages/stack/src/eql/v3/domain-registry.ts | Derives the domain registry from types, preserving null-prototype safety and adding collision protection. |
| packages/stack/tests/supabase-v3.test-d.ts | Expands type-level coverage for contains() operand typing and is(col, null) on storage-only columns. |
| packages/stack/tests/supabase-v3-wire.test.ts | New wire-format tests asserting the real serialized query string for encrypted in / not.in. |
| packages/stack/tests/supabase-v3-pgrest-live.test.ts | Extends live PostgREST suite with plaintext tags/meta containment coverage. |
| packages/stack/tests/supabase-v3-builder.test.ts | Updates mock-based assertions to reflect filter('in', ...) usage and adds additional edge-case coverage. |
| packages/stack/tests/supabase-schema-builder.test.ts | Pins null-prototype / __proto__-safety in schema merging. |
| packages/stack/tests/supabase-helpers.test.ts | Adds focused unit coverage for containment formatting and parsing stability. |
| packages/stack/tests/helpers/postgrest-wire.ts | New helper to capture actual PostgREST request URLs without a DB. |
| packages/stack/tests/eql-v3-domain-registry.test.ts | Pins the external domain-key contract via a literal list while validating derivation correctness. |
| .changeset/supabase-in-list-operands.md | Changeset documenting the wire-format fixes and type-surface widening. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| is<K extends FK>(column: K, value: null | boolean): Self | ||
| is<K extends StringKeyOf<T>>(column: K, value: null): Self |
| const builder = supabase.from('users') | ||
| builder.is('active', null) | ||
| builder.is('email', null) | ||
| builder.is('email', true) | ||
| }) |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed with a multi-angle pass (line-by-line, removed-behavior, cross-file tracing, reuse/simplification/efficiency/altitude), each candidate independently verified against the head commit, the vendored EQL v3 types, and the installed @supabase/postgrest-js. This is a solid fix — the two headline bugs (.in() on encrypted columns, whole-list .not(…,'in',…) encryption) are real and correctly addressed, and the new postgrest-wire.ts harness that asserts the emitted query string is exactly the right tool for this class of bug. Verification also cleared several plausible-looking concerns (the empty-list not.in.() is byte-identical to what postgrest-js itself emits; the {x} containment literal matches postgrest-js and the SDK's array-element quoting is actually more correct than postgrest's). Findings below, most severe first.
Correctness
1. splitOrString mis-parses a containment literal whose string value contains } — silently drops the rest of the .or() (helpers.ts:331)
The quote gate only tracks quotes at the top level: else if (char === '"' && depth === 0). Inside a {…} literal (depth > 0) quotes are not tracked, so a } that appears inside a JSON string value is miscounted as structural. Trace .or('meta.cs.{"note":"a}b"},email.eq.x') where meta is an encrypted column (which forces the parse→rebuild path — parseOrString runs unconditionally in toDbSpace): the } inside "a}b" drops depth to 0 prematurely, the following " then flips inQuotes true at depth 0, and the top-level comma before email.eq.x is no longer split. email.eq.x is swallowed into meta's operand and dropped from the query — PostgREST gets a syntactically valid but wrong filter tree and returns the wrong rows, with no error. This is the same silent-mismatch class the {}-tracking change set out to fix; the fix is incomplete because quotes must be tracked at every depth (and escapeOrValue can itself emit such a }-bearing literal, so emit and parse are mutually inconsistent). Related lower-severity gap: splitOrString tracks {} and () but not [], so a hand-written .or('meta.cs.[1,2,3],…') on an encrypted column splits mid-array — narrower because the SDK's own emit path never produces a top-level [...].
Deeper point: the SDK is re-encoding and re-parsing PostgREST's grammar by hand (splitOrString, formatOrValue, two separate reserved-char regexes POSTGREST_RESERVED and ARRAY_ELEMENT_RESERVED that already disagree). Every grammar edge (braces-in-strings, [], backslashes, unicode) is a latent bug the mock-based tests can't see. Worth considering whether the structured conditions can be carried through and serialized once by postgrest-js rather than round-tripped through a flat string the SDK must re-parse.
2. is(col, true) compiles on every non-storage-only encrypted column, contradicting the stated invariant — and a test bakes it in (types.ts:630, supabase-v3.test-d.ts)
The PR body and the doc comment both claim "is(col, true) remains a compile error on encrypted columns." It isn't. is<K extends FK>(column, value: null | boolean) is gated on FK = V3FilterableKeys, which excludes only NonQueryableV3Keys (storage-only columns). A text_search/text_eq/text_match/*_ord column carries a query capability, so it is in FK and matches the boolean overload. is('email', true) (where email is types.TextSearch) therefore typechecks and at runtime forwards q.is('email', true) → email=is.true against a jsonb ciphertext → Postgres error. The new test-d asserts builder.is('email', true) with no @ts-expect-error (the @ts-expect-error sits only on active, a storage-only types.Boolean, which is why the author's spot-check passed) — so the suite actively locks in the wrong behavior. The boolean is overload needs to be gated on storage-only-or-plaintext keys, not FK, and the test corrected to expect a compile error on email.
3. contains() admits plaintext scalar columns with an array/object operand → runtime 42883 (types.ts:118 / 151)
V3FreeTextSearchableKeys subtracts only encrypted-without-match-index keys, so every plaintext key — including a scalar text/varchar column — stays in the accepted set, and V3ContainsValue gives it NativeContainsValue (string | readonly unknown[] | Record<…>). So contains('note', ['vip']) where note is a plaintext scalar text column typechecks, then emits note.cs.{vip} → @> on text → Postgres 42883. The widening off the old blanket value: string is deliberate and the operand type intentionally mirrors postgrest-js's own untyped contains (which can't reject this either without column-type metadata), so it's bounded — but it's a real footgun that the old signature didn't have, worth a note in the doc comment at least.
Efficiency
4. v3 in-lists encrypt one FFI round-trip per element instead of one bulk call (query-builder-v3.ts:333)
The element-wise in/not(…,'in',…) change (correct for the bug) feeds N same-column terms into the v3 encryptCollectedTerms override, which does Promise.all(terms.map(t => client.encrypt(t.value, {column, table}))) — N concurrent ZeroKMS/FFI round-trips for in(col, [N values]). The v3 client already exposes bulkEncrypt(plaintexts, {column, table}) (encryption/v3.ts:112) whose shape is exactly a single-column in-list. Concurrent so not catastrophic, but an N=100 inArray is 100 invocations where one would do. (Same observation applies to the Drizzle v3 path in #565 — a shared bulkEncrypt routing for same-column term batches would fix both.)
Altitude / maintainability
5. Deriving DOMAIN_REGISTRY turns a shape change in a neighbouring export into a package-wide import crash (domain-registry.ts:31)
The derivation for (const factory of Object.values(types)) { … factory('_probe').getEqlType() … } runs at module load and assumes every value in types is a side-effect-free (name) => Column factory. If types ever gains a non-factory export (a constant, a helper), factory('_probe') throws during module evaluation — and domain-registry.ts is imported by introspect.ts, the schema builder, and the public @/eql/v3 barrel, so the throw takes down all v3 functionality at import, not one test. The hand-written map degraded gracefully (unknown key → undefined). The derived-plus-anchor-list pattern is otherwise good; consider constraining types's value type to V3ColumnFactory at the type level, or iterating a typed factory list rather than Object.values.
Cleanup
6. In-list term collection is copy-pasted across three sites (query-builder.ts:600)
The new not(…,'in') per-element block is near-identical to the regular-in block (~543) and the or-condition path (~661): same loop, isEncryptableTerm guard, terms.push({… queryType, returnType:'composite-literal'}), differing only in the termMap source tag. A shared collectInListTerms(op, values, column, pushMapping) would keep the three in lockstep — exactly the drift that left the not path unfixed until now. (Minor siblings: formatInListOperand is a one-line formatOrValue([...values]) wrapper, and the contains↔cs mapping is encoded twice in CONTAINMENT_OPS and orOperatorToken.)
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approved with non-blocking feedback, but some items should be followed up.
An `.or()` string is only rebuilt from its parse when it references an
encrypted column — otherwise the caller's string is forwarded verbatim —
so each of these corrupts precisely the mixed encrypted/plaintext case.
Quotes were tracked only at brace depth 0, so a `}` inside a quoted array
element or jsonb string value closed the literal early and the next `"`
re-opened quoting: the following top-level comma never split and its
condition was absorbed into the operand. Quotes are now opaque at every
depth.
A stray `}` or `)` drove the depth counter negative, after which no comma
split again. Neither is a PostgREST reserved character, so `a}b` is a
valid unquoted operand. Depth now floors at zero.
`in`-list elements were split on every comma, ignoring quotes, and the
quotes were left embedded in the fragments. On an encrypted column each
fragment became its own term, so `in.("Doe, John",Smith)` never matched.
Elements are now split on top-level commas and unquoted — the inverse of
what `rebuildOrString` emits.
A parenthesized operand was read as a list for every operator, so
`eq.(foo)` encrypted a JS array rather than the string. Only `in` and the
range operators take a paren-delimited operand.
A string operand spelling `null`/`true`/`false` is now quoted: PostgREST
reads a bare `null` as SQL NULL.
Finally, `contains(col, …)` on a union key spanning an encrypted and a
plaintext column accepted an array or object. A union is only as
permissive as its strictest member; any declared column in it pins the
operand to `string`. A literal column argument was never affected.
Fixes every finding from the PR #609 review that survived verification against HEAD. Each was reproduced with a failing test before the fix. `.or()` dropped a condition after an unbalanced opening brace or paren. The depth floor added in 8cd485d recovers from a stray CLOSING brace, but a stray OPENING one strands the counter above zero and no later comma ever splits, swallowing every condition behind it. With a plaintext column first, `referencesEncrypted` then reads false over the surviving conditions and the group is forwarded verbatim — running the swallowed condition against a ciphertext column with a plaintext operand. Braces are now quoted on emit (PostgREST treats them as structure inside `or=(…)`, so an unquoted scalar brace was malformed on the wire regardless), and `splitTopLevel` re-splits honouring quotes alone when its depth tracking does not balance. Containment literals keep the narrow reserved set, so `tags.cs.{vip}` stays unquoted. Direct `contains()` / `not(col, 'contains', …)` did not serialize their operand. postgrest-js joins array elements on `,` without quoting, so `contains('tags', ['with,comma'])` reached Postgres as two elements; its `not()` interpolates with `String(value)`, emitting `not.contains.with,comma` — no braces, wrong operator token — and `[object Object]` for a jsonb operand. Both now build the same containment literal the `.or()` path builds, and emit `cs`. `is(col, true)` compiled on queryable encrypted columns. The boolean overload was gated on the filterable keys, which exclude only the storage-only columns, so a `text_search`/`text_eq`/`*_ord` column matched it and emitted `IS TRUE` against a jsonb ciphertext. It now takes a dedicated `BK` parameter, kept distinct from the orderable `OK` so the two capability axes can diverge. The type test asserted the bug; it now asserts the compile error. `contains()` admitted arrays on plaintext SCALAR columns, emitting `@>` on `text` (42883). The plaintext operand now follows the column's own shape, mapping scalars to `never`. It distributes over `Row[K]`, which is sound only because the tuple guard already excludes every encrypted member; the residual case is documented. In-list operands spent one ZeroKMS crossing per element. Terms are now grouped by column — mandatory, since `bulkEncrypt` carries one `{table, column}` for the whole payload — and each group takes a single call, scattered back onto its original indices. Mirrors the Drizzle v3 path, keeps a per-term fallback, and rejects a length mismatch rather than silently truncating the predicate. `types` is now `satisfies Record<string, V3ColumnFactory>`. The derived `DOMAIN_REGISTRY` calls every value at module load, so a non-factory export would throw and take the supabase introspect/schema-build/verify path with it; that is now a compile error at the offending line. The blind `Object.values(types) as V3ColumnFactory[]` cast that would have silenced it is gone. In-list term collection is unified behind `collectInListTerms` — the three copies had already drifted once, leaving the `not` path unfixed.
`splitTopLevel` treated every unquoted brace and paren as structure. Neither
`{`/`(` mid-operand nor `}`/`)` is a PostgREST reserved character, so `a{b` and
`a}b` are valid unquoted scalars — and counting them desynchronised the split.
A stray closer drove the depth negative, a stray opener stranded it above zero,
and either way no later comma split: every remaining condition was absorbed
into the preceding operand.
Flooring the depth at zero fixed the closer. The opener was handled by
discarding the whole depth pass whenever the count ended unbalanced and
re-splitting on quotes alone — which trades the bug for its mirror image. Given
both a stray opener and a real containment literal, the re-split lands inside
the literal:
note.eq.a{b,tags.cs.{vip,admin}
-> ["note.eq.a{b", "tags.cs.{vip", "admin}"]
`admin}` carries no dot, so `parseOrString` drops it.
A brace or paren is structure only where the grammar can put one: opening a
logic group (`and(`, `or(`, and their `not.` forms), opening an operand right
after the operator dot, or nested inside a literal already open. Count those and
nothing else. The unbalanced-depth re-split stays as a backstop for the one case
the rule still misreads — a scalar whose brace follows an in-value dot,
`x.eq.a.{b` — where it is a recovery rather than the primary mechanism.
Both paths are reachable only through a raw-string `.or()`: `rebuildOrString`
quotes braces in scalars, so the adapter never emits the trigger itself.
Also pins the two containment operands `arrayLiteralElement` special-cases but
no wire test drove: an empty element must emit `{""}`, not the empty array `{}`,
and an element spelling `null` must emit `{"null"}`, not a SQL NULL.
Follow-up to #588, addressing the review feedback that landed after it merged.
Two of these are broken features rather than polish. Both were invisible to the existing tests for the same structural reason:
createMockSupabaserecords the arguments handed to each builder method and never builds a URL, so it can pin per-element encryption but never encoding..in()on an encrypted column was completely brokenEvery encrypted operand is a serialized envelope, dense with
"and,. postgrest-js wraps a comma-bearing element as`"${s}"`(PostgrestFilterBuilder.ts:811-819) and never escapes the quotes already inside it, so we emitted:Encrypted lists now go through
filter(col, 'in', …)with each element quoted and escaped, matching what the.or()path already did.This affects v2 as well as v3. v2's
("a@b.com")composite literal is itself quote-bearing and was equally broken. Three existing tests asserted the old, broken encoding and are updated..not(col, 'in', […])encrypted the whole list as one ciphertextThe not-collector had no element-wise
inbranch — compare the regular filter collector, which pushes one term per element.isEncryptableTermonly checksop !== 'is' && value != null, so an array sailed through and was encrypted whole. The filter then silently matched nothing, and postgrest emitted an unparenthesizednot.in.a,b.This is the same defect that was fixed for
.or()in81d2eb28and left unfixed on the direct.not()path. There was no test for it. It now encrypts element-wise and emitsnot.in.(…); a PostgREST list literal on an encrypted column throws rather than failing silently.is(col, null)on storage-only columnsisis never encrypted (isEncryptableTermrejects it outright, so no term is collected and no capability guard runs) and a NULL plaintext is stored as a SQL NULL. On a storage-only column (types.Boolean,types.Integer, …)IS NULLis therefore not merely legal but the only predicate available — yet the type surface rejected it. A null-only overload widens it to every row key.is(col, true)remains a compile error on encrypted columns, since you cannotIS TRUE-compare a jsonb ciphertext to a plaintext boolean.contains()operand, and plaintext containment in.or()A plaintext jsonb/array column falls through to PostgREST's native containment, so
contains('tags', ['vip'])andcontains('meta', { plan: 'pro' })now typecheck; encrypted match columns still take astringtoken.Relatedly,
.or([{ op: 'contains' }])only translatedcontains→csfor encrypted conditions, so a plaintext containment reached the wire as.contains.and failed to parse. The translation is now unconditional and lives inrebuildOrString, andsplitOrStringtracks{}so an array/jsonb literal's commas no longer split the condition mid-value.DOMAIN_REGISTRYderived fromtypesThe comment justifying the hand-written map ("
TextSearchhas a different arity") went stale when8123839cremoved the freeTextSearch tuner; all 39 factories are now uniformly(name: string) => Column. Deriving them makes drift impossible.Deriving alone would be unsafe: once the registry is built from
stripDomainSchema(factory().getEqlType()), any test that also derives its expectation measures the source against itself. Corrupt a domain constant incolumns.tsand the registry silently re-keys, introspection ships a wrong SQL parameter, real columns are misclassified as unmodelled — with every test green. So the derivation lands together with a hand-written literal list of the 39 domain names, which is the external-contract anchor. Verified keys-preserving: 39 keys, unchanged, null prototype intact.Tests
A new
__tests__/helpers/postgrest-wire.tsruns the realPostgrestClientagainst a capturingfetch, so the emitted query string is asserted without a database.@supabase/postgrest-jsis already a devDependency. Both P1 bugs above were invisible to the old harness and are caught trivially by this one.Also adds biting coverage for three null-prototype maps that were correct but unpinned (
mergeDeclaredTables, thev3Columnspattern-filter lookup, the mutation-model rebuild) and for the unreachable scalar-queryTypebackstop.Every fix was written test-first, and each test was verified to bite — the fix reverted, the test confirmed failing, then restored. That step is not ceremony here: the three null-prototype items exist precisely because correct fixes shipped alongside tests that never bit.
Verification
.in()and.not(…, 'in', …)are wire-format changes, so the live PostgREST suite (supabase-v3-pgrest-live.test.ts, requiresDATABASE_URL) is the end-to-end proof and should be run before merge.