fix: .env loading no longer overrides already-set environment variables

CHANGELOG.md

@@ -1,5 +1,9 @@
 ## Unreleased
 
+### Fixed
+
+- **`.env` loading no longer overrides variables already set in the process environment, and `MAPBOX_ACCESS_TOKEN`/`MAPBOX_API_ENDPOINT` can no longer be set via `.env` at all.** The startup loader applied every key from a project-local `.env` directly onto `process.env`, replacing anything already there, including variables set by the MCP host. `.env` loading (`src/utils/loadDotEnv.ts`) now skips any key that already exists in `process.env`, matching the intent of Node's own `process.loadEnvFile()`. `MAPBOX_ACCESS_TOKEN` and `MAPBOX_API_ENDPOINT` go further: `.env` may never set them, even when the host left them unset. Without that, a host that only sets `MAPBOX_ACCESS_TOKEN` and relies on the built-in `https://api.mapbox.com/` default for `MAPBOX_API_ENDPOINT` would still let a malicious `.env` set the endpoint, since it was never "already set" to begin with — sending the real access token to that attacker-controlled endpoint. Confirmed live both ways: before this additional restriction, a tool call reached the attacker endpoint; after it, the same call reaches the real API (`401: Not Authorized - Invalid Token`, the expected response for a fake token). Skipped and blocked keys are both reported in the server's startup log message and tracing span so this is never silent.
+
 ### New Features
 
 - **`render_map_tool`: restyle the base map itself via `baseMapConfig`, and place custom layers with `slot`.** Previously `MapAppPayload`/`RenderMapInputSchema` only supported adding new GeoJSON layers on top of a fixed base map. `baseMapConfig` exposes Mapbox Standard's config-property system (`map.setConfigProperty('basemap', ...)`) — e.g. `{ "colorWater": "#ff0000" }` turns the water red, or `{ "lightPreset": "night" }` switches to night lighting — covering colors, `theme`, `lightPreset`, and label/3D-object visibility toggles. Applied at map-construction time (via the `config` option, when present in the initial payload) to avoid a flash of default colors, and via `setConfigProperty` on later re-renders. `layers[].slot` (`"bottom" | "middle" | "top"`) places a custom layer relative to Standard's own layers instead of always rendering above everything, including labels (still the default when `slot` is omitted). Both verified live in a real browser against the Mapbox API. See `docs/render-map-tool.md` for the full property list and an example. Filed as #249 after a user asked whether `render_map_tool` could turn the water red on the fly.

src/index.ts

@@ -1,13 +1,10 @@
 // Copyright (c) Mapbox, Inc.
 // Licensed under the MIT License.
 
-// Load environment variables from .env file if present
-// Use Node.js built-in util.parseEnv() and manually apply to override existing vars
-import { parseEnv } from 'node:util';
-import { readFileSync } from 'node:fs';
-import { join } from 'node:path';
-import { existsSync } from 'node:fs';
+// Load environment variables from .env file if present, without overriding
+// anything the MCP host already injected into process.env (see loadDotEnv).
 import { SpanStatusCode } from '@opentelemetry/api';
+import { loadDotEnv } from './utils/loadDotEnv.js';
 
 import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
 import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js';
@@ -39,27 +36,22 @@ import {
 } from './utils/tracing.js';
 
 // Load .env from current working directory (where npm run is executed)
