mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-16 12:46:34 +01:00
🛡️ fix: Enforce MULTI_CONVO and agent ACL checks on addedConvo (#12243)
* 🛡️ fix: Enforce MULTI_CONVO and agent ACL checks on addedConvo
addedConvo.agent_id was passed through to loadAddedAgent without any
permission check, enabling an authenticated user to load and execute
another user's private agent via the parallel multi-convo feature.
The middleware now chains a checkAddedConvoAccess gate after the primary
agent check: when req.body.addedConvo is present it verifies the user
has MULTI_CONVO:USE role permission, and when the addedConvo agent_id is
a real (non-ephemeral) agent it runs the same canAccessResource ACL
check used for the primary agent.
* refactor: Harden addedConvo middleware and avoid duplicate agent fetch
- Convert checkAddedConvoAccess to curried factory matching Express
middleware signature: (requiredPermission) => (req, res, next)
- Call checkPermission directly for the addedConvo agent instead of
routing through canAccessResource's tempReq pattern; this avoids
orphaning the resolved agent document and enables caching it on
req.resolvedAddedAgent for downstream loadAddedAgent
- Update loadAddedAgent to use req.resolvedAddedAgent when available,
eliminating a duplicate getAgent DB call per chat request
- Validate addedConvo is a plain object and agent_id is a string
before passing to isEphemeralAgentId (prevents TypeError on object
injection, returns 400-equivalent early exit instead of 500)
- Fix JSDoc: "VIEW access" → "same permission as primary agent",
add @param/@returns to helpers, restore @example on factory
- Fix redundant return await in resolveAgentIdFromBody
* test: Add canAccessAgentFromBody spec covering IDOR fix
26 integration tests using MongoMemoryServer with real models, ACL
entries, and PermissionService — no mocks for core logic.
Covered paths:
- Factory validation (requiredPermission type check)
- Primary agent: missing agent_id, ephemeral, non-agents endpoint
- addedConvo absent / invalid shape (string, array, object injection)
- MULTI_CONVO:USE gate: denied, missing role, ADMIN bypass
- Agent resource ACL: no ACL → 403, insufficient bits → 403,
nonexistent agent → 404, valid ACL → next + cached on req
- End-to-end: both real agents, primary denied short-circuits,
ephemeral primary + real addedConvo
This commit is contained in:
Danny Avila
GitHub
parent
07d0ce4ce9
commit
8dc6d60750
3 changed files with 637 additions and 40 deletions
|
|
@ -0,0 +1,509 @@
|
|||
const mongoose = require('mongoose');
|
||||
const {
|
||||
ResourceType,
|
||||
SystemRoles,
|
||||
PrincipalType,
|
||||
PrincipalModel,
|
||||
} = require('librechat-data-provider');
|
||||
const { MongoMemoryServer } = require('mongodb-memory-server');
|
||||
const { canAccessAgentFromBody } = require('./canAccessAgentFromBody');
|
||||
const { User, Role, AclEntry } = require('~/db/models');
|
||||
const { createAgent } = require('~/models/Agent');
|
||||
|
||||
describe('canAccessAgentFromBody middleware', () => {
|
||||
let mongoServer;
|
||||
let req, res, next;
|
||||
let testUser, otherUser;
|
||||
|
||||
beforeAll(async () => {
|
||||
mongoServer = await MongoMemoryServer.create();
|
||||
await mongoose.connect(mongoServer.getUri());
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await mongoose.disconnect();
|
||||
await mongoServer.stop();
|
||||
});
|
||||
|
||||
beforeEach(async () => {
|
||||
await mongoose.connection.dropDatabase();
|
||||
|
||||
await Role.create({
|
||||
name: 'test-role',
|
||||
permissions: {
|
||||
AGENTS: { USE: true, CREATE: true, SHARE: true },
|
||||
MULTI_CONVO: { USE: true },
|
||||
},
|
||||
});
|
||||
|
||||
await Role.create({
|
||||
name: 'no-multi-convo',
|
||||
permissions: {
|
||||
AGENTS: { USE: true, CREATE: true, SHARE: true },
|
||||
MULTI_CONVO: { USE: false },
|
||||
},
|
||||
});
|
||||
|
||||
await Role.create({
|
||||
name: SystemRoles.ADMIN,
|
||||
permissions: {
|
||||
AGENTS: { USE: true, CREATE: true, SHARE: true },
|
||||
MULTI_CONVO: { USE: true },
|
||||
},
|
||||
});
|
||||
|
||||
testUser = await User.create({
|
||||
email: 'test@example.com',
|
||||
name: 'Test User',
|
||||
username: 'testuser',
|
||||
role: 'test-role',
|
||||
});
|
||||
|
||||
otherUser = await User.create({
|
||||
email: 'other@example.com',
|
||||
name: 'Other User',
|
||||
username: 'otheruser',
|
||||
role: 'test-role',
|
||||
});
|
||||
|
||||
req = {
|
||||
user: { id: testUser._id, role: testUser.role },
|
||||
params: {},
|
||||
body: {
|
||||
endpoint: 'agents',
|
||||
agent_id: 'ephemeral_primary',
|
||||
},
|
||||
};
|
||||
res = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn(),
|
||||
};
|
||||
next = jest.fn();
|
||||
|
||||
jest.clearAllMocks();
|
||||
});
|
||||
|
||||
describe('middleware factory', () => {
|
||||
test('throws if requiredPermission is missing', () => {
|
||||
expect(() => canAccessAgentFromBody({})).toThrow(
|
||||
'canAccessAgentFromBody: requiredPermission is required and must be a number',
|
||||
);
|
||||
});
|
||||
|
||||
test('throws if requiredPermission is not a number', () => {
|
||||
expect(() => canAccessAgentFromBody({ requiredPermission: '1' })).toThrow(
|
||||
'canAccessAgentFromBody: requiredPermission is required and must be a number',
|
||||
);
|
||||
});
|
||||
|
||||
test('returns a middleware function', () => {
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
expect(typeof middleware).toBe('function');
|
||||
expect(middleware.length).toBe(3);
|
||||
});
|
||||
});
|
||||
|
||||
describe('primary agent checks', () => {
|
||||
test('returns 400 when agent_id is missing on agents endpoint', async () => {
|
||||
req.body.agent_id = undefined;
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(res.status).toHaveBeenCalledWith(400);
|
||||
});
|
||||
|
||||
test('proceeds for ephemeral primary agent without addedConvo', async () => {
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
expect(res.status).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('proceeds for non-agents endpoint (ephemeral fallback)', async () => {
|
||||
req.body.endpoint = 'openAI';
|
||||
req.body.agent_id = undefined;
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('addedConvo — absent or invalid shape', () => {
|
||||
test('calls next when addedConvo is absent', async () => {
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('calls next when addedConvo is a string', async () => {
|
||||
req.body.addedConvo = 'not-an-object';
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('calls next when addedConvo is an array', async () => {
|
||||
req.body.addedConvo = [{ agent_id: 'agent_something' }];
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('addedConvo — MULTI_CONVO permission gate', () => {
|
||||
test('returns 403 when user lacks MULTI_CONVO:USE', async () => {
|
||||
req.user.role = 'no-multi-convo';
|
||||
req.body.addedConvo = { agent_id: 'agent_x', endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
expect(res.json).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ message: 'Multi-conversation feature is not enabled' }),
|
||||
);
|
||||
});
|
||||
|
||||
test('returns 403 when user.role is missing', async () => {
|
||||
req.user = { id: testUser._id };
|
||||
req.body.addedConvo = { agent_id: 'agent_x', endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
});
|
||||
|
||||
test('ADMIN bypasses MULTI_CONVO check', async () => {
|
||||
req.user.role = SystemRoles.ADMIN;
|
||||
req.body.addedConvo = { agent_id: 'ephemeral_x', endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
expect(res.status).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('addedConvo — agent_id shape validation', () => {
|
||||
test('calls next when agent_id is ephemeral', async () => {
|
||||
req.body.addedConvo = { agent_id: 'ephemeral_xyz', endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('calls next when agent_id is absent', async () => {
|
||||
req.body.addedConvo = { endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('calls next when agent_id is not a string (object injection)', async () => {
|
||||
req.body.addedConvo = { agent_id: { $gt: '' }, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('addedConvo — agent resource ACL (IDOR prevention)', () => {
|
||||
let addedAgent;
|
||||
|
||||
beforeEach(async () => {
|
||||
addedAgent = await createAgent({
|
||||
id: `agent_added_${Date.now()}`,
|
||||
name: 'Private Agent',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: otherUser._id,
|
||||
});
|
||||
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: otherUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: addedAgent._id,
|
||||
permBits: 15,
|
||||
grantedBy: otherUser._id,
|
||||
});
|
||||
});
|
||||
|
||||
test('returns 403 when requester has no ACL for the added agent', async () => {
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
expect(res.json).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
message: 'Insufficient permissions to access this agent',
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
test('returns 404 when added agent does not exist', async () => {
|
||||
req.body.addedConvo = {
|
||||
agent_id: 'agent_nonexistent_999',
|
||||
endpoint: 'agents',
|
||||
model: 'gpt-4',
|
||||
};
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(res.status).toHaveBeenCalledWith(404);
|
||||
});
|
||||
|
||||
test('proceeds when requester has ACL for the added agent', async () => {
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: testUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: addedAgent._id,
|
||||
permBits: 1,
|
||||
grantedBy: otherUser._id,
|
||||
});
|
||||
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
expect(res.status).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('denies when ACL permission bits are insufficient', async () => {
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: testUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: addedAgent._id,
|
||||
permBits: 1,
|
||||
grantedBy: otherUser._id,
|
||||
});
|
||||
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 2 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
});
|
||||
|
||||
test('caches resolved agent on req.resolvedAddedAgent', async () => {
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: testUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: addedAgent._id,
|
||||
permBits: 1,
|
||||
grantedBy: otherUser._id,
|
||||
});
|
||||
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
expect(req.resolvedAddedAgent).toBeDefined();
|
||||
expect(req.resolvedAddedAgent._id.toString()).toBe(addedAgent._id.toString());
|
||||
});
|
||||
|
||||
test('ADMIN bypasses agent resource ACL for addedConvo', async () => {
|
||||
req.user.role = SystemRoles.ADMIN;
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
expect(res.status).not.toHaveBeenCalled();
|
||||
expect(req.resolvedAddedAgent).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('end-to-end: primary real agent + addedConvo real agent', () => {
|
||||
let primaryAgent, addedAgent;
|
||||
|
||||
beforeEach(async () => {
|
||||
primaryAgent = await createAgent({
|
||||
id: `agent_primary_${Date.now()}`,
|
||||
name: 'Primary Agent',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: testUser._id,
|
||||
});
|
||||
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: testUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: primaryAgent._id,
|
||||
permBits: 15,
|
||||
grantedBy: testUser._id,
|
||||
});
|
||||
|
||||
addedAgent = await createAgent({
|
||||
id: `agent_added_${Date.now()}`,
|
||||
name: 'Added Agent',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: otherUser._id,
|
||||
});
|
||||
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: otherUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: addedAgent._id,
|
||||
permBits: 15,
|
||||
grantedBy: otherUser._id,
|
||||
});
|
||||
|
||||
req.body.agent_id = primaryAgent.id;
|
||||
});
|
||||
|
||||
test('both checks pass when user has ACL for both agents', async () => {
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: testUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: addedAgent._id,
|
||||
permBits: 1,
|
||||
grantedBy: otherUser._id,
|
||||
});
|
||||
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
expect(res.status).not.toHaveBeenCalled();
|
||||
expect(req.resolvedAddedAgent).toBeDefined();
|
||||
});
|
||||
|
||||
test('primary passes but addedConvo denied → 403', async () => {
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
});
|
||||
|
||||
test('primary denied → 403 without reaching addedConvo check', async () => {
|
||||
const foreignAgent = await createAgent({
|
||||
id: `agent_foreign_${Date.now()}`,
|
||||
name: 'Foreign Agent',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: otherUser._id,
|
||||
});
|
||||
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: otherUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: foreignAgent._id,
|
||||
permBits: 15,
|
||||
grantedBy: otherUser._id,
|
||||
});
|
||||
|
||||
req.body.agent_id = foreignAgent.id;
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
});
|
||||
});
|
||||
|
||||
describe('ephemeral primary + real addedConvo agent', () => {
|
||||
let addedAgent;
|
||||
|
||||
beforeEach(async () => {
|
||||
addedAgent = await createAgent({
|
||||
id: `agent_added_${Date.now()}`,
|
||||
name: 'Added Agent',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: otherUser._id,
|
||||
});
|
||||
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: otherUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: addedAgent._id,
|
||||
permBits: 15,
|
||||
grantedBy: otherUser._id,
|
||||
});
|
||||
});
|
||||
|
||||
test('runs full addedConvo ACL check even when primary is ephemeral', async () => {
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
});
|
||||
|
||||
test('proceeds when user has ACL for added agent (ephemeral primary)', async () => {
|
||||
await AclEntry.create({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: testUser._id,
|
||||
principalModel: PrincipalModel.USER,
|
||||
resourceType: ResourceType.AGENT,
|
||||
resourceId: addedAgent._id,
|
||||
permBits: 1,
|
||||
grantedBy: otherUser._id,
|
||||
});
|
||||
|
||||
req.body.addedConvo = { agent_id: addedAgent.id, endpoint: 'agents', model: 'gpt-4' };
|
||||
|
||||
const middleware = canAccessAgentFromBody({ requiredPermission: 1 });
|
||||
await middleware(req, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalled();
|
||||
expect(res.status).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue