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

CHANGELOG.md

@@ -14,6 +14,10 @@
   - Both tools now take `httpRequest` as a constructor dependency (matching every other network-calling tool in this repo) to make the mint call.
   - **Hosted-endpoint-aware error messages.** Per `README.md`, the hosted MCP endpoint authenticates with its own access token rather than a personal Mapbox account token, and that token isn't granted `tokens:write` — so auto-mint can't work there. Without this, a hosted caller who omits `accessToken` would have hit a raw, misleading `jwtUtils` error (`"MAPBOX_ACCESS_TOKEN is not in valid JWT format"`, referencing an env var the hosted deployment doesn't even use) or a bare `Token API 403`. A new `describeAutoMintFailure` helper rewrites both into an actionable message pointing at `accessToken`/`list_tokens_tool`/`create_token_tool` instead.
 
+### 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 (`fetch failed` against a non-resolving host); after it, the same call reaches the real API (`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.
+
 ### Breaking Changes
 
 - **Consolidated `GeojsonPreviewUIResource` and `PreviewStyleUIResource` into a single `MapPreviewUIResource`.** Both were near-identical hand-written MCP Apps templates — the same postMessage handshake, fullscreen/open-link controls, and resize handling copy-pasted across ~650 lines, differing only in what they drew on the map once a tool result arrived (a GeoJSON overlay on the default Standard style vs. swapping to an arbitrary preview style). `geojson_preview_tool` and `preview_style_tool` now both declare `_meta.ui.resourceUri: 'ui://mapbox/map-preview/index.html'`, served by the merged resource, which dispatches on the shape of the tool-result URL it receives (a `geojson.io` URL vs. a Styles API `.html?access_token=...` preview URL) rather than assuming a fixed mode. Neither tool's own input/output contract changed.

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 already set in 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';
@@ -30,28 +27,22 @@
 } 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));
-    // Error will be logged via MCP logging messages and traced if tracing is enabled
-  }
-}
+// 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();
 
@@ -102,7 +93,7 @@
 // This tells clients (like Claude Desktop) that this is an MCP App
 uiResources.forEach((resource) => {
   registerAppResource(
     server as any,
     resource.name,
     resource.uri,
     { mimeType: RESOURCE_MIME_TYPE, description: resource.description },
@@ -167,8 +158,10 @@
         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'
           }
         });
@@ -181,7 +174,11 @@
           });
           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 {
@@ -225,10 +222,34 @@
       level: 'warning',
       data: `Failed to load .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) {
+      // Already set (e.g. by the MCP host) and intentionally left unchanged
+      // -- a .env file never overrides an already-set variable.
+      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('; ')
     });
   } else {
     server.server.sendLoggingMessage({

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([]);
+  });
+});