diff --git a/.changeset/audit-hook-exclude-objects-registration.md b/.changeset/audit-hook-exclude-objects-registration.md new file mode 100644 index 0000000000..457cd01fda --- /dev/null +++ b/.changeset/audit-hook-exclude-objects-registration.md @@ -0,0 +1,29 @@ +--- +"@objectstack/plugin-audit": patch +--- + +perf(plugin-audit): 审计跳过名单上到注册面,平台内部表不再为白读买单 (#5860) + +plugin-audit 的五个写入注册(`captureBefore` 的 `beforeUpdate`/`beforeDelete`, +`writeAudit` 的 `afterInsert`/`afterUpdate`/`afterDelete`)此前**不带任何对象范围**, +因而在引擎眼里全部是全局 hook。"哪些对象要审计"这个知识一直存在 —— `SKIP_OBJECTS` +—— 但它停在 handler 内部的早退里,注册面上看不见。于是按对象计算需求的两道门只能保守 +判真:#5284 的单 id `update()` 前置行门、#5038 的批量门,对 `sys_job_queue`、 +`sys_job_run`、`sys_upload_session`、`ai_traces` 这些表同样判"需要",每次写入白读一遍 +行集,而 handler 的第一行就返回了。放大倍数最刺眼的是 `sys_job_queue`:每条队列消息 +至少三次写入(publish / lease / terminal),自 #5160 起每封邮件都走它。 + +现在这五个注册带上 `excludeObjects`(#5928 / PR #6575 落地的声明式排除面),名单由 +`SKIP_OBJECTS` **派生**而非重抄,两个面不可能各自漂移。handler 内的早退**保留**为纵深 +防御 —— 它护住的是每一个非 hook 调用方 —— 所以审计写入的行为逐位守恒,变的只是引擎 +能看见的范围。 + +**为什么是减法而不是允许列表**:对象全集在运行期是开放的。`/meta` PUT 会把新对象注册进 +运行中的引擎,而 `SchemaRegistry.registerObject` 不发任何事件,插件侧没有可订阅的通道去 +追平一份枚举出来的名单 —— 那样的名单会在启动时冻结,此后新建的对象**静默**不被审计,对 +合规插件是无声的倒退。排除面没有这个失败模式:安装时没人听说过的对象默认被审计。这条性质 +已单独钉在测试里。 + +顺带,`writeCommentMentions` 收为 `{ object: 'sys_comment' }` —— 它的 handler 第一行本就 +拒绝其他对象,这是一个封闭的单名允许列表,现有契约一直表达得了。行为不变,但它不再出现在 +其他任何对象的 `afterInsert` 需求里。 diff --git a/packages/plugins/plugin-audit/package.json b/packages/plugins/plugin-audit/package.json index c869ee1ddf..f2f1ae10ed 100644 --- a/packages/plugins/plugin-audit/package.json +++ b/packages/plugins/plugin-audit/package.json @@ -23,6 +23,7 @@ "@objectstack/spec": "workspace:*" }, "devDependencies": { + "@objectstack/objectql": "workspace:*", "@types/node": "^26.1.2", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts b/packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts new file mode 100644 index 0000000000..ffdb3657e2 --- /dev/null +++ b/packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts @@ -0,0 +1,512 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5860] plugin-audit declares its skip list ON THE REGISTRATION FACE, so the + * engine's per-object demand gates can see it. + * + * ## The defect + * + * The five audit registrations (`captureBefore` on `beforeUpdate`/`beforeDelete`, + * `writeAudit` on `afterInsert`/`afterUpdate`/`afterDelete`) carried no scope at + * all, so every one of them read as GLOBAL. The knowledge of which objects are + * audited existed — `SKIP_OBJECTS` — but it lived inside the handler as an early + * return, where no gate can reach it. `hasHooksFor` therefore answered "yes, a + * hook covers this object" for `sys_job_queue` as readily as for a business + * object, and #5284's single-id prior-row gate (`update()`) plus #5038's bulk + * gates bought a `driver.findOne` / matched-row read for a handler that was + * going to return on its first line. + * + * ## Why an allow list could not fix it (#5928, PR #6575) + * + * `SKIP_OBJECTS` is a DENY list over an OPEN universe: `/meta` PUT registers new + * objects into a running engine with no event a plugin could subscribe to, so a + * registrant that enumerated the complement would freeze its list at boot and + * silently stop auditing everything created afterwards — a compliance + * regression, and a quiet one. The `excludeObjects` face #6575 added is the + * expression this plugin was missing; `test 3` below is the pin that the deny + * direction is preserved. + * + * ## What each case measures + * + * 1. **the flip** — a SKIP object's single-id `update()` stops paying the + * prior-row read, and the gate itself answers `false`. RED before this + * change (delta 1 / gate `true`). + * 2. **not over-narrowed** — a business object still demands the read AND + * still gets its audit row. Green before and after: the pin exists so a + * narrowing that went one step too far cannot pass. + * 3. **the compliance direction** — an object registered AFTER the writers are + * installed is audited, because the declared face SUBTRACTS rather than + * enumerates. Green before and after by construction; it is the property + * that made `excludeObjects` necessary, so it is pinned where a future + * "just list the audited objects" refactor would break it. + * 4. **the two faces cannot drift** — every name on the registration's + * exclusion list is also refused by the handler's early return (kept as + * defence in depth, so audit behaviour is conserved). + * 5. **`writeCommentMentions`** is a closed single-name allow list and now says + * so. Behaviour-conserving by construction (the handler's first line + * already refused every other object), so this case is a scope pin, not a + * flip. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { installAuditWriters } from './audit-writers.js'; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +/** The audit ledger + its activity mirror, single-tenant shape (no org column). */ +const sysAuditLog = { + name: 'sys_audit_log', + label: 'Audit Log', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + action: { name: 'action', label: 'Action', type: 'text' as const }, + user_id: { name: 'user_id', label: 'User', type: 'text' as const }, + object_name: { name: 'object_name', label: 'Object', type: 'text' as const }, + record_id: { name: 'record_id', label: 'Record', type: 'text' as const }, + old_value: { name: 'old_value', label: 'Old', type: 'textarea' as const }, + new_value: { name: 'new_value', label: 'New', type: 'textarea' as const }, + tenant_id: { name: 'tenant_id', label: 'Tenant', type: 'text' as const }, + }, +}; + +const sysActivity = { + name: 'sys_activity', + label: 'Activity', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + type: { name: 'type', label: 'Type', type: 'text' as const }, + timestamp: { name: 'timestamp', label: 'At', type: 'datetime' as const }, + summary: { name: 'summary', label: 'Summary', type: 'text' as const }, + actor_id: { name: 'actor_id', label: 'Actor', type: 'text' as const }, + object_name: { name: 'object_name', label: 'Object', type: 'text' as const }, + record_id: { name: 'record_id', label: 'Record', type: 'text' as const }, + record_label: { name: 'record_label', label: 'Label', type: 'text' as const }, + metadata: { name: 'metadata', label: 'Metadata', type: 'textarea' as const }, + }, +}; + +/** + * IN `SKIP_OBJECTS` — engine-owned durable queue plumbing (#5193, ADR-0057 D5). + * The table this issue was measured on: ≥3 writes per queued message, each one + * buying a prior-row read for a handler that returns immediately. + */ +const sysJobQueue = { + name: 'sys_job_queue', + label: 'Job Queue', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + topic: { name: 'topic', label: 'Topic', type: 'text' as const }, + }, +}; + +/** NOT in `SKIP_OBJECTS` — ordinary business truth, must stay fully audited. */ +const bizTask = { + name: 'biz_task', + label: 'Task', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + }, +}; + +/** Registered only AFTER the writers are installed — case 3's whole point. */ +const bizLate = { + name: 'biz_late_arrival', + label: 'Late Arrival', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + }, +}; + +const sysComment = { + name: 'sys_comment', + label: 'Comment', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + thread_id: { name: 'thread_id', label: 'Thread', type: 'text' as const }, + body: { name: 'body', label: 'Body', type: 'textarea' as const }, + author_id: { name: 'author_id', label: 'Author', type: 'text' as const }, + mentions: { name: 'mentions', label: 'Mentions', type: 'textarea' as const }, + }, +}; + +// --------------------------------------------------------------------------- +// A driver that COUNTS reads, so "pays no extra read" is a measurement +// --------------------------------------------------------------------------- + +/** + * Same shape as `objectql`'s own `engine-update-prior-read-scope.test.ts` + * counter (this is the `IDataDriver` contract — object name first, primary key + * SECOND — not an engine double). `findOneOn` is per object because a write can + * legitimately read a different object, and conflating the two would make the + * delta below unreadable. + */ +function makeCountingDriver() { + const stores = new Map>>(); + const reads = { findOne: 0, find: 0, findOneOn: {} as Record }; + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and' && Array.isArray(v)) { + if (!v.every((sub) => matchesWhere(row, sub))) return false; + continue; + } + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + reads.find += 1; + return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + reads.findOne += 1; + reads.findOneOn[object] = (reads.findOneOn[object] ?? 0) + 1; + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row: Record = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + for (const r of rows) storeFor(object).set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, reads, storeFor }; +} + +const BASE_OBJECTS = [sysAuditLog, sysActivity, sysJobQueue, bizTask]; + +/** Owning package for the fixtures — the registry requires one. */ +const OWNER_PACKAGE = 'com.objectstack.test.audit-scope'; + +async function boot(objects: unknown[] = BASE_OBJECTS) { + const engine = new ObjectQL(); + const stub = makeCountingDriver(); + engine.registerDriver(stub.driver, true); + await engine.init(); + for (const o of objects) engine.registry.registerObject(o as any, OWNER_PACKAGE); + return { engine, reads: stub.reads, storeFor: stub.storeFor }; +} + +/** Audit ledger rows recorded for one object, in write order. */ +function auditRowsFor( + storeFor: (o: string) => Map>, + objectName: string, +): Array> { + return Array.from(storeFor('sys_audit_log').values()).filter((r) => r.object_name === objectName); +} + +/** The gate #5284 / #5038 consult. Private on purpose; read directly so the + * assertion names the gate rather than inferring it from a side effect. */ +const gateOpen = (engine: unknown, event: string, object: string): boolean => + (engine as any).hasHooksFor(event, object); + +// --------------------------------------------------------------------------- +// 1. The flip — this is the issue's acceptance criterion +// --------------------------------------------------------------------------- + +describe('[#5860] a SKIP_OBJECTS object no longer forces the prior-row read', () => { + it('single-id update() on `sys_job_queue` pays ZERO prior reads', async () => { + const { engine, reads } = await boot(); + installAuditWriters(engine); + + const row: any = await engine.insert('sys_job_queue', { topic: 'mail', status: 'pending' }); + const before = reads.findOneOn['sys_job_queue'] ?? 0; + await engine.update('sys_job_queue', { status: 'running' }, { where: { id: row.id } } as any); + + // Before this change: 1 — #5284's gate saw five global audit registrations + // and could not know the handler returns on its first line. + expect((reads.findOneOn['sys_job_queue'] ?? 0) - before).toBe(0); + }); + + it('the gate itself answers `false` for every audit event on a skipped object', async () => { + const { engine } = await boot(); + installAuditWriters(engine); + + // The direct reading of the acceptance criterion: not "a read was avoided" + // but "the per-object demand gate judges this object unhooked". + expect(gateOpen(engine, 'afterUpdate', 'sys_job_queue')).toBe(false); + expect(gateOpen(engine, 'afterInsert', 'sys_job_queue')).toBe(false); + expect(gateOpen(engine, 'afterDelete', 'sys_job_queue')).toBe(false); + expect(gateOpen(engine, 'beforeUpdate', 'sys_job_queue')).toBe(false); + expect(gateOpen(engine, 'beforeDelete', 'sys_job_queue')).toBe(false); + }); + + it('no audit or activity row is written for the skipped object either', async () => { + // Behaviour conservation, measured on the same run as the saving: the read + // that disappeared was never feeding a row. + const { engine, storeFor } = await boot(); + installAuditWriters(engine); + + const row: any = await engine.insert('sys_job_queue', { topic: 'mail', status: 'pending' }); + await engine.update('sys_job_queue', { status: 'running' }, { where: { id: row.id } } as any); + + expect(auditRowsFor(storeFor, 'sys_job_queue')).toEqual([]); + expect(Array.from(storeFor('sys_activity').values()) + .filter((r) => r.object_name === 'sys_job_queue')).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Not over-narrowed +// --------------------------------------------------------------------------- + +describe('[#5860] an audited object keeps its read and its ledger row', () => { + it('the gate stays open for a business object', async () => { + const { engine } = await boot(); + installAuditWriters(engine); + + expect(gateOpen(engine, 'afterUpdate', 'biz_task')).toBe(true); + expect(gateOpen(engine, 'afterInsert', 'biz_task')).toBe(true); + expect(gateOpen(engine, 'afterDelete', 'biz_task')).toBe(true); + expect(gateOpen(engine, 'beforeUpdate', 'biz_task')).toBe(true); + expect(gateOpen(engine, 'beforeDelete', 'biz_task')).toBe(true); + }); + + it('single-id update() on `biz_task` still reads the prior row and audits the diff', async () => { + const { engine, reads, storeFor } = await boot(); + installAuditWriters(engine); + + const row: any = await engine.insert('biz_task', { title: 'Ship it', status: 'todo' }); + const before = reads.findOneOn['biz_task'] ?? 0; + await engine.update('biz_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + + // The count and the effect together: a read that happened but fed nothing + // would pass the first assertion and fail the second. + expect((reads.findOneOn['biz_task'] ?? 0) - before).toBeGreaterThan(0); + const audited = auditRowsFor(storeFor, 'biz_task'); + expect(audited.map((r) => r.action)).toEqual(['create', 'update']); + expect(JSON.parse(String(audited[1]!.new_value))).toMatchObject({ status: 'in_progress' }); + expect(JSON.parse(String(audited[1]!.old_value))).toMatchObject({ status: 'todo' }); + }); +}); + +// --------------------------------------------------------------------------- +// 3. The compliance direction — why this is a deny list, not an allow list +// --------------------------------------------------------------------------- + +describe('[#5860] objects registered AFTER install are audited by default', () => { + it('a `/meta`-style late registration is covered with no re-install', async () => { + // This is the property that ruled out enumerating the complement of + // SKIP_OBJECTS (probe D on #5860, ruling #5928): the audit face must be a + // list of subtractions, so an object nobody had heard of at boot is + // audited, not silently skipped. `SchemaRegistry.registerObject` emits no + // event, so a plugin holding an allow list could never learn about this. + const { engine, storeFor } = await boot(); + installAuditWriters(engine); + + engine.registry.registerObject(bizLate as any, OWNER_PACKAGE); + + expect(gateOpen(engine, 'afterUpdate', 'biz_late_arrival')).toBe(true); + + const row: any = await engine.insert('biz_late_arrival', { title: 'New', status: 'todo' }); + await engine.update('biz_late_arrival', { status: 'done' }, { where: { id: row.id } } as any); + + expect(auditRowsFor(storeFor, 'biz_late_arrival').map((r) => r.action)).toEqual(['create', 'update']); + }); +}); + +// --------------------------------------------------------------------------- +// 4/5. The registration face itself +// --------------------------------------------------------------------------- + +interface Registration { + event: string; + handler: (ctx: any) => any; + options: Record | undefined; +} + +/** + * Records what `installAuditWriters` DECLARES. Deliberately not a data engine — + * it owns no write verbs, so there is no dispatch contract for it to be looser + * than; the behaviour half of every claim here is measured on the real + * `ObjectQL` above. + */ +function makeRecordingEngine() { + const registrations: Registration[] = []; + const created: Array<{ object: string; row: Record }> = []; + const sudoApi = { + object(name: string) { + return { + async create(row: Record) { + created.push({ object: name, row }); + return { id: 'generated-id', ...row }; + }, + }; + }, + }; + const engine = { + getSchema(name: string) { + return { name, fields: { id: { type: 'text' }, title: { type: 'text' } } }; + }, + registerHook(event: string, handler: (ctx: any) => any, options?: Record) { + registrations.push({ event, handler, options }); + }, + unregisterHooksByPackage() {}, + logger: { warn() {}, debug() {} }, + }; + const api = { sudo: () => sudoApi }; + return { engine, registrations, created, api }; +} + +/** The five writer registrations: global minus the skip list. */ +const AUDIT_WRITER_EVENTS = ['beforeUpdate', 'beforeDelete', 'afterInsert', 'afterUpdate', 'afterDelete']; + +describe('[#5860] the skip list is declared on the registration face', () => { + it('all five writer registrations carry `excludeObjects` and stay global otherwise', () => { + const { engine, registrations } = makeRecordingEngine(); + installAuditWriters(engine); + + for (const event of AUDIT_WRITER_EVENTS) { + const writers = registrations.filter( + (r) => r.event === event && r.options?.excludeObjects !== undefined, + ); + expect(writers, `no excluding registration for ${event}`).toHaveLength(1); + const opts = writers[0]!.options!; + // Still global on the allow half — narrowing THAT is the enumeration + // mistake case 3 exists to forbid. + expect(opts.object).toBeUndefined(); + const excluded: string[] = Array.isArray(opts.excludeObjects) + ? opts.excludeObjects + : [opts.excludeObjects]; + // Representatives of both reasons a table lands on the list: recursion / + // auth noise, and ADR-0057 operational telemetry. + expect(excluded).toContain('sys_audit_log'); + expect(excluded).toContain('sys_activity'); + expect(excluded).toContain('sys_comment'); + expect(excluded).toContain('sys_job_queue'); + expect(excluded).not.toContain('biz_task'); + // #6575 refuses both of these at registration; a spread of the real skip + // list can never produce them, and this says so out loud. + expect(excluded).not.toContain('*'); + expect(excluded.every((n) => typeof n === 'string' && n.trim().length > 0)).toBe(true); + } + }); + + it('the handler early return still refuses every name the registration excludes', async () => { + // Defence in depth, and the anti-drift pin: the two faces are one list, so + // a hand-edit that adds a name to only one of them fails here. A handler + // that stopped refusing would still be correct on a hook-dispatching + // engine — and wrong for every direct caller and every future dispatch + // path, which is why the early return was kept rather than deleted. + const { engine, registrations, created, api } = makeRecordingEngine(); + installAuditWriters(engine); + + const writeAudit = registrations.find( + (r) => r.event === 'afterUpdate' && r.options?.excludeObjects !== undefined, + )!; + const excluded: string[] = writeAudit.options!.excludeObjects; + expect(excluded.length).toBeGreaterThan(0); + + for (const object of excluded) { + await writeAudit.handler({ + event: 'afterUpdate', + object, + api, + input: { id: 'x1' }, + result: { id: 'x1', title: 'after' }, + previous: { id: 'x1', title: 'before' }, + }); + } + expect(created).toEqual([]); + + // Control: the same handler on a name that is NOT excluded does write. + await writeAudit.handler({ + event: 'afterUpdate', + object: 'biz_task', + api, + input: { id: 'x1' }, + result: { id: 'x1', title: 'after' }, + previous: { id: 'x1', title: 'before' }, + }); + expect(created.map((c) => c.object)).toContain('sys_audit_log'); + }); + + it('`writeCommentMentions` is registered for `sys_comment` only', () => { + const { engine, registrations } = makeRecordingEngine(); + installAuditWriters(engine); + + const mentions = registrations.filter( + (r) => r.event === 'afterInsert' && r.options?.object === 'sys_comment', + ); + expect(mentions).toHaveLength(1); + expect(mentions[0]!.options!.excludeObjects).toBeUndefined(); + expect(mentions[0]!.options!.packageId).toBe('com.objectstack.audit'); + }); + + it('mention notifications fire for a comment and for nothing else', async () => { + // Behaviour conservation, not a flip: the handler's first line already + // refused every other object, so this reads the same before and after the + // registration was narrowed. It is what makes the narrowing provably free. + const emit = vi.fn(async (_event: Record) => {}); + const { engine, storeFor } = await boot([...BASE_OBJECTS, sysComment]); + installAuditWriters(engine, 'com.objectstack.audit', { + getMessaging: () => ({ emit } as any), + }); + + await engine.insert('sys_comment', { + thread_id: 'biz_task:t1', + body: 'ping @u2', + author_id: 'u1', + mentions: JSON.stringify(['u2']), + }); + expect(emit).toHaveBeenCalledTimes(1); + expect(emit.mock.calls[0]![0]).toMatchObject({ topic: 'collab.mention', audience: ['u2'] }); + + // A non-comment insert carrying the same shape must not reach it. + emit.mockClear(); + await engine.insert('biz_task', { title: 'unrelated', status: 'todo' }); + expect(emit).not.toHaveBeenCalled(); + + // And sys_comment stays out of the ledger — it has its own feed. + expect(auditRowsFor(storeFor, 'sys_comment')).toEqual([]); + }); +}); diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index d2264aeef2..c70ef32294 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -141,6 +141,32 @@ const SKIP_OBJECTS = new Set([ 'ai_traces', // LLM trace telemetry ]); +/** + * [#5860] The same list, on the REGISTRATION face. + * + * `SKIP_OBJECTS` above is consulted inside the handlers, where it is invisible + * to the engine. That made every audit registration read as GLOBAL, so the + * per-object demand gates (#5284's single-id prior-row read on `update()`, + * #5038's bulk equivalents) had to answer "yes, a hook covers this object" for + * every one of these tables and buy a row read for a handler that returns on + * its first line. The knowledge existed; the contract had nowhere to put it + * until #5928 / PR #6575 added `excludeObjects`. + * + * Why the negative face and not `object: [...]` with the complement: the object + * universe is OPEN. `/meta` PUT registers new objects into a running engine and + * `SchemaRegistry.registerObject` emits no event, so an enumerated allow list + * would freeze at boot and silently stop auditing everything created after — + * for a compliance plugin, a regression that reports nothing. Subtraction has + * no such failure mode: an object nobody had heard of at install time is + * audited by default. Pinned in `audit-hook-object-scope.test.ts`. + * + * DERIVED, never re-typed: the registration face and the handlers' early return + * are one list, so neither can drift from the other. The early return stays as + * defence in depth — it is what protects every non-hook caller of these + * handlers, and it keeps audit behaviour bit-for-bit conserved by this change. + */ +const AUDIT_EXCLUDED_OBJECTS: string[] = [...SKIP_OBJECTS]; + /** Fields that are noise in diffs (always change, never user-meaningful). */ const NOISE_FIELDS = new Set([ 'updated_at', @@ -541,8 +567,11 @@ export function installAuditWriters( } }; - engine.registerHook('beforeUpdate', captureBefore, { packageId }); - engine.registerHook('beforeDelete', captureBefore, { packageId }); + // [#5860] Global MINUS the skip list — see `AUDIT_EXCLUDED_OBJECTS`. The + // handler's own `SKIP_OBJECTS` early return is kept (defence in depth), so + // what changes here is only what the ENGINE can see about the scope. + engine.registerHook('beforeUpdate', captureBefore, { excludeObjects: AUDIT_EXCLUDED_OBJECTS, packageId }); + engine.registerHook('beforeDelete', captureBefore, { excludeObjects: AUDIT_EXCLUDED_OBJECTS, packageId }); /** * afterInsert / afterUpdate / afterDelete: write audit_log + activity rows. @@ -743,9 +772,12 @@ export function installAuditWriters( } }; - engine.registerHook('afterInsert', writeAudit, { packageId }); - engine.registerHook('afterUpdate', writeAudit, { packageId }); - engine.registerHook('afterDelete', writeAudit, { packageId }); + // [#5860] Same subtraction as the before-phase pair above. `afterUpdate` and + // `afterDelete` are the two the bulk gates (#5038) read, and `afterUpdate` is + // the one #5284's single-id gate reads. + engine.registerHook('afterInsert', writeAudit, { excludeObjects: AUDIT_EXCLUDED_OBJECTS, packageId }); + engine.registerHook('afterUpdate', writeAudit, { excludeObjects: AUDIT_EXCLUDED_OBJECTS, packageId }); + engine.registerHook('afterDelete', writeAudit, { excludeObjects: AUDIT_EXCLUDED_OBJECTS, packageId }); /** * `enable.feeds` server-side enforcement (#2707). Comments are created @@ -878,7 +910,11 @@ export function installAuditWriters( } } }; - engine.registerHook('afterInsert', writeCommentMentions, { packageId }); + // [#5860] A CLOSED single-name allow list — the handler's first line already + // refused every other object, and unlike the skip list above that knowledge + // was always expressible. Saying it here is behaviour-conserving and takes + // this registration out of every other object's `afterInsert` demand. + engine.registerHook('afterInsert', writeCommentMentions, { object: 'sys_comment', packageId }); } // Re-export for convenience. diff --git a/packages/plugins/plugin-audit/vitest.config.ts b/packages/plugins/plugin-audit/vitest.config.ts index e12f1473a8..9aafd36b51 100644 --- a/packages/plugins/plugin-audit/vitest.config.ts +++ b/packages/plugins/plugin-audit/vitest.config.ts @@ -15,18 +15,31 @@ export default defineConfig({ environment: 'node', }, resolve: { - alias: { - '@objectstack/core': path.resolve(__dirname, '../../core/src/index.ts'), - '@objectstack/platform-objects/audit': path.resolve(__dirname, '../../platform-objects/src/audit/index.ts'), - '@objectstack/spec/contracts': path.resolve(__dirname, '../../spec/src/contracts/index.ts'), - '@objectstack/spec/data': path.resolve(__dirname, '../../spec/src/data/index.ts'), - '@objectstack/spec/system': path.resolve(__dirname, '../../spec/src/system/index.ts'), - '@objectstack/spec/api': path.resolve(__dirname, '../../spec/src/api/index.ts'), - '@objectstack/spec/kernel': path.resolve(__dirname, '../../spec/src/kernel/index.ts'), - // [ADR-0105 D1] Reached transitively via `@objectstack/types` (tenancy posture). - '@objectstack/spec/security': path.resolve(__dirname, '../../spec/src/security/index.ts'), - '@objectstack/spec': path.resolve(__dirname, '../../spec/src/index.ts'), - '@objectstack/types': path.resolve(__dirname, '../../types/src/index.ts'), - }, + // Array form with ANCHORED patterns, deliberately — the same correction + // `service-knowledge` recorded, arriving here for the same reason. The + // object form matches by PREFIX, so the bare `@objectstack/spec` entry + // swallowed every subpath not spelled out above it: `@objectstack/spec/ui` + // resolved to `spec/src/index.ts/ui` and failed with `ENOTDIR`. That kept + // the namespace list a thing every new import had to extend by hand, and + // the failure landed on whoever added the import, naming a path nobody + // wrote — which is exactly how it surfaced for #5860's real-engine test + // (`@objectstack/objectql` reaches `@objectstack/spec/ui` transitively). + // One rule for all namespaces cannot go stale that way. + alias: [ + { find: /^@objectstack\/core$/, replacement: path.resolve(__dirname, '../../core/src/index.ts') }, + { + find: /^@objectstack\/platform-objects\/audit$/, + replacement: path.resolve(__dirname, '../../platform-objects/src/audit/index.ts'), + }, + // Covers `data` / `system` / `kernel` / `api` / `contracts` / `ui` / + // `shared` and, [ADR-0105 D1], `security` reached transitively via + // `@objectstack/types` (tenancy posture). + { + find: /^@objectstack\/spec\/([a-z-]+)$/, + replacement: `${path.resolve(__dirname, '../../spec/src')}/$1/index.ts`, + }, + { find: /^@objectstack\/spec$/, replacement: path.resolve(__dirname, '../../spec/src/index.ts') }, + { find: /^@objectstack\/types$/, replacement: path.resolve(__dirname, '../../types/src/index.ts') }, + ], }, }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 091ce13199..a32f14f437 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1394,6 +1394,9 @@ importers: specifier: workspace:* version: link:../../spec devDependencies: + '@objectstack/objectql': + specifier: workspace:* + version: link:../../objectql '@types/node': specifier: ^26.1.2 version: 26.1.2