From 003abbfbd104ccc91a2d565e21bc002bc09e43a4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 18:58:17 +0000 Subject: [PATCH 1/7] Initial plan From 06307b4df997f42c068b4ba28cbb9c11441c8aa3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 19:30:07 +0000 Subject: [PATCH 2/7] Add workflow_run repository safety check to prevent cross-repo attacks Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/ci-doctor.lock.yml | 2 +- .github/workflows/dev-hawk.lock.yml | 3 +- .github/workflows/smoke-detector.lock.yml | 2 +- .../docs/reference/frontmatter-full.md | 34 --- docs/src/content/docs/status.mdx | 5 +- .../branch_normalize_integration_test.go | 4 +- pkg/workflow/compiler_jobs.go | 36 ++- pkg/workflow/main_job_env_test.go | 2 +- pkg/workflow/role_checks.go | 50 ++++ pkg/workflow/task_job_if_condition_test.go | 11 +- pkg/workflow/workflow_run_repo_safety_test.go | 279 ++++++++++++++++++ 11 files changed, 377 insertions(+), 51 deletions(-) create mode 100644 pkg/workflow/workflow_run_repo_safety_test.go diff --git a/.github/workflows/ci-doctor.lock.yml b/.github/workflows/ci-doctor.lock.yml index 5dbe841cb05..1144443365d 100644 --- a/.github/workflows/ci-doctor.lock.yml +++ b/.github/workflows/ci-doctor.lock.yml @@ -57,7 +57,7 @@ run-name: "CI Failure Doctor" jobs: activation: - if: ${{ github.event.workflow_run.conclusion == 'failure' }} + if: (github.event.workflow_run.conclusion == 'failure') && (github.event.workflow_run.repository.id == github.repository_id) runs-on: ubuntu-slim steps: - name: Check workflow file timestamps diff --git a/.github/workflows/dev-hawk.lock.yml b/.github/workflows/dev-hawk.lock.yml index 845489dc71f..9242bdde809 100644 --- a/.github/workflows/dev-hawk.lock.yml +++ b/.github/workflows/dev-hawk.lock.yml @@ -53,7 +53,8 @@ run-name: "Dev Hawk" jobs: activation: - if: ${{ github.event.workflow_run.event == 'workflow_dispatch' }} + if: > + (github.event.workflow_run.event == 'workflow_dispatch') && (github.event.workflow_run.repository.id == github.repository_id) runs-on: ubuntu-slim steps: - name: Check workflow file timestamps diff --git a/.github/workflows/smoke-detector.lock.yml b/.github/workflows/smoke-detector.lock.yml index 6be0322f084..1ba93fd46e4 100644 --- a/.github/workflows/smoke-detector.lock.yml +++ b/.github/workflows/smoke-detector.lock.yml @@ -70,7 +70,7 @@ run-name: "Smoke Detector - Smoke Test Failure Investigator" jobs: activation: - if: ${{ github.event.workflow_run.conclusion == 'failure' }} + if: (github.event.workflow_run.conclusion == 'failure') && (github.event.workflow_run.repository.id == github.repository_id) runs-on: ubuntu-slim permissions: discussions: write diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index bda6ecd89e2..4cfb9b369d1 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -1239,7 +1239,6 @@ safe-outputs: # (optional) max: 1 - # Target repository in format 'owner/repo' for cross-repository issue creation. # Takes precedence over trial target repo settings. # (optional) @@ -1269,10 +1268,6 @@ safe-outputs: # (optional) max: 1 - # Minimum number of agent tasks to create (default: 0 - no requirement) - # (optional) - min: 1 - # Target repository in format 'owner/repo' for cross-repository agent task # creation. Takes precedence over trial target repo settings. # (optional) @@ -1313,10 +1308,6 @@ safe-outputs: # (optional) max: 1 - # Minimum number of discussions to create (default: 0 - no requirement) - # (optional) - min: 1 - # Target repository in format 'owner/repo' for cross-repository discussion # creation. Takes precedence over trial target repo settings. # (optional) @@ -1340,10 +1331,6 @@ safe-outputs: # (optional) max: 1 - # Minimum number of comments to create (default: 0 - no requirement) - # (optional) - min: 1 - # Target for comments: 'triggering' (default), '*' (any issue), or explicit issue # number # (optional) @@ -1431,10 +1418,6 @@ safe-outputs: # (optional) max: 1 - # Minimum number of review comments to create (default: 0 - no requirement) - # (optional) - min: 1 - # Side of the diff for comments: 'LEFT' or 'RIGHT' (default: 'RIGHT') # (optional) side: "LEFT" @@ -1467,10 +1450,6 @@ safe-outputs: # (optional) max: 1 - # Minimum number of security findings to include (default: 0 - no requirement) - # (optional) - min: 1 - # Driver name for SARIF tool.driver.name field (default: 'GitHub Agentic Workflows # Security Scanner') # (optional) @@ -1504,10 +1483,6 @@ safe-outputs: # (optional) max: 1 - # Minimum number of labels to add (default: 0 - no requirement) - # (optional) - min: 1 - # Target for labels: 'triggering' (default), '*' (any issue/PR), or explicit # issue/PR number # (optional) @@ -1550,10 +1525,6 @@ safe-outputs: # (optional) max: 1 - # Minimum number of issues to update (default: 0 - no requirement) - # (optional) - min: 1 - # Target repository in format 'owner/repo' for cross-repository issue updates. # Takes precedence over trial target repo settings. # (optional) @@ -1621,10 +1592,6 @@ safe-outputs: # (optional) max: 1 - # Minimum number of missing tool reports (default: 0 - no requirement) - # (optional) - min: 1 - # GitHub token to use for this specific output type. Overrides global github-token # if specified. # (optional) @@ -1659,7 +1626,6 @@ safe-outputs: # (optional) max: 1 - # GitHub token to use for this specific output type. Overrides global github-token # if specified. # (optional) diff --git a/docs/src/content/docs/status.mdx b/docs/src/content/docs/status.mdx index 07b1f6caf35..c59b0cf9ce5 100644 --- a/docs/src/content/docs/status.mdx +++ b/docs/src/content/docs/status.mdx @@ -16,13 +16,14 @@ Browse the [workflow source files](https://github.com/githubnext/gh-aw/tree/main | [Basic Research Agent](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/research.md) | copilot | [![Basic Research Agent](https://github.com/githubnext/gh-aw/actions/workflows/research.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/research.lock.yml) | - | - | | [Blog Auditor](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/blog-auditor.md) | claude | [![Blog Auditor](https://github.com/githubnext/gh-aw/actions/workflows/blog-auditor.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/blog-auditor.lock.yml) | `0 12 * * 3` | - | | [Brave Web Search Agent](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/brave.md) | copilot | [![Brave Web Search Agent](https://github.com/githubnext/gh-aw/actions/workflows/brave.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/brave.lock.yml) | - | `/brave` | -| [Changeset Generator](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/changeset-generator.firewall.md) | copilot | [![Changeset Generator](https://github.com/githubnext/gh-aw/actions/workflows/changeset-generator.firewall.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/changeset-generator.firewall.lock.yml) | - | - | +| [Changeset Generator](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/changeset.md) | copilot | [![Changeset Generator](https://github.com/githubnext/gh-aw/actions/workflows/changeset.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/changeset.lock.yml) | - | - | | [CI Failure Doctor](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/ci-doctor.md) | copilot | [![CI Failure Doctor](https://github.com/githubnext/gh-aw/actions/workflows/ci-doctor.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/ci-doctor.lock.yml) | - | - | | [CLI Version Checker](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/cli-version-checker.md) | copilot | [![CLI Version Checker](https://github.com/githubnext/gh-aw/actions/workflows/cli-version-checker.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/cli-version-checker.lock.yml) | `0 15 * * *` | - | | [Commit Changes Analyzer](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/commit-changes-analyzer.md) | claude | [![Commit Changes Analyzer](https://github.com/githubnext/gh-aw/actions/workflows/commit-changes-analyzer.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/commit-changes-analyzer.lock.yml) | - | - | | [Copilot Agent PR Analysis](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/copilot-agent-analysis.md) | claude | [![Copilot Agent PR Analysis](https://github.com/githubnext/gh-aw/actions/workflows/copilot-agent-analysis.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/copilot-agent-analysis.lock.yml) | `0 18 * * *` | - | | [Copilot Agent Prompt Clustering Analysis](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/prompt-clustering-analysis.md) | claude | [![Copilot Agent Prompt Clustering Analysis](https://github.com/githubnext/gh-aw/actions/workflows/prompt-clustering-analysis.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/prompt-clustering-analysis.lock.yml) | `0 19 * * *` | - | | [Copilot PR Prompt Pattern Analysis](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/copilot-pr-prompt-analysis.md) | copilot | [![Copilot PR Prompt Pattern Analysis](https://github.com/githubnext/gh-aw/actions/workflows/copilot-pr-prompt-analysis.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/copilot-pr-prompt-analysis.lock.yml) | `0 9 * * *` | - | +| [Copilot Session Insights](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/copilot-session-insights.md) | claude | [![Copilot Session Insights](https://github.com/githubnext/gh-aw/actions/workflows/copilot-session-insights.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/copilot-session-insights.lock.yml) | `0 16 * * *` | - | | [Daily Documentation Updater](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/daily-doc-updater.md) | claude | [![Daily Documentation Updater](https://github.com/githubnext/gh-aw/actions/workflows/daily-doc-updater.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/daily-doc-updater.lock.yml) | `0 6 * * *` | - | | [Daily Firewall Logs Collector and Reporter](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/daily-firewall-report.md) | copilot | [![Daily Firewall Logs Collector and Reporter](https://github.com/githubnext/gh-aw/actions/workflows/daily-firewall-report.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/daily-firewall-report.lock.yml) | `0 10 * * *` | - | | [Daily News](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/daily-news.md) | copilot | [![Daily News](https://github.com/githubnext/gh-aw/actions/workflows/daily-news.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/daily-news.lock.yml) | `0 9 * * 1-5` | - | @@ -47,6 +48,7 @@ Browse the [workflow source files](https://github.com/githubnext/gh-aw/tree/main | [Mergefest](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/mergefest.md) | copilot | [![Mergefest](https://github.com/githubnext/gh-aw/actions/workflows/mergefest.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/mergefest.lock.yml) | - | `/mergefest` | | [Plan Command](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/plan.md) | copilot | [![Plan Command](https://github.com/githubnext/gh-aw/actions/workflows/plan.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/plan.lock.yml) | - | `/plan` | | [Poem Bot - A Creative Agentic Workflow](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/poem-bot.md) | copilot | [![Poem Bot - A Creative Agentic Workflow](https://github.com/githubnext/gh-aw/actions/workflows/poem-bot.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/poem-bot.lock.yml) | - | `/poem` | +| [Python Data Visualization Generator](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/python-data-charts.md) | copilot | [![Python Data Visualization Generator](https://github.com/githubnext/gh-aw/actions/workflows/python-data-charts.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/python-data-charts.lock.yml) | - | - | | [Q](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/q.md) | copilot | [![Q](https://github.com/githubnext/gh-aw/actions/workflows/q.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/q.lock.yml) | - | `/q` | | [Repository Tree Map Generator](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/repo-tree-map.md) | copilot | [![Repository Tree Map Generator](https://github.com/githubnext/gh-aw/actions/workflows/repo-tree-map.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/repo-tree-map.lock.yml) | - | - | | [Resource Summarizer Agent](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/pdf-summary.md) | copilot | [![Resource Summarizer Agent](https://github.com/githubnext/gh-aw/actions/workflows/pdf-summary.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/pdf-summary.lock.yml) | - | `/summarize` | @@ -67,6 +69,7 @@ Browse the [workflow source files](https://github.com/githubnext/gh-aw/tree/main | [Test Post-Steps Workflow](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/test-post-steps.md) | copilot | [![Test Post-Steps Workflow](https://github.com/githubnext/gh-aw/actions/workflows/test-post-steps.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/test-post-steps.lock.yml) | - | - | | [Test Secret Masking Workflow](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/test-secret-masking.md) | copilot | [![Test Secret Masking Workflow](https://github.com/githubnext/gh-aw/actions/workflows/test-secret-masking.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/test-secret-masking.lock.yml) | - | - | | [Test Svelte MCP](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/test-svelte.md) | copilot | [![Test Svelte MCP](https://github.com/githubnext/gh-aw/actions/workflows/test-svelte.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/test-svelte.lock.yml) | - | - | +| [Test Workflow Timestamp Check](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/test-timestamp-js.md) | copilot | [![Test Workflow Timestamp Check](https://github.com/githubnext/gh-aw/actions/workflows/test-timestamp-js.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/test-timestamp-js.lock.yml) | - | - | | [The Daily Repository Chronicle](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/daily-repo-chronicle.md) | copilot | [![The Daily Repository Chronicle](https://github.com/githubnext/gh-aw/actions/workflows/daily-repo-chronicle.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/daily-repo-chronicle.lock.yml) | `0 16 * * 1-5` | - | | [Tidy](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/tidy.md) | copilot | [![Tidy](https://github.com/githubnext/gh-aw/actions/workflows/tidy.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/tidy.lock.yml) | - | - | | [Video Analysis Agent](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/video-analyzer.md) | copilot | [![Video Analysis Agent](https://github.com/githubnext/gh-aw/actions/workflows/video-analyzer.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/video-analyzer.lock.yml) | - | - | diff --git a/pkg/workflow/branch_normalize_integration_test.go b/pkg/workflow/branch_normalize_integration_test.go index 54e3b51daa8..5838a401c38 100644 --- a/pkg/workflow/branch_normalize_integration_test.go +++ b/pkg/workflow/branch_normalize_integration_test.go @@ -27,7 +27,7 @@ func TestBranchNormalizationInlinedInMainJob(t *testing.T) { } // Build the main job - job, err := compiler.buildMainJob(data, false) + job, err := compiler.buildMainJob(data, false, "") if err != nil { t.Fatalf("Failed to build main job: %v", err) } @@ -64,7 +64,7 @@ func TestBranchNormalizationStepNotAddedWhenNoUploadAssets(t *testing.T) { } // Build the main job - job, err := compiler.buildMainJob(data, false) + job, err := compiler.buildMainJob(data, false, "") if err != nil { t.Fatalf("Failed to build main job: %v", err) } diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 68166a089ee..e3e8c02bfdd 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -42,11 +42,19 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { needsPermissionCheck := c.needsRoleCheck(data, frontmatter) hasStopTime := data.StopTime != "" + // Determine if we need to add workflow_run repository safety check + // Add the check whenever the compiled workflow has a workflow_run trigger + // This prevents cross-repository workflow_run attacks + var workflowRunRepoSafety string + if c.hasWorkflowRunTrigger(data) { + workflowRunRepoSafety = c.buildWorkflowRunRepoSafetyCondition() + } + // Build pre-activation job if needed (combines membership checks, stop-time validation, and command position check) var preActivationJobCreated bool hasCommandTrigger := data.Command != "" if needsPermissionCheck || hasStopTime || hasCommandTrigger { - preActivationJob, err := c.buildPreActivationJob(data, needsPermissionCheck) + preActivationJob, err := c.buildPreActivationJob(data, needsPermissionCheck, workflowRunRepoSafety) if err != nil { return fmt.Errorf("failed to build %s job: %w", constants.PreActivationJobName, err) } @@ -61,7 +69,7 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { var activationJobCreated bool if c.isActivationJobNeeded() { - activationJob, err := c.buildActivationJob(data, preActivationJobCreated) + activationJob, err := c.buildActivationJob(data, preActivationJobCreated, workflowRunRepoSafety) if err != nil { return fmt.Errorf("failed to build activation job: %w", err) } @@ -72,7 +80,7 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { } // Build main workflow job - mainJob, err := c.buildMainJob(data, activationJobCreated) + mainJob, err := c.buildMainJob(data, activationJobCreated, workflowRunRepoSafety) if err != nil { return fmt.Errorf("failed to build main job: %w", err) } @@ -346,7 +354,7 @@ func (c *Compiler) buildSafeOutputsJobs(data *WorkflowData, jobName, markdownPat // buildPreActivationJob creates a unified pre-activation job that combines membership checks and stop-time validation // This job exposes a single "activated" output that indicates whether the workflow should proceed -func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionCheck bool) (*Job, error) { +func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionCheck bool, workflowRunRepoSafety string) (*Job, error) { var steps []string var permissions string @@ -448,9 +456,13 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec "activated": activatedExpression, } + // Determine the job-level if condition + // Combine user's if condition with workflow_run repository safety check + jobIfCondition := c.combineJobIfConditions(data.If, workflowRunRepoSafety) + job := &Job{ Name: constants.PreActivationJobName, - If: data.If, // Use the existing condition (which may include alias checks) + If: jobIfCondition, RunsOn: c.formatSafeOutputsRunsOn(data.SafeOutputs), Permissions: permissions, Steps: steps, @@ -461,7 +473,7 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec } // buildActivationJob creates the preamble activation job that acts as a barrier for runtime conditions -func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreated bool) (*Job, error) { +func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreated bool, workflowRunRepoSafety string) (*Job, error) { outputs := map[string]string{} var steps []string @@ -555,6 +567,9 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate activationCondition = data.If } + // Combine activation condition with workflow_run repository safety check + activationCondition = c.combineJobIfConditions(activationCondition, workflowRunRepoSafety) + // Set permissions - add reaction permissions if reaction is configured and not "none" var permissions string if data.AIReaction != "" && data.AIReaction != "none" { @@ -580,13 +595,20 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate } // buildMainJob creates the main workflow job -func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) (*Job, error) { +func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool, workflowRunRepoSafety string) (*Job, error) { var steps []string var jobCondition = data.If if activationJobCreated { jobCondition = "" // Main job depends on activation job, so no need for inline condition } + + // If there's no activation job, we need to add the workflow_run repository safety check + // to the main job directly + if !activationJobCreated { + jobCondition = c.combineJobIfConditions(jobCondition, workflowRunRepoSafety) + } + // Permission checks are now handled by the separate check_membership job // No role checks needed in the main job diff --git a/pkg/workflow/main_job_env_test.go b/pkg/workflow/main_job_env_test.go index 359421ec80b..6a0a3f69f45 100644 --- a/pkg/workflow/main_job_env_test.go +++ b/pkg/workflow/main_job_env_test.go @@ -70,7 +70,7 @@ func TestMainJobEnvironmentVariables(t *testing.T) { data.SafeOutputs = compiler.extractSafeOutputsConfig(tt.frontmatter) // Build the main job - job, err := compiler.buildMainJob(data, false) + job, err := compiler.buildMainJob(data, false, "") if err != nil { t.Fatalf("Failed to build main job: %v", err) } diff --git a/pkg/workflow/role_checks.go b/pkg/workflow/role_checks.go index de6773458aa..1d2ec91a522 100644 --- a/pkg/workflow/role_checks.go +++ b/pkg/workflow/role_checks.go @@ -179,3 +179,53 @@ func (c *Compiler) hasSafeEventsOnly(data *WorkflowData, frontmatter map[string] // For command workflows, they are not considered "safe only" return false } + +// hasWorkflowRunTrigger checks if the compiled workflow has a workflow_run trigger in its "on" section +// This checks the compiled workflow's trigger configuration, NOT the agentic workflow's frontmatter +func (c *Compiler) hasWorkflowRunTrigger(data *WorkflowData) bool { + // Parse the "on" section from the compiled workflow data + // The "on" field contains the rendered YAML triggers + if data.On == "" { + return false + } + + // Simple string check for workflow_run trigger + // The data.On field contains the rendered "on:" section, so we check for "workflow_run:" + return strings.Contains(data.On, "workflow_run:") +} + +// buildWorkflowRunRepoSafetyCondition generates the if condition to ensure workflow_run is from same repo +func (c *Compiler) buildWorkflowRunRepoSafetyCondition() string { + // Use ExpressionNode to build the condition properly + repoIDCheck := BuildEquals( + BuildPropertyAccess("github.event.workflow_run.repository.id"), + BuildPropertyAccess("github.repository_id"), + ) + // Wrap in ${{ }} for GitHub Actions + return fmt.Sprintf("${{ %s }}", repoIDCheck.Render()) +} + +// combineJobIfConditions combines an existing if condition with workflow_run repository safety check +// Returns the combined condition, or just one of them if the other is empty +func (c *Compiler) combineJobIfConditions(existingCondition, workflowRunRepoSafety string) string { + // If no workflow_run safety check needed, return existing condition + if workflowRunRepoSafety == "" { + return existingCondition + } + + // If no existing condition, return just the workflow_run safety check + if existingCondition == "" { + return workflowRunRepoSafety + } + + // Both conditions present - combine them with AND + // Strip ${{ }} wrapper from existingCondition if present + unwrappedExisting := stripExpressionWrapper(existingCondition) + unwrappedSafety := stripExpressionWrapper(workflowRunRepoSafety) + + existingNode := &ExpressionNode{Expression: unwrappedExisting} + safetyNode := &ExpressionNode{Expression: unwrappedSafety} + + combinedExpr := buildAnd(existingNode, safetyNode) + return combinedExpr.Render() +} diff --git a/pkg/workflow/task_job_if_condition_test.go b/pkg/workflow/task_job_if_condition_test.go index cbe5e81531d..5f72ee300bd 100644 --- a/pkg/workflow/task_job_if_condition_test.go +++ b/pkg/workflow/task_job_if_condition_test.go @@ -61,9 +61,14 @@ Check the failed workflow and provide analysis.` t.Error("Expected activation job to be present in generated workflow") } - // Test 2: Verify activation job has the if condition - if !strings.Contains(lockContentStr, "if: ${{ github.event.workflow_run.conclusion == 'failure' }}") { - t.Error("Expected activation job to have the if condition") + // Test 2: Verify activation job has the user's if condition (may be combined with other conditions) + if !strings.Contains(lockContentStr, "github.event.workflow_run.conclusion == 'failure'") { + t.Error("Expected activation job to include the user's if condition") + } + + // Test 2b: Verify activation job has the workflow_run repository safety check + if !strings.Contains(lockContentStr, "github.event.workflow_run.repository.id == github.repository_id") { + t.Error("Expected activation job to include the workflow_run repository safety check") } // Test 3: Verify activation job has steps (specifically the dummy step) diff --git a/pkg/workflow/workflow_run_repo_safety_test.go b/pkg/workflow/workflow_run_repo_safety_test.go new file mode 100644 index 00000000000..21ee46dbfcf --- /dev/null +++ b/pkg/workflow/workflow_run_repo_safety_test.go @@ -0,0 +1,279 @@ +package workflow + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestWorkflowRunRepoSafetyCheck tests that workflow_run triggers get repository safety checks +func TestWorkflowRunRepoSafetyCheck(t *testing.T) { + tests := []struct { + name string + workflowContent string + expectSafetyCondition bool + expectedConditionString string + }{ + { + name: "workflow with workflow_run trigger declared in frontmatter SHOULD include repo safety check", + workflowContent: `--- +on: + workflow_run: + workflows: ["CI"] + types: [completed] +--- + +# Test Workflow + +Analyze the CI run.`, + expectSafetyCondition: true, + expectedConditionString: "github.event.workflow_run.repository.id == github.repository_id", + }, + { + name: "workflow with workflow_run and if condition should include repo safety check and combine conditions", + workflowContent: `--- +on: + workflow_run: + workflows: ["CI"] + types: [completed] +if: ${{ github.event.workflow_run.conclusion == 'failure' }} +--- + +# Test Workflow + +Analyze failed CI run.`, + expectSafetyCondition: true, + expectedConditionString: "github.event.workflow_run.repository.id == github.repository_id", + }, + { + name: "workflow with push trigger should NOT include repo safety check", + workflowContent: `--- +on: + push: + branches: [main] +--- + +# Test Workflow + +Do something on push.`, + expectSafetyCondition: false, + expectedConditionString: "", + }, + { + name: "workflow with issues trigger should NOT include repo safety check", + workflowContent: `--- +on: + issues: + types: [opened] +--- + +# Test Workflow + +Do something on issue.`, + expectSafetyCondition: false, + expectedConditionString: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a temporary directory for the test + tmpDir, err := os.MkdirTemp("", "workflow-run-safety-test*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + // Write the test workflow file + workflowFile := filepath.Join(tmpDir, "test-workflow.md") + err = os.WriteFile(workflowFile, []byte(tt.workflowContent), 0644) + if err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Compile the workflow + compiler := NewCompiler(false, "", "test") + + err = compiler.CompileWorkflow(workflowFile) + if err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + // Read the generated lock file + lockFile := strings.TrimSuffix(workflowFile, ".md") + ".lock.yml" + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + lockContentStr := string(lockContent) + + // Check if the safety condition is present in job if clauses + // Search for it specifically in "if:" lines to avoid false positives from JavaScript code + hasSafetyCondition := false + if tt.expectedConditionString != "" { + lines := strings.Split(lockContentStr, "\n") + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "if:") && strings.Contains(trimmed, tt.expectedConditionString) { + hasSafetyCondition = true + break + } + } + } + + if tt.expectSafetyCondition && !hasSafetyCondition { + t.Errorf("Expected workflow_run repository safety condition to be present in job if clause, but it was not found") + t.Logf("Searched for: %s", tt.expectedConditionString) + } + + if !tt.expectSafetyCondition && hasSafetyCondition { + t.Errorf("Expected NO workflow_run repository safety condition in job if clause, but it was found") + t.Logf("Found condition: %s", tt.expectedConditionString) + } + + // Additional check: if safety condition is expected, verify it's in a job's if clause + if tt.expectSafetyCondition { + // The condition should appear in one of these patterns: + // - "if: ${{ github.event.workflow_run.repository.id == github.repository_id }}" + // - "if: >\n github.event.workflow_run.repository.id == github.repository_id" + // - Combined with another condition using && + + // Look for the activation job and check its if condition + activationJobStart := strings.Index(lockContentStr, "activation:") + if activationJobStart == -1 { + // No activation job, check the main agent job + agentJobStart := strings.Index(lockContentStr, "agent:") + if agentJobStart == -1 { + t.Fatalf("Neither activation nor agent job found in lock file") + } + } + } + }) + } +} + +// TestWorkflowRunRepoSafetyInActivationJob tests that the safety check appears in activation job +func TestWorkflowRunRepoSafetyInActivationJob(t *testing.T) { + workflowContent := `--- +on: + workflow_run: + workflows: ["Daily Perf Improver", "Daily Test Coverage Improver"] + types: + - completed + stop-after: +48h + +if: ${{ github.event.workflow_run.conclusion == 'failure' }} +--- + +# CI Doctor + +This workflow runs when CI workflows fail to help diagnose issues.` + + // Create a temporary directory for the test + tmpDir, err := os.MkdirTemp("", "workflow-run-safety-activation-test*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + // Write the test workflow file + workflowFile := filepath.Join(tmpDir, "test-workflow.md") + err = os.WriteFile(workflowFile, []byte(workflowContent), 0644) + if err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Compile the workflow + compiler := NewCompiler(false, "", "test") + + err = compiler.CompileWorkflow(workflowFile) + if err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + // Read the generated lock file + lockFile := strings.TrimSuffix(workflowFile, ".md") + ".lock.yml" + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + lockContentStr := string(lockContent) + + // Verify the activation job exists + if !strings.Contains(lockContentStr, "activation:") { + t.Error("Expected activation job to be present") + } + + // Verify the repository safety condition is present + expectedCondition := "github.event.workflow_run.repository.id == github.repository_id" + if !strings.Contains(lockContentStr, expectedCondition) { + t.Errorf("Expected repository safety condition to be present in lock file") + t.Logf("Lock file content:\n%s", lockContentStr) + } + + // Verify the user's if condition is also present + userCondition := "github.event.workflow_run.conclusion == 'failure'" + if !strings.Contains(lockContentStr, userCondition) { + t.Errorf("Expected user's if condition to be preserved") + t.Logf("Lock file content:\n%s", lockContentStr) + } + + // Both conditions should be combined with && + if !strings.Contains(lockContentStr, "&&") { + t.Error("Expected conditions to be combined with &&") + } +} + +// TestNoWorkflowRunRepoSafetyForPushTrigger tests that push triggers don't get the safety check +func TestNoWorkflowRunRepoSafetyForPushTrigger(t *testing.T) { + workflowContent := `--- +on: + push: + branches: [main] +--- + +# Push Workflow + +Do something on push.` + + // Create a temporary directory for the test + tmpDir, err := os.MkdirTemp("", "no-workflow-run-safety-test*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + // Write the test workflow file + workflowFile := filepath.Join(tmpDir, "test-workflow.md") + err = os.WriteFile(workflowFile, []byte(workflowContent), 0644) + if err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Compile the workflow + compiler := NewCompiler(false, "", "test") + + err = compiler.CompileWorkflow(workflowFile) + if err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + // Read the generated lock file + lockFile := strings.TrimSuffix(workflowFile, ".md") + ".lock.yml" + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + lockContentStr := string(lockContent) + + // Verify the repository safety condition is NOT present + unexpectedCondition := "github.event.workflow_run.repository.id" + if strings.Contains(lockContentStr, unexpectedCondition) { + t.Errorf("Did not expect repository safety condition in push-triggered workflow") + t.Logf("Lock file content:\n%s", lockContentStr) + } +} From 2f73bd052aca70cddf9849ee019627e76b11854d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 20:27:46 +0000 Subject: [PATCH 3/7] Check frontmatter for workflow_run trigger instead of compiled output Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_jobs.go | 4 ++-- pkg/workflow/role_checks.go | 26 +++++++++++++++++--------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index e3e8c02bfdd..cc2fbef04c9 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -43,10 +43,10 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { hasStopTime := data.StopTime != "" // Determine if we need to add workflow_run repository safety check - // Add the check whenever the compiled workflow has a workflow_run trigger + // Add the check if the agentic workflow declares a workflow_run trigger // This prevents cross-repository workflow_run attacks var workflowRunRepoSafety string - if c.hasWorkflowRunTrigger(data) { + if c.hasWorkflowRunTrigger(frontmatter) { workflowRunRepoSafety = c.buildWorkflowRunRepoSafetyCondition() } diff --git a/pkg/workflow/role_checks.go b/pkg/workflow/role_checks.go index 1d2ec91a522..b7bc8a39205 100644 --- a/pkg/workflow/role_checks.go +++ b/pkg/workflow/role_checks.go @@ -180,18 +180,26 @@ func (c *Compiler) hasSafeEventsOnly(data *WorkflowData, frontmatter map[string] return false } -// hasWorkflowRunTrigger checks if the compiled workflow has a workflow_run trigger in its "on" section -// This checks the compiled workflow's trigger configuration, NOT the agentic workflow's frontmatter -func (c *Compiler) hasWorkflowRunTrigger(data *WorkflowData) bool { - // Parse the "on" section from the compiled workflow data - // The "on" field contains the rendered YAML triggers - if data.On == "" { +// hasWorkflowRunTrigger checks if the agentic workflow's frontmatter declares a workflow_run trigger +func (c *Compiler) hasWorkflowRunTrigger(frontmatter map[string]any) bool { + if frontmatter == nil { return false } - // Simple string check for workflow_run trigger - // The data.On field contains the rendered "on:" section, so we check for "workflow_run:" - return strings.Contains(data.On, "workflow_run:") + // Check the "on" section in frontmatter + if onValue, exists := frontmatter["on"]; exists { + // Handle map format (most common) + if onMap, ok := onValue.(map[string]any); ok { + _, hasWorkflowRun := onMap["workflow_run"] + return hasWorkflowRun + } + // Handle string format + if onStr, ok := onValue.(string); ok { + return onStr == "workflow_run" + } + } + + return false } // buildWorkflowRunRepoSafetyCondition generates the if condition to ensure workflow_run is from same repo From e3d92913834714cee48b8be654e2e46c9eeffd52 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 21:03:02 +0000 Subject: [PATCH 4/7] Add event_name check to workflow_run repository safety condition Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/ci-doctor.lock.yml | 3 +- .github/workflows/dev-hawk.lock.yml | 3 +- .github/workflows/smoke-detector.lock.yml | 3 +- pkg/workflow/role_checks.go | 15 ++++++++-- pkg/workflow/task_job_if_condition_test.go | 7 ++++- pkg/workflow/workflow_run_repo_safety_test.go | 28 +++++++++++-------- 6 files changed, 41 insertions(+), 18 deletions(-) diff --git a/.github/workflows/ci-doctor.lock.yml b/.github/workflows/ci-doctor.lock.yml index 1144443365d..abe8822e80f 100644 --- a/.github/workflows/ci-doctor.lock.yml +++ b/.github/workflows/ci-doctor.lock.yml @@ -57,7 +57,8 @@ run-name: "CI Failure Doctor" jobs: activation: - if: (github.event.workflow_run.conclusion == 'failure') && (github.event.workflow_run.repository.id == github.repository_id) + if: > + (github.event.workflow_run.conclusion == 'failure') && ((github.event_name == 'workflow_run') && (github.event.workflow_run.repository.id == github.repository_id)) runs-on: ubuntu-slim steps: - name: Check workflow file timestamps diff --git a/.github/workflows/dev-hawk.lock.yml b/.github/workflows/dev-hawk.lock.yml index 9242bdde809..5c86896dbd1 100644 --- a/.github/workflows/dev-hawk.lock.yml +++ b/.github/workflows/dev-hawk.lock.yml @@ -54,7 +54,8 @@ run-name: "Dev Hawk" jobs: activation: if: > - (github.event.workflow_run.event == 'workflow_dispatch') && (github.event.workflow_run.repository.id == github.repository_id) + (github.event.workflow_run.event == 'workflow_dispatch') && ((github.event_name == 'workflow_run') && + (github.event.workflow_run.repository.id == github.repository_id)) runs-on: ubuntu-slim steps: - name: Check workflow file timestamps diff --git a/.github/workflows/smoke-detector.lock.yml b/.github/workflows/smoke-detector.lock.yml index 1ba93fd46e4..c319ea842c5 100644 --- a/.github/workflows/smoke-detector.lock.yml +++ b/.github/workflows/smoke-detector.lock.yml @@ -70,7 +70,8 @@ run-name: "Smoke Detector - Smoke Test Failure Investigator" jobs: activation: - if: (github.event.workflow_run.conclusion == 'failure') && (github.event.workflow_run.repository.id == github.repository_id) + if: > + (github.event.workflow_run.conclusion == 'failure') && ((github.event_name == 'workflow_run') && (github.event.workflow_run.repository.id == github.repository_id)) runs-on: ubuntu-slim permissions: discussions: write diff --git a/pkg/workflow/role_checks.go b/pkg/workflow/role_checks.go index b7bc8a39205..45c9866b8c6 100644 --- a/pkg/workflow/role_checks.go +++ b/pkg/workflow/role_checks.go @@ -203,14 +203,25 @@ func (c *Compiler) hasWorkflowRunTrigger(frontmatter map[string]any) bool { } // buildWorkflowRunRepoSafetyCondition generates the if condition to ensure workflow_run is from same repo +// The condition only applies when the event is actually a workflow_run event func (c *Compiler) buildWorkflowRunRepoSafetyCondition() string { - // Use ExpressionNode to build the condition properly + // Check that event is workflow_run + eventCheck := BuildEquals( + BuildPropertyAccess("github.event_name"), + BuildStringLiteral("workflow_run"), + ) + + // Check that repository IDs match repoIDCheck := BuildEquals( BuildPropertyAccess("github.event.workflow_run.repository.id"), BuildPropertyAccess("github.repository_id"), ) + + // Combine both checks with AND + combinedCheck := buildAnd(eventCheck, repoIDCheck) + // Wrap in ${{ }} for GitHub Actions - return fmt.Sprintf("${{ %s }}", repoIDCheck.Render()) + return fmt.Sprintf("${{ %s }}", combinedCheck.Render()) } // combineJobIfConditions combines an existing if condition with workflow_run repository safety check diff --git a/pkg/workflow/task_job_if_condition_test.go b/pkg/workflow/task_job_if_condition_test.go index 5f72ee300bd..c3b70d90fd4 100644 --- a/pkg/workflow/task_job_if_condition_test.go +++ b/pkg/workflow/task_job_if_condition_test.go @@ -66,7 +66,12 @@ Check the failed workflow and provide analysis.` t.Error("Expected activation job to include the user's if condition") } - // Test 2b: Verify activation job has the workflow_run repository safety check + // Test 2b: Verify activation job has the workflow_run event_name check + if !strings.Contains(lockContentStr, "github.event_name == 'workflow_run'") { + t.Error("Expected activation job to include the event_name check") + } + + // Test 2c: Verify activation job has the workflow_run repository safety check if !strings.Contains(lockContentStr, "github.event.workflow_run.repository.id == github.repository_id") { t.Error("Expected activation job to include the workflow_run repository safety check") } diff --git a/pkg/workflow/workflow_run_repo_safety_test.go b/pkg/workflow/workflow_run_repo_safety_test.go index 21ee46dbfcf..06c1d686fa4 100644 --- a/pkg/workflow/workflow_run_repo_safety_test.go +++ b/pkg/workflow/workflow_run_repo_safety_test.go @@ -28,7 +28,7 @@ on: Analyze the CI run.`, expectSafetyCondition: true, - expectedConditionString: "github.event.workflow_run.repository.id == github.repository_id", + expectedConditionString: "github.event_name == 'workflow_run'", }, { name: "workflow with workflow_run and if condition should include repo safety check and combine conditions", @@ -44,7 +44,7 @@ if: ${{ github.event.workflow_run.conclusion == 'failure' }} Analyze failed CI run.`, expectSafetyCondition: true, - expectedConditionString: "github.event.workflow_run.repository.id == github.repository_id", + expectedConditionString: "github.event_name == 'workflow_run'", }, { name: "workflow with push trigger should NOT include repo safety check", @@ -110,17 +110,14 @@ Do something on issue.`, lockContentStr := string(lockContent) // Check if the safety condition is present in job if clauses - // Search for it specifically in "if:" lines to avoid false positives from JavaScript code + // For multiline conditions with "if: >", the condition is on subsequent lines + // So we check if the expected string exists anywhere in the lock file hasSafetyCondition := false if tt.expectedConditionString != "" { - lines := strings.Split(lockContentStr, "\n") - for _, line := range lines { - trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, "if:") && strings.Contains(trimmed, tt.expectedConditionString) { - hasSafetyCondition = true - break - } - } + // Simply check if the expected condition string exists in the file + // This is safe because we're looking for specific GitHub expression syntax + // that wouldn't appear in JavaScript code + hasSafetyCondition = strings.Contains(lockContentStr, tt.expectedConditionString) } if tt.expectSafetyCondition && !hasSafetyCondition { @@ -207,6 +204,13 @@ This workflow runs when CI workflows fail to help diagnose issues.` t.Error("Expected activation job to be present") } + // Verify the event_name check is present + eventNameCondition := "github.event_name == 'workflow_run'" + if !strings.Contains(lockContentStr, eventNameCondition) { + t.Errorf("Expected event_name check to be present in lock file") + t.Logf("Lock file content:\n%s", lockContentStr) + } + // Verify the repository safety condition is present expectedCondition := "github.event.workflow_run.repository.id == github.repository_id" if !strings.Contains(lockContentStr, expectedCondition) { @@ -221,7 +225,7 @@ This workflow runs when CI workflows fail to help diagnose issues.` t.Logf("Lock file content:\n%s", lockContentStr) } - // Both conditions should be combined with && + // All conditions should be combined with && if !strings.Contains(lockContentStr, "&&") { t.Error("Expected conditions to be combined with &&") } From d0d378fe582248870c0ac082df9582674d641123 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 21:38:28 +0000 Subject: [PATCH 5/7] Change safety condition to use OR logic: (event != workflow_run) || (repo matches) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/ci-doctor.lock.yml | 2 +- .github/workflows/dev-hawk.lock.yml | 2 +- .github/workflows/smoke-detector.lock.yml | 2 +- pkg/workflow/role_checks.go | 11 ++++++----- pkg/workflow/task_job_if_condition_test.go | 6 +++--- pkg/workflow/workflow_run_repo_safety_test.go | 14 +++++++------- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/.github/workflows/ci-doctor.lock.yml b/.github/workflows/ci-doctor.lock.yml index abe8822e80f..f7163eceb04 100644 --- a/.github/workflows/ci-doctor.lock.yml +++ b/.github/workflows/ci-doctor.lock.yml @@ -58,7 +58,7 @@ run-name: "CI Failure Doctor" jobs: activation: if: > - (github.event.workflow_run.conclusion == 'failure') && ((github.event_name == 'workflow_run') && (github.event.workflow_run.repository.id == github.repository_id)) + (github.event.workflow_run.conclusion == 'failure') && ((github.event_name != 'workflow_run') || (github.event.workflow_run.repository.id == github.repository_id)) runs-on: ubuntu-slim steps: - name: Check workflow file timestamps diff --git a/.github/workflows/dev-hawk.lock.yml b/.github/workflows/dev-hawk.lock.yml index 5c86896dbd1..40a0f47e335 100644 --- a/.github/workflows/dev-hawk.lock.yml +++ b/.github/workflows/dev-hawk.lock.yml @@ -54,7 +54,7 @@ run-name: "Dev Hawk" jobs: activation: if: > - (github.event.workflow_run.event == 'workflow_dispatch') && ((github.event_name == 'workflow_run') && + (github.event.workflow_run.event == 'workflow_dispatch') && ((github.event_name != 'workflow_run') || (github.event.workflow_run.repository.id == github.repository_id)) runs-on: ubuntu-slim steps: diff --git a/.github/workflows/smoke-detector.lock.yml b/.github/workflows/smoke-detector.lock.yml index c319ea842c5..bae85a0e8ae 100644 --- a/.github/workflows/smoke-detector.lock.yml +++ b/.github/workflows/smoke-detector.lock.yml @@ -71,7 +71,7 @@ run-name: "Smoke Detector - Smoke Test Failure Investigator" jobs: activation: if: > - (github.event.workflow_run.conclusion == 'failure') && ((github.event_name == 'workflow_run') && (github.event.workflow_run.repository.id == github.repository_id)) + (github.event.workflow_run.conclusion == 'failure') && ((github.event_name != 'workflow_run') || (github.event.workflow_run.repository.id == github.repository_id)) runs-on: ubuntu-slim permissions: discussions: write diff --git a/pkg/workflow/role_checks.go b/pkg/workflow/role_checks.go index 45c9866b8c6..8eca1fb35f4 100644 --- a/pkg/workflow/role_checks.go +++ b/pkg/workflow/role_checks.go @@ -203,10 +203,11 @@ func (c *Compiler) hasWorkflowRunTrigger(frontmatter map[string]any) bool { } // buildWorkflowRunRepoSafetyCondition generates the if condition to ensure workflow_run is from same repo -// The condition only applies when the event is actually a workflow_run event +// The condition uses: (event_name != 'workflow_run') OR (repository IDs match) +// This allows all non-workflow_run events, but requires repository match for workflow_run events func (c *Compiler) buildWorkflowRunRepoSafetyCondition() string { - // Check that event is workflow_run - eventCheck := BuildEquals( + // Check that event is NOT workflow_run + eventNotWorkflowRun := BuildNotEquals( BuildPropertyAccess("github.event_name"), BuildStringLiteral("workflow_run"), ) @@ -217,8 +218,8 @@ func (c *Compiler) buildWorkflowRunRepoSafetyCondition() string { BuildPropertyAccess("github.repository_id"), ) - // Combine both checks with AND - combinedCheck := buildAnd(eventCheck, repoIDCheck) + // Combine with OR: allow if NOT workflow_run OR repository matches + combinedCheck := buildOr(eventNotWorkflowRun, repoIDCheck) // Wrap in ${{ }} for GitHub Actions return fmt.Sprintf("${{ %s }}", combinedCheck.Render()) diff --git a/pkg/workflow/task_job_if_condition_test.go b/pkg/workflow/task_job_if_condition_test.go index c3b70d90fd4..182e26fab71 100644 --- a/pkg/workflow/task_job_if_condition_test.go +++ b/pkg/workflow/task_job_if_condition_test.go @@ -66,9 +66,9 @@ Check the failed workflow and provide analysis.` t.Error("Expected activation job to include the user's if condition") } - // Test 2b: Verify activation job has the workflow_run event_name check - if !strings.Contains(lockContentStr, "github.event_name == 'workflow_run'") { - t.Error("Expected activation job to include the event_name check") + // Test 2b: Verify activation job has the workflow_run event_name check (using !=) + if !strings.Contains(lockContentStr, "github.event_name != 'workflow_run'") { + t.Error("Expected activation job to include the event_name check with != operator") } // Test 2c: Verify activation job has the workflow_run repository safety check diff --git a/pkg/workflow/workflow_run_repo_safety_test.go b/pkg/workflow/workflow_run_repo_safety_test.go index 06c1d686fa4..9bfa225c18b 100644 --- a/pkg/workflow/workflow_run_repo_safety_test.go +++ b/pkg/workflow/workflow_run_repo_safety_test.go @@ -28,7 +28,7 @@ on: Analyze the CI run.`, expectSafetyCondition: true, - expectedConditionString: "github.event_name == 'workflow_run'", + expectedConditionString: "github.event_name != 'workflow_run'", }, { name: "workflow with workflow_run and if condition should include repo safety check and combine conditions", @@ -44,7 +44,7 @@ if: ${{ github.event.workflow_run.conclusion == 'failure' }} Analyze failed CI run.`, expectSafetyCondition: true, - expectedConditionString: "github.event_name == 'workflow_run'", + expectedConditionString: "github.event_name != 'workflow_run'", }, { name: "workflow with push trigger should NOT include repo safety check", @@ -204,8 +204,8 @@ This workflow runs when CI workflows fail to help diagnose issues.` t.Error("Expected activation job to be present") } - // Verify the event_name check is present - eventNameCondition := "github.event_name == 'workflow_run'" + // Verify the event_name check is present (using != instead of ==) + eventNameCondition := "github.event_name != 'workflow_run'" if !strings.Contains(lockContentStr, eventNameCondition) { t.Errorf("Expected event_name check to be present in lock file") t.Logf("Lock file content:\n%s", lockContentStr) @@ -225,9 +225,9 @@ This workflow runs when CI workflows fail to help diagnose issues.` t.Logf("Lock file content:\n%s", lockContentStr) } - // All conditions should be combined with && - if !strings.Contains(lockContentStr, "&&") { - t.Error("Expected conditions to be combined with &&") + // Verify OR operator is used in the safety condition + if !strings.Contains(lockContentStr, "||") { + t.Error("Expected safety condition to use || operator") } } From f47d4df6d4509c97ead40b197072bb39ea81cc37 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 22:36:51 +0000 Subject: [PATCH 6/7] Move workflow_run safety check to workflow-level if field (data.If) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../branch_normalize_integration_test.go | 4 +- pkg/workflow/compiler_jobs.go | 45 +++++++++---------- pkg/workflow/compiler_yaml.go | 3 ++ pkg/workflow/main_job_env_test.go | 2 +- 4 files changed, 26 insertions(+), 28 deletions(-) diff --git a/pkg/workflow/branch_normalize_integration_test.go b/pkg/workflow/branch_normalize_integration_test.go index 5838a401c38..9df80e52e6b 100644 --- a/pkg/workflow/branch_normalize_integration_test.go +++ b/pkg/workflow/branch_normalize_integration_test.go @@ -27,7 +27,7 @@ func TestBranchNormalizationInlinedInMainJob(t *testing.T) { } // Build the main job - job, err := compiler.buildMainJob(data, false, "") + job, err := compiler.buildMainJob(data, false, data.If) if err != nil { t.Fatalf("Failed to build main job: %v", err) } @@ -64,7 +64,7 @@ func TestBranchNormalizationStepNotAddedWhenNoUploadAssets(t *testing.T) { } // Build the main job - job, err := compiler.buildMainJob(data, false, "") + job, err := compiler.buildMainJob(data, false, data.If) if err != nil { t.Fatalf("Failed to build main job: %v", err) } diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 6bdca1316ee..2c401f15076 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -42,19 +42,20 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { needsPermissionCheck := c.needsRoleCheck(data, frontmatter) hasStopTime := data.StopTime != "" - // Determine if we need to add workflow_run repository safety check + // Determine if we need to add workflow_run repository safety check to workflow-level if condition // Add the check if the agentic workflow declares a workflow_run trigger // This prevents cross-repository workflow_run attacks - var workflowRunRepoSafety string if c.hasWorkflowRunTrigger(frontmatter) { - workflowRunRepoSafety = c.buildWorkflowRunRepoSafetyCondition() + workflowRunRepoSafety := c.buildWorkflowRunRepoSafetyCondition() + // Add to workflow-level if condition (data.If) + data.If = c.combineJobIfConditions(data.If, workflowRunRepoSafety) } // Build pre-activation job if needed (combines membership checks, stop-time validation, and command position check) var preActivationJobCreated bool hasCommandTrigger := data.Command != "" if needsPermissionCheck || hasStopTime || hasCommandTrigger { - preActivationJob, err := c.buildPreActivationJob(data, needsPermissionCheck, workflowRunRepoSafety) + preActivationJob, err := c.buildPreActivationJob(data, needsPermissionCheck, data.If) if err != nil { return fmt.Errorf("failed to build %s job: %w", constants.PreActivationJobName, err) } @@ -69,7 +70,7 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { var activationJobCreated bool if c.isActivationJobNeeded() { - activationJob, err := c.buildActivationJob(data, preActivationJobCreated, workflowRunRepoSafety) + activationJob, err := c.buildActivationJob(data, preActivationJobCreated, data.If) if err != nil { return fmt.Errorf("failed to build activation job: %w", err) } @@ -80,7 +81,7 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { } // Build main workflow job - mainJob, err := c.buildMainJob(data, activationJobCreated, workflowRunRepoSafety) + mainJob, err := c.buildMainJob(data, activationJobCreated, data.If) if err != nil { return fmt.Errorf("failed to build main job: %w", err) } @@ -354,7 +355,7 @@ func (c *Compiler) buildSafeOutputsJobs(data *WorkflowData, jobName, markdownPat // buildPreActivationJob creates a unified pre-activation job that combines membership checks and stop-time validation // This job exposes a single "activated" output that indicates whether the workflow should proceed -func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionCheck bool, workflowRunRepoSafety string) (*Job, error) { +func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionCheck bool, workflowIf string) (*Job, error) { var steps []string var permissions string @@ -456,9 +457,8 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec "activated": activatedExpression, } - // Determine the job-level if condition - // Combine user's if condition with workflow_run repository safety check - jobIfCondition := c.combineJobIfConditions(data.If, workflowRunRepoSafety) + // Use the workflow-level if condition (includes workflow_run safety check if applicable) + jobIfCondition := workflowIf job := &Job{ Name: constants.PreActivationJobName, @@ -473,7 +473,7 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec } // buildActivationJob creates the preamble activation job that acts as a barrier for runtime conditions -func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreated bool, workflowRunRepoSafety string) (*Job, error) { +func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreated bool, workflowIf string) (*Job, error) { outputs := map[string]string{} var steps []string @@ -553,9 +553,9 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate BuildPropertyAccess(fmt.Sprintf("needs.%s.outputs.%s", constants.PreActivationJobName, constants.ActivatedOutput)), BuildStringLiteral("true"), ) - if data.If != "" { - // Strip ${{ }} wrapper from data.If before combining - unwrappedIf := stripExpressionWrapper(data.If) + if workflowIf != "" { + // Strip ${{ }} wrapper from workflowIf before combining + unwrappedIf := stripExpressionWrapper(workflowIf) ifExpr := &ExpressionNode{Expression: unwrappedIf} combinedExpr := buildAnd(activatedExpr, ifExpr) activationCondition = combinedExpr.Render() @@ -563,12 +563,11 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate activationCondition = activatedExpr.Render() } } else { - // No pre-activation check needed - activationCondition = data.If + // No pre-activation check needed, use workflow-level if condition + activationCondition = workflowIf } - // Combine activation condition with workflow_run repository safety check - activationCondition = c.combineJobIfConditions(activationCondition, workflowRunRepoSafety) + // Note: workflow-level if (data.If) now includes workflow_run repository safety check // Set permissions - add reaction permissions if reaction is configured and not "none" var permissions string @@ -602,19 +601,15 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate } // buildMainJob creates the main workflow job -func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool, workflowRunRepoSafety string) (*Job, error) { +func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool, workflowIf string) (*Job, error) { var steps []string - var jobCondition = data.If + var jobCondition = workflowIf if activationJobCreated { jobCondition = "" // Main job depends on activation job, so no need for inline condition } - // If there's no activation job, we need to add the workflow_run repository safety check - // to the main job directly - if !activationJobCreated { - jobCondition = c.combineJobIfConditions(jobCondition, workflowRunRepoSafety) - } + // Note: workflow-level if (data.If) now includes workflow_run repository safety check // Permission checks are now handled by the separate check_membership job // No role checks needed in the main job diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index 31bbffec25c..8e1897467dd 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -106,6 +106,9 @@ func (c *Compiler) generateYAML(data *WorkflowData, markdownPath string) (string yaml.WriteString(fmt.Sprintf("name: \"%s\"\n", data.Name)) yaml.WriteString(data.On + "\n\n") + // Note: GitHub Actions doesn't support workflow-level if conditions + // The workflow_run safety check is added to individual jobs instead + // Add permissions if present if data.Permissions != "" { yaml.WriteString(data.Permissions + "\n\n") diff --git a/pkg/workflow/main_job_env_test.go b/pkg/workflow/main_job_env_test.go index 6a0a3f69f45..1227a2ddf5f 100644 --- a/pkg/workflow/main_job_env_test.go +++ b/pkg/workflow/main_job_env_test.go @@ -70,7 +70,7 @@ func TestMainJobEnvironmentVariables(t *testing.T) { data.SafeOutputs = compiler.extractSafeOutputsConfig(tt.frontmatter) // Build the main job - job, err := compiler.buildMainJob(data, false, "") + job, err := compiler.buildMainJob(data, false, data.If) if err != nil { t.Fatalf("Failed to build main job: %v", err) } From 3033c6be0e812012a09261cd55388cc5aa2ce11a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 22:51:52 +0000 Subject: [PATCH 7/7] Apply workflow_run safety check exclusively to activation job Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../branch_normalize_integration_test.go | 4 +- pkg/workflow/compiler_jobs.go | 45 ++++++++++--------- pkg/workflow/main_job_env_test.go | 2 +- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/pkg/workflow/branch_normalize_integration_test.go b/pkg/workflow/branch_normalize_integration_test.go index 9df80e52e6b..54e3b51daa8 100644 --- a/pkg/workflow/branch_normalize_integration_test.go +++ b/pkg/workflow/branch_normalize_integration_test.go @@ -27,7 +27,7 @@ func TestBranchNormalizationInlinedInMainJob(t *testing.T) { } // Build the main job - job, err := compiler.buildMainJob(data, false, data.If) + job, err := compiler.buildMainJob(data, false) if err != nil { t.Fatalf("Failed to build main job: %v", err) } @@ -64,7 +64,7 @@ func TestBranchNormalizationStepNotAddedWhenNoUploadAssets(t *testing.T) { } // Build the main job - job, err := compiler.buildMainJob(data, false, data.If) + job, err := compiler.buildMainJob(data, false) if err != nil { t.Fatalf("Failed to build main job: %v", err) } diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 2c401f15076..170af7cb367 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -42,20 +42,19 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { needsPermissionCheck := c.needsRoleCheck(data, frontmatter) hasStopTime := data.StopTime != "" - // Determine if we need to add workflow_run repository safety check to workflow-level if condition + // Determine if we need to add workflow_run repository safety check // Add the check if the agentic workflow declares a workflow_run trigger // This prevents cross-repository workflow_run attacks + var workflowRunRepoSafety string if c.hasWorkflowRunTrigger(frontmatter) { - workflowRunRepoSafety := c.buildWorkflowRunRepoSafetyCondition() - // Add to workflow-level if condition (data.If) - data.If = c.combineJobIfConditions(data.If, workflowRunRepoSafety) + workflowRunRepoSafety = c.buildWorkflowRunRepoSafetyCondition() } // Build pre-activation job if needed (combines membership checks, stop-time validation, and command position check) var preActivationJobCreated bool hasCommandTrigger := data.Command != "" if needsPermissionCheck || hasStopTime || hasCommandTrigger { - preActivationJob, err := c.buildPreActivationJob(data, needsPermissionCheck, data.If) + preActivationJob, err := c.buildPreActivationJob(data, needsPermissionCheck) if err != nil { return fmt.Errorf("failed to build %s job: %w", constants.PreActivationJobName, err) } @@ -70,7 +69,7 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { var activationJobCreated bool if c.isActivationJobNeeded() { - activationJob, err := c.buildActivationJob(data, preActivationJobCreated, data.If) + activationJob, err := c.buildActivationJob(data, preActivationJobCreated, workflowRunRepoSafety) if err != nil { return fmt.Errorf("failed to build activation job: %w", err) } @@ -81,7 +80,7 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { } // Build main workflow job - mainJob, err := c.buildMainJob(data, activationJobCreated, data.If) + mainJob, err := c.buildMainJob(data, activationJobCreated) if err != nil { return fmt.Errorf("failed to build main job: %w", err) } @@ -355,7 +354,7 @@ func (c *Compiler) buildSafeOutputsJobs(data *WorkflowData, jobName, markdownPat // buildPreActivationJob creates a unified pre-activation job that combines membership checks and stop-time validation // This job exposes a single "activated" output that indicates whether the workflow should proceed -func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionCheck bool, workflowIf string) (*Job, error) { +func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionCheck bool) (*Job, error) { var steps []string var permissions string @@ -457,8 +456,9 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec "activated": activatedExpression, } - // Use the workflow-level if condition (includes workflow_run safety check if applicable) - jobIfCondition := workflowIf + // Pre-activation job uses the user's original if condition (data.If) + // The workflow_run safety check is NOT applied here - it's only on the activation job + jobIfCondition := data.If job := &Job{ Name: constants.PreActivationJobName, @@ -473,7 +473,8 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec } // buildActivationJob creates the preamble activation job that acts as a barrier for runtime conditions -func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreated bool, workflowIf string) (*Job, error) { +// The workflow_run repository safety check is applied exclusively to this job +func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreated bool, workflowRunRepoSafety string) (*Job, error) { outputs := map[string]string{} var steps []string @@ -553,9 +554,9 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate BuildPropertyAccess(fmt.Sprintf("needs.%s.outputs.%s", constants.PreActivationJobName, constants.ActivatedOutput)), BuildStringLiteral("true"), ) - if workflowIf != "" { - // Strip ${{ }} wrapper from workflowIf before combining - unwrappedIf := stripExpressionWrapper(workflowIf) + if data.If != "" { + // Strip ${{ }} wrapper from data.If before combining + unwrappedIf := stripExpressionWrapper(data.If) ifExpr := &ExpressionNode{Expression: unwrappedIf} combinedExpr := buildAnd(activatedExpr, ifExpr) activationCondition = combinedExpr.Render() @@ -563,11 +564,15 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate activationCondition = activatedExpr.Render() } } else { - // No pre-activation check needed, use workflow-level if condition - activationCondition = workflowIf + // No pre-activation check needed, use user's if condition + activationCondition = data.If } - // Note: workflow-level if (data.If) now includes workflow_run repository safety check + // Apply workflow_run repository safety check exclusively to activation job + // This check is combined with any existing activation condition + if workflowRunRepoSafety != "" { + activationCondition = c.combineJobIfConditions(activationCondition, workflowRunRepoSafety) + } // Set permissions - add reaction permissions if reaction is configured and not "none" var permissions string @@ -601,15 +606,15 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate } // buildMainJob creates the main workflow job -func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool, workflowIf string) (*Job, error) { +func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) (*Job, error) { var steps []string - var jobCondition = workflowIf + var jobCondition = data.If if activationJobCreated { jobCondition = "" // Main job depends on activation job, so no need for inline condition } - // Note: workflow-level if (data.If) now includes workflow_run repository safety check + // Note: workflow_run repository safety check is applied exclusively to activation job // Permission checks are now handled by the separate check_membership job // No role checks needed in the main job diff --git a/pkg/workflow/main_job_env_test.go b/pkg/workflow/main_job_env_test.go index 1227a2ddf5f..359421ec80b 100644 --- a/pkg/workflow/main_job_env_test.go +++ b/pkg/workflow/main_job_env_test.go @@ -70,7 +70,7 @@ func TestMainJobEnvironmentVariables(t *testing.T) { data.SafeOutputs = compiler.extractSafeOutputsConfig(tt.frontmatter) // Build the main job - job, err := compiler.buildMainJob(data, false, data.If) + job, err := compiler.buildMainJob(data, false) if err != nil { t.Fatalf("Failed to build main job: %v", err) }