From 0de95e0144d36a33b581928aff9244dc067c3782 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Fri, 26 Jun 2026 16:12:03 +0200 Subject: [PATCH 1/3] Fix OAuth scope satisfaction to AND-of-ORs (required means required) Tool scope requirements conflate two relationships: conjunction (a tool needs several different capabilities, AND) and hierarchy/substitution (a requirement is met by itself or a higher scope, OR). Previously both collapsed into one OR list checked against the expanded AcceptedScopes, so a token holding only repo was treated as satisfying ui_get (which also needs read:org): the PAT filter showed it and no scope challenge was issued, then it failed at runtime. Make RequiredScopes the single source of truth and evaluate satisfaction as AND-of-ORs: expand the token downward through the hierarchy and require every declared scope to be present. AcceptedScopes becomes display-only metadata. - HasRequiredScopes: AND semantics over requiredScopes. - ToolScopeInfo.Satisfies (was HasAcceptedScope) + precise MissingScopes. - scope_filter: filter on RequiredScopes; keep read-only repo-only special case. - scope_challenge: challenge with only the missing required scopes. - generate-docs: drop "(any of)" rendering; all required scopes are AND. Affects ui_get, list_issue_types, and list_issue_fields (all {repo, read:org}). Filtering stays gated to classic ghp_ PATs and fails open; fine-grained/OAuth/ app tokens are unaffected. Split out of the PR #2751 review discussion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 4 +- cmd/github-mcp-server/feature_flag_docs.go | 6 +- cmd/github-mcp-server/generate_docs.go | 12 ++-- docs/feature-flags.md | 4 +- docs/insiders-features.md | 4 +- pkg/github/scope_filter.go | 23 +++++--- pkg/github/scope_filter_test.go | 66 +++++++++++++++------- pkg/http/middleware/scope_challenge.go | 16 +++--- pkg/scopes/map.go | 63 ++++++++++----------- pkg/scopes/map_test.go | 56 +++++++++++++----- pkg/scopes/scopes.go | 38 ++++++++----- pkg/scopes/scopes_test.go | 54 ++++++++++-------- 12 files changed, 208 insertions(+), 138 deletions(-) diff --git a/README.md b/README.md index 23c15dc86b..d1771f4ee2 100644 --- a/README.md +++ b/README.md @@ -922,13 +922,13 @@ The following sets of tools are available: - `type`: Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string, optional) - **list_issue_fields** - List issue fields - - **Required OAuth Scopes (any of)**: `repo`, `read:org` + - **Required OAuth Scopes**: `repo`, `read:org` - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` - `owner`: The account owner of the repository or organization. The name is not case sensitive. (string, required) - `repo`: The name of the repository. When provided, returns fields for this specific repository (inherited from its organization). When omitted, returns org-level fields directly. (string, optional) - **list_issue_types** - List available issue types - - **Required OAuth Scopes (any of)**: `repo`, `read:org` + - **Required OAuth Scopes**: `repo`, `read:org` - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` - `owner`: The account owner of the repository or organization. (string, required) - `repo`: The name of the repository. When provided, returns issue types for this specific repository. When omitted, returns org-level issue types directly. (string, optional) diff --git a/cmd/github-mcp-server/feature_flag_docs.go b/cmd/github-mcp-server/feature_flag_docs.go index e52237b138..cfb39b5274 100644 --- a/cmd/github-mcp-server/feature_flag_docs.go +++ b/cmd/github-mcp-server/feature_flag_docs.go @@ -63,10 +63,14 @@ func generateFlaggedToolsDoc(flags []string, emptyMessage string) string { if !hasAny { return emptyMessage } + // Clarify scope semantics for the rendered tools: every listed required + // scope is needed (AND), and a higher scope in the hierarchy also satisfies + // a required scope. + preamble := "> **OAuth scopes:** all listed required scopes are needed (AND). A higher scope in the hierarchy (e.g. `admin:org` for `read:org`, `repo` for `public_repo`) also satisfies a required scope.\n\n" // Leading/trailing newlines around the body produce blank lines between // our content and the surrounding marker comments, so the trailing comment // doesn't get absorbed into the final list item by markdown renderers. - return "\n" + strings.TrimSuffix(buf.String(), "\n") + "\n" + return "\n" + preamble + strings.TrimSuffix(buf.String(), "\n") + "\n" } // flaggedToolDiff returns the tools whose definition (input schema or meta) diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 212851c50d..f3b1219bb2 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -221,15 +221,11 @@ func writeToolDoc(buf *strings.Builder, tool inventory.ServerTool) { // OAuth scopes if present if len(tool.RequiredScopes) > 0 { - // Scope filtering uses "any of" semantics (see scopes.HasRequiredScopes), - // so when multiple required scopes are listed, render them as alternatives - // rather than implying all are required. + // All listed required scopes are needed (AND). A higher scope in the + // hierarchy also satisfies a required scope (see scopes.HasRequiredScopes). + // AcceptedScopes below is non-authoritative display metadata. scopeList := "`" + strings.Join(tool.RequiredScopes, "`, `") + "`" - if len(tool.RequiredScopes) > 1 { - fmt.Fprintf(buf, " - **Required OAuth Scopes (any of)**: %s\n", scopeList) - } else { - fmt.Fprintf(buf, " - **Required OAuth Scopes**: %s\n", scopeList) - } + fmt.Fprintf(buf, " - **Required OAuth Scopes**: %s\n", scopeList) // Only show accepted scopes if they differ from required scopes if len(tool.AcceptedScopes) > 0 && !scopesEqual(tool.RequiredScopes, tool.AcceptedScopes) { diff --git a/docs/feature-flags.md b/docs/feature-flags.md index bd1df9ce1b..5d78376c6f 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -32,6 +32,8 @@ runtime behavior (such as output formatting) won't appear here. +> **OAuth scopes:** all listed required scopes are needed (AND). A higher scope in the hierarchy (e.g. `admin:org` for `read:org`, `repo` for `public_repo`) also satisfies a required scope. + ### `remote_mcp_ui_apps` - **create_pull_request** - Open new pull request @@ -74,7 +76,7 @@ runtime behavior (such as output formatting) won't appear here. - `type`: Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string, optional) - **ui_get** - Get UI data - - **Required OAuth Scopes (any of)**: `repo`, `read:org` + - **Required OAuth Scopes**: `repo`, `read:org` - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` - `method`: The type of data to fetch (string, required) - `owner`: Repository owner (required for all methods) (string, required) diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 7bbeaddc3a..c55c9819c4 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -26,6 +26,8 @@ The list below is generated from the Go source. It covers tool **inventory and s +> **OAuth scopes:** all listed required scopes are needed (AND). A higher scope in the hierarchy (e.g. `admin:org` for `read:org`, `repo` for `public_repo`) also satisfies a required scope. + ### `remote_mcp_ui_apps` - **create_pull_request** - Open new pull request @@ -68,7 +70,7 @@ The list below is generated from the Go source. It covers tool **inventory and s - `type`: Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string, optional) - **ui_get** - Get UI data - - **Required OAuth Scopes (any of)**: `repo`, `read:org` + - **Required OAuth Scopes**: `repo`, `read:org` - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` - `method`: The type of data to fetch (string, required) - `owner`: Repository owner (required for all methods) (string, required) diff --git a/pkg/github/scope_filter.go b/pkg/github/scope_filter.go index 42f8e98b0c..f2cf572645 100644 --- a/pkg/github/scope_filter.go +++ b/pkg/github/scope_filter.go @@ -15,14 +15,14 @@ var repoScopesSet = map[string]bool{ string(scopes.PublicRepo): true, } -// onlyRequiresRepoScopes returns true if all of the tool's accepted scopes +// onlyRequiresRepoScopes returns true if all of the tool's required scopes // are repo-related scopes (repo, public_repo). Such tools work on public // repositories without needing any scope. -func onlyRequiresRepoScopes(acceptedScopes []string) bool { - if len(acceptedScopes) == 0 { +func onlyRequiresRepoScopes(requiredScopes []string) bool { + if len(requiredScopes) == 0 { return false } - for _, scope := range acceptedScopes { + for _, scope := range requiredScopes { if !repoScopesSet[scope] { return false } @@ -41,9 +41,13 @@ func onlyRequiresRepoScopes(acceptedScopes []string) bool { // token is known at startup and won't change during the session. // // The filter returns true (include tool) if: -// - The tool has no scope requirements (AcceptedScopes is empty) +// - The tool has no scope requirements (RequiredScopes is empty) // - The tool is read-only and only requires repo/public_repo scopes (works on public repos) -// - The token has at least one of the tool's accepted scopes +// - The token satisfies ALL of the tool's required scopes (AND-of-ORs, where +// each required scope may be met directly or by a higher scope) +// +// RequiredScopes is the single source of truth here; AcceptedScopes is +// display-only metadata and is intentionally not consulted. // // Example usage: // @@ -55,10 +59,11 @@ func onlyRequiresRepoScopes(acceptedScopes []string) bool { // inventory := github.NewInventory(t).WithFilter(filter).Build() func CreateToolScopeFilter(tokenScopes []string) inventory.ToolFilter { return func(_ context.Context, tool *inventory.ServerTool) (bool, error) { - // Read-only tools requiring only repo/public_repo work on public repos without any scope - if tool.Tool.Annotations != nil && tool.Tool.Annotations.ReadOnlyHint && onlyRequiresRepoScopes(tool.AcceptedScopes) { + // Read-only tools requiring only repo/public_repo work on public repos without any scope. + // Tools that also require a non-repo scope (e.g. {repo, read:org}) fall through to the AND check. + if tool.Tool.Annotations != nil && tool.Tool.Annotations.ReadOnlyHint && onlyRequiresRepoScopes(tool.RequiredScopes) { return true, nil } - return scopes.HasRequiredScopes(tokenScopes, tool.AcceptedScopes), nil + return scopes.HasRequiredScopes(tokenScopes, tool.RequiredScopes), nil } } diff --git a/pkg/github/scope_filter_test.go b/pkg/github/scope_filter_test.go index 9cdd4db19b..f7ca158416 100644 --- a/pkg/github/scope_filter_test.go +++ b/pkg/github/scope_filter_test.go @@ -11,20 +11,21 @@ import ( ) func TestCreateToolScopeFilter(t *testing.T) { - // Create test tools with various scope requirements + // Create test tools with various scope requirements. + // RequiredScopes is the single source of truth for filtering. toolNoScopes := &inventory.ServerTool{ Tool: mcp.Tool{Name: "no_scopes_tool"}, - AcceptedScopes: nil, + RequiredScopes: nil, } toolEmptyScopes := &inventory.ServerTool{ Tool: mcp.Tool{Name: "empty_scopes_tool"}, - AcceptedScopes: []string{}, + RequiredScopes: []string{}, } toolRepoScope := &inventory.ServerTool{ Tool: mcp.Tool{Name: "repo_tool"}, - AcceptedScopes: []string{"repo"}, + RequiredScopes: []string{"repo"}, } toolRepoScopeReadOnly := &inventory.ServerTool{ @@ -32,12 +33,12 @@ func TestCreateToolScopeFilter(t *testing.T) { Name: "repo_tool_readonly", Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, }, - AcceptedScopes: []string{"repo"}, + RequiredScopes: []string{"repo"}, } toolPublicRepoScope := &inventory.ServerTool{ Tool: mcp.Tool{Name: "public_repo_tool"}, - AcceptedScopes: []string{"public_repo", "repo"}, // repo is parent, also accepted + RequiredScopes: []string{"public_repo"}, } toolPublicRepoScopeReadOnly := &inventory.ServerTool{ @@ -45,17 +46,21 @@ func TestCreateToolScopeFilter(t *testing.T) { Name: "public_repo_tool_readonly", Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, }, - AcceptedScopes: []string{"public_repo", "repo"}, + RequiredScopes: []string{"public_repo"}, } toolGistScope := &inventory.ServerTool{ Tool: mcp.Tool{Name: "gist_tool"}, - AcceptedScopes: []string{"gist"}, + RequiredScopes: []string{"gist"}, } - toolMultiScope := &inventory.ServerTool{ - Tool: mcp.Tool{Name: "multi_scope_tool"}, - AcceptedScopes: []string{"repo", "admin:org"}, + // Models ui_get / list_issue_fields: read-only, but requires repo AND read:org. + toolRepoAndReadOrg := &inventory.ServerTool{ + Tool: mcp.Tool{ + Name: "ui_get", + Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, + }, + RequiredScopes: []string{"repo", "read:org"}, } tests := []struct { @@ -100,12 +105,6 @@ func TestCreateToolScopeFilter(t *testing.T) { tool: toolGistScope, expected: false, }, - { - name: "token with one of multiple accepted scopes can see tool", - tokenScopes: []string{"admin:org"}, - tool: toolMultiScope, - expected: true, - }, { name: "empty token scopes cannot see scoped tools", tokenScopes: []string{}, @@ -130,6 +129,24 @@ func TestCreateToolScopeFilter(t *testing.T) { tool: toolPublicRepoScope, expected: true, }, + { + name: "AND: repo-only classic PAT hides {repo, read:org} tool", + tokenScopes: []string{"repo"}, + tool: toolRepoAndReadOrg, + expected: false, + }, + { + name: "AND: {repo, read:org} token shows {repo, read:org} tool", + tokenScopes: []string{"repo", "read:org"}, + tool: toolRepoAndReadOrg, + expected: true, + }, + { + name: "AND: {repo, admin:org} token shows {repo, read:org} tool via hierarchy", + tokenScopes: []string{"repo", "admin:org"}, + tool: toolRepoAndReadOrg, + expected: true, + }, } for _, tt := range tests { @@ -149,17 +166,23 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) { { Tool: mcp.Tool{Name: "public_tool"}, Toolset: inventory.ToolsetMetadata{ID: "test"}, - AcceptedScopes: nil, // No scopes required + RequiredScopes: nil, // No scopes required }, { Tool: mcp.Tool{Name: "repo_tool"}, Toolset: inventory.ToolsetMetadata{ID: "test"}, - AcceptedScopes: []string{"repo"}, + RequiredScopes: []string{"repo"}, }, { Tool: mcp.Tool{Name: "gist_tool"}, Toolset: inventory.ToolsetMetadata{ID: "test"}, - AcceptedScopes: []string{"gist"}, + RequiredScopes: []string{"gist"}, + }, + { + // Requires repo AND read:org; hidden for a {repo}-only token. + Tool: mcp.Tool{Name: "list_issue_fields"}, + Toolset: inventory.ToolsetMetadata{ID: "test"}, + RequiredScopes: []string{"repo", "read:org"}, }, } @@ -177,7 +200,7 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) { // Get available tools availableTools := inv.AvailableTools(context.Background()) - // Should see public_tool and repo_tool, but not gist_tool + // Should see public_tool and repo_tool, but not gist_tool or list_issue_fields assert.Len(t, availableTools, 2) toolNames := make([]string, len(availableTools)) @@ -188,4 +211,5 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) { assert.Contains(t, toolNames, "public_tool") assert.Contains(t, toolNames, "repo_tool") assert.NotContains(t, toolNames, "gist_tool") + assert.NotContains(t, toolNames, "list_issue_fields") } diff --git a/pkg/http/middleware/scope_challenge.go b/pkg/http/middleware/scope_challenge.go index 1a86bf93ce..2f87a1fe51 100644 --- a/pkg/http/middleware/scope_challenge.go +++ b/pkg/http/middleware/scope_challenge.go @@ -109,14 +109,14 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter ctx = ghcontext.WithTokenScopes(ctx, activeScopes) r = r.WithContext(ctx) - // Check if user has the required scopes - if toolScopeInfo.HasAcceptedScope(activeScopes...) { + // Check if user satisfies all required scopes + if toolScopeInfo.Satisfies(activeScopes...) { next.ServeHTTP(w, r) return } - // User lacks required scopes - get the scopes they need - requiredScopes := toolScopeInfo.GetRequiredScopesSlice() + // User lacks one or more required scopes - request only the missing ones + missingScopes := toolScopeInfo.MissingScopes(activeScopes...) // Build the resource metadata URL using the shared utility // GetEffectiveResourcePath returns the original path (e.g., /mcp or /mcp/x/all) @@ -124,16 +124,16 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter resourcePath := oauth.ResolveResourcePath(r, oauthCfg) resourceMetadataURL := oauth.BuildResourceMetadataURL(r, oauthCfg, resourcePath) - // Build recommended scopes: existing scopes + required scopes - recommendedScopes := make([]string, 0, len(activeScopes)+len(requiredScopes)) + // Build recommended scopes: existing scopes + missing required scopes + recommendedScopes := make([]string, 0, len(activeScopes)+len(missingScopes)) recommendedScopes = append(recommendedScopes, activeScopes...) - recommendedScopes = append(recommendedScopes, requiredScopes...) + recommendedScopes = append(recommendedScopes, missingScopes...) // Build the WWW-Authenticate header value wwwAuthenticateHeader := fmt.Sprintf(`Bearer error="insufficient_scope", scope=%q, resource_metadata=%q, error_description=%q`, strings.Join(recommendedScopes, " "), resourceMetadataURL, - "Additional scopes required: "+strings.Join(requiredScopes, ", "), + "Additional scopes required: "+strings.Join(missingScopes, ", "), ) // Send scope challenge response with the superset of existing and required scopes diff --git a/pkg/scopes/map.go b/pkg/scopes/map.go index 3c98338347..b23fdf21c9 100644 --- a/pkg/scopes/map.go +++ b/pkg/scopes/map.go @@ -8,9 +8,13 @@ type ToolScopeMap map[string]*ToolScopeInfo // ToolScopeInfo contains scope information for a single tool. type ToolScopeInfo struct { // RequiredScopes contains the scopes that are directly required by this tool. + // They are ALL required (AND-of-ORs) and are the single source of truth for + // allow/challenge decisions. RequiredScopes []string - // AcceptedScopes contains all scopes that satisfy the requirements (including parent scopes). + // AcceptedScopes contains the required scopes plus higher scopes from the + // hierarchy. It is display-only metadata and is NOT used to make any + // allow/challenge/missing decision. AcceptedScopes []string } @@ -68,53 +72,44 @@ func GetToolScopeMapFromInventory(inv *inventory.Inventory) ToolScopeMap { return result } -// HasAcceptedScope checks if any of the provided user scopes satisfy the tool's requirements. -func (t *ToolScopeInfo) HasAcceptedScope(userScopes ...string) bool { - if t == nil || len(t.AcceptedScopes) == 0 { +// Satisfies reports whether the provided user scopes satisfy ALL of the tool's +// required scopes (AND-of-ORs). Each required scope may be satisfied directly or +// by a higher scope in the hierarchy, so the user scopes are expanded downward +// (via expandScopeSet) before checking. A tool with no required scopes is always +// satisfied. +// +// AcceptedScopes is display-only metadata and is intentionally not consulted. +func (t *ToolScopeInfo) Satisfies(userScopes ...string) bool { + if t == nil || len(t.RequiredScopes) == 0 { return true // No scopes required } - userScopeSet := make(map[string]bool) - for _, scope := range userScopes { - userScopeSet[scope] = true - } - - for _, scope := range t.AcceptedScopes { - if userScopeSet[scope] { - return true + granted := expandScopeSet(userScopes) + for _, required := range t.RequiredScopes { + if !granted[required] { + return false } } - return false + return true } -// MissingScopes returns the required scopes that are not present in the user's scopes. +// MissingScopes returns the subset of the tool's required scopes that the user +// scopes do not satisfy, after expanding the user scopes through the scope +// hierarchy. It returns nil when all required scopes are satisfied (or none are +// required). The result is the precise set of additional scopes needed for an +// OAuth scope challenge. func (t *ToolScopeInfo) MissingScopes(userScopes ...string) []string { if t == nil || len(t.RequiredScopes) == 0 { return nil } - // Create a set of user scopes for O(1) lookup - userScopeSet := make(map[string]bool, len(userScopes)) - for _, s := range userScopes { - userScopeSet[s] = true - } - - // Check if any accepted scope is present - hasAccepted := false - for _, scope := range t.AcceptedScopes { - if userScopeSet[scope] { - hasAccepted = true - break + granted := expandScopeSet(userScopes) + var missing []string + for _, required := range t.RequiredScopes { + if !granted[required] { + missing = append(missing, required) } } - - if hasAccepted { - return nil // User has sufficient scopes - } - - // Return required scopes as the minimum needed - missing := make([]string, len(t.RequiredScopes)) - copy(missing, t.RequiredScopes) return missing } diff --git a/pkg/scopes/map_test.go b/pkg/scopes/map_test.go index 5f33cdda2b..909ba66a52 100644 --- a/pkg/scopes/map_test.go +++ b/pkg/scopes/map_test.go @@ -47,7 +47,7 @@ func TestGetToolScopeInfo(t *testing.T) { assert.Nil(t, info) } -func TestToolScopeInfo_HasAcceptedScope(t *testing.T) { +func TestToolScopeInfo_Satisfies(t *testing.T) { testCases := []struct { name string scopeInfo *ToolScopeInfo @@ -90,6 +90,24 @@ func TestToolScopeInfo_HasAcceptedScope(t *testing.T) { userScopes: []string{"repo"}, expected: false, }, + { + name: "AND: repo-only does not satisfy {repo, read:org}", + scopeInfo: &ToolScopeInfo{ + RequiredScopes: []string{"repo", "read:org"}, + AcceptedScopes: []string{"public_repo", "read:org", "repo", "security_events", "write:org", "admin:org"}, + }, + userScopes: []string{"repo"}, + expected: false, + }, + { + name: "AND: {repo, admin:org} satisfies {repo, read:org} via hierarchy", + scopeInfo: &ToolScopeInfo{ + RequiredScopes: []string{"repo", "read:org"}, + AcceptedScopes: []string{"public_repo", "read:org", "repo", "security_events", "write:org", "admin:org"}, + }, + userScopes: []string{"repo", "admin:org"}, + expected: true, + }, { name: "no scope required", scopeInfo: &ToolScopeInfo{ @@ -115,7 +133,7 @@ func TestToolScopeInfo_HasAcceptedScope(t *testing.T) { expected: true, }, { - name: "missing repo scope", + name: "missing repo scope (child does not satisfy parent)", scopeInfo: &ToolScopeInfo{ RequiredScopes: []string{"repo"}, AcceptedScopes: []string{"repo"}, @@ -127,7 +145,7 @@ func TestToolScopeInfo_HasAcceptedScope(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result := tc.scopeInfo.HasAcceptedScope(tc.userScopes...) + result := tc.scopeInfo.Satisfies(tc.userScopes...) assert.Equal(t, tc.expected, result) }) } @@ -138,7 +156,6 @@ func TestToolScopeInfo_MissingScopes(t *testing.T) { name string scopeInfo *ToolScopeInfo userScopes []string - expectedLen int expectedScopes []string }{ { @@ -148,17 +165,33 @@ func TestToolScopeInfo_MissingScopes(t *testing.T) { AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, }, userScopes: []string{"read:org"}, - expectedLen: 0, expectedScopes: nil, }, { - name: "missing scope", + name: "parent scope satisfies via hierarchy - no missing", + scopeInfo: &ToolScopeInfo{ + RequiredScopes: []string{"read:org"}, + AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, + }, + userScopes: []string{"admin:org"}, + expectedScopes: nil, + }, + { + name: "single missing scope", scopeInfo: &ToolScopeInfo{ RequiredScopes: []string{"read:org"}, AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, }, userScopes: []string{"repo"}, - expectedLen: 1, + expectedScopes: []string{"read:org"}, + }, + { + name: "AND: precise missing subset for {repo, read:org} with repo only", + scopeInfo: &ToolScopeInfo{ + RequiredScopes: []string{"repo", "read:org"}, + AcceptedScopes: []string{"public_repo", "read:org", "repo", "security_events", "write:org", "admin:org"}, + }, + userScopes: []string{"repo"}, expectedScopes: []string{"read:org"}, }, { @@ -168,14 +201,12 @@ func TestToolScopeInfo_MissingScopes(t *testing.T) { AcceptedScopes: []string{}, }, userScopes: []string{}, - expectedLen: 0, expectedScopes: nil, }, { name: "nil scope info - no missing", scopeInfo: nil, userScopes: []string{}, - expectedLen: 0, expectedScopes: nil, }, } @@ -183,12 +214,7 @@ func TestToolScopeInfo_MissingScopes(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { missing := tc.scopeInfo.MissingScopes(tc.userScopes...) - assert.Len(t, missing, tc.expectedLen) - if tc.expectedScopes != nil { - for _, expected := range tc.expectedScopes { - assert.Contains(t, missing, expected) - } - } + assert.Equal(t, tc.expectedScopes, missing) }) } } diff --git a/pkg/scopes/scopes.go b/pkg/scopes/scopes.go index cb1b7681a7..5b4992f5ec 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -168,28 +168,38 @@ func expandScopeSet(scopes []string) map[string]bool { return expanded } -// HasRequiredScopes checks if tokenScopes satisfy the acceptedScopes requirement. -// A tool's acceptedScopes includes both the required scopes AND parent scopes -// that implicitly grant the required permissions (via ExpandScopes). +// HasRequiredScopes reports whether tokenScopes satisfy EVERY scope in +// requiredScopes. The requiredScopes are the literal scopes a tool declares and +// they are all required: satisfaction is AND-of-ORs (conjunctive normal form). // -// For PAT filtering: if ANY of the acceptedScopes are granted by the token -// (directly or via scope hierarchy), the tool should be visible. +// The AND is over the distinct required scopes. The OR is the hierarchy: each +// required scope can be satisfied either directly or by a higher scope that +// implicitly grants it. This is implemented by expanding the TOKEN downward +// through the hierarchy (via expandScopeSet, e.g. repo -> public_repo and +// admin:org -> read:org) and checking that every required scope is present in +// that expanded set. // -// Returns true if the tool should be visible to the token holder. -func HasRequiredScopes(tokenScopes []string, acceptedScopes []string) bool { +// An empty requiredScopes is always satisfied. +// +// Each required scope currently has exactly one satisfying alternative set (the +// scope itself plus its hierarchy ancestors). If a tool ever needs true +// arbitrary alternatives (e.g. "repo OR admin:org" for a single requirement), +// the place to add it is a list-of-groups extension to RequiredScopes; that is +// deliberately not built yet (YAGNI). +func HasRequiredScopes(tokenScopes []string, requiredScopes []string) bool { // No scopes required = always allowed - if len(acceptedScopes) == 0 { + if len(requiredScopes) == 0 { return true } // Expand token scopes to include child scopes they grant - grantedScopes := expandScopeSet(tokenScopes) + granted := expandScopeSet(tokenScopes) - // Check if any accepted scope is granted by the token - for _, accepted := range acceptedScopes { - if grantedScopes[accepted] { - return true + // Every required scope must be granted by the token (directly or via hierarchy) + for _, required := range requiredScopes { + if !granted[required] { + return false } } - return false + return true } diff --git a/pkg/scopes/scopes_test.go b/pkg/scopes/scopes_test.go index b8e0d8e421..3a99494d9b 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -228,104 +228,110 @@ func TestHasRequiredScopes(t *testing.T) { tests := []struct { name string tokenScopes []string - acceptedScopes []string + requiredScopes []string expected bool }{ { - name: "no accepted scopes - always allowed", + name: "no required scopes - always allowed", tokenScopes: []string{}, - acceptedScopes: []string{}, + requiredScopes: []string{}, expected: true, }, { - name: "nil accepted scopes - always allowed", + name: "nil required scopes - always allowed", tokenScopes: []string{"repo"}, - acceptedScopes: nil, + requiredScopes: nil, expected: true, }, { name: "token has exact required scope", tokenScopes: []string{"repo"}, - acceptedScopes: []string{"repo"}, + requiredScopes: []string{"repo"}, expected: true, }, { - name: "token has parent scope that grants access", + name: "token repo satisfies public_repo (expands down)", tokenScopes: []string{"repo"}, - acceptedScopes: []string{"public_repo"}, + requiredScopes: []string{"public_repo"}, expected: true, }, { name: "token has parent scope for security_events", tokenScopes: []string{"repo"}, - acceptedScopes: []string{"security_events"}, + requiredScopes: []string{"security_events"}, expected: true, }, { name: "token has admin:org which grants read:org", tokenScopes: []string{"admin:org"}, - acceptedScopes: []string{"read:org"}, + requiredScopes: []string{"read:org"}, expected: true, }, { name: "token has write:org which grants read:org", tokenScopes: []string{"write:org"}, - acceptedScopes: []string{"read:org"}, + requiredScopes: []string{"read:org"}, expected: true, }, { name: "token missing required scope", tokenScopes: []string{"gist"}, - acceptedScopes: []string{"repo"}, + requiredScopes: []string{"repo"}, expected: false, }, { name: "token has child but not parent - fails", tokenScopes: []string{"public_repo"}, - acceptedScopes: []string{"repo"}, + requiredScopes: []string{"repo"}, expected: false, }, { - name: "multiple token scopes - one matches", - tokenScopes: []string{"gist", "repo"}, - acceptedScopes: []string{"public_repo"}, + name: "AND: repo-only token does NOT satisfy {repo, read:org}", + tokenScopes: []string{"repo"}, + requiredScopes: []string{"repo", "read:org"}, + expected: false, + }, + { + name: "AND: {repo, read:org} token satisfies {repo, read:org}", + tokenScopes: []string{"repo", "read:org"}, + requiredScopes: []string{"repo", "read:org"}, expected: true, }, { - name: "multiple accepted scopes - token has one", - tokenScopes: []string{"repo"}, - acceptedScopes: []string{"repo", "admin:org"}, + name: "AND: {repo, admin:org} satisfies {repo, read:org} via hierarchy", + tokenScopes: []string{"repo", "admin:org"}, + requiredScopes: []string{"repo", "read:org"}, expected: true, }, { name: "empty token scopes - fails when scopes required", tokenScopes: []string{}, - acceptedScopes: []string{"repo"}, + requiredScopes: []string{"repo"}, expected: false, }, { name: "user scope grants read:user", tokenScopes: []string{"user"}, - acceptedScopes: []string{"read:user"}, + requiredScopes: []string{"read:user"}, expected: true, }, { name: "user scope grants user:email", tokenScopes: []string{"user"}, - acceptedScopes: []string{"user:email"}, + requiredScopes: []string{"user:email"}, expected: true, }, { name: "write:packages grants read:packages", tokenScopes: []string{"write:packages"}, - acceptedScopes: []string{"read:packages"}, + requiredScopes: []string{"read:packages"}, expected: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := HasRequiredScopes(tt.tokenScopes, tt.acceptedScopes) + result := HasRequiredScopes(tt.tokenScopes, tt.requiredScopes) assert.Equal(t, tt.expected, result) }) } From e10fbfe1588a8b41d6c688e4da712d0973e549f1 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Fri, 26 Jun 2026 16:17:09 +0200 Subject: [PATCH 2/3] Document scope-filter limitations and fail-open posture Correct the over-claim that the scope hierarchy covers all OR cases. The hierarchy only models ancestor substitution, not sibling alternatives, so it cannot represent every way a tool may legitimately be satisfied. - Reword HasRequiredScopes doc: AND-of-ORs is ancestor-only; note the best-effort/fail-open posture and make the future CNF extension concrete using code scanning as the motivating example (security_events OR public_repo OR repo). - Note in CreateToolScopeFilter that filtering is a best-effort UX filter, not an authorization boundary (gated to ghp_ PATs, skipped when scopes can't be fetched). - Verify security tools ({security_events}) stay behavior-neutral under AND: a repo token still satisfies them; a public_repo-only token was already hidden before. Add explicit neutrality tests at the scope and filter layers. - docs/scope-filtering.md: add a Limitations and Fail-Open Posture section (sibling scopes, org roles, repo visibility) and a best-effort intro note. No tool declarations or scopes changed; no CNF structure built. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/scope-filtering.md | 16 +++++++++++++++- pkg/github/scope_filter.go | 6 ++++++ pkg/github/scope_filter_test.go | 27 +++++++++++++++++++++++++++ pkg/scopes/scopes.go | 31 ++++++++++++++++++++----------- pkg/scopes/scopes_test.go | 18 ++++++++++++++++++ 5 files changed, 86 insertions(+), 12 deletions(-) diff --git a/docs/scope-filtering.md b/docs/scope-filtering.md index f29d631ca1..c10804dee3 100644 --- a/docs/scope-filtering.md +++ b/docs/scope-filtering.md @@ -4,6 +4,8 @@ The GitHub MCP Server automatically filters available tools based on your classi > **Note:** This feature applies to **classic PATs** (tokens starting with `ghp_`). Fine-grained PATs, GitHub App installation tokens, and server-to-server tokens don't support scope detection and show all tools. +> **Important:** Scope filtering is a best-effort UX convenience, **not an authorization boundary**. The GitHub API is always the source of truth and enforces real permissions. The server therefore **fails open**: it only hides a tool when confident your token cannot use it, and shows the tool whenever access is plausible. See [Limitations and Fail-Open Posture](#limitations-and-fail-open-posture). + ## How It Works When the server starts with a classic PAT, it makes a lightweight HTTP HEAD request to the GitHub API to discover your token's scopes from the `X-OAuth-Scopes` header. Tools that require scopes your token doesn't have are automatically hidden. @@ -76,6 +78,18 @@ If the server cannot fetch your token's scopes (e.g., network issues, rate limit WARN: failed to fetch token scopes, continuing without scope filtering ``` +## Limitations and Fail-Open Posture + +Scope filtering is a **best-effort UX nicety**, not an authorization boundary. The GitHub API is the source of truth and enforces real permissions regardless of what the server shows. Because of this, the server is designed to **fail open**: it only hides a tool (or, for OAuth, issues a scope challenge) when it is confident the token cannot use it. When access is plausible, it prefers to show the tool and let the API decide. Filtering is also limited to classic PATs (`ghp_`) and is skipped entirely when scopes can't be fetched. + +A tool's declared scopes are **all required** (logical AND), and each one may be satisfied directly or by an ancestor scope from the [hierarchy](#scope-hierarchy). However, some ways a tool can legitimately be used cannot be determined from OAuth scopes alone, so the server intentionally does not try to model them: + +- **Sibling scopes outside the hierarchy.** The hierarchy only models *ancestor* substitution. For example, code scanning alerts on a **public** repository are readable with `public_repo`, which is a *sibling* of the declared `security_events` (both are children of `repo`), not an ancestor. Token expansion can't bridge siblings. Capturing this faithfully would require representing requirements as a list of OR-groups (groups AND-ed, members within a group OR-ed), e.g. code scanning = `security_events` OR `public_repo` OR `repo`. That model isn't implemented today; instead the server relies on the fail-open posture so these cases aren't wrongly hidden. +- **Organization roles.** Roles such as *security manager* grant access orthogonally to OAuth scopes and are invisible to scope detection. A user may legitimately have access the server cannot see from scopes alone. +- **Public vs. private repositories.** Whether a given scope suffices depends on the target repository's visibility, which isn't known at filter time. + +In each of these cases the server errs toward showing the tool; if the token truly lacks access, the API returns the appropriate error. + ## Classic vs Fine-Grained Personal Access Tokens **Classic PATs** (`ghp_` prefix) support OAuth scopes and return them in the `X-OAuth-Scopes` header. Scope filtering works fully with these tokens. @@ -92,7 +106,7 @@ WARN: failed to fetch token scopes, continuing without scope filtering |---------|-------|----------| | Missing expected tools | Token lacks required scope | [Edit your PAT's scopes](https://github.com/settings/tokens) in GitHub settings | | All tools visible despite limited PAT | Scope detection failed | Check logs for warnings about scope fetching | -| "Insufficient permissions" errors | Tool visible but scope insufficient | This shouldn't happen with scope filtering; report as bug | +| "Insufficient permissions" errors | Tool visible but scope insufficient | Expected in some cases (fail-open, public/private ambiguity, org roles, or scope detection skipped). The API enforces the real boundary—grant the needed scope or access | > **Tip:** You can adjust the scopes of an existing classic PAT at any time via [GitHub's token settings](https://github.com/settings/tokens). After updating scopes, restart the MCP server to pick up the changes. diff --git a/pkg/github/scope_filter.go b/pkg/github/scope_filter.go index f2cf572645..bd0884555f 100644 --- a/pkg/github/scope_filter.go +++ b/pkg/github/scope_filter.go @@ -37,6 +37,12 @@ func onlyRequiresRepoScopes(requiredScopes []string) bool { // like we can with OAuth apps. Instead, we hide tools that require scopes // the token doesn't have. // +// This is a best-effort UX filter, not an authorization boundary: the GitHub +// API still enforces real permissions. It is gated to classic ghp_ PATs and is +// skipped entirely when scopes can't be fetched, so the posture is to fail open +// (prefer showing a tool when access is plausible). See docs/scope-filtering.md +// for the known limitations (sibling scopes, org roles, repo visibility). +// // This is the recommended way to filter tools for stdio servers where the // token is known at startup and won't change during the session. // diff --git a/pkg/github/scope_filter_test.go b/pkg/github/scope_filter_test.go index f7ca158416..7460ca33b6 100644 --- a/pkg/github/scope_filter_test.go +++ b/pkg/github/scope_filter_test.go @@ -63,6 +63,15 @@ func TestCreateToolScopeFilter(t *testing.T) { RequiredScopes: []string{"repo", "read:org"}, } + // Models security tools (code scanning etc.): read-only, single {security_events}. + toolSecurityEvents := &inventory.ServerTool{ + Tool: mcp.Tool{ + Name: "list_code_scanning_alerts", + Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, + }, + RequiredScopes: []string{"security_events"}, + } + tests := []struct { name string tokenScopes []string @@ -147,6 +156,24 @@ func TestCreateToolScopeFilter(t *testing.T) { tool: toolRepoAndReadOrg, expected: true, }, + { + name: "security tool: repo token satisfies security_events (neutral)", + tokenScopes: []string{"repo"}, + tool: toolSecurityEvents, + expected: true, + }, + { + name: "security tool: security_events token shows tool", + tokenScopes: []string{"security_events"}, + tool: toolSecurityEvents, + expected: true, + }, + { + name: "security tool: public_repo (sibling) does not show tool", + tokenScopes: []string{"public_repo"}, + tool: toolSecurityEvents, + expected: false, + }, } for _, tt := range tests { diff --git a/pkg/scopes/scopes.go b/pkg/scopes/scopes.go index 5b4992f5ec..5683e3bc89 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -172,20 +172,29 @@ func expandScopeSet(scopes []string) map[string]bool { // requiredScopes. The requiredScopes are the literal scopes a tool declares and // they are all required: satisfaction is AND-of-ORs (conjunctive normal form). // -// The AND is over the distinct required scopes. The OR is the hierarchy: each -// required scope can be satisfied either directly or by a higher scope that -// implicitly grants it. This is implemented by expanding the TOKEN downward -// through the hierarchy (via expandScopeSet, e.g. repo -> public_repo and -// admin:org -> read:org) and checking that every required scope is present in -// that expanded set. +// The AND is over the distinct required scopes. The OR, for each required +// scope, is limited to the scope hierarchy: a required scope is satisfied +// directly or by one of its ANCESTOR scopes that implicitly grants it. This is +// implemented by expanding the TOKEN downward through the hierarchy (via +// expandScopeSet, e.g. repo -> public_repo and admin:org -> read:org) and +// checking that every required scope is present in that expanded set. // // An empty requiredScopes is always satisfied. // -// Each required scope currently has exactly one satisfying alternative set (the -// scope itself plus its hierarchy ancestors). If a tool ever needs true -// arbitrary alternatives (e.g. "repo OR admin:org" for a single requirement), -// the place to add it is a list-of-groups extension to RequiredScopes; that is -// deliberately not built yet (YAGNI). +// Scope filtering is a best-effort UX nicety for classic PATs, NOT an +// authorization boundary: the GitHub API remains the source of truth, so the +// intended posture is to fail open (only hide when confident the token cannot +// work). See docs/scope-filtering.md. +// +// Limitation: the hierarchy only models ancestor substitution, not sibling +// alternatives, so it does not capture every way a tool may be satisfied. For +// example, reading code scanning alerts on a PUBLIC repo is possible with +// public_repo, a sibling of the declared security_events (both children of +// repo), which token expansion cannot bridge. Representing that faithfully +// would require RequiredScopes to become a CNF list of OR-groups (groups +// AND-ed, members within a group OR-ed), e.g. code scanning = +// {security_events OR public_repo OR repo}. That structure is deliberately not +// built yet (YAGNI). func HasRequiredScopes(tokenScopes []string, requiredScopes []string) bool { // No scopes required = always allowed if len(requiredScopes) == 0 { diff --git a/pkg/scopes/scopes_test.go b/pkg/scopes/scopes_test.go index 3a99494d9b..e562f7a818 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -309,6 +309,24 @@ func TestHasRequiredScopes(t *testing.T) { requiredScopes: []string{"repo"}, expected: false, }, + { + name: "security tool: repo token satisfies security_events (neutral)", + tokenScopes: []string{"repo"}, + requiredScopes: []string{"security_events"}, + expected: true, + }, + { + name: "security tool: security_events token satisfies security_events", + tokenScopes: []string{"security_events"}, + requiredScopes: []string{"security_events"}, + expected: true, + }, + { + name: "security tool: public_repo (sibling) does NOT satisfy security_events", + tokenScopes: []string{"public_repo"}, + requiredScopes: []string{"security_events"}, + expected: false, + }, { name: "user scope grants read:user", tokenScopes: []string{"user"}, From 02e9ccf8374be6538114c66503b97f906149c2cd Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Fri, 26 Jun 2026 16:20:16 +0200 Subject: [PATCH 3/3] docs(scope-filtering): expand limitations and fail-open posture Flesh out the Limitations section: public-vs-private repo ambiguity (code scanning public_repo vs private security_events/repo), sibling-OR alternatives naming the four security tools, org roles invisible to scopes, and other token types skipped (gated to ghp_) as fail-open by design. Note the OR-groups extension point. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/scope-filtering.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/scope-filtering.md b/docs/scope-filtering.md index c10804dee3..52c2f95a2c 100644 --- a/docs/scope-filtering.md +++ b/docs/scope-filtering.md @@ -80,15 +80,16 @@ WARN: failed to fetch token scopes, continuing without scope filtering ## Limitations and Fail-Open Posture -Scope filtering is a **best-effort UX nicety**, not an authorization boundary. The GitHub API is the source of truth and enforces real permissions regardless of what the server shows. Because of this, the server is designed to **fail open**: it only hides a tool (or, for OAuth, issues a scope challenge) when it is confident the token cannot use it. When access is plausible, it prefers to show the tool and let the API decide. Filtering is also limited to classic PATs (`ghp_`) and is skipped entirely when scopes can't be fetched. +Scope filtering is a **best-effort UX convenience** for classic PATs (`ghp_`) only. It is **NOT an authorization boundary** — the GitHub API is the source of truth and enforces real permissions regardless of what the server shows. The server therefore **fails open**: when access is plausible but unprovable at filter/challenge time, the tool is shown rather than hidden. -A tool's declared scopes are **all required** (logical AND), and each one may be satisfied directly or by an ancestor scope from the [hierarchy](#scope-hierarchy). However, some ways a tool can legitimately be used cannot be determined from OAuth scopes alone, so the server intentionally does not try to model them: +A tool's declared scopes are **all required** (logical AND), and each one may be satisfied directly or by an ancestor scope from the [hierarchy](#scope-hierarchy). Some ways a tool can legitimately be used cannot be determined from OAuth scopes alone, so the scope model intentionally does not fully capture them: -- **Sibling scopes outside the hierarchy.** The hierarchy only models *ancestor* substitution. For example, code scanning alerts on a **public** repository are readable with `public_repo`, which is a *sibling* of the declared `security_events` (both are children of `repo`), not an ancestor. Token expansion can't bridge siblings. Capturing this faithfully would require representing requirements as a list of OR-groups (groups AND-ed, members within a group OR-ed), e.g. code scanning = `security_events` OR `public_repo` OR `repo`. That model isn't implemented today; instead the server relies on the fail-open posture so these cases aren't wrongly hidden. -- **Organization roles.** Roles such as *security manager* grant access orthogonally to OAuth scopes and are invisible to scope detection. A user may legitimately have access the server cannot see from scopes alone. -- **Public vs. private repositories.** Whether a given scope suffices depends on the target repository's visibility, which isn't known at filter time. +- **Public vs. private repositories.** Which scope suffices can depend on the target repository, which isn't known when tools are filtered. For example, code scanning alerts on **public** repos are readable with `public_repo`, while **private** repos need `security_events` (or `repo`). +- **Sibling-OR alternatives.** `security_events` and `public_repo` are *siblings* under `repo` (not parent/child), so token hierarchy expansion can't treat one as satisfying the other. A `public_repo`-only token may therefore have the security tools (code scanning, secret scanning, Dependabot, security advisories) hidden even though it could read public-repo data. +- **Organization roles.** A *security manager* (or similar) org role grants access orthogonally to OAuth scopes and is invisible to scope filtering. +- **Other token types.** Fine-grained PATs, OAuth, and GitHub App tokens use different permission models; filtering is skipped for them entirely (gated to `ghp_`), which is fail-open by design. -In each of these cases the server errs toward showing the tool; if the token truly lacks access, the API returns the appropriate error. +These cases are deferred to runtime API enforcement. If precise sibling-OR modeling is ever needed, the extension point is making the required scopes a list of OR-groups (AND across groups, OR within a group) — deliberately not built yet. ## Classic vs Fine-Grained Personal Access Tokens