From 0cac504d5041734a060abaa711320ccf6c06040c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 13:56:21 +0000 Subject: [PATCH 1/2] fix(plugin-sharing): scope getRule's by-id branch to the caller's organization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SharingRuleService.getRule` resolved an id with a bare `{id: idOrName}` predicate under SYSTEM_CTX, so nothing re-scoped it downstream. An org-scoped sharing admin holding another organization's `srule_...` id could read that org's rule, evaluate it, and — because `deleteRule` resolves through `getRule` — delete it together with every `sys_record_share` grant it had materialised, silently revoking another tenant's record access. The by-id lookup now carries the same `adminOrgScope` predicate #7760 gave the by-name path: `id = {id} AND (organization_id = {orgId} OR organization_id IS NULL)` when the caller carries an organization, and unfiltered when it does not, so system/boot contexts are unchanged. A platform-global (organization_id = null) row stays reachable by id. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BVc1ekPpi6yaWywAUhfzfd --- .../src/sharing-rule-service.ts | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 2f280894d8..17c3476977 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -250,6 +250,12 @@ export class SharingRuleService implements ISharingRuleService { * a cross-tenant read of a platform-global row. A same-named POST therefore * still creates an org-stamped row of the tenant's own, and * {@link findRuleRowByName} prefers it. + * + * [#7761] It IS applied to {@link getRule}'s by-**id** branch, which the + * second paragraph above records as the one read that still worked while + * everything else was over-scoped. That was never a feature: unfiltered + * meant an org admin could resolve — and, through {@link deleteRule}, + * destroy — another organization's rule from its id alone. */ private adminOrgScope(where: Record, orgId: string | null | undefined): Record { if (!orgId) return where; @@ -280,8 +286,20 @@ export class SharingRuleService implements ISharingRuleService { if (!idOrName) return null; // `organizationId` is not on the envelope — see defineRule(). const orgId = (context as any)?.organizationId ?? context?.tenantId; + // [#7761] The by-id branch carries the SAME tenant scope as the by-name + // path — it used to be a bare `{id: idOrName}`, resolved under SYSTEM_CTX + // so nothing downstream re-scoped it. An org-scoped sharing admin holding + // another organization's opaque `srule_…` id could therefore read that + // org's rule, `evaluate` it, and — because {@link deleteRule} resolves + // through here — DELETE it along with every `sys_record_share` grant it + // had materialised, i.e. silently revoke another tenant's record access. + // `manage_sharing` is an org-level capability (`scope: 'org'` in the spec's + // capability registry) and an id is not a tenant boundary: ids leak through + // logs, exports, support tickets and the evaluate response's `{ruleId}`. + // A platform-global (`organization_id = null`) row stays reachable, for + // symmetry with the by-name path — see {@link adminOrgScope}. const byId = await this.engine.find('sys_sharing_rule', { - where: { id: idOrName }, + where: this.adminOrgScope({ id: idOrName }, orgId), limit: 1, context: SYSTEM_CTX, }); From 40d57ec3f19fe81acb3fa1b1a2e2e69072bf7c56 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 14:18:56 +0000 Subject: [PATCH 2/2] test(plugin-sharing): pin by-id tenant isolation for getRule/evaluateRule/deleteRule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the [#7761] describe block: another organization's rule is unreachable by id across all three verbs — and the delete pin asserts the victim's `sys_record_share` grants survive, not just its rule row, because grant purging is the actual harm. Platform-global (organization_id = null) rows, the caller's own rows, and no-org boot contexts are pinned as unchanged. Also adds the changeset (patch, @objectstack/plugin-sharing). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BVc1ekPpi6yaWywAUhfzfd --- .changeset/sharing-rule-byid-org-scope.md | 39 +++++ .../plugin-sharing/src/sharing-rule.test.ts | 144 ++++++++++++++++++ 2 files changed, 183 insertions(+) create mode 100644 .changeset/sharing-rule-byid-org-scope.md diff --git a/.changeset/sharing-rule-byid-org-scope.md b/.changeset/sharing-rule-byid-org-scope.md new file mode 100644 index 0000000000..3ad745aff2 --- /dev/null +++ b/.changeset/sharing-rule-byid-org-scope.md @@ -0,0 +1,39 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(plugin-sharing): scope `getRule`'s by-id branch to the caller's organization (#7761) + +**Cross-tenant security fix.** `SharingRuleService.getRule` resolved a rule id +with a bare `{id: idOrName}` predicate and no organization filter, executed +under the service's `SYSTEM_CTX` so nothing re-scoped it downstream. An +org-scoped sharing admin who held another organization's opaque `srule_…` id +could therefore reach that organization's rule through all three verbs that +resolve through `getRule`: + +- `GET /api/v1/sharing/rules/:id` — read another tenant's rule, including its + criteria, recipient and access level; +- `POST /api/v1/sharing/rules/:id/evaluate` — materialise that tenant's grants + on demand; +- `DELETE /api/v1/sharing/rules/:id` — delete the rule **and purge every + `sys_record_share` grant it had materialised**, silently revoking another + tenant's record access. + +The caller still needed `manage_sharing` (or the legacy +`manage_platform_settings`) in their own organization, but that is an +org-scoped capability — `scope: 'org'` in the spec's capability registry — and +a rule id is not a tenant boundary: ids leak through logs, exports, support +tickets, and the evaluate endpoint's own `{ruleId}` response. + +The by-id lookup now carries the same tenant predicate the by-name path has +carried since #7676: `id = {id} AND (organization_id = {orgId} OR +organization_id IS NULL)` when the caller carries an organization. Two +behaviours are deliberately preserved: a no-org (system / boot) context still +resolves any row by id, so boot seeding, hooks and backfills are unaffected; +and a platform-global (`organization_id = null`) row stays reachable by id, for +symmetry with the by-name path. + +Reaching another organization's rule by id is now indistinguishable from +addressing one that does not exist — `getRule` answers `null` (REST: 404), +`evaluateRule` throws `RULE_NOT_FOUND`, and `deleteRule` is a no-op that leaves +the row and its grants intact. diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index 92669d4593..78704802fc 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -1019,3 +1019,147 @@ describe('[#7676] admin visibility of package-seeded (org-null) sharing rules', expect(resolved?.organization_id).toBe('org1'); }); }); + +// ───────────────────────────────────────────────────────────────────── +// [#7761] `getRule`'s BY-ID branch is scoped to the caller's organization. +// +// The by-name path has been scoped since #7676; the by-id branch was a bare +// `{id: idOrName}` resolved under SYSTEM_CTX, so nothing re-scoped it +// downstream. An org-scoped sharing admin holding another organization's +// opaque `srule_…` id could read that org's rule, `evaluate` it, and — because +// `deleteRule` resolves through `getRule` — DELETE it along with every +// `sys_record_share` grant it had materialised: silently revoking another +// tenant's record access. `manage_sharing` is an org-level capability +// (`scope: 'org'`) and an id is not a tenant boundary — ids leak through logs, +// exports, support tickets and the evaluate response's `{ruleId}`. +// +// The delete assertions pin the ROW **and its grants**, because grant purging +// is the actual harm: a fix that kept the row but still purged the grants +// would satisfy a row-only assertion while leaving the damage intact. +// +// Ablation (predicted in advance): restore `where: {id: idOrName}` and the +// three "unreachable by id" tests flip red, while the platform-global, +// own-org and BOOT pins below stay green. +// ───────────────────────────────────────────────────────────────────── + +describe('[#7761] getRule by-id is scoped to the caller organization', () => { + let engine: ReturnType; + let rules: SharingRuleService; + + /** Authenticated org-scoped sharing admins — the shape the REST layer builds. */ + const ORG1_ADMIN = { userId: 'admin', organizationId: 'org1', systemPermissions: ['manage_sharing'] } as any; + const ORG2_ADMIN = { userId: 'other', organizationId: 'org2', systemPermissions: ['manage_sharing'] } as any; + /** + * The seeder's / boot context — carries NO organization, which is what + * stamps `organization_id: null`. Deliberately not this file's older `SYS` + * (`{isSystem: true, organizationId: 'org1'}`): `isSystem` bypasses the + * ADR-0111 D6 capability gate, never the org scope, so `SYS` would silently + * defeat every fixture below. + */ + const BOOT = { isSystem: true, positions: [], permissions: [] } as any; + + const SEEDED = 'share_red_projects_with_execs'; + let seededId = ''; + let otherOrgRuleId = ''; + let org1RuleId = ''; + + /** The `sys_record_share` rows a given rule materialised. */ + const grantsOf = (ruleId: string): Row[] => + (engine._tables.sys_record_share ?? []).filter((g) => g.source === 'rule' && g.source_id === ruleId); + + beforeEach(async () => { + engine = makeEngine(); + engine._tables.project = [ + { id: 'p_red', status: 'red', owner_id: 'someone' }, + { id: 'p_green', status: 'green', owner_id: 'someone' }, + ]; + rules = new SharingRuleService({ engine: engine as any, sharing: new SharingService({ engine: engine as any }) }); + + // Platform-global package seed — defined with no org, as the boot seeder does. + seededId = (await rules.defineRule({ + name: SEEDED, label: 'Red projects → execs', object: 'project', + criteria: { status: 'red' }, recipientType: 'user', recipientId: 'exec', + managedBy: 'package', + } as any, BOOT)).id; + // The victim: a rule owned by a DIFFERENT organization. + otherOrgRuleId = (await rules.defineRule({ + name: 'other_org_rule', label: 'Other org', object: 'project', + criteria: { status: 'green' }, recipientType: 'user', recipientId: 'mallory', + } as any, ORG2_ADMIN)).id; + // The caller's own rule — the positive half. + org1RuleId = (await rules.defineRule({ + name: 'org1_rule', label: 'Org1 own', object: 'project', + criteria: { status: 'red' }, recipientType: 'user', recipientId: 'alice', + } as any, ORG1_ADMIN)).id; + + // Materialise org2's grants under BOOT, so the fixture does not depend on + // the very scoping decision these tests are measuring. + await rules.evaluateRule(otherOrgRuleId, BOOT); + }); + + it('the fixture really does have three distinct rows and live grants on the victim', () => { + const rows = engine._tables.sys_sharing_rule; + expect(rows.find((r) => r.id === seededId)?.organization_id).toBeNull(); + expect(rows.find((r) => r.id === otherOrgRuleId)?.organization_id).toBe('org2'); + expect(rows.find((r) => r.id === org1RuleId)?.organization_id).toBe('org1'); + expect(new Set([seededId, otherOrgRuleId, org1RuleId]).size).toBe(3); + // Without this, the delete pin below could "pass" over a rule that never + // had any grants to lose. + expect(grantsOf(otherOrgRuleId)).toHaveLength(1); + }); + + // ── the defect: another organization's row, addressed by id ────────── + + it('another organization’s rule is unreachable BY ID — read', async () => { + expect(await rules.getRule(otherOrgRuleId, ORG1_ADMIN)).toBeNull(); + }); + + it('another organization’s rule is unreachable BY ID — evaluate', async () => { + await expect(rules.evaluateRule(otherOrgRuleId, ORG1_ADMIN)).rejects.toThrow(/RULE_NOT_FOUND/); + // Evaluate is a WRITE (it reconciles grants), so the refusal has to leave + // the victim's grants exactly as they were, not merely return an error. + expect(grantsOf(otherOrgRuleId)).toHaveLength(1); + }); + + it('another organization’s rule is unreachable BY ID — delete leaves the row AND its grants intact', async () => { + await rules.deleteRule(otherOrgRuleId, ORG1_ADMIN); + expect(engine._tables.sys_sharing_rule.find((r) => r.id === otherOrgRuleId)).toBeTruthy(); + // The harm in this defect is grant purging — `deleteRule` revokes every + // `sys_record_share` row the rule materialised — so the grants are the + // assertion that matters, not just the surviving rule row. + expect(grantsOf(otherOrgRuleId)).toHaveLength(1); + }); + + // ── platform-global stays reachable (what #7760 enabled by name) ───── + + it('a platform-global (organization_id = null) rule stays reachable BY ID — read', async () => { + const row = await rules.getRule(seededId, ORG1_ADMIN); + expect(row?.name).toBe(SEEDED); + expect(row?.organization_id).toBeNull(); + }); + + it('a platform-global rule stays reachable BY ID — evaluate', async () => { + const res = await rules.evaluateRule(seededId, ORG1_ADMIN); + expect(res.ruleId).toBe(seededId); + expect(res.matchedRecords).toBe(1); + expect(res.grantsCreated).toBe(1); + }); + + // ── unchanged behaviour ────────────────────────────────────────────── + + it('the caller’s OWN org rule is still fully addressable by id', async () => { + expect((await rules.getRule(org1RuleId, ORG1_ADMIN))?.organization_id).toBe('org1'); + expect((await rules.evaluateRule(org1RuleId, ORG1_ADMIN)).ruleId).toBe(org1RuleId); + // Delete works on the caller's own row — the control proving the refusal + // above comes from the org scope and not from a delete path that stopped + // working for everyone. + await rules.deleteRule(org1RuleId, ORG1_ADMIN); + expect(engine._tables.sys_sharing_rule.find((r) => r.id === org1RuleId)).toBeUndefined(); + }); + + it('a no-org (SYSTEM_CTX / boot) context still resolves ANY row by id', async () => { + expect((await rules.getRule(otherOrgRuleId, BOOT))?.organization_id).toBe('org2'); + expect((await rules.getRule(seededId, BOOT))?.organization_id).toBeNull(); + expect((await rules.getRule(org1RuleId, BOOT))?.organization_id).toBe('org1'); + }); +});