diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index 1c1e2cf4b9..83e47e445b 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -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; diff --git a/api/server/services/Tools/mcp.js b/api/server/services/Tools/mcp.js index f1ebcf9796..4b654fa11a 100644 --- a/api/server/services/Tools/mcp.js +++ b/api/server/services/Tools/mcp.js @@ -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 }; diff --git a/api/server/services/Tools/mcp.spec.js b/api/server/services/Tools/mcp.spec.js new file mode 100644 index 0000000000..bce51aa29a --- /dev/null +++ b/api/server/services/Tools/mcp.spec.js @@ -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); + }); +}); diff --git a/api/server/services/__tests__/ToolService.spec.js b/api/server/services/__tests__/ToolService.spec.js index be066d6792..ab95b389ca 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -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', () => { diff --git a/packages/api/src/mcp/__tests__/utils.test.ts b/packages/api/src/mcp/__tests__/utils.test.ts index 19f9c1eaf7..ed626cdb45 100644 --- a/packages/api/src/mcp/__tests__/utils.test.ts +++ b/packages/api/src/mcp/__tests__/utils.test.ts @@ -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 => ({ + 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 = { 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([]); + }); +}); diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index 872f43002c..a92755e1a1 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -18,6 +18,29 @@ export function hasCustomUserVars(config: Pick 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, + providedVars?: Record | 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),