From 85c018b3131bd6ebdcb8817cfe83701ff8ed7fac Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 25 Aug 2026 11:58:55 -0400 Subject: [PATCH 1/3] security: stop .env from overriding host-injected environment variables 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 --- CHANGELOG.md | 4 ++ src/index.ts | 57 ++++++++++----------- src/utils/loadDotEnv.ts | 53 ++++++++++++++++++++ test/utils/loadDotEnv.test.ts | 93 +++++++++++++++++++++++++++++++++++ 4 files changed, 177 insertions(+), 30 deletions(-) create mode 100644 src/utils/loadDotEnv.ts create mode 100644 test/utils/loadDotEnv.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index be463d54..73747542 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## Unreleased +### Security + +- **`.env` loading no longer overrides variables already set in the process environment.** The startup loader applied every key from a project-local `.env` (found via `process.cwd()`) directly onto `process.env`, replacing anything already there. A `.env` file added to (or modified within) the working directory — e.g. by a repo contributor, or anything else with write access to that directory — could therefore override `MAPBOX_API_ENDPOINT` after the MCP host (Claude Desktop, VS Code, a hosted deployment, etc.) had already configured it correctly, while the host-injected `MAPBOX_ACCESS_TOKEN` remained valid and kept getting sent as `access_token` to whatever host the overridden endpoint now pointed at — exfiltrating the token to an attacker-controlled server with no error surfaced to the operator. `.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()`; skipped keys are reported in the server's startup log message and tracing span so this is never silent. + ### Breaking Changes - **`place_details_tool` now calls the Mapbox Places API instead of the older Details API.** The tool previously called `search/details/v1/retrieve` (`docs.mapbox.com/api/search/details/`); it now calls `places/v1/details/retrieve` (`docs.mapbox.com/api/search/places/`), a separate, newer product built around a larger POI dataset. This is not a compatible upgrade: diff --git a/src/index.ts b/src/index.ts index c64ca974..a47e9e6a 100644 --- a/src/index.ts +++ b/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'; @@ -40,26 +37,11 @@ import { // 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)); - } -} +const envResult = loadDotEnv(process.cwd()); +const envPath = envResult.path; +const envLoadError = envResult.error; +const envLoadedCount = envResult.appliedCount; +const envSkippedKeys = envResult.skippedKeys; const versionInfo = getVersionInfo(); const cliMetadataResult = handleCliMetadataArgs( @@ -229,8 +211,9 @@ 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, 'operation.type': 'config_load' } }); @@ -243,7 +226,7 @@ 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) { span.setStatus({ code: SpanStatusCode.OK }); span.setAttribute('config.load.success', true); } else { @@ -297,10 +280,24 @@ async function main() { level: 'warning', data: `Warning loading .env file: ${envLoadError.message}` }); - } else if (envLoadedCount > 0) { + } else if (envLoadedCount > 0 || envSkippedKeys.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(', ')}` + ); + } server.server.sendLoggingMessage({ level: 'info', - data: `Loaded ${envLoadedCount} environment variables from ${envPath}` + data: parts.join('; ') }); } diff --git a/src/utils/loadDotEnv.ts b/src/utils/loadDotEnv.ts new file mode 100644 index 00000000..bfc39f9f --- /dev/null +++ b/src/utils/loadDotEnv.ts @@ -0,0 +1,53 @@ +// 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; + skippedKeys: 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.) must always win over a + * project-local `.env` — otherwise a `.env` file dropped into or committed + * to a cloned repo could silently redirect security-relevant variables + * (e.g. MAPBOX_API_ENDPOINT) out from under an operator who already + * configured them correctly at the host level, while the host-injected + * MAPBOX_ACCESS_TOKEN still gets sent to wherever that endpoint now points. + */ +export function loadDotEnv( + cwd: string, + env: NodeJS.ProcessEnv = process.env +): LoadDotEnvResult { + const path = join(cwd, '.env'); + const exists = existsSync(path); + let appliedCount = 0; + const skippedKeys: 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 (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, error }; +} diff --git a/test/utils/loadDotEnv.test.ts b/test/utils/loadDotEnv.test.ts new file mode 100644 index 00000000..5d103817 --- /dev/null +++ b/test/utils/loadDotEnv.test.ts @@ -0,0 +1,93 @@ +// 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 injected by the host (regression: host-injected values must win)', () => { + const dir = makeTempDirWithEnv( + [ + 'MAPBOX_ACCESS_TOKEN=attacker-controlled-token', + 'MAPBOX_API_ENDPOINT=https://attacker.example/' + ].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://attacker.example/', + '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'); + }); +}); From b535660e0d4daf0a93b31a22715885761057c79e Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 25 Aug 2026 12:01:50 -0400 Subject: [PATCH 2/3] docs: tone down comments/changelog to describe the fix, not an exploit No code behavior change -- just wording in comments, the changelog entry, and test fixture values. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 4 ++-- src/utils/loadDotEnv.ts | 9 +++------ test/utils/loadDotEnv.test.ts | 8 ++++---- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73747542..a78029de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,8 @@ ## Unreleased -### Security +### Fixed -- **`.env` loading no longer overrides variables already set in the process environment.** The startup loader applied every key from a project-local `.env` (found via `process.cwd()`) directly onto `process.env`, replacing anything already there. A `.env` file added to (or modified within) the working directory — e.g. by a repo contributor, or anything else with write access to that directory — could therefore override `MAPBOX_API_ENDPOINT` after the MCP host (Claude Desktop, VS Code, a hosted deployment, etc.) had already configured it correctly, while the host-injected `MAPBOX_ACCESS_TOKEN` remained valid and kept getting sent as `access_token` to whatever host the overridden endpoint now pointed at — exfiltrating the token to an attacker-controlled server with no error surfaced to the operator. `.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()`; skipped keys are reported in the server's startup log message and tracing span so this is never silent. +- **`.env` loading no longer overrides variables already set in the process environment.** 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()`; skipped keys are reported in the server's startup log message and tracing span so this is never silent. ### Breaking Changes diff --git a/src/utils/loadDotEnv.ts b/src/utils/loadDotEnv.ts index bfc39f9f..d0398d67 100644 --- a/src/utils/loadDotEnv.ts +++ b/src/utils/loadDotEnv.ts @@ -16,12 +16,9 @@ export interface LoadDotEnvResult { /** * 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.) must always win over a - * project-local `.env` — otherwise a `.env` file dropped into or committed - * to a cloned repo could silently redirect security-relevant variables - * (e.g. MAPBOX_API_ENDPOINT) out from under an operator who already - * configured them correctly at the host level, while the host-injected - * MAPBOX_ACCESS_TOKEN still gets sent to wherever that endpoint now points. + * 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. */ export function loadDotEnv( cwd: string, diff --git a/test/utils/loadDotEnv.test.ts b/test/utils/loadDotEnv.test.ts index 5d103817..a5550e43 100644 --- a/test/utils/loadDotEnv.test.ts +++ b/test/utils/loadDotEnv.test.ts @@ -36,11 +36,11 @@ describe('loadDotEnv', () => { expect(result.skippedKeys).toEqual([]); }); - it('never overrides a variable already injected by the host (regression: host-injected values must win)', () => { + it('never overrides a variable already set by the host process', () => { const dir = makeTempDirWithEnv( [ - 'MAPBOX_ACCESS_TOKEN=attacker-controlled-token', - 'MAPBOX_API_ENDPOINT=https://attacker.example/' + 'MAPBOX_ACCESS_TOKEN=dotenv-token', + 'MAPBOX_API_ENDPOINT=https://dotenv.example.com/' ].join('\n') ); const env: NodeJS.ProcessEnv = { @@ -61,7 +61,7 @@ describe('loadDotEnv', () => { it('applies unset keys while leaving already-set keys from the same file untouched', () => { const dir = makeTempDirWithEnv( [ - 'MAPBOX_API_ENDPOINT=https://attacker.example/', + 'MAPBOX_API_ENDPOINT=https://dotenv.example.com/', 'OTEL_SERVICE_NAME=my-service' ].join('\n') ); From 4e889b601d35c7e643e16c085e9eac3759a525ba Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 26 Aug 2026 12:53:02 -0400 Subject: [PATCH 3/3] fix: never let .env set MAPBOX_ACCESS_TOKEN/MAPBOX_API_ENDPOINT at all 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 --- CHANGELOG.md | 2 +- src/index.ts | 34 +++++++++++++++--- src/utils/loadDotEnv.ts | 23 ++++++++++-- test/utils/loadDotEnv.test.ts | 68 +++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e6c719d..9f075068 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Fixed -- **`.env` loading no longer overrides variables already set in the process environment.** 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()`; skipped keys are reported in the server's startup log message and tracing span so this is never silent. +- **`.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 diff --git a/src/index.ts b/src/index.ts index a47e9e6a..d5626c41 100644 --- a/src/index.ts +++ b/src/index.ts @@ -36,12 +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 envResult = loadDotEnv(process.cwd()); +// 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( @@ -214,6 +224,7 @@ async function main() { 'config.file.exists': envResult.exists, 'config.vars.loaded': envLoadedCount, 'config.vars.skipped': envSkippedKeys.length, + 'config.vars.blocked': envBlockedKeys.length, 'operation.type': 'config_load' } }); @@ -226,7 +237,11 @@ async function main() { }); span.setAttribute('error.type', envLoadError.name); span.setAttribute('error.message', envLoadError.message); - } else if (envLoadedCount > 0 || envSkippedKeys.length > 0) { + } else if ( + envLoadedCount > 0 || + envSkippedKeys.length > 0 || + envBlockedKeys.length > 0 + ) { span.setStatus({ code: SpanStatusCode.OK }); span.setAttribute('config.load.success', true); } else { @@ -280,7 +295,11 @@ async function main() { level: 'warning', data: `Warning loading .env file: ${envLoadError.message}` }); - } else if (envLoadedCount > 0 || envSkippedKeys.length > 0) { + } else if ( + envLoadedCount > 0 || + envSkippedKeys.length > 0 || + envBlockedKeys.length > 0 + ) { const parts: string[] = []; if (envLoadedCount > 0) { parts.push( @@ -295,6 +314,13 @@ async function main() { `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: parts.join('; ') diff --git a/src/utils/loadDotEnv.ts b/src/utils/loadDotEnv.ts index d0398d67..056433e4 100644 --- a/src/utils/loadDotEnv.ts +++ b/src/utils/loadDotEnv.ts @@ -9,7 +9,13 @@ 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; } @@ -19,21 +25,34 @@ export interface LoadDotEnvResult { * 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 + env: NodeJS.ProcessEnv = process.env, + protectedKeys: ReadonlySet = 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; @@ -46,5 +65,5 @@ export function loadDotEnv( } } - return { path, exists, appliedCount, skippedKeys, error }; + return { path, exists, appliedCount, skippedKeys, blockedKeys, error }; } diff --git a/test/utils/loadDotEnv.test.ts b/test/utils/loadDotEnv.test.ts index a5550e43..6e84d905 100644 --- a/test/utils/loadDotEnv.test.ts +++ b/test/utils/loadDotEnv.test.ts @@ -90,4 +90,72 @@ describe('loadDotEnv', () => { 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([]); + }); });