fix: [AI-656] single-pass env-var resolution for MCP configs (#697)

anandgupta42 · web-flow · commit daaaceb21009 · 2026-04-13T11:39:54.000+05:30
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.
diff --git 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_]*(?::-[^}]*)?\})|(?<!\$)\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}|\{env:([^}]+)\}/g
+
+  export interface EnvSubstitutionStats {
+    dollarRefs: number
+    dollarUnresolved: number
+    dollarDefaulted: number
+    dollarEscaped: number
+    legacyBraceRefs: number
+    legacyBraceUnresolved: number
+    unresolvedNames: string[]
+  }
+
+  /**
+   * Resolve ${VAR}, ${VAR:-default}, {env:VAR}, and $${VAR} patterns in a raw
+   * string value (i.e. a value that is already a parsed JS string, NOT JSON text).
+   * Returns the resolved string without any JSON escaping — safe for direct use in
+   * process environments, HTTP headers, or anywhere a plain string is needed.
+   *
+   * Does NOT JSON-escape — use the internal `substitute()` wrapper below if you
+   * need that (substitute() operates on raw JSON text pre-parse).
+   */
+  export function resolveEnvVarsInString(
+    value: string,
+    stats?: EnvSubstitutionStats,
+  ): string {
+    return value.replace(ENV_VAR_PATTERN, (match, escaped, dollarVar, dollarDefault, braceVar) => {
+      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_]*(?::-[^}]*)?\})|(?<!\$)\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}|\{env:([^}]+)\}/g,
-      (match, escaped, dollarVar, dollarDefault, braceVar) => {
-        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
@@ -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<string, unknown>,
+  context: { server: string; source: string; field: "env" | "headers" },
+): Record<string, string> {
+  const out: Record<string, string> = {}
+  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,16 +63,29 @@ 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<string, any>): Config.Mcp | undefined {
+function transform(
+  entry: Record<string, any>,
+  // 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<string, any> = {
       type: "remote" as const,
       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<string, unknown>, {
+        ...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<string, any>): 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<string, unknown>, {
+        ...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<string, any>)
+    const transformed = transform(entry as Record<string, any>, {
+      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<any | undefined> {
   } 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
@@ -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_]*(?::-[^}]*)?\})|(?<!\$)\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}|\{env:([^}]+)\}/g
-
-export function resolveEnvVars(environment: Record<string, string>): Record<string, string> {
-  const resolved: Record<string, string> = {}
-  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
