Skip to content

fix(plugin-security): stage RLS membership on the write check path so a membership-keyed check resolves on a bare insert (#16607) - #16722

Merged
os-zhuang merged 4 commits into
mainfrom
claude/issue-16607-rls-check-membership-staging
Sep 8, 2026
Merged

fix(plugin-security): stage RLS membership on the write check path so a membership-keyed check resolves on a bare insert (#16607)#16722
os-zhuang merged 4 commits into
mainfrom
claude/issue-16607-rls-check-membership-staging

Conversation

@os-trump

@os-trump os-trump commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16607

What

computeWriteCheckFilter now calls stageRlsMembership(context) immediately before it compiles the check clause — the same staging computeLayeredRlsFilter performs before Layer 1 compiles using. One line of behaviour, plus the comment that says why it is load-bearing.

The defect, reproduced before the fix

stageRlsMembership had exactly one call site, on the read-filter path. A bare insert performs no read, so an RLS check clause that reads a membership-resolver key (record.employer_org in current_user.employer_org_ids, ADR-0105 D11) compiled against a context in which the key had never been staged: unresolved variable → policy dropped → RLS_DENY_FILTER → refused with the check-gate envelope, on every bare insert, with or without the value the policy wanted. The two write shapes that did pass (by-id update, insert of a controlled_by_parent child) passed by accident of an earlier read on the same context object — the pre-image read at step 2.7 and the master read — having staged the key first.

The new suite packages/plugins/plugin-security/src/rls-check-membership-staging.test.ts transcribes the card's shape (a registered resolver publishing employer_org_ids; using/check twins with operation: 'all') and drives the real SecurityPlugin middleware over an in-memory engine that counts its reads. The repro is a bare insert on a fresh context: red on origin/main (refused, check envelope), green with the fix (row lands, resolver consulted once, and the harness proves no governed object was read before the verdict — so the green cannot be the read-first accident).

Why this is not a permissive degrade

The fix obtains the context the check should always have had; it relaxes nothing. Pinned on every leg the fix could have relaxed, all refused with the CHECK envelope (PERMISSION_DENIED / 403 / record_change_not_allowed / "would violate a row-level CHECK") and the row never lands:

  • a payload value outside the resolved set (the resolver was consulted; the refusal is the policy's verdict);
  • no resolver registered (the pre-D11 state);
  • a resolver that throws (and the throw is logged);
  • a resolver that omits the key.

The accidental paths, re-pinned without the accident

Per the triage boundary, the check is tested directly on a context nothing has read with: computeWriteCheckFilter on a fresh context compiles to { employer_org: { $in: [OWN_ORG] } }, equal to what the using twin compiles to on its own fresh context, with zero governed-object reads. The delegator leg (step 3.6 calls the same method with delegatorContext) is pinned the same way on a distinct context — staged for that principal, once. The by-id update and the controlled_by_parent insert are kept as middleware-level regression pins, each with the resolver consulted exactly once (staging is memoized per context, so the write-side call does not double-resolve).

Reverse verification

Committed the fix first, then removed only the write-side await this.stageRlsMembership(context); line (on-disk proof: injected marker count 1, remaining staging call sites 1 = the read-path site). Result: 6 of 13 red — the repro (expected { ok: false, … } to deeply equal { ok: true, message: 'written' }), the "write path consulted the resolver itself" pin (expected vi.fn() to be called 1 times, but got 0 times), the outside-the-set and throwing-resolver pins (refused for the wrong reason: the resolver was never called / the throw was never logged), the direct-method pin (RLS_DENY_FILTER instead of the $in filter) and the delegator pin. The two read-first middleware shapes stayed green on the defect, exactly the accident the card describes. Restored with git checkout HEAD -- path; git diff HEAD empty, blob hash identical before and after (81b0e94…), marker count 0. The subject resolves from src (./security-plugin.js), not through a dependency exports, so no dist rebuild was part of either leg.

Validation (all at f9adc5afa, after merging origin/main at be92d4680 — 8 incoming commits, 0 files in plugin-security source, 0 in spec, 0 in the lockfile)

  • pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 src/rls-check-membership-staging.test.tsTest Files 1 passed (1), Tests 13 passed (13) (VERDICT command-exit 0).
  • pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 (whole package, pre-merge at 706e6a5) — Test Files 102 passed (102), Tests 1902 passed (1902) (VERDICT command-exit 0).
  • pnpm --filter @objectstack/plugin-security typecheckVERDICT command-exit 0, 0 error TS lines, check:test-typecheck self-test and package pass.
  • Dependency closure built first: pnpm --workspace-concurrency=2 --filter '@objectstack/plugin-security^...' buildVERDICT command-exit 0.
  • Gate families derived by node scripts/pm/dispatch-gates.mjs --commands from the real change set and reconciled with --ran: 66 derived, 66 run, 0 UNRUN. Exit codes captured before any pipe. 63 exit 0 (including check:nul-bytes, check:engine-double-contract after the ledger learned the new file's three doubles via --write). Three are NOT MEASURED, not red: check:dual-build-cjs-loads, check:i18n, check:type-check-debt each exited 3 with PREREQUISITE NOT MET (the whole-repo built closure is absent in this worktree); CI builds that closure and owns those readings.
  • pnpm lint (repo-wide eslint) is CI-owned and not run here.

Clause-②: no — derived from the real export surface

Built @objectstack/plugin-security before and after and diffed dist/index.d.ts: 8 added lines, all 8 are JSDoc comment lines on the private stageRlsMembership member, 0 removed, 0 declaration-shaped lines (positive control: the same declaration regex fires 8195 times on the built file). scripts/pm/check-widening-tells.mjs --declaration no --diff on the branch diff: 3 changed file(s) read, no widening tell on any declared surface. No spec change, no new export, no schema key, no closed-set member; the accept set of the metadata contract is unchanged — an authored check clause now evaluates as authored on the write path. So needs:contract-review is not hung on either carrier.

Changeset

.changeset/rls-check-membership-staging.md@objectstack/plugin-security: patch. Judged against AGENTS.md's "A bug fix in a released package takes a patch changeset — never none, and never skip-changeset": this is a bug fix in a released package (17.3.0), so it is a patch and the label is not applied.

Tenancy pause (ADR-0131)

Nothing ADR-0131 ruled is touched: no change to organization ownership, organization_id nullability, the Layer 0 wall, or any posture. The change is context assembly on the write path of Layer 1's check clause.

Scope

Only #16607. #16608 (the insert-side check post-image being the pre-hook payload) lives in the same file and is deliberately not addressed here; the harness has no hooks, so nothing in this suite pins that behaviour either way.

验收备注

🤖 Generated with Claude Code


Generated by Claude Code

@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-security, touching 2 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/permissions/field-level-security.mdx (via SecurityPlugin (symbol, a top-level class))
  • content/docs/permissions/index.mdx (via SecurityPlugin (symbol, a top-level class))
  • content/docs/plugins/packages.mdx (via SecurityPlugin (symbol, a top-level class))
  • content/docs/ui/forms.mdx (via SecurityPlugin (symbol, a top-level class))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx (via SecurityPlugin (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 15 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json be92d468043c1b9e100e6e0f2c2fc9216a5aa850packageMentionDocs.

Which tree this was computed on

This run read content/docs from 29ed1a6156035486e50bfaf19fe091c7386e29d0 — the merge of head f9adc5afa472622ed6593321d922d69526dd54b4 into base be92d468043c1b9e100e6e0f2c2fc9216a5aa850, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 29ed1a6156035486e50bfaf19fe091c7386e29d0 && git checkout 29ed1a6156035486e50bfaf19fe091c7386e29d0
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin be92d468043c1b9e100e6e0f2c2fc9216a5aa850 f9adc5afa472622ed6593321d922d69526dd54b4 && git checkout -B drift-repro be92d468043c1b9e100e6e0f2c2fc9216a5aa850 && git merge --no-ff f9adc5afa472622ed6593321d922d69526dd54b4

node scripts/docs-audit/affected-docs.mjs --json be92d468043c1b9e100e6e0f2c2fc9216a5aa850

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs be92d468043c1b9e100e6e0f2c2fc9216a5aa850 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

3 participants