Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/example-permissions-warning.md
Original file line number Diff line number Diff line change
@@ -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.
37 changes: 26 additions & 11 deletions pkg/workflow/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}
}
}
Expand Down
26 changes: 25 additions & 1 deletion pkg/workflow/permissions_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,13 +261,37 @@ 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)
level := result.MissingPermissions[scope]
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")
}
3 changes: 2 additions & 1 deletion pkg/workflow/permissions_validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:",
Expand Down
242 changes: 242 additions & 0 deletions pkg/workflow/permissions_warning_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
Loading