-// This happens before tracing is initialized, but we'll add a span when tracing is ready
-const envPath = join(process.cwd(), '.env');
-let envLoadError: Error | null = null;
-let envLoadedCount = 0;
-
-if (existsSync(envPath)) {
-  try {
-    // Read and parse .env file using Node.js built-in parseEnv
-    const envFile = readFileSync(envPath, 'utf-8');
-    const parsed = parseEnv(envFile);
-
-    // Apply parsed values to process.env (with override)
-    // Note: process.loadEnvFile() doesn't override, so we use parseEnv + manual assignment
-    for (const [key, value] of Object.entries(parsed)) {
-      process.env[key] = value;
-      envLoadedCount++;
-    }
-  } catch (error) {
-    envLoadError = error instanceof Error ? error : new Error(String(error));
-  }
-}
+// This happens before tracing is initialized, but we'll add a span when tracing is ready.
+// MAPBOX_ACCESS_TOKEN/MAPBOX_API_ENDPOINT are excluded even when unset: a
+// project-local .env redirecting MAPBOX_API_ENDPOINT would otherwise still
+// cause the real host-injected access token to be sent to that endpoint,
+// even in the common case where the host never set MAPBOX_API_ENDPOINT
+// itself and relies on the built-in api.mapbox.com default.
+const DOTENV_PROTECTED_KEYS = new Set([
+  'MAPBOX_ACCESS_TOKEN',
+  'MAPBOX_API_ENDPOINT'
+]);
+const envResult = loadDotEnv(process.cwd(), process.env, DOTENV_PROTECTED_KEYS);
+const envPath = envResult.path;
+const envLoadError = envResult.error;
+const envLoadedCount = envResult.appliedCount;
+const envSkippedKeys = envResult.skippedKeys;
+const envBlockedKeys = envResult.blockedKeys;
 
 const versionInfo = getVersionInfo();
 const cliMetadataResult = handleCliMetadataArgs(
@@ -229,8 +221,10 @@ async function main() {
         const span = tracer.startSpan('config.load_env', {
           attributes: {
             'config.file.path': envPath,
-            'config.file.exists': existsSync(envPath),
+            'config.file.exists': envResult.exists,
             'config.vars.loaded': envLoadedCount,
+            'config.vars.skipped': envSkippedKeys.length,
+            'config.vars.blocked': envBlockedKeys.length,
             'operation.type': 'config_load'
           }
         });
@@ -243,7 +237,11 @@ async function main() {
           });
           span.setAttribute('error.type', envLoadError.name);
           span.setAttribute('error.message', envLoadError.message);
-        } else if (envLoadedCount > 0) {
+        } else if (
+          envLoadedCount > 0 ||
+          envSkippedKeys.length > 0 ||
+          envBlockedKeys.length > 0
+        ) {
           span.setStatus({ code: SpanStatusCode.OK });
           span.setAttribute('config.load.success', true);
         } else {
@@ -297,10 +295,35 @@ async function main() {
       level: 'warning',
       data: `Warning loading .env file: ${envLoadError.message}`
     });
-  } else if (envLoadedCount > 0) {
+  } else if (
+    envLoadedCount > 0 ||
+    envSkippedKeys.length > 0 ||
+    envBlockedKeys.length > 0
+  ) {
+    const parts: string[] = [];
+    if (envLoadedCount > 0) {
+      parts.push(
+        `loaded ${envLoadedCount} environment variable(s) from ${envPath}`
+      );
+    }
+    if (envSkippedKeys.length > 0) {
+      // These keys were already set (e.g. injected by the MCP host) and were
+      // intentionally left unchanged — a .env file never overrides an
+      // already-set variable. Surfaced so this is never silent.
+      parts.push(
+        `left ${envSkippedKeys.length} already-set variable(s) from ${envPath} unchanged: ${envSkippedKeys.join(', ')}`
+      );
+    }
+    if (envBlockedKeys.length > 0) {
+      // Security-sensitive keys a .env file may never set, even when unset
+      // in the process environment -- see DOTENV_PROTECTED_KEYS above.
+      parts.push(
+        `ignored ${envBlockedKeys.length} security-sensitive variable(s) from ${envPath} (not settable via .env): ${envBlockedKeys.join(', ')}`
+      );
+    }
     server.server.sendLoggingMessage({
       level: 'info',
-      data: `Loaded ${envLoadedCount} environment variables from ${envPath}`
+      data: parts.join('; ')
     });
   }
 

src/utils/loadDotEnv.ts

