From 06a21555ad68e26d28bcb66369ec9af8debd5ff3 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 18 Jun 2026 10:19:38 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A0=EF=B8=8F=20fix:=20Address=20Codex?= =?UTF-8?q?=20review=20on=20the=20audit-log=20substrate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - F1 (fail-closed atomicity): assign/revoke now compensate (rollback grant / restore grant) when a fail-closed audit write fails, so a 5xx never leaves an unaudited mutation. - F5: only emit grant.assigned for a real change — skip the audit when the role already holds the capability (idempotent re-assert). - F7: verifyAuditChain no longer silently trusts a non-genesis start; a purged prefix must be authorized by a trusted checkpoint (purge now returns {throughSeq, prevHash}), else verification fails as tampering. - F4: block Model.bulkWrite on AuditLog (would bypass the append-only middleware). - F3: CSV export appends an explicit TRUNCATED marker + logs when the row cap is hit. - F6: reject out-of-range date-only filters (2025-02-31) instead of normalizing. - F2: regenerate package-lock.json for the 0.0.54 data-schemas bump. Tests: +1 methods (bulkWrite) +2 verify (deleted-prefix / checkpoint mismatch), updated purge test for checkpoint flow; +4 api (re-assert skip, assign/revoke fail-closed rollback, date reject, CSV truncation marker). --- package-lock.json | 2 +- packages/api/src/admin/auditLog.spec.ts | 22 ++++++ packages/api/src/admin/auditLog.ts | 31 +++++++- packages/api/src/admin/grants.spec.ts | 52 ++++++++++++- packages/api/src/admin/grants.ts | 77 +++++++++++++++---- .../data-schemas/src/methods/auditLog.spec.ts | 49 +++++++++++- packages/data-schemas/src/methods/auditLog.ts | 50 ++++++++++-- packages/data-schemas/src/schema/auditLog.ts | 12 +++ packages/data-schemas/src/types/auditLog.ts | 24 +++++- 9 files changed, 286 insertions(+), 33 deletions(-) diff --git a/package-lock.json b/package-lock.json index 966e1c6a0e..21ea64466a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -44784,7 +44784,7 @@ }, "packages/data-schemas": { "name": "@librechat/data-schemas", - "version": "0.0.53", + "version": "0.0.54", "license": "MIT", "devDependencies": { "@rollup/plugin-alias": "^5.1.0", diff --git a/packages/api/src/admin/auditLog.spec.ts b/packages/api/src/admin/auditLog.spec.ts index 1dc97d1996..9b767e4204 100644 --- a/packages/api/src/admin/auditLog.spec.ts +++ b/packages/api/src/admin/auditLog.spec.ts @@ -1,5 +1,6 @@ import { Types } from 'mongoose'; import { EventEmitter } from 'events'; +import { MAX_AUDIT_EXPORT_ROWS } from '@librechat/data-schemas'; import type { AdminAuditLogEntry, AuditChainVerification, @@ -205,6 +206,15 @@ describe('createAdminAuditLogHandlers', () => { expect(status).toHaveBeenCalledWith(400); }); + it('rejects an out-of-range date-only value instead of normalizing it', async () => { + const deps = createDeps(); + const handlers = createAdminAuditLogHandlers(deps); + const { req, res, status } = createReqRes({ query: { from: '2025-02-31' } }); + await handlers.listAuditLog(req, res); + expect(status).toHaveBeenCalledWith(400); + expect(deps.listAuditLogPage).not.toHaveBeenCalled(); + }); + it.each([ ['action', { action: 'nope' }], ['category', { category: 'nope' }], @@ -412,5 +422,17 @@ describe('createAdminAuditLogHandlers', () => { expect(ctx.res.status as jest.Mock).toHaveBeenCalledWith(400); expect(deps.streamAuditLogEntries).not.toHaveBeenCalled(); }); + + it('appends an explicit marker when the export hits the row cap', async () => { + const deps = createDeps({ + streamAuditLogEntries: jest.fn().mockResolvedValue(MAX_AUDIT_EXPORT_ROWS), + }); + 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.endCalled()).toBe(true); + }); }); }); diff --git a/packages/api/src/admin/auditLog.ts b/packages/api/src/admin/auditLog.ts index 27e3657ab5..e79fb2816e 100644 --- a/packages/api/src/admin/auditLog.ts +++ b/packages/api/src/admin/auditLog.ts @@ -124,6 +124,17 @@ function parseIsoDate( if (!ISO_DATE_RE.test(raw)) return { ok: false, error: 'Date must be ISO 8601' }; const d = new Date(raw); if (Number.isNaN(d.getTime())) return { ok: false, error: 'Invalid date' }; + if (DATE_ONLY_RE.test(raw)) { + /** + * `new Date('2025-02-31')` silently normalizes to March 3 rather than + * throwing. Round-trip the parsed Y/M/D so an out-of-range day is rejected + * instead of quietly shifting the queried window. + */ + const [year, month, day] = raw.split('-').map(Number); + if (d.getUTCFullYear() !== year || d.getUTCMonth() + 1 !== month || d.getUTCDate() !== day) { + return { ok: false, error: 'Invalid date' }; + } + } if (boundary === 'end' && DATE_ONLY_RE.test(raw)) { d.setUTCHours(23, 59, 59, 999); } @@ -378,7 +389,7 @@ export function createAdminAuditLogHandlers(deps: AdminAuditLogDeps): { await writeChunk(formatCsvHeader()); await writeChunk('\r\n'); - await streamAuditLogEntries( + const written = await streamAuditLogEntries( caller.tenantId, filters.value, async (entry) => { @@ -391,7 +402,23 @@ export function createAdminAuditLogHandlers(deps: AdminAuditLogDeps): { res.removeListener('close', markAborted); req.removeListener('aborted', markAborted); - if (!clientAborted) res.end(); + 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. + */ + if (written >= MAX_AUDIT_EXPORT_ROWS) { + logger.warn('[adminAuditLog] CSV export truncated at row cap', { + maxRows: MAX_AUDIT_EXPORT_ROWS, + }); + await writeChunk( + `# TRUNCATED: export capped at ${MAX_AUDIT_EXPORT_ROWS} rows — narrow the from/to range to export the complete set\r\n`, + ); + } + res.end(); + } } catch (err) { logger.error('[adminAuditLog] export error:', err); if (!res.headersSent) { diff --git a/packages/api/src/admin/grants.spec.ts b/packages/api/src/admin/grants.spec.ts index b5624a2171..0ef58c54fc 100644 --- a/packages/api/src/admin/grants.spec.ts +++ b/packages/api/src/admin/grants.spec.ts @@ -1243,6 +1243,23 @@ describe('createAdminGrantsHandlers', () => { expect(status).toHaveBeenCalledWith(201); }); + it('does not audit re-assigning a capability the role already holds', async () => { + const recordAuditEntry = jest.fn().mockResolvedValue(undefined); + const deps = createDeps({ + recordAuditEntry, + getCapabilitiesForPrincipal: jest + .fn() + .mockResolvedValue([{ capability: SystemCapabilities.READ_USERS }]), + }); + const handlers = createAdminGrantsHandlers(deps); + const { req, res, status } = createReqRes({ body: assignBody, user: reqUser() }); + + await handlers.assignGrant(req, res); + + expect(status).toHaveBeenCalledWith(201); + expect(recordAuditEntry).not.toHaveBeenCalled(); + }); + it('fail-open: a failed audit write does not fail the grant', async () => { const recordAuditEntry = jest.fn().mockRejectedValue(new Error('audit down')); const deps = createDeps({ recordAuditEntry }); @@ -1254,7 +1271,7 @@ describe('createAdminGrantsHandlers', () => { expect(status).toHaveBeenCalledWith(201); }); - it('fail-closed: a failed audit write surfaces as a 500', async () => { + it('fail-closed: a failed assign audit rolls back the grant and 500s', async () => { const recordAuditEntry = jest.fn().mockRejectedValue(new Error('audit down')); const deps = createDeps({ recordAuditEntry, auditFailClosed: true }); const handlers = createAdminGrantsHandlers(deps); @@ -1266,6 +1283,39 @@ describe('createAdminGrantsHandlers', () => { expect.anything(), expect.objectContaining({ failClosed: true }), ); + // compensating rollback removes the grant we just created + expect(deps.revokeCapability).toHaveBeenCalledWith( + expect.objectContaining({ + principalType: PrincipalType.ROLE, + principalId: 'editor', + capability: SystemCapabilities.READ_USERS, + }), + ); + expect(status).toHaveBeenCalledWith(500); + }); + + it('fail-closed: a failed revoke audit restores the grant and 500s', async () => { + const recordAuditEntry = jest.fn().mockRejectedValue(new Error('audit down')); + const grantCapability = jest.fn().mockResolvedValue({ id: 'g1' }); + const deps = createDeps({ + recordAuditEntry, + auditFailClosed: true, + grantCapability, + revokeCapability: jest.fn().mockResolvedValue({ deletedCount: 1 }), + }); + const handlers = createAdminGrantsHandlers(deps); + const { req, res, status } = createReqRes({ params: assignBody, user: reqUser() }); + + await handlers.revokeGrant(req, res); + + // compensating re-grant restores access removed before the audit failed + expect(grantCapability).toHaveBeenCalledWith( + expect.objectContaining({ + principalType: PrincipalType.ROLE, + principalId: 'editor', + capability: SystemCapabilities.READ_USERS, + }), + ); expect(status).toHaveBeenCalledWith(500); }); }); diff --git a/packages/api/src/admin/grants.ts b/packages/api/src/admin/grants.ts index 27e83e6ba5..d937a2740e 100644 --- a/packages/api/src/admin/grants.ts +++ b/packages/api/src/admin/grants.ts @@ -422,6 +422,15 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps): { } } + /** Capture prior state before the idempotent upsert so a re-assignment of + * a capability the role already holds is not audited as a new change. */ + const priorGrants = await getCapabilitiesForPrincipal({ + principalType, + principalId, + tenantId, + }); + const alreadyHeld = priorGrants.some((g) => g.capability === capability); + const grant = await grantCapability({ principalType, principalId, @@ -432,14 +441,29 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps): { if (!grant) { return res.status(500).json({ error: 'Grant operation returned no result' }); } - await emitAudit({ - action: 'grant.assigned', - caller, - principalType, - principalId, - capability, - context: buildAuditContext(req), - }); + if (!alreadyHeld) { + try { + await emitAudit({ + action: 'grant.assigned', + caller, + principalType, + principalId, + capability, + context: buildAuditContext(req), + }); + } catch (auditErr) { + /** Fail-closed only (emitAudit rethrows here): roll back the grant we + * just created so a 5xx never leaves an unaudited grant in place. */ + await revokeCapability({ principalType, principalId, capability, tenantId }).catch((e) => + logger.error('[adminGrants] compensating revoke after audit failure failed', e), + ); + logger.error( + '[adminGrants] assignGrant audit failed (fail-closed) — rolled back grant', + auditErr, + ); + return res.status(500).json({ error: 'Failed to record audit entry' }); + } + } return res.status(201).json({ grant }); } catch (error) { logger.error('[adminGrants] assignGrant error:', error); @@ -490,14 +514,35 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps): { // a no-op revoke (the grant never existed) must not appear in the audit // trail or the forensic record becomes misleading. if (revokeResult.deletedCount > 0) { - await emitAudit({ - action: 'grant.removed', - caller, - principalType: principalType as PrincipalType, - principalId, - capability: capability as SystemCapability, - context: buildAuditContext(req), - }); + try { + await emitAudit({ + action: 'grant.removed', + caller, + principalType: principalType as PrincipalType, + principalId, + capability: capability as SystemCapability, + context: buildAuditContext(req), + }); + } catch (auditErr) { + /** Fail-closed only (emitAudit rethrows here): restore the grant we + * just removed so a 5xx never leaves an unaudited revoke. The original + * `grantedBy`/`grantedAt` are not recoverable here; restoring access is + * the priority on this rare error path. */ + await grantCapability({ + principalType: principalType as PrincipalType, + principalId, + capability: capability as SystemCapability, + tenantId, + grantedBy: caller.userId, + }).catch((e) => + logger.error('[adminGrants] compensating re-grant after audit failure failed', e), + ); + logger.error( + '[adminGrants] revokeGrant audit failed (fail-closed) — restored grant', + auditErr, + ); + return res.status(500).json({ error: 'Failed to record audit entry' }); + } } return res.status(200).json({ success: true }); } catch (error) { diff --git a/packages/data-schemas/src/methods/auditLog.spec.ts b/packages/data-schemas/src/methods/auditLog.spec.ts index a16d5f5fe1..3a2e83db5d 100644 --- a/packages/data-schemas/src/methods/auditLog.spec.ts +++ b/packages/data-schemas/src/methods/auditLog.spec.ts @@ -173,6 +173,20 @@ describe('auditLog methods', () => { await expect(doc!.save()).rejects.toThrow(/append-only/); await expect(doc!.deleteOne()).rejects.toThrow(/append-only/); }); + + it('rejects bulkWrite (which would bypass query/document middleware)', async () => { + await methods.recordAuditEntry(baseInput()); + await expect( + AuditLog.bulkWrite([ + { + updateOne: { + filter: { chainKey: 'tenant-a' }, + update: { $set: { 'actor.name': 'x' } }, + }, + }, + ]), + ).rejects.toThrow(/append-only/); + }); }); describe('listAuditLogPage', () => { @@ -338,6 +352,26 @@ describe('auditLog methods', () => { expect(result.ok).toBe(false); expect(result.brokenAt).toBe(2); }); + + it('does not silently trust a deleted prefix without a checkpoint', async () => { + await seed(3); + // attacker deletes the oldest row through the raw driver + await AuditLog.collection.deleteOne({ chainKey: 'tenant-a', seq: 1 }); + const result = await methods.verifyAuditChain('tenant-a'); + expect(result.ok).toBe(false); + expect(result.brokenAt).toBe(2); + expect(result.reason).toMatch(/non-genesis|prefix/); + }); + + it('rejects a checkpoint that does not match the remaining chain', async () => { + await seed(3); + await AuditLog.collection.deleteOne({ chainKey: 'tenant-a', seq: 1 }); + const result = await methods.verifyAuditChain('tenant-a', { + trustedCheckpoint: { throughSeq: 1, prevHash: 'f'.repeat(64) }, + }); + expect(result.ok).toBe(false); + expect(result.reason).toMatch(/checkpoint mismatch/); + }); }); describe('purgeAuditLogEntries', () => { @@ -365,11 +399,18 @@ describe('auditLog methods', () => { confirm: true, }); expect(result.deletedCount).toBeGreaterThanOrEqual(1); - expect(result.checkpoint?.seq).toBeGreaterThan(1); + expect(result.checkpoint?.throughSeq).toBeGreaterThanOrEqual(1); - const verification = await methods.verifyAuditChain('tenant-a'); - expect(verification.ok).toBe(true); - expect(verification.range?.firstSeq).toBeGreaterThan(1); + // Without the checkpoint, the now non-genesis start is flagged, not trusted. + const unverified = await methods.verifyAuditChain('tenant-a'); + expect(unverified.ok).toBe(false); + + // With the checkpoint the purge returned, the shorter chain verifies. + const verified = await methods.verifyAuditChain('tenant-a', { + trustedCheckpoint: result.checkpoint, + }); + expect(verified.ok).toBe(true); + expect(verified.range?.firstSeq).toBeGreaterThan(1); }); }); }); diff --git a/packages/data-schemas/src/methods/auditLog.ts b/packages/data-schemas/src/methods/auditLog.ts index 4dc79bfb91..539d508ccb 100644 --- a/packages/data-schemas/src/methods/auditLog.ts +++ b/packages/data-schemas/src/methods/auditLog.ts @@ -12,6 +12,7 @@ import type { PurgeAuditLogResult, RecordAuditEntryInput, RecordAuditEntryOptions, + VerifyAuditChainOptions, } from '~/types'; import { GENESIS_HASH, PLATFORM_CHAIN_KEY } from '~/schema/auditLog'; import { AUDIT_ACTION_CATEGORY } from '~/types/admin'; @@ -52,7 +53,10 @@ export interface AuditLogMethods { onEntry: (entry: AdminAuditLogEntry) => void | Promise, options?: { isCancelled?: () => boolean; maxRows?: number }, ) => Promise; - verifyAuditChain: (tenantId: string | undefined) => Promise; + verifyAuditChain: ( + tenantId: string | undefined, + options?: VerifyAuditChainOptions, + ) => Promise; purgeAuditLogEntries: ( tenantId: string | undefined, options: PurgeAuditLogOptions, @@ -450,7 +454,10 @@ export function createAuditLogMethods(mongoose: typeof import('mongoose')): Audi return count; } - async function verifyAuditChain(tenantId: string | undefined): Promise { + async function verifyAuditChain( + tenantId: string | undefined, + options?: VerifyAuditChainOptions, + ): Promise { const AuditLog = model(); const chainKey = resolveChainKey(tenantId); const cursor = AuditLog.find({ chainKey }) @@ -458,6 +465,7 @@ export function createAuditLogMethods(mongoose: typeof import('mongoose')): Audi .lean() .cursor({ batchSize: 500 }); + const trustedCheckpoint = options?.trustedCheckpoint; let prevHash = GENESIS_HASH; let expectedSeq: number | null = null; let firstSeq: number | null = null; @@ -469,9 +477,33 @@ export function createAuditLogMethods(mongoose: typeof import('mongoose')): Audi if (firstSeq === null) { firstSeq = doc.seq; expectedSeq = doc.seq; - /** Genesis must link to GENESIS_HASH; a purged prefix (firstSeq > 1) - * makes the earliest remaining entry's prevHash a trusted checkpoint. */ - prevHash = doc.seq === 1 ? GENESIS_HASH : doc.prevHash; + /** + * A non-genesis start (firstSeq > 1) means a prefix is gone. Only a + * matching trusted checkpoint proves it was an authorized retention + * purge; otherwise this is an attacker deleting the oldest rows, so we + * must NOT silently trust the first row's prevHash. + */ + if (doc.seq === 1) { + prevHash = GENESIS_HASH; + } else if ( + trustedCheckpoint && + trustedCheckpoint.throughSeq === doc.seq - 1 && + trustedCheckpoint.prevHash === doc.prevHash + ) { + prevHash = doc.prevHash; + } else { + await cursor.close(); + return { + ok: false, + chainKey, + checked, + brokenAt: doc.seq, + reason: trustedCheckpoint + ? 'checkpoint mismatch: purged prefix does not match the trusted checkpoint' + : `non-genesis start at seq ${doc.seq}: prefix purged or deleted (supply a trusted checkpoint to verify)`, + range: { firstSeq, lastSeq: doc.seq }, + }; + } } if (doc.seq !== expectedSeq) { return { @@ -556,7 +588,13 @@ export function createAuditLogMethods(mongoose: typeof import('mongoose')): Audi return { deletedCount: result.deletedCount ?? 0, - checkpoint: newFirst ? { seq: newFirst.seq, prevHash: newFirst.prevHash } : undefined, + /** Boundary for `verifyAuditChain`: the new earliest entry resumes the + * chain at `seq`, so the purged prefix ran through `seq - 1` and its last + * hash is this entry's `prevHash`. Callers persist this to later prove the + * purge was authorized. */ + checkpoint: newFirst + ? { throughSeq: newFirst.seq - 1, prevHash: newFirst.prevHash } + : undefined, }; } diff --git a/packages/data-schemas/src/schema/auditLog.ts b/packages/data-schemas/src/schema/auditLog.ts index 5058824c53..d2ef90f696 100644 --- a/packages/data-schemas/src/schema/auditLog.ts +++ b/packages/data-schemas/src/schema/auditLog.ts @@ -143,6 +143,18 @@ auditLogSchema.pre('save', function (next) { next(); }); +/** + * `Model.bulkWrite()` dispatches update/delete operations through the raw driver + * and skips the query/document middleware above. Block it wholesale: audit rows + * are only ever written via `recordAuditEntry` (`Model.create`), and a bulk + * insert would also break the sequential hash chain, so there is no legitimate + * bulkWrite path. The privileged retention purge uses `Model.collection` and is + * unaffected by this hook. + */ +auditLogSchema.pre('bulkWrite', 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, diff --git a/packages/data-schemas/src/types/auditLog.ts b/packages/data-schemas/src/types/auditLog.ts index c8c77b32c5..f1f6a94dda 100644 --- a/packages/data-schemas/src/types/auditLog.ts +++ b/packages/data-schemas/src/types/auditLog.ts @@ -148,11 +148,29 @@ export interface PurgeAuditLogOptions { confirm: boolean; } +/** A trusted boundary marker proving a prefix was purged by an authorized + * retention run rather than deleted by an attacker. Persisted by the caller + * (e.g. alongside retention-job records) and passed back to `verifyAuditChain`. */ +export interface AuditCheckpoint { + /** Highest `seq` that was purged; the chain resumes at `throughSeq + 1`. */ + throughSeq: number; + /** Hash of the last purged entry === `prevHash` of the new earliest entry. */ + prevHash: string; +} + export interface PurgeAuditLogResult { deletedCount: number; - /** The new earliest remaining entry, which becomes the trusted checkpoint the - * verifier chains forward from. Absent when the chain is now empty. */ - checkpoint?: { seq: number; prevHash: string }; + /** The boundary the verifier must be given to accept the now-shorter chain. + * Absent when the chain is now empty or nothing was purged. */ + checkpoint?: AuditCheckpoint; +} + +export interface VerifyAuditChainOptions { + /** When the chain no longer starts at `seq: 1` (a prefix was purged), the + * verifier requires this boundary to distinguish an authorized retention purge + * from an attacker deleting the oldest rows. Without it, a non-genesis start + * fails verification rather than being silently trusted. */ + trustedCheckpoint?: AuditCheckpoint; } /** Re-exported so consumers can import the entry shape and its supporting types