fix(plugin-security): fail closed on a non-object row in the platform-admin promotion predicate

.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.

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<string, string | undefined> = {};
   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

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;