mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
🛠️ fix: Address Codex round-3 on the audit-log substrate
- 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.
This commit is contained in:
parent
778d63c6b1
commit
4633460d9a
6 changed files with 88 additions and 24 deletions
|
|
@ -136,7 +136,9 @@ function createDeps(overrides: Partial<AdminAuditLogDeps> = {}): AdminAuditLogDe
|
|||
findAuditLogEntry: jest
|
||||
.fn<Promise<AdminAuditLogEntry | null>, unknown[]>()
|
||||
.mockResolvedValue(null),
|
||||
streamAuditLogEntries: jest.fn<Promise<number>, unknown[]>().mockResolvedValue(0),
|
||||
streamAuditLogEntries: jest
|
||||
.fn<Promise<{ count: number; truncated: boolean }>, unknown[]>()
|
||||
.mockResolvedValue({ count: 0, truncated: false }),
|
||||
verifyAuditChain: jest
|
||||
.fn<Promise<AuditChainVerification>, 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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -58,7 +58,7 @@ export interface AdminAuditLogDeps {
|
|||
filters: Omit<AuditLogFilters, 'offset' | 'limit' | 'cursor'>,
|
||||
onEntry: (entry: AdminAuditLogEntry) => void | Promise<void>,
|
||||
options?: { isCancelled?: () => boolean; maxRows?: number },
|
||||
) => Promise<number>;
|
||||
) => Promise<{ count: number; truncated: boolean }>;
|
||||
verifyAuditChain: (tenantId: string | undefined) => Promise<AuditChainVerification>;
|
||||
}
|
||||
|
||||
|
|
@ -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,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -52,7 +52,7 @@ export interface AuditLogMethods {
|
|||
filters: Omit<AuditLogFilters, 'offset' | 'limit' | 'cursor'>,
|
||||
onEntry: (entry: AdminAuditLogEntry) => void | Promise<void>,
|
||||
options?: { isCancelled?: () => boolean; maxRows?: number },
|
||||
) => Promise<number>;
|
||||
) => 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<AuditLogFilters, 'offset' | 'limit' | 'cursor'>,
|
||||
onEntry: (entry: AdminAuditLogEntry) => void | Promise<void>,
|
||||
options?: { isCancelled?: () => boolean; maxRows?: number },
|
||||
): Promise<number> {
|
||||
): 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(
|
||||
|
|
|
|||
|
|
@ -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 }),
|
||||
},
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue