diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0ed3f9dc0e..77ad68f0b2 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -357,7 +357,7 @@ runtime behavior (such as output formatting) won't appear here. ### `thread_resolution_reason` - **pull_request_review_write** - Write operations (create, submit, delete) on pull request reviews - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: Review comment text (string, optional) - `commitID`: SHA of commit to review (string, optional) - `event`: Review action to perform. (string, optional) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 9b6ee5da6b..5ab2f86d27 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1403,7 +1403,7 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"owner", "repo", "issue_number"}, }, }, - scopes.RequireAll(scopes.Repo), + publicRepositoryWriteScopeAccess(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -2524,7 +2524,7 @@ Options are: Required: []string{"method", "owner", "repo"}, }, }, - scopes.RequireAll(scopes.Repo), + publicRepositoryWriteScopeAccess(), func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { diff --git a/pkg/github/public_repo_scopes_test.go b/pkg/github/public_repo_scopes_test.go new file mode 100644 index 0000000000..754ca93d78 --- /dev/null +++ b/pkg/github/public_repo_scopes_test.go @@ -0,0 +1,103 @@ +package github + +import ( + "testing" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPublicRepoContributionToolScopeAccess(t *testing.T) { + t.Parallel() + + tools := []struct { + name string + tool inventory.ServerTool + }{ + {name: "fork_repository", tool: ForkRepository(translations.NullTranslationHelper)}, + {name: "create_branch", tool: CreateBranch(translations.NullTranslationHelper)}, + {name: "create_pull_request", tool: CreatePullRequest(translations.NullTranslationHelper)}, + {name: "issue_write", tool: IssueWrite(translations.NullTranslationHelper)}, + {name: "add_issue_comment", tool: AddIssueComment(translations.NullTranslationHelper)}, + } + + for _, tt := range tools { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, []string{string(scopes.Repo)}, tt.tool.ScopeAccess.Scopes) + require.NotNil(t, tt.tool.ScopeAccess.Visible) + assert.False(t, tt.tool.ScopeAccess.Visible(nil)) + assert.True(t, tt.tool.ScopeAccess.Visible([]string{string(scopes.PublicRepo)})) + assert.True(t, tt.tool.ScopeAccess.Visible([]string{string(scopes.Repo)})) + + require.NotNil(t, tt.tool.ScopeAccess.Challenge) + assert.Equal(t, []string{string(scopes.Repo)}, tt.tool.ScopeAccess.Challenge(nil, nil)) + assert.Equal(t, []string{string(scopes.Repo)}, tt.tool.ScopeAccess.Challenge(nil, []string{string(scopes.PublicRepo)})) + assert.Empty(t, tt.tool.ScopeAccess.Challenge(nil, []string{string(scopes.Repo)})) + }) + } +} + +func TestPublicRepoContributionToolsVisibleToPATs(t *testing.T) { + t.Parallel() + + tools := []inventory.ServerTool{ + ForkRepository(translations.NullTranslationHelper), + CreateBranch(translations.NullTranslationHelper), + PushFiles(translations.NullTranslationHelper), + CreatePullRequest(translations.NullTranslationHelper), + IssueWrite(translations.NullTranslationHelper), + AddIssueComment(translations.NullTranslationHelper), + } + + tests := []struct { + name string + tokenScopes []string + wantVisible bool + }{ + {name: "no scopes"}, + {name: "public_repo", tokenScopes: []string{string(scopes.PublicRepo)}, wantVisible: true}, + {name: "repo", tokenScopes: []string{string(scopes.Repo)}, wantVisible: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + filter := CreateToolScopeFilter(tt.tokenScopes) + for i := range tools { + included, err := filter(t.Context(), &tools[i]) + require.NoError(t, err) + assert.Equal(t, tt.wantVisible, included, tools[i].Tool.Name) + } + }) + } +} + +func TestPushFilesOAuthScopeChallenges(t *testing.T) { + t.Parallel() + + tool := PushFiles(translations.NullTranslationHelper) + regularFiles := map[string]any{ + "files": []any{map[string]any{"path": "README.md"}}, + } + workflowFiles := map[string]any{ + "files": []any{map[string]any{"path": ".github/workflows/ci.yml"}}, + } + + assert.Equal(t, []string{string(scopes.Repo), string(scopes.Workflow)}, tool.ScopeAccess.Scopes) + require.NotNil(t, tool.ScopeAccess.Visible) + assert.True(t, tool.ScopeAccess.Visible([]string{string(scopes.PublicRepo)})) + assert.True(t, tool.ScopeAccess.Visible([]string{string(scopes.Repo)})) + + assert.Equal(t, []string{string(scopes.Repo)}, tool.ScopeAccess.Challenge(regularFiles, nil)) + assert.Equal(t, []string{string(scopes.Repo)}, tool.ScopeAccess.Challenge(regularFiles, []string{string(scopes.PublicRepo)})) + assert.Empty(t, tool.ScopeAccess.Challenge(regularFiles, []string{string(scopes.Repo)})) + + assert.Equal(t, []string{string(scopes.Repo), string(scopes.Workflow)}, tool.ScopeAccess.Challenge(workflowFiles, nil)) + assert.Equal(t, []string{string(scopes.Repo), string(scopes.Workflow)}, tool.ScopeAccess.Challenge(workflowFiles, []string{string(scopes.PublicRepo)})) + assert.Equal(t, []string{string(scopes.Workflow)}, tool.ScopeAccess.Challenge(workflowFiles, []string{string(scopes.Repo)})) + assert.Equal(t, []string{string(scopes.Repo)}, tool.ScopeAccess.Challenge(workflowFiles, []string{string(scopes.Workflow)})) + assert.Equal(t, []string{string(scopes.Repo)}, tool.ScopeAccess.Challenge(workflowFiles, []string{string(scopes.PublicRepo), string(scopes.Workflow)})) + assert.Empty(t, tool.ScopeAccess.Challenge(workflowFiles, []string{string(scopes.Repo), string(scopes.Workflow)})) +} diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index afd2778510..cb6d1be3a3 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -706,7 +706,7 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo Required: []string{"owner", "repo", "title", "head", "base"}, }, }, - scopes.RequireAll(scopes.Repo), + publicRepositoryWriteScopeAccess(), func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 8575d994cc..d920b46247 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1229,7 +1229,7 @@ func ForkRepository(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo"}, }, }, - scopes.RequireAll(scopes.Repo), + publicRepositoryWriteScopeAccess(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1526,7 +1526,7 @@ func CreateBranch(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "branch"}, }, }, - scopes.RequireAll(scopes.Repo), + publicRepositoryWriteScopeAccess(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1658,7 +1658,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "branch", "files", "message"}, }, }, - scopes.RequireAll(scopes.Repo), + publicRepositoryWriteScopeAccess(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/repository_path.go b/pkg/github/repository_path.go index 4d502fe1db..78c8de5f96 100644 --- a/pkg/github/repository_path.go +++ b/pkg/github/repository_path.go @@ -75,8 +75,12 @@ func workflowScopeChallengeForFiles(arguments map[string]any, activeScopes []str containsWorkflow = true } } - if containsWorkflow { - return scopes.ChallengeAll(activeScopes, scopes.Repo, scopes.Workflow) + var challenge []string + if !scopes.HasAll(activeScopes, scopes.Repo) { + challenge = append(challenge, string(scopes.Repo)) } - return scopes.ChallengeAll(activeScopes, scopes.Repo) + if containsWorkflow && !scopes.HasAll(activeScopes, scopes.Workflow) { + challenge = append(challenge, string(scopes.Workflow)) + } + return challenge } diff --git a/pkg/github/repository_path_test.go b/pkg/github/repository_path_test.go index d9178ae2af..4ec51304ee 100644 --- a/pkg/github/repository_path_test.go +++ b/pkg/github/repository_path_test.go @@ -84,7 +84,7 @@ func TestFileWriteWorkflowScopeChallenges(t *testing.T) { map[string]any{"path": "README.md"}, map[string]any{"path": ".github/workflows/ci.yml"}, }}, - want: []string{"repo", "workflow"}, + want: []string{"workflow"}, }, { name: "push validates entries after workflow", diff --git a/pkg/github/tool_scopes.go b/pkg/github/tool_scopes.go index c61dbd3829..4482a6e243 100644 --- a/pkg/github/tool_scopes.go +++ b/pkg/github/tool_scopes.go @@ -5,6 +5,14 @@ import ( "github.com/github/github-mcp-server/pkg/scopes" ) +func publicRepositoryWriteScopeAccess() inventory.ScopeAccess { + access := scopes.RequireAll(scopes.Repo) + access.Visible = func(activeScopes []string) bool { + return scopes.HasAll(activeScopes, scopes.PublicRepo) + } + return access +} + func repositoryOrOrganizationScopeAccess() inventory.ScopeAccess { return scopes.DynamicChallenge( []scopes.Scope{scopes.Repo, scopes.ReadOrg}, diff --git a/pkg/scopes/scopes_test.go b/pkg/scopes/scopes_test.go index 8240e8454b..62bfc6178e 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -21,6 +21,7 @@ func TestOAuthScopeCatalog(t *testing.T) { func TestScopeChecks(t *testing.T) { assert.True(t, HasAll([]string{"repo", "workflow"}, Repo, Workflow)) + assert.True(t, HasAll([]string{"repo"}, PublicRepo)) assert.False(t, HasAll([]string{"repo"}, Repo, Workflow)) assert.True(t, HasAll([]string{"admin:org"}, ReadOrg)) assert.True(t, HasAllScopeNames([]string{"admin:org"}, []string{"read:org"}))