fix: [AI-696] single-pass env-var resolution for MCP configs#697
Closed
anandgupta42 wants to merge 1 commit intomainfrom
Closed
fix: [AI-696] single-pass env-var resolution for MCP configs#697anandgupta42 wants to merge 1 commit intomainfrom
anandgupta42 wants to merge 1 commit intomainfrom
Conversation
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.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThis PR consolidates environment variable interpolation to a single point at config load time, scoped to Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config Load<br/>(Old Two-Layer)
participant Parse as Parse Layer<br/>ConfigPaths.parseText
participant Launch as Launch Layer<br/>resolveEnvVars
participant MCP as MCP Server
Config->>Parse: readJsonSafe(file)
Parse->>Parse: Resolve $${VAR} → ${VAR}<br/>Resolve ${VAR} → env value
Parse->>Launch: pass JSON with resolved values
Launch->>Launch: Re-resolve ${VAR} patterns<br/>in already-resolved values
Launch->>MCP: Double-resolved env<br/>(${EVIL_VAR} chain injection)
Note over Parse,Launch: Bug: No escape syntax,<br/>Chain injection possible
sequenceDiagram
participant Load as Config Load<br/>(New Single-Layer)
participant Discover as Discover MCP<br/>discover.ts
participant Scope as Scoped Resolver<br/>resolveServerEnvVars
participant Transport as Transport<br/>mcp/index.ts
participant MCP as MCP Server
Load->>Discover: readJsonSafe(file, no pre-sub)
Discover->>Discover: parseJsonc() raw config
Discover->>Scope: transform() with context<br/>{server, source}
Scope->>Scope: Resolve vars in env/<br/>headers only
Scope->>Scope: Emit warning with<br/>unresolved names
Scope->>Discover: Scoped values<br/>${ESCAPED_VAR} preserved
Discover->>Transport: Pass resolved env
Transport->>MCP: Forward env directly<br/>No re-expansion
Note over Scope: Single pass,<br/>scoped, no chain injection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Follow-up to #666. Collapses MCP env-var interpolation to a single resolution point at config load time, scoped to
envandheadersfields only. Fixes two correctness bugs found during consensus code review of #666:$${VAR}escape broken end-to-end — the two-layer design resolved$${VAR}→${VAR}at parse time and then re-resolved${VAR}at launch. There was no syntax left to pass a literal${SOMETHING}to an MCP server.EVIL_VAR="${SECRET}"in the shell (common: BashPS1, templated snippets, GH Actions leftovers) and a config referencing${EVIL_VAR}, the two-layer design exposedSECRET's value even though the config only referencedEVIL_VAR. POC confirmed by Gemini during multi-model review.Changes:
packages/opencode/src/config/paths.ts— exported sharedENV_VAR_PATTERN,resolveEnvVarsInString(),newEnvSubstitutionStats(). Refactored internalsubstitute()to use the same regex grammar. One implementation, one place to maintain.packages/opencode/src/mcp/discover.ts— reverted theConfigPaths.parseTextcall inreadJsonSafe(was substituting across the whole JSON text, including command args and URLs). AddedresolveServerEnvVars()helper called fromtransform(), scoped toenvandheadersvalues only. Emitslog.warnfor unresolved vars with server + source context.packages/opencode/src/mcp/index.ts— removedresolveEnvVars()andENV_VAR_PATTERNentirely. The stdio launch site now usesmcp.environmentdirectly — single resolution already happened at config load time.packages/opencode/test/mcp/env-var-interpolation.test.ts— switched unit tests toConfigPaths.resolveEnvVarsInString. Added 5 regression tests: single-pass no-chain;$${VAR}end-to-end preservation; chain injection blocked;${VAR}resolution in remote serverheaders;${VAR}in command args / URL is NOT resolved (scope check). Also fixed fragileprocess.env = {...}teardown.Review items addressed (from #666 consensus review, #666 (comment)):
\$\${VAR}broken / chain injectionconfig/paths.tsandmcp/index.tsconfig/paths.ts${VAR}log.warnwith server + source + unresolved namesenvandheadersonlycatch {}inreadJsonSafeswallowed the errorresolveEnvVarsexported at top level ofmcp/index.tsmcp.headersnot resolvedprocess.env = {...}teardowndeleteType of change
Issue for this PR
Closes #696
Related: #656, #666
How did you verify your code works?
test/mcp/env-var-interpolation.test.ts+test/mcp/discover.test.tstest/mcp/*bun run script/upstream/analyze.ts --markers --base main --strict— cleanconfig.ts:1322is unrelated)$${TEST_MCP_TOKEN}in a discovered.vscode/mcp.json→ resolved MCPenvironmentcontains literal${TEST_MCP_TOKEN}EVIL_MCP_VAR="${TEST_MCP_SECRET}"in shell, config references${EVIL_MCP_VAR}→ resolved env contains literal${TEST_MCP_SECRET}, NOT the secret valueAuthorization: Bearer ${TEST_MCP_TOKEN}in headers → resolved toBearer glpat-secret-token${VAR}in command args and URLs → NOT touched (scope guard)Checklist
altimate_changemarkers🤖 Generated with Claude Code
Summary by cubic
Collapse MCP env-var interpolation to a single pass at config load time, scoped to
envandheaders, fixing escape handling and preventing variable-chain injection. Aligns with Linear AI-696 by removing double interpolation and adding clear warnings for unresolved vars.Bug Fixes
$${VAR}as a literal${VAR}end-to-end.${OTHER}.${VAR}in remote serverheaders; do not touch command args or URLs.log.warnwith server and source when variables are unresolved.Refactors
ConfigPaths(ENV_VAR_PATTERN,resolveEnvVarsInString,newEnvSubstitutionStats) and reuse in JSON substitution.mcp/index.ts; usemcp.environmentdirectly since resolution happens at load.readJsonSafeto parse only; resolution now runs intransform()viaresolveServerEnvVars()forenv/headersfields.Written for commit 9122153. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
envandheaderssections, leaving other config fields unchanged.Tests