Skip to content

Commit 11cfdb2

Browse files
CopilotCopilot
authored
Add permissions validator for GitHub MCP toolsets (#2768)
1 parent 029a88c commit 11cfdb2

11 files changed

Lines changed: 1281 additions & 0 deletions

‎pkg/cli/commands_compile_workflow_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -570,6 +570,8 @@ on:
570570
branches: [main]
571571
permissions:
572572
contents: read
573+
issues: read
574+
pull-requests: read
573575
---
574576
575577
# Test Workflow
@@ -617,6 +619,8 @@ on:
617619
branches: [main]
618620
permissions:
619621
contents: read
622+
issues: read
623+
pull-requests: read
620624
---
621625
622626
# Test Workflow

‎pkg/workflow/compiler.go‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,64 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath
275275
return errors.New(formattedErr)
276276
}
277277

278+
// Validate permissions against GitHub MCP toolsets
279+
log.Printf("Validating permissions for GitHub MCP toolsets")
280+
if githubTool, hasGitHub := workflowData.Tools["github"]; hasGitHub {
281+
// Parse permissions from the workflow data
282+
// WorkflowData.Permissions contains the raw YAML string (including "permissions:" prefix)
283+
permissions := NewPermissionsParser(workflowData.Permissions).ToPermissions()
284+
285+
// Validate permissions
286+
validationResult := ValidatePermissions(permissions, githubTool)
287+
288+
if validationResult.HasValidationIssues {
289+
// Format the validation message
290+
message := FormatValidationMessage(validationResult, c.strictMode)
291+
292+
if len(validationResult.MissingPermissions) > 0 {
293+
// Missing permissions are always errors
294+
formattedErr := console.FormatError(console.CompilerError{
295+
Position: console.ErrorPosition{
296+
File: markdownPath,
297+
Line: 1,
298+
Column: 1,
299+
},
300+
Type: "error",
301+
Message: message,
302+
})
303+
return errors.New(formattedErr)
304+
}
305+
306+
if len(validationResult.ExcessPermissions) > 0 {
307+
// Excess permissions are warnings by default, errors in strict mode
308+
if c.strictMode {
309+
formattedErr := console.FormatError(console.CompilerError{
310+
Position: console.ErrorPosition{
311+
File: markdownPath,
312+
Line: 1,
313+
Column: 1,
314+
},
315+
Type: "error",
316+
Message: message,
317+
})
318+
return errors.New(formattedErr)
319+
} else {
320+
formattedWarning := console.FormatError(console.CompilerError{
321+
Position: console.ErrorPosition{
322+
File: markdownPath,
323+
Line: 1,
324+
Column: 1,
325+
},
326+
Type: "warning",
327+
Message: message,
328+
})
329+
fmt.Fprintln(os.Stderr, formattedWarning)
330+
c.IncrementWarningCount()
331+
}
332+
}
333+
}
334+
}
335+
278336
// Note: Markdown content size is now handled by splitting into multiple steps in generatePrompt
279337

280338
log.Printf("Workflow: %s, Tools: %d", workflowData.Name, len(workflowData.Tools))

‎pkg/workflow/github_toolsets.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
package workflow
2+
3+
import (
4+
"strings"
5+
)
6+
7+
// DefaultGitHubToolsets defines the toolsets that are enabled by default
8+
// when toolsets are not explicitly specified in the GitHub MCP configuration.
9+
// These match the documented default toolsets in github-mcp-server.instructions.md
10+
var DefaultGitHubToolsets = []string{"context", "repos", "issues", "pull_requests", "users"}
11+
12+
// ParseGitHubToolsets parses the toolsets string and expands "default" and "all"
13+
// into their constituent toolsets. It handles comma-separated lists and deduplicates.
14+
func ParseGitHubToolsets(toolsetsStr string) []string {
15+
if toolsetsStr == "" {
16+
return DefaultGitHubToolsets
17+
}
18+
19+
toolsets := strings.Split(toolsetsStr, ",")
20+
var expanded []string
21+
seenToolsets := make(map[string]bool)
22+
23+
for _, toolset := range toolsets {
24+
toolset = strings.TrimSpace(toolset)
25+
if toolset == "" {
26+
continue
27+
}
28+
29+
if toolset == "default" {
30+
// Add default toolsets
31+
for _, dt := range DefaultGitHubToolsets {
32+
if !seenToolsets[dt] {
33+
expanded = append(expanded, dt)
34+
seenToolsets[dt] = true
35+
}
36+
}
37+
} else if toolset == "all" {
38+
// Add all toolsets from the toolset permissions map
39+
for t := range toolsetPermissionsMap {
40+
if !seenToolsets[t] {
41+
expanded = append(expanded, t)
42+
seenToolsets[t] = true
43+
}
44+
}
45+
} else {
46+
// Add individual toolset
47+
if !seenToolsets[toolset] {
48+
expanded = append(expanded, toolset)
49+
seenToolsets[toolset] = true
50+
}
51+
}
52+
}
53+
54+
return expanded
55+
}

