Skip to content

Commit f39ea95

Browse files
fix(plugin-security)!: refuse a sys_user_position write whose position names no sys_position row (#16712) (#20292)
Fixes #16712 Clause-②: yes Fixes #20297 Executes the maintainer-confirmed ruling on #16712 (batch #87, option A): a `sys_user_position` write whose `position` names **no `sys_position` row at all** is refused, instead of answering 201 over an assignment that resolves to nothing. Dispatch: PM session `session_01TEah6PeJGjxJfbHaySJjLQ`, claim comment 5858098043, branch `claude/issue-16712-position-catalog-refusal`. ## What changes A new engine middleware, `packages/plugins/plugin-security/src/position-catalog-refusal.ts`, registered by `SecurityPlugin.start()` right AFTER its security middleware (so it runs INSIDE it, after the delegated-admin gate and the CRUD check, the same placement the ADR-0094 permission-set data door uses). - **Predicate:** exactly "no catalog row carries this name". A deactivated position (`active: false`) is still a catalog row, so an assignment naming it is accepted and, as before, grants nothing (ADR-0049 shape E). The catalog is the WRITER's: it is read under `{ ...context, isSystem: true }` (the engine lookup probe's spelling), so the reach is the writer's organization plus organization-less positions. A name only another organization carries is refused exactly like a name no organization carries (#20297; see the patch-round section below). - **Envelope:** `400 VALIDATION_FAILED`, one `fields[]` entry per offending value: `field: 'position'`, `code: 'reference_not_found'`, `constraint: { target: 'sys_position', targetField: 'name' }`. This is the envelope the row's sibling lookup columns (`user_id`, `organization_id`, ...) already answer with. **No new error code**: `VALIDATION_FAILED` is a registered ADR-0112 code (built with `validationFailure` from `@objectstack/types`, which both HTTP doors map to 400), and `reference_not_found` is a member of the closed ADR-0114 field catalog. Nothing in `packages/spec` changes. - **Message:** names the value, says the column takes the catalog NAME, and names the fix. When the value is the record id of a position the writer's own organization can see (the hotclm id-spelling trap), it names that position and says to write its name. - **Writes judged:** every non-system insert (one row or a batch; the batch is refused whole), a non-system update by id that CHANGES `position`, and a predicate update (`multi: true`) that sets it. - A `position` that is not a string is judged too, by its string form: a number, bigint or boolean by `String(value)`, an object or array by its JSON text. The engine's `text` validation stores every one of these with 201, except an operator object (next bullet). - Left to the engine, and only these: `null` and a blank string (`required`), a value whose `String()` form is longer than the column (`max_length`), and an operator object. An operator object is a plain object with a declared filter operator as an own key, such as `{ $in: [...] }`; the engine refuses it as `invalid_type` (#5922). - An update that edits another column, or echoes the stored `position` back unchanged, is not judged, so an existing row whose name has left the catalog stays editable. The comparison reads string forms, so `123` echoed over a stored `'123'` counts as unchanged. - **Writes NOT judged, deliberately:** every `isSystem` write, the same stand-down the engine's own lookup check (`assertReferencesResolve`) takes. Measured reason for the seed door: on a fresh single-posture boot `AppPlugin.start()` writes `stack.data` inline (`packages/runtime/src/app-plugin.ts`, the `seedLoader.load` branch), while the declared position catalog is seeded on `kernel:ready` (`bootstrapDeclaredPositions`, `security-plugin.ts`), so a refusal at the seed door would fail every authored assignment seed on the first boot and pass it on the second. Also not judged: invitation acceptance (`invitation-placement.ts` `apply`, system context) and the platform bootstraps. This was a code reading, not a live boot of a seed-carrying stack. - **Fails open** (with a `warn`) when the catalog cannot be read, the engine lookup probe's stance. Two consequences outside `plugin-security/src`, both required by derived gates: - `content/docs/permissions/system-context.mdx`: new row 23b for the refusal's `isSystem` stand-down, and the census counts regenerated with `node scripts/check-system-context-census.mjs --fix` (105 to 106 sites). `check:system-context-census` requires an anchor for every elevation read site. - `.changeset/16712-position-catalog-refusal.md`: `@objectstack/plugin-security` `minor`, a `**BREAKING**` banner, and the ADR-0087 disposition `not-required (no-migration-prescription)`. `check:adr-0087-registration` reads it as `[BREAKING+bang] not-required (no-migration-prescription)`. The level follows the WHICH LEVEL rule (decision batch #35): `Clause-②: yes` takes at least `minor`, and breaking-ness is carried by the banner, not the level. ## The two pre-enumerations (the ruling's gate), in-repo half, tree `4d7e740d3b` The out-of-repo halves were measured EMPTY on the card (hotcrm `2f7b2326`, hotclm `14c899fc`, triage comment 5857658468). **Precondition 2 (a catalog-less or membership-derived name written into `sys_user_position.position`): EMPTY.** Every non-test writer: | writer | context | values written | catalog-less? | |:--|:--|:--|:--| | `packages/plugins/plugin-security/src/invitation-placement.ts` `apply` | `isSystem`, on invitation acceptance | `intent.positions`, the names the issuer put on the invitation (gate-checked at issuance). No literal. | no | | `examples/app-showcase/src/security/seed-approval-demo.ts` `assignPositions` | `isSystem`, on `kernel:bootstrapped` (after the catalog bootstrap) | `manager`, `finance`, `legal`, `exec`, `auditor`. All five are declared in `examples/app-showcase/src/security/positions.ts` `allPositions`. | no | | `packages/verify/src/rls.ts` `provisionRlsPositionPersona` | `isSystem`, post-boot | `declaredPositionNames(config)`, the stack's own declared names | no | - Search: the write verbs (`insert`/`create`/`upsert`/`update`/`*Many`) naming `sys_user_position` returned 11 hits, all in `*.test.ts`. The non-test write calls in every file naming the object add the three rows above. - Positive control: the same verb pattern finds the `sys_user_permission_set` writer in non-test code. - `org_member`, `everyone` and the other membership-derived or anchor names are resolver PROJECTIONS (`resolve-authz-context.ts`, the `mapMembershipRole` loop and the implicit `everyone` push), never stored rows. A `position:` value spelling any of the eight anchor or built-in names has 0 non-test hits, and 12 in tests (control). **Precondition 1 (in-repo half), re-confirmed on today's tree: EMPTY.** No shipped `stack.data` seed carries `sys_user_position`. The only `object: 'sys_user_position'` in non-test code is the gate dry-run literal in `invitation-placement.ts`. Seed files for other objects are found by the same search (control). ## Evidence (first round, head `cbdd0e70`; the patch-round section below is the evidence for the current head) **Live HTTP, real composition** (fresh showcase, `pnpm dev -- --fresh -p` on a random port, account created through `POST /api/v1/auth/admin/create-user`; the server was stopped by its recorded process group): | request | answer | |:--|:--| | `POST /data/sys_user_position` with `position` = the `auditor` row's id | **400** `VALIDATION_FAILED`, `fields[0]` = `position` / `reference_not_found`, message says to write `auditor` | | same, `position: 'totally_not_a_position_zzz'` | **400** `VALIDATION_FAILED` / `reference_not_found`, generic remedy | | same, `position: 'auditor'` | **201** | | that holder reads `showcase_inquiry` | **200, 3 rows** (the name resolves) | | `PATCH` that row, `position` = the auditor id | **400** | | `PATCH` that row, `reason` edited, `position` echoed unchanged | **200** | | `POST /data/sys_user_position/createMany`, `[finance, fin4nce_typo]` | **400**, names only `fin4nce_typo`; nothing stored | **Unit and integration** (`position-catalog-refusal.test.ts`: 14 cases at `cbdd0e70`; each patch-round section below gives the count for its head): a real `ObjectQL` over SQLite, with the real `SecurityPlugin` registered the way a kernel composition registers it. Every refusal pin asserts `code` and `status` through `resolveThrownHttpError`, never a bare throw. - **The ruling's control leg:** an id-spelled assignment is refused, and the same account reads 0 rows. A direct `sys_user_permission_set` grant of the same set to the SAME account then reads 3 rows. - **Both accepted halves:** a catalog name resolves (the holder reads 3 rows). A deactivated position's name is accepted and stored, and grants nothing. - **Authorization first:** a plain member, and an `organization_admin`-shaped caller (wildcard `modifyAllRecords` plus an explicit per-table deny), get `403 PERMISSION_DENIED` for a bogus name and for a real name alike. - **Walled posture, two organizations:** - a name no organization carries is refused; - a name only another organization carries: pinned ACCEPTED at `cbdd0e70`, under the literal predicate. Since #20297 it is REFUSED, and that pin is reversed (patch round below); - the id hint never names another organization's position; - **the org boundary for a VALID foreign organization** (the leg the card carried as NOT MEASURED): an `org_a` writer stamping `organization_id: 'org_b'` gets `403` at the tenant wall, identically for all three names. Measured at the engine layer with the `isolated` posture, not over HTTP. **Ablations** (each through `scripts/ablation-replace.mjs` in WRAP mode, plus a shell `trap` restoring the absolute path from `HEAD`; every restore proven by blob hash == `HEAD` and an empty `git diff HEAD`). The subject resolves through relative imports to `src/`, so no rebuild or `dist/` preflight applies: | ablation | mutation | result | |:--|:--|:--| | A1 | the refusal never fires | **7 red** (every refusal pin: "expected the write to be refused, but it succeeded"), 7 green | | A2 | the catalog judged at the TOP of the security middleware, before authorization | **2 red**: authz-first (`expected 'VALIDATION_FAILED' to be 'PERMISSION_DENIED'`) and the foreign-org wall. The first attempt was a no-op: the tool refused it because the replacement re-contained the anchor. It is reported, not counted. | | A3 | an unchanged `position` judged too | **1 red** (the fossil-edit pin) | | A5 | the id hint drops its organization filter (at `cbdd0e70`; the patch round removes that filter, and the hint read is scoped instead, see C1 below) | **1 red** (the hint named `qa_b_only`) | **Local verification at head `cbdd0e70`:** - `pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2`: **141 files / 2910 tests passed**. - `pnpm --filter @objectstack/plugin-security typecheck`: exit 0. The test layer compiles, with 0 debt. - `node scripts/pm/dispatch-gates.mjs --commands` over the actual diff: **94 derived, 94 run, 0 NOT-MEASURED, 0 UNRUN**, all exit 0 (`--ran` reconciliation green). This includes `check:dual-build-cjs-loads`, `check:i18n` and `check:type-check-debt` after a full workspace build. - Lint, narrowed and proven: - (1) The population read from eslint's own config (`--print-config`): the three changed `.ts` files are in it, and the `.mdx` and `.md` files are not. - (2) `eslint --no-inline-config --format json` counted 3 files, 0 errors and 0 warnings. - (3) No `parserOptions.project` is set anywhere (type-aware linting is not enabled), so this diff cannot move a verdict on an untouched file. - Declared to CI, not run locally: the `packages/qa/dogfood` suites. The three that write `sys_user_position` over HTTP (`delegation-of-duty`, `showcase-permission-zoo`, `delegated-admin-invite`) all write catalog names. ## The predicate's tenancy scope: decided on #20297 (B) The first round applied the ruled predicate literally and read the catalog unscoped. On a walled posture that accepted a name only another organization carries, and it raised the scope as an open question. - Triage routed that question as precedent-decided (#20297, comment 5859496956): the read is the writer's organization plus organization-less rows, in the engine lookup probe's `{ ...context, isSystem: true }` (#19808, #19819, #19860). - The patch round below executes it. The #16712 ruling itself is unchanged: a deactivated position is still a catalog row. ## Acceptance notes - **`security/explain` naming an unresolvable entry** (hotclm evidence, 5611199715): out of scope by dispatch. Noted, not filed. - **Lint as the authoring-time second carrier**: `packages/lint/src/validate-security-posture.ts` lints `sys_user_position` seed rows but does not check that `position` names a declared position. It is the natural carrier for the seed door this refusal stands down on. Out of scope by dispatch; noted, not filed. - **Invitation issuance is not judged by this refusal**: `assertIssuable` dry-runs only the delegated-admin gate, and acceptance writes under a system context. An invitation naming a catalog-less position is still accepted at issuance, and the placement lands silently at acceptance. This is a door this change does not cover. Carrier: whoever next touches `invitation-placement.ts`; no carrier is named today. - The refusal message is English only. Localizing it needs a message-catalog key in `packages/spec`, another lane's. - The write preview (`ObjectQL.validate`) runs no middleware, so it does not report this refusal, the same as it does not report the engine's lookup check today. - The claim's surface listed `plugin-security/src` plus the changeset. The dispatch's file-surface clause names `packages/plugins/plugin-security/src/` as the landing, so the middleware registration in `security-plugin.ts` (one `registerMiddleware` call, with its import and comment) sits inside it, away from the `writeCheckPolicies` docblock. The `system-context.mdx` row is the one addition outside it, owed to `check:system-context-census`. <sub>Seat's append (domain:services #6021, `session_01TEah6PeJGjxJfbHaySJjLQ`): the #20297 dev's patch-round section, verbatim from its report 5860074760. The seat also replaced the `Held-for:` line in the head with the closing line for that card.</sub> ## Patch round (#20297) Executes the triage routing on #20297 (comment 5859496956), option **B**: the predicate the #16712 ruling fixed now reads the WRITER's catalog, meaning the writer's organization plus organization-less rows, in the engine lookup probe's spelling `{ ...context, isSystem: true }` (`assertReferencesResolve`, #19808). The ruling is not re-opened: a deactivated position is still a catalog row, so shape E stays accepted. Dispatch: PM session `session_01TEah6PeJGjxJfbHaySJjLQ`, claim 5859557977. **What changed** (head `cb1d5be9`) - `namesWithoutCatalogRow(deps, names, context)` and `idSpellingHints(deps, values, context)` now take the writer's context as a required parameter. They read `sys_position` under `catalogReadContext(context)`, which is `{ ...context, isSystem: true }`, and `assertPositionNamesCatalogRow` passes `opCtx.context`. `security-plugin.ts` is unchanged in this round. - The id hint's organization post-filter is removed. The hint now reads exactly what the check reads (see the C1 ablation below). - The by-id pre-image read stays a bare `{ isSystem: true }`. It is measured unreachable for a foreign row id, and that is pinned (below). - Module docblock: a new "Whose catalog" section states B and cites #20297 with the engine precedent (#19808, #19819, #19860). "Where it runs" no longer says authorization-first is what protects an unscoped read. Authorization-first itself stays (P1 below). - Changeset: the "read across every organization" line is gone. A "Whose catalog" paragraph says the catalog is the writer's organization's positions plus the organization-less ones, and that a name only another organization carries is refused like any unknown name. `minor`, `**BREAKING**`, `!`, `Clause-②: yes` and the ADR-0087 disposition are kept; `check:adr-0087-registration` reads `[BREAKING+bang] not-required (no-migration-prescription)`. - `system-context.mdx` row 23b now describes the scoped read. - `origin/main` `7b1e4a48` is merged in (merge commit `2f036209`). The only conflict was the census file-count row. `pnpm gen:system-context-census` then re-derived symbols 88 → 89 (`75804ad6`). The PR now reads `mergeable_state: clean`. **Mechanism readings.** These use the walled two-organization fixture: a real `ObjectQL` over SQLite, with the real `SecurityPlugin`. The "before" column was measured at `75804ad6`, whose refusal module is byte-identical to `cbdd0e70`'s; the "after" column is the pins at `cb1d5be9`. | write, from an `org_a` admin | before | after | | --- | --- | --- | | insert `position: 'qa_b_only'` (only `org_b` carries it) | 201 | 400 `VALIDATION_FAILED` / `reference_not_found`, message identical to the exists-nowhere one | | insert `position: 'nope_position'` (no organization carries it) | 400 | 400 | | update by id of an `org_b` row, unknown name | 403 `PERMISSION_DENIED` | 403 | | update by id of an id that exists nowhere, unknown name | 403 `PERMISSION_DENIED`, the same message | 403 | - **The pre-image read is never reached for a foreign id.** The security middleware answers a foreign id and a nonexistent id identically, before the refusal runs, so the pre-image read is left bare. - **Scoped vs bare lookup by name.** Looking up `sys_position` by name under `{ ...orgAdminContext, isSystem: true }` finds `qa_b_only` 0 times and `qa_a_own` once. Under a bare context, or a context with no tenant, each is found once. - **A writer with no organization never reaches the refusal on the isolated posture.** The security middleware answers `403` ("no active organization") first, for an insert and for a predicate update. So the "reads every organization" behaviour is pinned directly on `namesWithoutCatalogRow`. - **`single` posture.** The fixture seeds its catalog the way a `single` deployment seeds its declared catalog, with no organization, so both rows read a null `organization_id`. That is not true of every row on a `single` deployment: a position created through the data door by a session with an active organization is stamped with it. On a deployment holding one organization, the scoped read (`organization_id = tenant OR organization_id IS NULL`) still covers the whole catalog. A `single` deployment holding more than one organization still boots; `TenancyService` reports that state at `error` (#17010). In that state each writer reads its active organization's positions plus the organization-less ones. The existing single-posture tests carry no tenant, so they never exercise the organization-less term. A new pin writes with `tenantId: 'org_a'`: `qa_auditor` is accepted and an unknown name is refused. **Tests** (`position-catalog-refusal.test.ts`, 14 → 18 cases) - **Reversed, not deleted.** A name only another organization carries is now refused `400 VALIDATION_FAILED`, `reference_not_found` at `position`. Its message is byte-identical to `positionNotInCatalogMessage('qa_b_only')`, and its envelope matches the exists-nowhere case key for key. A predicate update that sets that name is refused the same way. - **New.** The writer's own organization's name is accepted (the control). The scoped and unscoped readings are pinned on the function. A foreign row id is pinned to answer like a nonexistent id. An organization-bound writer on the single posture is pinned. - **Unchanged and green.** The control leg, shape E, the batch / multi / echo legs, the `isSystem` stand-down, 403-first, the foreign-organization wall and the id hint. **Ablations** (at `cb1d5be9`). Each ran through `scripts/ablation-replace.mjs` in WRAP mode plus a shell trap, and every restore was proven by blob == `HEAD` and an empty `git diff HEAD`. The subject resolves through relative `src/` imports, so no rebuild or `dist/` preflight applies. | ablation | mutation | result | | --- | --- | --- | | B1 | the catalog name read reverted to the bare `SYSTEM_CTX` | 2 red: the reversed pin ("expected the write to be refused, but it succeeded") and the organization-bound leg of the function pin | | C1 | the id-hint read reverted to the bare `SYSTEM_CTX` (post-filter already removed) | 1 red: the hint names `qa_b_only` | | P1 | the catalog judged at the top of the security middleware, before authorization | 3 red: 403-first, the foreign-row-id pin (`nope_position` answers 400 instead of 403), and the foreign-organization wall | C1 doubles as the post-filter measurement. With the scoped read and no post-filter, the full suite is green, and the hint pin fails only when the read itself is unscoped. So the post-filter is not kept. **Local verification at `cb1d5be9`** - `pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2`: 141 files / 2914 tests passed. - `pnpm --filter @objectstack/plugin-security typecheck`: exit 0. The refusal test is in the `tsconfig.test.json` program (checked with `--listFiles`). - `node scripts/pm/dispatch-gates.mjs --commands` over the actual diff: - 94 derived, 94 run, all exit 0; - `--ran` reconciliation: "94 run, 0 NOT-MEASURED (a DERIVED zero)"; - this includes `check:query-options-erasure`, back to 236 sites: the pre-image pin's read had first been cast to `any`, and `cb1d5be9` types it. - Also run: the four artifact-roster gates flagged for this diff, and the diff-scoped `check-issue-citations.mjs` (6 citations, 6 resolve). - Lint, narrowed and proven: - `eslint --print-config` puts the three `.ts` files in the population and the `.md` / `.mdx` files outside it; - `eslint --no-inline-config --format json` counted 3 files, 0 errors and 0 warnings; - `eslint.config.mjs` sets no `parserOptions.project`, so this diff cannot change a verdict on an untouched file. **Acceptance notes added this round** - **A context carrying only `organizationId`** (no `tenantId`) is not tenant-scoped by the engine's driver options. It reads every organization here (measured on the fixture) and, by the same driver-options reading, in the engine's own lookup probe (code reading). It cannot reach this refusal on the isolated posture, where it gets 403 first. Noted, not filed. - **Under the `group` posture** the scoped read follows the engine's membership union, so a name from another organization the writer belongs to is accepted. This is a code reading, not measured. Noted, not filed. <sub>Seat's append: the round-4 section, from the #20297 dev's report 5860889632, with "stored text" corrected to "string form" where the SQLite measurement shows the two differ (`123` is stored as `'123.0'`).</sub> ## Patch round 4 (contract re-review 5860712690) **Measured first.** On head `ebf00fd8`, before any code change, I ran non-system inserts and by-id updates as an admin, on both the single and the walled fixture, with the same result on each: | `position` written | answer | stored as | | --- | --- | --- | | `123` | 201 | `'123.0'` | | `true` | 201 | `'1.0'` | | `{}` | 201 | `'{}'` | | `['x']` | 201 | `'["x"]'` | | `[]` | 201 | `'[]'` | | `NaN` | 201 | `NULL` | | `10n` | 201 | `'10'` | | by-id update to `123` or `true` | 200 | `'123.0'` / `'1.0'` | | `''`, `' '` or `null` | 400 `VALIDATION_FAILED`, `required` | nothing | | a 101-character string | 400 `VALIDATION_FAILED`, `max_length` | nothing | So the reviewer's premise held: the engine refuses only `null`, blank strings and over-long values, and stores everything else. Branch (a) applies. **What changed** (head `6e1ef6aa`) - `judgedName(value)` in `position-catalog-refusal.ts` now judges every value except those the engine answers itself. - A number, bigint or boolean is judged by `String(value)`. - An object or array is judged by its JSON text (in the measurement, SQLite stored `{}` and `['x']` in exactly that form). It is never judged by `String(['x'])`, which reads `'x'` and would accept an array naming a real position that then resolves nothing. - The engine answers `null`, a blank string, and any value whose `String()` form is longer than the column (its `max_length` check reads `String(value)`). Patch round 6 below adds the one refusal this missed: an operator object, `invalid_type` (#5922). - The refusal is the usual one: `400 VALIDATION_FAILED`, `reference_not_found` at `position`, with the value's string form as `value` and in the message. - The by-id unchanged-value check compares string forms. - The docblock's stand-down sentence now names only the refusals the engine really makes. - The changeset and `system-context.mdx` are unchanged; their "Which writes" and "Such a write is now refused" text is true as written. **Tests** (21 cases, up from 18). Every refusal pin asserts `code` and `status`. - **Insert:** `123`, `true`, `{}` and `['qa_auditor']` are each refused, with `value` equal to `'123'`, `'true'`, `'{}'` and `'["qa_auditor"]'`, and nothing is stored. - **By-id update:** `123` and `true` are refused, and the stored row is untouched. - **Echo:** `123` over a stored `'123'` is not judged. - **Id hint:** now also asserts `VALIDATION_FAILED` / 400, plus `field` and `code` on `fields[0]`, for both refusals. **Ablations** (at `6e1ef6aa`, through `scripts/ablation-replace.mjs` in WRAP mode plus a shell trap; every restore was proven by blob == `HEAD` and an empty `git diff HEAD`): | ablation | mutation | result | | --- | --- | --- | | N1 | only strings are judged | 2 red: the insert and update pins ("expected the write to be refused, but it succeeded") | | N2 | the echo compares raw values | 1 red: the echo pin (`123` is refused as `'123'`) | | N3 | no JSON form, so objects fall back to `String()` | 1 red: the insert pin (`{}` is judged as `'[object Object]'`) | **Verification at `6e1ef6aa`** - plugin-security: 141 files / 2917 tests passed; typecheck exit 0. - `dispatch-gates --commands`: 94 derived, 94 run, all exit 0. The `--ran` reconciliation reports a derived zero. - `check:query-options-erasure` holds at 236, since no new `find` was added. - `check:adr-0087-registration` reads `[BREAKING+bang] not-required (no-migration-prescription)`. - The diff-scoped issue-citation check: 7 citations, all resolve. **Acceptance note.** On SQLite a non-string is stored as the driver's text (`'123.0'`, `'1.0'`), not as `String(value)`. So a non-string whose `String()` form happens to equal a catalog name, for example `true` where a position is named `true`, is accepted and stored in a form that resolves nothing. `sys_position.name` has no pattern that rules such names out. The root cause is the engine's `text` leniency for non-string input, which is outside this card; it is measured at the engine layer only. <sub>Seat's append: the round-6 section, verbatim from the #20297 dev's report.</sub> ## Patch round 6 (contract review 3, 5861153477) **Measured first** on head `0e5f4c79`, on the single fixture, with the refusal in place and then with it ablated (`ablation-replace` WRAP; the restore was proven by blob == `HEAD`): | `position` | with the refusal | engine alone | | --- | --- | --- | | `{ $in: ['x'] }`, `{ $in: [], a: 1 }`, `{ $regex: 'x' }`, `{ $or: [] }` | 400 `invalid_type` | 400 `invalid_type` | | by-id update to `{ $in: ['x'] }` | 400 `invalid_type`, row untouched | 400 `invalid_type` | | `{ a: 1 }`, `{ $foo: 1 }` | **201, stored** | 201 | | `'{nope_tok}'` | **201, stored** | 201 | | `'{current_user_id}'`, `'{today}'` | 400 `reference_not_found` | 201 | | `{}`, `[{ $in: 1 }]` | 400 `reference_not_found` | 201 | The reviewer's predicted outcome (`reference_not_found` pre-empting the engine) did not occur, but its cause is real. `invalid_type` came through only because the refusal failed open. - **The mechanism.** The catalog read put the value's text in `where`. A fully-wrapped `{…}` comparand is a filter placeholder (`classifyFilterToken` in `@objectstack/spec`, applied by `@objectstack/core`'s `resolveFilterTokens`). - An unknown one throws `FILTER_TOKEN_UNKNOWN`. The refusal then logged "catalog could not be read" and let the write through. - A known one is replaced by its value, so the lookup judged a different name. - **The consequence.** Every object without a declared operator but with at least one key, and every brace-wrapped string, bypassed the refusal. **What changed** (head `c282e608`, `position-catalog-refusal.ts` only): - **Operator objects are left to the engine.** `judgedName` stands down on them using `isFilterOperatorObject`, a narrow mirror of the engine's module-private `filterOperatorKeysIn` built from the same spec inputs (`isPlainRecord`, no `Date`, an own key in `ALL_OPERATORS` or the retired operators). It is not a `$`-prefix test: `{ $foo: 1 }` is judged. - **Placeholder-shaped names are looked up literally.** `catalogCarries` handles every name `classifyFilterToken` would treat as a placeholder: it reads the catalog names that share its first character (`$startsWith`) and compares in code, under the same scoped context. Such a name is therefore judged, never resolved and never failed open. - **The id hint** skips placeholder-shaped values. - **Docblock.** The stand-down bullet now names the engine's three refusals exactly: `required`, `max_length`, and `invalid_type` for an operator object. The `stringForm` doc now says a bigint is `String(value)` and only null, undefined, a symbol, a function or an unserialisable object gives `undefined`. "Fails open" says a placeholder-shaped name never falls open. - **The changeset is unchanged**; its "Which writes" is still true as written. **Tests** (24 cases, up from 21). Every refusal pin asserts `code` and `status`. - An insert of `{ $in: ['x'] }` or `{ $in: [], a: 1 }` gets the engine's `VALIDATION_FAILED` / 400 with `fields[0]` `{ field: 'position', code: 'invalid_type' }`, and nothing is stored. - `{ a: 1 }` and `{ $foo: 1 }` are refused `reference_not_found`, with value `'{"a":1}'` / `'{"$foo":1}'`, and no "could not be read" warning is logged. - `'{nope_tok}'` and `'{current_user_id}'` are refused `reference_not_found` as themselves. A catalog row really named `'{lit_pos}'` is found, and the assignment naming it is accepted. **Ablations** at `c282e608`; every restore was proven by blob == `HEAD` and an empty `git diff HEAD`: | ablation | mutation | result | | --- | --- | --- | | O1 | operator stand-down removed | 1 red: `{"$in":["x"]}` answered `reference_not_found` instead of `invalid_type` | | O2 | literal lookup removed | 2 red: `{ a: 1 }` and `'{nope_tok}'` were accepted ("expected the write to be refused, but it succeeded") | | B1 / C1 / P1 / N1 / N2 / N3 re-run | as before | 2 / 1 / 3 / 3 / 1 / 2 red | **Verification at `c282e608`** - plugin-security: 141 files / 2917 → 2920 tests passed; typecheck exit 0. - `dispatch-gates --commands`: 94 derived, 94 run, all exit 0. The `--ran` reconciliation reports a derived zero. - `check:query-options-erasure` holds at 236. - The diff-scoped issue-citation check: 11 citations, all resolve. --- _Generated by [Claude Code](https://claude.ai/code/session_01TEah6PeJGjxJfbHaySJjLQ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a36a691 commit f39ea95

5 files changed

Lines changed: 1252 additions & 9 deletions

File tree

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
---
2+
'@objectstack/plugin-security': minor
3+
---
4+
5+
fix(plugin-security)!: a `sys_user_position` write whose `position` names no `sys_position` row in the writer's catalog is refused (#16712, #20297), instead of answering 201 over an assignment that grants nothing
6+
7+
Clause-②: yes
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) Nothing authorable is renamed, retired or re-typed: `sys_user_position.position` stays a `Field.text`, every object key and every stored row parse exactly as before, and no stored assignment is rewritten or refused on read, so `objectstack migrate meta` has nothing to act on. What narrows is the ACCEPT SET of one runtime write path: a non-system insert, or an update that changes `position`, whose value names no `sys_position` row in the writer's organization or among the organization-less rows is refused `400 VALIDATION_FAILED` with the envelope the row's sibling lookup columns already answer with. The refusal text names the value and its own fix, and the measured in-repo and consumer writers (hotcrm, hotclm) all write catalog names, so there is no document for a migration to act on. -->
10+
11+
**BREAKING** accept-set narrowing on the `sys_user_position` write path —
12+
shipped as `minor` under the launch-window convention (`check-changeset-no-major`
13+
refuses `major`; breaking-ness is carried by this banner and the ADR-0087
14+
disposition, not by the level). Maintainer-confirmed ruling on #16712 (option A),
15+
with the catalog it reads settled on #20297: the writer's organization, under
16+
the platform's standing tenancy rule.
17+
18+
**What changed.** `sys_user_position.position` is the position's machine NAME
19+
(`sys_position.name`), but it is declared `Field.text`, so a value naming no
20+
catalog row — most often the position's record ID, written where its name
21+
belongs — was stored with a `201` and then resolved to nothing: the holder got
22+
no permission set, no sharing rule reached them, and they signed in to an app
23+
that reads nothing, with no error anywhere. Such a write is now refused:
24+
25+
- `400 VALIDATION_FAILED`, one `fields[]` entry per offending value at
26+
`field: 'position'`, `code: 'reference_not_found'`,
27+
`constraint: { target: 'sys_position', targetField: 'name' }` — the same
28+
envelope a bad `user_id` or `organization_id` on the same row already gets.
29+
- The message names the value and says the column takes the catalog NAME. When
30+
the value is the record id of a position the writer's own organization can
31+
see, it names that position and says to write its name.
32+
33+
**Which writes.** Every non-system insert (one row or a batch, refused whole),
34+
every non-system update by id that CHANGES `position`, and every predicate
35+
update (`multi: true`) that sets it. The check runs after authorization: a
36+
caller who may not write the table is still refused `403` on authority and never
37+
sees the catalog verdict.
38+
39+
**Whose catalog.** The writer's: the positions of the writer's own
40+
organization plus the organization-less ones — the same reach the engine gives
41+
its own lookup-reference check. A name that only ANOTHER organization's
42+
catalog carries is refused exactly like any unknown name, with the same
43+
envelope and the same message, so the answer says nothing about other
44+
organizations. On a single-organization deployment the declared positions
45+
carry no organization and any other position can only carry the one
46+
organization there is, so every writer sees the whole catalog. A writer whose
47+
context names no organization sees every organization's positions.
48+
49+
**What did not change.**
50+
51+
- A **deactivated** position is still a catalog row: an assignment naming it is
52+
accepted and, as before, grants nothing (ADR-0049).
53+
- **Stored rows** are untouched. An update that edits another column, or echoes
54+
the unchanged `position` back, is not judged, so an existing row whose name
55+
is no longer in the catalog stays editable.
56+
- **System-context writes** are not judged — the seed loader (which on a fresh
57+
single-organization boot writes `stack.data` before the declared position
58+
catalog exists), invitation acceptance and the platform's own bootstraps. That
59+
is the same stand-down the engine's lookup check takes.
60+
61+
**Who is affected.** A client, script or AI author that writes a position's id,
62+
a misspelled name, a name not yet created, or — on a deployment that walls
63+
organizations off — a name only another organization has. The fix is the one
64+
the refusal names: write the name of a position in the writer's own catalog, or
65+
create the position there first.

