Skip to content

fix(objectql,spec): run the pre-delete reference check under the system identity - #12596

Open
os-warren wants to merge 3 commits into
mainfrom
claude/issue-12166-reference-cleanup-system-identity
Open

fix(objectql,spec): run the pre-delete reference check under the system identity#12596
os-warren wants to merge 3 commits into
mainfrom
claude/issue-12166-reference-cleanup-system-identity

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes #12166

Maintainer ruling 2026-08-26, option A — the pre-delete reference check runs under the system identity — implemented with all four binding constraints.

The defect, reproduced before it was repaired

Deleting a record runs the platform's pre-delete reference check, which issues a find against every referencing object. That probe ran as the calling operator, so a caller with full delete rights on the target but no read grant on any referencing object got a blanket 403 — regardless of whether a reference existed, and with the referencing table empty.

The three-part fixture from the report (object A referenced by B's lookup; role with full delete on A and no grant on B; B empty), driven through the real SecurityPlugin middleware over a real ObjectQL engine, on origin/main at 7bd6447f41:

DELETE OUTCOME: {
  "code": "PERMISSION_DENIED",
  "status": 403,
  "message": "You do not have permission to perform this action. Contact your administrator if you need access.",
  "developerMessage": "[Security] Access denied: operation 'find' on object 'os_ehr_andon_record' is not permitted for positions []"
}

That is the reporting deployment's server log line, reproduced. After the fix the same fixture returns SUCCESS and the row is gone.

⚠️ Clause ② — this relaxes a permission check

Configurations that returned 403 now succeed. That is accept/reject behaviour on a shipped security surface, and the gate derivation flags it independently (packages/spec/src/**"the normal landing zone of a clause-② card").

 .changeset/delete-reference-check-system-identity.md                  |  78 ++++
 packages/objectql/src/engine-reference-check-system-identity.test.ts  | 228 +++++++++++
 packages/objectql/src/engine.ts                                       | 310 ++++++++++++++-
 .../delete-reference-cleanup-system-identity.test.ts                  | 427 +++++++++++++++++++++
 packages/spec/src/system/operation-message.ts                         |  40 ++
 5 files changed, 1078 insertions(+), 5 deletions(-)

What did NOT change — per constraint 1, and pinned:

  • the caller's own delete authorisation on the target is untouched; a caller without delete rights is refused exactly as before;
  • the set_null cleanup UPDATE still runs as the caller;
  • the cascade child DELETE still runs as the caller;
  • the target's own delete still runs as the caller.

Of the 1078 added lines, 655 are the two new pin suites and 78 the changeset. The behavioural change in engine.ts is one argument (the probe's context), one disclosure decision on the refusal path, and one audit record; the rest of that file's diff is the reasoning kept next to it.

Premise re-verified on the current ref — not inherited from the card

  • the isSystem precedent is live: packages/objectql/src/integrity/dangling-reference-audit.ts:623context: { isSystem: true };
  • the pre-delete probe was located by symbol (ObjectQL.cascadeDeleteRelations's dependents probe), not by the card's line numbers;
  • the 403 shape is current, reproduced above rather than quoted.

The four constraints

1 · Scope pinned. Only the reference check switches identity. The elevation is sudo()-shaped{ ...context, isSystem: true }, never a bare { isSystem: true } — so the caller's open transaction handle, tenant scope and userId survive. A bare system context would have widened the probe across the tenant wall, which is the opposite of what this card relaxes; the unit pin asserts tenantId/userId/timezone explicitly, so a test that only asked "is isSystem set?" cannot pass in its place.

2 · The error names the OBJECT, never record contents. The elevated probe sees rows the caller may not read, so DELETE_RESTRICTED now discloses the dependent count only when the caller's own identity would have produced the same rows — compared on row identity, not length, so row-level (RLS) narrowing counts too. Otherwise the count is withheld and the refusal renders one of two new catalog keys, delete_restricted_opaque / delete_restricted_required_opaque (the same sentences minus {{count}}, all four bundled locales). Without this the refusal would be an exact, repeatable cardinality oracle over a table the caller may not read.

The referenced object and relation field are named either way — declared metadata, and the whole of what makes the refusal self-diagnosable. dependentCount is absent rather than 0: 0 would be a false statement about the rows.

The suppression is conditional, and both halves are pinned. A caller who can read the referencing object still gets the count, byte for byte as before — no existing assertion, message catalog entry or REST envelope field changed for them.

3 · Audit records both halves. triggeredBy = the deleting operator, executedAs: 'system', plus the referenced object and relation field — never a row id, value or count. Filed before the probe, so a refused or failed check is recorded too. Declared limit: this is an engine log record, not a sys_audit_log row — the elevated operation is a read, and plugin-audit's read writer declares and pins that a system-elevated read produces no row. A durable row belongs to the plugin that owns that shape.

4 · Behaviour vs identity separation. The elevation is unconditional and does not read behavior; the call site carries an explicit ⛔ not to make it conditional if the spec later declares per-relationship on-delete behaviour.

The pin asserts the terminal state, not the call

Deliberately not "the probe was issued with isSystem" — that goes green the moment someone wraps the call differently while the caller still 403s. What is pinned is the report's own A/B control, in both directions:

  • empty referencing table + no read grant + full delete rights ⇒ the delete succeeds;
  • a caller without delete rights on the target ⇒ still refused, and refused about the target, not a referencing table.

No existing assertion was changed

No test in this repo pinned the 403, and none was edited or deleted. One assertion I wrote was corrected mid-review: it asserted the refusal contained no row id at all, which was wrong — the target's own id appears in developerMessage and is the caller's own input, not a disclosure. It is now scoped to the referencing object's row ids, with the reason on the line.

Verification

Every exit code captured before any pipe.

Suite Result
@objectstack/objectql (full, sharded 3×) 4202 passed, 239 files
@objectstack/plugin-security (full) 1548 passed, 84 files
@objectstack/spec (full, sharded 2×) 11476 passed, 432 files
objectql / plugin-security typecheck exit 0

Gates from node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack over the real changeset — 32 matched families plus the convention-triggered ones. All green, including check:authorable-surface, check:liveness, check:strictness-ledger, check:i18n (CLI built first — it refuses to measure otherwise), check:where-matcher, check:engine-double-contract, check:comment-mask-adoption, check:nul-bytes, check:cross-package-test-inputs, check:test-source-alias.

Three gates went red on this diff first and were repaired, not baselined:

  • check:objectql-double-limit — both new find doubles were limit-blind; they now apply the caller's bound by presence.
  • check:durability-read-invention — the disclosure probe's catch answered silently. It still withholds rather than rethrows (propagating a disclosure probe's failure would turn a correct 409 into a 500), but it now says so — the rule's own second remedy, "say something, or ask the error's type".
  • check:query-options-erasure — the new find call added an as any; it now carries the declared EngineQueryOptions type, so the engine.ts ratchet does not grow.

The union was re-derived and every ratchet re-run on the final head 31645534b2.

Not measured, and why (both are worktree preconditions, not findings — CI builds the workspace):

  • check:dev-prereqs — refuses because 50 of 67 packages have no dist/ in this worktree; it names @objectstack/hono, account, setup, none of which this diff touches.
  • check:type-check-debt --re-measure — needs the full workspace closure built. Its structural half, check:type-check-coverage, is green, and both touched packages typecheck clean.

One transient to record honestly: @objectstack/rest:build failed during a parallel turbo build with no TypeScript error printed. Rebuilt alone at a larger heap it exits 0 — resource contention in a shared container, not this change.

Ablations — direction and exact count predicted in writing first

Predictions were written to disk before any mutation. Neither needed a rebuild, justified by import form: the objectql suite imports ./engine.js (relative source), and the plugin-security suite's @objectstack/objectql is aliased to ../../objectql/src/index.ts by that package's own vitest.config.ts. @objectstack/spec does resolve to dist/ there — measured, not assumed: the first pin run returned the bare key delete_restricted_required_opaque against a stale dist, which is why the spec build came first.

Ablation Predicted Observed
A — revert the elevation RED, 6 failures (5 security + 1 objectql), with THE CONVERSE and the constraint-3 ledger pin staying green RED, exactly 6 — the six named tests, and exactly those two green
B — force discloseCount = true RED, 1 failure; the sighted CONTROL stays green RED, exactly 1expected 2 to be undefined

Both mutations were proven on disk with anchored grep -cF counts (removed-text 1→0, injected-text 0→1) before any result was read, restored under trap … EXIT INT TERM, and each restore verified with an empty git diff and a zero marker-residue count.

Confidence gap, carried forward unweighed

Nobody has measured whether any deployment relies on the 403 as a de-facto delete gate. I found no contrary evidence: no spec or doc declares the coupling as contract, and no test in this repo pinned the 403 as intended. Per the ruling that usage is itself non-standard and belongs in an explicit deleteBehavior: 'restrict'.


Generated by Claude Code

os-warren and others added 2 commits August 26, 2026 13:41
…entity

The pre-delete reference check issued a `find` against every referencing
object using the CALLING OPERATOR's identity. A caller with full delete
rights on the target but no read grant on any referencing object got a
blanket 403 — regardless of whether a reference existed, and with the
referencing table EMPTY. It silently made "delete permission" mean
"delete + read on every referencing table", a coupling invisible in the
permission UI.

The probe now runs `sudo()`-shaped (`{ ...context, isSystem: true }`), so
the caller's transaction, tenant scope and userId survive the elevation.
Nothing else about the delete path changes identity: the `set_null`
UPDATE, the `cascade` DELETE and the target's own delete still run as the
caller.

The refusal discloses the dependent COUNT only when the caller's own
identity would have produced the same rows — otherwise the elevated probe
would turn `DELETE_RESTRICTED` into a cardinality oracle over a table the
caller may not read. The referenced OBJECT is named either way.

Maintainer ruling 2026-08-26 (option A) with its four binding constraints.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o
…evation carried past

Follows the gate union derived over the real changeset:

- `check:objectql-double-limit` — both new `find` doubles now apply the
  caller's `limit` bound by presence, so neither is looser than the engine.
- `check:durability-read-invention` — the disclosure probe's catch no longer
  answers silently. It still withholds rather than rethrowing (propagating a
  DISCLOSURE probe's failure would turn a correct 409 into a 500), but it now
  says so, which is the rule's own second remedy.
- `check:query-options-erasure` — the new `find` call carries the declared
  `EngineQueryOptions` type instead of `as any`, so the engine.ts ratchet does
  not grow.

Adds the changeset (minor on both packages — a permission-behaviour change
should not arrive as a patch bump).

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

Copy link
Copy Markdown
Collaborator Author

PM review — accepted, flipped ready, auto-merge armed. Reviewed 从严, as a permission-boundary change requires.

Measured: 5 files, +1078/−5 — and 655 of the added lines are the two new pin suites, which is the right ratio for a change that relaxes a security check.

⭐ The elevation is sudo()-shaped, and that is not what the precedent does

The ruling named dangling-reference-audit.ts as the isSystem precedent to follow. That precedent is a bare context:

// dangling-reference-audit.ts:623  — the cited precedent
context: { isSystem: true },

The fix does not copy it:

// engine.ts — what landed
return { ...(context ?? {}), isSystem: true } as ExecutionContext;

Copying the precedent verbatim would have dropped the caller's tenantId, userId and transaction handle, widening the probe across the tenant wall — turning a fix for an over-tight permission check into a cross-tenant read. The dev followed the precedent's intent while correcting its form, and pinned the difference: the probe is asserted to carry isSystem and tenantId/userId/timezone, and the caller's context object is asserted un-mutated.

That is the single most important line in this diff and it is one word wide. Verified in the diff myself.

The four ruling constraints, each honoured and each pinned

# constraint how it landed
1 scope pinned to the probe the set_null UPDATE, the cascade child DELETE, and the target's own delete are each asserted NOT elevated
2 error names the object, never contents DELETE_RESTRICTED names dependentObject unconditionally
3 audit both halves triggeredBy = operator, executedAs = 'system', filed before the probe so a refused or failed check is recorded too
4 behaviour vs identity elevation is unconditional, does not read behavior, with a do-not-make-this-conditional note at the call site

A hazard the ruling did not ask about, found and closed

Because the elevated probe now sees rows the caller cannot, DELETE_RESTRICTED's dependent count would have become an exact, repeatable cardinality oracle over a table the caller may not read. Constraint 2 forbade disclosing row contents; nobody wrote down that a count is disclosure too.

The count is now withheld unless the caller's own identity would have produced the same rows — compared on row identity, not length, so RLS narrowing counts as a difference. When withheld it is absent from the message, the developerMessage and the dependentCount envelope field — absent, never 0, because 0 would be a false statement about the rows. A sighted caller still gets the count byte-for-byte as before, so no existing assertion, catalog entry or REST envelope field changed.

Deriving "a count is disclosure" from a constraint that only named contents is the kind of extension a security change should make, and it is conditional rather than blanket, with both halves pinned.

Constraint 3's declared limit is correct and I want it visible

The audit record is an engine LOG record, not a sys_audit_log row. The reason is measured, not assumed: the elevated operation is a read, and plugin-audit's read-audit writer already declares and pins that a system-elevated read produces no row. A durable row belongs to the plugin that owns that shape. Declared in the code, the changeset and the PR body — so anyone auditing "who deleted what under system identity" knows where to look and where not to.

Three gates went red on this diff and were repaired, not baselined

check:objectql-double-limit (both new find() doubles were limit-blind), check:durability-read-invention (the disclosure probe's catch answered silently — it still withholds rather than rethrows, because propagating a disclosure probe's failure would turn a correct 409 into a 500, but now says so, which is the rule's own second remedy), and check:query-options-erasure (a new as any grew engine.ts 8→9; now carries the declared EngineQueryOptions type). All three re-run green.

Also caught by capturing the exit code directly: check:i18n was first run in a form that measured nothing--silent swallowed the file pattern. Re-run properly after building the CLI.

Ablations

Both predicted in writing before any mutation, both exact. Leg A (revert the elevation): predicted RED, exactly 6 — 5 plugin-security + 1 objectql — with the converse pin green; observed exactly those six and those two green. Leg B (force discloseCount = true): predicted 1, observed 1.

The no-rebuild justification is measured rather than asserted, and it caught a real trap: @objectstack/spec does resolve to dist/ in the plugin-security suite — "the first pin run returned the bare key delete_restricted_required_opaque against a stale dist, which is why the spec build came first." That is the stale-dist hazard biting and being noticed, not reasoned away.


⚠️ One thing the reporter needs to know, and it is not a defect in this PR

#12597, filed from this work: the write half of reference cleanup — the set_null UPDATE and the cascade child DELETE — still runs as the caller, per constraint 1. So a role with full delete on the target and no grant on the referencing object now succeeds only while that table is empty. A non-empty one still 403s, now on update instead of find.

That is the ruling's deliberate boundary, correctly not renegotiated, and it is pinned as an assertion rather than left implicit. But the original report counted 17 role×object pairs with a delete button that always 403s, and this fix's reach across those 17 depends on how many of their referencing tables are non-empty — which nobody has measured. Cross-posting to #12166 so the reporter sees the boundary rather than discovering it.

CI is the remaining gate.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/objectql, @objectstack/spec, touching 11 documentable anchor(s).

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

  • content/docs/releases/v17.mdx (via cascadeDeleteRelations (symbol))

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 anchor(s) matched too much of the corpus to be a work list: ObjectQL (symbol, 64 pages)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 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 — 127 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 aa5994e17a47187c5a09bc82b1fc1163f250092bpackageMentionDocs.

Which tree this was computed on

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

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

⚠️ 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 aa5994e17a47187c5a09bc82b1fc1163f250092b → 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 protocol:system size/xl tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

删除记录时「引用清理」用操作人身份查询引用表,读权不足即整体 403(应以系统身份执行)

1 participant