From 614264b650852d9487fcae074a32ce25f687f6a0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Apr 2026 13:36:26 +0000 Subject: [PATCH 1/6] Initial plan From 2521e22e7a3ed3139d2a168e93cf08442517aa98 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Apr 2026 14:13:56 +0000 Subject: [PATCH 2/6] feat: add compiler validation for dangerous shell expansion patterns in safe-outputs.steps (#29085) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1035e50f-37cf-4419-9661-839ea3584a2e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_validators.go | 6 + ...utputs_steps_shell_expansion_validation.go | 194 +++++++++++++++++ ...s_steps_shell_expansion_validation_test.go | 205 ++++++++++++++++++ 3 files changed, 405 insertions(+) create mode 100644 pkg/workflow/safe_outputs_steps_shell_expansion_validation.go create mode 100644 pkg/workflow/safe_outputs_steps_shell_expansion_validation_test.go diff --git a/pkg/workflow/compiler_validators.go b/pkg/workflow/compiler_validators.go index cbc90359768..2289a08d499 100644 --- a/pkg/workflow/compiler_validators.go +++ b/pkg/workflow/compiler_validators.go @@ -195,6 +195,12 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow return formatCompilerError(markdownPath, "error", err.Error(), err) } + // Validate safe-outputs steps for dangerous shell expansion patterns + log.Printf("Validating safe-outputs steps for shell expansion patterns") + if err := validateSafeOutputsStepsShellExpansion(workflowData.SafeOutputs); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + // Validate safe-outputs allowed-domains configuration log.Printf("Validating safe-outputs allowed-domains") if err := c.validateSafeOutputsAllowedDomains(workflowData.SafeOutputs); err != nil { diff --git a/pkg/workflow/safe_outputs_steps_shell_expansion_validation.go b/pkg/workflow/safe_outputs_steps_shell_expansion_validation.go new file mode 100644 index 00000000000..a970bf5eff4 --- /dev/null +++ b/pkg/workflow/safe_outputs_steps_shell_expansion_validation.go @@ -0,0 +1,194 @@ +// 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. +// - "commandSubst" matches starting with "$({" or "$({{" that form "${{" are ignored +// because "${{" is not a valid shell construct (GitHub Actions uses "${{ }}"). +// +// 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\$\{[A-Za-z_][A-Za-z0-9_]*@[^}]*\})` + + `|` + + // Indirect expansion: ${!var} — '!' immediately after '{' + `(?P\$\{![A-Za-z_])` + + `|` + + // Command substitution: any $( sequence + `(?P\$\()` + + `|` + + // Backtick command substitution: `...` + "(?P`[^`\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 shell variable expansion 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 "+ + "without using shell expansion (e.g. with 'cat' or a script argument)", + stepIndex, + patternDescription, + snippet, + ) + } + + return nil +} diff --git a/pkg/workflow/safe_outputs_steps_shell_expansion_validation_test.go b/pkg/workflow/safe_outputs_steps_shell_expansion_validation_test.go new file mode 100644 index 00000000000..097aa56cc50 --- /dev/null +++ b/pkg/workflow/safe_outputs_steps_shell_expansion_validation_test.go @@ -0,0 +1,205 @@ +//go:build !integration + +package workflow + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateSafeOutputsStepsShellExpansion(t *testing.T) { + t.Run("nil config passes", func(t *testing.T) { + err := validateSafeOutputsStepsShellExpansion(nil) + assert.NoError(t, err, "nil config should pass validation") + }) + + t.Run("empty steps passes", func(t *testing.T) { + config := &SafeOutputsConfig{} + err := validateSafeOutputsStepsShellExpansion(config) + assert.NoError(t, err, "empty steps should pass validation") + }) + + t.Run("step without run field passes", func(t *testing.T) { + config := &SafeOutputsConfig{ + Steps: []any{ + map[string]any{ + "name": "Setup step", + "uses": "actions/checkout@v4", + }, + }, + } + err := validateSafeOutputsStepsShellExpansion(config) + assert.NoError(t, err, "step without run field should pass") + }) + + t.Run("simple run script without expansion passes", func(t *testing.T) { + config := &SafeOutputsConfig{ + Steps: []any{ + map[string]any{ + "name": "Print info", + "run": "echo 'hello world'\ncat /tmp/data.json", + }, + }, + } + err := validateSafeOutputsStepsShellExpansion(config) + assert.NoError(t, err, "simple run script should pass") + }) + + t.Run("simple variable reference passes", func(t *testing.T) { + config := &SafeOutputsConfig{ + Steps: []any{ + map[string]any{ + "name": "Use variable", + "run": "echo $MY_ENV_VAR", + }, + }, + } + err := validateSafeOutputsStepsShellExpansion(config) + assert.NoError(t, err, "simple $VAR should pass") + }) + + t.Run("braced variable reference passes", func(t *testing.T) { + config := &SafeOutputsConfig{ + Steps: []any{ + map[string]any{ + "name": "Use braced variable", + "run": "echo ${MY_ENV_VAR}", + }, + }, + } + err := validateSafeOutputsStepsShellExpansion(config) + assert.NoError(t, err, "${VAR} without operators should pass") + }) + + t.Run("GitHub Actions expression passes", func(t *testing.T) { + config := &SafeOutputsConfig{ + Steps: []any{ + map[string]any{ + "name": "Use expression", + "run": "echo ${{ github.repository }}", + "env": map[string]any{ + "REPO": "${{ github.repository }}", + }, + }, + }, + } + err := validateSafeOutputsStepsShellExpansion(config) + assert.NoError(t, err, "GitHub Actions ${{ }} expression should pass") + }) + + t.Run("non-map step entry is skipped", func(t *testing.T) { + config := &SafeOutputsConfig{ + Steps: []any{"not-a-map"}, + } + err := validateSafeOutputsStepsShellExpansion(config) + assert.NoError(t, err, "non-map step should be skipped without error") + }) +} + +func TestValidateSafeOutputsStepsShellExpansion_DangerousPatterns(t *testing.T) { + tests := []struct { + name string + runScript string + wantErrContain string + }{ + { + name: "command substitution $(...)", + runScript: `URL=$(cat /tmp/url.txt)`, + wantErrContain: "command substitution", + }, + { + name: "command substitution with space", + runScript: "RESULT=$(gh api /repos)\necho $RESULT", + wantErrContain: "command substitution", + }, + { + name: "nested command substitution in assignment", + runScript: `SENT_DIST_URL="$(gh aw upload-asset chart.png)"`, + wantErrContain: "command substitution", + }, + { + name: "backtick command substitution", + runScript: "RESULT=`cat /tmp/file.txt`", + wantErrContain: "backtick command substitution", + }, + { + name: "parameter transformation @P", + runScript: `echo ${MY_VAR@P}`, + wantErrContain: "parameter transformation", + }, + { + name: "parameter transformation @U", + runScript: `UPPER=${NAME@U}`, + wantErrContain: "parameter transformation", + }, + { + name: "indirect expansion ${!var}", + runScript: `echo ${!VARNAME}`, + wantErrContain: "indirect expansion", + }, + { + name: "indirect expansion in assignment", + runScript: `VALUE="${!KEY}"`, + wantErrContain: "indirect expansion", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := &SafeOutputsConfig{ + Steps: []any{ + map[string]any{ + "name": "Step with dangerous expansion", + "run": tt.runScript, + }, + }, + } + err := validateSafeOutputsStepsShellExpansion(config) + require.Error(t, err, "dangerous pattern should be rejected: %s", tt.name) + assert.Contains(t, err.Error(), tt.wantErrContain, + "error message should describe the pattern type") + assert.Contains(t, err.Error(), "safe-outputs.steps[0]", + "error message should include the step index") + }) + } +} + +func TestValidateRunScriptForShellExpansion(t *testing.T) { + t.Run("empty script passes", func(t *testing.T) { + err := validateRunScriptForShellExpansion(0, "") + assert.NoError(t, err, "empty script should pass") + }) + + t.Run("error includes step index", func(t *testing.T) { + err := validateRunScriptForShellExpansion(3, "$(echo bad)") + require.Error(t, err, "command substitution should be rejected") + assert.Contains(t, err.Error(), "safe-outputs.steps[3]", + "error should include the step index") + }) + + t.Run("error includes offending snippet", func(t *testing.T) { + err := validateRunScriptForShellExpansion(0, `URL=$(cat /tmp/url.txt)`) + require.Error(t, err, "should reject command substitution") + // The snippet includes at least the $( opener + assert.Contains(t, err.Error(), "$(", "error should include the offending snippet") + }) + + t.Run("error includes remediation guidance", func(t *testing.T) { + err := validateRunScriptForShellExpansion(0, "$(echo hi)") + require.Error(t, err, "should reject command substitution") + assert.Contains(t, err.Error(), "/tmp/gh-aw/agent/", + "error should include remediation guidance about writing to a file") + }) + + t.Run("long snippet is truncated", func(t *testing.T) { + longScript := "$(echo " + strings.Repeat("a", 100) + ")" + err := validateRunScriptForShellExpansion(0, longScript) + require.Error(t, err, "should reject command substitution") + // Snippet in error should not exceed 60+3="..." = 63 characters + assert.NotContains(t, err.Error(), strings.Repeat("a", 61), + "snippet in error message should be truncated") + }) +} From d1fea34c6ef3113d89bd8ccd7841f48d42b009cd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Apr 2026 14:25:08 +0000 Subject: [PATCH 3/6] fix: update copilot-pr-nlp-analysis prompt to avoid shell expansion when handling chart URLs Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7482b2f0-e47d-453a-9615-094212c7d72f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/copilot-pr-nlp-analysis.md | 42 ++++++++++++++++---- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/.github/workflows/copilot-pr-nlp-analysis.md b/.github/workflows/copilot-pr-nlp-analysis.md index 077605fb1cd..ea1de817c6e 100644 --- a/.github/workflows/copilot-pr-nlp-analysis.md +++ b/.github/workflows/copilot-pr-nlp-analysis.md @@ -206,16 +206,42 @@ For each generated chart: find /tmp/gh-aw/python/charts/ -maxdepth 1 -ls ``` -2. **Upload each chart** using the `upload asset` tool -3. **Collect returned URLs** for embedding in the discussion +2. **Upload each chart** using the `upload asset` MCP tool (call it directly — do NOT wrap in a shell command or use `$()` to capture the URL) + +3. **Record the returned URL** from each upload by writing it to a plain text file in `/tmp/gh-aw/agent/` immediately after the MCP tool returns: + - `sentiment_distribution.png` → write URL to `/tmp/gh-aw/agent/url-sentiment-distribution.txt` + - `sentiment_timeline.png` → write URL to `/tmp/gh-aw/agent/url-sentiment-timeline.txt` + - `topic_frequencies.png` → write URL to `/tmp/gh-aw/agent/url-topic-frequencies.txt` + - `topics_wordcloud.png` → write URL to `/tmp/gh-aw/agent/url-topics-wordcloud.txt` + - `keyword_trends.png` → write URL to `/tmp/gh-aw/agent/url-keyword-trends.txt` + + For example, after the `upload asset` tool returns `https://github.com/.../chart.png`, write it with: + ```bash + echo -n "https://github.com/.../chart.png" > /tmp/gh-aw/agent/url-sentiment-distribution.txt + ``` + + **Do NOT** store URLs in shell variables or use command substitution (`$(...)`) — this triggers the security harness. ### Phase 6: Create Analysis Discussion +Build the discussion body by reading the URL files saved in Phase 5, then post a comprehensive discussion. + +**Before constructing the body**, read the uploaded chart URLs: +```bash +SENTIMENT_DIST_URL=$(cat /tmp/gh-aw/agent/url-sentiment-distribution.txt 2>/dev/null || echo "") +SENTIMENT_TIME_URL=$(cat /tmp/gh-aw/agent/url-sentiment-timeline.txt 2>/dev/null || echo "") +TOPIC_FREQ_URL=$(cat /tmp/gh-aw/agent/url-topic-frequencies.txt 2>/dev/null || echo "") +TOPICS_CLOUD_URL=$(cat /tmp/gh-aw/agent/url-topics-wordcloud.txt 2>/dev/null || echo "") +KEYWORD_TRENDS_URL=$(cat /tmp/gh-aw/agent/url-keyword-trends.txt 2>/dev/null || echo "") +``` + +Use a Python script to write the fully-substituted discussion body to `/tmp/gh-aw/agent/discussion_body.md`, inserting the literal URL strings directly (no shell variable expansion in the final body). Then pass the body to the `create_discussion` safe-output tool. + Post a comprehensive discussion with the following structure: **Title**: `Copilot PR Conversation NLP Analysis - [DATE]` -**Content Template**: +**Content Template** (substitute `[SENTIMENT_DIST_URL]`, `[SENTIMENT_TIME_URL]`, `[TOPIC_FREQ_URL]`, `[TOPICS_CLOUD_URL]`, and `[KEYWORD_TRENDS_URL]` with the literal URL strings read from the files above): ````markdown # 🤖 Copilot PR Conversation NLP Analysis - [DATE] @@ -230,7 +256,7 @@ Post a comprehensive discussion with the following structure: ## Sentiment Analysis ### Overall Sentiment Distribution -![Sentiment Distribution](URL_FROM_UPLOAD_ASSET_sentiment_distribution) +![Sentiment Distribution]([SENTIMENT_DIST_URL]) **Key Findings**: - **Positive messages**: [count] ([percentage]%) @@ -239,7 +265,7 @@ Post a comprehensive discussion with the following structure: - **Average polarity**: [score] on scale of -1 (very negative) to +1 (very positive) ### Sentiment Over Conversation Timeline -![Sentiment Timeline](URL_FROM_UPLOAD_ASSET_sentiment_timeline) +![Sentiment Timeline]([SENTIMENT_TIME_URL]) **Observations**: - [e.g., "Conversations typically start neutral and become more positive as issues are resolved"] @@ -248,7 +274,7 @@ Post a comprehensive discussion with the following structure: ## Topic Analysis ### Identified Discussion Topics -![Topic Frequencies](URL_FROM_UPLOAD_ASSET_topic_frequencies) +![Topic Frequencies]([TOPIC_FREQ_URL]) **Major Topics Detected**: 1. **[Topic 1 Name]** ([count] messages, [percentage]%): [brief description] @@ -257,12 +283,12 @@ Post a comprehensive discussion with the following structure: 4. **[Topic 4 Name]** ([count] messages, [percentage]%): [brief description] ### Topic Word Cloud -![Topics Word Cloud](URL_FROM_UPLOAD_ASSET_topics_wordcloud) +![Topics Word Cloud]([TOPICS_CLOUD_URL]) ## Keyword Trends ### Most Common Keywords and Phrases -![Keyword Trends](URL_FROM_UPLOAD_ASSET_keyword_trends) +![Keyword Trends]([KEYWORD_TRENDS_URL]) **Top Recurring Terms**: - **Technical**: [list top 5 technical terms] From aaefd9fa67f40597ad6347de5d3c1e148e0769df Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 29 Apr 2026 14:37:00 +0000 Subject: [PATCH 4/6] docs(adr): add draft ADR-29123 for compile-time validation of safe-outputs shell expansion Generated by the Design Decision Gate workflow. The PR author should review, complete, and finalize before changing status from Draft to Accepted. Co-Authored-By: Claude Sonnet 4.6 --- ...dation-for-safe-outputs-shell-expansion.md | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 docs/adr/29123-compile-time-validation-for-safe-outputs-shell-expansion.md diff --git a/docs/adr/29123-compile-time-validation-for-safe-outputs-shell-expansion.md b/docs/adr/29123-compile-time-validation-for-safe-outputs-shell-expansion.md new file mode 100644 index 00000000000..8ec136bf5ab --- /dev/null +++ b/docs/adr/29123-compile-time-validation-for-safe-outputs-shell-expansion.md @@ -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.* From 83ddde4e7de8d4f72eb8376f1926bd548d29d0b9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Apr 2026 14:37:37 +0000 Subject: [PATCH 5/6] fix: downgrade safe-outputs shell expansion check to warning in non-strict mode Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4416e886-fc60-4fe4-be34-8583c9b1ca0d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_validators.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/workflow/compiler_validators.go b/pkg/workflow/compiler_validators.go index 2289a08d499..7df66793161 100644 --- a/pkg/workflow/compiler_validators.go +++ b/pkg/workflow/compiler_validators.go @@ -195,10 +195,16 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow return formatCompilerError(markdownPath, "error", err.Error(), err) } - // Validate safe-outputs steps for dangerous shell expansion patterns + // Validate safe-outputs steps for dangerous shell expansion patterns. + // In strict mode this is a hard error; in non-strict mode it is a warning + // so that existing workflows continue to compile while authors migrate them. log.Printf("Validating safe-outputs steps for shell expansion patterns") if err := validateSafeOutputsStepsShellExpansion(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) + if c.strictMode { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", err.Error())) + c.IncrementWarningCount() } // Validate safe-outputs allowed-domains configuration From 16b5df47edcbf1183a1f791d584a980080f1a1c2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Apr 2026 15:29:43 +0000 Subject: [PATCH 6/6] fix: address PR review comments on shell expansion validator and workflow prompt Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a049b317-5c77-409c-8266-695748d751b9 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/copilot-pr-nlp-analysis.md | 30 ++++++++++++------- ...utputs_steps_shell_expansion_validation.go | 13 ++++---- ...s_steps_shell_expansion_validation_test.go | 5 ++++ 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/.github/workflows/copilot-pr-nlp-analysis.md b/.github/workflows/copilot-pr-nlp-analysis.md index ea1de817c6e..af7a9746aee 100644 --- a/.github/workflows/copilot-pr-nlp-analysis.md +++ b/.github/workflows/copilot-pr-nlp-analysis.md @@ -224,24 +224,34 @@ For each generated chart: ### Phase 6: Create Analysis Discussion -Build the discussion body by reading the URL files saved in Phase 5, then post a comprehensive discussion. +Build the discussion body by reading the URL files saved in Phase 5 using Python, then post a comprehensive discussion. -**Before constructing the body**, read the uploaded chart URLs: -```bash -SENTIMENT_DIST_URL=$(cat /tmp/gh-aw/agent/url-sentiment-distribution.txt 2>/dev/null || echo "") -SENTIMENT_TIME_URL=$(cat /tmp/gh-aw/agent/url-sentiment-timeline.txt 2>/dev/null || echo "") -TOPIC_FREQ_URL=$(cat /tmp/gh-aw/agent/url-topic-frequencies.txt 2>/dev/null || echo "") -TOPICS_CLOUD_URL=$(cat /tmp/gh-aw/agent/url-topics-wordcloud.txt 2>/dev/null || echo "") -KEYWORD_TRENDS_URL=$(cat /tmp/gh-aw/agent/url-keyword-trends.txt 2>/dev/null || echo "") +**Before constructing the body**, use a Python script to read the uploaded chart URLs directly from the files (do not use shell variables or command substitution — read the files entirely within Python and treat missing files as empty strings): + +```python +import os + +def read_url(path): + try: + with open(path) as f: + return f.read().strip() + except FileNotFoundError: + return "" + +sentiment_dist_url = read_url("/tmp/gh-aw/agent/url-sentiment-distribution.txt") +sentiment_time_url = read_url("/tmp/gh-aw/agent/url-sentiment-timeline.txt") +topic_freq_url = read_url("/tmp/gh-aw/agent/url-topic-frequencies.txt") +topics_cloud_url = read_url("/tmp/gh-aw/agent/url-topics-wordcloud.txt") +keyword_trends_url = read_url("/tmp/gh-aw/agent/url-keyword-trends.txt") ``` -Use a Python script to write the fully-substituted discussion body to `/tmp/gh-aw/agent/discussion_body.md`, inserting the literal URL strings directly (no shell variable expansion in the final body). Then pass the body to the `create_discussion` safe-output tool. +Use this same Python script to write the fully-substituted discussion body to `/tmp/gh-aw/agent/discussion_body.md`, inserting the literal URL strings directly. Then pass the body to the `create_discussion` safe-output tool. Post a comprehensive discussion with the following structure: **Title**: `Copilot PR Conversation NLP Analysis - [DATE]` -**Content Template** (substitute `[SENTIMENT_DIST_URL]`, `[SENTIMENT_TIME_URL]`, `[TOPIC_FREQ_URL]`, `[TOPICS_CLOUD_URL]`, and `[KEYWORD_TRENDS_URL]` with the literal URL strings read from the files above): +**Content Template** (substitute `[SENTIMENT_DIST_URL]`, `[SENTIMENT_TIME_URL]`, `[TOPIC_FREQ_URL]`, `[TOPICS_CLOUD_URL]`, and `[KEYWORD_TRENDS_URL]` with the literal URL strings read by Python from the files above): ````markdown # 🤖 Copilot PR Conversation NLP Analysis - [DATE] diff --git a/pkg/workflow/safe_outputs_steps_shell_expansion_validation.go b/pkg/workflow/safe_outputs_steps_shell_expansion_validation.go index a970bf5eff4..00e571a1d23 100644 --- a/pkg/workflow/safe_outputs_steps_shell_expansion_validation.go +++ b/pkg/workflow/safe_outputs_steps_shell_expansion_validation.go @@ -47,8 +47,8 @@ var safeOutputsStepsShellExpansionLog = newValidationLogger("safe_outputs_steps_ // // After matching, callers must exclude false-positives: // - "commandSubst" matches starting with "$((" (arithmetic expansion) are ignored. -// - "commandSubst" matches starting with "$({" or "$({{" that form "${{" are ignored -// because "${{" is not a valid shell construct (GitHub Actions uses "${{ }}"). +// - 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. @@ -180,10 +180,11 @@ func validateRunScriptForShellExpansion(stepIndex int, script string) error { "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 shell variable expansion 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 "+ - "without using shell expansion (e.g. with 'cat' or a script argument)", + "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, diff --git a/pkg/workflow/safe_outputs_steps_shell_expansion_validation_test.go b/pkg/workflow/safe_outputs_steps_shell_expansion_validation_test.go index 097aa56cc50..c90696ae6bd 100644 --- a/pkg/workflow/safe_outputs_steps_shell_expansion_validation_test.go +++ b/pkg/workflow/safe_outputs_steps_shell_expansion_validation_test.go @@ -173,6 +173,11 @@ func TestValidateRunScriptForShellExpansion(t *testing.T) { assert.NoError(t, err, "empty script should pass") }) + t.Run("arithmetic expansion $(( )) passes", func(t *testing.T) { + err := validateRunScriptForShellExpansion(0, "echo $((1+1))") + assert.NoError(t, err, "arithmetic expansion $(( )) should be allowed") + }) + t.Run("error includes step index", func(t *testing.T) { err := validateRunScriptForShellExpansion(3, "$(echo bad)") require.Error(t, err, "command substitution should be rejected")