From e8c5727a224c7ba8a130e64287d1f6a8647c40e5 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Thu, 2 Apr 2026 14:55:01 +0900 Subject: [PATCH] fix(mcp): restrict env var expansion in MCP configs Block sensitive env var interpolation in MCP config expansion so repo and plugin MCP definitions cannot exfiltrate secrets by default. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- src/config/schema/oh-my-opencode-config.ts | 1 + .../configure-allowed-env-vars.ts | 24 ++++ .../env-expander.test.ts | 129 ++++++++++++++++++ .../claude-code-mcp-loader/env-expander.ts | 17 +++ src/features/claude-code-mcp-loader/index.ts | 1 + .../transformer.test.ts | 47 +++++++ src/plugin-config.ts | 7 + src/plugin-handlers/config-handler.test.ts | 32 +++++ src/plugin-handlers/config-handler.ts | 2 + 9 files changed, 260 insertions(+) create mode 100644 src/features/claude-code-mcp-loader/configure-allowed-env-vars.ts create mode 100644 src/features/claude-code-mcp-loader/env-expander.test.ts diff --git a/src/config/schema/oh-my-opencode-config.ts b/src/config/schema/oh-my-opencode-config.ts index 5db7b0559..eb9299769 100644 --- a/src/config/schema/oh-my-opencode-config.ts +++ b/src/config/schema/oh-my-opencode-config.ts @@ -36,6 +36,7 @@ export const OhMyOpenCodeConfigSchema = z.object({ disabled_commands: z.array(BuiltinCommandNameSchema).optional(), /** Disable specific tools by name (e.g., ["todowrite", "todoread"]) */ disabled_tools: z.array(z.string()).optional(), + mcp_env_allowlist: z.array(z.string()).optional(), /** Enable hashline_edit tool/hook integrations (default: false) */ hashline_edit: z.boolean().optional(), /** Enable model fallback on API errors (default: false). Set to true to enable automatic model switching when model errors occur. */ diff --git a/src/features/claude-code-mcp-loader/configure-allowed-env-vars.ts b/src/features/claude-code-mcp-loader/configure-allowed-env-vars.ts new file mode 100644 index 000000000..1aa204a56 --- /dev/null +++ b/src/features/claude-code-mcp-loader/configure-allowed-env-vars.ts @@ -0,0 +1,24 @@ +const BUILTIN_ALLOWED_MCP_ENV_VARS = ["PATH", "HOME", "USER", "SHELL", "TERM"] +const SENSITIVE_MCP_ENV_VAR_PATTERN = /KEY|TOKEN|SECRET|PASSWORD|AUTH|CREDENTIAL/i + +let additionalAllowedMcpEnvVars = new Set() + +export function getAllowedMcpEnvVars(): Set { + return new Set([...BUILTIN_ALLOWED_MCP_ENV_VARS, ...additionalAllowedMcpEnvVars]) +} + +export function isSensitiveMcpEnvVar(varName: string): boolean { + return SENSITIVE_MCP_ENV_VAR_PATTERN.test(varName) +} + +export function isAllowedMcpEnvVar(varName: string): boolean { + return getAllowedMcpEnvVars().has(varName) +} + +export function setAdditionalAllowedMcpEnvVars(varNames: string[]): void { + additionalAllowedMcpEnvVars = new Set(varNames) +} + +export function resetAdditionalAllowedMcpEnvVars(): void { + additionalAllowedMcpEnvVars = new Set() +} diff --git a/src/features/claude-code-mcp-loader/env-expander.test.ts b/src/features/claude-code-mcp-loader/env-expander.test.ts new file mode 100644 index 000000000..571f6219c --- /dev/null +++ b/src/features/claude-code-mcp-loader/env-expander.test.ts @@ -0,0 +1,129 @@ +import { afterEach, describe, expect, it, mock, spyOn } from "bun:test" +import * as shared from "../../shared/logger" +import { + resetAdditionalAllowedMcpEnvVars, + setAdditionalAllowedMcpEnvVars, +} from "./configure-allowed-env-vars" +import { expandEnvVars, expandEnvVarsInObject } from "./env-expander" + +describe("expandEnvVars", () => { + const originalEnv = { ...process.env } + + afterEach(() => { + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key] + } + } + + for (const [key, value] of Object.entries(originalEnv)) { + process.env[key] = value + } + + mock.restore() + resetAdditionalAllowedMcpEnvVars() + }) + + describe("#given a sensitive environment variable reference", () => { + it("#when expanding the value #then it returns an empty string and logs a warning", () => { + // given + process.env.GITHUB_TOKEN = "ghp-secret" + const logSpy = spyOn(shared, "log").mockImplementation(() => {}) + + // when + const expanded = expandEnvVars("${GITHUB_TOKEN}") + + // then + expect(expanded).toBe("") + expect(logSpy).toHaveBeenCalledWith( + expect.stringContaining("Blocked MCP env var expansion"), + expect.objectContaining({ varName: "GITHUB_TOKEN" }) + ) + }) + }) + + describe("#given a blocked variable with a default value", () => { + it("#when expanding the value #then it uses the default instead of the sensitive env var", () => { + // given + process.env.SECRET_KEY = "super-secret" + + // when + const expanded = expandEnvVars("${SECRET_KEY:-fallback}") + + // then + expect(expanded).toBe("fallback") + }) + }) + + describe("#given a safe allowlisted environment variable reference", () => { + it("#when expanding the value #then it returns the env value", () => { + // given + process.env.HOME = "/Users/tester" + + // when + const expanded = expandEnvVars("${HOME}") + + // then + expect(expanded).toBe("/Users/tester") + }) + }) + + describe("#given a sensitive environment variable listed in the user allowlist", () => { + it("#when expanding the value #then it returns the env value", () => { + // given + process.env.CUSTOM_API_KEY = "user-approved" + setAdditionalAllowedMcpEnvVars(["CUSTOM_API_KEY"]) + + // when + const expanded = expandEnvVars("${CUSTOM_API_KEY}") + + // then + expect(expanded).toBe("user-approved") + }) + }) +}) + +describe("expandEnvVarsInObject", () => { + const originalEnv = { ...process.env } + + afterEach(() => { + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key] + } + } + + for (const [key, value] of Object.entries(originalEnv)) { + process.env[key] = value + } + + mock.restore() + resetAdditionalAllowedMcpEnvVars() + }) + + describe("#given a nested MCP config object", () => { + it("#when expanding env vars in the object #then it only expands safe values", () => { + // given + process.env.HOME = "/Users/tester" + process.env.AWS_SECRET_ACCESS_KEY = "aws-secret" + + // when + const expanded = expandEnvVarsInObject({ + url: "https://example.com/${AWS_SECRET_ACCESS_KEY}", + args: ["--dir", "${HOME}"], + headers: { + Authorization: "Bearer ${AWS_SECRET_ACCESS_KEY}", + }, + }) + + // then + expect(expanded).toEqual({ + url: "https://example.com/", + args: ["--dir", "/Users/tester"], + headers: { + Authorization: "Bearer ", + }, + }) + }) + }) +}) diff --git a/src/features/claude-code-mcp-loader/env-expander.ts b/src/features/claude-code-mcp-loader/env-expander.ts index b3edf890a..5b4ff6843 100644 --- a/src/features/claude-code-mcp-loader/env-expander.ts +++ b/src/features/claude-code-mcp-loader/env-expander.ts @@ -1,7 +1,24 @@ +import { log } from "../../shared/logger" +import { + isAllowedMcpEnvVar, + isSensitiveMcpEnvVar, +} from "./configure-allowed-env-vars" + export function expandEnvVars(value: string): string { return value.replace( /\$\{([^}:]+)(?::-([^}]*))?\}/g, (_, varName: string, defaultValue?: string) => { + if (!isAllowedMcpEnvVar(varName)) { + if (isSensitiveMcpEnvVar(varName)) { + log(`Blocked MCP env var expansion for sensitive variable "${varName}"`, { + varName, + }) + } + + if (defaultValue !== undefined) return defaultValue + return "" + } + const envValue = process.env[varName] if (envValue !== undefined) return envValue if (defaultValue !== undefined) return defaultValue diff --git a/src/features/claude-code-mcp-loader/index.ts b/src/features/claude-code-mcp-loader/index.ts index 20f49725b..556f0ded1 100644 --- a/src/features/claude-code-mcp-loader/index.ts +++ b/src/features/claude-code-mcp-loader/index.ts @@ -9,3 +9,4 @@ export * from "./types" export * from "./loader" export * from "./transformer" export * from "./env-expander" +export * from "./configure-allowed-env-vars" diff --git a/src/features/claude-code-mcp-loader/transformer.test.ts b/src/features/claude-code-mcp-loader/transformer.test.ts index fa4508372..41fcbe3a6 100644 --- a/src/features/claude-code-mcp-loader/transformer.test.ts +++ b/src/features/claude-code-mcp-loader/transformer.test.ts @@ -26,4 +26,51 @@ describe("transformMcpServer", () => { }) }) }) + + describe("#given a server config containing sensitive env references", () => { + it("#when transforming a local MCP server #then it strips sensitive env vars from the environment", () => { + // given + process.env.GITHUB_TOKEN = "ghp-secret" + process.env.HOME = "/Users/tester" + + // when + const transformed = transformMcpServer("local-secure", { + command: "npx", + args: ["mcp-server", "${HOME}"], + env: { + HOME_DIR: "${HOME}", + AUTH_TOKEN: "${GITHUB_TOKEN}", + }, + }) + + // then + expect(transformed).toEqual({ + type: "local", + command: ["npx", "mcp-server", "/Users/tester"], + environment: { + HOME_DIR: "/Users/tester", + AUTH_TOKEN: "", + }, + enabled: true, + }) + }) + + it("#when transforming a remote MCP server #then it strips sensitive env vars from the url", () => { + // given + process.env.API_KEY = "secret-key" + + // when + const transformed = transformMcpServer("remote-secure", { + type: "http", + url: "https://mcp.example.com/${API_KEY}", + }) + + // then + expect(transformed).toEqual({ + type: "remote", + url: "https://mcp.example.com/", + enabled: true, + }) + }) + }) }) diff --git a/src/plugin-config.ts b/src/plugin-config.ts index fd41e24c9..78350cfda 100644 --- a/src/plugin-config.ts +++ b/src/plugin-config.ts @@ -20,6 +20,7 @@ const PARTIAL_STRING_ARRAY_KEYS = new Set([ "disabled_hooks", "disabled_commands", "disabled_tools", + "mcp_env_allowlist", ]); export function parseConfigPartially( @@ -154,6 +155,12 @@ export function mergeConfigs( ...(override.disabled_tools ?? []), ]), ], + mcp_env_allowlist: [ + ...new Set([ + ...(base.mcp_env_allowlist ?? []), + ...(override.mcp_env_allowlist ?? []), + ]), + ], claude_code: deepMerge(base.claude_code, override.claude_code), }; } diff --git a/src/plugin-handlers/config-handler.test.ts b/src/plugin-handlers/config-handler.test.ts index f79b5b681..050a5ab69 100644 --- a/src/plugin-handlers/config-handler.test.ts +++ b/src/plugin-handlers/config-handler.test.ts @@ -57,6 +57,7 @@ beforeEach(() => { spyOn(agentLoader, "loadProjectAgents" as any).mockReturnValue({}) spyOn(mcpLoader, "loadMcpConfigs" as any).mockResolvedValue({ servers: {} }) + spyOn(mcpLoader, "setAdditionalAllowedMcpEnvVars").mockImplementation(() => {}) spyOn(pluginLoader, "loadAllPluginComponents" as any).mockResolvedValue({ commands: {}, @@ -103,6 +104,7 @@ afterEach(() => { ;(agentLoader.loadUserAgents as any)?.mockRestore?.() ;(agentLoader.loadProjectAgents as any)?.mockRestore?.() ;(mcpLoader.loadMcpConfigs as any)?.mockRestore?.() + ;(mcpLoader.setAdditionalAllowedMcpEnvVars as any)?.mockRestore?.() ;(pluginLoader.loadAllPluginComponents as any)?.mockRestore?.() ;(mcpModule.createBuiltinMcps as any)?.mockRestore?.() ;(shared.log as any)?.mockRestore?.() @@ -173,6 +175,36 @@ describe("Sisyphus-Junior model inheritance", () => { }) }) +describe("MCP env allowlist initialization", () => { + test("sets the configured MCP env allowlist before plugin loading", async () => { + // given + const pluginConfig = createPluginConfig({ + mcp_env_allowlist: ["CUSTOM_API_KEY", "CUSTOM_AUTH_TOKEN"], + }) + const config: Record = { + model: "anthropic/claude-opus-4-6", + agent: {}, + } + const handler = createConfigHandler({ + ctx: { directory: "/tmp" }, + pluginConfig, + modelCacheState: { + anthropicContext1MEnabled: false, + modelContextLimitsCache: new Map(), + }, + }) + + // when + await handler(config) + + // then + expect(mcpLoader.setAdditionalAllowedMcpEnvVars).toHaveBeenCalledWith([ + "CUSTOM_API_KEY", + "CUSTOM_AUTH_TOKEN", + ]) + }) +}) + describe("Plan agent demote behavior", () => { test("orders core agents as sisyphus -> hephaestus -> prometheus -> atlas", async () => { // #given diff --git a/src/plugin-handlers/config-handler.ts b/src/plugin-handlers/config-handler.ts index b4836bd08..e75d95ab1 100644 --- a/src/plugin-handlers/config-handler.ts +++ b/src/plugin-handlers/config-handler.ts @@ -1,4 +1,5 @@ import type { OhMyOpenCodeConfig } from "../config"; +import { setAdditionalAllowedMcpEnvVars } from "../features/claude-code-mcp-loader"; import type { ModelCacheState } from "../plugin-state"; import { log } from "../shared"; import { applyAgentConfig } from "./agent-config-handler"; @@ -23,6 +24,7 @@ export function createConfigHandler(deps: ConfigHandlerDeps) { return async (config: Record) => { const formatterConfig = config.formatter; + setAdditionalAllowedMcpEnvVars(pluginConfig.mcp_env_allowlist ?? []) applyProviderConfig({ config, modelCacheState }); clearFormatterCache()