fix(plugin-audit,rest)!: sys_comment 的访问权从 thread_id 指向的记录继承 (#4630)

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

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<void> }> = [];
+    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 });
+  });
+});

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 —
@@ -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,40 @@ 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 {
+              // Typed with the slot's contract (#4251): the gate consults
+              // `canEdit` only, but it consults the REAL interface.
+              return ctx.getService<ISharingService>('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');
+      }
     });
   }