mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-31 04:47:19 +02:00
🔏 fix: Enforce MCP Server Authorization on Agent Tool Persistence (#12250)
* 🛡️ fix: Validate MCP tool authorization on agent create/update Agent creation and update accepted arbitrary MCP tool strings without verifying the user has access to the referenced MCP servers. This allowed a user to embed unauthorized server names in tool identifiers (e.g. "anything_mcp_<victimServer>"), causing mcpServerNames to be stored on the agent and granting consumeOnly access via hasAccessViaAgent(). Adds filterAuthorizedTools() that checks MCP tool strings against the user's accessible server configs (via getAllServerConfigs) before persisting. Applied to create, update, and duplicate agent paths. * 🛡️ fix: Harden MCP tool authorization and add test coverage Addresses review findings on the MCP agent tool authorization fix: - Wrap getMCPServersRegistry() in try/catch so uninitialized registry gracefully filters all MCP tools instead of causing a 500 (DoS risk) - Guard revertAgentVersionHandler: filter unauthorized MCP tools after reverting to a previous version snapshot - Preserve existing MCP tools on collaborative updates: only validate newly added tools, preventing silent stripping of tools the editing user lacks direct access to - Add audit logging (logger.warn) when MCP tools are rejected - Refactor to single-pass lazy-fetch (registry queried only on first MCP tool encountered) - Export filterAuthorizedTools for direct unit testing - Add 18 tests covering: authorized/unauthorized/mixed tools, registry unavailable fallback, create/update/duplicate/revert handler paths, collaborative update preservation, and mcpServerNames persistence * test: Add duplicate handler test, use Constants.mcp_delimiter, DB assertions - N1: Add duplicateAgentHandler integration test verifying unauthorized MCP tools are stripped from the cloned agent and mcpServerNames are correctly persisted in the database - N2: Replace all hardcoded '_mcp_' delimiter literals with Constants.mcp_delimiter to prevent silent false-positive tests if the delimiter value ever changes - N3: Add DB state assertion to the revert-with-strip test confirming persisted tools match the response after unauthorized tools are removed * fix: Enforce exact 2-segment format for MCP tool keys Reject MCP tool keys with multiple delimiters to prevent authorization/execution mismatch when `.pop()` vs `split[1]` extract different server names from the same key. * fix: Preserve existing MCP tools when registry is unavailable When the MCP registry is uninitialized (e.g. server restart), existing tools already persisted on the agent are preserved instead of silently stripped. New MCP tools are still rejected when the registry cannot verify them. Applies to duplicate and revert handlers via existingTools param; update handler already preserves existing tools via its diff logic.
This commit is contained in:
Danny Avila
GitHub
parent
aee1ced817
commit
a26eeea592
2 changed files with 801 additions and 10 deletions
|
|
@ -49,6 +49,7 @@ const { refreshS3Url } = require('~/server/services/Files/S3/crud');
|
|||
const { filterFile } = require('~/server/services/Files/process');
|
||||
const { updateAction, getActions } = require('~/models/Action');
|
||||
const { getCachedTools } = require('~/server/services/Config');
|
||||
const { getMCPServersRegistry } = require('~/config');
|
||||
const { getLogStores } = require('~/cache');
|
||||
|
||||
const systemTools = {
|
||||
|
|
@ -98,6 +99,78 @@ const validateEdgeAgentAccess = async (edges, userId, userRole) => {
|
|||
.map((a) => a.id);
|
||||
};
|
||||
|
||||
/**
|
||||
* Filters tools to only include those the user is authorized to use.
|
||||
* MCP tools must match the exact format `{toolName}_mcp_{serverName}` (exactly 2 segments).
|
||||
* Multi-delimiter keys are rejected to prevent authorization/execution mismatch.
|
||||
* Non-MCP tools must appear in availableTools (global tool cache) or systemTools.
|
||||
*
|
||||
* When `existingTools` is provided and the MCP registry is unavailable (e.g. server restart),
|
||||
* tools already present on the agent are preserved rather than stripped — they were validated
|
||||
* when originally added, and we cannot re-verify them without the registry.
|
||||
* @param {object} params
|
||||
* @param {string[]} params.tools - Raw tool strings from the request
|
||||
* @param {string} params.userId - Requesting user ID for MCP server access check
|
||||
* @param {Record<string, unknown>} params.availableTools - Global non-MCP tool cache
|
||||
* @param {string[]} [params.existingTools] - Tools already persisted on the agent document
|
||||
* @returns {Promise<string[]>} Only the authorized subset of tools
|
||||
*/
|
||||
const filterAuthorizedTools = async ({ tools, userId, availableTools, existingTools }) => {
|
||||
const filteredTools = [];
|
||||
let mcpServerConfigs;
|
||||
let registryUnavailable = false;
|
||||
const existingToolSet = existingTools?.length ? new Set(existingTools) : null;
|
||||
|
||||
for (const tool of tools) {
|
||||
if (availableTools[tool] || systemTools[tool]) {
|
||||
filteredTools.push(tool);
|
||||
continue;
|
||||
}
|
||||
|
||||
if (!tool?.includes(Constants.mcp_delimiter)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
if (mcpServerConfigs === undefined) {
|
||||
try {
|
||||
mcpServerConfigs = (await getMCPServersRegistry().getAllServerConfigs(userId)) ?? {};
|
||||
} catch (e) {
|
||||
logger.warn(
|
||||
'[filterAuthorizedTools] MCP registry unavailable, filtering all MCP tools',
|
||||
e.message,
|
||||
);
|
||||
mcpServerConfigs = {};
|
||||
registryUnavailable = true;
|
||||
}
|
||||
}
|
||||
|
||||
const parts = tool.split(Constants.mcp_delimiter);
|
||||
if (parts.length !== 2) {
|
||||
logger.warn(
|
||||
`[filterAuthorizedTools] Rejected malformed MCP tool key "${tool}" for user ${userId}`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
|
||||
if (registryUnavailable && existingToolSet?.has(tool)) {
|
||||
filteredTools.push(tool);
|
||||
continue;
|
||||
}
|
||||
|
||||
const [, serverName] = parts;
|
||||
if (!serverName || !Object.hasOwn(mcpServerConfigs, serverName)) {
|
||||
logger.warn(
|
||||
`[filterAuthorizedTools] Rejected MCP tool "${tool}" — server "${serverName}" not accessible to user ${userId}`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
|
||||
filteredTools.push(tool);
|
||||
}
|
||||
|
||||
return filteredTools;
|
||||
};
|
||||
|
||||
/**
|
||||
* Creates an Agent.
|
||||
* @route POST /Agents
|
||||
|
|
@ -132,15 +205,7 @@ const createAgentHandler = async (req, res) => {
|
|||
agentData.tools = [];
|
||||
|
||||
const availableTools = (await getCachedTools()) ?? {};
|
||||
for (const tool of tools) {
|
||||
if (availableTools[tool]) {
|
||||
agentData.tools.push(tool);
|
||||
} else if (systemTools[tool]) {
|
||||
agentData.tools.push(tool);
|
||||
} else if (tool.includes(Constants.mcp_delimiter)) {
|
||||
agentData.tools.push(tool);
|
||||
}
|
||||
}
|
||||
agentData.tools = await filterAuthorizedTools({ tools, userId, availableTools });
|
||||
|
||||
const agent = await createAgent(agentData);
|
||||
|
||||
|
|
@ -322,6 +387,26 @@ const updateAgentHandler = async (req, res) => {
|
|||
updateData.tools = ocrConversion.tools;
|
||||
}
|
||||
|
||||
if (updateData.tools) {
|
||||
const existingToolSet = new Set(existingAgent.tools ?? []);
|
||||
const newMCPTools = updateData.tools.filter(
|
||||
(t) => !existingToolSet.has(t) && t?.includes(Constants.mcp_delimiter),
|
||||
);
|
||||
|
||||
if (newMCPTools.length > 0) {
|
||||
const availableTools = (await getCachedTools()) ?? {};
|
||||
const approvedNew = await filterAuthorizedTools({
|
||||
tools: newMCPTools,
|
||||
userId: req.user.id,
|
||||
availableTools,
|
||||
});
|
||||
const rejectedSet = new Set(newMCPTools.filter((t) => !approvedNew.includes(t)));
|
||||
if (rejectedSet.size > 0) {
|
||||
updateData.tools = updateData.tools.filter((t) => !rejectedSet.has(t));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let updatedAgent =
|
||||
Object.keys(updateData).length > 0
|
||||
? await updateAgent({ id }, updateData, {
|
||||
|
|
@ -464,6 +549,17 @@ const duplicateAgentHandler = async (req, res) => {
|
|||
|
||||
const agentActions = await Promise.all(promises);
|
||||
newAgentData.actions = agentActions;
|
||||
|
||||
if (newAgentData.tools?.length) {
|
||||
const availableTools = (await getCachedTools()) ?? {};
|
||||
newAgentData.tools = await filterAuthorizedTools({
|
||||
tools: newAgentData.tools,
|
||||
userId,
|
||||
availableTools,
|
||||
existingTools: newAgentData.tools,
|
||||
});
|
||||
}
|
||||
|
||||
const newAgent = await createAgent(newAgentData);
|
||||
|
||||
try {
|
||||
|
|
@ -792,7 +888,24 @@ const revertAgentVersionHandler = async (req, res) => {
|
|||
|
||||
// Permissions are enforced via route middleware (ACL EDIT)
|
||||
|
||||
const updatedAgent = await revertAgentVersion({ id }, version_index);
|
||||
let updatedAgent = await revertAgentVersion({ id }, version_index);
|
||||
|
||||
if (updatedAgent.tools?.length) {
|
||||
const availableTools = (await getCachedTools()) ?? {};
|
||||
const filteredTools = await filterAuthorizedTools({
|
||||
tools: updatedAgent.tools,
|
||||
userId: req.user.id,
|
||||
availableTools,
|
||||
existingTools: updatedAgent.tools,
|
||||
});
|
||||
if (filteredTools.length !== updatedAgent.tools.length) {
|
||||
updatedAgent = await updateAgent(
|
||||
{ id },
|
||||
{ tools: filteredTools },
|
||||
{ updatingUserId: req.user.id },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
if (updatedAgent.author) {
|
||||
updatedAgent.author = updatedAgent.author.toString();
|
||||
|
|
@ -860,4 +973,5 @@ module.exports = {
|
|||
uploadAgentAvatar: uploadAgentAvatarHandler,
|
||||
revertAgentVersion: revertAgentVersionHandler,
|
||||
getAgentCategories,
|
||||
filterAuthorizedTools,
|
||||
};
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue