diff --git a/.github/workflows/ci-doctor.lock.yml b/.github/workflows/ci-doctor.lock.yml index 603448e27b..9658f4da21 100644 --- a/.github/workflows/ci-doctor.lock.yml +++ b/.github/workflows/ci-doctor.lock.yml @@ -61,7 +61,8 @@ run-name: "CI Failure Doctor" jobs: activation: - if: ${{ github.event.workflow_run.conclusion == 'failure' }} + 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 54b2429a37..07c3038e6a 100644 --- a/.github/workflows/dev-hawk.lock.yml +++ b/.github/workflows/dev-hawk.lock.yml @@ -53,7 +53,9 @@ run-name: "Dev Hawk" jobs: activation: - if: ${{ github.event.workflow_run.event == 'workflow_dispatch' }} + if: > + (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 e1cd2f6df6..756aa7b8b1 100644 --- a/.github/workflows/smoke-detector.lock.yml +++ b/.github/workflows/smoke-detector.lock.yml @@ -74,7 +74,8 @@ 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_name != 'workflow_run') || (github.event.workflow_run.repository.id == github.repository_id)) runs-on: ubuntu-slim permissions: discussions: write diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index d95352339b..170af7cb36 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -42,6 +42,14 @@ 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 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() + } + // Build pre-activation job if needed (combines membership checks, stop-time validation, and command position check) var preActivationJobCreated bool hasCommandTrigger := data.Command != "" @@ -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) } @@ -448,9 +456,13 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec "activated": activatedExpression, } + // 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, - 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,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) (*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 @@ -551,10 +564,16 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate activationCondition = activatedExpr.Render() } } else { - // No pre-activation check needed + // No pre-activation check needed, use user's if condition activationCondition = data.If } + // 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 if data.AIReaction != "" && data.AIReaction != "none" { @@ -594,6 +613,9 @@ func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) ( if activationJobCreated { jobCondition = "" // Main job depends on activation job, so no need for inline condition } + + // 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/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index 31bbffec25..8e1897467d 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/role_checks.go b/pkg/workflow/role_checks.go index de6773458a..8eca1fb35f 100644 --- a/pkg/workflow/role_checks.go +++ b/pkg/workflow/role_checks.go @@ -179,3 +179,73 @@ func (c *Compiler) hasSafeEventsOnly(data *WorkflowData, frontmatter map[string] // For command workflows, they are not considered "safe only" return false } + +// 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 + } + + // 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 +// 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 NOT workflow_run + eventNotWorkflowRun := BuildNotEquals( + 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 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()) +} + +// 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 cbe5e81531..182e26fab7 100644 --- a/pkg/workflow/task_job_if_condition_test.go +++ b/pkg/workflow/task_job_if_condition_test.go @@ -61,9 +61,19 @@ 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 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 + 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 0000000000..9bfa225c18 --- /dev/null +++ b/pkg/workflow/workflow_run_repo_safety_test.go @@ -0,0 +1,283 @@ +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_name != 'workflow_run'", + }, + { + 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_name != 'workflow_run'", + }, + { + 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 + // 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 != "" { + // 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 { + 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 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) + } + + // 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) + } + + // Verify OR operator is used in the safety condition + if !strings.Contains(lockContentStr, "||") { + t.Error("Expected safety condition to use || operator") + } +} + +// 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) + } +}