Fix template-injection vulnerabilities in safe outputs config heredocs#25888
Fix template-injection vulnerabilities in safe outputs config heredocs#25888
Conversation
…env block
Move GitHub Actions template expressions out of run: block heredocs
into the step's env: block to prevent template-injection vulnerabilities
flagged by zizmor. This extends the existing secret extraction pattern
to also cover ${{ github.* }} context expressions.
The safe outputs config JSON (config.json) previously contained
${{ github.workflow }} inside a run: block heredoc. Now the expression
is set via env: and referenced as ${GITHUB_WORKFLOW} shell variable.
Fixes 16 workflows: copilot-pr-nlp-analysis, copilot-session-insights,
copilot-token-audit, daily-code-metrics, daily-integrity-analysis,
daily-issues-report, daily-multi-device-docs-tester, daily-news,
daily-repo-chronicle, github-mcp-structural-analysis, org-health-report,
portfolio-analyst, python-data-charts, stale-repo-identifier,
weekly-editors-health-check, weekly-issue-summary.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/22cff3a9-de45-47b6-8dbb-f8e7acb36115
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…erence Agent-Logs-Url: https://github.com/github/gh-aw/sessions/22cff3a9-de45-47b6-8dbb-f8e7acb36115 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes high-severity GitHub Actions template-injection findings caused by ${{ github.* }} expressions being embedded in run: heredocs for safe-outputs config generation.
Changes:
- Added extraction of simple
${{ github.* }}context expressions and mapping toGITHUB_*env vars. - Updated MCP setup YAML generation to move
${{ secrets.* }}and${{ github.* }}expressions into a stepenv:block and substitute shell${VAR}references in the heredoc content. - Regenerated 16 affected workflow
.lock.ymlfiles to use the safer pattern.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/secret_extraction.go | Adds github-context expression extraction and mapping to runner env vars. |
| pkg/workflow/secret_extraction_test.go | Adds unit tests for github-context expression extraction. |
| pkg/workflow/mcp_setup_generator.go | Uses the new extraction to generate env: + sanitized heredocs for safe-outputs config. |
| actions/setup/js/git_helpers.cjs | Minor refactor of an ENOBUFS error string construction. |
| .github/workflows/weekly-issue-summary.lock.yml | Regenerated to move ${{ github.workflow }} into env: and use ${GITHUB_WORKFLOW} in heredoc content. |
| .github/workflows/weekly-editors-health-check.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/stale-repo-identifier.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/python-data-charts.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/portfolio-analyst.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/org-health-report.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/github-mcp-structural-analysis.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/daily-repo-chronicle.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/daily-news.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/daily-multi-device-docs-tester.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/daily-issues-report.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/daily-integrity-analysis.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/daily-code-metrics.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/copilot-token-audit.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/copilot-session-insights.lock.yml | Same regeneration pattern for safe-outputs config step. |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Same regeneration pattern for safe-outputs config step. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/workflow/secret_extraction.go:242
ExtractGitHubContextExpressionsFromValuereturns a map keyed by env var name, so multiple occurrences (or whitespace variants) of the same${{ github.* }}expression will overwrite each other. Downstream replacement then only replaces the last-seen exact expression string, potentially leaving other${{ }}occurrences un-sanitized in the generated heredoc.
func ExtractGitHubContextExpressionsFromValue(value string) map[string]string {
result := make(map[string]string)
matches := gitHubContextExprPattern.FindAllStringSubmatch(value, -1)
for _, match := range matches {
if len(match) < 2 {
continue
}
property := match[1]
fullExpr := match[0]
if envVar, known := gitHubContextEnvVarMap[property]; known {
result[envVar] = fullExpr
secretLog.Printf("Extracted GitHub context expression: %s -> %s", fullExpr, envVar)
}
}
return result
}
pkg/workflow/mcp_setup_generator.go:271
- Switching to an unquoted heredoc whenever
hasEnvVarsis true means the shell will perform all heredoc expansions (parameter expansion, command substitution via$(...)/backticks, etc.) on the JSON. SincesafeOutputConfigis produced viajson.Marshalfrom user-controlled config fields, this can corrupt JSON (e.g.$VARsequences) and can even execute command substitutions if such sequences appear in configured strings. Prefer a strategy that avoids shell evaluation (e.g., write the JSON via a tool/script that substitutes only the intended placeholders, or escape$/backticks everywhere except the generated${VAR}placeholders).
if hasEnvVars {
// Replace ${{ ... }} expressions with ${VAR} shell references and use
// an unquoted heredoc so the shell expands them at runtime.
sanitizedConfig := safeOutputConfig
for varName, secretExpr := range configSecrets {
sanitizedConfig = strings.ReplaceAll(sanitizedConfig, secretExpr, "${"+varName+"}")
}
for varName, ctxExpr := range configContextVars {
sanitizedConfig = strings.ReplaceAll(sanitizedConfig, ctxExpr, "${"+varName+"}")
}
yaml.WriteString(" cat > \"${RUNNER_TEMP}/gh-aw/safeoutputs/config.json\" << " + delimiter + "\n")
yaml.WriteString(" " + sanitizedConfig + "\n")
yaml.WriteString(" " + delimiter + "\n")
- Files reviewed: 20/20 changed files
- Comments generated: 2
pkg/workflow/secret_extraction.go
Outdated
| // Complex expressions with operators (||, &&) or nested event payloads (github.event.issue.title) | ||
| // are excluded because they may not map to well-known runner environment variables. |
There was a problem hiding this comment.
The comment claims the regex excludes nested event payloads like github.event.issue.title, but gitHubContextExprPattern will still match those (they're only skipped later because the property isn't in gitHubContextEnvVarMap). Either tighten the regex to actually exclude nested payloads, or update the comment to reflect that the map is the filter.
This issue also appears on line 224 of the same file.
| // Complex expressions with operators (||, &&) or nested event payloads (github.event.issue.title) | |
| // are excluded because they may not map to well-known runner environment variables. | |
| // Complex expressions with operators (||, &&) are excluded by the regex. Nested dotted | |
| // properties such as github.event.issue.title may still match this pattern, but are only | |
| // accepted later if they are present in gitHubContextEnvVarMap. |
| envKeys := make([]string, 0, len(configSecrets)+len(configContextVars)) | ||
| envValues := make(map[string]string, len(configSecrets)+len(configContextVars)) | ||
| for k, v := range configSecrets { | ||
| envKeys = append(envKeys, k) | ||
| envValues[k] = v | ||
| } | ||
| sort.Strings(secretKeys) | ||
| for _, varName := range secretKeys { | ||
| yaml.WriteString(" " + varName + ": " + configSecrets[varName] + "\n") | ||
| for k, v := range configContextVars { | ||
| envKeys = append(envKeys, k) | ||
| envValues[k] = v | ||
| } | ||
| sort.Strings(envKeys) | ||
| for _, varName := range envKeys { | ||
| yaml.WriteString(" " + varName + ": " + envValues[varName] + "\n") | ||
| } |
There was a problem hiding this comment.
When combining configSecrets and configContextVars, key collisions will silently overwrite earlier values (context vars currently override secrets). A secret named e.g. GITHUB_WORKFLOW would be replaced with the github context expression, producing incorrect behavior and potentially reintroducing ${{ }} in the rendered config. Consider detecting collisions and either erroring or namespacing context variables (or ensuring secrets always take precedence).
This issue also appears on line 259 of the same file.
|
@copilot review all comments |
…n key collision - secret_extraction.go: Clarify that the regex matches nested properties like github.event.issue.title but the map filters them out (not the regex itself). - mcp_setup_generator.go: Add context vars to envValues first, then secrets, so secrets always take precedence on key collision. - secret_extraction_test.go: Use assert.Len instead of assert.Equal(len, len) to satisfy testifylint. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ff433af4-2b6b-46b4-8f63-fa5b57ddba8e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both review comments in 322035b:
Also fixed a |
zizmor flags
${{ github.workflow }}insiderun:block heredocs as High-severity template injection across 16 workflows. The expression originates from the compiler's defaultupload-assetbranch name ("assets/${{ github.workflow }}") which gets serialized into the safe outputs config JSON written via heredoc.Compiler fix (
mcp_setup_generator.go)Extended the existing secret extraction pattern to also extract
${{ github.* }}context expressions from the config, moving them to the step'senv:block and referencing them as shell variables in an unquoted heredoc. Secrets always take precedence over context vars on key collision.Before:
After:
New utility (
secret_extraction.go)ExtractGitHubContextExpressionsFromValue()— extracts simple${{ github.* }}expressions and maps them to their well-knownGITHUB_*runner env varsgithub.*properties including nested ones likegithub.event.issue.title, but only well-known properties present ingitHubContextEnvVarMapare acceptedRecompiled lock files
All 16 affected
.lock.ymlfiles regenerated with the safe pattern.