From eb2f89eda3eb87d628045b7058f3e36cc6241339 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 02:30:28 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(plugin-audit,rest)!:=20sys=5Fcomment=20?= =?UTF-8?q?=E7=9A=84=E8=AE=BF=E9=97=AE=E6=9D=83=E4=BB=8E=20thread=5Fid=20?= =?UTF-8?q?=E6=8C=87=E5=90=91=E7=9A=84=E8=AE=B0=E5=BD=95=E7=BB=A7=E6=89=BF?= =?UTF-8?q?=20(#4630)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sys_attachment 的可见性由父记录派生,sys_comment 什么都不派生:同一条 记录、同一个用户,两者答案不同 —— 读不到 opportunity 的 rep2 依然能列出 它下面的评论,并且能 POST 一条(连 `"crm_opportunity:"` 这种空 id 的悬空 thread 也 201)。sys_comment 是 public、无 owner 列、父记录藏在 thread_id 字符串里,所以 OWD/sharing 与 RLS 从来没有收窄过它;而 enable.feeds 是 opt-out(spec 默认 true),于是每个应用的每个对象都挂着这条组织级可读可写 的旁路。 AuditPlugin 现在装上 service-storage 给 sys_attachment 装的同一套两件套, 按 thread_id 的 `{object_name}:{record_id}` 解析父记录: - 读侧:find/findOne/count/aggregate 中间件把查询与"调用者真的读得到的 thread"求交(用调用者上下文探测父对象,父对象自己的 OWD/sharing/RLS/ 对象级 CRUD 说了算)。count() 与 find() 同样被过滤,列表 total 不会泄露 被隐藏行的存在。 - 写侧:beforeInsert 要求对父记录可读(能看见的记录就能讨论); beforeUpdate / beforeDelete 要求调用者是评论作者,或对父记录有 EDIT (attachment 的 uploader-or-parent-editor 规则)。author_id 由服务端按 会话盖章,客户端传的值永远不生效 —— 否则"作者可删"本身就可伪造。 全部 fail closed:解析不出记录的 thread_id(悬空空 id、自由文本、指向 sys_comment 自身)写入拒绝、读取排除;过滤器算不出来就 deny-all。拒绝统一 答 403 `RECORD_NOT_ACCESSIBLE` —— 按 ADR-0112 的账本约定,通用权限条件用 标准目录里的码而不是新造同义词;`error.object` 报父记录的对象名,REST 的 映射分支与 attachment/feeds 两个网关同形。 正交且未改动:enable.feeds(FEEDS_DISABLED)仍然只管"这个对象有没有评论", 匿名调用仍然在这一切之前 401。 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny --- .../sys-comment-record-level-authorization.md | 69 +++ .../plugin-audit/src/audit-plugin.test.ts | 72 +++ .../plugins/plugin-audit/src/audit-plugin.ts | 33 ++ .../src/comment-access-hooks.test.ts | 303 +++++++++++ .../plugin-audit/src/comment-access-hooks.ts | 513 ++++++++++++++++++ .../src/comment-read-visibility.test.ts | 323 +++++++++++ packages/plugins/plugin-audit/src/index.ts | 12 + ...comments-permission-matrix.dogfood.test.ts | 222 ++++++++ .../dogfood/test/fixtures/comments-fixture.ts | 110 ++++ packages/rest/src/rest-server.ts | 18 + packages/rest/src/rest.test.ts | 30 + 11 files changed, 1705 insertions(+) create mode 100644 .changeset/sys-comment-record-level-authorization.md create mode 100644 packages/plugins/plugin-audit/src/comment-access-hooks.test.ts create mode 100644 packages/plugins/plugin-audit/src/comment-access-hooks.ts create mode 100644 packages/plugins/plugin-audit/src/comment-read-visibility.test.ts create mode 100644 packages/qa/dogfood/test/comments-permission-matrix.dogfood.test.ts create mode 100644 packages/qa/dogfood/test/fixtures/comments-fixture.ts diff --git a/.changeset/sys-comment-record-level-authorization.md b/.changeset/sys-comment-record-level-authorization.md new file mode 100644 index 0000000000..6be86e8ea7 --- /dev/null +++ b/.changeset/sys-comment-record-level-authorization.md @@ -0,0 +1,69 @@ +--- +"@objectstack/plugin-audit": major +"@objectstack/rest": patch +--- + +fix(plugin-audit,rest)!: `sys_comment` derives its access from the record its thread names (#4630) + +Attachments derive their visibility from the parent record; comments derived +nothing. On the *same* record, with the *same* user, the two answered +differently: + +``` +user: rep2 (does NOT own and cannot read the opportunity) +GET /api/v1/data/crm_opportunity?$filter=["id","=","1A7n…"] → 200, 0 rows +GET /api/v1/data/sys_attachment?$filter=["parent_id","=","1A7n…"] → 200, 0 rows +GET /api/v1/data/sys_comment?$filter=["thread_id","=","crm_opportunity:1A7n…"] + → 200, 1 row +POST /api/v1/data/sys_comment {"thread_id":"crm_opportunity:", …} → 201 Created +``` + +`sys_comment` is public, has no owner column, and hides its parent inside a +string (`thread_id` = `{object_name}:{record_id}`), so neither OWD/sharing nor +RLS ever narrowed it. Because `enable.feeds` is opt-OUT (spec default `true`), +every object in every app carried that org-wide readable, org-wide writable +side-channel — a deployment that carefully authored OWD, sharing rules and RLS +on its records still leaked their discussion. + +`AuditPlugin` now installs the same two-part kit `service-storage` installs for +`sys_attachment`, keyed off `thread_id`'s parent: + +- **read** — a `find`/`findOne`/`count`/`aggregate` middleware intersects every + query with the threads whose record the caller can actually read (resolved + through the caller-scoped engine, so the parent's own OWD/sharing/RLS/CRUD + decide). `count()` is filtered identically to `find()`, so a list `total` + cannot leak the hidden rows' existence either. +- **write** — `beforeInsert` requires READ on the record the thread names; + `beforeUpdate` / `beforeDelete` require the caller to be the comment's AUTHOR + or to hold EDIT on that record. `author_id` is server-stamped from the + session, so a client-supplied value never wins. + +Everything fails CLOSED: a `thread_id` that names no record — the dangling +`"crm_opportunity:"` above, a free-form thread, a thread on `sys_comment` +itself — is refused on write and excluded on read, and a filter that cannot be +computed denies all rather than falling open. Refusals answer **403 +`RECORD_NOT_ACCESSIBLE`** (the standard error catalog, per ADR-0112 — a generic +permission condition takes a catalogued code rather than a new synonym), with +`error.object` naming the record's object. + +**Breaking for deployments that depended on the gap.** Reads that used to +return other people's comments now return fewer rows (or none), and writes that +used to 201 now 403. Specifically: + +- Listing `sys_comment` without being able to read the parent record → the row + is gone, not merely unlabelled. Panels that render a thread must be reached by + a principal who can read the record. +- Threads whose `thread_id` is not `{object_name}:{record_id}` are no longer + usable at all: creating one is refused, and existing rows become invisible to + everyone but system context. Migrate free-form threads to a real record + reference (or keep them under a system-context surface). +- Deleting or editing another user's comment now requires EDIT on the record. + Note also that `sys_comment` delete already needed a permission set carrying + `allowDelete` — the `member_default` baseline has none (ADR-0090 D5). +- Posting a comment no longer requires the client to send `author_id` (it is + stamped); a client that sends someone else's is silently corrected rather than + believed. + +Orthogonal and unchanged: `enable.feeds` (`FEEDS_DISABLED`) still gates whether +an object has comments at all, and anonymous callers are still refused with 401 +before any of this runs. diff --git a/packages/plugins/plugin-audit/src/audit-plugin.test.ts b/packages/plugins/plugin-audit/src/audit-plugin.test.ts index 486349b167..ad6612175c 100644 --- a/packages/plugins/plugin-audit/src/audit-plugin.test.ts +++ b/packages/plugins/plugin-audit/src/audit-plugin.test.ts @@ -88,3 +88,75 @@ describe('AuditPlugin — system table provisioning', () => { await expect(fireReady()).resolves.toBeUndefined(); }); }); + +/** + * #4630 — the sys_comment record-level gates are only worth as much as their + * MOUNTING: `comment-access-hooks.test.ts` proves what the hooks decide, this + * proves the plugin actually installs them on a real kernel:ready, on the right + * object, alongside (not instead of) the audit writers. "Who mounts this" is a + * question about the composed runtime, and a gate that silently stops being + * registered fails exactly like a gate that was never written. + */ +describe('AuditPlugin — sys_comment access gates are mounted', () => { + function makeGateEngine() { + const hooks: Array<{ event: string; object?: string; packageId?: string; handler: (ctx: any) => Promise }> = []; + const middlewares: Array<{ object?: string }> = []; + const engine = { + registerHook(event: string, handler: any, options?: { object?: string; packageId?: string }) { + hooks.push({ event, handler, ...options }); + }, + registerMiddleware(_fn: any, options?: { object?: string }) { + middlewares.push({ ...options }); + }, + async find() { return [] as unknown[]; }, + async findOne() { return null; }, + async syncObjectSchema() {}, + }; + return { engine, hooks, middlewares }; + } + + it('registers the write hooks + the read middleware on sys_comment at kernel:ready', async () => { + const { engine, hooks, middlewares } = makeGateEngine(); + const { ctx, fireReady } = makeCtx(engine); + const plugin = new AuditPlugin(); + await plugin.init(ctx); + await plugin.start(ctx); + await fireReady(); + + const commentHooks = hooks.filter((h) => h.object === 'sys_comment'); + for (const event of ['beforeInsert', 'beforeUpdate', 'beforeDelete']) { + expect(commentHooks.some((h) => h.event === event)).toBe(true); + } + expect(middlewares).toContainEqual({ object: 'sys_comment' }); + // The audit writers are still installed — the gates are additive. + expect(hooks.some((h) => h.event === 'afterInsert' && !h.object)).toBe(true); + }); + + it('the mounted beforeInsert actually refuses a comment on an unreadable record', async () => { + const { engine, hooks } = makeGateEngine(); + const { ctx, fireReady } = makeCtx(engine); + const plugin = new AuditPlugin(); + await plugin.init(ctx); + await plugin.start(ctx); + await fireReady(); + + // Caller-scoped api that can read nothing — the #4630 rep2 situation. + const hookCtx = { + object: 'sys_comment', + event: 'beforeInsert', + input: { + data: { thread_id: 'crm_opportunity:1A7nlQpfEhWxIaeX', body: 'rep2 should not be here' }, + options: { context: { userId: 'rep2' } }, + }, + session: { userId: 'rep2' }, + api: { object: () => ({ findOne: async () => null }) }, + }; + const insertHooks = hooks.filter((h) => h.object === 'sys_comment' && h.event === 'beforeInsert'); + const results = await Promise.allSettled(insertHooks.map((h) => h.handler(hookCtx))); + const denials = results.filter( + (r): r is PromiseRejectedResult => r.status === 'rejected', + ); + expect(denials).toHaveLength(1); + expect(denials[0].reason).toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + }); +}); diff --git a/packages/plugins/plugin-audit/src/audit-plugin.ts b/packages/plugins/plugin-audit/src/audit-plugin.ts index e3889bee49..3e73c76b6d 100644 --- a/packages/plugins/plugin-audit/src/audit-plugin.ts +++ b/packages/plugins/plugin-audit/src/audit-plugin.ts @@ -14,6 +14,7 @@ import { SysAuditLog, SysActivity, SysComment } from './objects/index.js'; // @objectstack/service-storage for the same ownership reason (ADR-0052 §3: a // file↔record link belongs with storage, not the compliance ledger). import { installAuditWriters, type AuditI18nSurface, type MessagingEmitSurface } from './audit-writers.js'; +import { installCommentAccessHooks, installCommentReadVisibility } from './comment-access-hooks.js'; /** * AuditPlugin @@ -127,6 +128,38 @@ export class AuditPlugin implements Plugin { }; installAuditWriters(engine as any, this.name, { getMessaging, getI18n, getLocale }); ctx.logger.info('AuditPlugin: audit + activity writers installed'); + + // #4630 — record-level authorization for sys_comment: a comment's access + // derives from the record its `thread_id` names, exactly as an + // attachment's derives from its parent (service-storage's + // installAttachmentAccessHooks / installAttachmentReadVisibility). Both + // halves are needed: the hooks gate writes, the middleware is the only + // seam that filters `count()` (→ list `total`) like `find()`. Orthogonal + // to `enforceFeedsCapability` above, which gates `enable.feeds`, not + // access. The sharing service resolves lazily so plugin order doesn't + // matter; without it the edit checks degrade to parent read visibility. + if (typeof (engine as any).registerHook === 'function') { + installCommentAccessHooks( + engine as any, + () => { + try { + return ctx.getService('sharing'); + } catch { + return null; + } + }, + ctx.logger, + ); + if (typeof (engine as any).registerMiddleware === 'function') { + installCommentReadVisibility(engine as any, ctx.logger); + } else { + ctx.logger.warn( + 'AuditPlugin: engine has no middleware seam — sys_comment READ visibility NOT installed ' + + '(comments on records the caller cannot read would be listable)', + ); + } + ctx.logger.info('AuditPlugin: sys_comment record-level access gates installed'); + } }); } diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts new file mode 100644 index 0000000000..31305d2054 --- /dev/null +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts @@ -0,0 +1,303 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, vi } from 'vitest'; +import { + installCommentAccessHooks, + parseCommentThreadId, + type CommentAccessEngine, + type CommentSharingLike, +} from './comment-access-hooks.js'; + +const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); + +/** Capture the three registered hooks so tests can drive them directly. */ +function install(opts: { + comments?: Array>; + sharing?: CommentSharingLike | null; +}) { + const hooks = new Map Promise>(); + const engine: CommentAccessEngine = { + registerHook: (event, handler) => { + hooks.set(event, handler as any); + }, + find: async (_object, options: any) => { + const rows = (opts.comments ?? []).filter((r) => + Object.entries(options?.where ?? {}).every(([k, v]) => r[k] === v), + ); + return typeof options?.limit === 'number' ? rows.slice(0, options.limit) : rows; + }, + findOne: async (_object, options: any) => + (opts.comments ?? []).find((r) => + Object.entries(options?.where ?? {}).every(([k, v]) => r[k] === v), + ) ?? null, + }; + installCommentAccessHooks(engine, () => opts.sharing, silentLogger()); + return { + beforeInsert: hooks.get('beforeInsert')!, + beforeUpdate: hooks.get('beforeUpdate')!, + beforeDelete: hooks.get('beforeDelete')!, + }; +} + +/** Caller-scoped api fake: `visible` is the set of records the caller can + * read, keyed `object/id` — i.e. what the parent object's own OWD / sharing / + * RLS / object-CRUD would let through. */ +function apiFor(visible: string[]) { + return { + object: (name: string) => ({ + findOne: async ({ where }: any) => + visible.includes(`${name}/${where.id}`) ? { id: where.id } : null, + }), + }; +} + +type CallerOpts = { userId?: string; isSystem?: boolean; visible?: string[] }; + +const sessionFor = (opts: CallerOpts) => + opts.isSystem ? { isSystem: true, userId: opts.userId } : opts.userId ? { userId: opts.userId } : undefined; + +const insertCtx = (data: any, opts: CallerOpts = {}) => ({ + object: 'sys_comment', + event: 'beforeInsert', + input: { data, options: { context: { userId: opts.userId, permissions: [] } } }, + session: sessionFor(opts), + api: apiFor(opts.visible ?? []), +}); + +const writeCtx = (event: 'beforeUpdate' | 'beforeDelete', input: any, opts: CallerOpts = {}) => ({ + object: 'sys_comment', + event, + input: { ...input, options: { ...(input.options ?? {}), context: { userId: opts.userId, permissions: [] } } }, + session: sessionFor(opts), + api: apiFor(opts.visible ?? []), +}); + +describe('parseCommentThreadId', () => { + it('splits `{object}:{record_id}` on the FIRST colon', () => { + expect(parseCommentThreadId('crm_opportunity:1A7nlQpfEhWxIaeX')).toEqual({ + object: 'crm_opportunity', + recordId: '1A7nlQpfEhWxIaeX', + }); + // a record id may legally contain a colon + expect(parseCommentThreadId('sys_user:a:b')).toEqual({ object: 'sys_user', recordId: 'a:b' }); + }); + + it('rejects every thread id that names no authorizable record', () => { + expect(parseCommentThreadId('crm_opportunity:')).toBeNull(); // #4630 body: dangling empty id + expect(parseCommentThreadId(':abc')).toBeNull(); + expect(parseCommentThreadId('free-form thread')).toBeNull(); + expect(parseCommentThreadId('CrmOpportunity:r1')).toBeNull(); // not a machine name + expect(parseCommentThreadId('sys_comment:c1')).toBeNull(); // no probe re-entry + expect(parseCommentThreadId(undefined)).toBeNull(); + expect(parseCommentThreadId(42)).toBeNull(); + }); +}); + +describe('comment access — beforeInsert (parent readability + provenance)', () => { + // #4630 body, repro 2: rep2 POSTs a comment on an opportunity rep2 cannot read. + it('rejects commenting on a record the caller cannot read (403)', async () => { + const { beforeInsert } = install({}); + const ctx = insertCtx( + { thread_id: 'crm_opportunity:1A7nlQpfEhWxIaeX', body: 'rep2 should not be here' }, + { userId: 'rep2', visible: [] }, + ); + await expect(beforeInsert(ctx)).rejects.toMatchObject({ + code: 'RECORD_NOT_ACCESSIBLE', + status: 403, + object: 'crm_opportunity', + }); + }); + + // #4630 body, repro 2 verbatim: the empty-id thread that used to 201. + it('rejects a dangling thread_id with an empty record id', async () => { + const { beforeInsert } = install({}); + await expect( + beforeInsert(insertCtx({ thread_id: 'crm_opportunity:', body: 'x' }, { userId: 'rep2', visible: [] })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + }); + + it('rejects a free-form (colon-less) thread_id — unauthorizable is not unguarded', async () => { + const { beforeInsert } = install({}); + await expect( + beforeInsert(insertCtx({ thread_id: 'watercooler', body: 'x' }, { userId: 'u1', visible: [] })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + }); + + it('allows commenting on a record the caller CAN read (read is enough — no edit required)', async () => { + // A sharing service that denies every edit is present: commenting must + // still work for a user who can merely read the record. + const canEdit = vi.fn(async () => false); + const { beforeInsert } = install({ sharing: { canEdit } }); + const ctx = insertCtx( + { thread_id: 'crm_opportunity:opp1', body: 'kickoff booked' }, + { userId: 'rep1', visible: ['crm_opportunity/opp1'] }, + ); + await expect(beforeInsert(ctx)).resolves.toBeUndefined(); + expect(canEdit).not.toHaveBeenCalled(); + }); + + it('server-stamps author_id from the session, overwriting a spoofed value', async () => { + const { beforeInsert } = install({}); + const data = { thread_id: 'crm_opportunity:opp1', body: 'hi', author_id: 'someone-else' }; + await beforeInsert(insertCtx(data, { userId: 'rep1', visible: ['crm_opportunity/opp1'] })); + expect(data.author_id).toBe('rep1'); + }); + + it('bypasses for system context and context-less calls', async () => { + const { beforeInsert } = install({}); + await expect( + beforeInsert(insertCtx({ thread_id: 'x:1', body: 'b' }, { isSystem: true, userId: 'u1' })), + ).resolves.toBeUndefined(); + await expect(beforeInsert(insertCtx({ thread_id: 'x:1', body: 'b' }, {}))).resolves.toBeUndefined(); + }); + + // #3712 — a schedule-triggered flow run has provenance but no caller. + it('still bypasses a flow run that carries provenance but no session', async () => { + const { beforeInsert } = install({}); + await expect( + beforeInsert({ ...insertCtx({ thread_id: 'x:1' }, {}), provenance: { flowRunId: 'run_1' } }), + ).resolves.toBeUndefined(); + }); +}); + +describe('comment access — beforeUpdate (author or parent editor)', () => { + const row = { id: 'c1', thread_id: 'crm_opportunity:opp1', author_id: 'rep1', body: 'mine' }; + + it('the author may edit their own comment', async () => { + const canEdit = vi.fn(async () => false); + const { beforeUpdate } = install({ comments: [row], sharing: { canEdit } }); + await expect( + beforeUpdate(writeCtx('beforeUpdate', { id: 'c1', data: { body: 'edited' } }, { userId: 'rep1' })), + ).resolves.toBeUndefined(); + expect(canEdit).not.toHaveBeenCalled(); + }); + + it('a stranger without parent edit cannot rewrite someone else\'s comment', async () => { + const { beforeUpdate } = install({ comments: [row], sharing: { canEdit: async () => false } }); + await expect( + beforeUpdate(writeCtx('beforeUpdate', { id: 'c1', data: { body: 'tampered' } }, { userId: 'rep2' })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, object: 'crm_opportunity' }); + }); + + it('a parent editor may edit any comment on that record', async () => { + const { beforeUpdate } = install({ comments: [row], sharing: { canEdit: async () => true } }); + await expect( + beforeUpdate(writeCtx('beforeUpdate', { id: 'c1', data: { body: 'moderated' } }, { userId: 'manager' })), + ).resolves.toBeUndefined(); + }); + + it('re-pointing thread_id also requires READ on the NEW record', async () => { + const { beforeUpdate } = install({ comments: [row], sharing: { canEdit: async () => true } }); + // author moving their own comment onto a record they cannot read + await expect( + beforeUpdate( + writeCtx('beforeUpdate', { id: 'c1', data: { thread_id: 'crm_opportunity:secret' } }, { userId: 'rep1', visible: [] }), + ), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, object: 'crm_opportunity' }); + // ... and is allowed once that record is readable + await expect( + beforeUpdate( + writeCtx( + 'beforeUpdate', + { id: 'c1', data: { thread_id: 'crm_opportunity:opp2' } }, + { userId: 'rep1', visible: ['crm_opportunity/opp2'] }, + ), + ), + ).resolves.toBeUndefined(); + // an unchanged thread_id in the payload is not a move + await expect( + beforeUpdate( + writeCtx('beforeUpdate', { id: 'c1', data: { thread_id: row.thread_id, body: 'b' } }, { userId: 'rep1', visible: [] }), + ), + ).resolves.toBeUndefined(); + }); +}); + +describe('comment access — beforeDelete (author or parent editor)', () => { + const row = { id: 'c1', thread_id: 'crm_opportunity:opp1', author_id: 'rep1', body: 'mine' }; + + it('the author may always delete their own comment', async () => { + const canEdit = vi.fn(async () => false); + const { beforeDelete } = install({ comments: [row], sharing: { canEdit } }); + await expect(beforeDelete(writeCtx('beforeDelete', { id: 'c1' }, { userId: 'rep1' }))).resolves.toBeUndefined(); + expect(canEdit).not.toHaveBeenCalled(); + }); + + it('a non-author without parent edit is rejected (403)', async () => { + const { beforeDelete } = install({ comments: [row], sharing: { canEdit: async () => false } }); + await expect( + beforeDelete(writeCtx('beforeDelete', { id: 'c1' }, { userId: 'rep2' })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + }); + + it('a parent editor may delete another user\'s comment', async () => { + const canEdit = vi.fn(async (object: string, recordId: string, callerCtx: any) => { + expect(object).toBe('crm_opportunity'); + expect(recordId).toBe('opp1'); + expect(callerCtx.userId).toBe('manager'); + return true; + }); + const { beforeDelete } = install({ comments: [row], sharing: { canEdit } }); + await expect( + beforeDelete(writeCtx('beforeDelete', { id: 'c1' }, { userId: 'manager' })), + ).resolves.toBeUndefined(); + }); + + it('multi-delete requires EVERY matched row to pass', async () => { + const rows = [ + { ...row, id: 'c1', author_id: 'me' }, + { ...row, id: 'c2', author_id: 'someone-else' }, + ]; + const { beforeDelete } = install({ comments: rows, sharing: { canEdit: async () => false } }); + await expect( + beforeDelete( + writeCtx( + 'beforeDelete', + { options: { where: { thread_id: 'crm_opportunity:opp1' }, multi: true } }, + { userId: 'me' }, + ), + ), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE' }); + }); + + it('refuses an UNSCOPED multi-write (no id, no where) instead of reading it as "nothing to authorize"', async () => { + const { beforeDelete, beforeUpdate } = install({ comments: [row], sharing: { canEdit: async () => true } }); + await expect( + beforeDelete(writeCtx('beforeDelete', { options: { multi: true } }, { userId: 'me' })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + await expect( + beforeUpdate(writeCtx('beforeUpdate', { data: { body: 'x' }, options: { multi: true } }, { userId: 'me' })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + }); + + it('a dangling-thread comment is modifiable only by its author', async () => { + const orphan = { id: 'c9', thread_id: 'crm_opportunity:', author_id: 'rep1', body: 'orphan' }; + const { beforeDelete } = install({ comments: [orphan], sharing: { canEdit: async () => true } }); + await expect( + beforeDelete(writeCtx('beforeDelete', { id: 'c9' }, { userId: 'rep1' })), + ).resolves.toBeUndefined(); + await expect( + beforeDelete(writeCtx('beforeDelete', { id: 'c9' }, { userId: 'manager' })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE' }); + }); + + it('degrades to parent READ visibility when the sharing service is absent', async () => { + const { beforeDelete } = install({ comments: [row], sharing: null }); + await expect( + beforeDelete(writeCtx('beforeDelete', { id: 'c1' }, { userId: 'reader', visible: ['crm_opportunity/opp1'] })), + ).resolves.toBeUndefined(); + await expect( + beforeDelete(writeCtx('beforeDelete', { id: 'c1' }, { userId: 'reader', visible: [] })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE' }); + }); + + it('bypasses for system context; a no-match delete is not blocked', async () => { + const { beforeDelete } = install({ comments: [row], sharing: { canEdit: async () => false } }); + await expect( + beforeDelete(writeCtx('beforeDelete', { id: 'c1' }, { isSystem: true, userId: 'x' })), + ).resolves.toBeUndefined(); + await expect( + beforeDelete(writeCtx('beforeDelete', { id: 'missing' }, { userId: 'x' })), + ).resolves.toBeUndefined(); + }); +}); diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.ts new file mode 100644 index 0000000000..f33b21fa6f --- /dev/null +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.ts @@ -0,0 +1,513 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * sys_comment record-level authorization (#4630, ADR-0049 enforce-or-remove). + * + * `sys_comment` rows are written and read through the generic data path, and + * the default member permission sets grant wildcard CRUD with no row scoping. + * The object is public, carries no owner column, and its parent lives inside a + * *string* (`thread_id` = `{object_name}:{record_id}`), so neither OWD/sharing + * nor RLS ever narrows it: before this module any authenticated member could + * list — and post — comments on records they cannot see. `enable.feeds` is + * opt-OUT (spec default `true`), so that side-channel hung off every object of + * every app. + * + * This is the sys_attachment kit (`attachment-access-hooks.ts` in + * @objectstack/service-storage) applied to the comment thread: a comment's + * access is DERIVED FROM ITS PARENT RECORD, the same way an attachment's is + * (Salesforce ContentDocumentLink / Chatter semantics, generalizing ADR-0055 + * `controlled_by_parent` to a parent named at runtime). + * + * - {@link installCommentAccessHooks} — the write side: + * * `beforeInsert`: the caller must be able to READ the parent record — + * verified with a caller-scoped `findOne`, so the parent's own + * OWD/sharing, RLS and object-level CRUD all apply. `author_id` is + * server-stamped from the session; a client-supplied value never wins. + * * `beforeUpdate` / `beforeDelete`: the caller must be the comment's + * AUTHOR, or hold EDIT on the parent record (sharing's `canEdit`; + * public-model parents are editable by design) — the attachment kit's + * uploader-or-parent-editor rule. A multi-row write requires EVERY + * matched row to pass, and an update that re-points `thread_id` must + * additionally satisfy the insert rule on the NEW thread. + * - {@link installCommentReadVisibility} — the read side: a + * `find`/`findOne`/`count`/`aggregate` middleware that intersects the query + * with the threads whose parent record the caller can actually read. + * + * Why read gating requires EDIT on write but only READ on insert: posting a + * comment is collaboration — a user who may READ a record may discuss it (the + * attachment kit's stricter `canEdit` on insert exists because attaching a + * FILE mutates the record's content surface). Rewriting or removing someone + * else's words is moderation, hence the tighter author-or-parent-editor rule. + * + * Everything fails CLOSED. A `thread_id` that names no parseable parent (the + * dangling `"crm_opportunity:"` of #4630, a free-form thread, a thread on + * `sys_comment` itself) is REFUSED on write and EXCLUDED on read: an + * unauthorizable thread is not an unguarded one. + * + * System-context operations (engine self-writes, seeds, imports) bypass all of + * it, as do context-less programmatic calls on bare kernels (no principal to + * authorize — every real transport carries a context). + * + * These run alongside `enforceFeedsCapability` in `audit-writers.ts`, which is + * ORTHOGONAL: that gate answers "does this object allow comments at all" + * (`enable.feeds`), this one answers "may THIS caller see/touch THIS record's + * comments". Both are fail-closed 403s, so their relative order is not + * load-bearing. + */ + +/** Minimal engine surface these installers need — duck-typed (like + * service-storage's attachment seams) so tests can fake it and so plugin-audit + * keeps its dependency-free posture. */ +export interface CommentAccessEngine { + registerHook( + event: string, + 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()`. Optional: only the read-visibility installer needs it. */ + registerMiddleware?( + fn: (ctx: CommentReadMiddlewareCtx, next: () => Promise) => Promise, + options?: { object?: string }, + ): void; + find(object: string, options: Record): Promise>>; + findOne(object: string, options: Record): Promise | null>; +} + +/** Minimal shape of the engine `OperationContext` the read middleware reads. */ +export interface CommentReadMiddlewareCtx { + 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; +} + +/** Minimal surface of plugin-sharing's service this gate consults. */ +export interface CommentSharingLike { + canEdit(object: string, recordId: string, context: Record): Promise; +} + +export interface CommentAccessLogger { + info(msg: string, meta?: unknown): void; + warn(msg: string, meta?: unknown): void; + debug?(msg: string, meta?: unknown): void; +} + +/** Must match `AuditPlugin.name` — hooks are removable by owning package. */ +const PACKAGE_ID = 'com.objectstack.audit'; +const SYSTEM_CTX = { isSystem: true } as const; + +/** + * The one wire code these gates emit. Deliberately the STANDARD catalog member + * (`StandardErrorCode`, "Sharing rule restriction") rather than a new + * `COMMENT_*` extension: ADR-0112's ledger says to reach for the catalog when + * the condition is generic permission, and "you have no access to the record + * behind this thread" is exactly that. One condition, one code — the message + * names which gate refused. + */ +const DENY_CODE = 'RECORD_NOT_ACCESSIBLE'; + +/** Bound on rows authorized per multi-row update/delete; mirrors the + * attachment kit's bound. Larger multi-writes fail closed. */ +const MULTI_WRITE_AUTH_LIMIT = 1_000; + +/** Bound on the per-read candidate pre-scan. Beyond this the filter fails + * CLOSED (excludes the un-scanned rows) rather than leaking them. */ +const READ_SCAN_LIMIT = 2_000; + +const READ_OPS = new Set(['find', 'findOne', 'count', 'aggregate']); + +/** No real row matches — the fail-closed sentinel (mirrors plugin-sharing's + * `{ id: '__deny_all__' }` read-filter deny). */ +const READ_DENY_ALL = { id: '__comment_thread_denied__' } as const; + +/** Object machine-name shape (`ObjectSchema.name` in packages/spec). */ +const OBJECT_NAME_RE = /^[a-z_][a-z0-9_]*$/; + +/** The record a comment thread hangs off. */ +export interface CommentThreadTarget { + object: string; + recordId: string; +} + +/** + * Parse `thread_id` into the record it hangs off — the ONE definition both the + * write hooks and the read middleware use, so a thread can never be readable + * under one gate and writable under another. + * + * `{object_name}:{record_id}`, split on the FIRST colon (a record id may + * legally contain one). Returns `null` — meaning "no authorizable parent", + * which every caller here treats as DENY — for: + * - a non-string / colon-less / free-form thread id; + * - an empty object name or an empty record id (the dangling + * `"crm_opportunity:"` of #4630, which used to insert happily); + * - an object name that is not a machine name; + * - `sys_comment` itself: a comment is not a record-level parent (replies use + * the `parent_id` lookup), and probing it would re-enter these very gates. + */ +export function parseCommentThreadId(threadId: unknown): CommentThreadTarget | null { + if (typeof threadId !== 'string') return null; + const sep = threadId.indexOf(':'); + if (sep <= 0) return null; + const object = threadId.slice(0, sep); + const recordId = threadId.slice(sep + 1); + if (!recordId) return null; + if (!OBJECT_NAME_RE.test(object)) return null; + if (object === 'sys_comment') return null; + return { object, recordId }; +} + +function forbid(message: string, object?: string): never { + const err: any = new Error(message); + err.code = DENY_CODE; + err.status = 403; + if (object) err.object = object; + throw err; +} + +function asIdList(id: unknown): Array | null { + if (typeof id === 'string' || typeof id === 'number') return [id]; + if (id && typeof id === 'object' && Array.isArray((id as any).$in)) { + return (id as any).$in.filter((v: unknown) => typeof v === 'string' || typeof v === 'number'); + } + return null; +} + +/** The caller's ExecutionContext rides on the operation options — the session + * snapshot lacks `permissions`, which sharing bypasses need. */ +function callerContext(ctx: any): Record { + const exec = ctx?.input?.options?.context; + if (exec && typeof exec === 'object') { + return { + userId: exec.userId, + tenantId: exec.tenantId, + positions: exec.positions, + permissions: exec.permissions, + isSystem: exec.isSystem, + }; + } + const s = ctx?.session ?? {}; + return { userId: s.userId, tenantId: s.tenantId ?? s.organizationId, positions: s.positions }; +} + +/** Can the CALLER read `(object, recordId)`? A caller-scoped `findOne` through + * the hook api, so the parent's OWD/sharing, RLS and object-level CRUD all + * decide it. Any throw (unknown object, driver error) reads as "no". */ +async function callerCanRead(ctx: any, target: CommentThreadTarget): Promise { + try { + return !!(await ctx.api.object(target.object).findOne({ where: { id: target.recordId } })); + } catch { + return false; + } +} + +/** + * Install the write-side gates on `sys_comment` (insert / update / delete). + * + * `getSharing` resolves plugin-sharing's service lazily so plugin registration + * order does not matter, and returns `null` on a deployment without it — in + * which case the edit checks degrade to caller-scoped parent READ visibility, + * still strictly tighter than no gate at all. + */ +export function installCommentAccessHooks( + engine: CommentAccessEngine, + getSharing: () => CommentSharingLike | null | undefined, + logger: CommentAccessLogger, +): void { + /** May the caller EDIT the parent record behind `target`? Sharing's + * `canEdit` when the service is present, else caller-scoped parent read + * visibility (degraded mode). */ + const canEditParent = async (ctx: any, target: CommentThreadTarget, verb: string): Promise => { + const sharing = getSharing(); + if (sharing && typeof sharing.canEdit === 'function') { + return sharing.canEdit(target.object, target.recordId, callerContext(ctx)); + } + logger.debug?.( + `[audit] comment access: sharing service absent — ${verb} gated on parent read visibility`, + ); + return callerCanRead(ctx, target); + }; + + // ── Create: parent-record READ access + author_id stamping ────────── + engine.registerHook( + 'beforeInsert', + async (ctx: any) => { + if (ctx?.session?.isSystem) return; + if (!ctx?.session) return; // context-less programmatic call (bare kernel) + const data: any = ctx?.input?.data; + if (!data || typeof data !== 'object') return; + + // Server stamps provenance: the session identity wins over whatever the + // client sent. Without this the author-or-parent-editor rule below is + // spoofable — a caller could post as anyone, then "author-delete" it. + if (ctx.session.userId) data.author_id = ctx.session.userId; + + const target = parseCommentThreadId(data.thread_id); + if (!target) { + forbid( + `Cannot comment: thread_id ${JSON.stringify(data.thread_id ?? null)} does not name a record ` + + '(expected `{object_name}:{record_id}`)', + ); + } + // Commenting requires READ on the parent — a user who may see a record + // may discuss it. The parent's OWD/sharing/RLS/CRUD decide, so a + // permission set that reads nothing (a portal/guest set) cannot comment. + if (!(await callerCanRead(ctx, target))) { + forbid( + `Cannot comment on ${target.object}/${target.recordId}: the record does not exist or you cannot read it`, + target.object, + ); + } + }, + { object: 'sys_comment', packageId: PACKAGE_ID }, + ); + + /** Resolve every sys_comment row a write matches, under SYSTEM context — the + * caller may legitimately be unable to READ rows they are allowed to touch, + * and the read middleware must not narrow the authorization input. Fails + * closed on an unscoped multi-write and on an over-large match set. */ + const resolveTargetRows = async (ctx: any, verb: string): Promise>> => { + const ids = asIdList(ctx?.input?.id); + if (ids) { + const rows: Array> = []; + for (const id of ids) { + const row = await engine.findOne('sys_comment', { where: { id }, context: { ...SYSTEM_CTX } }); + if (row) rows.push(row); + } + return rows; + } + const where = ctx?.input?.options?.where; + if (where === undefined || where === null) { + // No id and no predicate: a bulk write over the WHOLE table. Nothing to + // authorize row-by-row, and "nothing to authorize" must never read as + // "allowed" (the engine would hand an unscoped AST to deleteMany). + forbid(`Refusing an unscoped multi-${verb} of comments — scope the write to the rows you mean`); + } + const rows = await engine.find('sys_comment', { + where, + limit: MULTI_WRITE_AUTH_LIMIT + 1, + context: { ...SYSTEM_CTX }, + }); + if (rows.length > MULTI_WRITE_AUTH_LIMIT) { + forbid(`Refusing to authorize a multi-${verb} matching more than ${MULTI_WRITE_AUTH_LIMIT} comments`); + } + return rows; + }; + + /** Author-or-parent-editor over every matched row (the attachment kit's + * uploader-or-parent-editor rule). Parent-edit verdicts are memoized per + * thread — a multi-row write usually targets one record. */ + const authorizeRows = async ( + ctx: any, + rows: Array>, + verb: 'update' | 'delete', + ): Promise => { + const userId = ctx.session.userId as string | undefined; + const canEditCache = new Map(); + for (const row of rows) { + if (userId && row.author_id === userId) continue; // authors govern their own words + + const threadId = row.thread_id; + const target = parseCommentThreadId(threadId); + if (!target) { + // A dangling thread has no parent to inherit authority from, and the + // caller is not the author → nobody but system may touch it. + forbid( + `Cannot ${verb} comment ${row.id}: its thread ${JSON.stringify(threadId ?? null)} names no record, ` + + 'so only its author may modify it', + ); + } + const cacheKey = String(threadId); + let allowed = canEditCache.get(cacheKey); + if (allowed === undefined) { + allowed = await canEditParent(ctx, target, verb); + canEditCache.set(cacheKey, allowed); + } + if (!allowed) { + forbid( + `Cannot ${verb} comment ${row.id}: only its author or a user who can edit the parent record ` + + `(${target.object}/${target.recordId}) may ${verb} it`, + target.object, + ); + } + } + }; + + // ── Update: author or parent editor (+ the insert rule on a re-point) ── + engine.registerHook( + 'beforeUpdate', + async (ctx: any) => { + if (ctx?.session?.isSystem) return; + if (!ctx?.session) return; + const rows = await resolveTargetRows(ctx, 'update'); + if (rows.length) await authorizeRows(ctx, rows, 'update'); + + // Moving a comment to another thread is an insert into that thread: the + // NEW parent must be readable, or an authorized edit of your own comment + // would be a way to plant content on a record you cannot see. + const nextThreadId: unknown = ctx?.input?.data?.thread_id; + if (nextThreadId === undefined) return; + if (rows.length && rows.every((r) => r.thread_id === nextThreadId)) return; // unchanged + const target = parseCommentThreadId(nextThreadId); + if (!target) { + forbid( + `Cannot move comment: thread_id ${JSON.stringify(nextThreadId ?? null)} does not name a record ` + + '(expected `{object_name}:{record_id}`)', + ); + } + if (!(await callerCanRead(ctx, target))) { + forbid( + `Cannot move comment to ${target.object}/${target.recordId}: the record does not exist or you cannot read it`, + target.object, + ); + } + }, + { object: 'sys_comment', packageId: PACKAGE_ID }, + ); + + // ── Delete: author or parent editor ───────────────────────────────── + engine.registerHook( + 'beforeDelete', + async (ctx: any) => { + if (ctx?.session?.isSystem) return; + if (!ctx?.session) return; + const rows = await resolveTargetRows(ctx, 'delete'); + if (!rows.length) return; // nothing matched — nothing to authorize + await authorizeRows(ctx, rows, 'delete'); + }, + { object: 'sys_comment', packageId: PACKAGE_ID }, + ); +} + +/** + * sys_comment READ visibility inheritance — the read half of the kit. + * + * The hooks above gate writes, but a member could still LIST `sys_comment` + * rows (body, author, thread_id) pointing at records they cannot read, which + * is the leak #4630 actually reported. `sys_comment` is public with no owner + * field, so the sharing/RLS static-predicate filters never narrow it. + * + * This is a data **middleware** (not a find-hook) on purpose: middleware runs + * for `find`, `findOne`, `count`, AND `aggregate`, so a list's `total` (which + * comes from `engine.count()`, NOT the find path) is filtered identically to + * the returned rows — a beforeFind/afterFind hook would leave `count()` + * unfiltered and leak the true row count via `total`. + * + * Mechanism: for each read, pre-scan the candidate `thread_id`s the query + * would touch (system context), resolve which of their parent records the + * caller can actually read through the caller-scoped engine (the parent's own + * RLS/OWD/sharing apply), and AND `{ thread_id: { $in: } }` + * into `ctx.ast.where`. Threads whose parent is invisible — and threads that + * name no parent at all — simply never enter that list. + */ +export function installCommentReadVisibility( + engine: CommentAccessEngine, + logger: CommentAccessLogger, +): void { + if (typeof engine.registerMiddleware !== 'function') return; // engine lacks the seam + const andIn = (ctx: CommentReadMiddlewareCtx, filter: unknown) => { + if (!ctx.ast) return; + ctx.ast.where = ctx.ast.where ? { $and: [ctx.ast.where, filter] } : filter; + }; + + engine.registerMiddleware( + async (ctx, next) => { + // Only reads carry an `ast` to constrain; writes are gated by the hooks + // above. System / context-less (internal) reads are not narrowed. + if (!READ_OPS.has(ctx.operation) || !ctx.ast || !ctx.context || ctx.context.isSystem) { + return next(); + } + try { + const filter = await computeThreadVisibilityFilter(engine, ctx, logger); + if (filter) andIn(ctx, filter); + } catch (err) { + // A filter-compute failure must never fall open into a leak. + logger.warn( + `[audit] comment read visibility: filter failed, denying all (${(err as Error)?.message ?? err})`, + ); + andIn(ctx, READ_DENY_ALL); + } + return next(); + }, + { object: 'sys_comment' }, + ); +} + +/** + * Resolve the thread-visibility WHERE predicate for one sys_comment read. + * Returns `null` when the query matches no rows (nothing to narrow), a + * `thread_id` `$in` of the visible threads, or the deny-all sentinel. + */ +async function computeThreadVisibilityFilter( + engine: CommentAccessEngine, + ctx: CommentReadMiddlewareCtx, + logger: CommentAccessLogger, +): Promise { + // 1. Candidate thread ids the query would touch — read under SYSTEM context + // (the caller may not see the rows yet; that is exactly what we are + // deciding). Bypasses this middleware (isSystem). + const candidates = await engine.find('sys_comment', { + where: (ctx.ast?.where as Record) ?? {}, + fields: ['thread_id'], + limit: READ_SCAN_LIMIT, + context: { ...SYSTEM_CTX }, + }); + if (!candidates.length) return null; + if (candidates.length >= READ_SCAN_LIMIT) { + // Not silent (fail-closed truncation): rows beyond the scan window are + // excluded from the visibility filter, so a very broad unscoped list may + // omit rows the caller could see. The comment panel scopes to one thread + // so never hits this; a global list should paginate by thread_id. + logger.warn( + `[audit] comment read visibility: candidate pre-scan hit the ${READ_SCAN_LIMIT}-row cap; ` + + 'the visibility filter for this broad read is fail-closed and may omit visible rows — scope the query by thread_id', + ); + } + + /** thread_id (verbatim, as stored) → its parent. Unparseable threads are + * dropped here, which is what makes them invisible. */ + const threads = new Map(); + const byObject = new Map>(); + for (const row of candidates) { + const threadId = row.thread_id; + if (typeof threadId !== 'string' || threads.has(threadId)) continue; + const target = parseCommentThreadId(threadId); + if (!target) continue; + threads.set(threadId, target); + let ids = byObject.get(target.object); + if (!ids) byObject.set(target.object, (ids = new Set())); + ids.add(target.recordId); + } + if (threads.size === 0) return READ_DENY_ALL; + + // 2. Per parent object, the visible id subset via the CALLER's context — + // the parent object's own RLS/OWD/sharing applies. + const visibleByObject = new Map>(); + for (const [parentObject, idSet] of byObject) { + const ids = [...idSet]; + let visible: string[] = []; + try { + const rows = await engine.find(parentObject, { + where: { id: { $in: ids } }, + fields: ['id'], + limit: ids.length, + context: { ...ctx.context }, + }); + visible = rows.map((r) => String(r.id)).filter(Boolean); + } catch { + // Unknown/failing parent object → none visible (fail closed). + visible = []; + } + visibleByObject.set(parentObject, new Set(visible)); + } + + // 3. Keep the thread ids VERBATIM (never re-assembled from the parts) so the + // emitted filter can only ever match rows the pre-scan actually saw. + const visibleThreads: string[] = []; + for (const [threadId, target] of threads) { + if (visibleByObject.get(target.object)?.has(target.recordId)) visibleThreads.push(threadId); + } + if (visibleThreads.length === 0) return READ_DENY_ALL; + return { thread_id: { $in: visibleThreads } }; +} diff --git a/packages/plugins/plugin-audit/src/comment-read-visibility.test.ts b/packages/plugins/plugin-audit/src/comment-read-visibility.test.ts new file mode 100644 index 0000000000..0e8b298594 --- /dev/null +++ b/packages/plugins/plugin-audit/src/comment-read-visibility.test.ts @@ -0,0 +1,323 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, vi } from 'vitest'; +import { + installCommentReadVisibility, + type CommentAccessEngine, + type CommentReadMiddlewareCtx, +} from './comment-access-hooks.js'; + +const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); + +/** + * Filter Protocol evaluator for the fake engine below — real `$and` / `$or` / + * `$not` semantics, not a shape check. Same discipline (and the same + * self-test at the bottom) as service-storage's + * `attachment-read-visibility.test.ts`: a fake that silently under-implements + * the protocol cannot see a WIDENING bug, so this one implements what it needs + * and **throws** on anything it does not, rather than quietly matching. + * + * Values compare as strings on purpose: the middleware emits thread ids + * verbatim, and a row's value has to match the filter's value exactly. + */ +function matchWhere(row: any, where: any): boolean { + if (where === null || where === undefined) return true; + if (typeof where !== 'object' || Array.isArray(where)) { + throw new Error(`harness matcher: a filter must be an object, got ${JSON.stringify(where)}`); + } + return Object.entries(where).every(([key, val]) => { + if (key === '$and') { + if (!Array.isArray(val)) throw new Error('harness matcher: $and expects an array'); + return val.every((branch) => matchWhere(row, branch)); + } + if (key === '$or') { + if (!Array.isArray(val)) throw new Error('harness matcher: $or expects an array'); + return val.some((branch) => matchWhere(row, branch)); + } + if (key === '$not') return !matchWhere(row, val); + if (key.startsWith('$')) throw new Error(`harness matcher: unsupported logical operator ${key}`); + return matchField(row[key], val); + }); +} + +/** One `field: ` entry. Multiple operators on the same field AND together. */ +function matchField(actual: any, spec: any): boolean { + if (spec === null || spec === undefined) return actual === null || actual === undefined; + if (Array.isArray(spec)) { + throw new Error('harness matcher: a bare array is not a field spec — use { $in: [...] }'); + } + if (typeof spec !== 'object') return String(actual) === String(spec); + return Object.entries(spec).every(([op, target]) => { + switch (op) { + case '$eq': + return String(actual) === String(target); + case '$ne': + return String(actual) !== String(target); + case '$in': + return (target as unknown[]).map(String).includes(String(actual)); + case '$nin': + return !(target as unknown[]).map(String).includes(String(actual)); + default: + throw new Error(`harness matcher: unsupported operator ${op} — implement it, don't let it match silently`); + } + }); +} + +/** + * Fake engine modelling: a sys_comment table (system pre-scan) + a per-object + * visibility map keyed by userId. A parent find under a caller context returns + * only the ids that user can see — i.e. what the parent's OWD / sharing / RLS + * / object-level CRUD would allow. + */ +function install(opts: { + comments: Array<{ id?: string; thread_id: string } & Record>; + /** parentObject -> userId -> visible record ids */ + visible: Record>; +}) { + let mw!: (ctx: CommentReadMiddlewareCtx, next: () => Promise) => Promise; + const calls = { parentFinds: [] as Array<{ object: string; ids: string[]; userId?: string }> }; + + const engine: CommentAccessEngine = { + registerHook: () => {}, + registerMiddleware: (fn) => { + mw = fn as any; + }, + find: async (object: string, options: any) => { + if (object === 'sys_comment') { + const rows = opts.comments.filter((r) => matchWhere(r, options?.where)); + return rows.slice(0, options?.limit ?? rows.length) as any; + } + 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, + }; + installCommentReadVisibility(engine, silentLogger()); + return { + mw, + calls, + /** Ids the given `where` actually selects from the fixture — i.e. what a + * spec-conformant driver would return once the middleware has narrowed the + * query. Asserting on this (not just the filter's shape) is what makes a + * widened scope visible. */ + selectIds: (where: unknown): string[] => + opts.comments.filter((r) => matchWhere(r, where)).map((r) => String(r.id)), + }; +} + +/** Drive a read op through the middleware and return the resulting where. */ +async function runRead( + mw: (ctx: CommentReadMiddlewareCtx, next: () => Promise) => Promise, + ctxPartial: Partial, +) { + const ctx: CommentReadMiddlewareCtx = { + object: 'sys_comment', + operation: 'find', + ast: { object: 'sys_comment', where: undefined }, + context: { userId: 'rep2' }, + ...ctxPartial, + }; + let ran = false; + await mw(ctx, async () => { + ran = true; + }); + return { where: ctx.ast?.where, ran }; +} + +describe('installCommentReadVisibility', () => { + // The #4630 repro, as data: one comment on an opportunity rep2 cannot read, + // one on a case rep2 can. + const dataset = { + comments: [ + { id: 'k1', thread_id: 'crm_opportunity:1A7nlQpfEhWxIaeX', body: 'Dogfood #602: kickoff call booked' }, + { id: 'k2', thread_id: 'crm_case:c1', body: 'visible' }, + ], + visible: { + crm_opportunity: { rep1: ['1A7nlQpfEhWxIaeX'], rep2: [], guest_portal: [] }, + crm_case: { rep1: ['c1'], rep2: ['c1'], guest_portal: [] }, + }, + }; + + // #4630 body, repro 1: the leaked list must come back empty for rep2. + it('excludes threads whose record the caller cannot read', async () => { + const { mw, selectIds } = install(dataset); + const { where, ran } = await runRead(mw, { context: { userId: 'rep2' } }); + expect(ran).toBe(true); + expect(where).toEqual({ thread_id: { $in: ['crm_case:c1'] } }); + expect(selectIds(where)).toEqual(['k2']); // the opportunity comment is gone + }); + + it('keeps every thread a reader of both records can see', async () => { + const { mw, selectIds } = install(dataset); + const { where } = await runRead(mw, { context: { userId: 'rep1' } }); + expect(where).toEqual({ thread_id: { $in: ['crm_opportunity:1A7nlQpfEhWxIaeX', 'crm_case:c1'] } }); + expect(selectIds(where)).toEqual(['k1', 'k2']); + }); + + // #4630 body, repro 3: an INSERT-only permission set that "reads nothing". + it('denies all for a principal who can read no record at all (guest_portal)', async () => { + const { mw, selectIds } = install(dataset); + const { where } = await runRead(mw, { context: { userId: 'guest_portal' } }); + expect(where).toEqual({ id: '__comment_thread_denied__' }); + expect(selectIds(where)).toEqual([]); + }); + + it('scoping the query to one unreadable thread still returns nothing', async () => { + const { mw, selectIds } = install(dataset); + const scoped = { thread_id: 'crm_opportunity:1A7nlQpfEhWxIaeX' }; + const { where } = await runRead(mw, { + context: { userId: 'rep2' }, + ast: { object: 'sys_comment', where: scoped }, + }); + expect(where).toEqual({ $and: [scoped, { id: '__comment_thread_denied__' }] }); + expect(selectIds(where)).toEqual([]); + }); + + it('ANDs the visibility filter onto an existing where (does not clobber it)', async () => { + const { mw, selectIds } = install(dataset); + const existing = { thread_id: 'crm_case:c1' }; + const { where } = await runRead(mw, { + context: { userId: 'rep2' }, + ast: { object: 'sys_comment', where: existing }, + }); + expect(where).toEqual({ $and: [existing, { thread_id: { $in: ['crm_case:c1'] } }] }); + expect(selectIds(where)).toEqual(['k2']); + }); + + it('filters count() identically (list total cannot leak the unfiltered count)', async () => { + const { mw } = install(dataset); + const { where } = await runRead(mw, { operation: 'count', context: { userId: 'rep2' } }); + expect(where).toEqual({ thread_id: { $in: ['crm_case: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(); + expect(calls.parentFinds).toHaveLength(0); + }); + + it('does not touch write operations (the hooks gate those)', async () => { + const { mw } = install(dataset); + const { where } = await runRead(mw, { + operation: 'delete', + ast: { object: 'sys_comment', where: { id: 'k1' } }, + }); + expect(where).toEqual({ id: 'k1' }); // unchanged + }); + + it('leaves an already-empty result set alone (no candidates → no filter)', async () => { + const { mw } = install({ comments: [], visible: {} }); + const { where } = await runRead(mw, {}); + expect(where).toBeUndefined(); + }); + + it('hides threads that name no record — dangling, free-form, or self-referential', async () => { + const { mw, calls } = install({ + comments: [ + { id: 'd1', thread_id: 'crm_opportunity:' }, // #4630's dangling id + { id: 'd2', thread_id: 'watercooler' }, // free-form + { id: 'd3', thread_id: 'sys_comment:c1' }, // would re-enter this middleware + ], + visible: {}, + }); + const { where } = await runRead(mw, {}); + expect(where).toEqual({ id: '__comment_thread_denied__' }); + // no probe was issued for any of them — in particular none against sys_comment + expect(calls.parentFinds).toHaveLength(0); + }); + + it('probes each parent object once with the deduped id set', async () => { + const { mw, calls } = install({ + comments: [ + { id: 'x1', thread_id: 'crm_case:c1' }, + { id: 'x2', thread_id: 'crm_case:c1' }, // same thread twice + { id: 'x3', thread_id: 'crm_case:c2' }, + ], + visible: { crm_case: { rep2: ['c1'] } }, + }); + const { where } = await runRead(mw, {}); + expect(calls.parentFinds).toEqual([{ object: 'crm_case', ids: ['c1', 'c2'], userId: 'rep2' }]); + expect(where).toEqual({ thread_id: { $in: ['crm_case:c1'] } }); + }); + + it('fails CLOSED when the parent probe throws (deny-all, never a fall-open)', async () => { + let mw!: (ctx: CommentReadMiddlewareCtx, next: () => Promise) => Promise; + const engine: CommentAccessEngine = { + registerHook: () => {}, + registerMiddleware: (fn) => { + mw = fn as any; + }, + find: async (object: string) => { + if (object === 'sys_comment') return [{ id: 'k1', thread_id: 'crm_case:c1' }] as any; + throw new Error('sharing service exploded'); + }, + findOne: async () => null, + }; + installCommentReadVisibility(engine, silentLogger()); + const { where } = await runRead(mw, {}); + expect(where).toEqual({ id: '__comment_thread_denied__' }); + }); + + it('fails CLOSED (and warns) when the whole filter computation throws', async () => { + const logger = silentLogger(); + let mw!: (ctx: CommentReadMiddlewareCtx, next: () => Promise) => Promise; + const engine: CommentAccessEngine = { + registerHook: () => {}, + registerMiddleware: (fn) => { + mw = fn as any; + }, + find: async () => { + throw new Error('pre-scan exploded'); + }, + findOne: async () => null, + }; + installCommentReadVisibility(engine, logger); + const { where, ran } = await runRead(mw, {}); + expect(where).toEqual({ id: '__comment_thread_denied__' }); + expect(ran).toBe(true); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('denying all')); + }); + + it('is inert on an engine without the middleware seam (no throw)', async () => { + const engine: CommentAccessEngine = { + registerHook: () => {}, + find: async () => [], + findOne: async () => null, + }; + expect(() => installCommentReadVisibility(engine, silentLogger())).not.toThrow(); + }); +}); + +/** + * Who tests the test double? The row assertions above are only worth as much + * as the matcher evaluating them, so it gets the same self-test as its + * service-storage twin: a correctly scoped filter and a widened one must + * select DIFFERENT rows, or every assertion in this file is decorative. + */ +describe('harness matcher — Filter Protocol semantics', () => { + const comments = [ + { id: 'k1', thread_id: 'crm_opportunity:o1' }, + { id: 'k2', thread_id: 'crm_case:c1' }, + ]; + const pick = (f: any) => comments.filter((r) => matchWhere(r, f)).map((r) => r.id); + + it('tells a correctly scoped $in apart from an absent one', () => { + expect(pick({ thread_id: { $in: ['crm_case:c1'] } })).toEqual(['k2']); + expect(pick({})).toEqual(['k1', 'k2']); + }); + + it('ANDs an outer where with the injected filter', () => { + expect(pick({ $and: [{ thread_id: 'crm_opportunity:o1' }, { thread_id: { $in: ['crm_case:c1'] } }] })).toEqual([]); + }); + + it('throws instead of silently ignoring an operator it does not implement', () => { + expect(() => pick({ thread_id: { $gt: 'x' } })).toThrow(/unsupported operator \$gt/); + expect(() => pick({ $nor: [{ thread_id: 'x' }] })).toThrow(/unsupported logical operator \$nor/); + }); +}); diff --git a/packages/plugins/plugin-audit/src/index.ts b/packages/plugins/plugin-audit/src/index.ts index 2cab3f1b73..83286f394d 100644 --- a/packages/plugins/plugin-audit/src/index.ts +++ b/packages/plugins/plugin-audit/src/index.ts @@ -9,3 +9,15 @@ export { AuditPlugin } from './audit-plugin.js'; export { installAuditWriters } from './audit-writers.js'; +export { + installCommentAccessHooks, + installCommentReadVisibility, + parseCommentThreadId, +} from './comment-access-hooks.js'; +export type { + CommentAccessEngine, + CommentAccessLogger, + CommentReadMiddlewareCtx, + CommentSharingLike, + CommentThreadTarget, +} from './comment-access-hooks.js'; diff --git a/packages/qa/dogfood/test/comments-permission-matrix.dogfood.test.ts b/packages/qa/dogfood/test/comments-permission-matrix.dogfood.test.ts new file mode 100644 index 0000000000..983265d779 --- /dev/null +++ b/packages/qa/dogfood/test/comments-permission-matrix.dogfood.test.ts @@ -0,0 +1,222 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// sys_comment record-level authorization (#4630) — the three repros from the +// issue, flipped, driven end-to-end through the REAL surfaces: better-auth +// sign-up members, the platform permission sets, the sharing service and the +// generic `/data` path. +// +// What the issue reported, on the SAME record with the SAME user: +// GET /data/cmt_private?$filter=… → 200, 0 rows (correct) +// GET /data/sys_attachment?parent_id=… → 200, 0 rows (correct) +// GET /data/sys_comment?thread_id=… → 200, 1 row (the leak) +// POST /data/sys_comment {thread_id:'cmt_private:'} → 201 (ungated write) +// +// Matrix legend: +// (a) list/read a thread whose record the caller cannot read +// (b) post to a thread whose record the caller cannot read +// (c) malformed / dangling thread_id +// (d) read is enough to comment; edit is what moderation needs +// (e) author_id provenance stamping +// (f) enable.feeds stays orthogonal, anonymous stays 401 + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { AuditPlugin } from '@objectstack/plugin-audit'; +import { commentsFixtureStack, commentsFixtureSecurity } from './fixtures/comments-fixture.js'; + +const SYS = { isSystem: true } as const; + +/** Extract the created record id from a REST create response ({id} | {record:{id}}). */ +async function createdId(res: Response): Promise { + const j = (await res.json()) as any; + const id = j.id ?? j.record?.id ?? j.data?.id; + if (!id) throw new Error(`create response carried no id: ${JSON.stringify(j)}`); + return String(id); +} + +const rowsOf = async (res: Response): Promise => ((await res.json()) as any).records ?? []; + +describe('sys_comment permission matrix (#4630)', () => { + let stack: VerifyStack; + let ql: any; + let adminTok: string, memberATok: string, memberBTok: string; + let adminId: string, memberAId: string, memberBId: string; + let openId: string; // cmt_open record — everyone reads AND edits + let privateId: string; // cmt_private record owned by admin — only admin reads + let readonlyId: string; // cmt_readonly record owned by admin — everyone reads, admin edits + + const uid = async (email: string) => + (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; + + const comment = (token: string, threadId: string, body: string, extra: any = {}) => + stack.apiAs(token, 'POST', '/data/sys_comment', { thread_id: threadId, body, ...extra }); + + beforeAll(async () => { + stack = await bootStack(commentsFixtureStack as never, { + security: commentsFixtureSecurity(), + // AuditPlugin owns sys_comment: the #2707 enable.feeds gate AND the + // #4630 record-level gates both ride on it. + extraPlugins: [new AuditPlugin()], + }); + adminTok = await stack.signIn(); + memberATok = await stack.signUp('cmt-member-a@verify.test'); + memberBTok = await stack.signUp('cmt-member-b@verify.test'); + ql = await stack.kernel.getServiceAsync('objectql'); + adminId = await uid('admin@objectos.ai'); + memberAId = await uid('cmt-member-a@verify.test'); + memberBId = await uid('cmt-member-b@verify.test'); + + // Domain grant (see commentManagerSet): both members may DELETE comments; + // the everyone-anchor baseline carries no delete bit. + const managerSet = await ql.findOne('sys_permission_set', { where: { name: 'cmt_comment_manager' }, context: SYS }); + expect(managerSet?.id, 'fixture permission set seeded').toBeTruthy(); + for (const userId of [memberAId, memberBId]) { + await ql.insert('sys_user_permission_set', { user_id: userId, permission_set_id: managerSet.id }, { context: { ...SYS } }); + } + + const openRes = await stack.apiAs(memberATok, 'POST', '/data/cmt_open', { name: 'open record' }); + expect(openRes.status).toBeLessThan(300); + openId = await createdId(openRes); + + privateId = (await ql.insert('cmt_private', { name: 'admin only', owner_id: adminId }, { context: { ...SYS } })).id; + readonlyId = (await ql.insert('cmt_readonly', { name: 'read all', owner_id: adminId }, { context: { ...SYS } })).id; + }, 120_000); + + afterAll(async () => { + await stack?.stop(); + }); + + // ── (a) the reported leak: LIST / READ ─────────────────────────────── + it('(a) a member CANNOT list or read comments whose record they cannot read', async () => { + // Admin — who owns the private record — posts on its thread. + const posted = await comment(adminTok, `cmt_private:${privateId}`, 'kickoff call booked with Apex procurement'); + expect(posted.status, await posted.text()).toBeLessThan(300); + const row = await ql.findOne('sys_comment', { where: { thread_id: `cmt_private:${privateId}` }, context: SYS }); + expect(row?.id).toBeTruthy(); + + // memberB cannot read the PARENT record — the control the issue points at. + const parentRead = await stack.apiAs(memberBTok, 'GET', `/data/cmt_private/${privateId}`); + expect([403, 404]).toContain(parentRead.status); + + // …so the thread is not listable either (unscoped list), + const list = await stack.apiAs(memberBTok, 'GET', '/data/sys_comment'); + expect(list.status).toBe(200); + expect((await rowsOf(list)).some((r: any) => r.id === row.id), 'comment on an invisible record must not be listable').toBe(false); + + // …nor via the exact query from the issue (scoped by thread_id), + const scoped = await stack.apiAs( + memberBTok, + 'GET', + `/data/sys_comment?$filter=${encodeURIComponent(JSON.stringify(['thread_id', '=', `cmt_private:${privateId}`]))}`, + ); + expect(scoped.status).toBe(200); + expect(await rowsOf(scoped)).toHaveLength(0); + + // …nor by id. + const byId = await stack.apiAs(memberBTok, 'GET', `/data/sys_comment/${row.id}`); + expect([403, 404]).toContain(byId.status); + + // Control: the owner still sees their own thread. + const ownerList = await stack.apiAs(adminTok, 'GET', '/data/sys_comment'); + expect((await rowsOf(ownerList)).some((r: any) => r.id === row.id)).toBe(true); + }); + + it('(a) the list total does not leak the hidden rows either', async () => { + const list = await stack.apiAs(memberBTok, 'GET', '/data/sys_comment'); + const body = (await list.json()) as any; + const rows = body.records ?? []; + if (typeof body.total === 'number') expect(body.total).toBe(rows.length); + }); + + // ── (b) the reported ungated write ─────────────────────────────────── + it('(b) a member CANNOT post to a thread whose record they cannot read → 403', async () => { + const denied = await comment(memberBTok, `cmt_private:${privateId}`, 'memberB should not be here'); + expect(denied.status).toBe(403); + expect(((await denied.json()) as any).code).toBe('RECORD_NOT_ACCESSIBLE'); + // and nothing was written + const rows = await ql.find('sys_comment', { where: { body: 'memberB should not be here' }, context: SYS }); + expect(rows).toHaveLength(0); + }); + + // ── (c) the issue's literal payload: an empty record id ────────────── + it('(c) a dangling or free-form thread_id is refused (the issue POSTed `cmt_private:` and got 201)', async () => { + for (const threadId of [`cmt_private:`, 'watercooler', ':orphan', 'sys_comment:whatever']) { + const res = await comment(memberATok, threadId, `dangling ${threadId}`); + expect(res.status, `thread_id ${JSON.stringify(threadId)}`).toBe(403); + expect(((await res.json()) as any).code).toBe('RECORD_NOT_ACCESSIBLE'); + } + const written = await ql.find('sys_comment', { where: { thread_id: 'watercooler' }, context: SYS }); + expect(written).toHaveLength(0); + }); + + // ── (d) read is enough to comment; edit is what moderation needs ───── + it('(d) a member who can only READ the record may still comment on it', async () => { + // cmt_readonly is public_read: memberA reads it, only admin (owner) edits. + const canRead = await stack.apiAs(memberATok, 'GET', `/data/cmt_readonly/${readonlyId}`); + expect(canRead.status).toBe(200); + const posted = await comment(memberATok, `cmt_readonly:${readonlyId}`, 'commenting without edit rights'); + expect(posted.status, await posted.clone().text()).toBeLessThan(300); + // …and memberB, who can also read that record, sees it. + const list = await stack.apiAs(memberBTok, 'GET', '/data/sys_comment'); + expect((await rowsOf(list)).some((r: any) => r.body === 'commenting without edit rights')).toBe(true); + }); + + it('(d) the author may delete their own comment; a stranger without parent EDIT may not', async () => { + const posted = await comment(memberATok, `cmt_readonly:${readonlyId}`, 'memberA owns these words'); + expect(posted.status).toBeLessThan(300); + const row = await ql.findOne('sys_comment', { where: { body: 'memberA owns these words' }, context: SYS }); + + // memberB holds the delete bit and can READ the record, but is neither the + // author nor able to EDIT the (admin-owned, public_read) parent → 403. + // Which layer fires first (RBAC/RLS pre-image vs this gate) decides the + // code; both are the fail-closed contract, as in the attachments matrix. + const denied = await stack.apiAs(memberBTok, 'DELETE', `/data/sys_comment/${row.id}`); + expect(denied.status).toBe(403); + expect(['RECORD_NOT_ACCESSIBLE', 'PERMISSION_DENIED']).toContain(((await denied.json()) as any).code); + expect(await ql.findOne('sys_comment', { where: { id: row.id }, context: SYS })).toBeTruthy(); + + // The author may. + const allowed = await stack.apiAs(memberATok, 'DELETE', `/data/sys_comment/${row.id}`); + expect(allowed.status).toBeLessThan(300); + expect(await ql.findOne('sys_comment', { where: { id: row.id }, context: SYS })).toBeFalsy(); + }); + + it('(d) a user who can EDIT the record may moderate anyone\'s comment on it', async () => { + // cmt_open is public_read_write → every member canEdit it. + const posted = await comment(memberATok, `cmt_open:${openId}`, 'memberA on the open record'); + expect(posted.status).toBeLessThan(300); + const row = await ql.findOne('sys_comment', { where: { body: 'memberA on the open record' }, context: SYS }); + const moderated = await stack.apiAs(memberBTok, 'DELETE', `/data/sys_comment/${row.id}`); + expect(moderated.status, await moderated.clone().text()).toBeLessThan(300); + }); + + // ── (e) provenance ─────────────────────────────────────────────────── + it('(e) author_id is server-stamped from the session, overwriting a spoofed value', async () => { + const posted = await comment(memberATok, `cmt_open:${openId}`, 'who wrote this?', { author_id: adminId }); + expect(posted.status).toBeLessThan(300); + const row = await ql.findOne('sys_comment', { where: { body: 'who wrote this?' }, context: SYS }); + expect(row?.author_id, 'server identity wins over the spoofed author_id').toBe(memberAId); + }); + + // ── (f) the neighbours are unchanged ───────────────────────────────── + it('(f) enable.feeds:false still answers FEEDS_DISABLED — the capability gate is orthogonal', async () => { + const rec = await stack.apiAs(memberATok, 'POST', '/data/cmt_nofeeds', { name: 'no feeds' }); + expect(rec.status).toBeLessThan(300); + const recId = await createdId(rec); + // The caller CAN read and edit this record — only the capability gate refuses. + const res = await comment(memberATok, `cmt_nofeeds:${recId}`, 'should be refused by the feeds gate'); + expect(res.status).toBe(403); + expect(((await res.json()) as any).code).toBe('FEEDS_DISABLED'); + }); + + it('(f) anonymous is still refused before any of this (401)', async () => { + const read = await stack.api('/data/sys_comment'); + expect(read.status).toBe(401); + const write = await stack.api('/data/sys_comment', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ thread_id: `cmt_open:${openId}`, body: 'anon' }), + }); + expect(write.status).toBe(401); + }); +}); diff --git a/packages/qa/dogfood/test/fixtures/comments-fixture.ts b/packages/qa/dogfood/test/fixtures/comments-fixture.ts new file mode 100644 index 0000000000..7527d07aae --- /dev/null +++ b/packages/qa/dogfood/test/fixtures/comments-fixture.ts @@ -0,0 +1,110 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Comments permission-matrix fixture (#4630). +// +// Four tiny objects spanning the enforcement axes of the `sys_comment` +// collaboration surface. `enable.feeds` is opt-OUT (spec default `true`), so +// every one of them has comments on unless it says otherwise — which is +// exactly why record-level authorization has to come from the PARENT record: +// +// cmt_open — public_read_write: any member reads AND edits it, so any +// member may comment on it and moderate its thread. +// cmt_private — DEFAULT sharing model (omitted ⇒ private, ADR-0090) + an +// `owner_id` field: the sharing service read-filters rows to +// the owner, so a non-owner member can neither read the +// record nor (now) see or post comments on it. This is the +// `crm_opportunity` of the #4630 report. +// cmt_readonly — public_read: every member READS it, only the owner EDITS. +// The case that separates "read is enough to comment" from +// "edit is required to moderate someone else's comment". +// cmt_nofeeds — enable.feeds:false: plugin-audit's FEEDS_DISABLED +// capability gate, which is ORTHOGONAL to this authorization +// and must keep behaving exactly as before. +// +// No custom SecurityPlugin beyond the platform defaults: a fresh signUp member +// falls back to the real `member_default` wildcard-CRUD set — exactly the +// posture #4630 reported against (the gates under test are the ones layered on +// TOP of that wildcard, which by itself scopes nothing). + +import { defineStack } from '@objectstack/spec'; +import { ObjectSchema, Field } from '@objectstack/spec/data'; +import { PermissionSetSchema, type PermissionSet } from '@objectstack/spec/security'; +import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; + +export const CmtOpen = ObjectSchema.create({ + name: 'cmt_open', + label: 'Comment Open Record', + pluralLabel: 'Comment Open Records', + sharingModel: 'public_read_write', + fields: { + name: Field.text({ label: 'Name', required: true }), + }, +}); + +export const CmtPrivate = ObjectSchema.create({ + name: 'cmt_private', + label: 'Comment Private Record', + pluralLabel: 'Comment Private Records', + // sharingModel omitted — a custom object defaults to PRIVATE (ADR-0090); + // owner_id is the sharing service's owner anchor. + fields: { + name: Field.text({ label: 'Name', required: true }), + owner_id: Field.text({ label: 'Owner' }), + }, +}); + +export const CmtReadonly = ObjectSchema.create({ + name: 'cmt_readonly', + label: 'Comment Readonly Record', + pluralLabel: 'Comment Readonly Records', + sharingModel: 'public_read', + fields: { + name: Field.text({ label: 'Name', required: true }), + owner_id: Field.text({ label: 'Owner' }), + }, +}); + +export const CmtNoFeeds = ObjectSchema.create({ + name: 'cmt_nofeeds', + label: 'Comment-Free Record', + pluralLabel: 'Comment-Free Records', + sharingModel: 'public_read_write', + enable: { feeds: false }, + fields: { + name: Field.text({ label: 'Name', required: true }), + }, +}); + +/** + * The domain grant a real app ships when it lets members manage a thread: + * `member_default` (the `everyone` anchor baseline) carries NO `allowDelete` + * (ADR-0090 D5 — delete is not a baseline right), so deleting a comment needs + * an ordinary position-distributed set with the delete bit on `sys_comment`. + */ +export const commentManagerSet: PermissionSet = PermissionSetSchema.parse({ + name: 'cmt_comment_manager', + label: 'Comments Fixture — comment manager', + objects: { + sys_comment: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, +}); + +/** SecurityPlugin carrying the platform defaults + the fixture's domain set. */ +export function commentsFixtureSecurity(): SecurityPlugin { + return new SecurityPlugin({ + defaultPermissionSets: [...securityDefaultPermissionSets, commentManagerSet], + }); +} + +export const commentsFixtureStack = defineStack({ + manifest: { + id: 'com.dogfood.comments_fixture', + namespace: 'cmt', + version: '0.0.0', + type: 'app', + name: 'Comments Permission Matrix Fixture', + description: + 'Four-object app exercising the #4630 comment permission matrix: thread visibility, author/parent-editor writes, the enable.feeds gate.', + }, + objects: [CmtOpen, CmtPrivate, CmtReadonly, CmtNoFeeds], +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index c79c1e7fa1..2988f2dd62 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -289,6 +289,24 @@ export function mapDataError(error: any, object?: string): { status: number; bod }, }; } + // Comment access gates (#4630): plugin-audit's engine hooks reject + // sys_comment writes fail-closed when the caller cannot read the record + // behind `thread_id` (create) or is neither the author nor a parent editor + // (update/delete). Uses the STANDARD catalog code rather than a bespoke + // one (ADR-0112: generic permission conditions take the catalog), and is + // matched here — ahead of the generic 4xx passthrough — for the same + // reason as the attachment gates: `error.object` names the record's object + // (not the join/comment table) and the passthrough would drop it. + if (error?.code === 'RECORD_NOT_ACCESSIBLE') { + return { + status: 403, + body: { + error: error?.message ?? 'Record access denied', + code: 'RECORD_NOT_ACCESSIBLE', + ...(error?.object || object ? { object: error?.object ?? object } : {}), + }, + }; + } // Short-circuit: explicit security denial → 403. Match by `code` / // `name` to avoid pulling a runtime dependency on plugin-security. if ( diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index ceb50597c7..675ed2c47e 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -2251,6 +2251,36 @@ describe('mapDataError — schema/constraint envelopes', () => { expect(r.body.object).toBe('crm_lead'); }); + // #4630: the sys_comment record-level gates from plugin-audit's engine + // hooks. They reuse the STANDARD catalog code rather than a bespoke + // COMMENT_* one, so this asserts the same object-preference the sibling + // gates get — the generic 4xx passthrough would report `sys_comment`. + it('maps RECORD_NOT_ACCESSIBLE → 403 with the record\'s object, not the comment table', () => { + const r = mapDataError( + Object.assign( + new Error('Cannot comment on crm_opportunity/1A7n: the record does not exist or you cannot read it'), + { code: 'RECORD_NOT_ACCESSIBLE', status: 403, object: 'crm_opportunity' }, + ), + 'sys_comment', + ); + expect(r.status).toBe(403); + expect(r.body.code).toBe('RECORD_NOT_ACCESSIBLE'); + expect(r.body.object).toBe('crm_opportunity'); + }); + + it('maps RECORD_NOT_ACCESSIBLE without a parent object (malformed thread_id) → 403', () => { + const r = mapDataError( + Object.assign(new Error('Cannot comment: thread_id "crm_opportunity:" does not name a record'), { + code: 'RECORD_NOT_ACCESSIBLE', + status: 403, + }), + 'sys_comment', + ); + expect(r.status).toBe(403); + expect(r.body.code).toBe('RECORD_NOT_ACCESSIBLE'); + expect(r.body.object).toBe('sys_comment'); + }); + it('maps ATTACHMENT_DELETE_DENIED → 403', () => { const r = mapDataError( Object.assign(new Error('Cannot delete attachment a1: only the uploader or a user who can edit the parent record (crm_lead/rec1) may delete it'), { From eb2e1a3c67a86d9f2aaf7701638cc93e60b494e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 02:53:40 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix(plugin-audit):=20sharing=20=E6=A7=BD?= =?UTF-8?q?=E4=BD=8D=E6=9F=A5=E6=89=BE=E5=B8=A6=E4=B8=8A=E5=A5=91=E7=BA=A6?= =?UTF-8?q?=E7=B1=BB=E5=9E=8B,=E4=B8=8D=E5=86=8D=E6=93=A6=E9=99=A4?= =?UTF-8?q?=E4=B8=BA=20any=20(#4251=20=E6=A3=98=E8=BD=AE)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:slot-lookup` 报 audit-plugin.ts 的擦除计数 1 → 2:文件的祖父豁免只 保存量点位,新点位必须带 slot 的契约类型。 - `ctx.getService('sharing')` —— 用真的契约接口。 - `CommentSharingLike` 从本地手写接口改为 `Pick`: "只用 canEdit"这个窄面是有意的,再手写一份它的形状不是(PD #12,一份契约 不要方言);`callerContext` 的返回类型同步换成 `SharingExecutionContext`。 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny --- packages/plugins/plugin-audit/src/audit-plugin.ts | 6 ++++-- .../plugin-audit/src/comment-access-hooks.ts | 15 ++++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/packages/plugins/plugin-audit/src/audit-plugin.ts b/packages/plugins/plugin-audit/src/audit-plugin.ts index 3e73c76b6d..cab179f646 100644 --- a/packages/plugins/plugin-audit/src/audit-plugin.ts +++ b/packages/plugins/plugin-audit/src/audit-plugin.ts @@ -2,7 +2,7 @@ import type { Plugin, PluginContext } from '@objectstack/core'; import { resolveLocalizationContext } from '@objectstack/core'; -import type { IDataEngine } from '@objectstack/spec/contracts'; +import type { IDataEngine, ISharingService } from '@objectstack/spec/contracts'; import { SysAuditLog, SysActivity, SysComment } from './objects/index.js'; // `sys_notification` was parked here "until that [ADR-0030] migration lands". // It has landed, so the contribution moved to @objectstack/service-messaging — @@ -143,7 +143,9 @@ export class AuditPlugin implements Plugin { engine as any, () => { try { - return ctx.getService('sharing'); + // Typed with the slot's contract (#4251): the gate consults + // `canEdit` only, but it consults the REAL interface. + return ctx.getService('sharing'); } catch { return null; } diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.ts index f33b21fa6f..45844c5503 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.ts @@ -55,6 +55,8 @@ * load-bearing. */ +import type { ISharingService, SharingExecutionContext } from '@objectstack/spec/contracts'; + /** Minimal engine surface these installers need — duck-typed (like * service-storage's attachment seams) so tests can fake it and so plugin-audit * keeps its dependency-free posture. */ @@ -83,10 +85,13 @@ export interface CommentReadMiddlewareCtx { context?: { userId?: string; tenantId?: string; positions?: string[]; permissions?: string[]; isSystem?: boolean } & Record; } -/** Minimal surface of plugin-sharing's service this gate consults. */ -export interface CommentSharingLike { - canEdit(object: string, recordId: string, context: Record): Promise; -} +/** + * The single method of plugin-sharing's contract this gate consults. Taken as + * a `Pick` of the real `ISharingService` rather than re-declared locally: the + * narrow surface is the point, a second hand-written shape for it is not + * (AGENTS.md PD #12 — one contract, no dialects). + */ +export type CommentSharingLike = Pick; export interface CommentAccessLogger { info(msg: string, meta?: unknown): void; @@ -176,7 +181,7 @@ function asIdList(id: unknown): Array | null { /** The caller's ExecutionContext rides on the operation options — the session * snapshot lacks `permissions`, which sharing bypasses need. */ -function callerContext(ctx: any): Record { +function callerContext(ctx: any): SharingExecutionContext { const exec = ctx?.input?.options?.context; if (exec && typeof exec === 'object') { return {