diff --git a/.github/workflows/smoke-call-workflow.lock.yml b/.github/workflows/smoke-call-workflow.lock.yml index 18136fb4320..4fb3e87b2ca 100644 --- a/.github/workflows/smoke-call-workflow.lock.yml +++ b/.github/workflows/smoke-call-workflow.lock.yml @@ -1113,10 +1113,8 @@ jobs: needs: safe_outputs if: needs.safe_outputs.outputs.call_workflow_name == 'smoke-workflow-call' permissions: - actions: read contents: read - issues: write - pull-requests: write + pull-requests: read uses: ./.github/workflows/smoke-workflow-call.lock.yml with: aw_context: ${{ fromJSON(needs.safe_outputs.outputs.call_workflow_payload).aw_context }} diff --git a/pkg/cli/workflows/test-copilot-call-workflow.md b/pkg/cli/workflows/test-copilot-call-workflow.md index cc557c11e85..c5aa9c4eec7 100644 --- a/pkg/cli/workflows/test-copilot-call-workflow.md +++ b/pkg/cli/workflows/test-copilot-call-workflow.md @@ -4,6 +4,8 @@ on: permissions: contents: read actions: read + issues: read + pull-requests: read engine: copilot safe-outputs: call-workflow: diff --git a/pkg/workflow/call_workflow_permissions.go b/pkg/workflow/call_workflow_permissions.go index 704d68e9c8c..cf851a1197b 100644 --- a/pkg/workflow/call_workflow_permissions.go +++ b/pkg/workflow/call_workflow_permissions.go @@ -3,6 +3,7 @@ package workflow import ( "fmt" "os" + "sort" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" @@ -10,6 +11,58 @@ import ( var callWorkflowPermissionsLog = logger.New("workflow:call_workflow_permissions") +// permissionLevelRank maps a permission level to a comparable rank where a higher +// number grants strictly more access (none < read < write). Used to determine +// whether one permission set covers another. Unknown or empty levels rank as 0. +func permissionLevelRank(level PermissionLevel) int { + switch level { + case PermissionWrite: + return 2 + case PermissionRead: + return 1 + default: // PermissionNone or empty + return 0 + } +} + +// findUncoveredWorkerPermissions returns the worker permission scopes (formatted as +// "scope: level") that the caller's declared permissions do not cover. A scope is +// uncovered when the caller grants a strictly lower level than the worker requires. +// The result is sorted for deterministic output; an empty result means the caller's +// declared permissions are sufficient for the worker. +// +// This is used to validate (not modify) the caller's permission envelope: callers +// control their own permission surface, and the compiler only warns when the declared +// permissions are insufficient for a worker the caller invokes. +func findUncoveredWorkerPermissions(caller, worker *Permissions) []string { + if worker == nil { + return nil + } + + scopes := append(GetAllPermissionScopes(), PermissionCopilotRequests) + var missing []string + for _, scope := range scopes { + workerLevel, workerWants := worker.Get(scope) + if !workerWants || workerLevel == PermissionNone { + continue + } + + callerLevel := PermissionNone + if caller != nil { + if level, has := caller.Get(scope); has { + callerLevel = level + } + } + + if permissionLevelRank(callerLevel) < permissionLevelRank(workerLevel) { + missing = append(missing, fmt.Sprintf("%s: %s", scope, workerLevel)) + } + } + + sort.Strings(missing) + return missing +} + // extractJobPermissionsFromParsedWorkflow extracts and merges all job-level permissions // from a parsed GitHub Actions workflow map. Returns the union of all jobs' permissions. func extractJobPermissionsFromParsedWorkflow(workflow map[string]any) *Permissions { @@ -54,8 +107,12 @@ func extractJobPermissionsFromParsedWorkflow(workflow map[string]any) *Permissio // permissions field is used as a proxy (the compiler will turn it into per-job // permissions when the worker is eventually compiled). // +// This is used purely to VALIDATE the caller's declared permissions against what the +// worker requires (see findUncoveredWorkerPermissions). The worker's permissions are +// never written into the caller's lockfile; the caller controls its own permission +// surface and the compiler only warns when it is insufficient. +// // Returns nil when no workflow file is found or no permissions are declared. -// The caller should omit the permissions block on the call-* job in that case. func extractCallWorkflowPermissions(workflowName, markdownPath string) (*Permissions, error) { fileResult, err := findWorkflowFile(workflowName, markdownPath) if err != nil { diff --git a/pkg/workflow/call_workflow_permissions_test.go b/pkg/workflow/call_workflow_permissions_test.go index c0f2d7411d3..5716630706e 100644 --- a/pkg/workflow/call_workflow_permissions_test.go +++ b/pkg/workflow/call_workflow_permissions_test.go @@ -93,6 +93,55 @@ func TestExtractJobPermissionsFromParsedWorkflow_NoPermissionsOnJobs(t *testing. assert.Empty(t, perms.RenderToYAML(), "Should return empty when jobs have no permissions") } +// TestFindUncoveredWorkerPermissions verifies that the caller's declared permissions are +// validated against the worker's required permissions without modifying either set. +func TestFindUncoveredWorkerPermissions(t *testing.T) { + parse := func(yaml string) *Permissions { + return NewPermissionsParser(yaml).ToPermissions() + } + + t.Run("caller covers worker", func(t *testing.T) { + caller := parse("permissions:\n contents: write\n issues: write") + worker := parse("permissions:\n contents: read\n issues: write") + assert.Empty(t, findUncoveredWorkerPermissions(caller, worker), + "caller granting >= worker's required levels should have no gaps") + }) + + t.Run("caller missing a scope", func(t *testing.T) { + caller := parse("permissions:\n contents: read") + worker := parse("permissions:\n contents: read\n issues: write") + assert.Equal(t, []string{"issues: write"}, findUncoveredWorkerPermissions(caller, worker), + "a scope the caller does not grant at all should be reported") + }) + + t.Run("caller grants lower level", func(t *testing.T) { + caller := parse("permissions:\n contents: read") + worker := parse("permissions:\n contents: write") + assert.Equal(t, []string{"contents: write"}, findUncoveredWorkerPermissions(caller, worker), + "read does not cover a required write") + }) + + t.Run("nil caller reports all worker scopes", func(t *testing.T) { + worker := parse("permissions:\n contents: write\n issues: read") + got := findUncoveredWorkerPermissions(nil, worker) + assert.Equal(t, []string{"contents: write", "issues: read"}, got, + "a nil caller covers nothing and results are sorted") + }) + + t.Run("nil worker has no gaps", func(t *testing.T) { + caller := parse("permissions:\n contents: read") + assert.Nil(t, findUncoveredWorkerPermissions(caller, nil), + "no worker requirements means nothing is uncovered") + }) + + t.Run("worker none-level scopes are ignored", func(t *testing.T) { + caller := parse("permissions: {}") + worker := parse("permissions:\n contents: none") + assert.Empty(t, findUncoveredWorkerPermissions(caller, worker), + "a worker scope explicitly set to none requires nothing") + }) +} + // TestExtractCallWorkflowPermissions_FromLockYML tests extracting permissions from a .lock.yml file func TestExtractCallWorkflowPermissions_FromLockYML(t *testing.T) { tmpDir := t.TempDir() @@ -220,7 +269,7 @@ func TestExtractCallWorkflowPermissions_FileNotFound(t *testing.T) { } // TestBuildCallWorkflowJobs_SetsPermissionsFromLockYML tests that call-workflow jobs -// include permissions extracted from the worker's .lock.yml file +// carry the CALLER's declared permissions, independent of the worker's .lock.yml. func TestBuildCallWorkflowJobs_SetsPermissionsFromLockYML(t *testing.T) { compiler := NewCompiler(WithVersion("1.0.0")) @@ -255,6 +304,8 @@ jobs: markdownPath := filepath.Join(workflowsDir, "gateway.md") workflowData := &WorkflowData{ + // Caller declares its own envelope; the call-* job uses exactly this. + Permissions: "permissions:\n contents: write\n issues: write\n pull-requests: write", SafeOutputs: &SafeOutputsConfig{ CallWorkflow: &CallWorkflowConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{Max: new("1")}, @@ -272,14 +323,14 @@ jobs: job, exists := compiler.jobManager.GetJob("call-worker-docs") require.True(t, exists, "Job should exist in job manager") - assert.NotEmpty(t, job.Permissions, "Job should have permissions set") - assert.Contains(t, job.Permissions, "contents: write", "Permissions should include contents: write") - assert.Contains(t, job.Permissions, "issues: write", "Permissions should include issues: write") - assert.Contains(t, job.Permissions, "pull-requests: write", "Permissions should include pull-requests: write") + assert.NotEmpty(t, job.Permissions, "Job should have caller's permissions set") + assert.Contains(t, job.Permissions, "contents: write", "Permissions should include caller's contents: write") + assert.Contains(t, job.Permissions, "issues: write", "Permissions should include caller's issues: write") + assert.Contains(t, job.Permissions, "pull-requests: write", "Permissions should include caller's pull-requests: write") } -// TestBuildCallWorkflowJobs_SetsPermissionsFromMD tests that call-workflow jobs -// include permissions from .md frontmatter for same-batch compilation targets +// TestBuildCallWorkflowJobs_SetsPermissionsFromMD tests that call-workflow jobs carry the +// CALLER's declared permissions even when the worker is a same-batch .md compilation target. func TestBuildCallWorkflowJobs_SetsPermissionsFromMD(t *testing.T) { compiler := NewCompiler(WithVersion("1.0.0")) @@ -304,6 +355,8 @@ permissions: markdownPath := filepath.Join(workflowsDir, "gateway.md") workflowData := &WorkflowData{ + // Caller declares its own envelope; the call-* job uses exactly this. + Permissions: "permissions:\n contents: read\n issues: write", SafeOutputs: &SafeOutputsConfig{ CallWorkflow: &CallWorkflowConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{Max: new("1")}, @@ -321,9 +374,9 @@ permissions: job, exists := compiler.jobManager.GetJob("call-worker-e") require.True(t, exists, "Job should exist in job manager") - assert.NotEmpty(t, job.Permissions, "Job should have permissions from .md frontmatter") - assert.Contains(t, job.Permissions, "contents: read", "Permissions should include contents: read") - assert.Contains(t, job.Permissions, "issues: write", "Permissions should include issues: write") + assert.NotEmpty(t, job.Permissions, "Job should have caller's permissions") + assert.Contains(t, job.Permissions, "contents: read", "Permissions should include caller's contents: read") + assert.Contains(t, job.Permissions, "issues: write", "Permissions should include caller's issues: write") } // TestBuildCallWorkflowJobs_NoPermissionsWhenWorkerHasNone tests that call-workflow @@ -370,8 +423,10 @@ jobs: assert.Empty(t, job.Permissions, "Job should have no permissions when worker has none") } -// TestCallWorkflowJobYAMLOutput_WithPermissions tests the YAML output of a call-workflow -// job includes the permissions block derived from the worker's .lock.yml +// TestCallWorkflowJobYAMLOutput_WithPermissions tests that the YAML output of a +// call-workflow job includes the permissions block derived from the CALLER's own +// declared permissions (not the worker's). The worker's permissions are only used +// for validation, never written into the caller's lockfile. func TestCallWorkflowJobYAMLOutput_WithPermissions(t *testing.T) { compiler := NewCompiler(WithVersion("1.0.0")) @@ -379,14 +434,16 @@ func TestCallWorkflowJobYAMLOutput_WithPermissions(t *testing.T) { workflowsDir := filepath.Join(tmpDir, ".github", "workflows") require.NoError(t, os.MkdirAll(workflowsDir, 0755), "Failed to create workflows directory") + // Worker requires contents: write and issues: write. The caller declares a + // broader envelope below; the call-* job must reflect the CALLER's permissions. workerContent := `name: Worker on: workflow_call: {} jobs: agent: permissions: - contents: read - issues: read + contents: write + issues: write runs-on: ubuntu-latest steps: - run: echo "agent" @@ -396,6 +453,8 @@ jobs: markdownPath := filepath.Join(workflowsDir, "gateway.md") workflowData := &WorkflowData{ + // Caller's declared permissions — these are what the call-* job must use. + Permissions: "permissions:\n contents: write\n issues: write\n pull-requests: write", SafeOutputs: &SafeOutputsConfig{ CallWorkflow: &CallWorkflowConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, @@ -417,8 +476,10 @@ jobs: assert.Contains(t, yamlOutput, "uses: ./.github/workflows/worker-a.lock.yml", "Should contain uses directive") assert.Contains(t, yamlOutput, "secrets: inherit", "Should inherit secrets") assert.Contains(t, yamlOutput, "permissions:", "Should include permissions block") - assert.Contains(t, yamlOutput, "contents: read", "Should include contents: read") - assert.Contains(t, yamlOutput, "issues: read", "Should include issues: read") + // The call-* job uses the CALLER's declared permissions. + assert.Contains(t, yamlOutput, "contents: write", "Should include caller's contents: write") + assert.Contains(t, yamlOutput, "issues: write", "Should include caller's issues: write") + assert.Contains(t, yamlOutput, "pull-requests: write", "Should include caller's pull-requests: write") // Verify permissions appear before uses in the YAML (job-level ordering) permIdx := strings.Index(yamlOutput, "permissions:") @@ -474,7 +535,7 @@ jobs: } // TestCallWorkflowPermissions_EndToEnd tests full gateway compilation with permissioned workers — -// the generated lock file must include job-level permissions blocks on every call-* job. +// every call-* job must carry the CALLER's declared permissions, never the worker's. func TestCallWorkflowPermissions_EndToEnd(t *testing.T) { compiler := NewCompiler(WithVersion("1.0.0")) @@ -589,17 +650,26 @@ Analyse the issue and determine which worker to run. } callASection := yamlOutput[callAStart:callAEnd] assert.Contains(t, callASection, "permissions:", "call-worker-a job must have permissions block") - // Worker A has contents: write (from safe_outputs job — write wins over read) - assert.Contains(t, callASection, "contents: write", "call-worker-a permissions should include contents: write") - - // Extract the YAML section for call-worker-b (union from its single job) + // The call-* job carries the CALLER's declared permissions (contents: read), NOT + // the worker's (which would otherwise be contents: write). The worker's broader + // requirements are surfaced as a compiler warning, not written into the lockfile. + assert.Contains(t, callASection, "contents: read", "call-worker-a permissions should be the caller's contents: read") + assert.NotContains(t, callASection, "contents: write", "call-worker-a must NOT inherit the worker's contents: write") + + // Extract the YAML section for call-worker-b (bounded to just this job, since later + // framework jobs such as conclusion legitimately carry issues: write). callBSection := yamlOutput[callBStart:] + if convIdx := strings.Index(callBSection, "\n conclusion:"); convIdx != -1 { + callBSection = callBSection[:convIdx] + } assert.Contains(t, callBSection, "permissions:", "call-worker-b job must have permissions block") - assert.Contains(t, callBSection, "issues: write", "call-worker-b permissions should include issues: write") + assert.Contains(t, callBSection, "contents: read", "call-worker-b permissions should be the caller's contents: read") + assert.NotContains(t, callBSection, "issues: write", "call-worker-b must NOT inherit the worker's issues: write") } -// TestCallWorkflowPermissions_EndToEnd_YMLWorker tests that a worker referenced via a .yml -// file (not .lock.yml) also gets its permissions propagated in the generated call-* job. +// TestCallWorkflowPermissions_EndToEnd_YMLWorker tests that when a worker is referenced via a +// .yml file (not .lock.yml), the generated call-* job still carries the CALLER's declared +// permissions (the worker's permissions are used only for validation/warnings). func TestCallWorkflowPermissions_EndToEnd_YMLWorker(t *testing.T) { compiler := NewCompiler(WithVersion("1.0.0")) @@ -660,7 +730,12 @@ Pick the right worker. require.NotEqual(t, -1, callStart, "call-worker-plain: must appear in generated YAML") callSection := yamlOutput[callStart:] + if convIdx := strings.Index(callSection, "\n conclusion:"); convIdx != -1 { + callSection = callSection[:convIdx] + } assert.Contains(t, callSection, "permissions:", "call-worker-plain job must have permissions block") - assert.Contains(t, callSection, "contents: read", "Permissions should include contents: read") - assert.Contains(t, callSection, "pull-requests: write", "Permissions should include pull-requests: write") + // The call-* job carries the CALLER's declared permissions (contents: read), NOT the + // worker's pull-requests: write. The worker's extra requirement is reported as a warning. + assert.Contains(t, callSection, "contents: read", "Permissions should be the caller's contents: read") + assert.NotContains(t, callSection, "pull-requests: write", "call-worker-plain must NOT inherit the worker's pull-requests: write") } diff --git a/pkg/workflow/compiler_safe_output_jobs.go b/pkg/workflow/compiler_safe_output_jobs.go index c74aa11e4d3..34842056ddb 100644 --- a/pkg/workflow/compiler_safe_output_jobs.go +++ b/pkg/workflow/compiler_safe_output_jobs.go @@ -2,6 +2,8 @@ package workflow import ( "fmt" + "os" + "strings" "github.com/github/gh-aw/pkg/logger" ) @@ -168,8 +170,10 @@ func (c *Compiler) buildSafeOutputsJobs(data *WorkflowData, jobName, markdownPat // - `payload` is forwarded as the raw transport only when the worker declares it // (GitHub Actions rejects undeclared inputs) // - inherits all caller secrets via `secrets: inherit` -// - includes a job-level `permissions:` block that is the union of all the -// worker's job-level permissions, so GitHub allows the nested jobs to run +// - includes a job-level `permissions:` block derived from the CALLER's own +// declared permissions (not the worker's). The caller controls its own +// permission surface; the compiler validates that the declared permissions +// cover what the worker requires and warns if they do not. // // Returns the names of all generated jobs so they can be added to the conclusion // job's `needs` list. @@ -287,24 +291,43 @@ func (c *Compiler) buildCallWorkflowJobs(data *WorkflowData, markdownPath string callJob.SecretsInherit = true } - // Compute the permission superset required by the worker's jobs and - // attach it to the caller job. GitHub validates reusable workflow calls - // against the caller job's declared permission envelope; without a - // permissions block the nested jobs are constrained to `none`. + // Derive the call- job's permission envelope from the CALLER's own + // declared permissions, not from the worker. GitHub validates reusable + // workflow calls against the caller job's declared permissions, so the caller + // must declare a scope that is sufficient for the worker. We never inflate the + // caller's permissions to match the worker (doing so would, for example, + // materialise speculative scopes like vulnerability-alerts that GitHub rejects). + // Instead the caller controls its own surface and we validate it below. + callerPerms := data.CachedPermissions + if callerPerms == nil { + callerPerms = NewPermissionsParser(data.Permissions).ToPermissions() + } + if callerPerms != nil { + rendered := callerPerms.RenderToYAML() + if rendered != "" { + callJob.Permissions = rendered + compilerSafeOutputJobsLog.Printf("Set permissions on call-workflow job '%s' from caller's declared permissions: %s", jobName, rendered) + } + } + + // Validate (without modifying) that the caller's declared permissions cover what + // the worker requires. Emit a warning when they do not, so the user can widen the + // caller's `permissions:` block. This never alters the compiled permissions. if markdownPath != "" { - perms, permErr := extractCallWorkflowPermissions(workflowName, markdownPath) + workerPerms, permErr := extractCallWorkflowPermissions(workflowName, markdownPath) if permErr != nil { - // Non-fatal: log and continue without permissions rather than aborting compilation. - // The call-* job will be created without a permissions block; this may cause - // GitHub to reject nested worker jobs that require non-none permissions. - compilerSafeOutputJobsLog.Printf("Warning: could not extract permissions for call-workflow job '%s': %v. "+ - "Ensure the target workflow file exists and contains valid YAML. "+ - "The job will be created without a permissions block.", jobName, permErr) - } else if perms != nil { - rendered := perms.RenderToYAML() - if rendered != "" { - callJob.Permissions = rendered - compilerSafeOutputJobsLog.Printf("Set permissions on call-workflow job '%s': %s", jobName, rendered) + // Non-fatal: log and continue. The worker file may not exist yet (it may be + // compiled in the same batch), in which case validation is simply skipped. + compilerSafeOutputJobsLog.Printf("Could not extract worker permissions for call-workflow job '%s' (validation skipped): %v", jobName, permErr) + } else if workerPerms != nil { + if missing := findUncoveredWorkerPermissions(callerPerms, workerPerms); len(missing) > 0 { + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", + fmt.Sprintf("call-workflow target '%s' may require permissions not granted by this workflow: %s.\n"+ + "GitHub Actions constrains a called workflow's GITHUB_TOKEN to the caller job's permissions, "+ + "so the worker's jobs may fail. Add the missing scope(s) to this workflow's `permissions:` block.", + workflowName, strings.Join(missing, ", ")))) + c.IncrementWarningCount() + compilerSafeOutputJobsLog.Printf("Caller permissions insufficient for worker '%s': missing %s", workflowName, strings.Join(missing, ", ")) } } }