From 3d89d7d8eb1ce3bed46e3ed9b45668cad8b7c0be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hartmut=20N=C3=B6renberg?= Date: Fri, 17 Apr 2026 09:25:15 +0200 Subject: [PATCH] security: redact sensitive fields in audit DB entries (#46) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createAuditEntry now deep-walks before/after/metadata and replaces values of password, newPassword, currentPassword, passwordHash, token, accessToken, refreshToken, sessionToken, apiKey, authorization, cookie, secret, totpSecret, backupCode(s) with "[REDACTED]" before the JSONB write. The pino logger already redacts these paths for stdout (see lib/logger.ts), but DB writes had no equivalent guard — the AI chat loop at assistant-chat-loop.ts:265 blindly stores parsedArgs from tool calls (e.g. set_user_password, create_user) into the AuditLog table. Matching is case-insensitive; nested objects and arrays are recursed to a depth of 8. Diffs are computed post-redaction so UPDATE entries that only changed a sensitive field are correctly collapsed to no-op. Co-Authored-By: Claude Opus 4.7 --- .../api/src/__tests__/audit-redaction.test.ts | 177 ++++++++++++++++++ packages/api/src/lib/audit.ts | 89 ++++++++- 2 files changed, 260 insertions(+), 6 deletions(-) create mode 100644 packages/api/src/__tests__/audit-redaction.test.ts diff --git a/packages/api/src/__tests__/audit-redaction.test.ts b/packages/api/src/__tests__/audit-redaction.test.ts new file mode 100644 index 0000000..c954203 --- /dev/null +++ b/packages/api/src/__tests__/audit-redaction.test.ts @@ -0,0 +1,177 @@ +import { describe, expect, it, vi } from "vitest"; +import { __test__, createAuditEntry } from "../lib/audit.js"; + +const { redactSensitive } = __test__; + +describe("audit log redaction", () => { + describe("redactSensitive", () => { + it("redacts top-level password fields", () => { + const result = redactSensitive({ userId: "u1", password: "hunter2" }); + expect(result).toEqual({ userId: "u1", password: "[REDACTED]" }); + }); + + it("redacts nested password fields", () => { + const result = redactSensitive({ + params: { userId: "u1", password: "hunter2" }, + executed: true, + }); + expect(result).toEqual({ + params: { userId: "u1", password: "[REDACTED]" }, + executed: true, + }); + }); + + it("redacts password inside arrays", () => { + const result = redactSensitive({ + users: [ + { id: "1", password: "secret" }, + { id: "2", password: "other" }, + ], + }); + expect(result).toEqual({ + users: [ + { id: "1", password: "[REDACTED]" }, + { id: "2", password: "[REDACTED]" }, + ], + }); + }); + + it("is case-insensitive", () => { + const result = redactSensitive({ + Password: "x", + PASSWORD: "y", + newPassword: "z", + currentPassword: "a", + }); + expect(result).toEqual({ + Password: "[REDACTED]", + PASSWORD: "[REDACTED]", + newPassword: "[REDACTED]", + currentPassword: "[REDACTED]", + }); + }); + + it("redacts tokens, secrets, and cookies", () => { + const result = redactSensitive({ + token: "t", + accessToken: "a", + refreshToken: "r", + apiKey: "k", + secret: "s", + totpSecret: "ts", + authorization: "Bearer x", + cookie: "sid=abc", + }); + for (const v of Object.values(result as Record)) { + expect(v).toBe("[REDACTED]"); + } + }); + + it("leaves non-sensitive fields untouched", () => { + const result = redactSensitive({ name: "Alice", email: "a@b.c", count: 42, flag: true }); + expect(result).toEqual({ name: "Alice", email: "a@b.c", count: 42, flag: true }); + }); + + it("handles null, undefined, and primitives", () => { + expect(redactSensitive(null)).toBe(null); + expect(redactSensitive(undefined)).toBe(undefined); + expect(redactSensitive("string")).toBe("string"); + expect(redactSensitive(123)).toBe(123); + }); + + it("stops recursion at MAX_REDACT_DEPTH", () => { + // Build a ~15-deep nested object; redaction should still work near the + // top but bail past the depth limit without throwing. + let v: Record = { password: "leaf" }; + for (let i = 0; i < 15; i++) { + v = { nested: v }; + } + expect(() => redactSensitive(v)).not.toThrow(); + }); + }); + + describe("createAuditEntry", () => { + it("redacts passwords in `after` before the DB write", async () => { + const create = vi.fn().mockResolvedValue({}); + const db = { auditLog: { create } }; + + await createAuditEntry({ + db: db as never, + entityType: "AiToolExecution", + entityId: "call_1", + action: "CREATE", + after: { params: { userId: "u1", password: "cleartext" }, executed: true }, + }); + + expect(create).toHaveBeenCalledTimes(1); + const data = create.mock.calls[0]![0]!.data; + const changes = data.changes as { after?: { params?: { password?: string } } }; + expect(changes.after?.params?.password).toBe("[REDACTED]"); + expect(changes.after?.params).toMatchObject({ userId: "u1" }); + }); + + it("redacts passwords in before/after when non-sensitive fields also changed", async () => { + const create = vi.fn().mockResolvedValue({}); + const db = { auditLog: { create } }; + + await createAuditEntry({ + db: db as never, + entityType: "User", + entityId: "u1", + action: "UPDATE", + before: { password: "old", name: "Alice" }, + after: { password: "new", name: "Bob" }, + }); + + expect(create).toHaveBeenCalledTimes(1); + const changes = create.mock.calls[0]![0]!.data.changes as { + before?: Record; + after?: Record; + diff?: Record; + }; + expect(changes.before?.["password"]).toBe("[REDACTED]"); + expect(changes.after?.["password"]).toBe("[REDACTED]"); + // The name change survives in the diff, but the password diff collapses + // (both values are the same placeholder). + expect(changes.diff).toEqual({ name: { old: "Alice", new: "Bob" } }); + }); + + it("skips UPDATE when both snapshots redact to the same value (empty diff)", async () => { + const create = vi.fn().mockResolvedValue({}); + const db = { auditLog: { create } }; + + await createAuditEntry({ + db: db as never, + entityType: "User", + entityId: "u1", + action: "UPDATE", + before: { password: "old" }, + after: { password: "new" }, + }); + + // Both redact to [REDACTED], diff is empty, create should NOT be called. + expect(create).not.toHaveBeenCalled(); + }); + + it("redacts sensitive fields in metadata", async () => { + const create = vi.fn().mockResolvedValue({}); + const db = { auditLog: { create } }; + + await createAuditEntry({ + db: db as never, + entityType: "Webhook", + entityId: "wh_1", + action: "CREATE", + after: { url: "https://example.com/hook" }, + metadata: { signingSecret: "ss", apiKey: "leak" }, + }); + + const changes = create.mock.calls[0]![0]!.data.changes as { + metadata?: Record; + }; + expect(changes.metadata?.["apiKey"]).toBe("[REDACTED]"); + // signingSecret is not in the set — verify the list is intentional + expect(changes.metadata?.["signingSecret"]).toBe("ss"); + }); + }); +}); diff --git a/packages/api/src/lib/audit.ts b/packages/api/src/lib/audit.ts index 84bd8d2..34135da 100644 --- a/packages/api/src/lib/audit.ts +++ b/packages/api/src/lib/audit.ts @@ -20,6 +20,61 @@ interface CreateAuditEntryParams { const INTERNAL_FIELDS = new Set(["id", "createdAt", "updatedAt"]); +// Field names whose values are never safe to persist into the audit log. +// Matching is case-insensitive and applied at every level of the object graph. +const SENSITIVE_FIELD_NAMES = new Set([ + "password", + "newpassword", + "currentpassword", + "oldpassword", + "passwordhash", + "passwordconfirmation", + "confirmpassword", + "token", + "accesstoken", + "refreshtoken", + "sessiontoken", + "apikey", + "authorization", + "cookie", + "secret", + "totpsecret", + "backupcode", + "backupcodes", +]); + +const REDACTED_PLACEHOLDER = "[REDACTED]"; +const MAX_REDACT_DEPTH = 8; + +/** + * Recursively strip values of fields whose names appear in SENSITIVE_FIELD_NAMES. + * Used to prevent password/token leaks into the audit log JSONB column. + * + * The pino logger has its own redact config for stdout; this function is the + * DB-write equivalent. + */ +function redactSensitive(value: unknown, depth: number = 0): unknown { + if (depth > MAX_REDACT_DEPTH) return value; + if (value === null || value === undefined) return value; + if (Array.isArray(value)) { + return value.map((v) => redactSensitive(v, depth + 1)); + } + if (typeof value === "object") { + const out: Record = {}; + for (const [k, v] of Object.entries(value as Record)) { + if (SENSITIVE_FIELD_NAMES.has(k.toLowerCase())) { + out[k] = REDACTED_PLACEHOLDER; + } else { + out[k] = redactSensitive(v, depth + 1); + } + } + return out; + } + return value; +} + +export const __test__ = { redactSensitive, SENSITIVE_FIELD_NAMES }; + /** * Compare two snapshots and return only the changed fields. * Skips internal fields (id, createdAt, updatedAt). @@ -91,15 +146,34 @@ export function generateSummary( */ export async function createAuditEntry(params: CreateAuditEntryParams): Promise { try { - const { db, entityType, entityId, entityName, action, userId, before, after, source, metadata } = params; + const { + db, + entityType, + entityId, + entityName, + action, + userId, + before, + after, + source, + metadata, + } = params; const auditLog = (db as Partial).auditLog; if (!auditLog || typeof auditLog.create !== "function") { return; } + // Redact sensitive field values before anything else — diffs and summaries + // must all be derived from already-sanitised snapshots. + const safeBefore = before ? (redactSensitive(before) as Record) : undefined; + const safeAfter = after ? (redactSensitive(after) as Record) : undefined; + const safeMetadata = metadata + ? (redactSensitive(metadata) as Record) + : undefined; + // Compute diff if both snapshots are available - const diff = before && after ? computeDiff(before, after) : undefined; + const diff = safeBefore && safeAfter ? computeDiff(safeBefore, safeAfter) : undefined; // Skip UPDATE entries where nothing actually changed if (action === "UPDATE" && diff && Object.keys(diff).length === 0) { @@ -111,10 +185,10 @@ export async function createAuditEntry(params: CreateAuditEntryParams): Promise< // Build the changes JSONB payload const changes: Record = {}; - if (before) changes.before = before; - if (after) changes.after = after; + if (safeBefore) changes.before = safeBefore; + if (safeAfter) changes.after = safeAfter; if (diff) changes.diff = diff; - if (metadata) changes.metadata = metadata; + if (safeMetadata) changes.metadata = safeMetadata; await auditLog.create({ data: { @@ -130,6 +204,9 @@ export async function createAuditEntry(params: CreateAuditEntryParams): Promise< }); } catch (error) { // Fire-and-forget: log but never propagate - logger.error({ err: error, entityType: params.entityType, entityId: params.entityId }, "Failed to create audit entry"); + logger.error( + { err: error, entityType: params.entityType, entityId: params.entityId }, + "Failed to create audit entry", + ); } }