Skip to content

security(engine): exempt the referential FK-clear write from the object-level CRUD check - #12941

Merged
os-zhuang merged 4 commits into
mainfrom
claude/issue-12597-referential-field-clear-crud-exemption
Aug 28, 2026
Merged

security(engine): exempt the referential FK-clear write from the object-level CRUD check#12941
os-zhuang merged 4 commits into
mainfrom
claude/issue-12597-referential-field-clear-crud-exemption

Conversation

@claude

@claude claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #12597

Ruled twice by the maintainer. The authoritative record is the 2026-08-28 "Maintainer ruling applied (second round)" comment on the card (option B, verbatim 「6272 A1 其他同意」), plus its 2026-08-28 correction removing needs:contract-review. Round 1's blanket-isSystem framing is superseded.

What changed

packages/plugins/plugin-security/src/security-plugin.ts — the security middleware's object-level CRUD check (step 2) now skips a write that carries the existing server-derived __referentialFieldClear marker, and skips nothing else:

const referentialFieldClearWrite =
  opCtx.operation === 'update' && opCtx.context?.__referentialFieldClear === true;

// 2. CRUD permission check ([#12597] except the referential FK clear)
if (permissionSets.length > 0 && !referentialFieldClearWrite) {

The marker has been stamped by ObjectQL.cascadeDeleteRelations's set_null arm since #3023 (packages/objectql/src/engine.ts); until now it reached only the ownership-anchor guard, so the cleanup UPDATE still had to pass the CRUD grant check on the referencing object. That is the residue this card measured: a role with full delete on A and nothing at all on B could delete an A only while B was empty.

Two narrowings inside the ruled scope, both stated in the code comment:

  • update only. A marker rides a context, and a context reaches whatever the write's own hooks do next. The engine stamps this one on the set_null UPDATE alone, so any other operation carrying it is inheritance, never the ruled write.
  • The ADR-0090 D10 delegator half goes with it, because it is the same object-level grant question asked of the second principal (checkObjectPermission on delegatorSets, same object, same operation). Leaving it standing would make the delete succeed directly and refuse on-behalf-of — the invisible coupling this card removes, reintroduced on the delegated path.

Not isSystem. That bypass at the top of the middleware is total — the repo's own page says so (content/docs/permissions/system-context.mdx: "Elevation is total, and it is not granular") — and the round-1 probe measured it switching off three guards that answer questions referential integrity does not ask.

The cascade arm is untouched. await this.delete(childName, { where: { id: depId }, context } as any) in cascadeDeleteRelations is byte-identical to origin/main; the child-row DELETE carries no marker and keeps the operator's own delete authority. Option C stays excluded.

The four pins, and what each measured

All in packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts, driving the real SecurityPlugin middleware over a real ObjectQL engine.

pin site it guards reading
the card's exact fixture — full delete on A, nothing on B, non-empty referencing table :1953 (exempted) succeeds; FK cleared, referencing row survives
field-level security on the FK column :2293 PERMISSION_DENIED / 403, details.forbiddenFields names product, FK unchanged
RLS using row scope on update :2058 PERMISSION_DENIED / 403, details.recordId = the row, developer sentence says row-level security, FK unchanged
RLS post-image check (product != null) :2581 PERMISSION_DENIED / 403, developer sentence says row-level CHECK, FK unchanged

Each refusal is asserted on its own gate's ADR-0112 envelope, never on the bare fact of a refusal — a refusal migrating between gates reddens instead of reading as "still refused". Three further arms: a control for the RLS using pin (same policy, in-scope row ⇒ the write is admitted, so the refusal above is the row scope and not the exemption failing to fire); the converse (no delete right on the target ⇒ still refused); and an ordinary, unmarked update on the referencing object by the same caller ⇒ still PERMISSION_DENIED op=update, which is what proves the exemption is scoped to the marker.

One pin was inverted, deliberately and in place. The suite's constraint-1 assertion (PERMISSION_DENIED, details.operation === 'update' on a non-empty referencing table) is what measured this card's residue; the ruling falsifies it. It now asserts the ruled behaviour and carries a comment naming this card and the round-2 ruling as the reason. It was not deleted and its expectations were not quietly edited to match new output.

Audit attribution — the measured shape is preserved by construction

The write is not elevated at all: it stays the operator's own identity plus a marker, so both attribution channels the round-1 gate-(a) measurement found intact are untouched — writeAudit's user_id/actor (keyed on session.userId) and the updated_by stamp at plugin.ts:1028. The contract pin captures the context that actually reaches the engine for the cleanup write: userId: 'u_lead', isSystem not true, __referentialFieldClear: true.

Verification

Union re-run at aae820b00, all through scripts/pm/os-verify-lock.sh; verdicts read off the lock's own VERDICT line and vitest's own summary. After the review fix below, the target suite and the ratchet family were re-run at the new head 0dd9a3496 (working tree clean): Test Files 1 passed (1) · Tests 14 passed (14), with check:where-matcher, check:engine-double-contract, check:query-options-erasure, check:nul-bytes and check-comment-mask-adoption all exit 0.

  • pnpm --filter @objectstack/plugin-security testTest Files 87 passed (87) · Tests 1585 passed (1585)
  • the target suite alone — Tests 14 passed (14)
  • pnpm --filter @objectstack/plugin-security typecheck — exit 0. ⚠️ Honest limit: this package's tsconfig.json excludes **/*.test.ts, verified with tsc --noEmit --listFiles (0 hits for the edited test file), so that green says nothing about the test file. The test file's correctness is measured by vitest executing it.
  • pnpm --filter @objectstack/objectql exec vitest run src/engine-cascade-delete.test.ts src/engine-reference-check-system-identity.test.ts2 files, 21 tests passed (the marker's producer side, unchanged)
  • pnpm lint (full repo, eslint . --no-inline-config) — exit 0, clean
  • derived gate families (node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack), all exit 0: check:where-matcher, check:engine-double-contract, check:objectql-double-limit, check:cross-package-test-inputs, check:test-source-alias, check:type-source-resolution, check:type-check-coverage, check:query-options-erasure, check:published-files, check:page-declaration-shape, check:slot-lookup, check:nul-bytes, check:changeset-gate-self-tests, check:objectui-changeset, check:pm-half-states, check:i18n-stale-fill, plus check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check-ci-filter-parity, check-comment-mask-adoption, check-plugin-teardown-shape, release-rehearsal-clone --self-test, docs-audit/check-affected-docs.
  • NOT MEASURED, not red: check:i18n refused with PREREQUISITE NOT MET — the workspace CLI is not built. Narrowing by inspection: this package's extract config (scripts/i18n-extract.config.ts) runs --objects-only over a closed list of six src/objects/* definitions plus the generated bundles; neither edited path is in that population, so the extract output cannot move. CI runs the gate.
  • Base failure hit during this run, since RESOLVED — recorded rather than deleted: while this branch was being built, pnpm check:adr-anchors was red on origin/main (ADR-0006 D4 is cited by 1 file(s), but ADR-0006 declares no D4packages/spec/src/data/object.zod.ts), tracked as [finding] The merge queue is blocked repo-wide: check-adr-anchors is red on mainobject.zod.ts cites ADR-0006 D4, which no ADR-0006 file declares #12913. It was not mine, packages/spec is not this seat's surface, and ⛔ check-adr-anchors was not touched to make it go away. fix(spec): namespace tombstone stops citing ADR-0006 D4 — unblocks the merge queue's check-adr-anchors red #12917 has since landed (750fff559, in origin/main's history) and the base is green on that gate: at origin/main grep -c 'ADR-0006 D4' on that file returns 0, with a positive control on the same ref and file (namespace) returning 8, so the zero is a real reading. Nothing here asserts a current base failure.

Reverse verification (ablation)

Prediction committed before the mutation (141ba0680, empty commit). Reverting the exemption to the bare if (permissionSets.length > 0):

  • mutation proven on disk: blob cfbb035f9acdc533, anchored grep -c in both directions (guarded spelling 1 → 0, bare spelling 3 → 4);
  • reading: Tests 6 failed | 8 passed (14) — exactly the six predicted, including all three guard pins, which go red despite still refusing because each asserts its own gate's envelope and the restored CRUD check answers with none of them;
  • restore proven: git hash-object equal to the HEAD blob (cfbb035f), git diff HEAD empty, git status --porcelain empty; trap … EXIT INT TERM with absolute paths throughout.
  • No rebuild leg applies: the suite imports the mutated file relatively (./security-plugin.js), so vitest reads the source in this checkout, not dist/.

Docs

Reviewer-of-record fix. The exemption comment cited delete-reference-cleanup-crud-exemption.test.ts, a filename that resolves to nothing — the pins live in this package's delete-reference-cleanup-system-identity.test.ts. Corrected in 0dd9a3496; git grep for the dead name now returns 0 files, and the cited name resolves to a tracked file. A citation that resolves to nothing is exactly the defect class the ADR-anchor gate exists to name, and worse in a comment explaining a permission boundary.

The docs-drift bot's 8 rows are anchored on a token this diff introduces only in PROSE. Measured independently:

grep -c set_null  packages/plugins/plugin-security/src/security-plugin.ts
  merge base aef1b7e64 : 0
  head       aae820b00 : 2   -> lines 1958 and 1988, BOTH comment lines (`//`)
positive control, same query shape, token really present at base:
  grep -c permissionSets @ aef1b7e64 : 117

No code line in this diff contains set_null. So every listed row (data-api, field-types, fields, validation-rules, troubleshooting, protocol/objectql/types, and the two release pages) is anchored via set_null (literal) on explanatory comment text. Reported as a bot finding rather than acted on — anchoring on comment text over-lists, and this lane has now hit that mode more than once.

Wrong rows are not a docs clearance, so the hand-read was done anyway. The bot's own blind spot is a page that declares a rule by its inputs, sharing no token with the emitter — and it listed no content/docs/permissions/ page at all, which is precisely where this diff could falsify something. One question was asked of authorization.mdx, access-matrix.mdx and system-context.mdx: does any sentence state or imply that the reference-cleanup write is subject to the caller's authority on the referencing object?

  • No. authorization.mdx's layer table describes the ladder for caller-initiated operations (layer 3 = object CRUD, fail-closed 403); it does not assert that the ladder has no exemptions, and it already coexists with two documented-elsewhere layer-3 exemptions it does not caveat either — the isSystem short-circuit and ADR-0056's publicFormGrant.
  • system-context.mdx is about isSystem, which this diff deliberately does not use; its row 18 (isSystem skips referential-integrity checks, engine.ts:3314) is untouched and still true, and its "Elevation is total, and it is not granular" is the sentence this design obeys rather than contradicts.
  • access-matrix.mdx is the snapshot-gate page — the {read, create, edit, delete} map is per (permission set × object) and is not changed by this diff.
  • Repo-wide, no page asserts that deleting a record requires rights on the referencing object. content/docs/deployment/troubleshooting.mdx's "Cannot delete record: delete restricted" entry is about restrict escalation and multiple: true member semantics, with zero permission/403 content.

No falsified sentence found; nothing in content/docs/ is edited by this PR.

The two release-owned rows were audited, never edited (content/docs/releases/ is release-owned):

Clause-② — self-assessed against the actual diff: no

The diff touches packages/plugins/plugin-security/src/** and .changeset/ only. No published contract's accept/reject face changes and no public surface widens: no packages/spec schema, no export added or removed, no authorable key, no REST parameter set. What changes is a runtime permission-gate verdict — the 安全/权限边界 manual floor, a different control, and one already exercised twice by the maintainer's own rulings on this card. This matches the maintainer's recorded correction removing needs:contract-review; ⛔ that label is not re-attached.

Changeset

.changeset/referential-field-clear-crud-exemption.md@objectstack/plugin-security: minor. MINOR because this changes which deletes succeed: an observable behavioural contract change on the delete path, not a patch-grade defect repair.


Generated by Claude Code


Generated by Claude Code

claude added 3 commits August 28, 2026 09:31
…lear

The engine's `set_null` reference cleanup already tags its UPDATE with the
server-derived `__referentialFieldClear` marker (#3023). The marker reached only
the ownership-anchor guard, so the cleanup write still had to pass the
object-level CRUD check on the referencing object: a role holding full delete
rights on A and no grant at all on B could delete an A only while B was empty.

plugin-security now exempts a marked UPDATE from the object-level CRUD grant
check, and from that check alone. Field-level security on the FK column, the RLS
`using` row scope and the RLS post-image `check` all stay enforced -- each pinned
-- and the `cascade` arm keeps the operator's own delete authority, untouched.
The write is not elevated: it stays the operator's identity, so audit
attribution (`user_id`/`actor`, `updated_by`) is unchanged.

Ruled by the maintainer on 2026-08-28 (#12597, second round, option B).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry
Committed BEFORE the mutation so the prediction cannot be written after the
reading. Reverting the exemption (restoring the bare
`if (permissionSets.length > 0)` at the object-level CRUD check) is predicted to
turn SIX of the fourteen tests in
`delete-reference-cleanup-system-identity.test.ts` RED:

  1. `[#12597 - PIN INVERTED] a non-empty referencing table no longer needs ...`
  2. `THE CONTRACT: full delete on A + NOTHING on B + a non-empty ...`
  3. `GUARD 1 - field-level security on the FK column still refuses ...`
  4. `GUARD 2 - the RLS \`using\` row scope ... still refuses ...`
  5. `GUARD 2 CONTROL - the same policy admits the write when the row IS in scope`
  6. `GUARD 3 - the RLS post-image \`check\` still refuses ...`

Guards 1-3 are predicted red DESPITE still refusing, because each asserts its
own gate's ADR-0112 envelope (`forbiddenFields`, `recordId` +
"row-level security", "row-level CHECK") and the reverted CRUD check answers
first with none of them. That is the whole reason those pins are written on
envelopes rather than on the bare fact of a refusal.

The other two new arms are predicted GREEN either way: the converse (no delete
right on the target) and the ordinary unmarked update, neither of which the
exemption is in a position to change.

Note on the ablation's mechanics: the mutated file is
`plugin-security/src/security-plugin.ts`, which this suite imports RELATIVELY
(`./security-plugin.js`), so vitest reads the SOURCE in this checkout and no
rebuild leg applies to it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry
MINOR by the maintainer's 2026-08-28 ruling: the change moves which deletes
succeed, an observable behavioural contract change on the delete path rather
than a patch-grade defect repair.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Aug 28, 2026
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/api/data-api.mdx (via set_null (literal))
  • content/docs/data-modeling/field-types.mdx (via set_null (literal))
  • content/docs/data-modeling/fields.mdx (via set_null (literal))
  • content/docs/data-modeling/validation-rules.mdx (via set_null (literal))
  • content/docs/deployment/troubleshooting.mdx (via set_null (literal))
  • content/docs/protocol/objectql/types.mdx (via set_null (literal))

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

  • content/docs/releases/v15.mdx (via set_null (literal))
  • content/docs/releases/v17.mdx (via set_null (literal))

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
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • 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 — 14 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 e5f66214a3106edb7086f24d6acb0c12e7f865d2packageMentionDocs.

Which tree this was computed on

This run read content/docs from d77aef4034f1fdd34f451910460308990edd8dc2 — the merge of head 0dd9a3496b89d561f55b494973873de10e1123c5 into base e5f66214a3106edb7086f24d6acb0c12e7f865d2, 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 d77aef4034f1fdd34f451910460308990edd8dc2 && git checkout d77aef4034f1fdd34f451910460308990edd8dc2
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e5f66214a3106edb7086f24d6acb0c12e7f865d2 0dd9a3496b89d561f55b494973873de10e1123c5 && git checkout -B drift-repro e5f66214a3106edb7086f24d6acb0c12e7f865d2 && git merge --no-ff 0dd9a3496b89d561f55b494973873de10e1123c5

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

⚠️ 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 e5f66214a3106edb7086f24d6acb0c12e7f865d2 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

The comment cited `delete-reference-cleanup-crud-exemption.test.ts`, a filename
that resolves to nothing: the pins live in this package's
`delete-reference-cleanup-system-identity.test.ts`, in its `#12597` describe.
Caught in review. A citation that resolves to nothing is the defect class the
ADR-anchor gate exists to name, and it is worse in a comment explaining a
permission boundary, where the next reader is looking for the evidence.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry
@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Docs-drift bot report: all 8 rows are anchored on a token this diff introduces only in a COMMENT

Filing this as a bot finding rather than acting on the rows.

grep -c set_null  packages/plugins/plugin-security/src/security-plugin.ts
  merge base aef1b7e64 : 0
  head       aae820b00 : 2   -> lines 1958 and 1988, BOTH comment lines
positive control (same query shape, token really present at the base):
  grep -c permissionSets @ aef1b7e64 : 117

No code line in this diff contains set_null; the two occurrences are the explanatory comment naming deleteBehavior: 'set_null' and the update-only narrowing. So data-api, field-types, fields, validation-rules, troubleshooting, protocol/objectql/types and the two release pages are all listed via set_null (literal) on prose. Anchoring on comment text over-lists, and this lane has hit that mode more than once.

…and a wrong row list is not a docs clearance, so the hand-read was done anyway

The bot names its own blind spot — a page that declares a rule by its inputs shares no token with the emitter — and it listed no content/docs/permissions/ page, which is exactly where a change to which deletes succeed could falsify a sentence. One question was put to authorization.mdx, access-matrix.mdx and system-context.mdx: does any sentence state or imply that the reference-cleanup write is subject to the caller's authority on the referencing object?

No falsified sentence found. authorization.mdx's layer table describes the ladder for caller-initiated operations and does not claim the ladder is exemption-free — it already coexists with two layer-3 exemptions it does not caveat (isSystem, ADR-0056's publicFormGrant). system-context.mdx is about isSystem, which this diff deliberately does not use; its row 18 and its "Elevation is total, and it is not granular" are both untouched and still true. access-matrix.mdx is the per (permission set × object) snapshot, unaffected. Repo-wide, no page asserts that deleting a record requires rights on the referencing object, and deployment/troubleshooting.mdx's "Cannot delete record" entry is about restrict escalation and multiple: true member semantics with zero permission/403 content.

The two release-owned rows: audited, not edited

Both audited read-only: a code PR does not edit content/docs/releases/.

Review fix pushed

0dd9a3496 corrects the dangling delete-reference-cleanup-crud-exemption.test.ts citation at security-plugin.ts:1983 to the suite that exists, delete-reference-cleanup-system-identity.test.ts. git grep for the dead name now returns 0 files; the cited name resolves to a tracked file. Re-run at that head: Test Files 1 passed (1) · Tests 14 passed (14), with check:where-matcher, check:engine-double-contract, check:query-options-erasure, check:nul-bytes and check-comment-mask-adoption all exit 0.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 28, 2026 10:22
@os-zhuang
os-zhuang enabled auto-merge August 28, 2026 10:22
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 28, 2026
Merged via the queue into main with commit f4e741b Aug 28, 2026
40 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-12597-referential-field-clear-crud-exemption branch August 28, 2026 10:42
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/m tests tooling

Projects

None yet

2 participants