Use safe RUNNER_TEMP paths in generated workflow scripts - #54303
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🔍 PR TriageCategory: bug · Risk: medium · Priority: medium · Score: 28/100 Recommended action: Security/CodeQL-motivated fix (runner.temp inline usage) but still early WIP, 0 files changed yet. Grouped for batch review once implementation lands.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch PR file list
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
Pull request overview
Hardens generated workflows against CodeQL injection alerts by resolving runner temporary paths at runtime.
Changes:
- Adds executable-body rewriting and validation.
- Applies finalization across compiler and standalone workflow generators.
- Updates tests, WASM goldens, and generated workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden |
Updates WASM output paths. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Updates WASM output paths. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden |
Updates WASM output paths. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden |
Updates WASM output paths. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden |
Updates Pi golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden |
Updates Gemini golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden |
Updates Copilot golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden |
Updates Codex golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden |
Updates Claude golden output. |
pkg/workflow/template_rendering_test.go |
Updates safe-path assertions. |
pkg/workflow/step_summary_test.go |
Updates helper-path assertions. |
pkg/workflow/side_repo_maintenance.go |
Finalizes side-repository workflows. |
pkg/workflow/runner_temp_safety.go |
Implements rewriting and validation. |
pkg/workflow/runner_temp_safety_test.go |
Tests path safety behavior. |
pkg/workflow/repo_memory_integration_test.go |
Updates repository-memory assertion. |
pkg/workflow/observability_job_summary_test.go |
Updates observability assertion. |
pkg/workflow/maintenance_workflow_yaml.go |
Finalizes maintenance workflows. |
pkg/workflow/maintenance_workflow_test.go |
Updates maintenance assertion. |
pkg/workflow/detection_success_test.go |
Updates detection assertions. |
pkg/workflow/compiler_yaml.go |
Finalizes standard compiler output. |
pkg/workflow/central_slash_command_workflow.go |
Finalizes slash-command workflow output. |
pkg/workflow/central_slash_command_workflow_test.go |
Updates slash-command assertions. |
pkg/workflow/aw_info_tmp_test.go |
Updates agent-information assertion. |
pkg/workflow/auto_update_workflow.go |
Finalizes auto-upgrade workflows. |
.github/workflows/notion-issue-summary.lock.yml |
Recompiles paths safely. |
.github/workflows/hippo-embed.lock.yml |
Recompiles paths safely. |
.github/workflows/firewall.lock.yml |
Recompiles paths safely. |
.github/workflows/example-permissions-warning.lock.yml |
Recompiles paths safely. |
.github/workflows/example-failure-category-filter.lock.yml |
Recompiles paths safely. |
.github/workflows/designer-drift-audit.lock.yml |
Recompiles paths safely. |
.github/workflows/daily-max-ai-credits-test.lock.yml |
Recompiles paths safely. |
.github/workflows/codex-github-remote-mcp-test.lock.yml |
Recompiles paths safely. |
.github/workflows/agentics-maintenance.yml |
Uses runtime-safe JavaScript paths. |
.github/workflows/agentic-auto-upgrade.yml |
Uses runtime-safe JavaScript paths. |
.github/workflows/agentic_commands.yml |
Uses runtime-safe JavaScript paths. |
Review details
- Files reviewed: 36/313 changed files
- Comments generated: 1
- Review effort level: Balanced
| finalYAML, err := finalizeRunnerTempSafety(yaml.String()) | ||
| if err != nil { | ||
| maintenanceLog.Printf("Runner temp safety validation failed for side-repo maintenance workflow: %v", err) | ||
| return yaml.String() |
There was a problem hiding this comment.
Fixed in 75c7592: side-repo workflow generation now propagates runner-temp safety failures and does not write unsafe YAML.
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: non-blocking
I didn't find a changed-line correctness or security regression in the runner-temp hardening itself.
What I checked
- the new executable-body rewrite/validation pass in
pkg/workflow/runner_temp_safety.go - all direct call sites that now finalize generated YAML
- representative generated workflow diffs for
agentic-auto-upgrade,agentic_commands, andagentics-maintenance - test updates covering github-script and shell rewrites
The background grumpy-coder pass did not produce usable output in this run, so this review is based on my own pass only.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 25.8 AIC · ⌖ 6.79 AIC · ⊞ 7K
Comment /review to run again
…ed workflows Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (591 new lines in business logic directories) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a security-critical fail-open pattern.
📋 Key Themes & Highlights
Key Themes
-
Fail-open on error (security regression):
auto_update_workflow.go,maintenance_workflow_yaml.go, andside_repo_maintenance.goall log-and-return the unsafe YAML whenfinalizeRunnerTempSafetyerrors. This means a validation or rewrite failure silently produces the exact unsafe output this PR is fixing. Compare withcentral_slash_command_workflow.go, which correctly propagates the error — the other three callers should follow that model. -
CRLF edge case in rewriter:
rewriteRunnerTempInExecutableBodiessplits on\nonly. CRLF line endings leave a trailing\ron each line that will defeat regex matching, silently leaving unsafe${{ runner.temp }}expressions in place. A normalisation step + regression test is needed. -
Missing idempotency test: No test covers running the rewriter twice on already-safe output, which could mask a double-injection bug.
Positive Highlights
- ✅
central_slash_command_workflow.goandcompiler_yaml.gocorrectly propagate errors fromfinalizeRunnerTempSafety— good model for the three failing callers. - ✅
validateNoRunnerTempInExecutableBodiescorrectly distinguishes executable bodies from plain YAML field values. - ✅ Test suite covers the main happy path, the shell-command case, and the regression-rejection case.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 65.7 AIC · ⌖ 11.6 AIC · ⊞ 7.8K
Comment /matt to run again
| finalYAML, err := finalizeRunnerTempSafety(yaml) | ||
| if err != nil { | ||
| autoUpdateWorkflowLog.Printf("Runner temp safety validation failed for auto-update workflow: %v", err) | ||
| return yaml |
There was a problem hiding this comment.
[/diagnosing-bugs] Silent fail-open: when finalizeRunnerTempSafety returns an error here, the function logs and returns the unsafe, un-rewritten YAML — the very content this PR exists to eliminate. The CodeQL injection path remains open whenever a rewrite/validation failure occurs.
💡 Suggested fix
Change the return signature to (string, error) to match central_slash_command_workflow.go, or at minimum never return the unsafe YAML:
finalYAML, err := finalizeRunnerTempSafety(yaml)
if err != nil {
return "", fmt.Errorf("runner temp safety: %w", err)
}
return finalYAMLThe same fail-open pattern exists in maintenance_workflow_yaml.go line 63 and side_repo_maintenance.go line 333 — all three need the same fix.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 75c7592: auto-update workflow generation now propagates safety-finalization errors instead of emitting unsafe YAML.
| finalYAML, err := finalizeRunnerTempSafety(yaml.String()) | ||
| if err != nil { | ||
| maintenanceWorkflowYAMLLog.Printf("Runner temp safety validation failed for maintenance workflow: %v", err) | ||
| return yaml.String() |
There was a problem hiding this comment.
[/diagnosing-bugs] Same silent fail-open as auto_update_workflow.go:235 — an error from finalizeRunnerTempSafety silently falls through and returns the unsafe YAML. This negates the security guarantee for the maintenance workflow.
💡 Suggested fix
Change the function signature to return (string, error) and propagate, or treat a safety error as fatal:
finalYAML, err := finalizeRunnerTempSafety(yaml.String())
if err != nil {
return "", fmt.Errorf("runner temp safety: %w", err)
}
return finalYAML, nil@copilot please address this.
There was a problem hiding this comment.
Fixed in 75c7592: maintenance workflow generation now fails closed when runner-temp safety finalization fails.
| finalYAML, err := finalizeRunnerTempSafety(yaml.String()) | ||
| if err != nil { | ||
| maintenanceLog.Printf("Runner temp safety validation failed for side-repo maintenance workflow: %v", err) | ||
| return yaml.String() |
There was a problem hiding this comment.
[/diagnosing-bugs] Same silent fail-open pattern — error from finalizeRunnerTempSafety is logged then the function returns the unpatched YAML. All three fail-open callers should be fixed consistently.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 75c7592: side-repo workflow generation now propagates safety failures before writing output.
| if inScriptBlock { | ||
| if strings.Contains(rewrittenLine, safeActionsDirLine) { | ||
| scriptBlockHasActionsDir = true | ||
| } |
There was a problem hiding this comment.
[/tdd] The rewriteRunnerTempInExecutableBodies function uses \n-only line splitting via strings.SplitAfter(yamlContent, "\n"). On Windows-generated YAML (CRLF), the trailing \r will be included in lineWithoutNewline, causing regex matches against blockScalarHeaderRE and githubScriptActionRequireRE to silently miss, leaving unsafe paths unrewritten.
💡 Suggested fix + test
Normalize line endings before splitting:
func rewriteRunnerTempInExecutableBodies(yamlContent string) string {
yamlContent = strings.ReplaceAll(yamlContent, "\r\n", "\n")
lines := strings.SplitAfter(yamlContent, "\n")
...Add a regression test:
func TestRewriteRunnerTempCRLF(t *testing.T) {
input := "steps:\r\n - run: node ${{ runner.temp }}/gh-aw/actions/foo.cjs\r\n"
got := rewriteRunnerTempInExecutableBodies(input)
if strings.Contains(got, "${{ runner.temp }}") {
t.Fatalf("CRLF input not rewritten: %s", got)
}
}@copilot please address this.
There was a problem hiding this comment.
Added CRLF regression coverage in 75c7592. The existing line handling preserves CRLF and safely rewrites the executable command.
| out.WriteString(indent) | ||
| out.WriteString(safeActionsDirLine) | ||
| out.WriteString("\n") | ||
| scriptBlockHasActionsDir = true |
There was a problem hiding this comment.
[/tdd] No test covers the case where a script: block already has actionsDir declared (e.g. from a partial rewrite) — in that case the function skips injecting the path prelude. A test verifying idempotency would guard against double-injection regressions.
💡 Suggested test
func TestRewriteIsIdempotent(t *testing.T) {
input := `steps:
- uses: actions/github-script@v7
with:
script: |
const path = require('path');
const actionsDir = path.join(process.env.RUNNER_TEMP, 'gh-aw', 'actions');
const { main } = require(path.join(actionsDir, 'generate_aw_info.cjs'));
`
once := rewriteRunnerTempInExecutableBodies(input)
twice := rewriteRunnerTempInExecutableBodies(once)
if once != twice {
t.Fatalf("rewrite is not idempotent\nfirst:\n%s\nsecond:\n%s", once, twice)
}
}@copilot please address this.
There was a problem hiding this comment.
Added an idempotency regression test in 75c7592 for an already-safe github-script block.
There was a problem hiding this comment.
Review: Use safe RUNNER_TEMP paths in generated workflow scripts
The approach is sound: rewrite ${{ runner.temp }}/gh-aw/actions/... expressions in generated YAML to runtime-resolved paths, then validate no unsafe expressions remain. The test coverage is good.
Blocking issues
1. shellActionCommandRE rewrites non-executable YAML fields (see inline comment on line 55)
The shellActionCommandRE.ReplaceAllString runs on every line unconditionally, not just lines in run: executable contexts. A YAML with: sub-field or other non-executable field containing node ${{ runner.temp }}/gh-aw/actions/foo.cjs would be incorrectly rewritten to a shell variable reference "${RUNNER_TEMP}/...". That shell syntax is only valid when executed as shell, not as a static string value. This should be gated on inRunBlock || singleLineExecutableRE.MatchString(lineWithoutNewline).
Non-blocking
2. Bare error return loses context (line 217, compiler_yaml.go) — all other error returns in generateYAML wrap with fmt.Errorf("context: %w", err). This one doesn't.
3. Misleading inScriptBlock variable — tracks only script: blocks; run: block content is implicitly handled by the unconditional shell rewrite. A comment or a parallel inRunBlock flag would clarify the intent.
Logic verified correct
path.joinprelude injection: correctly injected once per script block at the indent of the first matchingrequire()line.- CRLF handling:
strings.SplitAfter(s, "\n")+TrimSuffix(line, "\n")correctly preserves CRLF line endings. - Block exit logic: checking
trimmed != "" && currentIndent <= scriptBlockIndentbefore the enter check correctly handles back-to-back blocks. $$in Go regex replacement:"$${RUNNER_TEMP}/..."correctly produces"${RUNNER_TEMP}/..."in output.- Validation function: correctly rejects unsafe patterns in both block scalar and single-line executable contexts.
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 · 114.7 AIC · ⌖ 9.38 AIC · ⊞ 5.7K
| } | ||
| } | ||
|
|
||
| rewrittenLine := shellActionCommandRE.ReplaceAllString(lineWithoutNewline, `${1} "$${RUNNER_TEMP}/gh-aw/actions/${2}"`) |
There was a problem hiding this comment.
Bug: shellActionCommandRE rewrite runs on ALL YAML lines, not just executable contexts.
The rewrite at this line is unconditional — it fires on every line regardless of whether the line is inside a run: block scalar, a single-line run:, or an unrelated YAML field like command:, args:, or any with: sub-field.
This means a non-executable YAML field containing node ${{ runner.temp }}/gh-aw/actions/foo.cjs would be incorrectly rewritten to the shell-variable form "${RUNNER_TEMP}/gh-aw/actions/foo.cjs" — a form that is only valid in shell contexts, not as a static YAML string value.
The fix is to gate this rewrite on an inRunBlock flag (analogous to inScriptBlock), or at minimum check inRunBlock || singleLineExecutableRE.MatchString(lineWithoutNewline) before applying the substitution:
// Only rewrite shell commands inside run: executable contexts
if inRunBlock || singleLineExecutableRE.MatchString(lineWithoutNewline) {
rewrittenLine = shellActionCommandRE.ReplaceAllString(
lineWithoutNewline, `${1} "$${RUNNER_TEMP}/gh-aw/actions/${2}"`)
} else {
rewrittenLine = lineWithoutNewline
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 75c7592: shell command rewrites are now limited to block and single-line run: contexts; non-executable YAML fields remain unchanged.
| @@ -212,6 +212,11 @@ func (c *Compiler) generateYAML(data *WorkflowData, markdownPath string) (string | |||
| yamlContent = c.replaceIssueNumberReferences(yamlContent) | |||
| } | |||
|
|
|||
| yamlContent, err := finalizeRunnerTempSafety(yamlContent) | |||
| if err != nil { | |||
| return "", nil, nil, err | |||
There was a problem hiding this comment.
Inconsistency: error returned bare without context wrapping.
All other error returns in generateYAML wrap with fmt.Errorf("...%w", err) to provide context. This bare return loses the call site context, making it harder to diagnose failures.
Suggested fix:
yamlContent, err := finalizeRunnerTempSafety(yamlContent)
if err != nil {
return "", nil, nil, fmt.Errorf("failed to finalize runner temp safety in generated YAML: %w", err)
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 75c7592: the compiler now wraps runner-temp safety finalization failures with generated-YAML context.
| } | ||
| } | ||
|
|
||
| rewrittenLine := shellActionCommandRE.ReplaceAllString(lineWithoutNewline, `${1} "$${RUNNER_TEMP}/gh-aw/actions/${2}"`) |
There was a problem hiding this comment.
Misleading state variable name: inScriptBlock tracks only script: blocks, not run: blocks.
The variable inScriptBlock and its companion scriptBlockHasActionsDir are set only when the block header is script: (line 48: inScriptBlock = matches[2] == "script"). The shell command rewrite on line 55 then runs unconditionally on all lines.
This means:
run:block scalar content IS rewritten byshellActionCommandRE(correct), butinScriptBlockisfalseduring that content.- The
require()guard at line 56 (if inScriptBlock) correctly skips JS require-rewriting forrun:blocks.
However, the asymmetry — shell rewrite is unconditional but require-rewrite is guarded — makes the logic hard to follow and maintain. A run: block scalar should ideally be tracked similarly to script:. At minimum, add a comment explaining that shellActionCommandRE is intentionally applied to all lines:
// shellActionCommandRE is intentionally applied to all lines (including run: block scalars
// and single-line run: steps); the inScriptBlock guard below only applies to the JS require() rewrite.
rewrittenLine := shellActionCommandRE.ReplaceAllString(lineWithoutNewline, `${1} "$${RUNNER_TEMP}/gh-aw/actions/${2}"`)@copilot please address this.
There was a problem hiding this comment.
Fixed in 75c7592 by tracking run: block state explicitly, so shell rewrites are scoped to shell-executable contexts.
|
@copilot run pr-finisher skill |
|
@copilot Please take a maintainer-facing finish pass on this PR. Current blockers on this head:
Then run the
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
/smoke-copilot |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
|
@copilot Thanks for the quick follow-up on the review threads here. Several unresolved threads now have author replies on the latest head, but they still need thread-by-thread resolution and a maintainer-facing finish pass. Please:
|
Finish pass complete on Validated locally: Human actions: resolve the nine already-replied review threads, accept/link the ADR in the PR body, and re-trigger CI on this head. Merge state is currently |
|
@copilot Please take a maintainer-facing finish pass on this PR. Current blockers on this head:
Then run the
|
…eql-alerts Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Updated from The previous Smoke Copilot run failed in its agent job without an actionable product-test error in the available logs. CI has no checks on the merged head, so a maintainer must resolve the nine addressed threads and re-trigger CI. |
|
🎉 This pull request is included in a new release. Release: |
gh aw compilegenerated lock workflows that embedded${{ runner.temp }}directly in executableactions/github-scriptJavaScript and shell commands, triggering CodeQL code-injection alerts. This changes generated executable bodies to resolve runner temp paths at runtime instead.Compiler output hardening
github-scriptrequire()paths throughpath.join(process.env.RUNNER_TEMP, ...).${RUNNER_TEMP}paths.${{ runner.temp }}/gh-aw/actionsusage.Generated workflow coverage
pr-sous-chef run https://github.com/github/gh-aw/actions/runs/32416237836> Generated by 👨🍳 PR Sous Chef · gpt54 · 36.4 AIC · ⌖ 10.6 AIC · ⊞ 9.3K · ◷