[security] Remote MCP `headers` templates read arbitrary `process.env` — the stdio allowlist from #208 is not applied on the HTTP path

[uos1231234]

[security] Remote MCP headers templates read arbitrary process.env — the stdio allowlist from #208 is not applied on the HTTP path

Summary

expandHeaderTemplate resolves ${VAR} in a plugin's declared MCP headers by reading process.env[VAR] with no allowlist. The result is attached to the outbound HTTP request as http_headers.

PR #208 ("fix(mcp): restrict stdio server environment") deliberately narrowed the environment handed to stdio subprocesses to DEFAULT_INHERITED_ENV_VARS. That boundary is not applied on the HTTP path, so the same third-party manifest can read any environment variable the StepCode process holds and send it to an arbitrary remote URL.

A url-type MCP server executes nothing on the user's machine, so it is reasonable for a user to assume it cannot read their environment.

Impact

A plugin that declares a url-type MCP server can exfiltrate API keys, CI tokens and any other environment variable of the StepCode process to a server the plugin author controls. The declared tool surface looks harmless.

Reproduction

Real run against main at 519e4de with the repository's own runner, using canary values so nothing real is read.

import { test, expect } from "vitest";
import { expandHeaderTemplate } from "../src/step/mcp.ts";
import { DEFAULT_INHERITED_ENV_VARS } from "@modelcontextprotocol/sdk/client/stdio.js";

test("header templates expand any env var", () => {
  const canaries = [
    "AWS_SECRET_ACCESS_KEY", "AWS_SESSION_TOKEN", "GITHUB_TOKEN",
    "OPENAI_API_KEY", "ANTHROPIC_API_KEY", "DB_PASSWORD",
    "SCA_TOTALLY_UNRELATED_VAR",
  ];
  for (const name of canaries) process.env[name] = `CANARY-${name}`;

  for (const name of canaries) {
    console.log(`\${${name}} -> ${JSON.stringify(expandHeaderTemplate(`\${${name}}`))}`);
  }
  console.log("stdio allowlist =", JSON.stringify(DEFAULT_INHERITED_ENV_VARS));

  expect(expandHeaderTemplate("${AWS_SECRET_ACCESS_KEY}")).toBe("CANARY-AWS_SECRET_ACCESS_KEY");
  expect(DEFAULT_INHERITED_ENV_VARS).not.toContain("AWS_SECRET_ACCESS_KEY");
});

Observed output:

${AWS_SECRET_ACCESS_KEY} -> "CANARY-AWS_SECRET_ACCESS_KEY"
${AWS_SESSION_TOKEN} -> "CANARY-AWS_SESSION_TOKEN"
${GITHUB_TOKEN} -> "CANARY-GITHUB_TOKEN"
${OPENAI_API_KEY} -> "CANARY-OPENAI_API_KEY"
${ANTHROPIC_API_KEY} -> "CANARY-ANTHROPIC_API_KEY"
${DB_PASSWORD} -> "CANARY-DB_PASSWORD"
${SCA_TOTALLY_UNRELATED_VAR} -> "CANARY-SCA_TOTALLY_UNRELATED_VAR"
stdio allowlist = ["APPDATA","HOMEDRIVE","HOMEPATH","LOCALAPPDATA","PATH","PROCESSOR_ARCHITECTURE","SYSTEMDRIVE","SYSTEMROOT","TEMP","USERNAME","USERPROFILE","PROGRAMFILES"]

Every variable expands. None of the credential-shaped names are in the stdio allowlist, which is the asymmetry this report is about.

Root cause

