From 54b0537af9a98658edc87f1fd0b6fedad4e87a05 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 15 Sep 2026 18:24:16 +0000 Subject: [PATCH] Let MCP registry env override inherited parent keys glibc/macOS getenv uses the first KEY= in environ. Appending env_extra after a scrubbed parent copy left PATH/NODE_ENV/etc. stuck on the parent value. Skip inherited keys that env_extra sets so the registry value is the only copy. Co-authored-by: Tyler Reckart --- CHANGELOG.md | 8 ++++++++ docs/concepts/mcp.md | 2 +- include/mcp/subprocess.h | 8 +++++--- src/mcp/subprocess.cpp | 26 +++++++++++++++++++++++--- tests/test_mcp.cpp | 15 +++++++++++++++ 5 files changed, 52 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dae12b43..66c5b630 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ loosely while pre-1.0 (breaking changes can land on minor bumps). ## [Unreleased] +### Fixed +- **MCP registry `env` overrides parent keys.** Subprocess spawn skipped + secret-shaped parent keys then appended `env_extra`, leaving duplicate + `KEY=` entries. glibc/macOS `getenv` (and `execvp` PATH search) use the + first match, so registry `PATH` / `NODE_ENV` / `HTTP_PROXY` / non-secret + keys were ignored. Inherited keys that `env` sets are now omitted so + the registry value is the only copy. + ## [0.13.4] — 2026-09-10 ### Fixed diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index ca10ea6b..9e970179 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -34,7 +34,7 @@ Configure available servers via `~/.arbiter/mcp_servers.json` (path comes from ` |-------------------|----------------|----------|-------------| | `command` | string | yes | Executable to run. PATH-resolved via `execvp`. | | `args` | array | no | Arguments after `command`. | -| `env` | object | no | Extra `KEY: "VALUE"` strings appended to the parent environment. | +| `env` | object | no | Extra `KEY: "VALUE"` strings merged into the child environment. Same-name keys override the parent (secret-shaped parent keys are stripped; registry secrets are still passed through). | | `init_timeout_ms` | int | no | Wall-clock budget for the JSON-RPC `initialize` handshake. Defaults to 60s — first-run `npx` may install. | | `call_timeout_ms` | int | no | Per-`tools/call` timeout. Defaults to 30s. Playwright snapshots/navigation routinely take 5–15s. | diff --git a/include/mcp/subprocess.h b/include/mcp/subprocess.h index 80a6b9bc..2bdcce3f 100644 --- a/include/mcp/subprocess.h +++ b/include/mcp/subprocess.h @@ -29,9 +29,11 @@ constexpr size_t kReadBufMaxBytes = 256 * 1024; class Subprocess { public: // Spawn `argv[0]` as the executable, with the remaining args. `env_extra` - // is appended after a scrubbed copy of the parent environment (secret- - // shaped keys like *_API_KEY / ARBITER_* are stripped). Useful for - // passing PLAYWRIGHT_BROWSERS_PATH etc. Throws std::runtime_error on + // is applied after a scrubbed copy of the parent environment (secret- + // shaped keys like *_API_KEY / ARBITER_* are stripped). Extra keys + // override inherited parent keys of the same name so getenv sees the + // registry value (glibc first-match). Useful for PATH, NODE_ENV, + // PLAYWRIGHT_BROWSERS_PATH, etc. Throws std::runtime_error on // fork/exec failure (the child's exec failure is detected by an immediate // EOF on stdout, which manifests as recv_line returning std::nullopt // before any data — caller treats that as a startup failure). diff --git a/src/mcp/subprocess.cpp b/src/mcp/subprocess.cpp index 9ed58792..51d82c23 100644 --- a/src/mcp/subprocess.cpp +++ b/src/mcp/subprocess.cpp @@ -60,8 +60,11 @@ void purge_stdio_buffers() { } // True when an inherited env KEY should be stripped from MCP children. -// Registry `env_extra` is appended after the scrub and is allowed through +// Registry `env_extra` is applied after the scrub and is allowed through // even when the key would otherwise match — operators opt into those. +// Extra keys replace inherited parent keys of the same name so getenv / +// execvp PATH search / Node process.env see the registry value rather +// than a duplicate later in environ (glibc getenv is first-match). bool is_secret_env_key(std::string_view key) { if (key.empty()) return true; auto has_suffix = [&](std::string_view suf) { @@ -140,20 +143,37 @@ Subprocess::Subprocess(const std::vector& argv, purge_stdio_buffers(); - // Inherit parent environ minus secret-shaped keys, then append + // Inherit parent environ minus secret-shaped keys, then apply // registry env_extra. MCP children (often untrusted npx packages) // must not see provider/admin credentials from the arbiter process. + // Extra keys override inherited ones: glibc/macOS getenv and + // execvp's PATH search use the first KEY= in environ, so appending + // a duplicate left PATH / NODE_ENV / HTTP_PROXY / etc. stuck on + // the parent value. Skip parent keys that env_extra sets, then + // append extra (secrets in extra still pass — operator opt-in). // Done this way (rather than execvpe) because macOS doesn't expose // execvpe and we need the PATH-search behaviour for `npx`. + auto extra_key = [](const std::string& s) -> std::string_view { + const auto eq = s.find('='); + return eq == std::string::npos ? std::string_view(s) + : std::string_view(s.data(), eq); + }; + auto extra_has_key = [&](std::string_view key) { + for (const auto& s : env_extra) { + if (extra_key(s) == key) return true; + } + return false; + }; std::vector env_storage; for (char** e = environ; e && *e; ++e) { const char* eq = std::strchr(*e, '='); std::string_view key = eq ? std::string_view(*e, static_cast(eq - *e)) : std::string_view(*e); if (is_secret_env_key(key)) continue; + if (extra_has_key(key)) continue; env_storage.emplace_back(*e); } - for (auto& s : env_extra) env_storage.push_back(s); + for (const auto& s : env_extra) env_storage.push_back(s); std::vector envp; envp.reserve(env_storage.size() + 1); for (auto& s : env_storage) envp.push_back(s.data()); diff --git a/tests/test_mcp.cpp b/tests/test_mcp.cpp index 590f9349..654f5677 100644 --- a/tests/test_mcp.cpp +++ b/tests/test_mcp.cpp @@ -415,6 +415,21 @@ TEST_CASE("Subprocess: strips secret-shaped parent env; keeps env_extra") { ::unsetenv("ARBITER_ADMIN_TOKEN"); } +TEST_CASE("Subprocess: env_extra overrides inherited parent keys") { + // Non-secret parent keys used to stay first in environ; getenv and + // `printenv KEY` then returned the parent value despite env_extra. + ::setenv("MCP_ENV_OVERRIDE_PROBE", "from-parent", 1); + Subprocess proc({"/usr/bin/printenv", "MCP_ENV_OVERRIDE_PROBE"}, + {"MCP_ENV_OVERRIDE_PROBE=from-registry"}); + auto line = proc.recv_line(500ms); + REQUIRE(line.has_value()); + CHECK(*line == "from-registry"); + // No leftover parent copy for programs that iterate environ. + auto extra = proc.recv_line(200ms); + CHECK_FALSE(extra.has_value()); + ::unsetenv("MCP_ENV_OVERRIDE_PROBE"); +} + // ── 3. /mcp slash dispatch ───────────────────────────────────────── TEST_CASE("parse_agent_commands recognises /mcp") {