diff --git a/.changeset/16870-scope-beside-superuser-bit-refused.md b/.changeset/16870-scope-beside-superuser-bit-refused.md new file mode 100644 index 0000000000..63bcfa97d2 --- /dev/null +++ b/.changeset/16870-scope-beside-superuser-bit-refused.md @@ -0,0 +1,81 @@ +--- +'@objectstack/spec': minor +--- + +fix(spec): an object permission that declares a depth axis beside the super-user bit which short-circuits it is now REFUSED, instead of being stored and counted as coverage (#16870) + +**BREAKING** — `ObjectPermissionSchema` no longer accepts a `readScope` beside +`viewAllRecords: true`. Two sibling shapes are refused with it, read off the +same resolver lines rather than guessed at. + +The pair was accepted with **zero diagnostics**, materialised into +`sys_permission_set.object_permissions`, and counted by a capability census +reading the deployed shape as coverage — while the read stayed org-wide. +`PermissionEvaluator.getEffectiveScope` answers `org` on the super-user bit +**before** it consults the depth key, and `getDeclaredScope` (the ADR-0090 D10 +delegated-path input) carries the identical short-circuit ahead of the identical +read, so the declared narrowing was dropped from the delegation fold as well. + +⇒ the author declared a narrowing, the platform stored it, an audit of the +deployed shape reported the capability as exercised, and the read was still +org-wide. That is ADR-0049 `declared ≠ enforced` at the capability container +itself, and the accept set is the only door that stops the declaration from +being STORED: a diagnostic raised later fires after the shape is already there. + +``` +FROM ObjectPermissionSchema.parse({ allowRead: true, viewAllRecords: true, + readScope: 'own_and_reports' }) + -> { …, viewAllRecords: true, readScope: 'own_and_reports' } // stored, unread + +TO -> ZodError, located at ['readScope']: + "readScope: 'own_and_reports' is declared beside viewAllRecords: true, + which already grants org-wide read. … Delete readScope if the org-wide + read is intended, or set viewAllRecords: false if the narrowing is." +``` + +**Which pairs move, and the one that deliberately does not.** The refusal is the +two short-circuits, transcribed: + +| declaration | resolver | verdict | +|:--|:--|:--| +| `readScope` + `viewAllRecords: true` | `opClass === 'read' && (viewAllRecords \|\| modifyAllRecords)` | **refused** | +| `readScope` + `modifyAllRecords: true` | same disjunct | **refused** | +| `writeScope` + `modifyAllRecords: true` | `opClass === 'write' && modifyAllRecords` | **refused** | +| `writeScope` + `viewAllRecords: true` | the write short-circuit does not name `viewAllRecords` | **accepted — honoured, and refusing it would delete a real grant** | + +⛔ **What `viewAllRecords: true` GRANTS is untouched.** This changes which +declarations are accepted, never what an accepted one does — a permission- +semantics change is not in this change's remit. `viewAllRecords: true` alone, +`viewAllRecords: false` beside a `readScope` (the ordinary, honoured shape), and +a bare `readScope` all parse exactly as before; each is pinned as a +cost-direction guard in `permission.test.ts`, and an ablation that widens the +refusal one shape too far turns the `writeScope`-beside-`viewAllRecords` pin red. + +**The wire surface stays tolerant.** The refinement rides on the AUTHORING +wrapper only; `EffectiveObjectPermissionSchema` extends the unrefined base, so a +server still running an older toolchain can return a stored pair in an +effective-permission response without crashing a client (#4001's authorable/wire +split). `AccessMatrixEntry` likewise keeps describing the pair: it is a derived +SNAPSHOT shape whose committed `access-matrix.json` may predate this refusal, and +its tolerance is now stated with that reason in `explain.test.ts` rather than +reading as evidence that the platform accepts the declaration. + +**Scope is one object-permission entry**, which is exactly the resolver's input — +`resolveObjectPermission` returns a single entry (explicit, else the `'*'` +wildcard) and never merges two. A super-user bit in one permission set widening +past another set's `readScope` is ADR-0090's documented additive "widest wins" +semantics, not a contradictory declaration, and is not judged here. + +**Nothing in the fleet moves.** Measured across shipped defaults, both seeded +examples, two built access matrices, the built artifact fixture and every tracked +`.ts` / `.json`: **0** object permissions carry any refused pair, with lit +controls on every probe (130 nodes declaring `viewAllRecords`, 53 of them `true`, +18 declaring `readScope`; 133 brace-local `viewAllRecords: true` literals). + + diff --git a/content/docs/permissions/permission-sets.mdx b/content/docs/permissions/permission-sets.mdx index ac4a4663ea..e350d76782 100644 --- a/content/docs/permissions/permission-sets.mdx +++ b/content/docs/permissions/permission-sets.mdx @@ -131,6 +131,17 @@ the anchor decides *which* unit `unit*` means. > require the enterprise hierarchy resolver; without it they **fail closed to > `own`** (never fail-open). +> **A depth axis beside the super-user bit that bypasses it is REFUSED.** The +> read-scope resolver answers `org` on the super-user bit *before* it consults +> the depth key, so such a declaration would be stored and never read — and a +> capability census over the deployed shape would count it as coverage. The +> accept set now rejects it at the offending key, naming both remedies. Note the +> **asymmetry**, which follows the resolver exactly: `viewAllRecords` bypasses +> READ only, so `readScope` beside `viewAllRecords: true` is refused while +> `writeScope` beside it is **accepted and honoured**. `modifyAllRecords` +> bypasses both, so both `readScope` and `writeScope` beside it are refused. +> Declare the depth, or declare the bypass — not both on the same axis. + ## Capabilities & required permissions (ADR-0066) - **Capabilities** are first-class `sys_capability` records — named privileges diff --git a/packages/spec/src/security/explain.test.ts b/packages/spec/src/security/explain.test.ts index a67f5ecd9b..24a14b971a 100644 --- a/packages/spec/src/security/explain.test.ts +++ b/packages/spec/src/security/explain.test.ts @@ -28,6 +28,9 @@ import { ExplainRecordAttributionSchema, EXPLAIN_BATCH_MAX_RECORD_IDS, } from './explain.zod'; +// [#16870] Imported for ONE assertion: the boundary between the snapshot shape +// this file locks and the AUTHORING accept set that now refuses the same pair. +import { ObjectPermissionSchema } from './permission.zod'; describe('ExplainOperationSchema — the operation vocabulary is fixed', () => { it('accepts exactly the seven CRUD + lifecycle operations', () => { @@ -367,7 +370,32 @@ describe('ExplainDecisionSchema — the full decision report L3 consumes', () => }); describe('AccessMatrix schemas — the authoring-time companion', () => { - it('AccessMatrixEntry locks the crud bits + super-user bypass + scopes + sharingModel', () => { + // [#16870] ⚠️ The sample row below carries `viewAllRecords: true` BESIDE a + // `readScope`, and that is deliberate — but its reason changed, so read this + // before reading the assertion. + // + // That pair is a contradiction: the read-scope resolver answers `org` on the + // super-user bit before it ever consults `readScope`, so the depth is unread + // on every runtime path. The AUTHORING accept set (`ObjectPermissionSchema`) + // therefore REFUSES it now — pinned in `permission.test.ts`, and asserted + // from here in the next test so the two surfaces cannot drift apart silently. + // + // `AccessMatrixEntry` deliberately does NOT refuse it, for two measured + // reasons: + // + // 1. It is a DERIVED SNAPSHOT shape, not an accept set. `buildAccessMatrix` + // constructs entries from ALREADY-PARSED metadata, and `os build` reads a + // committed `access-matrix.json` back with a bare `JSON.parse` to diff it. + // A snapshot written by an older toolchain may well carry the pair, and + // the drift diff has to keep describing it. + // 2. Nothing in the tree parses through this schema outside this file — + // it is a TYPE contract that cloud's L3 product reads. A refusal added + // here would be a check no code path can ever run: exactly the + // declared-but-unenforced shape #16870 is about, reproduced one level up. + // + // ⇒ What this test locks is unchanged — the FIELD SHAPE, every key present + // and typed. It is no longer evidence that the platform accepts the pair. + it('AccessMatrixEntry locks the crud bits + super-user bypass + scopes + sharingModel (a SNAPSHOT shape: tolerant by design)', () => { const entry = AccessMatrixEntrySchema.parse({ permissionSet: 'crm_admin', object: 'crm_lead', create: true, read: true, edit: true, delete: false, @@ -382,6 +410,23 @@ describe('AccessMatrix schemas — the authoring-time companion', () => { }); }); + it('[#16870] the AUTHORING accept set refuses the very pair this snapshot shape tolerates', () => { + // The boundary, asserted rather than described. If a later change makes + // the authoring schema accept the pair again, this fails here too — the + // snapshot tolerance above is only defensible while the door upstream of + // it is shut. + const authored = ObjectPermissionSchema.safeParse({ + allowRead: true, viewAllRecords: true, readScope: 'unit_and_below', + }); + expect(authored.success, 'the accept set refuses a depth the resolver never reads').toBe(false); + const snapshot = AccessMatrixEntrySchema.safeParse({ + permissionSet: 'crm_admin', object: 'crm_lead', + create: true, read: true, edit: true, delete: false, + viewAllRecords: true, modifyAllRecords: false, readScope: 'unit_and_below', + }); + expect(snapshot.success, 'the snapshot shape still describes artifacts written before the refusal').toBe(true); + }); + it('the crud + bypass bits are REQUIRED (a missing bit is a contract break)', () => { expect(() => AccessMatrixEntrySchema.parse({ permissionSet: 'x', object: 'y', create: true, read: true, edit: true, delete: true, diff --git a/packages/spec/src/security/permission.test.ts b/packages/spec/src/security/permission.test.ts index 8e08348639..2b547d266a 100644 --- a/packages/spec/src/security/permission.test.ts +++ b/packages/spec/src/security/permission.test.ts @@ -109,6 +109,111 @@ describe('ObjectPermissionSchema', () => { }); }); +describe('[#16870] a depth axis beside the super-user bit that short-circuits it is REFUSED', () => { + // The defect: `PermissionEvaluator.getEffectiveScope` answers `org` on the + // super-user bit BEFORE it consults the depth key, and `getDeclaredScope` + // (the ADR-0090 D10 delegated-path input) carries the identical + // short-circuit ahead of the identical read. So the pair was accepted with + // zero diagnostics, materialised into `sys_permission_set.object_permissions` + // and counted by a capability census as coverage, while the read stayed + // org-wide. Refused at the accept set, which is the only site that stops the + // declaration from being STORED: `saveMetaItem` runs this parse before the + // runtime authoring gate, and `os build` runs it (compile.ts) before both the + // author-time rule registry and `buildAccessMatrix`. + + // ── REFUSAL direction ──────────────────────────────────────────────── + it('readScope beside viewAllRecords: true is refused, at the readScope path', () => { + const r = ObjectPermissionSchema.safeParse({ + allowRead: true, viewAllRecords: true, readScope: 'own_and_reports', + }); + expect(r.success).toBe(false); + const issue = r.error!.issues.find((i) => i.path[i.path.length - 1] === 'readScope')!; + expect(issue, 'the issue is LOCATED on the key the author must remove').toBeDefined(); + expect(issue.message).toContain('viewAllRecords: true'); + expect(issue.message).toContain('never enforced on any read path'); + expect(issue.message, 'both remedies are named, not just one').toContain('set viewAllRecords: false'); + }); + + it('readScope beside modifyAllRecords: true is refused too — the same disjunct short-circuits READ', () => { + const r = ObjectPermissionSchema.safeParse({ + allowRead: true, modifyAllRecords: true, readScope: 'unit', + }); + expect(r.success).toBe(false); + expect(r.error!.issues.some((i) => i.path[i.path.length - 1] === 'readScope')).toBe(true); + }); + + it('writeScope beside modifyAllRecords: true is refused, at the writeScope path', () => { + const r = ObjectPermissionSchema.safeParse({ + allowEdit: true, modifyAllRecords: true, writeScope: 'unit', + }); + expect(r.success).toBe(false); + const issue = r.error!.issues.find((i) => i.path[i.path.length - 1] === 'writeScope')!; + expect(issue).toBeDefined(); + expect(issue.message).toContain('modifyAllRecords: true'); + }); + + it('the refusal reaches through a whole permission set, located at objects..', () => { + const r = PermissionSetSchema.safeParse({ + name: 'sales_manager', + objects: { crm_opportunity: { allowRead: true, viewAllRecords: true, readScope: 'own_and_reports' } }, + }); + expect(r.success).toBe(false); + expect(r.error!.issues.some((i) => i.path.join('.') === 'objects.crm_opportunity.readScope')).toBe(true); + }); + + // ── COST direction: what the refusal must NOT cost ─────────────────── + // A pin set that only proves the new refusal measures nothing about its + // price. These four are the shapes the resolver DOES read, and every one of + // them has to keep parsing. + it('viewAllRecords: false beside a readScope stays accepted — the ordinary, honoured shape', () => { + const parsed = ObjectPermissionSchema.parse({ + allowRead: true, viewAllRecords: false, readScope: 'own_and_reports', + }); + expect(parsed.readScope).toBe('own_and_reports'); + }); + + it('a readScope with the super-user bits merely ABSENT stays accepted (the default is false)', () => { + const parsed = ObjectPermissionSchema.parse({ allowRead: true, readScope: 'unit' }); + expect(parsed.readScope).toBe('unit'); + expect(parsed.viewAllRecords).toBe(false); + }); + + it('viewAllRecords: true ALONE stays accepted — the refusal is about the pair, not the bit', () => { + const parsed = ObjectPermissionSchema.parse({ allowRead: true, viewAllRecords: true }); + expect(parsed.viewAllRecords).toBe(true); + }); + + it('writeScope beside viewAllRecords: true stays ACCEPTED — viewAllRecords does not bypass write', () => { + // The asymmetry is read off the resolver, not assumed: the write + // short-circuit is `opClass === 'write' && op.modifyAllRecords`, which does + // not name `viewAllRecords`. Refusing this pair would delete a grant the + // platform honours. + const parsed = ObjectPermissionSchema.parse({ + allowRead: true, allowEdit: true, viewAllRecords: true, writeScope: 'unit', + }); + expect(parsed.writeScope).toBe('unit'); + }); + + it('the WIRE surface stays tolerant — an older server may still emit a stored pair', () => { + // #4001's authorable/wire split: the refusal rides on the AUTHORING + // wrapper only. `EffectiveObjectPermissionSchema` extends the unrefined + // base, so a response carrying a pair stored before this refusal landed + // does not crash an older or newer client. + const parsed = EffectiveObjectPermissionSchema.parse({ + allowRead: true, viewAllRecords: true, readScope: 'own_and_reports', + }); + expect(parsed.readScope).toBe('own_and_reports'); + }); + + it('the refusal does not disturb the shape read-through the residue stage publishes', () => { + // `acceptRetiredDefaultResidue` re-attaches a read-through `shape`; the + // refinement rides INSIDE it precisely so that stays true (a + // `.superRefine()` on the pipe would have dropped it). + expect(Object.keys(ObjectPermissionSchema.shape)).toContain('readScope'); + expect(Object.keys(ObjectPermissionSchema.shape)).toContain('viewAllRecords'); + }); +}); + describe('allowRestore / allowPurge are RETIRED (#12497, ADR-0049)', () => { // Removed by the 2026-08-26 maintainer ruling accepting #1883's // recommendation B: the `restore`/`purge` ObjectQL operations the bits diff --git a/packages/spec/src/security/permission.zod.ts b/packages/spec/src/security/permission.zod.ts index c61f48c4bd..7c07328534 100644 --- a/packages/spec/src/security/permission.zod.ts +++ b/packages/spec/src/security/permission.zod.ts @@ -89,6 +89,88 @@ const OBJECT_PERMISSION_RETIRED_KEY_RESIDUE = { allowPurge: false, } as const; +/** + * [#16870] A depth axis declared beside the super-user bit that short-circuits + * it — refused, because nothing ever reads it. + * + * `PermissionEvaluator.getEffectiveScope` answers `org` on the super-user bit + * BEFORE it consults the depth key, and `getDeclaredScope` (the ADR-0090 D10 + * delegated-path input) carries the identical short-circuit ahead of the + * identical read — so the declared narrowing is dropped on the direct read + * path AND from the delegation fold. Which pairs are unreadable is read off + * those two short-circuits exactly, and is asymmetric: + * + * - `readScope` beside `viewAllRecords: true` — unread (`opClass === 'read' && + * (op.viewAllRecords || op.modifyAllRecords)`). + * - `readScope` beside `modifyAllRecords: true` — unread, same disjunct. + * - `writeScope` beside `modifyAllRecords: true` — unread (`opClass === + * 'write' && op.modifyAllRecords`). + * - `writeScope` beside `viewAllRecords: true` — READ AND HONOURED. The write + * short-circuit does not name `viewAllRecords`, so this pair stays accepted; + * refusing it would delete a legitimate grant. + * + * ## Why the accept set and not a warning + * + * Accepting-and-ignoring is the whole defect: the declaration materialises + * into `sys_permission_set.object_permissions` and a capability census reading + * the deployed shape counts it as coverage — the author declares a narrowing, + * the platform stores it, the audit reports the capability as exercised, and + * the read is still org-wide. A diagnostic raised after the shape is stored + * leaves that headline harm intact, so the refusal has to happen at the door + * (ADR-0049 `declared ≠ enforced`). ⛔ It does NOT change what + * `viewAllRecords: true` grants — the contradictory declaration is refused, + * the super-user semantics are untouched. + * + * The scope is ONE object-permission entry, which is exactly the resolver's + * input: `resolveObjectPermission` returns a single entry (explicit, else the + * `'*'` wildcard) and never merges two. A super-user bit in one set widening + * past another set's `readScope` is the documented additive "widest wins" + * semantics of ADR-0090, not a contradictory declaration, and is not judged + * here. + * + * Module-private on purpose: it has exactly one consumer, the schema below. + * Exporting it would put a refinement helper on the published API surface (the + * `security` barrel re-exports this module with `export *`) for no caller. + */ +function checkScopeAgainstSuperUserBits( + value: { + viewAllRecords?: boolean; + modifyAllRecords?: boolean; + readScope?: string; + writeScope?: string; + }, + ctx: z.RefinementCtx, +): void { + const bypassesRead = value.viewAllRecords === true || value.modifyAllRecords === true; + if (value.readScope !== undefined && bypassesRead) { + const bit = value.viewAllRecords === true ? 'viewAllRecords' : 'modifyAllRecords'; + ctx.addIssue({ + code: 'custom', + path: ['readScope'], + message: + `readScope: '${value.readScope}' is declared beside ${bit}: true, which already grants ` + + 'org-wide read. The read-scope resolver answers `org` on the super-user bit before it ' + + 'consults readScope, so this depth is never enforced on any read path — and the ' + + 'delegated (ADR-0090 D10) fold drops it too. Accepting it would store a narrowing ' + + 'that changes nothing and let a capability census count it as coverage. Delete ' + + `readScope if the org-wide read is intended, or set ${bit}: false if the narrowing is.`, + }); + } + if (value.writeScope !== undefined && value.modifyAllRecords === true) { + ctx.addIssue({ + code: 'custom', + path: ['writeScope'], + message: + `writeScope: '${value.writeScope}' is declared beside modifyAllRecords: true, which ` + + 'already grants org-wide write. The write-scope resolver answers `org` on ' + + 'modifyAllRecords before it consults writeScope, so this depth is never enforced on ' + + 'any write path. Delete writeScope if the org-wide write is intended, or set ' + + 'modifyAllRecords: false if the narrowing is. (writeScope beside viewAllRecords: true ' + + 'is a DIFFERENT shape and stays accepted — viewAllRecords does not bypass write.)', + }); + } +} + /** * The closed authoring shape, module-private: {@link ObjectPermissionSchema} * is this shape behind the #12840 residue-tolerance stage, and @@ -330,7 +412,20 @@ const ObjectPermissionBaseSchema = lazySchema(() => strictObject( * them, and the same check on parsed output measures nothing at all. */ export const ObjectPermissionSchema = lazySchema(() => - acceptRetiredDefaultResidue(ObjectPermissionBaseSchema, OBJECT_PERMISSION_RETIRED_KEY_RESIDUE), + acceptRetiredDefaultResidue( + // [#16870] The contradictory-pair refusal rides on the BASE, inside the + // residue stage, for two measured reasons. (1) In zod 4 `.superRefine()` + // on a `ZodObject` returns a `ZodObject` that keeps `.shape`, while the + // same call on the residue PIPE returns a schema with no `.shape` — and + // the pipe's read-through `shape` is what shape-reading consumers and the + // schema walkers duck-test. (2) `EffectiveObjectPermissionSchema` below + // extends `ObjectPermissionBaseSchema` — the UNREFINED base — so the WIRE + // surface stays tolerant, which is the #4001 authorable/wire split: a + // server still running an older toolchain may emit a stored pair in an + // effective-permission response, and an older client must not crash on it. + ObjectPermissionBaseSchema.superRefine(checkScopeAgainstSuperUserBits), + OBJECT_PERMISSION_RETIRED_KEY_RESIDUE, + ), ); /**