fix(plugin-audit): forward the caller's execution envelope to the sys_comment sharing gates (#7141)

.changeset/comment-gate-caller-envelope.md

@@ -0,0 +1,56 @@
+---
+"@objectstack/plugin-audit": patch
+---
+
+fix(plugin-audit): forward the caller's full execution envelope to the `sys_comment` sharing gates (#7141)
+
+`callerContext()` in `comment-access-hooks.ts` rebuilt a five-field projection
+of the caller's `ExecutionContext` (`userId` / `tenantId` / `positions` /
+`permissions` / `isSystem`) before handing it to `ISharingService.canEdit`,
+whose contract declares the **full** envelope and whose doc block tells callers
+they "MUST NOT rebuild a subset of it" (#6523 / the #6206 ruling). #7136 (PR
+#7140) widened the return *annotation*; this is the body.
+
+The projection was doing two jobs at once and only one of them was correct:
+
+- **Dropping the middleware-private keys was correct**, and is preserved.
+  plugin-security's middleware stamps the access DEPTH it resolved for the
+  object of the operation in flight — `sys_comment` — onto the context in place
+  (`sc.__readScope = …`), while these gates ask the sharing service about the
+  **parent record's** object. Forwarding that whole would hand one object's
+  widening to another object's owner-match, the stale-scope leak
+  `resolveWriteScopeForSharing` was extracted to prevent. The keys are now
+  dropped by the `__` **prefix** rather than by name, which also covers the
+  engine's other operation-private markers on that channel (`__expandRead`
+  waives the object-level CRUD check, `__referentialFieldClear` the
+  referential-clear write) and cannot go stale when a fifth key is added.
+- **Dropping the principal fields was the defect.** Two of them decide the
+  verdict this gate then trusts:
+  - `onBehalfOf` — `ISecurityService.hasWriteBypass`, the `modifyAllRecords`
+    probe `SharingService.canEdit` consults last, is documented to fail CLOSED
+    on a delegated context and implements that by reading exactly
+    `context?.onBehalfOf?.userId`. Stripped, the guard could never fire on this
+    path, and the `/mcp` OAuth agent principal that `resolve-execution-context`
+    builds *with* the delegation link reached the bypass probe looking like an
+    ordinary direct call.
+  - `principalKind` — `resolvePermissionSetsForContext` keys the ADR-0090 D10
+    rule "an agent's grants are EXACTLY its scope-derived ceiling" on
+    `principalKind === 'agent'`. Stripped, the additive human baseline was
+    appended to an agent's ceiling here, so the sets the bypass probe evaluated
+    were a superset of what the user consented to.
+
+  `systemPermissions`, `accessible_org_ids`, `posture`, `audience` and
+  `rlsMembership` were dropped by the same projection and are forwarded now for
+  the same reason.
+
+The same envelope-minus-private-keys rule is applied to the read side's
+parent-record probe, which spread the whole operation context into a `find` on
+a different object.
+
+No access depth is synthesised for the parent object: absent depth leaves the
+sharing owner-match at its narrowest (`own`), which is the safe direction and
+byte-for-byte what the projection produced. Resolving the parent's own depth
+would WIDEN this gate and is deliberately left as a separate decision.
+
+Enforcement effect: a delegated (`onBehalfOf`-carrying) principal is now refused
+where the contract says it is refused. No caller gains access.

packages/plugins/plugin-audit/src/comment-access-hooks.test.ts

@@ -301,3 +301,198 @@ describe('comment access — beforeDelete (author or parent editor)', () => {
     ).resolves.toBeUndefined();
   });
 });
