Skip to content

Commit 06c762e

Browse files
os-warrenclaude
andauthored
fix(plugin-security): retire seven dead { records } find-result limbs, and stop the permission-set loader inventing an empty page (#15838)
* fix(plugin-security): retire seven dead `{ records }` find-result limbs, and stop the permission-set loader inventing an empty page Six of the seven `Array.isArray(x) ? x : x.records` blocks in this plugin carried an unreachable envelope limb over an engine `find()` result. The limb is removed on a MEASUREMENT rather than on the declared type: `IDataEngine.find` says `Promise<any[]>`, but a declared type is not proof here — this repo also carries a `find()` that resolves a `QueryResult` envelope and never an array. A real `ObjectQL` over a real `SqlDriver` was booted and each seam driven through the shipped function that owns it; every one answered a bare array with no own `records` key, on a populated page and an empty one alike. Each of the six keeps its existing disposition for a non-array. Removing a dead limb must not quietly convert a seam that gaps into one that invents an empty. The seventh block is the opposite defect and is repaired in the opposite direction. `SecurityPlugin`'s `sys_permission_set` loader swallowed a thrown read into `[]` and mapped an unreadable result to `[]` too, so three distinct facts left by one door. This is the enforcement plane: "no permission sets" silently withdraws grants that exist while every request still looks normal, and the swallow made `PermissionEvaluator.resolvePermissionSets`' own "db lookup failed" warn unreachable — the diagnostic this repo had already built for exactly this loss. The read fault now propagates and an unreadable page is refused with `DATABASE_ERROR`; a page carrying a non-row refuses too, where the trailing filter used to drop it in silence. Enforcement is unchanged in both directions — an unanswered read still grants nothing — but it is now sayable. Seven pins, one per block, each driving its own seam against the real engine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y * test(plugin-security): route the pin's engine seam through the producer predicates, and re-anchor the rotted system-context rows Two gate repairs on the #15598 pins, neither a behaviour change. `check:engine-double-contract`: the pin file's `observed()` recorder forwards every verb to a real `ObjectQL`, but a seam that merely forwards is exactly the shape that reads as "not a double" and then admits a call the real engine would refuse. Its `update`/`findOne`/`delete` now open with the producer's own predicates (`assertEngineUpdateDispatch` / `assertEngineFindOnePredicate` / `assertEngineDeleteDispatch` from `@objectstack/metadata-core`), and the ledger learns about the newly pinned double — the ratchet grows, it is not weakened. `check:system-context-census`: line rot, not a finding. The loader repair added a net 66 lines to `security-plugin.ts` (77 added / 11 deleted), and every rotted anchor the census reported was off by exactly 66. Repaired with the gate's own `--fix`; only line numbers in `content/docs/permissions/system-context.mdx` change, no prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y * docs(changeset): qualify the enforcement claim — unchanged on every reachable input, fail-closed on the two that are not Clause-② review measured `PermissionEvaluator.resolvePermissionSets` through this PR's own `dbLoaderFor(ORG)` over a real ObjectQL/SqlDriver, at HEAD and at the merge-base blob, on five engine conditions. A healthy page, a thrown read and `undefined` resolve identically before and after — every result the shipped engine actually produces. An envelope and a page carrying a non-object element do NOT: they granted at base and refuse at HEAD. Both are fail-closed and both are unreachable on the measured engine, which is what the eleven pins establish, so this is a declared narrowing rather than a discovered move — but "unchanged in both directions" is an unqualified claim about behaviour and it sits in the changeset, which feeds release notes. The reviewer's wording replaces it verbatim. ⛔ No source change: the refusal itself was reviewed and passed exactly as it stands. Changeset and PR body carry identical wording. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 924f0fe commit 06c762e

11 files changed

Lines changed: 671 additions & 36 deletions

.changeset/quiet-pans-repair.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
Remove seven dead `{ records }` union-normalizer limbs on engine `find()` results, and repair the one that was silently dropping instead of gapping.
6+
7+
Six seams in this plugin normalized an engine read as `Array.isArray(x) ? x : x.records`. The envelope limb was unreachable: `ObjectQL.find` resolves a bare array of row objects, measured by booting a real engine over a real `SqlDriver` and driving each seam through the shipped function that owns it, rather than inferred from `IDataEngine.find`'s declared `Promise<any[]>` (a declared type is not proof — this repo also has a `find()` that resolves an envelope). Each seam keeps its existing disposition for a non-array; only the dead limb is gone.
8+
9+
The seventh is repaired in the opposite direction. `SecurityPlugin`'s `sys_permission_set` loader mapped three different facts onto one value: a read that succeeded on an empty catalog, a read that threw, and a read that resolved something it could not read all left as `[]`. On the enforcement plane that silently withdraws grants that exist while every request still looks normal, and it made `PermissionEvaluator`'s existing "db lookup failed" warning unreachable — so a transient database error and an empty catalog produced identical, undiagnosable 403s. The loader now lets the read fault propagate and refuses an unreadable result with `DATABASE_ERROR`. Enforcement is unchanged for every result the shipped engine produces; an envelope or a non-row element now refuses (fail-closed) where the old code read through it. An unanswered read still grants nothing; what changes is that it is now reported instead of silent.

content/docs/permissions/system-context.mdx

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -87,14 +87,14 @@ that silently does not happen.
8787

8888
| # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor |
8989
|:--|:---|:---|:---|:---|
90-
| 1 | **The whole security middleware short-circuits** before any gate runs | plugin-security | Get: every CRUD/FLS/tenant/owner gate below skipped in one branch. Lose: all of rows 2–6 at once — this is the single largest behaviour on the page | `security-plugin.ts:1620` |
91-
| 2 | **`owner_id` is not auto-stamped on INSERT** (the step 3.5 anchor guard is inside the block row 1 skips) | plugin-security | Lose: the row lands `owner_id = NULL`, so the default `owner_only_writes` policy hides it **from its own creator**. Get: nothing — this is a gap, not a capability | guard at `security-plugin.ts:2546` (the step 3.5 block), skipped by `:1620` |
92-
| 3 | Row-level read filter resolves to "no filter" | plugin-security | Get: unscoped reads. Lose: row-level scoping entirely | `security-plugin.ts:4374` |
93-
| 4 | Field-level security returns **all** fields | plugin-security | Get: every column readable. Lose: field masking | `security-plugin.ts:4525` |
94-
| 5 | Export permission granted unconditionally | plugin-security | Get: `canExport` is `true` | `security-plugin.ts:4603` |
95-
| 6 | Write bypass = `true`, effective write scope = `org` | plugin-security | Get: widest write scope without holding any capability | `security-plugin.ts:1447`, `:1469` |
90+
| 1 | **The whole security middleware short-circuits** before any gate runs | plugin-security | Get: every CRUD/FLS/tenant/owner gate below skipped in one branch. Lose: all of rows 2–6 at once — this is the single largest behaviour on the page | `security-plugin.ts:1686` |
91+
| 2 | **`owner_id` is not auto-stamped on INSERT** (the step 3.5 anchor guard is inside the block row 1 skips) | plugin-security | Lose: the row lands `owner_id = NULL`, so the default `owner_only_writes` policy hides it **from its own creator**. Get: nothing — this is a gap, not a capability | guard at `security-plugin.ts:2612` (the step 3.5 block), skipped by `:1686` |
92+
| 3 | Row-level read filter resolves to "no filter" | plugin-security | Get: unscoped reads. Lose: row-level scoping entirely | `security-plugin.ts:4440` |
93+
| 4 | Field-level security returns **all** fields | plugin-security | Get: every column readable. Lose: field masking | `security-plugin.ts:4591` |
94+
| 5 | Export permission granted unconditionally | plugin-security | Get: `canExport` is `true` | `security-plugin.ts:4669` |
95+
| 6 | Write bypass = `true`, effective write scope = `org` | plugin-security | Get: widest write scope without holding any capability | `security-plugin.ts:1513`, `:1535` |
9696
| 7 | Metadata-plane schema masking exempt (ADR-0106 D4) | metadata-core | Get: unmasked object schema. Note: the exemption is a **caller** property — it short-circuits before the security service is consulted | `object-schema-fls.ts:228` |
97-
| 8 | `explain()` may target a principal other than the caller | plugin-security | Get: no `manage_users` / delegated-admin check | `security-plugin.ts:3887` |
97+
| 8 | `explain()` may target a principal other than the caller | plugin-security | Get: no `manage_users` / delegated-admin check | `security-plugin.ts:3953` |
9898
| 9 | Anonymous-deny treats the caller as authenticated | core | Get: passes the 401 seam with no `userId` | `anonymous-deny.ts:154` |
9999
| 10 | Permission-set projection middleware skipped | plugin-security | Lose: projection of permission-set-derived columns | `permission-set-projection.ts:1015` |
100100
| 11 | Session-resolution middleware skipped | plugin-auth | Get: no session lookup attempted | `auth-plugin.ts:1427` |
@@ -120,7 +120,7 @@ that silently does not happen.
120120
| 26 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6881` |
121121
| 27 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:12477` |
122122
| 28 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:12406` |
123-
| 29 | **Bulk data event `organizationId` OMITTED** — the batch is published "not asserted" | plugin-security | Get: nothing — the `data.records.*` event still publishes. Lose: the per-organization attribution: this exit is taken before the security middleware composes any tenant wall, so it records no Layer 0 verdict on the operation (`OperationContext.tenantLayer0Verdict`, #15813), and the engine's bulk producer — which reads that recorded verdict and nothing else — omits the key rather than filling it from the caller's `tenantId`; a tenant-scoped consumer then does not deliver the event inside an organization wall (#15225) | `security-plugin.ts:1620` |
123+
| 29 | **Bulk data event `organizationId` OMITTED** — the batch is published "not asserted" | plugin-security | Get: nothing — the `data.records.*` event still publishes. Lose: the per-organization attribution: this exit is taken before the security middleware composes any tenant wall, so it records no Layer 0 verdict on the operation (`OperationContext.tenantLayer0Verdict`, #15813), and the engine's bulk producer — which reads that recorded verdict and nothing else — omits the key rather than filling it from the caller's `tenantId`; a tenant-scoped consumer then does not deliver the event inside an organization wall (#15225) | `security-plugin.ts:1686` |
124124

125125
### 3. Sharing (`plugin-sharing`)
126126

packages/plugins/plugin-security/src/auto-org-admin-grant.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,14 @@ async function tryFind(
149149
): Promise<any[]> {
150150
try {
151151
const rows = await ql.find(object, { where, limit }, { context });
152-
return Array.isArray(rows) ? rows : Array.isArray(rows?.records) ? rows.records : [];
152+
// Bare array, driven — see `engine-find-bare-array.pin.test.ts`, which boots
153+
// a real engine over a real `SqlDriver` and pins this seam. The `{ records }`
154+
// limb removed from here was dead code that read as a contract.
155+
//
156+
// The `[]` arm is left exactly as it was: this function's whole contract is
157+
// `Promise<any[]>` best-effort, and turning it into a gap is a different
158+
// change with a different blast radius than removing an unreachable limb.
159+
return Array.isArray(rows) ? rows : [];
153160
} catch (e) {
154161
// Reads legitimately fail before the tables exist (boot ordering), so this
155162
// is debug rather than warn — but it is no longer nothing (#4640).

packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,12 @@ async function defaultLookup(ql: any, name: string, organizationId?: string): Pr
9191
} catch {
9292
return { status: 'unknown' };
9393
}
94-
const list = Array.isArray(rows) ? rows : Array.isArray(rows?.records) ? rows.records : null;
94+
// Bare array, driven against a real engine over a real `SqlDriver` — not
95+
// inferred from `IDataEngine.find`'s declared `Promise<any[]>`, which is not
96+
// proof (this repo has a `find()` that resolves an envelope instead). The
97+
// `{ records }` limb that stood here was dead; `engine-find-bare-array.pin.test.ts`
98+
// pins this seam. A non-array is still `unknown` — never "no such row".
99+
const list = Array.isArray(rows) ? rows : null;
95100
if (list === null) return { status: 'unknown' };
96101
// [#10103] This organization's own row answers; an organization-less leftover
97102
// is reported beside `absent` and never returned as `present`. One spelling of

packages/plugins/plugin-security/src/claim-seed-ownership.ts

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -173,13 +173,18 @@ function affectedRowCount(value: unknown): number | undefined {
173173
return value;
174174
}
175175

176-
/** Ids from a `find` result, tolerating both the array and `{ records }` shapes. */
176+
/**
177+
* Ids from a `find` result.
178+
*
179+
* `ObjectQL.find` resolves a BARE array — driven against a real engine over a
180+
* real `SqlDriver` through this module's own paging fallback, the only path that
181+
* reaches here (`engine-find-bare-array.pin.test.ts`). It is driven rather than
182+
* read off `IDataEngine.find`'s declared `Promise<any[]>` because a declared
183+
* type is not proof: this repo also has a `find()` that resolves an envelope.
184+
* The `{ records }` limb this carried was dead.
185+
*/
177186
function idsFrom(rows: any): string[] {
178-
const list: any[] = Array.isArray(rows)
179-
? rows
180-
: Array.isArray(rows?.records)
181-
? rows.records
182-
: [];
187+
const list: any[] = Array.isArray(rows) ? rows : [];
183188
const out: string[] = [];
184189
for (const r of list) if (r?.id) out.push(String(r.id));
185190
return out;

0 commit comments

Comments
 (0)