From d49deb57117afb7eb03e010aaae40c4ef06ded89 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Jul 2026 15:29:42 +0000 Subject: [PATCH 1/3] feat(attachments): sys_attachment read inherits parent-record visibility (#2970) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_0187NT3Qer9oep5dCRb9b8Lt --- .changeset/attachment-read-visibility.md | 23 +++ ...achments-permission-matrix.dogfood.test.ts | 35 +++- .../src/attachment-access-hooks.ts | Bin 8202 -> 14282 bytes .../src/attachment-lifecycle.ts | 16 ++ .../src/attachment-read-visibility.test.ts | 180 ++++++++++++++++++ .../services/service-storage/src/index.ts | 2 +- .../src/storage-service-plugin.test.ts | 6 + .../src/storage-service-plugin.ts | 8 +- 8 files changed, 259 insertions(+), 11 deletions(-) create mode 100644 .changeset/attachment-read-visibility.md create mode 100644 packages/services/service-storage/src/attachment-read-visibility.test.ts diff --git a/.changeset/attachment-read-visibility.md b/.changeset/attachment-read-visibility.md new file mode 100644 index 0000000000..514769bfa3 --- /dev/null +++ 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 index a5fd57f822..7a4378e21f 100644 --- 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 index d0416cee0380b93f239e5db939897aefeaaab8c1..4aaea02cdcd644b50fa4fcaea323d21bbaacd1fd 100644 GIT binary patch delta 5705 zcmaJ_O^+K{6_v7J(I^NJ$S?@OotX$%HFmiZX*9w;N$8||M5Io4=+4YAYPIa=s%QI= za=oHnxgC?ViWLhMh{602ShC;`@H1x1kAQRUtMbPrLn}(V>*K!raqhY2`cJ?7xBbg+ zZtT!f-TcXNc=J~4*pA1#Xzvgml%QFAiFWkWAO1$=MAJx_v=gV+>4Y2%nM|h?OifYP!cm^i#wP2yi7HGs zZn+)WrNJvQNsn6nexkEQU!`fk-=gaQpOJJ%CM|VAd@GEo+U@SdHghGN9UUC@506g1 z=pP&((|uYQQL7mGIb6?5HfwYuMoBr6_xE=sl1FxyC1f*kJ=aBt&!kvoiAhwcDKB)# z;m?R3>4nXb3J)6R1NDw^yb{LaWFT2Cq(b3xfqN>W zRI3-r0zWv7Xgjj=#|J0<<0ns_JV$Kz`g?nO@Hn;ytI$uMo#RfX=X9>i;G#7$S<-5g z{65Vze~b}a$sg2sT<9_4X)Wi zJMYZ$HhpmK7a#4BDfN`@h7l^pMSG6mVkE27-9)DvshU{(!jT%-123Vl3|Hdn`~qy-)!oT<6Iv{Dvq^tfLLL!P`6QGr&7FU2WLko z;-Ei)f^(H>H?jrZU*5fOs{+EO3Vy^6*c^O43M074XT#LQG&kjhGD~)zAq^v=(*(X) zIjC-j?%DAJ5FU?Vqs9F^Rn8ZIt0Ij0k-RY`x(X9a| zL%~j#bht<%<2|1hIeN55>qn`WWpWGxqyaxZX#3y56Mqyu;lshnAq^N)m4FIY>Nrw^ z#vGKkROx_%`NYH%sfV35X&vYc{jVc26@+;82F;6nfD{%~ zTDHb*Rp@e7WEwycq)3rzsNuN&1a`V=1kT`3f=ZeLbP1Oq(DYH@Xtp)WRtKTw1t4O- zEM_F^#QBsuxqK*y@&pLa1Xds-El?A#8%rjinR5U;g zC`E45#ncu#62xA3Scg3{T|pWIEYJnqYw{77)|@m5>OQd5@V7cHH~T`O%9R1ta-M!S z)5W3=zqB)04ggZ_WcE(KEW&%TX zdtm!JChLXtaqaN~y5@-iPO!*)`7%ZIUm*LI#zjFpc_-w^`%g=6x%YWVM zR%S#&xyUt~tI-Lv1d?^J9)R@T{rlk7Sr(VZW-aizAaI|$-6q~t#o#~AqSI=MVe01; zf_svIsRMY`d)t7mRui^HeGjmFk%1>dy8nP)!4eVs_+A~?>TtQU6)eu#{`(5LVe!TuOGR}Jw)&=4d zJSHR~LCDGEpX>T}i8T0H@>P!wNqbeGYHF@biL29mR_bTceQTtCBGm~zINR(-n^Mti zt5ZBed>KP-s3#dm*OYnn+%*aUy#|$16t(LW`@>Z`sA15l-4H1^Lo%l`(7K;JBM>p6 z%Xy*-m9d6t5=^yM5hP>gks`KfaN$Z@sIk7>9J?}~uT0g*kOqcwuEum^%_k!sX9jm( zvCpjEj#x^D4qz7hDn`jqE)=KnTsvSxO(JvQ=#qhEx)|=vx=_SCLX;O zxyZD1wn3MkpE|-pUP*oatE00cChlf`pPd!QPLYMo1q)7R+_GuO|31ekXcYhEq>S zl!w(rKdpf8<(J?7;jcqR^(#Rip)Da69Voozi`uu6B3AbG1(UQcJ^E8KHD%9lN9!G; zy|et!JMUN50(w77n!LLx1+lQ~YVeN=F?sQ0E0>$6=)wK3+in;TB{%`=I*9hHgm~ZB z10km}@J_`f+w({s$~q;G2#S0o17IGg7 z){794q9}A@4LB?z*P%rH`5SQHm4tUy9m!UcE8bYe3)Yr~arYG?&y8ORXxn{~$Vb&rM&PY1%yb_*U z7yr5=N>=67J`L#S5(0B<0^10bCvVonA)I~ zWX9=`)Na%*FYFKy)7aNNthjS)L5tu;x%x!5=ye5&gNslzoEH|g+pa>|B171$XWA!( zjpdvzJj>LIJq-c#`}IX;^v%CpZ1F}clM;}za#u3sLu$v9x88ENp&SQM{bad8|@056*;yS|tKaIG(YU1!skLT=-dA9uTjUU`H z+HFh*8Pdz0+q=~Ut}pz{J|S(KjxM-lq&Kl`3eB8npaq%R-mUa*p4*Y_y6xI{uP zF8%#YS&G8naOJZ9&X20?qhARYnKSCC<-ct6OPG`v+t*rst>@KxF7eCtY=Wcp%vry} zm_z<}u|oiHwN~0|!+v_Pp*kSn!7iGrJP06Xu`ShbD3F6NjQo@c9$5>Sn$^f@BybZH ziuI}Ax4O)6++cYnq(HuG^wl5#3cY|a(3~s74bqCg void | Promise, 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) => Promise, + options?: { object?: string }, + ): void; find(object: string, options: Record): Promise>>; findOne(object: string, options: Record): Promise | null>; update(object: string, data: Record, options: Record): Promise; } +/** 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; + context?: { userId?: string; tenantId?: string; positions?: string[]; permissions?: string[]; isSystem?: boolean } & Record; +} + 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 new file mode 100644 index 0000000000..bb495fcc9c --- /dev/null +++ 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>; +}) { + let mw!: (ctx: AttachmentReadMiddlewareCtx, next: () => Promise) => Promise; + 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) => Promise, + ctxPartial: Partial, +) { + 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 index 981fa7a409..89d8603327 100644 --- 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 index a1e8327f00..3821d37b1f 100644 --- 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 index 60a3aa4299..0b19c51823 100644 --- 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('lifecycle'); if (lifecycle && typeof lifecycle.registerReapGuard === 'function') { From 0289d5ad33aa419bc1c87ec4346809e990df3550 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Jul 2026 15:54:54 +0000 Subject: [PATCH 2/3] feat(attachments): authenticated, parent-scoped downloads (#2970 item 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The storage download endpoints (GET /storage/files/:fileId and /files/:fileId/url) were anonymous capability URLs — anyone holding a fileId could mint a download with no session or 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 owner nor able to READ a record the file is attached to (parent-derived, resolved through the full caller context via resolveAuthzContext), else a short-lived signed URL (downloadTtl, default 300s). Non-attachments files (field files, avatars, org logos — embedded in which can't carry a bearer) keep the stable anonymous capability URL; bare kernels/tests without the seam stay open (back-compat). Tests: storage-routes gate unit tests (401/403/allow/public/non-attach/ open); the dogfood matrix (e-read) pin flips from a leak to a denial — anon 401, cross-user 403, owner 200, parent-inherited 200, 302 anon 401. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0187NT3Qer9oep5dCRb9b8Lt --- .../attachment-authenticated-downloads.md | 19 +++ ...achments-permission-matrix.dogfood.test.ts | 42 +++++- .../services/service-storage/src/index.ts | 2 +- .../src/storage-routes.test.ts | 83 +++++++++++ .../service-storage/src/storage-routes.ts | 73 +++++++++- .../src/storage-service-plugin.ts | 136 ++++++++++++++---- 6 files changed, 311 insertions(+), 44 deletions(-) create mode 100644 .changeset/attachment-authenticated-downloads.md diff --git a/.changeset/attachment-authenticated-downloads.md b/.changeset/attachment-authenticated-downloads.md new file mode 100644 index 0000000000..f184e45e0a --- /dev/null +++ b/.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 `` which cannot carry a bearer token) +keep the stable anonymous capability URL, and bare kernels/tests without the +seam wired stay open (back-compat). diff --git a/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts b/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts index 7a4378e21f..cc826ba289 100644 --- a/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts +++ b/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts @@ -284,13 +284,41 @@ describe('attachments permission matrix (#2755)', () => { 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 ───────────────────── diff --git a/packages/services/service-storage/src/index.ts b/packages/services/service-storage/src/index.ts index 89d8603327..0e5434f511 100644 --- a/packages/services/service-storage/src/index.ts +++ b/packages/services/service-storage/src/index.ts @@ -10,7 +10,7 @@ export type { S3StorageAdapterOptions } from './s3-storage-adapter.js'; export { StorageMetadataStore } from './metadata-store.js'; export type { FileRecord, UploadSessionRecord } from './metadata-store.js'; export { registerStorageRoutes } from './storage-routes.js'; -export type { StorageRoutesOptions } from './storage-routes.js'; +export type { StorageRoutesOptions, FileReadVerdict } 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'; diff --git a/packages/services/service-storage/src/storage-routes.test.ts b/packages/services/service-storage/src/storage-routes.test.ts index f0c54a8258..20fadbf000 100644 --- a/packages/services/service-storage/src/storage-routes.test.ts +++ b/packages/services/service-storage/src/storage-routes.test.ts @@ -250,6 +250,89 @@ describe('Storage REST Routes', () => { }); }); + describe('attachments download gate (#2970 item 2)', () => { + const commit = async (s: StorageMetadataStore, rec: Partial) => + s.createFile({ + id: rec.id ?? 'f-dl', + key: rec.key ?? `attachments/${rec.id ?? 'f-dl'}.bin`, + name: 'x.bin', + status: 'committed', + acl: rec.acl ?? 'private', + scope: rec.scope ?? 'attachments', + owner_id: rec.owner_id, + ...rec, + } as any); + + function serverWith(verdict: import('./storage-routes').FileReadVerdict | 'skip', extra: any = {}) { + const server = createMockHttpServer(); + const s = new StorageMetadataStore(null); + const authorizeFileRead = + verdict === 'skip' ? undefined : vi.fn(async () => verdict as any); + registerStorageRoutes(server as any, adapter, s, { + basePath: '/api/v1/storage', + authorizeFileRead, + ...extra, + }); + return { server, store: s, authorizeFileRead }; + } + + const hit = async (server: any, path: string, fileId: string) => { + const res = createMockRes(); + await server._getHandler('GET', path)!(createMockReq({ params: { fileId } }), res); + return res; + }; + + it('401s an unauthenticated download of an attachments file (both endpoints)', async () => { + const { server, store: s } = serverWith('unauthenticated'); + await commit(s, { id: 'a1' }); + for (const p of ['/api/v1/storage/files/:fileId/url', '/api/v1/storage/files/:fileId']) { + const res = await hit(server, p, 'a1'); + expect(res._status, p).toBe(401); + expect(res._json?.code, p).toBe('AUTH_REQUIRED'); + } + }); + + it('403s when the caller cannot read any parent record', async () => { + const { server, store: s } = serverWith('deny'); + await commit(s, { id: 'a2' }); + const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'a2'); + expect(res._status).toBe(403); + expect(res._json?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); + }); + + it('allows the download when authorized, minting a short-lived signed URL', async () => { + const { server, store: s, authorizeFileRead } = serverWith('allow', { downloadTtl: 120 }); + await commit(s, { id: 'a3' }); + const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'a3'); + expect(res._status).toBe(200); + expect(res._json.url).toContain('/_local/raw/'); + expect(authorizeFileRead).toHaveBeenCalledOnce(); + }); + + it('never gates a public_read attachments file (authorizer not consulted)', async () => { + const { server, store: s, authorizeFileRead } = serverWith('deny'); + await commit(s, { id: 'a4', acl: 'public_read' }); + const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'a4'); + expect(res._status).toBe(200); + expect(authorizeFileRead).not.toHaveBeenCalled(); + }); + + it('never gates a non-attachments file (avatars / field files stay open)', async () => { + const { server, store: s, authorizeFileRead } = serverWith('deny'); + await commit(s, { id: 'a5', scope: 'user', key: 'user/a5.png' }); + const res = await hit(server, '/api/v1/storage/files/:fileId', 'a5'); + expect(res._status).toBe(302); + expect(authorizeFileRead).not.toHaveBeenCalled(); + }); + + it('stays open when no authorizer is wired (back-compat)', async () => { + const { server, store: s } = serverWith('skip'); + await commit(s, { id: 'a6' }); + const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'a6'); + expect(res._status).toBe(200); + }); + }); + describe('PUT/GET /_local/raw/:token', () => { it('should accept raw upload with valid token and serve download', async () => { // Generate a presigned upload diff --git a/packages/services/service-storage/src/storage-routes.ts b/packages/services/service-storage/src/storage-routes.ts index 3a85ff018d..a547c3321d 100644 --- a/packages/services/service-storage/src/storage-routes.ts +++ b/packages/services/service-storage/src/storage-routes.ts @@ -2,9 +2,12 @@ import { randomUUID } from 'node:crypto'; import type { IHttpServer, IHttpRequest, IHttpResponse, IStorageService } from '@objectstack/spec/contracts'; -import type { StorageMetadataStore } from './metadata-store.js'; +import type { StorageMetadataStore, FileRecord } from './metadata-store.js'; import type { LocalStorageAdapter } from './local-storage-adapter.js'; +/** Authorization verdict for an attachments-scope download (#2970 item 2). */ +export type FileReadVerdict = 'allow' | 'deny' | 'unauthenticated'; + /** * Options for the storage route registration helper. */ @@ -24,6 +27,26 @@ export interface StorageRoutesOptions { * is a tracked follow-up needing cookie sessions or signed links). */ resolveSession?: (req: IHttpRequest) => Promise<{ userId?: string } | null | undefined>; + /** + * Authorize a DOWNLOAD of an `attachments`-scope file (#2970 item 2). When + * wired, the download endpoints (`/files/:fileId` and `/files/:fileId/url`) + * consult this for `scope==='attachments'`, non-`public_read` files only: + * - `unauthenticated` → 401 (no session) + * - `deny` → 403 (session, but cannot read a parent record the file is + * attached to and is not the owner) + * - `allow` → a short-lived signed URL is issued + * Non-attachments files (field files, avatars, org logos) keep the stable + * anonymous capability URL — they are embedded in `` which cannot + * carry a bearer token, and are out of scope for the attachments leak. + * When absent (bare kernels, tests), all downloads stay open (back-compat). + */ + authorizeFileRead?: (file: FileRecord, req: IHttpRequest) => Promise; + /** + * TTL (seconds) for the signed URL minted on a GATED attachments download. + * Short by design — the link is followed immediately after an explicit + * click. Default 300 (5 min). Non-gated downloads keep `presignedTtl`. + */ + downloadTtl?: number; /** Optional logger for the one-time open-mode notice. */ logger?: { info(msg: string): void; warn(msg: string): void }; } @@ -55,6 +78,40 @@ export function registerStorageRoutes( const basePath = opts.basePath ?? '/api/v1/storage'; const presignedTtl = opts.presignedTtl ?? 3600; const sessionTtl = opts.sessionTtl ?? 86400; + const downloadTtl = opts.downloadTtl ?? 300; + + // ── Download authorization gate (#2970 item 2) ─────────────────────── + // Only `attachments`-scope, non-public files are gated; everything else + // keeps the stable anonymous capability URL (image/avatar embedding). + // Returns the signed-URL TTL to use, or `false` if a response was already + // sent (401/403) and the handler must stop. + const authorizeDownload = async ( + file: FileRecord, + req: IHttpRequest, + res: IHttpResponse, + ): Promise => { + if (file.scope !== 'attachments' || file.acl === 'public_read' || !opts.authorizeFileRead) { + return presignedTtl; + } + let verdict: FileReadVerdict; + try { + verdict = await opts.authorizeFileRead(file, req); + } catch { + verdict = 'deny'; // a failed authz check must never fall open + } + if (verdict === 'unauthenticated') { + res.status(401).json({ error: 'Authentication required to download this file', code: 'AUTH_REQUIRED' }); + return false; + } + if (verdict === 'deny') { + res.status(403).json({ + error: 'You do not have access to a record this file is attached to', + code: 'ATTACHMENT_DOWNLOAD_DENIED', + }); + return false; + } + return downloadTtl; + }; // ── Upload auth gate (#2755) ───────────────────────────────────────── // `false` ⇒ the 401 was already sent and the handler must stop. @@ -431,12 +488,15 @@ export function registerStorageRoutes( return; } + const ttl = await authorizeDownload(file, req, res); + if (ttl === false) return; + let url: string; if (storage.getPresignedDownload) { - const desc = await storage.getPresignedDownload(file.key, presignedTtl); + const desc = await storage.getPresignedDownload(file.key, ttl); url = desc.downloadUrl; } else if (storage.getSignedUrl) { - url = await storage.getSignedUrl(file.key, presignedTtl); + url = await storage.getSignedUrl(file.key, ttl); } else { url = `${basePath}/_local/file/${encodeURIComponent(file.key)}`; } @@ -467,12 +527,15 @@ export function registerStorageRoutes( return; } + const ttl = await authorizeDownload(file, req, res); + if (ttl === false) return; + let url: string; if (storage.getPresignedDownload) { - const desc = await storage.getPresignedDownload(file.key, presignedTtl); + const desc = await storage.getPresignedDownload(file.key, ttl); url = desc.downloadUrl; } else if (storage.getSignedUrl) { - url = await storage.getSignedUrl(file.key, presignedTtl); + url = await storage.getSignedUrl(file.key, ttl); } else { url = `${basePath}/_local/file/${encodeURIComponent(file.key)}`; } diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index 0b19c51823..f64e890b62 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { Plugin, PluginContext } from '@objectstack/core'; +import { resolveAuthzContext } from '@objectstack/core'; import type { IHttpServer, IDataEngine, IStorageService } from '@objectstack/spec/contracts'; import { OBSERVABILITY_METRICS_SERVICE, @@ -12,7 +13,9 @@ import type { LocalStorageAdapterOptions } from './local-storage-adapter.js'; import { S3StorageAdapter } from './s3-storage-adapter.js'; import type { S3StorageAdapterOptions } from './s3-storage-adapter.js'; import { StorageMetadataStore } from './metadata-store.js'; +import type { FileRecord } from './metadata-store.js'; import { registerStorageRoutes } from './storage-routes.js'; +import type { FileReadVerdict } from './storage-routes.js'; import { installAttachmentLifecycleHooks, createSysFileReapGuard } from './attachment-lifecycle.js'; import { installAttachmentAccessHooks, installAttachmentReadVisibility } from './attachment-access-hooks.js'; import { SystemFile, SystemUploadSession } from './objects/index.js'; @@ -251,6 +254,7 @@ export class StorageServicePlugin implements Plugin { presignedTtl: this.options.presignedTtl, sessionTtl: this.options.sessionTtl, resolveSession: buildAuthSessionResolver(ctx), + authorizeFileRead: buildFileReadAuthorizer(ctx, engine), logger: ctx.logger, }); @@ -372,47 +376,53 @@ function resolveMetrics( return new NoopMetricsRegistry(); } +/** Normalize adapter request headers to a Web `Headers` (better-auth needs it). */ +function toWebHeaders(req: { headers?: unknown }): any | null { + const rawHeaders: any = req?.headers; + if (rawHeaders && typeof rawHeaders.get === 'function') return rawHeaders; + if (rawHeaders && typeof rawHeaders === 'object') { + const headers = new (globalThis as any).Headers(); + for (const [k, v] of Object.entries(rawHeaders)) { + if (Array.isArray(v)) v.forEach((x) => headers.append(k, String(x))); + else if (v != null) headers.set(k, String(v)); + } + return headers; + } + return null; +} + +/** A `getSession(headers)` bound to the kernel's `auth` service, or null. */ +function buildGetSession(ctx: PluginContext): ((headers: any) => Promise) | null { + let authService: any; + try { + authService = ctx.getService('auth'); + } catch { + return null; + } + if (!authService) return null; + return async (headers: any) => { + let api: any = authService.api; + if (!api && typeof authService.getApi === 'function') api = await authService.getApi(); + if (!api?.getSession) return undefined; + return api.getSession({ headers }); + }; +} + /** * Bridge the kernel's `auth` service (better-auth) into the storage routes' * upload gate (#2755). Returns `undefined` when no auth service is present — * the routes then stay open (bare kernels/tests, logged once there). - * - * Normalization mirrors rest-server's session resolution: the service may be - * the AuthManager wrapper (`getApi()`) or the raw better-auth instance - * (`.api`), and `getSession` needs a Web `Headers` instance. */ function buildAuthSessionResolver( ctx: PluginContext, ): ((req: { headers?: unknown }) => Promise<{ userId?: string } | null>) | undefined { - let authService: any; - try { - authService = ctx.getService('auth'); - } catch { - return undefined; - } - if (!authService) return undefined; - + const getSession = buildGetSession(ctx); + if (!getSession) return undefined; return async (req) => { try { - let api: any = authService.api; - if (!api && typeof authService.getApi === 'function') api = await authService.getApi(); - if (!api?.getSession) return null; - - const rawHeaders: any = req?.headers; - let headers: any; - if (rawHeaders && typeof rawHeaders.get === 'function') { - headers = rawHeaders; - } else if (rawHeaders && typeof rawHeaders === 'object') { - headers = new (globalThis as any).Headers(); - for (const [k, v] of Object.entries(rawHeaders)) { - if (Array.isArray(v)) v.forEach((x) => headers.append(k, String(x))); - else if (v != null) headers.set(k, String(v)); - } - } else { - return null; - } - - const session: any = await api.getSession({ headers }); + const headers = toWebHeaders(req); + if (!headers) return null; + const session: any = await getSession(headers); const userId = session?.user?.id; return userId ? { userId: String(userId) } : null; } catch { @@ -421,6 +431,70 @@ function buildAuthSessionResolver( }; } +/** + * Authorize an `attachments`-scope download (#2970 item 2). Builds the FULL + * caller ExecutionContext via `resolveAuthzContext` (the same shared resolver + * rest-server uses — a bare `{ userId }` would lack the resolved permissions + * the parent RLS needs), then allows when the caller is the file's owner or + * can READ a record the file is attached to. Returns `undefined` (routes stay + * open) when the auth service or engine is absent. + */ +function buildFileReadAuthorizer( + ctx: PluginContext, + engine: IDataEngine | null, +): ((file: FileRecord, req: { headers?: unknown }) => Promise) | undefined { + const getSession = buildGetSession(ctx); + if (!getSession || !engine || typeof (engine as any).find !== 'function') return undefined; + + return async (file, req) => { + try { + const headers = toWebHeaders(req); + if (!headers) return 'unauthenticated'; + const authz = await resolveAuthzContext({ ql: engine, headers, getSession }); + if (!authz.userId) return 'unauthenticated'; + + // Uploader / owner may always download. + if (file.owner_id && String(file.owner_id) === String(authz.userId)) return 'allow'; + + // Otherwise: readable via any parent record this file is attached to. + const links = (await (engine as any).find('sys_attachment', { + where: { file_id: file.id }, + fields: ['parent_object', 'parent_id'], + limit: 500, + context: { isSystem: true }, + })) as Array>; + + const byObject = new Map>(); + for (const link of links) { + const po = link.parent_object; + const pid = link.parent_id; + if (typeof po !== 'string' || !po || po === 'sys_attachment') continue; + if (pid === undefined || pid === null || pid === '') continue; + let ids = byObject.get(po); + if (!ids) byObject.set(po, (ids = new Set())); + ids.add(String(pid)); + } + for (const [parentObject, idSet] of byObject) { + const ids = [...idSet]; + try { + const visible = (await (engine as any).find(parentObject, { + where: { id: { $in: ids } }, + fields: ['id'], + limit: ids.length, + context: authz, + })) as Array>; + if (visible.length) return 'allow'; + } catch { + // unknown/failing parent object — try the next + } + } + return 'deny'; + } catch { + return 'deny'; // fail closed + } + }; +} + function extractOverrides(payload: unknown): Record { if (!payload || typeof payload !== 'object') return {}; const p = payload as Record; From 94b8e4428574c8e85ab803acfe6017d11dedc426 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Jul 2026 16:08:47 +0000 Subject: [PATCH 3/3] feat(attachments): edit-on-parent attach, session lifecycle, trash=false, showcase files (#2970 items 3-6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - item 3: attach requires EDIT on the parent record (sharing.canEdit), not merely read (Salesforce parity); public parents unchanged, private owner-scoped parents require own/edit. Read fallback without sharing. - item 4: sys_upload_session gains an ADR-0057 transient lifecycle (TTL 1d past expires_at + 7d retention for terminal statuses) so abandoned chunked-upload sessions are reaped. Row reap only; multipart-abort guard is a follow-up. - item 5: sys_attachment enable.trash -> false. The flag is dead in the liveness ledger and deletes are hard (reap guard reclaims bytes), so a restore would dangle; declare the honest state (ADR-0049). - item 6: showcase_project opts into enable.files so the attachments panel renders for browser dogfooding. - dogfood: att_readonly (public_read) fixture + matrix cases for edit-on-parent (read-but-not-edit → 403, still listable) and sys_upload_session reap; access-hook unit tests for the canEdit path. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0187NT3Qer9oep5dCRb9b8Lt --- .changeset/attachment-followups-rest.md | 25 +++++++++ .../src/data/objects/project.object.ts | 6 +++ ...achments-permission-matrix.dogfood.test.ts | 50 ++++++++++++++++++ .../test/fixtures/attachments-fixture.ts | 16 +++++- .../src/audit/sys-attachment.object.ts | 7 ++- .../src/attachment-access-hooks.test.ts | 25 +++++++++ .../src/attachment-access-hooks.ts | Bin 14282 -> 14884 bytes .../objects/system-upload-session.object.ts | 15 ++++++ 8 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 .changeset/attachment-followups-rest.md diff --git a/.changeset/attachment-followups-rest.md b/.changeset/attachment-followups-rest.md new file mode 100644 index 0000000000..868fcd71bc --- /dev/null +++ b/.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. diff --git a/examples/app-showcase/src/data/objects/project.object.ts b/examples/app-showcase/src/data/objects/project.object.ts index 07d5ce2874..b1cf6e3078 100644 --- a/examples/app-showcase/src/data/objects/project.object.ts +++ b/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 diff --git a/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts b/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts index cc826ba289..d35b3220c0 100644 --- a/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts +++ b/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 @@ -450,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(); + }); }); }); diff --git a/packages/dogfood/test/fixtures/attachments-fixture.ts b/packages/dogfood/test/fixtures/attachments-fixture.ts index e1b55f2dc8..6b1038f5e3 100644 --- a/packages/dogfood/test/fixtures/attachments-fixture.ts +++ b/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], }); diff --git a/packages/platform-objects/src/audit/sys-attachment.object.ts b/packages/platform-objects/src/audit/sys-attachment.object.ts index bc7ddf1329..6490481126 100644 --- a/packages/platform-objects/src/audit/sys-attachment.object.ts +++ b/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, }, diff --git a/packages/services/service-storage/src/attachment-access-hooks.test.ts b/packages/services/service-storage/src/attachment-access-hooks.test.ts index 9ab16fa0a0..c6ab30b84c 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/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)', () => { diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 4aaea02cdcd644b50fa4fcaea323d21bbaacd1fd..226ead9d07e50984f7345f242c9260346dd0f418 100644 GIT binary patch delta 599 zcmYL`zityj5XJ>aK{CidLW&UVOdKTcz_G0a@h7ejcBF^~5gvf`-q^R&-LA7cXUB>n zzX079yZ~LANO=Hyo`C`y3Z8%tW^E&HbK1R`-+bRU-^>@fY?W_yJ4YWtNQut*gL3OC zFK^$N9*YcJ-&lz-6jsp$qMf~0FIu1obJ&j$KoAL;Dm?%)Ai$zDHsQ}np)5KG)Dp~2 zv$?>BN=0?hh9F0a*@Wvw(tvkGFH>?iHwk889!@nm{zrw?AcgKGik2|41p;dWp(o7A zduSnY1}H_4CeKaiaR|ymp=B!c0F$^4L#sxDu)YMDu|~V=q#0r|8#|xM4yG38IFV&c zXWd-eM3xbr^9FQqU`4_{$t-ztR{bCNzWcfZBjr?2Wg)S6osM7EaJw9JC7g|7j3$=p-9oYZOAaMZcR#2ZS<46pmqt;jCQ87!}kFa*f(6v%|_rco+k6lK`VvMk{dD_dP@Uyd> mwgZ3VfA6VtWPx