Skip to content

fix(security): the curated capability seeder reconciles the row the platform owns, not whichever row shares the name (#8470) - #8537

Merged
os-zhuang merged 6 commits into
mainfrom
claude/issue-8470-capability-seed-lookup
Aug 13, 2026
Merged

fix(security): the curated capability seeder reconciles the row the platform owns, not whichever row shares the name (#8470)#8537
os-zhuang merged 6 commits into
mainfrom
claude/issue-8470-capability-seed-lookup

Conversation

@os-zhuang

@os-zhuangos-zhuang commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes#8470

The curated half of bootstrapSystemCapabilities now looks up the row the platform owns instead of the first row that happens to share the name.

constlookup=isDerived
? {name: def.name}
: {name: def.name,managed_by: 'platform',organization_id: null};

The DERIVED half is untouched (its own #5876 guard already refuses to write to a row it does not own).


The card's mechanism was right about the defect and wrong about the cause — and the difference selects the route

The card offered two directions: make the lookup deterministic, or scope it. I took scoping, and the measurement that selected it also falsifies the other option outright rather than merely ranking it second.

The lookup was already deterministic.#4363 shipped a pagination tie-breaker: orderKeysFor appends ORDER BY id to any paged read of a driver-managed table, and limit: 1 counts as paged on both SqlDriver and MongoDBDriver — only findOne opts out, via singleRowLookup. So find('sys_capability', { where: { name }, limit: 1 }) has carried an ORDER BY id ASC this whole time.

Measured on SqlDriver + better-sqlite3, against a fixture copying sys-capability.object.ts including its unique: 'organization' index, with a platform row (NULL organization) and an org row sharing manage_users:

physical insert orderorg row idreturned
org first, platform secondcap_orgcap_org
platform first, org secondcap_orgcap_org
platform first, org secondaaa_orgaaa_org
org first, platform secondzzz_orgaaa_platform

The selection tracks id ascending and ignores insertion order entirely. I predicted rows 1-2 would differ (insertion order) and was wrong; chasing that miss is what surfaced #4363.

Two consequences:

  1. Ordering is not the remedy. It is already there. Adding an explicit orderBy would change which wrong row is chosen, not that a wrong row can be chosen. Determinism was never the missing property — ownership was.
  2. The defect is more persistent than the card says. The card expects a choice that "can differ between boots on the same database". It does not: it is stable per installation, so a boot that reconciles the wrong row keeps reconciling it on every subsequent boot rather than self-healing on some later one.

The card's headline harm — platformRowExists: false — is real and unaffected by any of this. With no platform row there is only one candidate, so ordering never mattered for that case.

Why both predicates, and why limit: 1 is now safe with no ORDER BY

managed_by: 'platform' AND organization_id: null are the two facts that jointly define the platform's own row, and together they make the result set a provable singleton: the post-#8461 unique key is (COALESCE(organization_id, …), name), so the NULL-organization bucket admits at most one row per name. limit: 1 over a set of size 0 or 1 cannot be arbitrary. That is what retires the ordering question instead of answering it.

Neither predicate alone is enough. managed_by alone loses the singleton guarantee if a platform-marked row ever sits inside an organization (seed data, legacy import). organization_id alone cannot tell the platform's row from an admin's on a single-organization deployment, where both live in the same bucket.

Measured, same harness: { name, managed_by: 'platform', organization_id: null } returns the platform row under both physical orders; returns zero rows when only an org row exists; { organization_id: null } does not match an org-scoped row (it really compiles to IS NULL); and the platform's global insert is not blocked by an org row of the same name.

⛔ The installation-wide unique index is not restored and must not be — it is #8323's cross-tenant existence oracle, closed by #8461.

The platform-bucket provenance matrix

The predicate is a conjunction, so it can also match zero rows where it should match one. One row in the platform's own bucket, varying only by managed_by. Predicted before running; all four matched.

managed_byreachable?outcome
platformthe row the seeder ownsmatched, reconciled, blockedCurated: 0, no warn
adminyes, and ordinaryblocked, row untouched, warn names managed_by='admin'
packageyes, via name promotionblocked, row untouched, warn names managed_by='package'
absentno — not engine-reachableblocked, warn says "a row carrying no managed_by value"
  • admin is not a hypothetical: organization_id auto-stamping lives in the enterprise @objectstack/organizations runtime, which is also what activates every walled posture. A deployment without it is single posture with no stamper, so every Setup-authored capability row lands in the NULL-organization bucket. That is the default community shape.
  • package arises by promotion: bootstrapDeclaredCapabilities refuses a name already in PLATFORM_CAPABILITY_NAMES, but a package that declared a name before the platform curated it left a managed_by:'package' row there — and setup.write and manage_sharing were both added to the curated set after the fact.
  • absent is not producible through the engine: managed_by is required: true with defaultValue: 'admin', and applyFieldDefaults resolves defaults on insert before the beforeInsert hooks, so an insert omitting it stores 'admin'. Pinned anyway, because the diagnostic must not assert what it cannot observe.

The open question the card asked: measured, and the filer's expectation holds

does a never-seeded curated capability break any downstream assumption?

No authorization decision is affected. Method, on origin/main at 30f1b7488d:

  • The requiredPermissions AND-gate compares string sets: PermissionEvaluator.getSystemPermissions() unions permissionSets[].systemPermissions, and normalizeRequiredPermissions reads the resource's declared strings. No row is loaded.
  • validateCapabilityReferences — the candidate the card named — resolves against PLATFORM_CAPABILITY_NAMES (the spec constant), stack declarations and seed-data records. It never reads the table, so a curated name with no row does not warn.
  • A repo-wide search for row reads of the object (find/findOne/count/aggregate against 'sys_capability', plus every consumer of its platform-object-names entry) finds exactly two production call sites — the two seeders. Neither is on an authorization path.

So the harm is a registry/Setup-listing defect, as the filer expected. The severity and release targeting the card was given still stand; nothing here needed to widen.

That sweep turned up something the card did not ask about, filed separately as #8535: sys_capability.active is read by nothing, while the Deactivate action's confirmation dialog tells the admin that grants and requirements stop resolving. Not addressed here; that issue remains open.

Anti-vacuity: ablation, prediction first

Prediction written down before each run (fix committed first, so the restore came out of a real commit). Ablation = revert only the lookup to { name }, keep the pins and the upgraded double.

pinpredictedmeasured
seeds the platform row even when an organization already holds the nameREDRED
leaves the organization's authored copy exactly as its author wrote itREDRED
reconciles the platform row and only the platform row (adverse id order)REDRED
reconciles the platform row and only the platform row (benign id order)GREENGREEN
platform-bucket row, managed_by=platformGREENGREEN
platform-bucket row, managed_by=admin / package / absentREDRED
a clean install seeds every curated name with nothing blockedGREENGREEN
is unaffected by the NUMBER of organizations holding the nameREDRED
the derived guard still fires for an ORG-authored rowGREENGREEN
all pre-existing tests, both seeder filesGREENGREEN

7 failed | 16 passed (23) — the predicted seven, no others. Prediction and measurement did not diverge, including the deliberately-green rows, which are regression controls rather than gates.

One sub-prediction inside a red pin is worth naming because it is where the information is: in the collision cases the seeded === KNOWN - 1 assertion passes coincidentally under the ablation (the old code also fails to insert that row — it overwrites the other author's instead). The assertion carrying the information there is blockedCurated.

The three-way diagnostic split was ablated separately (two-way → three-way): 1 failed | 23 passed, exactly the new pin.

How the adverse ordering was forced

Per the card's warning, the old double returned rows in insertion order, which is a property of the double and of no driver in the system. The double now models what was measured instead:

  1. limit sorts by id ascending (the 分页读取在没有 orderBy 时同样不确定:tie-breaker 只覆盖了「排了序的翻页」 #4363 tie-breaker, BINARY collation as SQLite compares);
  2. a null comparand matches null-or-absent (IS NULL in SQL, value == condition in driver-memory, null-or-missing in Mongo) — strict === matched none of them and would have made organization_id: null unsatisfiable in the double while working in production;
  3. insert enforces (COALESCE(organization_id, '__global__'), name).

The adverse case is then forced by choosing ids, exactly as a real driver would order them: the org row gets aaa_org_authored, which sorts before every seeder-minted cap_… id. zzz_org_authored is the same fixture benign. The pair is the anti-vacuity argument: the old behaviour differs between the two ids, the fixed behaviour is identical.

Each of the three double changes was measured against a real driver first; none was made to get a test to pass.

The case the fix newly declines, and does not do silently

A curated name can be held in the platform bucket by a row the scoped lookup does not match. Previously the seeder "resolved" that by overwriting the other author. Now it declines — but tryInsert swallows the engine's unique-constraint refusal, so declining would look exactly like a clean boot with a curated capability missing installation-wide. The result gains blockedCurated, counted and warned.

The warning states what it observed rather than asserting authorship, and splits three ways, because "no row came back" and "a row came back carrying no managed_by" are different observations and the first one would otherwise be described as an ordinary unstamped row:

  • no blocking row visible → says so, and flags it as worth investigating (the insert was refused, so something stopped it);
  • a row with no managed_by → says that;
  • a row with a value → names the value.

⛔ It reports the distinction and deliberately does not act on it. Adopting a differently-stamped row into the platform's identity would reverse #5876's "not provably ours resolves to leave-it-alone", and backfilling a stamp is a data migration. Both are maintainer calls; neither is made here.

Scope

Verification — re-run at the final head, 1abef9fd27

An earlier round of this PR reported a green union that was run before the last two commits existed, and CI went red on 5a877ffe for a gate that had genuinely passed earlier. Everything below is from a single run at the final commit. That failure mode is filed as #8550 and remains open.

Changeset: minor (widened return type; breaking-for-constructors recorded in the body, per the launch-window convention).


Generated by Claude Code

@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 6:47pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-security.

12 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/deployment/cli.mdx(via @objectstack/plugin-security)
  • content/docs/kernel/runtime-services/sharing-service.mdx(via @objectstack/plugin-security)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/access-recipes.mdx(via packages/plugins/plugin-security)
  • content/docs/permissions/authorization.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/explain.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/permissions-matrix.mdx(via packages/plugins/plugin-security)
  • content/docs/permissions/sharing-rules.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/index.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/packages.mdx(via @objectstack/plugin-security)
  • content/docs/ui/audience-based-interfaces.mdx(via packages/plugins/plugin-security)
  • content/docs/ui/dashboards.mdx(via @objectstack/plugin-security)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/plugin-security)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 13, 2026
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

Q1 — measured. The worst case is not engine-reachable; the diagnostic was still wrong and is fixed.

Reachability: NO for the state you described, on every path I can find.

  1. Only two writers exist. A repo-wide search for sys_capability inserts returns exactly two production call sites: this seeder, which stamps managed_by: 'platform' literally, and bootstrapDeclaredCapabilities, which stamps 'package' and refuses a curated name outright (PLATFORM_CAPABILITY_NAMES.has(cap.name) returns before any write).
  2. The field cannot be null or absent through the engine.managed_by is Field.select({ required: true, defaultValue: 'admin' }), and applyFieldDefaults resolves defaultValue on insert before the beforeInsert hooks. An insert omitting it stores 'admin' — never null. The absent-value row is only producible by an out-of-band write to the database.
  3. This object never had a legacy vocabulary.normalize-managed-by.ts enumerates the three catalogs' historical dialects and records capability as "platform / package / admin (already canonical)"; it heals sys_position (system/config/user) and sys_permission_set (user) and deliberately does not touch sys_capability. The "older seeder stamped a different word" path really did exist — for positions, not for capabilities.

Limit of that evidence, stated rather than papered over: this clone is shallow (157 commits, git rev-parse --is-shallow-repository = true), so I could not inspect revisions before the boundary and cannot fully exclude a pre-boundary seeder that stamped something else. Point 2 bounds the damage even then — such a row would read 'admin', i.e. be data-indistinguishable from an admin-authored row, which is exactly the ambiguity #5876 already ruled on: "'not provably ours' resolves to leave-it-alone, never to overwrite."

But your reading of the message was right, and that part was a real defect

The warning asserted "a row this pass does not own". The seeder cannot observe that. All it knows is: no row matched managed_by='platform' AND organization_id IS NULL, and the insert was refused. On any row carrying some other managed_by the sentence would be false, printed every boot — the exact failure shape you named.

Fixed in 1d91faf: the collision branch now does one extra read (that branch only) and reports the provenance it read:

… a row with managed_by='admin' already holds the name in the platform
(NULL-organization) bucket, and the declared unique key admits only one.

with { name, blockingRowId, blockingManagedBy } in the payload. Behaviour is unchanged — still declines, still counts.

I did not act on the distinction, only report it. Tolerate / backfill / adopt remain your and the maintainer's call, and the diagnostic now hands whoever makes it the fact it needs. A pin asserts the message does not contain does not own.

The negative row you asked for, plus the whole matrix

platform-bucket row, managed_by=%s — one row in the platform's own bucket, varying only by provenance. Predicted before running, all four matched:

managed_byreachable?outcomeablation
platform— the row the seeder ownsmatched, reconciled, blockedCurated: 0, no warnGREEN (regression control, not a gate)
adminyes, ordinaryblocked 1, row untouched, warn names managed_by='admin'RED
packageyes, via name promotionblocked 1, row untouched, warn names managed_by='package'RED
absentno (point 2 above)blocked 1, warn says "a row carrying no managed_by value"RED

Re-ablated against the new set: 7 failed / 16 passed (23) in that file — the four from before plus these three, platform green as designed.

Q2 — reachable, and more ordinary than the PR claimed

Not a hypothetical deployment shape. From security-plugin.ts, verbatim:

Auto-stamping organization_id stays with the enterprise @objectstack/organizations runtime (its Middleware A), which is also what ACTIVATES every walled posture.

So a deployment without that runtime is single posture with no stamper at all — every Setup-authored capability row lands in the NULL-organization bucket. That is the default community shape, not an edge case, and it makes admin the common trigger for blockedCurated rather than a defensive one. The fixture is relabelled accordingly.

package is a second independent trigger: bootstrapDeclaredCapabilities refuses names already in PLATFORM_CAPABILITY_NAMES, but a package that declared a name before the platform curated it left a managed_by:'package' row there — and setup.write and manage_sharing both carry comments showing they were added to the curated set after the fact.

Verification after the change

  • pnpm --filter @objectstack/plugin-security test — 54 files, 1071 passed; typecheck clean.
  • check:type-check-debt (package script, both halves) — first run exited 1, refusing to measure: 38 workspace deps had no built type entry point, and the gate declines rather than measure a different world. Built the closure it names (turbo run build --filter='./packages/*' --filter='./packages/*/*') and re-ran: EXIT=0, 33 ledger entries re-measured in 235.1s, 1969 raw tsc errors, none above its recorded number; surplus: none. No ledger grown.
  • Full union re-run after the change, all EXIT=0: nul-bytes, cross-package-test-inputs, docs-audit-scope, test-source-alias, type-source-resolution, query-options-erasure, type-check-coverage, engine-double-contract, empty-changeset, i18n, check-changeset-no-major.mjs.
  • No git stash, no force-push, protocol.ts untouched, the installation-wide unique index stays gone.

Generated by Claude Code


Generated by Claude Code

…ing row visible' from 'row with no managed_by'
@os-zhuang
os-zhuang marked this pull request as ready for review August 13, 2026 19:03
@os-zhuang
os-zhuang added this pull request to the merge queueAug 13, 2026
Merged via the queue into main with commit 5c04b2aAug 13, 2026
29 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-8470-capability-seed-lookup branch August 13, 2026 19:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-zhuang@claude