fix(mcp): add environment variable support for stdio MCP servers #62

sarman · 2026-06-01T18:21:39Z

sarman commented

2026-06-01 18:21:39 +00:00

Description:

MCP servers that require environment variables at startup (e.g. GitHub MCP server which needs
GITHUB_PERSONAL_ACCESS_TOKEN) were failing with "connection closed: initialize response" because the stdio transport had
no mechanism to pass env vars to the spawned process.

Changes:

Database: Added env_config column (migration 023) for storing encrypted environment variables per MCP server
Backend: Plaintext env vars stored in transport_config.env; sensitive env vars encrypted with AES-256-GCM in env_config.
Both are merged at discovery time (encrypted takes precedence). Dangerous privilege-escalation vars (LD_PRELOAD,
DYLD_INSERT_LIBRARIES, etc.) are rejected
HTTP transport: Custom headers parsed from transport_config.headers (not yet applied — rmcp v1.7.0 limitation noted in
logs)
Frontend: Two new input fields on stdio servers — plaintext env (visible) and encrypted env (masked). HTTP servers get
an optional custom headers field
Resources optional: resources/list "Method not found" errors are now treated as non-fatal since many servers (including
GitHub MCP) don't implement the optional resources capability

Testing:

All 242 existing tests pass plus 15 new unit tests covering encryption at rest, update/clear/preserve semantics,
dangerous var rejection, and env var parsing
Manually verified: GitHub MCP server connects, initializes, and tools are listed when PATH and
GITHUB_PERSONAL_ACCESS_TOKEN are provided

Description: MCP servers that require environment variables at startup (e.g. GitHub MCP server which needs GITHUB_PERSONAL_ACCESS_TOKEN) were failing with "connection closed: initialize response" because the stdio transport had no mechanism to pass env vars to the spawned process. Changes: - Database: Added env_config column (migration 023) for storing encrypted environment variables per MCP server - Backend: Plaintext env vars stored in transport_config.env; sensitive env vars encrypted with AES-256-GCM in env_config. Both are merged at discovery time (encrypted takes precedence). Dangerous privilege-escalation vars (LD_PRELOAD, DYLD_INSERT_LIBRARIES, etc.) are rejected - HTTP transport: Custom headers parsed from transport_config.headers (not yet applied — rmcp v1.7.0 limitation noted in logs) - Frontend: Two new input fields on stdio servers — plaintext env (visible) and encrypted env (masked). HTTP servers get an optional custom headers field - Resources optional: resources/list "Method not found" errors are now treated as non-fatal since many servers (including GitHub MCP) don't implement the optional resources capability Testing: - All 242 existing tests pass plus 15 new unit tests covering encryption at rest, update/clear/preserve semantics, dangerous var rejection, and env var parsing - Manually verified: GitHub MCP server connects, initializes, and tools are listed when PATH and GITHUB_PERSONAL_ACCESS_TOKEN are provided

sarman added 2 commits 2026-06-01 18:21:40 +00:00

fix(mcp): treat missing resources/list as non-fatal for servers that don't implement it 7cc4f0f689

docs(wiki): update MCP-Servers.md with env var support, PATH requirement, and new schema column

Test / rust-fmt-check (pull_request) Successful in 1m24s

Details

Test / frontend-tests (pull_request) Successful in 1m40s

Details

Test / frontend-typecheck (pull_request) Successful in 1m44s

Details

Test / rust-clippy (pull_request) Successful in 3m13s

Details

Test / rust-tests (pull_request) Successful in 4m35s

Details

PR Review Automation / review (pull_request) Successful in 4m41s

Details

e5d3ff42f5

sarman reviewed 2026-06-01 18:26:16 +00:00

sarman left a comment

Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR adds environment variable support for stdio MCP servers, enhancing security by encrypting sensitive env vars and merging them with plaintext ones. However, it introduces a critical logic flaw where the environment variable merging incorrectly overwrites encrypted values with plaintext values instead of the intended precedence.

Findings

[BLOCKER] src-tauri/src/mcp/discovery.rs:86-93 - Environment variable precedence is inverted: plaintext values overwrite encrypted values instead of the reverse.
Evidence:

// Decrypt and parse encrypted env vars from env_config column
let encrypted_env = {
    let db = state.db.lock().map_err(|e| e.to_string())?;
    get_server_env_config(&db, &server.id)?
};

// Merge env vars (encrypted takes precedence over plaintext)
let mut merged_env = plaintext_env;
if let Some(enc_env) = encrypted_env {
    merged_env.extend(enc_env);
}

Fix: Swap the merge logic so encrypted values overwrite plaintext ones:

let mut merged_env = encrypted_env.unwrap_or_default();
merged_env.extend(plaintext_env);

(Per comment and docs, encrypted values must take precedence.)

Verdict: REQUEST CHANGES

Automated PR Review (qwen3-coder-next via liteLLM):\n\n**Summary** The PR adds environment variable support for stdio MCP servers, enhancing security by encrypting sensitive env vars and merging them with plaintext ones. However, it introduces a critical logic flaw where the environment variable merging incorrectly overwrites encrypted values with plaintext values instead of the intended precedence. **Findings** - [BLOCKER] src-tauri/src/mcp/discovery.rs:86-93 - Environment variable precedence is inverted: plaintext values overwrite encrypted values instead of the reverse. Evidence: ```rust // Decrypt and parse encrypted env vars from env_config column let encrypted_env = { let db = state.db.lock().map_err(|e| e.to_string())?; get_server_env_config(&db, &server.id)? }; // Merge env vars (encrypted takes precedence over plaintext) let mut merged_env = plaintext_env; if let Some(enc_env) = encrypted_env { merged_env.extend(enc_env); } ``` Fix: Swap the merge logic so encrypted values overwrite plaintext ones: ```rust let mut merged_env = encrypted_env.unwrap_or_default(); merged_env.extend(plaintext_env); ``` (Per comment and docs, encrypted values must take precedence.) **Verdict**: REQUEST CHANGES

sarman merged commit 590fec7dd4 into master

2026-06-01 18:27:59 +00:00

sarman deleted branch bug/mcp-env-vars-support

2026-06-01 18:27:59 +00:00

sarman referenced this issue from a commit

2026-06-01 18:28:00 +00:00

Merge pull request 'fix(mcp): add environment variable support for stdio MCP servers' (#62) from bug/mcp-env-vars-support into master

Sign in to join this conversation.

No reviewers