bug/mcp-env-vars-support #61
No reviewers
Labels
No Label
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sarman/tftsr-devops_investigation#61
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "bug/mcp-env-vars-support"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
⏺ Summary
Backend Changes (Rust)
Database & Models
Storage Layer
Discovery & Transport
Tests
Frontend Changes (TypeScript/React)
Type Definitions
UI Components
Security Considerations
Backward Compatibility
Known Limitations
HTTP custom headers not yet applied: rmcp v1.7.0's StreamableHttpClientTransportConfig lacks a .header() method. Headers are parsed and passed through but logged as unsupported. This will be addressed when rmcp adds header support or in a future
release with an alternative approach.
Testing
Unit Tests (Rust)
cargo test --manifest-path src-tauri/Cargo.toml mcp
cargo test --manifest-path src-tauri/Cargo.toml migrations
Frontend Type Checking
npx tsc --noEmit
npm run build
End-to-End Testing
stdio transport (GitHub MCP):
- Name: GitHub
- Transport: stdio
- Command: /opt/homebrew/bin/npx (or /usr/local/bin/npx)
- Args: -y @modelcontextprotocol/server-github
- Encrypted env: GITHUB_PERSONAL_ACCESS_TOKEN=ghp_yourtoken
Verification:
Fixes
Fixes inability to configure MCP servers that require environment variables (e.g., GitHub MCP server with GITHUB_PERSONAL_ACCESS_TOKEN). Previously, stdio transport only supported command + args with no mechanism to pass env vars to spawned processes.
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR adds support for environment variables and custom headers for MCP servers, including encrypted storage for sensitive values and UI forms to input them. A critical bug is found in the stdio transport where environment variables are merged incorrectly—plaintext env vars override encrypted env vars instead of vice versa. Additionally, the frontend form uses
type="password"for environment variable inputs despite no actual sensitive masking logic and exposes encrypted values via plaintext parsing.Findings
[BLOCKER] src-tauri/src/mcp/discovery.rs:101–106 - Environment variable merging logic is inverted; plaintext env vars overwrite encrypted ones, ignoring their precedence.
Evidence:
Fix: Reverse the merge order: start with
encrypted_envand extend withplaintext_env, or swap the precedence logic. Current comment says encrypted takes precedence, butextend()causes plaintext values to overwrite encrypted values.[WARNING] src-tauri/src/mcp/transport/http.rs:13–18 - Custom HTTP headers are logged as unsupported but still parsed and ignored; misleading UX and potential future breakage.
Evidence:
Fix: Either implement header support, document it as intentional technical debt, or remove the field from the form/transport config to avoid confusion.
[WARNING] src/pages/Settings/MCPServers.tsx:226–228 – Environment variable inputs use
type="password"despite lack of sensitive masking logic and inconsistent security.Evidence:
Fix: Replace
type="password"withtype="text"ortype="search"and clarify UX with placeholder hints or helper text to avoid misleading users.[WARNING] src/pages/Settings/MCPServers.tsx:309–312 – Encrypted env config is never shown (good), but
encrypted_envis populated only fromparseTransportConfig, which is never used for encrypted env; misleading empty field during edit.Evidence:
However, during edit,
encrypted_envis reset to empty, which may confuse users into thinking they must re-enter sensitive env vars.Fix: Clarify in UI (e.g., disabled/read-only placeholder like “Encrypted values are stored securely and not shown”) or remove the field entirely and only show “Secure Environment Variables” on creation.
[WARNING] src-tauri/src/mcp/store.rs:227–228 –
get_server_env_configfails to handle empty JSON string correctly;encrypt_token("")throws and is not caught.Evidence:
But in
update_server,req.env_config: Some("".to_string())is explicitly used to clear env_config, andencrypt_token("")would cause failure if not guarded. Guard appears correct in creation, but verifyget_server_env_confighandles empty decrypted string:serde_json::from_str("")fails withError::EndOfStream.Evidence:
Fix: Guard
serde_json::from_str("")by rejecting empty decrypted string early (e.g., treat as None), and ensureUpdateMcpServerRequestusesNoneto clear.Verdict: REQUEST CHANGES
I've verified all the claims. Here's my assessment:
✅ CLAIM 1 IS INCORRECT - Env merging logic is CORRECT
The reviewer claims that HashMap::extend() causes plaintext to overwrite encrypted values. This is FALSE. I tested this with actual Rust code:
let mut base = HashMap::from([("SHARED", "plaintext")]);
let encrypted = HashMap::from([("SHARED", "encrypted")]);
base.extend(encrypted);
// Result: SHARED = "encrypted" ✅
HashMap::extend() overwrites existing keys with values from the source, so encrypted DOES take precedence. The code at lines 78-81 is correct.
⚠️ CLAIM 2 IS VALID - HTTP headers are not applied
The warning log is accurate - rmcp v1.7.0 doesn't support custom headers. This is documented in the PR description under "Known Limitations". The headers are parsed for future use but not currently applied.
Recommendation: Accept as-is since it's documented technical debt. The warning prevents silent failures.
❌ CLAIM 3 IS MISLEADING - Password input is intentional
Using type="password" for environment variables is correct for security. It prevents shoulder-surfing and screen captures from exposing tokens. The reviewer incorrectly assumes password fields require additional masking logic - they don't, the browser
handles masking.
Recommendation: Keep as-is. This is a security best practice.
⚠️ CLAIM 4 IS VALID - UX could be clearer
The encrypted_env field being empty during edit is intentional (security: never show decrypted values), but the UX could be clearer about why it's blank.
Recommendation: Add helper text or placeholder to clarify this is intentional.
✅ CLAIM 5 IS INCORRECT - Empty string handling is correct
The code at lines 14-17 in update_server explicitly handles empty strings:
Some(env_json) if !env_json.trim().is_empty() => Some(encrypt_token(env_json)?),
Some(_) => None, // Empty string = clear env_config
Empty strings are never passed to encrypt_token(). The get_server_env_config function only receives already-encrypted values from the DB, never empty strings.
Recommendation: No fix needed.
Verdict: 2 Valid Concerns, 3 False Claims
Valid issues:
False claims:
Recommendation: Address the UX clarity issue (#4) with a small frontend change. The rest can be accepted as-is.
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR adds support for MCP server environment variable configuration, enabling both plaintext and encrypted env vars. The implementation introduces a new database column
env_config, backend encryption/decryption logic, and a frontend UI for managing env vars and HTTP headers. While the encryption logic is sound, there are critical usability and correctness issues in the frontend UI.Findings
[BLOCKER] src-tauri/src/mcp/discovery.rs:78 - Plaintext env vars are parsed from
transport_config.envbut then overwritten by encrypted env vars; however, the code does not validate that encrypted env var keys do not duplicate plaintext ones, risking silent overwrites of plaintext vars by encrypted onesEvidence:
// Merge env vars (encrypted takes precedence over plaintext)followed bymerged_env.extend(enc_env);Fix: Explicitly warn or error if duplicate keys exist between plaintext and encrypted env configs. At minimum, log a warning when keys overlap.
[BLOCKER] src-tauri/src/mcp/discovery.rs:87 - The decrypted env var map is not sanitized; non-string values from decrypted JSON will cause
extendto panic at runtimeEvidence:
let parsed: std::collections::HashMap<String, String> = serde_json::from_str(&decrypted)...?;— if env_config contains non-string values (e.g., numbers, booleans), parsing fails but is caught as a string error, not handled gracefullyFix: Enforce strict
Stringvalue type in JSON validation before parsing and provide clear error message on invalid types.[BLOCKER] src-tauri/src/mcp/store.rs:284-289 -
get_server_env_configreturnsOk(None)on any decryption/parsing failure, masking critical configuration issues (e.g., wrong DB key) as "no env vars" instead of failing discoveryEvidence:
decrypt_token(&enc)?→from_str(...).map_err(...)?→Ok(None)in case of any errorFix: Propagate errors directly instead of returning
Ok(None); discovery should fail if env config is present but invalid.[WARNING] src/pages/Settings/MCPServers.tsx:542-547 -
plaintext_envinput field usestype="password"for plaintext values (non-sensitive), misleading users and preventing copy/paste of non-password env valuesEvidence:
<Input type="password" value={form.plaintext_env} ... />Fix: Use
type="text"forplaintext_envinput to avoid confusion.[WARNING] src/pages/Settings/MCPServers.tsx:562 - Encrypted env field placeholder suggests leaving blank to preserve existing, but
form.encrypted_envis never set from existing data (line 251), so users cannot know what is currently storedEvidence:
encrypted_env: "" // Never populate (security: don't show encrypted values)Fix: Document clearly in UI that encrypted values cannot be viewed, and provide a clear "clear" button if needed. Add a warning that entering a different value will overwrite existing ones.
[WARNING] src-tauri/src/mcp/client.rs:40 - Hardcoded 30-second timeout for tool calls may be too short for complex MCP tools and will cause false-positive timeouts without context
Evidence:
tokio::time::timeout(std::time::Duration::from_secs(30), conn.call_tool(params))...Fix: Make timeout configurable per server via env config or discovery metadata.
Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR adds support for MCP server environment variables through a new encrypted
env_configcolumn in themcp_serverstable. It includes backend changes to store, retrieve, and merge plaintext and encrypted environment variables for stdio transports, along with frontend UI updates for secure environment variable input. However, a critical inconsistency in the database migration versioning exists, and a potential environment variable injection risk is introduced in the stdio transport.Findings
[BLOCKER] src-tauri/src/db/migrations.rs:266-267 - Migration
"023_add_mcp_env_config"is registered after migration"022_attachment_views"but there are two duplicate entries for migration"011_create_integrations"before it in themigrationsarray.Evidence:
"011_create_integrations"appears at lines 153, 232, and 233, and"023_add_mcp_env_config"starts at line 266. Therun_migrationsfunction expects unique, sequential migration names.Fix: Remove duplicate migration
"011_create_integrations"entries and ensure"023_add_mcp_env_config"is the next logical step with a unique and properly ordered migration name.[WARNING] src-tauri/src/mcp/transport/stdio.rs:19-20 - Environment variables are applied directly to the spawned process using
cmd.env(key, value), but there is no validation or sanitization of thekeyandvalue. Malicious or malformed env var names could potentially override critical process environment (e.g.,LD_PRELOAD,PATH, etc.).Evidence:
for (key, value) in env { cmd.env(key, value); }at lines 19-20 instdio.rs. TheenvHashMap originates from merged plaintext and decrypted encrypted config, but no validation is performed.Fix: Validate
keyagainst an allowlist of known-safe env var patterns (e.g.,^[A-Za-z_][A-Za-z0-9_]*$) before applying withcmd.env.[WARNING] src-tauri/src/mcp/discovery.rs:100-103 - When merging plaintext and encrypted environment variables, encrypted variables take precedence, but the code does not warn the user or log a warning if both sources provide the same key. This may lead to unexpected behavior and debugging confusion.
Evidence:
let mut merged_env = plaintext_env; if let Some(enc_env) = encrypted_env { merged_env.extend(enc_env); }at lines 100-103.extendoverwrites existing keys silently.Fix: Add a
tracing::warn!("Encrypted env var '{key}' overwrote plaintext value")when a key collision is detected.[SUGGESTION] src-tauri/src/mcp/store.rs:288-290 - The
get_server_env_configfunction returnsHashMap<String, String>, but there is no handling for environment variable values containing equals signs (e.g.,TOKEN=sha256=...). While valid in some contexts, these may cause parsing errors in downstreamparseEnvVars.Evidence:
parseEnvVarsinMCPServers.tsxusespair.split("=")which discards all but the first part of the value if multiple=are present. The Rust backend has no equivalent validation.Fix: Clarify expectations in documentation or change parsing to use
splitn(2, "=")in both backend and frontend.Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR adds MCP server environment variable support (encrypted and plaintext) and includes security-hardened stdio transport validation. A critical issue exists in the frontend
MCPServers.tsxwhere plaintext environment variables are stored unvalidated and could contain malicious values injected into stdio transport commands.Findings
[BLOCKER] src-tauri/src/mcp/transport/stdio.rs:20-25 - Environment variable name validation is case-sensitive in comparison but case-insensitive in the environment variable specification, allowing bypass via lowercase names
Evidence:
if DANGEROUS_ENV_VARS.contains(&upper_key.as_str()) { ... }Fix: The check uses
upper_key(uppercase) but stores originalkey. TheDANGEROUS_ENV_VARSlist is uppercase, sold_preloadbecomesLD_PRELOAD, butkeyremainsld_preloadin error message and error is returned for uppercase variant only. This is actually correct behavior, so no issue. However, the error message references the original key but checks uppercase — this is safe but inconsistent. Since it's checking uppercase against uppercase list, it is correct. No issue found.Correction: Actually, the check is case-insensitive and correct. No issue.
[BLOCKER] src-tauri/src/mcp/transport/stdio.rs:10-13 - Relative path rejection uses
Path::new(command).is_absolute()but does not canonicalize, allowing../traversal if command starts with/but contains relative componentsEvidence:
if !Path::new(command).is_absolute() { ... }Fix: Add path canonicalization before validation to ensure no relative traversal occurs in resolved command.
[WARNING] src-tauri/src/mcp/store.rs:177-180 - When updating
env_configwith empty string, the code usesSome("")which triggersSome(encrypt_token(env_json)?), producing an encrypted empty string instead of clearing the columnEvidence:
Some(env_json) if !env_json.trim().is_empty() => Some(encrypt_token(env_json)?)— this branch is correct, butSome("")falls through to the next armSome(_) => None— this logic is correct per the code.Correction: The logic is correct — empty string is treated as "clear", and
Some("")matchesSome(_) => None, which is correct. No issue.[WARNING] src-tauri/src/mcp/discovery.rs:86-92 - Encrypted env vars are merged with plaintext env vars, but encrypted take precedence. However, if the same key exists in both, the encrypted one overwrites the plaintext — this may surprise users and leak plaintext secrets if user intends plaintext to override
Evidence:
let mut merged_env = plaintext_env; if let Some(enc_env) = encrypted_env { merged_env.extend(enc_env); }Fix: Reverse merge order or document precedence explicitly in UI.
[BLOCKER] src/pages/Settings/MCPServers.tsx:356-366 - Plaintext environment variables field (
form.plaintext_env) accepts raw space-separatedKEY=valuepairs and is passed directly into stdio transport viaJSON.stringify({ env: plaintextEnvObj })without any validation or sanitization, enabling injection of arbitrary environment variables into stdio processes, including potentially dangerous onesEvidence:
const plaintextEnvObj = parseEnvVars(form.plaintext_env);env: plaintextEnvObj,The
parseEnvVarsfunction does not validatekeycontents — it accepts any string before=Fix: Enforce that environment variable names match
^[A-Za-z_][A-Za-z0-9_]*$and reject any key that starts with underscores or is empty; also exclude known dangerous keys (case-insensitively) in the frontend.[BLOCKER] .mcp.json:7 - Hardcoded GitHub PAT in plaintext repository configuration file
Evidence:
"GITHUB_PERSONAL_ACCESS_TOKEN": "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"Fix: Remove token, document how to configure securely, and update docs to warn against committing credentials.
Verdict: REQUEST CHANGES
2ad87b4c78to0469f121b1Add dual-mode environment variable support for stdio MCP servers (plaintext + encrypted) and custom HTTP header support for HTTP-based MCP servers. Enables proper configuration of MCP servers requiring authentication tokens (e.g., GitHub MCP with
GITHUB_PERSONAL_ACCESS_TOKEN).
Backend Changes (Rust)
Database & Models
Storage Layer
Discovery & Transport
Security Hardening
Tests
Frontend Changes (TypeScript/React)
Type Definitions
UI Components
Conditional UI Fields
For stdio transport:
For HTTP transport:
Security Considerations
Backward Compatibility
Known Limitations
release with an alternative approach.
Testing
Unit Tests (Rust)
cargo test --manifest-path src-tauri/Cargo.toml mcp
cargo test --manifest-path src-tauri/Cargo.toml migrations
All tests passing (15 new tests):
Frontend Type Checking
npx tsc --noEmit
npm run build
End-to-End Testing
stdio transport (GitHub MCP):
- Name: GitHub
- Transport: stdio
- Command: /opt/homebrew/bin/npx (or /usr/local/bin/npx)
- Args: -y @modelcontextprotocol/server-github
- Encrypted env: GITHUB_PERSONAL_ACCESS_TOKEN=ghp_yourtoken
Verification:
Fixes
Fixes inability to configure MCP servers that require environment variables (e.g., GitHub MCP server with GITHUB_PERSONAL_ACCESS_TOKEN). Previously, stdio transport only supported command + args with no mechanism to pass env vars to spawned processes.
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR adds support for MCP server environment variables via an encrypted
env_configcolumn inmcp_servers, alongside plaintext env in transport config. A security issue is found: the frontend allows users to input environment variables into both plaintext and encrypted fields, but the backend merges them without validation, potentially enabling dangerous environment variables (e.g.,LD_PRELOAD) to be passed to stdio processes. Additionally, the HTTP transport does not apply custom headers as noted in the code comments.Findings
[BLOCKER] src-tauri/src/mcp/transport/stdio.rs:18-29 - Dangerous environment variable filtering uses case-insensitive comparison, but the Rust
HashMapkey is case-sensitive, allowing attackers to bypass validation by using mixed-case keys likeLd_PrElOaD.Evidence:
let upper_key = key.to_uppercase(); if DANGEROUS_ENV_VARS.contains(&upper_key.as_str()) { ... }Fix: Convert all env var keys to uppercase before storing and comparing, or normalize keys during validation.
[BLOCKER] src-tauri/src/mcp/discovery.rs:82-93 - Merging logic applies encrypted env over plaintext, but no validation is performed on the decrypted env values before passing to
connect_stdio.Evidence:
merged_env.extend(enc_env);after decryptingencrypted_env, then passed toconnect_stdiowithout re-validating dangerous keys.Fix: Validate all env keys (plaintext and decrypted encrypted) against the dangerous list after merging.
[WARNING] src-tauri/src/mcp/transport/http.rs:14-31 - Custom HTTP headers are parsed but never applied; the comment indicates a limitation in
rmcp v1.7.0, but the feature is exposed to users and misleadingly accepted.Evidence:
if !custom_headers.is_empty() { tracing::warn!(...); }butStreamableHttpClientTransportConfighas no.header()method.Fix: Remove the UI field and API parameter until
rmcpsupports custom headers, or switch to a supported transport.[SUGGESTION] src-tauri/src/mcp/store.rs:337-347 -
get_server_env_configdecrypts and parses env_config without checking that the decrypted JSON is a map of strings; a malformed value could cause runtime errors.Evidence:
let parsed: HashMap<String, String> = serde_json::from_str(&decrypted) ...Fix: Add error handling or validation for the parsed JSON shape.
Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR adds MCP environment variable support, introducing encrypted
env_configstorage and plaintext env vars in transport config. While the encryption pattern reuse is sound, there are critical logic flaws in how environment variables are merged and validated, including a security bypass in stdio transport validation and incorrect handling of empty encrypted env values that can lead to data corruption and potential privilege escalation.Findings
[BLOCKER] src-tauri/src/mcp/store.rs:78-87 - Empty
env_configis encrypted as a non-empty string, corrupting the database and breaking decryptionEvidence:
let encrypted_env = match &req.env_config { Some(env_json) if !env_json.trim().is_empty() => Some(encrypt_token(env_json)?), _ => None, };then increate_serverit insertsencrypted_enveven if it'sNone, butget_server_env_configexpects a non-empty encrypted string orNone. When updated withSome(""), it encrypts an empty string, which decrypts to"", andserde_json::from_str("")fails.Fix: In
update_server, changeSome("")handling to explicitly clear:Some(env_json) if env_json.trim().is_empty() => None, and ensure DB stores NULL for empty.[BLOCKER] src-tauri/src/mcp/discovery.rs:123-124 - Plaintext env vars override encrypted env vars, inverting security priority
Evidence:
let mut merged_env = plaintext_env; if let Some(enc_env) = encrypted_env { merged_env.extend(enc_env); }— plaintext takes precedence, allowing a plaintextAPI_KEY=valueto overwrite a securely storedAPI_KEY=secret.Fix: Reverse merge order:
let mut merged_env = encrypted_env.unwrap_or_default(); merged_env.extend(plaintext_env);so encrypted values take precedence.[WARNING] src-tauri/src/mcp/transport/stdio.rs:35-51 - Environment variable name validation uses uppercase comparison, allowing bypass via mixed-case names
Evidence:
let upper_key = key.to_uppercase(); if DANGEROUS_ENV_VARS.contains(&upper_key.as_str())— an env var likeld_PreLoadpasses because"LD_PRELOAD".to_uppercase()is"LD_PRELOAD", butkeycould be"LD_PRELOAD"and"LD_PRELOAD".to_uppercase()is"LD_PRELOAD". The logic is correct, but the comment// Validate env var names to block dangerous variables...and testtest_rejects_dangerous_env_vars_case_insensitiveshow intent — yetkey.to_uppercase()is correct, but the listDANGEROUS_ENV_VARSis uppercase, so it works. However, the testtest_rejects_dangerous_env_vars_case_insensitiveinserts"ld_preload".to_string()and then useskey.to_uppercase()— this is correct. The issue is the test logic is sound, but the implementation is fragile and relies on strict casing handling. A more robust fix is to normalize the constant list instead.Fix: Normalize constants to lowercase and compare lowercase:
DANGEROUS_ENV_VARS.iter().any(|d| key.to_lowercase() == d.to_lowercase()).[WARNING] src-tauri/src/mcp/transport/stdio.rs:60-61 - No check prevents env vars from being too large, enabling potential DoS via memory exhaustion
Evidence:
for (key, value) in env { cmd.env(key, value); }— a malicious server config could specify extremely large values (e.g., 1GB strings), causing process spawn to fail or OOM.Fix: Add size limits:
if key.len() + value.len() > MAX_ENV_SIZE { return Err(...); }withconst MAX_ENV_SIZE: usize = 8192;.[WARNING] src-tauri/src/mcp/discovery.rs:60-66 - Empty string
env_configis encrypted and stored in DB, causingget_server_env_configto returnSome(Err(...))instead ofNoneEvidence: In
store.rscreate_server,req.env_config: Some("".to_string())leads toencrypt_token("")?producing a non-empty encrypted string. Thenget_server_env_configdecrypts""andserde_json::from_str("")returnsErr(...), causing discovery to fail.Fix: In
update_server, treatSome("")asNone:Some(env_json) if env_json.trim().is_empty() => None.[SUGGESTION] src-tauri/src/mcp/transport/http.rs:21-25 - Custom headers warning is logged but never implemented; feature is incomplete
Evidence:
if !custom_headers.is_empty() { tracing::warn!(...); }and// Log warning if custom headers are provided but not yet supported.Fix: Either implement using an upstream patch/dependency upgrade, or document the limitation and disable UI for HTTP headers until support is ready.
Verdict: REQUEST CHANGES