From 96fb06ef2d7451dc82c1b3d96d4c762bbf594c6c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 24 Jan 2026 03:14:40 +0000 Subject: [PATCH 1/4] Initial plan From bd49b91f03af5293ca6f967f0629d0338e56b1fe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 24 Jan 2026 03:27:41 +0000 Subject: [PATCH 2/4] Fix template injection validator to handle YAML key ordering - Updated runBlockRegex to match both run: | and run: |- indicators - Fixed regex to stop at any YAML key at step level (env:, if:, with:, etc.) - Added comprehensive tests for various YAML key orderings - Tests verify safe patterns (env after run) and unsafe patterns still detected Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/template_injection_validation.go | 7 +- .../template_injection_validation_test.go | 161 ++++++++++++++++++ 2 files changed, 165 insertions(+), 3 deletions(-) diff --git a/pkg/workflow/template_injection_validation.go b/pkg/workflow/template_injection_validation.go index 8b8b4eb762..d0721f4974 100644 --- a/pkg/workflow/template_injection_validation.go +++ b/pkg/workflow/template_injection_validation.go @@ -61,12 +61,13 @@ var ( // runBlockRegex matches YAML run: blocks and captures their content // This regex matches both single-line and multi-line run commands in YAML // Pattern explanation: - // ^\s+run:\s*\|\s*\n((?:[ \t]+.+\n?)+?)\s*(?:^[ \t]*-\s|\z) - matches multi-line block scalar (run: |) - // - Stops at next step (^[ \t]*-\s) or end of string (\z) + // ^\s+run:\s*\|(?:-|)\s*\n((?:[ \t]+.+\n?)+?)\s*(?:^[ \t]*(?:-\s|[a-z-]+:)|\z) - matches multi-line block scalar (run: | or run: |-) + // - Matches both run: | (keep) and run: |- (strip) indicators + // - Stops at next step (^[ \t]*-\s), next YAML key (^[ \t]*[a-z-]+:), or end of string (\z) // | - OR // ^\s+run:\s*(.+)$ - matches single-line run command // Group 1 = multi-line content, Group 2 = single-line content - runBlockRegex = regexp.MustCompile(`(?m)^\s+run:\s*\|\s*\n((?:[ \t]+.+\n?)+?)\s*(?:^[ \t]*-\s|\z)|^\s+run:\s*(.+)$`) + runBlockRegex = regexp.MustCompile(`(?m)^\s+run:\s*\|(?:-|)\s*\n((?:[ \t]+.+\n?)+?)\s*(?:^[ \t]*(?:-\s|[a-z-]+:)|\z)|^\s+run:\s*(.+)$`) // inlineExpressionRegex matches GitHub Actions template expressions ${{ ... }} inlineExpressionRegex = regexp.MustCompile(`\$\{\{[^}]+\}\}`) diff --git a/pkg/workflow/template_injection_validation_test.go b/pkg/workflow/template_injection_validation_test.go index 8a2064f6b1..260e50c7c1 100644 --- a/pkg/workflow/template_injection_validation_test.go +++ b/pkg/workflow/template_injection_validation_test.go @@ -746,3 +746,164 @@ EOF`, }) } } + +// TestTemplateInjectionYAMLKeyOrdering tests that validation correctly handles +// different YAML key orderings, particularly when env: appears after run: +func TestTemplateInjectionYAMLKeyOrdering(t *testing.T) { + tests := []struct { + name string + yaml string + shouldError bool + description string + }{ + { + name: "safe - env after run with pipe keep indicator", + yaml: `jobs: + test: + steps: + - name: Use value safely + run: | + echo "Line 1" + echo "Value: $MY_VALUE" + env: + MY_VALUE: ${{ steps.get_value.outputs.value }}`, + shouldError: false, + description: "Expression in env block should be safe even when env comes after run", + }, + { + name: "safe - env after run with pipe strip indicator", + yaml: `jobs: + test: + steps: + - name: Use value safely + run: |- + echo "Line 1" + echo "Value: $MY_VALUE" + env: + MY_VALUE: ${{ steps.get_value.outputs.value }}`, + shouldError: false, + description: "Expression in env block should be safe with run: |- (strip indicator)", + }, + { + name: "safe - env before run (original ordering)", + yaml: `jobs: + test: + steps: + - name: Use value safely + env: + MY_VALUE: ${{ steps.get_value.outputs.value }} + run: | + echo "Value: $MY_VALUE"`, + shouldError: false, + description: "Expression in env block should be safe when env comes before run", + }, + { + name: "safe - multiple YAML keys after run", + yaml: `jobs: + test: + steps: + - name: Complex step + run: | + echo "Value: $MY_VALUE" + env: + MY_VALUE: ${{ steps.get_value.outputs.value }} + if: always() + continue-on-error: true`, + shouldError: false, + description: "Multiple YAML keys after run should not be captured", + }, + { + name: "safe - with block after run", + yaml: `jobs: + test: + steps: + - name: Use action + run: echo "Running" + with: + value: ${{ steps.get_value.outputs.value }}`, + shouldError: false, + description: "with block after run should not be captured as run content", + }, + { + name: "unsafe - expression directly in run block", + yaml: `jobs: + test: + steps: + - name: Unsafe usage + run: | + echo "Value: ${{ steps.get_value.outputs.value }}" + env: + OTHER: "safe"`, + shouldError: true, + description: "Expression directly in run block should still be detected", + }, + { + name: "safe - env after run in custom job step", + yaml: `jobs: + test: + runs-on: ubuntu-latest + steps: + - id: get_value + run: echo "value=test" >> $GITHUB_OUTPUT + - name: Sign image + run: | + echo "Signing image with digest: $DIGEST" + env: + DIGEST: ${{ steps.build.outputs.digest }}`, + shouldError: false, + description: "Custom job step with env after run should be safe (reproduces issue)", + }, + { + name: "safe - if condition after run", + yaml: `jobs: + test: + steps: + - name: Conditional run + run: echo "test" + if: ${{ steps.get_value.outputs.value == 'test' }}`, + shouldError: false, + description: "if condition after run should not be captured", + }, + { + name: "safe - timeout-minutes after run", + yaml: `jobs: + test: + steps: + - name: Timed run + run: | + echo "Running with timeout" + timeout-minutes: 5 + env: + VALUE: ${{ steps.get_value.outputs.value }}`, + shouldError: false, + description: "timeout-minutes and env after run should not be captured", + }, + { + name: "safe - continue-on-error after run", + yaml: `jobs: + test: + steps: + - name: Allowed failure + run: echo "test" + continue-on-error: true + env: + VALUE: ${{ steps.get_value.outputs.value }}`, + shouldError: false, + description: "continue-on-error and env after run should not be captured", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateNoTemplateInjection(tt.yaml) + + if tt.shouldError { + require.Error(t, err, tt.description) + assert.Contains(t, err.Error(), "template injection", + "Error should mention template injection") + } else { + assert.NoError(t, err, tt.description) + } + }) + } +} From 9450b7fbabc616d23fb0d7722a32c8bbf55caea8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 24 Jan 2026 05:58:26 +0000 Subject: [PATCH 3/4] Refactor template injection validator to use YAML parsing - Replaced regex-based extraction with proper YAML tree walking - Added extractRunBlocks() to recursively extract run: fields from parsed YAML - Fixed test YAML to use proper quoting (invalid YAML was causing parse errors) - Validates by parsing YAML and walking the tree structure - More robust and handles all YAML key orderings correctly Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/template_injection_validation.go | 66 +++++++++++-------- .../template_injection_validation_test.go | 4 +- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/pkg/workflow/template_injection_validation.go b/pkg/workflow/template_injection_validation.go index d0721f4974..89d0a1daaf 100644 --- a/pkg/workflow/template_injection_validation.go +++ b/pkg/workflow/template_injection_validation.go @@ -52,23 +52,13 @@ import ( "strings" "github.com/githubnext/gh-aw/pkg/logger" + "github.com/goccy/go-yaml" ) var templateInjectionValidationLog = logger.New("workflow:template_injection_validation") // Pre-compiled regex patterns for template injection detection var ( - // runBlockRegex matches YAML run: blocks and captures their content - // This regex matches both single-line and multi-line run commands in YAML - // Pattern explanation: - // ^\s+run:\s*\|(?:-|)\s*\n((?:[ \t]+.+\n?)+?)\s*(?:^[ \t]*(?:-\s|[a-z-]+:)|\z) - matches multi-line block scalar (run: | or run: |-) - // - Matches both run: | (keep) and run: |- (strip) indicators - // - Stops at next step (^[ \t]*-\s), next YAML key (^[ \t]*[a-z-]+:), or end of string (\z) - // | - OR - // ^\s+run:\s*(.+)$ - matches single-line run command - // Group 1 = multi-line content, Group 2 = single-line content - runBlockRegex = regexp.MustCompile(`(?m)^\s+run:\s*\|(?:-|)\s*\n((?:[ \t]+.+\n?)+?)\s*(?:^[ \t]*(?:-\s|[a-z-]+:)|\z)|^\s+run:\s*(.+)$`) - // inlineExpressionRegex matches GitHub Actions template expressions ${{ ... }} inlineExpressionRegex = regexp.MustCompile(`\$\{\{[^}]+\}\}`) @@ -83,24 +73,22 @@ var ( func validateNoTemplateInjection(yamlContent string) error { templateInjectionValidationLog.Print("Validating compiled YAML for template injection risks") - // Find all run: blocks in the YAML - runMatches := runBlockRegex.FindAllStringSubmatch(yamlContent, -1) - templateInjectionValidationLog.Printf("Found %d run blocks to scan", len(runMatches)) + // Parse YAML to walk the tree and extract run fields + var workflow map[string]any + if err := yaml.Unmarshal([]byte(yamlContent), &workflow); err != nil { + templateInjectionValidationLog.Printf("Failed to parse YAML: %v", err) + // Fall back to skipping validation if YAML is malformed + // (compilation would have already failed if YAML is invalid) + return nil + } - var violations []TemplateInjectionViolation + // Extract all run blocks from the workflow + runBlocks := extractRunBlocks(workflow) + templateInjectionValidationLog.Printf("Found %d run blocks to scan", len(runBlocks)) - for _, match := range runMatches { - // Extract run content from the regex match groups - // Group 1 = multi-line block, Group 2 = single-line command - var runContent string - if len(match) > 1 && match[1] != "" { - runContent = match[1] // Multi-line run block - } else if len(match) > 2 && match[2] != "" { - runContent = match[2] // Single-line run command - } else { - continue - } + var violations []TemplateInjectionViolation + for _, runContent := range runBlocks { // Check if this run block contains inline expressions if !inlineExpressionRegex.MatchString(runContent) { continue @@ -140,6 +128,32 @@ func validateNoTemplateInjection(yamlContent string) error { return nil } +// extractRunBlocks walks the YAML tree and extracts all run: field values +func extractRunBlocks(data any) []string { + var runBlocks []string + + switch v := data.(type) { + case map[string]any: + // Check if this map has a "run" key + if runValue, ok := v["run"]; ok { + if runStr, ok := runValue.(string); ok { + runBlocks = append(runBlocks, runStr) + } + } + // Recursively process all values in the map + for _, value := range v { + runBlocks = append(runBlocks, extractRunBlocks(value)...) + } + case []any: + // Recursively process all items in the slice + for _, item := range v { + runBlocks = append(runBlocks, extractRunBlocks(item)...) + } + } + + return runBlocks +} + // removeHeredocContent removes heredoc sections from shell commands // Heredocs (e.g., cat > file << 'EOF' ... EOF) are safe for template expressions // because the content is written to files, not executed in the shell diff --git a/pkg/workflow/template_injection_validation_test.go b/pkg/workflow/template_injection_validation_test.go index 260e50c7c1..c5f44211e3 100644 --- a/pkg/workflow/template_injection_validation_test.go +++ b/pkg/workflow/template_injection_validation_test.go @@ -110,7 +110,7 @@ func TestValidateNoTemplateInjection(t *testing.T) { runs-on: ubuntu-latest steps: - name: Single line unsafe - run: echo "PR title: ${{ github.event.pull_request.title }}"`, + run: 'echo "PR title: ${{ github.event.pull_request.title }}"'`, shouldError: true, errorString: "github.event", }, @@ -548,7 +548,7 @@ func TestTemplateInjectionEdgeCases(t *testing.T) { runs-on: ubuntu-latest steps: - name: Test - run: echo "Issue: ${{ github.event.issue.title }}"`, + run: 'echo "Issue: ${{ github.event.issue.title }}"'`, shouldError: true, description: "Expressions with extra whitespace should still be detected", }, From dab38bcb861bd5db44eca5bb429c2c86ee32c22f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 24 Jan 2026 10:44:11 +0000 Subject: [PATCH 4/4] Add comprehensive tests for invalid YAML and edge cases - Added TestTemplateInjectionInvalidYAML with 10 test cases for malformed YAML - Added TestTemplateInjectionYAMLParsingEdgeCases with 14 test cases for YAML parsing edge cases - Tests cover: invalid YAML syntax, empty/null values, non-string run fields, complex nesting - Tests verify graceful handling of parse errors (skip validation without failing) - All 69 template injection tests pass Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../template_injection_validation_test.go | 325 ++++++++++++++++++ 1 file changed, 325 insertions(+) diff --git a/pkg/workflow/template_injection_validation_test.go b/pkg/workflow/template_injection_validation_test.go index c5f44211e3..32c1ada178 100644 --- a/pkg/workflow/template_injection_validation_test.go +++ b/pkg/workflow/template_injection_validation_test.go @@ -907,3 +907,328 @@ func TestTemplateInjectionYAMLKeyOrdering(t *testing.T) { }) } } + +// TestTemplateInjectionInvalidYAML tests how the validator handles invalid YAML +func TestTemplateInjectionInvalidYAML(t *testing.T) { + tests := []struct { + name string + yaml string + shouldError bool + description string + }{ + { + name: "malformed YAML - missing closing bracket", + yaml: `jobs: + test: + runs-on: ubuntu-latest + steps: + - name: Test + run: echo "${{ github.event.issue.title"`, + shouldError: false, + description: "Invalid YAML should skip validation gracefully (no error)", + }, + { + name: "malformed YAML - invalid indentation", + yaml: `jobs: + test: +runs-on: ubuntu-latest + steps: + - name: Test + run: echo "${{ github.event.issue.title }}"`, + shouldError: false, + description: "Invalid indentation should skip validation gracefully", + }, + { + name: "malformed YAML - missing colon", + yaml: `jobs + test: + runs-on: ubuntu-latest + steps: + - name: Test + run: echo "${{ github.event.issue.title }}"`, + shouldError: false, + description: "Missing colon should skip validation gracefully", + }, + { + name: "malformed YAML - tabs instead of spaces", + yaml: "jobs:\n\ttest:\n\t\truns-on: ubuntu-latest\n\t\tsteps:\n\t\t\t- name: Test\n\t\t\t\trun: echo \"${{ github.event.issue.title }}\"", + shouldError: false, + description: "Tabs in YAML should be handled (may or may not parse)", + }, + { + name: "empty YAML", + yaml: "", + shouldError: false, + description: "Empty YAML should not cause errors", + }, + { + name: "whitespace only YAML", + yaml: " \n\n \n", + shouldError: false, + description: "Whitespace-only YAML should not cause errors", + }, + { + name: "malformed YAML - unquoted colon in value", + yaml: `jobs: + test: + runs-on: ubuntu-latest + steps: + - name: Test + run: echo "Issue: ${{ github.event.issue.title }}"`, + shouldError: false, + description: "Unquoted colon in string value is invalid YAML, should skip gracefully", + }, + { + name: "valid YAML with complex nested structure", + yaml: `jobs: + test: + runs-on: ubuntu-latest + strategy: + matrix: + version: [1, 2, 3] + steps: + - name: Test with unsafe expression + run: 'echo "Version: ${{ github.event.issue.number }}"'`, + shouldError: true, + description: "Complex nested YAML with unsafe expression should be detected", + }, + { + name: "valid YAML with multiple jobs", + yaml: `jobs: + build: + runs-on: ubuntu-latest + steps: + - name: Build + env: + TITLE: ${{ github.event.issue.title }} + run: echo "$TITLE" + test: + runs-on: ubuntu-latest + steps: + - name: Test unsafe + run: 'echo "${{ github.event.issue.body }}"'`, + shouldError: true, + description: "Multiple jobs with one unsafe expression should be detected", + }, + { + name: "YAML with comments and unsafe expression", + yaml: `# This is a workflow +jobs: + test: + runs-on: ubuntu-latest + # These are steps + steps: + - name: Test + # Unsafe usage + run: 'echo "${{ github.event.issue.title }}"'`, + shouldError: true, + description: "YAML with comments should parse correctly and detect unsafe patterns", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateNoTemplateInjection(tt.yaml) + + if tt.shouldError { + require.Error(t, err, tt.description) + assert.Contains(t, err.Error(), "template injection", + "Error should mention template injection") + } else { + assert.NoError(t, err, tt.description) + } + }) + } +} + +// TestTemplateInjectionYAMLParsingEdgeCases tests edge cases in YAML parsing +func TestTemplateInjectionYAMLParsingEdgeCases(t *testing.T) { + tests := []struct { + name string + yaml string + shouldError bool + description string + }{ + { + name: "run field with null value", + yaml: `jobs: + test: + steps: + - name: Test + run: null`, + shouldError: false, + description: "Null run value should not cause errors", + }, + { + name: "run field with numeric value (invalid but should handle)", + yaml: `jobs: + test: + steps: + - name: Test + run: 123`, + shouldError: false, + description: "Non-string run value should be skipped", + }, + { + name: "run field with boolean value", + yaml: `jobs: + test: + steps: + - name: Test + run: true`, + shouldError: false, + description: "Boolean run value should be skipped", + }, + { + name: "run field with array value (invalid)", + yaml: `jobs: + test: + steps: + - name: Test + run: + - echo "test" + - echo "test2"`, + shouldError: false, + description: "Array run value should be skipped", + }, + { + name: "run field with map value (invalid)", + yaml: `jobs: + test: + steps: + - name: Test + run: + command: echo "test"`, + shouldError: false, + description: "Map run value should be skipped", + }, + { + name: "deeply nested jobs structure", + yaml: `jobs: + test: + runs-on: ubuntu-latest + container: + image: ubuntu:latest + options: --privileged + services: + postgres: + image: postgres + steps: + - name: Test + env: + VALUE: ${{ steps.test.outputs.value }} + run: echo "$VALUE"`, + shouldError: false, + description: "Deeply nested structure should parse correctly", + }, + { + name: "steps with uses instead of run", + yaml: `jobs: + test: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + - name: Setup + uses: actions/setup-node@v4 + with: + node-version: 20`, + shouldError: false, + description: "Steps without run fields should not cause errors", + }, + { + name: "mix of uses and run steps", + yaml: `jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Build + run: npm run build + - name: Test unsafe + run: 'echo "${{ github.event.issue.title }}"' + - uses: actions/upload-artifact@v4`, + shouldError: true, + description: "Mix of uses and run steps should detect unsafe run blocks", + }, + { + name: "empty steps array", + yaml: `jobs: + test: + runs-on: ubuntu-latest + steps: []`, + shouldError: false, + description: "Empty steps array should not cause errors", + }, + { + name: "missing steps field", + yaml: `jobs: + test: + runs-on: ubuntu-latest`, + shouldError: false, + description: "Missing steps field should not cause errors", + }, + { + name: "run with multiline string using pipe", + yaml: `jobs: + test: + steps: + - name: Multiline + run: | + echo "Line 1" + echo "${{ github.event.issue.title }}" + echo "Line 3"`, + shouldError: true, + description: "Multiline run with unsafe expression should be detected", + }, + { + name: "run with multiline string using greater than", + yaml: `jobs: + test: + steps: + - name: Folded + run: > + echo "Line 1" + echo "${{ github.event.issue.title }}" + echo "Line 3"`, + shouldError: true, + description: "Folded multiline run with unsafe expression should be detected", + }, + { + name: "run with literal block chomping indicators", + yaml: `jobs: + test: + steps: + - name: Strip chomping + run: |- + echo "${{ github.event.issue.title }}"`, + shouldError: true, + description: "Run with strip chomping indicator should detect unsafe patterns", + }, + { + name: "run with keep chomping indicator", + yaml: `jobs: + test: + steps: + - name: Keep chomping + run: |+ + echo "${{ github.event.issue.title }}"`, + shouldError: true, + description: "Run with keep chomping indicator should detect unsafe patterns", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateNoTemplateInjection(tt.yaml) + + if tt.shouldError { + require.Error(t, err, tt.description) + assert.Contains(t, err.Error(), "template injection", + "Error should mention template injection") + } else { + assert.NoError(t, err, tt.description) + } + }) + } +}