Replace template-injection grep with parsed workflow check - #55863
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality 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 test file diff
|
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.
Requesting changes
This narrows the false positives from the old grep, but it also weakens the guard from "no GitHub expressions in run scripts" to "only no github.event expressions in run scripts". That leaves a real regression hole for other untrusted contexts that were previously covered by the broader compiler validation.
Blocking themes
- The new test only flags
github.event.*, so direct${{ inputs.* }}and${{ steps.*.outputs.* }}interpolation inrun:blocks can now slip through this daily check even though they are still template-injection sources. - The workflow report now claims the check is a template-injection guard, but it is no longer aligned with the broader unsafe-context validation already implemented in
pkg/workflow.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 6.75 AIC · ⌖ 6.88 AIC · ⊞ 4.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — requesting changes on two correctness gaps and one silent-failure risk.
📋 Key Themes & Highlights
Key Themes
- Unquoted heredoc false negative (
/tdd):removeHeredocContentstrips unquoted heredocs, but GitHub substitutes${{ ... }}before the shell runs — so unquoted heredocs are still an injection vector and the test suite doesn't cover this case. - Narrow injection scope (
/tdd): The guard only matchesgithub.event.*; other well-known injectable contexts (github.head_ref,github.base_ref) are silently ignored with no test or documented rationale. - Silent guard bypass (
/diagnosing-bugs): The workflow script soft-skips whengois not onPATH, exiting 0 and printing an info message. If the container lacks Go, the entire template-injection check is a no-op.
Positive Highlights
- ✅ Replacing the line-oriented grep with an AST-aware parsed check is exactly the right fix — eliminating the
env:false positives was the core problem. - ✅ The test structure is clean and readable (Arrange/Act/Assert per case).
- ✅ Reusing the existing
extractRunBlocks/stripShellLineComments/removeHeredocContenthelpers rather than duplicating parsing logic is good deep-module design.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 48.4 AIC · ⌖ 11.4 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/lock_file_template_injection_test.go:100
[/tdd] The heredoc test only covers a quoted delimiter (<< 'EOF'). removeHeredocContent also strips unquoted heredocs, but GitHub Actions substitutes ${{ ... }} expressions before the shell runs — so content inside an unquoted heredoc is still attacker-controlled at expression-evaluation time. The current test suite won't catch a false negative here.
<details>
<summary>💡 Add an unquoted heredoc test and document the intended behaviour</summary>
{
name: "unquoted h…
</details>
<details><summary>pkg/workflow/lock_file_template_injection_test.go:33</summary>
**[/tdd]** The test checks `github.event.*` expressions but the guard could also miss **`github.event_path`** or **`github.head_ref`** which are equally injectable. The current check `strings.Contains(expression, "github.event.")` is a substring match, not a structural one — `github.event_path` does NOT match `github.event.` (note the dot), so it would silently pass.
<details>
<summary>💡 Broaden the match or add a test for the known cases</summary>
Add tests for the other commonly exploitabl…
</details>
<details><summary>.github/workflows/daily-secrets-analysis.md:10</summary>
**[/diagnosing-bugs]** The `go test` fallback in the workflow script uses `command -v go` to gate execution, and silently skips with an info message when Go is unavailable. This means the template-injection check **never actually runs** inside the compiled workflow's `run:` shell — the workflow executes in a sandboxed container where `go` might not be on `PATH`. If the guard is bypassed at runtime, it provides a false sense of security.
<details>
<summary>💡 Verify the Go toolchain is availabl…
</details>
There was a problem hiding this comment.
The new guard is useful, but the test can be trimmed by leaning on the existing validator instead of a parallel parser. net: -16 lines possible.
Generated by ✂️ Ponytail Reviewer for #55863 · codex · mai10 · 7.42 AIC · ⌖ 1.94 AIC · ⊞ 16.7K
Comment /ponytail to run again
| // findGitHubEventRunExpressions parses a compiled workflow and returns github.event | ||
| // expressions that reach an executable run: block. Expressions assigned to env: are | ||
| // deliberately excluded because the shell receives their values as data. | ||
| func findGitHubEventRunExpressions(lockContent []byte) ([]string, error) { |
There was a problem hiding this comment.
L19-35: shrink: bespoke YAML walk and string scan for github.event in run blocks. Reuse the existing validator path in pkg/workflow/template_injection_validation.go and assert on its error.
There was a problem hiding this comment.
Good improvement — replacing the fragile grep with parsed YAML inspection eliminates false positives from env: assignments and is a clear correctness win.
Two non-blocking issues worth addressing:
- Coverage gap (line 29):
strings.Contains(expression, "github.event.")(trailing dot) won't catch${{ toJSON(github.event) }}or bare${{ github.event }}. Drop the trailing dot. - Test isolation (line 50):
require.NoErroron YAML parse failures callst.FailNow()and masks results for all remaining lock files. Preferassert.NoError+continue.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 38.2 AIC · ⌖ 9.28 AIC · ⊞ 6.2K
| for _, runContent := range extractRunBlocks(workflow) { | ||
| executableContent := stripShellLineComments(removeHeredocContent(runContent)) | ||
| for _, expression := range InlineExpressionPattern.FindAllString(executableContent, -1) { | ||
| if strings.Contains(expression, "github.event.") { |
There was a problem hiding this comment.
The check strings.Contains(expression, "github.event.") (note the trailing dot) misses expressions like ${{ toJSON(github.event) }} or ${{ github.event }} that access the full event object without a property suffix. These can still carry attacker-controlled data and represent real injection risks.
Consider broadening the match to strings.Contains(expression, "github.event") (no trailing dot) and add a test case:
{
name: "toJSON(github.event) in run is reported",
yaml: `
jobs:
test:
steps:
- run: echo "${{ toJSON(github.event) }}"
`,
expected: []string{"${{ toJSON(github.event) }}"},
},@copilot please address this.
| require.NoError(t, err, "should read %s", lockFile) | ||
|
|
||
| violations, err := findGitHubEventRunExpressions(lockContent) | ||
| require.NoError(t, err, "should parse %s as YAML", lockFile) |
There was a problem hiding this comment.
The loop uses require.NoError for YAML parse errors, which calls t.FailNow() and stops the entire test on the first unparseable file. This means a corrupted lock file silently hides failures in all subsequent files.
Consider using assert.NoError + continue so all files are checked:
violations, err := findGitHubEventRunExpressions(lockContent)
if !assert.NoError(t, err, "should parse %s as YAML", lockFile) {
continue
}
assert.Empty(t, violations, ...)@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Replaces a noisy grep-based template-injection check with parsed workflow inspection.
Changes:
- Adds a Go guard for
github.event.*expressions inrun:blocks. - Integrates the guard into the daily secrets report.
- Regenerates workflow metadata.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/lock_file_template_injection_test.go |
Adds detection and regression tests. |
.github/workflows/daily-secrets-analysis.md |
Runs the parsed-workflow guard. |
.github/workflows/daily-secrets-analysis.lock.yml |
Updates generated metadata. |
Review details
Suppressed comments (1)
pkg/workflow/lock_file_template_injection_test.go:29
- This textual scan misses valid unsafe expressions.
InlineExpressionPatternfails on${{ format('{0}', github.event.issue.title) }}because the string literal contains}, and the subsequent substring check misses equivalent index syntax such as${{ github['event']['issue']['title'] }}. Use expression-aware extraction/reference detection and add both forms as regressions; otherwise the new security gate can pass direct event-data interpolation.
for _, expression := range InlineExpressionPattern.FindAllString(executableContent, -1) {
if strings.Contains(expression, "github.event.") {
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
|
|
||
| var violations []string | ||
| for _, runContent := range extractRunBlocks(workflow) { | ||
| executableContent := stripShellLineComments(removeHeredocContent(runContent)) |
| } | ||
|
|
||
| var violations []string | ||
| for _, runContent := range extractRunBlocks(workflow) { |
|
Test comment
|
🏗️ Design Decision Gate — ADR RequiredThis PR adds 111 lines to No ADR was found in the PR body, in What to doAdd an ADR at 📄 Draft ADR (click to expand)# ADR-55863: Replace Template-Injection Grep with Go AST-Based Check
**Date**: 2026-08-25
**Status**: Draft
**Deciders**: Unknown
---
### Context
The Daily Secrets Analysis report used a line-oriented grep (`grep -rn "github.event."`) to flag potential template-injection risks in compiled `.lock.yml` workflow files. Because grep cannot distinguish YAML structural contexts, it matched `github.event.*` references inside `env:` assignments—which are safe, as the shell receives those values as data—as well as inside shell comments and heredoc content. On the 2026-08-25 run this produced 4,946 flagged lines, explicitly noted as a "high false-positive rate," making the signal unreliable and requiring manual review of every run. The existing `pkg/workflow` package already parses `${{ }}` expressions during lock-file compilation, providing a reusable YAML/expression-parsing foundation for a precise check.
### Decision
We will replace the heuristic grep with a Go test (`TestCompiledLockFiles_NoGitHubEventExpressionsInRunScripts`) in `pkg/workflow/` that YAML-parses each compiled `.lock.yml` file, walks only the `run:` script values, strips shell comments and heredoc content, and reports any `github.event.*` inline expressions it finds there. The daily report's bash section delegates to this Go test when the Go toolchain is available, and prints an informational skip message otherwise. This produces a deterministic, zero-false-positive signal that can also gate CI.
### Alternatives Considered
#### Alternative 1: Enhanced Multi-Stage Grep
Extend the existing grep with additional pipe stages to exclude `env:` lines (e.g., `grep ... | grep -v "env:"` or negative-lookahead patterns). This avoids introducing a Go dependency in the daily report script. Rejected because text-based approaches cannot reliably distinguish structural YAML contexts—`env:` may appear in a block nested under `run:`, and line-by-line filtering cannot resolve that ambiguity without parsing the YAML structure.
#### Alternative 2: Accept False Positives and Review Manually
Keep the existing grep and rely on human reviewers to eyeball each hit per report cycle. No code change required. Rejected because 4,946 hits per run makes manual review infeasible; the signal is so noisy it is effectively suppressed in practice, leaving real template-injection risks undetected.
### Consequences
#### Positive
- Eliminates false positives: `env:` assignments, shell comments, and heredoc content are no longer flagged, reducing noise from ~4,946 hits to only genuine `run:` interpolations.
- The check is enforceable in CI as a standard Go test, not just during the daily report.
- Reuses the existing YAML and expression-parsing infrastructure in `pkg/workflow/`, minimising new code surface.
- Unit tests cover safe and unsafe patterns explicitly, making the detection logic auditable and easy to extend.
#### Negative
- Requires the Go toolchain at daily-report execution time; when Go is unavailable the shell script emits an informational skip and defers enforcement to CI, creating a gap in local/ad-hoc runs.
- Adds a new test file to `pkg/workflow/`, increasing the package's test surface and the scope of changes that could break the guard.
#### Neutral
- The daily-report bash section is simplified by delegating detection logic to the Go test rather than maintaining inline grep pipelines.
- The guard runs only over compiled `.lock.yml` files, not over raw `.md` skill sources, which is consistent with the threat model (compiled files are what GitHub Actions actually executes).
---
*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*
|
|
🎉 This pull request is included in a new release. Release: |
Daily Secrets Analysis reported thousands of
github.event.*matches using a line-oriented grep that could not distinguishenv:assignments from executable shell interpolation. This replaces it with a deterministic parsed-workflow check targeting only executablerun:blocks.Template-injection guard
.lock.yml.run:script values for directgithub.event.*expressions.env:, shell comments, and heredoc content.Daily report integration
Regression coverage