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 <clio-agent@sisyphuslabs.ai>
This commit is contained in:
@@ -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. */
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
@@ -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 ",
|
||||
},
|
||||
})
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -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
|
||||
|
||||
@@ -9,3 +9,4 @@ export * from "./types"
|
||||
export * from "./loader"
|
||||
export * from "./transformer"
|
||||
export * from "./env-expander"
|
||||
export * from "./configure-allowed-env-vars"
|
||||
|
||||
@@ -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,
|
||||
})
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -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<string, unknown> = {
|
||||
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
|
||||
|
||||
@@ -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<string, unknown>) => {
|
||||
const formatterConfig = config.formatter;
|
||||
|
||||
setAdditionalAllowedMcpEnvVars(pluginConfig.mcp_env_allowlist ?? [])
|
||||
applyProviderConfig({ config, modelCacheState });
|
||||
clearFormatterCache()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user