🔐 fix: Preserve Plugin MCP Provenance Through Registry Storage (#14744)

* 🔐 fix: Preserve Plugin MCP Provenance Through Registry Storage

Agent Plugins MCP servers are tagged `source: 'plugin'` by the plugin
loader so `processMCPEnv` returns them verbatim, keeping any `${VAR}` a
plugin declared literal. The registry derived `source` from the storage
tier alone, so every startup server routed through
`addServer(..., 'CACHE')` was retagged `'yaml'` — before inspection and
again before persistence.

That dropped the marker for deployment plugin servers merged into the
startup MCP config, so `processMCPEnv` treated plugin-authored strings as
operator-authored templates and expanded them from `process.env`. A
malicious plugin declaring `Authorization: Bearer ${OPENAI_API_KEY}`
received the host's key at its own endpoint, at both boot-time inspection
and every runtime connection.

`resolveServerSource` now carries an existing plugin marker through
instead of re-deriving it, and is applied at all four tag sites
(`addServer`, `addServerStub`, `inspectServerUpdate`, and config-tier
lazy init, which hardcoded `'config'` and would have re-opened the same
hole). The marker is only honored for operator-loaded tiers: a DB entry
is user-authored and stays `'user'`, so user input cannot claim plugin
provenance to escape the sandboxed placeholder rules.

* 🔒 fix: Address Codex review — config-override provenance & upgrade re-tag

Two follow-ups from the Codex review of the provenance fix, plus a test
cleanup.

P2 — a Config-tier override that shadows a same-name plugin base inherited
the base's `source: 'plugin'` through the merge in `getServerConfig` /
`getAllServerConfigs`, so `processMCPEnv` stopped resolving the operator's
own `${VAR}` placeholders and silently broke their server. New
`overlaySource` helper keeps an operator override on its own trusted
source when the base is plugin-sourced; all other bases still inherit as
before. Fails safe (never a leak), but the regression is real.

P1 — the init fingerprint hashes only the raw MCP config, which already
carried `source: 'plugin'` before the provenance fix, so the hash is
unchanged by it. On a Redis-backed rolling restart with no config change,
followers short-circuit on the stale `INITIALIZED_CONFIG_HASH` and the
old `source: 'yaml'` plugin entries survive with no expiry — the fix
never takes effect. Fold a `REGISTRY_STORAGE_SCHEMA_VERSION` into the
fingerprint so an upgrade forces exactly one cluster-wide re-init.

Also drop unnecessary `as` casts in the provenance tests (declare the
fixture as `ParsedServerConfig`, assert with `toMatchObject`) and add a
regression test for the P2 override case.

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,

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<t.AddServerResult> {
     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<string>,
   ): Promise<t.AddServerResult> {
     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<t.ParsedServerConfig> {
     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 {

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,

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';