diff --git a/.github/workflows/example-permissions-warning.md b/.github/workflows/example-permissions-warning.md new file mode 100644 index 0000000000..9075cbfa65 --- /dev/null +++ b/.github/workflows/example-permissions-warning.md @@ -0,0 +1,26 @@ +--- +on: + workflow_dispatch: +permissions: + contents: read + issues: read +tools: + github: + toolsets: [repos, issues, pull_requests] + read-only: false +--- + +# Example: Under-provisioned Permissions Warning + +This workflow demonstrates the new warning behavior for under-provisioned permissions. + +When compiled in non-strict mode (default), this workflow will produce a warning because: +- The `repos` toolset requires `contents: write` (but we only have `read`) +- The `issues` toolset requires `issues: write` (but we only have `read`) +- The `pull_requests` toolset requires `pull-requests: write` (but we don't have it at all) + +The warning will suggest two options: +1. Add the missing write permissions +2. Or reduce the toolsets to only those that work with read-only permissions + +In strict mode (with --strict flag), this would fail with an error instead. diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index cd91a81a50..cb53ec4e93 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -296,17 +296,32 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath message := FormatValidationMessage(validationResult, c.strictMode) if len(validationResult.MissingPermissions) > 0 { - // Missing permissions are always errors - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "error", - Message: message, - }) - return errors.New(formattedErr) + if c.strictMode { + // In strict mode, missing permissions are errors + formattedErr := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: markdownPath, + Line: 1, + Column: 1, + }, + Type: "error", + Message: message, + }) + return errors.New(formattedErr) + } else { + // In non-strict mode, missing permissions are warnings + formattedWarning := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: markdownPath, + Line: 1, + Column: 1, + }, + Type: "warning", + Message: message, + }) + fmt.Fprintln(os.Stderr, formattedWarning) + c.IncrementWarningCount() + } } } } diff --git a/pkg/workflow/permissions_validator.go b/pkg/workflow/permissions_validator.go index 000fd19d83..a3b9a2ccf8 100644 --- a/pkg/workflow/permissions_validator.go +++ b/pkg/workflow/permissions_validator.go @@ -261,7 +261,9 @@ func formatMissingPermissionsMessage(result *PermissionsValidationResult) string lines = append(lines, "Missing required permissions for github toolsets:") lines = append(lines, permLines...) lines = append(lines, "") - lines = append(lines, "Add to your workflow frontmatter:") + lines = append(lines, "To fix this, you can either:") + lines = append(lines, "") + lines = append(lines, "Option 1: Add missing permissions to your workflow frontmatter:") lines = append(lines, "permissions:") for _, scopeStr := range scopes { scope := PermissionScope(scopeStr) @@ -269,5 +271,27 @@ func formatMissingPermissionsMessage(result *PermissionsValidationResult) string lines = append(lines, fmt.Sprintf(" %s: %s", scope, level)) } + // Add suggestion to reduce toolsets if we have toolset details + if len(result.MissingToolsetDetails) > 0 { + lines = append(lines, "") + lines = append(lines, "Option 2: Reduce the required toolsets in your workflow:") + lines = append(lines, "Remove or adjust toolsets that require these permissions:") + + // Get unique toolsets from MissingToolsetDetails + toolsetsMap := make(map[string]bool) + for toolset := range result.MissingToolsetDetails { + toolsetsMap[toolset] = true + } + var toolsetsList []string + for toolset := range toolsetsMap { + toolsetsList = append(toolsetsList, toolset) + } + sort.Strings(toolsetsList) + + for _, toolset := range toolsetsList { + lines = append(lines, fmt.Sprintf(" - %s", toolset)) + } + } + return strings.Join(lines, "\n") } diff --git a/pkg/workflow/permissions_validator_test.go b/pkg/workflow/permissions_validator_test.go index 3fda680da0..723ed40fc2 100644 --- a/pkg/workflow/permissions_validator_test.go +++ b/pkg/workflow/permissions_validator_test.go @@ -275,7 +275,8 @@ func TestFormatValidationMessage(t *testing.T) { "Missing required permissions for github toolsets:", "contents: write (required by repos)", "issues: write (required by issues)", - "Add to your workflow frontmatter:", + "Option 1: Add missing permissions to your workflow frontmatter:", + "Option 2: Reduce the required toolsets in your workflow:", }, expectNotContains: []string{ "ERROR:", diff --git a/pkg/workflow/permissions_warning_test.go b/pkg/workflow/permissions_warning_test.go new file mode 100644 index 0000000000..0665a2c3a6 --- /dev/null +++ b/pkg/workflow/permissions_warning_test.go @@ -0,0 +1,242 @@ +package workflow + +import ( + "bytes" + "io" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestPermissionsWarningInNonStrictMode tests that under-provisioned permissions +// produce warnings in non-strict mode rather than errors +func TestPermissionsWarningInNonStrictMode(t *testing.T) { + tests := []struct { + name string + content string + strict bool + expectError bool + expectWarning bool + warningMessage string + }{ + { + name: "missing permissions in non-strict mode produces warning", + content: `--- +on: push +permissions: + contents: read +tools: + github: + toolsets: [repos, issues] + read-only: false +--- + +# Test Workflow +`, + strict: false, + expectError: false, + expectWarning: true, + warningMessage: "Missing required permissions for github toolsets:", + }, + { + name: "missing permissions in strict mode produces error", + content: `--- +on: push +permissions: + contents: read +engine: copilot +network: + allowed: + - "api.example.com" +tools: + github: + toolsets: [repos, issues] + read-only: false +--- + +# Test Workflow +`, + strict: true, + expectError: true, + expectWarning: false, + warningMessage: "", + }, + { + name: "sufficient permissions in non-strict mode produces no warning", + content: `--- +on: push +permissions: + contents: write + issues: write +tools: + github: + toolsets: [repos, issues] +--- + +# Test Workflow +`, + strict: false, + expectError: false, + expectWarning: false, + }, + { + name: "sufficient permissions in strict mode produces no error", + content: `--- +on: push +permissions: + contents: read + issues: read +engine: copilot +network: + allowed: + - "api.example.com" +tools: + github: + toolsets: [repos, issues] + read-only: true +--- + +# Test Workflow +`, + strict: true, + expectError: false, + expectWarning: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "permissions-warning-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + testFile := filepath.Join(tmpDir, "test-workflow.md") + if err := os.WriteFile(testFile, []byte(tt.content), 0644); err != nil { + t.Fatal(err) + } + + // Capture stderr to check for warnings + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + compiler := NewCompiler(false, "", "test") + compiler.SetStrictMode(tt.strict) + err = compiler.CompileWorkflow(testFile) + + // Restore stderr + w.Close() + os.Stderr = oldStderr + var buf bytes.Buffer + io.Copy(&buf, r) + stderrOutput := buf.String() + + // Check error expectation + if tt.expectError && err == nil { + t.Error("Expected compilation to fail but it succeeded") + } else if !tt.expectError && err != nil { + t.Errorf("Expected compilation to succeed but it failed: %v", err) + } + + // Check warning expectation + if tt.expectWarning { + if !strings.Contains(stderrOutput, tt.warningMessage) { + t.Errorf("Expected warning containing '%s', got stderr:\n%s", tt.warningMessage, stderrOutput) + } + if !strings.Contains(stderrOutput, "warning:") { + t.Errorf("Expected 'warning:' in stderr output, got:\n%s", stderrOutput) + } + // Check for the new suggestion format + if !strings.Contains(stderrOutput, "Option 1: Add missing permissions") { + t.Errorf("Expected 'Option 1: Add missing permissions' in warning, got:\n%s", stderrOutput) + } + if !strings.Contains(stderrOutput, "Option 2: Reduce the required toolsets") { + t.Errorf("Expected 'Option 2: Reduce the required toolsets' in warning, got:\n%s", stderrOutput) + } + } else { + // For non-warning cases, we should not see the warning message content + if tt.warningMessage != "" && strings.Contains(stderrOutput, tt.warningMessage) { + t.Errorf("Unexpected warning in stderr output:\n%s", stderrOutput) + } + } + + // Verify warning count + if tt.expectWarning { + warningCount := compiler.GetWarningCount() + if warningCount == 0 { + t.Error("Expected warning count > 0 but got 0") + } + } + }) + } +} + +// TestPermissionsWarningMessageFormat tests that the warning message format +// includes both options for fixing the issue +func TestPermissionsWarningMessageFormat(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "permissions-warning-format-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + content := `--- +on: push +permissions: + contents: read +tools: + github: + toolsets: [repos, issues, pull_requests] + read-only: false +--- + +# Test Workflow +` + + testFile := filepath.Join(tmpDir, "test-workflow.md") + if err := os.WriteFile(testFile, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + // Capture stderr to check for warnings + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + compiler := NewCompiler(false, "", "test") + compiler.SetStrictMode(false) + err = compiler.CompileWorkflow(testFile) + + // Restore stderr + w.Close() + os.Stderr = oldStderr + var buf bytes.Buffer + io.Copy(&buf, r) + stderrOutput := buf.String() + + if err != nil { + t.Fatalf("Expected compilation to succeed but it failed: %v", err) + } + + // Check that the warning includes both options + expectedPhrases := []string{ + "Missing required permissions for github toolsets:", + "Option 1: Add missing permissions to your workflow frontmatter:", + "Option 2: Reduce the required toolsets in your workflow:", + "issues", + "pull_requests", + "repos", + "contents: write", + "issues: write", + "pull-requests: write", + } + + for _, phrase := range expectedPhrases { + if !strings.Contains(stderrOutput, phrase) { + t.Errorf("Expected warning to contain '%s', got:\n%s", phrase, stderrOutput) + } + } +}