‎content/docs/permissions/system-context.mdx‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ the seed loader replaying package fixtures, a plugin's boot reconciler, a
1010
service self-write, a migration.
1111

1212
This page is **the authority** for what that flag actually does. It exists
13-
because the flag is not one concept: it is a single boolean read at **105
13+
because the flag is not one concept: it is a single boolean read at **106
1414
distinct sites across 19 packages**, and knowing three of those behaviours gives
1515
no hint that the other hundred-and-four exist. Every documented app-side bug
1616
traced to `isSystem` had the same shape — the metadata was complete and correct,
@@ -125,6 +125,7 @@ that silently does not happen.
125125
| 21 | **`readonly` strip bypassed — INSERT** | objectql | Same, on create — one gate over BOTH create-side passes since the 2026-09-03 ruling moved the static-`readonly` strip in beside the runtime-owned one and deleted the DataProtocol ingress copy. `isSystem` is the **only** exemption on this path: `preserveAudit` is deliberately not read on create, so a non-system historical import is still stripped | `packages/objectql/src/engine.ts#insert` |
126126
| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `packages/objectql/src/engine.ts#insert`, `packages/objectql/src/readonly-strict-errors.ts#READONLY_CLASS_REASONS` |
127127
| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `packages/objectql/src/engine.ts#assertReferencesResolve` |
128+
| 23b | **Position-catalog check skipped** — a `sys_user_position` write whose `position` names no `sys_position` row is not refused | plugin-security | Get: a system writer can store an assignment naming no catalog row — the seed loader writes `stack.data` from `AppPlugin.start()`, before `kernel:ready` seeds the declared position catalog, so a refusal there would fail every authored assignment seed on a fresh boot. Lose: such a row grants nothing and nothing says so. A non-system insert, or a non-system update that changes `position`, is refused `400 VALIDATION_FAILED` / `reference_not_found` instead when no catalog row the writer's context reaches carries the name: its organization's rows plus the organization-less ones, read as `{ ...context, isSystem: true }`, never a bare `{ isSystem: true }`, so a name only another organization carries is refused like one nobody carries. It is the same stand-down, and the same tenant scope, as the engine's referential-integrity check for a lookup column | `packages/plugins/plugin-security/src/position-catalog-refusal.ts#assertPositionNamesCatalogRow` |
128129
| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `packages/objectql/src/engine.ts#buildDriverOptions` |
129130
| 25 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `packages/plugins/plugin-security/src/system-write-guard.ts#isUserContextWrite`, `#assertEngineOwnedWriteAllowed` |
130131
| 26 | Identity write guard bypassed (ADR-0092) | plugin-auth | Get: direct writes to identity tables through the generic data path | `packages/plugins/plugin-auth/src/identity-write-guard.ts#isUserContextWrite` |
@@ -136,7 +137,7 @@ that silently does not happen.
136137

