fix(plugin-security): explain's record update/delete verdict uses the by-id write path's inputs - #19984
Conversation
…rite path's inputs security/explain answered record.visible: false for update/delete on rows the by-id PATCH/DELETE admits, on private-OWD objects: - M1: Layer 1 kept the platform ownership floor the pre-image gate drops when the sharing write verdict is allow. The gate's floor decision moves into resolvePreImageFloorDrop, and explain's computeLayeredRlsFilter binding asks it for update/delete (behaviour of the gate unchanged). - M2: canEditRecord / canDeleteRecord asked plugin-sharing with the bare context. They now stamp __writeScope from resolveWriteScopeForSharing, overwritten as the middleware's step 2.6 overwrites it. The on-behalf-of path keeps its previous inputs on both halves. Claude-Session: https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF Co-authored-by: Claude <noreply@anthropic.com>
…gine doubles node scripts/check-engine-double-contract.mjs --write: three added rows (delete, findOne, update) for the new test file, 0 seams added or lost. Claude-Session: https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift CheckThis PR changes 1 package(s): 5 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 15 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 fdfb498a4733704a55d4b8654f3da779b8dc628d && git checkout fdfb498a4733704a55d4b8654f3da779b8dc628d
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 2c1011b01bc071c545f72f2761647b8d9ab56375 aa9013ffb9712f779d8fc11bcb4a39b0f1906c49 && git checkout -B drift-repro 2c1011b01bc071c545f72f2761647b8d9ab56375 && git merge --no-ff aa9013ffb9712f779d8fc11bcb4a39b0f1906c49
node scripts/docs-audit/affected-docs.mjs --json 2c1011b01bc071c545f72f2761647b8d9ab56375
|
… of an array insert and a predicate update (objectstack-ai#19988) Fixes objectstack-ai#19950 Fixes objectstack-ai#19964 Clause-②: no (narrowing) ## What this fixes A row-level security `check` (declared on the policy, or defaulted from its `using`) is the write-side half of the policy: a row the check refuses is never stored (ADR-0058 D4: "on the write pre-image path that already exists for by-id writes … and on the AST-injected bulk path"). The write gate enforced it for a single-row insert and a by-id update, and not for the two multi-row write shapes: - **objectstack-ai#19964, array insert.** Step 3.6 excluded an array payload, so no judgement was installed and every row was stored unjudged, including under configurations that refuse every single-row insert. - **objectstack-ai#19950, predicate update** (`multi: true`, no row address). Step 3.6 skipped the new-row check and logged "governed by the using-scoped where". A policy that declares only `check` scopes nothing, and a scoped `where` says nothing about the new row in any case. The skip was unconditional, so it also covered a declared `check` that differs from `using` and a `check` defaulted from `using`. Both shapes are now judged row by row with the existing refusal (`PERMISSION_DENIED` / 403, nothing stored). One failing row refuses the whole write. The judgement is the existing `satisfiesCheck` over `matchesFilterCondition`: no predicate is compiled differently, and nothing is lowered into the `where`, so no compile surface moves. ## Landing: two packages, and why the engine is one of them - `packages/plugins/plugin-security/src/security-plugin.ts`, step 3.6 plus the seam declaration and the post-`next()` fail-closed guard (generalised from "the insert" to the operation). The by-id update branch is unchanged. The `explainAccessForCaller` wiring is not touched. - `packages/objectql/src/engine.ts`, the predicate-update branch of `update()` (`domain:engine`), plus the seam's doc comments. **Why the producer is the engine (measured, not assumed):** the rows a predicate update changes are the rows the middleware-COMPOSED AST selects. That AST is complete only after every middleware has run: step 3 of this middleware composes its own scope after step 3.6, and plugin-sharing composes its editable-rows filter onto the same AST (`sharing-plugin.ts:1405`), in plugin order. The suggested route, reading "under the caller's context" in the middleware, was measured two ways and does not hold: - A caller-context read applies READ scope and field masking. Where the read scope is narrower than the write scope, written rows go unjudged (fail-open). Where it is wider, rows that will not be written get judged (false refusals). Ablation 3 below shows the second: a judgement that is not over the actual matched rows falsely refuses the USING-only in-scope control. - The engine already holds the exact set: the D7 matched-row read (`readPriorRows`, bound to the composed AST), which the ruling says is read once and reused, and which already serves validation, the `readonlyWhen` strip and both per-row hook phases. So the security layer installs its judgement on the existing `OperationContext.postHookWriteImageCheck` seam (the one objectstack-ai#19952 built for inserts), and the engine calls it on the predicate branch. It hands over every matched row merged with the payload (the same shape as the per-row `afterUpdate` `result`), placed after `assertNoStrictDrops()`, where the payload is final. That placement follows the insert seam's contract review: "the row the seam judges must be the row that is stored". The readonly strips run earlier on this branch, so a pre-strip placement would judge values that never land. ## Mechanism hypotheses (dispatch Section 2), as measured on `2c1011b01b` 1. **Held.** Line 3000 carried `!Array.isArray(opCtx.data)`. Lines 3078-3083 set `postImage = null` for `extractSingleId(opCtx) == null` and logged "governed by the using-scoped where". 2. **Held.** `engine.ts` (the `postHookWriteImageCheck` call in `insert()`) hands `evaluate` every live row of an array insert. The array fix is plugin-side only. 3. **Refined.** The memoized `getCallerPreImage` is by-id and caller-context, so it is not reusable per row for the reasons above. The engine's memo serves instead, at no extra read wherever per-row hooks already read it. 4. **Held.** The skip was unconditional. A cell pins a `using` plus a differing `check`. One further hole, found and closed: the middleware treats a falsy scalar id (`''`, `0`) as a row address, and the engine does not (`resolveEngineUpdateDispatch`). A falsy payload id therefore carried a bulk update past the per-row judgement, admitted on a change-set-only image. The seam is now installed whenever the engine will not treat the write as addressing one row. The falsy case keeps its by-id judgement too, so it only refuses more. ## Surface beyond the claim, with reasons - `packages/objectql/src/engine.ts`: see above (cross-lane, `domain:engine`). - Existing plugin-security tests: `check-only-write-scope.test.ts` and `security-plugin.test.ts` carry engine doubles that must now honour the seam on a predicate update, the way the real engine does. Otherwise the fail-closed guard refuses them, which is the intended behaviour. One test title and comment said step 3.6 "declines to check" the bulk path; it now says 3.6 can refuse a bulk write but never scope one. That pin still discriminates a site-1 revert, now by refusal. Two comment-only edits (`rls-check-defaults-to-using.test.ts`, `rls-phantom-column-negation.test.ts`) stated the array exclusion as a fact. - **A pending release note, corrected in place: `.changeset/rls-check-defaults-to-using.md`** (from objectstack-ai#19952, not yet released). Its "What does not change" list said bulk updates "are not checked row by row", which this PR makes false. The bullet now says their new rows are checked row by row too, by this PR's entry. `check-empty-changeset` is RED on this by design: it is the DELIBERATE CORRECTION class (ruling D on objectstack-ai#17712). Its prescribed remedy is to keep the correction and get it confirmed on the PR; restoring the file would publish a false sentence. Under the landing rule objectstack-ai#19970 set, 「DELIBERATE CORRECTION 红:同 head 达档复核 PASS 记录即确认,⛔ 不等维护者」, the confirmation is a same-head at-tier review record with a PASS verdict, not the maintainer. The red is expected until that record is on the PR for the landing head. ## Behaviour that changes (all in the refusing direction) - A predicate update under a check-only policy is refused when any matched row's new image fails the check. - A predicate update that moves a matched row out of a policy's `using`, when no applicable policy declares `check`, is refused: the defaulted check, which is the answer the by-id update has given since objectstack-ai#19952. Triage's "已声明 `using` 的策略,行为保持不变" is held as: in-scope bulk updates under a `using` policy are admitted and scoped exactly as before (pinned). Only a bulk write that moves rows out of the `using` is newly refused, on the same terms as by-id. The seat confirmed this reading: by-id and bulk are two implementations of one operation and must not disagree (`RowLevelSecurityPolicySchema.check` 「defaults to USING clause if not specified」; ADR-0058 D4). - An array insert is refused when any row fails, including every configuration that already refused each single insert. - A host that installs the judgement on a predicate update and never runs it is refused (403, `error` log), as an insert already is. ## Tests **Patch round 2 (head `3247efeecd`, after merging `origin/main` `9bfbacbf8b` as merge commit `938b2acfde`):** `rls-check-multi-row-writes.test.ts` 32 passed (32); plugin-security suite 123 files, 2348 tests passed; objectql re-run because objectstack-ai#19979 touched that package: local project 155 files / 2434 tests + 154 files / 2758 tests, repo project 1 file / 5 tests; `typecheck` green for objectql and plugin-security (VERDICT command-exit 0 each). **Patch round 1 (head `9fff66cfdc`, after merging `origin/main` `3fd3a4f91b` as merge commit `a5ca1db166`):** `rls-check-multi-row-writes.test.ts` 32 passed (32); plugin-security suite 123 files, 2348 tests passed (VERDICT command-exit 0 each). The merge brought no change under `packages/objectql` or `packages/plugins/plugin-security`, so the objectql suite was not re-run; its last run is the one below, on a byte-identical `engine.ts`. **Round 1 (head `6d28dfe98d`):** New: `packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts`, 32 cells on driver-sql (better-sqlite3) and driver-sqlite-wasm, real `SecurityPlugin` + `ObjectQL`. - Failing first, on the unmodified tree: 18 red and 12 green (the controls). Every negative cell failed as "admitted" (`expected true to be false`). In the unresolvable-policy cells the single-insert leg was refused and only the array leg was admitted. - objectstack-ai#19950 cells: the check-only repro; per row not per change set (a matched row failing on an unchanged field refuses the whole write); `using` plus a differing `check`; fail-closed (an inner middleware strips the seam, and the write is refused with "the update on 'qa_ticket' was executed without the row-level CHECK being evaluated"); falsy payload id. Controls: over-fix admit, USING-only in-scope admit and scoped, `using`+`check` in-scope admit, by-id unchanged, and USING-only move-out refused on both bulk and by-id. - objectstack-ai#19964 cells: the `[admitted, refused]` repro; over-fix admit; single insert unchanged; three refuse-every-insert configurations (an unresolvable sole `using` on `insert` and on `all`, an unresolvable declared `check`). - Every refusal asserts `code` `PERMISSION_DENIED`, `status` 403 and the developer half naming the gate and verb, then reads the stored rows back under a system context. Suites: plugin-security 123 files, 2348 tests pass. objectql 309 files, 5182 tests pass (local project in two halves, plus the repo project). `typecheck` is green for both packages (plugin-security test-layer debt: 0 files, 0 errors). Ablations: each committed first, mutated through `scripts/ablation-replace.mjs` (anchor hit 1 to 0, blob changed), restored with blob equal to HEAD and an empty `git diff HEAD`. Resolution path: the plugin is imported relatively, and `@objectstack/objectql` is aliased to `src/index.ts` in this package's `vitest.config.ts`, so no `dist/` sits between the mutation and the test. | # | mutation | result | |---|---|---| | 1 | restore the non-array guard for inserts | 8 red: the 4 array-insert negative cells x 2 drivers | | 2 | never install the seam on a predicate update | 12 red: the 6 bulk negative cells x 2 | | 3 | engine judges the payload alone, not the matched rows | 6 red: the per-row cell, the falsy-id cell, and the USING-only in-scope control (falsely refused) | | 4 | install only when the id is null (falsy counts as by-id) | 2 red: the falsy-id cell, admitted | | 5 | engine never calls the seam | 16 red: refusals now carry the not-evaluated message, controls refused | | 6 | disable the post-`next()` fail-closed guard | 2 red: the fail-closed cell, admitted | ## Gates **Patch round 2 (head `3247efeecd`):** the four comment lines this change rewrote now cite the surviving record, commit `a016f08b8a` (the insert-side check), instead of a card that answers 404, and say in words that the original card no longer resolves. `GITHUB_TOKEN="$GH_TOKEN" node scripts/check-issue-citations.mjs` probes the board and exits 0: "every citation this change adds resolves (or is a declared cross-repo reference)", with 9 judged, 9 resolving and 0 unresolved added. `dispatch-gates --commands` derived the same 68 families from the same 9 paths against merge base `9bfbacbf8`. All 68 were run; `--ran` answers "68 derived, 68 run, 0 NOT-MEASURED, 0 UNRUN". 67 exit 0. `check-empty-changeset` exits 1 on `.changeset/rls-check-defaults-to-using.md` only. **Patch round 1 (head `9fff66cfdc`):** `dispatch-gates --commands` derived the same 68 families from the same 9 paths, now against merge base `3fd3a4f91`. All 68 were run; `--ran` answers "68 derived, 68 run, 0 NOT-MEASURED, 0 UNRUN". 67 exit 0. `check-empty-changeset` exits 1 on `.changeset/rls-check-defaults-to-using.md` only (the deliberate correction under "Surface beyond the claim"). The changeset gates the seat named: `check-adr-0087-registration --base origin/main` exits 0 ("1 declared-breaking changeset(s), each carrying an ADR-0087 disposition"), `check-changeset-no-major --base origin/main` exits 0, and `pnpm check:changeset-gate-self-tests` exits 0. **Round 1 (head `6d28dfe98d`):** - `node scripts/pm/dispatch-gates.mjs --commands` derived 68 families from the 9 changed paths. All 68 were run with exit codes recorded. `--ran` answers "68 derived, 68 run, 0 NOT-MEASURED, 0 UNRUN". - 67 exit 0. One exits 1: `check-empty-changeset`, the deliberate correction above. - Three first answered `PREREQUISITE NOT MET` (exit 3): `check:dual-build-cjs-loads`, `check:i18n` and `check:type-check-debt`. They pass after the workspace closure build. `check-engine-split-ratio` passes after deepening the shallow clone to its window. - Lint, as a proven narrowing: `eslint --no-inline-config --format json` over the 7 changed `.ts` files reports 7 files linted, 0 errors and 0 warnings. All 7 fall in the config's `**/*.{ts,…}` block. The config never enables type-aware linting (`eslint.config.mjs:326-328`), so this diff cannot move a verdict on an untouched file. The full `pnpm lint` is CI's. ## Acceptance notes - **Refusal cost on the predicate path.** The judgement runs where the payload is final, after the per-row `beforeUpdate` hooks and after the credential channel (`encryptSecretFields`), which runs above the strips on this branch. A refused bulk update whose payload carries a `secret` field has therefore already minted its `sys_secret` row. A validation refusal two lines below pays the same cost today. Moving the credential channel below the strips is a separate engine change. A `check` naming a secret field judges the stored reference. - **Image timing differs between the two update paths.** A predicate update is judged on the post-hook image; a by-id update is still judged in the middleware on the pre-hook change set merged with the caller-visible pre-image. Where a `beforeUpdate` hook rewrites a checked field, the bulk path is the stricter of the two. The by-id path is untouched here. - **Read cost.** On an object with no per-row hooks, a checked predicate update now reads its matched rows. The read is unbounded by the per-row hook ceiling, which applies only when hooks dispatch. On a kernel with the usual global hooks the read already happens and is shared. - **Partial-row array insert** (`__partialRowErrors`): a failing row refuses the whole call rather than being reported per row. - **Version skew.** A plugin-security built from this change, run over an engine without the predicate-path call, refuses checked bulk updates (fail-closed). Both packages carry the changeset. - `.changeset/19950-rls-check-multi-row-writes.md` is declared a narrowing, per the seat's ruling and following objectstack-ai#19952: `minor` for `@objectstack/plugin-security` and `@objectstack/objectql`, a `!` headline, `Clause-②: no (narrowing)`, an ADR-0087 `not-required (no-migration-prescription)` disposition, and a **BREAKING** paragraph listing the newly refused writes and the remedy (declare `check` on the policy, or fix the data). - `origin/main` was merged twice with merge commits, both clean with no regeneration owed: at `3fd3a4f91b` (`a5ca1db166`) and at `9bfbacbf8b` (`938b2acfde`). PR objectstack-ai#19984 (objectstack-ai#19963, the explain wiring in the same file) had not landed by the second merge. - Patch round 2: citation fix only (four comment lines in `security-plugin.ts`); no behaviour change. --------- Co-authored-by: Claude <noreply@anthropic.com>
… the caller's read depth (objectstack-ai#20000) Fixes objectstack-ai#19986 Clause-②: no ## What changes `POST /api/v1/security/explain` with `{ object, operation: 'read', recordId }` now asks plugin-sharing's read filter with the same read depth the find path hands it. `decision.record.visible` for `read` equals whether the same caller's `find` returns the row. Enforcement admits and refuses exactly what it did before. Only the explain report changes. Landing: `packages/plugins/plugin-security/src/security-plugin.ts` (the `explainAccessForCaller` dependency wiring only), a new test file, the changeset, and one generated row in `scripts/engine-double-contract.pinned.json` (see "Surface note"). The step-3.6 post-image block and the seam declaration are untouched. The producer is the explain wiring, as the card says; no other package is involved. ## The two sides, quoted on `origin/main` `ccccdcc35e` before the change - **Explain** (`security-plugin.ts:4426`): `sharingReadFilter: (o: string, c: any) => sharing.buildReadFilter(o, c)`. The bare context, no `__readScope`. - **The find path, step 2.6** (`security-plugin.ts:2375`, `:2403`): for `find` / `findOne` / `count` / `aggregate` on a non-delegated context, `sc.__readScope` is set to `this.permissionEvaluator.getEffectiveScope('read', opCtx.object, permissionSets, { isPrivate: secMeta.isPrivate })`. `permissionSets` comes from `resolvePermissionSetsForContext` and `secMeta` from `getObjectSecurityMeta`. plugin-sharing's `buildReadFilter` then reads it: `org` gives no owner filter (null), and a unit depth gives the unit's owners. - **The resolver already exists.** `resolveSharingReadFilter` (`security-plugin.ts:4490` after this change; it is the analytics path's seam) computes the depth from the same two inputs and the same evaluator call, stamps it, and calls `buildReadFilter`. Its docblock names the situation explain is in: no middleware runs on this path, so the depth is computed there. The change reuses it. Nothing new is computed. ## Reproduced before the change The new test file was committed first on the unfixed tree (`07f447e0c0`, base `ccccdcc35e`). It wires the real `SecurityPlugin`, the real `SharingService` and sharing middleware, and the real platform `member_default` seed. The object has an unset OWD (private). The one variable is the principal's read depth, on the same three rows: | principal (read depth) | row | explain `record` | find | |:--|:--|:--|:--| | `hr_reviewer` (`org`) | shared to someone else | `visible: false`, `decidedBy: sharing` | returns it | | `hr_reviewer` (`org`) | owned by someone else | `visible: false`, `decidedBy: sharing` | returns it | | `hr_reviewer` (`org`) | unshared | `visible: false`, `decidedBy: sharing` | returns it | | `team_lead` (`unit`) | owned inside the unit | `visible: false`, `decidedBy: sharing` | returns it | | `dept_reporter` (`own`), the control | read-shared / owned / unshared | `true` / `true` / `false` | returns / returns / refuses: agrees | Result: `Tests 7 failed | 17 passed (24)`. The seventh red is a second direction on the old binding. A `__readScope: 'org'` already present on the explained context decided the report (`dept_reporter` × unshared: explain `true`, find refused), because the bare context was passed straight through. ## The change - **The non-delegated `sharingReadFilter` binding** calls `resolveSharingReadFilter(o, { ...c, __readScope: undefined })`. The caller's own key is cleared first, so the depth is always the computed one, the way step 2.6 overwrites it. Where resolution yields nothing (no sets, or a failed resolution), the helper stamps nothing and plugin-sharing reads `own`, the safe direction. - **On-behalf-of contexts keep `sharing.buildReadFilter(o, c)` unchanged**, as objectstack-ai#19984 did on the write side. The middleware's delegated read (the agent-leg depth, plus a second filter AND-ed in under the delegator's identity) is not modelled on explain's record path. Stamping the agent's own depth without that second filter could only widen the report. - `delegatedWrite` is renamed `actsOnBehalfOf`: it now guards read and write bindings, and `delegated` is already a name inside this function (the delegated-admin gate). ## Tests: `explain-read-verdict-inputs.test.ts` (24 cases) Every cell asserts that `record.visible` for `read` equals whether the find returns the row, and it asserts the find's own answer too. A cell cannot go green because both sides drifted. The find runs through both real middlewares, and the engine double evaluates the composed AST `where`. The double's matcher throws on an operator it does not know, so a refused cell cannot go green because the double silently answered `false`. Both sides read one hierarchy resolver: `unit` = the lead and the reporter. - Read matrix, private OWD: builtin admin × unshared; `hr_reviewer` (`org`) × shared / owned / unshared; `team_lead` (`unit`) × owned inside the unit / owned outside it / shared to someone else; `dept_reporter` (`own`) × read-shared / owned / unshared. Negative control: `dept_reporter` × unshared, refused by both, and explain names `sharing` (layer `excluded`). - Depth: the `org` reader's sharing layer is `admitted` with `rowFilter: null`. A `__readScope` the caller brings does not decide the report, in either direction: `dept_reporter` + `'org'` × unshared is refused by both, and `hr_reviewer` + `'own'` × unshared is admitted by both. - On-behalf-of: the context explain hands plugin-sharing's read filter carries no `__readScope`. - OWD control `public_read`: all nine principal × row cells admit on both sides. Unfixed tree: `Tests 7 failed | 17 passed (24)`. Fixed (`461187a0d2`) and at the final head: `Tests 24 passed (24)`. ## Ablations The fix was committed first (`461187a0d2`). Each mutation used `scripts/ablation-replace.mjs`: the anchor went from 1 hit to 0, the replacement from 0 to 1, and the blob changed on disk. After each restore the blob matched HEAD (`229605a70e3a`) and `git diff HEAD` was empty. The test imports `./security-plugin.js` from source, so there is no `dist/` hop and `ablation-dist-preflight` does not apply. Directions were predicted before each run. | ablation | mutation | result | |:--|:--|:--| | A | the old binding: `sharing.buildReadFilter(o, c)` for every context | `7 failed / 17 passed`: `hr_reviewer` × 3, `team_lead` × owned inside the unit, the `org` layer pin, and both caller-depth pins. The widening one reads `u_reporter × unshared × read: … expected true to be false`. The plain `visible: false` cells stay green, as predicted: the old binding is narrower and cannot widen. | | B | over-widening: every non-delegated context asked at `__readScope: 'org'` | `5 failed / 19 passed`: every `visible: false` pin (`team_lead` × unshared, `team_lead` × shared, `dept_reporter` × unshared, the negative control, the widening caller-depth pin), each `expected true to be false`. | | C | the on-behalf-of guard dropped | `1 failed / 23 passed`: the on-behalf-of pin (`no read depth stamped on a delegated context: expected true to be false`). | | D | the caller-key clear dropped: `resolveSharingReadFilter(o, c)` | `24 passed`, as predicted. The clear matters only where depth resolution yields nothing, and there explain's own CRUD layer already refuses, so `record.visible` cannot show it. It is declared, unpinned hygiene. | The first C attempt was a no-op. Its replacement text was a substring of its anchor, so `ablation-replace` saw the replacement count stay at 1, refused with exit 1, and ran no tests. The re-run with a distinct marker is the row above. ## Local verification: final head `8644de0cbe` - `pnpm --filter '@objectstack/plugin-security...' build`: exit 0. - Full plugin-security `vitest run`: `Test Files 124 passed`, `Tests 2365 passed`, at `6471f5564d`. Later commits add only the ledger row and a `main` merge whose source changes sit in `packages/mcp` and `packages/runtime`, outside plugin-security's build closure. The three explain suites re-ran at `8644de0cbe`: `Tests 138 passed (138)`. - `pnpm --filter @objectstack/plugin-security typecheck`: exit 0. The new test is in `tsconfig.test.json`'s program (`--listFiles`: 1 hit) and `check:test-typecheck` is OK. - `dispatch-gates --commands` at `8644de0cbe`, reconciled with `--ran`: 70 derived, 68 run with exit 0, 2 NOT MEASURED. `check:dual-build-cjs-loads` and `check:type-check-debt` exit 3 (PREREQUISITE NOT MET): they need every workspace package's `dist/`, which CI's Build Core owns. `check:i18n` was measured after building its declared closure: exit 0, 9 packages in sync. - `GITHUB_TOKEN=… node scripts/check-issue-citations.mjs`: exit 0, 1 citation, resolves. - Narrowed eslint (`--no-inline-config --format json`) over the two changed TS files: 2 files, 0 errors, 0 warnings. Both files resolve a non-empty config under `--print-config`. `eslint.config.mjs` enables no type-aware linting (no `parserOptions.project`, no `projectService`), so this diff cannot move the verdict of any untouched file. - Two `main` merges before opening (`ccccdcc35e` then `bfa23a8f49` then `615c0856ef`). Neither touched plugin-security, plugin-sharing, the ledger or the lockfile. objectstack-ai#19988 had not landed. ## Surface note `scripts/engine-double-contract.pinned.json` gains one row (+5 lines: the new test file's `findOne` double). It was written by `node scripts/check-engine-double-contract.mjs --write`, which that gate prescribes for a new pinned double ("0 added or grown, 0 lost"). It lies outside the claim's declared file surface, the same as the three rows objectstack-ai#19984 added. objectstack-ai#19988 does not touch this file. ## Acceptance notes - **Explain's record path does not model the on-behalf-of read, before or after this PR.** That is the agent-leg depth intersection plus the delegator's filter AND-ed in. It was not measured here. objectstack-ai#19984 recorded the same boundary for writes. - **Under a sharing-service fault, explain over-reports `read` visibility.** This predates the PR, which does not change it. `explain-engine.ts:958` catches a rejected `sharingReadFilter` into `null`, and the record matcher reads `null` as "no filter". A one-off probe on this branch made `sys_record_share` reads throw. `dept_reporter` × unshared then got explain `visible: true` (sharing `admitted`, `rowFilter: null`), while the same find threw. The old bare binding rejected into the same catch. This is reported for the seat to judge. It is not fixed here: the site is `explain-engine.ts`, outside this card's surface. - **The test double's nested reads** are not scoped by plugin-sharing's read filter. That boundary is recorded in `row-write-widener-composition.test.ts`, and no cell here depends on a nested read. --- _Generated by [Claude Code](https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
…s with enforcement throws (objectstack-ai#20030) Fixes objectstack-ai#20002 Clause-②: no The `security/explain` route promises that it runs 「the same code paths the enforcement middleware runs」. Enforcement does not catch a failure in those calls, so the request fails. The explain engine caught the same failure and turned it into a value that its record matcher reads as an answer. So a request that fails was reported as one that succeeds. This is the same defect class as objectstack-ai#19963 and objectstack-ai#19986 (PR objectstack-ai#19984, PR objectstack-ai#20000): explain and enforcement give two different answers. Enforcement is not touched. This PR changes the report only. ## Measurement first (on `origin/main` `fc6ddb87a4`, before any fix) The stack is the real `SecurityPlugin` + `SharingService` + both middlewares over one in-memory engine, in the shape of PR objectstack-ai#20000's test file. For each caught fall-back in `explain-engine.ts`, the dependency was made to fail, and the same request was run through both middlewares. Commit `02067686d4` adds the test file that pins the fix. On the unfixed engine its 10 fault cells are red and its 6 controls are green. The commit message carries this table. | Site | Caught into | What explain reported when the dependency failed | What enforcement did | Verdict | |---|---|---|---|---| | `:848` `fetchRecord` | `null` | Record not found: `visible: false`, no `decidedBy` | The find throws on the same store fault | Fails toward not visible. Left as is. The plugin's own binding already catches to `null`, so in this wiring this catch is never reached. | | `:858` `computeLayeredRlsFilter` | `{ layer0: null, layer1: null }` | `allowed: false` (the object-level catch denies), but `record.visible: true` for a shared row, an owned row and an org-depth row | The find throws the injected error | **Fails OPEN** at row level, and contradicts `allowed`. Changed. | | `:941` `listRecordShares` | `[]` | Verdict unchanged, because the read filter decides it. `rules[]` is empty | Unaffected: enforcement never calls `listShares` | Only the attribution changes: it can only drop an `admits` rule. Left as is. | | `:958` `sharingReadFilter` (the card) | `null` | Share store down, own-depth reader: `visible: true` on the unshared, shared and owned rows. Sharing is `admitted` with `rowFilter: null` | The find throws the store error | **Fails OPEN.** Changed. | | `:966` write gate (`canEdit` / `canDelete`) | `undefined` ("no gate wired") | Update on an owned row: `visible: true` (private and `public_read`). On a row with only a READ share, sharing reported `admitted` | The by-id PATCH throws the injected error | **Fails OPEN.** Changed. | | `:1128` `resolveSets` | `[]` | `allowed: false`, `decidedBy: object_crud` | 403 `PERMISSION_DENIED` | Fails toward deny. Left as is. | | `:1140` `resolveDelegatorContext` (not on the dispatch list, found by the same sweep) | `{ kind: 'none' }` ("no delegation") | Delegator's grant store down: `allowed: true`, `principal` `neutral`. The D10 intersection was dropped (a healthy delegator gives `allowed: false`) | 503 `SERVICE_UNAVAILABLE` | **Fails OPEN** (object-level `allowed`). Changed. | | `:1147` delegator `resolveSets` | `[]` | `allowed: false`, `decidedBy: object_crud` | The find throws | Fails toward deny. Left as is. | Two storage-level controls from the same probe: - With the share store down, an `org`-depth reader's read filter answers `null` before it reads a share, so both sides admit. - The per-record gate catches its own store faults (`writeGateFailClosed`), so an update during the same outage already got the same answer from both sides. ## What changed In `packages/plugins/plugin-security/src/explain-engine.ts`: - **The helper.** A `settle` helper keeps a failure apart from every value the call can return: it returns a `DEPENDENCY_FAULT` sentinel instead. It is used only at the four fail-open sites. - **Sharing read filter and write gate.** The sharing layer's record outcome is `not_evaluated`, with no `rowFilter` and no `matchesRecord`. Its `detail` says the layer could not be evaluated and that the request fails on the same call. `record.visible` is `false`, with `decidedBy: 'sharing'`. - The call that is checked is the one the operation's verdict depends on: the read filter for a read, and the per-record gate for a write. - It is checked before the OWD, because the sharing middleware calls it whatever the OWD is. - **Layered RLS composition.** The `tenant_isolation` and `rls` record attributions are `not_evaluated`, with a detail that names the failure. `record.visible` is `false`, with `decidedBy: 'rls'`. This matches the object-level `rls` layer, which already reports the same failure as a denial. - **Delegator resolution.** A new `delegatorUnresolved` flag fails closed like a missing delegator. `principal` and `object_crud` deny with their own wording, and `allowed` is `false`. - **Order in the record verdict.** Unchanged first: capability, CRUD, missing record, tenant exclusion, RLS exclusion. Then an RLS-composition failure, then a sharing failure. This follows the pipeline, where the RLS composition runs before the sharing middleware. **Outcome vocabulary.** `ExplainRecordAttributionSchema.outcome` in `packages/spec/src/security/explain.zod.ts` is `admitted | excluded | not_evaluated`. The engine already uses `not_evaluated` for "Tenant layer split is unavailable on this engine build" and for a record that is not found. So `not_evaluated` plus a detail that names the failure says "could not be evaluated". The response has no new value and no new key, and `packages/spec` is not edited. **Not changed:** - `security-plugin.ts`. PR objectstack-ai#20012 has since landed there, and this branch merges it. - `default-permission-sets.ts`. - `packages/spec`. - Enforcement. ## Tests The new file `packages/plugins/plugin-security/src/explain-dependency-fault.test.ts` uses the real `SecurityPlugin` + `SharingService` + both middlewares over one in-memory engine. Each fault cell asserts both sides: - **Explain:** `record.visible === false` with the named `decidedBy`, and the layer is `not_evaluated` with no `rowFilter` and no `matchesRecord`. - **Enforcement:** the same request through both real middlewares fails **with the injected fault itself**, compared by identity. For the delegator, the cell asserts the ADR-0112 envelope: `SERVICE_UNAVAILABLE` / 503. The fault cells: - Share store down × unshared / shared / owned row (read), and `buildReadFilter` failing on a `public_read` object. - `canEdit` failing × owned row (private and `public_read`). - `computeLayeredRlsFilter` failing × read-shared / owned / org-depth row. These cells also assert `allowed: false`. - Delegator's grant store down (`sys_user_position`): `allowed: false`, and `principal` and `object_crud` deny. The controls: - A healthy shared row: admitted, `decidedBy: sharing`. - A healthy unshared row: excluded. - An org-depth reader whose filter answers `null` while the share store is down: still admitted, because only a failure is a fault. - A healthy update gate, a healthy tenant wall, and a healthy delegator (`principal` `neutral`). **Ablation of every negative pin, on HEAD `e8e677786a`.** Each leg put one swallowing catch back with `scripts/ablation-replace.mjs`. The anchor went from 1 hit to 0, and the file's blob changed on disk. The test file then went red on exactly that site's cells. Each leg restored the file: blob `fca6431074ac` equals HEAD, and `git diff HEAD` is empty. | Leg | Catch put back | Red | Green | |---|---|---|---| | A1 | `:958` `.catch(() => null)` | 4, the read-filter cells (`record.visible`: `expected true to be false`) | 12 | | A2 | `:966` `.catch(() => undefined)` | 2, the write-gate cells | 14 | | A3 | `:858` `.catch(() => ({ layer0: null, layer1: null }))` | 3, the layered-RLS cells | 13 | | A4 | `:1140` `.catch(() => ({ kind: 'none' }))` | 1 (`allowed`: `expected true to be false`) | 15 | The test reaches the code under test through relative `src` imports (`./security-plugin.js` → `./explain-engine.js`), so no `dist` sits between the mutation and the run. **Runs on HEAD `e8e677786a`**, after merging `origin/main` at `44639665ee`: - `@objectstack/plugin-security`: `typecheck` exits 0, and `tsconfig.test.json` lists the new file. The full suite passes: **130 files, 2541 tests**. - Consumers of the explain route, found with `grep -rln "security/explain\|explainAccess" packages --include=*.test.ts`: - rest: `security-routes`, `security-explain-envelope`, `rest-write-response-internal-fields.tripwire`: 45/45. - plugin-sharing: `sharing-service`: 131/131. - client: `client.test`: 217/217. - dogfood: `api-key-owner-revoke`, `owd-public-read-write-write-floor`, `showcase-d7-default-profile`: 23/23. - spec: `type-alias-convention.pin`, `explain-zero-rows-sentinels.pin`: 11/11. - objectql: `engine-middleware-operation-vocabulary`: 5/5. - **Gates.** `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` on this HEAD derives 71 commands, and all 71 ran. - `check-changeset-fixed`, `check:error-code-casing` and `check:filter-alias-parity` also ran, because their rosters sit under paths this diff touches. - Reconciliation, with every line recorded with its exit code: `Run reconciliation — 71 derived, 71 run, 0 NOT-MEASURED, 0 UNRUN`. - `check:type-check-debt` first answered PREREQUISITE NOT MET, because the plugin-security `dist` was older than its sources. After a rebuild it answered `OK — 4 ledger entr(ies) re-measured … none above its recorded number`. - **Lint, narrowed to the changed files.** `eslint --no-inline-config --format json` on the two changed TS files: 2 files, 0 errors, 0 warnings. - The changeset and the JSON ledger are outside eslint's configured file set ("File ignored because no matching configuration was supplied"). - The config never enables type-aware linting (no `parserOptions.project`), so this diff cannot change the lint result of any other file. ## Also in the diff - `scripts/engine-double-contract.pinned.json`: one row added (`explain-dependency-fault.test.ts`, `findOne`), regenerated with `--write`. `check:engine-double-contract` (RETAINED) needs the coverage ledger to record every new pinned double. No row was lost. - `.changeset/20002-explain-sharing-fault.md`: `@objectstack/plugin-security: patch`, `Clause-②: no`. The report now matches what enforcement already does, and no accept set moves. ## Acceptance notes - **No log line is added at the four sites.** The suggested route said to log the failure the way enforcement does, but I did not, for two reasons: - The explain engine has no logger dependency, and wiring one would mean editing `security-plugin.ts`, which is out of scope here. - AGENTS.md "Degradation log levels" says that a failure delivered to the caller is not a degradation. The report delivers it: `not_evaluated`, a detail, and a fail-closed verdict. The real request's failure shows up on the request that fails. - **The detail names the failure, not the thrown message.** Explaining your own access needs no capability, so a raw store error message would reach an ordinary caller. - **`:848` and `:941` word a failure imprecisely.** They still say "Record not found" or "0 share(s) attached". Their verdicts fail toward not visible, so they are left as is. - **The `tenant_isolation` layer-level `verdict` stays `not_applicable` under a layered-RLS failure.** That code is unchanged, and the verdict enum has no "unknown". The record attribution carries the failure. - **A spec describe text does not mention failures.** `ExplainRecordAttributionSchema.outcome`'s describe text reads "not_evaluated (skipped/not row-scoped)". The engine already uses the value for "unavailable", so this is a docs detail in `packages/spec`, and it is not edited here. --- _Generated by [Claude Code](https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Fixes #19963
Clause-②: no
What changes
POST /api/v1/security/explainwith{ object, operation: 'update' | 'delete', recordId }now builds its record verdict from the by-id write path's own inputs.decision.record.visibleequals what the by-idPATCH/DELETEdoes for the same caller. Enforcement admits and refuses exactly what it did before. Only the explain report changes.Landing:
packages/plugins/plugin-security/src/security-plugin.ts(theexplainAccessForCallerwiring, plus one helper extracted from the step-2.7 gate), a new test file, the changeset, and three generated rows inscripts/engine-double-contract.pinned.json(see "Surface note").The two mechanisms, reproduced on
origin/main2c1011b01bbefore the changeThe old bindings, quoted from
2c1011b01b:computeLayeredRlsFilter: (sets, o, engineOp, c) => this.computeLayeredRlsFilter(sets as any, o, engineOp, c): no options, so the platform floor stays (M1).canEditRecord: (o: string, rid: string, c: any) => sharing.canEdit(o, rid, c): bare context, no__writeScope(M2).canDeleteRecordhad the same shape.One-variable causal legs. Each ran on the unfixed tree through the real
SecurityPlugin, the realSharingServiceand sharing middleware, and the real platformmember_default. The object has an unset OWD (private).recorddept_reporter× edit-shared rowcreated_by= adminvisible: false,decidedBy: rlscreated_by=dept_reportervisible: truehr_reviewer(write depthorg, noorg_member, so the floor is out of play) × unshared rowvisible: false,decidedBy: sharing__writeScope: 'org'stamped on the contextvisible: trueDelete was unmeasured on the card, and it has the same shape. On
2c1011b01b,hr_reviewer× unshared anddept_reporter× owned (owner, not creator) both readvisible: false(rls), while the by-id DELETE admitted. This PR covers delete too.The change
resolvePreImageFloorDrop(rlsOperation, object, recordId, context, sets, delegated)is step 2.7's floor decision, extracted clause for clause. The floor must be in play, the sharing tri-state verdict must beallow, and the on-behalf-of path is excluded. The gate calls it with the arguments it used before. The whole plugin-security suite stays green (2341 tests), including the Row-level write gate consults neithermodifyAllRecordsnorsys_record_share.access_level— both declared write-widening mechanisms are inert #5492 / Acontrolled_by_parentdetail's own ownership floor is never droppable, so a cross-creator by-id UPDATE of a child is refused before the master gate — Modify All Data included #8757 / ADR-0055 composition suites that drive this gate.computeLayeredRlsFilterbinding passes{ dropPlatformOwnershipFloor }from that helper forupdate/delete. It does not passmasterGateCoversThisWrite: that option vouches that ADR-0055's master gate runs after the filter, and explain runs no master gate. An unanswerable decision keeps the floor standing.canEditRecord/canDeleteRecordbindings stamp__writeScopefromresolveWriteScopeForSharing. The stamp always overwrites the context's value, the way step 2.6 overwrites it.Tests:
explain-write-verdict-inputs.test.ts(25 cases)Every cell asserts that
record.visibleequals the write's outcome. An admitted write must really change the row. A refused write is checked by its ADR-0112code+status:PERMISSION_DENIED/403 for the pre-image gate,FORBIDDEN/403 for the sharing gate.hr_reviewer× shared / owned / unshared;dept_reporter× edit-shared, × owned-not-created. Negative control:dept_reporter× unshared, where both deny.hr_reviewer× unshared anddept_reporter× owned admit.dept_reporter× edit-shared (ADR-0111 D3) and × unshared are refused.created_bymoved only, and the verdict must not move.orgwriter with no floor is admitted. A__writeScopethat the caller brings does not decide the verdict (negative pin).public_read_write: all nine principal × row cells admit on both sides.Unfixed tree
2c1011b01b:Tests 10 failed | 15 passed (25). Fixed:Tests 25 passed (25).Ablations
The fix was committed first (
81dba2597). Each mutation usedscripts/ablation-replace.mjs: the anchor went from 1 hit to 0, and the blob changed on disk. After each restore the blob matched HEAD (be403e07a5ae) andgit diff HEADwas empty. A driver trap also re-verified both.computeLayeredRlsFilterbinding, no options8 failed / 17 passed:hr_reviewer× 3 update,dept_reportershared and owned update, 2 delete cells, the M1 created-by-admin leg. The M2 pins stay green.canEditRecordbinding, bare context5 failed:hr_reviewer× 3 update (decidedBy: sharing), the M2org-writer pin, and the forged-stamp negative pin (explaintrue, PATCH refused).canDeleteRecordbinding1 failed:hr_reviewer× unshared delete.__writeScope: 'org'stamped and the floor always dropped5 failed: the update negative control (both cases), delete × edit-shared, delete × unshared, and the forged stamp.The first A4 attempt was a no-op on its second mutation. The driver read that anchor file after a
cd, andablation-replacerefused the empty anchor with exit 2. The re-run with absolute paths is the row above. The test imports./security-plugin.jsfrom source, so there is nodist/hop andablation-dist-preflightdoes not apply.Local verification: final head
aa9013ffbpnpm --filter @objectstack/plugin-security buildexit 0.typecheckexit 0: the new test is intsconfig.test.json's program (--listFiles: 1 hit).vitest run:Test Files 123 passed,Tests 2341 passed. This ran at81dba2597;aa9013ffbadds only the ledger rows.dispatch-gates --commandsunion ataa9013ffb, reconciled with--ran: 70 derived, 69 ran with exit 0, 1 NOT MEASURED.check:dual-build-cjs-loadsexits 3 (PREREQUISITE NOT MET): it needs every workspace package'sdist/, and CI's Build Core owns that. Four more roster gates keep their roster under a directory this diff touches:check-changeset-fixed,check:authz-resolver,check:error-code-casing,check:filter-alias-parity. All four exit 0.--no-inline-config --format json) over the two changed TS files: 2 files, 0 errors, 0 warnings. Both files are in the population ofeslint.config.mjs's**/*.{ts,…}block. That config enables no type-aware linting (noparserOptions.project), so this diff cannot move the verdict of any untouched file.origin/mainwas still2c1011b01bwhen this PR opened, so the pre-PR merge was a no-op.Surface note
scripts/engine-double-contract.pinned.json(+15 lines: the new test file'sdelete/findOne/updatedoubles) was written bynode scripts/check-engine-double-contract.mjs --write, which is what that gate prescribes for new pinned doubles ("0 seams added or lost"). It lies outside the claim's declared file surface. The step-3.6 post-image block is untouched.Acceptance notes
sharingReadFilterassharing.buildReadFilter(o, c)with no__readScope. On2c1011b01b,explain(read)forhr_reviewer(read depthorg) answeredvisible: false,decidedBy: sharing, on all three rows. The samefindthrough the real security and sharing middleware returned each row, because enforcement stamps__readScope: 'org'and the sharing read filter is then null. Thedept_reporter(own) control agrees on both sides. This is reported for the seat to file.controlled_by_parentdetails;probeAuthoredRowWritedeferral;transfer/restore/purge, whose Layer 1 is computed with the raw verb rather than the gate'srlsOperationmapping.row-write-widener-composition.test.tsrecords. In every admitted cell, the caller can read the row on the real stack too: throughorgread depth, an edit share, or ownership.check-only row-level policy does not gate a bulk update (update(…, { where, multi: true })): the post-image check is skipped as "governed by the using-scoped where", and nousingexists to scope it #19950 and [finding] security: a row-levelcheck(declared, or defaulted fromusing) is never evaluated on an ARRAY insert —engine.insert(object, [rows]), whichcreateManyDatacalls, stores rows a single-record insert refuses #19964 edits the step-3.6 block of the same file. It had not landed when this PR opened. Whichever of the two lands second merges main and re-runs.Generated by Claude Code