Skip to content

Commit 76192bd

Browse files
authored
🛂 feat: Audit Resource Sharing Changes and Fail Closed on Reversible Audit Writes (#17053)
* feat: Audit Resource ACL Changes and Fail Closed by Default * fix: Keep Non-Reversible Admin Audit Handlers Fail-Open * docs: Narrow AUDIT_LOG_FAIL_CLOSED Description * fix: Audit and Restore ACL and Insights Changes in One Pass * fix: Break Audit and Insights Permission Type Cycle * test: Cover Resource ACL Audit Scenarios * refactor: Audit Resource Permission Changes Before Writing * test: Align Concurrent Revoke Audit Expectation * fix: Skip Deleting ACL Entries That Were Not Audited * fix: Read ACL Snapshots Through the Write Session * fix: Never Block Revocations on Audit Outages and Normalize Principal Keys * fix: Let an Explicit Public Disable Win Over a Public Update * fix: Audit Duplicate ACL Rows as Combined Permissions * fix: Audit Permission Changes Under One Target and Dedupe Revokes * fix: Audit Capability Grants Before Writing Them * feat: Serialize Resource Permission Updates With a Per-Resource Lease * fix: Fence the Resource Lease Before Writes and Always Audit Grant Assignments * fix: Renew the Resource Lease Before Each ACL Write Phase * fix: Serialize System Grant Assignment and Revocation With a Per-Grant Lease * fix: Keep Leases Alive During Audits, Bound Lock Index Wait, Canonicalize Audit Target IDs * fix: Retry Deferred Revoke Audits Under the Lease and Audit Canonical Principal and Role IDs * test: Expect Role IDs in Permission Audit Metadata * fix: Read Mutation Snapshots From the Primary Outside Transactions * fix: Flush Deferred Audits Before Cache Invalidation, Keep Renewals Monotonic, Keep Role Case in Grant Lease Keys * refactor: Map the Busy Permission Error to Its Code in the API Package * style: Sort Lease Imports and Type the Renewal Spec Read * fix: Keep Success Audits After Commit, Report Downgrades as Revocations, Code Audit Outages * refactor: Move the Audit Outage Code to Its Own Module * fix: Translate Lock Failures, Lease Non-Transactional Sessions, and Code Grant Audit Outages * fix: Sanitize Lease Renewal Failures at the Permission Boundary * fix: Annotate the Unavailable Error Code for Isolated Declarations * fix: Keep the Admin Route Fail-Closed Opt-In and Document Lease Clock Skew Safety * fix: Translate Grant Lease Store Failures to a Retryable 503 * fix: Log Failed Grant Audit Writes With Safe Metadata Only * test: Detect Raw Error Arguments in the Grant Audit Log Spec * feat: Make Permission Lease Limits Configurable and Localize Retryable Sharing Failures * fix: Bound Lock Index Readiness by the Lease Wait and Carry the Resource ID in Every Audit Entry * test: Cover Lease Config Reaching the Leases and the Sharing Dialog Failure Messages * fix: Load App Config on the Sharing and Grant Routes and Log Failed Lease Releases * perf: Load App Config Only on the Grant Mutation Routes
1 parent ccae5b9 commit 76192bd

46 files changed

Lines changed: 2836 additions & 849 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.env.example‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,17 @@ TRUST_PROXY=1
177177
# instead of exiting the process. Not recommended for production unless necessary.
178178
# CONTINUE_ON_UNCAUGHT_EXCEPTION=false
179179

180+
#===============#
181+
# Audit Log #
182+
#===============#
183+
184+
# System grants and resource sharing are audited before they take effect: when a grant or a
185+
# widening permission change cannot be audited, the request fails and nothing changes.
186+
# Revocations are never blocked by an audit outage; their entry is retried after the change.
187+
# Set to false to apply every change anyway and only log audit failures.
188+
# Default: true
189+
# AUDIT_LOG_FAIL_CLOSED=true
190+
180191
#===============#
181192
# JSON Logging #
182193
#===============#

‎api/server/controllers/PermissionsController.js‎

Lines changed: 19 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,15 @@ const {
99
enrichRemoteAgentPrincipals,
1010
createPrincipalSearch,
1111
backfillRemoteAgentPermissions,
12-
auditInsightsPermissionChanges,
12+
runAuditedPermissionUpdate,
13+
permissionErrorCode,
1314
getInsightsPrincipalState,
1415
maskAgentInsightsBit,
1516
sanitizeInsightsPermissionPrincipals,
1617
validateInsightsPermissionUpdates,
1718
} = require('@librechat/api');
1819
const {
1920
bulkUpdateResourcePermissions,
20-
restoreInsightsPermissionChanges,
2121
ensureGroupPrincipalExists,
2222
getResourcePermissionsMap,
2323
findAccessibleResources,
@@ -170,31 +170,24 @@ const updateResourcePermissions = async (req, res) => {
170170
});
171171
}
172172