137138
### 3. Sharing (`plugin-sharing`)
138139

139-
The largest single consumer — **17 of the 105 sites**.
140+
The largest single consumer — **17 of the 106 sites**.
140141

141142
| # | Behaviour when `isSystem` | What you get / what you lose | Anchor |
142143
|:--|:---|:---|:---|
@@ -279,7 +280,7 @@ Ownership injection, `readonly` bypass and sharing materialisation are
279280
independent decisions, and a seed loader plausibly wants the first two but not
280281
the third. The concept is nevertheless **staying as one boolean**:
281282

282-
- **Shipped semantics.** `isSystem` is a published contract with 105 read sites
283+
- **Shipped semantics.** `isSystem` is a published contract with 106 read sites
283284
in 19 packages. Splitting it is a breaking contract change across all of them.
284285
(The ruling was taken when the census read 80 sites in 18 packages; the count
285286
has grown, which strengthens rather than weakens the argument.)
@@ -353,16 +354,16 @@ still holds equal to the census on every pull request:
353354
| Appearances of the bare identifier `isSystem` in non-test sources | 813 | — |
354355
| — parsed as a declaration | 22 | ✅ |
355356
| — parsed as an object-literal / type key (producers and option objects) | 310 | — |
356-
| — parsed as a property **read** | 111 | ✅ |
357+
| — parsed as a property **read** | 112 | ✅ |
357358
| — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ |
358359
| — the remainder: text inside comments and string literals | 358 | — |
359360
| Of those reads: reads of one of the unrelated metadata fields | 6 | ✅ |
360-
| Of those reads: reads of `ExecutionContext.isSystem` | **105** | ✅ |
361-
| — behaviour-bearing (rows 1–61 above) | 102 | ✅ |
361+
| Of those reads: reads of `ExecutionContext.isSystem` | **106** | ✅ |
362+
| — behaviour-bearing (rows 1–61 above) | 103 | ✅ |
362363
| — carry the flag onward only (rows 62–64 above) | 3 | ✅ |
363364
| Packages containing at least one elevation read | **19** | ✅ |
364-
| Files containing at least one elevation read | 44 | ✅ |
365-
| — the distinct symbols those reads live in — what this page anchors | 88 | ✅ |
365+
| Files containing at least one elevation read | 45 | ✅ |
366+
| — the distinct symbols those reads live in — what this page anchors | 89 | ✅ |
366367
| — of those files, the ones holding more than one read in one symbol | 8 | ✅ |
367368

368369
The six rows marked — are a **dated decomposition, not a live claim**: they were
@@ -426,7 +427,7 @@ same resolver, and the same registration shape, that holds `docs/adr/**`.
426427
Renaming a symbol is now a loud red instead of a silent misdirection.
427428

428429
⚠️ **The precision that costs, priced here rather than buried.** A symbol anchor
429-
cannot say WHICH read inside a function it means, and **8** of the **44**
430+
cannot say WHICH read inside a function it means, and **8** of the **45**
430431
anchored files hold more than one read inside a single symbol. So the population
431432
check runs per file at symbol granularity: every file the census finds a read in
432433
must be anchored, and the set of symbols this page cites into that file must

0 commit comments

Comments
 (0)