Skip to content

perf(workflow): cut YAMLGeneration overhead in run-script expression guardrail #44438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jul 9, 2026
+37 −1
Merged

perf(workflow): cut YAMLGeneration overhead in run-script expression guardrail #44438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c365fe4
Initial plan
Copilot Jul 9, 2026
3ac7e30
perf: skip run-script expression guardrail work when no inline expres…
Copilot Jul 9, 2026
09cc500
test(benchmark): add b.ResetTimer() after YAML parsing setup
Copilot Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/workflow/template_injection_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,10 @@ func validateNoGitHubExpressionsInRunScriptsFromParsed(workflow map[string]any)
var violations []TemplateInjectionViolation

for _, runContent := range runBlocks {
if !mayContainInlineExpression(runContent) {

@github-actions github-actions Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The sibling function validateNoTemplateInjectionFromParsed (line 183) uses InlineExpressionPattern.MatchString(runContent) as its precheck, while this function now uses mayContainInlineExpression(runContent). These are semantically different — full regex vs cheap substring scan — and the asymmetry may confuse future readers.

💡 Suggested clarifying comment

A brief inline comment would make the intentional trade-off explicit:

// Use the cheap substring precheck (not InlineExpressionPattern.MatchString as in
// validateNoTemplateInjectionFromParsed) because for the common no-expression case
// this avoids regex engine overhead. mayContainInlineExpression is zero-false-negative:
// any real "${{ ... }}" must contain both substrings.
if !mayContainInlineExpression(runContent) {
    continue
}

Alternatively, the sibling function at line 183 could be updated to use mayContainInlineExpression for consistency — it would be a strict improvement there too.

@copilot please address this.

@github-actions github-actions Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guard asymmetry with sibling function makes security auditing harder: validateNoTemplateInjectionFromParsed (line 183) uses the full compiled regex InlineExpressionPattern.MatchString(runContent) as its fast-path guard, while this new guard uses the cheaper mayContainInlineExpression. There is no comment explaining the intentional divergence.

💡 Details

Both guards are safe in terms of false negatives — mayContainInlineExpression is documented as zero-false-negative. But the two functions now have subtly different skip conditions: mayContainInlineExpression accepts false positives (e.g. a run block containing ${{ in a heredoc body and }} elsewhere) that the full regex would reject. A security auditor comparing the two functions will see inconsistent guards and need to reason through both to confirm neither introduces a bypass. A short inline comment explaining why the cheaper check is correct here would close this gap:

// mayContainInlineExpression is zero-false-negative (see its doc), so it is
// safe to use here instead of the full InlineExpressionPattern regex.
if !mayContainInlineExpression(runContent) {
    continue
}

continue
}

// Align with template-injection validation by excluding non-executable regions:
// heredoc bodies and bash # comments.
contentWithoutHeredocs := stripShellLineComments(removeHeredocContent(runContent))
Expand Down
34 changes: 33 additions & 1 deletion pkg/workflow/template_injection_validation_benchmark_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package workflow

import "testing"
import (
"testing"

"github.com/goccy/go-yaml"
)

func BenchmarkValidateTemplateInjectionFastPath(b *testing.B) {
compiler := NewCompiler()
Expand All @@ -27,3 +31,31 @@ jobs:
}
}
}

func BenchmarkValidateNoGitHubExpressionsInRunScriptsFromParsed_NoExpressions(b *testing.B) {

@github-actions github-actions Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The benchmark covers the no-expression fast path but there is no unit test that exercises validateNoGitHubExpressionsInRunScriptsFromParsed directly in template_injection_validation_test.go. Searching the test file shows no TestValidateNoGitHubExpressions* function at all — coverage of this function relies entirely on integration-level compiler tests.

💡 Suggested additions

Adding targeted unit tests would make the fast-path correctness guarantee explicit and catch regressions faster:

func TestValidateNoGitHubExpressionsInRunScriptsFromParsed(t *testing.T) {
    tests := []struct {
        name    string
        yaml    string
        wantErr bool
    }{
        {
            name: "no expressions — fast path, no error",
            yaml: `jobs:
  test:
    steps:
      - run: echo hello`,
            wantErr: false,
        },
        {
            name: "allowed generated expression passes",
            yaml: `jobs:
  test:
    steps:
      - run: node ${{ runner.temp }}/foo.cjs`,
            wantErr: false,
        },
        {
            name: "disallowed raw expression is rejected",
            yaml: `jobs:
  test:
    steps:
      - run: echo ${{ github.actor }}`,
            wantErr: true,
        },
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            var parsed map[string]any
            require.NoError(t, yaml.Unmarshal([]byte(tt.yaml), &parsed))
            err := validateNoGitHubExpressionsInRunScriptsFromParsed(parsed)
            if tt.wantErr {
                require.Error(t, err)
            } else {
                require.NoError(t, err)
            }
        })
    }
}

The no-expression case is particularly important to lock in the short-circuit behaviour introduced by this PR.

@copilot please address this.

