The Where vocabulary and its lowering; the filter module retyped - #1220
Conversation
`.where()` and `.orderBy()` take the closed Where vocabulary — the nine
scalar operators, AND/OR/NOT, and some/every/none on a relation of any
cardinality — resolved against the config in one pass and lowered onto
Prisma's predicate lambda in another. `contains` is engine-escaped and
case-insensitive, `equals: null` is IS NULL, a relation predicate ANDs the
related list's `query` access inside the EXISTS (denied meaning empty), and
`orderBy` is scalar-only.
The lowering is now total: a condition that resolved to `undefined` is
refused rather than dropped, on both spellings. The Access Filter goes
through the same seam, so an access rule spelled
`({ session }) => ({ authorId: session?.userId })` no longer matches every
row for an anonymous caller — it throws. The tests that pinned the old
widening are inverted rather than deleted.
An unknown key or operator is a ValidationError naming the list and the key,
`sudo` included. A key the session cannot read carries the identical message
a key the list does not declare gets, and access is still resolved first, so
a denied caller sees the Silent failure and no validation error at all.
The filter module's condition type is a vocabulary value and still imports
nothing from the ORM; a to-one label filter emits `some`, a to-many count
filter shrinks to presence, and the count-filter marker, its resolvers and
the label pass-through are deleted.
Implements #1147. Part of #1123.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: eda95d0 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report for Core Package Coverage (./packages/core)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File CoverageNo changed files found. |
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
| if (access === false) return { kind: 'false' } | ||
| if (access === true) return { kind: 'true' } | ||
| const filter: PrismaFilter = access | ||
| return await resolveWhere(filter, { |
There was a problem hiding this comment.
Unbounded recursion: a relation named inside an Access Filter re-enters access resolution.
relatedAccessPlan resolves the related list's own Access Filter through resolveWhere while carrying applyRelationAccess forward unchanged (only checkFieldRead is overridden). So a relation key inside an access filter triggers another relatedAccessPlan, which resolves another access filter, and so on — with no cycle set and no depth cap.
Self-referential list is enough to hang a request:
Comment: list({
fields: {
published: checkbox(),
parent: relationship({ ref: 'Comment.children' }),
children: relationship({ ref: 'Comment.parent', many: true }),
},
access: { operation: { query: () => ({ parent: { some: { published: { equals: true } } } }) } },
})context.db.Comment.all() → resolvePlan resolves the filter → key parent → resolveRelation → relatedAccessPlan(Comment) → same filter → parent → … forever. Two mutually-referencing lists (Post.query naming author, User.query naming posts) do the same. The terminal never returns; the async chain grows until the process OOMs.
The seam this replaces did not have this shape: buildAccessScopedWhere folds accessWhere into the nested clause and recurses only into the caller's nested clause, never into the access filter it just folded in.
Either resolve access filters with applyRelationAccess: false, or thread a visited-list set (or the #1148 depth cap) through ResolveContext.
| relation: key, | ||
| relatedListName: related.listName, | ||
| quantifier: quantifier === 'some' ? 'some' : quantifier === 'every' ? 'every' : 'none', | ||
| node: access.kind === 'true' ? caller : { kind: 'and', nodes: [caller, access] }, |
There was a problem hiding this comment.
every ANDs the access filter into the quantified predicate, which is not "every visible row matches" — and it leaks the existence of invisible rows.
For some and none, caller AND access is right. For every it inverts the intent: every(caller ∧ access) requires every related row to be access-visible and matching, rather than every visible row matching (none(access ∧ ¬caller)).
Concretely, with the fixture in read.test.ts: anonymously Post scopes to published, and handles({ posts: { every: { published: true } } }) returns only ['bob'] — ada drops out solely because she owns a draft the caller cannot see. The caller has just learned that an invisible Post row exists for ada, which is the class of signal the { kind: 'false' } branch above exists to close.
The legacy path made the same conservative choice but documented it loudly (access-filter.ts, "One quantifier is deliberately conservative rather than exactly precise…"). Here neither the code nor the changeset says it, and the test comment on line ~527 asserts the opposite ("every is measured against the same scoped set"). If the imprecision is intentional, please carry the old caveat forward; otherwise lower every as none(access AND NOT caller).
| } | ||
| const resolved = resolveQueryField(key, ctx.listConfig.fields) | ||
| if (!resolved) throw unqueryableKey(ctx.listName, key) | ||
| if (resolved.isRelationship) { |
There was a problem hiding this comment.
orderBy checks isRelationship before the field-read gate, so a read-denied relationship field is an existence oracle.
resolveKey (the where path) gets the order right: resolveQueryField → field-read check → relation branch, so a read-denied key always comes back with the identical unqueryableKey message. resolveOrderBy does it the other way round — the relation refusal on line 439 runs before the checkFieldRead block on line 443.
Given secrets: relationship({ ref: 'Secret.owner', many: true, access: { read: () => false } }):
.orderBy({ secrets: 'asc' })→Cannot order "User" by "secrets" — orderBy takes scalar columns only..orderBy({ nope: 'asc' })→… "nope" is not a queryable field of this list.
The caller can now tell that secrets exists and is a relation, which is exactly what ADR-0031 / #915 close elsewhere (and what the read.test.ts message-equality test asserts for where). Move the field-read check above the relationship branch.
| // `some` is false, `none` and `every` are true. That keeps a relation | ||
| // token from distinguishing parent rows by a list the session cannot see. | ||
| if (access.kind === 'false') { | ||
| nodes.push(quantifier === 'some' ? { kind: 'false' } : { kind: 'true' }) |
There was a problem hiding this comment.
The denied-relation short circuit skips validating the nested predicate, which makes "this related list denies query" observable.
When access.kind === 'false' the loop continues without ever calling resolveWhere(nested, …), so nothing inside that predicate is checked. A caller can probe with a deliberately bogus nested key:
where({ secrets: { some: { nope: 1 } } })→ resolves fine, returns[]where({ posts: { some: { nope: 1 } } })→ValidationError: Cannot query "Post" — "nope" …
So the refusal (or its absence) distinguishes a related list the session cannot query from one it can — the very distinction the empty-set treatment on these lines is meant to erase. Resolving nested for its validation side effect and discarding the plan keeps the empty-set semantics while making the two cases refuse identically.
| authorId: { equals: session?.userId }, | ||
| }) | ||
| // Filter object: Scope access to specific records. Deny outright when there is | ||
| // no session to scope to — the engine refuses a predicate that resolved to |
There was a problem hiding this comment.
"the engine refuses a predicate that resolved to undefined" is only true of the secured surface — the findMany path still fails open.
The totality fix lives in resolveWhere, which only context.db.<List>.where(...).all()/.first() goes through. createFindMany (and count/updateMany/delete) still does mergeFilters(scopedWhere, accessResult), which just returns { AND: [accessFilter, userFilter] } — an access filter of { authorId: undefined } reaches the client with the key dropped, i.e. it still matches every row.
So for an app on context.db.Post.findMany() — the API this very file and the generated context's own docblock document — query: ({ session }) => ({ authorId: session?.userId }) remains the silent match-everything read for an anonymous caller, while this doc now tells the reader it is refused. Same for the anonymous-access-control.md hunk.
Either scope the wording to the composed read surface, or route the legacy merge through the same total resolution.
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES (posted as a comment review — GitHub refuses a formal Request-changes verdict from the PR author identity).
Code review — high effort
Verdict: REQUEST CHANGES. One blocking defect (unbounded recursion, DoS-reachable), plus three correctness/leak issues and a docs overclaim. Five inline comments are attached to the exact lines.
The core of this PR is right: predicate lowering really is total now, the vocabulary is closed, and the contains escaping is correct. The problems are all in the relation and access-filter edges.
Blocking
- Unbounded recursion in
relatedAccessPlan—packages/core/src/secured/vocabulary.ts:307. When it recurses to resolve the related list's Access Filter it clearscheckFieldReadbut leavesapplyRelationAccess: true. So a relation key inside an access filter re-enters access resolution, with no cycle set and no depth cap. A self-referentialqueryfilter (Comment.query = () => ({ parent: { some: … } })), or two mutually-referencing lists, makes every read of that list recurse until the process OOMs. The legacybuildAccessScopedWhererecursed only into the caller's clause, never into the filter it folded in. NeedsapplyRelationAccess: falseon the recursive call, or a visited-set/depth cap.
Should fix before merge
-
everyhas the wrong empty-set semantics — vocabulary.ts:365.everygetscaller AND accesslikesome/none, which means "every related row is visible and matches" rather than "every visible row matches". The PR's own test demonstrates the leak: anonymously, ada drops out of{ posts: { every: { published: true } } }purely because she owns an invisible draft — that is a positive signal about a row the caller cannot see, exactly the class of leak the denied-list branch two lines above exists to prevent. The legacy path made the same conservative choice but said so; here the code is silent and the test comment claims the opposite. Either fix the lowering (everyshould not AND the access filter into the quantifier body) or state the trade-off in the code and correct the test comment. -
orderByis an existence oracle — vocabulary.ts:438. TheisRelationshiprefusal runs before the field-read gate;resolveKeyon thewherepath gets this order right.orderBy({ secretRel: 'asc' })on a read-denied relationship returns "orderBy takes scalar columns only", while an undeclared key returns "not a queryable field" — two distinguishable errors, contradicting this PR's own stated invariant that a read-denied field must be indistinguishable from a nonexistent one. Move the field-read check above the relationship refusal. -
Docs overclaim the fix —
docs/content/reference/config-api.md:506(and theanonymous-access-control.mdhunk). "The engine refuses a predicate that resolved toundefined" is true only for.where().all()/.first().createFindMany/count/updateMany/deletestill route throughmergeFilters(context/index.ts:1198,1362,1641), which passes{ authorId: undefined }to the client verbatim. The anonymous match-everything read is unchanged on the path these docs describe and the generated context actually exposes. Either scope the wording to the builder terminals or close the hole inmergeFilterstoo. -
Denied-related-list short circuit skips nested validation — vocabulary.ts:350 (low). The
continuereturns without resolving the nested predicate, sowhere({ secrets: { some: { nope: 1 } } })succeeds whilewhere({ posts: { some: { nope: 1 } } })throws — the refusal itself distinguishes a related list the session cannot query from one it can, undoing the indistinguishability those lines exist to provide.
Checked and clean
undefinedtotality (both spellings).resolveKeythrows on a bareundefinedvalue andresolveScalarthrows on{ equals: undefined }— the({ session }) => ({ authorId: session?.userId })hole is genuinely closed on the builder path (see item 4 for the path where it is not).- Test inversion. Checked
git diffon every touched test file. The changes are honest adaptations to the Prisma 8 vocabulary (startsWithremoved, count-marker → presence), not inversions concealing a regression.relationship-label-filter.test.tsis deleted alongside its module. Worth confirming as a product decision, though:posts:>5in a saved list filter now silently degrades to free text ({ name: { contains: '5' } }) rather than filtering. It is tested and deliberate, but it is a user-visible loss. containsescaping.containsPatternescapes\first, then%and_— correct order, all three characters, andilikesupplies case-insensitivity. NoESCAPEclause is needed because the pattern is a bind value and Postgres's default LIKE escape is backslash; ADR-0055 is cited in the docblock.- Deletions.
RELATIONSHIP_COUNT_FILTER_KEY,RelationshipCountFilterMarker,resolveRelationshipCountFilters,resolveRelationshipLabelFilters,isToOneRelationshipField,ColumnEquality,UnsupportedPredicateError— no dangling references across packages, examples or docs;ListView's dropped count-resolver call is covered by the updatedListView.test.tsx. - No
anyor type assertions introduced aroundListPredicate/ListQuery/ListSortor the dynamic import;ListPredicate/ListSortreach the list surface throughListOpsas intended. sudostill passes through vocabulary validation while skipping access — correct.
Minor
- Lazy ORM import (
secured/lower.ts:66).pending ??= import(...)is race-free (assignment precedes any await) and costs one import, not one per call — but a rejected import is cached permanently, so a transient failure poisons the process. Consider clearingpendingon rejection. - The purity invariant is enforced, but by proxy.
filter/purity.test.tsgreps a hand-listed file set for@prisma/— it does not walk the package root's real import graph, andlower.ts(the file carrying the invariant comment) is excluded from the list because it holds a type-only import. Reasonable, but weaker than the comment claims. - Changeset is
minorfor@opensaas/stack-corewhile removing four public exports. Fine ifprisma-8ships as a single major — worth confirming.
…and mergeFilters Review fixes on #1220. An Access Filter that scopes by a relation is expanded into the related list's own Access Filter, so a filter that names its own list — or two that name each other — recursed until the process ran out of memory. The expansion now carries the path of lists it is expanding and refuses a repeat by name, with a second bound at ten lists deep for an acyclic chain. The refusal is an error naming the chain rather than a truncated filter: a truncated Access Filter is a widened read. `every` asked whether every related row was visible AND matched, which dropped a parent for owning a row the caller could not see. It now asks whether every VISIBLE row matches — lowered as "no visible row fails the predicate" — so an invisible row never decides a parent's membership. `some` and `none` are unchanged, and a denied related list is still the empty set. `orderBy` ran its relationship refusal before the field-read gate, so ordering by a read-denied relationship answered differently from ordering by a key the list does not declare. The gate now runs first, as `resolveKey` already did. The denied-related-list short circuit likewise validated nothing nested inside it, so the refusal itself said which related lists the session could query; the nested predicate is resolved first. The `undefined` refusal reached only the secured builder's terminals, while the docs claimed it unqualified. `mergeFilters` — the seam the legacy findMany, count, updateMany and delete paths fold their access filter through — now applies the same rule, throwing `UndefinedAccessFilterError` for an `undefined` condition anywhere in what an access rule returned. A caller's own `where` is untouched. The filter module's purity test walks the real import graph from its own files rather than grepping a hand-listed set, with a negative control proving the walk detects an ORM import. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review fixes pushed as 1. Unbounded recursion in I kept Cap: 10, and why. This recursion is the engine's own — a relation key inside a trusted config filter — so ADR-0043's caller-facing depth cap does not bound it and reusing that number would conflate two different things. With cycle detection in place the chain is already bounded by the number of distinct lists, so the cap only ever fires on an acyclic chain of ten different lists each scoping by a relation into the next. That is well past any real ownership chain ( Fails closed and loudly, as asked: both reasons throw an error naming the full chain ( Tested on both shapes with real rows: a self-referential filter ( 2. Confirmed the other two: The existing test's comment and its assertion disagreed; both are fixed and they now agree. The invisible-draft case is asserted directly: anonymously, ada (who owns an invisible draft) and bob both satisfy 3. 4. Docs overclaim (BLOCKING) — I closed the hole; the docs are unchanged and now true. Scope call worth flagging: the refusal covers what an access rule returned, not the caller's own Blast radius re-verified: the full monorepo suite is green with the refusal in place (core 1564 passing, no config in Pinned with tests in 5. Denied-list short circuit (low) — fixed. 6. Purity test (minor) — fixed. Note this deliberately does not match 7. Changeset (minor) — updated, bump unchanged. Not touched, as instructed: refinements/depth cap (#1148), select and widen-and-strip (#1149), the aggregate family (#1150), Also not addressed (from your Minor list, not in scope for this pass): Verification: |
…e.ts The base moved under this branch while it was in review, so GitHub could compute no merge ref and CI never ran for 300b17d. The one content conflict was `packages/core/src/internal.ts`, where this branch's `where/like.ts` re-exports and the base's `findDatabaseConnection` (#1218) were added at the same point; both are kept. #1220 landed the Where vocabulary from its own branch rather than adopting this branch's engine-owned escaper, leaving a second implementation: `containsPattern()` in `secured/vocabulary.ts`. It performed the same three replacements in the same order with the same wrapping as `likeContainsPattern`, so it is deleted and the `contains` case now calls the shared builder. One escaper, as ADR-0060 requires. #1220's Where-vocabulary and filter tests pass unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Implements #1147. Part of #1123.
Builds on #1146 (
26f47b0a): thelowerPredicateseam that ticket deliberately narrowed to equality is replaced by a two-pass vocabulary, and the generatedListPredicatewidens with it.What landed
.where()and.orderBy()take ADR-0055's closed Where vocabulary:Lowering is split in two, which is what lets a relation quantifier's predicate be a plain synchronous lambda:
secured/vocabulary.ts— pure, ORM-free. Resolves a predicate against the list config and the session: every key checked, every operator checked against the closed set, every relation hop'squeryaccess decided. Produces a plan.secured/lower.ts— the one place the vocabulary meets the ORM. Builds the expression from a plan. Prisma'sand/orcombinators are loaded through a lazyimport()so the package root keeps its static module graph free of@prisma/orm-postgres.secured/operators.ts— the operator set as a leaf module, so the vocabulary and read-path key validation can both name it without importing each other.Behaviour, per the acceptance criteria:
containsis engine-escaped and case-insensitive, socontains: '50%'matches a literal per-cent sign rather than binding a wildcard.equals: nulllowers toIS NULL,not: nulltoIS NOT NULL.queryaccess inside theEXISTS. A related list the session cannot query is the empty set:somefalse,none/everytrue.orderByis scalar-only; a relation is refused rather than ignored.ValidationErrornaming the list and the key, undersudotoo —sudobypasses access, not the vocabulary.FilterConditionis a vocabulary value and still imports nothing from the ORM (pinned by a newsrc/filter/purity.test.ts, which also asserts the deleted count-filter symbols are gone).The totality finding (the carried-forward comment on #1147)
lowerPredicateused to doif (condition === undefined) continue. Because the Access Filter is lowered through the same seam, an access rule spelledlowered to
{}for an anonymous caller and matched every row, while the explicit{ equals: undefined }spelling of the same rule was refused — two spellings of one rule behaving oppositely, and the widening one the idiomatic spelling.Both spellings now throw.
resolveKeyrefuses anundefinedcondition, and so does every operator slot inside a condition object; the Access Filter is resolved through the same call with only the field-read gate relaxed (it is trusted config), so it cannot lower to a widening predicate. The tests #1146 added to pin the old behaviour — including the end-to-endWidenedfixture proving the anonymous widening — are inverted, not deleted: they now assert the refusal, and the authenticated case still returns the correctly-scoped row.The refusal is not an oracle. A key the session cannot read is refused with the identical message a key the list does not declare gets (a test compares the two message strings), and #1146's ordering is preserved — operation access resolves first, so a denied caller gets the Silent failure and never reaches key validation at all.
Blast radius on existing config. I swept packages and examples for the widening signature (
session?.inside a filter-returning access rule). Every example config already guards withif (!session) return false, so no runtime config broke. What did need fixing was documentation that taught the widening idiom:docs/content/reference/config-api.mdanddocs/content/how-to/anonymous-access-control.mdnow say the engine refuses rather than silently matching everything, and the CLI's MCP documentation provider no longer hands outquery: ({ session }) => ({ userId: { equals: session?.userId } })as an example.Deletions, and the tests that moved with them
RELATIONSHIP_COUNT_FILTER_KEY,RelationshipCountFilterMarker,resolveRelationshipCountFilters,mergeResolvedMember, and the wholerelationship-label-filter.tsmodule (resolveRelationshipLabelFilters,isToOneRelationshipField).buildRelationshipCountSelectandisToManyRelationshipFieldstay — count display is unaffected.{ author: { some: { name: { contains } } } }rather thanis(Prisma 8 has no to-one predicate form). A to-many count filter shrinks to presence:orders:0→none,orders:>0/orders:>=1→some, any other comparison degrades to free text under ADR-0017's own rule, so a bookmarkedorders:>5stops filtering rather than erroring.packages/ui'sListViewno longer calls the count resolver; its two tests changed with the behaviour.query-validation.tsgained the operator check, which is why two long-standingstartsWithprobes inpackages/core/tests/context.test.tsnow usecontains—startsWith/endsWith/mode/searchare outside the vocabulary by decision, not oversight.Out of scope, left as clean seams
Refinements and the depth cap (#1148),
select/ widen-and-strip (#1149),aggregate/combine/distinct/cursor(#1150),nearest()(#1151), all writes (#1152/#1124).Verification
pnpm build— 11/11 tasks successfultsc --noEmitonpackages/core— cleanpnpm lint— 0 errors (2 pre-existing warnings, both untouched by this change)pnpm -r --filter='./packages/*' test— all 9 packages green; core 1552 passed / 1 skipped across 78 filespnpm manypkg fixandpnpm formatrun🤖 Generated with Claude Code