From 91221531d134e2018eddafec800bf2ad2012141f Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Fri, 10 Apr 2026 18:49:52 -0700 Subject: [PATCH] fix: [AI-656] single-pass env-var resolution for MCP configs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #666. The two-layer design (parse-time `ConfigPaths.parseText` in `readJsonSafe` + launch-time `resolveEnvVars` in `mcp/index.ts`) created two correctness bugs: 1. `$${VAR}` escape broken end-to-end: Layer 1 turned `$${VAR}` into literal `${VAR}`; Layer 2 then re-resolved it to the live env value. No syntax remained for passing a literal `${SOMETHING}` to an MCP server. 2. Variable-chain injection: with `EVIL_VAR="${SECRET}"` in the shell and a config referencing `${EVIL_VAR}`, Layer 1 produced the literal `"${SECRET}"` which Layer 2 then resolved to the actual secret — exfiltrating a variable the config never mentioned. The fix collapses to a single resolution point at config load time, scoped to the fields that need it. Changes: - `config/paths.ts`: export shared `ENV_VAR_PATTERN`, `resolveEnvVarsInString()`, and `newEnvSubstitutionStats()`. Internal `substitute()` reuses the same regex grammar — no more duplicate implementation in `mcp/index.ts`. - `mcp/discover.ts`: revert the `ConfigPaths.parseText` call in `readJsonSafe` (was substituting across the whole JSON text, not just env blocks). Add `resolveServerEnvVars()` helper called from `transform()`, scoped to `env` and `headers` values only. Emits `log.warn` for unresolved vars with server and source context. - `mcp/index.ts`: remove `resolveEnvVars()` and `ENV_VAR_PATTERN` entirely. The stdio launch site now uses `mcp.environment` directly — single resolution point already ran at config load time. - `test/mcp/env-var-interpolation.test.ts`: switch to `ConfigPaths.resolveEnvVarsInString`. Add 5 regression tests: single-pass no-chain; `$${VAR}` survives end-to-end; chain injection blocked; `${VAR}` resolved in remote server `headers`; `${VAR}` in command args / URL is NOT resolved (scope check). Also fixes fragile `process.env = {...}` teardown. Addresses the PR #666 consensus code review: - Critical: double interpolation (resolution collapsed to one pass) - Major: regex duplication (single source in `config/paths.ts`) - Major: silent empty-string on unresolved vars (now `log.warn` with context) - Major: whole-JSON-text scope (narrowed to `env` and `headers` only) - Major: swallowed `catch {}` in `readJsonSafe` (removed with the whole block) - Minor: `resolveEnvVars` top-level export (removed) - Minor: `mcp.headers` not resolved (now resolved) Test results: 36/36 tests pass in `test/mcp/env-var-interpolation.test.ts` + `test/mcp/discover.test.ts`. Marker guard clean. No new TS errors in touched files. Relates to #656 and #666. --- packages/opencode/src/config/paths.ts | 152 ++++++++--- packages/opencode/src/mcp/discover.ts | 72 +++-- packages/opencode/src/mcp/index.ts | 31 +-- .../test/mcp/env-var-interpolation.test.ts | 249 +++++++++++++----- 4 files changed, 363 insertions(+), 141 deletions(-) diff --git a/packages/opencode/src/config/paths.ts b/packages/opencode/src/config/paths.ts index f0526ff62..2d7e34a9d 100644 --- a/packages/opencode/src/config/paths.ts +++ b/packages/opencode/src/config/paths.ts @@ -84,51 +84,127 @@ export namespace ConfigPaths { return typeof input === "string" ? path.dirname(input) : input.dir } + // altimate_change start — shared env-var interpolation primitives + // Unified regex for env-var interpolation, single source of truth. + // Syntaxes (alternation, left-to-right): + // 1. $${VAR} or $${VAR:-default} — literal escape (docker-compose style) + // 2. ${VAR} or ${VAR:-default} — shell/dotenv substitution + // 3. {env:VAR} — raw text injection (backward compat) + // Exported so other modules (e.g. mcp/discover) can reuse the exact same grammar + // without forking the regex. See issue #635, #656. + export const ENV_VAR_PATTERN = + /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(? { + if (escaped !== undefined) { + // $${VAR} → literal ${VAR} + if (stats) stats.dollarEscaped++ + return "$" + escaped + } + if (dollarVar !== undefined) { + // ${VAR} / ${VAR:-default} + if (stats) stats.dollarRefs++ + const envValue = process.env[dollarVar] + const resolved = envValue !== undefined && envValue !== "" + if (!resolved && dollarDefault !== undefined && stats) stats.dollarDefaulted++ + if (!resolved && dollarDefault === undefined) { + if (stats) { + stats.dollarUnresolved++ + stats.unresolvedNames.push(dollarVar) + } + } + return resolved ? envValue : (dollarDefault ?? "") + } + if (braceVar !== undefined) { + // {env:VAR} → raw text injection + if (stats) stats.legacyBraceRefs++ + const v = process.env[braceVar] + if ((v === undefined || v === "") && stats) { + stats.legacyBraceUnresolved++ + stats.unresolvedNames.push(braceVar) + } + return v || "" + } + return match + }) + } + + export function newEnvSubstitutionStats(): EnvSubstitutionStats { + return { + dollarRefs: 0, + dollarUnresolved: 0, + dollarDefaulted: 0, + dollarEscaped: 0, + legacyBraceRefs: 0, + legacyBraceUnresolved: 0, + unresolvedNames: [], + } + } + // altimate_change end + /** Apply {env:VAR} and {file:path} substitutions to config text. */ async function substitute(text: string, input: ParseSource, missing: "error" | "empty" = "error") { // altimate_change start — unified env-var interpolation // Single-pass substitution against the ORIGINAL text prevents output of one // pattern being re-matched by another (e.g. {env:A}="${B}" expanding B). - // Syntaxes (order tried, in one regex via alternation): - // 1. $${VAR} or $${VAR:-default} — literal escape (docker-compose style) - // 2. ${VAR} or ${VAR:-default} — string-safe, JSON-escaped (shell/dotenv) - // 3. {env:VAR} — raw text injection (backward compat) - // Users arriving from Claude Code / VS Code / dotenv / docker-compose expect - // ${VAR}. Use {env:VAR} for raw unquoted injection. See issue #635. - let dollarRefs = 0 - let dollarUnresolved = 0 - let dollarDefaulted = 0 - let dollarEscaped = 0 - let legacyBraceRefs = 0 - let legacyBraceUnresolved = 0 - text = text.replace( - /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(? { - if (escaped !== undefined) { - // $${VAR} → literal ${VAR} - dollarEscaped++ - return "$" + escaped - } - if (dollarVar !== undefined) { - // ${VAR} / ${VAR:-default} → JSON-escaped string-safe substitution - dollarRefs++ - const envValue = process.env[dollarVar] - const resolved = envValue !== undefined && envValue !== "" - if (!resolved && dollarDefault !== undefined) dollarDefaulted++ - if (!resolved && dollarDefault === undefined) dollarUnresolved++ - const value = resolved ? envValue : (dollarDefault ?? "") - return JSON.stringify(value).slice(1, -1) + // Uses the shared ENV_VAR_PATTERN grammar, adding JSON-escaping for values + // substituted into raw JSON text (which is then parsed). This is the ONLY + // call site that applies JSON escaping; raw-string callers should use + // `resolveEnvVarsInString` instead. + const stats = newEnvSubstitutionStats() + text = text.replace(ENV_VAR_PATTERN, (match, escaped, dollarVar, dollarDefault, braceVar) => { + if (escaped !== undefined) { + stats.dollarEscaped++ + return "$" + escaped + } + if (dollarVar !== undefined) { + stats.dollarRefs++ + const envValue = process.env[dollarVar] + const resolved = envValue !== undefined && envValue !== "" + if (!resolved && dollarDefault !== undefined) stats.dollarDefaulted++ + if (!resolved && dollarDefault === undefined) { + stats.dollarUnresolved++ + stats.unresolvedNames.push(dollarVar) } - if (braceVar !== undefined) { - // {env:VAR} → raw text injection - legacyBraceRefs++ - const v = process.env[braceVar] - if (v === undefined || v === "") legacyBraceUnresolved++ - return v || "" + const value = resolved ? envValue : (dollarDefault ?? "") + // JSON-escape because this substitution happens against raw JSON text. + return JSON.stringify(value).slice(1, -1) + } + if (braceVar !== undefined) { + stats.legacyBraceRefs++ + const v = process.env[braceVar] + if (v === undefined || v === "") { + stats.legacyBraceUnresolved++ + stats.unresolvedNames.push(braceVar) } - return match - }, - ) + return v || "" + } + return match + }) + const { dollarRefs, dollarUnresolved, dollarDefaulted, dollarEscaped, legacyBraceRefs, legacyBraceUnresolved } = stats // Emit telemetry if any env interpolation happened. Dynamic import avoids a // circular dep with @/altimate/telemetry (which imports @/config/config). if (dollarRefs > 0 || legacyBraceRefs > 0 || dollarEscaped > 0) { diff --git a/packages/opencode/src/mcp/discover.ts b/packages/opencode/src/mcp/discover.ts index d56312e95..916f10ac3 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -3,10 +3,41 @@ import path from "path" import { parse as parseJsonc } from "jsonc-parser" import { Log } from "../util/log" import { Filesystem } from "../util/filesystem" +import { ConfigPaths } from "../config/paths" import type { Config } from "../config/config" const log = Log.create({ service: "mcp.discover" }) +// altimate_change start — per-field env-var resolution for discovered MCP configs +// Discovered configs (.vscode/mcp.json, .cursor/mcp.json, ~/.claude.json, etc.) +// are parsed with plain parseJsonc and thus never pass through ConfigPaths.substitute. +// Resolve ${VAR} / {env:VAR} patterns only on the env and headers fields so that +// scoping is narrow (we don't touch command args, URLs, or server names) and so +// that the launch site does NOT need a second resolution pass. +// See PR #666 review — double-interpolation regression fixed by doing this once, +// here, rather than twice. +function resolveServerEnvVars( + obj: Record, + context: { server: string; source: string; field: "env" | "headers" }, +): Record { + const out: Record = {} + const stats = ConfigPaths.newEnvSubstitutionStats() + for (const [key, raw] of Object.entries(obj)) { + if (typeof raw !== "string") continue + out[key] = ConfigPaths.resolveEnvVarsInString(raw, stats) + } + if (stats.unresolvedNames.length > 0) { + log.warn("unresolved env var references in MCP config — substituting empty string", { + server: context.server, + source: context.source, + field: context.field, + unresolved: stats.unresolvedNames.join(", "), + }) + } + return out +} +// altimate_change end + interface ExternalMcpSource { /** Relative path from base directory */ file: string @@ -32,8 +63,16 @@ const SOURCES: ExternalMcpSource[] = [ * Transform a single external MCP entry into our Config.Mcp shape. * Returns undefined if the entry is invalid (no command or url). * Preserves recognized fields: timeout, enabled. + * + * altimate_change — `context` is used to scope env-var resolution to the + * `env` and `headers` fields and to tag warnings with the source + server name. */ -function transform(entry: Record): Config.Mcp | undefined { +function transform( + entry: Record, + // altimate_change start — context for env-var resolution warnings + context: { server: string; source: string }, + // altimate_change end +): Config.Mcp | undefined { // Remote server — handle both "url" and Claude Code's "type: http" format if (entry.url && typeof entry.url === "string") { const result: Record = { @@ -41,7 +80,12 @@ function transform(entry: Record): Config.Mcp | undefined { url: entry.url, } if (entry.headers && typeof entry.headers === "object") { - result.headers = entry.headers + // altimate_change start — resolve env vars in headers (e.g. Authorization: Bearer ${TOKEN}) + result.headers = resolveServerEnvVars(entry.headers as Record, { + ...context, + field: "headers", + }) + // altimate_change end } if (typeof entry.timeout === "number") result.timeout = entry.timeout if (typeof entry.enabled === "boolean") result.enabled = entry.enabled @@ -63,7 +107,12 @@ function transform(entry: Record): Config.Mcp | undefined { command: cmd, } if (entry.env && typeof entry.env === "object") { - result.environment = entry.env + // altimate_change start — resolve env vars in environment block + result.environment = resolveServerEnvVars(entry.env as Record, { + ...context, + field: "env", + }) + // altimate_change end } if (typeof entry.timeout === "number") result.timeout = entry.timeout if (typeof entry.enabled === "boolean") result.enabled = entry.enabled @@ -93,7 +142,10 @@ function addServersFromFile( if (Object.prototype.hasOwnProperty.call(result, name)) continue // first source wins if (!entry || typeof entry !== "object") continue - const transformed = transform(entry as Record) + const transformed = transform(entry as Record, { + server: name, + source: sourceLabel, + }) if (transformed) { // Project-scoped servers are discovered but disabled by default for security. // User-owned home-directory configs are auto-enabled. @@ -117,18 +169,6 @@ async function readJsonSafe(filePath: string): Promise { } catch { return undefined } - // altimate_change start — apply env-var interpolation to external MCP configs - // External configs (Claude Code, Cursor, Copilot, Gemini) may contain ${VAR} - // or {env:VAR} references that need resolving before JSON parsing. - // Uses ConfigPaths.parseText which runs substitute() then parses JSONC. - try { - const { ConfigPaths } = await import("../config/paths") - return await ConfigPaths.parseText(text, filePath, "empty") - } catch { - // Substitution or parse failure — fall back to direct parse without interpolation - log.debug("env-var interpolation failed for external MCP config, falling back to direct parse", { file: filePath }) - } - // altimate_change end const errors: any[] = [] const result = parseJsonc(text, errors, { allowTrailingComma: true }) if (errors.length > 0) { diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 4042b984d..bc1b35d10 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -25,29 +25,6 @@ import { TuiEvent } from "@/cli/cmd/tui/event" import open from "open" import { Telemetry } from "@/telemetry" -// altimate_change start — resolve env-var references in MCP environment values -// Handles ${VAR}, ${VAR:-default}, and {env:VAR} patterns that may have survived -// config parsing (e.g. discovered external configs, config updates via updateGlobal). -const ENV_VAR_PATTERN = - /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(?): Record { - const resolved: Record = {} - for (const [key, value] of Object.entries(environment)) { - resolved[key] = value.replace(ENV_VAR_PATTERN, (match, escaped, dollarVar, dollarDefault, braceVar) => { - if (escaped !== undefined) return "$" + escaped - if (dollarVar !== undefined) { - const envValue = process.env[dollarVar] - return envValue !== undefined && envValue !== "" ? envValue : (dollarDefault ?? "") - } - if (braceVar !== undefined) return process.env[braceVar] || "" - return match - }) - } - return resolved -} -// altimate_change end - export namespace MCP { const log = Log.create({ service: "mcp" }) const DEFAULT_TIMEOUT = 30_000 @@ -532,8 +509,12 @@ export namespace MCP { env: { ...process.env, ...(cmd === "altimate" || cmd === "altimate-code" ? { BUN_BE_BUN: "1" } : {}), - // altimate_change start — resolve ${VAR} / {env:VAR} patterns that survived config parsing - ...(mcp.environment ? resolveEnvVars(mcp.environment) : {}), + // altimate_change start — env-var references in mcp.environment are resolved once + // at config load time: `ConfigPaths.substitute()` for `opencode.json`, and + // `resolveServerEnvVars()` for discovered external configs (`.vscode/mcp.json`, + // `.cursor/mcp.json`, etc.). A second pass here would re-expand already-resolved + // values and break the `$${VAR}` escape convention — see PR #666 review. + ...(mcp.environment ?? {}), // altimate_change end }, }) diff --git a/packages/opencode/test/mcp/env-var-interpolation.test.ts b/packages/opencode/test/mcp/env-var-interpolation.test.ts index a79ef40be..8acb6fcea 100644 --- a/packages/opencode/test/mcp/env-var-interpolation.test.ts +++ b/packages/opencode/test/mcp/env-var-interpolation.test.ts @@ -1,107 +1,95 @@ -// altimate_change start — tests for MCP env-var interpolation (closes #656) +// altimate_change start — tests for MCP env-var interpolation (closes #656, addresses PR #666 review) import { describe, test, expect, beforeEach, afterEach } from "bun:test" import { mkdir, writeFile } from "fs/promises" import path from "path" -import { resolveEnvVars } from "../../src/mcp" +import { ConfigPaths } from "../../src/config/paths" import { tmpdir } from "../fixture/fixture" // ------------------------------------------------------------------------- -// resolveEnvVars — safety-net resolver at MCP launch site +// resolveEnvVarsInString — shared raw-string resolver in ConfigPaths // ------------------------------------------------------------------------- +// This is the single source of truth for env-var interpolation on already-parsed +// string values. `ConfigPaths.substitute()` uses the same regex grammar but +// JSON-escapes its output (because it runs on raw JSON text before parsing). -describe("resolveEnvVars", () => { - const ORIGINAL_ENV = { ...process.env } +describe("ConfigPaths.resolveEnvVarsInString", () => { + const KEYS_TO_CLEAR = [ + "TEST_TOKEN", + "TEST_HOST", + "UNSET_VAR", + "COMPLETELY_UNSET_VAR_XYZ", + "UNSET_VAR_ABC", + ] beforeEach(() => { process.env["TEST_TOKEN"] = "secret-123" process.env["TEST_HOST"] = "gitlab.example.com" + delete process.env["UNSET_VAR"] + delete process.env["COMPLETELY_UNSET_VAR_XYZ"] + delete process.env["UNSET_VAR_ABC"] }) afterEach(() => { - process.env = { ...ORIGINAL_ENV } + for (const k of KEYS_TO_CLEAR) delete process.env[k] }) test("resolves ${VAR} syntax", () => { - const result = resolveEnvVars({ - API_TOKEN: "${TEST_TOKEN}", - HOST: "${TEST_HOST}", - }) - expect(result.API_TOKEN).toBe("secret-123") - expect(result.HOST).toBe("gitlab.example.com") + expect(ConfigPaths.resolveEnvVarsInString("${TEST_TOKEN}")).toBe("secret-123") + expect(ConfigPaths.resolveEnvVarsInString("${TEST_HOST}")).toBe("gitlab.example.com") }) test("resolves {env:VAR} syntax", () => { - const result = resolveEnvVars({ - API_TOKEN: "{env:TEST_TOKEN}", - }) - expect(result.API_TOKEN).toBe("secret-123") + expect(ConfigPaths.resolveEnvVarsInString("{env:TEST_TOKEN}")).toBe("secret-123") }) test("resolves ${VAR:-default} with fallback when unset", () => { - const result = resolveEnvVars({ - MODE: "${UNSET_VAR:-production}", - }) - expect(result.MODE).toBe("production") + expect(ConfigPaths.resolveEnvVarsInString("${UNSET_VAR:-production}")).toBe("production") }) test("resolves ${VAR:-default} to env value when set", () => { - const result = resolveEnvVars({ - TOKEN: "${TEST_TOKEN:-fallback}", - }) - expect(result.TOKEN).toBe("secret-123") + expect(ConfigPaths.resolveEnvVarsInString("${TEST_TOKEN:-fallback}")).toBe("secret-123") }) test("preserves $${VAR} as literal ${VAR}", () => { - const result = resolveEnvVars({ - TEMPLATE: "$${TEST_TOKEN}", - }) - expect(result.TEMPLATE).toBe("${TEST_TOKEN}") + expect(ConfigPaths.resolveEnvVarsInString("$${TEST_TOKEN}")).toBe("${TEST_TOKEN}") }) - test("resolves unset variable to empty string", () => { - const result = resolveEnvVars({ - MISSING: "${COMPLETELY_UNSET_VAR_XYZ}", - }) - expect(result.MISSING).toBe("") + test("resolves unset variable with no default to empty string", () => { + expect(ConfigPaths.resolveEnvVarsInString("${COMPLETELY_UNSET_VAR_XYZ}")).toBe("") }) test("passes through plain values without modification", () => { - const result = resolveEnvVars({ - PLAIN: "just-a-string", - URL: "https://gitlab.com/api/v4", - }) - expect(result.PLAIN).toBe("just-a-string") - expect(result.URL).toBe("https://gitlab.com/api/v4") + expect(ConfigPaths.resolveEnvVarsInString("just-a-string")).toBe("just-a-string") + expect(ConfigPaths.resolveEnvVarsInString("https://gitlab.com/api/v4")).toBe("https://gitlab.com/api/v4") }) test("resolves multiple variables in a single value", () => { - const result = resolveEnvVars({ - URL: "https://${TEST_HOST}/api?token=${TEST_TOKEN}", - }) - expect(result.URL).toBe("https://gitlab.example.com/api?token=secret-123") + expect(ConfigPaths.resolveEnvVarsInString("https://${TEST_HOST}/api?token=${TEST_TOKEN}")).toBe( + "https://gitlab.example.com/api?token=secret-123", + ) }) - test("handles mixed resolved and plain entries", () => { - const result = resolveEnvVars({ - TOKEN: "${TEST_TOKEN}", - STATIC_URL: "https://gitlab.com/api/v4", - HOST: "{env:TEST_HOST}", - }) - expect(result.TOKEN).toBe("secret-123") - expect(result.STATIC_URL).toBe("https://gitlab.com/api/v4") - expect(result.HOST).toBe("gitlab.example.com") + test("does not interpolate bare $VAR without braces", () => { + expect(ConfigPaths.resolveEnvVarsInString("$TEST_TOKEN")).toBe("$TEST_TOKEN") }) - test("does not interpolate bare $VAR without braces", () => { - const result = resolveEnvVars({ - TOKEN: "$TEST_TOKEN", - }) - expect(result.TOKEN).toBe("$TEST_TOKEN") + test("records unresolved variable names in stats", () => { + const stats = ConfigPaths.newEnvSubstitutionStats() + ConfigPaths.resolveEnvVarsInString("${COMPLETELY_UNSET_VAR_XYZ}", stats) + expect(stats.dollarUnresolved).toBe(1) + expect(stats.unresolvedNames).toContain("COMPLETELY_UNSET_VAR_XYZ") }) - test("handles empty environment object", () => { - const result = resolveEnvVars({}) - expect(result).toEqual({}) + test("does NOT double-expand a resolved ${VAR} value that itself contains ${OTHER}", () => { + // Single-pass: a value like `prefix ${FOO}` resolves to the env value of FOO, + // and even if that value is itself a `${...}` string, it is NOT re-resolved. + // This is the CRITICAL regression from PR #666 — no chain injection. + process.env["EVIL"] = "${TEST_TOKEN}" + try { + expect(ConfigPaths.resolveEnvVarsInString("${EVIL}")).toBe("${TEST_TOKEN}") + } finally { + delete process.env["EVIL"] + } }) }) @@ -110,15 +98,22 @@ describe("resolveEnvVars", () => { // ------------------------------------------------------------------------- describe("discoverExternalMcp with env-var interpolation", () => { - const ORIGINAL_ENV = { ...process.env } + const KEYS_TO_CLEAR = [ + "TEST_MCP_TOKEN", + "TEST_MCP_HOST", + "UNSET_VAR_ABC", + "EVIL_MCP_VAR", + "TEST_MCP_SECRET", + ] beforeEach(() => { process.env["TEST_MCP_TOKEN"] = "glpat-secret-token" process.env["TEST_MCP_HOST"] = "https://gitlab.internal.com" + delete process.env["UNSET_VAR_ABC"] }) afterEach(() => { - process.env = { ...ORIGINAL_ENV } + for (const k of KEYS_TO_CLEAR) delete process.env[k] }) test("resolves ${VAR} in discovered .vscode/mcp.json environment", async () => { @@ -205,5 +200,135 @@ describe("discoverExternalMcp with env-var interpolation", () => { const env = (servers["svc"] as any).environment expect(env.MODE).toBe("production") }) + + // ---------- regression tests from PR #666 review ---------- + + test("regression: $${VAR} survives end-to-end as literal ${VAR}", async () => { + // Pre-fix: Layer 1 (parseText in readJsonSafe) would turn `$${VAR}` into `${VAR}`, + // then Layer 2 (resolveEnvVars at MCP launch) would re-resolve it to the env value. + // After the fix: single resolution in transform() → `$${VAR}` → `${VAR}` and stays. + await using tmp = await tmpdir() + const dir = tmp.path + await mkdir(path.join(dir, ".vscode"), { recursive: true }) + await writeFile( + path.join(dir, ".vscode/mcp.json"), + JSON.stringify({ + servers: { + svc: { + command: "node", + args: ["svc.js"], + env: { + TEMPLATE: "$${TEST_MCP_TOKEN}", + }, + }, + }, + }), + ) + + const { discoverExternalMcp } = await import("../../src/mcp/discover") + const { servers } = await discoverExternalMcp(dir) + + const env = (servers["svc"] as any).environment + expect(env.TEMPLATE).toBe("${TEST_MCP_TOKEN}") + }) + + test("regression: chain-injection — env var whose value contains ${OTHER} is not re-resolved", async () => { + // Pre-fix: EVIL_MCP_VAR="${TEST_MCP_SECRET}" in shell. Config references ${EVIL_MCP_VAR}. + // Layer 1 resolved to `"${TEST_MCP_SECRET}"` literal, then Layer 2 resolved that literal + // to the actual secret — exfiltrating TEST_MCP_SECRET even though the config only + // referenced EVIL_MCP_VAR. After the fix: single pass, result is the literal string. + process.env["EVIL_MCP_VAR"] = "${TEST_MCP_SECRET}" + process.env["TEST_MCP_SECRET"] = "leaked-secret-value" + + await using tmp = await tmpdir() + const dir = tmp.path + await mkdir(path.join(dir, ".vscode"), { recursive: true }) + await writeFile( + path.join(dir, ".vscode/mcp.json"), + JSON.stringify({ + servers: { + svc: { + command: "node", + args: ["svc.js"], + env: { + X: "${EVIL_MCP_VAR}", + }, + }, + }, + }), + ) + + const { discoverExternalMcp } = await import("../../src/mcp/discover") + const { servers } = await discoverExternalMcp(dir) + + const env = (servers["svc"] as any).environment + expect(env.X).toBe("${TEST_MCP_SECRET}") + expect(env.X).not.toBe("leaked-secret-value") + }) + + test("regression: resolves ${VAR} in headers for remote MCP servers", async () => { + // The original PR only fixed environment. Headers (Authorization, etc.) + // commonly contain tokens and need the same resolution. + await using tmp = await tmpdir() + const dir = tmp.path + await mkdir(path.join(dir, ".vscode"), { recursive: true }) + await writeFile( + path.join(dir, ".vscode/mcp.json"), + JSON.stringify({ + servers: { + "remote-svc": { + url: "https://api.example.com/mcp", + headers: { + Authorization: "Bearer ${TEST_MCP_TOKEN}", + "X-Host": "${TEST_MCP_HOST}", + "X-Static": "fixed-value", + }, + }, + }, + }), + ) + + const { discoverExternalMcp } = await import("../../src/mcp/discover") + const { servers } = await discoverExternalMcp(dir) + + expect(servers["remote-svc"]).toBeDefined() + expect(servers["remote-svc"].type).toBe("remote") + const headers = (servers["remote-svc"] as any).headers + expect(headers.Authorization).toBe("Bearer glpat-secret-token") + expect(headers["X-Host"]).toBe("https://gitlab.internal.com") + expect(headers["X-Static"]).toBe("fixed-value") + }) + + test("scopes resolution to env/headers — does not touch command args or URLs", async () => { + // Regression guard for Major #5 from the PR review: env-var substitution must not + // run over the whole JSON text. Only env and headers values should be resolved. + await using tmp = await tmpdir() + const dir = tmp.path + await mkdir(path.join(dir, ".vscode"), { recursive: true }) + await writeFile( + path.join(dir, ".vscode/mcp.json"), + JSON.stringify({ + servers: { + // command args contain ${TEST_MCP_TOKEN} — must NOT be resolved + svc: { + command: "node", + args: ["--token=${TEST_MCP_TOKEN}", "server.js"], + }, + "remote-svc": { + // URL contains ${TEST_MCP_HOST} — must NOT be resolved + url: "https://${TEST_MCP_HOST}/mcp", + }, + }, + }), + ) + + const { discoverExternalMcp } = await import("../../src/mcp/discover") + const { servers } = await discoverExternalMcp(dir) + + const local = servers["svc"] as any + expect(local.command).toEqual(["node", "--token=${TEST_MCP_TOKEN}", "server.js"]) + const remote = servers["remote-svc"] as any + expect(remote.url).toBe("https://${TEST_MCP_HOST}/mcp") + }) }) // altimate_change end