From 466e43772ee60324e58a225f319cff2370fbac7e Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Wed, 30 Sep 2026 11:48:22 +0800 Subject: [PATCH] fix(mcp): restrict stdio server environment --- .../coding-agent/src/step/mcp-environment.ts | 14 ++++++++-- packages/coding-agent/src/step/mcp.test.ts | 28 ++++++++++++++++++- packages/coding-agent/src/step/plugins.ts | 15 +++++----- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/packages/coding-agent/src/step/mcp-environment.ts b/packages/coding-agent/src/step/mcp-environment.ts index c177387e..d46ee0c3 100644 --- a/packages/coding-agent/src/step/mcp-environment.ts +++ b/packages/coding-agent/src/step/mcp-environment.ts @@ -8,6 +8,7 @@ * describes starts fine. */ +import { DEFAULT_INHERITED_ENV_VARS } from "@modelcontextprotocol/sdk/client/stdio.js"; import { readStoredCredential } from "../core/auth-storage.ts"; import { getStepAuthPath } from "./auth.ts"; @@ -18,13 +19,22 @@ import { getStepAuthPath } from "./auth.ts"; */ export const STEP_LOGIN_SUPPLIED_ENV: readonly string[] = ["STEPFUN_API_KEY"]; -/** Resolve the environment passed to a plugin server, including Step login fallback. */ +/** + * Resolve the environment passed to a plugin server, including Step login fallback. + * + * Match the MCP SDK's default environment allowlist instead of exposing every + * variable held by the Step process to an arbitrary local MCP executable. + */ export function resolveStepMcpEnvironment( declared: Record | undefined, input: { env?: NodeJS.ProcessEnv; authPath?: string } = {}, ): Record { const resolved: Record = {}; - for (const [key, value] of Object.entries(input.env ?? process.env)) if (value !== undefined) resolved[key] = value; + const inherited = input.env ?? process.env; + for (const key of DEFAULT_INHERITED_ENV_VARS) { + const value = inherited[key]; + if (value !== undefined && !value.startsWith("()")) resolved[key] = value; + } Object.assign(resolved, declared ?? {}); if (!resolved.STEPFUN_API_KEY?.trim()) { const credential = readStoredCredential("step", input.authPath ?? getStepAuthPath()); diff --git a/packages/coding-agent/src/step/mcp.test.ts b/packages/coding-agent/src/step/mcp.test.ts index 780968cc..8c4e61c5 100644 --- a/packages/coding-agent/src/step/mcp.test.ts +++ b/packages/coding-agent/src/step/mcp.test.ts @@ -46,6 +46,32 @@ afterEach(async () => { await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))); }); +test("inherits only the MCP SDK safe environment defaults", () => { + const resolved = resolveStepMcpEnvironment(undefined, { + env: { + PATH: "/bin", + AWS_SECRET_ACCESS_KEY: "aws-secret", + OPENAI_API_KEY: "openai-secret", + UNRELATED_PRIVATE_TOKEN: "private", + }, + authPath: "/definitely/missing/auth.json", + }); + + expect(resolved).toEqual({ PATH: "/bin" }); +}); + +test("adds explicitly declared server variables without inheriting unrelated secrets", () => { + const resolved = resolveStepMcpEnvironment( + { SERVER_TOKEN: "declared", PATH: "/server/bin" }, + { + env: { PATH: "/shell/bin", UNRELATED_PRIVATE_TOKEN: "private" }, + authPath: "/definitely/missing/auth.json", + }, + ); + + expect(resolved).toEqual({ PATH: "/server/bin", SERVER_TOKEN: "declared" }); +}); + test("uses the logged-in Step credential only as a server env fallback", async () => { const root = await mkdtemp(path.join(os.tmpdir(), "step-mcp-auth-")); roots.push(root); @@ -69,7 +95,7 @@ test("uses the logged-in Step credential only as a server env fallback", async ( expect(resolved.STEPFUN_API_KEY).toBe("login-key"); }); -test("explicit declaration wins over both shell and login credentials", async () => { +test("explicit declaration wins over the login credential", async () => { const root = await mkdtemp(path.join(os.tmpdir(), "step-mcp-auth-")); roots.push(root); const authPath = path.join(root, "auth.json"); diff --git a/packages/coding-agent/src/step/plugins.ts b/packages/coding-agent/src/step/plugins.ts index 4ed67f0b..6c3c6f2b 100644 --- a/packages/coding-agent/src/step/plugins.ts +++ b/packages/coding-agent/src/step/plugins.ts @@ -645,12 +645,11 @@ export async function diagnoseStepPlugin( if (read.manifest.entry) warnings.push("Executable plugin entries are recorded but not loaded by the Step marketplace facade."); // Judged against the environment the matching servers are actually spawned - // with: `connectStepMcpServer` layers the process environment, the server's - // own declared `env`, and the Step login credential. Checking `process.env` - // alone reported every logged-in user as missing a variable they were never - // expected to export by hand. Only an inline `mcpServers` record can start a - // server — discovery skips a string declaration path — so that is the only - // shape whose declared `env` can satisfy a requirement. + // with: `connectStepMcpServer` layers the SDK's safe inherited environment, + // the server's own declared `env`, and the Step login credential. Only an + // inline `mcpServers` record can start a server — discovery skips a string + // declaration path — so that is the only shape whose declared `env` can + // satisfy a requirement. const requiredEnvironment = read.manifest.provision?.requiresEnv ?? []; if (requiredEnvironment.length > 0) { const candidates = provisionedServerEnvironments(read.manifest).map((declared) => @@ -665,12 +664,12 @@ export async function diagnoseStepPlugin( const missingOther = missingEnvironment.filter((name) => !STEP_LOGIN_SUPPLIED_ENV.includes(name)); if (missingLogin.length > 0) { warnings.push( - `Plugin provisioning has no value for ${missingLogin.join(", ")}; run /login or export it before using this plugin.`, + `Plugin provisioning has no value for ${missingLogin.join(", ")}; run /login or declare it in the plugin's mcpServers env before using this plugin.`, ); } if (missingOther.length > 0) { warnings.push( - `Plugin provisioning has no value for ${missingOther.join(", ")}; export it or declare it in the plugin's mcpServers env before using this plugin.`, + `Plugin provisioning has no value for ${missingOther.join(", ")}; declare it in the plugin's mcpServers env before using this plugin.`, ); } }