diff --git a/.changeset/config-change-reaches-audit-log.md b/.changeset/config-change-reaches-audit-log.md new file mode 100644 index 0000000000..beac6f4f21 --- /dev/null +++ b/.changeset/config-change-reaches-audit-log.md @@ -0,0 +1,45 @@ +--- +"@objectstack/service-settings": patch +--- + +fix(service-settings): settings writes reach `sys_audit_log` as `config_change` (#8145) + +`GET /api/v1/data/sys_audit_log?$filter={"action":"config_change"}` answered +**total 0** after any settings write, for the whole life of that enum member. So +did the shipped `config_changes` list view and the console filter that offers the +value: three surfaces advertising a class of audit event the platform never +recorded. A settings change was audited — into `sys_setting_audit`, with +`action: 'set'` — and nowhere else, while the settings service's own type +documentation promised `sys_audit_log` rows "for every successful write". + +The cause was one argument. `SettingsAuditSink` — the slot documented since +Phase 3 as the one that writes the generic ledger — is the second parameter of +`SettingsService.bindEngine`, and `SettingsServicePlugin` passed `undefined` +there. Nothing else was missing: the service called the sink on every write, the +enum declared the value, the view filtered on it. + +**Both ledgers are written now** (the dual-write half of the 2026-08-12 ruling on +#7675, which left the choice to the implementation): + +- `sys_audit_log`, `action: 'config_change'` — the platform-wide compliance + ledger. One row per changed key, attributed on `user_id` and `actor`, stamped + with the caller's tenant (and `organization_id` where the deployment declares + it, without which RLS would hide every row and leave the view as empty as + before). `metadata` carries the namespace/key/scope and whether the key is + encrypted; `new_value` carries a **digest**, never a value. +- `sys_setting_audit`, `action: 'set' | 'reset'` — unchanged. It keeps its rows + because it has live readers and because it records what the generic ledger has + no columns for (`namespace`, `key`, `scope`, `old_hash`/`new_hash`, `source`, + `reason`). + +The new write is **best-effort and can never fail a settings write**: +`sys_audit_log` belongs to the optional `@objectstack/plugin-audit`, so on a +deployment without that plugin the table does not exist, and the sink reports the +gap once per process rather than raising. A write that is REFUSED — an +unauthorised caller, an env-pinned key, the #8026 fail-closed crypto refusal — +still emits no row on either ledger: a refused write is not a successful one, and +a ledger listing configuration changes that never happened would be a worse lie +than the empty view. + +`settings-service.types.ts`'s contract line now states what is built rather than +what was intended, per the ruling's 以实现定契约. diff --git a/packages/qa/dogfood/test/settings-config-change-audit.dogfood.test.ts b/packages/qa/dogfood/test/settings-config-change-audit.dogfood.test.ts new file mode 100644 index 0000000000..2e503b4b42 --- /dev/null +++ b/packages/qa/dogfood/test/settings-config-change-audit.dogfood.test.ts @@ -0,0 +1,200 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8145, from #7675] `PUT /api/settings/branding` must land on `sys_audit_log` + * as a `config_change` row — the parent bug's reproduction, INVERTED. + * + * ## The reproduction this file owns + * + * #7675 step 2, verbatim: *"`PUT /api/settings/branding {"workspace_name":"X"}` + * → 200, then filter `{"action":"config_change"}` → **total 0**; the event went + * to `sys_setting_audit` with action `set` instead."* Every declared + * `config_change` surface was therefore permanently empty — the enum member, the + * shipped `config_changes` list view, and the console filter that offers the + * value. + * + * The maintainer's 2026-08-12 ruling allowed a dual-write or a reroute and left + * the choice to the implementation (以实现定契约). This lane chose **dual-write**, + * so this file asserts BOTH halves of one settings write: + * + * A. a `sys_audit_log` row with `action: 'config_change'` now exists — the + * inverted repro, red on `origin/main` at the very first assertion; + * B. the `sys_setting_audit` row is STILL written, unchanged — the half that + * would silently disappear under a reroute, and the one the platform QA + * checklist (`docs/qa/platform-checklist/areas/platform-core.json`) reads. + * + * ## Why it has to be here rather than in `service-settings` + * + * `sys_audit_log` is `@objectstack/plugin-audit`'s object, and service-settings + * must not depend on that plugin — the write is best-effort precisely because + * the plugin is OPTIONAL. So the package's own suite + * (`packages/services/service-settings/src/config-change-audit.test.ts`) pins + * the WIRING and the ROW SHAPE at the engine seam, and can see neither the real + * object, nor its real `action` enum, nor the shipped list view. Only a booted + * stack with plugin-audit installed can, and only through the real routes is + * this the reported bug rather than a restatement of the fix. Neither file is + * sufficient alone. + * + * Harness notes: + * - `bootStack` installs no audit plugin, so `AuditPlugin` is added here — the + * same reason `admin-identity-audit-trail.dogfood.test.ts` adds it. + * - The settings route and the audit insert are both awaited inside the + * request, but the row is written through a separate engine call, so reads + * poll rather than reading once (same shape as the sibling audit fixture). + * - NOT eligible for the shared showcase project: it writes org-wide settings. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { AuditPlugin } from '@objectstack/plugin-audit'; + +const SYSTEM_CTX = { isSystem: true }; + +async function findRows(ql: any, object: string, where: any, limit = 200): Promise { + const rows = await ql.find(object, { where, limit }, { context: SYSTEM_CTX }); + return Array.isArray(rows) ? rows : (rows?.records ?? []); +} + +/** Poll until `predicate` holds over the rows, then return them. */ +async function waitForRows( + load: () => Promise, + predicate: (rows: any[]) => boolean, + what: string, +): Promise { + let rows: any[] = []; + for (let i = 0; i < 40; i++) { + rows = await load(); + if (predicate(rows)) return rows; + await new Promise((r) => setTimeout(r, 250)); + } + throw new Error(`${what} — last saw ${rows.length} row(s)`); +} + +describe('#8145: a settings write reaches sys_audit_log as config_change', () => { + let stack: VerifyStack; + let ql: any; + let token: string; + const WORKSPACE_NAME = 'ObjectStack 8145'; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, { extraPlugins: [new AuditPlugin()] }); + token = await stack.signIn(); // the seeded dev admin (platform admin) + ql = await stack.kernel.getServiceAsync('objectql'); + + // The parent's step 2, unchanged. + const put = await stack.raw('/api/settings/branding', { + method: 'PUT', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` }, + body: JSON.stringify({ workspace_name: WORKSPACE_NAME }), + }); + expect(put.status, await put.clone().text()).toBe(200); + }, 180_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + // ── A. The inverted reproduction ──────────────────────────────────────── + + it('the `{"action":"config_change"}` filter returns the event (was total 0)', async () => { + // Read it exactly as the bug report did: the real REST data route, the real + // filter, as the real admin — not through the engine. + const rows = await waitForRows( + async () => { + const res = await stack.apiAs( + token, + 'GET', + `/data/sys_audit_log?$filter=${encodeURIComponent(JSON.stringify({ action: 'config_change' }))}`, + ); + expect(res.status).toBe(200); + const body = (await res.json()) as any; + return (body?.data ?? body?.records ?? []) as any[]; + }, + (r) => r.length >= 1, + 'expected at least one config_change row after PUT /api/settings/branding', + ); + + // `total 0` on `origin/main` — this is the assertion the card inverts. + expect(rows.length).toBeGreaterThanOrEqual(1); + + const mine = rows.filter((r) => String(r.metadata ?? '').includes('"key":"workspace_name"')); + expect(mine.length).toBeGreaterThanOrEqual(1); + const row = mine[0]; + expect(row.action).toBe('config_change'); + expect(row.object_name).toBe('sys_setting'); + // The admin who made the call is attributed on both channels (ADR-0014 D2). + expect(row.user_id).toBeTruthy(); + expect(row.actor).toBe(row.user_id); + + const meta = JSON.parse(String(row.metadata)); + expect(meta).toMatchObject({ + event: 'settings.set', + namespace: 'branding', + key: 'workspace_name', + encrypted: false, + }); + // A digest, never the value — the ledger describes the change, not the data. + expect(JSON.parse(String(row.new_value)).digest).toBeTruthy(); + // Generous timeout so a REGRESSION reports `waitForRows`'s own message + // ("expected at least one config_change row…") rather than vitest's 5s + // default cutting the poll short and reporting a timeout instead. Measured + // on `origin/main`, where this case is red: with the default it failed as a + // bare timeout, which names neither the object nor the filter. + }, 30_000); + + it('the shipped `config_changes` list view — its OWN declared filter — now matches', async () => { + // The view's filter is read off the registered object rather than retyped, + // so this cannot pass against a filter that agrees only with this test. It + // is `action IN ['config_change','import']` today; whatever it becomes, the + // question stays "does the shipped view return rows". + const schema: any = ql.getSchema('sys_audit_log'); + const view = schema?.listViews?.config_changes; + expect(view, 'the config_changes list view must still be declared').toBeTruthy(); + + const clause = (view.filter ?? []).find((f: any) => f.field === 'action'); + expect(clause?.operator).toBe('in'); + expect(clause.value).toContain('config_change'); + + const rows = await findRows(ql, 'sys_audit_log', { action: { $in: clause.value } }); + // Empty for the whole life of the enum member before this card. + expect(rows.length).toBeGreaterThanOrEqual(1); + expect(rows.some((r) => r.action === 'config_change')).toBe(true); + }); + + // ── B. Dual-write: the settings-specific ledger is untouched ───────────── + + it('`sys_setting_audit` still records the same write (dual-write, not reroute)', async () => { + const rows = await waitForRows( + () => findRows(ql, 'sys_setting_audit', { namespace: 'branding', key: 'workspace_name' }), + (r) => r.length >= 1, + 'expected the pre-existing sys_setting_audit row to still be written', + ); + + // Asserted as a PRESENCE against real stored rows: a reroute would empty + // this table, and an "unchanged" claim measured on a fixture that never + // wrote here would pass emptily. + const row = rows[0]; + expect(row.action).toBe('set'); + expect(row.source).toBe('api'); + expect(row.scope).toBeTruthy(); + expect(row.new_hash).toBeTruthy(); + + // The two rows are complements, not copies: each carries what the other's + // columns cannot hold. + const ledger = await findRows(ql, 'sys_audit_log', { action: 'config_change' }); + expect(ledger.length).toBeGreaterThanOrEqual(1); + expect(row.object_name).toBeUndefined(); + expect(ledger[0].new_hash).toBeUndefined(); + }, 30_000); + + it('the setting itself is readable back — the audit is a complement, not the write', async () => { + // The settings routes are mounted at `/api/settings`, outside `/api/v1`. + const res = await stack.raw('/api/settings/branding', { + headers: { Authorization: `Bearer ${token}` }, + }); + expect(res.status).toBe(200); + const body = (await res.json()) as any; + expect(body.data.values.workspace_name.value).toBe(WORKSPACE_NAME); + }); +}); diff --git a/packages/services/service-settings/src/config-change-audit.test.ts b/packages/services/service-settings/src/config-change-audit.test.ts new file mode 100644 index 0000000000..6e36a75851 --- /dev/null +++ b/packages/services/service-settings/src/config-change-audit.test.ts @@ -0,0 +1,608 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8145] `config_change` reaches `sys_audit_log` — the SEAM half. + * + * ## What was broken, and where + * + * `SettingsServicePlugin.start()` called + * `bindEngine(engine, undefined, { auditWriter, … })`. That second argument is + * the `SettingsAuditSink` slot — documented since Phase 3 as the one that writes + * the generic `sys_audit_log` — and nothing ever supplied it. So every settings + * write landed on `sys_setting_audit` with `action: 'set'` and NOWHERE else, + * which is #7675's `config_change` half: the declared enum member, the shipped + * `config_changes` list view and every `$filter={"action":"config_change"}` were + * permanently empty. + * + * **The defect is the WIRING, not the row shape.** A test that called + * `buildConfigChangeAuditSink` directly would stay green on a plugin that never + * wired it, so every case here drives the real `SettingsServicePlugin` through + * its real `init`/`start`/`kernel:ready` sequence and lets it build its own + * sinks. Case 1 is red on `origin/main` for exactly the reason above. + * + * ## What this file can and cannot see + * + * `sys_setting` and `sys_setting_audit` are `@objectstack/platform-objects` + * declarations this package already depends on, so those rows are REAL here — + * registered in a real `ObjectQL` over a memory driver, and read back out of the + * driver's own store. `sys_audit_log` belongs to the optional + * `@objectstack/plugin-audit`, which this package must not depend on (the + * dependency would invert the optional-plugin relationship the best-effort write + * exists to respect), so its insert is intercepted at the engine seam and the + * ROW SHAPE is asserted there. + * + * That the shape lands in the real object, that the real `action` enum accepts + * it, and that the shipped `config_changes` list view returns it, are pinned end + * to end against the real routes and the real object in + * `packages/qa/dogfood/test/settings-config-change-audit.dogfood.test.ts` — the + * parent's reproduction, inverted. Neither file is sufficient alone. + */ + +import { describe, expect, it, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SysSetting, SysSettingAudit } from '@objectstack/platform-objects/system'; +import type { SettingsManifest } from '@objectstack/spec/system'; +import type { IHttpRequest, IHttpResponse, IHttpServer, RouteHandler } from '@objectstack/spec/contracts'; +import { + SettingsServicePlugin, + buildSettingAuditWriter, + wrapEngineAsSettingsEngine, +} from './settings-service-plugin.js'; +import { + buildConfigChangeAuditSink, + CONFIG_CHANGE_ACTION, + CONFIG_CHANGE_OBJECT_NAME, +} from './config-change-audit.js'; +import { registerSettingsRoutes } from './settings-routes.js'; +import { NoopCryptoAdapter } from './crypto-adapter.js'; +import { SettingsCryptoUnavailableError } from './settings-service.types.js'; +import { SettingsService } from './settings-service.js'; + +const OWNER_PACKAGE = 'com.objectstack.test.config-change-audit'; + +const SECRET = 'sk_live_8145_do_not_log'; + +/** + * One plain key, one encrypted key, one namespace. Deliberately minimal: the + * shipped manifests carry `visible` predicates and cross-field `required` rules, + * and this file is about what a write puts on the two ledgers. + */ +const manifest: SettingsManifest = { + namespace: 'branding_test', + version: 1, + label: 'Branding (test)', + scope: 'global', + readPermission: 'setup.access', + writePermission: 'setup.write', + specifiers: [ + { type: 'text', key: 'workspace_name', label: 'Workspace name', required: false }, + { type: 'password', key: 'api_key', label: 'API key', required: false }, + ], +}; + +type Store = Map>>; + +/** A driver over plain Maps — enough of `IDataDriver` for the settings write path. */ +function makeMemoryDriver() { + const store: Store = new Map(); + let nextId = 0; + const copy = (r: Record) => ({ ...r }); + const rowsOf = (object: string) => { + let s = store.get(object); + if (!s) { s = new Map(); store.set(object, s); } + return s; + }; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]) => (row[k] ?? null) === (v ?? null)); + }; + 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 [...rowsOf(object).values()].filter((r) => matches(r, ast?.where)).map(copy); + }, + async findOne(object: string, ast: any) { + for (const r of rowsOf(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) ?? `row_${nextId}`; + const row = { ...data, id }; + rowsOf(object).set(id, row); + return copy(row); + }, + async update(object: string, id: string, data: Record) { + const s = rowsOf(object); + const cur = s.get(id); + if (!cur) return null; + const next = { ...cur, ...data, id }; + s.set(id, next); + return copy(next); + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + return id && rowsOf(object).has(id) ? this.update(object, id, data) : this.create(object, data); + }, + async delete(object: string, id: string) { return rowsOf(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 = rowsOf(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) rowsOf(object).delete(r.id as string); + return rows.length; + }, + async syncSchema() {}, async dropTable() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, rowsOf }; +} + +/** Minimal `IHttpServer` that just keeps the handlers so a route can be invoked. */ +class MockHttp implements IHttpServer { + routes = new Map(); + private add(method: string, path: string, handler: RouteHandler) { + this.routes.set(`${method} ${path}`, handler); + } + get(p: string, h: RouteHandler) { this.add('GET', p, h); return this as any; } + post(p: string, h: RouteHandler) { this.add('POST', p, h); return this as any; } + put(p: string, h: RouteHandler) { this.add('PUT', p, h); return this as any; } + delete(p: string, h: RouteHandler) { this.add('DELETE', p, h); return this as any; } + patch(p: string, h: RouteHandler) { this.add('PATCH', p, h); return this as any; } + use() { return this as any; } + listen() { return Promise.resolve(); } + close() { return Promise.resolve(); } + getInstance() { return null; } +} + +interface BootOptions { + /** Make every `sys_audit_log` insert fail, to exercise the best-effort path. */ + ledgerThrows?: boolean; + /** Identity the write runs under. */ + userId?: string; + tenantId?: string; + /** `OS_*` overrides — an env-pinned key makes the service REFUSE its write. */ + env?: Record; +} + +/** + * Boot the REAL `SettingsServicePlugin` against a real engine, through a + * hand-rolled `PluginContext` — the production sequence (`init` → + * `start` → the `kernel:ready` hook) rather than a re-creation of its wiring. + * The plugin builds its own secret store, its own `sys_setting_audit` writer and + * its own `config_change` sink; nothing here supplies any of them. + */ +async function bootPlugin(opts: BootOptions = {}) { + const engine = new ObjectQL(); + const { driver, rowsOf } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(SysSetting as any, OWNER_PACKAGE); + engine.registry.registerObject(SysSettingAudit as any, OWNER_PACKAGE); + + // `sys_audit_log` is plugin-audit's object and is deliberately not resolvable + // from this package (see the file header), so its insert is answered at the + // engine seam and recorded. Every other call — including the `sys_setting` and + // `sys_setting_audit` writes — goes to the real engine untouched, so those + // rows are real. + const ledgerInserts: Array<{ row: Record; opts: any }> = []; + const realInsert = engine.insert.bind(engine); + (engine as any).insert = async (object: string, data: any, options?: any) => { + if (object !== 'sys_audit_log') return realInsert(object, data, options); + ledgerInserts.push({ row: data, opts: options }); + if (opts.ledgerThrows) throw new Error('no such table: sys_audit_log'); + return { ...data, id: `audit_${ledgerInserts.length}` }; + }; + + const logged: string[] = []; + const logger = { + info: () => {}, + warn: (m: string) => { logged.push(m); }, + error: (m: string) => { logged.push(m); }, + debug: () => {}, + }; + + const http = new MockHttp(); + let readyHook: (() => Promise) | undefined; + const services: Record = { objectql: engine, 'http-server': http }; + const ctx: any = { + logger, + registerService: (name: string, svc: unknown) => { services[name] = svc; }, + getService: (name: string) => { + if (!(name in services)) throw new Error(`no service '${name}'`); + return services[name]; + }, + hook: (event: string, fn: () => Promise) => { + if (event === 'kernel:ready') readyHook = fn; + }, + }; + + const plugin = new SettingsServicePlugin({ + manifests: [manifest], + env: opts.env ?? {}, + // The bundled `mail`/`sms`/`storage`/`ai` test-connection handlers register + // against manifests this fixture does not load; opting out keeps the boot to + // the one namespace under test. + actionHandlers: {}, + }); + await plugin.init(ctx); + await plugin.start(ctx); + await readyHook!(); + + const service = services.settings as SettingsService; + const writeCtx = { userId: opts.userId, tenantId: opts.tenantId, requestId: 'req_8145' }; + + return { + service, + writeCtx, + /** Every `sys_audit_log` row the write path asked the engine to insert. */ + ledgerRows: () => ledgerInserts.map((i) => i.row), + ledgerOpts: () => ledgerInserts.map((i) => i.opts), + /** REAL `sys_setting_audit` rows, read out of the driver's store. */ + settingAuditRows: () => [...rowsOf('sys_setting_audit').values()], + /** REAL `sys_setting` rows. */ + settingRows: () => [...rowsOf('sys_setting').values()], + logged, + http, + }; +} + +/** Parse a row's `metadata` JSON. */ +const metaOf = (row: Record): any => JSON.parse(String(row.metadata)); + +/** Drive `PUT /api/settings/branding_test` through a mounted route set. */ +async function put(http: MockHttp, body: Record) { + const handler = http.routes.get('PUT /api/settings/:namespace')!; + const req = { + params: { namespace: 'branding_test' }, + query: {}, + body, + headers: {}, + method: 'PUT', + path: '/api/settings/branding_test', + } as unknown as IHttpRequest; + const state: { status: number; body?: any } = { status: 200 }; + const res = { + json: vi.fn((data: any) => { state.body = data; }), + send: vi.fn(), + status: vi.fn((code: number) => { state.status = code; return res; }), + header: vi.fn(() => res), + } as unknown as IHttpResponse; + await handler(req, res); + return { state }; +} + +// --------------------------------------------------------------------------- +// 1. The wiring — the defect itself +// --------------------------------------------------------------------------- + +describe('#8145 — a settings write reaches sys_audit_log as `config_change`', () => { + it('the plugin WIRES the generic sink: one write, one config_change row', async () => { + const boot = await bootPlugin({ userId: 'usr_admin', tenantId: 'org_1' }); + + await boot.service.setMany('branding_test', { workspace_name: 'ObjectStack' }, boot.writeCtx); + + // RED on `origin/main`: `bindEngine`'s sink argument was `undefined`, so this + // array is empty there and the length assertion is the one that fails. + const rows = boot.ledgerRows(); + expect(rows).toHaveLength(1); + expect(rows[0].action).toBe(CONFIG_CHANGE_ACTION); + expect(rows[0].action).toBe('config_change'); + expect(rows[0].object_name).toBe(CONFIG_CHANGE_OBJECT_NAME); + // A settings row has no single id — the composite key is in `metadata`. + expect(rows[0].record_id).toBeNull(); + // Attribution: both channels, per ADR-0014 D2. + expect(rows[0].user_id).toBe('usr_admin'); + expect(rows[0].actor).toBe('usr_admin'); + // Tenant context — without it RLS hides the row from non-platform readers. + expect(rows[0].tenant_id).toBe('org_1'); + // Written as the platform, on an append-only, all-`readonly` table. + expect(boot.ledgerOpts()[0]?.context).toMatchObject({ isSystem: true }); + + const meta = metaOf(rows[0]); + expect(meta).toMatchObject({ + event: 'settings.set', + namespace: 'branding_test', + key: 'workspace_name', + scope: 'global', + encrypted: false, + requestId: 'req_8145', + }); + expect(JSON.parse(String(rows[0].new_value))).toMatchObject({ + namespace: 'branding_test', + key: 'workspace_name', + scope: 'global', + }); + }); + + it('records one row per CHANGED KEY, not one per request', async () => { + const boot = await bootPlugin({ userId: 'usr_admin' }); + await boot.service.setMany( + 'branding_test', + { workspace_name: 'A', api_key: SECRET }, + boot.writeCtx, + ); + const keys = boot.ledgerRows().map((r) => metaOf(r).key).sort(); + expect(keys).toEqual(['api_key', 'workspace_name']); + }); + + it('a CLEARED key is still a config_change, with no new state described', async () => { + const boot = await bootPlugin({ userId: 'usr_admin' }); + await boot.service.setMany('branding_test', { workspace_name: 'A' }, boot.writeCtx); + await boot.service.setMany('branding_test', { workspace_name: null }, boot.writeCtx); + + const rows = boot.ledgerRows(); + expect(rows).toHaveLength(2); + // The action stays `config_change` — `set` vs `reset` is a settings-shaped + // distinction and lives in `metadata`, because the generic enum has no + // member for it (and #8147 retired, rather than added, enum members). + expect(rows[1].action).toBe('config_change'); + expect(metaOf(rows[1]).event).toBe('settings.reset'); + expect(rows[1].new_value).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Dual-write: the settings-specific ledger is UNCHANGED +// --------------------------------------------------------------------------- + +describe('#8145 — dual-write: `sys_setting_audit` keeps its rows', () => { + it('one write leaves exactly one row on EACH ledger', async () => { + const boot = await bootPlugin({ userId: 'usr_admin', tenantId: 'org_1' }); + + await boot.service.setMany('branding_test', { workspace_name: 'ObjectStack' }, boot.writeCtx); + + // The presence half. This is a REAL row in the REAL `sys_setting_audit` + // object, so the assertion cannot pass emptily on a fixture that never wrote + // there — which is exactly how an "existing behaviour unchanged" pin goes + // green for the wrong reason. + const settingAudit = boot.settingAuditRows(); + expect(settingAudit).toHaveLength(1); + expect(settingAudit[0]).toMatchObject({ + namespace: 'branding_test', + key: 'workspace_name', + scope: 'global', + action: 'set', + source: 'api', + actor_id: 'usr_admin', + }); + expect(settingAudit[0].new_hash).toBeTruthy(); + + // …and the generic ledger got its own, distinct row for the same event. + expect(boot.ledgerRows()).toHaveLength(1); + expect(boot.ledgerRows()[0].action).toBe('config_change'); + + // The two are not copies: each carries what the other's columns cannot hold. + expect(settingAudit[0].object_name).toBeUndefined(); + expect(boot.ledgerRows()[0].new_hash).toBeUndefined(); + }); + + it('the settings row itself still lands (neither ledger is in the write path)', async () => { + const boot = await bootPlugin({ userId: 'usr_admin' }); + await boot.service.setMany('branding_test', { workspace_name: 'ObjectStack' }, boot.writeCtx); + const rows = boot.settingRows(); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ namespace: 'branding_test', key: 'workspace_name' }); + expect(await boot.service.get('branding_test', 'workspace_name')).toMatchObject({ + value: 'ObjectStack', + }); + }); +}); + +// --------------------------------------------------------------------------- +// 3. No plaintext on either ledger +// --------------------------------------------------------------------------- + +describe('#8145 — an encrypted key never reaches the ledger in cleartext', () => { + it('records a masked digest, and the secret appears nowhere in the row', async () => { + const boot = await bootPlugin({ userId: 'usr_admin' }); + await boot.service.setMany('branding_test', { api_key: SECRET }, boot.writeCtx); + + const row = boot.ledgerRows()[0]; + expect(metaOf(row).encrypted).toBe(true); + const digest = JSON.parse(String(row.new_value)).digest as string; + expect(digest).toMatch(/^ { + it('an ANONYMOUS write over the plugin\'s own routes: 403, and BOTH ledgers untouched', async () => { + // The plugin's `verifiedContextFromRequest` fails closed with no auth + // service resolvable (Finding-1), so this is the deny an unauthenticated + // caller really meets on a running server — reached through the routes the + // plugin mounted itself, no context injected. + const boot = await bootPlugin({ userId: 'usr_admin' }); + const { state } = await put(boot.http, { workspace_name: 'never-written' }); + + // Both halves of the ADR-0112 envelope — a status-only assertion could not + // tell this refusal from any other 4xx, and "it threw" is not a pin. + expect(state.status).toBe(403); + expect(state.body.error.code).toBe('SETTINGS_FORBIDDEN'); + + expect(boot.ledgerRows()).toHaveLength(0); + expect(boot.settingAuditRows()).toHaveLength(0); + expect(boot.settingRows()).toHaveLength(0); + + // ⚠️ NON-VACUITY — see the case below; the same argument applies, and the + // authorized write there runs against this same wiring. + await boot.service.setMany('branding_test', { workspace_name: 'ok' }, boot.writeCtx); + expect(boot.ledgerRows()).toHaveLength(1); + }); + + it('an env-locked key: 409 SETTINGS_LOCKED, and BOTH ledgers untouched', async () => { + const boot = await bootPlugin({ + userId: 'usr_admin', + env: { OS_BRANDING_TEST_WORKSPACE_NAME: 'pinned-by-env' }, + }); + // The plugin's service, its plugin-built sinks — but routes mounted with an + // AUTHORIZED context, so the refusal under test is the env lock rather than + // the anonymous deny above. Only the identity resolution is stubbed; the + // handler, the error mapping and both ledgers are the real ones. + const authorized = new MockHttp(); + registerSettingsRoutes(authorized, boot.service, { + contextFromRequest: () => ({ + enforced: true, + userId: 'usr_admin', + permissions: ['setup.access', 'setup.write'], + }), + }); + + const { state } = await put(authorized, { workspace_name: 'never-written' }); + expect(state.status).toBe(409); + expect(state.body.error.code).toBe('SETTINGS_LOCKED'); + + expect(boot.ledgerRows()).toHaveLength(0); + expect(boot.settingAuditRows()).toHaveLength(0); + expect(boot.settingRows()).toHaveLength(0); + + // ⚠️ NON-VACUITY. Every assertion above is zero-length, and a fixture that + // never reaches the emitter at all would satisfy each one. So the SAME + // routes are now driven through a write that IS allowed (a different key, + // not env-pinned): if the emitter were unreachable in this fixture, this + // half would be zero too and the case would fail instead of passing emptily. + const ok = await put(authorized, { api_key: SECRET }); + expect(ok.state.status).toBe(200); + expect(boot.ledgerRows()).toHaveLength(1); + expect(boot.ledgerRows()[0].action).toBe('config_change'); + expect(boot.settingAuditRows()).toHaveLength(1); + }); + + it('#8026 fail-closed: no config_change row where that refusal IS reachable', async () => { + // The engine-less/host-bound shape described above: a service with nothing + // able to encrypt, wired with the SAME two ledger writers the plugin builds + // (imported, not re-created — a hand-copied row shape here would be pinning + // the test's own copy of the thing under test). + const engine = new ObjectQL(); + const { driver, rowsOf } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(SysSetting as any, OWNER_PACKAGE); + engine.registry.registerObject(SysSettingAudit as any, OWNER_PACKAGE); + + const ledgerRows: Array> = []; + const realInsert = engine.insert.bind(engine); + (engine as any).insert = async (object: string, data: any, options?: any) => { + if (object !== 'sys_audit_log') return realInsert(object, data, options); + ledgerRows.push(data); + return { ...data, id: `audit_${ledgerRows.length}` }; + }; + + const svc = new SettingsService({ + env: {}, + engine: wrapEngineAsSettingsEngine(engine as any), + // No `cryptoProvider`, no `secretStore`, and the base64 default adapter — + // which declares no confidentiality, so the write is refused. + crypto: new NoopCryptoAdapter(), + audit: buildConfigChangeAuditSink(engine as any), + auditWriter: buildSettingAuditWriter(engine as any), + logger: { error: () => {} }, + }); + svc.registerManifest(manifest); + + const err = await svc + .setMany('branding_test', { workspace_name: 'sibling', api_key: SECRET }) + .then(() => null, (e) => e); + + expect(err).toBeInstanceOf(SettingsCryptoUnavailableError); + expect(err.code).toBe('SETTINGS_CRYPTO_UNAVAILABLE'); + + // The #8026 pre-flight refuses the WHOLE batch before the write loop, so the + // plain sibling key is neither persisted nor audited — a half-audited batch + // would be worse than no audit at all. + const settingAudit = () => [...rowsOf('sys_setting_audit').values()]; + expect(ledgerRows).toHaveLength(0); + expect(settingAudit()).toHaveLength(0); + expect([...rowsOf('sys_setting').values()]).toHaveLength(0); + + // NON-VACUITY, same argument as the case above: the refusal is scoped to the + // secret, so a plain-only write on this same service still reaches both + // ledgers. + await svc.setMany('branding_test', { workspace_name: 'ok' }); + expect(ledgerRows).toHaveLength(1); + expect(ledgerRows[0].action).toBe('config_change'); + expect(settingAudit()).toHaveLength(1); + }); +}); + +// --------------------------------------------------------------------------- +// 5. Best-effort: the ledger never breaks the write +// --------------------------------------------------------------------------- + +describe('#8145 — the config_change write is best-effort', () => { + it('a failing sys_audit_log insert leaves the settings write landed and reported', async () => { + // The shape of a deployment WITHOUT plugin-audit: the table does not exist. + const boot = await bootPlugin({ ledgerThrows: true, userId: 'usr_admin' }); + + const out = await boot.service.setMany( + 'branding_test', + { workspace_name: 'ObjectStack' }, + boot.writeCtx, + ); + + expect(out.workspace_name.value).toBe('ObjectStack'); + expect(boot.settingRows()).toHaveLength(1); + // The settings-specific trail is unaffected by the generic one failing. + expect(boot.settingAuditRows()).toHaveLength(1); + // …and the operator is told, once, with the consequence and the cause. + const reported = boot.logged.filter((l) => l.includes('config_change audit row NOT written')); + expect(reported).toHaveLength(1); + expect(reported[0]).toContain('plugin-audit'); + }); + + it('reports ONCE per process, not once per write', async () => { + const boot = await bootPlugin({ ledgerThrows: true, userId: 'usr_admin' }); + await boot.service.setMany('branding_test', { workspace_name: 'A' }, boot.writeCtx); + await boot.service.setMany('branding_test', { workspace_name: 'B' }, boot.writeCtx); + await boot.service.setMany('branding_test', { workspace_name: 'C' }, boot.writeCtx); + expect(boot.logged.filter((l) => l.includes('config_change audit row NOT written'))).toHaveLength(1); + // Every one of those writes still landed. + expect(boot.settingAuditRows()).toHaveLength(3); + }); +}); diff --git a/packages/services/service-settings/src/config-change-audit.ts b/packages/services/service-settings/src/config-change-audit.ts new file mode 100644 index 0000000000..af6b0bb26a --- /dev/null +++ b/packages/services/service-settings/src/config-change-audit.ts @@ -0,0 +1,225 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8145] `config_change` — the settings service's row on the PLATFORM audit + * ledger (`sys_audit_log`), alongside the settings-specific one it already + * writes to `sys_setting_audit`. + * + * ## What was broken + * + * `sys_audit_log.action` declares `config_change`, the shipped `config_changes` + * list view filters on it, and `settings-service.types.ts` documented the + * service as emitting `sys_audit_log` rows — while every settings write went to + * `sys_setting_audit` with `action: 'set'` and nothing else. The filter, the + * list view and the dashboard widget over that value were therefore empty for + * the whole life of the enum member (#7675: `PUT /api/settings/branding` then + * `$filter={"action":"config_change"}` → total 0). + * + * ## Dual-write, not reroute — and why (maintainer ruling 2026-08-12 on #7675) + * + * The ruling permits either and leaves the choice to the implementation + * (以实现定契约). Measured, `sys_setting_audit` keeps its rows: + * + * - **It has live consumers.** `docs/qa/platform-checklist/areas/platform-core.json` + * asserts a `sys_setting_audit` row per settings write (namespace/key/scope/ + * `action: 'set'`/`source`/`actor_id`/`new_hash`) as a shipped platform + * behaviour, and `manifest.test.ts` pins the object's registration. Measured + * across `packages/`, `apps/`, `examples/` **and the `objectui` checkout** — + * no other reader, and no settings-audit view in the console. + * - **The two rows are not duplicates.** `sys_setting_audit` records what + * `sys_audit_log` structurally cannot hold: `namespace`, `key`, `scope`, + * `old_hash`/`new_hash`, `source`, `encrypted`, `reason` — a settings-shaped + * ledger with no `object_name`/`record_id` analogue. `sys_audit_log` answers + * the compliance question ("who changed platform configuration, when") that a + * per-namespace table cannot answer across subsystems. + * - **A reroute would leave a shipped-but-never-written table**, which is the + * exact defect class the ruling condemns (审计面宁窄勿谎), and retiring + * `sys_setting_audit` is `packages/platform-objects` surface — a different + * seat — plus a stored-row migration this card is not the place to decide. + * + * Duplicate-row cost is bounded and deliberately accepted: one extra row per + * CHANGED KEY per settings write. Settings writes are admin-rate operations, not + * a hot path — this is nothing like the per-tick/per-chunk writers ADR-0057 D5 + * excluded from auditing (`sys_job_queue`, `sys_upload_session`), and + * `sys_audit_log` carries its own retention (hot 90d → archive) so the growth is + * policy-capped. + * + * ## Best-effort, by construction + * + * `sys_audit_log` belongs to `plugin-audit`, which is OPTIONAL: on a deployment + * without it the table does not exist and every insert here throws. An audit + * write must never turn a successful settings write into an error, so the sink + * swallows and reports — the same posture `settings-service-plugin.ts` takes for + * `sys_setting_audit`, and the same one `plugin-auth` takes for its own explicit + * `sys_audit_log` rows. + * + * ## No plaintext, ever + * + * The sink receives a DIGEST, never a value — `SettingsService` masks an + * encrypted key's digest as `` before it calls. Nothing on this + * path can reach the cleartext, so there is no redaction step to forget. + */ + +import type { IDataEngine } from '@objectstack/spec/contracts'; +import type { SettingsAuditSink, SettingsDiagnosticsLogger } from './settings-service.types.js'; + +/** The object this service records platform configuration changes against. */ +export const CONFIG_CHANGE_OBJECT_NAME = 'sys_setting'; + +/** + * The `sys_audit_log.action` value settings writes carry. + * + * A declared member of the object's action enum + * (`plugin-audit/src/objects/sys-audit-log.object.ts`) and the value the shipped + * `config_changes` list view filters on. Named here rather than spelled inline + * so the writer and its pins cannot drift from each other. + */ +export const CONFIG_CHANGE_ACTION = 'config_change'; + +/** + * Execution context the ledger row is written under. + * + * `sys_audit_log` is `managedBy: 'append-only'` with every field `readonly: + * true` — a platform-owned table written only by internal system paths. This is + * the platform recording its own event, after the settings service's capability, + * lock and validation gates have already passed on the caller's write. + */ +const SYSTEM_CTX = Object.freeze({ isSystem: true }); + +/** Structural minimum of the logger this module reports through. */ +type ConfigChangeLogger = SettingsDiagnosticsLogger & { warn?: (message: string) => void }; + +/** + * Whether the registered `sys_audit_log` schema declares `field`. + * + * `organization_id` is auto-injected by the SchemaRegistry ONLY in multi-tenant + * mode, so it is present on some deployments and absent on others. + * Unconditionally stamping it made every audit INSERT fail on a single-tenant + * stack ("table sys_audit_log has no column named organization_id"); never + * stamping it makes the SecurityPlugin's RLS predicate + * (`organization_id = current_user.organization_id`) hide every row from + * non-platform-admin readers on a multi-tenant one — which would leave the + * `config_changes` view exactly as empty as the defect this card fixes, one + * layer further down. `plugin-audit`'s own writer resolves it the same way, off + * the same lazily-read schema. + * + * Best-effort: an engine that exposes no `getSchema` simply skips the stamp, + * which is the pre-#8145 behaviour of every other explicit `sys_audit_log` + * writer in the repo. + */ +function makeFieldProbe(engine: IDataEngine): (field: string) => boolean { + let fields: Set | null | undefined; + return (field: string): boolean => { + if (fields === undefined) { + fields = null; + try { + // `getSchema` is not on `IDataEngine`; it is an ObjectQL member every + // real engine carries. Guarded rather than declared, so a lean engine + // double stays assignable. + const schema: any = (engine as any).getSchema?.('sys_audit_log'); + const declared = schema?.fields; + if (declared && typeof declared === 'object' && !Array.isArray(declared)) { + fields = new Set(Object.keys(declared)); + } else if (Array.isArray(declared)) { + fields = new Set(declared.map((f: any) => f?.name).filter(Boolean)); + } + } catch { + /* best-effort — absence just means we skip the stamp */ + } + } + return fields != null && fields.has(field); + }; +} + +function safeStringify(value: unknown): string { + try { + return JSON.stringify(value) ?? String(value); + } catch { + return String(value); + } +} + +/** + * Build the `SettingsAuditSink` that records every successful settings write on + * `sys_audit_log` as a `config_change` row. + * + * Exported (rather than left private on the plugin) for the same reason + * `wrapEngineAsSettingsEngine` is: the row shape is the contract this card + * makes true, and a pin that reconstructs it by hand would be pinning the test's + * copy instead of the writer's. + */ +export function buildConfigChangeAuditSink( + engine: IDataEngine, + logger?: ConfigChangeLogger, +): SettingsAuditSink { + const eng: any = engine; + const declares = makeFieldProbe(engine); + let failureReported = false; + + return { + record: async (entry) => { + try { + const actor = entry.actor ?? entry.userId ?? null; + const isReset = entry.action === 'reset'; + const row: Record = { + action: CONFIG_CHANGE_ACTION, + // A strict `sys_user` lookup — only a real user id may land here. + user_id: entry.userId ?? null, + // The first-class principal label (ADR-0014 D2): a user id, a service + // principal, or null for an in-process/boot write. + actor, + object_name: CONFIG_CHANGE_OBJECT_NAME, + // A settings write has no single record id: `sys_setting` is keyed on + // the composite `(namespace, key, scope, user_id)`. Null is the honest + // answer and the shape `plugin-auth`'s run-level `import` row already + // uses; WHICH setting changed is in `metadata` and `new_value`. + record_id: null, + // The digest, never the value — see the module header. A reset has no + // new state to describe. + new_value: isReset + ? null + : safeStringify({ + namespace: entry.namespace, + key: entry.key, + scope: entry.scope, + digest: entry.valueDigest, + }), + tenant_id: entry.tenantId ?? null, + metadata: safeStringify({ + event: isReset ? 'settings.reset' : 'settings.set', + namespace: entry.namespace, + key: entry.key, + scope: entry.scope, + encrypted: entry.encrypted, + ...(entry.requestId ? { requestId: entry.requestId } : {}), + }), + }; + if (declares('organization_id')) row.organization_id = entry.tenantId ?? null; + + await eng.insert('sys_audit_log', row, { context: SYSTEM_CTX }); + } catch (err: any) { + // Reported once per process, not once per settings write: a failure here + // is systemic (plugin-audit not installed, table unreachable), so a line + // per write would train a reader to skim the channel. + if (failureReported) return; + failureReported = true; + const detail = String(err?.message ?? err); + const message = + 'SettingsServicePlugin: config_change audit row NOT written — the settings write itself ' + + 'SUCCEEDED and is on disk, only its `sys_audit_log` entry is missing, and nothing retries it. ' + + 'The `config_changes` list view and any `action: "config_change"` filter will under-report ' + + 'until this is fixed (reported once per process). Cause: ' + + detail + + '. Fix: confirm `sys_audit_log` is reachable — it is owned by the OPTIONAL ' + + '`@objectstack/plugin-audit`, so on a deployment without that plugin the table does not ' + + 'exist and this is expected. `sys_setting_audit` still carries the settings-specific trail.'; + try { + if (logger?.warn) logger.warn(message); + else if (logger?.error) logger.error(message); + } catch { + /* logging must never break the audited write */ + } + } + }, + }; +} diff --git a/packages/services/service-settings/src/index.ts b/packages/services/service-settings/src/index.ts index 3cbe3dcee3..395a4150ed 100644 --- a/packages/services/service-settings/src/index.ts +++ b/packages/services/service-settings/src/index.ts @@ -59,7 +59,21 @@ export { export { SettingsServicePlugin, type SettingsServicePluginOptions, + // #8145 — the settings-shaped ledger writer, published alongside the generic + // sink below so a host that binds its own engine can wire BOTH halves of the + // dual write rather than half of it. + buildSettingAuditWriter, } from './settings-service-plugin.js'; +// #8145 — the `sys_audit_log` `config_change` sink the plugin wires. Published +// because a host that binds its own engine (control-plane mocks, embedded +// runtimes) has to be able to get the SAME ledger row rather than re-deriving +// its shape; the action value and target-object constants travel with it for the +// same reason a pin must not spell them by hand. +export { + buildConfigChangeAuditSink, + CONFIG_CHANGE_ACTION, + CONFIG_CHANGE_OBJECT_NAME, +} from './config-change-audit.js'; export { registerSettingsRoutes, type SettingsRoutesOptions, diff --git a/packages/services/service-settings/src/settings-service-plugin.ts b/packages/services/service-settings/src/settings-service-plugin.ts index 30b54fc46b..1102afb4d5 100644 --- a/packages/services/service-settings/src/settings-service-plugin.ts +++ b/packages/services/service-settings/src/settings-service-plugin.ts @@ -10,6 +10,7 @@ import type { ICryptoProvider } from '@objectstack/spec/contracts'; import type { SettingsAuditWriter, SettingsEngine, SettingsSecretStore } from './settings-service.types.js'; import type { CryptoAdapter } from './crypto-adapter.js'; import { LocalCryptoProvider } from './local-crypto-provider.js'; +import { buildConfigChangeAuditSink } from './config-change-audit.js'; import { registerSettingsRoutes } from './settings-routes.js'; import { settingsObjects, @@ -69,7 +70,10 @@ export interface SettingsServicePluginOptions { * and ship `sys_setting` to the manifest service so the engine * auto-provisions the table. * 2. `start` → `kernel:ready`: bind the data engine (when present), - * wire the audit sink (when present), mount REST routes. + * wire BOTH audit ledgers — `sys_audit_log` `config_change` rows + * ({@link buildConfigChangeAuditSink}) and the settings-specific + * `sys_setting_audit` trail ({@link SettingsServicePlugin.buildAuditWriter}) + * — and mount REST routes. */ export class SettingsServicePlugin implements Plugin { name = SETTINGS_PLUGIN_ID; @@ -197,7 +201,16 @@ export class SettingsServicePlugin implements Plugin { // its narrow, bundled signature. this.service!.bindEngine( wrapEngineAsSettingsEngine(engine), - undefined, + // [#8145] The generic-ledger sink — `sys_audit_log` rows with + // `action: 'config_change'`. This argument was `undefined` from the + // day the slot was declared, which is the whole of #7675's + // `config_change` half: the settings service audited into + // `sys_setting_audit` and nothing else, so the shipped + // `config_changes` list view and every `action: 'config_change'` + // filter were permanently empty. Both ledgers are written now — see + // `config-change-audit.ts` for why the settings-specific one keeps + // its rows rather than being rerouted. + buildConfigChangeAuditSink(engine, ctx.logger), { secretStore: this.buildSecretStore(engine), auditWriter: this.buildAuditWriter(ctx, engine), @@ -323,37 +336,62 @@ export class SettingsServicePlugin implements Plugin { } /** - * Phase 3: append-only writer for `sys_setting_audit`. Failures here - * MUST NOT abort the settings write, so all calls are wrapped in a - * try/catch and reported through the plugin logger. + * Phase 3: append-only writer for `sys_setting_audit`. + * See {@link buildSettingAuditWriter} — this is the plugin-context-bound + * spelling of it. */ private buildAuditWriter(ctx: PluginContext, engine: IDataEngine): SettingsAuditWriter { - const eng: any = engine; - return { - write: async (entry) => { - try { - await eng.insert('sys_setting_audit', { - namespace: entry.namespace, - key: entry.key, - scope: entry.scope, - action: entry.action, - source: entry.source ?? 'api', - actor_id: entry.actorId ?? null, - old_hash: entry.oldHash ?? null, - new_hash: entry.newHash ?? null, - encrypted: !!entry.encrypted, - request_id: entry.requestId ?? null, - reason: entry.reason ?? null, - created_at: new Date().toISOString(), - }, { bypassTenantAudit: true }); - } catch (err: any) { - ctx.logger?.warn?.('SettingsServicePlugin: setting-audit write failed: ' + (err?.message ?? err)); - } - }, - }; + return buildSettingAuditWriter(engine, ctx.logger as any); } } +/** + * Phase 3: append-only writer for `sys_setting_audit`. Failures here + * MUST NOT abort the settings write, so all calls are wrapped in a + * try/catch and reported through the plugin logger. + * + * [#8145] The settings-SHAPED half of the dual write. Its sibling — + * {@link buildConfigChangeAuditSink} — records the same event on the + * platform-wide `sys_audit_log` as a `config_change` row. Neither replaces the + * other: `namespace`/`key`/`scope`/`old_hash`/`new_hash`/`source`/`reason` below + * have no columns on the generic ledger, and `object_name`/`record_id`/`actor` + * have none here. + * + * Lifted out of the class body (and exported) by the same card, for the same + * reason `wrapEngineAsSettingsEngine` is: a pin over the DUAL write has to + * exercise both writers as the plugin builds them. Reconstructing this row shape + * inside a test would make the test assert against its own copy — the one place + * a "the existing ledger is unchanged" claim must not be measured. + */ +export function buildSettingAuditWriter( + engine: IDataEngine, + logger?: { warn?: (message: string) => void }, +): SettingsAuditWriter { + const eng: any = engine; + return { + write: async (entry) => { + try { + await eng.insert('sys_setting_audit', { + namespace: entry.namespace, + key: entry.key, + scope: entry.scope, + action: entry.action, + source: entry.source ?? 'api', + actor_id: entry.actorId ?? null, + old_hash: entry.oldHash ?? null, + new_hash: entry.newHash ?? null, + encrypted: !!entry.encrypted, + request_id: entry.requestId ?? null, + reason: entry.reason ?? null, + created_at: new Date().toISOString(), + }, { bypassTenantAudit: true }); + } catch (err: any) { + logger?.warn?.('SettingsServicePlugin: setting-audit write failed: ' + (err?.message ?? err)); + } + }, + }; +} + /** * Translate an `IDataEngine` instance into the narrower `SettingsEngine` * surface used inside `SettingsService`. The two interfaces diverge on diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index 36df9b856e..fb851625e6 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -1451,16 +1451,29 @@ export class SettingsService { await this.reapRotatedSecret(previousEnc, storedEnc); if (this.audit) { - await this.audit.record({ - namespace, - key, - scope, - userId: ctx.userId, - action: isNull ? 'reset' : 'set', - valueDigest: isEncrypted ? '' : digest, - encrypted: isEncrypted, - requestId: ctx.requestId, - }); + try { + await this.audit.record({ + namespace, + key, + scope, + userId: ctx.userId, + // [#8145] The ledger row's tenant context. Its absence is what makes + // an audit row invisible to RLS readers — see `SettingsAuditSink`. + tenantId: ctx.tenantId, + action: isNull ? 'reset' : 'set', + valueDigest: isEncrypted ? '' : digest, + encrypted: isEncrypted, + requestId: ctx.requestId, + }); + } catch { + // [#8145] Never fail a write because a ledger is unhappy — the same + // rule `auditWriter` below has always had, now applied to both sinks. + // Load-bearing rather than defensive: this sink writes `sys_audit_log`, + // owned by the OPTIONAL plugin-audit, so on a deployment without that + // plugin the insert throws on every single settings write. The sink + // the plugin supplies swallows and reports on its own; this guard is + // what protects a HOST-supplied sink from taking down the write path. + } } if (this.auditWriter) { diff --git a/packages/services/service-settings/src/settings-service.types.ts b/packages/services/service-settings/src/settings-service.types.ts index a3ca335927..6345b3fdde 100644 --- a/packages/services/service-settings/src/settings-service.types.ts +++ b/packages/services/service-settings/src/settings-service.types.ts @@ -13,8 +13,23 @@ * precedence and tag every value with provenance. * - Encrypt-at-rest for `encrypted: true` specifiers using a pluggable * {@link CryptoAdapter}. - * - Emit `sys_audit_log` rows for every successful write (encrypted - * values are masked). + * - Record every successful write on BOTH audit ledgers, best-effort, with + * encrypted values masked to a digest (#8145 — 以实现定契约, so this line + * states what is built rather than what was once intended): + * · `sys_audit_log` with `action: 'config_change'` — the platform-wide + * compliance ledger, via {@link SettingsAuditSink}. This is what the + * shipped `config_changes` list view and any + * `$filter={"action":"config_change"}` read. + * · `sys_setting_audit` with `action: 'set' | 'reset'` — the + * settings-specific append-only trail, via {@link SettingsAuditWriter}, + * carrying `namespace`/`key`/`scope`/`old_hash`/`new_hash`/`source` + * which the generic ledger has no columns for. + * A REFUSED write is not a successful one and emits NEITHER row: the + * fail-closed crypto refusal (#8026) is raised before anything is + * persisted, so no ledger records a write that did not happen. + * Both sinks are optional and both are best-effort — a failing audit write + * is reported, never raised, because it must not undo a settings write that + * already landed. * - Dispatch `runAction` for `action_button` specifiers — used by * "Test connection" / "Send test email" etc. * @@ -115,13 +130,38 @@ export interface SettingsEngine { delete?(objectName: string, opts: { where: Record }): Promise; } -/** Optional audit hook — service-settings won't crash if absent. */ +/** + * Optional audit hook — service-settings won't crash if absent. + * + * [#8145] This is the GENERIC-ledger sink: the plugin wires it to + * `sys_audit_log` with `action: 'config_change'` + * ({@link buildConfigChangeAuditSink} in `config-change-audit.ts`), which is + * what makes the shipped `config_changes` list view and the + * `$filter={"action":"config_change"}` reproduction in #7675 return rows. The + * slot pre-dates that card and was documented as writing there all along; what + * was missing was a plugin supplying it, so every settings write landed only on + * {@link SettingsAuditWriter}'s `sys_setting_audit`. + * + * ⚠️ A sink's `record` MUST NOT be relied on to succeed and must not be used to + * veto a write: the settings service calls it AFTER the row is persisted and + * swallows anything it throws (see the call site). `sys_audit_log` is owned by + * the optional `plugin-audit`, so on a deployment without that plugin the write + * genuinely cannot land, and a settings write must not fail for it. + */ export interface SettingsAuditSink { record(entry: { namespace: string; key: string; scope: SpecifierScope; userId?: string; + /** + * [#8145] Tenant context of the caller, when known. Recorded on the ledger + * row's `tenant_id` (and, where the deployment declares the column, + * `organization_id`) — without it the SecurityPlugin's RLS predicate hides + * every `config_change` row from non-platform-admin readers, leaving the + * `config_changes` view as empty as the defect this fixes. + */ + tenantId?: string; actor?: string; action: 'set' | 'reset'; valueDigest: string; @@ -184,9 +224,15 @@ export interface SettingsSecretStore { /** * Append-only writer for the `sys_setting_audit` object — Phase 3 - * audit trail. Distinct from `SettingsAuditSink` (which still writes - * to the generic `sys_audit_log`) so audit consumers can subscribe + * audit trail. Distinct from {@link SettingsAuditSink} (which writes the + * generic `sys_audit_log` `config_change` row) so audit consumers can subscribe * to settings activity without scanning the firehose. + * + * [#8145] Both are wired in production and both fire on the same write — the + * dual-write half of the 2026-08-12 ruling. The rows are not duplicates: the + * fields below (`namespace`, `key`, `scope`, `oldHash`/`newHash`, `source`, + * `reason`) have no columns on `sys_audit_log`, and this table is what the + * platform QA checklist reads for per-key settings history. */ export interface SettingsAuditWriter { write(entry: {