Access control fails OPEN past include depth 5: caller includes pass through unscoped and nested rows skip field-read filtering

[borisno2]

Summary

Both halves of the read-side access-control pipeline stop at MAX_DEPTH = 5, and both fail open rather than closed. Past that depth a caller-supplied include is neither row-scoped nor field-filtered, so a relation returns every row of the related list with read-denied columns intact.

The two cut-offs are independent but line up, so they compound:

1. Row scoping — packages/core/src/access/access-filter.ts

buildIncludeWithAccessControl returns undefined at depth >= MAX_DEPTH. mergeIncludeWithAccessControl treats whole-object undefined as "nothing to merge against" and returns the caller's include unchanged. The behaviour is deliberate and documented in the source:

The access-controlled include is recursive to MAX_DEPTH (see buildIncludeWithAccessControl); beyond that depth no auto-include exists, so deeper caller selections pass through unscoped — consistent with the existing auto-include behaviour.

The distinction the code draws — a whole-object undefined is a passthrough, an undefined entry is a denial — is reasonable for the resolveOutput/virtual-field and no-relationships cases it also covers. The MAX_DEPTH case is the one where passthrough is the wrong default, because it is the only one where a relation genuinely should have been scoped and wasn't.

2. Field visibility — packages/core/src/access/field-visibility.ts

filterReadableFields recurses only while depth < MAX_DEPTH. Past that, a relationship value falls through to resolveReadableFieldValue, which checks the read gate on the relationship field itself and then returns the nested object as-is. The related row's own per-field access.read gates are never evaluated, and its resolveOutput hooks never run.

Why this matters

The failure is silent and invisible. There is no warning, no thrown error, and nothing in the returned shape that distinguishes "this relation was access-filtered" from "this relation was returned raw because it sat one level too deep". A reviewer reading the call site sees an ordinary include and reasonably assumes the engine scoped it — the framework's central promise.

It is also discontinuous: the same relation on the same list is filtered at depth 4 and unfiltered at depth 5. Adding one intermediate hop to an existing include — a refactor that looks purely additive — can silently switch off access control on the leaf. Nothing in the type system or at runtime flags it.

The consequences depend on what sits at the leaf:

• Row scoping lost → a relation that should be scoped to the caller's own tenant/owner returns rows belonging to everyone.
• Field filtering lost → columns gated with access: { read: … } (tokens, secrets, internal-only fields) are returned to callers who should not see them.
• resolveOutput skipped → field types that normalise on read (the date-only normaliser, wrapper types) return their raw database representation instead, so a deeply-nested value silently changes type.

Reproduction

1. Define lists A → B → C → D → E → F, each related to the next.
2. Put a row-scoping filter on F's query access, and a read-denied field on F.
3. As a non-privileged session, read A with a caller-supplied include chain down to F.
4. F comes back with rows outside the filter and with the read-denied field populated.

The same chain terminating at depth 4 behaves correctly.

Suggested direction

Fail closed at the boundary. Concretely, one of:

• Drop the relation past MAX_DEPTH rather than passing it through — treat "I could not compute an access include for this" as a denial, distinct from the resolveOutput/no-relationships cases that legitimately mean "nothing to scope".
• Or throw on a caller include that exceeds the depth the engine can scope, so the limit surfaces at development time instead of becoming a quiet data leak in production.

Either way, filterReadableFields' cut-off should move in step with buildIncludeWithAccessControl's — right now a row can be scoped correctly and still return unfiltered fields, or vice versa, because the two limits are enforced independently.

If the depth cap must stay a passthrough for backwards compatibility, it would help to at least make it observable: a logged warning, or a flag on the result, so applications can detect that a read escaped the engine rather than discovering it by audit.

Related consideration: MAX_DEPTH = 5 is also duplicated in context/nested-operations.ts and context/transaction-boundary.ts. If the cap stays, a single shared constant with a comment on the security implication would make the coupling explicit.

Environment

• @opensaas/stack-core 0.32.0 (verified against main at time of writing — the code and the documenting comments are both present in the current source)

[borisno2]

This was generated by AI during triage.

Agent Brief

Category: bug (security-relevant correctness)
Summary: Make the read pipeline fail closed when it cannot compute an access scope, instead of passing caller includes through unscoped and unfiltered.

Verification

Both halves reproduce — with one correction to the report, which matters for where the tests go.

The boundary is nesting level 6, not 5. With a chain A→B→C→D→E→F→G and a caller include down to G:

MERGED:  {"b":{…{"f":{"include":{"g":true}}}}}
G ENTRY (level 6): true          ← G's query filter {tenantId:{equals:'mine'}} never applied
F ENTRY (level 5): has where     ← correctly scoped

Field visibility cuts at the same level:

FILTERED: {…"f":{"id":"f1","name":"F","g":{"id":"g1","name":"G","secret":"LEAKED-TOKEN","tenantId":"someone-else"}}}

secret carries access: { read: () => false } and survives at level 6; the same field is stripped at level 5. The off-by-one is because buildIncludeWithAccessControl evaluates a relation's query access in the loop at depth N while recursing for that relation's own relations at N+1 — so the list reached at depth 4 still gets its children scoped.

A related detail worth knowing while fixing: the MAX_DEPTH guard in filterReadableFields gates only relationship recursion, not scalar filtering. A leaf list's own fields are still filtered as long as the relationship field pointing at it sits below the cap.

Second trigger, far more reachable. The same whole-object passthrough fires at the first hop inside a resolveOutput/virtual-field context:

resolveOutputDepth=0  access={"b":{"where":{"tenantId":{"equals":"mine"}}}}  merged={"b":{"where":{…}}}
resolveOutputDepth=1  access=undefined                                       merged={"b":true}