@github-actions github-actions Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark only covers the fast-path skip; the optimization's benefit is not measurable from this benchmark alone: The new benchmark measures a single strings.Cut + strings.Contains call on a short fixed string. It cannot demonstrate the actual performance gain, because there is no companion benchmark for the slow path (run blocks with expressions present).

💡 Details

The PR body claims a significant regression was fixed in BenchmarkYAMLGeneration. Without a slow-path counterpart (e.g. BenchmarkValidateNoGitHubExpressionsInRunScriptsFromParsed_WithExpressions), future contributors cannot:

  1. Verify the before/after improvement with go test -bench=.
  2. Detect a future regression where the slow path gets expensive again

A minimal companion benchmark:

func BenchmarkValidateNoGitHubExpressionsInRunScriptsFromParsed_WithExpressions(b *testing.B) {
    // workflow with allowed expressions present
    yamlContent := `name: with-expressions\non: workflow_dispatch\njobs:\n  test:\n    runs-on: ubuntu-latest\n    steps:\n      - run: echo ${{ env.FOO }}\n`
    var parsed map[string]any
    if err := yaml.Unmarshal([]byte(yamlContent), &parsed); err != nil {
        b.Fatal(err)
    }
    b.ReportAllocs()
    for i := 0; i < b.N; i++ {
        _ = validateNoGitHubExpressionsInRunScriptsFromParsed(parsed)
    }
}

This also locks in that the slow path itself does not regress in future refactors.

yamlContent := `
name: no-inline-expression
on: workflow_dispatch
jobs:
test:
runs-on: ubuntu-latest
steps:
- name: No expression run
run: |
echo "hello"
echo "world"
`

var parsed map[string]any
if err := yaml.Unmarshal([]byte(yamlContent), &parsed); err != nil {
b.Fatal(err)
}

b.ResetTimer()
b.ReportAllocs()
for b.Loop() {
Comment on lines +49 to +56

@github-actions github-actions Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new benchmark uses b.Loop() while the existing benchmark in this file uses for i := 0; i < b.N; i++ with an explicit nolint comment:

(nolint/redacted):intrange // Use the standard testing.B loop form for broad tool compatibility.
for i := 0; i < b.N; i++ {

Consider using the same b.N form here for consistency and to match the stated codebase policy:

(nolint/redacted):intrange // Use the standard testing.B loop form for broad tool compatibility.
for i := 0; i < b.N; i++ {
    if err := validateNoGitHubExpressionsInRunScriptsFromParsed(parsed); err != nil {
        b.Fatalf("validateNoGitHubExpressionsInRunScriptsFromParsed() error = %v", err)
    }
}

b.Loop() is fine in Go 1.24+ (this module declares go 1.26.3), but the inconsistency with the adjacent benchmark and its explanatory comment is worth aligning.

@copilot please address this.

@github-actions github-actions Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] Benchmark loop style is inconsistent with the sibling benchmark in this file — the existing BenchmarkValidateTemplateInjectionFastPath uses for i := 0; i < b.N; i++ with a //nolint:intrange comment explaining the choice for "broad tool compatibility".

💡 Options

b.Loop() is actually the better API (Go 1.24+): it resets timers correctly around the loop body. The rest of the codebase already uses it. Two paths forward:

  1. Preferred: Update the sibling BenchmarkValidateTemplateInjectionFastPath to for b.Loop() as a follow-up (the //nolint:intrange comment becomes unnecessary since b.Loop() is not a range form). This makes the file consistent and removes the stale nolint comment.
  2. Conservative: Add a brief comment to the new benchmark explaining the choice, e.g. // b.Loop() — preferred over b.N loop since Go 1.24 (resets timer correctly).

Neither blocks this PR, but leaving two incompatible patterns in the same file is a maintenance hazard.

@copilot please address this.

@github-actions github-actions Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b.Loop() inconsistent with the existing benchmark's explicit b.N style in the same file: The benchmark immediately above uses for i := 0; i < b.N; i++ with a //nolint:intrange comment explaining the rationale for that style ('broad tool compatibility'). This new benchmark silently adopts the opposite convention.

💡 Details

Either both benchmarks should use the same style, or this one should carry a comment explaining the intentional divergence. Without that, the next maintainer reading both benchmarks back-to-back will wonder which convention to follow when adding new benchmarks.

To stay consistent with the existing benchmark:

b.ReportAllocs()
(nolint/redacted):intrange // Use the standard testing.B loop form for broad tool compatibility.
for i := 0; i < b.N; i++ {
    if err := validateNoGitHubExpressionsInRunScriptsFromParsed(parsed); err != nil {
        b.Fatalf("validateNoGitHubExpressionsInRunScriptsFromParsed() error = %v", err)
    }
}

Or, if b.Loop() is the desired going-forward style, update the existing benchmark and remove its nolint comment too.

if err := validateNoGitHubExpressionsInRunScriptsFromParsed(parsed); err != nil {
b.Fatalf("validateNoGitHubExpressionsInRunScriptsFromParsed() error = %v", err)
}
}
}
Loading