Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add env block to Setup MCPs step with GitHub expressions - Replace GitHub expressions in JSON with shell variable references - Update tests to verify secure behavior - Affects safe-outputs, agentic-workflows, and GitHub MCP configs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot the generated TOML has syntax errors 30 | env = { "GH_AW_SAFE_OUTPUTS" = "/tmp/gh-aw/safeoutputs/outputs.jsonl", "GH_AW_SAFE_OUTPUTS_CONFIG" = ""{\"create_issue\":{\"max\":1,\"min\":1},\"missing_tool\":{}}"", "GH_AW_ASSETS_BRANCH" = "", "GH_AW_ASSETS_MAX_SIZE_KB" = "", "GH_AW_ASSETS_ALLOWED_EXTS" = "" } |
|
Claude, copilot ok |
🔍 Smoke Test Investigation - Run #68SummaryThe Smoke Codex workflow is failing due to invalid TOML syntax in the generated MCP server configuration. The issue is caused by using Failure Details
Root Cause AnalysisThe Problem: Invalid TOML syntax in MCP configuration The error occurs in the generated TOML configuration at pkg/workflow/mcp-config.go:207: env = { "GH_AW_SAFE_OUTPUTS" = "${{ env.GH_AW_SAFE_OUTPUTS }}", "GH_AW_SAFE_OUTPUTS_CONFIG" = ${{ toJSON(env.GH_AW_SAFE_OUTPUTS_CONFIG) }}, ... }When "GH_AW_SAFE_OUTPUTS_CONFIG" = "\"{\\\"create_issue\\\":{\\\"max\\\":1,\\\"min\\\":1},\\\"missing_tool\\\":{}}\""This creates invalid TOML because:
Failed Jobs and ErrorsJob Sequence:
Error Message: Investigation FindingsWhy This PR fixes template injection vulnerabilities by moving GitHub expressions from HERE documents to step env blocks. In HERE documents, Why TOML inline tables have different quoting requirements than JSON or YAML. When you use Comparison of Contexts:
Recommended ActionsCritical Priority
High Priority
Prevention Strategies
Historical ContextThis issue is related to previous investigations of similar
Key Difference: Previous issues involved JSON format for Copilot. This issue involves TOML inline table format for Codex, which has stricter syntax requirements. Technical DetailsGenerated TOML (Current - Broken): [mcp_servers.safeoutputs]
command = "node"
args = ["/tmp/gh-aw/safeoutputs/mcp-server.cjs"]
env = { "GH_AW_SAFE_OUTPUTS_CONFIG" = ${{ toJSON(env.GH_AW_SAFE_OUTPUTS_CONFIG) }} }After GitHub Actions evaluation: env = { "GH_AW_SAFE_OUTPUTS_CONFIG" = "\"{\\\"create_issue\\\":{...}}\"" }❌ Invalid - double-quoted value confuses TOML parser Generated TOML (Fixed): env = { "GH_AW_SAFE_OUTPUTS_CONFIG" = "${{ env.GH_AW_SAFE_OUTPUTS_CONFIG }}" }After GitHub Actions evaluation: env = { "GH_AW_SAFE_OUTPUTS_CONFIG" = "{\"create_issue\":{...}}" }✅ Valid - properly quoted string value in TOML Investigation stored:
|
- Update renderSafeOutputsMCPConfigTOML to use shell variables - Update renderAgenticWorkflowsMCPConfigTOML to use shell variables - Update tests to verify secure TOML generation - Regenerate all workflow lock files with secure TOML Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 382c18a. The TOML generation now uses shell variables instead of GitHub Actions expressions. Before: env = { "GH_AW_SAFE_OUTPUTS" = "${{ env.GH_AW_SAFE_OUTPUTS }}", "GH_AW_SAFE_OUTPUTS_CONFIG" = ${{ toJSON(env.GH_AW_SAFE_OUTPUTS_CONFIG) }}, ... }After: env = { "GH_AW_SAFE_OUTPUTS" = "$GH_AW_SAFE_OUTPUTS", "GH_AW_SAFE_OUTPUTS_CONFIG" = "$GH_AW_SAFE_OUTPUTS_CONFIG", ... }The GitHub expressions are now in the step's env block: env:
GH_AW_SAFE_OUTPUTS: ${{ env.GH_AW_SAFE_OUTPUTS }}
GH_AW_SAFE_OUTPUTS_CONFIG: ${{ toJSON(env.GH_AW_SAFE_OUTPUTS_CONFIG) }} |
🔍 Smoke Test Investigation - Run #69SummaryThe Smoke Codex workflow failed during agent initialization due to a TOML parse error in the MCP server configuration. The error occurs when a JSON-valued environment variable is substituted into a TOML inline table, creating invalid syntax with double-quoted strings. Failure Details
Root Cause AnalysisThe template injection security fix (PR #2871) changed MCP configuration generation to use shell variables instead of GitHub expressions. However, when Problematic TOML (line 30, column 104): env = { "GH_AW_SAFE_OUTPUTS" = "/tmp/gh-aw/safeoutputs/outputs.jsonl", "GH_AW_SAFE_OUTPUTS_CONFIG" = ""{\"create_issue\":{\"max\":1,\"min\":1},\"missing_tool\":{}}"", ... }Notice the double quotes: The TOML parser sees the first closing quote as the end of the string value, then encounters unexpected content ( Failed Jobs and Errors
Error Location: Investigation FindingsPattern Recognition: This is the second occurrence of TOML-related issues in this PR:
Evolution:
Root Problem: TOML inline tables with shell variable substitution of JSON values is inherently problematic due to escaping complexity. Recommended ActionsOption A: File-Based Configuration (Recommended) ⭐Change Codex to use a file-based TOML configuration, matching the pattern used by Claude engine: Current (Inline TOML): run: |
codex --config-toml "env = { \"VAR\" = \"$VALUE\" }"Proposed (File-Based): run: |
cat > /tmp/gh-aw/mcp-config/config.toml <<'EOF'
[mcp_servers.safeoutputs]
env = { "GH_AW_SAFE_OUTPUTS" = "$GH_AW_SAFE_OUTPUTS", "GH_AW_SAFE_OUTPUTS_CONFIG" = "$GH_AW_SAFE_OUTPUTS_CONFIG" }
EOF
codex --config /tmp/gh-aw/mcp-config/config.tomlBenefits:
Files to Modify:
Estimated Effort: 2-3 hours Option B: Proper TOML Escaping (Alternative)Use a TOML library to properly escape JSON strings when generating inline TOML: Benefits:
Drawbacks:
Estimated Effort: 1-2 hours + ongoing maintenance risk Prevention Strategies
Historical ContextThis is part of a broader pattern of MCP configuration challenges across engines:
The common theme: Inline configuration with complex escaping is error-prone. File-based configuration eliminates these issues. Technical DetailsTOML Inline Table Syntax: # Valid:
env = { "KEY" = "simple-value" }
# Invalid (what we're generating):
env = { "KEY" = ""{"nested":"json"}"" }Why It Fails:
Investigation Metadata:
Recommendation: Implement Option A (File-Based Configuration) to permanently resolve TOML escaping issues and align with best practices used by other engines.
|
|
@copilot try option A |
|
@copilot revert changes for codex and only keep changes in Claude/copilot engines |
- Revert renderSafeOutputsMCPConfigTOML to original implementation - Revert renderAgenticWorkflowsMCPConfigTOML to original implementation - Revert TOML tests to original expectations - Codex workflows now use original TOML with GitHub expressions - Claude/Copilot workflows retain secure JSON with shell variables Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reverted in cd5e668. Codex TOML now uses the original implementation with GitHub Actions expressions, while Claude/Copilot engines retain the secure JSON implementation with shell variables. Codex TOML (unchanged): env = { "GH_AW_SAFE_OUTPUTS" = "${{ env.GH_AW_SAFE_OUTPUTS }}", ... }Claude/Copilot JSON (secure): {
"env": {
"GH_AW_SAFE_OUTPUTS": "$GH_AW_SAFE_OUTPUTS"
}
} |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a security vulnerability by fixing template injection in MCP server configuration generation for Claude and Copilot engines. GitHub Actions expressions like ${{ env.GH_AW_SAFE_OUTPUTS }} were previously embedded directly in HERE documents, creating a security risk where attackers could inject arbitrary commands. The fix moves these expressions to safe env: blocks and uses plain shell variable references in the generated JSON/TOML configuration files.
Key Changes:
- Added environment variable blocks to "Setup MCPs" steps to evaluate GitHub expressions safely
- Updated MCP configuration generation to use shell variables (
$VAR) instead of GitHub expressions (${{ ... }}) - Modified test files to verify the security improvements
Reviewed Changes
Copilot reviewed 73 out of 74 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_servers.go |
Adds env: block logic to Setup MCPs step for safe GitHub expression evaluation |
pkg/workflow/mcp-config.go |
Updates safe-outputs and agentic-workflows config to use shell variables |
pkg/workflow/engine_helpers.go |
Modifies GitHub MCP Docker config to use shell variables instead of effective tokens |
pkg/workflow/claude_mcp.go |
Removes unused imports and updates to use shell variables for GitHub tokens |
pkg/workflow/custom_engine.go |
Updates custom engine to use shell variables for GitHub MCP configuration |
pkg/workflow/schemas/github-workflow.json |
Formatting changes to enum arrays (multi-line formatting) |
pkg/workflow/*_test.go (7 files) |
Updates test expectations to verify env blocks and shell variable usage |
.github/workflows/*.lock.yml (50+ files) |
Compiled workflow files showing env blocks and shell variable usage in action |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot merge main format |
…injection-vulnerability # Conflicts: # .github/workflows/example-permissions-warning.lock.yml
- Merged latest changes from main branch - Applied code formatting with make fmt - Resolved merge conflict in example-permissions-warning.lock.yml - Removed trailing whitespace from mcp_servers.go - Updated workflow schemas and action pins Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fix template injection in MCP server configuration generation (Claude/Copilot engines)
Problem
Security scan identified 176 template injection vulnerabilities, with 74 occurrences in MCP server configuration generation. GitHub Actions expressions like
${{ env.GH_AW_ASSETS_BRANCH }}were being used directly inside HERE documents, allowing attackers to inject arbitrary commands.Solution
pkg/workflow/mcp-config.goandpkg/workflow/engine_helpers.gomcp_servers.goto addenv:block to "Setup MCPs" stepmcp-config.goto use shell environment variables instead of GitHub expressions (Claude/Copilot only)Implementation Details
Changes Made
Added env block to Setup MCPs step (
mcp_servers.go)env:blockUpdated MCP configuration generation (Claude/Copilot engines only)
mcp-config.go,engine_helpers.go):$GH_AW_SAFE_OUTPUTSinstead of${{ env.GH_AW_SAFE_OUTPUTS }}$GITHUB_TOKENinstead of${{ secrets.GITHUB_TOKEN }}$GITHUB_MCP_SERVER_TOKENinstead of${{ secrets.GH_AW_GITHUB_TOKEN }}Updated affected engines
claude_mcp.go): Uses plain shell variables in JSON ✅copilot_engine.go): Uses escaped variables for CLI interpolation in JSON ✅custom_engine.go): Uses plain shell variables in JSON ✅codex_engine.go): Unchanged - retains original TOML with GitHub expressionsUpdated tests (7 test files)
Merged main branch and formatted code
make fmtSecurity Impact
Claude/Copilot Engines (Secure) - JSON
Before (Vulnerable):
{ "env": { "GH_AW_SAFE_OUTPUTS": "${{ env.GH_AW_SAFE_OUTPUTS }}" } }After (Secure):
Codex Engine (Unchanged) - TOML
The Codex engine retains its original TOML implementation:
Results
✅ Claude/Copilot workflows fixed with secure JSON using shell variables
⚠️ Codex workflows unchanged - retains original TOML with GitHub expressions
✅ All unit tests passing
✅ Code review feedback addressed
✅ Merged with main and code formatted
Security Summary
The fix eliminates template injection vulnerabilities for Claude and Copilot engines by:
env:contextNote: The Codex engine was intentionally excluded from this fix and retains its original implementation.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.