diff --git a/.changeset/23124-comment-edit-stamp.md b/.changeset/23124-comment-edit-stamp.md new file mode 100644 index 00000000000..57c311f1ba9 --- /dev/null +++ b/.changeset/23124-comment-edit-stamp.md @@ -0,0 +1,16 @@ +--- +'@objectstack/plugin-audit': minor +--- + +`sys_comment` edit marks are platform-written: an update that changes a comment's `body` stamps `is_edited: true` and `edited_at` from the server clock, and a client can no longer set or clear either field. + +Clause-②: no (narrowing) + + + +**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes. + +- **Stamped by the platform.** The comment's `beforeUpdate` sets both fields when the update's `body` differs from the stored body, for every caller, the way the engine stamps `updated_at`. An update that does not name `body`, or sends the stored text again beside new `mentions`, is not an edit and stamps nothing. A create is not an edit either. +- **Read-only at the data door.** `is_edited` and `edited_at` now declare `readonly: true`. On an update that edits the body, the stamp replaces whatever the client sent; on any other update a non-system caller's value is dropped and reported in the response's `droppedFields`; on a create the platform sets both to "not edited" over a client value. Nothing is refused for carrying the fields — a client that still sends them on every edit, as the console released before this change does, keeps working and its values are ignored. Read both fields back from the row instead of writing them. +- **Predicate updates.** A `multi: true` update is stamped per matched row. One whose matched rows disagree — some bodies change, some already hold the new text — is now refused whole with `MULTI_UPDATE_HOOK_KEY_DIVERGENCE` (400) and writes nothing, because one SET clause cannot mark some rows and not others. Narrow the predicate to the rows whose text changes, or update by id. +- A `preserveAudit` (historical import) update keeps a mark it supplies, as it keeps `updated_at`. diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts index d94369c6e6b..2721c032dda 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts @@ -71,8 +71,11 @@ function install(opts: { const parentReads: Array<{ object: string; options: any }> = []; const parentFind = readableParentFind(opts.readable ?? [], parentReads); const engine: CommentAccessEngine = { + // The FIRST handler per event is the authorization gate these tests drive: + // `beforeUpdate` also carries the edit stamp (#23124), registered after it + // and pinned on the real engine in `comment-edit-stamp.integration.test.ts`. registerHook: (event, handler) => { - hooks.set(event, handler as any); + if (!hooks.has(event)) hooks.set(event, handler as any); }, find: async (object, options: any) => { if (object !== 'sys_comment') return parentFind(object, options); @@ -644,6 +647,10 @@ const COMMENT_FIELDS = { thread_id: { name: 'thread_id', label: 'Thread', type: 'text' as const }, author_id: { name: 'author_id', label: 'Author', type: 'text' as const }, body: { name: 'body', label: 'Body', type: 'text' as const }, + // The edit stamp `installCommentAccessHooks` also registers (#23124) writes + // these on a body change; declared as `sys_comment` declares them. + is_edited: { name: 'is_edited', label: 'Edited', type: 'boolean' as const, readonly: true }, + edited_at: { name: 'edited_at', label: 'Edited At', type: 'datetime' as const, readonly: true }, }; const sysCommentObject = { name: 'sys_comment', label: 'Comment', fields: COMMENT_FIELDS }; const crmOpportunityObject = { diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.ts index 08767b94821..1618e3e54de 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.ts @@ -39,6 +39,10 @@ * platform's not-visible refusal instead of the named one (#21755); see * {@link refuseNotVisible}. The named refusal is what a caller who can * read the parent, but may not edit it, receives. + * * The edit stamp (#23124, ruling A): a SECOND `beforeUpdate` handler, not + * part of the authorization above, sets `is_edited` / `edited_at` when the + * body changes — see {@link stampCommentBodyEdit}. The create hook sets + * both to "not edited" over a client value. * - {@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. @@ -319,6 +323,59 @@ function refuseNotVisible( throw err; } +/** + * [#23124, maintainer ruling A] The platform marks a comment edited: when an + * update's `body` differs from the stored body, set `is_edited: true` and + * `edited_at` to the server clock. Both fields are `readonly` on + * `sys_comment`, so on an update by a non-system caller this hook is their one + * writer, as the create hook is on a create (`objects/sys-comment.object.ts`, + * "Platform-written"). + * + * What counts as an edit is the BODY and nothing else: an update that does not + * name `body`, or names the stored text again (the console rewrites `mentions` + * beside it), stamps nothing. A row with no readable pre-image is stamped — + * the engine binds `previous` on both update paths, and a mark that cannot be + * proven unneeded is the safe side of a reader's trust signal. + * + * Every caller is stamped, system included, the way the engine's `updated_at` + * stamp is: the mark records that the text changed after it was posted, not + * who was allowed to change it (the authorization handler is separate and + * exempts system callers; this one does not). The one exception is the + * engine's own: a `preserveAudit` write (a historical import) keeps a value it + * supplies, as `updated_at` does, and is stamped only where it supplies none. + * + * Why the stamp is an assignment and the client's value needs no handling here: + * the engine hides a non-system caller's `readonly` values from the hooks and + * strips them after, and keeps a value a hook ASSIGNED (`hookWrittenKeys`). So + * on an edit the stamp lands whatever the client sent, and without one the + * client's value is dropped and reported as `droppedFields`. + * + * A predicate (`multi: true`) update dispatches this handler once per matched + * row, with that row as `previous`, over ONE shared payload. If every row's + * body changes, every row is stamped; if none does, none is. A batch whose rows + * disagree is refused whole by the engine (`MULTI_UPDATE_HOOK_KEY_DIVERGENCE`, + * 400, nothing written) — one SET clause cannot mark some rows and not others, + * and stamping all of them would mark text that never changed. + */ +async function stampCommentBodyEdit(ctx: any): Promise { + const data: unknown = ctx?.input?.data; + if (!data || typeof data !== 'object' || Array.isArray(data)) return; + const payload = data as Record; + if (!Object.prototype.hasOwnProperty.call(payload, 'body')) return; + const previous: unknown = ctx?.previous; + if (previous && typeof previous === 'object' && Object.is((previous as Record).body, payload.body)) { + return; + } + const now = new Date().toISOString(); + if (ctx?.session?.preserveAudit === true) { + payload.is_edited = payload.is_edited ?? true; + payload.edited_at = payload.edited_at ?? now; + return; + } + payload.is_edited = true; + payload.edited_at = now; +} + 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)) { @@ -539,6 +596,14 @@ export function installCommentAccessHooks( // spoofable — a caller could post as anyone, then "author-delete" it. if (ctx.session.userId) data.author_id = ctx.session.userId; + // [#23124] A new comment is not an edited one, whatever the client sent. + // The engine's create-side `readonly` strip leaves `sys_` objects to their + // own guards, so this is the guard: the platform's value, assigned over a + // client's, exactly as `author_id` above. Only a key the client named is + // touched; an absent one takes the field's default. + if (Object.prototype.hasOwnProperty.call(data, 'is_edited')) data.is_edited = false; + if (Object.prototype.hasOwnProperty.call(data, 'edited_at')) data.edited_at = null; + const target = parseCommentThreadId(data.thread_id); if (!target) { forbid( @@ -729,6 +794,13 @@ export function installCommentAccessHooks( { object: 'sys_comment', packageId: PACKAGE_ID, dispatchUnscopedMultiWrite: true }, ); + // ── Update: the platform stamps an edit of the body (#23124) ───────── + // Its own registration, deliberately: the authorization handler above + // returns early for system and context-less callers and declares the + // whole-operation dispatch, and the stamp wants neither — see + // {@link stampCommentBodyEdit}. + engine.registerHook('beforeUpdate', stampCommentBodyEdit, { object: COMMENT_OBJECT, packageId: PACKAGE_ID }); + // ── Delete: author or parent editor ───────────────────────────────── engine.registerHook( 'beforeDelete', diff --git a/packages/plugins/plugin-audit/src/comment-edit-stamp.integration.test.ts b/packages/plugins/plugin-audit/src/comment-edit-stamp.integration.test.ts new file mode 100644 index 00000000000..a6368622a8e --- /dev/null +++ b/packages/plugins/plugin-audit/src/comment-edit-stamp.integration.test.ts @@ -0,0 +1,283 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The platform marks a comment edited (#23124, maintainer ruling A). + * + * `sys_comment.is_edited` / `edited_at` were declared with no platform writer: + * an author's edit left both untouched unless the client that made it wrote + * them, and any client could write them on any update or create. Under the + * ruling the comment's `beforeUpdate` stamps both when the BODY changes, and + * both are read-only at the data door. + * + * Measured on this change's base (`1ca7a805c`), same rig as this file (real + * kernel, real ObjectQL, real AuditPlugin, real SQLite driver), as the author: + * a body edit stored `is_edited: false` / `edited_at: null`; an update carrying + * `{ is_edited: true, edited_at: }` stored both; a create carrying them + * stored both. Run against that base's implementation, seven of the pins + * below were red. The other three — the two no-stamp pins and the + * `preserveAudit` one — are green there by construction (nothing stamped, and + * nothing was read-only), so they carry no weight on their own; the no-stamp + * pins were ablated against the stamp instead (stamping without the `body` + * guard, or without the unchanged-body comparison, turns them red). + * + * Which shape "read-only" takes was measured too, on the `created_at` precedent: + * on update the engine hides a non-system caller's `readonly` value from the + * hooks and strips it afterwards, reporting it through `onFieldsDropped` (the + * REST door's `droppedFields`); it refuses only for an in-process caller that + * opts into `strictReadonlyWrites`, which no transport passes. On create a + * `sys_` object is left to its own guards. So a client's value never lands — + * overwritten by the stamp on an edit, stripped and reported without one, and + * replaced by the create hook on a create — and the write is never refused: the + * console released before this change sends both fields on every edit. + * + * Assertions are on the stored row, the dropped-fields report and, for the one + * refusal, the wire envelope — never on a bare throw. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { ObjectKernel } from '@objectstack/core'; +import { ObjectQL, ObjectQLPlugin } from '@objectstack/objectql'; +import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm'; +import { mapDataError } from '@objectstack/types'; + +import { AuditPlugin } from './audit-plugin.js'; + +const COMMENT = 'sys_comment'; +/** A parent object every caller reads. */ +const BOARD = 'es_board'; +const HARNESS_PACKAGE = 'com.objectstack.audit.test.comment-edit-stamp'; + +const SYS = { isSystem: true } as const; +const AUTHOR = { userId: 'u_author', positions: ['org_member'] }; +/** A value no server clock in this run can produce. */ +const FORGED = '2000-01-01T00:00:00.000Z'; + +const boardObject = { + name: BOARD, + label: 'Board', + fields: { name: { name: 'name', label: 'Name', type: 'text' as const } }, +}; + +type Row = Record; + +const refusal = async (run: () => Promise): Promise => { + try { + await run(); + } catch (e) { + return e; + } + return undefined; +}; + +describe('sys_comment — the platform stamps an edit of the body (#23124)', () => { + let kernel: ObjectKernel; + let engine: ObjectQL; + + beforeAll(async () => { + kernel = new ObjectKernel({ logger: { level: 'silent' } }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AuditPlugin()); + await kernel.bootstrap(); + + engine = kernel.getService('objectql'); + const driver = new SqliteWasmDriver({ filename: ':memory:' }); + await driver.connect(); + engine.registerDriver(driver, true); + engine.registry.registerObject(boardObject as any, HARNESS_PACKAGE); + await engine.syncSchemas(); + }, 120_000); + + afterAll(async () => { + if (kernel) { + await Promise.race([kernel.shutdown(), new Promise((r) => setTimeout(r, 10_000))]); + } + }, 30_000); + + /** A fresh thread per test, so a predicate write reaches only its own rows. */ + const newThread = async (): Promise => { + const board = await engine.insert(BOARD, { name: 'board' }, { context: SYS }); + return `${BOARD}:${board.id}`; + }; + const post = async (threadId: string, body: string, extra: Row = {}): Promise => { + const created = await engine.insert( + COMMENT, + { thread_id: threadId, author_id: AUTHOR.userId, body, ...extra }, + { context: AUTHOR }, + ); + return String(created.id); + }; + const read = async (id: string): Promise => + (await engine.findOne(COMMENT, { where: { id }, context: SYS })) as Row; + /** The stored mark, and nothing else, so a failure prints exactly it. */ + const mark = async (id: string) => { + const row = await read(id); + return { is_edited: row.is_edited, edited_at: row.edited_at ?? null }; + }; + /** `edited_at` was taken from the server clock inside [from, to]. */ + const expectServerClock = (value: unknown, from: number, to: number) => { + expect(typeof value, `edited_at ${JSON.stringify(value)} is not a timestamp`).toBe('string'); + const at = Date.parse(value as string); + expect(at, `edited_at ${String(value)} is not the server clock of this write`).toBeGreaterThanOrEqual(from); + expect(at, `edited_at ${String(value)} is not the server clock of this write`).toBeLessThanOrEqual(to); + }; + + it('an author edit of the body stamps is_edited and edited_at from the server clock', async () => { + const id = await post(await newThread(), 'original'); + const from = Date.now(); + await engine.update(COMMENT, { body: 'edited' }, { where: { id }, context: AUTHOR }); + const to = Date.now(); + + const row = await read(id); + expect(row.body).toBe('edited'); + expect(row.is_edited).toBe(true); + expectServerClock(row.edited_at, from, to); + }); + + it('a mentions-only edit is not an edit — with or without the unchanged body beside it', async () => { + const id = await post(await newThread(), 'hello @ada'); + + await engine.update(COMMENT, { mentions: '["u_ada"]' }, { where: { id }, context: AUTHOR }); + expect(await mark(id), 'an update that does not name `body` stamped the comment').toEqual({ + is_edited: false, + edited_at: null, + }); + + // The console's shape: the stored text sent again, `mentions` rewritten. + await engine.update(COMMENT, { body: 'hello @ada', mentions: '[]' }, { where: { id }, context: AUTHOR }); + expect(await mark(id), 'an update re-sending the stored body stamped the comment').toEqual({ + is_edited: false, + edited_at: null, + }); + expect((await read(id)).mentions).toBe('[]'); + }); + + it('a create does not stamp — and a create cannot arrive already marked edited', async () => { + const thread = await newThread(); + const plain = await post(thread, 'new'); + expect(await mark(plain)).toEqual({ is_edited: false, edited_at: null }); + + const forged = await post(thread, 'new, claiming an edit', { is_edited: true, edited_at: FORGED }); + expect(await mark(forged), 'a client-supplied mark landed on create').toEqual({ + is_edited: false, + edited_at: null, + }); + }); + + it('on an edit a client cannot clear or forge the mark: the stamp overwrites what it sent', async () => { + const id = await post(await newThread(), 'original'); + const from = Date.now(); + await engine.update( + COMMENT, + { body: 'edited', is_edited: false, edited_at: FORGED }, + { where: { id }, context: AUTHOR }, + ); + const to = Date.now(); + + const row = await read(id); + expect(row.is_edited, 'the client cleared the mark on its own edit').toBe(true); + expectServerClock(row.edited_at, from, to); + }); + + it('without an edit a client can neither set nor clear the mark: the value is stripped and reported', async () => { + const id = await post(await newThread(), 'original'); + + const forgeDrops: Row[] = []; + await engine.update( + COMMENT, + { is_edited: true, edited_at: FORGED }, + { where: { id }, context: AUTHOR, onFieldsDropped: (e: Row) => forgeDrops.push(e) } as any, + ); + expect(await mark(id), 'a client marked an unedited comment edited').toEqual({ + is_edited: false, + edited_at: null, + }); + expect(forgeDrops).toEqual([ + expect.objectContaining({ object: COMMENT, reason: 'readonly', fields: expect.arrayContaining(['is_edited', 'edited_at']) }), + ]); + + await engine.update(COMMENT, { body: 'edited' }, { where: { id }, context: AUTHOR }); + const stamped = await mark(id); + expect(stamped.is_edited).toBe(true); + + const clearDrops: Row[] = []; + await engine.update( + COMMENT, + { is_edited: false, edited_at: null }, + { where: { id }, context: AUTHOR, onFieldsDropped: (e: Row) => clearDrops.push(e) } as any, + ); + expect(await mark(id), 'a client cleared the mark of an edited comment').toEqual(stamped); + expect(clearDrops).toEqual([ + expect.objectContaining({ object: COMMENT, reason: 'readonly', fields: expect.arrayContaining(['is_edited', 'edited_at']) }), + ]); + }); + + it('a predicate update runs the stamp per matched row: every row whose body changes is stamped', async () => { + const thread = await newThread(); + const a = await post(thread, 'first'); + const b = await post(thread, 'second'); + const from = Date.now(); + await engine.update(COMMENT, { body: 'rewritten' }, { where: { thread_id: thread }, multi: true, context: AUTHOR } as any); + const to = Date.now(); + + for (const id of [a, b]) { + const row = await read(id); + expect(row.body).toBe('rewritten'); + expect(row.is_edited, `matched row ${id} was not stamped`).toBe(true); + expectServerClock(row.edited_at, from, to); + } + }); + + it('a predicate update that changes no body stamps no row', async () => { + const thread = await newThread(); + const a = await post(thread, 'same'); + const b = await post(thread, 'same'); + await engine.update(COMMENT, { body: 'same' }, { where: { thread_id: thread }, multi: true, context: AUTHOR } as any); + + for (const id of [a, b]) { + expect(await mark(id), `unchanged row ${id} was stamped`).toEqual({ is_edited: false, edited_at: null }); + } + }); + + it('a predicate update whose rows disagree is refused whole — nothing is written', async () => { + const thread = await newThread(); + const already = await post(thread, 'target'); + const changing = await post(thread, 'other'); + + const error = await refusal(() => + engine.update(COMMENT, { body: 'target' }, { where: { thread_id: thread }, multi: true, context: AUTHOR } as any), + ); + expect(error, 'a mixed predicate update was accepted').toBeDefined(); + const wire = mapDataError(error, COMMENT); + expect(wire.status).toBe(400); + expect(wire.body.code).toBe('MULTI_UPDATE_HOOK_KEY_DIVERGENCE'); + expect((error as { keys?: unknown }).keys).toEqual(['edited_at', 'is_edited']); + + expect((await read(changing)).body).toBe('other'); + for (const id of [already, changing]) { + expect(await mark(id)).toEqual({ is_edited: false, edited_at: null }); + } + }); + + it('every caller is stamped: a system-context edit of the body marks the comment too', async () => { + const id = await post(await newThread(), 'original'); + const from = Date.now(); + await engine.update(COMMENT, { body: 'moderated' }, { where: { id }, context: SYS }); + const to = Date.now(); + + const row = await read(id); + expect(row.is_edited).toBe(true); + expectServerClock(row.edited_at, from, to); + }); + + it('a historical import (preserveAudit) keeps the mark it supplies, as it keeps updated_at', async () => { + const id = await post(await newThread(), 'original'); + await engine.update( + COMMENT, + { body: 'imported text', is_edited: true, edited_at: FORGED }, + { where: { id }, context: { ...AUTHOR, preserveAudit: true } } as any, + ); + const row = await read(id); + expect(row.is_edited).toBe(true); + expect(row.edited_at).toBe(FORGED); + }); +}); diff --git a/packages/plugins/plugin-audit/src/objects/sys-comment.object.ts b/packages/plugins/plugin-audit/src/objects/sys-comment.object.ts index 405c8c5de43..a42f32e4527 100644 --- a/packages/plugins/plugin-audit/src/objects/sys-comment.object.ts +++ b/packages/plugins/plugin-audit/src/objects/sys-comment.object.ts @@ -61,6 +61,38 @@ import { ObjectSchema, Field } from '@objectstack/spec/data'; * (`400 INVALID_FIELD` on a write that names it), and * `os migrate apply --allow-destructive` drops it. * + * ## Platform-written: `is_edited` / `edited_at` (maintainer ruling A on #23124) + * + * The two edit-mark fields were declared here with no platform writer: nothing + * stamped them when an author changed a comment, so the one client that did + * (the console's comment editor) was the only producer, an API or agent edit + * left no mark, and any writer could clear or forge the mark at will. A + * declared lifecycle field that the platform does not keep is the ADR-0049 + * shape this docblock already removed twice; this time the fields are kept and + * ENFORCED instead, because readers measurably rely on the mark (the record + * feed shows "edited" from `is_edited`): + * + * - **The stamp.** `installCommentAccessHooks` registers a `beforeUpdate` + * handler that sets `is_edited: true` and `edited_at` (server clock) when the + * update's `body` differs from the stored body — for every caller, as the + * engine's own `updated_at` stamp is. An update that does not name `body`, or + * names it unchanged (a `mentions`-only edit), is not an edit and stamps + * nothing. A predicate (`multi: true`) update is stamped per matched row; a + * batch whose rows disagree (some bodies change, some do not) is refused whole + * by the engine (`MULTI_UPDATE_HOOK_KEY_DIVERGENCE`), because one SET clause + * cannot carry a per-row mark. + * - **Read-only at the data door.** Both fields declare `readonly: true`. On + * update the engine hides a non-system caller's value from the hooks and + * strips it afterwards (reported as `droppedFields`, logged at `warn`), so a + * client can neither clear nor forge the mark; the stamp's own write is kept. + * On create the engine leaves `sys_` objects to their own guards, so the + * create hook sets `is_edited: false` / `edited_at: null` over a client value: + * a new comment is not an edited one. ⛔ Not a refusal: the console released + * before this change still sends both fields on every edit, and refusing them + * would fail that edit; the value simply never lands. A `preserveAudit` + * (historical import) update keeps a supplied value, as it does for + * `updated_at`. + * * @namespace sys */ export const SysComment = ObjectSchema.create({ @@ -145,15 +177,20 @@ export const SysComment = ObjectSchema.create({ // stored or computed on the comment row (#22573). // ── Lifecycle ──────────────────────────────────────────────── + // Platform-written (#23124): the `beforeUpdate` stamp in + // `comment-access-hooks.ts` sets both when the body changes; a client's + // value never lands (see "Platform-written" above). is_edited: Field.boolean({ label: 'Edited', defaultValue: false, + readonly: true, group: 'Lifecycle', }), edited_at: Field.datetime({ label: 'Edited At', required: false, + readonly: true, group: 'Lifecycle', }),