From 982b00ca7394fe3157f58beba1fcf549fcede058 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 10:19:17 +0000 Subject: [PATCH 1/3] fix(plugin-security): stop letting org-admin row count decide whether a platform admin already exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --- .../src/bootstrap-platform-admin.ts | 201 ++++++++++++++++-- 1 file changed, 183 insertions(+), 18 deletions(-) diff --git a/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts b/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts index 73543200ba..a36b32f72e 100644 --- a/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts +++ b/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts @@ -150,6 +150,42 @@ const SYSTEM_CTX = { isSystem: true }; export const PLATFORM_ADMIN_CANDIDATE_PAGE_SIZE = 200; export const PLATFORM_ADMIN_CANDIDATE_SCAN_CEILING = 5000; +/** + * [#16861] The `already_have_admin` guard's page size and hard ceiling — what + * replaced the bare, unordered `50` at the HOLDERS read, one read above the + * candidate scan. + * + * Deliberately the same numbers and the same shape as the candidate scan's + * pair above rather than a second set of tuning knobs: two adjacent reads in + * one function that bound themselves differently is a future reader's trap. + * They are separate CONSTANTS because the populations are different objects — + * `sys_user` there, `sys_user_permission_set` here — and tuning one must not + * silently retune the other. + * + * Exported for the same reason as the pair above: a test that restates the + * numbers goes quietly vacuous the day one is tuned. ⚠️ Neither pair is + * re-exported from this package's `index.ts`, so neither is on the published + * `.` surface — `bootstrapPlatformAdmin` and its RETURN OBJECT are. + */ +export const PLATFORM_ADMIN_GRANT_PAGE_SIZE = 200; +export const PLATFORM_ADMIN_GRANT_SCAN_CEILING = 5000; + +/** + * The order the grant scan states TO THE DRIVER, so a page is a deterministic + * slice of the population instead of whatever that driver produced first. + * + * `id` and not `created_at`: `id` is this object's declared primary key, so it + * is present and total on every family, and it was MEASURED honoured on this + * very object through `ObjectQL` on both SQL families. That measurement is not + * a formality — {@link 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 + * of the boundary the guard exists to hold. + */ +const ADMIN_GRANT_SCAN_ORDER: { field: string; order: 'asc' | 'desc' }[] = [ + { field: 'id', order: 'asc' }, +]; + /** * One read, with the sort and the page WHERE THE DRIVER CAN SEE THEM. * @@ -358,6 +394,15 @@ export async function bootstrapPlatformAdmin( * should not be auditable only by reading which code path ran. */ basis?: 'declared-owner' | 'oldest-authenticable'; + /** + * [#16861] How many `admin_full_access` grant rows the `already_have_admin` + * guard actually examined before answering. The old read looked at "up to 50, + * whichever the driver produced first" and said nothing, so a guard that had + * seen the whole population and a guard that had seen a truncated sample of + * it returned BYTE-IDENTICAL payloads. Present on every return the guard + * reaches; absent on the returns that precede it. + */ + adminGrantRowsExamined?: number; }> { const logger = options.logger; if (!ql || typeof ql.find !== 'function' || typeof ql.insert !== 'function') { @@ -476,25 +521,134 @@ export async function bootstrapPlatformAdmin( return { seeded: seededCount, adminPromoted: false, reason: 'admin_permission_set_missing', ...resyncCounts }; } - const existingAdminLinks = await tryFind( + // ── Does this deployment ALREADY have a platform admin? (#16861) ────────── + // + // This read was `tryFind(ql, 'sys_user_permission_set', { permission_set_id: + // adminPsId }, 50)` — no `orderBy`, cap 50 — with the predicate that actually + // decides (`!organization_id`) applied CLIENT-SIDE to whatever 50 rows the + // driver produced 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 with the number of 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 it does on a + // genuinely fresh install. What fails open there is #14348 case D: + // 「Moving an already-granted platform admin is reserved to the maintainer.」 + // + // ## Why this is TWO reads and not `organization_id: null` in the `where` + // + // The card's suggested one-line narrowing was MEASURED before it was taken, + // and on its own it would have RELAXED this guard. Null matching itself is + // uniform across the families that can be measured — each answers "the column + // holds no value": + // + // driver-sql (better-sqlite3) via ObjectQL + these real objects -> the unscoped row only + // driver-sqlite-wasm via ObjectQL + these real objects -> the unscoped row only + // driver-memory driver face -> null-valued AND key-absent rows + // driver-mongodb translator (its own live suites -> `{organization_id: null}`, + // need a 123 MB binary download) Mongo's null-or-missing reading + // + // What is NOT uniform is the narrowed read against THIS code's own predicate. + // `organization_id: ''` is storable on both SQL families and reads back as + // `''`: `!organization_id` counts that row UNSCOPED, and `where: { + // organization_id: null }` does NOT return it. A narrowing that REPLACED the + // client-side predicate would therefore stop seeing a legacy unscoped holder + // stored that way, fire less often, and mint the second grant this card is + // about. ⛔ This card only tightens, so the predicate is untouched and the + // READ is what changes: + // + // Leg A — ask the driver the narrow question. Independent of how many + // org-scoped grants exist, so no org-admin count can crowd the + // answer out of a window. + // Leg B — only when leg A found nobody: scan the grant population for this + // set, ORDERED so each page is a deterministic slice rather than + // "whatever the driver produced first", bounded, and WARNING at the + // bound with the number of rows examined. This is the leg that + // still sees a `''`-shaped legacy row. + // + // Both legs are strictly ADDITIVE to what the old read could see, so the + // guard can only fire MORE often than before, never less. + // + // The seed-data owner `usr_system` (provisioned by the SeedLoader, see + // runtime/app-plugin.ts `ensureSeedIdentity`) never counts — otherwise a DB + // where it was wrongly promoted would block every real admin forever. + // Ignoring it here makes the bootstrap self-healing on restart. + const isUnscopedHumanHolder = (r: any) => + !r.organization_id && r.user_id !== SystemUserId.SYSTEM; + + let adminGrantRowsExamined = 0; + let adminGrantScanTruncated = false; + + // Leg A — the narrow question, asked of the driver. + const unscopedGrantRows = await tryFind( ql, 'sys_user_permission_set', - { permission_set_id: adminPsId }, - 50, - ); - // Human holders of the cross-tenant grant. The seed-data owner `usr_system` - // (provisioned by the SeedLoader, see runtime/app-plugin.ts - // `ensureSeedIdentity`) never counts — otherwise a DB where it was wrongly - // promoted would block every real admin forever. Ignoring it here makes the - // bootstrap self-healing on restart. - const humanUnscopedHolders = existingAdminLinks.filter( - (r) => !r.organization_id && r.user_id !== SystemUserId.SYSTEM, + { permission_set_id: adminPsId, organization_id: null }, + PLATFORM_ADMIN_GRANT_PAGE_SIZE, + ADMIN_GRANT_SCAN_ORDER, ); + adminGrantRowsExamined += unscopedGrantRows.length; + let unscopedHolder: any | undefined = unscopedGrantRows.find(isUnscopedHumanHolder); + + // Leg B — the ordered, bounded scan that still applies the exact predicate. + if (!unscopedHolder) { + const pageSize = PLATFORM_ADMIN_GRANT_PAGE_SIZE; + const ceiling = PLATFORM_ADMIN_GRANT_SCAN_CEILING; + for (let offset = 0; offset < ceiling && !unscopedHolder; offset += pageSize) { + const pageLimit = Math.min(pageSize, ceiling - offset); + const page = await tryFind( + ql, + 'sys_user_permission_set', + { permission_set_id: adminPsId }, + pageLimit, + ADMIN_GRANT_SCAN_ORDER, + offset, + ); + if (page.length === 0) break; + adminGrantRowsExamined += page.length; + unscopedHolder = page.find(isUnscopedHumanHolder); + if (unscopedHolder) break; + if (page.length < pageLimit) break; + if (offset + page.length >= ceiling) adminGrantScanTruncated = true; + } + } + + // ⛔ The truncation is never silent (#16861). Reaching the ceiling is the one + // way this scan still answers "no platform admin yet" while one exists, and + // that answer does not merely skip a log line — it MINTS A SECOND unscoped + // grant and hands it the seeded business records. So it says the number it + // examined rather than letting the promotion below read as a statement about + // the whole table. + if (adminGrantScanTruncated && !unscopedHolder) { + const truncation = + '[security] the existing-platform-admin check stopped at its ceiling of ' + + `${PLATFORM_ADMIN_GRANT_SCAN_CEILING} admin_full_access grant row(s) ` + + `(${adminGrantRowsExamined} examined) without finding an unscoped human grant — rows beyond ` + + 'that point were NOT examined, so this deployment may ALREADY have a platform administrator ' + + 'this boot did not see. Promoting now would mint a SECOND unscoped grant and re-own the seeded ' + + `business records to it. Name the intended administrator with ${PLATFORM_OWNER_EMAIL_ENV} rather ` + + 'than leaving the answer to a scan bound.'; + if (logger?.warn) logger.warn(truncation); + else logger?.info?.(truncation); + } + + // Attached to every return the guard reaches, so a caller can tell a guard + // that saw the whole population from one that saw a bounded slice of it. + const grantScanCounts = { adminGrantRowsExamined }; + // `single`: a platform admin "already exists" — the promotion is a no-op // forever. Under walled postures that same row is the LEGACY anchor and gets // the deprecation pointer below instead of a silent early exit. - if (!walled && humanUnscopedHolders.length > 0) { - return { seeded: seededCount, adminPromoted: false, reason: 'already_have_admin', ...resyncCounts }; + if (!walled && unscopedHolder) { + return { + seeded: seededCount, + adminPromoted: false, + reason: 'already_have_admin', + ...resyncCounts, + ...grantScanCounts, + }; } if (walled) { @@ -506,8 +660,8 @@ export async function bootstrapPlatformAdmin( // ONCE per process through the same latch the derivation-site reporter // uses (`reportLegacyPlatformAdminGrant`): boot-time detection here and // request-time detection there can never add up to two lines. - if (humanUnscopedHolders.length > 0) { - const holder = humanUnscopedHolders[0]; + if (unscopedHolder) { + const holder = unscopedHolder; const holderRows = await tryFind(ql, 'sys_user', { id: holder.user_id }, 1); reportLegacyPlatformAdminGrant({ userId: String(holder.user_id), @@ -527,7 +681,7 @@ export async function bootstrapPlatformAdmin( // administrator, on the old anchor. const platformAdminConfig = resolvePlatformAdminEmails(); if (platformAdminConfig.emails.length === 0) { - if (humanUnscopedHolders.length === 0) { + if (!unscopedHolder) { const message = `[security] tenancy posture is walled but ${PLATFORM_OWNER_EMAIL_ENV} declares no usable ` + 'platform administrator (unset, blank, or refused for an unparseable entry) — ' + @@ -544,6 +698,7 @@ export async function bootstrapPlatformAdmin( adminPromoted: false, reason: 'walled_owner_email_undeclared', ...resyncCounts, + ...grantScanCounts, }; } @@ -572,6 +727,7 @@ export async function bootstrapPlatformAdmin( adminPromoted: false, reason: 'walled_config_derived', ...resyncCounts, + ...grantScanCounts, }; } @@ -674,7 +830,13 @@ export async function bootstrapPlatformAdmin( }); if (!inserted) { logger?.warn?.(`[security] failed to grant admin_full_access to first user ${chosen.email ?? chosen.id}`); - return { seeded: seededCount, adminPromoted: false, reason: 'insert_failed', ...resyncCounts }; + return { + seeded: seededCount, + adminPromoted: false, + reason: 'insert_failed', + ...resyncCounts, + ...grantScanCounts, + }; } logger?.info?.( `[security] first user promoted to platform admin: ${chosen.email ?? chosen.id} ` @@ -699,6 +861,7 @@ export async function bootstrapPlatformAdmin( ownershipClaimed, basis: audit.basis, ...resyncCounts, + ...grantScanCounts, }; }; @@ -870,6 +1033,7 @@ export async function bootstrapPlatformAdmin( // "click the link in your mailbox". reason: unverifiedOnly ? 'declared_owner_not_verified' : 'declared_owner_not_authenticable', ...resyncCounts, + ...grantScanCounts, }; } @@ -892,7 +1056,7 @@ export async function bootstrapPlatformAdmin( } if (scannedHumans === 0) { logger?.info?.('[security] no human users yet — first sign-up will be promoted to platform admin'); - return { seeded: seededCount, adminPromoted: false, reason: 'no_users', ...resyncCounts }; + return { seeded: seededCount, adminPromoted: false, reason: 'no_users', ...resyncCounts, ...grantScanCounts }; } if (!target) { // [#14348] Humans exist, but not one of them can sign in. Measured on a @@ -931,6 +1095,7 @@ export async function bootstrapPlatformAdmin( adminPromoted: false, reason: 'no_authenticable_user', ...resyncCounts, + ...grantScanCounts, }; } From e91934804197f5336aafdcc0bef0a0cccef1be83 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 10:25:16 +0000 Subject: [PATCH 2/3] 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 Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --- .../platform-admin-existing-holder-scan.md | 17 + ...latform-admin-existing-holder-scan.test.ts | 538 ++++++++++++++++++ .../src/bootstrap-platform-admin.ts | 18 +- 3 files changed, 570 insertions(+), 3 deletions(-) create mode 100644 .changeset/platform-admin-existing-holder-scan.md create mode 100644 packages/plugins/plugin-security/src/bootstrap-platform-admin-existing-holder-scan.test.ts diff --git a/.changeset/platform-admin-existing-holder-scan.md b/.changeset/platform-admin-existing-holder-scan.md new file mode 100644 index 0000000000..18e204a702 --- /dev/null +++ b/.changeset/platform-admin-existing-holder-scan.md @@ -0,0 +1,17 @@ +--- +"@objectstack/plugin-security": minor +--- + +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. + +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. + +That is the guarantee #14348 case D pins — 「Moving an already-granted platform admin is reserved to the maintainer.」 — failing open by row count. + +- **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. +- **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. +- **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). +- **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. +- **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. + +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. diff --git a/packages/plugins/plugin-security/src/bootstrap-platform-admin-existing-holder-scan.test.ts b/packages/plugins/plugin-security/src/bootstrap-platform-admin-existing-holder-scan.test.ts new file mode 100644 index 0000000000..e9169f0c5c --- /dev/null +++ b/packages/plugins/plugin-security/src/bootstrap-platform-admin-existing-holder-scan.test.ts @@ -0,0 +1,538 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #16861 — whether this deployment ALREADY has a platform admin, and why the + * answer stopped being a function of how many ORG admins it has. + * + * ## The defect, re-measured on this branch's base before anything changed + * + * The `already_have_admin` short-circuit read `tryFind(ql, + * 'sys_user_permission_set', { permission_set_id: adminPsId }, 50)` — no + * `orderBy`, cap 50 — and then applied the predicate that actually decides, + * `!organization_id`, CLIENT-SIDE to whatever 50 rows the driver produced + * 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 this query counts grows with the + * number of ORG admins. A tenant with fifty-odd of them fills the window with + * rows that all fail the filter, and the guard answers "no platform admin + * here" about a deployment that has one. + * + * The consequence is not a missing log line. A SECOND unscoped + * `admin_full_access` grant is minted, and `claimSeedOwnership` re-owns the + * seeded business records to the newly promoted user — silently, because the + * boot logs a successful promotion exactly as on a genuinely fresh install. + * The guarantee that fails open is #14348 case D: 「Moving an already-granted + * platform admin is reserved to the maintainer.」 + * + * ## ⭐ The contrast IS the test — both halves predicted before running + * + * The card's reproduction sketch is a cell, not a failure: + * + * | fixture | on the base | after the fix | + * |---|---|---| + * | 60 org-scoped grants + 1 unscoped human grant sorting LAST (61 rows) | `adminPromoted: true`, **two** unscoped grant rows | `already_have_admin`, **one** unscoped grant row | + * | the SAME fixture with 9 org-scoped grants (10 rows, under the cap) | `already_have_admin` | `already_have_admin` — unchanged | + * + * ⛔ The 60-row failure on its own would be half a test. The 10-row row is the + * CONTROL that proves the fixture measures TRUNCATION and not some other + * difference between the two populations: the same code, the same shapes, the + * same posture, one number changed. + * + * Both rows were predicted in writing first and then observed. The measured + * base run is quoted on the PR. + * + * ## Why the fix is TWO reads, and why this file pins the second one + * + * The card's suggested `organization_id: null` in the `where` was measured + * before it was taken. Null matching IS uniform across the driver families + * that can be measured — but the narrowed read is NOT equivalent to the + * predicate this code applies. `organization_id: ''` is storable and reads + * back as `''`: `!organization_id` counts it UNSCOPED, `where: { + * organization_id: null }` does not return it. A narrowing that REPLACED the + * client-side predicate would therefore have stopped seeing a legacy unscoped + * holder stored that way — firing the guard LESS often and minting exactly the + * second grant this card is about. ⛔ This card only tightens, so the + * `''`-shaped holder has its own case below: it is the one the ordered, + * bounded scan exists for. + * + * ## Why the row ORDER is permuted rather than a second driver package + * + * Same reason as `bootstrap-platform-admin-promotion-selection.test.ts` + * (#16682): `@objectstack/driver-memory` cannot be declared here without a + * `scripts/driver-memory-census.ledger.json` disposition, which is a + * maintainer ruling. Each case runs the REAL engine over the REAL + * better-sqlite3 driver behind a facade that permutes a result ONLY when the + * query carried no `orderBy` — precisely the freedom a driver has there. When + * the query DOES carry `orderBy` the facade forwards it and returns the result + * untouched, so a fix that sent an order and a driver that ignored it would + * still be caught, and `AS_RETURNED` is an unwrapped real-driver run. + */ + +import { describe, it, expect, afterEach, beforeEach } from 'vitest'; +import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { resetPlatformAdminEmailMemo } from '@objectstack/core'; +import { SysUser, SysAccount } from '@objectstack/platform-objects/identity'; +import { + bootstrapPlatformAdmin, + PLATFORM_ADMIN_GRANT_PAGE_SIZE, + PLATFORM_ADMIN_GRANT_SCAN_CEILING, +} from './bootstrap-platform-admin.js'; +import { SysPermissionSet } from './objects/sys-permission-set.object.js'; +import { SysUserPermissionSet } from './objects/sys-user-permission-set.object.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; + +const SYSTEM_CTX = { isSystem: true }; +const OWNER_ENV = 'OS_PLATFORM_OWNER_EMAIL'; +/** The permission-set id the fixture pre-seeds, so grant rows can point at it. */ +const ADMIN_PS_ID = 'ps_admin_full'; + +const engines: ObjectQL[] = []; + +afterEach(async () => { + while (engines.length) { + try { + await engines.pop()?.destroy(); + } catch { + /* noop */ + } + } +}); + +beforeEach(() => { + delete process.env[OWNER_ENV]; + resetPlatformAdminEmailMemo(); +}); + +afterEach(() => { + delete process.env[OWNER_ENV]; + resetPlatformAdminEmailMemo(); +}); + +/** A fresh engine on its own `:memory:` sqlite database with the REAL declarations. */ +async function boot(): Promise { + const engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), + true, + ); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.security-objects', + name: 'Security Objects', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [SysPermissionSet, SysUserPermissionSet, SysUser, SysAccount], + } as any); + await engine.syncSchemas(); + engines.push(engine); + return engine; +} + +const NATURAL_ORDERS = ['AS_RETURNED', 'INSERTION', 'REVERSED'] as const; +type NaturalOrder = (typeof NATURAL_ORDERS)[number]; + +/** + * Wrap a real engine so an UNORDERED read comes back in `order`. + * + * ⚠️ The rule that keeps this honest: when the query carries `orderBy`, the + * query is forwarded verbatim and the RESULT is returned untouched. The facade + * never sorts, so everything the fix relies on is done by the real SQL engine. + */ +function withNaturalOrder(engine: ObjectQL, order: NaturalOrder): any { + const insertionRank = new Map(); + let nextRank = 0; + const rankKey = (object: string, id: unknown) => `${object}:${String(id)}`; + return { + async find(object: string, query: any, options: any) { + const rows = await (engine as any).find(object, query, options); + if (!Array.isArray(rows)) return rows; + if (order === 'AS_RETURNED') return rows; + if (query?.orderBy) return rows; + if (order === 'REVERSED') return [...rows].reverse(); + return [...rows].sort( + (a, b) => + (insertionRank.get(rankKey(object, a?.id)) ?? 0) - + (insertionRank.get(rankKey(object, b?.id)) ?? 0), + ); + }, + async insert(object: string, data: any, options: any) { + const result = await (engine as any).insert(object, data, options); + const id = data?.id ?? result?.id; + if (id !== undefined) insertionRank.set(rankKey(object, id), nextRank++); + return result; + }, + // The shared engine-double contract (`check:engine-double-contract`): + // asserted BEFORE delegating, so this wrapper can never be the loose link. + async update(object: string, data: any, options: any) { + assertEngineUpdateDispatch(data, options); + return (engine as any).update(object, data, options); + }, + }; +} + +// ─────────────────────────────────────────────────────────────────────────── +// Fixtures +// ─────────────────────────────────────────────────────────────────────────── + +async function seedUser( + ql: any, + id: string, + email: string, + createdAt: string, + withAccount: boolean, +): Promise { + await ql.insert( + 'sys_user', + { id, email, name: email.split('@')[0], created_at: createdAt, email_verified: false }, + { context: SYSTEM_CTX }, + ); + if (withAccount) { + await ql.insert( + 'sys_account', + { id: `acc_${id}`, user_id: id, account_id: email, provider_id: 'credential' }, + { context: SYSTEM_CTX }, + ); + } +} + +/** + * The card's population: a tenant whose `admin_full_access` rows are dominated + * by ORGANIZATION-scoped grants, plus the one unscoped human holder that IS its + * platform admin. + * + * The unscoped grant row's id collates AFTER every org-scoped one, so the real + * driver's own unordered window is exactly the window the card describes: full + * of rows that fail `!organization_id`, with the row that decides outside it. + * + * `usr_orgadmin_001` is the OLDEST authenticable human — an org admin imported + * before the founder signed up, which is what a tenant with sixty org admins + * actually looks like. So when the guard fails, the promotion does not merely + * duplicate the founder's grant: it hands platform admin to an ORG admin. + * + * @param orgGrants how many organization-scoped grants of the same set exist + * @param unscopedOrganizationId the value the founder's unscoped grant stores + */ +async function seedTenant( + ql: any, + orgGrants: number, + unscopedOrganizationId: null | '' = null, +): Promise { + await ql.insert( + 'sys_permission_set', + { id: ADMIN_PS_ID, name: 'admin_full_access', label: 'Admin Full Access', managed_by: 'platform', active: true }, + { context: SYSTEM_CTX }, + ); + + // The founder — this deployment's platform admin, and the row the guard has + // to find. Inserted FIRST so the insertion-order family puts it in row 1. + await seedUser(ql, 'usr_founder', 'founder@tenant.example', '2026-01-01T00:00:00.000Z', true); + await ql.insert( + 'sys_user_permission_set', + { + id: 'ups_zzz_founder', + user_id: 'usr_founder', + permission_set_id: ADMIN_PS_ID, + organization_id: unscopedOrganizationId, + }, + { context: SYSTEM_CTX }, + ); + + for (let i = 1; i <= orgGrants; i++) { + const n = String(i).padStart(3, '0'); + await seedUser( + ql, + `usr_orgadmin_${n}`, + `orgadmin${n}@tenant.example`, + `2025-01-01T00:00:${String(i % 60).padStart(2, '0')}.000Z`, + true, + ); + await ql.insert( + 'sys_user_permission_set', + { + id: `ups_org_${n}`, + user_id: `usr_orgadmin_${n}`, + permission_set_id: ADMIN_PS_ID, + organization_id: `org_${n}`, + }, + { context: SYSTEM_CTX }, + ); + } +} + +async function findRows(engine: ObjectQL, object: string, where: any = {}): Promise { + const rows = await (engine as any).find(object, { where, limit: 5000 }, { context: SYSTEM_CTX }); + return Array.isArray(rows) ? rows : []; +} + +/** Every `admin_full_access` grant row that no organization scopes. */ +async function unscopedGrants(engine: ObjectQL): Promise { + const links = await findRows(engine, 'sys_user_permission_set', { permission_set_id: ADMIN_PS_ID }); + expect(links.length, 'ANTI-VACUITY: the fixture must have written grant rows').toBeGreaterThan(0); + return links.filter((r) => !r.organization_id); +} + +function collectingLogger() { + const info: string[] = []; + const warn: string[] = []; + const error: string[] = []; + return { + info, + warn, + error, + logger: { + info: (m: string) => info.push(m), + warn: (m: string) => warn.push(m), + error: (m: string) => error.push(m), + }, + }; +} + +describe('#16861 — an existing platform admin is found, not sampled for', () => { + // ───────────────────────────────────────────────────────────────────────── + // ANTI-VACUITY: the truncated window really is driver-shaped + // ───────────────────────────────────────────────────────────────────────── + + it('ANTI-VACUITY: the old unordered 50-row read really does hide the unscoped grant', async () => { + // Without this the 60-row case could be green because the fixture happens + // to be shaped some other way, rather than because the guard was repaired. + const engine = await boot(); + await seedTenant(engine as any, 60); + + const all = await findRows(engine, 'sys_user_permission_set', { permission_set_id: ADMIN_PS_ID }); + expect(all).toHaveLength(61); + + // The exact read the defect shipped: same object, same where, same cap, no order. + const window50 = await (engine as any).find( + 'sys_user_permission_set', + { where: { permission_set_id: ADMIN_PS_ID }, limit: 50 }, + { context: SYSTEM_CTX }, + ); + expect(window50).toHaveLength(50); + expect(window50.some((r: any) => r.id === 'ups_zzz_founder')).toBe(false); + // …and every row it DID return fails the predicate that decides. + expect(window50.every((r: any) => !!r.organization_id)).toBe(true); + // The row that decides exists all the same. + expect((await unscopedGrants(engine)).map((r) => r.id)).toEqual(['ups_zzz_founder']); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 1. The card's cell: 60 org grants FAILS on the base, and its 10-row CONTROL + // ───────────────────────────────────────────────────────────────────────── + + describe("the card's cell — the answer must not be a function of the org-admin count", () => { + for (const [label, orgGrants] of [ + ['60 organization-scoped grants (61 rows — over the old cap)', 60], + ['CONTROL: 9 organization-scoped grants (10 rows — under the old cap)', 9], + ] as const) { + for (const order of NATURAL_ORDERS) { + it(`${label}, natural order ${order} ⇒ already_have_admin`, async () => { + const engine = await boot(); + const ql = withNaturalOrder(engine, order); + await seedTenant(ql, orgGrants); + + const { info, logger } = collectingLogger(); + const report = await bootstrapPlatformAdmin(ql, defaultPermissionSets, { logger }); + + expect(report.adminPromoted).toBe(false); + expect(report.reason).toBe('already_have_admin'); + // ⭐ The harm, stated as the card states it: no SECOND unscoped grant. + expect((await unscopedGrants(engine)).map((r) => r.id)).toEqual(['ups_zzz_founder']); + // …and nothing was re-owned, because nothing was promoted. + expect(info.join('\n')).not.toContain('promoted to platform admin'); + }); + } + } + }); + + // ───────────────────────────────────────────────────────────────────────── + // 2. The leg the `where` narrowing alone could not have covered + // ───────────────────────────────────────────────────────────────────────── + + it("a legacy holder storing organization_id '' is still seen — the fix only tightens", async () => { + // Measured on both SQL families through ObjectQL and these real objects: + // `organization_id: ''` is storable, reads back as `''`, is UNSCOPED to + // `!organization_id`, and is NOT returned by `where: { organization_id: + // null }`. So the ordered bounded scan — not the narrowed read — is what + // answers here, and dropping it would have RELAXED this guard. + const engine = await boot(); + const ql = withNaturalOrder(engine, 'AS_RETURNED'); + await seedTenant(ql, 60, ''); + + const { logger } = collectingLogger(); + const report = await bootstrapPlatformAdmin(ql, defaultPermissionSets, { logger }); + + expect(report.reason).toBe('already_have_admin'); + expect((await unscopedGrants(engine)).map((r) => r.id)).toEqual(['ups_zzz_founder']); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 3. The guard says how many rows it examined + // ───────────────────────────────────────────────────────────────────────── + + it('reports the number of grant rows examined, on the answer as well as in the log', async () => { + const engine = await boot(); + const ql = withNaturalOrder(engine, 'AS_RETURNED'); + await seedTenant(ql, 60); + + const { logger } = collectingLogger(); + const report = await bootstrapPlatformAdmin(ql, defaultPermissionSets, { logger }); + + expect(report.reason).toBe('already_have_admin'); + // The narrow leg answers this fixture on its own, so the number is the + // unscoped population — not "up to 50, whichever the driver produced first". + expect(report.adminGrantRowsExamined).toBe(1); + }); + + it('a fresh install examines nothing and still promotes', async () => { + const engine = await boot(); + const ql = withNaturalOrder(engine, 'AS_RETURNED'); + await seedUser(ql, 'usr_first', 'first@tenant.example', '2026-02-01T00:00:00.000Z', true); + + const { logger } = collectingLogger(); + const report = await bootstrapPlatformAdmin(ql, defaultPermissionSets, { logger }); + + expect(report.adminPromoted).toBe(true); + expect(report.adminGrantRowsExamined).toBe(0); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 4. The system row still never counts — the fix did not over-tighten + // ───────────────────────────────────────────────────────────────────────── + + it('an unscoped grant held by usr_system does NOT block promotion', async () => { + // Self-healing on restart: a DB where the seed identity was wrongly + // promoted must not block every real admin forever. The scan sees the row + // and the predicate still refuses it. + const engine = await boot(); + const ql = withNaturalOrder(engine, 'AS_RETURNED'); + await ql.insert( + 'sys_permission_set', + { id: ADMIN_PS_ID, name: 'admin_full_access', label: 'Admin Full Access', managed_by: 'platform', active: true }, + { context: SYSTEM_CTX }, + ); + await ql.insert( + 'sys_user_permission_set', + { id: 'ups_system', user_id: 'usr_system', permission_set_id: ADMIN_PS_ID, organization_id: null }, + { context: SYSTEM_CTX }, + ); + await seedUser(ql, 'usr_real', 'real@tenant.example', '2026-03-01T00:00:00.000Z', true); + + const { logger } = collectingLogger(); + const report = await bootstrapPlatformAdmin(ql, defaultPermissionSets, { logger }); + + expect(report.adminPromoted).toBe(true); + expect(report.adminGrantRowsExamined).toBe(1); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 5. The bound is real, and it is never silent + // ───────────────────────────────────────────────────────────────────────── + + describe('the grant scan is bounded but never silent', () => { + /** + * A synthetic grant population — the one case whose subject is a row COUNT + * larger than any fixture worth storing. Every row is ORGANIZATION-scoped, + * which is the shape that fills the window in the first place, and the + * generator serves `offset`/`limit` so the paging is exercised for real. + */ + function makeSyntheticQl(grantCount: number) { + const permissionSets: any[] = []; + const inserted: any[] = []; + return { + inserted, + async find(object: string, q: any) { + const bound = (rows: any[]) => + typeof q?.limit === 'number' ? rows.slice(0, q.limit) : rows; + const where = q?.where ?? {}; + for (const k of Object.keys(where)) { + // Refuse loudly rather than reading a combinator as a field name — a + // matcher that answers `false` for `$or` reports a row it never + // understood as absent. + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + } + if (object === 'sys_permission_set') { + return bound( + permissionSets.filter((r) => Object.entries(where).every(([k, v]) => r[k] === v)), + ); + } + if (object === 'sys_user_permission_set') { + // Every synthetic row is organization-scoped, so the narrow leg + // (`organization_id: null`) legitimately answers with nothing. + if ('organization_id' in where && where.organization_id === null) return []; + const offset = q?.offset ?? 0; + const limit = q?.limit ?? 100; + const out: any[] = []; + for (let i = offset; i < Math.min(offset + limit, grantCount); i++) { + out.push({ + id: `ups_${String(i).padStart(6, '0')}`, + user_id: `usr_${String(i).padStart(6, '0')}`, + permission_set_id: permissionSets.find((p) => p.name === 'admin_full_access')?.id, + organization_id: `org_${i}`, + }); + } + return out; + } + if (object === 'sys_user') { + if (Object.keys(where).length > 0) return []; + const offset = q?.offset ?? 0; + return offset === 0 + ? [{ id: 'usr_promotable', email: 'p@demo.example', created_at: '2026-01-01T00:00:00.000Z' }] + : []; + } + if (object === 'sys_account') return [{ id: 'acc_p', user_id: 'usr_promotable' }]; + return []; + }, + async insert(object: string, data: any) { + if (object === 'sys_permission_set') permissionSets.push({ ...data }); + inserted.push({ object, data }); + return { id: data.id }; + }, + async update(_object: string, data: any, options?: any) { + assertEngineUpdateDispatch(data, options); + return 0; + }, + }; + } + + it('warns, naming the ceiling and the number examined, when the scan stops short', async () => { + const ql = makeSyntheticQl( + PLATFORM_ADMIN_GRANT_SCAN_CEILING + PLATFORM_ADMIN_GRANT_PAGE_SIZE, + ); + const { warn, logger } = collectingLogger(); + + const report = await bootstrapPlatformAdmin(ql as any, defaultPermissionSets, { logger }); + + // The population is entirely org-scoped, so promoting IS the right answer + // here — what must never happen is doing it silently. + expect(report.adminPromoted).toBe(true); + expect(report.adminGrantRowsExamined).toBe(PLATFORM_ADMIN_GRANT_SCAN_CEILING); + const said = warn.join('\n'); + expect(said).toContain('stopped at its ceiling'); + expect(said).toContain(String(PLATFORM_ADMIN_GRANT_SCAN_CEILING)); + expect(said).toContain('were NOT examined'); + expect(said).toContain('SECOND unscoped grant'); + }); + + it('CONTROL: a population inside the ceiling produces no truncation warning', async () => { + const ql = makeSyntheticQl( + PLATFORM_ADMIN_GRANT_SCAN_CEILING - PLATFORM_ADMIN_GRANT_PAGE_SIZE, + ); + const { warn, logger } = collectingLogger(); + + const report = await bootstrapPlatformAdmin(ql as any, defaultPermissionSets, { logger }); + + expect(report.adminPromoted).toBe(true); + expect(report.adminGrantRowsExamined).toBe( + PLATFORM_ADMIN_GRANT_SCAN_CEILING - PLATFORM_ADMIN_GRANT_PAGE_SIZE, + ); + expect(warn.join('\n')).not.toContain('stopped at its ceiling'); + }); + }); +}); diff --git a/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts b/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts index a36b32f72e..9ae10b0b3d 100644 --- a/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts +++ b/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts @@ -578,7 +578,18 @@ export async function bootstrapPlatformAdmin( const isUnscopedHumanHolder = (r: any) => !r.organization_id && r.user_id !== SystemUserId.SYSTEM; - let adminGrantRowsExamined = 0; + // Counted by row IDENTITY, not by read: the two legs overlap by construction + // (leg A's rows are a subset of leg B's population), and a number that + // double-counted them would answer "how many reads did you make" while + // calling itself rows examined. + const examinedGrantRowIds = new Set(); + const countExamined = (rows: any[]) => { + for (const r of rows) { + examinedGrantRowIds.add( + r?.id === undefined || r?.id === null ? `?${examinedGrantRowIds.size}` : String(r.id), + ); + } + }; let adminGrantScanTruncated = false; // Leg A — the narrow question, asked of the driver. @@ -589,7 +600,7 @@ export async function bootstrapPlatformAdmin( PLATFORM_ADMIN_GRANT_PAGE_SIZE, ADMIN_GRANT_SCAN_ORDER, ); - adminGrantRowsExamined += unscopedGrantRows.length; + countExamined(unscopedGrantRows); let unscopedHolder: any | undefined = unscopedGrantRows.find(isUnscopedHumanHolder); // Leg B — the ordered, bounded scan that still applies the exact predicate. @@ -607,7 +618,7 @@ export async function bootstrapPlatformAdmin( offset, ); if (page.length === 0) break; - adminGrantRowsExamined += page.length; + countExamined(page); unscopedHolder = page.find(isUnscopedHumanHolder); if (unscopedHolder) break; if (page.length < pageLimit) break; @@ -621,6 +632,7 @@ export async function bootstrapPlatformAdmin( // grant and hands it the seeded business records. So it says the number it // examined rather than letting the promotion below read as a statement about // the whole table. + const adminGrantRowsExamined = examinedGrantRowIds.size; if (adminGrantScanTruncated && !unscopedHolder) { const truncation = '[security] the existing-platform-admin check stopped at its ceiling of ' From 57421b4ae457394bf1975c06c52b50820433a9d2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 10:44:27 +0000 Subject: [PATCH 3/3] 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 Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --- ...latform-admin-existing-holder-scan.test.ts | 19 ++++++++++++------- scripts/engine-double-contract.pinned.json | 5 +++++ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/packages/plugins/plugin-security/src/bootstrap-platform-admin-existing-holder-scan.test.ts b/packages/plugins/plugin-security/src/bootstrap-platform-admin-existing-holder-scan.test.ts index e9169f0c5c..ee8362b74a 100644 --- a/packages/plugins/plugin-security/src/bootstrap-platform-admin-existing-holder-scan.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-platform-admin-existing-holder-scan.test.ts @@ -451,17 +451,22 @@ describe('#16861 — an existing platform admin is found, not sampled for', () = const bound = (rows: any[]) => typeof q?.limit === 'number' ? rows.slice(0, q.limit) : rows; const where = q?.where ?? {}; - for (const k of Object.keys(where)) { - // Refuse loudly rather than reading a combinator as a field name — a - // matcher that answers `false` for `$or` reports a row it never - // understood as absent. - if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); - } if (object === 'sys_permission_set') { return bound( - permissionSets.filter((r) => Object.entries(where).every(([k, v]) => r[k] === v)), + permissionSets.filter((r) => + Object.entries(where).every(([k, v]) => { + // Refuse loudly rather than reading a combinator as a field + // name — a matcher that answers `false` for `$or` reports a + // row it never understood as absent. + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + return r[k] === v; + }), + ), ); } + for (const k of Object.keys(where)) { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + } if (object === 'sys_user_permission_set') { // Every synthetic row is organization-scoped, so the narrow leg // (`organization_id: null`) legitimately answers with nothing. diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 9a100fabb3..2aae71a29b 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2656,6 +2656,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-security/src/bootstrap-platform-admin-existing-holder-scan.test.ts", + "verb": "update", + "pinned": 2 + }, { "file": "packages/plugins/plugin-security/src/bootstrap-platform-admin-promotion-selection.test.ts", "verb": "update",