173-
const results = await bulkUpdateResourcePermissions({
174-
resourceType,
175-
resourceId,
176-
maxWriteAttempts: req.config?.config?.permissions?.maxWriteAttempts,
177-
updatedPrincipals: validatedPrincipals,
178-
revokedPrincipals,
179-
grantedBy: userId,
180-
});
181-
182-
await auditInsightsPermissionChanges({
173+
const results = await runAuditedPermissionUpdate({
183174
req,
175+
resourceType,
184176
resourceId,
185-
changes: results.insightsChanges ?? [],
186-
failClosed: process.env.AUDIT_LOG_FAIL_CLOSED === 'true',
187-
deps: {
188-
getAgent: db.getAgent,
189-
recordAuditEntry: db.recordAuditEntry,
190-
restoreInsightsPermissionChanges: (changes) =>
191-
restoreInsightsPermissionChanges({
192-
resourceType: ResourceType.AGENT,
193-
resourceId,
194-
changes,
195-
}),
196-
logger,
197-
},
177+
deps: { recordAuditEntry: db.recordAuditEntry, getAgent: db.getAgent, logger },
178+
update: (beforeWrite, afterWrite) =>
179+
bulkUpdateResourcePermissions({
180+
resourceType,
181+
resourceId,
182+
maxWriteAttempts: req.config?.config?.permissions?.maxWriteAttempts,
183+
leaseTtlMs: req.config?.config?.permissions?.leaseTtlMs,
184+
leaseWaitMs: req.config?.config?.permissions?.leaseWaitMs,
185+
updatedPrincipals: validatedPrincipals,
186+
revokedPrincipals,
187+
grantedBy: userId,
188+
beforeWrite,
189+
afterWrite,
190+
}),
198191
});
199192

200193
if (resourceType === ResourceType.CODE_ENVIRONMENT) {
@@ -238,6 +231,7 @@ const updateResourcePermissions = async (req, res) => {
238231
res.status(error.statusCode ?? 400).json({
239232
error: 'Failed to update permissions',
240233
details: error.message,
234+
...permissionErrorCode(error),
241235
});
242236
}
243237
};

‎api/server/controllers/__tests__/PermissionsController.spec.js‎

Lines changed: 147 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,17 @@ jest.mock('~/server/services/Config', () => ({
3333
}));
3434

3535
const mockBulkUpdateResourcePermissions = jest.fn();
36-
const mockRestoreInsightsPermissionChanges = jest.fn();
3736

3837
jest.mock('~/server/services/PermissionService', () => ({
39-
bulkUpdateResourcePermissions: (...args) => mockBulkUpdateResourcePermissions(...args),
40-
restoreInsightsPermissionChanges: (...args) => mockRestoreInsightsPermissionChanges(...args),
38+
/** The service audits through `beforeWrite` before writing; the mock result stands in for the computed plan. */
39+
bulkUpdateResourcePermissions: async (params) => {
40+
const results = await mockBulkUpdateResourcePermissions(params);
41+
await params.beforeWrite?.({
42+
aclChanges: results?.aclChanges ?? [],
43+
insightsChanges: results?.insightsChanges ?? [],
44+
});
45+
return results;
46+
},
4147
ensureGroupPrincipalExists: jest.fn(),
4248
getEffectivePermissions: jest.fn(),
4349
ensurePrincipalExists: jest.fn(),
@@ -249,7 +255,6 @@ describe('PermissionsController', () => {
249255
mockRemoveAgentFromUserFavorites.mockResolvedValue(undefined);
250256
db.getAgent.mockResolvedValue({ _id: agentObjectId, id: 'agent-a', name: 'Agent A' });
251257
mockRecordAuditEntry.mockResolvedValue({});
252-
mockRestoreInsightsPermissionChanges.mockResolvedValue(undefined);
253258
});
254259

255260
it('rejects Insights permission changes from non-admin users', async () => {
@@ -339,28 +344,33 @@ describe('PermissionsController', () => {
339344
});
340345
const res = createMockRes();
341346

342-
req.config = { config: { permissions: { maxWriteAttempts: 7 } } };
347+
req.config = {
348+
config: { permissions: { maxWriteAttempts: 7, leaseTtlMs: 45000, leaseWaitMs: 1500 } },
349+
};
343350
await updateResourcePermissions(req, res);
344351

345352
expect(mockBulkUpdateResourcePermissions).toHaveBeenCalledWith(
346-
expect.objectContaining({ maxWriteAttempts: 7 }),
353+
expect.objectContaining({ maxWriteAttempts: 7, leaseTtlMs: 45000, leaseWaitMs: 1500 }),
347354
);
348355
expect(mockRecordAuditEntry).toHaveBeenCalledWith(
349356
expect.objectContaining({
350357
action: 'permission.insights_assigned',
351358
tenantId: 'tenant-a',
352359
target: expect.objectContaining({ id: 'agent-a' }),
353-
metadata: { principalType: PrincipalType.ROLE, principalId: 'USER' },
360+
metadata: expect.objectContaining({
361+
principalType: PrincipalType.ROLE,
362+
principalId: 'USER',
363+
}),
354364
}),
355-
{ failClosed: false },
365+
{ failClosed: true },
356366
);
357367
expect(res.status).toHaveBeenCalledWith(200);
358368
expect(res.json.mock.calls[0][0].results.principals[0]).toEqual(
359369
expect.objectContaining({ viewInsights: true }),
360370
);
361371
});
362372

363-
it('restores unaudited Insights transitions when fail-closed auditing fails', async () => {
373+
it('fails the request before writing when fail-closed Insights auditing fails', async () => {
364374
process.env.AUDIT_LOG_FAIL_CLOSED = 'true';
365375
const changes = [{ action: 'assigned', principal: { type: PrincipalType.ROLE, id: 'USER' } }];
366376
mockBulkUpdateResourcePermissions.mockResolvedValue({
@@ -390,15 +400,10 @@ describe('PermissionsController', () => {
390400

391401
await updateResourcePermissions(req, res);
392402

393-
expect(mockRestoreInsightsPermissionChanges).toHaveBeenCalledWith({
394-
resourceType: ResourceType.AGENT,
395-
resourceId: agentObjectId,
396-
changes,
397-
});
398403
expect(res.status).toHaveBeenCalledWith(500);
399404
});
400405

401-
it('keeps a successful permission response when audit target lookup fails open', async () => {
406+
it('audits the Insights change with the agent id when the target lookup fails', async () => {
402407
mockBulkUpdateResourcePermissions.mockResolvedValue({
403408
granted: [],
404409
updated: [],
@@ -417,7 +422,133 @@ describe('PermissionsController', () => {
417422

418423
await updateResourcePermissions(req, res);
419424

420-
expect(mockRestoreInsightsPermissionChanges).not.toHaveBeenCalled();
425+
expect(mockRecordAuditEntry).toHaveBeenCalledWith(
426+
expect.objectContaining({
427+
action: 'permission.insights_assigned',
428+
target: expect.objectContaining({ id: agentObjectId }),
429+
}),
430+
{ failClosed: true },
431+
);
432+
expect(res.status).toHaveBeenCalledWith(200);
433+
});
434+
435+
it('audits ACL grants, revokes and public changes without names or emails', async () => {
436+
const aclChanges = [
437+
{
438+
action: 'assigned',
439+
previous: null,
440+
written: { permBits: 3, roleId: 'role-1' },
441+
principal: { type: PrincipalType.USER, id: revokedUserId, name: 'Jane Doe' },
442+
},
443+
{
444+
action: 'removed',
445+
previous: { permBits: 1, roleId: 'role-2' },
446+
written: null,
447+
principal: { type: PrincipalType.GROUP, id: 'group-1', name: 'Team' },
448+
},
449+
{
450+
action: 'assigned',
451+
previous: null,
452+
written: { permBits: 1, roleId: 'role-2' },
453+
principal: { type: PrincipalType.PUBLIC, id: null },
454+
},
455+
];
456+
mockBulkUpdateResourcePermissions.mockResolvedValue({
457+
granted: [],
458+
updated: [],
459+
revoked: [],
460+
aclChanges,
461+
insightsChanges: [],
462+
errors: [],
463+
});
464+
const req = createMockReq({
465+
params: { resourceType: ResourceType.PROMPTGROUP, resourceId: agentObjectId },
466+
user: { id: 'user-id', name: 'Owner', role: SystemRoles.USER, tenantId: 'tenant-a' },
467+
});
468+
const res = createMockRes();
469+
470+
await updateResourcePermissions(req, res);
471+
472+
const entries = mockRecordAuditEntry.mock.calls.map(([input]) => input);
473+
expect(entries.map((entry) => entry.action)).toEqual([
474+
'acl.granted',
475+
'acl.public_changed',
476+
'acl.revoked',
477+
]);
478+
expect(entries[0]).toEqual(
479+
expect.objectContaining({
480+
target: { type: ResourceType.PROMPTGROUP, id: agentObjectId },
481+
metadata: {
482+
principalType: PrincipalType.USER,
483+
principalId: revokedUserId,
484+
permBits: 3,
485+
previousPermBits: 0,
486+
roleId: 'role-1',
487+
previousRoleId: '',
488+
resourceId: agentObjectId,
489+
},
490+
}),
491+
);
492+
expect(JSON.stringify(entries)).not.toMatch(/Jane Doe|Team/);
493+
expect(res.status).toHaveBeenCalledWith(200);
494+
});
495+
496+
it('fails the request before writing when the ACL audit write fails closed', async () => {
497+
const aclChanges = [
498+
{
499+
action: 'assigned',
500+
previous: null,
501+
written: { permBits: 3, roleId: 'role-1' },
502+
principal: { type: PrincipalType.USER, id: revokedUserId },
503+
},
504+
];
505+
mockBulkUpdateResourcePermissions.mockResolvedValue({
506+
granted: [],
507+
updated: [],
508+
revoked: [],
509+
aclChanges,
510+
insightsChanges: [],
511+
errors: [],
512+
});
513+
mockRecordAuditEntry.mockRejectedValue(new Error('audit unavailable'));
514+
const req = createMockReq({
515+
params: { resourceType: ResourceType.PROMPTGROUP, resourceId: agentObjectId },
516+
user: { id: 'user-id', role: SystemRoles.USER },
517+
});
518+
const res = createMockRes();
519+
520+
await updateResourcePermissions(req, res);
521+
522+
expect(res.status).toHaveBeenCalledWith(500);
523+
});
524+
525+
it('applies the permissions and logs when the audit write fails with the opt-out', async () => {
526+
process.env.AUDIT_LOG_FAIL_CLOSED = 'false';
527+
mockBulkUpdateResourcePermissions.mockResolvedValue({
528+
granted: [],
529+
updated: [],
530+
revoked: [],
531+
aclChanges: [
532+
{
533+
action: 'removed',
534+
previous: { permBits: 1, roleId: 'role-2' },
535+
written: null,
536+
principal: { type: PrincipalType.USER, id: revokedUserId },
537+
},
538+
],
539+
insightsChanges: [],
540+
errors: [],
541+
});
542+
mockRecordAuditEntry.mockRejectedValue(new Error('audit unavailable'));
543+
const req = createMockReq({
544+
params: { resourceType: ResourceType.PROMPTGROUP, resourceId: agentObjectId },
545+
user: { id: 'user-id', role: SystemRoles.USER },
546+
});
547+
const res = createMockRes();
548+
549+
await updateResourcePermissions(req, res);
550+
551+
expect(mockRecordAuditEntry).toHaveBeenCalledWith(expect.anything(), { failClosed: false });
421552
expect(res.status).toHaveBeenCalledWith(200);
422553
});
423554

‎api/server/routes/accessPermissions.js‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,13 @@ const {
1919
checkShareAccess,
2020
checkSharePublicAccess,
2121
} = require('~/server/middleware/checkSharePublicAccess');
22-
const { requireJwtAuth, checkBan, uaParser, canAccessResource } = require('~/server/middleware');
22+
const {
23+
requireJwtAuth,
24+
checkBan,
25+
uaParser,
26+
canAccessResource,
27+
configMiddleware,
28+
} = require('~/server/middleware');
2329
const { checkPeoplePickerAccess } = require('~/server/middleware/checkPeoplePickerAccess');
2430
const db = require('~/models');
2531
const { findMCPServerByObjectId, getSkillById } = db;
@@ -198,6 +204,7 @@ router.put(
198204
checkShareAccessUnlessAgentAdmin,
199205
checkSharePublicAccess,
200206
rejectSharedLinkOwnerPermissionChanges,
207+
configMiddleware,
201208
updateResourcePermissions,
202209
);
203210

‎api/server/routes/accessPermissions.sharePolicy.test.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@ jest.mock('~/server/middleware', () => ({
99
checkBan: (_req, _res, next) => next(),
1010
uaParser: (_req, _res, next) => next(),
1111
canAccessResource: jest.fn(() => (_req, _res, next) => next()),
12+
configMiddleware: (req, _res, next) => {
13+
req.config = { config: { permissions: { leaseTtlMs: 45000 } } };
14+
next();
15+
},
1216
}));
1317

1418
jest.mock('~/server/middleware/checkPeoplePickerAccess', () => ({
@@ -207,6 +211,22 @@ describe('Access permissions share policy', () => {
207211
expect(updateResourcePermissions).toHaveBeenCalledTimes(1);
208212
});
209213

214+
it('loads the app config before the update so permissions.lease* values reach the controller', async () => {
215+
getRoleByName.mockResolvedValue({
216+
permissions: {
217+
[PermissionTypes.SKILLS]: { [Permissions.SHARE]: true, [Permissions.SHARE_PUBLIC]: false },
218+
},
219+
});
220+
221+
await request(app)
222+
.put(`/api/permissions/${ResourceType.SKILL}/${resourceId}`)
223+
.send({ updated: [createUpdatedPrincipal(AccessRoleIds.SKILL_VIEWER)], public: false });
224+
225+
expect(updateResourcePermissions.mock.calls[0][0].config.config.permissions.leaseTtlMs).toBe(
226+
45000,
227+
);
228+
});
229+
210230
it('preserves resource management capability bypass for non-public skill sharing', async () => {
211231
hasCapability.mockResolvedValue(true);
212232

0 commit comments

Comments
 (0)