diff --git a/api/models/Action.js b/api/models/Action.js index 20aa20a7e4..f14c415d5b 100644 --- a/api/models/Action.js +++ b/api/models/Action.js @@ -4,9 +4,7 @@ const { Action } = require('~/db/models'); * Update an action with new data without overwriting existing properties, * or create a new action if it doesn't exist. * - * @param {Object} searchParams - The search parameters to find the action to update. - * @param {string} searchParams.action_id - The ID of the action to update. - * @param {string} searchParams.user - The user ID of the action's author. + * @param {{ action_id: string, agent_id?: string, assistant_id?: string, user?: string }} searchParams * @param {Object} updateData - An object containing the properties to update. * @returns {Promise} The updated or newly created action document as a plain object. */ @@ -47,10 +45,8 @@ const getActions = async (searchParams, includeSensitive = false) => { /** * Deletes an action by params. * - * @param {Object} searchParams - The search parameters to find the action to delete. - * @param {string} searchParams.action_id - The ID of the action to delete. - * @param {string} searchParams.user - The user ID of the action's author. - * @returns {Promise} A promise that resolves to the deleted action document as a plain object, or null if no document was found. + * @param {{ action_id: string, agent_id?: string, assistant_id?: string, user?: string }} searchParams + * @returns {Promise} The deleted action document as a plain object, or null if no match. */ const deleteAction = async (searchParams) => { return await Action.findOneAndDelete(searchParams).lean(); diff --git a/api/models/Action.spec.js b/api/models/Action.spec.js new file mode 100644 index 0000000000..61a3b10f0f --- /dev/null +++ b/api/models/Action.spec.js @@ -0,0 +1,250 @@ +const mongoose = require('mongoose'); +const { MongoMemoryServer } = require('mongodb-memory-server'); +const { actionSchema } = require('@librechat/data-schemas'); +const { updateAction, getActions, deleteAction } = require('./Action'); + +let mongoServer; + +beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + const mongoUri = mongoServer.getUri(); + if (!mongoose.models.Action) { + mongoose.model('Action', actionSchema); + } + await mongoose.connect(mongoUri); +}, 20000); + +afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); +}); + +beforeEach(async () => { + await mongoose.models.Action.deleteMany({}); +}); + +const userId = new mongoose.Types.ObjectId(); + +describe('Action ownership scoping', () => { + describe('updateAction', () => { + it('updates when action_id and agent_id both match', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_1', + agent_id: 'agent_A', + metadata: { domain: 'example.com' }, + }); + + const result = await updateAction( + { action_id: 'act_1', agent_id: 'agent_A' }, + { metadata: { domain: 'updated.com' } }, + ); + + expect(result).not.toBeNull(); + expect(result.metadata.domain).toBe('updated.com'); + expect(result.agent_id).toBe('agent_A'); + }); + + it('does not update when agent_id does not match (creates a new doc via upsert)', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_1', + agent_id: 'agent_B', + metadata: { domain: 'victim.com', api_key: 'secret' }, + }); + + const result = await updateAction( + { action_id: 'act_1', agent_id: 'agent_A' }, + { user: userId, metadata: { domain: 'attacker.com' } }, + ); + + expect(result.metadata.domain).toBe('attacker.com'); + + const original = await mongoose.models.Action.findOne({ + action_id: 'act_1', + agent_id: 'agent_B', + }).lean(); + expect(original).not.toBeNull(); + expect(original.metadata.domain).toBe('victim.com'); + expect(original.metadata.api_key).toBe('secret'); + }); + + it('updates when action_id and assistant_id both match', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_2', + assistant_id: 'asst_X', + metadata: { domain: 'example.com' }, + }); + + const result = await updateAction( + { action_id: 'act_2', assistant_id: 'asst_X' }, + { metadata: { domain: 'updated.com' } }, + ); + + expect(result).not.toBeNull(); + expect(result.metadata.domain).toBe('updated.com'); + }); + + it('does not overwrite when assistant_id does not match', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_2', + assistant_id: 'asst_victim', + metadata: { domain: 'victim.com', api_key: 'secret' }, + }); + + await updateAction( + { action_id: 'act_2', assistant_id: 'asst_attacker' }, + { user: userId, metadata: { domain: 'attacker.com' } }, + ); + + const original = await mongoose.models.Action.findOne({ + action_id: 'act_2', + assistant_id: 'asst_victim', + }).lean(); + expect(original).not.toBeNull(); + expect(original.metadata.domain).toBe('victim.com'); + expect(original.metadata.api_key).toBe('secret'); + }); + }); + + describe('deleteAction', () => { + it('deletes when action_id and agent_id both match', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_del', + agent_id: 'agent_A', + metadata: { domain: 'example.com' }, + }); + + const result = await deleteAction({ action_id: 'act_del', agent_id: 'agent_A' }); + expect(result).not.toBeNull(); + expect(result.action_id).toBe('act_del'); + + const remaining = await mongoose.models.Action.countDocuments(); + expect(remaining).toBe(0); + }); + + it('returns null and preserves the document when agent_id does not match', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_del', + agent_id: 'agent_B', + metadata: { domain: 'victim.com' }, + }); + + const result = await deleteAction({ action_id: 'act_del', agent_id: 'agent_A' }); + expect(result).toBeNull(); + + const remaining = await mongoose.models.Action.countDocuments(); + expect(remaining).toBe(1); + }); + + it('deletes when action_id and assistant_id both match', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_del_asst', + assistant_id: 'asst_X', + metadata: { domain: 'example.com' }, + }); + + const result = await deleteAction({ action_id: 'act_del_asst', assistant_id: 'asst_X' }); + expect(result).not.toBeNull(); + + const remaining = await mongoose.models.Action.countDocuments(); + expect(remaining).toBe(0); + }); + + it('returns null and preserves the document when assistant_id does not match', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_del_asst', + assistant_id: 'asst_victim', + metadata: { domain: 'victim.com' }, + }); + + const result = await deleteAction({ + action_id: 'act_del_asst', + assistant_id: 'asst_attacker', + }); + expect(result).toBeNull(); + + const remaining = await mongoose.models.Action.countDocuments(); + expect(remaining).toBe(1); + }); + }); + + describe('getActions (unscoped baseline)', () => { + it('returns actions by action_id regardless of agent_id', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_shared', + agent_id: 'agent_B', + metadata: { domain: 'example.com' }, + }); + + const results = await getActions({ action_id: 'act_shared' }, true); + expect(results).toHaveLength(1); + expect(results[0].agent_id).toBe('agent_B'); + }); + + it('returns actions scoped by agent_id when provided', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_scoped', + agent_id: 'agent_A', + metadata: { domain: 'a.com' }, + }); + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_other', + agent_id: 'agent_B', + metadata: { domain: 'b.com' }, + }); + + const results = await getActions({ agent_id: 'agent_A' }); + expect(results).toHaveLength(1); + expect(results[0].action_id).toBe('act_scoped'); + }); + }); + + describe('cross-type protection', () => { + it('updateAction with agent_id filter does not overwrite assistant-owned action', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_cross', + assistant_id: 'asst_victim', + metadata: { domain: 'victim.com', api_key: 'secret' }, + }); + + await updateAction( + { action_id: 'act_cross', agent_id: 'agent_attacker' }, + { user: userId, metadata: { domain: 'evil.com' } }, + ); + + const original = await mongoose.models.Action.findOne({ + action_id: 'act_cross', + assistant_id: 'asst_victim', + }).lean(); + expect(original).not.toBeNull(); + expect(original.metadata.domain).toBe('victim.com'); + expect(original.metadata.api_key).toBe('secret'); + }); + + it('deleteAction with agent_id filter does not delete assistant-owned action', async () => { + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_cross_del', + assistant_id: 'asst_victim', + metadata: { domain: 'victim.com' }, + }); + + const result = await deleteAction({ action_id: 'act_cross_del', agent_id: 'agent_attacker' }); + expect(result).toBeNull(); + + const remaining = await mongoose.models.Action.countDocuments(); + expect(remaining).toBe(1); + }); + }); +}); diff --git a/api/server/controllers/agents/__tests__/v1.duplicate-actions.spec.js b/api/server/controllers/agents/__tests__/v1.duplicate-actions.spec.js new file mode 100644 index 0000000000..cc298bd03a --- /dev/null +++ b/api/server/controllers/agents/__tests__/v1.duplicate-actions.spec.js @@ -0,0 +1,159 @@ +jest.mock('~/server/services/PermissionService', () => ({ + findPubliclyAccessibleResources: jest.fn(), + findAccessibleResources: jest.fn(), + hasPublicPermission: jest.fn(), + grantPermission: jest.fn().mockResolvedValue({}), +})); + +jest.mock('~/server/services/Config', () => ({ + getCachedTools: jest.fn(), + getMCPServerTools: jest.fn(), +})); + +const mongoose = require('mongoose'); +const { actionDelimiter } = require('librechat-data-provider'); +const { agentSchema, actionSchema } = require('@librechat/data-schemas'); +const { MongoMemoryServer } = require('mongodb-memory-server'); +const { duplicateAgent } = require('../v1'); + +let mongoServer; + +beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + const mongoUri = mongoServer.getUri(); + if (!mongoose.models.Agent) { + mongoose.model('Agent', agentSchema); + } + if (!mongoose.models.Action) { + mongoose.model('Action', actionSchema); + } + await mongoose.connect(mongoUri); +}, 20000); + +afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); +}); + +beforeEach(async () => { + await mongoose.models.Agent.deleteMany({}); + await mongoose.models.Action.deleteMany({}); +}); + +describe('duplicateAgentHandler — action domain extraction', () => { + it('builds duplicated action entries using metadata.domain, not action_id', async () => { + const userId = new mongoose.Types.ObjectId(); + const originalAgentId = `agent_original`; + + const agent = await mongoose.models.Agent.create({ + id: originalAgentId, + name: 'Test Agent', + author: userId.toString(), + provider: 'openai', + model: 'gpt-4', + tools: [], + actions: [`api.example.com${actionDelimiter}act_original`], + versions: [{ name: 'Test Agent', createdAt: new Date(), updatedAt: new Date() }], + }); + + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_original', + agent_id: originalAgentId, + metadata: { domain: 'api.example.com' }, + }); + + const req = { + params: { id: agent.id }, + user: { id: userId.toString() }, + }; + const res = { + status: jest.fn().mockReturnThis(), + json: jest.fn(), + }; + + await duplicateAgent(req, res); + + expect(res.status).toHaveBeenCalledWith(201); + + const { agent: newAgent, actions: newActions } = res.json.mock.calls[0][0]; + + expect(newAgent.id).not.toBe(originalAgentId); + expect(String(newAgent.author)).toBe(userId.toString()); + expect(newActions).toHaveLength(1); + expect(newActions[0].metadata.domain).toBe('api.example.com'); + expect(newActions[0].agent_id).toBe(newAgent.id); + + for (const actionEntry of newAgent.actions) { + const [domain, actionId] = actionEntry.split(actionDelimiter); + expect(domain).toBe('api.example.com'); + expect(actionId).toBeTruthy(); + expect(actionId).not.toBe('act_original'); + } + + const allActions = await mongoose.models.Action.find({}).lean(); + expect(allActions).toHaveLength(2); + + const originalAction = allActions.find((a) => a.action_id === 'act_original'); + expect(originalAction.agent_id).toBe(originalAgentId); + + const duplicatedAction = allActions.find((a) => a.action_id !== 'act_original'); + expect(duplicatedAction.agent_id).toBe(newAgent.id); + expect(duplicatedAction.metadata.domain).toBe('api.example.com'); + }); + + it('strips sensitive metadata fields from duplicated actions', async () => { + const userId = new mongoose.Types.ObjectId(); + const originalAgentId = 'agent_sensitive'; + + await mongoose.models.Agent.create({ + id: originalAgentId, + name: 'Sensitive Agent', + author: userId.toString(), + provider: 'openai', + model: 'gpt-4', + tools: [], + actions: [`secure.api.com${actionDelimiter}act_secret`], + versions: [{ name: 'Sensitive Agent', createdAt: new Date(), updatedAt: new Date() }], + }); + + await mongoose.models.Action.create({ + user: userId, + action_id: 'act_secret', + agent_id: originalAgentId, + metadata: { + domain: 'secure.api.com', + api_key: 'sk-secret-key-12345', + oauth_client_id: 'client_id_xyz', + oauth_client_secret: 'client_secret_xyz', + }, + }); + + const req = { + params: { id: originalAgentId }, + user: { id: userId.toString() }, + }; + const res = { + status: jest.fn().mockReturnThis(), + json: jest.fn(), + }; + + await duplicateAgent(req, res); + + expect(res.status).toHaveBeenCalledWith(201); + + const duplicatedAction = await mongoose.models.Action.findOne({ + agent_id: { $ne: originalAgentId }, + }).lean(); + + expect(duplicatedAction.metadata.domain).toBe('secure.api.com'); + expect(duplicatedAction.metadata.api_key).toBeUndefined(); + expect(duplicatedAction.metadata.oauth_client_id).toBeUndefined(); + expect(duplicatedAction.metadata.oauth_client_secret).toBeUndefined(); + + const originalAction = await mongoose.models.Action.findOne({ + action_id: 'act_secret', + }).lean(); + expect(originalAction.metadata.api_key).toBe('sk-secret-key-12345'); + }); +}); diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index a2c0d55186..1abba8b2c8 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -371,7 +371,7 @@ const duplicateAgentHandler = async (req, res) => { */ const duplicateAction = async (action) => { const newActionId = nanoid(); - const [domain] = action.action_id.split(actionDelimiter); + const { domain } = action.metadata; const fullActionId = `${domain}${actionDelimiter}${newActionId}`; // Sanitize sensitive metadata before persisting @@ -381,7 +381,7 @@ const duplicateAgentHandler = async (req, res) => { } const newAction = await updateAction( - { action_id: newActionId }, + { action_id: newActionId, agent_id: newAgentId }, { metadata: filteredMetadata, agent_id: newAgentId, diff --git a/api/server/routes/agents/actions.js b/api/server/routes/agents/actions.js index 12168ba28a..4643f096aa 100644 --- a/api/server/routes/agents/actions.js +++ b/api/server/routes/agents/actions.js @@ -143,6 +143,9 @@ router.post( if (actions_result && actions_result.length) { const action = actions_result[0]; + if (action.agent_id !== agent_id) { + return res.status(403).json({ message: 'Action does not belong to this agent' }); + } metadata = { ...action.metadata, ...metadata }; } @@ -184,7 +187,7 @@ router.post( } /** @type {[Action]} */ - const updatedAction = await updateAction({ action_id }, actionUpdateData); + const updatedAction = await updateAction({ action_id, agent_id }, actionUpdateData); const sensitiveFields = ['api_key', 'oauth_client_id', 'oauth_client_secret']; for (let field of sensitiveFields) { @@ -251,7 +254,13 @@ router.delete( { tools: updatedTools, actions: updatedActions }, { updatingUserId: req.user.id, forceVersion: true }, ); - await deleteAction({ action_id }); + const deleted = await deleteAction({ action_id, agent_id }); + if (!deleted) { + logger.warn('[Agent Action Delete] No matching action document found', { + action_id, + agent_id, + }); + } res.status(200).json({ message: 'Action deleted successfully' }); } catch (error) { const message = 'Trouble deleting the Agent Action'; diff --git a/api/server/routes/assistants/actions.js b/api/server/routes/assistants/actions.js index 57975d32a7..b085fbd36a 100644 --- a/api/server/routes/assistants/actions.js +++ b/api/server/routes/assistants/actions.js @@ -60,6 +60,9 @@ router.post('/:assistant_id', async (req, res) => { if (actions_result && actions_result.length) { const action = actions_result[0]; + if (action.assistant_id !== assistant_id) { + return res.status(403).json({ message: 'Action does not belong to this assistant' }); + } metadata = { ...action.metadata, ...metadata }; } @@ -117,7 +120,7 @@ router.post('/:assistant_id', async (req, res) => { // For new actions, use the assistant owner's user ID actionUpdateData.user = assistant_user || req.user.id; } - promises.push(updateAction({ action_id }, actionUpdateData)); + promises.push(updateAction({ action_id, assistant_id }, actionUpdateData)); /** @type {[AssistantDocument, Action]} */ let [assistantDocument, updatedAction] = await Promise.all(promises); @@ -196,9 +199,15 @@ router.delete('/:assistant_id/:action_id/:model', async (req, res) => { assistantUpdateData.user = req.user.id; } promises.push(updateAssistantDoc({ assistant_id }, assistantUpdateData)); - promises.push(deleteAction({ action_id })); + promises.push(deleteAction({ action_id, assistant_id })); - await Promise.all(promises); + const [, deletedAction] = await Promise.all(promises); + if (!deletedAction) { + logger.warn('[Assistant Action Delete] No matching action document found', { + action_id, + assistant_id, + }); + } res.status(200).json({ message: 'Action deleted successfully' }); } catch (error) { const message = 'Trouble deleting the Assistant Action';