+
+
+// ─────────────────────────────────────────────────────────────────────────
+// #7141 — what the gate FORWARDS to the sharing service
+// ─────────────────────────────────────────────────────────────────────────
+
+/**
+ * The caller's execution envelope as a real transport builds it — an OAuth MCP
+ * agent principal acting on behalf of a human (`resolve-execution-context.ts`
+ * is the live producer of `principalKind: 'agent'` + `onBehalfOf`) — with the
+ * middleware-private keys plugin-security stamps for the object of the
+ * operation in flight (`sys_comment`) riding along, because that is exactly
+ * what `sc.__readScope = …` leaves on the context these hooks receive.
+ */
+const DELEGATED_ENVELOPE = {
+  userId: 'human_1',
+  tenantId: 'org_1',
+  email: 'human@example.com',
+  positions: [],
+  permissions: ['mcp_agent_data_write'],
+  systemPermissions: [],
+  principalKind: 'agent',
+  onBehalfOf: { userId: 'human_1', principalKind: 'human' },
+  audience: 'internal',
+  posture: 'authenticated',
+  accessible_org_ids: ['org_1'],
+  rlsMembership: { team: ['t1'] },
+  isSystem: false,
+  // Middleware-private, resolved for `sys_comment` — NOT for the parent.
+  __readScope: 'org',
+  __writeScope: 'org',
+  __delegatorReadScope: 'org',
+  __delegatorWriteScope: 'org',
+  __expandRead: true,
+} as const;
+
+/** The same context, shaped the way the write hooks receive it. */
+const envelopeWriteCtx = (
+  event: 'beforeUpdate' | 'beforeDelete',
+  input: any,
+  exec: Record<string, unknown>,
+) => ({
+  object: 'sys_comment',
+  event,
+  input: { ...input, options: { ...(input.options ?? {}), context: exec } },
+  session: { userId: exec.userId as string },
+  api: apiFor([]),
+});
+
+/** The deployment's `fallbackPermissionSet` (ADR-0056 D7: an app's `isDefault`
+ * profile, else the built-in `member_default`). */
+const DEPLOYMENT_BASELINE_SET = 'app_default_profile';
+
+/**
+ * `ISecurityService.hasWriteBypass` as plugin-security implements it
+ * (`security-plugin.ts`) — the three guard lines, then the `modifyAllRecords`
+ * set probe. A DOUBLE, not a copy of production logic: plugin-audit does not
+ * depend on plugin-security (dependency-free posture), so the only way to pin
+ * the OUTCOME on this side of the seam is to model the contract the gate is
+ * documented to be talking to. `setsWithBypass` names which permission sets
+ * carry the bit in the modelled deployment.
+ */
+function hasWriteBypassDouble(context: any, setsWithBypass: string[]): boolean {
+  if (context?.isSystem) return true;
+  if (!context?.userId) return false;
+  if (context?.onBehalfOf?.userId) return false; // documented fail-CLOSED on delegation
+  // `resolvePermissionSetsForContext`: positions + explicit sets, plus the
+  // ADDITIVE human baseline — which an ADR-0090 D10 agent principal must NOT
+  // receive (its grants are exactly its scope-derived ceiling).
+  const requested = [...(context?.positions ?? []), ...(context?.permissions ?? [])];
+  const resolved =
+    context?.principalKind === 'agent' ? requested : [...requested, DEPLOYMENT_BASELINE_SET];
+  return resolved.some((name: string) => setsWithBypass.includes(name));
+}
+
+/**
+ * `SharingService.checkEdit`'s positive bases, in order: ownership widened by
+ * the middleware-stamped write DEPTH (`matchesOwnerScope` — `__writeScope ===
+ * 'org'` short-circuits to true), then the `modifyAllRecords` bypass. The share
+ * branch is omitted (no grants in these fixtures).
+ */
+function sharingCanEditDouble(opts: { ownerId: string; setsWithBypass?: string[] }) {
+  return vi.fn(async (_object: string, _recordId: string, callerCtx: any) => {
+    if (callerCtx?.isSystem) return true;
+    if (!callerCtx?.userId) return false;
+    if ((callerCtx as any).__writeScope === 'org') return true; // depth fast-exit
+    if (String(callerCtx.userId) === opts.ownerId) return true;
+    return hasWriteBypassDouble(callerCtx, opts.setsWithBypass ?? []);
+  });
+}
+
+describe('#7141 — caller envelope forwarded to the sharing gate', () => {
+  const row = { id: 'c1', thread_id: 'crm_opportunity:opp1', author_id: 'someone_else', body: 'hi' };
+
+  it('forwards the whole envelope MINUS the operation-private keys', async () => {
+    const canEdit = vi.fn(async (_object: string, _recordId: string, _callerCtx: any) => true);
+    const { beforeDelete } = install({ comments: [row], sharing: { canEdit } });
+    await beforeDelete(envelopeWriteCtx('beforeDelete', { id: 'c1' }, { ...DELEGATED_ENVELOPE }));
+
+    const forwarded = canEdit.mock.calls[0]![2] as unknown as Record<string, unknown>;
+    // Every principal field survives — the #6523 contract's unit is the envelope
+    // and #6206 forbids rebuilding a subset of it.
+    expect(forwarded).toEqual({
+      userId: 'human_1',
+      tenantId: 'org_1',
+      email: 'human@example.com',
+      positions: [],
+      permissions: ['mcp_agent_data_write'],
+      systemPermissions: [],
+      principalKind: 'agent',
+      onBehalfOf: { userId: 'human_1', principalKind: 'human' },
+      audience: 'internal',
+      posture: 'authenticated',
+      accessible_org_ids: ['org_1'],
+      rlsMembership: { team: ['t1'] },
+      isSystem: false,
+    });
+    // …and every middleware-private key resolved for `sys_comment` is gone.
+    for (const key of ['__readScope', '__writeScope', '__delegatorReadScope', '__delegatorWriteScope', '__expandRead']) {
+      expect(forwarded).not.toHaveProperty(key);
+    }
+  });
+
+  it('hands the service a COPY, so a callee stamping its own depth cannot write back', async () => {
+    const exec: Record<string, unknown> = { ...DELEGATED_ENVELOPE };
+    const canEdit = vi.fn(async (_o: string, _r: string, callerCtx: any) => {
+      // What plugin-security does right before it calls the sharing service.
+      callerCtx.__writeScope = 'unit';
+      return true;
+    });
+    const { beforeDelete } = install({ comments: [row], sharing: { canEdit } });
+    await beforeDelete(envelopeWriteCtx('beforeDelete', { id: 'c1' }, exec));
+
+    expect(canEdit.mock.calls[0]![2]).not.toBe(exec);
+    expect(exec.__writeScope).toBe('org'); // untouched: still sys_comment's own
+  });
+
+  it('REFUSES a delegated principal whose sets carry modifyAllRecords (fail-closed, #7141)', async () => {
+    // The exploit shape the card names: an OAuth agent on the `/mcp` surface
+    // presenting sets that carry the super-user write bypass. `hasWriteBypass`
+    // is documented to fail CLOSED on `onBehalfOf` — it can only do that if the
+    // field reaches it.
+    const canEdit = sharingCanEditDouble({ ownerId: 'other_owner', setsWithBypass: ['admin_full_access'] });
+    const { beforeDelete } = install({ comments: [row], sharing: { canEdit } });
+    await expect(
+      beforeDelete(
+        envelopeWriteCtx('beforeDelete', { id: 'c1' }, {
+          ...DELEGATED_ENVELOPE,
+          permissions: ['admin_full_access'],
+        }),
+      ),
+    ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 });
+    expect(canEdit).toHaveBeenCalledTimes(1);
+  });
+
+  it('keeps an AGENT principal capped at its ceiling — no additive human baseline (ADR-0090 D10)', async () => {
+    // `resolvePermissionSetsForContext` keys that rule on `principalKind`, which
+    // the old projection dropped: the agent was resolved as a human and the
+    // deployment's default profile was appended to its consented ceiling.
+    const canEdit = sharingCanEditDouble({
+      ownerId: 'other_owner',
+      setsWithBypass: [DEPLOYMENT_BASELINE_SET],
+    });
+    const { beforeDelete } = install({ comments: [row], sharing: { canEdit } });
+    await expect(
+      beforeDelete(
+        envelopeWriteCtx('beforeDelete', { id: 'c1' }, {
+          ...DELEGATED_ENVELOPE,
+          onBehalfOf: undefined, // isolate the ceiling rule from the delegation guard
+        }),
+      ),
+    ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 });
+  });
+
+  it('does NOT carry sys_comment\'s access DEPTH into the parent\'s owner-match', async () => {
+    // The half of the old projection that was CORRECT and must survive: the
+    // context carries `__writeScope: 'org'` resolved for `sys_comment`, and the
+    // gate asks about `crm_opportunity`. Forwarding it whole would widen one
+    // object's question with another object's answer.
+    const canEdit = sharingCanEditDouble({ ownerId: 'other_owner' });
+    const { beforeDelete } = install({ comments: [row], sharing: { canEdit } });
+    await expect(
+      beforeDelete(
+        envelopeWriteCtx('beforeDelete', { id: 'c1' }, {
+          ...DELEGATED_ENVELOPE,
+          principalKind: 'human',
+          onBehalfOf: undefined,
+          userId: 'plain_member',
+          permissions: [],
+        }),
+      ),
+    ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 });
+    expect((canEdit.mock.calls[0]![2] as any).__writeScope).toBeUndefined();
+  });
+});

