Repository navigation
fix(plugin-security): the boot heal converges on duplicated permission-set names, and a refused existence read never inserts - #22214
Conversation
…yered lazily, decline insert on refused existence read The boot heal re-resolved each drifted sys_permission_set row by NAME, so with duplicate rows only the lowest-id one was ever written and the rest were warned about on every boot. Heal the row that was read, by its id, facets only; read the layered item lazily, once per name, only when no SchemaRegistry answers. bootstrapPlatformAdmin now declines the insert when its existence read was refused and says so once per pass. Claude-Session: https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ Co-authored-by: Claude <noreply@anthropic.com>
…the refused existence read Claude-Session: https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ Co-authored-by: Claude <noreply@anthropic.com>
…-set heal Claude-Session: https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ Co-authored-by: Claude <noreply@anthropic.com>
…uble; bound the bootstrap double's find Claude-Session: https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 16 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 4919987a48ed68b73bdaed5dd6420e0e705aa666 && git checkout 4919987a48ed68b73bdaed5dd6420e0e705aa666
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7d7943dd0dfba6c98afa2200a08983ec85f9849a e41760d89c605042380022ab8e00fa27033074d4 && git checkout -B drift-repro 7d7943dd0dfba6c98afa2200a08983ec85f9849a && git merge --no-ff e41760d89c605042380022ab8e00fa27033074d4
node scripts/docs-audit/affected-docs.mjs --json 7d7943dd0dfba6c98afa2200a08983ec85f9849a
|
… gate stamps the writer on every non-system insert, and the column is readonly (objectstack-ai#22243) Fixes objectstack-ai#22201 Clause-②: no (narrowing) `granted_by` on `sys_user_permission_set` and `sys_user_position` records who wrote a grant. This PR enforces that, following the seat's ruling A in the claim: - Both columns are now `readonly: true`. - `DelegatedAdminGate.assert` first runs the authority decision (`authorize`, the old body). It then stamps the caller's `userId` into `granted_by` on every non-system insert it admits into either table, whatever the payload carried. Tenant-level admins, scoped delegates and self-delegations are stamped alike. A refused write throws before the stamp. - This one stamp (`stampWriter`) replaces the three old conditional stamps. Those ran on the delegate and self-delegation paths only, and only when the value was null. - System-context writers are unchanged. `isSystem` short-circuits the security middleware before the gate. The change does not widen who may grant or what a grant gives. It touches nothing in `packages/objectql` or `packages/spec`, and it writes no data. ## Measured: stored `granted_by` per writer case All cases ran on a real `ObjectQL` engine over `SqlDriver` (better-sqlite3), with the real `SecurityPlugin` middleware, and so the real gate, in front. OTHER is another existing `sys_user` that the caller names as the granter. - **Before** = `origin/main` 3b49318 source, read with the rig at 295745e. - **Readonly only** = option B: `readonly: true`, gate unchanged. - **After** = this branch. | writer case | table | before | readonly only | after | |---|---|---|---|---| | tenant-level admin, names OTHER | both | OTHER | OTHER | writer | | tenant-level admin, names none | both | null | null | writer | | scoped delegate, names OTHER | both | OTHER | OTHER | writer | | scoped delegate, names none | both | writer | writer | writer | | system-context insert, names OTHER | both | OTHER | OTHER | OTHER | | invitation placement `apply` (system) | `sys_user_position` | issuer | issuer | issuer | | organization-admin reconcile, triggered by a system `sys_member` write attributed to a human | `sys_user_permission_set` | that human | that human | that human | | non-system UPDATE renaming the granter (admin or delegate) | both | lands: OTHER | stripped: unchanged | stripped: unchanged | **Mechanism finding (premise correction).** Readonly alone does not change what an insert stores. On insert, the engine's create-side static readonly strip skips every `sys_` object (`staticReadonlyInsertSubject`). The card's premise was that "an explicit caller value … is stripped by `stripReadonlyFields`, so the row lands NULL". That holds on update only. On insert, the caller's value lands as sent. The gate's in-place middleware stamp is therefore what lands, and objectstack-ai#14088's hook-write record is not involved. The update strip does judge `sys_` objects, and that is what closes the update door. ## Measured: audit bucket per case (`ObjectQL.inspectDanglingReferences`) | row | readonly removed (ablation B1/B2) | after | |---|---|---| | `granted_by` names an id with no `sys_user` row | `dangling` | `provenance`; no `[integrity]` warning logged | | control: `user_id` names an id with no `sys_user` row | `dangling` | `dangling`; exactly one `[integrity]` warning | Both tables are covered. ## Family, measured one by one - **`sys_user_position.granted_by`: same writer class, same change.** Its non-system writes go through the gate, for both tenant-level admins and delegates (including self-delegation). Its system writer, invitation placement, sets the issuer. - **`sys_record_share.granted_by` (plugin-sharing): a different class, left unchanged.** - The object is `managedBy: 'engine-owned'`, so the ADR-0103 engine-owned guard refuses a user-context generic write. - Its one writer is the sharing service's grant. That writer inserts under a system context and stamps `granted_by` from the acting context's `userId`, which is null for a rule reconcile. - `GrantShareInput` carries no granter, so no caller can name one. "granted_by = writer" already holds there. ## Tests - **`granted-by-writer-provenance.test.ts` (new, 20 cases).** Each runs on the real engine with the real `SecurityPlugin`. - Non-system inserts store the writer: 2 tables × 2 callers × named or absent. - System writers keep their value: the plain system insert, invitation placement and the organization-admin reconcile. - A non-system update cannot rewrite the granter: 2 tables × 2 callers. - The audit files a deleted granter under `provenance` with no warning, beside the business-lookup control that lands in `dangling` and warns: per table. - **`delegated-admin-gate.test.ts` (+16 cases).** These pin the gate's own half: - named or absent granter, per principal and table; - batch inserts; - a refused insert is not stamped; - an update is not stamped; - a self-delegation that names another granter; - `sys_member` and `sys_position_permission_set` never gain the key. - **`write-preview-field-gate-parity.test.ts` (fixture).** The suite's `sys_user_position` stand-in now declares `granted_by`, as the shipped object does. The gate now stamps a tenant-level admin's insert, and the stand-in's missing column was refused as an unknown field before the rule that suite pins was reached. - **The platform-admin promotion.** `bootstrap-platform-admin.ts` is read only here (PR objectstack-ai#22214 is in flight), so it is not called. Its path, a plain `{ isSystem: true }` insert, is the system-insert pin. ## Ablations Each mutation went to disk through `scripts/ablation-replace.mjs`, with the anchor hitting exactly once and the blob changing. Each restore was proven by the blob hash equalling the HEAD blob and `git diff HEAD` being empty. The subject resolves through relative source imports, with `@objectstack/objectql` aliased to source, so no `dist/` was in the path. | ablation | mutation | red / green | restore | |---|---|---|---| | A | `stampWriter` keeps a supplied value (`granted_by == null` guard) | 11 red (every "names another user" pin, real-engine and unit) / 92 green | blob 8fb5c41 == HEAD | | B1 | `sys_user_permission_set.granted_by` `readonly: false` | 3 red (2 update pins, provenance pin: the row lands in `dangling`) / 17 green | blob 2bed0a5 == HEAD | | B2 | `sys_user_position.granted_by` `readonly: false` | 3 red (same three, for that table) / 17 green | blob 9df5977 == HEAD | | C | `stampWriter` call removed | 22 red (11 named, 8 absent, 3 legacy stamp pins) / 81 green | blob 8fb5c41 == HEAD | The first B1 attempt was a no-op. The replacement was a substring of the anchor, so the tool refused (`replace x1 -> x1`), and the command never ran. It was re-run with `readonly: false`, and the table above shows that run. ## Validation All readings below were taken at `b4ed57b0ad`, after merging `origin/main` (0e9371f). - **plugin-security tests.** `vitest run --maxWorkers=2` (whole package): 177 files, 3766 passed, 45 skipped, exit 0. - **plugin-security typecheck.** `pnpm --filter @objectstack/plugin-security typecheck`: exit 0. That covers the build program, the scripts program and `check:test-typecheck`, which reports 0 errors. `tsc --listFiles` over `tsconfig.test.json` lists all three touched test files. - **Build.** `turbo run build --filter=!@objectstack/docs`: 72 of 72 tasks succeeded. - **Gates.** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`, with no paths, derives 65 commands. All 65 ran, plus `pnpm --filter @objectstack/spec check:generated`, and every one exited 0. - `--ran` reconciliation, with an exit code recorded per command: 65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN. - `check-adr-0087-registration`: green, disposition `not-required (no-migration-prescription)`. - `check-changeset-no-major`: green, "This diff introduces no `major` bump". The level axis needs a PR payload, so it is left to CI. - `check:engine-double-contract`: green. - `check:i18n`: green, 9 packages in sync. - `check:dual-build-cjs-loads`: green, 106 entry points across 66 packages. - **ESLint, narrowed to the changed files.** - The 6 changed `.ts` files ran under the repo's own `eslint.config.mjs` with `--no-inline-config`. The JSON report lists 6 files, with 0 errors and 0 warnings, so none was ignored. - That config enables no type-aware linting (no `parserOptions.project`, no typed rules; see its own comment), so this diff cannot change the verdict on any file it does not touch. - The repo-wide `pnpm lint` is left to CI. ## Acceptance notes - **The supplied value is replaced, not refused.** A non-system insert that names a granter gets the writer, which shows in the returned row. On insert there is no `droppedFields` entry or strip warning for it, because the create-side strip does not run on `sys_` objects. - **One description is now incomplete.** `sys_user_position.granted_by` still says "(stamped by the delegated-admin gate for delegate writes)". That is still true, but it no longer covers every case. Rewording it would re-key the translated bundles in four locales, so it is left alone. The docs pages under `content/docs/permissions/` describe the delegate stamp, which also still holds. - **No stored row is rewritten.** The production row's sentinel is out of scope (triage ruling); clearing it is cloud's maintainer's write. - **Dogfood is declared to CI and was not run locally.** The dogfood suites that read `granted_by` expect the writer or a truthy value (`delegation-of-duty`, `showcase-permission-zoo`, `membership-actor-attribution`), which this change keeps. --- _Generated by [Claude Code](https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Fixes #22169
Clause-②: no
What was wrong (measured on
mainat6ed0c0f3e5)All three mechanisms the card names were confirmed on current
mainbefore any edit, with an in-repo reproduction: a realObjectQLover a realSqlDriver(better-sqlite3, in memory), a realObjectStackProtocolImplementation, and the per-organization unique index dropped after schema sync. Dropping it gives the shape of a remote table built before the index retrofit, the only shape that can hold duplicates.reconcilePermissionSetProjectionhanded each drifted, overlay-less record toupsertEnvPermissionSet, which re-read the record by name (first row, lowest id). With 8 declared names × 3 rows, boot 1 wrote 8 rows, logged 24 drift warnings and left 16 rows drifted. Boot 2, boot 3 and every later boot logged 16 warnings and wrote nothing.getMetaItemLayeredwas awaited for every candidate row before the trust rule decided whether it needed the answer. While aSchemaRegistryis readable, the rule never uses it. One layered read is 4 statements on the real protocol, so the shape above paid 24 × 4 per boot.bootstrapPlatformAdminreads each default set by name through atryFindthat answers[]for a refused read, and it inserted on both "absent" and "refused". The heal's by-name upsert had the same insert branch. When by-name reads were refused, the unpatched heal inserted 24 rows (24 → 48) and the bootstrap inserted 2 more.The change (
@objectstack/plugin-securityonly: 2 source files)permission-set-projection.ts,reconcilePermissionSetProjectionheal branch:tryUpdatewith{ id: row.id, ...permissionSetRowFields(effective) }, the same columns the upsert's update leg wrote. Identity,active,customizedand provenance are not touched. The heal no longer asks an existence question, so it has no insert branch.id, and a refused write is logged as refused instead of "re-projected".driftHealedcounts landed writes, now one per drifted row.SchemaRegistryanswers, and the answer is cached per name for the pass. The cache entry is dropped after this pass backfills that name, because the backfill changes the answer. A later row of that name then sees the backfilled definition and heals to it, instead of backfilling a second time.bootstrap-platform-admin.ts, the default-set seed loop:tryFind's existingonRefusalobserver tells "refused" apart from "empty".warnline per pass, not one per set, naming the sets. The line goes throughreportThroughSink, the derivation the sibling seeders' unreadable lines use. Whenadmin_full_accessis one of the declined sets, the promotion half returnsadmin_permission_set_unreadableinstead ofadmin_permission_set_missing. Nothing in the repository branches on that string; the only reads are doc comments.patchchangeset for@objectstack/plugin-security.Not changed (card scope):
upsertEnvPermissionSet's per-item read on the live single-mutation path is unchanged. A refused read there still inserts.packages/spec, no new gate.Measurement: statements per boot, in-repo reproduction
This was measured with the harness above on the fix at
5ccce70966(source unchanged since; later commits touch tests and the changeset only). Each count is every SQL statement knex emitted duringreconcilePermissionSetProjection. The harness was not committed.main6ed0c0f3e5On the healing boot, each by-id
UPDATEcosts the engine one extraSELECT. That is the 52 − 4 = 48 selects for 24 updates.⛔ The hosted timing is not measured here. The card's cloud reproduction (cloud's
ArtifactKernelFactoryon the hosted Turso face, ~36 ms per statement, 99.85 s / 170.70 s) cannot be reached from this session, and no cloud time is claimed. The before/after boot times on that reproduction belong to the cloud seat.Pins (new) and ablations
Pins:
permission-set-projection-heal-by-id.test.ts(real engine, spied and never replaced; index dropped). Five cases:bootstrap-platform-admin-refused-existence-read.test.ts:admin_permission_set_unreadable.permission-set-projection.test.tsgains one case: a just-backfilled name is re-read and the second row heals to the backfilled body, with onesaveMetaItem, not two.Ablations. Each leg ran on
e41760d89cthroughscripts/ablation-replace.mjsin WRAP mode, with the anchor-hit count and blob change verified on disk. Each restore was proven with blob == HEAD and an emptygit diff HEAD. The subjects are imported by relative path fromsrc/, so nodist/leg applies.['ps_a']instead of['ps_a','ps_b']; refused read inserted 2; no-registry case['ps_a']; backfill casedriftHealed0. The control and the no-layered-read case stayed green.[]; once-per-name and backfill read counts were also red['member_default','member_default']instead of one readsaveMetaItemcalls instead of 1[]; both controls stayed greenA first A2 attempt was a refused no-op: its replacement contained the anchor, so the anchor count stayed x1 → x1, nothing ran, and the restore was proven. It was re-run with a falling anchor.
Verification (HEAD
e41760d89c)pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2: 176 files, 3695 passed, 45 skipped.pnpm --filter @objectstack/plugin-security typecheckexits 0, includingcheck:test-typecheckOK.tsc -p tsconfig.test.json --listFileslists all 3 touched test files.node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstackderived 66 commands, pluspnpm check:durability-log-level; all exit 0.--ran: 66 derived, 66 run, 0 NOT-MEASURED, 0 UNRUN.af7b0a339f, hadcheck:engine-double-contractandcheck:objectql-double-limitred on this PR's new test doubles. Both were fixed ine41760d89c: the heal tests spy on the real engine instead of an object-literal double, and the bootstrap double applieslimitby presence after the filter.--no-inline-config --format json.--print-configresolves each, and no ignored-file warning was emitted.eslint.config.mjs, noparserOptions.project), so this diff cannot move any untouched file's verdict.init()/start()or declaration was edited, socheck:init-service-contractandcheck:startup-registry-verdictwere not owed.Acceptance notes
member_defaultplus an active envsys_metadataoverlay for it.ps_aonly, andps_bkept its drifted facets on boots 1, 2 and 3 with 0 warnings. Step 1 resolves one row per name throughbuildExistingByName, and step 2 skips every overlay name.upsertEnvPermissionSet's per-item read on the live single-mutation path still inserts on a refused read.retirePermissionSetRecordhas the same by-nameLIMIT 1shape: on a metadata delete it removes only the first row of a duplicated name. Read, not exercised.singleand refused under a wall (ADR-0131 D2/D3/D5/D13) #15204,pm:blocked, not scheduled). C3 would delete these paths. That does not change this card.Generated by Claude Code