• packages/coding-agent/src/step/mcp.ts:424 — const value = name === undefined ? undefined : process.env[name]; inside expandHeaderTemplate, with no filtering of name.
• packages/coding-agent/src/step/mcp.ts:476-486 — applyPluginHeaderAliases calls it for every string entry in a manifest's headers, then stores the expanded value on the declaration.
• packages/coding-agent/src/step/mcp.ts:368-370 — discoverStepMcpServers applies this to every plugin mcpServers entry, so the path is reachable from any installed plugin.
• Contrast: packages/coding-agent/src/step/mcp-environment.ts restricts the stdio child environment to DEFAULT_INHERITED_ENV_VARS (the change from fix(mcp): restrict stdio server environment #208).

Attribution

expandHeaderTemplate predates #208 — commit 0903f1f only moved the line. The asymmetry is that #208 tightened the stdio side without applying the same constraint to headers. This reads as an omission in #208 rather than an intentional design choice, which is why I am reporting it against the current boundary rather than asking for a behaviour change.

Suggested fix

Apply an explicit allowlist to expandHeaderTemplate rather than to the transport, so both paths share one rule. Two reasonable options:

• restrict ${VAR} interpolation to DEFAULT_INHERITED_ENV_VARS, or
• require the manifest to declare which variables it may reference and validate name against that declaration.

The first is the smaller change and matches what #208 already established for stdio.

Verification performed

• Repository: stepfun-ai/Step-Code, main at 519e4de.
• Reproduction run with the repo's pinned vitest (4.1.9) on Node 24.19.0, Windows.
• Canary environment variables only; no real secret was read or transmitted, and no network request was made.
• No repository files were modified.

[uos1231234]

I tried to break this report and could not falsify the core claim. Three things in it are wrong or too weak, and one materially understates the exposure. Correcting all four.

1. Correction: the allowlist has 12 entries, not 11

DEFAULT_INHERITED_ENV_VARS on win32 is 12 entries (SDK stdio.js:8-22): APPDATA, HOMEDRIVE, HOMEPATH, LOCALAPPDATA, PATH, PROCESSOR_ARCHITECTURE, SYSTEMDRIVE, SYSTEMROOT, TEMP, USERNAME, USERPROFILE, PROGRAMFILES. The substantive point — none of them are credential names — is unaffected.

2. Correction: this is a documented, tested feature, not a leftover from #208

I framed the asymmetry as "#208 tightened stdio and forgot the header path". The mechanics were right, the characterization was not. The interpolation is an intentional Claude-compat feature, documented in the code:

mcp.ts:466-475 — "A plugin's .mcp.json may write headers and interpolate the environment ("Bearer ${API_KEY:-}"), which is Claude's format."

and covered by the repository's own end-to-end test:

mcp-startup.test.ts:241-305 — "carries Claude plugin headers, including environment interpolation", asserting expect(seen).toContain("from-env") against a real HTTP server.

docs/step-unified-config-and-mcp.md §1.5 also lists bearer_token_env_var / http_headers / env_http_headers as intended env-driven header features. PR #208 = merge ec1cd22, whose stat touches only mcp-environment.ts, mcp.test.ts and plugins.ts — not mcp.ts.

The accurate framing is therefore: the HTTP header path has no allowlist of its own, while #208 established one for stdio, and the two paths were never aligned. That is a real gap, but it is a missing-allowlist gap rather than an accidental omission, and the fix should be scoped that way — tightening the shared rule, not removing the feature.

3. The exposure is larger than I reported: ${} is not required

There are two additional template-free paths in the same file, both reading process.env directly with no allowlist. I missed them, which makes my report an undercount:

• env_http_headers — accepted at mcp.ts:513-518, expanded at mcp.ts:621-628 via process.env[envName]?.trim(). A manifest can write {"X-Plain":"AWS_SECRET_ACCESS_KEY"} and needs no $ syntax at all.
• bearer_token_env_var — mcp.ts:629-634 turns any variable name directly into Authorization: Bearer $VAR.

plugins.ts:310 copies mcpServers with structuredClone and no per-server field validation, and readPluginManifestAtPath (plugins.ts:350-382) also auto-adopts a sibling .mcp.json, so all three are reachable from a plain manifest.

4. Closing the gap in my reproduction: wire-level evidence

My report only exercised expandHeaderTemplate as a pure function, which was the weakest part. An independent pass built the full unmocked chain — real addMarketplaceSource → real installMarketplacePlugin → real discoverStepMcpServers → real StreamableHTTPClientTransport → a local node:http server capturing the bytes:

x-aws:       CANARY-AWS-LEAK-0001
x-gh:        CANARY-GH-LEAK-0002
x-oai:       CANARY-OAI-LEAK-0003
x-unrelated: CANARY-UNRELATED-LEAK-0004
x-plain:     CANARY-PLAIN-LEAK-0005      (via env_http_headers, no ${} needed)

The path from http_headers to the wire is a straight line with no branch: mcp.ts:553 resolveHttpHeaders → :554-555 requestInit: { headers } → SDK streamableHttp.js:58-77 _commonHeaders() merges them into the Headers used on GET/SSE, POST and DELETE.

One property worth stating precisely, because it is not a mitigation: an unset variable causes the header to be omitted entirely (the literal template never reaches the server), so only variables that are actually set can be exfiltrated. That is not a mitigation — the variables worth stealing are exactly the ones that are set.

Threat model, stated explicitly

This requires the user to install a plugin (or have one preinstalled via #214) and the URL is chosen by the plugin author, so it is not an unauthenticated remote issue. The property that makes it worth fixing anyway: a plugin that declares only a remote server executes no local code, so the user sees no local execution signal, while their environment is still exfiltrated. That is a different trust decision from "a stdio plugin can run arbitrary code anyway".