packages/plugins/plugin-audit/src/comment-access-hooks.ts

@@ -180,35 +180,90 @@ function asIdList(id: unknown): Array<string | number> | null {
   return null;
 }
 
+/**
+ * Keys plugin-security's middleware STAMPS onto the operation context, resolved
+ * for the object of the CURRENT operation — `sys_comment` here.
+ *
+ * They are middleware-private vocabulary, not fields of `ExecutionContext`, and
+ * they are all read as WIDENING inputs by whoever consumes them: the ADR-0057
+ * D1 access DEPTH the sharing owner-match expands to (`__readScope` /
+ * `__writeScope`, plus the ADR-0090 D10 delegator halves
+ * `__delegatorReadScope` / `__delegatorWriteScope`, `security-plugin.ts` — `sc.__readScope = …`),
+ * and the engine's internal privilege markers on the same channel
+ * (`__expandRead` waives the object-level CRUD check for a lookup expansion,
+ * `__referentialFieldClear` the referential-clear write).
+ *
+ * Every gate in this module asks about the PARENT record's object, never about
+ * `sys_comment`, so carrying any of these across is one object's widening
+ * applied to another object's question — the exact stale-scope leak
+ * `resolveWriteScopeForSharing` was extracted to prevent ("a stale value can
+ * never leak in through a spread", `security-plugin.ts`). They are therefore
+ * dropped by PREFIX rather than by a name list: the `__` convention is what
+ * marks a key as belonging to the operation in flight, and a list would go
+ * stale the day the middleware stamps a fifth one.
+ */
+const OPERATION_PRIVATE_KEY_PREFIX = '__';
+
+/**
+ * The caller's execution envelope, minus the operation-private keys above.
+ *
+ * [#7141] A FRESH object every time, so a callee that stamps its own
+ * `__writeScope` onto what it receives (which is exactly what plugin-security
+ * does before it calls the sharing service) can never write back into the
+ * operation context this hook was handed.
+ */
+function withoutOperationPrivateKeys(exec: Record<string, unknown>): ExecutionContext {
+  const out: Record<string, unknown> = {};
+  for (const [key, value] of Object.entries(exec)) {
+    if (key.startsWith(OPERATION_PRIVATE_KEY_PREFIX)) continue;
+    out[key] = value;
+  }
+  return out as ExecutionContext;
+}
+
 /** The caller's ExecutionContext rides on the operation options — the session
  * snapshot lacks `permissions`, which sharing bypasses need.
  *
  * [#7136] Typed as the full envelope, which is what `ISharingService` declares
  * for every parameter this value is handed to (#6523 / the #6206 ruling).
  *
- * ⚠️ The BODY still projects a five-field subset, which the same ruling tells
- * callers not to do — and that half is deliberately NOT changed here, because
- * it is not the inert half. Widening the annotation is type-side; forwarding
- * `exec` whole is a RUNTIME change. plugin-security's middleware MUTATES the
- * operation context in place (`sc.__readScope = …`, `security-plugin.ts`), so
- * the context this hook receives carries the depth resolved for `sys_comment` —
- * the object of the operation — while these gates ask the sharing service about
- * the PARENT record's object. Forwarding it would hand one object's access
- * depth to another object's owner-match, the exact stale-scope leak
- * `resolveWriteScopeForSharing` was extracted to prevent ("a stale value can
- * never leak in through a spread"). This projection is currently what stops
- * that, so replacing it needs its own card and its own evidence — filed rather
- * than folded in. */
+ * [#7141] And FORWARDED as the full envelope, which is the other half of that
+ * ruling: a caller "MUST NOT rebuild a subset of it". The five-field projection
+ * this replaced (`userId` / `tenantId` / `positions` / `permissions` /
+ * `isSystem`) was doing two jobs at once, and only one of them was correct:
+ *
+ *  - dropping the middleware-private keys — CORRECT, and preserved above by
+ *    {@link withoutOperationPrivateKeys}: `return exec;` would hand
+ *    `sys_comment`'s access depth to the parent object's owner-match;
+ *  - dropping the PRINCIPAL fields — the defect. Two of them decide the
+ *    verdict the gate then trusts:
+ *    * `onBehalfOf` — `ISecurityService.hasWriteBypass`, the `modifyAllRecords`
+ *      probe `SharingService.canEdit` consults last, is documented to fail
+ *      CLOSED on a delegated context and implements that by reading exactly
+ *      `context?.onBehalfOf?.userId` (`security-plugin.ts`). Stripped, that
+ *      guard could never fire here, and a `/mcp` OAuth agent principal (which
+ *      `resolve-execution-context.ts` builds WITH the delegation link) reached
+ *      the bypass probe looking like an ordinary direct call.
+ *    * `principalKind` — `resolvePermissionSetsForContext` keys the ADR-0090
+ *      D10 rule "an agent's grants are EXACTLY its scope-derived ceiling" on
+ *      `principalKind === 'agent'`; stripped, the additive human baseline
+ *      (`member_default`) was appended to an agent's ceiling on this path, so
+ *      the sets the bypass probe evaluated were a SUPERSET of what the user
+ *      consented to.
+ *
+ *    `systemPermissions`, `accessible_org_ids`, `posture`, `audience` and
+ *    `rlsMembership` were dropped by the same projection; they are forwarded
+ *    now for the same reason — the envelope is the contract's unit.
+ *
+ * Note what deliberately did NOT change: no access DEPTH is synthesised for the
+ * parent object. Absent depth leaves the sharing owner-match at its narrowest
+ * (`own`) — the safe direction, and byte-for-byte the behaviour the projection
+ * produced. Resolving the parent's own depth (the other candidate shape) would
+ * WIDEN this gate and is a separate decision; see #7141's PR discussion. */
 function callerContext(ctx: any): ExecutionContext {
   const exec = ctx?.input?.options?.context;
   if (exec && typeof exec === 'object') {
-    return {
-      userId: exec.userId,
-      tenantId: exec.tenantId,
-      positions: exec.positions,
-      permissions: exec.permissions,
-      isSystem: exec.isSystem,
-    };
+    return withoutOperationPrivateKeys(exec as Record<string, unknown>);
   }
   const s = ctx?.session ?? {};
   return { userId: s.userId, tenantId: s.tenantId ?? s.organizationId, positions: s.positions };
@@ -506,6 +561,17 @@ async function computeThreadVisibilityFilter(
 
   // 2. Per parent object, the visible id subset via the CALLER's context —
   //    the parent object's own RLS/OWD/sharing applies.
+  //
+  //    [#7141] The caller's envelope, minus the operation-private keys: this
+  //    probe reads a DIFFERENT object than the one the middleware resolved its
+  //    depth for, and `__readScope` / `__expandRead` are widening inputs that
+  //    would arrive attached to the wrong question (the security middleware
+  //    re-stamps the depth for THIS object when it resolves any set, so the
+  //    only thing dropping them can do is leave the owner-match at its
+  //    narrowest — the safe direction). Same rule as `callerContext` above.
+  const callerEnvelope = withoutOperationPrivateKeys(
+    (ctx.context ?? {}) as Record<string, unknown>,
+  );
   const visibleByObject = new Map<string, Set<string>>();
   for (const [parentObject, idSet] of byObject) {
     const ids = [...idSet];
@@ -515,7 +581,7 @@ async function computeThreadVisibilityFilter(
         where: { id: { $in: ids } },
         fields: ['id'],
         limit: ids.length,
-        context: { ...ctx.context },
+        context: { ...callerEnvelope },
       });
       visible = rows.map((r) => String(r.id)).filter(Boolean);
     } catch {