fix(plugin-sharing): guard the record-share $in against a nullish record_id before String() coerces it

.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.

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` |

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<typeof makeFakeEngine>;
+  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']);
+  });
+});

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 } }] };