From 1b2a80f9a49ca3e3495ea27bcd1f879cb10fdc7f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 12:08:42 +0000 Subject: [PATCH 1/2] feat(service-storage): gate sys_attachment beforeUpdate with the uploader-or-parent-editor rule (#10091) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt --- .../src/attachment-access-hooks.test.ts | 353 +++++++++++++++++- .../src/attachment-access-hooks.ts | 337 ++++++++++++----- .../src/storage-service-plugin.test.ts | 5 +- 3 files changed, 603 insertions(+), 92 deletions(-) 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 1b479e209c..e103447b22 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.test.ts @@ -7,7 +7,7 @@ import type { AttachmentLifecycleEngine } from './attachment-lifecycle.js'; const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); -/** Capture the two registered hooks so tests can drive them directly. */ +/** Capture the three registered hooks so tests can drive them directly. */ function install(opts: { attachments?: Array>; sharing?: AttachmentSharingLike | null; @@ -32,6 +32,7 @@ function install(opts: { installAttachmentAccessHooks(engine, () => opts.sharing, silentLogger()); return { beforeInsert: hooks.get('beforeInsert')!, + beforeUpdate: hooks.get('beforeUpdate')!, beforeDelete: hooks.get('beforeDelete')!, }; } @@ -63,6 +64,17 @@ const deleteCtx = (input: any, opts: { userId?: string; isSystem?: boolean; visi api: apiFor(opts.visible ?? []), }); +/** An update ctx: `input.id`/`input.options.where` name the target rows, + * `data` is the SET payload (the per-row dispatch binds `input.id`; the + * whole-operation dispatch carries neither id nor where). */ +const updateCtx = (input: any, data: any, opts: { userId?: string; isSystem?: boolean; visible?: string[] } = {}) => ({ + object: 'sys_attachment', + event: 'beforeUpdate', + input: { ...input, data, options: { ...(input.options ?? {}), context: { userId: opts.userId, permissions: [] } } }, + session: opts.isSystem ? { isSystem: true, userId: opts.userId } : opts.userId ? { userId: opts.userId } : undefined, + api: apiFor(opts.visible ?? []), +}); + describe('attachment access — beforeInsert (parent visibility + provenance)', () => { it('rejects attaching to a parent the caller cannot read (403 ATTACHMENT_PARENT_ACCESS)', async () => { const { beforeInsert } = install({}); @@ -209,6 +221,165 @@ describe('attachment access — beforeDelete (uploader or parent editor)', () => // engine below — see "#4757 through the wired engine". }); +// ───────────────────────────────────────────────────────────────────────── +// #10091 — beforeUpdate: uploader or parent editor, + the attach rule on a +// re-point. The delete gate's rule applied to the verb that could otherwise +// rewrite it away (the comment kit — derived from this one — has gated +// update since #4630; the source kit was missing the limb its derivative +// copied). Wire code is the STANDARD catalog member `RECORD_NOT_ACCESSIBLE` +// (see UPDATE_DENY_CODE in the module), not an ATTACHMENT_* sibling. +// ───────────────────────────────────────────────────────────────────────── +describe('attachment access — beforeUpdate (uploader or parent editor, #10091)', () => { + const row = { id: 'a1', file_id: 'f1', parent_object: 'att_secret', parent_id: 'r1', uploaded_by: 'uploader' }; + + it('the uploader may update their attachment (no parent-edit consult)', async () => { + const canEdit = vi.fn(async () => false); + const { beforeUpdate } = install({ attachments: [row], sharing: { canEdit } }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { description: 'mine' }, { userId: 'uploader' })), + ).resolves.toBeUndefined(); + expect(canEdit).not.toHaveBeenCalled(); + }); + + it('a non-uploader without parent edit is rejected (403 RECORD_NOT_ACCESSIBLE)', async () => { + const { beforeUpdate } = install({ attachments: [row], sharing: { canEdit: async () => false } }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { description: 'x' }, { userId: 'stranger' })), + ).rejects.toMatchObject({ + code: 'RECORD_NOT_ACCESSIBLE', + status: 403, + message: expect.stringContaining('Cannot update attachment a1'), + object: 'att_secret', + }); + }); + + it("a parent editor may update another user's attachment", async () => { + const canEdit = vi.fn(async (object: string, recordId: string, ctx: any) => { + expect(object).toBe('att_secret'); + expect(recordId).toBe('r1'); + expect(ctx.userId).toBe('editor'); + return true; + }); + const { beforeUpdate } = install({ attachments: [row], sharing: { canEdit } }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { description: 'moderated' }, { userId: 'editor' })), + ).resolves.toBeUndefined(); + }); + + // The escalation the issue names, pinned shut: with no update gate a + // stranger could rewrite `uploaded_by` to themselves and then walk through + // the delete gate's uploader shortcut. The row rule refuses the WRITE, so + // what the payload tries to claim never matters. + it('a stranger cannot rewrite uploaded_by to capture the uploader shortcut', async () => { + const { beforeUpdate } = install({ attachments: [row], sharing: { canEdit: async () => false } }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { uploaded_by: 'stranger' }, { userId: 'stranger' })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + }); + + it('multi-update requires EVERY matched row to pass', async () => { + const rows = [ + { ...row, id: 'a1', uploaded_by: 'me' }, + { ...row, id: 'a2', uploaded_by: 'someone-else', parent_id: 'r2' }, + ]; + const { beforeUpdate } = install({ attachments: rows, sharing: { canEdit: async () => false } }); + await expect( + beforeUpdate( + updateCtx({ options: { where: { parent_object: 'att_secret' }, multi: true } }, { description: 'x' }, { userId: 'me' }), + ), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + }); + + it('degrades to parent READ visibility when the sharing service is absent', async () => { + const { beforeUpdate } = install({ attachments: [row], sharing: null }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { description: 'x' }, { userId: 'reader', visible: ['att_secret/r1'] })), + ).resolves.toBeUndefined(); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { description: 'x' }, { userId: 'reader', visible: [] })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + }); + + it('bypasses for system context; ids naming no live row are not blocked', async () => { + const { beforeUpdate } = install({ attachments: [row], sharing: { canEdit: async () => false } }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { description: 'x' }, { isSystem: true, userId: 'x' })), + ).resolves.toBeUndefined(); + await expect( + beforeUpdate(updateCtx({ id: 'missing' }, { description: 'x' }, { userId: 'x' })), + ).resolves.toBeUndefined(); + }); +}); + +describe('attachment access — beforeUpdate re-point (the attach rule on the NEW parent, #10091)', () => { + const row = { id: 'a1', file_id: 'f1', parent_object: 'att_secret', parent_id: 'r1', uploaded_by: 'uploader' }; + + it('re-pointing parent_id requires EDIT on the NEW parent (403 ATTACHMENT_PARENT_ACCESS)', async () => { + // The caller is the uploader, so the row rule passes without consulting + // sharing — the ONE canEdit call below is the attach rule on the target. + const canEdit = vi.fn(async () => false); + const { beforeUpdate } = install({ attachments: [row], sharing: { canEdit } }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { parent_id: 'r2' }, { userId: 'uploader' })), + ).rejects.toMatchObject({ + code: 'ATTACHMENT_PARENT_ACCESS', + status: 403, + message: expect.stringContaining('Cannot move attachment a1 to att_secret/r2'), + object: 'att_secret', + }); + expect(canEdit).toHaveBeenCalledTimes(1); + expect(canEdit).toHaveBeenCalledWith('att_secret', 'r2', expect.objectContaining({ userId: 'uploader' })); + }); + + it('a partial re-point resolves the missing half from the row (parent_object from the row)', async () => { + const canEdit = vi.fn(async () => true); + const { beforeUpdate } = install({ attachments: [row], sharing: { canEdit } }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { parent_id: 'r2' }, { userId: 'uploader' })), + ).resolves.toBeUndefined(); + expect(canEdit).toHaveBeenCalledWith('att_secret', 'r2', expect.objectContaining({ userId: 'uploader' })); + }); + + it('writing the SAME parent back is not a re-point — no attach-rule consult', async () => { + const canEdit = vi.fn(async () => false); + const { beforeUpdate } = install({ attachments: [row], sharing: { canEdit } }); + await expect( + beforeUpdate( + updateCtx({ id: 'a1' }, { parent_object: 'att_secret', parent_id: 'r1', description: 'x' }, { userId: 'uploader' }), + ), + ).resolves.toBeUndefined(); + expect(canEdit).not.toHaveBeenCalled(); + }); + + it('an unauthorizable target is refused, never left to validation (null / empty halves)', async () => { + const { beforeUpdate } = install({ attachments: [row], sharing: { canEdit: async () => true } }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { parent_id: '' }, { userId: 'uploader' })), + ).rejects.toMatchObject({ + code: 'ATTACHMENT_PARENT_ACCESS', + status: 403, + message: expect.stringContaining('does not name a record'), + }); + await expect( + beforeUpdate(updateCtx({ id: 'a1' }, { parent_object: null }, { userId: 'uploader' })), + ).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403 }); + }); + + it('degraded mode (no sharing): the NEW parent must be READ-visible to the caller', async () => { + const { beforeUpdate } = install({ attachments: [row], sharing: null }); + await expect( + beforeUpdate( + updateCtx({ id: 'a1' }, { parent_id: 'r2' }, { userId: 'uploader', visible: ['att_secret/r1', 'att_secret/r2'] }), + ), + ).resolves.toBeUndefined(); + await expect( + beforeUpdate( + updateCtx({ id: 'a1' }, { parent_id: 'r_hidden' }, { userId: 'uploader', visible: ['att_secret/r1'] }), + ), + ).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403 }); + }); +}); + // ───────────────────────────────────────────────────────────────────────── // #4757 through the WIRED engine (#9719) // @@ -232,6 +403,7 @@ const ATT_FIELDS = { parent_object: { name: 'parent_object', label: 'Parent Object', type: 'text' as const }, parent_id: { name: 'parent_id', label: 'Parent Id', type: 'text' as const }, uploaded_by: { name: 'uploaded_by', label: 'Uploaded By', type: 'text' as const }, + description: { name: 'description', label: 'Description', type: 'text' as const }, }; const sysAttachmentObject = { name: 'sys_attachment', label: 'Attachment', fields: ATT_FIELDS }; const attSecretObject = { @@ -279,7 +451,14 @@ function makeWiredDriver() { storeFor(o).set(id, row); return row; }, - async update() { return null; }, + async update(o: string, id: string, data: Record) { + const s = storeFor(o); + const row = s.get(String(id)); + if (!row) return null; + const next = { ...row, ...data, id: row.id }; + s.set(String(id), next); + return next; + }, async delete(o: string, id: string) { return storeFor(o).delete(String(id)); }, async count(o: string, ast: any) { return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)).length; @@ -289,7 +468,11 @@ function makeWiredDriver() { for (const r of doomed) storeFor(o).delete(String(r.id)); return doomed.length; }, - async updateMany() { return 0; }, + async updateMany(o: string, ast: any, data: Record) { + const hit = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + for (const r of hit) storeFor(o).set(String(r.id), { ...r, ...data, id: r.id }); + return hit.length; + }, }; return d; } @@ -434,6 +617,145 @@ describe('unscoped multi-delete (no id, no where) — #4757 through the wired en }); }); +// ───────────────────────────────────────────────────────────────────────── +// #10091 through the WIRED engine — the update verb. +// +// Same rig as the #4757 block above, driving `ql.update('sys_attachment', …)` +// end to end: the unscoped refusal reaches the handler through the +// `dispatchUnscopedMultiWrite` declaration on the NEW `beforeUpdate` +// registration (#9974 made the flag valid on both write verbs); the per-row +// gate fires per matched row; scoped, entitled writes keep resolving. Every +// refusal asserts the row VALUES survived — an overwrite leaves no pre-image, +// so "the table still has N rows" alone proves nothing here. +// ───────────────────────────────────────────────────────────────────────── + +const UNSCOPED_UPDATE_REFUSAL = expect.objectContaining({ + code: 'RECORD_NOT_ACCESSIBLE', + status: 403, + // The first sentence IS the declared contract (mirrors #4757's delete + // refusal, standard catalog code per UPDATE_DENY_CODE's doc block). + message: expect.stringContaining('Refusing an unscoped multi-update of attachments'), +}); + +describe('unscoped multi-update (no id, no where) — #10091 through the wired engine', () => { + it('refuses `{ multi: true }` even when the caller is the uploader of EVERY matched row — and the values survive', async () => { + const { ql, driver } = await bootWired({ + attachments: [wiredRow('a1', 'uploader'), wiredRow('a2', 'uploader', 'r2')], + }); + await expect( + ql.update('sys_attachment', { description: 'swept' }, { multi: true, context: { userId: 'uploader' } } as any), + ).rejects.toEqual(UNSCOPED_UPDATE_REFUSAL); + const store = driver.stores.get('sys_attachment')!; + expect(store.get('a1')!.description).toBeUndefined(); + expect(store.get('a2')!.description).toBeUndefined(); + }); + + it('refuses an explicitly null `where` the same way', async () => { + const { ql, driver } = await bootWired({ attachments: [wiredRow('a1', 'uploader')] }); + await expect( + ql.update('sys_attachment', { description: 'swept' }, { multi: true, where: null, context: { userId: 'uploader' } } as any), + ).rejects.toEqual(UNSCOPED_UPDATE_REFUSAL); + expect(driver.stores.get('sys_attachment')!.get('a1')!.description).toBeUndefined(); + }); + + it('refuses on an EMPTY table — "nothing was ever queried" is not "nothing to authorize"', async () => { + const { ql } = await bootWired({ attachments: [] }); + await expect( + ql.update('sys_attachment', { description: 'swept' }, { multi: true, context: { userId: 'uploader' } } as any), + ).rejects.toEqual(UNSCOPED_UPDATE_REFUSAL); + }); + + it('positive control: `where: {}` is a REAL match-all query and an entitled caller may sweep with it', async () => { + const { ql, driver } = await bootWired({ attachments: [wiredRow('a1', 'uploader')] }); + await expect( + ql.update('sys_attachment', { description: 'swept' }, { multi: true, where: {}, context: { userId: 'uploader' } } as any), + ).resolves.toBeDefined(); + expect(driver.stores.get('sys_attachment')!.get('a1')!.description).toBe('swept'); + }); + + it('the per-row gate is a DIFFERENT refusal and still fires through the wire', async () => { + const { ql, driver } = await bootWired({ + attachments: [wiredRow('a1', 'member'), wiredRow('a2', 'someone-else', 'r2')], + sharing: { canEdit: async () => false }, + }); + await expect( + ql.update('sys_attachment', { description: 'x' }, { + multi: true, + where: { parent_object: 'att_secret' }, + context: { userId: 'member' }, + } as any), + ).rejects.toEqual( + expect.objectContaining({ + code: 'RECORD_NOT_ACCESSIBLE', + status: 403, + message: expect.stringContaining('Cannot update attachment'), + }), + ); + const store = driver.stores.get('sys_attachment')!; + expect(store.get('a1')!.description).toBeUndefined(); + expect(store.get('a2')!.description).toBeUndefined(); + }); + + it('scoped controls still pass and actually write: by id, and by a real `where`', async () => { + const byId = await bootWired({ attachments: [wiredRow('a1', 'uploader')] }); + await expect( + byId.ql.update('sys_attachment', { id: 'a1', description: 'edited' }, { context: { userId: 'uploader' } } as any), + ).resolves.toBeDefined(); + expect(byId.driver.stores.get('sys_attachment')!.get('a1')!.description).toBe('edited'); + + const byWhere = await bootWired({ + attachments: [wiredRow('a1', 'uploader'), wiredRow('a2', 'uploader', 'r2')], + }); + await expect( + byWhere.ql.update('sys_attachment', { description: 'batch' }, { + multi: true, + where: { uploaded_by: 'uploader' }, + context: { userId: 'uploader' }, + } as any), + ).resolves.toBeDefined(); + expect(byWhere.driver.stores.get('sys_attachment')!.get('a1')!.description).toBe('batch'); + expect(byWhere.driver.stores.get('sys_attachment')!.get('a2')!.description).toBe('batch'); + }); + + it('re-pointing through the wire requires the attach rule on the NEW parent', async () => { + const canEdit = vi.fn(async (_o: string, recordId: string) => recordId !== 'r_hidden'); + const { ql, driver } = await bootWired({ + attachments: [wiredRow('a1', 'uploader')], + sharing: { canEdit }, + }); + // Uploader row rule passes without consulting sharing; the target does not. + await expect( + ql.update('sys_attachment', { id: 'a1', parent_id: 'r_hidden' }, { context: { userId: 'uploader' } } as any), + ).rejects.toEqual( + expect.objectContaining({ + code: 'ATTACHMENT_PARENT_ACCESS', + status: 403, + message: expect.stringContaining('Cannot move attachment a1 to att_secret/r_hidden'), + }), + ); + expect(driver.stores.get('sys_attachment')!.get('a1')!.parent_id).toBe('r1'); + + await expect( + ql.update('sys_attachment', { id: 'a1', parent_id: 'r2' }, { context: { userId: 'uploader' } } as any), + ).resolves.toBeDefined(); + expect(driver.stores.get('sys_attachment')!.get('a1')!.parent_id).toBe('r2'); + }); + + it('still bypasses for system context and for context-less programmatic calls (unscoped shape included)', async () => { + const system = await bootWired({ attachments: [wiredRow('a1', 'someone')] }); + await expect( + system.ql.update('sys_attachment', { description: 'sys' }, { multi: true, context: { isSystem: true } } as any), + ).resolves.toBeDefined(); + expect(system.driver.stores.get('sys_attachment')!.get('a1')!.description).toBe('sys'); + + const bare = await bootWired({ attachments: [wiredRow('a1', 'someone')] }); + await expect( + bare.ql.update('sys_attachment', { description: 'bare' }, { multi: true } as any), + ).resolves.toBeDefined(); + expect(bare.driver.stores.get('sys_attachment')!.get('a1')!.description).toBe('bare'); + }); +}); + // ───────────────────────────────────────────────────────────────────────── // #7145 — what the gate FORWARDS to the sharing service // @@ -518,6 +840,15 @@ const envelopeDeleteCtx = (input: any, exec: Record) => ({ api: apiFor([]), }); +/** An update ctx carrying an explicit execution envelope. */ +const envelopeUpdateCtx = (input: any, data: any, exec: Record) => ({ + object: 'sys_attachment', + event: 'beforeUpdate', + input: { ...input, data, options: { ...(input.options ?? {}), context: exec } }, + session: { userId: exec.userId as string }, + api: apiFor([]), +}); + /** The deployment's `fallbackPermissionSet` (ADR-0056 D7: an app's `isDefault` * profile, else the built-in `member_default`). */ const DEPLOYMENT_BASELINE_SET = 'app_default_profile'; @@ -598,6 +929,22 @@ describe('#7145 — caller envelope forwarded to the sharing gate', () => { for (const key of OPERATION_PRIVATE_KEYS) expect(forwarded).not.toHaveProperty(key); }); + it('beforeUpdate forwards the whole envelope MINUS the operation-private keys — on BOTH its call sites (#10091)', async () => { + // One update that exercises the row rule (caller is not the uploader) + // AND the re-point attach rule (parent_id changes): two canEdit calls, + // both reading the same callerContext() seam the other verbs pinned. + const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true); + const { beforeUpdate } = install({ attachments: [attRow], sharing: { canEdit } }); + await beforeUpdate(envelopeUpdateCtx({ id: 'a1' }, { parent_id: 'r2' }, { ...DELEGATED_ENVELOPE })); + + expect(canEdit).toHaveBeenCalledTimes(2); // row rule on r1, attach rule on r2 + for (const call of canEdit.mock.calls) { + const forwarded = call[2] as unknown as Record; + expect(forwarded).toEqual(DELEGATED_PRINCIPAL_FIELDS); + for (const key of OPERATION_PRIVATE_KEYS) expect(forwarded).not.toHaveProperty(key); + } + }); + it('hands the service a COPY, so a callee stamping its own depth cannot write back', async () => { const exec: Record = { ...DELEGATED_ENVELOPE }; const canEdit = vi.fn(async (_o: string, _r: string, callerCtx: any) => { diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 1ab6188e92..c4d5a09cf4 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.ts @@ -15,9 +15,9 @@ import type { * `sys_attachment` rows are written through the generic data path, and the * default member permission sets grant wildcard CRUD with no row scoping — * without these hooks any member can attach files to records they cannot - * see and delete any other user's attachments. Salesforce semantics - * (ContentDocumentLink): an attachment's access is derived from its PARENT - * record. + * see and rewrite or delete any other user's attachments. Salesforce + * semantics (ContentDocumentLink): an attachment's access is derived from + * its PARENT record. * * - beforeInsert: the caller must be able to READ the parent record — * verified with a caller-scoped findOne so RLS/OWD/sharing apply. @@ -25,6 +25,20 @@ import type { * on the parent; v1 enforces read visibility — strictly better than * nothing, edit-parity is a tracked follow-up.) `uploaded_by` is * server-stamped from the session — a client-supplied value never wins. + * - beforeUpdate (#10091): the caller must be the uploader OR hold edit on + * the parent record — the delete rule, applied to the verb that could + * otherwise rewrite the other two gates away: an ungated update let any + * member re-point `parent_id` at a record they cannot see, or rewrite + * `uploaded_by` and walk through the delete gate's uploader shortcut. + * Fail-closed 403, standard catalog code `RECORD_NOT_ACCESSIBLE` (see + * {@link UPDATE_DENY_CODE}). A re-point of `parent_object`/`parent_id` + * must additionally satisfy the attach rule on the NEW parent (403 + * `ATTACHMENT_PARENT_ACCESS`), and an unscoped multi-update is refused + * outright — mirroring the delete verb below, through the same + * `dispatchUnscopedMultiWrite` declaration (#9974 made it valid on both + * write verbs). This is the rule the derived sys_comment kit + * (`comment-access-hooks.ts`) has carried on update since #4630; the + * source kit was missing the limb its derivative copied. * - beforeDelete: the caller must be the uploader OR hold edit on the * parent record (sharing service's `canEdit`; public-model parents are * editable by design). Fail-closed 403 `ATTACHMENT_DELETE_DENIED`; a @@ -37,8 +51,8 @@ import type { * per-row dispatch alone can never deliver the shape it refuses. * * System-context operations (engine self-writes, seeds, lifecycle sweeps) - * bypass both gates, as do context-less programmatic calls on bare kernels - * (no principal to authorize — REST always carries a context). + * bypass all three gates, as do context-less programmatic calls on bare + * kernels (no principal to authorize — REST always carries a context). * * These run alongside plugin-audit's `enforceFilesCapability` (the * enable.files opt-in gate); both are fail-closed 403s, so their relative @@ -52,9 +66,25 @@ export interface AttachmentSharingLike { const PACKAGE_ID = 'com.objectstack.service.storage'; const SYSTEM_CTX = { isSystem: true } as const; -/** Bound on join rows authorized per multi-delete; mirrors the lifecycle - * hooks' resolve bound. Larger multi-deletes fail closed. */ -const MULTI_DELETE_AUTH_LIMIT = 1_000; +/** Bound on join rows authorized per multi-update/-delete; mirrors the + * lifecycle hooks' resolve bound. Larger multi-writes fail closed. */ +const MULTI_WRITE_AUTH_LIMIT = 1_000; + +/** + * The wire code the UPDATE gate emits. Deliberately the STANDARD catalog + * member (`StandardErrorCode`, "Sharing rule restriction") rather than an + * `ATTACHMENT_UPDATE_DENIED` sibling of the insert/delete codes: ADR-0112 + * says a generic permission condition takes the catalog, and since #8211 the + * error-code ledger's admission gate mechanically REFUSES a new extension + * code that shadows a standard member. `ATTACHMENT_PARENT_ACCESS` / + * `ATTACHMENT_DELETE_DENIED` predate that rule (grandfathered, wire values + * unchanged — consolidating either is a deliberate wire change with its own + * card per the #8211 adjudication), so the delete gate keeps its code and + * the new verb takes the vocabulary the rule asks for — exactly what the + * derived comment kit did (`DENY_CODE` in `comment-access-hooks.ts`). REST + * already maps this code 403 + object-preserving (`error-response.ts`). + */ +const UPDATE_DENY_CODE = 'RECORD_NOT_ACCESSIBLE'; function forbid(code: string, message: string, object?: string): never { const err: any = new Error(message); @@ -152,6 +182,117 @@ export function installAttachmentAccessHooks( getSharing: () => AttachmentSharingLike | null | undefined, logger: AttachmentLifecycleLogger, ): void { + /** Resolve every sys_attachment row a write matches, under SYSTEM context + * — the caller may legitimately be unable to READ rows they are allowed to + * touch (and the read-visibility middleware below must not narrow the + * authorization input). Fails closed on an unscoped multi-write and on an + * over-large match set. One resolver for BOTH write verbs — the shape of + * `resolveTargetRows` in the derived sys_comment kit — so the two gates + * cannot drift apart on what "the matched rows" means. */ + const resolveTargetRows = async ( + ctx: any, + verb: 'update' | 'delete', + denyCode: string, + ): Promise>> => { + const ids = asIdList(ctx?.input?.id); + if (ids) { + const rows: Array> = []; + for (const id of ids) { + const row = await engine.findOne('sys_attachment', { where: { id }, context: { ...SYSTEM_CTX } }); + if (row) rows.push(row); + } + return rows; + } + const where = ctx?.input?.options?.where; + if (where === undefined || where === null) { + // #4757 — no id AND no predicate: the engine hands the driver an AST of + // `{ object }`, i.e. the WHOLE table. Falling through here would + // authorize that by resolving zero rows, so refuse instead. "Nothing to + // authorize" and "nothing was ever queried" are not the same verdict; + // reading the second as the first is fail-open. (Mirrors #4630's + // `resolveTargetRows` for sys_comment.) + // + // [#9719/#9974] Reached through the wired engine ONLY via the + // `dispatchUnscopedMultiWrite` whole-operation dispatch BOTH write + // registrations below declare: the per-row contract (#5038/#5574) + // binds `input.id` on every predicate dispatch — which routes into the + // by-id branch above — and a zero-match predicate dispatches nothing + // at all, so without that declaration this refusal cannot fire, + // whatever this file says. #9719 built the dispatch for `beforeDelete` + // only; #9974 (option A, 2026-08-19) ruled it onto `beforeUpdate`, on + // the recoverability asymmetry: an overwrite leaves no trace and no + // pre-image, so the verb with the less recoverable failure must not be + // the less guarded one. + // + // ⛔ If this branch ever stops firing on a verb, the fix is the + // DISPATCH, not this policy — do not widen the branch to compensate. + forbid( + denyCode, + `Refusing an unscoped multi-${verb} of attachments — scope the ${verb} to the rows you mean (an id or a where predicate)`, + ); + } + const rows = await engine.find('sys_attachment', { + where, + limit: MULTI_WRITE_AUTH_LIMIT + 1, + context: { ...SYSTEM_CTX }, + }); + if (rows.length > MULTI_WRITE_AUTH_LIMIT) { + forbid( + denyCode, + `Refusing to authorize a multi-${verb} matching more than ${MULTI_WRITE_AUTH_LIMIT} attachments`, + ); + } + return rows; + }; + + /** Uploader-or-parent-editor over every matched row. Parent-edit verdicts + * are memoized per (object, id) — a multi-row write usually targets one + * record. Degrades to caller-scoped parent READ visibility when no sharing + * service is present — still strictly tighter than no gate. */ + const authorizeRows = async ( + ctx: any, + rows: Array>, + verb: 'update' | 'delete', + denyCode: string, + ): Promise => { + const userId = ctx.session.userId as string | undefined; + const sharing = getSharing(); + const callerCtx = callerContext(ctx); + const canEditCache = new Map(); + for (const row of rows) { + if (userId && row.uploaded_by === userId) continue; // the uploader governs their own attachment + + const parentObject = String(row.parent_object ?? ''); + const parentId = String(row.parent_id ?? ''); + const cacheKey = `${parentObject}\u0000${parentId}`; + let allowed = canEditCache.get(cacheKey); + if (allowed === undefined) { + if (sharing && typeof sharing.canEdit === 'function') { + allowed = await sharing.canEdit(parentObject, parentId, callerCtx); + } else { + // Degraded mode (no sharing service): fall back to caller-scoped + // parent READ visibility — still strictly tighter than no gate. + try { + allowed = !!(await ctx.api.object(parentObject).findOne({ where: { id: parentId } })); + } catch { + allowed = false; + } + logger.debug?.( + `[storage] attachment access: sharing service absent — ${verb} gated on parent read visibility`, + ); + } + canEditCache.set(cacheKey, allowed); + } + if (!allowed) { + forbid( + denyCode, + `Cannot ${verb} attachment ${row.id}: only the uploader or a user who can edit the parent record (${parentObject}/${parentId}) may ${verb} it`, + parentObject, + ); + } + } + }; + // ── Create: parent-record EDIT access + uploaded_by stamping ──────── engine.registerHook( 'beforeInsert', @@ -201,112 +342,134 @@ export function installAttachmentAccessHooks( { object: 'sys_attachment', packageId: PACKAGE_ID }, ); - // ── Delete: uploader or parent editor ─────────────────────────────── + // ── Update: uploader or parent editor (+ the attach rule on a re-point) ── engine.registerHook( - 'beforeDelete', + 'beforeUpdate', async (ctx: any) => { if (ctx?.session?.isSystem) return; if (!ctx?.session) return; // context-less programmatic call (bare kernel) - const userId = ctx.session.userId as string | undefined; + const rows = await resolveTargetRows(ctx, 'update', UPDATE_DENY_CODE); + // Reached only after a real resolve (the unscoped shape was refused in + // the resolver): ids that name no live row mean the driver writes + // nothing, so there is genuinely nothing to gate. The re-point check + // below is deliberately per-ROW for the same reason — with no matched + // row a partial re-point (`parent_object` and `parent_id` are two + // independent columns) has no effective target to authorize, and no + // write to carry it. + if (!rows.length) return; + await authorizeRows(ctx, rows, 'update', UPDATE_DENY_CODE); - // Resolve every row this delete matches (system read — the caller may - // legitimately be unable to READ rows they are allowed to detach). - let rows: Array> = []; - const ids = asIdList(ctx?.input?.id); - if (ids) { - for (const id of ids) { - const row = await engine.findOne('sys_attachment', { where: { id }, context: { ...SYSTEM_CTX } }); - if (row) rows.push(row); - } - } else { - const where = ctx?.input?.options?.where; - if (where === undefined || where === null) { - // #4757 — no id AND no predicate: the engine hands `deleteMany` an - // AST of `{ object }`, i.e. the WHOLE table. Falling through here - // would authorize that by resolving zero rows, so refuse instead. - // "Nothing to authorize" and "nothing was ever queried" are not the - // same verdict; reading the second as the first is fail-open. - // (Mirrors #4630's `resolveTargetRows` for sys_comment.) - // - // [#9719] Reached through the wired engine ONLY via the - // `dispatchUnscopedMultiWrite` whole-operation dispatch declared on - // this registration (see the registration options below): the - // per-row contract (#5038/#5574) binds `input.id` on every predicate - // dispatch — which routes into the by-id branch above — and a - // zero-match predicate dispatches nothing at all, so without that - // declaration this refusal cannot fire, whatever this file says. - forbid( - 'ATTACHMENT_DELETE_DENIED', - 'Refusing an unscoped multi-delete of attachments — scope the delete to the rows you mean (an id or a where predicate)', - ); + // NOTE `uploaded_by` is deliberately NOT re-stamped here: the insert + // stamp records provenance at creation, and re-stamping on update would + // hand the row to whoever edits it. Nor is a client-supplied value + // refused: to reach this point at all the caller is already the + // uploader or a parent editor, and a parent editor already holds every + // verb the uploader shortcut grants — so rewriting `uploaded_by` buys + // no privilege the row rule did not just verify. (Same posture as the + // derived comment kit takes for `author_id` on update. The escalation + // the issue names — rewrite `uploaded_by`, then delete as "uploader" — + // is closed by the row rule itself, not by stamping.) + + // Re-pointing an attachment is an INSERT into the new parent's files + // panel: the NEW parent must satisfy the attach rule (EDIT via the + // sharing service, degrading to caller-scoped read visibility — the + // beforeInsert gate above), or an authorized edit of your own + // attachment would be a way to plant files on records you cannot see. + // Mirrors the comment kit's thread re-point rule (#4630). + const data: any = ctx?.input?.data; + if (!data || typeof data !== 'object') return; + if (data.parent_object === undefined && data.parent_id === undefined) return; + + const sharing = getSharing(); + /** Attach-rule verdicts memoized per effective (object, id) target. */ + const canAttachCache = new Map(); + for (const row of rows) { + const nextObject = data.parent_object === undefined ? row.parent_object : data.parent_object; + const nextId = data.parent_id === undefined ? row.parent_id : data.parent_id; + if ( + String(nextObject ?? '') === String(row.parent_object ?? '') && + String(nextId ?? '') === String(row.parent_id ?? '') + ) { + continue; // parent unchanged on this row — no re-point to authorize } - rows = await engine.find('sys_attachment', { - where, - limit: MULTI_DELETE_AUTH_LIMIT + 1, - context: { ...SYSTEM_CTX }, - }); - if (rows.length > MULTI_DELETE_AUTH_LIMIT) { + // An unauthorizable target is a REFUSED one (fail closed, like the + // comment kit's unparseable thread_id) — never "let validation report + // the miss" as the insert gate does for absent fields: here the field + // IS present, and a `null`/empty half that validation happened to + // admit would otherwise sail past this gate. + if (typeof nextObject !== 'string' || !nextObject || nextId === undefined || nextId === null || nextId === '') { forbid( - 'ATTACHMENT_DELETE_DENIED', - `Refusing to authorize a multi-delete matching more than ${MULTI_DELETE_AUTH_LIMIT} attachments`, + 'ATTACHMENT_PARENT_ACCESS', + `Cannot move attachment ${row.id}: ${JSON.stringify(nextObject ?? null)}/${JSON.stringify(nextId ?? null)} does not name a record`, ); } - } - // Reached only after a real resolve: the query ran and matched no row - // (or the ids name no live row), so there is genuinely nothing to gate. - if (!rows.length) return; - - const sharing = getSharing(); - const callerCtx = callerContext(ctx); - /** Parent-edit results memoized per (object, id) — multi-deletes on one record. */ - const canEditCache = new Map(); - - for (const row of rows) { - if (userId && row.uploaded_by === userId) continue; // uploader may always detach - - const parentObject = String(row.parent_object ?? ''); - const parentId = String(row.parent_id ?? ''); - const cacheKey = `${parentObject}\u0000${parentId}`; - let allowed = canEditCache.get(cacheKey); + const cacheKey = `${nextObject}\u0000${String(nextId)}`; + let allowed = canAttachCache.get(cacheKey); if (allowed === undefined) { if (sharing && typeof sharing.canEdit === 'function') { - allowed = await sharing.canEdit(parentObject, parentId, callerCtx); + allowed = await sharing.canEdit(nextObject, String(nextId), callerContext(ctx)); } else { - // Degraded mode (no sharing service): fall back to caller-scoped - // parent READ visibility — still strictly tighter than no gate. try { - allowed = !!(await ctx.api.object(parentObject).findOne({ where: { id: parentId } })); + allowed = !!(await ctx.api.object(nextObject).findOne({ where: { id: nextId } })); } catch { allowed = false; } logger.debug?.( - '[storage] attachment access: sharing service absent — delete gated on parent read visibility', + '[storage] attachment access: sharing service absent — re-point gated on parent read visibility', ); } - canEditCache.set(cacheKey, allowed); + canAttachCache.set(cacheKey, allowed); } if (!allowed) { forbid( - 'ATTACHMENT_DELETE_DENIED', - `Cannot delete attachment ${row.id}: only the uploader or a user who can edit the parent record (${parentObject}/${parentId}) may delete it`, - parentObject, + 'ATTACHMENT_PARENT_ACCESS', + `Cannot move attachment ${row.id} to ${nextObject}/${String(nextId)}: the parent record does not exist or you cannot edit it`, + nextObject, ); } } }, - // [#9719] `dispatchUnscopedMultiWrite` is what makes the #4757 branch - // above REACHABLE through the wired engine: the predicate path dispatches - // per row with `input.id` bound (so the by-id branch shadows the check), - // and a zero-match predicate dispatches nothing at all — the engine's - // opt-in whole-operation dispatch is the one call that arrives with no id - // and the caller's raw `options`, before any row is resolved. + // [#9974] `dispatchUnscopedMultiWrite` is what makes the unscoped refusal + // in `resolveTargetRows` REACHABLE on update: the per-row contract + // (#5038/#5574) binds `input.id` on every predicate dispatch (so the + // by-id branch shadows the shape check), and a zero-match predicate + // dispatches nothing at all — the whole-operation dispatch is the one + // call that arrives with no id and the caller's raw `options`, before any + // row is resolved. Same declaration the `beforeDelete` registration below + // carries (#9719), and the same both-verbs pairing the derived comment + // kit ships. + { object: 'sys_attachment', packageId: PACKAGE_ID, dispatchUnscopedMultiWrite: true }, + ); + + // ── Delete: uploader or parent editor ─────────────────────────────── + engine.registerHook( + 'beforeDelete', + async (ctx: any) => { + if (ctx?.session?.isSystem) return; + if (!ctx?.session) return; // context-less programmatic call (bare kernel) + // Resolve every row this delete matches (system read — the caller may + // legitimately be unable to READ rows they are allowed to detach). + const rows = await resolveTargetRows(ctx, 'delete', 'ATTACHMENT_DELETE_DENIED'); + // Reached only after a real resolve: the query ran and matched no row + // (or the ids name no live row), so there is genuinely nothing to gate. + if (!rows.length) return; + await authorizeRows(ctx, rows, 'delete', 'ATTACHMENT_DELETE_DENIED'); + }, + // [#9719] `dispatchUnscopedMultiWrite` is what makes the #4757 branch in + // `resolveTargetRows` REACHABLE through the wired engine: the predicate + // path dispatches per row with `input.id` bound (so the by-id branch + // shadows the check), and a zero-match predicate dispatches nothing at + // all — the engine's opt-in whole-operation dispatch is the one call that + // arrives with no id and the caller's raw `options`, before any row is + // resolved. // - // [#9974] The flag was renamed from `dispatchUnscopedMultiDelete` when the - // mechanism was ruled onto `beforeUpdate` as well. This guard stays - // DELETE-ONLY on purpose: #4757 declares an unscoped-multi refusal for the - // delete verb only, and `sys_attachment` registers no `beforeUpdate` guard - // at all — the flag is per-registration, so declaring it here and nowhere - // else says exactly that. Nothing about this object's accept set changed. + // [#9974] The flag was renamed from `dispatchUnscopedMultiDelete` when + // the mechanism was ruled onto `beforeUpdate` as well; the refusal stays + // PER REGISTRATION. This one carries #4757's delete refusal under its + // grandfathered `ATTACHMENT_DELETE_DENIED` envelope; the `beforeUpdate` + // registration above declares the update-verb refusal (#10091) under the + // standard catalog code — the same both-verbs pairing the derived + // comment kit ships. { object: 'sys_attachment', packageId: PACKAGE_ID, dispatchUnscopedMultiWrite: true }, ); } 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 ded8c658cf..1d32c6cce5 100644 --- a/packages/services/service-storage/src/storage-service-plugin.test.ts +++ b/packages/services/service-storage/src/storage-service-plugin.test.ts @@ -355,14 +355,15 @@ describe('StorageServicePlugin: sys_file orphan lifecycle wiring (#2755)', () => await ctx._flushReady(); // Lifecycle hooks (beforeDelete/afterDelete/afterInsert) + access hooks - // (beforeInsert/beforeDelete) — see attachment-lifecycle.ts and - // attachment-access-hooks.ts. + // (beforeInsert/beforeUpdate/beforeDelete, #10091 added the update verb) + // — see attachment-lifecycle.ts and attachment-access-hooks.ts. expect(hookEvents.sort()).toEqual([ 'afterDelete', 'afterInsert', 'beforeDelete', 'beforeDelete', 'beforeInsert', + 'beforeUpdate', ]); // Field-reference ownership: claim/copy on write, release on delete // (file-reference-lifecycle.ts). Registered without an object filter. From bd928759b01479104f9208dbdff3254681786af1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 12:11:43 +0000 Subject: [PATCH 2/2] chore: changeset for the sys_attachment beforeUpdate gate (#10091) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt --- .changeset/attachment-before-update-guard.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 .changeset/attachment-before-update-guard.md diff --git a/.changeset/attachment-before-update-guard.md b/.changeset/attachment-before-update-guard.md new file mode 100644 index 0000000000..5ac0c9df7b --- /dev/null +++ b/.changeset/attachment-before-update-guard.md @@ -0,0 +1,15 @@ +--- +"@objectstack/service-storage": patch +--- + +**Behaviour change (tightening):** updates of `sys_attachment` rows are now authorization-gated, where they previously ran with **no record-level check at all** (#10091). + +`installAttachmentAccessHooks` gated insert (parent-edit access, `uploaded_by` server-stamped) and delete (uploader-or-parent-editor), but registered **no `beforeUpdate` hook** — so under the default member permission sets (wildcard CRUD, no row scoping) any member could rewrite any attachment row: re-point `parent_id` at a record they cannot read, or rewrite `uploaded_by` and then walk through the delete gate's uploader shortcut. The `sys_comment` kit — explicitly derived from this one — has gated update with the same rule since #4630; the source kit was missing the limb its derivative copied. + +The new `beforeUpdate` gate narrows the accept set as follows; if a currently-working update starts failing, the caller lacked rights the other two verbs already required: + +- **Row rule:** the caller must be the attachment's uploader OR hold edit on its parent record (`ISharingService.canEdit`; degrades to caller-scoped parent READ visibility when no sharing service is present). A multi-row update requires EVERY matched row to pass. Refusals are HTTP 403 with the **standard catalog code `RECORD_NOT_ACCESSIBLE`** (ADR-0112: generic permission conditions take the catalog — the same envelope the comment kit's update gate emits; the insert/delete gates keep their grandfathered `ATTACHMENT_*` codes). +- **Re-point rule:** an update that changes `parent_object`/`parent_id` must additionally satisfy the attach rule on the NEW parent (edit access, read visibility in degraded mode) — 403 `ATTACHMENT_PARENT_ACCESS` otherwise, and a re-point half that names no record (`null`/empty) is refused rather than left to validation. +- **Unscoped shape:** an unscoped `multi: true` update (no `where` at all) is refused outright via the `dispatchUnscopedMultiWrite` whole-operation dispatch (#9974), mirroring the delete verb's #4757 refusal. The explicit match-all `where: {}` is still accepted and authorized per row. + +System-context operations and context-less programmatic calls on bare kernels bypass the gate exactly as the insert/delete gates do. `uploaded_by` is deliberately not re-stamped on update: the caller is already verified as uploader or parent editor before the write proceeds, so the rewrite-then-uploader-delete escalation is closed by the row rule itself.