From 4633460d9af2e48506b1927a8f2cbc4edd7c50cb Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 18 Jun 2026 11:11:27 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A0=EF=B8=8F=20fix:=20Address=20Codex?= =?UTF-8?q?=20round-3=20on=20the=20audit-log=20substrate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - R3-3 (P2): make a grant re-assert a true no-op — move grantedAt/grantedBy to $setOnInsert so an existing grant is never silently mutated when the audit is skipped (created:false now means nothing changed). grantedAt/grantedBy record the original grant. - R3-2 (P2): report CSV export truncation exactly. streamAuditLogEntries returns { count, truncated }; truncated is true only when rows existed beyond the cap, so an exact-cap export is no longer falsely marked truncated. - R3-5 (P2): block AuditLog.insertMany (another bulk path that skips the save hook and could inject forged seq/prevHash/hash and poison the chain). Tests: +insertMany rejection, +exact-cap vs truncated stream cases, +exact-cap export-not-truncated handler case. ds 142, api 108 green. R3-1 (deprecated query aliases) and R3-4 (role-deletion cascade audit) are re-flags of R2-3/R2-6 — holding the prior decisions (pre-release surface; separate roles.ts workflow tracked as a follow-up), pending maintainer direction. --- packages/api/src/admin/auditLog.spec.ts | 28 +++++++++++---- packages/api/src/admin/auditLog.ts | 15 ++++---- .../data-schemas/src/methods/auditLog.spec.ts | 36 ++++++++++++++++--- packages/data-schemas/src/methods/auditLog.ts | 11 ++++-- .../data-schemas/src/methods/systemGrant.ts | 12 ++++--- packages/data-schemas/src/schema/auditLog.ts | 10 ++++++ 6 files changed, 88 insertions(+), 24 deletions(-) diff --git a/packages/api/src/admin/auditLog.spec.ts b/packages/api/src/admin/auditLog.spec.ts index e8d690f7d5..cdb221bcc5 100644 --- a/packages/api/src/admin/auditLog.spec.ts +++ b/packages/api/src/admin/auditLog.spec.ts @@ -136,7 +136,9 @@ function createDeps(overrides: Partial = {}): AdminAuditLogDe findAuditLogEntry: jest .fn, unknown[]>() .mockResolvedValue(null), - streamAuditLogEntries: jest.fn, unknown[]>().mockResolvedValue(0), + streamAuditLogEntries: jest + .fn, unknown[]>() + .mockResolvedValue({ count: 0, truncated: false }), verifyAuditChain: jest .fn, unknown[]>() .mockResolvedValue(mockVerification()), @@ -373,7 +375,7 @@ describe('createAdminAuditLogHandlers', () => { await onEntry(entry); count++; } - return count; + return { count, truncated: false }; }); } @@ -432,15 +434,29 @@ describe('createAdminAuditLogHandlers', () => { expect(deps.streamAuditLogEntries).not.toHaveBeenCalled(); }); - it('appends an explicit marker when the export hits the row cap', async () => { + it('appends an explicit marker when the export is truncated', async () => { const deps = createDeps({ - streamAuditLogEntries: jest.fn().mockResolvedValue(MAX_AUDIT_EXPORT_ROWS), + streamAuditLogEntries: jest + .fn() + .mockResolvedValue({ count: MAX_AUDIT_EXPORT_ROWS, truncated: true }), }); const handlers = createAdminAuditLogHandlers(deps); const ctx = createCsvContext(); await handlers.exportAuditLogCsv(ctx.req, ctx.res); - const body = ctx.chunks.join(''); - expect(body).toContain('TRUNCATED'); + expect(ctx.chunks.join('')).toContain('TRUNCATED'); + expect(ctx.endCalled()).toBe(true); + }); + + it('does not mark an exact-cap export as truncated', async () => { + const deps = createDeps({ + streamAuditLogEntries: jest + .fn() + .mockResolvedValue({ count: MAX_AUDIT_EXPORT_ROWS, truncated: false }), + }); + const handlers = createAdminAuditLogHandlers(deps); + const ctx = createCsvContext(); + await handlers.exportAuditLogCsv(ctx.req, ctx.res); + expect(ctx.chunks.join('')).not.toContain('TRUNCATED'); expect(ctx.endCalled()).toBe(true); }); }); diff --git a/packages/api/src/admin/auditLog.ts b/packages/api/src/admin/auditLog.ts index db52cdc7e5..6b21e02f78 100644 --- a/packages/api/src/admin/auditLog.ts +++ b/packages/api/src/admin/auditLog.ts @@ -58,7 +58,7 @@ export interface AdminAuditLogDeps { filters: Omit, onEntry: (entry: AdminAuditLogEntry) => void | Promise, options?: { isCancelled?: () => boolean; maxRows?: number }, - ) => Promise; + ) => Promise<{ count: number; truncated: boolean }>; verifyAuditChain: (tenantId: string | undefined) => Promise; } @@ -390,7 +390,7 @@ export function createAdminAuditLogHandlers(deps: AdminAuditLogDeps): { await writeChunk(formatCsvHeader()); await writeChunk('\r\n'); - const written = await streamAuditLogEntries( + const { truncated } = await streamAuditLogEntries( caller.tenantId, filters.value, async (entry) => { @@ -405,12 +405,13 @@ export function createAdminAuditLogHandlers(deps: AdminAuditLogDeps): { req.removeListener('aborted', markAborted); if (!clientAborted) { /** - * Hitting the row cap means the export is incomplete. Surfacing this is a - * compliance requirement — a silently truncated export reads as a - * complete record. Emit an explicit trailing marker and log it; callers - * should narrow the date range for the full set. + * `truncated` is true only when rows existed beyond the cap (an exact-cap + * match is reported as complete). Surfacing this is a compliance + * requirement — a silently truncated export reads as a complete record. + * Emit an explicit trailing marker and log it; callers should narrow the + * date range for the full set. */ - if (written >= MAX_AUDIT_EXPORT_ROWS) { + if (truncated) { logger.warn('[adminAuditLog] CSV export truncated at row cap', { maxRows: MAX_AUDIT_EXPORT_ROWS, }); diff --git a/packages/data-schemas/src/methods/auditLog.spec.ts b/packages/data-schemas/src/methods/auditLog.spec.ts index 13fdc2d82f..e1fda64b4c 100644 --- a/packages/data-schemas/src/methods/auditLog.spec.ts +++ b/packages/data-schemas/src/methods/auditLog.spec.ts @@ -189,6 +189,27 @@ describe('auditLog methods', () => { ]), ).rejects.toThrow(/append-only/); }); + + it('rejects insertMany (which would let callers poison the chain)', async () => { + await expect( + AuditLog.insertMany([ + { + schemaVersion: 1, + category: 'grant', + action: 'grant.assigned', + outcome: 'success', + severity: 'info', + actor: { type: 'user', name: 'Mallory' }, + target: { type: 'role', id: 'ADMIN', name: 'ADMIN' }, + chainKey: CK_A, + seq: 999, + prevHash: GENESIS_HASH, + hash: 'f'.repeat(64), + createdAt: new Date(), + }, + ]), + ).rejects.toThrow(/append-only/); + }); }); describe('listAuditLogPage', () => { @@ -289,22 +310,29 @@ describe('auditLog methods', () => { it('streams all matching rows newest-first', async () => { await seed(3); const seen: number[] = []; - const count = await methods.streamAuditLogEntries('tenant-a', {}, (e) => { + const { count, truncated } = await methods.streamAuditLogEntries('tenant-a', {}, (e) => { seen.push(e.integrity.seq); }); expect(count).toBe(3); + expect(truncated).toBe(false); expect(seen).toEqual([3, 2, 1]); }); - it('honors isCancelled and maxRows', async () => { + it('honors isCancelled and reports truncation only when the cap cuts rows off', async () => { await seed(5); const cancelled = await methods.streamAuditLogEntries('tenant-a', {}, () => {}, { isCancelled: () => true, }); - expect(cancelled).toBe(0); + expect(cancelled.count).toBe(0); const capped = await methods.streamAuditLogEntries('tenant-a', {}, () => {}, { maxRows: 2 }); - expect(capped).toBe(2); + expect(capped.count).toBe(2); + expect(capped.truncated).toBe(true); + + // exact-cap match exhausts naturally and is NOT truncated + const exact = await methods.streamAuditLogEntries('tenant-a', {}, () => {}, { maxRows: 5 }); + expect(exact.count).toBe(5); + expect(exact.truncated).toBe(false); }); }); diff --git a/packages/data-schemas/src/methods/auditLog.ts b/packages/data-schemas/src/methods/auditLog.ts index abb0b6265a..74391a4192 100644 --- a/packages/data-schemas/src/methods/auditLog.ts +++ b/packages/data-schemas/src/methods/auditLog.ts @@ -52,7 +52,7 @@ export interface AuditLogMethods { filters: Omit, onEntry: (entry: AdminAuditLogEntry) => void | Promise, options?: { isCancelled?: () => boolean; maxRows?: number }, - ) => Promise; + ) => Promise<{ count: number; truncated: boolean }>; verifyAuditChain: ( tenantId: string | undefined, options?: VerifyAuditChainOptions, @@ -431,7 +431,7 @@ export function createAuditLogMethods(mongoose: typeof import('mongoose')): Audi filters: Omit, onEntry: (entry: AdminAuditLogEntry) => void | Promise, options?: { isCancelled?: () => boolean; maxRows?: number }, - ): Promise { + ): Promise<{ count: number; truncated: boolean }> { const AuditLog = model(); const query = buildFilter(auditChainKey(tenantId), filters); const cursor = AuditLog.find(query) @@ -442,6 +442,7 @@ export function createAuditLogMethods(mongoose: typeof import('mongoose')): Audi const isCancelled = options?.isCancelled; const maxRows = options?.maxRows; let count = 0; + let truncated = false; try { for await (const doc of cursor) { if (isCancelled?.()) { @@ -449,6 +450,10 @@ export function createAuditLogMethods(mongoose: typeof import('mongoose')): Audi break; } if (maxRows != null && count >= maxRows) { + /** The cap fired only because at least one more row exists beyond it, + * so this is a genuine truncation — an exact-cap match exhausts the + * cursor naturally and never reaches here. */ + truncated = true; await cursor.close(); break; } @@ -458,7 +463,7 @@ export function createAuditLogMethods(mongoose: typeof import('mongoose')): Audi } finally { await cursor.close().catch(() => undefined); } - return count; + return { count, truncated }; } async function verifyAuditChain( diff --git a/packages/data-schemas/src/methods/systemGrant.ts b/packages/data-schemas/src/methods/systemGrant.ts index ce1087f41f..833a31f07d 100644 --- a/packages/data-schemas/src/methods/systemGrant.ts +++ b/packages/data-schemas/src/methods/systemGrant.ts @@ -277,15 +277,19 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')): { tenantId: tenantId != null ? tenantId : { $exists: false }, }; + /** + * Insert-only: re-asserting an existing grant is a true no-op (no field is + * mutated), so `created === false` reliably means "nothing changed" and the + * caller can safely skip audit emission. `grantedAt`/`grantedBy` therefore + * record the original grant, not the last re-assert. + */ const update = { - $set: { - grantedAt: new Date(), - ...(grantedBy != null && { grantedBy }), - }, $setOnInsert: { principalType, principalId: normalizedPrincipalId, capability, + grantedAt: new Date(), + ...(grantedBy != null && { grantedBy }), ...(tenantId != null && { tenantId }), }, }; diff --git a/packages/data-schemas/src/schema/auditLog.ts b/packages/data-schemas/src/schema/auditLog.ts index d2ef90f696..51a33f7a73 100644 --- a/packages/data-schemas/src/schema/auditLog.ts +++ b/packages/data-schemas/src/schema/auditLog.ts @@ -155,6 +155,16 @@ auditLogSchema.pre('bulkWrite', function (next) { next(new Error(APPEND_ONLY_MESSAGE)); }); +/** + * `Model.insertMany()` is another bulk path that skips the document `save` + * hook, so it could insert rows with attacker-chosen `seq`/`prevHash`/`hash` + * and poison the chain. Entries are only ever written one-at-a-time via + * `recordAuditEntry` (`Model.create`), so block bulk inserts outright. + */ +auditLogSchema.pre('insertMany', function (next) { + next(new Error(APPEND_ONLY_MESSAGE)); +}); + /** * Unique per-chain sequence. This is both the keyset-pagination key and the * integrity backstop: concurrent appends race to claim the next `seq`, one wins,