fix: .env loading no longer overrides already-set environment variables - #252
fix: .env loading no longer overrides already-set environment variables#252mattpodwysocki wants to merge 4 commits into
Conversation
A malicious/updated .env in the project working directory could override MAPBOX_API_ENDPOINT after the MCP host (Claude Desktop, VS Code, etc.) had already set it correctly, while the host-injected MAPBOX_ACCESS_TOKEN survived and kept being sent to the overridden endpoint -- exfiltrating it to an attacker-controlled host with no error surfaced. .env loading now skips any key already present in process.env, matching the intent of Node's own loadEnvFile(), and reports skipped keys in the startup log/span instead of silently dropping them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
No code behavior change -- just wording in comments, the changelog entry, and test fixture values. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
Reviewer feedback on the equivalent mcp-devkit-server PR (Mofei) pointed out a real gap in the prior fix: skipping .env keys only when already set doesn't help when the host never set MAPBOX_API_ENDPOINT in the first place -- the common case, since most operators only set MAPBOX_ACCESS_TOKEN and rely on the built-in https://api.mapbox.com/ default. In that case a malicious .env could still set MAPBOX_API_ENDPOINT, since "already set" was false, and the real host-injected access token would still be sent to that endpoint. loadDotEnv now takes a set of protectedKeys that .env may never set at all, regardless of whether the target env already has a value for them. index.ts passes MAPBOX_ACCESS_TOKEN/MAPBOX_API_ENDPOINT. Confirmed live both ways: before this change, a tool call with a malicious .env and no host-set MAPBOX_API_ENDPOINT reached the attacker-controlled endpoint; after it, the same call reaches the real API. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Update: pushed a fix for a gap flagged in review of the equivalent mcp-devkit-server PR (mapbox/mcp-devkit-server#138). The original fix only skipped Fixed by making Confirmed live both ways: with the prior fix, a tool call with the malicious New regression tests cover this exact scenario in |
Summary
.envloading previously applied every key ontoprocess.envwith override semantics, so a.envfile present in the working directory could take precedence over variables already set by the host process (e.g.MAPBOX_API_ENDPOINT,MAPBOX_ACCESS_TOKEN).src/utils/loadDotEnv.tsand changed it to skip any key that's already set, matching the intent of Node's ownprocess.loadEnvFile(). Already-set keys are now reported in the startup log message and theconfig.load_envtracing span instead of being silently skipped.Test plan
npx vitest run— all tests pass, including new regression tests intest/utils/loadDotEnv.test.tscovering override precedencenpm run buildsucceeds🤖 Generated with Claude Code