‎pkg/workflow/github_toolsets_test.go‎

Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
package workflow
2+
3+
import (
4+
"testing"
5+
)
6+
7+
func TestDefaultGitHubToolsets(t *testing.T) {
8+
// Verify the default toolsets match the documented defaults
9+
expected := []string{"context", "repos", "issues", "pull_requests", "users"}
10+
11+
if len(DefaultGitHubToolsets) != len(expected) {
12+
t.Errorf("Expected %d default toolsets, got %d", len(expected), len(DefaultGitHubToolsets))
13+
}
14+
15+
for i, toolset := range expected {
16+
if i >= len(DefaultGitHubToolsets) || DefaultGitHubToolsets[i] != toolset {
17+
t.Errorf("Expected default toolset[%d] to be %s, got %s", i, toolset, DefaultGitHubToolsets[i])
18+
}
19+
}
20+
}
21+
22+
func TestParseGitHubToolsets(t *testing.T) {
23+
tests := []struct {
24+
name string
25+
input string
26+
expected []string
27+
}{
28+
{
29+
name: "Empty string returns default",
30+
input: "",
31+
expected: []string{"context", "repos", "issues", "pull_requests", "users"},
32+
},
33+
{
34+
name: "Default expands to default toolsets",
35+
input: "default",
36+
expected: []string{"context", "repos", "issues", "pull_requests", "users"},
37+
},
38+
{
39+
name: "Specific toolsets",
40+
input: "repos,issues",
41+
expected: []string{"repos", "issues"},
42+
},
43+
{
44+
name: "Default plus additional",
45+
input: "default,discussions",
46+
expected: []string{"context", "repos", "issues", "pull_requests", "users", "discussions"},
47+
},
48+
{
49+
name: "All expands to all toolsets",
50+
input: "all",
51+
// Should include all 19 toolsets - we'll check the count
52+
expected: nil,
53+
},
54+
{
55+
name: "Deduplication",
56+
input: "repos,issues,repos",
57+
expected: []string{"repos", "issues"},
58+
},
59+
{
60+
name: "Whitespace handling",
61+
input: " repos , issues , pull_requests ",
62+
expected: []string{"repos", "issues", "pull_requests"},
63+
},
64+
{
65+
name: "Single toolset",
66+
input: "actions",
67+
expected: []string{"actions"},
68+
},
69+
{
70+
name: "Multiple with default in middle",
71+
input: "actions,default,discussions",
72+
expected: []string{"actions", "context", "repos", "issues", "pull_requests", "users", "discussions"},
73+
},
74+
}
75+
76+
for _, tt := range tests {
77+
t.Run(tt.name, func(t *testing.T) {
78+
result := ParseGitHubToolsets(tt.input)
79+
80+
if tt.name == "All expands to all toolsets" {
81+
// Check that all toolsets are present
82+
if len(result) != len(toolsetPermissionsMap) {
83+
t.Errorf("Expected %d toolsets for 'all', got %d", len(toolsetPermissionsMap), len(result))
84+
}
85+
return
86+
}
87+
88+
if len(result) != len(tt.expected) {
89+
t.Errorf("Expected %d toolsets, got %d: %v", len(tt.expected), len(result), result)
90+
return
91+
}
92+
93+
// Check that all expected toolsets are present (order doesn't matter for some tests)
94+
resultMap := make(map[string]bool)
95+
for _, ts := range result {
96+
resultMap[ts] = true
97+
}
98+
99+
for _, expected := range tt.expected {
100+
if !resultMap[expected] {
101+
t.Errorf("Expected toolset %s not found in result: %v", expected, result)
102+
}
103+
}
104+
})
105+
}
106+
}
107+
108+
func TestParseGitHubToolsetsPreservesOrder(t *testing.T) {
109+
// Test that specific toolsets maintain their order
110+
input := "repos,issues,pull_requests"
111+
result := ParseGitHubToolsets(input)
112+
expected := []string{"repos", "issues", "pull_requests"}
113+
114+
if len(result) != len(expected) {
115+
t.Fatalf("Expected %d toolsets, got %d", len(expected), len(result))
116+
}
117+
118+
for i, toolset := range expected {
119+
if result[i] != toolset {
120+
t.Errorf("Expected toolset[%d] to be %s, got %s", i, toolset, result[i])
121+
}
122+
}
123+
}
124+
125+
func TestParseGitHubToolsetsDeduplication(t *testing.T) {
126+
tests := []struct {
127+
name string
128+
input string
129+
expected int
130+
}{
131+
{
132+
name: "Duplicate in simple list",
133+
input: "repos,issues,repos,issues",
134+
expected: 2,
135+
},
136+
{
137+
name: "Default includes duplicates",
138+
input: "context,default",
139+
expected: 5, // context already in default, so only 5 unique
140+
},
141+
{
142+
name: "All with duplicates",
143+
input: "all,repos,issues",
144+
expected: len(toolsetPermissionsMap), // All toolsets, duplicates ignored
145+
},
146+
}
147+
148+
for _, tt := range tests {
149+
t.Run(tt.name, func(t *testing.T) {
150+
result := ParseGitHubToolsets(tt.input)
151+
if len(result) != tt.expected {
152+
t.Errorf("Expected %d unique toolsets, got %d: %v", tt.expected, len(result), result)
153+
}
154+
155+
// Verify no duplicates
156+
seen := make(map[string]bool)
157+
for _, toolset := range result {
158+
if seen[toolset] {
159+
t.Errorf("Found duplicate toolset: %s", toolset)
160+
}
161+
seen[toolset] = true
162+
}
163+
})
164+
}
165+
}

