From 83184467043016d3e48b0d86c212d73b510eefa3 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 14 Mar 2026 21:22:25 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=92=81=20refactor:=20Better=20Config=20UX?= =?UTF-8?q?=20for=20MCP=20STDIO=20with=20`customUserVars`=20(#12226)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: Better UX for MCP stdio with Custom User Variables - Updated the ConnectionsRepository to prevent connections when customUserVars are defined, improving security and access control. - Modified the MCPServerInspector to skip capabilities fetch when customUserVars are present, streamlining server inspection. - Added tests to validate connection restrictions with customUserVars, ensuring robust handling of various server configurations. This change enhances the overall integrity of the connection management process by enforcing stricter rules around custom user variables. * fix: guard against empty customUserVars and add JSDoc context - Extract `hasCustomUserVars()` helper to guard against truthy `{}` (Zod's `.record().optional()` yields `{}` on empty input, not `undefined`) - Add JSDoc to `isAllowedToConnectToServer` explaining why customUserVars servers are excluded from app-level connections * test: improve customUserVars test coverage and fixture hygiene - Add no-connection-provided test for MCPServerInspector (production path) - Fix test descriptions to match actual fixture values - Replace real package name with fictional @test/mcp-stdio-server --- packages/api/src/mcp/ConnectionsRepository.ts | 18 +++++-- .../__tests__/ConnectionsRepository.test.ts | 44 +++++++++++++++++ .../src/mcp/registry/MCPServerInspector.ts | 7 ++- .../__tests__/MCPServerInspector.test.ts | 48 +++++++++++++++++++ packages/api/src/mcp/utils.ts | 5 ++ 5 files changed, 116 insertions(+), 6 deletions(-) diff --git a/packages/api/src/mcp/ConnectionsRepository.ts b/packages/api/src/mcp/ConnectionsRepository.ts index e629934dda..970e7ea4b9 100644 --- a/packages/api/src/mcp/ConnectionsRepository.ts +++ b/packages/api/src/mcp/ConnectionsRepository.ts @@ -1,8 +1,9 @@ import { logger } from '@librechat/data-schemas'; -import { MCPConnectionFactory } from '~/mcp/MCPConnectionFactory'; -import { MCPConnection } from './connection'; -import { MCPServersRegistry } from '~/mcp/registry/MCPServersRegistry'; import type * as t from './types'; +import { MCPServersRegistry } from '~/mcp/registry/MCPServersRegistry'; +import { MCPConnectionFactory } from '~/mcp/MCPConnectionFactory'; +import { hasCustomUserVars } from './utils'; +import { MCPConnection } from './connection'; const CONNECT_CONCURRENCY = 3; @@ -139,12 +140,19 @@ export class ConnectionsRepository { return `[MCP][${serverName}]`; } + /** + * App-level (shared) connections cannot serve servers that need per-user context: + * env/header placeholders like `{{MY_KEY}}` are only resolved by `processMCPEnv()` + * when real `customUserVars` values exist — which requires a user-level connection. + */ private isAllowedToConnectToServer(config: t.ParsedServerConfig) { if (config.inspectionFailed) { return false; } - //the repository is not allowed to be connected in case the Connection repository is shared (ownerId is undefined/null) and the server requires Auth or startup false. - if (this.ownerId === undefined && (config.startup === false || config.requiresOAuth)) { + if ( + this.ownerId === undefined && + (config.startup === false || config.requiresOAuth || hasCustomUserVars(config)) + ) { return false; } return true; diff --git a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts index 3b827774d0..98e15eca18 100644 --- a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts +++ b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts @@ -392,6 +392,36 @@ describe('ConnectionsRepository', () => { expect(await repository.has('oauthDisabledServer')).toBe(false); }); + it('should NOT allow connection to servers with customUserVars', async () => { + mockServerConfigs.customVarServer = { + type: 'stdio', + command: 'npx', + args: ['-y', '@test/mcp-stdio-server'], + env: { API_KEY: '{{MY_KEY}}' }, + customUserVars: { + MY_KEY: { title: 'API Key', description: 'Your API key' }, + }, + }; + + expect(await repository.has('customVarServer')).toBe(false); + }); + + it('should NOT allow connection when customUserVars is defined, even when startup is explicitly true', async () => { + mockServerConfigs.customVarStartupServer = { + type: 'stdio', + command: 'npx', + args: ['-y', '@test/mcp-stdio-server'], + env: { TOKEN: '{{USER_TOKEN}}' }, + startup: true, + requiresOAuth: false, + customUserVars: { + USER_TOKEN: { title: 'Token', description: 'Your token' }, + }, + }; + + expect(await repository.has('customVarStartupServer')).toBe(false); + }); + it('should disconnect existing connection when server becomes not allowed', async () => { // Initially setup as regular server mockServerConfigs.changingServer = { @@ -471,6 +501,20 @@ describe('ConnectionsRepository', () => { expect(await repository.has('oauthDisabledServer')).toBe(true); }); + it('should allow connection to servers with customUserVars', async () => { + mockServerConfigs.customVarServer = { + type: 'stdio', + command: 'npx', + args: ['-y', '@test/mcp-stdio-server'], + env: { API_KEY: '{{MY_KEY}}' }, + customUserVars: { + MY_KEY: { title: 'API Key', description: 'Your API key' }, + }, + }; + + expect(await repository.has('customVarServer')).toBe(true); + }); + it('should return null from get() when server config does not exist', async () => { const connection = await repository.get('nonexistent'); expect(connection).toBeNull(); diff --git a/packages/api/src/mcp/registry/MCPServerInspector.ts b/packages/api/src/mcp/registry/MCPServerInspector.ts index eea52bbf2e..a477d9b412 100644 --- a/packages/api/src/mcp/registry/MCPServerInspector.ts +++ b/packages/api/src/mcp/registry/MCPServerInspector.ts @@ -6,6 +6,7 @@ import { isMCPDomainAllowed, extractMCPServerDomain } from '~/auth/domain'; import { MCPConnectionFactory } from '~/mcp/MCPConnectionFactory'; import { MCPDomainNotAllowedError } from '~/mcp/errors'; import { detectOAuthRequirement } from '~/mcp/oauth'; +import { hasCustomUserVars } from '~/mcp/utils'; import { isEnabled } from '~/utils'; /** @@ -54,7 +55,11 @@ export class MCPServerInspector { private async inspectServer(): Promise { await this.detectOAuth(); - if (this.config.startup !== false && !this.config.requiresOAuth) { + if ( + this.config.startup !== false && + !this.config.requiresOAuth && + !hasCustomUserVars(this.config) + ) { let tempConnection = false; if (!this.connection) { tempConnection = true; diff --git a/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts index b79f2d044a..f0ab75c9b4 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts @@ -100,6 +100,54 @@ describe('MCPServerInspector', () => { }); }); + it('should skip capabilities fetch when customUserVars is defined', async () => { + const rawConfig: t.MCPOptions = { + type: 'stdio', + command: 'npx', + args: ['-y', '@test/mcp-stdio-server'], + env: { API_KEY: '{{MY_KEY}}' }, + customUserVars: { + MY_KEY: { title: 'API Key', description: 'Your API key' }, + }, + }; + + const result = await MCPServerInspector.inspect('test_server', rawConfig, mockConnection); + + expect(result).toEqual({ + type: 'stdio', + command: 'npx', + args: ['-y', '@test/mcp-stdio-server'], + env: { API_KEY: '{{MY_KEY}}' }, + customUserVars: { + MY_KEY: { title: 'API Key', description: 'Your API key' }, + }, + requiresOAuth: false, + initDuration: expect.any(Number), + }); + + expect(MCPConnectionFactory.create).not.toHaveBeenCalled(); + expect(mockConnection.disconnect).not.toHaveBeenCalled(); + }); + + it('should NOT create a temp connection when customUserVars is defined and no connection is provided', async () => { + const rawConfig: t.MCPOptions = { + type: 'stdio', + command: 'npx', + args: ['-y', '@test/mcp-stdio-server'], + env: { API_KEY: '{{MY_KEY}}' }, + customUserVars: { + MY_KEY: { title: 'API Key', description: 'Your API key' }, + }, + }; + + const result = await MCPServerInspector.inspect('test_server', rawConfig); + + expect(MCPConnectionFactory.create).not.toHaveBeenCalled(); + expect(result.requiresOAuth).toBe(false); + expect(result.capabilities).toBeUndefined(); + expect(result.toolFunctions).toBeUndefined(); + }); + it('should keep custom serverInstructions string and not fetch from server', async () => { const rawConfig: t.MCPOptions = { type: 'stdio', diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index c517388a76..ff367725fc 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -3,6 +3,11 @@ import type { ParsedServerConfig } from '~/mcp/types'; export const mcpToolPattern = new RegExp(`^.+${Constants.mcp_delimiter}.+$`); +/** Checks that `customUserVars` is present AND non-empty (guards against truthy `{}`) */ +export function hasCustomUserVars(config: Pick): boolean { + return !!config.customUserVars && Object.keys(config.customUserVars).length > 0; +} + /** * Allowlist-based sanitization for API responses. Only explicitly listed fields are included; * new fields added to ParsedServerConfig are excluded by default until allowlisted here.