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/docs/scope-filtering.md b/docs/scope-filtering.md index f29d631ca1..52c2f95a2c 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,19 @@ 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 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). 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: + +- **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. + +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 **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 +107,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 42f8e98b0c..bd0884555f 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 } @@ -37,13 +37,23 @@ func onlyRequiresRepoScopes(acceptedScopes []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. // // 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 +65,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..7460ca33b6 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,30 @@ 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"}, + } + + // 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 { @@ -100,12 +114,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 +138,42 @@ 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, + }, + { + 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 { @@ -149,17 +193,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 +227,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 +238,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..5683e3bc89 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -168,28 +168,47 @@ 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, 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. // -// 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. +// +// 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(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..e562f7a818 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -228,104 +228,128 @@ 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: "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"}, - 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) }) }