From 633841efddb7a8438ec4de7f6ee0b63dc0504a41 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 10:00:03 +0000 Subject: [PATCH] fix(plugin-audit): drop record_views' always-empty ip_address column, replace with actor sys_audit_log's record_views list view declared an ip_address column that no read-path writer ever stamps: buildRow in read-audit.ts stamps action, created_at, user_id, object_name, record_id, old_value, new_value, tenant_id, and conditionally organization_id/actor -- never ip_address, since client- fingerprint fields are populated on auth events only. On a compliance screen an always-empty column reads as "captured, and none" rather than "not captured" -- the same narrow-not-untruthful defect class #7675/#8147/ #8315 retired from this object's action enum, one layer down on a column. Replaced with actor, which the read writer DOES stamp on every row and which attributes a service principal that user_id structurally cannot hold. Pinned by sys-audit-log-record-views-columns.test.ts: the stamped key set is derived at runtime from a real engine run of the writer, never hand-copied, so the class can't regrow silently. Ablated (put ip_address back, confirmed red, restored byte-identically) per the standing lane clause. Deleted the one README bullet (from #9517/PR #9541) that documented the column as always-empty, since it no longer applies. Maintainer ruling 2026-08-18 + triage auto-adjudication 2026-08-19 (both Option 1). Stamping viewer IP (Option 2) is explicitly NOT commissioned. Fixes #9539 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .../record-views-drop-ip-address-column.md | 22 ++ packages/plugins/plugin-audit/README.md | 4 +- ...sys-audit-log-record-views-columns.test.ts | 208 ++++++++++++++++++ .../src/objects/sys-audit-log.object.ts | 16 +- 4 files changed, 246 insertions(+), 4 deletions(-) create mode 100644 .changeset/record-views-drop-ip-address-column.md create mode 100644 packages/plugins/plugin-audit/src/objects/sys-audit-log-record-views-columns.test.ts diff --git a/.changeset/record-views-drop-ip-address-column.md b/.changeset/record-views-drop-ip-address-column.md new file mode 100644 index 0000000000..e2106c0b57 --- /dev/null +++ b/.changeset/record-views-drop-ip-address-column.md @@ -0,0 +1,22 @@ +--- +"@objectstack/plugin-audit": patch +--- + +fix(audit): `record_views` list view drops its always-empty `ip_address` column, replaced with `actor` (#9539) + +`sys_audit_log`'s `record_views` list view (the "who viewed this record" screen, #8992) +declared an `ip_address` column, but `buildRow` in `read-audit.ts` never stamps that key +on a `read` row — client-fingerprint fields are populated on auth events only. The column +was structurally empty on every row this view can ever show, which on a compliance +surface reads as "we captured the fingerprint and this request had none" rather than +"not captured" — the same 审计面宁窄勿谎 (narrow-not-untruthful) defect class #7675 / +#8147 / #8315 retired from this object's `action` enum, one layer down on a column. + +Replaced with `actor`, which the read writer DOES stamp on every row and which attributes +a service principal (`svc:`) that `user_id` structurally cannot hold. Pinned by +`sys-audit-log-record-views-columns.test.ts`, which derives the read writer's actually- +stamped key set from a real engine run rather than a hand-copied list, so the class can't +regrow silently. + +Maintainer ruling 2026-08-18 + triage auto-adjudication 2026-08-19 (both Option 1). +Stamping viewer IP (Option 2) was explicitly NOT commissioned in this change. diff --git a/packages/plugins/plugin-audit/README.md b/packages/plugins/plugin-audit/README.md index 15be3bc659..a8fe565207 100644 --- a/packages/plugins/plugin-audit/README.md +++ b/packages/plugins/plugin-audit/README.md @@ -113,9 +113,7 @@ without the secret itself reaching the ledger. **`ip_address` / `user_agent` are populated on auth events only.** Neither the record-level writer nor the record-view writer stamps them: a `create` / `update` / `delete` / `read` row records who and what, not from where. Do not read a null client -fingerprint on such a row as "the request had none". ⚠️ The shipped `record_views` list -view carries an `ip_address` column, and on a `read` row that column is **always empty** -for this reason. +fingerprint on such a row as "the request had none". **`old_value` / `new_value` are null on every `read` row**, deliberately and not as an omission — see [Record-view auditing](#record-view-auditing--the-read-action). diff --git a/packages/plugins/plugin-audit/src/objects/sys-audit-log-record-views-columns.test.ts b/packages/plugins/plugin-audit/src/objects/sys-audit-log-record-views-columns.test.ts new file mode 100644 index 0000000000..9cea192c10 --- /dev/null +++ b/packages/plugins/plugin-audit/src/objects/sys-audit-log-record-views-columns.test.ts @@ -0,0 +1,208 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #9539 — the `record_views` list view's columns must stay a SUBSET of the + * keys the `read` writer actually stamps on a row. + * + * `sys_audit_log` is `readonly: true` on every field, and `validateRecord` + * skips readonly fields on insert (the same structural gap + * `sys-audit-log-retired-actions.test.ts` pins for the `action` enum) — so + * nothing else in the repo rejects a view column the writer never produces. + * `record_views` shipped with `ip_address` in its column list even though + * `buildRow` in `read-audit.ts` never sets that key: the column was + * structurally empty on every row it could ever show, which on a compliance + * screen reads as "we captured the fingerprint and this request had none" — + * a stronger and wrong claim (maintainer ruling 2026-08-18; triage + * auto-adjudication 2026-08-19; both Option 1: drop the column, replace it + * with `actor`, which IS stamped). + * + * The stamped key set is DERIVED here, never copied. `buildRow` is a private + * closure inside `installReadAuditWriter` — it cannot be imported and + * introspected directly — so this test runs the writer for real, against a + * real engine, on a read shaped to make every conditionally-stamped key + * present (a human principal that ALSO carries a service `actor` label, on a + * record that carries an `organization_id`), and reads the keys back off the + * row the writer actually persisted. If `buildRow` ever stops stamping a key + * this view lists, the observed key set shrinks and the assertion goes red — + * no hand-kept list to fall out of sync with the writer it is supposed to + * police. + */ + +import { describe, it, expect, beforeAll } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { installReadAuditWriter } from '../read-audit.js'; +import { SysAuditLog } from './sys-audit-log.object.js'; + +/** Minimal in-memory driver — just enough for one findOne + insert round trip. */ +function makeStubDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { + s = new Map(); + stores.set(obj, s); + } + return s; + }; + let nextId = 0; + 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') { + if (!(v as any[]).every((m) => matches(row, m))) return false; + continue; + } + if (k.startsWith('$')) continue; + const expected = v && typeof v === 'object' && '$eq' in (v as any) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', + version: '0.0.0', + supports: {} as any, + async connect() {}, + async disconnect() {}, + async checkHealth() { + return true; + }, + async execute() { + return null; + }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row: Record = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { + return storeFor(object).delete(id); + }, + async count(object: string, ast: any) { + return (await this.find(object, ast)).length; + }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { + return []; + }, + async bulkDelete() {}, + async updateMany() { + return 0; + }, + async beginTransaction() { + return { commit: async () => {}, rollback: async () => {} }; + }, + async commit() {}, + async rollback() {}, + }; + return driver; +} + +const contactObject = { + name: 'contact', + label: 'Contact', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + full_name: { name: 'full_name', label: 'Name', type: 'text' as const }, + organization_id: { name: 'organization_id', label: 'Org', type: 'text' as const }, + }, +}; + +const HARNESS_PACKAGE = 'com.objectstack.audit.test.record-views-columns'; + +/** Every key stamped on the one row the writer actually persists — captured, never copied. */ +let stampedKeys: Set; + +beforeAll(async () => { + const engine = new ObjectQL(); + engine.registerDriver(makeStubDriver(), true); + await engine.init(); + engine.registry.registerObject(contactObject as any, HARNESS_PACKAGE); + // The REAL sys_audit_log object under test — not a hand-copied stand-in — + // so `objectHasField` (the conditional-stamp gate for `organization_id` / + // `actor` in `buildRow`) reads the actual production field declarations. + engine.registry.registerObject(SysAuditLog as any, HARNESS_PACKAGE); + + await engine.insert( + 'contact', + { id: 'c1', full_name: 'Wei Zhang', organization_id: 'org_a' }, + { context: { isSystem: true } }, + ); + + const writer = installReadAuditWriter(engine, { objects: ['contact'] })!; + // A principal that carries BOTH a `userId` and a service `actor` label, on + // a record that carries `organization_id` — the one read shape that makes + // every conditionally-stamped key in `buildRow` present at once, so the + // captured set is the writer's FULL vocabulary, not just today's default + // path through it. + await engine.findOne('contact', { + where: { id: 'c1' }, + context: { userId: 'u_alice', actor: 'svc:export-worker', tenantId: 'org_a' }, + }); + await writer.flush(); + + const rows = (await engine.find('sys_audit_log', {})) as Array>; + expect(rows).toHaveLength(1); + stampedKeys = new Set(Object.keys(rows[0])); +}); + +/** The columns the shipped `record_views` list view declares. */ +function recordViewsColumns(): string[] { + const view = (SysAuditLog as { listViews?: Record }).listViews + ?.record_views; + const columns = view?.columns; + return Array.isArray(columns) ? columns.map(String) : []; +} + +describe('#9539 record_views columns stay inside the read writer\'s stamped key set', () => { + it('the writer actually stamped at least one row to derive the set from', () => { + expect(stampedKeys.size).toBeGreaterThan(0); + }); + + it.each(recordViewsColumns().map((c) => [c] as const))( + 'column %s is a key the read writer actually stamps', + (column) => { + expect( + stampedKeys.has(column), + `record_views declares column '${column}', but the read writer's buildRow() in ` + + 'read-audit.ts never sets that key on a persisted row — this view would show it ' + + 'structurally empty on every row it can ever display, which on a compliance ' + + 'screen reads as a false capability claim (#9539, 审计面宁窄勿谎). Stamped keys ' + + `observed on the writer's own output: ${[...stampedKeys].sort().join(', ')}.`, + ).toBe(true); + }, + ); + + it('ip_address specifically stays out — the read writer structurally cannot stamp it', () => { + // Named explicitly, not just covered by the loop above: this is the exact + // regression #9539 fixed, and `ReadAuditEvent` (read-audit.ts) carries no + // field for a client fingerprint at all, so this is not a near-miss the + // writer could accidentally start passing. + expect(recordViewsColumns()).not.toContain('ip_address'); + expect(stampedKeys.has('ip_address')).toBe(false); + }); +}); diff --git a/packages/plugins/plugin-audit/src/objects/sys-audit-log.object.ts b/packages/plugins/plugin-audit/src/objects/sys-audit-log.object.ts index 37601eb1a7..8b4fa30e16 100644 --- a/packages/plugins/plugin-audit/src/objects/sys-audit-log.object.ts +++ b/packages/plugins/plugin-audit/src/objects/sys-audit-log.object.ts @@ -77,12 +77,26 @@ export const SysAuditLog = ObjectSchema.create({ // adds the action and the screen that answers its question in one stroke. // `record_views` is the "who viewed this record" query as a list: actor // first, because that is the column an auditor scans. + // + // [#9539, maintainer ruling 2026-08-18 + triage auto-adjudication + // 2026-08-19, both Option 1] `ip_address` was dropped from this column + // list: `buildRow` in `read-audit.ts` never stamps it (client-fingerprint + // fields are populated on auth events only — see the README), so on every + // `read` row this column could ever show, it was structurally empty. On a + // compliance screen a blank cell reads as "captured, and none" rather than + // "not captured" — 审计面宁窄勿谎, the same principle #7675/#8147/#8315 + // applied to enum values, one layer down on a column. Replaced with + // `actor`, which the read writer DOES stamp on every row and which is the + // one column that attributes a service principal (`svc:`) rather + // than just falling back to a null `user_id`. Pinned by + // `sys-audit-log-record-views-columns.test.ts`: this view's columns must + // stay a subset of the read writer's actually-stamped key set. record_views: { type: 'grid', name: 'record_views', label: 'Record Views', data: { provider: 'object', object: 'sys_audit_log' }, - columns: ['created_at', 'user_id', 'object_name', 'record_id', 'ip_address'], + columns: ['created_at', 'user_id', 'object_name', 'record_id', 'actor'], filter: [{ field: 'action', operator: 'in', value: ['read'] }], sort: [{ field: 'created_at', order: 'desc' }], pagination: { pageSize: 50 },