Skip to content

Commit 9bf9ee9

Browse files
CopilotCopilot
authored
Treat under-provisioned permissions as warnings in non-strict mode (#2843)
1 parent cf9598e commit 9bf9ee9

File tree

5 files changed

+321
-13
lines changed

5 files changed

+321
-13
lines changed

‎.github/workflows/example-permissions-warning.md‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
on:
3+
workflow_dispatch:
4+
permissions:
5+
contents: read
6+
issues: read
7+
tools:
8+
github:
9+
toolsets: [repos, issues, pull_requests]
10+
read-only: false
11+
---
12+
13+
# Example: Under-provisioned Permissions Warning
14+
15+
This workflow demonstrates the new warning behavior for under-provisioned permissions.
16+
17+
When compiled in non-strict mode (default), this workflow will produce a warning because:
18+
- The `repos` toolset requires `contents: write` (but we only have `read`)
19+
- The `issues` toolset requires `issues: write` (but we only have `read`)
20+
- The `pull_requests` toolset requires `pull-requests: write` (but we don't have it at all)
21+
22+
The warning will suggest two options:
23+
1. Add the missing write permissions
24+
2. Or reduce the toolsets to only those that work with read-only permissions
25+
26+
In strict mode (with --strict flag), this would fail with an error instead.

‎pkg/workflow/compiler.go‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -297,17 +297,32 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath
297297
message := FormatValidationMessage(validationResult, c.strictMode)
298298

299299
if len(validationResult.MissingPermissions) > 0 {
300-
// Missing permissions are always errors
301-
formattedErr := console.FormatError(console.CompilerError{
302-
Position: console.ErrorPosition{
303-
File: markdownPath,
304-
Line: 1,
305-
Column: 1,
306-
},
307-
Type: "error",
308-
Message: message,
309-
})
310-
return errors.New(formattedErr)
300+
if c.strictMode {
301+
// In strict mode, missing permissions are errors
302+
formattedErr := console.FormatError(console.CompilerError{
303+
Position: console.ErrorPosition{
304+
File: markdownPath,
305+
Line: 1,
306+
Column: 1,
307+
},
308+
Type: "error",
309+
Message: message,
310+
})
311+
return errors.New(formattedErr)
312+
} else {
313+
// In non-strict mode, missing permissions are warnings
314+
formattedWarning := console.FormatError(console.CompilerError{
315+
Position: console.ErrorPosition{
316+
File: markdownPath,
317+
Line: 1,
318+
Column: 1,
319+
},
320+
Type: "warning",
321+
Message: message,
322+
})
323+
fmt.Fprintln(os.Stderr, formattedWarning)
324+
c.IncrementWarningCount()
325+
}
311326
}
312327
}
313328
}

‎pkg/workflow/permissions_validator.go‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,13 +261,37 @@ func formatMissingPermissionsMessage(result *PermissionsValidationResult) string
261261
lines = append(lines, "Missing required permissions for github toolsets:")
262262
lines = append(lines, permLines...)
263263
lines = append(lines, "")
264-
lines = append(lines, "Add to your workflow frontmatter:")
264+
lines = append(lines, "To fix this, you can either:")
265+
lines = append(lines, "")
266+
lines = append(lines, "Option 1: Add missing permissions to your workflow frontmatter:")
265267
lines = append(lines, "permissions:")
266268
for _, scopeStr := range scopes {
267269
scope := PermissionScope(scopeStr)
268270
level := result.MissingPermissions[scope]
269271
lines = append(lines, fmt.Sprintf(" %s: %s", scope, level))
270272
}
271273

274+
// Add suggestion to reduce toolsets if we have toolset details
275+
if len(result.MissingToolsetDetails) > 0 {
276+
lines = append(lines, "")
277+
lines = append(lines, "Option 2: Reduce the required toolsets in your workflow:")
278+
lines = append(lines, "Remove or adjust toolsets that require these permissions:")
279+
280+
// Get unique toolsets from MissingToolsetDetails
281+
toolsetsMap := make(map[string]bool)
282+
for toolset := range result.MissingToolsetDetails {
283+
toolsetsMap[toolset] = true
284+
}
285+
var toolsetsList []string
286+
for toolset := range toolsetsMap {
287+
toolsetsList = append(toolsetsList, toolset)
288+
}
289+
sort.Strings(toolsetsList)
290+
291+
for _, toolset := range toolsetsList {
292+
lines = append(lines, fmt.Sprintf(" - %s", toolset))
293+
}
294+
}
295+
272296
return strings.Join(lines, "\n")
273297
}

