Skip to content

Commit 1c83ca2

Browse files
os-trumpclaude
andauthored
fix(plugin-security): stop letting org-admin row count decide whether a platform admin already exists (#17116)
* fix(plugin-security): stop letting org-admin row count decide whether a platform admin already exists The `already_have_admin` short-circuit read `sys_user_permission_set` with no `orderBy` and a cap of 50, then applied the predicate that actually decides — `!organization_id` — client-side to whatever 50 rows the driver returned first. `admin_full_access` is not only the platform-admin set: every organization-scoped grant of it writes a row carrying the same `permission_set_id`, so the population grows with the number of org admins. A tenant with fifty-odd of them filled the window with rows that all fail the filter, the short-circuit did not fire, a second unscoped grant was minted, and `claimSeedOwnership` re-owned the seeded business rows to the newly promoted user — silently. The read is now two legs, both ordered server-side and bounded, and the bound warns with the number of rows it examined: Leg A asks the driver the narrow question (`organization_id: null`), so no org-admin count can crowd the answer out of a window. Leg B scans the grant population for the set, ordered and bounded, still applying the exact client-side predicate. Leg B is not redundant: `organization_id: ''` is storable and reads back as `''`, which `!organization_id` counts as unscoped and `where: { organization_id: null }` does not return — so the card's suggested one-line `where` narrowing would have RELAXED this guard on its own. Both legs are strictly additive to what the old read could see, so the guard can only fire more often, never less. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * test(plugin-security): pin the already_have_admin guard against the org-admin row count, with its under-cap control The card's reproduction sketch as a cell rather than a failure: 60 organization-scoped grants plus one unscoped human grant whose row sorts last must return already_have_admin, and the SAME fixture with 9 organization-scoped grants must return it too. The under-cap row is the control that proves the fixture measures truncation and not some other difference between the two populations. Also pinned: the `organization_id: ''` legacy holder the narrowed read alone could not have seen; that usr_system still never counts; the reported adminGrantRowsExamined; and the ceiling warning with its under-ceiling control. Counts examined rows by identity rather than by read, so the two legs' overlap does not inflate a number that calls itself rows examined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * chore(plugin-security): record the new test's engine doubles and refuse combinators in its fake matcher `check:engine-double-contract` RETAINED the two engine doubles the new suite pins, so the ledger learns about them or it never protects the file. `check:where-matcher` flagged the synthetic driver's permission-set matcher as combinator-blind: it now refuses a `$`-prefixed key inside the matcher itself rather than one frame out, so a double that does not implement `$or` says so instead of reporting a row it never understood as absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3c557e2 commit 1c83ca2

4 files changed

Lines changed: 760 additions & 18 deletions

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
"@objectstack/plugin-security": minor
3+
---
4+
5+
The first-boot `already_have_admin` short-circuit now FINDS an existing platform admin instead of sampling for one, so a tenant's organization-admin count can no longer decide whether a second unscoped `admin_full_access` grant is minted.
6+
7+
Before this change the holders read was `sys_user_permission_set` with **no `orderBy` and a cap of 50**, and the predicate that actually decides — `!organization_id` — was applied **client-side to whatever 50 rows the driver returned first**. `admin_full_access` is not only the platform-admin set: every *organization-scoped* grant of it writes a row carrying the same `permission_set_id`, so this population grows with the number of **org** admins, not platform admins. A tenant with fifty-odd of them filled the window with rows that all fail the filter, the short-circuit did not fire, a **second** unscoped grant was minted, and `claimSeedOwnership` re-owned the seeded business records to the newly promoted user — silently, because the boot logs a successful promotion exactly as on a genuinely fresh install. Measured on the real better-sqlite3 driver: with 60 organization-scoped grants plus one unscoped human grant, the unordered 50-row window contained 50 organization-scoped rows and not the one that decides.
8+
9+
That is the guarantee #14348 case D pins — 「Moving an already-granted platform admin is reserved to the maintainer.」 — failing open by row count.
10+
11+
- **The read asks the driver the narrow question first.** `{ permission_set_id, organization_id: null }`, ordered and bounded. Because it is narrowed server-side, no number of organization-scoped grants can crowd the answer out of a window.
12+
- **A second, ordered and bounded leg still applies the exact predicate.** It runs only when the narrow leg found nobody. This is deliberate rather than redundant: `organization_id: ''` is storable and reads back as `''` on both SQL families, which `!organization_id` counts as **unscoped** and `where: { organization_id: null }` does **not** return — so replacing the client-side predicate with the narrowed read alone would have made this guard fire *less* often and mint the very grant this fixes. Both legs are strictly additive to what the old read could see, so the guard can only fire more often than before, never less.
13+
- **The bound is never silent.** The scan pages 200 rows at a time up to a 5000-row ceiling, and reaching that ceiling without finding an unscoped human holder now WARNS — naming the ceiling, the number of rows examined, and the consequence (promoting from here would mint a second unscoped grant and re-own the seeded records).
14+
- **The answer says how many rows it examined.** `bootstrapPlatformAdmin`'s returned report gains an optional `adminGrantRowsExamined`, counted by row identity across both legs, on every return the guard reaches. A guard that had seen the whole population and one that had seen a truncated slice of it previously returned byte-identical payloads.
15+
- **The ordering is stated to the driver, and it is measured, not assumed.** `tryFind` answers `[]` when a query is refused, and on this guard `[]` reads as "no platform admin exists yet" — which promotes. An order this object could not serve would therefore be a silent relaxation, so `id` ascending was measured honoured through ObjectQL on both SQL driver families against the real declarations.
16+
17+
Unchanged: an unscoped grant held by the seed identity `usr_system` still never counts, so a database where it was wrongly promoted stays self-healing on restart; the walled postures still mint no grant row and still point a legacy unscoped holder at the config path; and a genuinely fresh install still promotes exactly as before.

0 commit comments

Comments
 (0)