Repository navigation
Commit a18c514
test(plugin-security): pin explain controlled_by_parent update/delete verdict under a Modify All Data master beside the door (#22760)
Fixes #22721
Clause-②: no
## What this is
Stage 1 of #22721: the measurement the triage direction (comment
6101562558) asks for, plus one test file that pins the one route the
measurement found unpinned. No source file changes.
**The card's defect does not reproduce on `main`.** On a booted stack at
`origin/main` `d7b26df5`, which contains #22529 (`d303b3e7af`) and
#22549 (`86bf9ed7d5`), `git merge-base --is-ancestor` exit 0 for both,
the record-grained `security/explain` answers `record.visible: true` for
both `update` and `delete` of a `controlled_by_parent` child whose
master another user owns and the caller may edit. The data door answers
200 for the same caller and row, through each of the three routes
measured. A caller who may not edit the master is refused by both. The
console header (objectui `origin/main` `5330afd`,
`packages/plugin-detail/src/useRecordEditable.ts`) posts exactly
`{object, operation, recordId}` for `update` and `delete` and reads
`decision.record.visible`, so on `main` it offers Edit and Delete to the
HotCRM sales manager. The card was measured on 17.7.0, which predates
both corrections; hosted tenants receive them with cloud's v18 pin.
## The measurement, through the real HTTP doors
Booted with `@objectstack/verify` `bootStack`: better-auth sign-ups, the
REST explain route, the generic `/data` door, plugin-sharing and
plugin-security. The boot is `orgContext: true`, so the ownership floor
(`owner_only_writes` / `owner_only_deletes`, positions `org_member`)
binds every caller. This was a scratch probe, untracked and deleted
after the reading.
- **Master** `hmx_account`: `sharingModel: 'private'`, owned and created
by another member (`rep`).
- **Details** `hmx_contract`, `hmx_quote`, `hmx_note`: all
`controlled_by_parent` under it. Every child was inserted as the system,
so no caller created any of them.
- `hmx_contract` has HotCRM's `crm_contract` grant shape: edit, no
delete, no write bypass.
- `hmx_quote` has HotCRM's `crm_quote` grant shape: CRUD, with
`modifyAllRecords` on the detail too.
- `hmx_note` has plain CRUD and no bypass, so a delete reaches the
master check alone.
- **Grants:** each caller's set is distributed through a position's
`permissionSets` (the `main` spelling since `bfc15d27`). Each caller's
resolved positions and sets were asserted before anything was measured.
| route | caller | how the caller may edit the master | HotCRM analogue
(hotcrm `1d7148bf`) |
|---|---|---|---|
| A | `mgr` | `viewAllRecords` + `modifyAllRecords` on the master | the
`sales_manager` profile on `crm_account`
(`src/sales/profiles/sales-manager.profile.ts`) |
| B | `team` | a criteria sharing rule on the master, `accessLevel:
'edit'`, recipient a position the caller holds | `account_team_sharing`
(`src/sales/sharing/account.sharing.ts`) |
| C | `unitmgr` | a business-unit subtree sharing rule
(`unit_and_subordinates`, edit); the caller is in the parent unit of the
owner's unit | none. This is the open edition's "manager over the
owner"; hierarchy write depth needs `hierarchy-security` (enterprise),
and `defineStack` refuses it here |
| X | `control` | object-level edit on master and details, and no route
to the master row | none |
The master's own `PATCH` answered 200 for A, B and C, and 404 for X.
Each cell below shows explain's `record.visible` (with `decidedBy` where
it is a refusal), then the door's status. Explain was `POST
/api/v1/security/explain {object, operation, recordId}`, 200 on every
cell. The door was `PATCH` or `DELETE /api/v1/data/OBJECT/ID`.
| caller, master | contract update | contract delete | quote update |
quote delete | note update | note delete |
|---|---|---|---|---|---|---|
| A, a master rule B matches | true · 200 | false, object_crud · 403 |
true · 200 | true · 200 | true · 200 | true · 200 |
| A, a master rule B does not match | true · 200 | false, object_crud ·
403 | true · 200 | true · 200 | true · 200 | true · 200 |
| B, a master the rule matches | true · 200 | false, object_crud · 403 |
true · 200 | true · 200 | true · 200 | true · 200 |
| B, a master the rule does not match (control) | false, no decider ·
404 | false, object_crud · 403 | false, no decider · 404 | false, no
decider · 404 | false, no decider · 404 | false, no decider · 404 |
| C | true · 200 | false, object_crud · 403 | true · 200 | true · 200 |
true · 200 | true · 200 |
| X (control) | false, no decider · 404 | false, object_crud · 403 |
false, no decider · 404 | false, no decider · 404 | false, no decider ·
404 | false, no decider · 404 |
Explain's verdict equals the door's on all 36 cells.
- **Contract delete** is refused by both faces at the object gate, as
HotCRM intends (`allowDelete: false` on `crm_contract`).
- **The two controls** cannot read the master, so the child is
unreadable. The door answers 404 (Ruling A on the write doors), and
explain answers the matching missing-record shape: `visible: false` with
no decider.
## What was already pinned, and why one new file
**Already pinned, explain beside the door, on `controlled_by_parent`:**
- `controlled-by-parent-write-member.test.ts` covers update, delete and
transfer for four masters: an owned master, a master shared at `edit` to
the caller (`c_shared`), an RLS-admitted master and a two-hop chain.
- Its REST twin,
`packages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts`,
covers the owned master.
- Route B reduces to the `c_shared` shape at evaluation. The probe read
the rule's grants as three `sys_record_share` rows with `recipient_type:
'user'` and `access_level: 'edit'`.
**Not pinned anywhere for explain:** route A, `modifyAllRecords` on the
master alone, which is HotCRM's actual route.
`controlled-by-parent-detail-write-authority.test.ts` pins the door with
the bypass on both objects and never asks explain. The master check
reaches the bypass through two seams of its own:
- the master's ownership floor comes off on the sharing write verdict
`allow`;
- the record-sharing leg is the bypass branch of `canEdit`.
**New:**
`packages/plugins/plugin-security/src/explain-controlled-by-parent-master-modify-all.test.ts`.
The rig is the real `SecurityPlugin`, `ObjectQL` over better-sqlite3,
the real `SharingService` and its middleware, and the platform's
`member_default` floor armed (`org_member`) for both callers.
- **Armed:** every row was created by another user. For each caller,
explain's `rls` record on a row neither caller created names the floor's
predicate, `{ created_by: CALLER }`.
- **Update and delete by the bypass holder:** explain answers `visible:
true`, and the door admits.
- **Control** (`viewAllRecords` on the master, without
`modifyAllRecords`): the door answers `PERMISSION_DENIED` / 403 with
"requires edit access to its master record". Explain answers `{ visible:
false, decidedBy: 'sharing' }`, and its sharing detail names the master
check's leg.
- **Reference:** the master's own door admits the holder and refuses the
viewer.
## Ablation of the negative pin
The mutation went through `scripts/ablation-replace.mjs` in WRAP mode,
in `packages/plugins/plugin-security/src/explain-engine.ts`. It changes
`const masterOrFault = MASTER_CHECKED_BY_ID_WRITES.has(engineOp) &&
recordExists && deps.checkControlledByParentWrite` to `const
masterOrFault = false && MASTER_CHECKED_BY_ID_WRITES.has(engineOp) &&
...`, so explain stops asking the master check. That is the pre-#22529
shape.
- **Landed:** anchor count 1 to 0, blob `1d8f2ff1` to `ee54fcff`.
- **Run:** 2 failed, 4 passed. The failures are exactly the two control
cells. Their door assertion (403) held, and explain answered:
```text
- "decidedBy": "sharing",
+ "decidedBy": "object_crud",
"recordId": "c_viewer_update",
- "visible": false,
+ "visible": true,
```
(`delete` reads the same.)
- **Restored:** the blob after restore is `1d8f2ff1`, equal to HEAD;
`git diff HEAD` is empty and `git status --porcelain` is clean. The
mutation was never staged or committed.
- **Run twice:** at `7dc3c671` and again at `da157165` (after the merge
below), with the same reading both times. `explain-engine.ts` and the
test are byte-identical across the merge.
## Tests and gates, at HEAD `da157165`
The branch merges `origin/main` `149294c02`. Two commits there touch
`plugin-security` and `plugin-sharing` (#22671, #22745); the delta
against `main` is still exactly the one new file (+275). After `pnpm
install --frozen-lockfile`, the `@objectstack/plugin-security...`
closure was rebuilt (18 tasks).
- **The new file:** 6 passed.
- **The full `@objectstack/plugin-security` suite (`vitest run
--maxWorkers=2`):** 200 files passed, 4145 tests passed and 45 skipped.
The lock reported VERDICT command-exit 0. Before the merge, at
`7dc3c671`, it read 199 files, 4132 passed and 45 skipped.
- **`pnpm --filter @objectstack/plugin-security typecheck`:** exit 0.
The test-layer program (`tsconfig.test.json`, `--listFilesOnly`) lists
the new file, with 0 errors and 0 debt.
- **Derived gates** (`node scripts/pm/dispatch-gates.mjs --commands
--repo objectstack-ai/objectstack`): 57 derived, all exit 0. Each exit
code was recorded before any pipe. The `--ran` reconciliation reads: 57
derived, 57 run, 0 NOT-MEASURED (derived from the recorded codes), 0
UNRUN.
- In the first battery, at `7dc3c671`, three gates exited 3,
PREREQUISITE NOT MET.
- `check-plugin-teardown-shape.mjs --self-test` lacked its pinned
fixture commit on this shallow clone. It exits 0 after `git fetch
--depth=1` of `621a4876`.
- `check:dual-build-cjs-loads` and `check:i18n` lacked other packages'
`dist/`, which that battery's `check:type-check-debt` then built. Both
were rerun at `da157165` on post-merge builds.
`check:dual-build-cjs-loads` reports 107 require entry points across 66
packages load. `check:i18n` reports 9 packages, all bundles in sync.
- **Lint, narrowed to the one touched file:** `eslint --no-inline-config
--format json` reads 1 file, 0 errors and 0 warnings. eslint's own
resolved config for the file (`--print-config`) is a linted config, and
it carries no `parserOptions.project` and no `projectService`. With no
type-aware linting, a new test file cannot move any other file's
verdict, and the merge did not change `eslint.config.mjs`. The repo-wide
`pnpm lint` is CI's.
- **Changeset:** none, and `skip-changeset` applies. `files[]` is
`dist`, `README.md` and `CHANGELOG.md`. After a rebuild of
`plugin-security`, `dist/` carries 0 hits for the test's symbols,
against 4 files each for two positive controls.
## Acceptance notes
- **Out-of-scope finding, reported to the seat:** for the two
unreadable-child controls, explain's `sharing` layer detail says "The
update answers 403 PERMISSION_DENIED" (and likewise for delete), while
the door answers 404 `RECORD_NOT_FOUND`. The top-level record verdict is
the correct missing-record shape; only the layer prose names a status
the door does not give. The read-absent rewrite in `explainAccess`
replaces `record` and leaves layer prose as computed. Nothing here
touches it.
- **The fixture's position binding:** the fixture binds sets through a
position's `permissionSets`, which is `main`'s ADR-0131 spelling. HotCRM
on 17.7.0 binds its `sales_manager` set by naming it after the position.
The route under test, the master check's answer for a bypass holder on
the master, does not depend on which spelling delivered the set.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01CBAfsWMSfM3EToQGVStEcp)_
Co-authored-by: Claude <noreply@anthropic.com>1 parent 2ff0825 commit a18c514
1 file changed
Lines changed: 275 additions & 0 deletions
| 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 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
0 commit comments