feat(attachments): sys_attachment read inherits parent-record visibility (#2970)

claude · claude · commit d49deb57117a · 2026-07-15T15:29:42.000Z
Follow-up to #2755. The create/delete gates landed, but a member could still LIST sys_attachment rows (file_name, size, parent_id) pointing at records they cannot read — an info leak, since attachment access derives from the parent record. sys_attachment is public with no owner field, so the sharing/RLS static predicates never narrowed it. installAttachmentReadVisibility registers a sys_attachment-scoped engine MIDDLEWARE (not a find-hook): middleware runs for find/findOne/count/ aggregate, so the list total (from engine.count(), never the find path) is filtered identically and cannot leak the hidden-row count — a before/afterFind hook would leave count() unfiltered. Generalizing ADR-0055 controlled_by_parent to the polymorphic parent, each read resolves the visible parent ids per parent_object through the caller-scoped engine (the parent's own RLS/OWD/sharing apply) and ANDs a $or of { parent_object, parent_id: { $in } } into the query; no visible parent → deny-all sentinel; fails closed on compute error; the candidate pre-scan cap logs (not silent) when it truncates. System/context-less internal reads are not narrowed. Dogfood: the (c) matrix pin flips from leak-asserted to a denial — a member cannot list/read/by-id-fetch attachments of an invisible parent, total does not leak the count, and the control proves they still see attachments on records they CAN read. 19 unit tests; full dogfood 256 ✓. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0187NT3Qer9oep5dCRb9b8Lt
diff --git a/.changeset/attachment-read-visibility.md b/.changeset/attachment-read-visibility.md
@@ -0,0 +1,23 @@
+---
+'@objectstack/service-storage': minor
+---
+
+feat(attachments): sys_attachment read inherits parent-record visibility (#2970)
+
+Follow-up to #2755. The create/delete gates landed, but a member could still
+LIST `sys_attachment` rows (file_name, size, parent_id) pointing at records
+they cannot read — an information leak, since attachment access derives from
+the PARENT record (Salesforce ContentDocumentLink semantics). `sys_attachment`
+is a public system object with no owner field, so the sharing/RLS static
+predicates never narrowed it.
+
+`installAttachmentReadVisibility` registers a `sys_attachment`-scoped engine
+**middleware** (not a find-hook) so it filters `find`, `findOne`, `count`, and
+`aggregate` identically — critically, the list `total` (which comes from
+`engine.count()`, never the find path) is filtered too, so it cannot leak the
+count of hidden rows. Generalizing ADR-0055 `controlled_by_parent` to the
+polymorphic parent, each read resolves the visible parent ids per
+`parent_object` through the caller-scoped engine (the parent's own RLS/OWD/
+sharing apply) and ANDs a `$or` of `{ parent_object, parent_id: { $in } }`
+into the query; no visible parent ⇒ a deny-all sentinel. Fails closed on any
+compute error. System / context-less internal reads are not narrowed.
diff --git a/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts b/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts
@@ -246,25 +246,42 @@ describe('attachments permission matrix (#2755)', () => {
     expect(ownDelete.status).toBeLessThan(300);
   });
 
-  // ── (c) known gap: listing does not inherit parent visibility ────────
-  it('(c, KNOWN GAP pin) a member can LIST sys_attachment rows pointing at records they cannot read', async () => {
+  // ── (c) attachment LIST inherits parent visibility (#2970 item 1) ────
+  it('(c) a member CANNOT list/read sys_attachment rows whose parent record they cannot read', async () => {
     const adminFile = await uploadFile(stack, adminTok);
     const created = await attach(adminTok, 'att_secret', secretId, adminFile);
     expect(created.status).toBeLessThan(300);
+    const secretRow = await ql.findOne('sys_attachment', { where: { file_id: adminFile }, context: SYS });
 
     // memberB cannot read the att_secret PARENT…
     const parentRead = await stack.apiAs(memberBTok, 'GET', `/data/att_secret/${secretId}`);
     expect([403, 404]).toContain(parentRead.status);
 
-    // …but CAN see the join row (file name, size, parent id) through the
-    // generic list path. Attachment read visibility does not yet inherit
-    // parent-record visibility — enforce-or-remove follow-up filed with
-    // #2755; flip this pin to a denial assertion when inheritance lands.
+    // …and now the join row is filtered out of the generic list too (the
+    // read-visibility middleware inherits the parent's visibility, and the
+    // list `total` is filtered identically via count()).
     const list = await stack.apiAs(memberBTok, 'GET', '/data/sys_attachment');
     expect(list.status).toBe(200);
-    const rows = ((await list.json()) as any).records ?? [];
-    const leaked = rows.some((r: any) => r.parent_object === 'att_secret' && r.parent_id === secretId);
-    expect(leaked, 'KNOWN GAP: join rows of invisible parents are listable').toBe(true);
+    const body = (await list.json()) as any;
+    const rows = body.records ?? [];
+    expect(
+      rows.some((r: any) => r.id === secretRow.id),
+      'attachment of an invisible parent must not be listable',
+    ).toBe(false);
+    // total must not leak the hidden row's existence either.
+    expect(rows.every((r: any) => r.parent_object !== 'att_secret' || r.parent_id !== secretId)).toBe(true);
+
+    // A by-id read of the hidden attachment is a 404/403, not a leak.
+    const byId = await stack.apiAs(memberBTok, 'GET', `/data/sys_attachment/${secretRow.id}`);
+    expect([403, 404]).toContain(byId.status);
+
+    // Control: memberB CAN still see attachments on a record they can read.
+    const okFile = await uploadFile(stack, memberBTok);
+    const okAttach = await attach(memberBTok, 'att_case', caseAId, okFile);
+    expect(okAttach.status).toBeLessThan(300);
+    const okList = await stack.apiAs(memberBTok, 'GET', '/data/sys_attachment');
+    const okRows = ((await okList.json()) as any).records ?? [];
+    expect(okRows.some((r: any) => r.file_id === okFile)).toBe(true);
   });
 
   // ── (e-read) known gap: anonymous downloads ──────────────────────────
diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts
diff --git a/packages/services/service-storage/src/attachment-lifecycle.ts b/packages/services/service-storage/src/attachment-lifecycle.ts
@@ -37,11 +37,27 @@ export interface AttachmentLifecycleEngine {
     handler: (ctx: any) => void | Promise<void>,
     options?: { object?: string; packageId?: string },
   ): void;
+  /** Onion-model data middleware (runs for find/findOne/count/aggregate AND
+   * writes) — the only seam that filters `count()` (→ list `total`)
+   * identically to `find()`. Used for polymorphic parent-visibility on reads.
+   * Optional: only the read-visibility installer needs it. */
+  registerMiddleware?(
+    fn: (ctx: AttachmentReadMiddlewareCtx, next: () => Promise<void>) => Promise<void>,
+    options?: { object?: string },
+  ): void;
   find(object: string, options: Record<string, unknown>): Promise<Array<Record<string, unknown>>>;
   findOne(object: string, options: Record<string, unknown>): Promise<Record<string, unknown> | null>;
   update(object: string, data: Record<string, unknown>, options: Record<string, unknown>): Promise<unknown>;
 }
 
+/** Minimal shape of the engine `OperationContext` the read middleware reads. */
+export interface AttachmentReadMiddlewareCtx {
+  object: string;
+  operation: 'find' | 'findOne' | 'insert' | 'update' | 'delete' | 'count' | 'aggregate';
+  ast?: { object?: string; where?: unknown } & Record<string, unknown>;
+  context?: { userId?: string; tenantId?: string; positions?: string[]; permissions?: string[]; isSystem?: boolean } & Record<string, unknown>;
+}
+
 export interface AttachmentLifecycleLogger {
   info(msg: string, meta?: unknown): void;
   warn(msg: string, meta?: unknown): void;
diff --git a/packages/services/service-storage/src/attachment-read-visibility.test.ts b/packages/services/service-storage/src/attachment-read-visibility.test.ts
@@ -0,0 +1,180 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+import { describe, it, expect, vi } from 'vitest';
+import { installAttachmentReadVisibility } from './attachment-access-hooks.js';
+import type { AttachmentLifecycleEngine, AttachmentReadMiddlewareCtx } from './attachment-lifecycle.js';
+
+const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() });
+
+/**
+ * Fake engine modelling: a sys_attachment table (system pre-scan) + a
+ * per-parent-object visibility map keyed by userId. A parent find under a
+ * caller context returns only the ids that user can see.
+ */
+function install(opts: {
+  attachments: Array<{ parent_object: string; parent_id: string; id?: string }>;
+  /** parentObject -> userId -> visible parent ids */
+  visible: Record<string, Record<string, string[]>>;
+}) {
+  let mw!: (ctx: AttachmentReadMiddlewareCtx, next: () => Promise<void>) => Promise<void>;
+  const calls = { parentFinds: [] as Array<{ object: string; ids: string[]; userId?: string }> };
+
+  const matchWhere = (row: any, where: any): boolean => {
+    if (!where || typeof where !== 'object') return true;
+    return Object.entries(where).every(([k, v]) => {
+      if (v && typeof v === 'object' && Array.isArray((v as any).$in)) {
+        return (v as any).$in.map(String).includes(String(row[k]));
+      }
+      return String(row[k]) === String(v);
+    });
+  };
+
+  const engine: AttachmentLifecycleEngine = {
+    registerHook: () => {},
+    registerMiddleware: (fn) => {
+      mw = fn as any;
+    },
+    find: async (object: string, options: any) => {
+      if (object === 'sys_attachment') {
+        // system candidate pre-scan
+        const rows = opts.attachments.filter((r) => matchWhere(r, options?.where));
+        return rows.slice(0, options?.limit ?? rows.length) as any;
+      }
+      // caller-scoped parent visibility probe
+      const userId = options?.context?.userId as string | undefined;
+      const ids: string[] = (options?.where?.id?.$in ?? []).map(String);
+      calls.parentFinds.push({ object, ids, userId });
+      const vis = opts.visible[object]?.[userId ?? ''] ?? [];
+      return ids.filter((id) => vis.includes(id)).map((id) => ({ id })) as any;
+    },
+    findOne: async () => null,
+    update: async () => ({}),
+  };
+  installAttachmentReadVisibility(engine, silentLogger());
+  return { mw, calls };
+}
+
+/** Drive a read op through the middleware and return the resulting where. */
+async function runRead(
+  mw: (ctx: AttachmentReadMiddlewareCtx, next: () => Promise<void>) => Promise<void>,
+  ctxPartial: Partial<AttachmentReadMiddlewareCtx>,
+) {
+  const ctx: AttachmentReadMiddlewareCtx = {
+    object: 'sys_attachment',
+    operation: 'find',
+    ast: { object: 'sys_attachment', where: undefined },
+    context: { userId: 'u1' },
+    ...ctxPartial,
+  };
+  let ran = false;
+  await mw(ctx, async () => {
+    ran = true;
+  });
+  return { where: ctx.ast?.where, ran };
+}
+
+describe('installAttachmentReadVisibility', () => {
+  const dataset = {
+    attachments: [
+      { id: 'a1', parent_object: 'att_case', parent_id: 'c1' },
+      { id: 'a2', parent_object: 'att_case', parent_id: 'c2' }, // invisible to u1
+      { id: 'a3', parent_object: 'att_secret', parent_id: 's1' }, // invisible to u1
+    ],
+    visible: {
+      att_case: { u1: ['c1'] }, // u1 sees only c1
+      att_secret: { u1: [] }, // u1 sees no secrets
+    },
+  };
+
+  it('constrains the query to only visible parents (single parent_object → bare clause)', async () => {
+    const { mw } = install(dataset);
+    const { where, ran } = await runRead(mw, {});
+    expect(ran).toBe(true);
+    // u1 sees att_case/c1 only; att_secret none → dropped.
+    expect(where).toEqual({ parent_object: 'att_case', parent_id: { $in: ['c1'] } });
+  });
+
+  it('emits a $or across multiple visible parent objects', async () => {
+    const { mw } = install({
+      attachments: [
+        { id: 'a1', parent_object: 'att_case', parent_id: 'c1' },
+        { id: 'a2', parent_object: 'att_todo', parent_id: 't1' },
+      ],
+      visible: { att_case: { u1: ['c1'] }, att_todo: { u1: ['t1'] } },
+    });
+    const { where } = await runRead(mw, {});
+    expect(where).toEqual({
+      $or: [
+        { parent_object: 'att_case', parent_id: { $in: ['c1'] } },
+        { parent_object: 'att_todo', parent_id: { $in: ['t1'] } },
+      ],
+    });
+  });
+
+  it('denies all when no candidate parent is visible', async () => {
+    const { mw } = install({
+      attachments: [{ id: 'a3', parent_object: 'att_secret', parent_id: 's1' }],
+      visible: { att_secret: { u1: [] } },
+    });
+    const { where } = await runRead(mw, {});
+    expect(where).toEqual({ id: '__attachment_parent_denied__' });
+  });
+
+  it('ANDs the visibility filter onto an existing where (does not clobber it)', async () => {
+    // The pre-scan honors the caller's where, so the candidate rows must
+    // carry the filtered field for this fake to surface them.
+    const { mw } = install({
+      attachments: [
+        { id: 'a1', parent_object: 'att_case', parent_id: 'c1', mime_type: 'application/pdf' } as any,
+        { id: 'a2', parent_object: 'att_case', parent_id: 'c2', mime_type: 'text/plain' } as any,
+      ],
+      visible: { att_case: { u1: ['c1', 'c2'] } },
+    });
+    const existing = { mime_type: 'application/pdf' };
+    const { where } = await runRead(mw, { ast: { object: 'sys_attachment', where: existing } });
+    // Only a1 matched the pre-scan (mime_type), and its parent c1 is visible.
+    expect(where).toEqual({
+      $and: [existing, { parent_object: 'att_case', parent_id: { $in: ['c1'] } }],
+    });
+  });
+
+  it('filters count() identically (list total cannot leak the unfiltered count)', async () => {
+    const { mw } = install(dataset);
+    const { where } = await runRead(mw, { operation: 'count' });
+    expect(where).toEqual({ parent_object: 'att_case', parent_id: { $in: ['c1'] } });
+  });
+
+  it('bypasses system context and context-less reads (internal calls)', async () => {
+    const { mw, calls } = install(dataset);
+    const sys = await runRead(mw, { context: { isSystem: true } as any });
+    expect(sys.where).toBeUndefined();
+    const anon = await runRead(mw, { context: undefined });
+    expect(anon.where).toBeUndefined();
+    // no parent probes were issued for the bypassed reads
+    expect(calls.parentFinds).toHaveLength(0);
+  });
+
+  it('does not touch write operations', async () => {
+    const { mw } = install(dataset);
+    const { where } = await runRead(mw, { operation: 'delete', ast: { object: 'sys_attachment', where: { id: 'a1' } } });
+    expect(where).toEqual({ id: 'a1' }); // unchanged
+  });
+
+  it('leaves an already-empty result set alone (no candidates → no filter)', async () => {
+    const { mw } = install({ attachments: [], visible: {} });
+    const { where } = await runRead(mw, {});
+    expect(where).toBeUndefined();
+  });
+
+  it('ignores self-referential (parent_object=sys_attachment) rows — prevents probe re-entry', async () => {
+    const { mw, calls } = install({
+      attachments: [{ id: 'a1', parent_object: 'sys_attachment', parent_id: 'a0' }],
+      visible: {},
+    });
+    const { where } = await runRead(mw, {});
+    // only a self-ref candidate → nothing to grant → deny all, and NO parent
+    // probe was issued against sys_attachment.
+    expect(where).toEqual({ id: '__attachment_parent_denied__' });
+    expect(calls.parentFinds).toHaveLength(0);
+  });
+});
diff --git a/packages/services/service-storage/src/index.ts b/packages/services/service-storage/src/index.ts
@@ -14,5 +14,5 @@ export type { StorageRoutesOptions } from './storage-routes.js';
 export { SystemFile, SystemUploadSession } from './objects/index.js';
 export { installAttachmentLifecycleHooks, createSysFileReapGuard } from './attachment-lifecycle.js';
 export type { AttachmentLifecycleEngine, AttachmentLifecycleLogger } from './attachment-lifecycle.js';
-export { installAttachmentAccessHooks } from './attachment-access-hooks.js';
+export { installAttachmentAccessHooks, installAttachmentReadVisibility } from './attachment-access-hooks.js';
 export type { AttachmentSharingLike } from './attachment-access-hooks.js';
diff --git a/packages/services/service-storage/src/storage-service-plugin.test.ts b/packages/services/service-storage/src/storage-service-plugin.test.ts
@@ -197,11 +197,15 @@ describe('StorageServicePlugin: sys_file orphan lifecycle wiring (#2755)', () =>
     const ctx = makeCtx();
 
     const hookEvents: string[] = [];
+    const middlewares: Array<{ object?: string }> = [];
     ctx.registerService('objectql', {
       registerHook: (event: string, _fn: unknown, opts: any) => {
         expect(opts?.object).toBe('sys_attachment');
         hookEvents.push(event);
       },
+      registerMiddleware: (_fn: unknown, opts: any) => {
+        middlewares.push({ object: opts?.object });
+      },
       find: async () => [],
       findOne: async () => null,
       update: async () => ({}),
@@ -228,6 +232,8 @@ describe('StorageServicePlugin: sys_file orphan lifecycle wiring (#2755)', () =>
     expect(guards).toHaveLength(1);
     expect(guards[0].object).toBe('sys_file');
     expect(typeof guards[0].guard).toBe('function');
+    // Read-visibility middleware registered on sys_attachment (#2970 item 1).
+    expect(middlewares).toEqual([{ object: 'sys_attachment' }]);
   });
 
   it('degrades silently on a bare kernel (no engine, no lifecycle service)', async () => {
diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts
@@ -14,7 +14,7 @@ import type { S3StorageAdapterOptions } from './s3-storage-adapter.js';
 import { StorageMetadataStore } from './metadata-store.js';
 import { registerStorageRoutes } from './storage-routes.js';
 import { installAttachmentLifecycleHooks, createSysFileReapGuard } from './attachment-lifecycle.js';
-import { installAttachmentAccessHooks } from './attachment-access-hooks.js';
+import { installAttachmentAccessHooks, installAttachmentReadVisibility } from './attachment-access-hooks.js';
 import { SystemFile, SystemUploadSession } from './objects/index.js';
 // ADR-0052 §3 ownership: `sys_attachment` (a file↔record link) belongs with the
 // storage domain, not the audit/compliance ledger. Definition stays in
@@ -212,6 +212,12 @@ export class StorageServicePlugin implements Plugin {
           },
           ctx.logger,
         );
+        // Parent-derived READ visibility (#2970 item 1) — list/find/count of
+        // sys_attachment only returns rows whose parent record the caller can
+        // read. Middleware (not a hook) so list `total` is filtered too.
+        if (typeof (engine as any).registerMiddleware === 'function') {
+          installAttachmentReadVisibility(engine as any, ctx.logger);
+        }
         try {
           const lifecycle = ctx.getService<any>('lifecycle');
           if (lifecycle && typeof lifecycle.registerReapGuard === 'function') {
