fix(plugin-security): choose the platform-admin promotion target instead of sampling it — order the candidate read server-side and prefer the declared owner - #16863
Conversation
…ead of sampling it The `single`-posture first-boot promotion read `sys_user` with no `orderBy` and a cap of 50, then sorted that array client-side. So "the oldest authenticable user" meant the oldest authenticable user among whatever 50 rows the driver produced first, and a client-side sort cannot notice: it sorts a sample and reports a global answer. Measured on 113 seeded users with the intended owner inserted first, holding the oldest created_at and an id that collates last: the in-memory driver returned it in row 1 and promoted it; the default sqlite driver returned id order, never saw it, and gave the unscoped admin_full_access grant — plus, through claimSeedOwnership, ownership of every seeded business record — to a seeded job-seeker persona. Same code, same config, same data. PLATFORM_OWNER_EMAIL_ENV was imported into this same file and read only on the walled branch, so a deployment that had declared its owner could still have somebody else promoted. Both halves land together: ordering alone still promotes someone the operator never chose, and honouring the declaration alone leaves the no-declaration path sorting a truncated unordered sample. - the candidate read carries orderBy created_at asc, id asc, and no client-side re-sort is left behind - a declared, authenticable, human holder of an OS_PLATFORM_OWNER_EMAIL address is preferred; verified matches rank ahead of unverified ones - a declared owner nobody can sign in as REFUSES loudly and promotes nobody, never falling back to whoever happens to be oldest - the bare cap 50 becomes a 200-row page with a 5000-row ceiling walked oldest-first, and reaching the ceiling warns with the number examined - the promotion log line and the returned report record the basis and the candidate-pool size Unchanged: no declaration still means promotion by age; a user nobody can authenticate as is still never promoted (#14348); an existing unscoped grant still short-circuits before any selection runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
…rst-user-promotion-selection
…ared engine-double contracts The three doubles the new selection guard introduces are brought up to the contracts the repo's gates enforce, and the pinned ledger is grown so it actually protects this file: - update() routes through assertEngineUpdateDispatch, so a fixture drifting to a call shape ObjectQL.update would refuse fails loudly (check:engine-double-contract) - find() applies the caller's limit AFTER the filter and by presence (check:objectql-double-limit) - the WHERE matchers refuse a $-combinator by name instead of reading it as a field and answering false (check:where-matcher) scripts/engine-double-contract.pinned.json gains the new seams via `--write`; the shrink-only baseline is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
…rst-user-promotion-selection
📓 Docs Drift CheckThis PR changes 1 package(s): 9 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 5 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 15 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 3a12d9d825fea3ab65bbc7164a1d6c3eac38efcc && git checkout 3a12d9d825fea3ab65bbc7164a1d6c3eac38efcc
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 12babac137cc826fa5ed888ce63c266c4d219ce3 141876814df6763fca3b71f0b1ed970ad196869c && git checkout -B drift-repro 12babac137cc826fa5ed888ce63c266c4d219ce3 && git merge --no-ff 141876814df6763fca3b71f0b1ed970ad196869c
node scripts/docs-audit/affected-docs.mjs --json 12babac137cc826fa5ed888ce63c266c4d219ce3
|
Contract review (
|
Fixes #16682
The
single-posture first-boot promotion readsys_userwith noorderByand a cap of 50, then sorted that array client-side. So "the oldest authenticable user" meant the oldest authenticable user among whatever 50 rows the driver produced first — a sample sorted and reported as a global answer. AndPLATFORM_OWNER_EMAIL_ENV, imported into that same file, was read only on the walled branch, so a deployment that had declared its owner could still have someone else promoted.Both halves land together, per triage's ruling: ordering alone still promotes someone the operator never chose, and honouring the declaration alone leaves the no-declaration path sorting a truncated unordered sample.
Measured, on this branch, both drivers
113 seeded
sys_userrows, 7 holding credentials, the intended owner inserted first with the oldestcreated_atand an id that collates last,OS_PLATFORM_OWNER_EMAIL=admin@objectos.ai:InMemoryDriver)window[0] = usr_zzz_owner(insertion order)admin@objectos.aiadmin@objectos.aiSqlDriver, the default)window[0] = usr_ats_c001— the owner is not in the window at all (id order)candidate001@mail.exampleadmin@objectos.aiRe-measured with
OS_PLATFORM_OWNER_EMAILunset as well (the pure ordering leg): before, the same split; after,admin@objectos.aion both drivers,basis: oldest-authenticable. The raw unordered 50-row window is still driver-shaped after the change — that is a fact about the drivers, and the file pins it as an anti-vacuity case. What is no longer driver-shaped is the answer.What changed
created_atascending withidas tie-breaker (seeded populations routinely share one timestamp, and among ties an unordered read is exactly the sample-dependent answer this fixes). There is deliberately no client-side re-sort left behind: one would re-rank the returned page and keep the guard passing if the ordering were ever lost again.OS_PLATFORM_OWNER_EMAILaddress is the target.isHumanUserstill applies, so a declared address on arole: 'system'row is not a route to the grant.reason: 'declared_owner_not_authenticable', a warning naming the variable and the addresses, and no grant row at all. No silent fall-back to whoever happens to be oldest; that is the outcome the card is about. The replay predicate promotes the declared owner as soon as their login exists.matchesConfiguredPlatformAdminstates the threat for the walled derivation — "an attacker who registers the operator's address before the operator does gains no standing by it" — and this answers it by ORDER rather than by refusal: somebody who cannot read the operator's mailbox cannot verify. It is a preference, not a requirement, becausesinglehas no verification vocabulary (bootstrap-platform-admin-walled-owner.test.tspins that an unverified first user is still promoted here), and makingsingleverification-keyed is a policy change, not a repair.The cap's disposition
Replaced, not merely raised, and never silent again. The bare
50becomesPLATFORM_ADMIN_CANDIDATE_PAGE_SIZE = 200walked oldest-first with a hardPLATFORM_ADMIN_CANDIDATE_SCAN_CEILING = 5000.sys_user/sys_accountinsert until an admin exists (shouldReplayBootstrapFor), so an unbounded scan would be a per-sign-up full-table read on exactly the deployments that have not been promoted yet.orderBy: an ORDERED page holds the OLDEST rows, which is exactly the set the age rule ranks, so truncation can only bite when every one of the oldest 5000 humans is non-authenticable. The unordered50could drop the answer on a 51-row install.The upgraded log line — exact text
Prefix unchanged (existing readers match on it); the basis and the pool are appended, and repeated in
metaasbasis/candidatePoolSize/userIdfor structured sinks. The returned report carriesbasistoo.Truncation warning:
验收备注
Triage's rubric, copied, each condition with its evidence. All in
packages/plugins/plugin-security/src/bootstrap-platform-admin-promotion-selection.test.ts(23 cases) unless noted.idand first bycreated_at, then assert the same user is promoted across driver row orders. ✅ The113-row fixture is the card's, verbatim.AS_RETURNED,INSERTION,REVERSED), all on the real engine over the real better-sqlite3 driver, plus a case asserting all three agree. An anti-vacuity case proves the unordered 50-row window really does hide the owner on the real driver, so the file cannot pass for a reason unrelated to the repair.OS_PLATFORM_OWNER_EMAILset to a user that is neither the oldest (itscreated_atis the newest of 113) nor in the first 50 rows (its id collates last); asserted promoted under all three orders, withbasis: declared-owner, and explicitly not the oldest authenticable human. ✅OS_PLATFORM_OWNER_EMAIL时回落到最老的可认证用户,且两驱动一致 — a fixture deliberately different from condition 1, where the oldest authenticable row is not the id-last row, so "oldest wins" and "the owner happens to sort last" are separated. Same answer under all three orders. ✅min(created_at)fails here while condition 1 stays green. And a 60-row population where nobody can authenticate promotes nobody (no_authenticable_user, zero grant rows, and no warning, because that population is far under the ceiling). ✅OS_PLATFORM_OWNER_EMAIL但该邮箱没有可用账号 — two sub-cases, nosys_userrow at all and a row with nosys_account. Both refuse:declared_owner_not_authenticable, a warning naming the variable, the address and the phrase "NOT falling back to the oldest", and zero grant rows written. ⛔ No silent fall-back. ✅already_have_adminbefore any selection runs, so a declared owner cannot re-point an existing platform admin. ✅metafields, and that the published prefix still matches. ✅:426那处精确读 — untouched.git diffover the whole change contains no edit to that line. ✅Declared deviation: the memory-driver arm is measured, not pinned
Condition 1 asks for the assertion on the memory driver and sqlite.
@objectstack/driver-memorycannot be imported into this suite, for two reasons that are each outside a repair's authority:packages/plugins/plugin-security/package.json— claimed by open PR fix(plugin-security)!: evaluate the insert-side RLScheckon the row that will be stored, afterbeforeInsert#16805 under the single-writer rule, which the dispatch names as a stop-and-report.scripts/driver-memory-census.ledger.json, and a test consumer's only fitting axis isruled-permanent, which the ledger states is "a maintainer ruling and lives inruledConsumers; nothing else may claim it." The gate's own header says a third arrival "is now refused at the gate".So the real memory-driver readings are in the table at the top of this PR — taken out-of-tree on this branch, before and after, and reported here rather than pinned. What the suite pins in their place is the property those two drivers were standing in for, over more orders than they produce between them, with the un-permuted arm being an ordinary real-driver run. If the maintainer wants the literal two-driver arm, it needs the ledger entry and the manifest edit, and both are theirs to make.
The facade is honest by construction: it permutes a result only when the query carried no
orderBy— which is precisely the freedom a driver has there — and forwards an ordered query verbatim, returning the real SQL engine's rows untouched. It never sorts. So a fix that sentorderByto a driver that ignored it would still be caught.A pin was re-authored, and it deserves a maintainer's eye
bootstrap-platform-admin-walled-owner.test.tscarried a case asserting the exact behaviour triage ruled defective:It is #11974's over-denial guard, and what it guards is unchanged: retiring the WALLED write must not retire the
singleone —adminPromoted === truewith a grant row actually minted, still asserted. What changed is the incumbent it happened to snapshot alongside that invariant, and the file header's "byte-for-byte" wording. The case is re-authored with the ruling that replaced it, quoted verbatim in the test:Flagging it explicitly because it is a collision between two rulings — #11974 / Choice 4A (2026-08-25) and #16682's triage (2026-09-08) — resolved in favour of the newer one because the dispatch carries it as binding.
Security boundary
This narrows who receives
admin_full_access, which is the ruled direction. Three things worth naming rather than leaving to be found:email_verified, for the reason above; it prefers it. Relative to the incumbent this is not a widening — today's rule hands the grant to whoever registers first, with no config knowledge required at all — but it is a deliberate choice and the maintainer's to overrule.packages/cli'sos meta resynccalls this function and therefore inherits the declared-owner preference. Intended, and consistent.Verification
Final head
141876814, on a tree merged withorigin/main(no STALE TREE warning from the deriver).pnpm --filter @objectstack/plugin-security test— 103 files, 1925 tests, all passing.pnpm --filter @objectstack/plugin-security typecheck— clean, includingcheck:test-typecheck(0 files / 0 errors / 0 pinned signatures in debt).pnpm lint— the full repo-wideeslint . --no-inline-config, exit 0. Not narrowed, so no narrowing evidence is owed.dispatch-gates --ran, reconciled). 63 exit 0. The three non-zero are all exit 3, PREREQUISITE NOT MET —check:dual-build-cjs-loads,check:i18n,check:type-check-debteach require a full workspace build that CI does before them, and each says in its own words that this is "NOT a pass" and nothing was measured. Zero findings (exit 1).check:engine-double-contract(update()now routes throughassertEngineUpdateDispatch; the pinned ledger grew via--write, the shrink-only baseline untouched),check:objectql-double-limit(the caller's bound applied after the filter, by presence),check:where-matcher(the matchers refuse a$-combinator by name instead of reading it as a field).check:route-envelope— does not apply, measured not assumed: the whole change adds0lines matchingc.json(orres.json(.docs/adr/**,.claude/**,skills/**,AGENTS.md,CLAUDE.md): none touched, so no maintainer-speed-read section is owed here.packages/plugins/plugin-security/package.json,tsconfig.jsonorvitest.config.ts— the three files PR fix(plugin-security)!: evaluate the insert-side RLScheckon the row that will be stored, afterbeforeInsert#16805 claims. The suite reaches the real driver through devDependencies and vitest aliases that already existed.Clause ② derivation: no
node scripts/pm/check-widening-tells.mjs --declaration no --diff PR.DIFF→ exit 0: "4 changed file(s) read, no widening tell on any declared surface."node scripts/pm/dispatch-gates.mjs --tier→ "no path-derived mandate: the surface hits none of the 3 declared glob(s)".basisfield on a returned object — additive, and no consumer can be rejected by it.needs:contract-reviewis therefore not hung. Say the word and it goes on both carriers.Because the change is additive on a cross-package type, the "inject a key the new type rejects" recipe has nothing to reject. The equivalent two-legged proof was run against
packages/cli, the one package importing this function as a value, after rebuildingplugin-security:report.basisinto'declared-owner' | 'oldest-authenticable' | undefinedproduced 0 errors on the probe file, so the consumer read the rebuilt.d.tsand not a cache;report.basistonumberproducederror TS2322: Type 'string | undefined' is not assignable to type 'number', so the type is live and rejecting. (The package's 111 pre-existing errors are unbuilt sibling packages, identical on both legs; the probe was removed andgit statusforpackages/cliis clean.)Ablation — four enforcement points, each proven able to fail
Each leg mutated one point, proved the mutation reached disk (anchor
grep -c1 to 0, injected-text count 0 to 1, andgit hash-objectdiffering from the HEAD blob), ran the guard, then restored withgit checkout HEAD -- ABSPATHunder atrap ... EXIT INT TERMand proved the restore (blob equal to the HEAD blob and an emptygit diff HEAD).OLDEST_FIRSTfrom the candidate readAS_RETURNED, the un-permuted real-driver armscanTruncatedbranch unreachableA fifth attempt was refused by the harness, not by me: replacing the log fragment with an empty string makes the injected-text count unobservable (
grep -c ""matches every line), so the run was aborted as a possible no-op and redone with a countable marker. The tree is at HEAD withgit statusclean and the suite green after all four legs.Out of scope, filed
already_have_adminshort-circuit readssys_user_permission_setwith an UNORDERED cap of 50, so an existing unscoped platform admin can be missed and a SECOND one minted #16861 — the same defect class on the other read in this function:already_have_adminreadssys_user_permission_setwith an unordered cap of 50 and applies the deciding!organization_idpredicate client-side, so on an install with 50-plus organization-scoped grants ofadmin_full_accessthe existing unscoped holder can be missed and a second unscoped admin minted. Not folded in: triage scoped this card to the promotion read, and repairing that one changes when the short-circuit fires, which is permission-boundary behaviour needing its own tests.sys_user.emailcarries a UNIQUE index, so on the SQL family two rows can never hold one address — measured asSQLITE_CONSTRAINT_UNIQUEwhile writing the verified-first tie-break's fixture, which is why those two cases run on a double.Generated by Claude Code