-
Notifications
You must be signed in to change notification settings - Fork 431
fix: compiler validates dangerous shell expansion in safe-outputs.steps; fix copilot-pr-nlp-analysis prompt #29123
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.
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
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
614264b
Initial plan
Copilot 2521e22
feat: add compiler validation for dangerous shell expansion patterns …
Copilot d1fea34
fix: update copilot-pr-nlp-analysis prompt to avoid shell expansion w…
Copilot bc5de8e
Merge branch 'main' into copilot/aw-failures-fix-copilot-nlp-analysis
pelikhan aaefd9f
docs(adr): add draft ADR-29123 for compile-time validation of safe-ou…
github-actions[bot] 83ddde4
fix: downgrade safe-outputs shell expansion check to warning in non-s…
Copilot 16b5df4
fix: address PR review comments on shell expansion validator and work…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
70 changes: 70 additions & 0 deletions
70
docs/adr/29123-compile-time-validation-for-safe-outputs-shell-expansion.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # ADR-29123: Compile-Time Validation for Dangerous Shell Expansion in safe-outputs Steps | ||
|
|
||
| **Date**: 2026-04-29 | ||
| **Status**: Draft | ||
| **Deciders**: pelikhan | ||
|
|
||
| --- | ||
|
|
||
| ## Part 1 — Narrative (Human-Friendly) | ||
|
|
||
| ### Context | ||
|
|
||
| The safe-outputs security harness blocks a set of dangerous bash expansion patterns at runtime — specifically `${var@operator}` (parameter transformation), `${!var}` (indirect expansion), `$(...)` (command substitution), and backtick substitution — to prevent shell injection attacks in GitHub Actions steps that post results to GitHub APIs. Prior to this change, workflow authors had no compile-time feedback when their `run:` scripts contained these patterns; the first signal was a confusing runtime failure when the harness silently rejected the step. The Copilot PR NLP Analysis workflow exposed this gap when it used `$(upload_asset ...)` command substitution to capture chart upload URLs inside a safe-outputs step. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will add a compile-time validator (`validateSafeOutputsStepsShellExpansion`) that scans every `run:` script in `safe-outputs.steps[]` for the four blocked shell expansion patterns and fails compilation with a descriptive error before the workflow reaches runtime. This closes the feedback loop between the runtime harness rules and the authoring experience, making violations detectable and actionable during `gh aw compile` instead of at execution time. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Improved Runtime Error Messages Only | ||
|
|
||
| The runtime harness already blocked these patterns; the simplest fix was to enrich the runtime error message with remediation guidance rather than adding a compile step. This was not chosen because it leaves the feedback loop long: an author must deploy and run the workflow before seeing the error, and the agent that runs the workflow may have already produced non-idempotent side effects (uploaded assets, posted partial comments) before the step fails. | ||
|
|
||
| #### Alternative 2: Allowlist Specific Patterns in the safe-outputs Harness | ||
|
|
||
| An alternative was to relax the harness to permit specific safe uses of `$(...)` (e.g., `$(cat /tmp/file.txt)`) rather than blocking all command substitution. This was not chosen because it significantly increases the attack surface of the harness — distinguishing safe from unsafe command substitutions reliably requires a full shell parser — and the pattern of writing intermediate values to files and reading them with `cat` in the safe-outputs step is a viable and already-supported escape hatch. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Workflow authors receive an actionable compiler error with the offending snippet and remediation guidance, instead of a cryptic runtime failure. | ||
| - The compiler and the runtime harness stay in sync: any new pattern blocked by the harness can be added to the validator in the same change. | ||
|
|
||
| #### Negative | ||
| - The validator must be maintained in tandem with the runtime harness. If the harness gains or removes a blocked pattern without updating the validator, they drift out of sync. | ||
| - The regex-based approach requires post-match filtering for false positives (arithmetic `$((`, GitHub Actions `${{ }}` expressions) because Go's RE2 engine does not support lookaheads. Edge cases in the filtering logic can yield false positives or false negatives. | ||
|
|
||
| #### Neutral | ||
| - The validator is registered in `compiler_validators.go` after `validateSafeOutputsMax`, making it part of the standard compilation pipeline with no change to the CLI interface. | ||
| - The companion fix to the Copilot PR NLP Analysis prompt (writing URLs to files instead of using `$(...)`) demonstrates the recommended pattern that workflow authors should follow. | ||
|
|
||
| --- | ||
|
|
||
| ## Part 2 — Normative Specification (RFC 2119) | ||
|
|
||
| > The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). | ||
|
|
||
| ### Shell Expansion Validation | ||
|
|
||
| 1. The compiler **MUST** validate every `run:` script in `safe-outputs.steps[]` for dangerous shell expansion patterns before emitting a compiled workflow artifact. | ||
| 2. The compiler **MUST** reject any `run:` script that contains `${var@operator}` (bash parameter transformation), `${!var}` (indirect expansion), `$(...)` (command substitution), or `` `...` `` (backtick command substitution). | ||
| 3. The compiler **MUST NOT** reject `run:` scripts that contain only `$VAR`, `${VAR}` (without operators), or `${{ expression }}` GitHub Actions template expressions. | ||
| 4. The compiler **MUST NOT** reject `run:` scripts that contain `$((` arithmetic expansion. | ||
| 5. Compiler error messages **MUST** include the zero-based step index, the offending snippet (truncated to 60 characters), and remediation guidance directing the author to write dynamic values to files in `/tmp/gh-aw/agent/` and read them with `cat` in the safe-outputs step. | ||
|
|
||
| ### Safe-Outputs Step Authoring | ||
|
|
||
| 1. Workflow authors **MUST NOT** use command substitution (`$(...)` or backticks) inside `safe-outputs.steps[].run` scripts to capture dynamic values. | ||
| 2. Workflow authors **MUST NOT** use indirect expansion (`${!var}`) or parameter transformation (`${var@operator}`) in `safe-outputs.steps[].run` scripts. | ||
| 3. When a safe-outputs step needs a value computed during the agent turn (e.g., an uploaded asset URL), the agent **MUST** write that value to a file in `/tmp/gh-aw/agent/` during its regular bash turn, and the safe-outputs step **MUST** read it using `cat` without shell expansion. | ||
| 4. Workflow authors **SHOULD** use Python scripts to assemble complex multi-value payloads (e.g., discussion bodies) before passing them to a safe-outputs step, inserting literal strings rather than shell variables. | ||
|
|
||
| ### Conformance | ||
|
|
||
| An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. | ||
|
|
||
| --- | ||
|
|
||
| *This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25114923934) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
195 changes: 195 additions & 0 deletions
195
pkg/workflow/safe_outputs_steps_shell_expansion_validation.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,195 @@ | ||
| // This file validates safe-outputs.steps run scripts for dangerous shell expansion | ||
| // patterns that would be blocked at runtime by the safe-outputs security harness. | ||
| // | ||
| // # Shell Expansion Security in Safe-Outputs Steps | ||
| // | ||
| // The safe-outputs security harness blocks shell scripts that contain dangerous | ||
| // bash expansion patterns. This validator detects those patterns at compile time | ||
| // so workflow authors receive a clear error during `gh aw compile` rather than | ||
| // a confusing runtime failure. | ||
| // | ||
| // # Blocked Patterns | ||
| // | ||
| // The following bash constructs are rejected: | ||
| // - ${var@operator}: bash 4.4+ parameter transformation (e.g. ${foo@P}, ${bar@U}) | ||
| // - ${!var}: bash indirect expansion | ||
| // - $(command): command substitution | ||
| // - `command`: backtick command substitution | ||
| // | ||
| // GitHub Actions template expressions (${{ ... }}) are explicitly allowed and are | ||
| // excluded from the checks. | ||
| // | ||
| // # When to Add Validation Here | ||
| // | ||
| // Add validation to this file when: | ||
| // - A new dangerous shell expansion variant must be detected in safe-outputs run scripts | ||
| // - The safe-outputs harness blocks a new class of shell pattern at runtime | ||
| // | ||
| // For general safe-outputs validation, see safe_outputs_validation.go. | ||
|
|
||
| package workflow | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "regexp" | ||
| "strings" | ||
| ) | ||
|
|
||
| var safeOutputsStepsShellExpansionLog = newValidationLogger("safe_outputs_steps_shell_expansion") | ||
|
|
||
| // shellExpansionPattern matches dangerous bash expansion constructs inside a run: script. | ||
| // | ||
| // Captured groups by name: | ||
| // - "paramTransform": ${var@operator} — bash 4.4+ parameter transformation | ||
| // - "indirectExpand": ${!var} — bash indirect expansion | ||
| // - "commandSubst": $(...) — command substitution (any $( sequence) | ||
| // - "backtick": `...` — backtick command substitution | ||
| // | ||
| // After matching, callers must exclude false-positives: | ||
| // - "commandSubst" matches starting with "$((" (arithmetic expansion) are ignored. | ||
| // - GitHub Actions expressions use "${{ ... }}", which starts with "{" not "(", | ||
| // so they do not match the "commandSubst" pattern (\$\() at all. | ||
| // | ||
| // Note: Go's regexp/RE2 does not support lookaheads, so post-match filtering is used | ||
| // instead of inline negative lookaheads. | ||
| var shellExpansionPattern = regexp.MustCompile( | ||
| // Parameter transformation: ${var@op} — the @ must follow word characters | ||
| `(?P<paramTransform>\$\{[A-Za-z_][A-Za-z0-9_]*@[^}]*\})` + | ||
| `|` + | ||
| // Indirect expansion: ${!var} — '!' immediately after '{' | ||
| `(?P<indirectExpand>\$\{![A-Za-z_])` + | ||
| `|` + | ||
| // Command substitution: any $( sequence | ||
| `(?P<commandSubst>\$\()` + | ||
| `|` + | ||
| // Backtick command substitution: `...` | ||
| "(?P<backtick>`[^`\n]+`)", | ||
| ) | ||
|
|
||
| // dangerousPatternDescription maps a named capture group to a human-readable | ||
| // description used in compiler error messages. | ||
| var dangerousPatternDescription = map[string]string{ | ||
| "paramTransform": "parameter transformation (e.g. ${var@P})", | ||
| "indirectExpand": "indirect expansion (e.g. ${!var})", | ||
| "commandSubst": "command substitution (e.g. $(command))", | ||
| "backtick": "backtick command substitution (e.g. `command`)", | ||
| } | ||
|
|
||
| // validateSafeOutputsStepsShellExpansion checks every run: script in the | ||
| // safe-outputs.steps list for dangerous bash expansion patterns that would be | ||
| // blocked by the safe-outputs security harness at runtime. | ||
| // | ||
| // Returning a non-nil error causes compilation to fail with a descriptive message | ||
| // that includes the step index, the offending snippet, and the pattern category. | ||
| func validateSafeOutputsStepsShellExpansion(config *SafeOutputsConfig) error { | ||
| if config == nil || len(config.Steps) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| safeOutputsStepsShellExpansionLog.Printf("Validating %d safe-outputs steps for dangerous shell expansion patterns", len(config.Steps)) | ||
|
|
||
| for i, step := range config.Steps { | ||
| stepMap, ok := step.(map[string]any) | ||
| if !ok { | ||
| continue | ||
| } | ||
| runVal, exists := stepMap["run"] | ||
| if !exists { | ||
| continue | ||
| } | ||
| runScript, ok := runVal.(string) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| if err := validateRunScriptForShellExpansion(i, runScript); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| safeOutputsStepsShellExpansionLog.Print("Safe-outputs steps shell expansion validation passed") | ||
| return nil | ||
| } | ||
|
|
||
| // validateRunScriptForShellExpansion checks a single run: script for dangerous | ||
| // bash expansion patterns. stepIndex is 0-based and is included in error messages. | ||
| func validateRunScriptForShellExpansion(stepIndex int, script string) error { | ||
| // Fast path: no '$' or backtick character means no expansion pattern can be present. | ||
| if !strings.ContainsAny(script, "$`") { | ||
| return nil | ||
| } | ||
|
|
||
| // Scan all matches so we can skip false positives (e.g. arithmetic $((, GitHub | ||
| // Actions ${{ expressions) before reporting the first true violation. | ||
| groupNames := shellExpansionPattern.SubexpNames() | ||
| allMatches := shellExpansionPattern.FindAllStringSubmatchIndex(script, -1) | ||
|
|
||
| for _, match := range allMatches { | ||
| snippet := "" | ||
| patternDescription := "dangerous shell expansion" | ||
| groupName := "" | ||
| matchStart := match[0] | ||
|
|
||
| for gi, name := range groupNames { | ||
| if name == "" { | ||
| continue | ||
| } | ||
| start, end := match[gi*2], match[gi*2+1] | ||
| if start < 0 { | ||
| continue // group did not participate in this match | ||
| } | ||
| if _, known := dangerousPatternDescription[name]; known { | ||
| raw := script[start:end] | ||
| groupName = name | ||
| // For short patterns like $( or ${! that don't capture the full construct, | ||
| // extend the snippet to include context up to the end of the line or 60 chars. | ||
| if len(raw) < 10 { | ||
| lineEnd := strings.IndexByte(script[start:], '\n') | ||
| if lineEnd < 0 { | ||
| lineEnd = len(script) - start | ||
| } | ||
| raw = script[start : start+lineEnd] | ||
| // Remove any trailing control characters. | ||
| raw = strings.TrimRight(raw, "\r\t ") | ||
| } | ||
| // Clip the snippet to 60 characters to keep the error readable. | ||
| if len(raw) > 60 { | ||
| raw = raw[:57] + "..." | ||
| } | ||
| snippet = raw | ||
| patternDescription = dangerousPatternDescription[name] | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if groupName == "" { | ||
| continue | ||
| } | ||
|
|
||
| // Skip arithmetic expansion $((: it uses $(( not $(command). | ||
| if groupName == "commandSubst" { | ||
| // Check the two characters after $( to detect $(( | ||
| if matchStart+3 <= len(script) && script[matchStart:matchStart+3] == "$((" { | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| safeOutputsStepsShellExpansionLog.Printf("Dangerous pattern found in safe-outputs step %d: %s", stepIndex, patternDescription) | ||
|
|
||
| return fmt.Errorf( | ||
| "safe-outputs.steps[%d]: run script contains %s, which is blocked by the "+ | ||
| "safe-outputs security harness at runtime\n\n"+ | ||
| " Offending snippet: %s\n\n"+ | ||
| "Avoid command substitution, backticks, indirect expansion, and parameter "+ | ||
| "transformation in safe-outputs run scripts. Write URL values and other "+ | ||
| "dynamic content to files in /tmp/gh-aw/agent/ during the agent turn, then "+ | ||
| "read the file contents in the safe-outputs step (e.g. with 'cat' or by "+ | ||
| "passing a script argument)", | ||
| stepIndex, | ||
| patternDescription, | ||
| snippet, | ||
| ) | ||
| } | ||
|
|
||
| return nil | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phase 5 explicitly says not to store URLs in shell variables or use command substitution (
$(...)), but Phase 6 immediately usesSENTIMENT_DIST_URL=$(cat ...)(both a shell variable and command substitution). If the intent is "don’t do this inside safe-outputs.steps run scripts" but it’s OK in the agent bash turn, please clarify the wording in Phase 5 (or adjust Phase 6 to avoid$()/shell vars by reading the files directly in Python) to remove the internal contradiction.See below for a potential fix: