Repository navigation
Commit b9e9609
fix(plugin-security): explain's record read verdict asks sharing with the caller's read depth (#20000)
Fixes #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 #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. #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
#19984 added. #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. #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>1 parent 1f05ea4 commit b9e9609
4 files changed
Lines changed: 387 additions & 7 deletions
File tree
- .changeset
- packages/plugins/plugin-security/src
- scripts
| 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 | + | |
| 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 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
0 commit comments