Repository navigation
Commit 3eb38ae
fix(service-storage): a parent-record editor may delete another user's attachment (#21753)
Fixes #21729
Clause-②: yes (widening)
## What this changes
A member holding `org_member` who can edit a record may now delete
another user's attachment on it. The attachment gate
(`installAttachmentAccessHooks`, delete branch) has always declared this
rule: the uploader OR a user who can edit the parent record. Before this
PR, the platform's row-level delete floor in `member_default`
(`owner_only_deletes`: `created_by == current_user.id`, positions
`['org_member']`) refused the delete in the security middleware's by-id
pre-image step (`security-plugin.ts`, the `PermissionDeniedError` of
step 2.7). The gate's parent-editor branch was never reached.
This follows the maintainer's ruling A on the card (comment 5979859114):
a delete-only alternate match, contributed TOGETHER with the gate that
judges it.
- **The seam (`@objectstack/plugin-security`).** Before this PR, no
plugin other than plugin-security could contribute a row policy.
Permission sets resolve by name from metadata, the bootstrap list and
`sys_permission_set`, and the human baseline is `member_default` plus an
app `isDefault` set. The new method is
`contributeOwnershipFloorAlternates(plugin, alternates)` on the
registered `security` service. It extends `ISecurityService` the same
way `getMetadataReadableFields` and `discardPermissionSetOverlay` do, so
there is no `packages/spec` edit. Contributions are keyed by the
contributing plugin: a second call replaces the first, and an empty list
withdraws it. The seam throws on three kinds of contribution:
- an object of `'*'`, which would withdraw the floor instead of
relieving it;
- an operation that is not one floor limb (`all`, `select`, `insert`);
- a policy that does not parse as `RowLevelSecurityPolicySchema`.
- **Placement.** `collectRLSPolicies` puts each alternate beside every
enabled shipped floor policy of its limb
(`withOwnershipFloorAlternates`), and copies that floor policy's
`positions`. The alternate therefore reaches exactly the principals the
floor binds. It can only add an OR match to a write class the floor
already occupies, so it can never make an empty write class non-empty.
An empty write class is what triggers the derive-from-select rule, which
is the reason `sys_comment_moderation`'s comment gives for its
load-bearing domain. Each materialized alternate also carries the
floor's provenance (`isPlatformOwnershipFloorPolicy`). Every composition
that drops the floor drops its relief too, and `checkAuthoredRowWrite`
never reads an alternate as an app-authored widener.
- **The contribution (`@objectstack/service-storage`).**
`contributeAttachmentDeleteFloorAlternate` contributes
`sys_attachment_parent_editor_delete` (`object: 'sys_attachment'`,
`operation: 'delete'`, `using: 'id != null'`). Its domain is inherited
from the floor: `['org_member']`. The call sits in the same
`kernel:ready` branch that installs the gate, right after
`installAttachmentAccessHooks`. Without the gate there is no relief. If
the security service lacks the seam or refuses the contribution, the
plugin logs it at `warn` and the floor stays in force (fail closed).
- **Not changed:** `owner_only_deletes`, `member_default`, the floor's
edit limb, `attachment-access-hooks.ts`, and `packages/spec`.
## Measured at the door
Measured on an org-bound dogfood boot (`orgContext: true`, both members
armed with `org_member` and the delete grant):
| caller, row | before | after |
| --- | --- | --- |
| parent editor, not the uploader, `DELETE` | 403 `PERMISSION_DENIED`
(floor) | **200**, row gone |
| non-uploader who can READ but not EDIT the parent, `DELETE` | 403
`PERMISSION_DENIED` (floor) | 403 `ATTACHMENT_DELETE_DENIED` (gate) |
| non-uploader who cannot READ the parent, `DELETE` | 403
`PERMISSION_DENIED` (floor) | 403 `PERMISSION_DENIED` (pre-image: the
row is invisible to the caller) |
| parent editor, not the uploader, `PATCH` (edit limb) | 403
`PERMISSION_DENIED` | 403 `PERMISSION_DENIED`, row unchanged |
| boot WITHOUT service-storage (`SysAttachment` registered by a host
plugin): parent editor `DELETE` / creator `DELETE` | n/a | 403
`PERMISSION_DENIED` / 200 |
**One deviation from the expected pin, measured and kept.** The REST pin
at case (a), memberB on the private `att_secret` parent, now asserts ONE
code. That code is `PERMISSION_DENIED`, not `ATTACHMENT_DELETE_DENIED`.
memberB cannot read that parent, so read visibility
(`installAttachmentReadVisibility`) hides the attachment from them: its
by-id GET answers 404. The pre-image re-read refuses a row the caller
cannot see before the gate runs. The gate's refusal names the parent
object and id, so this order also keeps them from a caller who cannot
read that record. The gate's own code is pinned on a parent the caller
CAN read (`att_readonly`): `ATTACHMENT_DELETE_DENIED`. Getting the
gate's code for an invisible parent would mean bypassing read visibility
in the pre-image, and would disclose the parent's identity.
## Pins
-
`packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts`
(declared cross-lane path, test only; no assertion deleted). The matrix
boots org-bound with an `assertArmed` precondition. Before this PR it
booted org-less, so the floor never applied and case (a) accepted
"either" code. The case (a) assertion now checks one code. Three new
cases cover the parent-editor 200, the readable non-editor
`ATTACHMENT_DELETE_DENIED`, and the untouched edit limb. A new describe
boots without service-storage.
- `plugin-security/src/ownership-floor-alternates.test.ts` covers the
seam's refusals and plugin keying, and the placement beside the enabled
shipped floor only, with floor provenance. It also checks
**byte-identity**: on a real `SecurityPlugin`, every (principal × object
× operation) cell's collected policies AND compiled filter are compared
with and without the contribution. The principals are member, org-less,
org admin and an MCP agent holding `org_member`. The objects are
`sys_attachment`, `sys_comment` and an ordinary object. The operations
are delete, update, find and insert. Exactly one cell moves: the
member's `sys_attachment` delete. Withdrawing the contribution restores
every cell.
- `service-storage`: `attachment-delete-floor-alternate.test.ts` covers
the contribution and its no-security, no-seam and refused outcomes.
`storage-service-plugin.test.ts` checks that the contribution happens
exactly once and after the gate, and that nothing is contributed on an
engine without a hook seam.
## Ablations
Each ablation was run through `scripts/ablation-replace.mjs` with a trap
restore, a rebuild and `ablation-dist-preflight.mjs`. Each was restored,
and the restore was proven: blob equals HEAD, `git diff HEAD` is empty,
the dist marker is back, and every pin is green again.
1. **Drop the contribution.** Prediction: two dogfood cells red, not
one. Measured: 2 red. The parent-editor delete answered 403
`PERMISSION_DENIED` instead of 200, and the readable non-editor answered
`PERMISSION_DENIED` instead of `ATTACHMENT_DELETE_DENIED`. One storage
wiring pin was also red. The other 17 dogfood cells stayed green,
including the edit limb and the no-storage boot.
2. **Widen the alternate to `operation: 'all'`.** Prediction: the seam
refuses it, so the result matches ablation 1 and the edit-limb pin stays
green. Measured: the seam refused it ("is not one floor limb"), the same
2 dogfood cells were red, and the edit-limb pin was green. 2b, relieving
the edit limb as well by contributing a second `update` alternate: only
the edit-limb pin went red (PATCH answered 200).
3. **Drop the inherited domain in `withOwnershipFloorAlternates`.**
Measured: the byte-identity pin went red. The cells that moved were the
org-less principal's and the org admin's `sys_attachment` delete, the
derive-from-select regression.
## Verification at `158aa1a3dd`
- **Gates:** `dispatch-gates --repo objectstack-ai/objectstack
--commands` derived 104 families, and all 104 exited 0. `dispatch-gates
--ran` reports 104 accounted for and 0 NOT-MEASURED. Three gates needed
work before passing:
- `check:skill-examples` and `check:dual-build-cjs-loads` first exited 3
(prerequisite: unbuilt dists). They are green after building those
packages.
- `check:engine-double-contract` was red once, on the new test's engine
double. The fix routes `findOne` through `assertEngineFindOnePredicate`
and records the ledger row.
- **Dogfood:** the attachments dogfood file passes 19 tests, with 1
skipped (block (g) is `skipIf` the organizations package is
unavailable).
- **Package suites** (at `a90a609a20`; only a dogfood comment changed
after it):
- `@objectstack/service-storage` `test`: 42 files, 640 tests pass.
- `@objectstack/plugin-security` `test`: 165 files, 3551 tests pass and
45 are skipped.
- `typecheck` is clean for both packages and for `@objectstack/dogfood`.
- `pnpm lint` was not run locally; it is left to CI.
## Docs
`content/docs/permissions/attachments-access.mdx`: the delete section
now names which refusal answers when, and says that the floor relief
exists only where service-storage installs the gate. No `skills/**`
sentence is made false. `skills/objectstack-data/rules/security.md`
still correctly recommends `owner_only_writes` / `owner_only_deletes`.
## Acceptance notes
- **Out of scope, reported to the seat to file (class a, a possible data
leak).** This is a pre-existing problem and this PR neither touches nor
widens it. A principal outside the floor's `org_member` domain can send
a delete for an attachment whose parent it cannot read. The gate's
delete-branch refusal (`ATTACHMENT_DELETE_DENIED`) then answers, not the
platform's not-visible refusal, and that refusal names the parent
record. Members inside the domain are unaffected: read visibility
refuses first in the pre-image. The measurement is in the dev report on
the card.
- The triage note said `sys_attachment` is "registered by
platform-objects". Measured: it is DEFINED there and publicly exported
(`@objectstack/platform-objects/audit`). In this repo it is REGISTERED
only by service-storage's manifest. The ruling's concern still holds,
because a host can register the exported object without the storage
plugin. The new describe block does exactly that.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01DiCSbmJrkzNhuEAier4VoJ)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent c7a60e1 commit 3eb38ae
12 files changed
Lines changed: 1055 additions & 13 deletions
File tree
- .changeset
- content/docs/permissions
- packages
- plugins/plugin-security/src
- qa/dogfood/test
- services/service-storage/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 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
70 | 70 | | |
71 | 71 | | |
72 | 72 | | |
73 | | - | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
74 | 86 | | |
75 | 87 | | |
76 | 88 | | |
| |||
151 | 163 | | |
152 | 164 | | |
153 | 165 | | |
154 | | - | |
| 166 | + | |
155 | 167 | | |
156 | 168 | | |
157 | 169 | | |
| |||
Lines changed: 262 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 | + | |
0 commit comments