Fix template injection validator false positives when env: follows run:#11586
Fix template injection validator false positives when env: follows run:#11586
Conversation
- 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>
All template injection validator tests pass successfully: ✅ Template injection tests (45/45 passing):
✅ Manual verification: ./gh-aw compile workflow.md
⚠ Compiled 1 workflow(s): 0 error(s), 1 warning(s)The fix correctly handles expressions in Note: There are some pre-existing test failures in |
| // It detects cases where GitHub Actions expressions are used directly in shell commands | ||
| // instead of being passed through environment variables | ||
| func validateNoTemplateInjection(yamlContent string) error { | ||
| templateInjectionValidationLog.Print("Validating compiled YAML for template injection risks") |
There was a problem hiding this comment.
@copilot try parsing the yaml string, and walk the tree to extra run fields
There was a problem hiding this comment.
Refactored to use YAML parsing instead of regex (commit 9450b7f).
Changes:
- Replaced regex-based extraction with
extractRunBlocks()that recursively walks the parsed YAML tree - Uses
github.com/goccy/go-yamlto parse workflow and extract allrun:field values - More robust - handles any YAML key ordering correctly
Benefits:
- No fragile regex patterns trying to match YAML structure
- Works with
env:before or afterrun: - More maintainable and easier to understand
All 45 template injection tests pass ✅
- 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>
|
@copilot add more tests, including invalid yaml |
- 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>
Added comprehensive test coverage (commit dab38bc): TestTemplateInjectionInvalidYAML - 10 tests
TestTemplateInjectionYAMLParsingEdgeCases - 14 tests
Total: 69 template injection tests (all passing) ✅ Invalid YAML is handled gracefully - parse errors cause validation to skip without crashing. |
The template injection validator incorrectly flagged expressions in
env:blocks when the YAML serializer outputsenv:afterrun:(common in custom job steps). The original implementation used regex patterns to extract run blocks, which was fragile and couldn't properly distinguish between run content and subsequent YAML keys.Changes
Refactored to use proper YAML parsing instead of regex
runBlockRegex) that tried to match YAML structureextractRunBlocks()function that recursively walks the parsed YAML treegithub.com/goccy/go-yamlto parse workflow and extract allrun:field valuesAdded comprehensive test coverage
Example
Before: False positive when env follows run
After: Correctly recognized as safe
# Same YAML now compiles without errorsWhy YAML Parsing Is Better
Previous approach (regex):
Current approach (YAML parsing):
run:field values from parsed treeTest Coverage
The validator is now thoroughly tested with:
The validator handles env blocks correctly regardless of YAML serialization order while maintaining detection of actual template injection vulnerabilities.
Original prompt
This section details on the original issue you should resolve
<issue_title>Template injection validator incorrectly flags env: blocks due to YAML serialization order</issue_title>
<issue_description>## Bug Description
The template injection validator incorrectly flags expressions in
env:blocks as unsafe, even though using environment variables is the documented safe pattern. This happens because:env:to appear beforerun:in the YAMLrun:beforeenv:for custom job stepsenv:blocks as part ofrun:blocks, flagging safe code as unsafeReproduction Steps
Run
gh aw compileObserve the error:
Expected Behavior
The workflow should compile successfully because the expression is used in an
env:block, which is the documented safe pattern.Actual Behavior
The compiler flags the expression as unsafe, even though it's in an
env:block.Root Cause Analysis
1. Validator Regex Issue
In
pkg/workflow/template_injection_validation.go:69, the regex for multi-line run blocks:This regex captures content until it finds:
-(next step)But
env:lines at the same indentation don't match either pattern, so they get included in the capture.2. YAML Serialization Order Issue
The compiled YAML has different key ordering:
For AI agent steps (correct order - validation passes):
For custom job steps (incorrect order - validation fails):
3. Test Case Confirms the Issue
In
pkg/workflow/template_injection_validation_test.go:382-394, the safe test case hasenv:BEFORErun::And the test passes. But when
run:comes beforeenv:(as in the custom jobs serialization), the validation fails.Suggested Fixes
Either:
Fix the YAML serializer to output
env:beforerun:for custom job steps (consistent with AI agent steps)Fix the regex to properly detect YAML step boundaries by stopping at any YAML key at the same indentation level (
env:,if:,name:,with:, etc.), not just-markersUse proper YAML parsing instead of regex to extract run block content
Environment
Comments on the Issue (you are @copilot in this section)
@Mossaka ## Minimal Reproduction FoundThe bug triggers when:
run: |(literal block with keep indicator)env:appears afterrun:in the compiled YAMLIt does NOT trigger when the serializer outputs
run: |-(literal block with strip indicator).Reproduction Files
Triggers bug (trailing blank line before
---causesrun: |):Compiled output (note
run: |andenv:afterrun:):Does NOT trigger bug (no trailing blank line causes
run: |-):