Any read issued from inside a resolveOutput hook loses relation row scoping immediately. (Only the include half of this was verified; whether field stripping still runs for those reads was not checked.)

Design decisions (settled during triage — do not re-derive)

Grilled and recorded in ADR-0022, "Access control fails closed when it cannot scope a read", with the Access Filter glossary entry in CONTEXT.md extended to state the denial-vs-nothing-to-scope distinction. Read the ADR first; it records the rejected options and why.

Current behavior:
buildIncludeWithAccessControl returns undefined for three unrelated outcomes — inside a resolveOutput context, at the depth cap, and when the list declares no relationships — and mergeIncludeWithAccessControl treats all three as "nothing to merge against", passing the caller's include through unchanged. For the depth case that means the relation is fetched with no access where. Independently, filterReadableFields stops recursing into relationships at its own cap, so nested rows past it are returned with their own field-level read gates never evaluated and their resolveOutput hooks never run.

Desired behavior:

• A caller-supplied include nested deeper than the Access Filter can scope throws a dedicated error carrying the list key, the relation field, and the depth reached. Failing to compute a scope is a denial, never a passthrough.
• The auto-include stopping at the cap stays silent. Nothing unscoped is returned there — the relation is simply not fetched — and throwing would make deep schemas unreadable. Only an explicit caller selection can trigger the error.
• Inside a resolveOutput/virtual-field context the builder scopes without expanding: each relation still gets its access where, but nested relations are not auto-included. Loop prevention is preserved, because scoping adds a filter rather than an include.
• The post-query phase no longer caps its recursion. It walks a materialized, finite, acyclic result tree whose depth is already bounded by the include the pre-query phase permitted, so everything fetched is field-filtered by construction — "scoped rows with unfiltered fields" becomes impossible rather than merely aligned.
• A list that declares no relationships still passes the caller's include through unchanged. That case is genuinely "nothing to scope" and must not start throwing.

Key interfaces:

• The include builder's return type becomes a discriminated result distinguishing scoped, nothing-to-scope, and depth-exceeded, replacing the overloaded undefined. The merge function branches on it: pass through for nothing-to-scope, throw for depth-exceeded when the caller named a relation there. These functions are package-internal (not exported from any entry point, no cross-package importers), so the signature change carries no semver cost.
• A new error class exported from the root entry point, carrying listKey, the relation field name, and the depth reached. Do not reuse ValidationError — that type means user-input validation, and handlers written for bad form data must not swallow this.
• Two named depth constants replacing the four local MAX_DEPTH declarations: one for read-include nesting (used by the Access Filter) and one for nested-write payload depth (used by nested operations and imported by the transaction-boundary walk, which already documents itself as tracking it). Each carries a comment on its security implication. Do not collapse them into a single shared constant — they measure different quantities.

Acceptance criteria:

• A caller include one level past the read cap throws the new error, naming the list and relation field
• The same include one level shallower still returns data, correctly row-scoped and field-filtered (guard against fixing the leak by moving the cliff)
• An ordinary read on a schema deeper than the cap, with no caller include, does not throw — the auto-include simply stops
• A read issued inside a resolveOutput hook returns relations carrying their access where, and does not auto-include their nested relations
• A read inside a resolveOutput hook on a self-referential relationship still terminates (the loop guard that motivated the original passthrough is not regressed)
• Nested rows at any depth have their field-level read access applied and their resolveOutput hooks run
• A list with no relationships still passes a caller include through unchanged
• Regression tests cover the exact boundary (scoped at the last good level, throws one deeper) and both the row-scoping and field-visibility halves
• A changeset for @opensaas/stack-core (minor), whose description leads with the behaviour change — reads that previously returned deeply-nested data now throw, because that data was unscoped
• Reference docs state the depth limit and the new error (the cap is currently undocumented)

Out of scope:

• The write-path sibling — processNestedOperations returns caller data unprocessed past its cap and the Write Pipeline persists it. Tracked separately so a verified read-path security fix isn't held up by an unverified write-path change.
• Removing or raising the read cap; it stays as a cost bound (see ADR-0022)
• Making the cap configurable
• Any change to ADR-0001's two-phase split
• Reworking the cycle guard, which is a separate mechanism and already correct

---

Generated by Claude Code

[borisno2]

This was generated by AI during triage.

The write-path sibling noted as out of scope above is now tracked as #835 (needs-triage — it needs reproducing before it can be briefed).

---

Generated by Claude Code

[borisno2]

This was generated by AI during triage.

Amendment to the agent brief: drop MAX_NESTED_WRITE_DEPTH

The brief's "Key interfaces" section calls for two named depth constants — one for read-include nesting and one for nested-write payload depth, the latter used by nested operations and imported by the transaction-boundary walk. That second constant should not be created.

Triage of #835 established (by reproduction) that neither write-side cap survives:

• processNestedOperations' depth guard is unreachable — no caller passes depth, so the counter is always 0. Transaction-boundary hooks never fire for lists involved only past the enumeration depth cap #835 deletes the constant, the branch, and the unused parameter rather than preserving them.
• The transaction-boundary enumeration cap is being removed outright, replaced by a saturation check on (list, operation) pairs.

With both gone there is no nested-write depth limit for a constant to name.

Revised instruction for this issue: introduce only the read-side constant (MAX_READ_INCLUDE_DEPTH or equivalent) for the Access Filter, documented with its security implication. Do not add a shared limits module for a write-side counterpart, and do not treat the existing write-side MAX_DEPTH declarations as limits to preserve — #835 removes them.

Everything else in the brief stands. The ADR-0022 consequence bullet that referred to a write-path fail-open has been corrected in PR #837; the read-path decisions are unaffected.

---

Generated by Claude Code