Skip to content

Commit b3de42c

Browse files
os-warrenclaude
andauthored
fix(plugin-audit): drop record_views' always-empty ip_address column, replace with actor (#9956)
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 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9527980 commit b3de42c

4 files changed

Lines changed: 246 additions & 4 deletions

File tree

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
"@objectstack/plugin-audit": patch
3+
---
4+
5+
fix(audit): `record_views` list view drops its always-empty `ip_address` column, replaced with `actor` (#9539)
6+
7+
`sys_audit_log`'s `record_views` list view (the "who viewed this record" screen, #8992)
8+
declared an `ip_address` column, but `buildRow` in `read-audit.ts` never stamps that key
9+
on a `read` row — client-fingerprint fields are populated on auth events only. The column
10+
was structurally empty on every row this view can ever show, which on a compliance
11+
surface reads as "we captured the fingerprint and this request had none" rather than
12+
"not captured" — the same 审计面宁窄勿谎 (narrow-not-untruthful) defect class #7675 /
13+
#8147 / #8315 retired from this object's `action` enum, one layer down on a column.
14+
15+
Replaced with `actor`, which the read writer DOES stamp on every row and which attributes
16+
a service principal (`svc:<name>`) that `user_id` structurally cannot hold. Pinned by
17+
`sys-audit-log-record-views-columns.test.ts`, which derives the read writer's actually-
18+
stamped key set from a real engine run rather than a hand-copied list, so the class can't
19+
regrow silently.
20+
21+
Maintainer ruling 2026-08-18 + triage auto-adjudication 2026-08-19 (both Option 1).
22+
Stamping viewer IP (Option 2) was explicitly NOT commissioned in this change.

packages/plugins/plugin-audit/README.md

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,7 @@ without the secret itself reaching the ledger.
113113
**`ip_address` / `user_agent` are populated on auth events only.** Neither the
114114
record-level writer nor the record-view writer stamps them: a `create` / `update` /
115115
`delete` / `read` row records who and what, not from where. Do not read a null client
116-
fingerprint on such a row as "the request had none". ⚠️ The shipped `record_views` list
117-
view carries an `ip_address` column, and on a `read` row that column is **always empty**
118-
for this reason.
116+
fingerprint on such a row as "the request had none".
119117

120118
**`old_value` / `new_value` are null on every `read` row**, deliberately and not as an
121119
omission — see [Record-view auditing](#record-view-auditing--the-read-action).
Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #9539 — the `record_views` list view's columns must stay a SUBSET of the
5+
* keys the `read` writer actually stamps on a row.
6+
*
7+
* `sys_audit_log` is `readonly: true` on every field, and `validateRecord`
8+
* skips readonly fields on insert (the same structural gap
9+
* `sys-audit-log-retired-actions.test.ts` pins for the `action` enum) — so
10+
* nothing else in the repo rejects a view column the writer never produces.
11+
* `record_views` shipped with `ip_address` in its column list even though
12+
* `buildRow` in `read-audit.ts` never sets that key: the column was
13+
* structurally empty on every row it could ever show, which on a compliance
14+
* screen reads as "we captured the fingerprint and this request had none" —
15+
* a stronger and wrong claim (maintainer ruling 2026-08-18; triage
16+
* auto-adjudication 2026-08-19; both Option 1: drop the column, replace it
17+
* with `actor`, which IS stamped).
18+
*
19+
* The stamped key set is DERIVED here, never copied. `buildRow` is a private
20+
* closure inside `installReadAuditWriter` — it cannot be imported and
21+
* introspected directly — so this test runs the writer for real, against a
22+
* real engine, on a read shaped to make every conditionally-stamped key
23+
* present (a human principal that ALSO carries a service `actor` label, on a
24+
* record that carries an `organization_id`), and reads the keys back off the
25+
* row the writer actually persisted. If `buildRow` ever stops stamping a key
26+
* this view lists, the observed key set shrinks and the assertion goes red —
27+
* no hand-kept list to fall out of sync with the writer it is supposed to
28+
* police.
29+
*/
30+
31+
import { describe, it, expect, beforeAll } from 'vitest';
32+
import { ObjectQL } from '@objectstack/objectql';
33+
import { installReadAuditWriter } from '../read-audit.js';
34+
import { SysAuditLog } from './sys-audit-log.object.js';
35+
36+
/** Minimal in-memory driver — just enough for one findOne + insert round trip. */
37+
function makeStubDriver() {
38+
const stores = new Map<string, Map<string, Record<string, unknown>>>();
39+
const storeFor = (obj: string) => {
40+
let s = stores.get(obj);
41+
if (!s) {
42+
s = new Map();
43+
stores.set(obj, s);
44+
}
45+
return s;
46+
};
47+
let nextId = 0;
48+
const matches = (row: Record<string, unknown>, where: any): boolean => {
49+
if (!where || typeof where !== 'object') return true;
50+
for (const [k, v] of Object.entries(where)) {
51+
if (k === '$and') {
52+
if (!(v as any[]).every((m) => matches(row, m))) return false;
53+
continue;
54+
}
55+
if (k.startsWith('$')) continue;
56+
const expected = v && typeof v === 'object' && '$eq' in (v as any) ? (v as any).$eq : v;
57+
if ((row[k] ?? null) !== (expected ?? null)) return false;
58+
}
59+
return true;
60+
};
61+
const driver: any = {
62+
name: 'memory',
63+
version: '0.0.0',
64+
supports: {} as any,
65+
async connect() {},
66+
async disconnect() {},
67+
async checkHealth() {
68+
return true;
69+
},
70+
async execute() {
71+
return null;
72+
},
73+
async find(object: string, ast: any) {
74+
return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where));
75+
},
76+
async findOne(object: string, ast: any) {
77+
for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r;
78+
return null;
79+
},
80+
async create(object: string, data: Record<string, unknown>) {
81+
nextId += 1;
82+
const id = (data.id as string) ?? `r_${nextId}`;
83+
const row: Record<string, unknown> = { ...data, id };
84+
storeFor(object).set(id, row);
85+
return row;
86+
},
87+
async update(object: string, id: string, data: Record<string, unknown>) {
88+
const s = storeFor(object);
89+
const cur = s.get(id);
90+
if (!cur) return null;
91+
const updated = { ...cur, ...data, id };
92+
s.set(id, updated);
93+
return updated;
94+
},
95+
async upsert(object: string, data: Record<string, unknown>) {
96+
const id = data.id as string | undefined;
97+
if (id && storeFor(object).has(id)) return this.update(object, id, data);
98+
return this.create(object, data);
99+
},
100+
async delete(object: string, id: string) {
101+
return storeFor(object).delete(id);
102+
},
103+
async count(object: string, ast: any) {
104+
return (await this.find(object, ast)).length;
105+
},
106+
async bulkCreate(object: string, rows: Record<string, unknown>[]) {
107+
return Promise.all(rows.map((r) => this.create(object, r)));
108+
},
109+
async bulkUpdate() {
110+
return [];
111+
},
112+
async bulkDelete() {},
113+
async updateMany() {
114+
return 0;
115+
},
116+
async beginTransaction() {
117+
return { commit: async () => {}, rollback: async () => {} };
118+
},
119+
async commit() {},
120+
async rollback() {},
121+
};
122+
return driver;
123+
}
124+
125+
const contactObject = {
126+
name: 'contact',
127+
label: 'Contact',
128+
fields: {
129+
id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true },
130+
full_name: { name: 'full_name', label: 'Name', type: 'text' as const },
131+
organization_id: { name: 'organization_id', label: 'Org', type: 'text' as const },
132+
},
133+
};
134+
135+
const HARNESS_PACKAGE = 'com.objectstack.audit.test.record-views-columns';
136+
137+
/** Every key stamped on the one row the writer actually persists — captured, never copied. */
138+
let stampedKeys: Set<string>;
139+
140+
beforeAll(async () => {
141+
const engine = new ObjectQL();
142+
engine.registerDriver(makeStubDriver(), true);
143+
await engine.init();
144+
engine.registry.registerObject(contactObject as any, HARNESS_PACKAGE);
145+
// The REAL sys_audit_log object under test — not a hand-copied stand-in —
146+
// so `objectHasField` (the conditional-stamp gate for `organization_id` /
147+
// `actor` in `buildRow`) reads the actual production field declarations.
148+
engine.registry.registerObject(SysAuditLog as any, HARNESS_PACKAGE);
149+
150+
await engine.insert(
151+
'contact',
152+
{ id: 'c1', full_name: 'Wei Zhang', organization_id: 'org_a' },
153+
{ context: { isSystem: true } },
154+
);
155+
156+
const writer = installReadAuditWriter(engine, { objects: ['contact'] })!;
157+
// A principal that carries BOTH a `userId` and a service `actor` label, on
158+
// a record that carries `organization_id` — the one read shape that makes
159+
// every conditionally-stamped key in `buildRow` present at once, so the
160+
// captured set is the writer's FULL vocabulary, not just today's default
161+
// path through it.
162+
await engine.findOne('contact', {
163+
where: { id: 'c1' },
164+
context: { userId: 'u_alice', actor: 'svc:export-worker', tenantId: 'org_a' },
165+
});
166+
await writer.flush();
167+
168+
const rows = (await engine.find('sys_audit_log', {})) as Array<Record<string, unknown>>;
169+
expect(rows).toHaveLength(1);
170+
stampedKeys = new Set(Object.keys(rows[0]));
171+
});
172+
173+
/** The columns the shipped `record_views` list view declares. */
174+
function recordViewsColumns(): string[] {
175+
const view = (SysAuditLog as { listViews?: Record<string, { columns?: unknown }> }).listViews
176+
?.record_views;
177+
const columns = view?.columns;
178+
return Array.isArray(columns) ? columns.map(String) : [];
179+
}
180+
181+
describe('#9539 record_views columns stay inside the read writer\'s stamped key set', () => {
182+
it('the writer actually stamped at least one row to derive the set from', () => {
183+
expect(stampedKeys.size).toBeGreaterThan(0);
184+
});
185+
186+
it.each(recordViewsColumns().map((c) => [c] as const))(
187+
'column %s is a key the read writer actually stamps',
188+
(column) => {
189+
expect(
190+
stampedKeys.has(column),
191+
`record_views declares column '${column}', but the read writer's buildRow() in ` +
192+
'read-audit.ts never sets that key on a persisted row — this view would show it ' +
193+
'structurally empty on every row it can ever display, which on a compliance ' +
194+
'screen reads as a false capability claim (#9539, 审计面宁窄勿谎). Stamped keys ' +
195+
`observed on the writer's own output: ${[...stampedKeys].sort().join(', ')}.`,
196+
).toBe(true);
197+
},
198+
);
199+
200+
it('ip_address specifically stays out — the read writer structurally cannot stamp it', () => {
201+
// Named explicitly, not just covered by the loop above: this is the exact
202+
// regression #9539 fixed, and `ReadAuditEvent` (read-audit.ts) carries no
203+
// field for a client fingerprint at all, so this is not a near-miss the
204+
// writer could accidentally start passing.
205+
expect(recordViewsColumns()).not.toContain('ip_address');
206+
expect(stampedKeys.has('ip_address')).toBe(false);
207+
});
208+
});

