From 81b5a6cf0112dd807e4caeaa733deaed48162b78 Mon Sep 17 00:00:00 2001 From: ZacharyZcR Date: Tue, 28 Jul 2026 19:23:46 +0800 Subject: [PATCH] stop deleting audit trails, and say when they are dropped (#1132) Two ways audit evidence still disappeared silently. Deleting an account removed its audit entries and session recordings outright. #1128 relaxed those foreign keys to ON DELETE SET NULL, but deleteUserAndRelatedData deletes the rows explicitly, so the schema change had no effect on the path that actually matters. Both repositories gain anonymizeByUserId, which nulls the reference and leaves the row; username is already denormalised on both tables, so entries stay attributable to whoever acted. Separately, the log pruned itself at a hard-coded 10000 rows with no signal. Entries well inside any retention window were discarded and nothing recorded it. Retention is now configurable by age via AUDIT_LOG_RETENTION_DAYS, the row cap via AUDIT_LOG_MAX_ENTRIES, and the two are reported differently: expiring an old entry is routine and logged at info, while hitting the cap means the ceiling is too low for how much this install audits and is logged at warn, naming the range discarded and how to stop it. --- .../repositories/audit-log-repository.ts | 122 ++++++++++-- .../session-recording-repository.ts | 19 ++ .../database/routes/delete-user-data.ts | 6 +- .../repositories/audit-log-repository.test.ts | 44 ++++- .../repositories/audit-log-retention.test.ts | 174 ++++++++++++++++++ 5 files changed, 348 insertions(+), 17 deletions(-) create mode 100644 src/backend/tests/database/repositories/audit-log-retention.test.ts diff --git a/src/backend/database/repositories/audit-log-repository.ts b/src/backend/database/repositories/audit-log-repository.ts index 6e9c86a9..c7682546 100644 --- a/src/backend/database/repositories/audit-log-repository.ts +++ b/src/backend/database/repositories/audit-log-repository.ts @@ -1,6 +1,8 @@ -import { and, asc, desc, eq, gte, inArray, lte, sql } from "drizzle-orm"; +import { and, asc, desc, eq, gte, inArray, lt, lte, sql } from "drizzle-orm"; import { auditLogs } from "../db/schema.js"; import type { DatabaseContext } from "./database-context.js"; +import { sqlTimestampDaysAgo } from "./sql-timestamp.js"; +import { databaseLogger } from "../../utils/logger.js"; export type AuditLogRecord = typeof auditLogs.$inferSelect; export type NewAuditLogRecord = typeof auditLogs.$inferInsert; @@ -19,8 +21,31 @@ export type AuditLogPage = { total: number; }; -const PRUNE_MAX = 10000; -const PRUNE_TARGET = 9000; +export const AUDIT_RETENTION_DAYS_ENV = "AUDIT_LOG_RETENTION_DAYS"; +export const AUDIT_MAX_ENTRIES_ENV = "AUDIT_LOG_MAX_ENTRIES"; + +const DEFAULT_MAX_ENTRIES = 10000; +const PRUNE_TARGET_RATIO = 0.9; + +function positiveIntEnv(key: string, env: NodeJS.ProcessEnv): number | null { + const raw = Number(env[key]); + return Number.isFinite(raw) && raw > 0 ? Math.floor(raw) : null; +} + +/** + * How long entries are kept. Unset means "no time limit", in which case only + * the row cap applies. + */ +export function auditRetentionDays( + env: NodeJS.ProcessEnv = process.env, +): number | null { + return positiveIntEnv(AUDIT_RETENTION_DAYS_ENV, env); +} + +/** Hard ceiling on stored entries, so a busy install cannot fill the disk. */ +export function auditMaxEntries(env: NodeJS.ProcessEnv = process.env): number { + return positiveIntEnv(AUDIT_MAX_ENTRIES_ENV, env) ?? DEFAULT_MAX_ENTRIES; +} export class AuditLogRepository { constructor( @@ -70,6 +95,29 @@ export class AuditLogRepository { return rows.map((row) => row.action); } + /** + * Detaches entries from a user being deleted instead of removing them. + * + * The schema already relaxed this foreign key to ON DELETE SET NULL, but the + * account-deletion path deletes the rows explicitly, which undoes that. An + * audit trail that vanishes with the account it recorded cannot answer the + * question it exists for, and offboarding is exactly when that question gets + * asked. `username` is denormalised, so the entry stays attributable. + */ + async anonymizeByUserId(userId: string): Promise { + const rows = await this.context.drizzle + .update(auditLogs) + .set({ userId: null }) + .where(eq(auditLogs.userId, userId)) + .returning({ id: auditLogs.id }); + + if (rows.length > 0) { + await this.afterWrite(); + } + + return rows.length; + } + async deleteByUserId(userId: string): Promise { const rows = await this.context.drizzle .delete(auditLogs) @@ -105,28 +153,74 @@ export class AuditLogRepository { } private async pruneIfNeeded(): Promise { + await this.pruneExpired(); + await this.pruneOverflow(); + } + + /** Drops entries past the configured retention window. */ + private async pruneExpired(): Promise { + const days = auditRetentionDays(); + if (days === null) return; + + const cutoff = sqlTimestampDaysAgo(days); + const rows = await this.context.drizzle + .delete(auditLogs) + .where(lt(auditLogs.timestamp, cutoff)) + .returning({ id: auditLogs.id }); + + if (rows.length > 0) { + databaseLogger.info( + `Pruned ${rows.length} audit entries past retention`, + { + operation: "audit_retention_prune", + removed: rows.length, + retentionDays: days, + cutoff, + }, + ); + } + } + + /** + * Enforces the row cap. Unlike retention this discards entries that are still + * within the window, so it is reported as a warning: it means the ceiling is + * too low for how much this install audits, and evidence is being lost. + */ + private async pruneOverflow(): Promise { + const max = auditMaxEntries(); const countResult = await this.context.drizzle .select({ count: sql`COUNT(*)` }) .from(auditLogs); const count = countResult[0]?.count ?? 0; - if (count < PRUNE_MAX) { - return; - } + if (count < max) return; - const deleteCount = count - PRUNE_TARGET; + const deleteCount = count - Math.floor(max * PRUNE_TARGET_RATIO); const rows = await this.context.drizzle - .select({ id: auditLogs.id }) + .select({ id: auditLogs.id, timestamp: auditLogs.timestamp }) .from(auditLogs) .orderBy(asc(auditLogs.timestamp)) .limit(deleteCount); - const ids = rows.map((row) => row.id); + if (rows.length === 0) return; - if (ids.length > 0) { - await this.context.drizzle - .delete(auditLogs) - .where(inArray(auditLogs.id, ids)); - } + await this.context.drizzle.delete(auditLogs).where( + inArray( + auditLogs.id, + rows.map((row) => row.id), + ), + ); + + databaseLogger.warn( + `Audit log hit its ${max}-entry cap; discarded ${rows.length} entries`, + { + operation: "audit_overflow_prune", + removed: rows.length, + maxEntries: max, + oldestRemoved: rows[0]?.timestamp, + newestRemoved: rows[rows.length - 1]?.timestamp, + hint: `Raise ${AUDIT_MAX_ENTRIES_ENV}, or set ${AUDIT_RETENTION_DAYS_ENV} and export older entries before they are dropped.`, + }, + ); } private async afterWrite(): Promise { diff --git a/src/backend/database/repositories/session-recording-repository.ts b/src/backend/database/repositories/session-recording-repository.ts index fa3efc10..9259d108 100644 --- a/src/backend/database/repositories/session-recording-repository.ts +++ b/src/backend/database/repositories/session-recording-repository.ts @@ -197,6 +197,25 @@ export class SessionRecordingRepository { return rows.length > 0; } + /** + * Detaches recordings from a user being deleted instead of removing them. + * A recording is evidence about the host as much as about the person, and the + * file stays on disk regardless — deleting only the row would orphan it. + */ + async anonymizeByUserId(userId: string): Promise { + const rows = await this.context.drizzle + .update(sessionRecordings) + .set({ userId: null }) + .where(eq(sessionRecordings.userId, userId)) + .returning({ id: sessionRecordings.id }); + + if (rows.length > 0) { + await this.afterWrite(); + } + + return rows.length; + } + async deleteByUserId(userId: string): Promise { const rows = await this.context.drizzle .delete(sessionRecordings) diff --git a/src/backend/database/routes/delete-user-data.ts b/src/backend/database/routes/delete-user-data.ts index b3ec33e1..268cd92d 100644 --- a/src/backend/database/routes/delete-user-data.ts +++ b/src/backend/database/routes/delete-user-data.ts @@ -44,7 +44,9 @@ export async function deleteUserAndRelatedData(userId: string): Promise { userId, ); - await createCurrentSessionRecordingRepository().deleteByUserId(userId); + // Retained rather than deleted: these outlive the account by design. + // See anonymizeByUserId on each repository. + await createCurrentSessionRecordingRepository().anonymizeByUserId(userId); await createCurrentRbacAccessRepository().deleteHostAccessForUserReferences( userId, @@ -56,7 +58,7 @@ export async function deleteUserAndRelatedData(userId: string): Promise { await createCurrentRoleRepository().removeAllRolesFromUser(userId); await createCurrentAlertRepository().deleteByUserId(userId); - await createCurrentAuditLogRepository().deleteByUserId(userId); + await createCurrentAuditLogRepository().anonymizeByUserId(userId); await createCurrentSshCredentialUsageRepository().deleteByUserId(userId); diff --git a/src/backend/tests/database/repositories/audit-log-repository.test.ts b/src/backend/tests/database/repositories/audit-log-repository.test.ts index e113e89e..91d80261 100644 --- a/src/backend/tests/database/repositories/audit-log-repository.test.ts +++ b/src/backend/tests/database/repositories/audit-log-repository.test.ts @@ -28,7 +28,7 @@ describe("AuditLogRepository", () => { CREATE TABLE audit_logs ( id INTEGER PRIMARY KEY AUTOINCREMENT, - user_id TEXT NOT NULL, + user_id TEXT REFERENCES users(id) ON DELETE SET NULL, username TEXT NOT NULL, action TEXT NOT NULL, resource_type TEXT NOT NULL, @@ -129,4 +129,46 @@ describe("AuditLogRepository", () => { ).logs.map((log) => log.userId), ).toEqual(["user-2"]); }); + + it("keeps entries when their user is deleted, detaching instead of removing", async () => { + const repo = await createRepository(); + + await repo.create({ + userId: "user-1", + username: "alice", + action: "delete_host", + resourceType: "host", + resourceId: "9", + success: true, + timestamp: "2026-07-01T00:00:00.000Z", + }); + await repo.create({ + userId: "user-2", + username: "bob", + action: "create_host", + resourceType: "host", + resourceId: "8", + success: true, + timestamp: "2026-07-02T00:00:00.000Z", + }); + + expect(await repo.anonymizeByUserId("user-1")).toBe(1); + + const { logs } = await repo.listPage({ filters: {}, limit: 10, offset: 0 }); + expect(logs).toHaveLength(2); + + const detached = logs.find((log) => log.action === "delete_host"); + // The account is gone; the entry and its actor name are not. + expect(detached?.userId).toBeNull(); + expect(detached?.username).toBe("alice"); + expect(logs.find((log) => log.action === "create_host")?.userId).toBe( + "user-2", + ); + }); + + it("reports nothing to detach for a user with no entries", async () => { + const repo = await createRepository(); + + expect(await repo.anonymizeByUserId("user-2")).toBe(0); + }); }); diff --git a/src/backend/tests/database/repositories/audit-log-retention.test.ts b/src/backend/tests/database/repositories/audit-log-retention.test.ts new file mode 100644 index 00000000..a8d768f4 --- /dev/null +++ b/src/backend/tests/database/repositories/audit-log-retention.test.ts @@ -0,0 +1,174 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const logs = vi.hoisted(() => ({ info: vi.fn(), warn: vi.fn() })); + +vi.mock("../../../utils/logger.js", () => ({ + databaseLogger: logs, +})); + +const { TestSqliteDatabase } = await import("./test-support.js"); +const { + AuditLogRepository, + auditRetentionDays, + auditMaxEntries, + AUDIT_RETENTION_DAYS_ENV, + AUDIT_MAX_ENTRIES_ENV, +} = await import("../../../database/repositories/audit-log-repository.js"); + +let adapter: InstanceType | null = null; +const savedEnv: Record = {}; + +beforeEach(() => { + logs.info.mockReset(); + logs.warn.mockReset(); + for (const key of [AUDIT_RETENTION_DAYS_ENV, AUDIT_MAX_ENTRIES_ENV]) { + savedEnv[key] = process.env[key]; + delete process.env[key]; + } +}); + +afterEach(async () => { + for (const [key, value] of Object.entries(savedEnv)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + if (adapter) { + await adapter.close(); + adapter = null; + } +}); + +async function createRepository() { + adapter = new TestSqliteDatabase(); + const context = await adapter.connect(); + adapter.exec(` + CREATE TABLE audit_logs ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + user_id TEXT, + username TEXT NOT NULL, + action TEXT NOT NULL, + resource_type TEXT NOT NULL, + resource_id TEXT, + resource_name TEXT, + details TEXT, + ip_address TEXT, + user_agent TEXT, + success INTEGER NOT NULL, + error_message TEXT, + timestamp TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP + ); + `); + return new AuditLogRepository(context); +} + +function daysAgo(days: number): string { + const d = new Date(Date.now() - days * 24 * 60 * 60 * 1000); + return d.toISOString().slice(0, 19).replace("T", " "); +} + +async function seed( + repo: Awaited>, + timestamp: string, + action = "create_host", +) { + await repo.create({ + userId: "u-1", + username: "alice", + action, + resourceType: "host", + success: true, + timestamp, + }); +} + +describe("audit retention configuration", () => { + it("has no time limit unless one is configured", () => { + expect(auditRetentionDays({})).toBeNull(); + expect(auditRetentionDays({ [AUDIT_RETENTION_DAYS_ENV]: "90" })).toBe(90); + }); + + it("ignores values that are not a positive count", () => { + for (const bad of ["0", "-5", "", "abc"]) { + expect( + auditRetentionDays({ [AUDIT_RETENTION_DAYS_ENV]: bad }), + ).toBeNull(); + } + }); + + it("falls back to the built-in cap", () => { + expect(auditMaxEntries({})).toBe(10000); + expect(auditMaxEntries({ [AUDIT_MAX_ENTRIES_ENV]: "250" })).toBe(250); + expect(auditMaxEntries({ [AUDIT_MAX_ENTRIES_ENV]: "-1" })).toBe(10000); + }); +}); + +describe("audit retention pruning", () => { + it("keeps everything when no retention is set", async () => { + const repo = await createRepository(); + + await seed(repo, daysAgo(400)); + await seed(repo, daysAgo(1)); + + const { total } = await repo.listPage({ + filters: {}, + limit: 10, + offset: 0, + }); + expect(total).toBe(2); + expect(logs.info).not.toHaveBeenCalled(); + }); + + it("drops entries past the retention window and says so", async () => { + process.env[AUDIT_RETENTION_DAYS_ENV] = "30"; + const repo = await createRepository(); + + await seed(repo, daysAgo(90), "old_action"); + await seed(repo, daysAgo(5), "recent_action"); + + const { logs: rows } = await repo.listPage({ + filters: {}, + limit: 10, + offset: 0, + }); + expect(rows.map((r) => r.action)).toEqual(["recent_action"]); + + expect(logs.info).toHaveBeenCalledWith( + expect.stringContaining("past retention"), + expect.objectContaining({ operation: "audit_retention_prune" }), + ); + }); + + it("warns when the row cap discards entries still inside the window", async () => { + process.env[AUDIT_MAX_ENTRIES_ENV] = "5"; + const repo = await createRepository(); + + for (let i = 0; i < 6; i++) { + await seed(repo, daysAgo(10 - i), `action_${i}`); + } + + // The cap is not a retention policy: these entries were still current. + expect(logs.warn).toHaveBeenCalledWith( + expect.stringContaining("cap"), + expect.objectContaining({ + operation: "audit_overflow_prune", + maxEntries: 5, + }), + ); + + const { total } = await repo.listPage({ + filters: {}, + limit: 20, + offset: 0, + }); + expect(total).toBeLessThan(6); + }); + + it("stays quiet while under the cap", async () => { + process.env[AUDIT_MAX_ENTRIES_ENV] = "100"; + const repo = await createRepository(); + + await seed(repo, daysAgo(1)); + + expect(logs.warn).not.toHaveBeenCalled(); + }); +});