Repository navigation
Commit decc807
fix(plugin-security): explain's update verdict on a controlled_by_parent record comes from the master-detail write check (#22529)
Fixes #22514
Clause-②: yes (widening)
`POST /api/v1/security/explain` now answers an `update` of a
`controlled_by_parent` record the way that record's own by-id `PATCH` is
answered. The record verdict comes from the master-detail write check
(ADR-0055) that the write path runs at step 2.8. explain reads it
through `ISecurityService.checkControlledByParentWrite`, served since PR
#22513, and keeps no second copy of the check or of its
`controlled_by_parent` predicate.
## Reproduction (before)
Measured on `origin/main` `ee8751d41`, with an untracked scratch dogfood
probe on PR #22513's fixture (`cbp-parent-gates-fixture.ts`, org-bound
boot, so the platform ownership floor `owner_only_writes` binds
`org_member`). The probe was deleted after the reading. `cpg_contract`
is `controlled_by_parent` under `cpg_account` (`public_read`, edited by
its owner).
| principal | request | explain `allowed` | explain `record` | the door
|
|:--|:--|:--|:--|:--|
| member, cannot edit the master | update, child of the admin's master |
`true` | `visible: false`, `decidedBy: 'rls'` | `PATCH` 403
`PERMISSION_DENIED` (master, row-level security) |
| admin explaining that member (`userId`) | same | `true` | `visible:
false`, `decidedBy: 'rls'` | (the member's `PATCH`: 403) |
| member, edits the master, did not create the child | update, child of
the member's own master | `true` | `visible: false`, `decidedBy: 'rls'`
| `PATCH` **200** |
| deleter, edits the master, did not create the child | delete, child of
their own master | `true` | `visible: false`, `decidedBy: 'rls'` |
`DELETE` **200** |
| member | update, `cpg_board` (`public_read_write`) | `true` |
`visible: true` | `PATCH` 200 |
**What the reading says about the card.** The card's mechanism holds:
explain never asks the master-detail write check, and its sharing gate
(`canEdit`) abstains on `controlled_by_parent` and reads as writable.
Its symptom needs restating. The card quotes `allowed: true`, which is
the object-level field (may this principal update `cpg_contract` at
all). By the published contract and the parity table in
`explain-enforce-parity.test.ts`, a record question is answered by
`record.visible`. On `main` that verdict was decided by the ownership
floor, which the write path hands over to the master check through step
2.7's `masterGateCoversThisWrite` vouch. explain did not vouch, because
it ran no master gate (the comment in the wiring said so). The result
was a two-sided divergence:
- the master's non-editor was refused on the wrong layer (`rls`), with
no word about the master;
- the master's editor was refused beside a `PATCH` that answers 200.
## What changed
- `explain-engine.ts`: a new optional dependency,
`checkControlledByParentWrite`. For every `update` of a record that
exists, the engine asks it with the context being EXPLAINED. Asking for
every update keeps the predicate in the member: `not_applicable` changes
nothing.
- `security-plugin.ts` (the `explain` wiring only):
- It hands the engine the served member,
`this.checkControlledByParentWrite`. That is the write path's own
composition: the context prologue, step 2.8 for the principal and then
for the delegator.
- The update's Layer 1 now carries step 2.7's coverage vouch,
`masterGateCoversThisWrite`, on step 2.7's condition (not on behalf of
anyone). The ownership floor is handed over to the master check here
exactly as the write path hands it.
- `delete` keeps its floor, because explain asks no master check for it.
- The knob's docblock lists this new caller.
- `.changeset/22514-explain-cbp-update-master-check.md`: `minor` for
`@objectstack/plugin-security`, not the `patch` the dispatch suggested.
See the acceptance notes.
## Outcome to layer mapping
This mirrors the gates PR #22513 built and the review `6085973866`'s
truth table. The door proceeds on `allow` and `not_applicable`, and
refuses on every other outcome.
| member outcome | `record` verdict | `sharing` layer `record` | rest of
the report |
|:--|:--|:--|:--|
| `allow` | the existing path's | unchanged | byte-identical to a deps
bag without the member |
| `not_applicable` | the existing path's | unchanged | byte-identical |
| `deny` + `leg` | `visible: false`, `decidedBy: 'sharing'` |
`excluded`; the detail names the leg (`object_permission` /
`row_level_security` / `record_sharing` / `master_chain`) and the 403 |
unchanged |
| `unresolvable` + `reason` | `visible: false`, `decidedBy: 'sharing'` |
`not_evaluated`; the detail names the reason and the status the update
answers (422 / 404 / 422) | unchanged |
| rejection (refused context, store fault) | `visible: false`,
`decidedBy: 'sharing'` | `not_evaluated`, no predicate; fail-closed
fault detail | unchanged |
| an outcome outside the vocabulary | `visible: false`, `decidedBy:
'sharing'` | `not_evaluated` | unchanged |
| member absent (a deps bag without it) | the existing path's |
unchanged; claims nothing about a master | unchanged |
- **Where the master is named.** The spec's layer vocabulary is closed,
with no master layer. The refusing leg is named on the `sharing` layer,
the record's write gate. `describeOwd`'s baseline wording stays, per
triage.
- **Precedence.** The record's own row-level security (`rls`) still
decides first, because step 2.7 runs above step 2.8. The not-found
answer for a record the principal cannot read (the read question) still
overrides both.
- **No master named on `allow`.** The member answers `allow` for a
system context on ANY object, which is the write path's first exit. So
`allow` is not evidence that the record's access derives from a master.
- **Absent member.** Feature detection on the engine's optional
dependency keeps today's answer, as in the gates (PM assumption 4
holds).
- **The context.** It is the TARGET's, not the caller's, because
`explainAccessForCaller` already hands the engine the explained user's
context (PM assumption 2 holds, and it is pinned below).
## Pins
| pin | where | proves |
|:--|:--|:--|
| parity, refused: the member cannot edit the master. `PATCH` 403 naming
the master's row-level security; explain `visible: false`, `decidedBy:
'sharing'`, leg `row_level_security` named |
`packages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts`
(REST explain beside the REST `PATCH`) | the card's pin, end to end |
| target principal: the admin explains the member by `userId` and gets
the member's refusal; the admin's own explain and `PATCH` admit | same
dogfood file | the member is asked with the explained context |
| control, the master's editor: explain `visible: true` beside `PATCH`
200, for a child they did not create, with the floor armed
(`assertArmed`, `org_member`) | same dogfood file | the floor is handed
over as the door hands it |
| control, non-`controlled_by_parent`: `cpg_board` explain `visible:
true` beside `PATCH` 200 | same dogfood file | unchanged |
| every leg (`record_sharing`, `row_level_security`,
`object_permission`, `master_chain`) refused beside a refused `PATCH`;
four admitted updates explained writable beside an admitted `PATCH`; a
system caller explaining the viewer by `userId` gets the viewer's
`object_permission` refusal |
`controlled-by-parent-write-member.test.ts` (the registered service, the
member's own double) | the wiring, through `security.explain` |
| each outcome's mapping, as in the table above. Also: `allow` and
`not_applicable` are byte-identical (`toEqual`) to the report without
the member, on `controlled_by_parent`, `public_read_write`, and private
objects with the gate admitting and refusing. The record's own RLS
decides first. The member is asked once, with the explained context
object itself, and never for read, create, delete, transfer, export, an
object-level request or a missing record |
`explain-controlled-by-parent-write.test.ts` (engine, deps bag, no
engine double) | the mapping |
## Ablations
Each leg went through `scripts/ablation-replace.mjs` (wrap mode, with an
anchor hit, and a restore proven by the blob equal to HEAD and an empty
`git diff HEAD`). Then `pnpm --filter @objectstack/plugin-security
build`, then `scripts/ablation-dist-preflight.mjs` (marker present in
`dist/`), then the run. The restore leg rebuilt and ran the preflight
with `--absent` (marker absent from all 6 built files, tree clean
against HEAD).
| ablation | mutation | predicted | observed |
|:--|:--|:--|:--|
| A: the `canEdit`-only verdict back | the explain wiring's
`checkControlledByParentWrite` key renamed, so the engine sees no member
| the refusal pins red, the controls green | dogfood: 2 red (`visible:
true`, `decidedBy: 'object_crud'` beside the 403), 2 green. member
suite: 2 red (the same shape for `c_other`), 33 green |
| B: no floor hand-over | `masterGateCoversThisWrite` forced false |
dogfood refusals red on `decidedBy`, the editor control red, the plugin
suites green (no floor in their harness) | dogfood: 3 red (`decidedBy:
'rls'` on both refusals; the editor control `visible: false` beside
`PATCH` 200), the board green. plugin suites: 35/35 green |
Ablation B's first attempt is void: its marker was a comment, which the
build strips, so the dist preflight could not prove the mutation landed
(exit 1). It was re-run with a string-literal marker; the table reports
the second run.
## Local verification (HEAD `16a937c47`)
- `pnpm --filter @objectstack/plugin-security typecheck`: exit 0. The
test-layer program (`tsconfig.test.json`) lists both edited test files,
counted with `--listFiles`.
- `pnpm --filter @objectstack/plugin-security exec vitest run`: 192
files, 3994 passed, 45 skipped, exit 0.
- `pnpm --filter @objectstack/dogfood typecheck`: exit 0.
- Dogfood: `cbp-explain-master-write`,
`cbp-parent-attachment-comment-gates`, `controlled-by-parent` and
`showcase-invoice-cbp`: 4 files, 22 tests passed. The full dogfood suite
(three CI shards) is declared to CI.
- `node scripts/pm/dispatch-gates.mjs --commands`: 71 commands derived
and 71 run, each exit code recorded. The `--ran` reconciliation reads
"71 run, 0 NOT-MEASURED (a DERIVED zero)". `pnpm
check:dual-build-cjs-loads` first answered `PREREQUISITE NOT MET` (exit
3: eight unrelated packages had no `dist/`). After building those eight
it answered exit 0, which is the reading recorded.
- Lint, narrowed. CI's `pnpm lint` owns the full run.
- Population: `eslint.config.mjs` lints `**/*.{ts,…}`, so the five
changed `.ts` files are the touched population.
- Count: `--format json` read 5 files, 0 errors, 0 warnings.
- Invariance: the config enables no type-aware linting (no
`parserOptions.project`, stated in the config itself) and loads no
import-graph plugin, so this diff cannot move any untouched file's
verdict.
## Acceptance notes
- **Changeset level `minor`, not the dispatch's `patch`.**
`ExplainEngineDeps` is exported from `@objectstack/plugin-security`'s
index, and it gains an optional key. Under the WHICH LEVEL rule in
`pr-automation.yml`'s Check Changeset step, an additive widening of a
published package's public surface takes at least `minor`. The
`Clause-②` line is `yes (widening)` (the seat amended the claim and line
2 at review): the optional deps key is a widening of a published type.
`explain`'s response payload gains no key, and no runtime accept set
widens. The same reading is the one the review on PR #22513 gave its
additive types.
- **`allowed` is unchanged.** It answers the object question and stays
`true` for a member who holds `update` on the object. The record's
bottom line is `record.visible`, as for every other record-level refusal
(a private record the caller does not own reads the same way).
- **`delete` keeps today's answer.** The member is declared for an
UPDATE, so explain asks no master check for a delete and keeps the floor
there. The measured gap is in the out-of-lane findings below.
- **A context the write path refuses before any gate.** Examples: a
principal-less context, permission sets that cannot be resolved, a
dangling delegator. The member rejects for these on any object, and
explain now reports the sharing layer `not_evaluated`, with the fault
detail, on an update of an existing record. That is refuse-only and
matches the door, which refuses these contexts first. explain's earlier
layers (`object_crud`, the D10 delegator handling) already decide those
records, so `decidedBy` does not move. The review on PR #22513 named the
same shape for the gates.
- **On-behalf-of.** The update's Layer 1 keeps the floor for a delegated
context, as step 2.7 does. The member still runs both legs (principal,
then delegator), as step 2.8 does.
- **Cost.** One more permission-set resolution per update explain. It is
a memo hit: the member is handed the same context object the engine
resolved.
- **Product effect.** Consoles that gate an Edit affordance on
`record.visible` for an update now show it to a master editor who did
not create the child, and hide it from a non-editor for the right
reason.
- **`transfer` was not measured.** explain's `transfer` record verdict
asks the sharing gate, and the door runs step 2.8 for `transfer` too.
This is a read-only inference, noted here and not filed.
## Out-of-lane findings (for the seat to file)
1. **class (a).** `explain` `delete` on a `controlled_by_parent` record
asks no master-detail check and keeps the `owner_only_deletes` floor,
which the door hands over to that check.
- **reach** (public door, measured with the scratch probe at
`ee8751d41`, unchanged at this head): a principal who edits the master
and holds delete on the child, but did not create it, gets explain
`delete` `record.visible: false`, `decidedBy: 'rls'` beside `DELETE
/api/v1/data/cpg_contract/ID` 200. For the non-editor the refusal is
reported on `rls` with no word about the master, beside a `DELETE` 403
that names the master's row-level security.
- **Seam:** `spec:ISecurityService.checkControlledByParentWrite`
(declared for an update only) → `runtime:explain-engine.ts
applyRecordAttribution` (the delete branch asks `canDeleteRecord`
alone).
- **Route:** the member's contract would have to name `delete`, or a
sibling would have to be declared (spec lane) before explain can ask it.
The legs themselves are operation-independent in
`assertControlledByParentWrite`.
- **Dedupe words:** `explain delete controlled_by_parent master check`,
`explain canDeleteRecord owner_only_deletes floor`,
`checkControlledByParentWrite delete operation`.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent b3af956 commit decc807
6 files changed
Lines changed: 576 additions & 9 deletions
File tree
- .changeset
- packages
- plugins/plugin-security/src
- qa/dogfood/test
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
248 | 248 | | |
249 | 249 | | |
250 | 250 | | |
251 | | - | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
252 | 259 | | |
253 | 260 | | |
254 | 261 | | |
| |||
375 | 382 | | |
376 | 383 | | |
377 | 384 | | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
0 commit comments