Add permissions validator for GitHub MCP toolsets#2768
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| } | ||
|
|
||
| // defaultToolsets defines the toolsets that are enabled when "default" is specified | ||
| var defaultToolsets = []string{"context", "repos", "issues", "pull_requests", "users"} |
There was a problem hiding this comment.
@copilot review GitHub mcp server for default toolsets constant
There was a problem hiding this comment.
Verified that DefaultGitHubToolsets matches the documented default toolsets in github-mcp-server.instructions.md (context, repos, issues, pull_requests, users). Moved to exported constant in github_toolsets.go (commit 63576b4).
| } | ||
|
|
||
| // parseToolsets parses the toolsets string and expands "default" and "all" | ||
| func parseToolsets(toolsetsStr string) []string { |
There was a problem hiding this comment.
@copilot move to separate github file and reuse existing GitHub MCP sever code, add tests
There was a problem hiding this comment.
Moved GitHub toolsets parsing to github_toolsets.go with comprehensive tests in github_toolsets_test.go. The ParseGitHubToolsets function now reuses the existing toolsetPermissionsMap and is properly separated from the validator logic (commit 63576b4).
- Move defaultToolsets constant to DefaultGitHubToolsets in github_toolsets.go - Move parseToolsets function to ParseGitHubToolsets in github_toolsets.go - Add comprehensive tests in github_toolsets_test.go - Update permissions_validator.go to use new exported functions - Remove duplicate test cases from permissions_validator_test.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Agentic Changeset Generator triggered by this pull request. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive permissions validation system for GitHub MCP toolsets. The system validates that workflow permissions align with the configured GitHub MCP toolsets to ensure security best practices by detecting both missing and over-provisioned permissions.
Key changes:
- Created GitHub toolsets parsing logic with support for "default" and "all" expansions
- Implemented permissions validation that maps toolsets to required permissions
- Integrated validation into the compilation process with appropriate error/warning handling
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/permissions_validator.go | Core permissions validation logic with toolset-to-permission mapping |
| pkg/workflow/permissions_validator_test.go | Comprehensive test suite for validation functionality |
| pkg/workflow/github_toolsets.go | GitHub toolsets parsing with "default" and "all" expansion |
| pkg/workflow/github_toolsets_test.go | Tests for GitHub toolsets parsing logic |
| pkg/workflow/compiler.go | Integration of permissions validation into compilation process |
| Various test files | Updated test workflows with required permissions to fix test failures |
| .github/workflows/*.lock.yml | Updated Node.js action versions in locked workflows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lines = append(lines, "Consider removing these permissions or adjusting your toolsets configuration.") | ||
|
|
||
| return strings.Join(lines, "\n") | ||
| } |
There was a problem hiding this comment.
The permissions validator module lacks comprehensive documentation. Consider adding package-level documentation explaining the validation system, toolset-to-permission mappings, and usage examples.
| } | ||
| } else if toolset == "all" { | ||
| // Add all toolsets from the toolset permissions map | ||
| for t := range toolsetPermissionsMap { |
There was a problem hiding this comment.
The function depends on the global toolsetPermissionsMap variable which creates tight coupling. Consider passing the toolset map as a parameter or making it a method on a struct to improve testability and modularity.
|
|
||
| // Check for excess permissions (only for specific toolsets, not "all") | ||
| // Note: "default" is expanded to specific toolsets, so excess checking should still happen | ||
| shouldCheckExcess := !containsAllToolset(toolsetsStr) |
There was a problem hiding this comment.
The excess permission checking logic uses the original toolsets string rather than the parsed toolsets. This creates inconsistency and could be confusing. Consider using a flag or checking the parsed toolsets instead.
| result.ExcessPermissions[scope] = grantedLevel | ||
| result.ExcessPermissionScopes = append(result.ExcessPermissionScopes, scope) | ||
| continue | ||
| } | ||
|
|
||
| // If write permission is granted but only read is required | ||
| if grantedLevel == PermissionWrite && requiredLevel == PermissionRead { | ||
| result.ExcessPermissions[scope] = PermissionWrite |
There was a problem hiding this comment.
The logic stores the excess permission as the granted level (PermissionWrite) rather than indicating what the excess is (Write when Read would suffice). This could be clearer for users trying to understand what needs to be changed.
| result.ExcessPermissions[scope] = grantedLevel | |
| result.ExcessPermissionScopes = append(result.ExcessPermissionScopes, scope) | |
| continue | |
| } | |
| // If write permission is granted but only read is required | |
| if grantedLevel == PermissionWrite && requiredLevel == PermissionRead { | |
| result.ExcessPermissions[scope] = PermissionWrite | |
| result.ExcessPermissions[scope] = fmt.Sprintf("granted: %s, required: none", grantedLevel) | |
| result.ExcessPermissionScopes = append(result.ExcessPermissionScopes, scope) | |
| continue | |
| } | |
| // If write permission is granted but only read is required | |
| if grantedLevel == PermissionWrite && requiredLevel == PermissionRead { | |
| result.ExcessPermissions[scope] = fmt.Sprintf("granted: %s, required: %s", grantedLevel, requiredLevel) |
Permissions Validation Checker for GitHub MCP Toolsets
Latest Changes (Refactoring)
New Files:
pkg/workflow/github_toolsets.go- GitHub MCP toolsets parsing logicDefaultGitHubToolsetsconstant matching documented defaultsParseGitHubToolsets()function for expanding "default" and "all"pkg/workflow/github_toolsets_test.go- Comprehensive tests for toolsets parsingModified Files:
pkg/workflow/permissions_validator.go- Now uses exported functions from github_toolsets.gopkg/workflow/permissions_validator_test.go- Removed duplicate test casesBenefits:
All tests passing ✅
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.