Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
anandgupta42 wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 114 additions & 38 deletions packages/opencode/src/config/paths.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
72 changes: 56 additions & 16 deletions packages/opencode/src/mcp/discover.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down
31 changes: 6 additions & 25 deletions packages/opencode/src/mcp/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
},
})
Expand Down
Loading
Loading