Merge pull request #3026 from code-yeongyu/fix/security-mcp-env-expansion

fix(security): restrict env var expansion in MCP configs

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<string>()
+
+export function getAllowedMcpEnvVars(): Set<string> {
+  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()
+}

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 ",
+        },
+      })
+    })
+  })
+})

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

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"

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,
+      })
+    })
+  })
 })