From b7e3788570ed096128093d171018baeb53845e34 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Sun, 9 Aug 2026 07:15:22 +0000 Subject: [PATCH 1/2] fix(plugin-audit): consume the engine's bound ctx.previous and normalise both sides of the diff (#6656) Retires captureBefore's own pre-image read (the engine binds `previous` before every before* dispatch, under the same predicate that dispatches the hook), and masks the credential field classes on both sides of the audit diff so the two sides share one view as well as one source. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ --- .changeset/audit-consume-bound-previous.md | 63 +++ packages/plugins/plugin-audit/package.json | 4 +- .../src/audit-bound-previous.test.ts | 521 ++++++++++++++++++ .../src/audit-hook-object-scope.test.ts | 52 +- .../plugin-audit/src/audit-writers.test.ts | 68 ++- .../plugins/plugin-audit/src/audit-writers.ts | 224 ++++++-- pnpm-lock.yaml | 6 +- 7 files changed, 853 insertions(+), 85 deletions(-) create mode 100644 .changeset/audit-consume-bound-previous.md create mode 100644 packages/plugins/plugin-audit/src/audit-bound-previous.test.ts diff --git a/.changeset/audit-consume-bound-previous.md b/.changeset/audit-consume-bound-previous.md new file mode 100644 index 0000000000..aa65a85703 --- /dev/null +++ b/.changeset/audit-consume-bound-previous.md @@ -0,0 +1,63 @@ +--- +"@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. +- **Computed fields leave the full snapshots.** `diff()` has always skipped + them; create `new_value` and delete `old_value` never got the same rule, so + they would have disagreed with each other once the pre-image stopped carrying + hydrated formulas. + +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..c982bfd1da --- /dev/null +++ b/packages/plugins/plugin-audit/src/audit-bound-previous.test.ts @@ -0,0 +1,521 @@ +// 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 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 create snapshot carries no computed field — the diff face never did either', async () => { + // `ctx.result` hydrates formulas since #5504 while the raw pre-image has no + // such column, so leaving them in would make the create and delete + // snapshots disagree with each other and with `diff()`. + const { engine, storeFor } = await boot(); + installAuditWriters(engine as any); + await seedTask(engine); + + expect(parse(rowFor(storeFor, 'biz_task', 'create')!.new_value)).not.toHaveProperty('upper_title'); + }); + + 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..14793c44a6 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,6 +226,13 @@ 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']); @@ -532,46 +549,128 @@ 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. **computed fields** (`COMPUTED_FIELD_TYPES`) — dropped from FULL + * SNAPSHOTS only. `diff()` has always skipped them, so this merely + * extends one existing rule to the create/delete faces, which never got + * it. Without it the two snapshot faces would disagree with each other + * and with the diff: `ctx.result` carries hydrated formulas (#5504) so + * `new_value` had them, the raw pre-image has no such column so + * `old_value` would not — a fresh asymmetry introduced by the very + * change removing the old one. Bulk delete already wrote snapshots + * without them. + * + * **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' && COMPUTED_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 +685,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 +750,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 +799,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 +832,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 From d2b5372094e5398142f7c8b8c7af2fa272a0c183 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Sun, 9 Aug 2026 07:22:43 +0000 Subject: [PATCH 2/2] fix(plugin-audit): drop only VIRTUAL computed fields from audit snapshots (#6656) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverse verification of the snapshot-normalisation limb showed it could only be made red on the create face. Chasing why exposed an over-reach: the limb keyed off `COMPUTED_FIELD_TYPES`, but of that set only `formula` is virtual. Both `autonumber` (seeded at insert) and `summary` (written by `recomputeSummaries`) are stored columns, present and equal on BOTH sides — dropping them deleted the record's human-facing number and its roll-up values from the ledger to fix an asymmetry they never had. Narrowed to a new `VIRTUAL_FIELD_TYPES` set, documented against the wider one so the two are not unified later, and pinned in both directions: the fixture now carries a stored `autonumber` alongside the `formula`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ --- .changeset/audit-consume-bound-previous.md | 10 ++-- .../src/audit-bound-previous.test.ts | 32 ++++++++++--- .../plugins/plugin-audit/src/audit-writers.ts | 48 +++++++++++++++---- 3 files changed, 70 insertions(+), 20 deletions(-) diff --git a/.changeset/audit-consume-bound-previous.md b/.changeset/audit-consume-bound-previous.md index aa65a85703..0bff3c7a4f 100644 --- a/.changeset/audit-consume-bound-previous.md +++ b/.changeset/audit-consume-bound-previous.md @@ -47,10 +47,12 @@ upward rather than down to raw store contents: `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. -- **Computed fields leave the full snapshots.** `diff()` has always skipped - them; create `new_value` and delete `old_value` never got the same rule, so - they would have disagreed with each other once the pre-image stopped carrying - hydrated formulas. +- **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 diff --git a/packages/plugins/plugin-audit/src/audit-bound-previous.test.ts b/packages/plugins/plugin-audit/src/audit-bound-previous.test.ts index c982bfd1da..935011e8d7 100644 --- a/packages/plugins/plugin-audit/src/audit-bound-previous.test.ts +++ b/packages/plugins/plugin-audit/src/audit-bound-previous.test.ts @@ -121,6 +121,10 @@ const bizTask = { 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}' }), }, }; @@ -460,15 +464,31 @@ describe('[#6656] no credential value reaches `new_value` on create or update', expect(raw).not.toContain('hunter2'); // the cleartext, leaked before this change }); - it('a create snapshot carries no computed field — the diff face never did either', async () => { - // `ctx.result` hydrates formulas since #5504 while the raw pre-image has no - // such column, so leaving them in would make the create and delete - // snapshots disagree with each other and with `diff()`. + 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); - await seedTask(engine); + const row: any = await seedTask(engine); - expect(parse(rowFor(storeFor, 'biz_task', 'create')!.new_value)).not.toHaveProperty('upper_title'); + 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 () => { diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index 14793c44a6..d980d2ea51 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -236,6 +236,35 @@ function recordLabel(record: any, id: string): string { */ 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 @@ -571,15 +600,14 @@ export function installAuditWriters( * `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. **computed fields** (`COMPUTED_FIELD_TYPES`) — dropped from FULL - * SNAPSHOTS only. `diff()` has always skipped them, so this merely - * extends one existing rule to the create/delete faces, which never got - * it. Without it the two snapshot faces would disagree with each other - * and with the diff: `ctx.result` carries hydrated formulas (#5504) so - * `new_value` had them, the raw pre-image has no such column so - * `old_value` would not — a fresh asymmetry introduced by the very - * change removing the old one. Bulk delete already wrote snapshots - * without them. + * 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` @@ -620,7 +648,7 @@ export function installAuditWriters( const defs = getFieldDefs(objectName); for (const key of Object.keys(out)) { const type = defs?.[key]?.type; - if (typeof type === 'string' && COMPUTED_FIELD_TYPES.has(type)) delete out[key]; + if (typeof type === 'string' && VIRTUAL_FIELD_TYPES.has(type)) delete out[key]; } } return out;