Repository navigation
fix: restrict the stdio client server environment - #665
Conversation
9c09ac4 to
17f1dd9
Compare
17f1dd9 to
c5f69d5
Compare
| # 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 |
There was a problem hiding this comment.
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:.
There was a problem hiding this comment.
And, It might be better to extract this into a constant.
There was a problem hiding this comment.
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.
|
|
||
| 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`. |
There was a problem hiding this comment.
Drop BUNDLE_GEMFILE here, in the README sentence and in the test expectation as well.
There was a problem hiding this comment.
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:.
| %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 |
There was a problem hiding this comment.
It might be better to extract this into a constant.
Ditto.
There was a problem hiding this comment.
The Unix list is now the frozen UNIX_INHERITED_ENV_VARS constant. DEFAULT_INHERITED_ENV_VARS selects the appropriate named platform list.
c5f69d5 to
25d3f95
Compare
Motivation and Context
Client::Stdiopassed the whole parent environment to the server process, becauseOpen3.popen3mergesenv: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, withGEM_HOMEandGEM_PATHadded for Ruby), withenv:merged over it. Explicit values override inherited ones, andnilremoves one.BUNDLE_GEMFILEis excluded from the defaults because it selects an application's Gemfile. A caller can pass it throughenv:.inherit_env: truerestores full inheritance. I used Ruby'sunsetenv_others: trueso that Ruby enforces the boundary when it spawns the process. README and the transport docs describe the new default.Spec and reference SDK checks:
DEFAULT_INHERITED_ENV_VARSandgetDefaultEnvironment: "Environment variables to inherit by default, if an environment is not explicitly given." Its spawn code merges...getDefaultEnvironment(), ...this._serverParams.env, so an explicit caller value remains accepted.envdocumentation says: "Extra environment variables, merged over get_default_environment()."Neither reference SDK includes
BUNDLE_GEMFILEin its defaults. Our platform base follows Python's; TypeScript's Windows list is broader. Ruby addsGEM_HOMEandGEM_PATHso 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 explicitenv:, the base values, RubyGems paths, default omission and explicit selection ofBUNDLE_GEMFILE, case-insensitive lookup as on Windows, override and removal, andinherit_env: true. The default-isolation cases fail onmain. 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.rbhas 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_HOMEandGEM_PATHon Unix and Windows. Pass other variables a server needs throughenv:, or setinherit_env: trueto keep the old behavior.Types of changes
Checklist