‎pkg/workflow/permissions_validator_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,8 @@ func TestFormatValidationMessage(t *testing.T) {
275275
"Missing required permissions for github toolsets:",
276276
"contents: write (required by repos)",
277277
"issues: write (required by issues)",
278-
"Add to your workflow frontmatter:",
278+
"Option 1: Add missing permissions to your workflow frontmatter:",
279+
"Option 2: Reduce the required toolsets in your workflow:",
279280
},
280281
expectNotContains: []string{
281282
"ERROR:",

‎pkg/workflow/permissions_warning_test.go‎

Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
package workflow
2+
3+
import (
4+
"bytes"
5+
"io"
6+
"os"
7+
"path/filepath"
8+
"strings"
9+
"testing"
10+
)
11+
12+
// TestPermissionsWarningInNonStrictMode tests that under-provisioned permissions
13+
// produce warnings in non-strict mode rather than errors
14+
func TestPermissionsWarningInNonStrictMode(t *testing.T) {
15+
tests := []struct {
16+
name string
17+
content string
18+
strict bool
19+
expectError bool
20+
expectWarning bool
21+
warningMessage string
22+
}{
23+
{
24+
name: "missing permissions in non-strict mode produces warning",
25+
content: `---
26+
on: push
27+
permissions:
28+
contents: read
29+
tools:
30+
github:
31+
toolsets: [repos, issues]
32+
read-only: false
33+
---
34+
35+
# Test Workflow
36+
`,
37+
strict: false,
38+
expectError: false,
39+
expectWarning: true,
40+
warningMessage: "Missing required permissions for github toolsets:",
41+
},
42+
{
43+
name: "missing permissions in strict mode produces error",
44+
content: `---
45+
on: push
46+
permissions:
47+
contents: read
48+
engine: copilot
49+
network:
50+
allowed:
51+
- "api.example.com"
52+
tools:
53+
github:
54+
toolsets: [repos, issues]
55+
read-only: false
56+
---
57+
58+
# Test Workflow
59+
`,
60+
strict: true,
61+
expectError: true,
62+
expectWarning: false,
63+
warningMessage: "",
64+
},
65+
{
66+
name: "sufficient permissions in non-strict mode produces no warning",
67+
content: `---
68+
on: push
69+
permissions:
70+
contents: write
71+
issues: write
72+
tools:
73+
github:
74+
toolsets: [repos, issues]
75+
---
76+
77+
# Test Workflow
78+
`,
79+
strict: false,
80+
expectError: false,
81+
expectWarning: false,
82+
},
83+
{
84+
name: "sufficient permissions in strict mode produces no error",
85+
content: `---
86+
on: push
87+
permissions:
88+
contents: read
89+
issues: read
90+
engine: copilot
91+
network:
92+
allowed:
93+
- "api.example.com"
94+
tools:
95+
github:
96+
toolsets: [repos, issues]
97+
read-only: true
98+
---
99+
100+
# Test Workflow
101+
`,
102+
strict: true,
103+
expectError: false,
104+
expectWarning: false,
105+
},
106+
}
107+
108+
for _, tt := range tests {
109+
t.Run(tt.name, func(t *testing.T) {
110+
tmpDir, err := os.MkdirTemp("", "permissions-warning-test")
111+
if err != nil {
112+
t.Fatal(err)
113+
}
114+
defer os.RemoveAll(tmpDir)
115+
116+
testFile := filepath.Join(tmpDir, "test-workflow.md")
117+
if err := os.WriteFile(testFile, []byte(tt.content), 0644); err != nil {
118+
t.Fatal(err)
119+
}
120+
121+
// Capture stderr to check for warnings
122+
oldStderr := os.Stderr
123+
r, w, _ := os.Pipe()
124+
os.Stderr = w
125+
126+
compiler := NewCompiler(false, "", "test")
127+
compiler.SetStrictMode(tt.strict)
128+
err = compiler.CompileWorkflow(testFile)
129+
130+
// Restore stderr
131+
w.Close()
132+
os.Stderr = oldStderr
133+
var buf bytes.Buffer
134+
io.Copy(&buf, r)
135+
stderrOutput := buf.String()
136+
137+
// Check error expectation
138+
if tt.expectError && err == nil {
139+
t.Error("Expected compilation to fail but it succeeded")
140+
} else if !tt.expectError && err != nil {
141+
t.Errorf("Expected compilation to succeed but it failed: %v", err)
142+
}
143+
144+
// Check warning expectation
145+
if tt.expectWarning {
146+
if !strings.Contains(stderrOutput, tt.warningMessage) {
147+
t.Errorf("Expected warning containing '%s', got stderr:\n%s", tt.warningMessage, stderrOutput)
148+
}
149+
if !strings.Contains(stderrOutput, "warning:") {
150+
t.Errorf("Expected 'warning:' in stderr output, got:\n%s", stderrOutput)
151+
}
152+
// Check for the new suggestion format
153+
if !strings.Contains(stderrOutput, "Option 1: Add missing permissions") {
154+
t.Errorf("Expected 'Option 1: Add missing permissions' in warning, got:\n%s", stderrOutput)
155+
}
156+
if !strings.Contains(stderrOutput, "Option 2: Reduce the required toolsets") {
157+
t.Errorf("Expected 'Option 2: Reduce the required toolsets' in warning, got:\n%s", stderrOutput)
158+
}
159+
} else {
160+
// For non-warning cases, we should not see the warning message content
161+
if tt.warningMessage != "" && strings.Contains(stderrOutput, tt.warningMessage) {
162+
t.Errorf("Unexpected warning in stderr output:\n%s", stderrOutput)
163+
}
164+
}
165+
166+
// Verify warning count
167+
if tt.expectWarning {
168+
warningCount := compiler.GetWarningCount()
169+
if warningCount == 0 {
170+
t.Error("Expected warning count > 0 but got 0")
171+
}
172+
}
173+
})
174+
}
175+
}
176+
177+
// TestPermissionsWarningMessageFormat tests that the warning message format
178+
// includes both options for fixing the issue
179+
func TestPermissionsWarningMessageFormat(t *testing.T) {
180+
tmpDir, err := os.MkdirTemp("", "permissions-warning-format-test")
181+
if err != nil {
182+
t.Fatal(err)
183+
}
184+
defer os.RemoveAll(tmpDir)
185+
186+
content := `---
187+
on: push
188+
permissions:
189+
contents: read
190+
tools:
191+
github:
192+
toolsets: [repos, issues, pull_requests]
193+
read-only: false
194+
---
195+
196+
# Test Workflow
197+
`
198+
199+
testFile := filepath.Join(tmpDir, "test-workflow.md")
200+
if err := os.WriteFile(testFile, []byte(content), 0644); err != nil {
201+
t.Fatal(err)
202+
}
203+
204+
// Capture stderr to check for warnings
205+
oldStderr := os.Stderr
206+
r, w, _ := os.Pipe()
207+
os.Stderr = w
208+
209+
compiler := NewCompiler(false, "", "test")
210+
compiler.SetStrictMode(false)
211+
err = compiler.CompileWorkflow(testFile)
212+
213+
// Restore stderr
214+
w.Close()
215+
os.Stderr = oldStderr
216+
var buf bytes.Buffer
217+
io.Copy(&buf, r)
218+
stderrOutput := buf.String()
219+
220+
if err != nil {
221+
t.Fatalf("Expected compilation to succeed but it failed: %v", err)
222+
}
223+
224+
// Check that the warning includes both options
225+
expectedPhrases := []string{
226+
"Missing required permissions for github toolsets:",
227+
"Option 1: Add missing permissions to your workflow frontmatter:",
228+
"Option 2: Reduce the required toolsets in your workflow:",
229+
"issues",
230+
"pull_requests",
231+
"repos",
232+
"contents: write",
233+
"issues: write",
234+
"pull-requests: write",
235+
}
236+
237+
for _, phrase := range expectedPhrases {
238+
if !strings.Contains(stderrOutput, phrase) {
239+
t.Errorf("Expected warning to contain '%s', got:\n%s", phrase, stderrOutput)
240+
}
241+
}
242+
}

0 commit comments

Comments
 (0)