Make the access-scoped include walk caller-directed - #873
Merged
Conversation
Naming a relation in an include now fetches only that relation's own columns and stops, at every level (not just the root) — the "one hop" rule that completes ADR-0024. buildAccessScopedInclude replaces the old build-full-tree-then-merge pair (buildIncludeWithAccessControl + mergeIncludeWithAccessControl) with a single caller-directed walk that only evaluates a related list's query access for relations a request actually names. foldDeclaredDependencies now recursively folds a field's needs beneath any reached relation, since the old free ride via auto-expansion no longer exists, using its own cycle guard (re-pointed from the retired relationship-graph walk) as a defensive backstop to needs-closure.ts's generate-time validation. Closes #852 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFBtkhz5Eed9usmnVck79b
🦋 Changeset detectedLatest commit: 01b8cd4 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Coverage Report for Core Package Coverage (./packages/core)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Contributor
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Contributor
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Contributor
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Contributor
Coverage Report for RAG Package Coverage (./packages/rag)
File CoverageNo changed files found. |
Contributor
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Contributor
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
…dded edges `foldDeclaredDependencies` applied its `visitedLists` cycle guard to every edge it walked, including relations the request itself named. A request is a finite literal and cannot loop, so the guard stopped the fold at any path that merely revisits a list — `Post → author → posts`, or a self-referential `parent` — leaving the revisited list's own `needs` unsatisfied and its computed fields resolving over `undefined`, which is the failure ADR-0025 exists to prevent "at every level a field is computed". The guard now applies only to declaration-added edges, which is where the unbounded recursion actually comes from: a branch this fold adds carries no caller include of its own, so everything beneath it is declaration-added too and each such edge either reaches a list not yet on the path or stops, bounding that suffix by the number of lists. A caller-named edge is bounded by the request's own literal instead. The two-list mutual-`needs` cycle still terminates with an identical include. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTktGvFM8QztXEDr265GHJ
There was a problem hiding this comment.
🟡 Human review recommended
It changes core access-control include scoping semantics and runtime behavior in security-sensitive code paths, warranting final human review despite strong test coverage.
Pull request overview
This PR refactors read-side access scoping to be caller-directed: access-scoped include trees are now built only along branches explicitly named by the request (caller include, fragment query, or needs-folded dependencies). It also completes the “One hop” rule (ADR-0026), where naming a relation fetches only that relation’s own columns unless deeper relations are explicitly included.
Changes:
- Replace the previous “auto-walk then merge” include scoping (
buildIncludeWithAccessControl+mergeIncludeWithAccessControl) withbuildAccessScopedInclude, which scopes only requested branches. - Update
foldDeclaredDependenciesto recursively foldneedsbeneath any reached relation, with a defensive cycle guard re-pointed to the declaration fold. - Update tests, docs, and changesets to reflect caller-directed include behavior and depth-cap messaging semantics.
File summaries
| File | Description |
|---|---|
| packages/core/src/context/index.ts | Switch read include resolution to buildAccessScopedInclude and pass listKey into foldDeclaredDependencies. |
| packages/core/src/access/access-filter.ts | Introduce caller-directed buildAccessScopedInclude and remove legacy auto-walk/merge machinery. |
| packages/core/src/access/index.ts | Update access module exports to expose the new include scoping API. |
| packages/core/src/access/declared-dependencies.ts | Make needs folding recursive under reached relations; add cycle-guarded recursion inputs. |
| packages/core/src/access/declared-dependencies.test.ts | Add unit tests for recursive needs folding and defensive cycle termination. |
| packages/core/src/access/errors.ts | Reword AccessScopeDepthExceededError messaging to describe a cost refusal. |
| packages/core/src/access/depth-limits.ts | Reframe READ_INCLUDE_MAX_DEPTH as a cost limit under caller-directed walking. |
| packages/core/src/access/field-visibility.ts | Update references/comments to the new access filter function name. |
| packages/core/src/access/relationship-count.ts | Update docs/comments to reference buildAccessScopedInclude. |
| packages/core/src/query/relationship-options.ts | Update comment to reflect that access-scoped include has nothing to scope in this path. |
| packages/core/src/access/access-filter.test.ts | Rewrite tests to assert ADR-0026 “one hop” behavior, depth-cap semantics, and no wasted access calls. |
| packages/core/tests/context.test.ts | Add integration coverage ensuring unrequested relations don’t invoke query access (#852). |
| packages/core/tests/resolve-chain.test.ts | Update concurrency regression test to reflect explicit nested include requirements (one-hop). |
| packages/core/tests/needs-declared-dependencies.test.ts | Adjust cycle-guard test narrative to match new declaration-fold guard semantics. |
| packages/core/tests/access-relationships.test.ts | Update tests to use buildAccessScopedInclude instead of legacy APIs. |
| packages/core/CLAUDE.md | Document ADR-0026 “One hop” rule and caller-directed include scoping. |
| docs/content/concepts/queries.md | Document one-hop include behavior in user-facing queries docs. |
| docs/content/concepts/access-control.md | Document caller-directed include scoping and cost-limit framing for depth cap. |
| .changeset/one-hop-scopes-relations.md | Minor changeset documenting one-hop behavior + migration note + depth error messaging update. |
| .changeset/gentle-otters-fold.md | Patch changeset for the revisited-list needs folding fix. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+88
to
+95
| /** The explicit nested `include` on an include entry, if the entry is a structured object naming one. */ | ||
| function getExplicitInclude(value: unknown): Record<string, unknown> | undefined { | ||
| if (value && typeof value === 'object' && 'include' in value) { | ||
| const include = (value as { include?: unknown }).include | ||
| return include && typeof include === 'object' ? (include as Record<string, unknown>) : undefined | ||
| } | ||
| return undefined | ||
| } |
Comment on lines
36
to
40
| @@ -194,9 +40,7 @@ type IncludeEntryObject = { where?: PrismaFilter; include?: IncludeObject; take? | |||
| * | |||
Addresses Copilot review feedback on #873: asEntryObject and getExplicitInclude both narrowed an unknown value straight to Record<string, unknown> via `as`, which would (in principle) admit an array or a `null`-adjacent object as a valid include entry. Both now go through a shared isPlainObject type guard first, so no field is read off a value TypeScript hasn't actually narrowed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFBtkhz5Eed9usmnVck79b
Merged
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.
Summary
buildIncludeWithAccessControl+mergeIncludeWithAccessControl(build the full access-scoped tree for every relationship, then reconcile against whatever the caller asked for) with a single caller-directedbuildAccessScopedIncludethat walks only the branches a request names.includenow fetches only that relation's own columns and stops, at every level — not just the root (the "One hop" rule, completing ADR-0024). Reaching further means naming further:include: { author: { include: { organization: true } } }.include, fragmentquery, or a field's foldedneeds) never has its list's operation-levelqueryaccess evaluated at all — the wasted-access-calls problem the issue describes.foldDeclaredDependencies(needs, ADR-0025) now recursively folds a field's declared dependencies beneath any reached relation (declaration-added or caller-named, bare or not), since the old free ride via auto-expansion no longer exists. Its ownvisitedListscycle guard is the re-pointed version of the old relationship-graph walk's guard — defense in depth behindneeds-closure.ts's generate-time validation, which is the primary backstop against a cyclicneedsclosure.READ_INCLUDE_MAX_DEPTH/AccessScopeDepthExceededErrorkeep their value, type, and throw sites — fail-closed per ADR-0022 — but the cap is now a cost limit on a caller-directed request rather than a safety boundary on an engine-generated walk, and the error message wording was reworded accordingly.Why
Follow-up from #848 / ADR-0024, filed as ADR-0026. After #848, a bare read (no
include) stopped auto-including every relation — but a callerincludeas small as{ author: true }still triggered a full walk of every other relation on the list, evaluating every related list'squeryaccess even for relations the caller never asked about, and auto-expanded the named relation's own subtree to the depth cap.Detection / migration (silent break)
An
includethat named a relation bare and read past it —item.<named>[0].<unnamed>— now getsundefinedfor the unnamed part, with no error. See the changeset for the grep and the fix (name the deeper relation explicitly).Test plan
pnpm --filter @opensaas/stack-core test— 946/946 passing, including the full existing Explicit include bypasses relation-level access filtering on reads — caller include REPLACES the access-controlled include #566/Admin UI: bound relationship-table row fetch with a limit + "showing N of M" footer #752/Access control fails OPEN past include depth 5: caller includes pass through unscoped and nested rows skip field-read filtering #830/0.33.0: reads insideresolveOutputrecurse without bound — server OOMs on any access-scoped read #844/Reads with noincludeauto-include every relation to depth 5 — Prisma returns scalars only, and this default is the root cause behind #566 / #830 / #844 #848 regression suites (all pass unmodified at thecontext.dbintegration level intests/context.test.ts,tests/resolve-chain.test.ts,tests/bare-read-scalars.test.ts,tests/needs-declared-dependencies.test.ts)queryaccess invoked (asserted directly on the access function, both at the unit level inaccess-filter.test.tsand at thecontext.dbintegration level incontext.test.ts)declared-dependencies.test.ts— recursiveneedsfolding beneath declaration-added and caller-named-bare branches, and the defensive cycle guard for a two-listneedscyclepnpm lint,pnpm manypkg fix,pnpm format,tsc --noEmitall cleanpackages/core/CLAUDE.md,docs/content/concepts/access-control.md,docs/content/concepts/queries.md)Closes #852
Generated by Claude Code