-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix: use common process.env values to launch MCP #7709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2425a83 to
c660cb5
Compare
core/context/mcp/MCPConnection.ts
Outdated
| const env = { | ||
| ...processEnv, | ||
| ...(options.transport.env ?? {}), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to pass every environment variable to the subprocess or just a few important ones?
|
@fbricon @panyamkeerthana merging this has been delayed because of security concerns (i.e. @panyamkeerthana's comment). Just want to reduce the chance a rogue MCP goes haywire on users. I'd +1 to getting 80% of the way there with HOME and other common vars that are usually safe @fbricon would any vars specifically fix your case? Would that be transferable to most users? |
|
@fbricon wondering if just need to add
|
Signed-off-by: Fred Bricon <[email protected]>
c660cb5 to
b1f147c
Compare
|
I switched to using common ENV vars. That should be enough for most MCPs hopefully. |
…bricon/fix-mcp-env
…bricon/fix-mcp-env
|
@fbricon I have a feeling that eventually we'll end up just doing your original full pass through implementation, but while we wrap our heads around MCP security this should work. Thanks for the contribution |
|
🎉 This PR is included in version 1.27.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.24.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
My nodejs versions are managed by asdf. Continue fails to resolve npx to launch MCPs because it doesn't pass the proper system environment variables (basically, just process.env.PATH is passed):
The proposed fix merges options.transport.env with all process.env variables before launching the MCP. I can't see a reason why other env vars would be ignored by default. But, if that is an issue, we could get away with adding env.HOME and maybe a couple other variables.
Summary by cubic
Fixes MCP launch failures caused by missing env vars. MCP processes now inherit the full process.env, so tools like npx resolve correctly with asdf and similar managers.