mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-06-30 03:12:11 +00:00
🧷 fix: Preserve Document Priority on Section-Scoped Config Writes (#13772)
The patch and tombstone admin-config handlers accept a priority field on the request body, which controls the position of the entire Config document in the precedence cascade. Today, that priority is written unconditionally, even when the caller holds only a section-scoped grant such as manage:configs:memory. On a document containing overrides for other sections, this lets a section-scoped caller silently reorder overrides they have no permission to author. This is a defense-in-depth fix for deployments using section-scoped grants. In a vanilla setup where admins hold broad manage:configs, the path is unreachable because the broad-capability short-circuit lets every priority change through, so the fix is a no-op for those callers. The change makes the contract safe-by-construction for any deployment that narrows the auth model, at no cost to upstream behavior.
This commit is contained in:
Dustin Healy
GitHub
parent
68d142d0e9
commit
84886c56fb
2 changed files with 195 additions and 7 deletions
|
|
@ -647,6 +647,174 @@ describe('createAdminConfigHandlers', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('patchConfigField: section-scoped priority preservation', () => {
|
||||
it('ignores request-supplied priority when caller lacks broad manage:configs', async () => {
|
||||
const { handlers, deps } = createHandlers({
|
||||
hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'),
|
||||
findConfigByPrincipal: jest
|
||||
.fn()
|
||||
.mockResolvedValue({ _id: 'c1', priority: 7, overrides: {} }),
|
||||
});
|
||||
const req = mockReq({
|
||||
params: { principalType: 'role', principalId: 'admin' },
|
||||
body: {
|
||||
entries: [{ fieldPath: 'memory.context', value: 'updated' }],
|
||||
priority: 999,
|
||||
},
|
||||
});
|
||||
const res = mockRes();
|
||||
|
||||
await handlers.patchConfigField(req, res);
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0];
|
||||
expect(priorityArg).toBe(7);
|
||||
});
|
||||
|
||||
it('falls back to default priority when no existing doc and caller lacks broad manage', async () => {
|
||||
const { handlers, deps } = createHandlers({
|
||||
hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'),
|
||||
findConfigByPrincipal: jest.fn().mockResolvedValue(null),
|
||||
});
|
||||
const req = mockReq({
|
||||
params: { principalType: 'role', principalId: 'admin' },
|
||||
body: {
|
||||
entries: [{ fieldPath: 'memory.context', value: 'updated' }],
|
||||
priority: 999,
|
||||
},
|
||||
});
|
||||
const res = mockRes();
|
||||
|
||||
await handlers.patchConfigField(req, res);
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0];
|
||||
expect(priorityArg).toBe(10);
|
||||
});
|
||||
|
||||
it('honors request-supplied priority when caller holds broad manage:configs', async () => {
|
||||
const { handlers, deps } = createHandlers({
|
||||
hasConfigCapability: jest.fn().mockResolvedValue(true),
|
||||
});
|
||||
const req = mockReq({
|
||||
params: { principalType: 'role', principalId: 'admin' },
|
||||
body: {
|
||||
entries: [{ fieldPath: 'memory.context', value: 'updated' }],
|
||||
priority: 999,
|
||||
},
|
||||
});
|
||||
const res = mockRes();
|
||||
|
||||
await handlers.patchConfigField(req, res);
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0];
|
||||
expect(priorityArg).toBe(999);
|
||||
expect(deps.findConfigByPrincipal).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('preserves priority 0 when broad caller supplies it', async () => {
|
||||
const { handlers, deps } = createHandlers({
|
||||
hasConfigCapability: jest.fn().mockResolvedValue(true),
|
||||
});
|
||||
const req = mockReq({
|
||||
params: { principalType: 'role', principalId: 'admin' },
|
||||
body: {
|
||||
entries: [{ fieldPath: 'memory.context', value: 'updated' }],
|
||||
priority: 0,
|
||||
},
|
||||
});
|
||||
const res = mockRes();
|
||||
|
||||
await handlers.patchConfigField(req, res);
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0];
|
||||
expect(priorityArg).toBe(0);
|
||||
});
|
||||
|
||||
it('preserves existing priority 0 for section-scoped callers', async () => {
|
||||
const { handlers, deps } = createHandlers({
|
||||
hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'),
|
||||
findConfigByPrincipal: jest
|
||||
.fn()
|
||||
.mockResolvedValue({ _id: 'c1', priority: 0, overrides: {} }),
|
||||
});
|
||||
const req = mockReq({
|
||||
params: { principalType: 'role', principalId: 'admin' },
|
||||
body: {
|
||||
entries: [{ fieldPath: 'memory.context', value: 'updated' }],
|
||||
priority: 999,
|
||||
},
|
||||
});
|
||||
const res = mockRes();
|
||||
|
||||
await handlers.patchConfigField(req, res);
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0];
|
||||
expect(priorityArg).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('tombstoneConfigField: section-scoped priority preservation', () => {
|
||||
it('ignores request-supplied priority when caller lacks broad manage:configs', async () => {
|
||||
const { handlers, deps } = createHandlers({
|
||||
hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'),
|
||||
findConfigByPrincipal: jest
|
||||
.fn()
|
||||
.mockResolvedValue({ _id: 'c1', priority: 7, overrides: {} }),
|
||||
});
|
||||
const req = mockReq({
|
||||
params: { principalType: 'role', principalId: 'admin' },
|
||||
body: { fieldPath: 'memory.context', priority: 999 },
|
||||
});
|
||||
const res = mockRes();
|
||||
|
||||
await handlers.tombstoneConfigField(req, res);
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const [, , , , priorityArg] = deps.tombstoneConfigField.mock.calls[0];
|
||||
expect(priorityArg).toBe(7);
|
||||
});
|
||||
|
||||
it('falls back to default priority when no existing doc and caller lacks broad manage', async () => {
|
||||
const { handlers, deps } = createHandlers({
|
||||
hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'),
|
||||
findConfigByPrincipal: jest.fn().mockResolvedValue(null),
|
||||
});
|
||||
const req = mockReq({
|
||||
params: { principalType: 'role', principalId: 'admin' },
|
||||
body: { fieldPath: 'memory.context', priority: 999 },
|
||||
});
|
||||
const res = mockRes();
|
||||
|
||||
await handlers.tombstoneConfigField(req, res);
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const [, , , , priorityArg] = deps.tombstoneConfigField.mock.calls[0];
|
||||
expect(priorityArg).toBe(10);
|
||||
});
|
||||
|
||||
it('honors request-supplied priority when caller holds broad manage:configs', async () => {
|
||||
const { handlers, deps } = createHandlers({
|
||||
hasConfigCapability: jest.fn().mockResolvedValue(true),
|
||||
});
|
||||
const req = mockReq({
|
||||
params: { principalType: 'role', principalId: 'admin' },
|
||||
body: { fieldPath: 'memory.context', priority: 999 },
|
||||
});
|
||||
const res = mockRes();
|
||||
|
||||
await handlers.tombstoneConfigField(req, res);
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const [, , , , priorityArg] = deps.tombstoneConfigField.mock.calls[0];
|
||||
expect(priorityArg).toBe(999);
|
||||
expect(deps.findConfigByPrincipal).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('upsertConfigOverrides — Bug 2 regression', () => {
|
||||
it('returns 403 for empty overrides when user lacks MANAGE_CONFIGS', async () => {
|
||||
const { handlers } = createHandlers({
|
||||
|
|
|
|||
|
|
@ -466,14 +466,16 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
|||
return true;
|
||||
});
|
||||
|
||||
const hasBroadManage = await hasConfigCapability(user, null, 'manage');
|
||||
|
||||
if (validEntries.length === 0) {
|
||||
if (!(await hasConfigCapability(user, null, 'manage'))) {
|
||||
if (!hasBroadManage) {
|
||||
return res.status(403).json({ error: 'Insufficient permissions' });
|
||||
}
|
||||
return res.status(200).json({ message: 'No actionable field entries provided' });
|
||||
}
|
||||
|
||||
if (!(await hasConfigCapability(user, null, 'manage'))) {
|
||||
if (!hasBroadManage) {
|
||||
const sections = [...new Set(validEntries.map((e) => getTopLevelSection(e.fieldPath)))];
|
||||
const allowed = await Promise.all(
|
||||
sections.map((s) => hasConfigCapability(user, s as ConfigSection, 'manage')),
|
||||
|
|
@ -496,8 +498,15 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
|||
fields[entry.fieldPath] = entry.value;
|
||||
}
|
||||
|
||||
if (priority != null && !hasBroadManage) {
|
||||
logger.warn(
|
||||
`[adminConfig] Ignoring caller-supplied priority on section-scoped patch to ${principalType}/${principalId}: only broad manage:configs may modify document priority`,
|
||||
);
|
||||
}
|
||||
const requestedPriority = hasBroadManage ? priority : undefined;
|
||||
|
||||
const existing =
|
||||
priority == null
|
||||
requestedPriority == null
|
||||
? await findConfigByPrincipal(principalType, principalId, { includeInactive: true })
|
||||
: null;
|
||||
|
||||
|
|
@ -506,7 +515,7 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
|||
principalId,
|
||||
principalModel(principalType),
|
||||
fields,
|
||||
priority ?? existing?.priority ?? DEFAULT_PRIORITY,
|
||||
requestedPriority ?? existing?.priority ?? DEFAULT_PRIORITY,
|
||||
);
|
||||
|
||||
invalidateConfigCaches?.(user.tenantId)?.catch((err) =>
|
||||
|
|
@ -557,7 +566,11 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
|||
|
||||
const section = getTopLevelSection(fieldPath);
|
||||
|
||||
if (!(await hasConfigCapability(user, section as ConfigSection, 'manage'))) {
|
||||
const hasBroadManage = await hasConfigCapability(user, null, 'manage');
|
||||
if (
|
||||
!hasBroadManage &&
|
||||
!(await hasConfigCapability(user, section as ConfigSection, 'manage'))
|
||||
) {
|
||||
return res.status(403).json({
|
||||
error: `Insufficient permissions for config section: ${section}`,
|
||||
});
|
||||
|
|
@ -570,8 +583,15 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
|||
return res.status(200).json({ message: 'No actionable field path provided' });
|
||||
}
|
||||
|
||||
if (priority != null && !hasBroadManage) {
|
||||
logger.warn(
|
||||
`[adminConfig] Ignoring caller-supplied priority on section-scoped tombstone for ${principalType}/${principalId}: only broad manage:configs may modify document priority`,
|
||||
);
|
||||
}
|
||||
const requestedPriority = hasBroadManage ? priority : undefined;
|
||||
|
||||
const existing =
|
||||
priority == null
|
||||
requestedPriority == null
|
||||
? await findConfigByPrincipal(principalType, principalId, { includeInactive: true })
|
||||
: null;
|
||||
|
||||
|
|
@ -580,7 +600,7 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): {
|
|||
principalId,
|
||||
principalModel(principalType),
|
||||
fieldPath,
|
||||
priority ?? existing?.priority ?? DEFAULT_PRIORITY,
|
||||
requestedPriority ?? existing?.priority ?? DEFAULT_PRIORITY,
|
||||
);
|
||||
|
||||
invalidateConfigCaches?.(user.tenantId)?.catch((err) =>
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue