From 5985bf754a112acb23db4b4ea869a34639463b60 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 2 Jun 2026 10:19:03 +0000 Subject: [PATCH] fix(audit): only stamp organization_id when the audit table declares it (#1532) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On single-tenant stacks the SchemaRegistry's applySystemFields() injects `organization_id` only when multiTenant is true, so sys_audit_log / sys_activity have no `organization_id` column. The audit writer stamped it unconditionally, so every record write triggered an audit INSERT that failed with "table sys_audit_log has no column named organization_id". The error was swallowed ("Audit write failed"), leaving audit logging silently non-functional on the sql (better-sqlite3) driver and paying a failed SQL round-trip on every mutation. The writer now resolves each audit/activity object's registered field set via engine.getSchema() (cached) and only includes `organization_id` when the column actually exists — preserving the RLS tenant stamp in multi-tenant deployments while keeping single-tenant writes valid. tenant_id (an explicitly declared lookup) is unaffected. Adds regression tests covering both the single-tenant (column absent → omitted) and multi-tenant (column present → stamped) paths. --- .../plugin-audit/src/audit-writers.test.ts | 121 ++++++++++++++++++ .../plugins/plugin-audit/src/audit-writers.ts | 62 +++++++-- 2 files changed, 171 insertions(+), 12 deletions(-) create mode 100644 packages/plugins/plugin-audit/src/audit-writers.test.ts diff --git a/packages/plugins/plugin-audit/src/audit-writers.test.ts b/packages/plugins/plugin-audit/src/audit-writers.test.ts new file mode 100644 index 000000000..373852460 --- /dev/null +++ b/packages/plugins/plugin-audit/src/audit-writers.test.ts @@ -0,0 +1,121 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { installAuditWriters } from './audit-writers.js'; + +/** + * Regression coverage for #1532 — on single-tenant stacks the + * SchemaRegistry does NOT auto-inject `organization_id` into + * `sys_audit_log` / `sys_activity`, so the audit writer must not emit that + * column. Previously it stamped `organization_id` unconditionally, making + * every audit INSERT fail with "table sys_audit_log has no column named + * organization_id" (swallowed → audit logging silently non-functional). + */ + +interface CapturedRow { + object: string; + row: Record; +} + +/** + * Build a fake ObjectQL engine that records hook registrations and the rows + * written through `api.sudo().object(name).create(row)`. + * + * @param schemas Map of object short-name → declared field set. Mirrors what + * `engine.getSchema(name)` returns after `applySystemFields` has (or has + * not) injected `organization_id`. + */ +function makeEngine(schemas: Record) { + const hooks = new Map any>>(); + const created: CapturedRow[] = []; + + const sudoApi = { + object(name: string) { + return { + async create(row: Record) { + created.push({ object: name, row }); + return { id: 'generated-id', ...row }; + }, + }; + }, + }; + // `writeAudit` calls `ctx.api.sudo()` to get the object accessor above. + const api = { sudo: () => sudoApi }; + + const engine = { + getSchema(name: string) { + const fields = schemas[name]; + if (!fields) return undefined; + return { name, fields: Object.fromEntries(fields.map((f) => [f, { type: 'text' }])) }; + }, + registerHook(event: string, fn: (ctx: any) => any) { + const list = hooks.get(event) ?? []; + list.push(fn); + hooks.set(event, list); + }, + unregisterHooksByPackage() { + /* no-op */ + }, + logger: { warn() {} }, + }; + + async function fire(event: string, ctx: any) { + for (const fn of hooks.get(event) ?? []) { + await fn({ ...ctx, event, api }); + } + } + + return { engine, fire, created }; +} + +const SINGLE_TENANT = { + // No `organization_id` — single-tenant stacks skip the auto-injection. + sys_audit_log: ['id', 'action', 'user_id', 'object_name', 'record_id', 'old_value', 'new_value', 'tenant_id'], + sys_activity: ['id', 'type', 'timestamp', 'summary', 'actor_id', 'object_name', 'record_id', 'record_label', 'metadata'], +}; + +const MULTI_TENANT = { + sys_audit_log: [...SINGLE_TENANT.sys_audit_log, 'organization_id'], + sys_activity: [...SINGLE_TENANT.sys_activity, 'organization_id'], +}; + +describe('audit writers — organization_id stamping (#1532)', () => { + it('omits organization_id on single-tenant tables that lack the column', async () => { + const { engine, fire, created } = makeEngine(SINGLE_TENANT); + installAuditWriters(engine as any, 'test.audit'); + + await fire('afterInsert', { + object: 'crm_lead', + input: { id: 'lead-1' }, + result: { id: 'lead-1', name: 'Acme' }, + session: {}, + }); + + const audit = created.find((c) => c.object === 'sys_audit_log'); + const activity = created.find((c) => c.object === 'sys_activity'); + expect(audit).toBeDefined(); + expect(activity).toBeDefined(); + // The fix: no undeclared column is emitted, so the INSERT would succeed. + expect('organization_id' in audit!.row).toBe(false); + expect('organization_id' in activity!.row).toBe(false); + // tenant_id is schema-declared and still written. + expect('tenant_id' in audit!.row).toBe(true); + }); + + it('stamps organization_id on multi-tenant tables when the column exists', async () => { + const { engine, fire, created } = makeEngine(MULTI_TENANT); + installAuditWriters(engine as any, 'test.audit'); + + await fire('afterInsert', { + object: 'crm_lead', + input: { id: 'lead-1' }, + result: { id: 'lead-1', name: 'Acme', organization_id: 'org-9' }, + session: { tenantId: 'org-9', userId: 'user-1' }, + }); + + const audit = created.find((c) => c.object === 'sys_audit_log'); + const activity = created.find((c) => c.object === 'sys_activity'); + expect(audit?.row.organization_id).toBe('org-9'); + expect(activity?.row.organization_id).toBe('org-9'); + }); +}); diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index ab95e3661..83881b266 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -147,6 +147,37 @@ export function installAuditWriters( engine.unregisterHooksByPackage(packageId); } + // Whether a given object's *registered* schema declares a field. The + // SchemaRegistry auto-injects `organization_id` only in multi-tenant mode + // (`applySystemFields({ multiTenant })`), so on single-tenant stacks the + // `sys_audit_log` / `sys_activity` tables have no `organization_id` column. + // Unconditionally stamping it there made every audit INSERT fail with + // "table sys_audit_log has no column named organization_id" (the error was + // swallowed, so audit logging was silently non-functional). Resolve the + // field set lazily from the engine schema and cache it — object schemas are + // static after registration. + const fieldSetCache = new Map | null>(); + const objectHasField = (objectName: string, field: string): boolean => { + let set = fieldSetCache.get(objectName); + if (set === undefined) { + set = null; + try { + const schema: any = + typeof (engine as any).getSchema === 'function' ? (engine as any).getSchema(objectName) : null; + const fields = schema?.fields; + if (fields && typeof fields === 'object' && !Array.isArray(fields)) { + set = new Set(Object.keys(fields)); + } else if (Array.isArray(fields)) { + set = new Set(fields.map((f: any) => f?.name).filter(Boolean)); + } + } catch { + /* ignore — best-effort; absence just means we skip the stamp */ + } + fieldSetCache.set(objectName, set); + } + return set != null && set.has(field); + }; + /** * beforeUpdate / beforeDelete: capture "previous" snapshot via api.sudo() * so we can compute the diff in the afterXxx hook. We attach the snapshot @@ -250,17 +281,21 @@ export function installAuditWriters( record_id: recordId ?? null, old_value: oldValue ? safeStringify(oldValue) : null, new_value: newValue ? safeStringify(newValue) : null, - // `tenant_id` is the schema-declared "tenant context" lookup; the - // platform-default `organization_id` column is what RLS gates on - // (`organization_id = current_user.organization_id`). The audit - // writer runs through `api.sudo()` which bypasses the - // SecurityPlugin's auto-stamping of `organization_id`, so we - // stamp both columns explicitly here. Without `organization_id`, - // non-admin members would see 0 rows on Setup dashboards because - // RLS would deny every audit row as wrong-tenant. - organization_id: tenantId ?? null, + // `tenant_id` is the schema-declared "tenant context" lookup. tenant_id: tenantId ?? null, }; + // The platform-default `organization_id` column is what RLS gates on + // (`organization_id = current_user.organization_id`). The audit writer + // runs through `api.sudo()` which bypasses the SecurityPlugin's + // auto-stamping of `organization_id`, so we stamp it explicitly here — + // without it, non-admin members would see 0 rows on Setup dashboards + // because RLS would deny every audit row as wrong-tenant. But the column + // only exists in multi-tenant deployments (the SchemaRegistry auto-injects + // it conditionally); stamping it on a single-tenant table that lacks the + // column made every audit INSERT fail. Only stamp it when declared. + if (objectHasField('sys_audit_log', 'organization_id')) { + auditRow.organization_id = tenantId ?? null; + } const label = recordLabel(after ?? before, recordId ?? ''); const summary = @@ -280,10 +315,13 @@ export function installAuditWriters( record_id: recordId ?? null, record_label: label, metadata: newValue || oldValue ? safeStringify({ old: oldValue, new: newValue }) : null, - // Same rationale as auditRow: stamp the tenant column so RLS - // matches the recipient's organization on read. - organization_id: tenantId ?? null, }; + // Same rationale as auditRow: stamp the tenant column so RLS matches the + // recipient's organization on read — but only when the (auto-injected) + // column actually exists, so single-tenant activity writes don't fail. + if (objectHasField('sys_activity', 'organization_id')) { + activityRow.organization_id = tenantId ?? null; + } try { const sys = api.sudo();