diff --git a/api/server/routes/admin/audit.js b/api/server/routes/admin/audit.js index 083e905fe3..b21287e7ac 100644 --- a/api/server/routes/admin/audit.js +++ b/api/server/routes/admin/audit.js @@ -10,7 +10,6 @@ const router = express.Router(); const requireAdminAccess = requireCapability(SystemCapabilities.ACCESS_ADMIN); const handlers = createAdminAuditLogHandlers({ - recordAuditEntry: db.recordAuditEntry, listAuditLogPage: db.listAuditLogPage, findAuditLogEntry: db.findAuditLogEntry, streamAuditLogEntries: db.streamAuditLogEntries, diff --git a/packages/api/src/admin/auditLog.ts b/packages/api/src/admin/auditLog.ts index d5a54a85de..c48c80e31d 100644 --- a/packages/api/src/admin/auditLog.ts +++ b/packages/api/src/admin/auditLog.ts @@ -1,11 +1,6 @@ import { PrincipalType } from 'librechat-data-provider'; import { logger } from '@librechat/data-schemas'; -import type { - AdminAuditLogEntryWire, - AuditAction, - AuditLogPage, - RecordAuditEntryInput, -} from '@librechat/data-schemas'; +import type { AdminAuditLogEntryWire, AuditAction, AuditLogPage } from '@librechat/data-schemas'; import type { Response } from 'express'; import type { ServerRequest } from '~/types/http'; @@ -15,11 +10,14 @@ const CSV_BOM = ''; const VALID_ACTIONS = new Set(['grant_assigned', 'grant_removed']); const VALID_PRINCIPAL_TYPES = new Set(Object.values(PrincipalType)); -const ISO_DATE_RE = /^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}(:\d{2}(\.\d{1,3})?)?Z?)?$/; +// Accepts `YYYY-MM-DD` (interpreted as UTC by the downstream Date parse) or a +// full ISO 8601 / RFC 3339 timestamp that includes either `Z` or a `±HH:MM` +// offset. Local-time strings without a zone are rejected so every input maps +// to an unambiguous instant. +const ISO_DATE_RE = /^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}(:\d{2}(\.\d{1,3})?)?(Z|[+-]\d{2}:\d{2}))?$/; const OBJECT_ID_RE = /^[a-fA-F0-9]{24}$/; export interface AdminAuditLogDeps { - recordAuditEntry: (input: RecordAuditEntryInput) => Promise; listAuditLogPage: ( tenantId: string | undefined, filters: { @@ -253,16 +251,36 @@ export function createAdminAuditLogHandlers(deps: AdminAuditLogDeps) { res.setHeader('Content-Disposition', `attachment; filename="audit-log-${filenameStamp}.csv"`); res.setHeader('Cache-Control', 'no-store'); - res.write(CSV_BOM); - res.write(formatCsvHeader()); - res.write('\r\n'); + // Stop pulling rows from Mongo as soon as the client disconnects so a + // cancelled download doesn't pin a cursor open or burn through memory. + let clientAborted = false; + const markAborted = () => { + clientAborted = true; + }; + res.once('close', markAborted); + req.once('aborted', markAborted); - await streamAuditLogEntries(caller.tenantId, filters.value, (entry) => { - res.write(formatCsvRow(entry)); - res.write('\r\n'); + // Wait for `drain` when the kernel/socket buffer is full so we never + // queue an unbounded amount of CSV in Node memory for slow consumers. + const writeChunk = (chunk: string): Promise => { + if (clientAborted) return Promise.resolve(); + if (res.write(chunk)) return Promise.resolve(); + return new Promise((resolve) => res.once('drain', () => resolve())); + }; + + await writeChunk(CSV_BOM); + await writeChunk(formatCsvHeader()); + await writeChunk('\r\n'); + + await streamAuditLogEntries(caller.tenantId, filters.value, async (entry) => { + if (clientAborted) return; + await writeChunk(formatCsvRow(entry)); + await writeChunk('\r\n'); }); - res.end(); + res.removeListener('close', markAborted); + req.removeListener('aborted', markAborted); + if (!clientAborted) 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 cd6d1e8d38..575c406426 100644 --- a/packages/api/src/admin/grants.spec.ts +++ b/packages/api/src/admin/grants.spec.ts @@ -54,7 +54,7 @@ function createDeps(overrides: Partial = {}): AdminGrantsDeps { getCapabilitiesForPrincipal: jest.fn().mockResolvedValue([]), getCapabilitiesForPrincipals: jest.fn().mockResolvedValue([]), grantCapability: jest.fn().mockResolvedValue(mockGrant()), - revokeCapability: jest.fn().mockResolvedValue(undefined), + revokeCapability: jest.fn().mockResolvedValue({ deletedCount: 1 }), getUserPrincipals: jest.fn().mockResolvedValue([ { principalType: PrincipalType.USER, principalId: new Types.ObjectId() }, { principalType: PrincipalType.ROLE, principalId: 'admin' }, @@ -969,7 +969,7 @@ describe('createAdminGrantsHandlers', () => { it('returns 200 idempotently even if the grant does not exist', async () => { const deps = createDeps({ - revokeCapability: jest.fn().mockResolvedValue(undefined), + revokeCapability: jest.fn().mockResolvedValue({ deletedCount: 1 }), }); const handlers = createAdminGrantsHandlers(deps); const { req, res, status, json } = createReqRes({ params: validParams }); diff --git a/packages/api/src/admin/grants.ts b/packages/api/src/admin/grants.ts index a124f0f969..b7cf33445a 100644 --- a/packages/api/src/admin/grants.ts +++ b/packages/api/src/admin/grants.ts @@ -50,7 +50,7 @@ export interface AdminGrantsDeps { principalId: string | Types.ObjectId; capability: SystemCapability; tenantId?: string; - }) => Promise; + }) => Promise<{ deletedCount: number }>; getUserPrincipals: (params: { userId: string; role?: string | null; @@ -439,19 +439,24 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps) { return res.status(403).json({ error: 'Insufficient permissions' }); } - await revokeCapability({ + const revokeResult = await revokeCapability({ principalType: principalType as PrincipalType, principalId, capability: capability as SystemCapability, tenantId, }); - await emitAudit({ - action: 'grant_removed', - caller, - principalType: principalType as PrincipalType, - principalId, - capability: capability as SystemCapability, - }); + // Only emit the audit entry when a grant document was actually deleted — + // 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, + }); + } return res.status(200).json({ success: true }); } catch (error) { logger.error('[adminGrants] revokeGrant error:', error); diff --git a/packages/data-schemas/src/methods/auditLog.spec.ts b/packages/data-schemas/src/methods/auditLog.spec.ts new file mode 100644 index 0000000000..67d1dc03af --- /dev/null +++ b/packages/data-schemas/src/methods/auditLog.spec.ts @@ -0,0 +1,171 @@ +import mongoose, { Types } from 'mongoose'; +import { PrincipalType } from 'librechat-data-provider'; +import { MongoMemoryServer } from 'mongodb-memory-server'; +import type * as t from '~/types'; +import { createAuditLogMethods, type RecordAuditEntryInput } from './auditLog'; +import auditLogSchema from '~/schema/auditLog'; + +jest.mock('~/config/winston', () => ({ + error: jest.fn(), + warn: jest.fn(), + info: jest.fn(), + debug: jest.fn(), +})); + +let mongoServer: MongoMemoryServer; +let AuditLog: mongoose.Model; +let methods: ReturnType; + +const actorObjectId = new Types.ObjectId(); +const targetObjectId = new Types.ObjectId(); + +function baseInput(over: Partial = {}): RecordAuditEntryInput { + return { + action: 'grant_assigned', + actorId: actorObjectId, + actorName: 'Alice Admin', + targetPrincipalType: PrincipalType.USER, + targetPrincipalId: targetObjectId, + targetName: 'Bob User', + capability: 'manage:users', + tenantId: 'tenant-a', + ...over, + }; +} + +beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + AuditLog = mongoose.models.AuditLog || mongoose.model('AuditLog', auditLogSchema); + methods = createAuditLogMethods(mongoose); + await mongoose.connect(mongoServer.getUri()); +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); +}); + +beforeEach(async () => { + await AuditLog.deleteMany({}); +}); + +describe('auditLog methods', () => { + describe('recordAuditEntry', () => { + it('persists a tenant-scoped entry and wires denormalized fields through', async () => { + const doc = await methods.recordAuditEntry(baseInput()); + expect(doc).not.toBeNull(); + const persisted = await AuditLog.findOne({ tenantId: 'tenant-a' }).lean(); + expect(persisted?.actorName).toBe('Alice Admin'); + expect(persisted?.targetName).toBe('Bob User'); + expect(persisted?.capability).toBe('manage:users'); + expect(persisted?.createdAt).toBeInstanceOf(Date); + }); + + it('omits the tenantId field entirely for platform-level entries', async () => { + await methods.recordAuditEntry(baseInput({ tenantId: undefined })); + const raw = await AuditLog.collection.findOne({}); + expect(raw && Object.prototype.hasOwnProperty.call(raw, 'tenantId')).toBe(false); + }); + }); + + describe('listAuditLogPage', () => { + beforeEach(async () => { + await methods.recordAuditEntry(baseInput({ capability: 'manage:users' })); + await methods.recordAuditEntry( + baseInput({ action: 'grant_removed', capability: 'manage:roles' }), + ); + await methods.recordAuditEntry(baseInput({ tenantId: 'tenant-b', capability: 'read:users' })); + await methods.recordAuditEntry( + baseInput({ tenantId: undefined, capability: 'access:admin' }), + ); + }); + + it('returns only the calling tenant by default', async () => { + const page = await methods.listAuditLogPage('tenant-a', {}); + expect(page.total).toBe(2); + const tenants = new Set(page.entries.map((e) => e.capability)); + expect(tenants).toEqual(new Set(['manage:users', 'manage:roles'])); + }); + + it('only returns platform-level entries when tenantId is omitted', async () => { + const page = await methods.listAuditLogPage(undefined, {}); + expect(page.total).toBe(1); + expect(page.entries[0]?.capability).toBe('access:admin'); + }); + + it('filters by a single action', async () => { + const page = await methods.listAuditLogPage('tenant-a', { action: ['grant_removed'] }); + expect(page.total).toBe(1); + expect(page.entries[0]?.action).toBe('grant_removed'); + }); + + it('filters by multiple actions via $in', async () => { + const page = await methods.listAuditLogPage('tenant-a', { + action: ['grant_assigned', 'grant_removed'], + }); + expect(page.total).toBe(2); + }); + + it('partial-matches the actorId param against actorName', async () => { + await methods.recordAuditEntry(baseInput({ actorName: 'Charlotte Auditor' })); + const page = await methods.listAuditLogPage('tenant-a', { actorId: 'charlotte' }); + expect(page.total).toBe(1); + expect(page.entries[0]?.actorName).toBe('Charlotte Auditor'); + }); + + it('partial-matches the capability filter case-insensitively', async () => { + const page = await methods.listAuditLogPage('tenant-a', { capability: 'ROLES' }); + expect(page.total).toBe(1); + expect(page.entries[0]?.capability).toBe('manage:roles'); + }); + + it('applies a `from` / `to` window using createdAt', async () => { + const allBefore = await AuditLog.find({ tenantId: 'tenant-a' }).sort({ createdAt: 1 }).lean(); + const cutoff = allBefore[0]!.createdAt; + const after = new Date(cutoff.getTime() + 1); + const page = await methods.listAuditLogPage('tenant-a', { from: after }); + expect(page.total).toBeLessThan(2); + }); + + it('paginates by offset and limit and reports the total of the full match set', async () => { + const page1 = await methods.listAuditLogPage('tenant-a', { offset: 0, limit: 1 }); + const page2 = await methods.listAuditLogPage('tenant-a', { offset: 1, limit: 1 }); + expect(page1.total).toBe(2); + expect(page2.total).toBe(2); + expect(page1.entries.length).toBe(1); + expect(page2.entries.length).toBe(1); + expect(page1.entries[0]?.id).not.toBe(page2.entries[0]?.id); + }); + + it('stringifies ObjectIds and dates on the wire', async () => { + const page = await methods.listAuditLogPage('tenant-a', { limit: 1 }); + const entry = page.entries[0]!; + expect(typeof entry.id).toBe('string'); + expect(typeof entry.actorId).toBe('string'); + expect(typeof entry.targetPrincipalId).toBe('string'); + expect(typeof entry.timestamp).toBe('string'); + expect(() => new Date(entry.timestamp).toISOString()).not.toThrow(); + }); + + it('escapes regex metacharacters in the search input', async () => { + await methods.recordAuditEntry(baseInput({ actorName: 'risky.dot+plus' })); + const page = await methods.listAuditLogPage('tenant-a', { search: 'risky.dot+plus' }); + expect(page.total).toBe(1); + }); + }); + + describe('streamAuditLogEntries', () => { + it('invokes onEntry for every match without skipping tenant boundaries', async () => { + await methods.recordAuditEntry(baseInput({ capability: 'manage:users' })); + await methods.recordAuditEntry(baseInput({ capability: 'manage:roles' })); + await methods.recordAuditEntry(baseInput({ tenantId: 'tenant-b' })); + + const collected: string[] = []; + const count = await methods.streamAuditLogEntries('tenant-a', {}, (entry) => { + collected.push(entry.capability); + }); + expect(count).toBe(2); + expect(new Set(collected)).toEqual(new Set(['manage:users', 'manage:roles'])); + }); + }); +}); diff --git a/packages/data-schemas/src/methods/systemGrant.ts b/packages/data-schemas/src/methods/systemGrant.ts index 2e8eea80cf..fcd5b35351 100644 --- a/packages/data-schemas/src/methods/systemGrant.ts +++ b/packages/data-schemas/src/methods/systemGrant.ts @@ -226,7 +226,9 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')) { } /** - * Revoke a capability from a principal. + * Revoke a capability from a principal. Returns the deletion result so + * callers (e.g. audit emission) can distinguish a real revoke from a + * no-op against a grant that didn't exist. */ async function revokeCapability( { @@ -241,7 +243,7 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')) { tenantId?: string; }, session?: ClientSession, - ): Promise { + ): Promise<{ deletedCount: number }> { const SystemGrant = mongoose.models.SystemGrant as Model; const normalizedPrincipalId = normalizePrincipalId(principalId, principalType); @@ -254,7 +256,8 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')) { }; const options = session ? { session } : {}; - await SystemGrant.deleteOne(filter, options); + const result = await SystemGrant.deleteOne(filter, options); + return { deletedCount: result.deletedCount ?? 0 }; } /** diff --git a/packages/data-schemas/src/schema/auditLog.ts b/packages/data-schemas/src/schema/auditLog.ts index ed4833100f..4de1a9de64 100644 --- a/packages/data-schemas/src/schema/auditLog.ts +++ b/packages/data-schemas/src/schema/auditLog.ts @@ -51,15 +51,20 @@ const auditLogSchema = new Schema( type: String, required: false, validate: { - validator: (v: unknown) => v !== null && v !== '', - message: 'tenantId must be a non-empty string or omitted entirely — never null or empty', + validator: (v: unknown) => typeof v === 'string' && v.trim().length > 0, + message: + 'tenantId must be a non-empty string or omitted entirely — never null, empty, or a non-string value', }, }, }, { timestamps: true }, ); -/** Primary listing: tenant + newest-first + tiebreak on _id for keyset pagination. */ +/** + * Primary listing: tenant + newest-first sort with `_id` as a deterministic + * tiebreak for offset pagination so adjacent pages never duplicate or skip rows + * when two entries share a `createdAt` timestamp. + */ auditLogSchema.index({ tenantId: 1, createdAt: -1, _id: -1 }); /** Target facet: filter by who was affected. */ diff --git a/packages/data-schemas/src/types/auditLog.ts b/packages/data-schemas/src/types/auditLog.ts index f9dd172d46..836a5f98f9 100644 --- a/packages/data-schemas/src/types/auditLog.ts +++ b/packages/data-schemas/src/types/auditLog.ts @@ -1,7 +1,6 @@ import type { Document, Types } from 'mongoose'; import type { PrincipalType } from 'librechat-data-provider'; - -export type AuditAction = 'grant_assigned' | 'grant_removed'; +import type { AuditAction } from './admin'; export type AuditLog = { action: AuditAction;