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/.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/.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/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 a5fd57f822..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 @@ -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(); + }); }); }); 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 d0416cee03..226ead9d07 100644 Binary files a/packages/services/service-storage/src/attachment-access-hooks.ts and b/packages/services/service-storage/src/attachment-access-hooks.ts differ diff --git a/packages/services/service-storage/src/attachment-lifecycle.ts b/packages/services/service-storage/src/attachment-lifecycle.ts index 97bf520df5..e705b7f7f2 100644 --- 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, 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..0e5434f511 100644 --- a/packages/services/service-storage/src/index.ts +++ b/packages/services/service-storage/src/index.ts @@ -10,9 +10,9 @@ 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'; -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/objects/system-upload-session.object.ts b/packages/services/service-storage/src/objects/system-upload-session.object.ts index 5863fb6dea..8972e32393 100644 --- a/packages/services/service-storage/src/objects/system-upload-session.object.ts +++ b/packages/services/service-storage/src/objects/system-upload-session.object.ts @@ -118,4 +118,19 @@ export const SystemUploadSession = ObjectSchema.create({ label: 'Updated At', }), }, + + // ADR-0057 (#2970 item 4): an upload session is ephemeral state, never + // business truth — completed, failed, expired, and abandoned in-progress + // sessions would otherwise accumulate forever (the sys_file reaper in + // #2755 covers files, not sessions). The TTL reaps any row 1d past its own + // `expires_at` (abandoned in-progress sessions included); the retention + // backstop reaps terminal-status rows by age even if `expires_at` was + // never set. NOTE: this reaps the session ROW only — a reap guard that + // aborts the backend multipart upload for partial S3 sessions is a filed + // follow-up (row reap is the declared scope of this item). + lifecycle: { + class: 'transient', + ttl: { field: 'expires_at', expireAfter: '1d' }, + retention: { maxAge: '7d', onlyWhen: { status: { $in: ['completed', 'failed', 'expired'] } } }, + }, }); 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.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..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,9 +13,11 @@ 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 } 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 +215,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') { @@ -245,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, }); @@ -366,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 { @@ -415,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;