Follow-up to #656 / #666.
The fix merged in #666 uses a two-layer design (parse-time ConfigPaths.parseText in readJsonSafe + launch-time resolveEnvVars in mcp/index.ts) that creates two correctness bugs:
1. $${VAR} escape broken end-to-end
Layer 1 turns $${VAR} into the literal ${VAR}. Layer 2 then sees the literal ${VAR} and re-resolves it to the live env value. There is no syntax left for passing a literal ${SOMETHING} string to an MCP server.
2. Variable-chain injection
With EVIL_VAR="${SECRET}" set in the shell (common: Bash PS1, templated shell snippets, GH Actions leftovers) and a config referencing ${EVIL_VAR}:
- Layer 1 resolves
${EVIL_VAR} to the literal string "${SECRET}"
- Layer 2 sees
${SECRET} in the resolved value and expands again to the actual secret
The config only referenced EVIL_VAR but the child MCP server now receives the value of SECRET.
Confirmed by POC test during multi-model code review of #666.
Additional issues flagged in review
- Regex duplicated between
config/paths.ts and mcp/index.ts with silent behavioral divergence
- Unresolved
${VAR} silently substitutes empty string with no log/telemetry — opaque failures when tokens are missing
- Layer 1 substitution scope is the whole JSON text (command args, URLs, server names), not just
env/headers
mcp.headers field is not resolved at all — Authorization: Bearer ${TOKEN} still leaks as a literal
catch {} in readJsonSafe swallows the error with only log.debug
Proposed fix
Collapse to a single resolution point at config load time, scoped to env and headers fields only. Extract shared regex/resolver into ConfigPaths so there is exactly one implementation.
See the review comment on #666 for the full multi-model analysis: #666 (comment)
Follow-up to #656 / #666.
The fix merged in #666 uses a two-layer design (parse-time
ConfigPaths.parseTextinreadJsonSafe+ launch-timeresolveEnvVarsinmcp/index.ts) that creates two correctness bugs:1.
$${VAR}escape broken end-to-endLayer 1 turns
$${VAR}into the literal${VAR}. Layer 2 then sees the literal${VAR}and re-resolves it to the live env value. There is no syntax left for passing a literal${SOMETHING}string to an MCP server.2. Variable-chain injection
With
EVIL_VAR="${SECRET}"set in the shell (common: BashPS1, templated shell snippets, GH Actions leftovers) and a config referencing${EVIL_VAR}:${EVIL_VAR}to the literal string"${SECRET}"${SECRET}in the resolved value and expands again to the actual secretThe config only referenced
EVIL_VARbut the child MCP server now receives the value ofSECRET.Confirmed by POC test during multi-model code review of #666.
Additional issues flagged in review
config/paths.tsandmcp/index.tswith silent behavioral divergence${VAR}silently substitutes empty string with no log/telemetry — opaque failures when tokens are missingenv/headersmcp.headersfield is not resolved at all —Authorization: Bearer ${TOKEN}still leaks as a literalcatch {}inreadJsonSafeswallows the error with onlylog.debugProposed fix
Collapse to a single resolution point at config load time, scoped to
envandheadersfields only. Extract shared regex/resolver intoConfigPathsso there is exactly one implementation.See the review comment on #666 for the full multi-model analysis: #666 (comment)