🛡️ refactor: Scope Action Mutations by Parent Resource Ownership (#12237)

* 🛡️ fix: Scope action mutations by parent resource ownership

Prevent cross-tenant action overwrites by validating that an existing
action's agent_id/assistant_id matches the URL parameter before allowing
updates or deletes. Without this, a user with EDIT access on their own
agent could reference a foreign action_id to hijack another agent's
action record.

* 🛡️ fix: Harden action ownership checks and scope write filters

- Remove && short-circuit that bypassed the guard when agent_id or
  assistant_id was falsy (e.g. assistant-owned actions have no agent_id,
  so the check was skipped entirely on the agents route).
- Include agent_id / assistant_id in the updateAction and deleteAction
  query filters so the DB write itself enforces ownership atomically.
- Log a warning when deleteAction returns null (silent no-op from
  data-integrity mismatch).

* 📝 docs: Update Action model JSDoc to reflect scoped query params

*  test: Add Action ownership scoping tests

Cover update, delete, and cross-type protection scenarios using
MongoMemoryServer to verify that scoped query filters (agent_id,
assistant_id) prevent cross-tenant overwrites and deletions at the
database level.

* 🛡️ fix: Scope updateAction filter in agent duplication handler

* 🐛 fix: Use action metadata domain instead of action_id when duplicating agent actions

The duplicate handler was splitting `action.action_id` by `actionDelimiter`
to extract the domain, but `action_id` is a bare nanoid that doesn't
contain the delimiter. This produced malformed entries in the duplicated
agent's actions array (nanoid_action_newNanoid instead of
domain_action_newNanoid). The domain is available on `action.metadata.domain`.

*  test: Add integration tests for agent duplication action handling

Uses MongoMemoryServer with real Agent and Action models to verify:
- Duplicated actions use metadata.domain (not action_id) for the
  agent actions array entries
- Sensitive metadata fields are stripped from duplicated actions
- Original action documents are not modified
This commit is contained in:
Danny Avila 2026-03-15 10:19:29 -04:00 committed by GitHub
parent 7c39a45944
commit 0c27ad2d55
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 437 additions and 14 deletions

View file

@ -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<Action>} 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<Action>} 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<Action|null>} The deleted action document as a plain object, or null if no match.
*/
const deleteAction = async (searchParams) => {
return await Action.findOneAndDelete(searchParams).lean();

250
api/models/Action.spec.js Normal file
View file

@ -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);
});
});
});

View file

@ -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');
});
});

View file

@ -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,

View file

@ -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';

View file

@ -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';