Skip to content

fix: restrict the stdio client server environment - #665

Merged
koic merged 1 commit into
modelcontextprotocol:mainfrom
atesgoral:fix/stdio-client-environment
Nov 8, 2026
Merged

koic merged 1 commit into
modelcontextprotocol:mainfrom
atesgoral:fix/stdio-client-environment

Conversation

@atesgoral

@atesgoral atesgoral commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Motivation and Context

Client::Stdio passed the whole parent environment to the server process, because Open3.popen3 merges env: into it. That includes variables the server has no use for. The server process now starts with a small base environment (an OS-specific allow-list based on the Python SDK lists, with GEM_HOME and GEM_PATH added for Ruby), with env: merged over it. Explicit values override inherited ones, and nil removes one. BUNDLE_GEMFILE is excluded from the defaults because it selects an application's Gemfile. A caller can pass it through env:. inherit_env: true restores full inheritance. I used Ruby's unsetenv_others: true so that Ruby enforces the boundary when it spawns the process. README and the transport docs describe the new default.

Spec and reference SDK checks:

Neither reference SDK includes BUNDLE_GEMFILE in its defaults. Our platform base follows Python's; TypeScript's Windows list is broader. Ruby adds GEM_HOME and GEM_PATH so RubyGems can find nondefault gem locations. Both platform arrays are now named frozen constants.

Fixes #662.

How Has This Been Tested?

test/mcp/client/stdio_environment_test.rb (new) starts real subprocesses. It covers omitted, empty and explicit env:, the base values, RubyGems paths, default omission and explicit selection of BUNDLE_GEMFILE, case-insensitive lookup as on Windows, override and removal, and inherit_env: true. The default-isolation cases fail on main. The Gemfile-omission regression fails on the previous published implementation. Explicit Gemfile selection and RubyGems-path inheritance pass there and remain covered. test/mcp/client/stdio_test.rb has a small update.

The stdio tests pass (69 runs, 290 assertions), and so does the full suite (2,513 runs, 8,280 assertions). RuboCop is clean on the changed files. I did not run it on Windows.

Breaking Changes

Yes. The default changes: the server process no longer inherits the full parent environment. The inherited base includes GEM_HOME and GEM_PATH on Unix and Windows. Pass other variables a server needs through env:, or set inherit_env: true to keep the old behavior.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@atesgoral
atesgoral force-pushed the fix/stdio-client-environment branch from 9c09ac4 to 17f1dd9 Compare November 6, 2026 06:34
@atesgoral
atesgoral marked this pull request as ready for review November 6, 2026 06:50
@atesgoral
atesgoral marked this pull request as draft November 6, 2026 07:54
@atesgoral
atesgoral force-pushed the fix/stdio-client-environment branch from 17f1dd9 to c5f69d5 Compare November 6, 2026 13:59
@atesgoral
atesgoral marked this pull request as ready for review November 6, 2026 14:15
Comment thread lib/mcp/client/stdio.rb Outdated
# Base process environment plus RubyGems and Bundler configuration.
DEFAULT_INHERITED_ENV_VARS = if Gem.win_platform?
%w[APPDATA HOMEDRIVE HOMEPATH LOCALAPPDATA PATH PATHEXT PROCESSOR_ARCHITECTURE
SYSTEMDRIVE SYSTEMROOT TEMP USERNAME USERPROFILE GEM_HOME GEM_PATH BUNDLE_GEMFILE].freeze

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUNDLE_GEMFILE should not be inherited by default. Under bundle exec it names the client application's Gemfile, so a server from another project started with bundle exec resolves its gems against the client's Gemfile and loads the wrong versions or fails to start; with RUBYOPT, RUBYLIB and BUNDLE_BIN_PATH not inherited, the Bundler state is partial as well. GEM_HOME and GEM_PATH are fine to keep. A caller that wants the same Gemfile can pass it through env:.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, It might be better to extract this into a constant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUNDLE_GEMFILE is no longer in either default list. GEM_HOME and GEM_PATH stay inherited, and a caller can supply a Gemfile through env:. I also extracted the Windows list into the frozen WINDOWS_INHERITED_ENV_VARS constant.

Comment thread docs/_client/transports.md Outdated

By default, the server inherits only these variables that are present in the parent environment:

- Unix: `HOME`, `LOGNAME`, `PATH`, `SHELL`, `TERM`, `USER`, `GEM_HOME`, `GEM_PATH`, `BUNDLE_GEMFILE`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop BUNDLE_GEMFILE here, in the README sentence and in the test expectation as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed BUNDLE_GEMFILE from the README, both documented default lists and the default-inheritance expectation. Added regression tests for default omission and explicit selection through env:.

Comment thread lib/mcp/client/stdio.rb Outdated
%w[APPDATA HOMEDRIVE HOMEPATH LOCALAPPDATA PATH PATHEXT PROCESSOR_ARCHITECTURE
SYSTEMDRIVE SYSTEMROOT TEMP USERNAME USERPROFILE GEM_HOME GEM_PATH BUNDLE_GEMFILE].freeze
else
%w[HOME LOGNAME PATH SHELL TERM USER GEM_HOME GEM_PATH BUNDLE_GEMFILE].freeze

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to extract this into a constant.

Ditto.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Unix list is now the frozen UNIX_INHERITED_ENV_VARS constant. DEFAULT_INHERITED_ENV_VARS selects the appropriate named platform list.

@atesgoral
atesgoral marked this pull request as draft November 7, 2026 15:18
@atesgoral
atesgoral force-pushed the fix/stdio-client-environment branch from c5f69d5 to 25d3f95 Compare November 7, 2026 15:30
@atesgoral
atesgoral marked this pull request as ready for review November 7, 2026 15:56
@atesgoral
atesgoral requested a review from koic November 7, 2026 20:06
@koic
koic merged commit e197191 into modelcontextprotocol:main Nov 8, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client::Stdio passes the whole parent environment to the server process

2 participants