diff --git a/.changeset/audit-consume-bound-previous.md b/.changeset/audit-consume-bound-previous.md new file mode 100644 index 0000000000..0bff3c7a4f --- /dev/null +++ b/.changeset/audit-consume-bound-previous.md @@ -0,0 +1,65 @@ +--- +"@objectstack/plugin-audit": patch +--- + +fix(plugin-audit): consume the engine's bound `ctx.previous` and record one normalised view on both sides of the diff (#6656) + +`plugin-audit` used to fetch its own pre-image. `captureBefore`, registered on +`beforeUpdate` / `beforeDelete`, issued a `ql.findOne` for the target row and +stashed it on `ctx.__previous`, because `HookContext.previous` was "officially +typed but not always populated by the engine itself". That is no longer true on +any path this plugin registers for, so the read is retired and the writer reads +the contract value. + +**The read that goes away** (measured with a counting driver on the audited +object, `driver.findOne` per write): + +| write | before | after | +|:--|--:|--:| +| single-id `update()` | 2 | 1 | +| single-id `delete()` | 2 | 1 | +| predicate `update()`, 3 matched rows | 3 | 0 | +| predicate `delete()`, 3 matched rows | 3 | 0 | + +The predicate column is the larger half and was pure waste. #5574 binds +`input.id` on every per-row *before* context, which defeated the handler's own +`if (!id) return` bulk guard — so it read every matched row, and every result +was discarded, because `__previous` landed on the per-row *before* context while +the per-row *after* contexts (the ones the writer actually runs on) never saw +it. The engine's own matched-row read is untouched and still serves both phases, +so the ledger is unchanged. + +**What the ledger records changes, and deliberately.** The two sides of an audit +diff came from two different pipelines: `before` through the engine's read path +(credentials masked, formulas hydrated, file references resolved) and `after` +from the raw write result. That asymmetry — not the redundant read — is why a +write that touched one field recorded phantom "changes" for every secret, file +and formula field on the record. Retiring the read makes both sides +same-source; the writer now also gives them one view, so the surface levels +upward rather than down to raw store contents: + +- **Credential fields are masked on both sides.** Single-id delete `old_value` + still reads `••••••••` for a `secret` field — that face is byte-identical. + Change detection still runs on the raw values, so rotating a secret is still + recorded as a change; only the recorded values are masked. +- **A pre-existing leak is closed.** The stored `secret:` ref was already + reaching `sys_audit_log.new_value` on every create and update, and a + `password` field — which ADR-0100 stores in cleartext at rest — was landing + there **in plaintext**, in the audit ledger and in the `sys_activity` summary + rendered in the record feed. Both now record the mask. +- **Virtual (`formula`) fields leave the full snapshots.** `ctx.result` carries + hydrated formulas (#5504) and the raw pre-image structurally cannot, so + create `new_value` would have described a field delete `old_value` could + never carry. Only genuinely virtual fields are dropped: `autonumber` and + `summary` are stored columns present and equal on both sides, and they stay + in the snapshot. + +Two consequences worth naming, both narrowing single-id delete to what bulk +delete already did: its `old_value` now records a file field's stored id rather +than the resolved `{id, name, size, url}` object, and drops formula values. An +object whose label field is a formula falls back to the record id in the +`sys_activity` label on delete for the same reason. + +No audit coverage is removed: the plugin keeps its `afterInsert` / `afterUpdate` +/ `afterDelete` registrations, which is what holds the engine's pre-image demand +gates open, and every one of them keeps the `excludeObjects` face from #5860. diff --git a/packages/plugins/plugin-audit/package.json b/packages/plugins/plugin-audit/package.json index f2f1ae10ed..67a7625cd7 100644 --- a/packages/plugins/plugin-audit/package.json +++ b/packages/plugins/plugin-audit/package.json @@ -2,7 +2,7 @@ "name": "@objectstack/plugin-audit", "version": "17.0.0-rc.5", "license": "Apache-2.0", - "description": "Audit Plugin for ObjectStack — System audit log object and audit trail", + "description": "Audit Plugin for ObjectStack \u2014 System audit log object and audit trail", "main": "dist/index.js", "types": "dist/index.d.ts", "exports": { @@ -19,11 +19,11 @@ }, "dependencies": { "@objectstack/core": "workspace:*", + "@objectstack/objectql": "workspace:*", "@objectstack/platform-objects": "workspace:*", "@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-bound-previous.test.ts b/packages/plugins/plugin-audit/src/audit-bound-previous.test.ts new file mode 100644 index 0000000000..935011e8d7 --- /dev/null +++ b/packages/plugins/plugin-audit/src/audit-bound-previous.test.ts @@ -0,0 +1,541 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6656, Option A+] The audit writer consumes the engine's bound + * `ctx.previous` and normalises what it records. + * + * ## The two halves, and why one without the other was rejected + * + * **Half 1 — the read is retired.** `captureBefore` used to issue its own + * `ql.findOne` on `beforeUpdate` / `beforeDelete`. The engine binds `previous` + * before every `before*` dispatch and its demand gate is the SAME predicate as + * its dispatch gate, so that read was redundant on every path; on the predicate + * path it was worse than redundant, because #5574's per-row `input.id` binding + * defeated the handler's own `if (!id) return` bulk guard and it read every + * matched row, then discarded every result. + * + * **Half 2 — both sides of the diff get one view.** Half 1 alone (the ruled-out + * "Option A") would have made both sides same-SOURCE and stopped there — which + * levels the surface DOWNWARD: single-id delete `old_value` would have started + * recording the stored `secret:` ref where it records `••••••••` today. So the + * writer masks the credential classes on BOTH sides, which additionally closes + * a leak that predates this card: the ref, and for `password` fields the + * CLEARTEXT, were already reaching `sys_audit_log.new_value` on every create + * and update. + * + * ## What each case measures + * + * 1. **the read is gone, single-id** — `update()` and `delete()` each pay + * exactly ONE `driver.findOne`: the engine's. Red if `captureBefore` + * returns (delta 2). + * 2. **the read is gone, predicate** — ZERO per-row `findOne`, where the old + * code paid one per matched row. This is the larger half of the saving. + * 3. **the engine still buys the read, and audit still consumes it** — the + * saving is a read that disappeared, not a hook that stopped seeing the + * prior row. Pinned together with case 1 so "0 reads" can never pass by + * audit going blind. + * 4. **the compliance face** — single-id delete `old_value` still reads + * `••••••••` for a `secret` field, byte-identical to before this change. + * This is the face the ruling protects explicitly. + * 5. **the phantom rows** — a write that touches neither the secret, the file + * reference nor the formula records a diff for NONE of them. Three field + * classes, one case, because one root cause (two pipelines) produced all + * three. + * 6. **a real credential change is still recorded** — the guard against + * "fixed the phantom rows by deleting the audit trail". Change detection + * runs on the raw values, so a rotation still writes a row; only the + * recorded VALUES are masked. + * 7. **the pre-existing leak** — neither the `secret:` ref nor a `password` + * field's cleartext reaches `new_value` on create or update. + * 8. **non-mutating** — normalising must not rewrite `ctx.result`, which is + * the record the caller gets back, nor the row a driver handed out by + * reference. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SECRET_MASK } from '@objectstack/objectql/core'; +import { installAuditWriters } from './audit-writers.js'; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +const f = (name: string, type: string, extra: Record = {}) => + ({ name, label: name, type, ...extra }) as any; + +const sysAuditLog = { + name: 'sys_audit_log', label: 'Audit Log', + fields: { + id: f('id', 'text', { primaryKey: true }), action: f('action', 'text'), + user_id: f('user_id', 'text'), object_name: f('object_name', 'text'), + record_id: f('record_id', 'text'), old_value: f('old_value', 'textarea'), + new_value: f('new_value', 'textarea'), tenant_id: f('tenant_id', 'text'), + }, +}; + +const sysActivity = { + name: 'sys_activity', label: 'Activity', + fields: { + id: f('id', 'text', { primaryKey: true }), type: f('type', 'text'), + timestamp: f('timestamp', 'datetime'), summary: f('summary', 'text'), + actor_id: f('actor_id', 'text'), object_name: f('object_name', 'text'), + record_id: f('record_id', 'text'), record_label: f('record_label', 'text'), + metadata: f('metadata', 'textarea'), + }, +}; + +/** Backing store for `secret`-typed fields — the engine writes a row per value. */ +const sysSecret = { + name: 'sys_secret', label: 'Secret', + fields: { + id: f('id', 'text', { primaryKey: true }), namespace: f('namespace', 'text'), + key: f('key', 'text'), kms_key_id: f('kms_key_id', 'text'), alg: f('alg', 'text'), + version: f('version', 'number'), ciphertext: f('ciphertext', 'textarea'), + created_at: f('created_at', 'datetime'), + }, +}; + +/** `status: 'committed'` is what the read path's file resolver requires. */ +const sysFile = { + name: 'sys_file', label: 'File', + fields: { + id: f('id', 'text', { primaryKey: true }), name: f('name', 'text'), + url: f('url', 'text'), size: f('size', 'number'), status: f('status', 'text'), + }, +}; + +/** + * One object carrying all three classes whose read-path view differs from the + * stored one, plus a `password` field — ADR-0100 stores those PLAINTEXT at + * rest and masks them on read, so it is the same class as `secret` for this + * writer and the worse of the two leaks. + */ +const bizTask = { + name: 'biz_task', label: 'Task', + fields: { + id: f('id', 'text', { primaryKey: true }), + title: f('title', 'text'), + status: f('status', 'text'), + api_key: f('api_key', 'secret'), + login_pw: f('login_pw', 'password'), + attachment: f('attachment', 'file'), + upper_title: f('upper_title', 'formula', { expression: 'record.title + "!"' }), + // A STORED computed column — `COMPUTED_FIELD_TYPES` contains it, but it is + // seeded at insert and present on both sides, so it must survive the + // snapshot normalisation. See `VIRTUAL_FIELD_TYPES`. + ticket_no: f('ticket_no', 'autonumber', { format: 'TK-{00000}' }), + }, +}; + +// --------------------------------------------------------------------------- +// A driver that COUNTS reads and returns COPIES +// --------------------------------------------------------------------------- + +/** + * Same contract shape as the counter in `audit-hook-object-scope.test.ts` + * (`IDataDriver` — object name first), with one deliberate difference: + * **every read returns a deep copy**. + * + * A driver that hands back live store references lets the engine's read-path + * `maskSecretFields` rewrite the store in place, after which the two views + * this file is here to distinguish look identical and every assertion below + * passes for the wrong reason. That contamination is not hypothetical — it + * produced the OPPOSITE conclusion on the first investigation of this card + * before it was caught. Real drivers serialise; so does this one. + */ +function makeCountingDriver() { + const stores = new Map>>(); + const reads = { findOneOn: {} as Record, findOn: {} as Record }; + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + let nextId = 0; + const copy = (r: T): T => (r == null ? r : JSON.parse(JSON.stringify(r))); + const matches = (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) => matches(row, sub))) return false; + continue; + } + if (k.startsWith('$')) continue; + if (v && typeof v === 'object' && '$in' in v) { + if (!(v.$in as unknown[]).includes(row[k])) return false; + continue; + } + const expected = (v && typeof v === 'object' && '$eq' in v) ? v.$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.findOn[object] = (reads.findOn[object] ?? 0) + 1; + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)).map(copy); + }, + async findOne(object: string, ast: any) { + reads.findOneOn[object] = (reads.findOneOn[object] ?? 0) + 1; + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return copy(r); + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return copy(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 copy(updated); + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + return id && storeFor(object).has(id) ? this.update(object, id, data) : 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); + const s = storeFor(object); + for (const r of rows) s.set(r.id as string, { ...s.get(r.id as string), ...data, id: r.id }); + return rows.length; + }, + async deleteMany(object: string, ast: any) { + const rows = await this.find(object, ast); + for (const r of rows) storeFor(object).delete(r.id as string); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, reads, storeFor }; +} + +/** Deterministic, reversible stand-in for a KMS — `secret` writes need one. */ +const cryptoProvider: any = { + calls: 0, + async encrypt(plain: string) { + cryptoProvider.calls += 1; + return { + id: `sec_${cryptoProvider.calls}`, kmsKeyId: 'test-key', alg: 'test', + version: 1, ciphertext: Buffer.from(plain).toString('base64'), + }; + }, + async decrypt(handle: any) { return Buffer.from(handle.ciphertext, 'base64').toString(); }, + async rotateKey(handle: any) { return handle; }, + digest(plain: string) { return plain; }, +}; + +const OWNER_PACKAGE = 'com.objectstack.test.audit-bound-previous'; + +async function boot() { + const engine = new ObjectQL(); + const stub = makeCountingDriver(); + engine.registerDriver(stub.driver, true); + await engine.init(); + engine.setCryptoProvider(cryptoProvider); + for (const o of [sysAuditLog, sysActivity, sysSecret, sysFile, bizTask]) { + engine.registry.registerObject(o as any, OWNER_PACKAGE); + } + return { engine, ...stub }; +} + +const auditRowsFor = ( + storeFor: (o: string) => Map>, + objectName: string, +) => Array.from(storeFor('sys_audit_log').values()).filter((r) => r.object_name === objectName); + +const rowFor = ( + storeFor: (o: string) => Map>, + objectName: string, + action: string, +) => auditRowsFor(storeFor, objectName).find((r) => r.action === action); + +const parse = (v: unknown): Record => JSON.parse(String(v)); + +/** A committed file the read path would expand, and a fully-populated task. */ +async function seedTask(engine: any) { + await engine.insert('sys_file', { + id: 'file_abc', name: 'a.png', url: '/f/a.png', size: 12, status: 'committed', + }); + return engine.insert('biz_task', { + title: 'Ship it', status: 'todo', + api_key: 'sk-live-abc123', login_pw: 'hunter2', attachment: 'file_abc', + }); +} + +// --------------------------------------------------------------------------- +// 1-3. The read is retired — and audit still sees the prior row +// --------------------------------------------------------------------------- + +describe('[#6656] the plugin issues no pre-image read of its own', () => { + it('single-id update() pays exactly ONE findOne — the engine\'s', async () => { + const { engine, reads } = await boot(); + installAuditWriters(engine as any); + const row: any = await seedTask(engine); + + const before = reads.findOneOn['biz_task'] ?? 0; + await engine.update('biz_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + + // Was 2 (engine + plugin). Restoring `captureBefore` makes this 2 again. + expect((reads.findOneOn['biz_task'] ?? 0) - before).toBe(1); + }); + + it('single-id delete() pays exactly ONE findOne — the engine\'s (bound since #5272)', async () => { + const { engine, reads } = await boot(); + installAuditWriters(engine as any); + const row: any = await seedTask(engine); + + const before = reads.findOneOn['biz_task'] ?? 0; + await engine.delete('biz_task', { where: { id: row.id } } as any); + + expect((reads.findOneOn['biz_task'] ?? 0) - before).toBe(1); + }); + + it('a predicate update() pays ZERO per-row findOne, where it used to pay one per matched row', async () => { + const { engine, reads } = await boot(); + installAuditWriters(engine as any); + for (let i = 0; i < 3; i += 1) await engine.insert('biz_task', { title: `T${i}`, status: 'todo' }); + + const before = reads.findOneOn['biz_task'] ?? 0; + await engine.update('biz_task', { status: 'done' }, { where: { status: 'todo' }, multi: true } as any); + + // Was 3 — one per matched row, every result discarded (`__previous` landed + // on the per-row BEFORE context; the per-row AFTER contexts never saw it). + // The engine's own matched-row `find` is untouched and still serves both + // phases, which is why the ledger below is unchanged. + expect((reads.findOneOn['biz_task'] ?? 0) - before).toBe(0); + }); + + it('a predicate delete() pays ZERO per-row findOne', async () => { + const { engine, reads } = await boot(); + installAuditWriters(engine as any); + for (let i = 0; i < 3; i += 1) await engine.insert('biz_task', { title: `T${i}`, status: 'todo' }); + + const before = reads.findOneOn['biz_task'] ?? 0; + await engine.delete('biz_task', { where: { status: 'todo' }, multi: true } as any); + + expect((reads.findOneOn['biz_task'] ?? 0) - before).toBe(0); + }); + + it('the ledger still records the prior row — the saving is a read, not a blind spot', async () => { + // The other half of every count above. A writer that stopped consuming the + // pre-image would satisfy all four and be a regression, so the two claims + // are asserted on the same runs' worth of behaviour. + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + const row: any = await engine.insert('biz_task', { title: 'Ship it', status: 'todo' }); + + await engine.update('biz_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + expect(parse(rowFor(storeFor, 'biz_task', 'update')!.old_value)).toEqual({ status: 'todo' }); + + await engine.delete('biz_task', { where: { id: row.id } } as any); + expect(parse(rowFor(storeFor, 'biz_task', 'delete')!.old_value)).toMatchObject({ + title: 'Ship it', status: 'in_progress', + }); + + // Predicate writes keep their per-row rows too — one per matched row. + for (let i = 0; i < 3; i += 1) await engine.insert('biz_task', { title: `T${i}`, status: 'todo' }); + await engine.delete('biz_task', { where: { status: 'todo' }, multi: true } as any); + expect(auditRowsFor(storeFor, 'biz_task').filter((r) => r.action === 'delete')).toHaveLength(4); + }); +}); + +// --------------------------------------------------------------------------- +// 4. The compliance face +// --------------------------------------------------------------------------- + +describe('[#6656] single-id delete `old_value` keeps the masked credential view', () => { + it('records `••••••••` for a secret field — byte-identical to before this change', async () => { + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + const row: any = await seedTask(engine); + + await engine.delete('biz_task', { where: { id: row.id } } as any); + + const old = parse(rowFor(storeFor, 'biz_task', 'delete')!.old_value); + // The literal, not just the imported constant: "byte-identical" is a claim + // about the exact bytes the ledger has always carried on this face. + expect(old.api_key).toBe('••••••••'); + expect(old.api_key).toBe(SECRET_MASK); + // The stored ref must not appear under any spelling. + expect(String(rowFor(storeFor, 'biz_task', 'delete')!.old_value)).not.toContain('secret:'); + }); + + it('masks a `password` field the same way — ADR-0100 stores those in cleartext', async () => { + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + const row: any = await seedTask(engine); + + await engine.delete('biz_task', { where: { id: row.id } } as any); + + const raw = String(rowFor(storeFor, 'biz_task', 'delete')!.old_value); + expect(parse(raw).login_pw).toBe('••••••••'); + expect(raw).not.toContain('hunter2'); + }); + + it('an UNSET credential stays null rather than becoming a mask', async () => { + // `maskSecretFields`' own rule, mirrored: "a secret is set" and "there is + // no secret" must stay distinguishable in the ledger. + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + const row: any = await engine.insert('biz_task', { + title: 'No creds', status: 'todo', api_key: null, login_pw: null, + }); + + await engine.delete('biz_task', { where: { id: row.id } } as any); + + const old = parse(rowFor(storeFor, 'biz_task', 'delete')!.old_value); + expect(old.api_key).toBeNull(); + expect(old.login_pw).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// 5-6. The phantom rows — and the real change that must survive +// --------------------------------------------------------------------------- + +describe('[#6656] a write that changes nothing about a field records no diff for it', () => { + it('secret, file-reference and formula fields all stay out of an unrelated update', async () => { + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + const row: any = await seedTask(engine); + + // Payload touches ONE field. + await engine.update('biz_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + + const audit = rowFor(storeFor, 'biz_task', 'update')!; + // The whole diff, not a subset: any extra key is a phantom row. + expect(parse(audit.old_value)).toEqual({ status: 'todo' }); + expect(parse(audit.new_value)).toEqual({ status: 'in_progress' }); + }); + + it('records a REAL credential rotation — masking the values must not delete the trail', async () => { + // The failure mode this case exists to forbid: masking both sides BEFORE + // comparing would make every secret change compare equal and silently + // vanish from the ledger. Detection runs on the raw values; only the + // recorded values are masked, so the row is written and says "it changed" + // without saying to what. + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + const row: any = await seedTask(engine); + + await engine.update('biz_task', { api_key: 'sk-live-rotated' }, { where: { id: row.id } } as any); + + const audit = rowFor(storeFor, 'biz_task', 'update'); + expect(audit, 'a secret rotation must still produce an audit row').toBeDefined(); + expect(parse(audit!.old_value)).toEqual({ api_key: '••••••••' }); + expect(parse(audit!.new_value)).toEqual({ api_key: '••••••••' }); + expect(String(audit!.new_value)).not.toContain('sk-live-rotated'); + }); +}); + +// --------------------------------------------------------------------------- +// 7. The pre-existing leak, closed +// --------------------------------------------------------------------------- + +describe('[#6656] no credential value reaches `new_value` on create or update', () => { + it('create records the mask, not the stored ref and not the cleartext', async () => { + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + await seedTask(engine); + + const created = rowFor(storeFor, 'biz_task', 'create')!; + const raw = String(created.new_value); + expect(parse(raw).api_key).toBe('••••••••'); + expect(parse(raw).login_pw).toBe('••••••••'); + expect(raw).not.toContain('secret:'); // the ref, leaked before this change + expect(raw).not.toContain('hunter2'); // the cleartext, leaked before this change + }); + + it('a snapshot drops the VIRTUAL computed field but keeps the STORED one', async () => { + // Two halves of one rule, and the second is why `VIRTUAL_FIELD_TYPES` is + // narrower than the `COMPUTED_FIELD_TYPES` set `diff()` uses. + // + // - `upper_title` is a `formula`: virtual, hydrated onto `ctx.result` by + // #5504 and structurally absent from the raw pre-image. Leaving it in + // would make create `new_value` describe a field delete `old_value` + // could never carry. + // - `ticket_no` is an `autonumber`: seeded at insert into a real column, + // so it is present and equal on BOTH sides. Dropping it would delete the + // record's human-facing identifier from the ledger to fix an asymmetry + // it never had — which is what keying this limb off the wider set did. + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + const row: any = await seedTask(engine); + + const created = parse(rowFor(storeFor, 'biz_task', 'create')!.new_value); + expect(created).not.toHaveProperty('upper_title'); + expect(created.ticket_no).toBe(row.ticket_no); + expect(created.ticket_no).toBeTruthy(); + + await engine.delete('biz_task', { where: { id: row.id } } as any); + const deleted = parse(rowFor(storeFor, 'biz_task', 'delete')!.old_value); + expect(deleted).not.toHaveProperty('upper_title'); + expect(deleted.ticket_no).toBe(row.ticket_no); + }); + + it('the activity summary never renders a credential either', async () => { + // `sys_activity.metadata` mirrors the diff and the summary is rendered + // verbatim in the record feed and Setup dashboards, so the mask has to + // hold on this face too. + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + const row: any = await seedTask(engine); + + await engine.update('biz_task', { api_key: 'sk-live-rotated' }, { where: { id: row.id } } as any); + + const activity = Array.from(storeFor('sys_activity').values()) + .filter((r) => r.object_name === 'biz_task'); + const blob = JSON.stringify(activity); + expect(blob).not.toContain('sk-live-rotated'); + expect(blob).not.toContain('secret:'); + expect(blob).not.toContain('hunter2'); + }); +}); + +// --------------------------------------------------------------------------- +// 8. Non-mutating +// --------------------------------------------------------------------------- + +describe('[#6656] normalising the ledger view does not rewrite the record', () => { + it('the value returned to the caller keeps its real field values', async () => { + // `ctx.result` IS the record the caller receives. Masking it in place would + // hand every audited writer a masked row back — and on a driver that + // returns live references, would mask the store itself. + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + const created: any = await seedTask(engine); + + expect(created.api_key).toMatch(/^secret:/); + expect(created.login_pw).toBe('hunter2'); + + const updated: any = await engine.update( + 'biz_task', { status: 'in_progress' }, { where: { id: created.id } } as any, + ); + expect(updated.login_pw).toBe('hunter2'); + + // …and the row still on disk is untouched. + expect(storeFor('biz_task').get(created.id)!.login_pw).toBe('hunter2'); + + // The ledger, on the same run, holds the masked view — so the two are + // genuinely different objects rather than one that was never masked. + expect(parse(rowFor(storeFor, 'biz_task', 'create')!.new_value).login_pw).toBe('••••••••'); + }); +}); 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 index ffdb3657e2..37363e1290 100644 --- a/packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts +++ b/packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts @@ -300,15 +300,27 @@ describe('[#5860] a SKIP_OBJECTS object no longer forces the prior-row read', () // --------------------------------------------------------------------------- describe('[#5860] an audited object keeps its read and its ledger row', () => { - it('the gate stays open for a business object', async () => { + it('the gate stays open for a business object — on the AFTER terms (#6656)', 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); + + // [#6656] These two were `true` when this case was written, because + // `captureBefore` was registered on them. It is retired, so plugin-audit + // now declares NO before-phase hook for any object. + // + // That is deliberately not a weakening of #5860: both demand gates are + // `hasHooksFor(before*) || hasHooksFor(after*) || getSummaryDescriptors()` + // (`engine.ts:6992` for update, `:7857` for delete), so the AFTER terms + // asserted above hold them open and the engine still buys the one + // pre-image read `writeAudit` consumes. The case below measures exactly + // that, which is what keeps this pair of expectations from silently + // meaning "audit stopped seeing the prior row". + expect(gateOpen(engine, 'beforeUpdate', 'biz_task')).toBe(false); + expect(gateOpen(engine, 'beforeDelete', 'biz_task')).toBe(false); }); it('single-id update() on `biz_task` still reads the prior row and audits the diff', async () => { @@ -397,11 +409,39 @@ function makeRecordingEngine() { return { engine, registrations, created, api }; } -/** The five writer registrations: global minus the skip list. */ -const AUDIT_WRITER_EVENTS = ['beforeUpdate', 'beforeDelete', 'afterInsert', 'afterUpdate', 'afterDelete']; +/** + * The writer registrations: global minus the skip list. + * + * [#6656] Was five. `captureBefore`'s `beforeUpdate` / `beforeDelete` pair is + * retired — the engine binds `previous` before every `before*` dispatch — so + * three remain. #5860's property is unchanged and is what this case still + * measures: every registration the plugin DOES declare carries the exclusion + * on its registration face, and none of them narrows the allow half. + */ +const AUDIT_WRITER_EVENTS = ['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', () => { + it('plugin-audit declares NO `beforeUpdate` / `beforeDelete` hook (#6656)', () => { + const { engine, registrations } = makeRecordingEngine(); + installAuditWriters(engine); + + // The retirement, asserted on the declaration itself rather than inferred + // from a read count — this is the face `hasHooksFor` reads, so it is what + // decides whether the engine's per-row bulk dispatch runs at all. + // + // Scoped to the two events `captureBefore` held. The plugin's OTHER + // before-phase registrations are unrelated capability gates on a single + // named object each (`beforeInsert` on `sys_comment` for `enable.feeds`, + // on `sys_attachment` for `enable.files`); they read no prior row, and + // asserting "no before-phase hook at all" would fail on them while + // measuring nothing about this card. + const preImageEvents = registrations + .map((r) => r.event) + .filter((e) => e === 'beforeUpdate' || e === 'beforeDelete'); + expect(preImageEvents).toEqual([]); + }); + + it('all writer registrations carry `excludeObjects` and stay global otherwise', () => { const { engine, registrations } = makeRecordingEngine(); installAuditWriters(engine); diff --git a/packages/plugins/plugin-audit/src/audit-writers.test.ts b/packages/plugins/plugin-audit/src/audit-writers.test.ts index 8e23905f29..67e38b50b2 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.test.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.test.ts @@ -158,7 +158,7 @@ describe('audit writers — actor attribution (ADR-0014 D2, cloud#340)', () => { await fire('afterDelete', { object: 'sys_environment', input: { id: 'os-790m7q' }, - __previous: { id: 'os-790m7q', name: 'test' }, + previous: { id: 'os-790m7q', name: 'test' }, result: { id: 'os-790m7q' }, session: { actor: 'svc:cloud-control' }, }); @@ -196,7 +196,7 @@ describe('audit writers — actor attribution (ADR-0014 D2, cloud#340)', () => { await fire('afterUpdate', { object: 'sys_member', input: { id: 'mem-1' }, - __previous: { id: 'mem-1', role: 'member' }, + previous: { id: 'mem-1', role: 'member' }, result: { id: 'mem-1', role: 'admin' }, // Exactly the envelope `withSystemContext` produces for an // `organization/update-member-role` call. @@ -268,7 +268,7 @@ describe('audit writers — operational plumbing is excluded (#5193, ADR-0057 D5 'afterUpdate', { input: { id: 'msg-1', status: 'running' }, - __previous: { id: 'msg-1', queue: 'email_delivery', status: 'pending', attempts: 0 }, + previous: { id: 'msg-1', queue: 'email_delivery', status: 'pending', attempts: 0 }, result: { id: 'msg-1', queue: 'email_delivery', status: 'running', attempts: 1, locked_by: 'worker-1' }, }, ], @@ -276,7 +276,7 @@ describe('audit writers — operational plumbing is excluded (#5193, ADR-0057 D5 'afterUpdate', { input: { id: 'msg-1', status: 'completed' }, - __previous: { id: 'msg-1', queue: 'email_delivery', status: 'running', attempts: 1, locked_by: 'worker-1' }, + previous: { id: 'msg-1', queue: 'email_delivery', status: 'running', attempts: 1, locked_by: 'worker-1' }, result: { id: 'msg-1', queue: 'email_delivery', status: 'completed', attempts: 1, completed_at: '2026-08-04T00:00:00.000Z' }, }, ], @@ -284,7 +284,7 @@ describe('audit writers — operational plumbing is excluded (#5193, ADR-0057 D5 'afterDelete', { input: { id: 'msg-1' }, - __previous: { id: 'msg-1', queue: 'email_delivery', status: 'completed', attempts: 1 }, + previous: { id: 'msg-1', queue: 'email_delivery', status: 'completed', attempts: 1 }, result: { id: 'msg-1' }, }, ], @@ -318,7 +318,19 @@ describe('audit writers — operational plumbing is excluded (#5193, ADR-0057 D5 } }); - it('does not pay the beforeUpdate snapshot read for a skipped object', async () => { + it('pays no before-phase snapshot read — for the skipped object OR the business one (#6656)', async () => { + // [#6656] This case used to assert the saving for `sys_job_queue` only, + // with `crm_lead` as the CONTROL that still paid — because #5860 could + // only narrow the scope of a read the plugin was still issuing. The read + // itself is now retired for every object (the engine binds `previous` + // before each `before*` dispatch), so the control's direction inverts: the + // business object must pay nothing either. + // + // The case therefore still bites, and on a strictly larger surface — + // restoring `captureBefore` turns it red on `crm_lead` where the old + // version REQUIRED that read. What replaces the control is the two cases + // above: `crm_lead` still reaches the ledger, so this zero is a read that + // disappeared, not a hook that stopped running. const { engine, fire } = makeEngine(SCHEMA); installAuditWriters(engine as any, 'test.audit'); const reads: string[] = []; @@ -329,15 +341,11 @@ describe('audit writers — operational plumbing is excluded (#5193, ADR-0057 D5 }, }; - // Every queue state transition would otherwise re-read its own row… await fire('beforeUpdate', { object: 'sys_job_queue', input: { id: 'msg-1', status: 'running' }, ql }); await fire('beforeDelete', { object: 'sys_job_queue', input: { id: 'msg-1' }, ql }); - expect(reads).toEqual([]); - - // …and the control proves the assertion above can fail: a business object - // on the same harness DOES get snapshotted. await fire('beforeUpdate', { object: 'crm_lead', input: { id: 'lead-1', name: 'Acme' }, ql }); - expect(reads).toEqual(['crm_lead']); + await fire('beforeDelete', { object: 'crm_lead', input: { id: 'lead-1' }, ql }); + expect(reads).toEqual([]); }); it('still audits ordinary business writes (the skip stays narrow)', async () => { @@ -402,7 +410,7 @@ describe('audit writers — chunked upload sessions are excluded (#5202, ADR-005 'afterUpdate', { input: snapshot(n, 'in_progress'), - __previous: snapshot(n - 1, 'in_progress'), + previous: snapshot(n - 1, 'in_progress'), result: snapshot(n, 'in_progress'), }, ]); @@ -412,7 +420,7 @@ describe('audit writers — chunked upload sessions are excluded (#5202, ADR-005 'afterUpdate', { input: { id: 'ups-1', status: terminal.status }, - __previous: snapshot(chunks, 'in_progress'), + previous: snapshot(chunks, 'in_progress'), result: snapshot(chunks, terminal.status), }, ]); @@ -420,7 +428,7 @@ describe('audit writers — chunked upload sessions are excluded (#5202, ADR-005 if (terminal.via === 'afterDelete') { writes.push([ 'afterDelete', - { input: { id: 'ups-1' }, __previous: snapshot(chunks, terminal.status), result: { id: 'ups-1' } }, + { input: { id: 'ups-1' }, previous: snapshot(chunks, terminal.status), result: { id: 'ups-1' } }, ]); } return writes; @@ -473,12 +481,12 @@ describe('audit writers — chunked upload sessions are excluded (#5202, ADR-005 await fire('beforeUpdate', { object: 'sys_upload_session', input: { id: 'ups-1', uploaded_chunks: n }, ql }); } await fire('beforeDelete', { object: 'sys_upload_session', input: { id: 'ups-1' }, ql }); - expect(reads).toEqual([]); - - // Control — the assertion above can fail: a business object on the same - // harness DOES get snapshotted. + // [#6656] `crm_lead` was the control that still paid this read. The read is + // retired for every object now, so it joins the assertion instead of + // opposing it — see the twin case in the `sys_job_queue` group for why + // that makes the pin stronger rather than weaker. await fire('beforeUpdate', { object: 'crm_lead', input: { id: 'lead-1', name: 'Acme' }, ql }); - expect(reads).toEqual(['crm_lead']); + expect(reads).toEqual([]); }); it('still audits sys_file — mostly permanent business truth, deliberately NOT exempted', async () => { @@ -541,7 +549,7 @@ describe('audit writers — declarative trackHistory activity (ADR-0052 §5b)', object: 'crm_opportunity', input: { id: 'opp-1', stage: 'closed_won' }, result: { id: 'opp-1', name: 'Acme Renewal', stage: 'closed_won' }, - __previous: { id: 'opp-1', name: 'Acme Renewal', stage: 'proposal' }, + previous: { id: 'opp-1', name: 'Acme Renewal', stage: 'proposal' }, session: {}, }); @@ -558,7 +566,7 @@ describe('audit writers — declarative trackHistory activity (ADR-0052 §5b)', object: 'crm_opportunity', input: { id: 'opp-1', amount: 200 }, result: { id: 'opp-1', name: 'Acme Renewal', amount: 200, stage: 'proposal' }, - __previous: { id: 'opp-1', name: 'Acme Renewal', amount: 100, stage: 'proposal' }, + previous: { id: 'opp-1', name: 'Acme Renewal', amount: 100, stage: 'proposal' }, session: {}, }); @@ -603,7 +611,7 @@ describe('audit writers — declarative milestones (ADR-0052 §5b.2)', () => { object: 'crm_opportunity', input: { id: 'opp-1', stage: 'closed_won' }, result: { id: 'opp-1', name: 'Acme Renewal', stage: 'closed_won' }, - __previous: { id: 'opp-1', name: 'Acme Renewal', stage: 'negotiation' }, + previous: { id: 'opp-1', name: 'Acme Renewal', stage: 'negotiation' }, session: {}, }); @@ -621,7 +629,7 @@ describe('audit writers — declarative milestones (ADR-0052 §5b.2)', () => { object: 'crm_opportunity', input: { id: 'opp-1', stage: 'negotiation' }, result: { id: 'opp-1', name: 'Acme Renewal', stage: 'negotiation' }, - __previous: { id: 'opp-1', name: 'Acme Renewal', stage: 'proposal' }, + previous: { id: 'opp-1', name: 'Acme Renewal', stage: 'proposal' }, session: {}, }); @@ -804,7 +812,7 @@ describe('audit writers — update diff hygiene (objectui detail-history report) object: 'gantt_plan', input: { id: 'p-1', plan_start: '2026-08-04T12:00:00.000Z' }, // before: query-path snapshot carries the computed formula value… - __previous: { id: 'p-1', name: 'Plan C', plan_start: '2026-07-26T00:00:00.000Z', deps_rendered: ['LnLJIsTwXbv1E2gF'] }, + previous: { id: 'p-1', name: 'Plan C', plan_start: '2026-07-26T00:00:00.000Z', deps_rendered: ['LnLJIsTwXbv1E2gF'] }, // …after: raw write result does not. result: { id: 'p-1', name: 'Plan C', plan_start: '2026-08-04T12:00:00.000Z' }, session: { userId: 'user-1' }, @@ -827,7 +835,7 @@ describe('audit writers — update diff hygiene (objectui detail-history report) await fire('afterUpdate', { object: 'gantt_plan', input: { id: 'p-1' }, - __previous: { id: 'p-1', name: 'Plan C', deps_rendered: ['LnLJIsTwXbv1E2gF'] }, + previous: { id: 'p-1', name: 'Plan C', deps_rendered: ['LnLJIsTwXbv1E2gF'] }, result: { id: 'p-1', name: 'Plan C' }, session: { userId: 'user-1' }, }); @@ -844,7 +852,7 @@ describe('audit writers — update diff hygiene (objectui detail-history report) object: 'gantt_plan', input: { id: 'p-1', plan_start: null }, // `plan_start` key absent before, explicit null after: not a change. - __previous: { id: 'p-1', name: 'Plan C' }, + previous: { id: 'p-1', name: 'Plan C' }, result: { id: 'p-1', name: 'Plan C', plan_start: null }, session: { userId: 'user-1' }, }); @@ -859,7 +867,7 @@ describe('audit writers — update diff hygiene (objectui detail-history report) await fire('afterUpdate', { object: 'gantt_plan', input: { id: 'p-1', plan_start: null }, - __previous: { id: 'p-1', name: 'Plan C', plan_start: '2026-07-26T00:00:00.000Z' }, + previous: { id: 'p-1', name: 'Plan C', plan_start: '2026-07-26T00:00:00.000Z' }, result: { id: 'p-1', name: 'Plan C', plan_start: null }, session: { userId: 'user-1' }, }); @@ -918,7 +926,7 @@ describe('audit writers — localized activity summaries (framework#3039)', () = const { fire, created } = setup('zh-CN', makeI18n()); await fire('afterInsert', insertCtx()); - await fire('afterDelete', { ...insertCtx(), result: null, __previous: { id: 'q-1', name: 'OC-00001' } }); + await fire('afterDelete', { ...insertCtx(), result: null, previous: { id: 'q-1', name: 'OC-00001' } }); const summaries = created.filter((c) => c.object === 'sys_activity').map((c) => c.row.summary); expect(summaries).toEqual(['创建了 人员资质 "OC-00001"', '删除了 人员资质 "OC-00001"']); @@ -928,7 +936,7 @@ describe('audit writers — localized activity summaries (framework#3039)', () = const { fire, created } = setup('zh-CN', makeI18n()); await fire('afterUpdate', { ...insertCtx(), - __previous: { id: 'q-1', name: 'OC-00001', status: 'draft' }, + previous: { id: 'q-1', name: 'OC-00001', status: 'draft' }, result: { id: 'q-1', name: 'OC-00001', status: 'active' }, }); const activity = created.find((c) => c.object === 'sys_activity'); diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index c70ef32294..d980d2ea51 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -2,6 +2,16 @@ import type { HookContext } from '@objectstack/spec/data'; import type { IDataEngine } from '@objectstack/spec/contracts'; +// [#6656] The read-mask CONTRACT, imported rather than re-typed. `SECRET_MASK` +// is the exact string the engine's read path substitutes and +// `collectMaskedReadFields` is the exact predicate that picks the fields +// (`secret` always; `password` unless the object is `managedBy: 'better-auth'` +// — ADR-0100). A hand-copied `'••••••••'` or a re-typed "is it secret?" test +// here would be a second de-facto contract that drifts from the engine's by one +// character and leaks on the day it does; `secret-fields.ts` makes the same +// argument for its own single definition. The `/core` subpath is the +// engine-free surface of the same package. +import { SECRET_MASK, collectMaskedReadFields } from '@objectstack/objectql/core'; /** * Minimal structural view of `NotificationService.emit` (ADR-0030). Declared @@ -216,9 +226,45 @@ function recordLabel(record: any, id: string): string { * exclusion is kept on its own merit — the derived-is-implied reason above — * and it is keyed on the field TYPE from `fieldDefs`, never on a key being * missing, which is why the fix one layer down did not disturb it. + * + * [#6656] Since the pre-image became the engine's raw row, `before` no longer + * carries formula keys at all while `after` still does (#5504's write-path + * hydration) — so the asymmetry is back on the SNAPSHOT faces, which this set + * never reached. `ledgerView(..., { dropComputed: true })` now applies the same + * rule to create `new_value` and delete `old_value`; this set is the single + * definition both faces read. */ const COMPUTED_FIELD_TYPES = new Set(['formula', 'summary', 'rollup', 'autonumber', 'auto_number']); +/** + * [#6656] Field types that are **virtual** — no driver ever returns a column + * for one. Dropped from the FULL SNAPSHOTS (create `new_value`, delete + * `old_value`), which is a different job from {@link COMPUTED_FIELD_TYPES} + * above, on a deliberately different set. + * + * Why a snapshot needs the rule at all: `ctx.result` hydrates formulas onto + * what a write hands back (#5504) while the pre-image is the engine's RAW row, + * which structurally cannot carry them. Left in, create `new_value` would + * describe formula fields and delete `old_value` could not — the two snapshot + * faces disagreeing about their own vocabulary. + * + * ⚠️ Why it is NARROWER than `COMPUTED_FIELD_TYPES`, and must stay so. Of that + * set only `formula` is virtual — "formulas are virtual: no driver ever returns + * a column for one" (`engine.ts`, #5504). The rest are ordinary **stored** + * columns the engine maintains: `autonumber` is seeded at insert + * (`seedAutonumber`), and a `summary` roll-up is written to the parent row by + * `recomputeSummaries` and seeded at create time (#5749/#6063). They are + * present on BOTH sides and symmetric, so dropping them from a snapshot would + * delete real ledger content — the record's human-facing number, and its + * roll-up values — to fix an asymmetry they never had. + * + * The wider set stays correct for `diff()`, whose rule is a different one: a + * derived value's CHANGE is already implied by the source fields that produced + * it. That is about change reporting; this is about what a snapshot can + * contain. Do not unify them. + */ +const VIRTUAL_FIELD_TYPES = new Set(['formula']); + /** * Compute a shallow JSON diff between two records. Returns only keys whose * value changed (and ignores keys in `NOISE_FIELDS` plus computed field @@ -532,46 +578,127 @@ export function installAuditWriters( }; /** - * beforeUpdate / beforeDelete: capture "previous" snapshot via api.sudo() - * so we can compute the diff in the afterXxx hook. We attach the snapshot - * to the context (`(ctx as any).__previous`) since `HookContext.previous` - * is officially typed but not always populated by the engine itself. + * [#6656, Option A+] The view the ledger records for one record. + * + * Retiring `captureBefore` (below) makes both sides of every diff come from + * the SAME pipeline — the raw driver row — where before they came from two: + * `before` through the engine's read path (masked, formula-hydrated, + * file-references resolved) and `after` from the raw write result. That + * asymmetry, not the redundant read, was the root cause of the phantom diff + * rows. Same-source is therefore delivered by the retirement itself; this + * function delivers the second half — same *view* — so levelling the two + * sides levels them UPWARD rather than down to the raw store contents. + * + * Two limbs, and only two, because only two field classes still differ once + * both sides are raw (each measured, not assumed): + * + * 1. **credential fields** (`secret`, and `password` off better-auth + * objects). The raw value is a `secret:` ref, or — for `password`, which + * ADR-0100 stores PLAINTEXT at rest — the cleartext itself. Masked here + * to exactly what the read path substitutes, on BOTH sides. This is the + * compliance face the #6656 ruling protects: single-id delete + * `old_value` keeps reading `••••••••`, byte-identical to before this + * change, and the ref/cleartext that today reaches `new_value` on every + * create and update stops doing so. + * 2. **virtual fields** (`VIRTUAL_FIELD_TYPES` — `formula` only) — dropped + * from FULL SNAPSHOTS. `ctx.result` carries hydrated formulas (#5504) so + * `new_value` had them; the raw pre-image has no such column, so + * `old_value` structurally could not — a fresh asymmetry that the very + * change removing the old one would otherwise introduce. Deliberately + * NARROWER than the `COMPUTED_FIELD_TYPES` set `diff()` uses: see that + * constant's note for why `autonumber` and `summary` are stored, + * symmetric, and must stay in the snapshot. + * + * **File-reference fields need no limb, and that is a measurement rather + * than an omission.** They are named in the ruling because today's `before` + * carries the resolved `{id, name, size, url}` object against a raw id on + * the `after` side. Once the pre-image is the engine's, both sides hold the + * stored id token and agree by construction. Writing an "if it looks + * resolved, take `.id`" limb would be a consumer-side tolerance for a + * producer that no longer exists (PD #12) — and it could never be shown to + * go red, because no path reaches it. + * + * Non-mutating on purpose: `ctx.previous` and `ctx.result` are the engine's + * own objects — `result` is the record the caller gets back — and some + * drivers hand out live store references. Masking in place would rewrite the + * record, and on such a driver the store itself. (That aliasing is real: it + * contaminated the first probe run on this card, making the two views look + * identical.) + * + * @param dropComputed pass `true` for full snapshots (create `new_value`, + * delete `old_value`); `false` for diff output, which `diff()` has already + * filtered, and for the label source, where a formula `name` is the point. */ - const captureBefore = async (ctx: HookContext) => { - if (SKIP_OBJECTS.has(ctx.object)) return; - const id = (ctx.input as any)?.id; - if (!id) return; // bulk update/delete — too costly to snapshot every row here - try { - // Use the engine directly (not api.sudo) so we can thread the - // active transaction through. On drivers with single-connection - // pools (e.g. SQLite via knex) a sudo() findOne that does NOT - // carry the open transaction will deadlock for the full - // acquireConnectionTimeout (~60s) because the outer transaction - // holds the only connection. - const trx = (ctx as any).transaction; - const ql = (ctx as any).ql ?? (ctx as any).api?.engine; - if (ql?.findOne) { - const prev = await ql.findOne(ctx.object, { - where: { id }, - context: { isSystem: true, ...(trx ? { transaction: trx } : {}) }, - }); - if (prev) (ctx as any).__previous = prev; - return; + const ledgerView = ( + objectName: string, + record: any, + { dropComputed }: { dropComputed: boolean }, + ): Record | null => { + if (!record || typeof record !== 'object') return null; + const out: Record = { ...record }; + for (const field of collectMaskedReadFields(getObjectDef(objectName))) { + // `field in out` and the null-preserving branch both mirror + // `maskSecretFields` exactly: an unset credential stays `null` (so "no + // secret" and "a secret is set" remain distinguishable), and a field the + // row does not carry is never invented. + if (!(field in out)) continue; + out[field] = out[field] == null ? null : SECRET_MASK; + } + if (dropComputed) { + const defs = getFieldDefs(objectName); + for (const key of Object.keys(out)) { + const type = defs?.[key]?.type; + if (typeof type === 'string' && VIRTUAL_FIELD_TYPES.has(type)) delete out[key]; } - const api: any = (ctx as any).api; - if (!api?.sudo) return; - const prev = await api.sudo().object(ctx.object).findOne({ where: { id } }); - if (prev) (ctx as any).__previous = prev; - } catch { - /* ignore — best-effort */ } + return out; }; - // [#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 }); + /** + * ⛔ RETIRED — `captureBefore` on `beforeUpdate` / `beforeDelete` (#6656, + * ADR-0049 enforce-or-remove). Do not reintroduce it. + * + * It existed because `HookContext.previous` was "officially typed but not + * always populated by the engine itself". That is no longer true on any + * path this plugin registers for, and it was verified in code rather than + * taken from the card (re-measured on `97b079896`; the anchors below moved + * from the ones #5846 recorded, so they are restated, not copied): + * + * single-id update `engine.ts:7012` dispatch, bound `:7010` under the + * `wantsPriorRecord` gate at `:6992` + * single-id delete `:7899` dispatch, bound `:7896`/`:7897` + * (`readPreImage` → `bindPreImage`) under `:7857` + * predicate update `:7084` → `dispatchPerRowBeforeHooks`, bound per row `:1825` + * predicate delete `:7962` → `dispatchPerRowBeforeHooks`, bound per row `:1825` + * + * and the after phase is bound too — `buildPerRowAfterContexts` (`:1746`) + * binds `previous` on every per-row `afterUpdate` / `afterDelete` context, + * which is the one `writeAudit` actually reads. + * + * Each demand gate is the SAME predicate as its dispatch gate + * (`hasHooksFor(before*) || hasHooksFor(after*) || getSummaryDescriptors()`), + * so there is no path on which an audit hook runs with `previous` unbound. + * `writeAudit` stays registered on `afterUpdate`/`afterDelete`, so the gates + * stay open on the after-hook term and the engine still buys the one read. + * + * What the retirement removes, measured with a counting driver on this + * branch point (`driver.findOne` on the audited object, per write): + * + * single-id update 2 → 1 single-id delete 2 → 1 + * predicate update 3 → 0 predicate delete 3 → 0 (3 matched rows) + * + * The predicate column is the larger half and it was pure waste: #5574 binds + * `input.id` on every per-row *before* context, which defeated the + * `if (!id) return` bulk guard this handler opened with, so it read every + * row — and every result was discarded, because `__previous` landed on the + * per-row *before* context while the per-row *after* contexts never saw it. + * + * Retired rather than left as a no-op registration: a hook whose only + * remaining effect is holding a demand gate open is exactly what + * `sys_fetch_previous_delete` had become when #5929 removed it from + * objectql's own plugin — reproducing that shape here, one package over, + * would re-create the defect the engine lane just closed. + */ /** * afterInsert / afterUpdate / afterDelete: write audit_log + activity rows. @@ -586,8 +713,20 @@ export function installAuditWriters( const api: any = (ctx as any).api; if (!api?.sudo) return; + // [#6656] Both sides, RAW — the engine's write result and the engine's + // bound pre-image, now one pipeline. These two are what CHANGE DETECTION + // reads (`diff`, `matchMilestone`); what the ledger RECORDS is their + // `ledgerView`. Keeping those roles apart is what lets a credential + // rotation still produce a row — the values compare unequal while both + // render as the mask — instead of masking first and thereby deleting the + // audit trail of every secret change along with the phantom ones. + // + // ⛔ Read `previous` unconditionally. It is a contract value; do NOT gate + // it on `ctx.input.options.multi` or otherwise re-derive the engine's + // dispatch ladder here (`asScalarId` is unexported for exactly this + // reason — #4434 / #4550, restated in the #6656 ruling). const after: any = ctx.result; - const before: any = (ctx as any).__previous ?? (ctx as any).previous ?? null; + const before: any = (ctx as any).previous ?? null; // Resolve record id from after (insert/update) or before (delete) or input. let recordId: string | undefined = @@ -639,22 +778,25 @@ export function installAuditWriters( // though writes succeed. const recordOrgId: string | undefined = (typeof (ctx.result as any)?.organization_id === 'string' && (ctx.result as any).organization_id) || - (typeof ((ctx as any).__previous as any)?.organization_id === 'string' && ((ctx as any).__previous as any).organization_id) || + (typeof before?.organization_id === 'string' && before.organization_id) || undefined; const tenantId: string | undefined = sess.tenantId ?? recordOrgId; let oldValue: Record | null = null; let newValue: Record | null = null; if (action === 'create') { - newValue = (after && typeof after === 'object') ? { ...after } : null; + newValue = ledgerView(ctx.object, after, { dropComputed: true }); } else if (action === 'update') { + // Detect on the raw values, record the masked ones — see the note on + // `before`/`after` above. `diff` has already dropped computed fields, so + // its output needs no second pass for them. const d = diff(before || {}, after || {}, getFieldDefs(ctx.object)); - oldValue = d.old; - newValue = d.next; // If nothing meaningfully changed, skip the audit row to avoid noise. - if (Object.keys(newValue).length === 0) return; + if (Object.keys(d.next).length === 0) return; + oldValue = ledgerView(ctx.object, d.old, { dropComputed: false }); + newValue = ledgerView(ctx.object, d.next, { dropComputed: false }); } else if (action === 'delete') { - oldValue = before && typeof before === 'object' ? { ...before } : null; + oldValue = ledgerView(ctx.object, before, { dropComputed: true }); } const auditRow: Record = { @@ -685,7 +827,16 @@ export function installAuditWriters( auditRow.actor = actorLabel; } - const label = recordLabel(after ?? before, recordId ?? ''); + // [#6656] Masked, but computed fields KEPT: `recordLabel` reads + // `name`/`title`/… and an object whose label field is a formula would + // otherwise degrade to the bare id (#5504 names that exact symptom). The + // mask still applies, so no credential value can reach a user-facing + // activity summary through the label. + const label = recordLabel( + ledgerView(ctx.object, after, { dropComputed: false }) ?? + ledgerView(ctx.object, before, { dropComputed: false }), + recordId ?? '', + ); // Summaries are user-facing (the record Discussion feed and Setup // dashboards render them verbatim), so name the object by its display // label ("Semantic Zoo"), not its API name ("showcase_semantic_zoo"), and @@ -709,7 +860,20 @@ export function installAuditWriters( // ADR-0052 §5b — declarative activity, precedence: a configured semantic // milestone (§5b.2) wins; else a tracked field-change diff ("Stage: // Proposal → Closed Won", §5b.1); else the generic fallback. - const milestone = matchMilestone(getObjectDef(ctx.object), getFieldDefs(ctx.object), before, after); + // [#6656] Masked views, not the raw pair. `matchMilestone` interpolates + // `{field}` from the after-row straight into a summary the record feed + // and Setup dashboards render verbatim, so it must never see a raw + // credential value. Computed fields are kept (a milestone may key on a + // formula); detection is unaffected for every other class, since the + // mask touches credential fields only — and a milestone whose declared + // `value` is a secret's plaintext would be a leak in the metadata + // itself, not a case worth preserving. + const milestone = matchMilestone( + getObjectDef(ctx.object), + getFieldDefs(ctx.object), + ledgerView(ctx.object, before, { dropComputed: false }), + ledgerView(ctx.object, after, { dropComputed: false }), + ); if (milestone) { summary = milestone.summary; if (milestone.type) activityType = milestone.type; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8cbc0524b0..981971c9df 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1387,6 +1387,9 @@ importers: '@objectstack/core': specifier: workspace:* version: link:../../core + '@objectstack/objectql': + specifier: workspace:* + version: link:../../objectql '@objectstack/platform-objects': specifier: workspace:* version: link:../../platform-objects @@ -1394,9 +1397,6 @@ importers: specifier: workspace:* version: link:../../spec devDependencies: - '@objectstack/objectql': - specifier: workspace:* - version: link:../../objectql '@types/node': specifier: ^26.1.2 version: 26.1.2