diff --git a/api/models/Agent.js b/api/models/Agent.js index 004a51cfec..be9fe62e6b 100644 --- a/api/models/Agent.js +++ b/api/models/Agent.js @@ -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) { diff --git a/api/models/Agent.spec.js b/api/models/Agent.spec.js index 65a31c49af..db23fadaec 100644 --- a/api/models/Agent.spec.js +++ b/api/models/Agent.spec.js @@ -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 = {}) { diff --git a/api/server/controllers/agents/v1.spec.js b/api/server/controllers/agents/v1.spec.js index dd12634d64..c31839feb1 100644 --- a/api/server/controllers/agents/v1.spec.js +++ b/api/server/controllers/agents/v1.spec.js @@ -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} @@ -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); + }); + }); });