Repository navigation
Commit a5afe38
fix(plugin-security): the delegated-admin gate resolves a scope's business-unit anchor inside the caller's own organization (#19800)
Fixes #19775
Clause-②: no
The delegated-administration gate resolved a scope's business-unit
anchor **by NAME across organizations**. In a single-database multi-org
posture (ADR-0105 D1 `group` / `isolated`; ADR-0132 「single-database
organization isolation ships open」) a name collision therefore crossed
an organization boundary, in both directions at once.
## The defect, verified at source before the repair
Verified on `origin/main` at the branch point `2cf9db7c43`, not taken
from the card:
- `delegated-admin-gate.ts:44` — `const SYSTEM_CTX = { isSystem: true }
as const;` carries no tenant.
- `delegated-admin-gate.ts:912` — the anchor lookup:
`ql.find('sys_business_unit', { where: { name: businessUnitName },
limit: 1, context: SYSTEM_CTX })`. **That is the by-name call.** The
descendant walk at `:933-937` runs under the same context with no
organization predicate.
- `packages/objectql/src/engine.ts:4295` — `hasTenant` is
`execCtx?.tenantId !== undefined && …`, and `:4316` sets `opts.tenantId`
only under it. It is independent of `isSystem`, which is what makes the
repair possible at all.
- `packages/drivers/driver-sql/src/sql-driver.ts:13771` —
`applyTenantScope` returns the builder untouched when `tenantId` is
undefined, null or empty.
-
`packages/platform-objects/src/identity/sys-business-unit.object.ts:260-263`
— `name` carries no uniqueness; the only unique index is `['code',
'organization_id']`.
So nothing scoped that read: `limit: 1` plus the SQL driver's `ORDER BY
id ASC` answered "whichever id sorts first", across every organization.
## The repair — fail closed, and narrower at every choice
The anchor read, the descendant walk, and the two catalog reads behind
`describeDelegableScope` now carry the caller's own organization, and
the candidates that come back are reduced to the caller's own rows.
- **The organization is `organizationId ?? tenantId`** — the same
spelling `SecurityPlugin.callerOrganizationId`
(`security-plugin.ts:989`) already resolves a caller's permission sets
with. The `adminScope` this gate reads arrives on a set loaded out of
THAT organization's catalog, so anchoring its business unit anywhere
else pairs an authority minted in one tenant with a tree owned by
another.
- **Two arms, both load-bearing, each measured** (ablation below): the
context carries the organization so `applyTenantScope` composes a
predicate, AND `resolveOwnOrganizationRow` (reused from
`per-organization-catalog.ts`, no new symbol) picks the caller's own row
out of what came back. The driver's compatibility arm deliberately also
returns organization-less rows, and a driver with no tenant scoping at
all returns every organization's.
- **Narrower where there was a choice.** Under the `group` posture the
anchor resolves in the caller's ACTIVE organization, not their whole
membership set. Under a walled posture an organization-less business
unit no longer answers a delegation boundary — that posture already
declares such a row invalid state (`per-organization-catalog.ts`). A
tenant admin's picker is unconstrained inside its organization and never
across organizations.
- **Unchanged where there is no boundary to cross**: a caller carrying
no organization (the `single` posture) keeps the by-name answer, pinned
by its own test.
- **No new exported symbol, no new key on any published payload.**
`describeDelegableScope` and `scopesCoverUser` take the caller's context
as a new OPTIONAL argument; `DelegableScopeReport` is byte-identical.
`ISecurityService.describeDelegableScope(callerContext)` in
`packages/spec` already declared that parameter, so this lane touches
`packages/spec` in zero lines.
- **No new error code, no new refusal message.** The crossing lands on
the refusals that already existed: "outside the delegated subtree" for a
write, and the empty fail-closed subtree a misconfigured scope already
produced.
## Evidence — the crossing itself, failing before and passing after
Two suites, deliberately complementary.
`delegated-admin-gate-cross-organization.test.ts` is a REAL `ObjectQL`
over a REAL `SqlDriver` on in-memory SQLite: two organizations each
holding a unit named `sales` with one child, ids ordered so the OTHER
organization's sort first (`bu_0_*` before `bu_a_*`).
`delegated-admin-gate.test.ts` gains the companion block on a fake `ql`
that ignores `context` entirely — a driver with no tenant scoping at
all.
**BEFORE** — the two source files reverted to the branch point
`2cf9db7c43` (proved on disk by blob hash, `07ee0a9ff2…` for the gate),
the new test kept:
```
$ pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 \
src/delegated-admin-gate-cross-organization.test.ts
× the org A delegate KEEPS its own subtree — a write inside org A `sales` is approved
AssertionError: promise rejected "PermissionDeniedError: [Security] Access …" instead of resolving
× the org A delegate cannot reach org B — a write anchored in the other organization is REFUSED
AssertionError: promise resolved "undefined" instead of rejecting
× describeDelegableScope names the caller's own units and NOTHING of the other organization
AssertionError: expected [ 'bu_0_sales', 'bu_0_sales_east' ] to deeply equal [ 'bu_a_sales', 'bu_a_sales_east' ]
× a tenant admin is unconstrained INSIDE its organization, never across organizations
AssertionError: expected [ 'bu_0_sales', …(3) ] to deeply equal [ 'bu_a_sales', 'bu_a_sales_east' ]
Test Files 1 failed (1)
Tests 4 failed | 4 passed (8)
```
Both measured directions of the defect are in those four lines: the org
A delegate **lost its own subtree** (rejected where it should resolve),
and the gate **approved** a delegated write anchored in org B (resolved
where it should reject). The four that passed are the controls — ground
truth, the single-organization dark control, the firing control, and the
organization-less caller.
**AFTER** — same command, same tree, the repair restored (`git checkout
HEAD -- …`, restore proved by `git diff HEAD` empty and a matching `git
hash-object`):
```
Test Files 1 passed (1)
Tests 8 passed (8)
```
**ABLATION — each arm proved independently load-bearing.** Removing only
the row-level arm (`resolveOwnOrganizationRow(...).own` replaced by
`rows[0] ?? null`; the injected text confirmed present on disk and the
deleted text confirmed absent, under an `EXIT INT TERM` trap that
restores from `HEAD`):
```
$ pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 \
src/delegated-admin-gate.test.ts src/delegated-admin-gate-cross-organization.test.ts
× resolves the caller's own `sales`, not the first row the driver handed over
× approves the same write inside the caller's own organization
× an anchor that exists ONLY in another organization approves nothing (fail closed)
Test Files 1 failed | 1 passed (2)
Tests 3 failed | 69 passed (72)
```
The unscoping-driver half goes red while the real-engine half stays
green — exactly the split the code comment claims, so neither arm is
decoration. The tree was restored byte-exact afterwards (`git
hash-object` = `732cc64311b9a36af7a2e338e6fc86c8ff80f698`, the `HEAD`
blob).
**No existing test asserted the defect.** The package's 118 files / 2261
tests pass unchanged; nothing had to be corrected, and no admission
decision was widened to make anything green.
## Verification
| what | command | result |
|:--|:--|:--|
| package tests | `pnpm --filter @objectstack/plugin-security test` |
118 files / **2261 passed** |
| package typecheck | `pnpm --filter @objectstack/plugin-security
typecheck` | exit 0, test layer 0 errors |
| dependency closure | `pnpm --filter '@objectstack/plugin-security^...'
build` | exit 0 |
| derived gate families | `node scripts/pm/dispatch-gates.mjs
--commands` then `--ran` | **62 derived, 62 run, 0 NOT-MEASURED, 0
UNRUN** |
| repo lint (whole tree, not narrowed) | `pnpm lint` (`eslint .
--no-inline-config`) | exit 0 |
Three of the 62 first answered `PREREQUISITE NOT MET` (exit 3, not a
finding) because they read built output; all three are green after `pnpm
exec turbo run build --filter='./packages/*' --filter='./packages/*/*'`
(72/72 successful): `check:dual-build-cjs-loads`, `check:i18n`,
`check:type-check-debt`. `check:where-matcher` found one real defect in
my own new fixture — a `$`-prefixed key read as a field name — fixed in
`da621549c0` and green. All readings are taken at `da621549c0`, the
final commit.
Not run here and left to CI: the five path-scheduled CI jobs, the
type-check lanes over the whole workspace, and the artifact-roster
families the derivation scores `silent` for every card.
## Acceptance notes
Observed while reading the fix face, deliberately NOT changed here —
handed back to the PM rather than filed, and rather than widened into
this PR:
- **The sibling by-name reads of `sys_position` in the same file are the
same defect class, unrepaired.** `positionIsDelegatable` (`:586`
pre-fix) and `setsBoundToPosition` (`:1005` pre-fix) both do
`ql.find('sys_position', { where: { name: positionName }, limit: 1,
context: SYSTEM_CTX })`, and `sys_position` is a per-organization
catalog row upserted by `(name, organization_id)`. They decide whether a
position may be self-delegated and which permission sets it distributes,
so a cross-organization row answering either one is an authority
decision. The card listed them under "Not measured"; they need their own
fixture and their own witness, which is a new verification surface this
card's scope does not carry.
- `businessUnitsOfUser` and `assignmentAnchorsOfPosition` read
`sys_business_unit_member` / `sys_user_position` under the same bare
system context. Both are keyed on a globally unique id rather than a
name, so the collision this card is about does not reach them — noted as
boundary, not claimed as a defect.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01AhQASwqJr2Z7XfGWUdvnbF)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 4a12ac0 commit a5afe38
5 files changed
Lines changed: 572 additions & 29 deletions
File tree
- .changeset
- packages/plugins/plugin-security/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 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 | + | |
0 commit comments