fix(mcp): block dangerous stdio env overrides#69540
Conversation
Greptile SummaryThis PR closes a security gap in MCP stdio transport config parsing: the Confidence Score: 5/5Safe to merge — the security fix is correct and well-scoped; remaining findings are P2 quality suggestions. The denylist reuse is sound: isDangerousHostEnvVarName covers blockedEverywhereKeys (NODE_OPTIONS, BASH_ENV, …) and blockedPrefixes (LD_, DYLD_, BASH_FUNC_), while isDangerousHostEnvOverrideVarName adds blockedOverrideOnlyKeys (credentials, HOME, …) and blockedOverridePrefixes. HTTP header parsing is untouched. The two remaining comments are P2 (no logging on drop; all-dangerous env collapsing to undefined) — both are pre-existing design characteristics that do not introduce new regressions. No files require special attention for merge safety. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/mcp-stdio.ts
Line: 38
Comment:
**Silent drop of dangerous keys — no diagnostic emitted**
`toMcpEnvRecord` is called without an `onDroppedEntry` callback, so when `NODE_OPTIONS`, `LD_PRELOAD`, `BASH_ENV`, or any other blocked key appears in the config, it is silently removed. The subprocess launches with the key absent and no warning is ever surfaced to the caller or logged. The `onDroppedEntry` hook was designed for exactly this case — passing it through to `resolveStdioMcpServerLaunchConfig`'s return value or emitting a structured log would make the behavior observable when users debug unexpected MCP server startup failures.
```suggestion
env: toMcpEnvRecord(raw.env, {
onDroppedEntry: (key) => {
// TODO: surface via structured log when logger is available
// e.g. logger?.warn(`MCP stdio env: dropping blocked key "${key}"`);
},
}),
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/mcp-config-shared.ts
Line: 50-54
Comment:
**All-dangerous env config silently collapses to `undefined`**
If every key in the configured `env` object is on the denylist, `toMcpFilteredStringRecord` returns `undefined` (line 36: `entries.length > 0 ? ... : undefined`). The caller (`resolveStdioMcpServerLaunchConfig`) then sets `env: undefined` on the launch config, which causes the spawned subprocess to inherit the full parent-process environment rather than receiving an empty env. This means a maximally-restricted config (all-dangerous keys) ends up less restricted than intended because the subprocess inherits everything. If the intent is to pass no env at all, the function should return an empty object instead of `undefined` when all entries were filtered out by `shouldDropKey`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(mcp): block dangerous stdio env over..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c23f60dd4f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
c23f60d to
959ebaa
Compare
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟡 Empty environment `{}` can be passed to StdioClientTransport, causing spawned MCP servers to run with no inherited env (DoS/unintended behavior)
Description
That value is propagated into MCP stdio transport creation:
In Node.js process spawning semantics,
This behavior is reachable specifically when a config supplies only blocked env keys (as tested in RecommendationAvoid passing an explicit empty environment to the spawned stdio server. Option A (preferred): inherit parent env, applying only safe overrides
Example (at the point of transport creation): const safeEnv = resolved.env;
const env = safeEnv ? { ...process.env, ...safeEnv } : undefined;
const transport = new StdioClientTransport({
command: resolved.command,
args: resolved.args,
env,
cwd: resolved.cwd,
stderr: "pipe",
});Option B: change Add a regression test ensuring that when all env keys are blocked, the resulting transport/spawn still inherits 2. 🔵 Log forging via unsanitized `serverName` and env var key in warning log
DescriptionThe warning callback
Vulnerable code: onDroppedEnv: (key) => {
logWarn(
`bundle-mcp: server "${serverName}": env "${key}" is blocked for stdio startup safety and was ignored.`,
);
},RecommendationSanitize/escape untrusted fields before including them in log messages, or switch to structured logging. Minimal mitigation: escape control characters (CR/LF, tabs, ANSI) in Example: function escapeForLog(s: string): string {
return s.replace(/[\r\n\t]/g, (c) => ({"\r":"\\r","\n":"\\n","\t":"\\t"}[c]!))
.replace(/\x1b\[[0-9;]*m/g, ""); // strip ANSI color codes
}
onDroppedEnv: (key) => {
logWarn(
`bundle-mcp: server "${escapeForLog(serverName)}": env "${escapeForLog(key)}" is blocked for stdio startup safety and was ignored.`,
);
},Preferred: use a structured logger and pass fields separately (e.g., Analyzed PR: #69540 at commit Last updated on: 2026-04-21T06:59:39Z |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 959ebaa2f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -35,7 +38,7 @@ export function resolveStdioMcpServerLaunchConfig(raw: unknown): StdioMcpServerL | |||
| config: { | |||
| command: raw.command, | |||
| args: toMcpStringArray(raw.args), | |||
| env: toMcpStringRecord(raw.env), | |||
| env: toMcpEnvRecord(raw.env, { onDroppedEntry: options?.onDroppedEnv }), | |||
There was a problem hiding this comment.
Scope dangerous env filtering to MCP servers only
This change applies toMcpEnvRecord inside resolveStdioMcpServerLaunchConfig, which is also used by the bundle LSP runtime (src/agents/pi-bundle-lsp-runtime.ts:321). As a result, .lsp.json server env keys like PYTHONPATH/LD_LIBRARY_PATH are now stripped by the MCP denylist, and in the LSP path they are dropped silently because no onDroppedEnv callback is passed. That creates an unintended cross-surface regression where previously working LSP servers can fail to start or locate dependencies even though this PR is scoped to MCP transport hardening.
Useful? React with 👍 / 👎.
959ebaa to
c71ddb9
Compare
c71ddb9 to
760d753
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 760d753d16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -35,7 +38,7 @@ export function resolveStdioMcpServerLaunchConfig(raw: unknown): StdioMcpServerL | |||
| config: { | |||
| command: raw.command, | |||
| args: toMcpStringArray(raw.args), | |||
| env: toMcpStringRecord(raw.env), | |||
| env: toMcpEnvRecord(raw.env, { onDroppedEntry: options?.onDroppedEnv }), | |||
There was a problem hiding this comment.
Limit stdio env denylist to MCP transport resolution
resolveStdioMcpServerLaunchConfig now always routes raw.env through toMcpEnvRecord, so every caller gets host-env denylist filtering, not just MCP transport parsing. The bundle LSP runtime still uses this same resolver (src/agents/pi-bundle-lsp-runtime.ts:321) for .lsp.json servers, which means LSP env keys are now silently removed and can break servers that rely on startup paths (for example PYTHONPATH/LD_LIBRARY_PATH). Fresh evidence in this commit: toMcpEnvRecord switched to isDangerousHostEnvVarName, and the current host policy marks PYTHONPATH and LD_ as blocked, so this regression remains even after allowing credential/proxy vars.
Useful? React with 👍 / 👎.
760d753 to
0226421
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02264216ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -35,7 +38,7 @@ export function resolveStdioMcpServerLaunchConfig(raw: unknown): StdioMcpServerL | |||
| config: { | |||
| command: raw.command, | |||
| args: toMcpStringArray(raw.args), | |||
| env: toMcpStringRecord(raw.env), | |||
| env: toMcpEnvRecord(raw.env, { onDroppedEntry: options?.onDroppedEnv }), | |||
There was a problem hiding this comment.
Scope stdio env filtering to MCP transport path
resolveStdioMcpServerLaunchConfig is shared by both MCP transport resolution and bundle LSP startup, so applying toMcpEnvRecord here strips env keys for .lsp.json servers too. In the current host policy, PYTHONPATH is blocked explicitly and LD_LIBRARY_PATH is blocked via the LD_ prefix, so LSP servers that depend on those variables can fail to start or resolve runtime dependencies; this regression is also silent in the LSP path because pi-bundle-lsp-runtime calls this resolver without onDroppedEnv logging. Fresh evidence: this commit switched this shared resolver from toMcpStringRecord to toMcpEnvRecord at the call site below.
Useful? React with 👍 / 👎.
0226421 to
f49bf72
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f49bf72963
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -35,7 +38,7 @@ export function resolveStdioMcpServerLaunchConfig(raw: unknown): StdioMcpServerL | |||
| config: { | |||
| command: raw.command, | |||
| args: toMcpStringArray(raw.args), | |||
| env: toMcpStringRecord(raw.env), | |||
| env: toMcpEnvRecord(raw.env, { onDroppedEntry: options?.onDroppedEnv }), | |||
There was a problem hiding this comment.
Apply dangerous-env filtering only in MCP transport path
resolveStdioMcpServerLaunchConfig is shared by MCP transport parsing and bundle LSP startup, so switching this resolver to toMcpEnvRecord strips env keys for .lsp.json servers as well. Fresh evidence in this commit is the new call at src/agents/mcp-stdio.ts:41; that resolver is still used by src/agents/pi-bundle-lsp-runtime.ts:321 with no MCP-only guard. Since isDangerousHostEnvVarName blocks runtime-path keys such as PYTHONPATH and LD_*, LSP servers that depend on those variables can fail to start or resolve dependencies, which is a regression outside the intended MCP hardening scope.
Useful? React with 👍 / 👎.
f49bf72 to
54fbe30
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54fbe30014
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onDroppedEnv: (key) => { | ||
| logWarn( | ||
| `bundle-mcp: server "${serverName}": env "${key}" is blocked for stdio startup safety and was ignored.`, | ||
| ); |
There was a problem hiding this comment.
Distinguish blocked env keys from malformed env values
resolveMcpTransportConfig now logs every dropped stdio env entry as "blocked for stdio startup safety", but toMcpEnvRecord calls the same drop callback for both dangerous keys and unsupported value types (see toMcpFilteredStringRecord branches in src/agents/mcp-config-shared.ts). With configs like env: { SAFE_VAR: { nested: true } }, the value is dropped because its type is invalid, yet the warning claims a security block, which misdiagnoses user config errors and makes MCP setup debugging materially harder.
Useful? React with 👍 / 👎.
|
Thanks @drobison00, landed in 85d86eb. Validation before merge:
I also reviewed the post-merge security notes. The empty-env concern is covered by the MCP SDK behavior: |
|
Appreciate the hardening intent; flagging one legitimate use case that this denylist as written would break silently. When an MCP stdio server runs as a child of a gateway deployed behind an HTTP CONNECT proxy with an MITM TLS re-signing CA, the child's Node runtime needs:
Without both, stdio MCP servers silently fail TLS handshake on first outbound request; cached access tokens can mask the failure until refresh. The ops-side gap behind this (TLS handshake gap for MCP stdio under proxy) is tracked at NVIDIA/OpenShell#886. The current denylist removes
Happy to send a patch for either approach. One additional nit: the failure mode post-denylist is silent — the child just fails with |
* 'main' of https://github.com/openclaw/openclaw: (653 commits) docs(changelog): deduplicate openclaw#67800 entries in Unreleased (openclaw#69670) fix(agents): honor explicit long Anthropic cache TTL on custom hosts (openclaw#67800) fix: fix Telegram media file delivery (openclaw#69641) fix(media): preserve outbound attachment filenames fix(media): parse lowercase media directives fix(bluebubbles): add opt-in coalesceSameSenderDms for split-send DMs (openclaw#69258) fix: centralize provider thinking profiles docs: prepare 2026.4.20 changelog fix: stage ACP and Codex runtime deps fix(gateway): drop stale service env on reinstall test: add bundled channel dependency Docker smoke test: relax detached task recovery timing assertion fix: ignore placeholder shells in runtime detection (openclaw#69308) shell: fall back to sh when SHELL is /usr/bin/false or nologin fix(browser): clarify DevToolsActivePort attach failures fix: sanitize mcp transport warning fields fix: launch Windows startup gateway directly fix(openai-codex): normalize legacy copilot transport fix: narrow MCP stdio env safety filter (openclaw#69540) fix(mcp): block dangerous stdio env overrides ...
Summary
Describe the problem and fix in 2–5 bullets:
If this PR fixes a plugin beta-release blocker, title it
fix(<plugin-id>): beta blocker - <summary>and link the matchingBeta blocker: <plugin-name> - <summary>issue labeledbeta-blocker. Contributors cannot label PRs, so the title is the PR-side signal for maintainers and automation.NODE_OPTIONS,LD_PRELOAD, andBASH_ENVcan execute code before the intended MCP server logic starts.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write
N/A. If the cause is unclear, writeUnknown.Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should catch this. Otherwise write
N/A.src/agents/mcp-transport-config.test.tsUser-visible / Behavior Changes
NODE_OPTIONS,LD_PRELOAD, andBASH_ENV.Diagram (if applicable)
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) YesYes/No) NoYes, explain risk + mitigation: MCP subprocess launches no longer honor dangerous interpreter-startup env keys from config. The change narrows subprocess startup behavior by reusing the existing host-env denylist while preserving safe env entries and leaving HTTP header parsing unchanged.Repro + Verification
Environment
Steps
NODE_OPTIONS,LD_PRELOAD, andBASH_ENV.resolveMcpTransportConfig().Expected
Actual
Evidence
Attach at least one:
Validation run:
node scripts/run-vitest.mjs run --config test/vitest/vitest.unit.config.ts src/agents/mcp-transport-config.test.ts(pass)Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) NoYes/No) NoYes/No) YesRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.