Uh oh!
There was an error while loading. Please reload this page.
fix(driver-sql): route every read door through the tenant chokepoint (#6792) - #6908
Merged
Merged
Conversation
…#6792) Not yet measured red/green and no fix applied — checkpoint only, so the work survives container reclamation during a scheduled pause.
…6792) `applyTenantScope()` claimed to be the single chokepoint every CRUD method routes through. Nothing checked it, and it was false: three read doors built through `getBuilder()` and never arrived. - `findWithWindowFunctions()` returns ROWS — a caller passing `options.tenantId` got every tenant's rows. Measured with two tenants seeded: `[a1,a2,b1,b2,p1]` here against `find()`'s `[a1,a2,p1]`. - `analyzeQuery()`/`explain()` returns a PLAN whose statement `find()` would never run — the same defect #6577 fixed on these methods one builder line lower, argued on its own merits rather than folded into the row door's. - `distinct()` returns one column's VALUES for every tenant. In no card; #6792 states the opposite. The 13th read site is `aggregate()`, not `distinct`. Found by measuring the invariant rather than re-reading it. All three now call `applyTenantScope` beside their `getBuilder()` line, the position `findRows()` uses — routing through the chokepoint rather than re-deriving a predicate, since a local equality drops NULL-org platform rows (#2734) and collapses the group posture (#3623). Both early-outs are inherited, so unscoped admin reads and objects with no tenant field are unchanged. The durable half is `pnpm check:tenant-chokepoint`, wired into lint.yml: it re-derives the invariant from the AST across the SqlDriver family. Keyed on the BUILDER, not the method signature — the signature criterion the card sketches misses `distinct` (no query parameter) and `analyzeQuery` (returns a plan). Verified red on the pre-fix tree, red on a newly-added unscoped door, silent once that door is scoped. It asserts the call EXISTS, never where it sits, so the fixture is what proves the call works; that limit is stated in its header. The docstring no longer asserts the invariant — it names the gate that proves it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011p3oMjCGif84dGt4zBQLxL
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
August 9, 2026 03:52
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 9, 2026
This was referenced Aug 9, 2026
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 freeto 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.
Fixes#6792
What was open
SqlDriver.applyTenantScope()owns read-side tenant isolation for the whole SQL family:the
tenantIdearly-out, the "object has no tenant field" early-out, the NULL-orgplatform-row rule (#2734) and the ADR-0105 D2 union posture (#3623). Its own docstring
said "every CRUD method routes through it". Nothing ever checked that, and it was false
for as long as it had existed.
Three read doors built their query through
getBuilder()and never arrived.findWithWindowFunctions()options.tenantId.analyzeQuery()/explain()find()would never run.distinct()The third was in no card — see "What the card got wrong" below.
The exposure, measured end to end
The filing was explicit that it had not done this ("not measured end-to-end"), so this PR
does.
packages/drivers/driver-sql/src/sql-driver-tenant-scope-read-doors.test.tsseeds twotenants plus one NULL-org platform row and reads through each door. On
main@6595262,before any line moved:
21 assertions: 9 red before, all green after.
The fix
Each door now calls
this.applyTenantScope(builder, object, options)beside itsgetBuilder()line — the positionfindRows()uses. They route through the chokepointrather than re-deriving a predicate: a local equality would silently drop NULL-org platform
rows (#2734) and collapse group reads to active-org reach (#3623). Both early-outs are
inherited unchanged, so an unscoped admin/seed read and any object without a tenant field
behave exactly as before — each of those is pinned as its own test.
The two doors are not blurred together.
findWithWindowFunctionsis a security fix.analyzeQueryis a correctness fix of lower severity, argued on its own merits in its owncomment: it is the same defect #6577 fixed on these methods one builder line lower, and a
plan missing the tenant predicate has different selectivity and picks different indexes.
The durable half: a gate, not three lines
Both the filing and triage concluded the lasting fix is enforcement — nothing made a new
read door route through the chokepoint, which is how these got out.
pnpm check:tenant-chokepoint(scripts/check-tenant-chokepoint.mjs, wired intolint.ymlbeside its neighbours) re-derives the invariant from the AST on every run.
Keyed on the builder, not the signature. The card sketches "every method taking
(object, …, options)and returning rows". That criterion is measurably too narrow — itmisses
distinct(noqueryparameter) andanalyzeQuery(returns a plan, not rows), twoof the three real doors.
getBuilder()is the single constructor of every statement thisdriver sends, so the gate classifies every builder; one it cannot classify is a fatal
error, never a pass (#4690's family). Insert builders are exempt structurally — write-side
tenancy is
injectTenantOnInsert— not by a name list.Evidence it works, not just that it is green:
analyzeQueryonlyfindRecentlyTouched()--self-testA limitation, stated rather than left to be discovered. The gate asserts the call
exists on that binding, never where it sits. Measured: with the call relocated below
builder.toSQL()and belowawait builder, the gate reported clean (19/19) while nineassertions in the fixture went red. Making it position-aware would have it re-implement
knex's evaluation order from the AST and be wrong in a new way. The gate proves the call
is there; only the fixture proves it works — both are in this PR, and neither is
redundant. This is written into the script header.
What the card got wrong
distinct()was found by measuring the invariant rather than re-reading it, and the cardsays the opposite: it lists
distinctamong the 13 scoped call sites. It is not one —the 13th read site is
aggregate(). The triage comment repeated the count withoutre-deriving which methods it covered, and both rounds of PM measurement inherited that
sentence. Two of the three doors were found by a human reading the file for another reason;
the third needed a machine.
distinct()is documented with a runnable example(
content/docs/protocol/objectql/query-syntax.mdx), so it is exposed the same way thewindow door is. It is fixed here because the in-scope gate is red without it, and exempting
a live documented disclosure to make my own gate green would be exactly the fake green this
repo's gate conventions exist to prevent.
Frozen drivers (#5499)
No DEBT row, and the reason is a real absence rather than an omission:
driver-memoryanddriver-mongodbdo not use this mechanism at all — neither file containsgetBuilderorapplyTenantScope(mongodb enforces its own wall inmongodb-tenancy-guard.ts, a differentshape). The gate's scan set is the
SqlDriverfamily only:driver-sqlplus the twosubclasses that inherit the chokepoint, so a new door added one layer down is caught too.
Docs
The three pages that teach these doors with runnable examples now say that tenancy works as
it does on
find()— the predicate applies only when the call carriesoptions.tenantId,and the examples omit it. No
content/docs/releases/edits.The docstring
It no longer asserts the invariant; it enumerates the doors, explains why the write path is
deliberately a different mechanism, records that the old sentence was false, and names the
gate that now proves it.
🤖 Generated with Claude Code
https://claude.ai/code/session_011p3oMjCGif84dGt4zBQLxL
Generated by Claude Code