diff --git a/.changeset/sharing-granted-ids-nullish-guard.md b/.changeset/sharing-granted-ids-nullish-guard.md new file mode 100644 index 0000000000..2bb3fe94f1 --- /dev/null +++ b/.changeset/sharing-granted-ids-nullish-guard.md @@ -0,0 +1,45 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(plugin-sharing): the record-share `$in` guard now tests `record_id` before `String()` coerces it (#13551) + +`buildReadFilter` and the bulk-write half of `buildWriteFilter` each turned the +`sys_record_share` rows granted to the caller into the members of a security +predicate, `{ id: { $in: [...] } }`, with the same expression: + +```ts +grants.map((g: any) => String(g.record_id)).filter(Boolean) +``` + +`.filter(Boolean)` reads as "drop rows whose `record_id` is nullish". It cannot: +`String(null)` is `'null'` and `String(undefined)` is `'undefined'`, and both are +truthy. The only value that spelling could drop was the empty string, so the +guard was dead for exactly the case its spelling advertised, and a +`sys_record_share` row with a nullish `record_id` put the literal string +`'null'` into the emitted `$in`. + +**Direction — this was not an open bypass, and the repair is not a bypass fix.** +The emitted member is a bogus id that matches no row on any backend, and both +sites are positive polarity (an OR-ed branch beside the owner match, never +negated), so a corrupt row lost its grant rather than widening anyone's scope. +It also took an already-corrupt row to reach at all. What was actually broken is +the guard's honesty: a reader — or an audit asking which security paths already +handle nullish ids — would have counted these two sites as covered when they +provably were not. + +Both sites now share one module-private helper that tests the raw column value +first and coerces after, the shape the sibling id-list guards already use +(`plugin-sharing`'s own `sharing-rule-service.ts` and `primary-bu-projection.ts`, +`core`'s `resolve-authz-context.ts`, `plugin-security`'s controlled-by-parent +`masterIds`, `objectql`'s master-detail parent resolution). Factoring it into one +helper is deliberate: the expression stood in two places, and repairing one would +have left the other advertising a guarantee it does not keep. + +The non-null path is unchanged. Every non-nullish value still stringifies exactly +as it did — a driver-numeric primary key still becomes its decimal string — and +the empty string, the one value the old spelling really did drop, is still +dropped. The only behavioural difference is that rows with a nullish `record_id` +now contribute no member at all; when they were the *only* grants, the filter +collapses to the plain owner match instead of OR-ing in a branch that matched +nothing. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index a4cc57e713..91cc436dd9 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -129,11 +129,11 @@ The largest single consumer — **20 of the 109 sites**. | # | Behaviour when `isSystem` | What you get / what you lose | Anchor | |:--|:---|:---|:---| | 30 | **Sharing-rule grant materialisation is skipped on all four record-write hooks** | Lose: **no `sys_record_share` rows are created**. A fully configured sharing rule grants **nothing** on seeded data until a rule is re-evaluated or the boot backfill runs. This is the behaviour that motivated #4707. Since #6783 the skip is no longer silent — it emits an INFO notice (rough edge 2) | `rule-hooks.ts:250`, `:274`, `:293`, `:322` | -| 31 | Sharing write verdict short-circuits to `allow` | Get: writes pass the sharing gate unconditionally | `plugin-sharing/src/sharing-service.ts:625` | -| 32 | Record visibility / manage-shares checks return true | Get: no ownership or Modify-All requirement | `plugin-sharing/src/sharing-service.ts:891`, `:978`, `:1568` | -| 33 | `grant()` skips the enforcement + manage-shares assertions | Get: the rule evaluator can materialise through the public API. Note it is **not** a bare skip: the system branch asserts the grant is not *inert* instead (a grant on an object no verdict can consult is refused) | `plugin-sharing/src/sharing-service.ts:1179` | -| 34 | `revoke()` deletes directly, **before** the non-manual-source guard | Get: the evaluator can revoke its own grants. Lose: the `CONFLICT` guard that warns a rule-materialised grant will be silently re-granted on the next reconcile | `plugin-sharing/src/sharing-service.ts:1257` (guard at `:1282`) | -| 35 | `listShares()` skips the management gate | Get: full enumeration of who can see a record | `plugin-sharing/src/sharing-service.ts:1309` | +| 31 | Sharing write verdict short-circuits to `allow` | Get: writes pass the sharing gate unconditionally | `plugin-sharing/src/sharing-service.ts:654` | +| 32 | Record visibility / manage-shares checks return true | Get: no ownership or Modify-All requirement | `plugin-sharing/src/sharing-service.ts:920`, `:1007`, `:1597` | +| 33 | `grant()` skips the enforcement + manage-shares assertions | Get: the rule evaluator can materialise through the public API. Note it is **not** a bare skip: the system branch asserts the grant is not *inert* instead (a grant on an object no verdict can consult is refused) | `plugin-sharing/src/sharing-service.ts:1208` | +| 34 | `revoke()` deletes directly, **before** the non-manual-source guard | Get: the evaluator can revoke its own grants. Lose: the `CONFLICT` guard that warns a rule-materialised grant will be silently re-granted on the next reconcile | `plugin-sharing/src/sharing-service.ts:1286` (guard at `:1311`) | +| 35 | `listShares()` skips the management gate | Get: full enumeration of who can see a record | `plugin-sharing/src/sharing-service.ts:1338` | | 36 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `sharing-plugin.ts:1077` | | 37 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link creation/resolution while the policy is off | `plugin-sharing/src/share-link-service.ts:413`, `:467`, `:471`, `:544`, `:574` | | 38 | Sharing-rule provenance stamp skipped | Lose: the row is not marked as an admin customization — seeder / `defineRule` / boot reconcilers are "the package door" | `sharing-rule-provenance.ts:47` | diff --git a/packages/plugins/plugin-sharing/src/sharing-service.test.ts b/packages/plugins/plugin-sharing/src/sharing-service.test.ts index 7bd0f24b33..8e63b5ccd0 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.test.ts @@ -1704,3 +1704,85 @@ describe('[#6428] the boolean projection does not drift (compatibility clause)', expect(await svc.checkEdit('account', 'a1', { isSystem: true })).toBe('allow'); }); }); + +// ───────────────────────────────────────────────────────────────────── + +describe('[#13551] the record-share `$in` drops nullish `record_id` rows', () => { + // The guard standing in front of BOTH `$in` constructions used to read + // `.map((g) => String(g.record_id)).filter(Boolean)`, which cannot drop a + // nullish `record_id`: `String(null)` is `'null'`, `String(undefined)` is + // `'undefined'`, and both are truthy. What follows pins the MECHANISM — a + // row with no `record_id` contributes no member — and asserts nothing about + // whether such a row exists in the wild. + // + // The rows are seeded straight into the fake table on purpose: `grant()` + // refuses a nullish `recordId` at the front door, so writing the row + // directly is the only way to stand up the already-corrupt state the guard + // exists for — the shape a bad backfill or an out-of-band + // `sys_record_share` write would leave behind. + let engine: ReturnType; + let svc: SharingService; + + const shareRow = (record_id: unknown) => ({ + id: `shr_${String(record_id)}`, + object_name: 'account', + record_id, + recipient_type: 'user', + recipient_id: 'alice', + access_level: 'edit', // in WRITE_ACCESS_LEVELS, so the write filter reads it too + }); + + beforeEach(() => { + engine = makeFakeEngine({ + account: ACCOUNT_SCHEMA, + sys_record_share: { name: 'sys_record_share' }, + }); + svc = new SharingService({ engine }); + }); + + it('read filter: a null / undefined `record_id` contributes NO member, and the real grant survives', async () => { + engine._tables.sys_record_share = [shareRow('a1'), shareRow(null), shareRow(undefined)]; + const f: any = await svc.buildReadFilter('account', { userId: 'alice' }); + expect(f.$or[1].id.$in).toEqual(['a1']); + // Named literally: these are the two members the dead guard used to emit. + expect(f.$or[1].id.$in).not.toContain('null'); + expect(f.$or[1].id.$in).not.toContain('undefined'); + }); + + it('write filter: the same rows, the same outcome — both construction sites are repaired', async () => { + engine._tables.sys_record_share = [shareRow('a1'), shareRow(null), shareRow(undefined)]; + const f: any = await svc.buildWriteFilter('account', { userId: 'alice' }, 'update'); + expect(f.$or[1].id.$in).toEqual(['a1']); + expect(f.$or[1].id.$in).not.toContain('null'); + expect(f.$or[1].id.$in).not.toContain('undefined'); + }); + + it('when EVERY grant is nullish the share branch disappears, on both filters', async () => { + engine._tables.sys_record_share = [shareRow(null), shareRow(undefined)]; + // Not an `$or` carrying a member that matches nothing: zero usable grants + // collapses to the owner match, which is what "no grants" already meant. + expect(await svc.buildReadFilter('account', { userId: 'alice' })) + .toEqual({ owner_id: 'alice' }); + expect(await svc.buildWriteFilter('account', { userId: 'alice' }, 'update')) + .toEqual({ owner_id: 'alice' }); + }); + + it('over-denial control: an ordinary grant set still produces exactly its ids, on both filters', async () => { + engine._tables.sys_record_share = [shareRow('a1'), shareRow('a2'), shareRow('a3')]; + const read: any = await svc.buildReadFilter('account', { userId: 'alice' }); + const write: any = await svc.buildWriteFilter('account', { userId: 'alice' }, 'update'); + expect(read.$or[0]).toEqual({ owner_id: 'alice' }); + expect(write.$or[0]).toEqual({ owner_id: 'alice' }); + expect(read.$or[1].id.$in).toEqual(['a1', 'a2', 'a3']); + expect(write.$or[1].id.$in).toEqual(['a1', 'a2', 'a3']); + }); + + it('the non-null path is unchanged: a driver-numeric id still stringifies, an empty string is still dropped', async () => { + engine._tables.sys_record_share = [shareRow(42), shareRow(''), shareRow('a1')]; + const f: any = await svc.buildReadFilter('account', { userId: 'alice' }); + // `42` becomes `'42'` exactly as `String()` always made it, and `''` — the + // one value the old `.filter(Boolean)` could actually drop — is still + // dropped. Only the nullish rows are newly excluded. + expect(f.$or[1].id.$in).toEqual(['42', 'a1']); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-service.ts b/packages/plugins/plugin-sharing/src/sharing-service.ts index b97e31c29a..888f774853 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.ts @@ -143,6 +143,39 @@ function hasOwnerField(schema: any): boolean { return Boolean(schema?.fields && OWNER_FIELD in schema.fields); } +/** + * The `record_id` column of a `sys_record_share` read, as the members of a + * security `$in`. A row whose `record_id` is nullish or empty contributes + * NOTHING — the nullish test runs on the RAW column value, BEFORE `String()`. + * + * The order is the whole point. The previous spelling coerced first and + * filtered after — `grants.map((g) => String(g.record_id)).filter(Boolean)` — + * which cannot drop a nullish `record_id` at all: `String(null)` is `'null'` + * and `String(undefined)` is `'undefined'`, and both are truthy. The only + * value it could drop was the empty string, so the guard was dead for exactly + * the case its spelling advertised, and a corrupt row put the literal string + * `'null'` into `{ id: { $in: [...] } }`. Both call sites are positive + * polarity (an OR-ed branch, never negated) and no real record id matches that + * member, so the effect was a silently DROPPED grant rather than a widened + * scope — but an audit asking which security paths already handle nullish ids + * would have counted these two as covered when they provably were not. + * + * `String()` is kept for the surviving values: a driver may hand back a + * numeric primary key, and the members must compare against the string ids the + * rest of the filter is built from. Every non-nullish value therefore + * stringifies exactly as it did before, and the trailing `!== ''` drops + * precisely what `filter(Boolean)` used to drop — so the non-null path is + * unchanged and only the nullish rows are newly excluded. + */ +function grantedRecordIds(grants: unknown): string[] { + if (!Array.isArray(grants)) return []; + return grants + .map((g: any) => g?.record_id) + .filter((recordId: unknown) => recordId != null) + .map((recordId: unknown) => String(recordId)) + .filter((recordId: string) => recordId !== ''); +} + /** * [#8418] The one WARN line a write gate emits when it refuses because the * ownership fast-path was defeated by a FEDERATED object's phantom `owner_id` @@ -416,9 +449,7 @@ export class SharingService implements ISharingService { context: SYSTEM_CTX, }); - const grantedIds: string[] = Array.isArray(grants) - ? grants.map((g: any) => String(g.record_id)).filter(Boolean) - : []; + const grantedIds: string[] = grantedRecordIds(grants); if (grantedIds.length === 0) { return ownerMatch; @@ -494,9 +525,7 @@ export class SharingService implements ISharingService { limit: 5000, context: SYSTEM_CTX, }); - const grantedIds: string[] = Array.isArray(grants) - ? grants.map((g: any) => String(g.record_id)).filter(Boolean) - : []; + const grantedIds: string[] = grantedRecordIds(grants); if (grantedIds.length === 0) return ownerMatch; return { $or: [ownerMatch, { id: { $in: grantedIds } }] };