From 85d86ebc4b8495f4bb9692ae717f85bb6fc33e2c Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 21 Apr 2026 04:58:26 +0100 Subject: [PATCH] fix: narrow MCP stdio env safety filter (#69540) --- CHANGELOG.md | 1 + src/agents/mcp-config-shared.ts | 15 ++++++---- src/agents/mcp-stdio.ts | 7 +++-- src/agents/mcp-transport-config.test.ts | 38 ++++++++++++++++++++++++- src/agents/mcp-transport-config.ts | 8 +++++- 5 files changed, 59 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a102ba5c1ee..448312f35e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ Docs: https://docs.openclaw.ai - Lobster/TaskFlow: allow managed approval resumes to use `approvalId` without a resume token, and persist that id in approval wait state. (#69559) Thanks @kirkluokun. - Plugins/startup: install bundled runtime dependencies into each plugin's own runtime directory, reuse source-checkout repair caches after rebuilds, and log only packages that were actually installed so repeated Gateway starts stay quiet once deps are present. - Plugins/startup: ignore pnpm's `npm_execpath` when repairing bundled plugin runtime dependencies and skip workspace-only package specs so npm-only install flags or local workspace links do not break packaged plugin startup. +- MCP: block interpreter-startup env keys such as `NODE_OPTIONS` for stdio servers while preserving ordinary credential and proxy env vars. (#69540) Thanks @drobison00. - Setup/TUI: relaunch the setup hatch TUI in a fresh process while preserving the configured gateway target and auth source, so onboarding recovers terminal state cleanly without exposing gateway secrets on command-line args. (#69524) Thanks @shakkernerd. - Codex: avoid re-exposing the image-generation tool on native vision turns with inbound images, and keep bare image-model overrides on the configured image provider. (#65061) Thanks @zhulijin1991. - Sessions/reset: clear auto-sourced model, provider, and auth-profile overrides on `/new` and `/reset` while preserving explicit user selections, so channel sessions stop staying pinned to runtime fallback choices. (#69419) Thanks @sk7n4k3d. diff --git a/src/agents/mcp-config-shared.ts b/src/agents/mcp-config-shared.ts index a29c70d4ab4..302d035eea0 100644 --- a/src/agents/mcp-config-shared.ts +++ b/src/agents/mcp-config-shared.ts @@ -1,7 +1,4 @@ -import { - isDangerousHostEnvOverrideVarName, - isDangerousHostEnvVarName, -} from "../infra/host-env-security.js"; +import { isDangerousHostEnvVarName } from "../infra/host-env-security.js"; export function isMcpConfigRecord(value: unknown): value is Record { return value !== null && typeof value === "object" && !Array.isArray(value); @@ -11,15 +8,18 @@ function toMcpFilteredStringRecord( value: unknown, options?: { onDroppedEntry?: (key: string, value: unknown) => void; + preserveEmptyWhenKeysDropped?: boolean; shouldDropKey?: (key: string) => boolean; }, ): Record | undefined { if (!isMcpConfigRecord(value)) { return undefined; } + let droppedByKey = false; const entries = Object.entries(value) .map(([key, entry]) => { if (options?.shouldDropKey?.(key)) { + droppedByKey = true; options?.onDroppedEntry?.(key, entry); return null; } @@ -33,6 +33,9 @@ function toMcpFilteredStringRecord( return null; }) .filter((entry): entry is readonly [string, string] => entry !== null); + if (entries.length === 0 && droppedByKey && options?.preserveEmptyWhenKeysDropped) { + return {}; + } return entries.length > 0 ? Object.fromEntries(entries) : undefined; } @@ -49,8 +52,8 @@ export function toMcpEnvRecord( ): Record | undefined { return toMcpFilteredStringRecord(value, { ...options, - shouldDropKey: (key) => - isDangerousHostEnvVarName(key) || isDangerousHostEnvOverrideVarName(key), + preserveEmptyWhenKeysDropped: true, + shouldDropKey: (key) => isDangerousHostEnvVarName(key), }); } diff --git a/src/agents/mcp-stdio.ts b/src/agents/mcp-stdio.ts index c74f5717cf1..7776e2296c4 100644 --- a/src/agents/mcp-stdio.ts +++ b/src/agents/mcp-stdio.ts @@ -11,7 +11,10 @@ type StdioMcpServerLaunchResult = | { ok: true; config: StdioMcpServerLaunchConfig } | { ok: false; reason: string }; -export function resolveStdioMcpServerLaunchConfig(raw: unknown): StdioMcpServerLaunchResult { +export function resolveStdioMcpServerLaunchConfig( + raw: unknown, + options?: { onDroppedEnv?: (key: string, value: unknown) => void }, +): StdioMcpServerLaunchResult { if (!isMcpConfigRecord(raw)) { return { ok: false, reason: "server config must be an object" }; } @@ -35,7 +38,7 @@ export function resolveStdioMcpServerLaunchConfig(raw: unknown): StdioMcpServerL config: { command: raw.command, args: toMcpStringArray(raw.args), - env: toMcpEnvRecord(raw.env), + env: toMcpEnvRecord(raw.env, { onDroppedEntry: options?.onDroppedEnv }), cwd, }, }; diff --git a/src/agents/mcp-transport-config.test.ts b/src/agents/mcp-transport-config.test.ts index 082d514e8c5..44e77642108 100644 --- a/src/agents/mcp-transport-config.test.ts +++ b/src/agents/mcp-transport-config.test.ts @@ -1,7 +1,14 @@ -import { describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { logWarn } from "../logger.js"; import { resolveMcpTransportConfig } from "./mcp-transport-config.js"; +vi.mock("../logger.js", () => ({ logWarn: vi.fn() })); + describe("resolveMcpTransportConfig", () => { + beforeEach(() => { + vi.mocked(logWarn).mockClear(); + }); + it("resolves stdio config with connection timeout", () => { const resolved = resolveMcpTransportConfig("probe", { command: "node", @@ -25,6 +32,8 @@ describe("resolveMcpTransportConfig", () => { SAFE_VALUE: "ok", PORT: 3000, ENABLED: true, + GITHUB_TOKEN: "token", + HTTP_PROXY: "http://proxy.example", NODE_OPTIONS: "--require=./evil.js", LD_PRELOAD: "/tmp/pwn.so", BASH_ENV: "/tmp/pwn.sh", @@ -40,11 +49,38 @@ describe("resolveMcpTransportConfig", () => { SAFE_VALUE: "ok", PORT: "3000", ENABLED: "true", + GITHUB_TOKEN: "token", + HTTP_PROXY: "http://proxy.example", }, cwd: undefined, description: "node", connectionTimeoutMs: 30_000, }); + expect(logWarn).toHaveBeenCalledWith( + 'bundle-mcp: server "probe": env "NODE_OPTIONS" is blocked for stdio startup safety and was ignored.', + ); + expect(logWarn).toHaveBeenCalledWith( + 'bundle-mcp: server "probe": env "LD_PRELOAD" is blocked for stdio startup safety and was ignored.', + ); + expect(logWarn).toHaveBeenCalledWith( + 'bundle-mcp: server "probe": env "BASH_ENV" is blocked for stdio startup safety and was ignored.', + ); + }); + + it("uses an explicit empty stdio env when all configured env keys are blocked", () => { + const resolved = resolveMcpTransportConfig("probe", { + command: "node", + env: { + NODE_OPTIONS: "--require=./evil.js", + BASH_ENV: "/tmp/pwn.sh", + }, + }); + + expect(resolved).toMatchObject({ + kind: "stdio", + command: "node", + env: {}, + }); }); it("resolves SSE config by default", () => { diff --git a/src/agents/mcp-transport-config.ts b/src/agents/mcp-transport-config.ts index 795062c7cc9..475ac44cc52 100644 --- a/src/agents/mcp-transport-config.ts +++ b/src/agents/mcp-transport-config.ts @@ -96,7 +96,13 @@ export function resolveMcpTransportConfig( rawServer: unknown, ): ResolvedMcpTransportConfig | null { const requestedTransport = getRequestedTransport(rawServer); - const stdioLaunch = resolveStdioMcpServerLaunchConfig(rawServer); + const stdioLaunch = resolveStdioMcpServerLaunchConfig(rawServer, { + onDroppedEnv: (key) => { + logWarn( + `bundle-mcp: server "${serverName}": env "${key}" is blocked for stdio startup safety and was ignored.`, + ); + }, + }); if (stdioLaunch.ok) { return { kind: "stdio",