fix: escape MCP env secrets with \${VAR} for non-Copilot engines to prevent heredoc JSON corruption - #49450
Conversation
…prevent JSON corruption Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes heredoc expansion corrupting MCP gateway JSON for non-Copilot engines.
Changes:
- Escapes custom MCP environment placeholders consistently across JSON engines.
- Adds coverage for secrets and fallback expressions.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_config_custom.go |
Unifies JSON environment placeholder rendering. |
pkg/workflow/mcp_config_copilot_test.go |
Tests escaped placeholders across engines. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| // expressions with \${VAR} passthrough syntax. This keeps raw secret values | ||
| // out of the heredoc (RGS-008) and produces valid JSON regardless of the | ||
| // characters contained in the secret at runtime. | ||
| envValue = ReplaceTemplateExpressionsWithEnvVars(envValue) |
There was a problem hiding this comment.
Addressed in commit 12a5090 — ReplaceSecretsWithBashVars and all its tests were removed entirely (no remaining callers after the fix). The stale doc comment in secret_extraction.go:165-168 is gone along with the function.
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent Analyzed 1 test (1 table-driven with 3 scenarios): 1 design, 0 implementation, 0 violations. Test: TestRenderCustomMCPEnvVars_NonCopilotSecretsEscaped (pkg/workflow/mcp_config_copilot_test.go:213) This table-driven test verifies a security-critical behavioral contract: MCP server env secrets must be rendered as \${VAR} (backslash-escaped) in non-Copilot JSON engines to prevent bash from expanding unquoted heredocs. Strengths:
Verdict: ✅ PASSED. 0% implementation tests (threshold 30%). Excellent security-critical test coverage.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Fix looks correct
The core change — collapsing both Copilot and non-Copilot paths to always call ReplaceTemplateExpressionsWithEnvVars — is correct and well-reasoned. The heredoc injection bug (raw ${VAR} being expanded by bash before the MCP gateway parses the JSON) is a real issue, and \${VAR} is the right fix.
Tests: The new TestRenderCustomMCPEnvVars_NonCopilotSecretsEscaped test correctly covers all three cases (non-Copilot secret, Copilot secret, fallback expression). The unexpectedContent assertions are safe — "${MY_TOKEN}" is not a substring of "\${MY_TOKEN}" so the negative check is valid.
Stale doc comment: The existing review comment at line 81 correctly flags that ReplaceSecretsWithBashVars doc comment still claims it is used for non-Copilot MCP server env blocks — that should be updated. This is a non-blocking maintenance item already captured.
No new blocking issues found.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 32.6 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two minor findings, no blocking issues. The fix is correct and well-motivated.
📋 Key Themes & Highlights
Key Themes
- Stale doc comment:
ReplaceSecretsWithBashVarsinsecret_extraction.gostill documents itself as used for non-Copilot MCP env blocks, but that caller was removed by this PR. - Test coverage: The new tests assert the escaping pattern (
\${VAR}) is present/absent, but don't include a case that validates the rendered output is structurally valid JSON — which is exactly the failure mode the bug caused.
Positive Highlights
- ✅ Root cause properly addressed: collapsing the two branches into
ReplaceTemplateExpressionsWithEnvVarsis the correct fix, not just a symptom patch. - ✅ The
collectMCPEnvironmentVariablespath still injects secrets into the step env block so the gateway container can resolve${VAR}at runtime — no functional regression. - ✅ New test covers all three cases: non-Copilot, Copilot, and fallback expression with default — good specification breadth.
- ✅ The doc comment on
renderCustomMCPEnvVarsclearly explains the heredoc injection rationale and the RGS-008 reference. - ✅ The
requiresCopilotFieldsparameter removal simplifies the API correctly since the behavior no longer differs by engine.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 53 AIC · ⊞ 7K
Comment /matt to run again
| RequiresCopilotFields: false, | ||
| }, | ||
| expectedContent: []string{ | ||
| `"DD_SITE": "\${DD_SITE}"`, |
There was a problem hiding this comment.
[/tdd] The tests assert \${MY_TOKEN} is present and ${MY_TOKEN} is absent, but they don't include a case with special characters (e.g. a " or \ in the secret value) going through the full render path. That's the exact root cause scenario from the bug — a secret containing a double-quote would have corrupted the JSON. A regression test that validates the rendered output is still valid JSON would prevent this class of bug from recurring.
💡 Suggested additional test case
Add a test case that verifies the rendered config is parseable JSON even when a secret contains characters that would break unescaped heredoc injection:
{
name: "Non-Copilot stdio - special chars in secret name don't affect JSON validity",
// The var *name* is always a safe identifier; but verify the \${VAR}
// placeholder produces structurally valid JSON (the gateway resolves it later).
toolConfig: map[string]any{
"type": "stdio",
"container": "some/image:latest",
"env": map[string]any{
"SPECIAL_KEY": `${{ secrets.SPECIAL_KEY }}`,
},
},
renderer: MCPConfigRenderer{
IndentLevel: " ",
Format: "json",
},
expectedContent: []string{`"SPECIAL_KEY": "\${SPECIAL_KEY}"`},
// Verify the output is still valid JSON after placeholder substitution
// with a benign value (simulating what the gateway sees at runtime).
validateJSON: true,
},@copilot please address this.
There was a problem hiding this comment.
Added in the latest commit — TestRenderCustomMCPEnvVars_NonCopilotSecretsEscaped now includes a validateJSON bool field on the test struct. The new "Non-Copilot stdio - env placeholder produces valid JSON after gateway resolution" case sets validateJSON: true, which:
- Uses
regexpto replace\${VAR}placeholders withplaceholder-value(simulating bash heredoc stripping the backslash + gateway env resolution). - Wraps the fragment in
{}and callsjson.Unmarshal— the test fails if the fragment is not parseable as a JSON object.
All 4 sub-tests pass.
Smoke Test: Copilot Engine — PR fix: escape MCP env secrets with \${VAR} for non-Copilot engines to prevent heredoc JSON corruption
Overall status: FAIL (Serena timeout) cc Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke test: automated review submission.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · auto · 73 AIC · ⌖ 5.05 AIC · ⊞ 8.7K
Comment /smoke-copilot to run again
Add label smoke to run again
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! Run 30716130254 — Claude engine validation complete.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · sonnet46 · 93.5 AIC · ⌖ 19.4 AIC · ⊞ 6.3K
Comment /smoke-claude to run again
| // expanding the variable; bash only strips the leading backslash, leaving the | ||
| // literal ${VAR} string in the JSON, which the gateway then resolves safely | ||
| // from its own environment (RGS-008 compliance). | ||
| func renderCustomMCPEnvVars(env map[string]string, tomlFormat bool) map[string]string { |
There was a problem hiding this comment.
✅ Smoke Test Review #1 — The simplified function signature is clean. Removing the parameter is the right call since both code paths now behave identically. Run 30716130254.
| // expressions with \${VAR} passthrough syntax. This keeps raw secret values | ||
| // out of the heredoc (RGS-008) and produces valid JSON regardless of the | ||
| // characters contained in the secret at runtime. | ||
| envValue = ReplaceTemplateExpressionsWithEnvVars(envValue) |
There was a problem hiding this comment.
✅ Smoke Test Review #2 — ReplaceTemplateExpressionsWithEnvVars is called correctly here for both engine types. The unified code path ensures consistent \${VAR} escaping that survives heredoc expansion. Good fix! Run 30716130254.
|
@copilot merge main, recompile, review |
|
@copilot quick triage for this PR:
|
…tainer-env-secrets Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit
CI still needs a human re-trigger on the current head to get a green signal before merge. |
Current status after the Reviews — 4 unresolved threads, no further code changes needed
The agent cannot resolve review threads via the available MCP tools (no Branch refresh — merged CI — Only the active Copilot gate check is on the current head ( Maintainer actions needed before merge:
|
|
@copilot quick triage for this PR:
|
No branch work needed — Current maintainer-facing blockers:
No agent-actionable work remains. |
🔍 Triage Summary
Carry-over from prior run. Escapes MCP env secrets to prevent heredoc JSON corruption. CI green, multiple automated approvals. Ready for human merge decision. Automated triage — see the triage report for full context.
|
|
@copilot quick triage for this PR:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
PR finisher pass complete. Reviews — 4 unresolved threads, all addressed in code, awaiting maintainer resolve
Checks (local) — green
Checks (CI) — actively running on new HEAD Mergeable ✅ — PR is open, not draft, no conflicts. Maintainer actions before merge:
|
For non-Copilot engines (Claude, Codex, etc.), secrets in stdio/container custom MCP server
env:blocks were substituted as bare${VAR}inside an unquoted heredoc. Bash expands those before the MCP gateway parses the config, so any secret containing"or\silently corrupts the JSON and the Start MCP Gateway step fails.Root cause
renderCustomMCPEnvVarshad separate branches for Copilot (\${VAR}, heredoc-safe) and non-Copilot (${VAR}, bash-expanded). The Copilot path was correct; the non-Copilot path was not.Changes
mcp_config_custom.go— collapsed both branches into a single call toReplaceTemplateExpressionsWithEnvVars, which produces\${VAR}for all JSON engines. Bash strips the leading\in the heredoc, leaving a literal${VAR}that the gateway resolves from its own environment after the JSON is parsed. Removed the now-unusedrequiresCopilotFieldsparam fromrenderCustomMCPEnvVars. Updated doc comment with the heredoc injection rationale.mcp_config_copilot_test.go— addedTestRenderCustomMCPEnvVars_NonCopilotSecretsEscapedcovering non-Copilot secrets, Copilot secrets, and fallback expressions (${{ secrets.X || 'default' }}), asserting\${VAR}and asserting the absence of${VAR}.Before / after
The
\${VAR}pattern was already used correctly for HTTP header secrets and the built-in safe-outputs MCP server in the same generated file; this aligns the custom env block path with the rest.Run: https://github.com/github/gh-aw/actions/runs/30689325959
Run: https://github.com/github/gh-aw/actions/runs/30700817659
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
Run details: https://github.com/github/gh-aw/actions/runs/30702262954
✨ PR Review Safe Output Test - Run 30716130254
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
run: https://github.com/github/gh-aw/actions/runs/30716547955