Skip to content

fix(plugin-security): a check-only write policy no longer disables both write-side row gates (#8059) - #8099

Merged
hotlong merged 1 commit into
mainfrom
claude/issue-8059-check-only-policy-write-scope
Aug 12, 2026
Merged

fix(plugin-security): a check-only write policy no longer disables both write-side row gates (#8059)#8099
hotlong merged 1 commit into
mainfrom
claude/issue-8059-check-only-policy-write-scope

Conversation

@hotlong

Copy link
Copy Markdown
Contributor

Fixes#8059

The exposure

On the shipped showcase, on stock main, a persona holding the plain contributor position:

  • GET /data/showcase_invoice/:id404invoice_own_rows correctly hides an invoice it does not own, and
  • PATCH /data/showcase_invoice/:id200, and the row actually changes (re-read as admin).

Same on showcase_invoice_line, which derives access from that invoice via controlled_by_parent. A caller who cannot read a record can write it by id — including reassigning its owner to itself. The #1994 class, reopened for this authoring shape one issue after #7665 closed it for the select-only shape.

Not showcase-specific: any app authoring a using narrowing for a position plus any update-class policy — even one that only validates the post-image — loses by-id write scoping on that object. The showcase is the shipped example of the recommended authoring style.

Both sites move, because one authoring shape disabled both belts

The showcase's invoice_owner_immutable is operation: 'update' with a check clause and no using.

Site 1 — computeLayeredRlsFilter, the write-visibility floor.#7665's derivation triggered on collected.length === 0. A check-only policy is fully applicable (object, positions, operation all match), so the set was non-empty and the derivation never ran — while that same policy compiled to no row filter at all, having no using for the compiler to read. Layer 1 was null and every write-side row gate composed from it went back to being a no-op: the by-id pre-image gate, the controlled_by_parent master check, the bulk AST injection. The trigger now asks whether a write-scope predicate applies, which is what #7665 criterion 5 says: a check clause is post-image validation (ADR-0058 D4), not a scope predicate. The predicate test is a strict superset of the emptiness test, so every path #7665 pinned is reached on the same inputs as before.

Site 2 — computeWriteCheckFilter, the post-image belt.heldPositions was not passed, so getApplicablePolicies evaluated the ADR-0090 P2 applicability domain against [] and dropped every policy declaring positions from the check, for every caller — holder and non-holder alike. A position-scoped check clause was inert (ADR-0049 enforce-or-remove; the class #3539 closed for org-scoped policies). Positions are now threaded through exactly as every other collectRLSPolicies call site threads them, and the domain still decides: a non-holder is still outside it.

Third site, a direct consequence of site 1 and fixed with it.checkAuthoredRowWrite reached its abstain verdict for check-only policies only because Layer 1 happened to be null. With a scope now derived in exactly that case, the derived readable-set scope would have begun answering admit — which the by-id widener uses to widen. Its authored-set pre-check now requires a policy that actually declares a row scope, so a derived scope still cannot masquerade as an authored admission (#5493 / #7281). This preserves the previous verdict rather than changing it.

policyDeclaresClause in rls-compiler.ts is the single definition of "declares a clause" — the same test the compiler already applies per policy when it skips a policy carrying no predicate for the clause being compiled.

verify --rls on the showcase: 2 holes → 0 holes

The base probe persona holds no positions, so this class is unreachable for it — verify --rls on this branch alone reports 0 holes, and so does stock main. That number proves nothing, so the real signal was measured with #8067's position personas, composed locally with this fix in one tree, toggling only this commit:

contributor personaall personasexit
this commit reverted5 PROVEN (3 consistent, 2 HOLES)35 PROVEN (33 consistent, 2 HOLES)1
this commit applied5 PROVEN (5 consistent, 0 HOLES)35 PROVEN (35 consistent, 0 HOLES)0

The ablated run reproduces #8067's reported before-state exactly, down to the sentence:

✗✗ showcase_invoice [rls-hole] the probe cannot read it (GET 404) yet MUTATED it by id (PATCH 200)
✗✗ showcase_invoice_line [rls-hole] the probe cannot read it (GET 404) yet MUTATED it by id (PATCH 200)

Both objects flip to proven and consistent — the proven count rises 33 → 35, so they became proofs, not skips. ✓ verify passed. Nothing in packages/verify, the probe, or the showcase's policies was touched; the composition was local and never pushed.

Tests

packages/plugins/plugin-security/src/check-only-write-scope.test.ts — 21 new pins on the showcase's exact invoice shape (select-only using + a check-only update policy declaring positions, public_read_write OWD, the member_default floor present and out of domain).

Every refusal is asserted on its ADR-0112 envelope, never on the bare fact of a refusal, because the two belts overlap on the obvious probe: revert site 1 and a "the PATCH was refused" assertion stays green, since the check gate refuses it instead (the caller cannot read the pre-image, so the post-image is missing owner and fails the check by accident). The belts have distinct envelopes — record_access_denied / "(row-level security)" for the row gate, record_change_not_allowed / "would violate a row-level CHECK" for the check gate — so a refusal migrating between them reddens.

Site 1 is additionally pinned where the check gate provably cannot stand in for it:

  • a payload that satisfies the check — the caller patches the unreadable invoice to owner: itself, a record steal that belt 2 passes by design, so only the derived row scope can refuse it;
  • the bulk path, which step 3.6 explicitly declines to post-image validate.

Every refusal is followed by an admin re-read of the row through the full middleware chain plus the stored row — a refusal that still wrote is the defect.

Also pinned: Layer 1 really derives (explain reports rls narrows, not not_applicable; the derived scope admits the caller's own rows rather than being a deny sentinel); heldPositions reaches the check filter for a holder and not for a non-holder; nothing over-blocks (in-scope update, detail update, detail insert and delete by a legitimate holder all still land); and #7665 criterion 5 is unchanged where a real update-scope usingis authored — the widener still widens and its boundary still refuses.

Independent reverse verification, each site ablated alone from the committed state:

  • site 1 reverted, site 2 intact — 6 failed / 15 passed. The record steal completed (expected a refusal, got a completed update); the ordinary PATCH was still refused but by the wrong belt ('You are not allowed to save this record with the values you entered...' instead of 'You do not have access to this record...'); the controlled_by_parent line update and insert both completed; the bulk update rewrote the unreadable row; explain reported not_applicable. Every site-2 pin stayed green.
  • site 2 reverted, site 1 intact — 2 failed / 19 passed, both check-gate pins, both expected a refusal, got a completed update (the owner reassignment landed). Every site-1 pin stayed green.

Verification

  • pnpm --filter @objectstack/plugin-security test51 files, 1022 tests, all pass (1001 before this PR, +21 new).
  • pnpm --filter '...@objectstack/plugin-security' typecheck26 packages clean. That filter is the prefix / downstream-consumer direction; the dependency closure was built first (turbo run build --filter='...@objectstack/plugin-security', 66 tasks).
  • pnpm check:type-check-debt — OK, "none above its recorded number". Not raised: plugin-security's test layer measures 11 against a recorded ceiling of 21.
  • pnpm --filter @objectstack/dogfood test — 624 passed, 3 skipped, 0 failed.
  • node scripts/check-nul-bytes.mjs — OK.

Generated by Claude Code

…th write-side row gates (#8059)
On the shipped showcase a `contributor` persona got GET 404 and PATCH 200 with
the row actually changed, on `showcase_invoice` and its `controlled_by_parent`
lines: a caller who could not read a record could write it by id. One authoring
shape — an update-class policy declaring `check` and no `using` — switched off
both write-side belts at once.
Site 1: the #7665 write-visibility floor triggered on an EMPTY applicable set. A
check-only policy is applicable (object + positions + operation) but carries no
`using`, so the derivation was skipped while the policy compiled to no row
filter — Layer 1 null, and the by-id pre-image gate, the controlled_by_parent
master check and the bulk AST injection all no-ops again. The trigger now asks
whether a write-scope PREDICATE applies, per #7665 criterion 5; a `check` clause
is post-image validation (ADR-0058 D4), not a scope predicate.
Site 2: `computeWriteCheckFilter` withheld `heldPositions`, so the ADR-0090 P2
applicability domain was evaluated against `[]` and every policy declaring
`positions` was dropped from the post-image check for EVERY caller — a
position-scoped `check` clause was inert (ADR-0049 enforce-or-remove).
Consequence of site 1, fixed with it: `checkAuthoredRowWrite` reached `abstain`
for check-only policies only because Layer 1 was null. It now requires an
authored policy that declares a row scope, so a derived scope cannot masquerade
as an authored admission (#5493 / #7281) — preserving the previous verdict.
`policyDeclaresClause` in `rls-compiler.ts` is the single definition of
"declares a clause", the same test the compiler applies per policy.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V
@vercel

vercelBot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 12, 2026 3:21pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-security.

12 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/deployment/cli.mdx(via @objectstack/plugin-security)
  • content/docs/kernel/runtime-services/sharing-service.mdx(via @objectstack/plugin-security)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/access-recipes.mdx(via packages/plugins/plugin-security)
  • content/docs/permissions/authorization.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/explain.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/permissions-matrix.mdx(via packages/plugins/plugin-security)
  • content/docs/permissions/sharing-rules.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/index.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/packages.mdx(via @objectstack/plugin-security)
  • content/docs/ui/audience-based-interfaces.mdx(via packages/plugins/plugin-security)
  • content/docs/ui/dashboards.mdx(via @objectstack/plugin-security)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/plugin-security)

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.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 12, 2026
@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — domain:cli seat (#6024) — accepted, enqueued

Reviewed against the three red lines the dispatch made binding, all three cleared:

1. Both sites must move together. They do, and the PR proves why they had to: one authoring shape (operation: 'update' + check, no using) disabled both belts at once. Site 1 because a check-only policy is fully applicable yet compiles to no row filter, so collected.length === 0 was false while the filter was empty; site 2 because heldPositions was never threaded, so the ADR-0090 P2 domain was evaluated against [] and dropped every position-scoped policy from the check for every caller. Landing either alone leaves a live hole — the ablation runs below measure exactly that.

2. The refusal proven by an admin re-read showing the row unchanged. Every refusal in the new suite is followed by an admin re-read through the full middleware chain plus the stored row. This is the assertion that separates "the API said no" from "the write did not happen," and both ablation runs failed on precisely that distinction (expected a refusal, got a completed update), so the re-read is load-bearing, not decoration.

3. Each site reverse-verified independently. Site 1 ablated alone: 6 failed / 15 passed, every site-2 pin green. Site 2 ablated alone: 2 failed / 19 passed, every site-1 pin green. Disjoint failure sets in both directions — neither fix is riding on the other's coverage.

What raises this above the brief

The dispatch did not ask for it, and it is the single most important thing in the PR:

revert site 1 and a "the PATCH was refused" assertion stays green, since the check gate refuses it instead (the caller cannot read the pre-image, so the post-image is missing owner and fails the check by accident).

The obvious probe for this defect is vacuous. A refusal migrating between belts is indistinguishable from a refusal holding, unless you assert the ADR-0112 envelope — and the ablation run confirms the migration is real (record_change_not_allowed / "would violate a row-level CHECK" where record_access_denied / "(row-level security)" belongs). Site 1 is then additionally pinned where belt 2 provably cannot stand in: the record steal (a payload that satisfies the check by design) and the bulk path (step 3.6 explicitly declines to post-image validate). That is this lane's recurring finding — a check that cannot fail is worse than no check, because it is consumed as evidence — found and closed by the developer without being told to look.

The showcase measurement is reported the same way. verify --rls on this branch alone reports 0 holes and so does stock main, and the PR says so rather than banking the 0 (ADR-0076 D12 / #2462). The signal is the local composition with #8067's position personas, both numbers taken in one tree toggling only this commit, with the proven count rising 33 → 35 — so the objects became proofs, not skips. Attribution is not inferred from the merge.

Carried forward

The checkAuthoredRowWrite guard is behavior-preserving and must not be deleted as cleanup. It restores the abstain that a null Layer 1 used to produce incidentally; with site 1 deriving a scope in exactly that case, a derived readable-set scope would otherwise start answering admit to the by-id widener (#5493 / #7281). If a future card wants a derived scope to widen a by-id write, that is a deliberate contract decision — it must be reached on purpose, never by removing this guard because it "looks redundant."

Not raised: check:type-check-debt OK (plugin-security test layer 11 against a ceiling of 21). Pre-existing 271-error surplus across 9 under-ceiling entries left alone — #6376, out of scope here, correctly not touched.

Flipping ready and enabling auto-merge.Dogfood Verify CLI is already green on this head; the remaining gates are in flight and auto-merge will hold until they converge. Once this lands, #8067 comes off its hold — it was held only because dogfood-verify gates the merge queue for every lane, and this is the fix that turns its two reds green.


Generated by Claude Code

@hotlong
hotlong marked this pull request as ready for review August 12, 2026 15:25
@hotlong
hotlong enabled auto-merge August 12, 2026 15:25
@hotlong
hotlong added this pull request to the merge queueAug 12, 2026
Merged via the queue into main with commit 6af87d7Aug 12, 2026
26 checks passed
@hotlong
hotlong deleted the claude/issue-8059-check-only-policy-write-scope branch August 12, 2026 16:00
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[security] A check-only update policy suppresses the #7665 write-scope derivation — a contributor by-id-writes showcase_invoice rows it cannot read

2 participants

@hotlong@claude