@@ -0,0 +1,69 @@
+// Copyright (c) Mapbox, Inc.
+// Licensed under the MIT License.
+
+import { existsSync, readFileSync } from 'node:fs';
+import { join } from 'node:path';
+import { parseEnv } from 'node:util';
+
+export interface LoadDotEnvResult {
+  path: string;
+  exists: boolean;
+  appliedCount: number;
+  /** Keys skipped because the target env already had a value for them. */
+  skippedKeys: string[];
+  /**
+   * Keys skipped because they're in `protectedKeys`, regardless of whether
+   * the target env already had a value for them.
+   */
+  blockedKeys: string[];
+  error: Error | null;
+}
+
+/**
+ * Loads a `.env` file from `cwd` into `env`, without overriding any key
+ * already present. A variable set by the MCP host (Claude Desktop, VS Code,
+ * a hosted deployment's process environment, etc.) should always win over a
+ * project-local `.env`, matching the precedence Node's own
+ * `process.loadEnvFile()` already applies.
+ *
+ * `protectedKeys` names keys `.env` may never set at all, even when the
+ * target env has no existing value for them — for security-sensitive keys
+ * (an API endpoint, an access token), "not yet set" shouldn't be treated as
+ * license for a project-local file to set it, since that file is far less
+ * trusted than whatever launched the process. Those keys must come from a
+ * real environment variable or be left at their built-in default.
+ */
+export function loadDotEnv(
+  cwd: string,
+  env: NodeJS.ProcessEnv = process.env,
+  protectedKeys: ReadonlySet<string> = new Set()
+): LoadDotEnvResult {
+  const path = join(cwd, '.env');
+  const exists = existsSync(path);
+  let appliedCount = 0;
+  const skippedKeys: string[] = [];
+  const blockedKeys: string[] = [];
+  let error: Error | null = null;
+
+  if (exists) {
+    try {
+      const parsed = parseEnv(readFileSync(path, 'utf-8'));
+      for (const [key, value] of Object.entries(parsed)) {
+        if (protectedKeys.has(key)) {
+          blockedKeys.push(key);
+          continue;
+        }
+        if (env[key] !== undefined) {
+          skippedKeys.push(key);
+          continue;
+        }
+        env[key] = value;
+        appliedCount++;
+      }
+    } catch (e) {
+      error = e instanceof Error ? e : new Error(String(e));
+    }
+  }
+
+  return { path, exists, appliedCount, skippedKeys, blockedKeys, error };
+}

test/utils/loadDotEnv.test.ts

