diff --git a/.changeset/human-user-predicate-fail-closed.md b/.changeset/human-user-predicate-fail-closed.md new file mode 100644 index 0000000000..3257f1d594 --- /dev/null +++ b/.changeset/human-user-predicate-fail-closed.md @@ -0,0 +1,48 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): fail CLOSED on a non-object row in the platform-admin promotion predicate (#12515) + +`bootstrapPlatformAdmin`'s local `isHumanUser` decided "is this `sys_user` row a +HUMAN?" with a bare truthiness check followed by two property comparisons: + +```ts +const isHumanUser = (u: any) => u && u.id !== SystemUserId.SYSTEM && u.role !== 'system'; +``` + +On a truthy NON-object input (`'usr_alice'`, a number, `true`) both comparisons +read `undefined` and therefore both pass, so the input scored **human**. The +same question's consolidated owner — `isHumanUserRow` in `@objectstack/plugin-auth` +— requires `typeof row === 'object'` and answers **non-human** for those inputs. +Two owners of one question, disagreeing, and the disagreement fell the wrong way +on the security-critical side: this is the copy that performs the +**platform-admin promotion**, so it failed OPEN. Its worst shape is the system +account's own id arriving as a bare string, which the old spelling would have +promoted. + +The predicate now mirrors `isHumanUserRow` — the same `typeof` guard, and a real +boolean return instead of echoing a falsy input back: + +```ts +const isHumanUser = (u: any) => + !!u && typeof u === 'object' && u.id !== SystemUserId.SYSTEM && u.role !== 'system'; +``` + +**Why mirroring rather than a stricter rule of its own.** Over-tightening this +predicate has a worse failure mode than the bug: an install that cannot promote +its first admin is locked out of itself. The guard was therefore measured before +it was chosen, not after. Against a real `SqlDriver` over the shipped `SysUser` +declaration, every row a real `sys_user` read yields is a plain object — zero +truthy non-objects, and zero rows whose verdict moves when the guard is added. +The mirrored guard is also already the incumbent on this exact population: +`plugin-auth`'s dev-admin seed filters the byte-identical read (`sys_user`, +`where: {}`, `limit: 50`, system context) through `isHumanUserRow` today. + +**No reachable behaviour changes.** The divergence is unreachable through any +live call site, so this ships as a hardening of malformed-input handling rather +than a behavioural fix. The 14 existing agreement cases in the cross-package +pin are byte-for-byte unmoved; the pin gains the non-object class it previously +had to exclude (it would have failed), which is what now stops the asymmetry +returning — consolidating the two copies into a shared package stays declined, +so nothing else was going to retire it. diff --git a/packages/plugins/plugin-auth/src/human-user-predicate-agreement.pin.test.ts b/packages/plugins/plugin-auth/src/human-user-predicate-agreement.pin.test.ts index a01053d6dd..c5c71b7712 100644 --- a/packages/plugins/plugin-auth/src/human-user-predicate-agreement.pin.test.ts +++ b/packages/plugins/plugin-auth/src/human-user-predicate-agreement.pin.test.ts @@ -31,6 +31,19 @@ * the legacy `usr_system` service row (`SystemUserId.SYSTEM` — no longer * provisioned, but present in every DB an older runtime created). * + * ## Two populations, held apart on purpose + * + * {@link CORPUS} is the REACHABLE one: every entry is a shape a `sys_user` + * read can really return, so a failure there is a live defect. + * {@link NON_OBJECT_CORPUS} is the unreachable one — truthy non-objects, which + * no real read yields. It was originally left out of this file because the two + * owners genuinely disagreed on it and it would have failed; [#12515] closed + * that disagreement by giving plugin-security the same `typeof` guard + * `isHumanUserRow` already had, which is what made the class pinnable. The two + * stay in separate arrays so the arrays keep saying different things: a red in + * `CORPUS` means a reachable answer moved, a red in `NON_OBJECT_CORPUS` means + * the fail-closed guard was dropped. + * * ## Why the pin lives in plugin-auth and not in plugin-security * * Reaching both predicates from one test is a package-boundary problem, and @@ -184,6 +197,59 @@ const CORPUS: { name: string; row: unknown }[] = [ { name: 'an undefined row', row: undefined }, ]; +/** + * The NON-OBJECT input class — held separately from {@link CORPUS} on purpose. + * + * ## Why it is a second array and not four more corpus entries + * + * `CORPUS`'s contract is that every entry is a shape a `sys_user` read can + * really return, and these are not: a real read yields plain objects, measured + * against a real `SqlDriver` over the shipped `SysUser` declaration. Filing + * them into `CORPUS` would quietly falsify that promise and blur the one + * distinction that decides how a failure here should be read. + * + * ## Why it is pinned at all, given it is unreachable + * + * This class is the gap the original pin deliberately left: it was excluded + * because at the time it would have FAILED, not because it was uninteresting. + * The two owners genuinely disagreed on it — `isHumanUserRow` requires + * `typeof row === 'object'` and answered `false`, while plugin-security's + * hand-spelled copy ran a bare truthiness check whose two property comparisons + * are both `undefined` on a non-object and therefore both pass, answering + * `true`. That direction fails OPEN on the copy that performs the + * platform-admin promotion. + * + * Unreachable-today would be a fine reason to shrug if the asymmetry had a + * scheduled end. It does not: consolidating the predicate into a package both + * plugins depend on stays declined (it would widen a published surface), so + * nothing is going to delete this divergence on its own. The guard closed it + * instead, and this group is what stops it coming back — if a refactor ever + * makes a non-object row reachable, or if the guard is dropped as noise, these + * cases are the only mechanism that says so. Without them the pin sits green + * through exactly the edit that reopens the hole. + * + * Both owners must answer NON-HUMAN here. That is the fail-closed direction, + * and for a promotion predicate the safe answer to malformed input is "no". + */ +const NON_OBJECT_CORPUS: { name: string; row: unknown }[] = [ + { + name: 'a bare id STRING where a row was expected', + row: 'usr_alice', + }, + { + name: "the SYSTEM account's own id as a bare string — fail-open would promote the service account", + row: SystemUserId.SYSTEM, + }, + { name: 'a number', row: 42 }, + { name: 'the boolean true', row: true }, + { + name: 'a function — truthy, and every property read on it is undefined', + row: () => 'not a row', + }, + { name: 'the number zero — falsy, so the decision already agreed', row: 0 }, + { name: 'an empty string — falsy, so the decision already agreed', row: '' }, +]; + describe('human-user predicate agreement — plugin-security `isHumanUser` vs plugin-auth `isHumanUserRow`', () => { const saved: Record = {}; const PINNED_ENV = ['OS_TENANCY_POSTURE', 'OS_PLATFORM_OWNER_EMAIL']; @@ -239,6 +305,56 @@ describe('human-user predicate agreement — plugin-security `isHumanUser` vs pl expect(CORPUS.map(({ row }) => isHumanUserRow(row)).some((v) => !v)).toBe(true); }); + describe('the non-object input class — unreachable today, and fail-CLOSED on both sides', () => { + for (const { name, row } of NON_OBJECT_CORPUS) { + it(`agrees on ${name}`, async () => { + const authSays = isHumanUserRow(row); + const security = await securityVerdict(row); + + // Stated as an absolute, not just as agreement: two predicates could + // agree by both failing OPEN, which is the outcome this group exists + // to forbid. `isHumanUserRow` is asserted false first so a regression + // in the OWNER cannot be laundered into "well, they still agree". + expect( + authSays, + `plugin-auth isHumanUserRow must answer NON-HUMAN for a non-object row.\n` + + ` row: ${String(row)} (typeof ${typeof row})`, + ).toBe(false); + + expect( + security.human, + `plugin-security and plugin-auth disagree on a NON-OBJECT row — the security\n` + + `copy is failing OPEN on malformed input, and it is the copy that PERFORMS\n` + + `platform-admin promotion.\n` + + ` row: ${String(row)} (typeof ${typeof row})\n` + + ` plugin-auth isHumanUserRow -> ${authSays}\n` + + ` plugin-security isHumanUser -> ${security.human} (reason: ${security.reason ?? 'none'})\n` + + `The fix is the \`typeof\` guard in bootstrap-platform-admin.ts, mirroring\n` + + `isHumanUserRow — not a relaxation of this expectation.`, + ).toBe(false); + + // Same anti-vacuity guard the reachable corpus uses: only the human + // filter reaches `no_users`, so this proves the negative came from the + // predicate rather than from a harness that broke earlier. + expect(security.reason, 'negative verdict did not come from the human filter').toBe( + 'no_users', + ); + }); + } + + it('anti-vacuity: this group really carries truthy non-objects, not just falsy ones', () => { + // A falsy row is non-human on both sides even with the guard removed, so + // a group that had quietly lost its truthy members would keep passing + // through the very regression it is here to catch. + const truthyNonObjects = NON_OBJECT_CORPUS.filter( + ({ row }) => Boolean(row) && typeof row !== 'object', + ); + expect(truthyNonObjects.length, 'no truthy non-object rows left in the group').toBeGreaterThan( + 0, + ); + }); + }); + it('the legacy usr_system row alone leaves the install with NO admin and awaiting a human', async () => { // The card's harm model, stated as an outcome rather than a predicate call: // a DB carrying only the legacy service row must be "no humans yet" on BOTH diff --git a/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts b/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts index 4ca4c6abe9..945365e675 100644 --- a/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts +++ b/packages/plugins/plugin-security/src/bootstrap-platform-admin.ts @@ -398,7 +398,26 @@ export async function bootstrapPlatformAdmin( // it is the earliest user and steals the platform-admin promotion, leaving // the real admin login without `setup.access` / `studio.access` (Setup and // Studio then stay invisible even though login succeeds). - const isHumanUser = (u: any) => u && u.id !== SystemUserId.SYSTEM && u.role !== 'system'; + // + // [#12515] The `typeof` guard mirrors `isHumanUserRow` + // (`plugin-auth/src/audience-posture.ts`) — the #11767-consolidated owner of + // this same question — rather than inventing a stricter rule of its own. + // Without it a truthy NON-object input (`'usr_alice'`, a number, `true`) + // scores HUMAN here: `.id` and `.role` are both `undefined` on a non-object, + // so both comparisons pass. `isHumanUserRow` calls that same input non-human, + // and this is the copy that PERFORMS the platform-admin promotion — so the + // divergence failed OPEN on the security-critical side, which is why this + // copy moves rather than the other. `!!` completes the mirror: both now + // return a real boolean instead of echoing a falsy input back. + // + // Direction checked before tightening, because over-tightening here would + // mean an install unable to promote its first admin: every row a real + // `sys_user` read yields is a plain object, so the guard changes no + // reachable answer. The identical read (`sys_user`, `where: {}`, `limit: 50`, + // system context) is already filtered by `isHumanUserRow` in `plugin-auth`'s + // dev-admin seed, so this guard is the incumbent on this very population. + const isHumanUser = (u: any) => + !!u && typeof u === 'object' && u.id !== SystemUserId.SYSTEM && u.role !== 'system'; const oldestOf = (users: any[]) => [...users].sort((a, b) => { const ta = a.created_at ? new Date(a.created_at).getTime() : 0;