diff --git a/docs/src/content/docs/reference/steps-jobs.md b/docs/src/content/docs/reference/steps-jobs.md index 87cc81d80ca..4a306680e4a 100644 --- a/docs/src/content/docs/reference/steps-jobs.md +++ b/docs/src/content/docs/reference/steps-jobs.md @@ -90,11 +90,13 @@ post-steps: uses: actions/upload-artifact@v4 with: name: workflow-results - path: /tmp/gh-aw/ + path: /tmp/gh-aw/agent/ retention-days: 7 ``` Useful for artifact uploads, summaries, cleanup, or triggering downstream workflows. +Post-steps must not create issues directly, read safe-output files, or expose secrets through a step-level `env` section. Avoid uploading the entire `/tmp/gh-aw/` directory, which can contain protected safe-output files. +Use [`safe-outputs.create-issue`](/gh-aw/reference/safe-outputs/) to publish agent-generated issues instead. ## Custom Jobs (`jobs:`) diff --git a/pkg/workflow/compiler_jobs_test.go b/pkg/workflow/compiler_jobs_test.go index 0b400ec29d1..375f1329ae4 100644 --- a/pkg/workflow/compiler_jobs_test.go +++ b/pkg/workflow/compiler_jobs_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/github/gh-aw/pkg/constants" + "github.com/goccy/go-yaml" ) // ======================================== @@ -465,6 +466,22 @@ Marker: ${{ needs.select.outputs.marker }} if !strings.Contains(lock, " activation:\n name: Activation\n needs: select") { t.Fatalf("activation should depend on runtime-import referenced job; lock excerpt:\n%s", lock) } + var compiledWorkflow map[string]any + if err := yaml.Unmarshal(lockContent, &compiledWorkflow); err != nil { + t.Fatalf("could not parse compiled workflow: %v", err) + } + jobs, ok := compiledWorkflow["jobs"].(map[string]any) + if !ok { + t.Fatal("compiled workflow is missing jobs") + } + activation, ok := jobs["activation"].(map[string]any) + if !ok { + t.Fatal("compiled workflow is missing activation job") + } + needs, ok := activation["needs"].(string) + if !ok || needs != "select" { + t.Errorf("activation should depend on runtime-import referenced job select; got needs: %#v", activation["needs"]) + } if !strings.Contains(lock, "GH_AW_NEEDS_SELECT_OUTPUTS_ISSUE_NUMBERS: ${{ needs.select.outputs.issue_numbers }}") { t.Errorf("lock file missing issue_numbers env mapping") } diff --git a/pkg/workflow/post_steps_validation.go b/pkg/workflow/post_steps_validation.go new file mode 100644 index 00000000000..665b410d4dc --- /dev/null +++ b/pkg/workflow/post_steps_validation.go @@ -0,0 +1,333 @@ +package workflow + +import ( + "fmt" + "regexp" + "slices" + "strings" +) + +var ( + postStepIssueCreateRE = regexp.MustCompile(`(?i)(?:^|[\s|;&])gh\s+(?:(?:-R|--repo)(?:=|\s+)[^\s]+\s+)*issue\s+create\b`) + postStepIssueAPIRE = regexp.MustCompile(`(?i)(?:^|[/\s"'=])(?:https://api\.github\.com/)?repos/(?:[^/\s"'?]+/[^/\s"'?]+|\$\{\{\s*github\.repository\s*\}\}|\$\{?GITHUB_REPOSITORY\}?)/issues(?:[?"'\s;|&]|$)`) + postStepAPIPostRE = regexp.MustCompile(`(?i)(?:^|\s)(?:-X\s*POST\b|--method(?:=|\s+)POST\b|-f(?:\s|=)|-F(?:\s|=)|--(?:raw-)?field(?:\s|=)|--input(?:\s|=))`) + postStepAPIGetRE = regexp.MustCompile(`(?i)(?:^|\s)(?:-X\s*GET\b|--method(?:=|\s+)GET\b)`) + postStepCurlRE = regexp.MustCompile(`(?i)(?:^|[\s|;&])curl(?:\s|$)`) + postStepOctokitRE = regexp.MustCompile(`\b(?:github|octokit)(?:\.rest)?\.issues\.create\s*\(`) + postStepRequestRE = regexp.MustCompile(`(?i)\b(?:github|octokit)\.request\s*\(\s*['"\x60]POST\s+/repos/[^/'"\x60]+/[^/'"\x60]+/issues(?:[?#\s'"\x60]|$)`) + postStepOutputRE = regexp.MustCompile(`(?i)(?:\bagent_output\.json\b|(?:safeoutputs|safe-outputs)/output\.json\b|\boutputs\.jsonl\b|\bsafeoutputs\.jsonl\b|\bGH_AW_AGENT_OUTPUT\b|\bGH_AW_SAFE_OUTPUTS\b|steps\.set-runtime-paths\.outputs\.GH_AW_SAFE_OUTPUTS)`) + postStepSecretRE = regexp.MustCompile(`\bsecrets\b`) + postStepArtifactRoot = regexp.MustCompile(`(?i)^(?:/tmp/gh-aw|\$\{?RUNNER_TEMP\}?/gh-aw|\$\{\{\s*runner\.temp\s*\}\}/gh-aw)(?:/\*\*)?/?$`) +) + +var postStepFileInputActions = map[string][]string{ + "actions/cache": {"path"}, + "actions/upload-artifact": {"path"}, + "actions/upload-pages-artifact": {"path"}, +} + +// validatePostStepsSafeOutputs checks only user-provided post-steps, not compiler-generated steps. +func validatePostStepsSafeOutputs(steps []any) error { + for i, raw := range steps { + step, ok := raw.(map[string]any) + if !ok { + continue + } + location := fmt.Sprintf("post-steps step %d", i+1) + if name, ok := step["name"].(string); ok && name != "" { + location += fmt.Sprintf(" (%q)", name) + } + + if err := validatePostStepInputs(step, location); err != nil { + return err + } + + scripts := []string{} + if run, ok := step["run"].(string); ok { + scripts = append(scripts, run) + } + if with, ok := step["with"].(map[string]any); ok { + if script, ok := with["script"].(string); ok { + scripts = append(scripts, script) + } + } + + for _, script := range scripts { + if postStepOctokitRE.MatchString(script) || postStepRequestRE.MatchString(script) { + return issueCreationError(location) + } + for _, command := range postStepLogicalCommands(script) { + if postStepIssueCreateRE.MatchString(command) { + return issueCreationError(location) + } + for _, match := range ghAPICmdRE.FindAllStringSubmatch(command, -1) { + args := regexCapture(match, 1) + if args != "" && postStepIssueAPIRE.MatchString(parsePostStepGHAPIEndpoint(args)) && + postStepAPIPostRE.MatchString(args) && !postStepAPIGetRE.MatchString(args) { + return issueCreationError(location) + } + } + if curlCreatesIssue(command) { + return issueCreationError(location) + } + } + if postStepOutputRE.MatchString(script) { + return fmt.Errorf("%s accesses the safe outputs output file directly; use safe-outputs instead. See: https://github.github.com/gh-aw/reference/safe-outputs/", location) + } + } + } + return nil +} + +func validatePostStepInputs(step map[string]any, location string) error { + if containsPostStepSecretEnv(step["env"]) { + return fmt.Errorf("%s exposes a secret through its env section; secrets are not allowed in post-steps", location) + } + uses, ok := step["uses"].(string) + if !ok { + return nil + } + action, _, _ := strings.Cut(uses, "@") + with, ok := step["with"].(map[string]any) + if !ok { + return nil + } + for _, input := range postStepFileInputActions[strings.ToLower(action)] { + if path, exists := with[input]; exists && fileInputContainsSafeOutput(path) { + return fmt.Errorf("%s reads a safe outputs file or its containing directory; use a narrower file path", location) + } + } + return nil +} + +func issueCreationError(location string) error { + return fmt.Errorf("%s attempts to create an issue directly; use safe-outputs.create-issue instead. See: https://github.github.com/gh-aw/reference/safe-outputs/", location) +} + +func containsPostStepSecret(value string) bool { + return slices.ContainsFunc(InlineExpressionPattern.FindAllString(value, -1), postStepSecretRE.MatchString) +} + +func containsPostStepSecretEnv(env any) bool { + switch values := env.(type) { + case map[string]any: + for _, value := range values { + if text, ok := value.(string); ok && containsPostStepSecret(text) { + return true + } + } + case map[string]string: + for _, value := range values { + if containsPostStepSecret(value) { + return true + } + } + } + return false +} + +func fileInputContainsSafeOutput(path any) bool { + switch value := path.(type) { + case string: + trimmed := strings.Trim(strings.TrimSpace(value), `"'`) + return postStepOutputRE.MatchString(value) || postStepArtifactRoot.MatchString(trimmed) + case []any: + return slices.ContainsFunc(value, fileInputContainsSafeOutput) + } + return false +} + +// postStepLogicalCommands joins shell lines continued with a trailing backslash. +func postStepLogicalCommands(script string) []string { + var commands []string + var current strings.Builder + lines := strings.Split(script, "\n") + for i, line := range lines { + line = strings.TrimRight(line, " \t\r") + continued := strings.HasSuffix(line, `\`) && i < len(lines)-1 + if continued { + line = strings.TrimSuffix(line, `\`) + } + if current.Len() > 0 { + current.WriteByte(' ') + } + current.WriteString(line) + if !continued { + commands = appendPostStepShellCommands(commands, current.String()) + current.Reset() + } + } + if current.Len() > 0 { + commands = appendPostStepShellCommands(commands, current.String()) + } + return commands +} + +func appendPostStepShellCommands(commands []string, line string) []string { + var command []rune + inSingle, inDouble, inBacktick := false, false, false + escaped := false + for _, char := range line { + if escaped { + command = append(command, char) + escaped = false + continue + } + if char == '\\' && !inSingle { + command = append(command, char) + escaped = true + continue + } + switch char { + case '\'': + if !inDouble && !inBacktick { + inSingle = !inSingle + } + case '"': + if !inSingle && !inBacktick { + inDouble = !inDouble + } + case '`': + if !inSingle && !inDouble { + inBacktick = !inBacktick + } + } + if !inSingle && !inDouble && !inBacktick { + if char == ';' || char == '|' || char == '\n' || char == '&' { + if len(command) > 0 { + commands = append(commands, string(command)) + command = nil + } + continue + } + } + command = append(command, char) + } + if len(command) > 0 { + commands = append(commands, string(command)) + } + return commands +} + +func curlCreatesIssue(command string) bool { + match := postStepCurlRE.FindStringIndex(command) + if match == nil { + return false + } + tokens := splitShellTokens(strings.TrimSpace(command[sliceIntAt(match, 1):])) + method, hasData, useGet, urls := parseCurlRequest(tokens) + if method == "" { + if hasData && !useGet { + method = "POST" + } else { + method = "GET" + } + } + return method == "POST" && slices.ContainsFunc(urls, postStepIssueAPIRE.MatchString) +} + +func parseCurlRequest(tokens []string) (string, bool, bool, []string) { + method := "" + hasData, useGet := false, false + var urls []string + for i := 0; i < len(tokens); i++ { + token, ok := shellToken(tokens, i) + if !ok { + break + } + token = strings.Trim(token, `"'`) + if token == "" { + continue + } + switch { + case token == "-X" || token == "--request": + if value, exists := shellToken(tokens, i+1); exists { + i++ + method = strings.ToUpper(strings.Trim(value, `"'`)) + } + case strings.HasPrefix(token, "--request="): + method = strings.ToUpper(strings.TrimPrefix(token, "--request=")) + case strings.HasPrefix(token, "-X") && len(token) > 2: + method = strings.ToUpper(strings.TrimPrefix(token, "-X")) + case token == "-G" || token == "--get": + useGet = true + case token == "-d" || token == "--data" || token == "--data-raw" || + token == "--data-binary" || token == "--data-urlencode" || + token == "-F" || token == "--form" || token == "--json": + hasData = true + if value, exists := shellToken(tokens, i+1); exists && !strings.HasPrefix(value, "-") { + i++ + } + case strings.HasPrefix(token, "--data=") || strings.HasPrefix(token, "--data-raw=") || + strings.HasPrefix(token, "--data-binary=") || strings.HasPrefix(token, "--data-urlencode=") || + strings.HasPrefix(token, "--form=") || strings.HasPrefix(token, "--json="): + hasData = true + case token == "--url": + if value, exists := shellToken(tokens, i+1); exists { + i++ + urls = append(urls, strings.Trim(value, `"'`)) + } + case strings.HasPrefix(token, "-"): + if curlOptionConsumesValue(token) { + if _, exists := shellToken(tokens, i+1); exists { + i++ + } + } + default: + urls = append(urls, token) + } + } + return method, hasData, useGet, urls +} + +func shellToken(tokens []string, index int) (string, bool) { + if index < 0 || index >= len(tokens) { + return "", false + } + return tokens[index], true +} + +func regexCapture(matches []string, index int) string { + if index < 0 || index >= len(matches) { + return "" + } + return matches[index] +} + +func sliceIntAt(values []int, index int) int { + if index < 0 || index >= len(values) { + return 0 + } + return values[index] +} + +func parsePostStepGHAPIEndpoint(args string) string { + tokens := splitShellTokens(strings.TrimSpace(args)) + filtered := make([]string, 0, len(tokens)) + for i := 0; i < len(tokens); i++ { + token, ok := shellToken(tokens, i) + if !ok { + break + } + if token == "--input" { + i++ + continue + } + if strings.HasPrefix(token, "--input=") { + continue + } + filtered = append(filtered, token) + } + return parseGHAPIEndpoint(strings.Join(filtered, " ")) +} + +func curlOptionConsumesValue(option string) bool { + switch option { + case "-H", "--header", "-u", "--user", "-o", "--output", "-b", "--cookie", + "-c", "--cookie-jar", "-A", "--user-agent", "-e", "--referer", "-m", + "--max-time", "-K", "--config", "-T", "--upload-file": + return true + default: + return false + } +} diff --git a/pkg/workflow/post_steps_validation_test.go b/pkg/workflow/post_steps_validation_test.go new file mode 100644 index 00000000000..dc6d1e2924f --- /dev/null +++ b/pkg/workflow/post_steps_validation_test.go @@ -0,0 +1,163 @@ +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/github/gh-aw/pkg/parser" + "github.com/goccy/go-yaml" + "github.com/stretchr/testify/require" +) + +func TestValidatePostStepsSafeOutputs(t *testing.T) { + tests := []struct { + name, script, want string + }{ + {"gh issue create", `gh issue create --title "test"`, "safe-outputs.create-issue"}, + {"gh issue create in pipeline", `echo title | gh issue create --body-file -`, "safe-outputs.create-issue"}, + {"gh issue create with repo flag", `gh -R owner/repo issue create --title test --body body`, "safe-outputs.create-issue"}, + {"gh api post", `gh api -X POST /repos/octo/repo/issues -f title=test`, "safe-outputs.create-issue"}, + {"gh api implicit post", `gh api repos/octo/repo/issues -f title=test`, "safe-outputs.create-issue"}, + {"gh api input before endpoint", `gh api --input payload.json -X POST repos/octo/repo/issues`, "safe-outputs.create-issue"}, + {"gh api continued command", "gh api repos/octo/repo/issues \\\n -X POST -f title=test", "safe-outputs.create-issue"}, + {"gh api repository expression", `gh api "repos/${{ github.repository }}/issues" -f title=test`, "safe-outputs.create-issue"}, + {"gh api repository variable", `gh api "repos/$GITHUB_REPOSITORY/issues" -f title=test`, "safe-outputs.create-issue"}, + {"curl post", `curl -X POST https://api.github.com/repos/octo/repo/issues`, "safe-outputs.create-issue"}, + {"curl implicit post", `curl --data '{"title":"test"}' https://api.github.com/repos/octo/repo/issues`, "safe-outputs.create-issue"}, + {"curl json post", `curl --json '{"title":"test"}' https://api.github.com/repos/octo/repo/issues`, "safe-outputs.create-issue"}, + {"curl explicit POST overrides get", `curl -G --data-urlencode state=open -X POST https://api.github.com/repos/octo/repo/issues`, "safe-outputs.create-issue"}, + {"curl continued post", "curl -X POST \\\n https://api.github.com/repos/octo/repo/issues", "safe-outputs.create-issue"}, + {"octokit create", `await github.rest.issues.create({owner, repo, title})`, "safe-outputs.create-issue"}, + {"github request post", `await github.request('POST /repos/{owner}/{repo}/issues', {owner, repo, title})`, "safe-outputs.create-issue"}, + {"octokit request post", `await octokit.request('POST /repos/{owner}/{repo}/issues', {owner, repo, title})`, "safe-outputs.create-issue"}, + {"agent output", `jq '.items' /tmp/gh-aw/agent_output.json`, "safe outputs"}, + {"agent output env", `cat "$GH_AW_AGENT_OUTPUT"`, "safe outputs"}, + {"safe outputs output file", `cat /tmp/gh-aw/safeoutputs/output.json`, "safe outputs"}, + {"safe outputs MCP storage", `cat "$RUNNER_TEMP/gh-aw/safeoutputs/outputs.jsonl"`, "safe outputs"}, + {"collected safe outputs", `cat /tmp/gh-aw/safeoutputs.jsonl`, "safe outputs"}, + {"safe outputs runtime path", `cat "${{ steps.set-runtime-paths.outputs.GH_AW_SAFE_OUTPUTS }}"`, "safe outputs"}, + {"read issues", `gh issue list --limit 10`, ""}, + {"read issues with repo flag", `gh -R owner/repo issue list --limit 10`, ""}, + {"read issues api", `gh api /repos/octo/repo/issues`, ""}, + {"read issues api with fields", `gh api -X GET /repos/octo/repo/issues -f state=open`, ""}, + {"curl GET with data", `curl -G --data-urlencode state=open https://api.github.com/repos/octo/repo/issues`, ""}, + {"curl explicit GET with data", `curl -X GET --data '{"state":"open"}' https://api.github.com/repos/octo/repo/issues`, ""}, + {"curl explicit GET overrides JSON", `curl --json '{"state":"open"}' -X GET https://api.github.com/repos/octo/repo/issues`, ""}, + {"curl POST to unrelated route after read", `gh api /repos/octo/repo/issues; curl -X POST https://example.com/other`, ""}, + {"read on previous line before unrelated curl", "gh api /repos/octo/repo/issues\ncurl -X POST https://example.com/other", ""}, + {"comment on issue", `gh api -X POST /repos/octo/repo/issues/12/comments`, ""}, + {"github request GET", `await github.request('GET /repos/{owner}/{repo}/issues', {owner, repo})`, ""}, + {"github request issue comment", `await octokit.request('POST /repos/{owner}/{repo}/issues/{issue_number}/comments', {owner, repo})`, ""}, + {"unrelated output", `cat /tmp/gh-aw/cache-memory/output.json`, ""}, + {"cleanup", `rm -rf /tmp/temp-report`, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validatePostStepsSafeOutputs([]any{map[string]any{"name": "Example", "run": tt.script}}) + if tt.want == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tt.want) + require.ErrorContains(t, err, `post-steps step 1 ("Example")`) + } + }) + } +} + +func TestValidatePostStepsSafeOutputsRejectsArtifactFileInputs(t *testing.T) { + tests := []struct { + name string + path any + want string + }{ + {"agent output file", "/tmp/gh-aw/agent_output.json", "safe outputs"}, + {"safe-output root", "/tmp/gh-aw/", "narrower file path"}, + {"safe-output file in array", []any{"/tmp/gh-aw/agent/", "/tmp/gh-aw/safeoutputs.jsonl"}, "safe outputs"}, + {"permitted artifact directory", "/tmp/gh-aw/agent/", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + steps := []any{map[string]any{ + "name": "Upload", "uses": "actions/upload-artifact@v4", + "with": map[string]any{"path": tt.path}, + }} + err := validatePostStepsSafeOutputs(steps) + if tt.want == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tt.want) + } + }) + } +} + +func TestValidatePostStepsSafeOutputsRejectsSafeOutputCachePaths(t *testing.T) { + steps := []any{map[string]any{ + "name": "Cache", "uses": "actions/cache@v4", + "with": map[string]any{"path": "/tmp/gh-aw/agent_output.json"}, + }} + require.ErrorContains(t, validatePostStepsSafeOutputs(steps), "safe outputs") +} + +func TestValidatePostStepsSafeOutputsRejectsSecretEnv(t *testing.T) { + steps := []any{map[string]any{ + "name": "Expose secret", "run": "echo done", + "env": map[string]any{"TOKEN": "${{ secrets.API_TOKEN }}"}, + }} + require.ErrorContains(t, validatePostStepsSafeOutputs(steps), "secrets are not allowed in post-steps") +} + +func TestValidatePostStepsSafeOutputsScriptInput(t *testing.T) { + steps := []any{map[string]any{ + "name": "Create issue", "uses": "actions/github-script@v7", + "with": map[string]any{"script": "await github.rest.issues.create({owner, repo, title})"}, + }} + require.ErrorContains(t, validatePostStepsSafeOutputs(steps), "safe-outputs.create-issue") +} + +func TestProcessAndMergePostStepsRejectsImportedSafeOutputAccess(t *testing.T) { + imported, err := yaml.Marshal([]any{map[string]any{ + "name": "Read output", "run": "cat /tmp/gh-aw/agent_output.json", + }}) + require.NoError(t, err) + c := NewCompiler() + data := &WorkflowData{} + err = c.processAndMergePostSteps( + map[string]any{"post-steps": []any{map[string]any{"name": "Cleanup", "run": "echo done"}}}, + data, + &parser.ImportsResult{MergedPostSteps: string(imported)}, + ) + require.ErrorContains(t, err, `post-steps step 2 ("Read output")`) + require.ErrorContains(t, err, "safe-outputs") + require.Empty(t, data.PostSteps) +} + +func TestProcessAndMergePostStepsRejectsMainIssueCreation(t *testing.T) { + c := NewCompiler() + data := &WorkflowData{} + err := c.processAndMergePostSteps( + map[string]any{"post-steps": []any{map[string]any{"name": "Publish", "run": "gh issue create --title bug"}}}, + data, + &parser.ImportsResult{}, + ) + require.ErrorContains(t, err, "safe-outputs.create-issue") + require.ErrorContains(t, err, "Publish") +} + +func TestCompileWorkflowRejectsPostStepSafeOutputAccess(t *testing.T) { + file := filepath.Join(t.TempDir(), "workflow.md") + content := `--- +on: workflow_dispatch +engine: copilot +post-steps: + - name: Inspect safe outputs + run: cat /tmp/gh-aw/agent_output.json +--- +# Check results +` + require.NoError(t, os.WriteFile(file, []byte(content), 0600)) + err := NewCompiler().CompileWorkflow(file) + require.ErrorContains(t, err, "post-steps step 1") + require.ErrorContains(t, err, "safe-outputs") +} diff --git a/pkg/workflow/workflow_builder_steps.go b/pkg/workflow/workflow_builder_steps.go index 85b60924cbb..b760954d9e3 100644 --- a/pkg/workflow/workflow_builder_steps.go +++ b/pkg/workflow/workflow_builder_steps.go @@ -8,7 +8,7 @@ import ( ) // processAndMergeSteps handles the merging of imported steps with main workflow steps. -func (c *Compiler) processAndMergeSteps(frontmatter map[string]any, workflowData *WorkflowData, importsResult *parser.ImportsResult) error { +func (c *Compiler) processAndMergeSteps(frontmatter map[string]any, workflowData *WorkflowData, importsResult *parser.ImportsResult) error { //nolint:largefunc // Existing step merge handles main and imported steps together. workflowBuilderLog.Print("Processing and merging custom steps") workflowData.CustomSteps = c.extractTopLevelYAMLSection(frontmatter, "steps") @@ -278,6 +278,9 @@ func (c *Compiler) processAndMergePostSteps(frontmatter map[string]any, workflow if len(mainPostSteps) > 0 || len(importedPostSteps) > 0 { allPostSteps = append(allPostSteps, mainPostSteps...) allPostSteps = append(allPostSteps, importedPostSteps...) + if err := validatePostStepsSafeOutputs(allPostSteps); err != nil { + return err + } stepsWrapper := map[string]any{"post-steps": allPostSteps} stepsYAML, err := yaml.Marshal(stepsWrapper)