mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-06-09 17:31:19 +00:00
🪬 fix: Skip MCP Tools When Required Custom User Vars Are Unset (#13152)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
* fix: skip MCP tools when required customUserVars are unset (#10969) * fix: whitespace-only values Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: guard MCP registry lookup and unknown server config in customUserVars gate * fix: fail closed on MCP registry lookup errors --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Danny Avila <danny@librechat.ai>
This commit is contained in:
parent
abda15f4eb
commit
1746153c17
6 changed files with 335 additions and 2 deletions
|
|
@ -20,6 +20,7 @@ const {
|
|||
buildImageToolContext,
|
||||
buildOAuthToolCallName,
|
||||
buildToolClassification,
|
||||
getMissingCustomUserVars,
|
||||
buildWebSearchDynamicContext,
|
||||
getCodeApiAuthHeaders,
|
||||
} = require('@librechat/api');
|
||||
|
|
@ -67,7 +68,7 @@ const { recordUsage } = require('~/server/services/Threads');
|
|||
const { loadTools } = require('~/app/clients/tools/util');
|
||||
const { redactMessage } = require('~/config/parsers');
|
||||
const { findPluginAuthsByKeys } = require('~/models');
|
||||
const { getFlowStateManager } = require('~/config');
|
||||
const { getFlowStateManager, getMCPServersRegistry } = require('~/config');
|
||||
const { getLogStores } = require('~/cache');
|
||||
|
||||
const domainSeparatorRegex = new RegExp(actionDomainSeparator, 'g');
|
||||
|
|
@ -638,6 +639,38 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to
|
|||
};
|
||||
|
||||
const getOrFetchMCPServerTools = async (userId, serverName) => {
|
||||
let serverConfig;
|
||||
try {
|
||||
serverConfig =
|
||||
configServers?.[serverName] ??
|
||||
(await getMCPServersRegistry().getServerConfig(serverName, userId, configServers));
|
||||
} catch (err) {
|
||||
logger.warn(
|
||||
`[Tool Definitions] MCP registry unavailable while resolving '${serverName}': ${
|
||||
err?.message ?? err
|
||||
}. Skipping MCP tool exposure for this lookup.`,
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
if (!serverConfig) {
|
||||
logger.warn(
|
||||
`[Tool Definitions] Skipping MCP server '${serverName}': no server config found (server may have been removed).`,
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
const customUserVars = userMCPAuthMap?.[`${Constants.mcp_prefix}${serverName}`];
|
||||
const missingUserVars = getMissingCustomUserVars(serverConfig, customUserVars);
|
||||
if (missingUserVars.length > 0) {
|
||||
logger.warn(
|
||||
`[Tool Definitions] Skipping MCP server '${serverName}': required user-provided variable(s) not set: ${missingUserVars.join(
|
||||
', ',
|
||||
)}. Tools will not be exposed until the user configures them.`,
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
const cached = await getMCPServerTools(userId, serverName);
|
||||
if (cached) {
|
||||
return cached;
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
const { logger } = require('@librechat/data-schemas');
|
||||
const { getMissingCustomUserVars } = require('@librechat/api');
|
||||
const { CacheKeys, Constants } = require('librechat-data-provider');
|
||||
const { getMCPManager, getMCPServersRegistry, getFlowStateManager } = require('~/config');
|
||||
const { findToken, createToken, updateToken, deleteTokens } = require('~/models');
|
||||
|
|
@ -86,6 +87,27 @@ async function reinitMCPServer({
|
|||
}
|
||||
|
||||
const customUserVars = userMCPAuthMap?.[`${Constants.mcp_prefix}${serverName}`];
|
||||
|
||||
const missingUserVars = getMissingCustomUserVars(serverConfig ?? {}, customUserVars);
|
||||
if (missingUserVars.length > 0) {
|
||||
logger.warn(
|
||||
`[MCP Reinitialize] Skipping server '${serverName}': required user-provided variable(s) not set: ${missingUserVars.join(
|
||||
', ',
|
||||
)}. Tools will not be exposed until the user configures them.`,
|
||||
);
|
||||
return {
|
||||
availableTools: null,
|
||||
success: false,
|
||||
message: `MCP server '${serverName}' requires user-provided variable(s) [${missingUserVars.join(
|
||||
', ',
|
||||
)}] which are not set`,
|
||||
oauthRequired: false,
|
||||
serverName,
|
||||
oauthUrl: null,
|
||||
tools: null,
|
||||
};
|
||||
}
|
||||
|
||||
const flowManager = _flowManager ?? getFlowStateManager(getLogStores(CacheKeys.FLOWS));
|
||||
const mcpManager = getMCPManager();
|
||||
const tokenMethods = { findToken, updateToken, createToken, deleteTokens };
|
||||
|
|
|
|||
105
api/server/services/Tools/mcp.spec.js
Normal file
105
api/server/services/Tools/mcp.spec.js
Normal file
|
|
@ -0,0 +1,105 @@
|
|||
const { Constants } = require('librechat-data-provider');
|
||||
|
||||
const mockGetConnection = jest.fn();
|
||||
|
||||
jest.mock('~/config', () => ({
|
||||
getMCPManager: jest.fn(() => ({ getConnection: mockGetConnection })),
|
||||
getMCPServersRegistry: jest.fn(() => ({ getServerConfig: jest.fn() })),
|
||||
getFlowStateManager: jest.fn(() => ({})),
|
||||
}));
|
||||
jest.mock('~/models', () => ({
|
||||
findToken: jest.fn(),
|
||||
createToken: jest.fn(),
|
||||
updateToken: jest.fn(),
|
||||
deleteTokens: jest.fn(),
|
||||
}));
|
||||
jest.mock('~/server/services/Config', () => ({
|
||||
updateMCPServerTools: jest.fn(),
|
||||
}));
|
||||
jest.mock('~/cache', () => ({
|
||||
getLogStores: jest.fn(() => ({})),
|
||||
}));
|
||||
|
||||
const { reinitMCPServer } = require('./mcp');
|
||||
|
||||
describe('reinitMCPServer — customUserVars gating (issue #10969)', () => {
|
||||
const user = { id: 'user-123' };
|
||||
const serverName = 'Thingy';
|
||||
const serverConfig = {
|
||||
type: 'streamable-http',
|
||||
url: 'https://thingy.example.com/mcp',
|
||||
customUserVars: {
|
||||
THINGY_TOKEN: { title: 'Thingy Access Token', description: 'Create this in Thingy' },
|
||||
},
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
});
|
||||
|
||||
it('does not connect and exposes no tools when a required customUserVar is unset', async () => {
|
||||
const result = await reinitMCPServer({
|
||||
user,
|
||||
serverName,
|
||||
serverConfig,
|
||||
userMCPAuthMap: undefined,
|
||||
});
|
||||
|
||||
expect(mockGetConnection).not.toHaveBeenCalled();
|
||||
expect(result).toMatchObject({
|
||||
availableTools: null,
|
||||
success: false,
|
||||
tools: null,
|
||||
oauthRequired: false,
|
||||
serverName,
|
||||
});
|
||||
expect(result.message).toContain('THINGY_TOKEN');
|
||||
});
|
||||
|
||||
it('does not connect when the stored value for a required customUserVar is empty', async () => {
|
||||
const result = await reinitMCPServer({
|
||||
user,
|
||||
serverName,
|
||||
serverConfig,
|
||||
userMCPAuthMap: { [`${Constants.mcp_prefix}${serverName}`]: { THINGY_TOKEN: '' } },
|
||||
});
|
||||
|
||||
expect(mockGetConnection).not.toHaveBeenCalled();
|
||||
expect(result.success).toBe(false);
|
||||
expect(result.availableTools).toBeNull();
|
||||
});
|
||||
|
||||
it('proceeds to connect once every required customUserVar is provided', async () => {
|
||||
mockGetConnection.mockResolvedValue({ fetchTools: jest.fn().mockResolvedValue([]) });
|
||||
|
||||
await reinitMCPServer({
|
||||
user,
|
||||
serverName,
|
||||
serverConfig,
|
||||
userMCPAuthMap: {
|
||||
[`${Constants.mcp_prefix}${serverName}`]: { THINGY_TOKEN: 'secret-token' },
|
||||
},
|
||||
});
|
||||
|
||||
expect(mockGetConnection).toHaveBeenCalledTimes(1);
|
||||
expect(mockGetConnection).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
serverName,
|
||||
customUserVars: { THINGY_TOKEN: 'secret-token' },
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('proceeds to connect when the server declares no customUserVars', async () => {
|
||||
mockGetConnection.mockResolvedValue({ fetchTools: jest.fn().mockResolvedValue([]) });
|
||||
|
||||
await reinitMCPServer({
|
||||
user,
|
||||
serverName,
|
||||
serverConfig: { type: 'streamable-http', url: 'https://thingy.example.com/mcp' },
|
||||
userMCPAuthMap: undefined,
|
||||
});
|
||||
|
||||
expect(mockGetConnection).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
|
@ -35,6 +35,8 @@ const mockDomainParser = jest.fn();
|
|||
const mockLegacyDomainEncode = jest.fn();
|
||||
const mockDecryptMetadata = jest.fn();
|
||||
const mockCreateActionTool = jest.fn();
|
||||
const mockGetServerConfig = jest.fn();
|
||||
const mockResolveConfigServers = jest.fn();
|
||||
jest.mock('~/server/services/Tools/credentials', () => ({
|
||||
loadAuthValues: jest.fn().mockResolvedValue({}),
|
||||
}));
|
||||
|
|
@ -69,9 +71,12 @@ jest.mock('~/models', () => ({
|
|||
}));
|
||||
jest.mock('~/config', () => ({
|
||||
getFlowStateManager: jest.fn(() => ({})),
|
||||
getMCPServersRegistry: jest.fn(() => ({
|
||||
getServerConfig: (...args) => mockGetServerConfig(...args),
|
||||
})),
|
||||
}));
|
||||
jest.mock('~/server/services/MCP', () => ({
|
||||
resolveConfigServers: jest.fn().mockResolvedValue({}),
|
||||
resolveConfigServers: (...args) => mockResolveConfigServers(...args),
|
||||
}));
|
||||
jest.mock('~/cache', () => ({
|
||||
getLogStores: jest.fn(() => ({})),
|
||||
|
|
@ -113,6 +118,11 @@ describe('ToolService - Action Capability Gating', () => {
|
|||
});
|
||||
mockLoadToolsUtil.mockResolvedValue({ loadedTools: [], toolContextMap: {} });
|
||||
mockLoadActionSets.mockResolvedValue([]);
|
||||
mockGetMCPServerTools.mockResolvedValue(null);
|
||||
mockGetCachedTools.mockResolvedValue(null);
|
||||
mockGetUserMCPAuthMap.mockResolvedValue({});
|
||||
mockGetServerConfig.mockResolvedValue(undefined);
|
||||
mockResolveConfigServers.mockResolvedValue({});
|
||||
});
|
||||
|
||||
describe('resolveAgentCapabilities', () => {
|
||||
|
|
@ -259,6 +269,92 @@ describe('ToolService - Action Capability Gating', () => {
|
|||
|
||||
expect(result.actionsEnabled).toBe(false);
|
||||
});
|
||||
|
||||
it('should not expose cached MCP tool definitions when the registry lookup fails', async () => {
|
||||
const serverName = 'private-server';
|
||||
const mcpTool = `search${Constants.mcp_delimiter}${serverName}`;
|
||||
const capabilities = [AgentCapabilities.tools];
|
||||
const req = createMockReq(capabilities);
|
||||
mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities));
|
||||
mockGetServerConfig.mockImplementation(() => {
|
||||
throw new Error('MCPServersRegistry has not been initialized.');
|
||||
});
|
||||
mockGetMCPServerTools.mockResolvedValue({
|
||||
[mcpTool]: {
|
||||
function: {
|
||||
name: mcpTool,
|
||||
description: 'Cached private search',
|
||||
parameters: {},
|
||||
},
|
||||
},
|
||||
});
|
||||
mockLoadToolDefinitions.mockImplementation(async (params, deps) => {
|
||||
const serverTools = await deps.getOrFetchMCPServerTools(params.userId, serverName);
|
||||
return {
|
||||
toolDefinitions: serverTools ? Object.keys(serverTools) : [],
|
||||
toolRegistry: new Map(),
|
||||
hasDeferredTools: false,
|
||||
};
|
||||
});
|
||||
|
||||
const result = await loadAgentTools({
|
||||
req,
|
||||
res: {},
|
||||
agent: { id: 'agent_123', tools: [mcpTool] },
|
||||
definitionsOnly: true,
|
||||
});
|
||||
|
||||
expect(result.toolDefinitions).toEqual([]);
|
||||
expect(mockGetMCPServerTools).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should use request-scoped MCP config before falling back to the registry', async () => {
|
||||
const serverName = 'config-server';
|
||||
const mcpTool = `search${Constants.mcp_delimiter}${serverName}`;
|
||||
const capabilities = [AgentCapabilities.tools];
|
||||
const req = createMockReq(capabilities);
|
||||
mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities));
|
||||
mockResolveConfigServers.mockResolvedValue({
|
||||
[serverName]: {
|
||||
type: 'streamable-http',
|
||||
url: 'https://config.example.com/mcp',
|
||||
customUserVars: {
|
||||
TOKEN: { title: 'Token', description: 'Token' },
|
||||
},
|
||||
},
|
||||
});
|
||||
mockGetUserMCPAuthMap.mockResolvedValue({
|
||||
[`${Constants.mcp_prefix}${serverName}`]: { TOKEN: 'secret' },
|
||||
});
|
||||
mockGetMCPServerTools.mockResolvedValue({
|
||||
[mcpTool]: {
|
||||
function: {
|
||||
name: mcpTool,
|
||||
description: 'Config search',
|
||||
parameters: {},
|
||||
},
|
||||
},
|
||||
});
|
||||
mockLoadToolDefinitions.mockImplementation(async (params, deps) => {
|
||||
const serverTools = await deps.getOrFetchMCPServerTools(params.userId, serverName);
|
||||
return {
|
||||
toolDefinitions: serverTools ? Object.keys(serverTools) : [],
|
||||
toolRegistry: new Map(),
|
||||
hasDeferredTools: false,
|
||||
};
|
||||
});
|
||||
|
||||
const result = await loadAgentTools({
|
||||
req,
|
||||
res: {},
|
||||
agent: { id: 'agent_123', tools: [mcpTool] },
|
||||
definitionsOnly: true,
|
||||
});
|
||||
|
||||
expect(result.toolDefinitions).toEqual([mcpTool]);
|
||||
expect(mockGetServerConfig).not.toHaveBeenCalled();
|
||||
expect(mockGetMCPServerTools).toHaveBeenCalledWith(req.user.id, serverName);
|
||||
});
|
||||
});
|
||||
|
||||
describe('loadAgentTools (definitionsOnly=false) — action tool filtering', () => {
|
||||
|
|
|
|||
|
|
@ -5,6 +5,8 @@ import {
|
|||
redactServerSecrets,
|
||||
isInvalidClientMessage,
|
||||
isClientRejectionMessage,
|
||||
getMissingCustomUserVars,
|
||||
hasCustomUserVars,
|
||||
isUserSourced,
|
||||
} from '~/mcp/utils';
|
||||
import type { ParsedServerConfig } from '~/mcp/types';
|
||||
|
|
@ -340,3 +342,55 @@ describe('isUserSourced', () => {
|
|||
expect(isUserSourced({})).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getMissingCustomUserVars', () => {
|
||||
const configWithVars = (keys: string[]): Pick<ParsedServerConfig, 'customUserVars'> => ({
|
||||
customUserVars: Object.fromEntries(
|
||||
keys.map((key) => [key, { title: key, description: `${key} description` }]),
|
||||
),
|
||||
});
|
||||
|
||||
it('returns an empty array when the server declares no customUserVars', () => {
|
||||
expect(getMissingCustomUserVars({}, {})).toEqual([]);
|
||||
expect(getMissingCustomUserVars({ customUserVars: undefined }, undefined)).toEqual([]);
|
||||
});
|
||||
|
||||
it('returns an empty array when customUserVars is an empty object', () => {
|
||||
const config: Pick<ParsedServerConfig, 'customUserVars'> = { customUserVars: {} };
|
||||
expect(hasCustomUserVars(config)).toBe(false);
|
||||
expect(getMissingCustomUserVars(config, undefined)).toEqual([]);
|
||||
});
|
||||
|
||||
it('reports every declared variable when no values are provided', () => {
|
||||
const config = configWithVars(['THINGY_TOKEN', 'THINGY_REGION']);
|
||||
expect(getMissingCustomUserVars(config, undefined)).toEqual(['THINGY_TOKEN', 'THINGY_REGION']);
|
||||
expect(getMissingCustomUserVars(config, null)).toEqual(['THINGY_TOKEN', 'THINGY_REGION']);
|
||||
expect(getMissingCustomUserVars(config, {})).toEqual(['THINGY_TOKEN', 'THINGY_REGION']);
|
||||
});
|
||||
|
||||
it('reports only the variables the user has not set', () => {
|
||||
const config = configWithVars(['THINGY_TOKEN', 'THINGY_REGION']);
|
||||
expect(getMissingCustomUserVars(config, { THINGY_TOKEN: 'abc123' })).toEqual(['THINGY_REGION']);
|
||||
});
|
||||
|
||||
it('treats empty-string and whitespace-only values as missing', () => {
|
||||
const config = configWithVars(['THINGY_TOKEN']);
|
||||
expect(getMissingCustomUserVars(config, { THINGY_TOKEN: '' })).toEqual(['THINGY_TOKEN']);
|
||||
expect(getMissingCustomUserVars(config, { THINGY_TOKEN: ' ' })).toEqual(['THINGY_TOKEN']);
|
||||
expect(getMissingCustomUserVars(config, { THINGY_TOKEN: '\t\n ' })).toEqual(['THINGY_TOKEN']);
|
||||
});
|
||||
|
||||
it('returns an empty array when every declared variable has a value', () => {
|
||||
const config = configWithVars(['THINGY_TOKEN', 'THINGY_REGION']);
|
||||
expect(
|
||||
getMissingCustomUserVars(config, { THINGY_TOKEN: 'abc123', THINGY_REGION: 'eu-west-1' }),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('ignores provided values for variables the server does not declare', () => {
|
||||
const config = configWithVars(['THINGY_TOKEN']);
|
||||
expect(
|
||||
getMissingCustomUserVars(config, { THINGY_TOKEN: 'abc123', UNRELATED: 'value' }),
|
||||
).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -18,6 +18,29 @@ export function hasCustomUserVars(config: Pick<ParsedServerConfig, 'customUserVa
|
|||
return !!config.customUserVars && Object.keys(config.customUserVars).length > 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns the names of `customUserVars` declared on the server config for which
|
||||
* the user has not supplied a non-blank value (unset, empty, or whitespace-only
|
||||
* values count as missing, since they still fail auth). An empty array means
|
||||
* every declared variable is satisfied (or the server declares none).
|
||||
*
|
||||
* Used to gate tool exposure: a server that requires user-provided credentials
|
||||
* should not surface its tools to the model until those values are set,
|
||||
* otherwise every tool call fails authentication. See issue #10969.
|
||||
*/
|
||||
export function getMissingCustomUserVars(
|
||||
config: Pick<ParsedServerConfig, 'customUserVars'>,
|
||||
providedVars?: Record<string, string> | null,
|
||||
): string[] {
|
||||
if (!hasCustomUserVars(config)) {
|
||||
return [];
|
||||
}
|
||||
return Object.keys(config.customUserVars ?? {}).filter((key) => {
|
||||
const value = providedVars?.[key];
|
||||
return value == null || (typeof value === 'string' && value.trim() === '');
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Determines whether a server config is user-sourced (sandboxed placeholder resolution).
|
||||
* When `source` is set, it is authoritative. When absent (pre-upgrade cached configs),
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue