guard(tenancy): mechanical per-chain scan across three table tiers - #2551
guard(tenancy): mechanical per-chain scan across three table tiers#2551BigSimmo wants to merge 3 commits into
Conversation
…J43Z6B)
Application code is the only tenancy boundary in this repo: migration
20260719070000_align_existing_acls revokes every table privilege from
public/anon/authenticated, so the RLS policies written TO `authenticated`
can never be evaluated and every read path uses the RLS-bypassing admin
client. One missing owner predicate has nothing behind it.
The two existing guards left five blind spots. This adds
scripts/lib/tenancy-scan.mjs — shared verbatim by npm run check:owner-scope
and tests/retrieval-owner-filter-guard.test.ts — which closes them:
A Join-through tables were invisible. Both guards only considered tables
with an owner_id column, so document_chunks, document_pages,
document_images, ingestion_jobs, ingestion_job_stages,
source_review_events and rag_visual_eval_runs — which hold the actual
document text and images — were never checked. A third "derived" tier
now requires every one of its 29 query sites to appear in a reviewed
inventory naming how ownership was established.
B user_id tenancy was not modelled. user_favourites, user_favourite_sets
and user_preferences now form a "user-keyed" tier; withOwnerReadScope
is unusable there, so all 18 call sites must carry .eq("user_id", …).
C Scope was attributed per FUNCTION. Both guards asked whether a token
appeared anywhere in the enclosing function, so a handler that scoped
query 1 and forgot query 2 passed. The predicate must now ride the same
fluent chain as the .from() call, or that chain must be handed to
withOwnerReadScope. ingestion/quality's document_index_quality read went
from passing by accident to passing by review as a result.
D The file set was too narrow. Now every .ts under src/app/api plus a
NAMED list of server-side read modules (document-detail,
document-source-loader, answer-slo, spend-metrics) — a named list, not a
src/lib/** glob, so the scan never acquires authority over the protected
src/lib/rag/** ranking surface. worker/**, scripts/** and
supabase/functions/** are out of scope by decision, documented as such.
E Dynamic RPC dispatch. The primary retrieval RPCs never appear as
.rpc("literal"); they go through callVersionedRetrievalRpc, which also
rewrites owner_filter to the public sentinel on the legacy merge path.
The guard now pins it as the only non-literal .rpc() site in src/ and
requires string-literal RPC names at all 8 of its call sites.
Five of the six proof kinds are verified in the AST rather than trusted
from a reason string: the identifier an entry names must be the same
identifier the ownership proof pinned. Every new rule ships with a
synthetic pass/fail fixture, and each sweep keeps an anti-vacuous count
assertion. No RAG file, migration or schema file is touched.
docs/audit/tenancy-defense-in-depth-review.md §6 item 2 is updated with
what the guard covers, both declared tables, and the residual gaps.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015sjekpEw82gMp57C8xzSxZ
…J43Z6B)
A guard that passes when it should fail is the exact thing this work exists to
prevent. The review constructed queries that are genuinely cross-tenant and that
the scanner reported as scoped. None is load-bearing today, so all six were
latent guard defects — but every one of them is now rejected, and each ships a
synthetic fixture asserting the rejection.
P1-1 `update:owner_id` was accepted as a tenancy predicate. It is a written
VALUE, not a filter: `.update({ owner_id: user.id, title })` with no
filter reassigns EVERY tenant's documents to the caller, and reported
ownerScoped with zero violations. `update` is dropped from the
write-payload proof entirely (insert/upsert stay — there the row is
created owned); an update is scoped by its `.eq(...)` filters or not at
all. `subtreeStampsColumn` also recursed the whole argument, so an
`owner_id` key inside a JSON `metadata` column counted as a stamp; the
stamp is now bounded to top-level properties of the payload object, of
every element of an array payload, and of every row a `.map()` callback
returns. All 15 sites whose only proof is a write-payload stamp are
insert/upsert, and none regressed.
P1-2 `.or("owner_id.…")` was accepted on a substring match, but PostgREST
`or=` is a DISJUNCTION: `.or("owner_id.is.null,status.eq.indexed")` and
`.or("owner_id.eq.<uuid>,id.eq.abc")` both return other tenants' rows and
both passed. Every top-level disjunct must now constrain the column, split
parenthesis-aware so the one real producer of the shape —
withOwnerReadScope's `owner_id.eq.X,and(owner_id.is.null,
metadata->>public_corpus.eq.true)` — still resolves. The proof is kept
rather than deleted because it is now sound; no current site relies on it.
P2-3 Write-payload identifiers resolved file-wide by name, so two functions
sharing `rows`/`payload`/`record` collided and an owner-stamped variable
in helper A vouched for `rows = body.rows` in handler B. Resolution is now
lexical: nearest enclosing function scope without descending into nested
functions, then a top-level module `const`.
P2-4 Sanctioned wrappers and owning-document helpers matched on identifier
TEXT, so a file-local no-op named `withOwnerReadScope` or
`loadOwnedDocument` passed — and this is not hypothetical, since
`loadOwnedDocument` IS a file-local function in table-facts/route.ts (that
instance is correct; the idiom is normalised). A name now counts only when
imported from its sanctioned module or defined in the file by a
declaration that itself carries an owner predicate. `scopeFacts` also
collected every shorthand/identifier argument, so
`loadOwnedDocument({ supabase, documentId: id })` registered `supabase` as
an owning-document argument; only the documentId-shaped argument now
registers, resolved through the helper's own parameter names for a
positional call.
P2-5 A table in NONE of the three tiers got zero coverage with zero signal, and
one exists: `clinical_quality_feedback_triage` carries `owner_role` and
`owner_user_id` — a tenancy column under a fourth name — is read by
clinical-quality, and was in no tier, no exemption and no note. A fourth
signal now requires any queried table outside every tier to be declared
with a reason; the triage table is declared with its administrator-gated
reason. The tier parse is anchored to database.types.ts's exact
indentation, so a reformat empties every tier and the shipped gate would
have printed "0 direct, 0 user-keyed and 0 derived-tier" and exited 0;
`main()` now fails on any empty tier count.
P2-6 answer-slo's exemption states a HEALTH_DEEP_PROBE_SECRET gate, but
health-response.ts:63 had no `tokenAuthorized` — unlike `spendSnapshot` —
so the cross-tenant SLO aggregate also ran for any caller passing
`allowUnauthenticatedDeep`. The claim was held by one opt-out flag at one
caller, not by a gate. Added, and pinned by a test.
P2-7 table-facts PATCH's `document_images` write now restates
`.eq("document_id", id)` on its own chain, which both hardens the write and
promotes its inventory entry from prose-only `reviewed-indirect` to a
mechanically checked `owned-document-helper`.
Recorded in the review §6 rather than coded around: the mechanical proofs are
order-insensitive (scopeFacts scans the whole scope, so a proof is satisfied by
an ownership query running AFTER the protected read; the "404 before this query"
clauses are prose — `parent-document-verified` is weakest, requiring only that
some owner-scoped documents query exists); tables outside the three tiers, now
signalled; and the two phases discovering files differently (`git ls-files` vs
`readdirSync` recursive), deliberately not aligned because the stronger phase
must see an untracked new route.
No real call site regressed: a per-site diff of the whole repo against the
previous scanner is byte-identical except for the one newly visible untiered
site. No migration, no SQL, no src/lib/rag/** file, no provider-backed command.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015sjekpEw82gMp57C8xzSxZ
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 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 |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5b421597-fc6e-48f0-8fb3-d57110602cca) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b183d14c3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| node.arguments.length === 1 && | ||
| isStringLiteral(node.arguments[0]) |
There was a problem hiding this comment.
When a database table is passed through an identifier rather than a string literal, this condition silently omits the query from every tier. This already occurs in src/app/api/upload/route.ts, where adminSupabase.from(table) delegates the owner-scoped documents query to src/lib/document-naming.ts, which is outside SCANNED_LIB_MODULES; removing its owner_id filter would therefore leave both phases green. Reject or explicitly inventory dynamic PostgREST .from(...) calls, while distinguishing storage-bucket calls.
Useful? React with 👍 / 👎.
| if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name)) localDeclarations.add(node.name.text); | ||
| ts.forEachChild(node, visit); |
There was a problem hiding this comment.
When a derived-table query's handler contains a nested helper or callback with an owner-scoped documents query, this recursive traversal imports that nested query into the handler's facts even if the helper is never called. A never-invoked nested function that filters documents by id and owner_id is enough to make an otherwise unscoped document_chunks read pass OWNER_PINNED_DOCUMENT_ID, defeating the per-scope proof. Do not descend into nested function-like nodes while collecting facts for the enclosing scope.
Useful? React with 👍 / 👎.
| if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name) && node.name.text === name && node.initializer) { | ||
| found = node.initializer; | ||
| return; |
There was a problem hiding this comment.
When the payload identifier is shadowed in a sibling block, this depth-first search returns the first same-named declaration anywhere in the function rather than the declaration bound at the insert/upsert use site. For example, an unreachable block containing const rows = [{ owner_id: user.id }] makes a later insert(rows) using an untrusted parameter or a different block-local rows pass with insert:owner_id. Resolve the identifier's lexical binding at its use site instead of scanning the whole function by name.
Useful? React with 👍 / 👎.
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch claude/tenancy-mechanical at starting commit b183d14; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:claude/tenancy-mechanical, then verify that the pull request head contains the published commit before reporting success. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
20260719070000_align_existing_aclsrevokes all table privileges fromanon/authenticatedand grants onlyservice_role, andsupabase/roles.sqlmakes that the default for future objects. The ~30 RLS policies writtenTO authenticatedtherefore can never be evaluated, every read path uses the RLS-bypassing admin client, and application code is the only tenancy boundary. That design is deliberate and is pinned bytests/supabase-schema.test.ts— this PR does not change it. What it changes is how much of that boundary is machine-checked.scripts/check-owner-scope-api.mjsandtests/retrieval-owner-filter-guard.test.tseach attributed scope per function by substring match, so a handler that scoped query 1 and forgot query 2 passed both. Both ignored tables with noowner_idcolumn, both ignoreduser_idtenancy, and both stopped atsrc/app/api/**/route.ts.scripts/lib/tenancy-scan.mjs, imported by the script and the test so the two can never disagree. Three table tiers are derived from the generatedsrc/lib/supabase/database.types.ts: direct (tables withowner_id), user-keyed (user_favourites,user_favourite_sets,user_preferences— previously zero mechanical coverage of any kind), and derived (join-through tables carrying onlydocument_id, which is where the document text and images live and which both old guards ignored entirely)..from("table")to its top and inward over its method calls. Recognised on-chain:.eq("owner_id"|"user_id"|"documents.owner_id", …),.is("owner_id", null), a string-literal.or("owner_id.…"), anowner_id:/user_id:key in an insert/upsert payload (resolved through a locally-declared payload variable), and the chain being argument 0 ofwithOwnerReadScope(…).documents-inner-join,owner-pinned-document-id,owned-document-helper,owner-scoped-id-list,parent-document-verified; onlyreviewed-indirectis prose-only. Moving a query site drops its entry and forces a review..tsundersrc/app/api(not justroute.ts), plus a named list of server-side read modules rather than a blanket glob, so the boundary is a decision:src/lib/document-detail.tsandsrc/lib/sources/document-source-loader.ts(which together serve six server-rendered pages) and the twosrc/lib/observabilityoperator aggregates..rpc("literal")— they go through one wrapper — so the scan asserts that wrapper's RPC-name arguments are string literals at all 8 call sites. Nothing undersrc/lib/rag/**is edited.update({ owner_id })was treated as a tenancy predicate. It is the column being written, not a filter, so an unfiltered.update({ owner_id: user.id })— a mass reassignment of every tenant's documents to the caller — passed.updateis dropped from the write-payload proof (insert/upsertstay: there the row is created owned), and the payload search is bounded to top-level keys, soowner_idburied in ametadataJSON column no longer counts..or("owner_id.…")was accepted on one matching term. PostgRESTor=is a disjunction, so.or("owner_id.is.null,status.eq.indexed")returns other tenants' rows and passed. Every top-level disjunct must now constrain the column, with parenthesis-aware splitting so the realwithOwnerReadScopeshape still resolves andnot.-prefixed groups fail closed.rowsin one function vouched forrows = body.rowsin another. Resolution is now lexical: nearest enclosing function, then a top-level module const.withOwnerReadScopepassed — not hypothetical, sinceloadOwnedDocumentis already a file-local function in this repo. A name now counts only when imported from its sanctioned module, or defined locally by a declaration that itself carries an owner predicate.clinical_quality_feedback_triagekeys tenancy onowner_user_id, a fourth column name. Untiered tables queried by the scanned set must now be declared with a reason; that one is, with its administrator-gated justification.main()also asserts no tier is empty, so a reformat of the generated types file can no longer turn the shipped gate into a silent no-op.answer-sloexemption named a gate that was not the gate.src/lib/health-response.tsran the SLO snapshot whenever the token matched or a caller passedallowUnauthenticatedDeep; only one caller's opt-out flag held the claim true. AddedtokenAuthorized &&, matching the siblingspendSnapshot, with a test pinning that an unauthenticated deep probe omits it.Result: 84 direct, 18 user-keyed, 29 derived-tier and 1 untiered-table query site are now scoped on their own chain or individually declared. Previously the derived and user-keyed tiers — 47 of those sites — were not checked at all, and three of the recognised proofs were unsound.
No migration file is added and no SQL is applied to any database.
Verification
npm run verify:pr-localVerification not run: verify:pr-local was not invoked as a wrapper.Its constituent gates were run individually and are quoted below.Decisive output:
npm run test:focusedrefused, correctly:Focused test selection is unsafe: test or configuration paths changed. The full unit suite was run instead.Every new rule ships with a synthetic pass/fail fixture, so each is demonstrated to fail on the thing it claims to catch. A guard that has only ever passed is worth nothing. The review fixes add 16 further must-fail fixtures, every one verified to have been accepted by the previous version.
Independently re-probed against the real scanner rather than taking the fixtures on trust — the three unsound shapes now yield zero proofs, and the three legitimate shapes keep theirs:
No real call site regressed. Every site's verdict was diffed against the previous scanner across the whole repo: identical per-site results and identical counts, plus exactly one new site — the untiered triage read.
npm run verify:ui— not applicable, no UI, routing, styling or browser behaviour changed.npm run verify:release— not claimed.npm run check:production-readiness— not run: provider-backed, and this batch does not contact Supabase. The two runtime edits are a narrowing filter and an added authorization condition; neither changes deployment, startup or environment behaviour.Risk and rollout
.eq("document_id", id)on a write whose target was already constrained by the preceding read, and an addedtokenAuthorized &&on a health-probe branch whose only caller already opted out. The realistic failure mode is a false positive blocking a future PR, which surfaces as a named file and line with the declaration to add. The opposite failure — a scanner that passes when it should fail — is what the fixtures exist to prevent, and the review found three of those in the first version.src/lib/rag/**; no ranking, selection, ordering, or retrieval code path is modified.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)This diff strengthens the document-access boundary and weakens nothing: no clinical content, retrieval behaviour, credential handling, or deployment behaviour is touched, and no decision-support behaviour changed, so the SaMD classification is unaffected.
tests/supabase-schema.test.ts's "keeps browser Data API table privileges disabled" assertion is untouched, and no pre-existing exemption was broadened.Notes
Findings, reported rather than silenced.
No derived-tier query has zero ownership proof. All 29 traced to a real check. Nothing was written into the inventory to make a gap disappear.
The one weak spot found is now closed rather than documented.
src/app/api/documents/[id]/table-facts/route.tsPATCH wrote review metadata todocument_imagesfiltered by.eq("id", fact.source_image_id)alone, with no document constraint on the write's own chain. It was safe — the fact row was read under.eq("document_id", id).eq("owner_id", user.id)and the preceding read confirmed the image carried the samedocument_id— but it relied on earlier statements rather than on itself. The write now carries.eq("document_id", id), and its inventory entry is a mechanically-checkedowned-document-helperproof instead of prose.Three cross-tenant reads are now documented that previously were not.
src/lib/observability/answer-slo.ts(×2,rag_queries) andsrc/lib/observability/spend-metrics.ts(rag_retrieval_logs) are deliberate operator aggregates behindHEALTH_DEEP_PROBE_SECRET. They are legitimate, and until now they sat outside every guard with no entry anywhere.Flagged, not fixed here.
docs/audit/tenancy-defense-in-depth-review.mdrecordsTEN-N2:summarizeDocument(documentId, ownerId?)insrc/lib/rag/rag.tstakes an optional owner id and would skip the filter entirely if ever called withundefined. The only caller passes a real id, so there is no live exploit. Making the parameter required is a one-line change inside a RAG-protected file, which this repository requires be flagged before it is touched at all. Flagged; not touched.The largest remaining hole, stated rather than glossed. The mechanical proofs are order-insensitive: the scanner reads the whole enclosing scope, so a proof is satisfied by an ownership query that runs after the protected read, or whose result is discarded. The reason strings' "a 404 is returned before this query" clauses are prose, not checked, and
parent-document-verifiedis weakest — it requires only that some owner-scopeddocumentsquery exists in scope. Closing that needs control-flow analysis, not a wider AST match. It is now recorded in the audit document's residual-gaps section, which previously claimed only the cross-function proofs were unchecked.Out of scope by decision, not oversight.
worker/**is job-scoped rather than request-scoped andscripts/**is operator tooling — a different tenancy model in both cases. That is stated in the scanner and in the audit document rather than left as an unexplained gap. Cross-function proofs (ownedChunkReference,ownedChunkExists,selectLabels,updateStorageCleanupJob,createDocumentSourceQuery) need dataflow across call boundaries and are declared with file and function in the key, so moving any of them drops its entry and forces a review — that ratchet is the substitute for the analysis.Refs
#J43Z6B.🤖 Generated with Claude Code
https://claude.ai/code/session_015sjekpEw82gMp57C8xzSxZ
Generated by Claude Code
Note
Medium Risk
Touches the application tenancy boundary (static enforcement plus health deep-probe and document-image write scoping). Changes are defensive and mostly CI/docs, but regressions would surface as blocked PRs or stricter probe behavior.
Overview
Widens the CI tenancy guard from handler-level regex to a shared AST scanner (
scripts/lib/tenancy-scan.mjs) that bothcheck:owner-scopeandtests/retrieval-owner-filter-guard.test.tsrun, so the checks cannot drift.The scanner classifies tables from generated types into direct (
owner_id), user-keyed (user_id), and derived (document_idjoin-through), requires tenancy predicates on each query chain (not anywhere in the handler), and forces declared inventories for indirect scope, derived reads, and tables with nonstandard tenancy columns (e.g.clinical_quality_feedback_triage). It also pins dynamic retrieval RPC usage tocallVersionedRetrievalRpcwith literal RPC names and fails closed if a tier parse goes empty.Hardens the first scanner version after review: no longer treats unfiltered
.update({ owner_id })or leaky.or(...)as scoped; resolves write payloads lexically; resolveswithOwnerReadScope/ owning-document helpers by import or real local implementation, not identifier text.Two small runtime tightenings: health deep probes only load the cross-tenant answer SLO snapshot when
tokenAuthorized(aligned with spend metrics), and table-facts PATCH adds.eq("document_id", id)ondocument_imagesupdates so derived-tier writes are mechanically provable.The tenancy audit doc and scripts index are updated to document exemptions, inventories, and residual static-analysis gaps.
Reviewed by Cursor Bugbot for commit 353d6a6. Configure here.