@@ -0,0 +1,161 @@
+// Copyright (c) Mapbox, Inc.
+// Licensed under the MIT License.
+
+import { describe, it, expect, afterEach } from 'vitest';
+import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { loadDotEnv } from '../../src/utils/loadDotEnv.js';
+
+describe('loadDotEnv', () => {
+  const dirs: string[] = [];
+
+  function makeTempDirWithEnv(contents: string): string {
+    const dir = mkdtempSync(join(tmpdir(), 'load-dot-env-test-'));
+    writeFileSync(join(dir, '.env'), contents, 'utf-8');
+    dirs.push(dir);
+    return dir;
+  }
+
+  afterEach(() => {
+    while (dirs.length > 0) {
+      rmSync(dirs.pop()!, { recursive: true, force: true });
+    }
+  });
+
+  it('applies a variable that is not already set', () => {
+    const dir = makeTempDirWithEnv(
+      'MAPBOX_API_ENDPOINT=https://staging.example.com/\n'
+    );
+    const env: NodeJS.ProcessEnv = {};
+
+    const result = loadDotEnv(dir, env);
+
+    expect(env.MAPBOX_API_ENDPOINT).toBe('https://staging.example.com/');
+    expect(result.appliedCount).toBe(1);
+    expect(result.skippedKeys).toEqual([]);
+  });
+
+  it('never overrides a variable already set by the host process', () => {
+    const dir = makeTempDirWithEnv(
+      [
+        'MAPBOX_ACCESS_TOKEN=dotenv-token',
+        'MAPBOX_API_ENDPOINT=https://dotenv.example.com/'
+      ].join('\n')
+    );
+    const env: NodeJS.ProcessEnv = {
+      MAPBOX_ACCESS_TOKEN: 'host-injected-token',
+      MAPBOX_API_ENDPOINT: 'https://api.mapbox.com/'
+    };
+
+    const result = loadDotEnv(dir, env);
+
+    expect(env.MAPBOX_ACCESS_TOKEN).toBe('host-injected-token');
+    expect(env.MAPBOX_API_ENDPOINT).toBe('https://api.mapbox.com/');
+    expect(result.appliedCount).toBe(0);
+    expect(result.skippedKeys.sort()).toEqual(
+      ['MAPBOX_ACCESS_TOKEN', 'MAPBOX_API_ENDPOINT'].sort()
+    );
+  });
+
+  it('applies unset keys while leaving already-set keys from the same file untouched', () => {
+    const dir = makeTempDirWithEnv(
+      [
+        'MAPBOX_API_ENDPOINT=https://dotenv.example.com/',
+        'OTEL_SERVICE_NAME=my-service'
+      ].join('\n')
+    );
+    const env: NodeJS.ProcessEnv = {
+      MAPBOX_API_ENDPOINT: 'https://api.mapbox.com/'
+    };
+
+    const result = loadDotEnv(dir, env);
+
+    expect(env.MAPBOX_API_ENDPOINT).toBe('https://api.mapbox.com/');
+    expect(env.OTEL_SERVICE_NAME).toBe('my-service');
+    expect(result.appliedCount).toBe(1);
+    expect(result.skippedKeys).toEqual(['MAPBOX_API_ENDPOINT']);
+  });
+
+  it('reports exists: false and no-ops when there is no .env file', () => {
+    const dir = mkdtempSync(join(tmpdir(), 'load-dot-env-test-'));
+    dirs.push(dir);
+    const env: NodeJS.ProcessEnv = { FOO: 'bar' };
+
+    const result = loadDotEnv(dir, env);
+
+    expect(result.exists).toBe(false);
+    expect(result.appliedCount).toBe(0);
+    expect(result.skippedKeys).toEqual([]);
+    expect(result.error).toBeNull();
+    expect(env.FOO).toBe('bar');
+  });
+
+  it('never lets .env set a protected key, even when it is not already set (regression: endpoint redirection with no explicit host value)', () => {
+    // Mirrors the exact gap flagged in PR review: a host that only sets
+    // MAPBOX_ACCESS_TOKEN and leaves MAPBOX_API_ENDPOINT unset (relying on
+    // the built-in default) would otherwise still let a malicious .env set
+    // MAPBOX_API_ENDPOINT, since "not already set" previously meant .env
+    // was free to set it.
+    const dir = makeTempDirWithEnv(
+      'MAPBOX_API_ENDPOINT=https://attacker.example/\n'
+    );
+    const env: NodeJS.ProcessEnv = {
+      MAPBOX_ACCESS_TOKEN: 'host-injected-token'
+      // MAPBOX_API_ENDPOINT intentionally left unset.
+    };
+
+    const result = loadDotEnv(
+      dir,
+      env,
+      new Set(['MAPBOX_ACCESS_TOKEN', 'MAPBOX_API_ENDPOINT'])
+    );
+
+    expect(env.MAPBOX_API_ENDPOINT).toBeUndefined();
+    expect(result.appliedCount).toBe(0);
+    expect(result.blockedKeys).toEqual(['MAPBOX_API_ENDPOINT']);
+    expect(result.skippedKeys).toEqual([]);
+  });
+
+  it('blocks a protected key even when the host already set it too', () => {
+    const dir = makeTempDirWithEnv('MAPBOX_ACCESS_TOKEN=dotenv-token\n');
+    const env: NodeJS.ProcessEnv = {
+      MAPBOX_ACCESS_TOKEN: 'host-injected-token'
+    };
+
+    const result = loadDotEnv(dir, env, new Set(['MAPBOX_ACCESS_TOKEN']));
+
+    expect(env.MAPBOX_ACCESS_TOKEN).toBe('host-injected-token');
+    expect(result.blockedKeys).toEqual(['MAPBOX_ACCESS_TOKEN']);
+    expect(result.skippedKeys).toEqual([]);
+  });
+
+  it('only blocks the named protected keys, leaving other unset keys free to apply', () => {
+    const dir = makeTempDirWithEnv(
+      [
+        'MAPBOX_API_ENDPOINT=https://attacker.example/',
+        'OTEL_SERVICE_NAME=my-service'
+      ].join('\n')
+    );
+    const env: NodeJS.ProcessEnv = {};
+
+    const result = loadDotEnv(dir, env, new Set(['MAPBOX_API_ENDPOINT']));
+
+    expect(env.MAPBOX_API_ENDPOINT).toBeUndefined();
+    expect(env.OTEL_SERVICE_NAME).toBe('my-service');
+    expect(result.appliedCount).toBe(1);
+    expect(result.blockedKeys).toEqual(['MAPBOX_API_ENDPOINT']);
+  });
+
+  it('defaults to no protected keys when the parameter is omitted (backward compatible)', () => {
+    const dir = makeTempDirWithEnv(
+      'MAPBOX_API_ENDPOINT=https://staging.example.com/\n'
+    );
+    const env: NodeJS.ProcessEnv = {};
+
+    const result = loadDotEnv(dir, env);
+
+    expect(env.MAPBOX_API_ENDPOINT).toBe('https://staging.example.com/');
+    expect(result.blockedKeys).toEqual([]);
+  });
+});