fix(service-storage): forward the caller's execution envelope to the sys_attachment sharing gates (#7145)

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

@@ -0,0 +1,61 @@
+---
+"@objectstack/service-storage": patch
+---
+
+fix(service-storage): forward the caller's full execution envelope to the `sys_attachment` sharing gates (#7145)
+
+`callerContext()` in `attachment-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). This is the same defect PR #7143 fixed for the `sys_comment`
+kit (#7141), one package over — the attachment kit is what the comment kit was
+derived from.
+
+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_attachment` here — 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 these gates then trust:
+  - `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 the
+    attachment 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.
+
+Both `canEdit` call sites are covered — the `beforeInsert` parent gate and the
+`beforeDelete` per-row authorization loop — and the same
+envelope-minus-private-keys rule is applied to the read middleware's
+parent-visibility 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 these gates and is deliberately left to the separate decision
+tracked as #7144.
+
+Enforcement effect: a delegated (`onBehalfOf`-carrying) principal is now refused
+where the contract says it is refused. No caller gains access.

packages/services/service-storage/src/attachment-access-hooks.test.ts

@@ -268,3 +268,295 @@ describe('attachment access — beforeDelete (uploader or parent editor)', () =>
     });
   });
 });
+
+// ─────────────────────────────────────────────────────────────────────────
+// #7145 — what the gate FORWARDS to the sharing service
+//
+// The mirror of #7141 / PR #7143 for the attachment kit: `callerContext()`
+// rebuilt a five-field projection of the caller's execution envelope 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).
+// ─────────────────────────────────────────────────────────────────────────
+
+/**
+ * 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_attachment`) 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_attachment` — NOT for the parent.
+  __readScope: 'org',
+  __writeScope: 'org',
+  __delegatorReadScope: 'org',
+  __delegatorWriteScope: 'org',
+  __expandRead: true,
+} as const;
+
+/** The principal half of {@link DELEGATED_ENVELOPE} — everything that must
+ * survive the forward, and nothing that must not. */
+const DELEGATED_PRINCIPAL_FIELDS = {
+  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,
+};
+
+const OPERATION_PRIVATE_KEYS = [
+  '__readScope',
+  '__writeScope',
+  '__delegatorReadScope',
+  '__delegatorWriteScope',
+  '__expandRead',
+];
+
+/** An insert ctx carrying an explicit execution envelope. */
+const envelopeInsertCtx = (data: any, exec: Record<string, unknown>) => ({
+  object: 'sys_attachment',
+  event: 'beforeInsert',
+  input: { data, options: { context: exec } },
+  session: { userId: exec.userId as string },
+  api: apiFor([]),
+});
+
+/** A delete ctx carrying an explicit execution envelope. */
+const envelopeDeleteCtx = (input: any, exec: Record<string, unknown>) => ({
+  object: 'sys_attachment',
+  event: 'beforeDelete',
+  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: service-storage does not
+ * depend on plugin-security (it consults a duck-typed `AttachmentSharingLike`),
+ * 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('#7145 — caller envelope forwarded to the sharing gate', () => {
+  const attRow = {
+    id: 'a1',
+    file_id: 'f1',
+    parent_object: 'att_secret',
+    parent_id: 'r1',
+    uploaded_by: 'someone_else',
+  };
+
+  // ── The forward itself, on BOTH call sites ────────────────────────────
+  it('beforeInsert forwards the whole envelope MINUS the operation-private keys', async () => {
+    const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true);
+    const { beforeInsert } = install({ sharing: { canEdit } });
+    await beforeInsert(
+      envelopeInsertCtx({ parent_object: 'att_case', parent_id: 'r1', file_id: 'f1' }, {
+        ...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. `uploaded_by`
+    // stamping does not touch the context.
+    expect(forwarded).toEqual(DELEGATED_PRINCIPAL_FIELDS);
+    // …and every middleware-private key resolved for `sys_attachment` is gone.
+    for (const key of OPERATION_PRIVATE_KEYS) expect(forwarded).not.toHaveProperty(key);
+  });
+
+  it('beforeDelete forwards the whole envelope MINUS the operation-private keys', async () => {
+    const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true);
+    const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } });
+    await beforeDelete(envelopeDeleteCtx({ id: 'a1' }, { ...DELEGATED_ENVELOPE }));
+
+    const forwarded = canEdit.mock.calls[0]![2] as unknown as Record<string, unknown>;
+    expect(forwarded).toEqual(DELEGATED_PRINCIPAL_FIELDS);
+    for (const key of OPERATION_PRIVATE_KEYS) 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({ attachments: [attRow], sharing: { canEdit } });
+    await beforeDelete(envelopeDeleteCtx({ id: 'a1' }, exec));
+
+    expect(canEdit.mock.calls[0]![2]).not.toBe(exec);
+    expect(exec.__writeScope).toBe('org'); // untouched: still sys_attachment's own
+  });
+
+  // ── What the restored fields BUY: the two verdict-deciding ones ───────
+  it('REFUSES a delegated principal whose sets carry modifyAllRecords (fail-closed, #7145)', 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({ attachments: [attRow], sharing: { canEdit } });
+    await expect(
+      beforeDelete(
+        envelopeDeleteCtx({ id: 'a1' }, {
+          ...DELEGATED_ENVELOPE,
+          permissions: ['admin_full_access'],
+        }),
+      ),
+    ).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED', status: 403 });
+    expect(canEdit).toHaveBeenCalledTimes(1);
+  });
+
+  it('REFUSES the same delegated principal on the beforeInsert parent gate too', async () => {
+    // Both `canEdit` call sites read the same `callerContext()`, so the guard
+    // has to arrive on the attach path as well as the detach one.
+    const canEdit = sharingCanEditDouble({
+      ownerId: 'other_owner',
+      setsWithBypass: ['admin_full_access'],
+    });
+    const { beforeInsert } = install({ sharing: { canEdit } });
+    await expect(
+      beforeInsert(
+        envelopeInsertCtx({ parent_object: 'att_secret', parent_id: 'r1', file_id: 'f1' }, {
+          ...DELEGATED_ENVELOPE,
+          permissions: ['admin_full_access'],
+        }),
+      ),
+    ).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', 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({ attachments: [attRow], sharing: { canEdit } });
+    await expect(
+      beforeDelete(
+        envelopeDeleteCtx({ id: 'a1' }, {
+          ...DELEGATED_ENVELOPE,
+          onBehalfOf: undefined, // isolate the ceiling rule from the delegation guard
+        }),
+      ),
+    ).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED', status: 403 });
+  });
+
+  // ── The half of the old projection that was CORRECT ───────────────────
+  it("does NOT carry sys_attachment's access DEPTH into the parent's owner-match", async () => {
+    // The context carries `__writeScope: 'org'` resolved for `sys_attachment`,
+    // and the gate asks about `att_secret`. Forwarding it whole would widen one
+    // object's question with another object's answer — a plain member would
+    // detach any file on any record in the org.
+    const canEdit = sharingCanEditDouble({ ownerId: 'other_owner' });
+    const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } });
+    await expect(
+      beforeDelete(
+        envelopeDeleteCtx({ id: 'a1' }, {
+          ...DELEGATED_ENVELOPE,
+          principalKind: 'human',
+          onBehalfOf: undefined,
+          userId: 'plain_member',
+          permissions: [],
+        }),
+      ),
+    ).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED', status: 403 });
+    expect((canEdit.mock.calls[0]![2] as any).__writeScope).toBeUndefined();
+  });
+
+  it('does not carry the depth into the beforeInsert parent gate either', async () => {
+    const canEdit = sharingCanEditDouble({ ownerId: 'other_owner' });
+    const { beforeInsert } = install({ sharing: { canEdit } });
+    await expect(
+      beforeInsert(
+        envelopeInsertCtx({ parent_object: 'att_secret', parent_id: 'r1', file_id: 'f1' }, {
+          ...DELEGATED_ENVELOPE,
+          principalKind: 'human',
+          onBehalfOf: undefined,
+          userId: 'plain_member',
+          permissions: [],
+        }),
+      ),
+    ).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403 });
+    expect((canEdit.mock.calls[0]![2] as any).__writeScope).toBeUndefined();
+  });
+
+  // ── The session fallback is unchanged ─────────────────────────────────
+  it('still falls back to the session snapshot when no execution context rides along', async () => {
+    const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true);
+    const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } });
+    await beforeDelete({
+      object: 'sys_attachment',
+      event: 'beforeDelete',
+      input: { id: 'a1' },
+      session: { userId: 'u1', tenantId: 'org_1', positions: ['p1'] },
+      api: apiFor([]),
+    });
+    expect(canEdit.mock.calls[0]![2]).toEqual({ userId: 'u1', tenantId: 'org_1', positions: ['p1'] });
+  });
+});