Merge pull request #2984 from objectstack-ai/claude/attachments-v1-followups-8jhvin

feat(attachments): v2 follow-ups — read visibility, authed downloads, edit-on-parent, lifecycle, enforce-or-remove (#2970)

Author: os-zhuang

.changeset/attachment-authenticated-downloads.md

@@ -0,0 +1,19 @@
+---
+'@objectstack/service-storage': minor
+---
+
+feat(attachments): authenticated, parent-scoped downloads for attachments files (#2970)
+
+Closes item 2 of #2970. The storage download endpoints (`GET /storage/files/:fileId`
+and `/files/:fileId/url`) were anonymous capability URLs — anyone holding a
+`fileId` could mint a download without a session or any access check.
+
+For `scope === 'attachments'`, non-`public_read` files, both endpoints now gate
+on a new `authorizeFileRead` seam: `401 AUTH_REQUIRED` without a session, `403
+ATTACHMENT_DOWNLOAD_DENIED` when the caller is neither the file's owner nor able
+to READ a record the file is attached to (parent-derived, resolved through the
+full caller context via `resolveAuthzContext`), and otherwise a **short-lived**
+signed URL (`downloadTtl`, default 300s). Non-attachments files (field files,
+avatars, org logos — embedded in `<img src>` which cannot carry a bearer token)
+keep the stable anonymous capability URL, and bare kernels/tests without the
+seam wired stay open (back-compat).

.changeset/attachment-followups-rest.md

@@ -0,0 +1,25 @@
+---
+'@objectstack/service-storage': minor
+'@objectstack/platform-objects': patch
+---
+
+feat(attachments): edit-on-parent attach, upload-session lifecycle, trash=false (#2970 items 3-5)
+
+Closes the remaining enforce-or-remove / lifecycle items of #2970:
+
+- **Edit-on-parent for attach (item 3, Salesforce parity).** Creating a
+  `sys_attachment` now requires EDIT access to the parent record (via the
+  sharing service's `canEdit`), not merely read — public-model parents are
+  unchanged (canEdit is true for any member), private/owner-scoped parents
+  require the caller to own/edit them. Degrades to read visibility when no
+  sharing service is present.
+- **`sys_upload_session` lifecycle (item 4).** Abandoned / terminal chunked
+  upload sessions are reaped by the platform LifecycleService (`transient`;
+  TTL 1d past `expires_at`; retention 7d for terminal statuses). Row reap
+  only — a reap guard that aborts backend multipart uploads for partial S3
+  sessions is a filed follow-up.
+- **`sys_attachment.enable.trash` → `false` (item 5, ADR-0049).** The flag is
+  `dead` in the liveness ledger (no engine soft-delete reader) and attachment
+  deletes are hard (the reap guard reclaims a file's bytes once its last join
+  row is gone, so a restore would dangle) — declare the honest state rather
+  than claim a restore capability the runtime does not provide.

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

examples/app-showcase/src/data/objects/project.object.ts

@@ -19,6 +19,12 @@ export const Project = ObjectSchema.create({
   icon: 'folder-kanban',
   description: 'A delivery project for an account.',
 
+  // [#2727 / #2970] Opt in to the generic Attachments panel on the record
+  // detail page — projects carry briefs, SOWs, and deliverable files. This is
+  // the showcase's files-enabled object for browser-dogfooding the attachments
+  // surface (upload / list / download / delete + parent-derived access).
+  enable: { files: true },
+
   fields: {
     name: Field.text({ label: 'Project Name', required: true, searchable: true, maxLength: 200 }),
     // `relatedList*` is the read-side mirror of inline editing: the Account's

packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts

@@ -199,6 +199,31 @@ describe('attachments permission matrix (#2755)', () => {
     expect(allowed.status).toBeLessThan(300);
   });
 
+  // ── (item 3) edit-on-parent: read is not enough to attach ────────────
+  it('(item 3) a member who can READ but not EDIT the parent cannot attach — yet can still list its attachments', async () => {
+    // att_readonly is public_read: every member reads it, only the owner edits.
+    const ro = await ql.insert('att_readonly', { name: 'ro', owner_id: adminId }, { context: { ...SYS } });
+
+    // memberA can READ the record…
+    const canRead = await stack.apiAs(memberATok, 'GET', `/data/att_readonly/${ro.id}`);
+    expect(canRead.status).toBe(200);
+
+    // …but attaching requires EDIT (Salesforce parity) → 403.
+    const file = await uploadFile(stack, memberATok);
+    const denied = await attach(memberATok, 'att_readonly', ro.id, file);
+    expect(denied.status).toBe(403);
+    expect(((await denied.json()) as any).code).toBe('ATTACHMENT_PARENT_ACCESS');
+
+    // The owner (admin) can attach, and memberA — who can read the parent —
+    // then sees that attachment in the list (read-visibility, item 1).
+    const adminFile = await uploadFile(stack, adminTok);
+    const attached = await attach(adminTok, 'att_readonly', ro.id, adminFile);
+    expect(attached.status).toBeLessThan(300);
+    const list = await stack.apiAs(memberATok, 'GET', '/data/sys_attachment');
+    const rows = ((await list.json()) as any).records ?? [];
+    expect(rows.some((r: any) => r.file_id === adminFile)).toBe(true);
+  });
+
   // ── (a) delete gate ──────────────────────────────────────────────────
   it('(a, DOGFOOD FINDING pin) the everyone baseline carries NO delete bit: an ungranted member cannot delete even their OWN attachment (403 PERMISSION_DENIED)', async () => {
     // ADR-0090 D5 / #2753: `member_default` is the anchor-bound baseline and
@@ -246,34 +271,79 @@ 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 ──────────────────────────
-  it('(e-read, KNOWN GAP pin) anonymous download of a committed file id succeeds (capability URL)', async () => {
-    const fileId = await uploadFile(stack, memberATok);
-    const res = await stack.api(`/storage/files/${fileId}/url`);
-    // Anyone holding a fileId can mint a download URL without a session.
-    // Authenticated downloads / signed-link gating is a filed follow-up.
-    expect(res.status).toBe(200);
+  // ── (e-read) attachment downloads inherit parent visibility (#2970 item 2) ──
+  it('(e-read) attachments download requires auth AND read access to a parent record', async () => {
+    // A file attached to the admin-owned, private att_secret record.
+    const adminFile = await uploadFile(stack, adminTok);
+    const linked = await attach(adminTok, 'att_secret', secretId, adminFile);
+    expect(linked.status).toBeLessThan(300);
+
+    // Anonymous → 401 (was a 200 capability URL before #2970).
+    const anon = await stack.api(`/storage/files/${adminFile}/url`);
+    expect(anon.status).toBe(401);
+    expect(((await anon.json()) as any).code).toBe('AUTH_REQUIRED');
+
+    // memberB is authenticated but cannot read att_secret and is not the
+    // owner → 403.
+    const denied = await stack.apiAs(memberBTok, 'GET', `/storage/files/${adminFile}/url`);
+    expect(denied.status).toBe(403);
+    expect(((await denied.json()) as any).code).toBe('ATTACHMENT_DOWNLOAD_DENIED');
+
+    // The owner (admin) → 200 with a signed URL.
+    const owner = await stack.apiAs(adminTok, 'GET', `/storage/files/${adminFile}/url`);
+    expect(owner.status).toBe(200);
+    expect(((await owner.json()) as any).url).toBeTruthy();
+
+    // Parent-inherited read: a file on the PUBLIC att_case record is
+    // downloadable by any member who can read that record — even a
+    // non-uploader (memberB downloads memberA's attachment).
+    const caseFile = await uploadFile(stack, memberATok);
+    const caseLink = await attach(memberATok, 'att_case', caseAId, caseFile);
+    expect(caseLink.status).toBeLessThan(300);
+    const inherited = await stack.apiAs(memberBTok, 'GET', `/storage/files/${caseFile}/url`);
+    expect(inherited.status).toBe(200);
+
+    // The stable 302 endpoint is gated the same way (anon → 401).
+    const anon302 = await stack.api(`/storage/files/${adminFile}`);
+    expect(anon302.status).toBe(401);
   });
 
   // ── Part 1: sys_file orphan lifecycle, end to end ─────────────────────
@@ -405,6 +475,31 @@ describe('attachments permission matrix (#2755)', () => {
       expect(report.errors, JSON.stringify(report.errors)).toEqual([]);
       expect(await ql.findOne('sys_file', { where: { id: data.fileId }, context: SYS })).toBeNull();
     });
+
+    it('(item 4) abandoned sys_upload_session rows are reaped past their expiry window', async () => {
+      // Initiate a chunked upload (creates a sys_upload_session) but never
+      // complete it.
+      const init = await stack.api('/storage/upload/chunked', {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${memberATok}` },
+        body: JSON.stringify({ filename: 'big.bin', mimeType: 'application/octet-stream', totalSize: 10_485_760 }),
+      });
+      expect(init.status).toBe(200);
+      const { uploadId } = ((await init.json()) as any).data;
+      const session = await ql.findOne('sys_upload_session', { where: { id: uploadId }, context: SYS });
+      expect(session?.id, 'session row created').toBeTruthy();
+
+      // Backdate expires_at past the 1d TTL grace.
+      await ql.update(
+        'sys_upload_session',
+        { id: uploadId, expires_at: new Date(Date.now() - 2 * DAY_MS) },
+        { context: { ...SYS } },
+      );
+
+      const report = await lifecycle.sweep();
+      expect(report.errors, JSON.stringify(report.errors)).toEqual([]);
+      expect(await ql.findOne('sys_upload_session', { where: { id: uploadId }, context: SYS })).toBeNull();
+    });
   });
 });
 

packages/dogfood/test/fixtures/attachments-fixture.ts

@@ -60,6 +60,20 @@ export const AttNoFiles = ObjectSchema.create({
   },
 });
 
+export const AttReadonly = ObjectSchema.create({
+  name: 'att_readonly',
+  label: 'Attachment Readonly',
+  pluralLabel: 'Attachment Readonlys',
+  // public_read: every member can READ, only the owner can EDIT — the case
+  // that distinguishes edit-on-parent (#2970 item 3) from read visibility.
+  sharingModel: 'public_read',
+  enable: { files: true },
+  fields: {
+    name: Field.text({ label: 'Name', required: true }),
+    owner_id: Field.text({ label: 'Owner' }),
+  },
+});
+
 /**
  * The domain grant a real app ships when it turns the attachments panel on
  * for members: `member_default` (the `everyone` anchor baseline) carries NO
@@ -93,5 +107,5 @@ export const attachmentsFixtureStack = defineStack({
     description:
       'Three-object app exercising the #2755 attachment permission matrix: parent visibility, uploader/editor delete, enable.files gate.',
   },
-  objects: [AttCase, AttSecret, AttNoFiles],
+  objects: [AttCase, AttSecret, AttNoFiles, AttReadonly],
 });

packages/platform-objects/src/audit/sys-attachment.object.ts

@@ -132,7 +132,12 @@ export const SysAttachment = ObjectSchema.create({
     trackHistory: false,
     searchable: true,
     apiEnabled: true,
-    trash: true,
+    // [#2970 item 5 / ADR-0049] `trash` is `dead` in the liveness ledger (no
+    // engine soft-delete reader) and attachment deletes ARE hard (#2755): the
+    // reap guard reclaims a file's bytes once its last join row is gone, so a
+    // "restore" would dangle. Declare `false` — the honest state — rather than
+    // claim a restore capability the runtime does not provide.
+    trash: false,
     mru: false,
     clone: false,
   },

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

@@ -99,6 +99,31 @@ describe('attachment access — beforeInsert (parent visibility + provenance)',
     ).resolves.toBeUndefined();
     await expect(beforeInsert(insertCtx({ parent_object: 'x', parent_id: 'r' }, {}))).resolves.toBeUndefined();
   });
+
+  // #2970 item 3 — with a sharing service present, attach requires EDIT
+  // (canEdit), not merely read visibility.
+  it('with sharing: rejects attaching when the caller cannot EDIT the parent (even if readable)', async () => {
+    const canEdit = vi.fn(async () => false);
+    const { beforeInsert } = install({ sharing: { canEdit } });
+    const ctx = insertCtx(
+      { parent_object: 'att_readonly', parent_id: 'r1', file_id: 'f1' },
+      { userId: 'u1', visible: ['att_readonly/r1'] }, // readable, but canEdit=false
+    );
+    await expect(beforeInsert(ctx)).rejects.toMatchObject({
+      code: 'ATTACHMENT_PARENT_ACCESS',
+      status: 403,
+    });
+    expect(canEdit).toHaveBeenCalledWith('att_readonly', 'r1', expect.objectContaining({ userId: 'u1' }));
+  });
+
+  it('with sharing: allows attaching when the caller CAN edit the parent', async () => {
+    const { beforeInsert } = install({ sharing: { canEdit: async () => true } });
+    const ctx = insertCtx(
+      { parent_object: 'att_case', parent_id: 'r1', file_id: 'f1' },
+      { userId: 'u1', visible: [] }, // not readable via api, but canEdit=true governs
+    );
+    await expect(beforeInsert(ctx)).resolves.toBeUndefined();
+  });
 });
 
 describe('attachment access — beforeDelete (uploader or parent editor)', () => {

packages/services/service-storage/src/attachment-access-hooks.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;