‎pkg/workflow/imports_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ tools:
3232
on: issues
3333
permissions:
3434
contents: read
35+
issues: read
36+
pull-requests: read
3537
engine: copilot
3638
imports:
3739
- shared-tool.md
@@ -110,6 +112,8 @@ tools:
110112
on: issues
111113
permissions:
112114
contents: read
115+
issues: read
116+
pull-requests: read
113117
engine: copilot
114118
imports:
115119
- shared-tool-1.md
@@ -184,6 +188,8 @@ mcp-servers:
184188
on: issues
185189
permissions:
186190
contents: read
191+
issues: read
192+
pull-requests: read
187193
engine: copilot
188194
imports:
189195
- shared-mcp.md

‎pkg/workflow/mcp_fields_schema_test.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ mcp-servers:
4040
on: issues
4141
permissions:
4242
contents: read
43+
issues: read
44+
pull-requests: read
4345
engine: copilot
4446
imports:
4547
- mcp-with-fields.md
@@ -95,6 +97,10 @@ mcp-servers:
9597
workflowPath := filepath.Join(tempDir, "workflow.md")
9698
workflowContent := `---
9799
on: push
100+
permissions:
101+
contents: read
102+
issues: read
103+
pull-requests: read
98104
engine: copilot
99105
imports:
100106
- mcp-entrypoint.md
@@ -142,6 +148,10 @@ mcp-servers:
142148
workflowPath := filepath.Join(tempDir, "workflow.md")
143149
workflowContent := `---
144150
on: push
151+
permissions:
152+
contents: read
153+
issues: read
154+
pull-requests: read
145155
engine: copilot
146156
imports:
147157
- mcp-headers.md
@@ -185,6 +195,10 @@ mcp-servers:
185195
workflowPath := filepath.Join(tempDir, "workflow.md")
186196
workflowContent := `---
187197
on: push
198+
permissions:
199+
contents: read
200+
issues: read
201+
pull-requests: read
188202
engine: copilot
189203
imports:
190204
- mcp-url.md

‎pkg/workflow/network_merge_edge_cases_test.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ network:
3131
workflowContent := `---
3232
on: issues
3333
engine: claude
34+
permissions:
35+
contents: read
36+
issues: read
37+
pull-requests: read
3438
network:
3539
allowed:
3640
- github.com
@@ -81,6 +85,10 @@ network: {}
8185
workflowContent := `---
8286
on: issues
8387
engine: claude
88+
permissions:
89+
contents: read
90+
issues: read
91+
pull-requests: read
8492
network:
8593
allowed:
8694
- github.com

0 commit comments

Comments
 (0)