diff --git a/CHANGELOG.md b/CHANGELOG.md index f1da222..00a7f80 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/index.ts b/src/index.ts index 96cbe82..82f3c52 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 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 @@ 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)); - // 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(); @@ -167,8 +158,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' } }); @@ -181,7 +174,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 { @@ -225,10 +222,34 @@ async function main() { 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({ diff --git a/src/utils/loadDotEnv.ts b/src/utils/loadDotEnv.ts new file mode 100644 index 0000000..056433e --- /dev/null +++ b/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 = 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 }; +} diff --git a/test/utils/loadDotEnv.test.ts b/test/utils/loadDotEnv.test.ts new file mode 100644 index 0000000..6e84d90 --- /dev/null +++ b/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([]); + }); +});