Pass all environment variables to MCP server instead of just PATH #8602
Conversation
|
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="extensions/cli/src/services/MCPService.ts">
<violation number="1" location="extensions/cli/src/services/MCPService.ts:513">
This change introduces a severe security vulnerability. By passing all `process.env` variables to `StdioMcpServer` child processes, it allows potentially malicious configuration files (loaded via `--config`) to execute arbitrary commands with access to all secrets in the user's environment. The previous behavior of only passing `PATH` was safer. This change makes it possible for a malicious project to steal a user's API keys and other credentials.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| if (process.env.PATH !== undefined) { | ||
| env.PATH = process.env.PATH; | ||
| if (process.env) { | ||
| for (const [key, value] of Object.entries(process.env)) { |
There was a problem hiding this comment.
This change introduces a severe security vulnerability. By passing all process.env variables to StdioMcpServer child processes, it allows potentially malicious configuration files (loaded via --config) to execute arbitrary commands with access to all secrets in the user's environment. The previous behavior of only passing PATH was safer. This change makes it possible for a malicious project to steal a user's API keys and other credentials.
Prompt for AI agents
Address the following comment on extensions/cli/src/services/MCPService.ts at line 513:
<comment>This change introduces a severe security vulnerability. By passing all `process.env` variables to `StdioMcpServer` child processes, it allows potentially malicious configuration files (loaded via `--config`) to execute arbitrary commands with access to all secrets in the user's environment. The previous behavior of only passing `PATH` was safer. This change makes it possible for a malicious project to steal a user's API keys and other credentials.</comment>
<file context>
@@ -509,8 +509,12 @@ export class MCPService
- if (process.env.PATH !== undefined) {
- env.PATH = process.env.PATH;
+ if (process.env) {
+ for (const [key, value] of Object.entries(process.env)) {
+ if (!(key in env) && !!value) {
+ env[key] = value;
</file context>
|
🎉 This PR is included in version 1.32.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.29.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by cubic
Pass all environment variables to MCP servers instead of only PATH to improve compatibility and reduce startup failures. We merge process.env into serverConfig.env, keep explicit overrides, and skip empty values.
Written for commit 1aa83f2. Summary will update automatically on new commits.