diff --git a/packages/api/src/mcp/registry/MCPServersInitializer.ts b/packages/api/src/mcp/registry/MCPServersInitializer.ts index 7c19f6338f..b4595a74d4 100644 --- a/packages/api/src/mcp/registry/MCPServersInitializer.ts +++ b/packages/api/src/mcp/registry/MCPServersInitializer.ts @@ -10,6 +10,20 @@ import { isLeader } from '~/cluster'; const DEFAULT_MCP_INIT_TIMEOUT_MS = 30_000; const DEFAULT_FOLLOWER_RETRY_MS = 3000; +/** + * Bumped whenever the registry's persisted storage semantics change in a way the + * MCP config fingerprint cannot otherwise capture — e.g. how a server's `source` + * provenance is tagged. It is folded into the init fingerprint so an upgrade + * forces exactly one cluster-wide re-initialization even when the MCP config is + * unchanged. + * + * Without it, a rolling restart on a Redis-backed cluster leaves the persisted + * `INITIALIZED_CONFIG_HASH` matching the unchanged config, so replacement + * followers short-circuit on the stale status and never re-tag entries written + * by the previous version. Bumped to 2 for plugin-provenance preservation. + */ +const REGISTRY_STORAGE_SCHEMA_VERSION = 2; + const parseDurationMs = ( value: string | undefined, fallback: number, @@ -188,6 +202,7 @@ export class MCPServersInitializer { private static configHash(rawConfigs: t.MCPServers): string { const registry = MCPServersRegistry.getInstance(); const fingerprint = { + schemaVersion: REGISTRY_STORAGE_SCHEMA_VERSION, rawConfigs, allowedDomains: registry.getAllowedDomains() ?? null, allowedAddresses: registry.getAllowedAddresses() ?? null, diff --git a/packages/api/src/mcp/registry/MCPServersRegistry.ts b/packages/api/src/mcp/registry/MCPServersRegistry.ts index 99d3d0c41f..e91c0c8dcf 100644 --- a/packages/api/src/mcp/registry/MCPServersRegistry.ts +++ b/packages/api/src/mcp/registry/MCPServersRegistry.ts @@ -9,6 +9,7 @@ import { CONFIG_CACHE_NAMESPACE, } from './cache/ServerConfigsCacheFactory'; import { MCPInspectionFailedError, isMCPDomainNotAllowedError } from '~/mcp/errors'; +import { isPluginSourced, MCP_PLUGIN_SOURCE } from '~/utils/env'; import { MCPServerInspector } from './MCPServerInspector'; import { ServerConfigsDB } from './db/ServerConfigsDB'; import { cacheConfig } from '~/cache/cacheConfig'; @@ -17,6 +18,48 @@ import { withTimeout } from '~/utils'; /** How long a failure stub is considered fresh before re-attempting inspection (5 minutes). */ const CONFIG_STUB_RETRY_MS = 5 * 60 * 1000; +/** + * Provenance to persist for a config being stored in `tier`. + * + * SECURITY INVARIANT — an Agent Plugins server keeps its own `'plugin'` marker + * rather than taking the tier's tag. `processMCPEnv` reads that marker to decide + * whether a `${VAR}` the plugin authored stays literal, so retagging here would + * expand host secrets into a plugin-controlled header or URL at both inspection + * and connect time. Only operator-loaded tiers may carry the marker: a DB entry + * is user-authored and is always `'user'`, so user input can never claim plugin + * provenance and skip the sandboxed placeholder rules. + */ +function resolveServerSource( + config: t.ParsedServerConfig, + tier: t.MCPServerSource, +): t.MCPServerSource { + if (tier === 'user') { + return 'user'; + } + return isPluginSourced(config) ? MCP_PLUGIN_SOURCE : tier; +} + +/** + * Source an overlaid config should carry when a Config-tier override shadows a + * same-name base entry. The base's source is normally inherited so downstream + * recovery routes to the base's storage tier. + * + * SECURITY INVARIANT — a `'plugin'` base is the exception: its no-resolve + * provenance must never transfer to an operator-authored override, or + * `processMCPEnv` would stop resolving the operator's own `${VAR}` placeholders. + * The override supersedes the plugin (operator config outranks a plugin server), + * so it keeps its own trusted source instead. + */ +function overlaySource( + base: t.ParsedServerConfig, + override: t.ParsedServerConfig, +): t.MCPServerSource | undefined { + if (base.source === MCP_PLUGIN_SOURCE) { + return override.source ?? 'config'; + } + return base.source; +} + /** * Fields an admin override can legitimately set. Used to detect whether a * resolved entry differs from its YAML base so unmodified YAML servers can @@ -295,7 +338,7 @@ export class MCPServersRegistry { if (!candidate) return base; if (base?.source === 'user') return base; if (candidate.inspectionFailed) return base ?? candidate; - return base ? { ...candidate, source: base.source } : candidate; + return base ? { ...candidate, source: overlaySource(base, candidate) } : candidate; } /** Returns whether an effective config exactly matches the operator-owned base config. */ @@ -317,7 +360,9 @@ export class MCPServersRegistry { * base entry; the healthy base is preserved for the duration of the retry window. * 2. User-DB entries (`source: 'user'`) are never replaced by Config-tier overlays. * On a successful overlay the base entry's `source` field is preserved so downstream - * recovery logic routes to the correct storage location. + * recovery logic routes to the correct storage location — except a `'plugin'` base, + * whose no-resolve provenance must not transfer to the operator override (see + * `overlaySource`). */ public async getAllServerConfigs( userId?: string, @@ -335,8 +380,10 @@ export class MCPServersRegistry { continue; } if (override.inspectionFailed && result[name]) continue; - const baseSource = result[name]?.source; - result[name] = baseSource ? { ...override, source: baseSource } : override; + const baseEntry = result[name]; + result[name] = baseEntry + ? { ...override, source: overlaySource(baseEntry, override) } + : override; } return result; } @@ -401,7 +448,11 @@ export class MCPServersRegistry { userId?: string, ): Promise { const configRepo = this.getConfigRepository(storageLocation); - const stubConfig: t.ParsedServerConfig = { ...config, inspectionFailed: true, source: 'yaml' }; + const stubConfig: t.ParsedServerConfig = { + ...config, + inspectionFailed: true, + source: resolveServerSource(config, 'yaml'), + }; const result = await configRepo.add(serverName, stubConfig, userId); await this.invalidateServerReadCaches(result.serverName, userId); this.resetYamlServerNamesMemo(); @@ -416,7 +467,7 @@ export class MCPServersRegistry { reservedServerNames?: Iterable, ): Promise { const configRepo = this.getConfigRepository(storageLocation); - const source = (storageLocation === 'CACHE' ? 'yaml' : 'user') as t.MCPServerSource; + const source = resolveServerSource(config, storageLocation === 'CACHE' ? 'yaml' : 'user'); const configForInspection = { ...config, source } as t.ParsedServerConfig; const { allowedDomains, allowedAddresses } = await this.resolveAllowlists({ userId }); let parsedConfig: t.ParsedServerConfig; @@ -512,7 +563,7 @@ export class MCPServersRegistry { userId?: string, ): Promise { const configRepo = this.getConfigRepository(storageLocation); - const source = (storageLocation === 'CACHE' ? 'yaml' : 'user') as t.MCPServerSource; + const source = resolveServerSource(config, storageLocation === 'CACHE' ? 'yaml' : 'user'); // Merge existing admin API key if not provided in update (needed for inspection) let configForInspection = { ...config }; @@ -699,11 +750,10 @@ export class MCPServersRegistry { const prefix = `[MCP][config][${serverName}]`; logger.info(`${prefix} Lazy-initializing config-source server`); + const source = resolveServerSource(rawConfig, 'config'); + try { - const configForInspection = { - ...rawConfig, - source: 'config' as const, - } as t.ParsedServerConfig; + const configForInspection = { ...rawConfig, source } as t.ParsedServerConfig; const { allowedDomains, allowedAddresses } = allowlists; const inspected = await withTimeout( MCPServerInspector.inspect( @@ -717,7 +767,7 @@ export class MCPServersRegistry { `${prefix} Server initialization timed out`, ); - const parsedConfig: t.ParsedServerConfig = { ...inspected, source: 'config' }; + const parsedConfig: t.ParsedServerConfig = { ...inspected, source }; await this.upsertConfigCache(cacheKey, parsedConfig); logger.info( @@ -731,7 +781,7 @@ export class MCPServersRegistry { const stubConfig: t.ParsedServerConfig = { ...rawConfig, inspectionFailed: true, - source: 'config', + source, updatedAt: Date.now(), }; try { diff --git a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts index ef1ec81e75..7bee1e0f05 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts @@ -2,6 +2,7 @@ import { logger } from '@librechat/data-schemas'; import type * as t from '~/mcp/types'; import { MCPServersRegistry } from '~/mcp/registry/MCPServersRegistry'; import { MCPServerInspector } from '~/mcp/registry/MCPServerInspector'; +import { processMCPEnv } from '~/utils/env'; // Mock MCPServerInspector to avoid actual server connections jest.mock('~/mcp/registry/MCPServerInspector'); @@ -240,6 +241,134 @@ describe('MCPServersRegistry', () => { }); }); + /** + * Agent Plugins servers reach the registry through the same startup path as + * librechat.yaml servers. Deriving `source` from the storage tier alone used to + * retag them `'yaml'`, which dropped the marker `processMCPEnv` needs to keep + * plugin-authored placeholders literal and let a plugin exfiltrate `process.env` + * secrets through its own headers. + */ + describe('plugin provenance', () => { + const pluginConfig: t.ParsedServerConfig = { + source: 'plugin', + type: 'streamable-http', + url: 'https://plugin.example.com/mcp', + headers: { Authorization: 'Bearer ${TEST_PLUGIN_SECRET}' }, + }; + + it('keeps the plugin marker through inspection and cache storage', async () => { + const inspectSpy = jest.spyOn(MCPServerInspector, 'inspect'); + + const result = await registry.addServer('plugin_server', pluginConfig, 'CACHE'); + + expect(inspectSpy).toHaveBeenCalledWith( + 'plugin_server', + expect.objectContaining({ + source: 'plugin', + headers: { Authorization: 'Bearer ${TEST_PLUGIN_SECRET}' }, + }), + undefined, + undefined, + undefined, + ); + expect(result.config.source).toBe('plugin'); + await expect(registry['cacheConfigsRepo'].get('plugin_server')).resolves.toMatchObject({ + source: 'plugin', + headers: { Authorization: 'Bearer ${TEST_PLUGIN_SECRET}' }, + }); + }); + + it('still tags operator-authored cache servers as yaml', async () => { + const result = await registry.addServer('yaml_server', { ...testParsedConfig }, 'CACHE'); + + expect(result.config.source).toBe('yaml'); + }); + + it('keeps the plugin marker on a recovery stub when inspection fails', async () => { + const result = await registry.addServerStub('plugin_server', pluginConfig, 'CACHE'); + + expect(result.config).toMatchObject({ source: 'plugin', inspectionFailed: true }); + }); + + it('keeps the plugin marker through config-tier lazy init', async () => { + const result = await registry.ensureConfigServers({ plugin_server: pluginConfig }); + + expect(result.plugin_server.source).toBe('plugin'); + }); + + it('never lets a DB-stored config claim plugin provenance', async () => { + const inspectSpy = jest.spyOn(MCPServerInspector, 'inspect'); + + const result = await registry.addServer('forged_server', pluginConfig, 'DB', 'user-1'); + + expect(inspectSpy).toHaveBeenCalledWith( + 'forged_server', + expect.objectContaining({ source: 'user' }), + undefined, + undefined, + undefined, + ); + expect(result.config.source).toBe('user'); + }); + + it('leaves a plugin-authored header literal after a registry round trip', async () => { + process.env.TEST_PLUGIN_SECRET = 'host-secret-value'; + try { + await registry.addServer('plugin_server', pluginConfig, 'CACHE'); + const stored = await registry.getServerConfig('plugin_server'); + expect(stored).toBeDefined(); + + const runtimeConfig = processMCPEnv({ options: stored! }); + + expect(runtimeConfig).toMatchObject({ + headers: { Authorization: 'Bearer ${TEST_PLUGIN_SECRET}' }, + }); + } finally { + delete process.env.TEST_PLUGIN_SECRET; + } + }); + + /** + * An operator Config override that shadows a same-name plugin base must keep + * its own trusted `'config'` source. Inheriting the base's `'plugin'` marker + * would make `processMCPEnv` stop resolving the operator's own placeholders + * and silently break their server. + */ + it('does not lend plugin provenance to an operator config override of the same name', async () => { + const pluginBase: t.ParsedServerConfig = { + source: 'plugin', + type: 'streamable-http', + url: 'https://plugin.example.com/mcp', + requiresOAuth: false, + }; + await registry['cacheConfigsRepo'].add('shared', pluginBase); + + const override: t.ParsedServerConfig = { + source: 'config', + type: 'streamable-http', + url: 'https://operator.example.com/mcp', + headers: { Authorization: 'Bearer ${TEST_OPERATOR_SECRET}' }, + requiresOAuth: false, + }; + + const all = await registry.getAllServerConfigs('user-1', { shared: override }); + expect(all.shared.source).toBe('config'); + + const single = await registry.getServerConfig('shared', 'user-1', { shared: override }); + expect(single?.source).toBe('config'); + + process.env.TEST_OPERATOR_SECRET = 'operator-secret-value'; + try { + const runtimeConfig = processMCPEnv({ options: all.shared }); + expect(runtimeConfig).toMatchObject({ + headers: { Authorization: 'Bearer operator-secret-value' }, + }); + } finally { + delete process.env.TEST_OPERATOR_SECRET; + } + }); + }); + describe('resolveAllowlists (per-request, tenant-scoped)', () => { const createWith = ( allowedDomains?: string[] | null, diff --git a/packages/api/src/mcp/types/index.ts b/packages/api/src/mcp/types/index.ts index be8d93e755..f544984fe5 100644 --- a/packages/api/src/mcp/types/index.ts +++ b/packages/api/src/mcp/types/index.ts @@ -153,6 +153,11 @@ export type FormattedToolResponse = FormattedContentResult; * - `'yaml'` — operator-defined in librechat.yaml, full trust, boot-time init * - `'config'` — admin-defined via Config override, full trust, lazy init * - `'user'` — user-provided via UI, sandboxed (restricted placeholder resolution) + * - `'plugin'` — contributed by an Agent Plugins package, no placeholder resolution + * + * This tag is load-bearing, not descriptive: `processMCPEnv` reads it to decide + * which placeholders may resolve. Code that stores a config must carry the tag + * through rather than re-deriving it from the storage tier. */ export type MCPServerSource = 'yaml' | 'config' | 'user' | 'plugin';