Uh oh!
There was an error while loading. Please reload this page.
fix(security): platform default permission sets are stamped managed_by: 'platform' so os meta resync reconciles them (#8692) - #9129
Conversation
…latform'` so `os meta resync` reconciles them (#8692) `bootstrapPlatformAdmin` seeded the default permission sets without writing `managed_by`, so the value fell to the declaration's `defaultValue: 'admin'`. `os meta resync` reconciles only platform-owned rows, so every shipped default set took the skip branch (measured: resynced 0 / resyncSkipped 8) and each skip was logged as an "intentional override" for a row no admin had touched. Implements the maintainer ruling of 2026-08-15 (Option A): - the seed insert stamps `managed_by: 'platform'` explicitly, matching `bootstrap-builtin-positions.ts` and `bootstrap-system-capabilities.ts`; - FORWARD ONLY -- no migration, no restamp of existing 'admin' rows, since a stored 'admin' cannot be told apart from a real Setup takeover and rewriting it could silently overwrite admin edits on the next resync; - the skip warn drops its "(intentional override)" claim of intent; - the seeder docblock records what pre-ruling installs carry and why they keep the skip; - two comments claiming the posture "keeps the platform defaults env-authored -- the posture bootstrapDeclaredPermissions relies on" are removed: measured false, since bootstrapDeclaredPermissions special-cases only 'package'. The #8804 pin suite now asserts both sides: a fresh install stores 'platform' and resyncs every set, and a pre-ruling 'admin' row is still skipped with its content intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza
📓 Docs Drift CheckThis PR changes 1 package(s): 12 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also reference the affected code. These are read-only:
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#8692
Implements the maintainer ruling of 2026-08-15 (Option A): the platform default permission sets are platform-owned and resyncable. The decision is not re-opened here and the measurement is not re-run — PR #8804 delivered it (a seeded row stored
'admin'; a real resync returnedresynced 0 / resyncSkipped 8) and it is the ruling's confirmed basis.The defect
bootstrapPlatformAdminseeded the default sets without writingmanaged_by, so the value fell to the declareddefaultValue: 'admin'onsys_permission_set.os meta resyncreconciles only rows the platform still owns (managed_byabsent or'platform'), so the platform's own defaults took the skip branch — every shipped set — and each skip was logged as an "intentional override" for a row no admin had ever touched. That is the inverse of what #2705 built the flag for.The ruling's four constraints, each implemented
1. Forward-stamp only. The seed insert now stamps
managed_by: 'platform', matchingbootstrap-builtin-positions.tsandbootstrap-system-capabilities.ts, which have always stamped it. ⛔ No migration, no restamp of existing'admin'rows — a stored'admin'is indistinguishable between "the old seeder's field default" and "an administrator took this set over in Setup", so rewriting it could silently overwrite real admin edits on the next resync. Report-don't-rewrite.A detail worth naming, because it is what keeps constraint 1 true at runtime: the stamp is written at the insert call site, deliberately not added to
platformOwnedFields(). That helper is shared by the insert and the resyncUPDATE, so puttingmanaged_byin it would make every resync restamp the row it reconciles — converting a legacy'admin'row into a platform-owned one and clobbering it on the same pass, which is exactly the data loss the ruling forbids. A comment on the helper now says so.2. The warn stops lying. It read
… row is admin-owned (intentional override); it now reads… row is admin-owned— provenance and action, no claim about intent. For a pre-ruling row that claim was false: the only writer may have been this same seeder one call earlier.3. Both pins, not one. The #8804 suite now asserts both sides of the line the ruling drew — a fresh install (stores
'platform', resyncs every set, warns about nothing) and a legacy install (a pre-ruling'admin'row is still skipped, with its content intact and its provenance unrestamped).4. Recorded in the docblock. The seeder's module docblock states what pre-ruling installs carry, why they keep the skip permanently, and that re-owning the rows or
--freshis an operator's choice rather than one a boot makes for them — so the next reader finds a decision, not a mystery.The endorsed side-finding, removed with the change
The comment claiming the posture "keeps the platform defaults env-authored — the posture
bootstrapDeclaredPermissionsrelies on" was measured false and is gone.bootstrap-declared-permissions.tsspecial-cases onlymanaged_by === 'package'(its line 124); every other value —'platform'included — falls to the sameskippedEnvAuthoredbranch, so its behaviour is identical before and after this change. The same claim appeared twice in the file (the inline comment above the resync branch and theresync?option docblock); both were removed, since leaving either would keep the file asserting a reason the ruling measured false.Tests
bootstrap-platform-admin-seeded-provenance.test.tskeeps driving a realObjectQLengine over better-sqlite3 with the real shippeddefaultPermissionSets— 8 cases, up from 5:'platform', both at the driver and through the engine read the resync branch usesresynced== the shipped set,resyncSkipped0resyncSkipped1 /resyncedn-1 on an upgraded install, stale payload still staleintentional overrideabsentThe legacy pre-image is built through its original mechanism — inserted the way the old seeder wrote it, i.e. without
managed_by, sodefaultValue: 'admin'supplies the value — rather than hand-writing'admin'. Each case asserts the resulting value before relying on it, so a moved default surfaces as a failure instead of quietly turning the legacy pin into a test of something else.The ATTRIBUTION case now runs the opposite way round from the pre-ruling version, deliberately. Before, the seeder wrote no
managed_by, so respelling the declared default flipped the stored value — which is what proved the default was the source. Now the seeder stamps explicitly, so the declared default must no longer be able to move the value at all: a clone of the declaration defaulting to'package'still stores'platform'. Without that case, respellingdefaultValue: 'admin'to'platform'in the object file would make every forward assertion pass with the stamp deleted.The three hand-seeded cases in
bootstrap-platform-admin.test.tsare untouched and still green. The shippeddefaultValue: 'admin'on the declaration is also untouched — it is the right default for a set an admin creates in Setup, and the ruling changed the seeder, not the declaration.Reverse verification
Run from the committed state, direction predicted before running: delete the stamp and the forward pins go red while ANTI-VACUITY stays green. Observed exactly that — 7 failed, 1 passed:
The ATTRIBUTION line is the informative one: with the stamp gone the row falls to the clone's
'package'default, which is the direct proof that the stamp — not a declaration that happens to agree — is what supplies the value. Restored by checking the file back out of the branch (the fix was committed first, so a restore point existed); the numbers below are from the restored tree.Verification — all at
6e744ce70, the head this PR proposespnpm --filter @objectstack/plugin-security test -- --maxWorkers=2— 65 files, 1246 tests passedpnpm --filter @objectstack/plugin-security typecheck— clean, exit 0node scripts/pm/dispatch-gates.mjs(the dispatch prompt named no families, so this derivation is the list): 9 path-matched + 6 convention-triggered, all run and green —check:changeset-gate-self-tests,check:cross-package-test-inputs,check:objectui-changeset,check:test-source-alias,check:type-source-resolution,check-adr-0087-registration,check-changeset-no-major,check-empty-changeset,check:query-options-erasure,check:type-check-coverage,check:engine-double-contract,check:where-matcher,check:i18n,check:type-check-debt, pluscheck:nul-bytes.Two gates first refused to run, and a refusal was not taken as a pass.
check:i18nrefused pending an@objectstack/clibuild;check:type-check-debt --re-measurerefused pending the full workspace closure (@objectstack/service-knowledgeunbuilt). Both were built exactly as each gate's message prescribes and then ran for real:check-i18n-bundles: OK (9 package(s) — all bundles in sync)andcheck-type-check-coverage --re-measure: OK — 33 ledger entr(ies) re-measured, none above its recorded number. Exit codes were read withecho "EXIT=$?"into a log rather than through a pipe, since both scripts warn that piping reports the pipe's status.Changeset:
.changeset/platform-default-permission-sets-platform-owned.md(patch,@objectstack/plugin-security) — this changes what a fresh install stores and whatos meta resyncdoes.Generated by Claude Code