From 73d407fe734338973499375d272c0fec6bc804e0 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Tue, 7 Apr 2026 15:11:47 +0900 Subject: [PATCH] fix(mcp): bypass env var allowlist for trusted skill MCP configs (#3168) Added 'trusted' option to expandEnvVars. Skill MCPs are user-controlled and now bypass the security allowlist. 3 files changed. TDD verified. tsc clean. Closes #3168 --- .../env-expander.test.ts | 91 ++++++++ .../claude-code-mcp-loader/env-expander.ts | 17 +- .../connection-env-vars.test.ts | 209 ++++++++++++++++++ src/features/skill-mcp-manager/connection.ts | 2 +- 4 files changed, 312 insertions(+), 7 deletions(-) create mode 100644 src/features/skill-mcp-manager/connection-env-vars.test.ts diff --git a/src/features/claude-code-mcp-loader/env-expander.test.ts b/src/features/claude-code-mcp-loader/env-expander.test.ts index ae93e01dc..1aa9c4419 100644 --- a/src/features/claude-code-mcp-loader/env-expander.test.ts +++ b/src/features/claude-code-mcp-loader/env-expander.test.ts @@ -120,6 +120,32 @@ describe("expandEnvVars", () => { expect(expanded).toBe("user-approved") }) }) + + describe("#given a sensitive environment variable expanded in trusted mode", () => { + it("#when expanding the value #then it returns the env value bypassing the allowlist", () => { + // given + process.env.SLACK_USER_TOKEN = "xoxp-trusted" + + // when + const expanded = expandEnvVars("${SLACK_USER_TOKEN}", { trusted: true }) + + // then + expect(expanded).toBe("xoxp-trusted") + }) + }) + + describe("#given an unset env var expanded in trusted mode with a default", () => { + it("#when expanding the value #then it returns the default value", () => { + // given + delete process.env.UNSET_TRUSTED_VAR + + // when + const expanded = expandEnvVars("${UNSET_TRUSTED_VAR:-fallback}", { trusted: true }) + + // then + expect(expanded).toBe("fallback") + }) + }) }) describe("expandEnvVarsInObject", () => { @@ -165,4 +191,69 @@ describe("expandEnvVarsInObject", () => { }) }) }) + + describe("#given a trusted skill MCP config object with sensitive env vars", () => { + it("#when expanding env vars in trusted mode #then it expands all referenced env vars", () => { + // given + process.env.SLACK_USER_TOKEN = "xoxp-trusted-token" + process.env.HOME = "/Users/tester" + + // when + const expanded = expandEnvVarsInObject( + { + command: "npx", + args: [ + "-y", + "mcp-remote", + "https://mcp.slack.com/mcp", + "--header", + "Authorization:Bearer ${SLACK_USER_TOKEN}", + ], + env: { + HOME_DIR: "${HOME}", + }, + }, + { trusted: true } + ) + + // then + expect(expanded).toEqual({ + command: "npx", + args: [ + "-y", + "mcp-remote", + "https://mcp.slack.com/mcp", + "--header", + "Authorization:Bearer xoxp-trusted-token", + ], + env: { + HOME_DIR: "/Users/tester", + }, + }) + }) + + it("#when expanding a remote http skill MCP config in trusted mode #then it expands sensitive headers", () => { + // given + process.env.SLACK_USER_TOKEN = "xoxp-trusted-token" + + // when + const expanded = expandEnvVarsInObject( + { + url: "https://mcp.slack.com/mcp", + headers: { + Authorization: "Bearer ${SLACK_USER_TOKEN}", + }, + }, + { trusted: true } + ) + + // then + expect(expanded).toEqual({ + url: "https://mcp.slack.com/mcp", + headers: { + Authorization: "Bearer xoxp-trusted-token", + }, + }) + }) + }) }) diff --git a/src/features/claude-code-mcp-loader/env-expander.ts b/src/features/claude-code-mcp-loader/env-expander.ts index 254d7a6a2..ad3264f96 100644 --- a/src/features/claude-code-mcp-loader/env-expander.ts +++ b/src/features/claude-code-mcp-loader/env-expander.ts @@ -4,11 +4,16 @@ import { isSensitiveMcpEnvVar, } from "./configure-allowed-env-vars" -export function expandEnvVars(value: string): string { +export interface ExpandEnvVarsOptions { + trusted?: boolean +} + +export function expandEnvVars(value: string, options: ExpandEnvVarsOptions = {}): string { + const { trusted = false } = options return value.replace( /\$\{([^}:]+)(?::-([^}]*))?\}/g, (_, varName: string, defaultValue?: string) => { - if (!isAllowedMcpEnvVar(varName)) { + if (!trusted && !isAllowedMcpEnvVar(varName)) { const isSensitive = isSensitiveMcpEnvVar(varName) const reason = isSensitive ? "sensitive variable" : "not in allowlist" @@ -29,16 +34,16 @@ export function expandEnvVars(value: string): string { ) } -export function expandEnvVarsInObject(obj: T): T { +export function expandEnvVarsInObject(obj: T, options: ExpandEnvVarsOptions = {}): T { if (obj === null || obj === undefined) return obj - if (typeof obj === "string") return expandEnvVars(obj) as T + if (typeof obj === "string") return expandEnvVars(obj, options) as T if (Array.isArray(obj)) { - return obj.map((item) => expandEnvVarsInObject(item)) as T + return obj.map((item) => expandEnvVarsInObject(item, options)) as T } if (typeof obj === "object") { const result: Record = {} for (const [key, value] of Object.entries(obj)) { - result[key] = expandEnvVarsInObject(value) + result[key] = expandEnvVarsInObject(value, options) } return result as T } diff --git a/src/features/skill-mcp-manager/connection-env-vars.test.ts b/src/features/skill-mcp-manager/connection-env-vars.test.ts new file mode 100644 index 000000000..157904a00 --- /dev/null +++ b/src/features/skill-mcp-manager/connection-env-vars.test.ts @@ -0,0 +1,209 @@ +import { afterAll, afterEach, beforeEach, describe, expect, it, mock } from "bun:test" +import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types" +import type { SkillMcpClientInfo, SkillMcpManagerState } from "./types" + +const trackedStates: SkillMcpManagerState[] = [] +const createdStdioTransports: MockStdioClientTransport[] = [] +const createdHttpTransports: MockStreamableHTTPClientTransport[] = [] + +class MockClient { + readonly close = mock(async () => {}) + + constructor( + _clientInfo: { name: string; version: string }, + _options: { capabilities: Record } + ) {} + + async connect(_transport: unknown): Promise { + // Successful connect, env-related assertions happen on transport constructor args + } +} + +class MockStdioClientTransport { + readonly close = mock(async () => {}) + readonly options: { command: string; args?: string[]; env?: Record; stderr?: string } + + constructor(options: { command: string; args?: string[]; env?: Record; stderr?: string }) { + this.options = options + createdStdioTransports.push(this) + } +} + +interface MockHttpTransportOptions { + requestInit?: { headers?: Record } +} + +class MockStreamableHTTPClientTransport { + readonly close = mock(async () => {}) + readonly url: URL + readonly options?: MockHttpTransportOptions + + constructor(url: URL, options?: MockHttpTransportOptions) { + this.url = url + this.options = options + createdHttpTransports.push(this) + } + + async start() {} +} + +mock.module("@modelcontextprotocol/sdk/client/index.js", () => ({ + Client: MockClient, +})) + +mock.module("@modelcontextprotocol/sdk/client/stdio.js", () => ({ + StdioClientTransport: MockStdioClientTransport, +})) + +mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ + StreamableHTTPClientTransport: MockStreamableHTTPClientTransport, +})) + +afterAll(() => { + mock.restore() +}) + +const { disconnectAll } = await import("./cleanup") +const { getOrCreateClient } = await import("./connection") + +function createState(): SkillMcpManagerState { + const state: SkillMcpManagerState = { + clients: new Map(), + pendingConnections: new Map(), + disconnectedSessions: new Map(), + authProviders: new Map(), + cleanupRegistered: false, + cleanupInterval: null, + cleanupHandlers: [], + idleTimeoutMs: 5 * 60 * 1000, + shutdownGeneration: 0, + inFlightConnections: new Map(), + disposed: false, + createOAuthProvider: () => ({ + tokens: () => null, + login: async () => ({ accessToken: "test-token" }), + refresh: async () => ({ accessToken: "test-token" }), + }), + } + trackedStates.push(state) + return state +} + +function createClientInfo(serverName: string): SkillMcpClientInfo { + return { + serverName, + skillName: "env-skill", + sessionID: "session-env", + } +} + +function createClientKey(info: SkillMcpClientInfo): string { + return `${info.sessionID}:${info.skillName}:${info.serverName}` +} + +const ORIGINAL_ENV = { ...process.env } + +beforeEach(() => { + createdStdioTransports.length = 0 + createdHttpTransports.length = 0 +}) + +afterEach(async () => { + for (const state of trackedStates) { + await disconnectAll(state) + } + trackedStates.length = 0 + + for (const key of Object.keys(process.env)) { + if (!(key in ORIGINAL_ENV)) { + delete process.env[key] + } + } + for (const [key, value] of Object.entries(ORIGINAL_ENV)) { + process.env[key] = value + } +}) + +describe("getOrCreateClient env var expansion", () => { + describe("#given a stdio skill MCP config with sensitive env vars in args", () => { + it("#when creating the client #then sensitive env vars in args are expanded", async () => { + // given + process.env.SLACK_USER_TOKEN = "xoxp-secret-token" + const state = createState() + const info = createClientInfo("slack-stdio") + const clientKey = createClientKey(info) + const config: ClaudeCodeMcpServer = { + command: "npx", + args: [ + "-y", + "mcp-remote", + "https://mcp.slack.com/mcp", + "--header", + "Authorization:Bearer ${SLACK_USER_TOKEN}", + ], + } + + // when + await getOrCreateClient({ state, clientKey, info, config }) + + // then + expect(createdStdioTransports).toHaveLength(1) + expect(createdStdioTransports[0]?.options.args).toEqual([ + "-y", + "mcp-remote", + "https://mcp.slack.com/mcp", + "--header", + "Authorization:Bearer xoxp-secret-token", + ]) + }) + }) + + describe("#given a stdio skill MCP config with sensitive env vars in env map", () => { + it("#when creating the client #then sensitive env vars in env map are expanded", async () => { + // given + process.env.MY_SLACK_USER_TOKEN_VALUE = "token-123" + const state = createState() + const info = createClientInfo("env-stdio") + const clientKey = createClientKey(info) + const config: ClaudeCodeMcpServer = { + command: "node", + args: ["server.js"], + env: { + SLACK_BOT_USER_ID: "${MY_SLACK_USER_TOKEN_VALUE}", + }, + } + + // when + await getOrCreateClient({ state, clientKey, info, config }) + + // then + expect(createdStdioTransports).toHaveLength(1) + expect(createdStdioTransports[0]?.options.env?.SLACK_BOT_USER_ID).toBe("token-123") + }) + }) + + describe("#given an http skill MCP config with sensitive env vars in headers", () => { + it("#when creating the client #then sensitive env vars in headers are expanded", async () => { + // given + process.env.SLACK_USER_TOKEN = "xoxp-http-secret" + const state = createState() + const info = createClientInfo("slack-http") + const clientKey = createClientKey(info) + const config: ClaudeCodeMcpServer = { + url: "https://mcp.slack.com/mcp", + headers: { + Authorization: "Bearer ${SLACK_USER_TOKEN}", + }, + } + + // when + await getOrCreateClient({ state, clientKey, info, config }) + + // then + expect(createdHttpTransports).toHaveLength(1) + expect(createdHttpTransports[0]?.options?.requestInit?.headers?.Authorization).toBe( + "Bearer xoxp-http-secret" + ) + }) + }) +}) diff --git a/src/features/skill-mcp-manager/connection.ts b/src/features/skill-mcp-manager/connection.ts index 890444bce..79754e712 100644 --- a/src/features/skill-mcp-manager/connection.ts +++ b/src/features/skill-mcp-manager/connection.ts @@ -38,7 +38,7 @@ export async function getOrCreateClient(params: { return pending } - const expandedConfig = expandEnvVarsInObject(config) + const expandedConfig = expandEnvVarsInObject(config, { trusted: true }) let currentConnectionPromise!: Promise state.inFlightConnections.set(info.sessionID, (state.inFlightConnections.get(info.sessionID) ?? 0) + 1) currentConnectionPromise = (async () => {