🤖 fix: Edge Case for Agent List Access Control

- Refactored `getListAgentsByAccess` to streamline query construction for accessible agents.
- Added comprehensive security tests for `getListAgentsByAccess` and `getListAgentsHandler` to ensure proper access control and filtering based on user permissions.
- Enhanced test coverage for various scenarios, including pagination, category filtering, and handling of non-existent IDs.
This commit is contained in:
Danny Avila 2025-08-05 19:25:03 -04:00
parent c191af6c9b
commit 9585db14ba
No known key found for this signature in database
GPG key ID: BF31EEB2C5CA0956
3 changed files with 600 additions and 6 deletions

View file

@ -533,11 +533,7 @@ const getListAgentsByAccess = async ({
const normalizedLimit = isPaginated ? Math.min(Math.max(1, parseInt(limit) || 20), 100) : null;
// Build base query combining ACL accessible agents with other filters
const baseQuery = { ...otherParams };
if (accessibleIds.length > 0) {
baseQuery._id = { $in: accessibleIds };
}
const baseQuery = { ...otherParams, _id: { $in: accessibleIds } };
// Add cursor condition
if (after) {

View file

@ -22,6 +22,7 @@ const {
updateAgent,
deleteAgent,
getListAgents,
getListAgentsByAccess,
revertAgentVersion,
updateAgentProjects,
addAgentResourceFile,
@ -3034,6 +3035,212 @@ describe('Support Contact Field', () => {
// Verify support_contact is undefined when not provided
expect(agent.support_contact).toBeUndefined();
});
describe('getListAgentsByAccess - Security Tests', () => {
let userA, userB;
let agentA1, agentA2, agentA3;
beforeEach(async () => {
Agent = mongoose.models.Agent || mongoose.model('Agent', agentSchema);
await Agent.deleteMany({});
await AclEntry.deleteMany({});
// Create two users
userA = new mongoose.Types.ObjectId();
userB = new mongoose.Types.ObjectId();
// Create agents for user A
agentA1 = await createAgent({
id: `agent_${uuidv4().slice(0, 12)}`,
name: 'Agent A1',
description: 'User A agent 1',
provider: 'openai',
model: 'gpt-4',
author: userA,
});
agentA2 = await createAgent({
id: `agent_${uuidv4().slice(0, 12)}`,
name: 'Agent A2',
description: 'User A agent 2',
provider: 'openai',
model: 'gpt-4',
author: userA,
});
agentA3 = await createAgent({
id: `agent_${uuidv4().slice(0, 12)}`,
name: 'Agent A3',
description: 'User A agent 3',
provider: 'openai',
model: 'gpt-4',
author: userA,
});
});
test('should return empty list when user has no accessible agents (empty accessibleIds)', async () => {
// User B has no agents and no shared agents
const result = await getListAgentsByAccess({
accessibleIds: [],
otherParams: {},
});
expect(result.data).toHaveLength(0);
expect(result.has_more).toBe(false);
expect(result.first_id).toBeNull();
expect(result.last_id).toBeNull();
});
test('should not return other users agents when accessibleIds is empty', async () => {
// User B trying to list agents with empty accessibleIds should not see User A's agents
const result = await getListAgentsByAccess({
accessibleIds: [],
otherParams: { author: userB },
});
expect(result.data).toHaveLength(0);
expect(result.has_more).toBe(false);
});
test('should only return agents in accessibleIds list', async () => {
// Give User B access to only one of User A's agents
const accessibleIds = [agentA1._id];
const result = await getListAgentsByAccess({
accessibleIds,
otherParams: {},
});
expect(result.data).toHaveLength(1);
expect(result.data[0].id).toBe(agentA1.id);
expect(result.data[0].name).toBe('Agent A1');
});
test('should return multiple accessible agents when provided', async () => {
// Give User B access to two of User A's agents
const accessibleIds = [agentA1._id, agentA3._id];
const result = await getListAgentsByAccess({
accessibleIds,
otherParams: {},
});
expect(result.data).toHaveLength(2);
const returnedIds = result.data.map((agent) => agent.id);
expect(returnedIds).toContain(agentA1.id);
expect(returnedIds).toContain(agentA3.id);
expect(returnedIds).not.toContain(agentA2.id);
});
test('should respect other query parameters while enforcing accessibleIds', async () => {
// Give access to all agents but filter by name
const accessibleIds = [agentA1._id, agentA2._id, agentA3._id];
const result = await getListAgentsByAccess({
accessibleIds,
otherParams: { name: 'Agent A2' },
});
expect(result.data).toHaveLength(1);
expect(result.data[0].id).toBe(agentA2.id);
});
test('should handle pagination correctly with accessibleIds filter', async () => {
// Create more agents
const moreAgents = [];
for (let i = 4; i <= 10; i++) {
const agent = await createAgent({
id: `agent_${uuidv4().slice(0, 12)}`,
name: `Agent A${i}`,
description: `User A agent ${i}`,
provider: 'openai',
model: 'gpt-4',
author: userA,
});
moreAgents.push(agent);
}
// Give access to all agents
const allAgentIds = [agentA1, agentA2, agentA3, ...moreAgents].map((a) => a._id);
// First page
const page1 = await getListAgentsByAccess({
accessibleIds: allAgentIds,
otherParams: {},
limit: 5,
});
expect(page1.data).toHaveLength(5);
expect(page1.has_more).toBe(true);
expect(page1.after).toBeTruthy();
// Second page
const page2 = await getListAgentsByAccess({
accessibleIds: allAgentIds,
otherParams: {},
limit: 5,
after: page1.after,
});
expect(page2.data).toHaveLength(5);
expect(page2.has_more).toBe(false);
// Verify no overlap between pages
const page1Ids = page1.data.map((a) => a.id);
const page2Ids = page2.data.map((a) => a.id);
const intersection = page1Ids.filter((id) => page2Ids.includes(id));
expect(intersection).toHaveLength(0);
});
test('should return empty list when accessibleIds contains non-existent IDs', async () => {
// Try with non-existent agent IDs
const fakeIds = [new mongoose.Types.ObjectId(), new mongoose.Types.ObjectId()];
const result = await getListAgentsByAccess({
accessibleIds: fakeIds,
otherParams: {},
});
expect(result.data).toHaveLength(0);
expect(result.has_more).toBe(false);
});
test('should handle undefined accessibleIds as empty array', async () => {
// When accessibleIds is undefined, it should be treated as empty array
const result = await getListAgentsByAccess({
accessibleIds: undefined,
otherParams: {},
});
expect(result.data).toHaveLength(0);
expect(result.has_more).toBe(false);
});
test('should combine accessibleIds with author filter correctly', async () => {
// Create an agent for User B
const agentB1 = await createAgent({
id: `agent_${uuidv4().slice(0, 12)}`,
name: 'Agent B1',
description: 'User B agent 1',
provider: 'openai',
model: 'gpt-4',
author: userB,
});
// Give User B access to one of User A's agents
const accessibleIds = [agentA1._id, agentB1._id];
// Filter by author should further restrict the results
const result = await getListAgentsByAccess({
accessibleIds,
otherParams: { author: userB },
});
expect(result.data).toHaveLength(1);
expect(result.data[0].id).toBe(agentB1.id);
expect(result.data[0].author).toBe(userB.toString());
});
});
});
function createBasicAgent(overrides = {}) {

View file

@ -1,5 +1,6 @@
const mongoose = require('mongoose');
const { v4: uuidv4 } = require('uuid');
const { nanoid } = require('nanoid');
const { MongoMemoryServer } = require('mongodb-memory-server');
const { agentSchema } = require('@librechat/data-schemas');
@ -41,7 +42,27 @@ jest.mock('~/models/File', () => ({
deleteFileByFilter: jest.fn(),
}));
const { createAgent: createAgentHandler, updateAgent: updateAgentHandler } = require('./v1');
jest.mock('~/server/services/PermissionService', () => ({
findAccessibleResources: jest.fn().mockResolvedValue([]),
findPubliclyAccessibleResources: jest.fn().mockResolvedValue([]),
grantPermission: jest.fn(),
hasPublicPermission: jest.fn().mockResolvedValue(false),
}));
jest.mock('~/models', () => ({
getCategoriesWithCounts: jest.fn(),
}));
const {
createAgent: createAgentHandler,
updateAgent: updateAgentHandler,
getListAgents: getListAgentsHandler,
} = require('./v1');
const {
findAccessibleResources,
findPubliclyAccessibleResources,
} = require('~/server/services/PermissionService');
/**
* @type {import('mongoose').Model<import('@librechat/data-schemas').IAgent>}
@ -79,6 +100,7 @@ describe('Agent Controllers - Mass Assignment Protection', () => {
},
body: {},
params: {},
query: {},
app: {
locals: {
fileStrategy: 'local',
@ -668,4 +690,373 @@ describe('Agent Controllers - Mass Assignment Protection', () => {
expect(agentInDb.futureFeature).toBeUndefined();
});
});
describe('getListAgentsHandler - Security Tests', () => {
let userA, userB;
let agentA1, agentA2, agentA3, agentB1;
beforeEach(async () => {
await Agent.deleteMany({});
jest.clearAllMocks();
// Create two test users
userA = new mongoose.Types.ObjectId();
userB = new mongoose.Types.ObjectId();
// Create agents for User A
agentA1 = await Agent.create({
id: `agent_${nanoid(12)}`,
name: 'Agent A1',
description: 'User A agent 1',
provider: 'openai',
model: 'gpt-4',
author: userA,
versions: [
{
name: 'Agent A1',
description: 'User A agent 1',
provider: 'openai',
model: 'gpt-4',
createdAt: new Date(),
updatedAt: new Date(),
},
],
});
agentA2 = await Agent.create({
id: `agent_${nanoid(12)}`,
name: 'Agent A2',
description: 'User A agent 2',
provider: 'openai',
model: 'gpt-4',
author: userA,
versions: [
{
name: 'Agent A2',
description: 'User A agent 2',
provider: 'openai',
model: 'gpt-4',
createdAt: new Date(),
updatedAt: new Date(),
},
],
});
agentA3 = await Agent.create({
id: `agent_${nanoid(12)}`,
name: 'Agent A3',
description: 'User A agent 3',
provider: 'openai',
model: 'gpt-4',
author: userA,
category: 'productivity',
versions: [
{
name: 'Agent A3',
description: 'User A agent 3',
provider: 'openai',
model: 'gpt-4',
category: 'productivity',
createdAt: new Date(),
updatedAt: new Date(),
},
],
});
// Create an agent for User B
agentB1 = await Agent.create({
id: `agent_${nanoid(12)}`,
name: 'Agent B1',
description: 'User B agent 1',
provider: 'openai',
model: 'gpt-4',
author: userB,
versions: [
{
name: 'Agent B1',
description: 'User B agent 1',
provider: 'openai',
model: 'gpt-4',
createdAt: new Date(),
updatedAt: new Date(),
},
],
});
});
test('should return empty list when user has no accessible agents', async () => {
// User B has no permissions and no owned agents
mockReq.user.id = userB.toString();
findAccessibleResources.mockResolvedValue([]);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
expect(findAccessibleResources).toHaveBeenCalledWith({
userId: userB.toString(),
role: 'USER',
resourceType: 'agent',
requiredPermissions: 1, // VIEW permission
});
expect(mockRes.json).toHaveBeenCalledWith({
object: 'list',
data: [],
first_id: null,
last_id: null,
has_more: false,
after: null,
});
});
test('should not return other users agents when accessibleIds is empty', async () => {
// User B trying to see agents with no permissions
mockReq.user.id = userB.toString();
findAccessibleResources.mockResolvedValue([]);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(0);
// Verify User A's agents are not included
const agentIds = response.data.map((a) => a.id);
expect(agentIds).not.toContain(agentA1.id);
expect(agentIds).not.toContain(agentA2.id);
expect(agentIds).not.toContain(agentA3.id);
});
test('should only return agents user has access to', async () => {
// User B has access to one of User A's agents
mockReq.user.id = userB.toString();
findAccessibleResources.mockResolvedValue([agentA1._id]);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(1);
expect(response.data[0].id).toBe(agentA1.id);
expect(response.data[0].name).toBe('Agent A1');
});
test('should return multiple accessible agents', async () => {
// User B has access to multiple agents
mockReq.user.id = userB.toString();
findAccessibleResources.mockResolvedValue([agentA1._id, agentA3._id, agentB1._id]);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(3);
const agentIds = response.data.map((a) => a.id);
expect(agentIds).toContain(agentA1.id);
expect(agentIds).toContain(agentA3.id);
expect(agentIds).toContain(agentB1.id);
expect(agentIds).not.toContain(agentA2.id);
});
test('should apply category filter correctly with ACL', async () => {
// User has access to all agents but filters by category
mockReq.user.id = userB.toString();
mockReq.query.category = 'productivity';
findAccessibleResources.mockResolvedValue([agentA1._id, agentA2._id, agentA3._id]);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(1);
expect(response.data[0].id).toBe(agentA3.id);
expect(response.data[0].category).toBe('productivity');
});
test('should apply search filter correctly with ACL', async () => {
// User has access to multiple agents but searches for specific one
mockReq.user.id = userB.toString();
mockReq.query.search = 'A2';
findAccessibleResources.mockResolvedValue([agentA1._id, agentA2._id, agentA3._id]);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(1);
expect(response.data[0].id).toBe(agentA2.id);
});
test('should handle pagination with ACL filtering', async () => {
// Create more agents for pagination testing
const moreAgents = [];
for (let i = 4; i <= 10; i++) {
const agent = await Agent.create({
id: `agent_${nanoid(12)}`,
name: `Agent A${i}`,
description: `User A agent ${i}`,
provider: 'openai',
model: 'gpt-4',
author: userA,
versions: [
{
name: `Agent A${i}`,
description: `User A agent ${i}`,
provider: 'openai',
model: 'gpt-4',
createdAt: new Date(),
updatedAt: new Date(),
},
],
});
moreAgents.push(agent);
}
// User has access to all agents
const allAgentIds = [agentA1, agentA2, agentA3, ...moreAgents].map((a) => a._id);
mockReq.user.id = userB.toString();
mockReq.query.limit = '5';
findAccessibleResources.mockResolvedValue(allAgentIds);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(5);
expect(response.has_more).toBe(true);
expect(response.after).toBeTruthy();
});
test('should mark publicly accessible agents', async () => {
// User has access to agents, some are public
mockReq.user.id = userB.toString();
findAccessibleResources.mockResolvedValue([agentA1._id, agentA2._id]);
findPubliclyAccessibleResources.mockResolvedValue([agentA2._id]);
await getListAgentsHandler(mockReq, mockRes);
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(2);
const publicAgent = response.data.find((a) => a.id === agentA2.id);
const privateAgent = response.data.find((a) => a.id === agentA1.id);
expect(publicAgent.isPublic).toBe(true);
expect(privateAgent.isPublic).toBeUndefined();
});
test('should handle requiredPermission parameter', async () => {
// Test with different permission levels
mockReq.user.id = userB.toString();
mockReq.query.requiredPermission = '15'; // FULL_ACCESS
findAccessibleResources.mockResolvedValue([agentA1._id]);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
expect(findAccessibleResources).toHaveBeenCalledWith({
userId: userB.toString(),
role: 'USER',
resourceType: 'agent',
requiredPermissions: 15,
});
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(1);
});
test('should handle promoted filter with ACL', async () => {
// Create a promoted agent
const promotedAgent = await Agent.create({
id: `agent_${nanoid(12)}`,
name: 'Promoted Agent',
description: 'A promoted agent',
provider: 'openai',
model: 'gpt-4',
author: userA,
is_promoted: true,
versions: [
{
name: 'Promoted Agent',
description: 'A promoted agent',
provider: 'openai',
model: 'gpt-4',
is_promoted: true,
createdAt: new Date(),
updatedAt: new Date(),
},
],
});
mockReq.user.id = userB.toString();
mockReq.query.promoted = '1';
findAccessibleResources.mockResolvedValue([agentA1._id, agentA2._id, promotedAgent._id]);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(1);
expect(response.data[0].id).toBe(promotedAgent.id);
expect(response.data[0].is_promoted).toBe(true);
});
test('should handle errors gracefully', async () => {
mockReq.user.id = userB.toString();
findAccessibleResources.mockRejectedValue(new Error('Permission service error'));
await getListAgentsHandler(mockReq, mockRes);
expect(mockRes.status).toHaveBeenCalledWith(500);
expect(mockRes.json).toHaveBeenCalledWith({
error: 'Permission service error',
});
});
test('should respect combined filters with ACL', async () => {
// Create agents with specific attributes
const productivityPromoted = await Agent.create({
id: `agent_${nanoid(12)}`,
name: 'Productivity Pro',
description: 'A promoted productivity agent',
provider: 'openai',
model: 'gpt-4',
author: userA,
category: 'productivity',
is_promoted: true,
versions: [
{
name: 'Productivity Pro',
description: 'A promoted productivity agent',
provider: 'openai',
model: 'gpt-4',
category: 'productivity',
is_promoted: true,
createdAt: new Date(),
updatedAt: new Date(),
},
],
});
mockReq.user.id = userB.toString();
mockReq.query.category = 'productivity';
mockReq.query.promoted = '1';
findAccessibleResources.mockResolvedValue([
agentA1._id,
agentA2._id,
agentA3._id,
productivityPromoted._id,
]);
findPubliclyAccessibleResources.mockResolvedValue([]);
await getListAgentsHandler(mockReq, mockRes);
const response = mockRes.json.mock.calls[0][0];
expect(response.data).toHaveLength(1);
expect(response.data[0].id).toBe(productivityPromoted.id);
expect(response.data[0].category).toBe('productivity');
expect(response.data[0].is_promoted).toBe(true);
});
});
});