Uh oh!
There was an error while loading. Please reload this page.
fix(objectql,spec): run the pre-delete reference check under the system identity - #12596
Conversation
…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
os-warren
commented
Aug 26, 2026
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 |
| # | 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 developerMessageand 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/specdoes 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
📓 Docs Drift CheckThis PR changes 2 package(s): ⛔ 1 release-owned page(s) name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 127 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin b47eda92db7ff51a84ea5874b6bfa5e216a1ec54 && git checkout b47eda92db7ff51a84ea5874b6bfa5e216a1ec54
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 0c77ea45584b6b54c510f85ca69b772cf3659703 16d9a430569523a82afb363130a7d1a7aa230c9a && git checkout -B drift-repro 0c77ea45584b6b54c510f85ca69b772cf3659703 && git merge --no-ff 16d9a430569523a82afb363130a7d1a7aa230c9a
node scripts/docs-audit/affected-docs.mjs --json 0c77ea45584b6b54c510f85ca69b772cf3659703
|
os-warren
commented
Aug 27, 2026
PM — |
…ference-cleanup-system-identity
…ed registerObject arity `Type Check · debt ledger` went red on this branch: `check:type-check-debt --re-measure` measured `@objectstack/objectql` at 356 against a recorded 354 (+2) and `@objectstack/plugin-security` at 12 against a recorded 11 (+1). All three are the same diagnostic in the two suites this PR added: engine-reference-check-system-identity.test.ts(125,61): error TS2554: Expected 2-5 arguments, but got 1. engine-reference-check-system-identity.test.ts(176,61): error TS2554: Expected 2-5 arguments, but got 1. delete-reference-cleanup-system-identity.test.ts(233,60): error TS2554: Expected 2-5 arguments, but got 1. `ObjectRegistry.registerObject(schema, packageId, namespace?, ownership?, priority?)` declares `packageId` as required; the three fixture registrations passed the schema alone. The remedy the gate names is to fix the errors -- a shrink-only ledger is raised by the maintainer, never by an author -- so the calls now pass the owning package id, `'test'`, the spelling 30 other suites in this workspace already use. Type-level only: no source file, contract face or assertion is touched, and the three call sites are the whole diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnijPVVDakqK2J335JoJtq
Uh oh!
There was an error while loading. Please reload this page.
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
findagainst 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 blanket403— 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
SecurityPluginmiddleware over a realObjectQLengine, onorigin/mainat7bd6447f41:That is the reporting deployment's server log line, reproduced. After the fix the same fixture returns
SUCCESSand the row is gone.Configurations that returned
403now 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").What did NOT change — per constraint 1, and pinned:
set_nullcleanup UPDATE still runs as the caller;cascadechild 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.tsis 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
isSystemprecedent is live:packages/objectql/src/integrity/dangling-reference-audit.ts:623→context: { isSystem: true };ObjectQL.cascadeDeleteRelations's dependents probe), not by the card's line numbers;403shape 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 anduserIdsurvive. 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 assertstenantId/userId/timezoneexplicitly, so a test that only asked "isisSystemset?" 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_RESTRICTEDnow 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.
dependentCountis absent rather than0:0would 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 asys_audit_logrow — the elevated operation is a read, andplugin-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 still403s. What is pinned is the report's own A/B control, in both directions: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 indeveloperMessageand 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.
@objectstack/objectql(full, sharded 3×)@objectstack/plugin-security(full)@objectstack/spec(full, sharded 2×)objectql/plugin-securitytypecheckGates from
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackover the real changeset — 32 matched families plus the convention-triggered ones. All green, includingcheck: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 newfinddoubles 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 correct409into a500), but it now says so — the rule's own second remedy, "say something, or ask the error's type".check:query-options-erasure— the newfindcall added anas any; it now carries the declaredEngineQueryOptionstype, so theengine.tsratchet 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 nodist/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:buildfailed 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/objectqlis aliased to../../objectql/src/index.tsby that package's ownvitest.config.ts.@objectstack/specdoes resolve todist/there — measured, not assumed: the first pin run returned the bare keydelete_restricted_required_opaqueagainst a stale dist, which is why the spec build came first.THE CONVERSEand the constraint-3 ledger pin staying greendiscloseCount = trueexpected 2 to be undefinedBoth mutations were proven on disk with anchored
grep -cFcounts (removed-text 1→0, injected-text 0→1) before any result was read, restored undertrap … EXIT INT TERM, and each restore verified with an emptygit diffand a zero marker-residue count.Confidence gap, carried forward unweighed
Nobody has measured whether any deployment relies on the
403as 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 the403as intended. Per the ruling that usage is itself non-standard and belongs in an explicitdeleteBehavior: 'restrict'.Generated by Claude Code