packages/plugins/plugin-audit/src/objects/sys-audit-log.object.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,12 +77,26 @@ export const SysAuditLog = ObjectSchema.create({
7777
// adds the action and the screen that answers its question in one stroke.
7878
// `record_views` is the "who viewed this record" query as a list: actor
7979
// first, because that is the column an auditor scans.
80+
//
81+
// [#9539, maintainer ruling 2026-08-18 + triage auto-adjudication
82+
// 2026-08-19, both Option 1] `ip_address` was dropped from this column
83+
// list: `buildRow` in `read-audit.ts` never stamps it (client-fingerprint
84+
// fields are populated on auth events only — see the README), so on every
85+
// `read` row this column could ever show, it was structurally empty. On a
86+
// compliance screen a blank cell reads as "captured, and none" rather than
87+
// "not captured" — 审计面宁窄勿谎, the same principle #7675/#8147/#8315
88+
// applied to enum values, one layer down on a column. Replaced with
89+
// `actor`, which the read writer DOES stamp on every row and which is the
90+
// one column that attributes a service principal (`svc:<name>`) rather
91+
// than just falling back to a null `user_id`. Pinned by
92+
// `sys-audit-log-record-views-columns.test.ts`: this view's columns must
93+
// stay a subset of the read writer's actually-stamped key set.
8094
record_views: {
8195
type: 'grid',
8296
name: 'record_views',
8397
label: 'Record Views',
8498
data: { provider: 'object', object: 'sys_audit_log' },
85-
columns: ['created_at', 'user_id', 'object_name', 'record_id', 'ip_address'],
99+
columns: ['created_at', 'user_id', 'object_name', 'record_id', 'actor'],
86100
filter: [{ field: 'action', operator: 'in', value: ['read'] }],
87101
sort: [{ field: 'created_at', order: 'desc' }],
88102
pagination: { pageSize: 50 },

0 commit comments

Comments
 (0)