Skip to content

Commit 0de95e0

Browse files
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>
1 parent 0503f2f commit 0de95e0

12 files changed

Lines changed: 208 additions & 138 deletions

File tree

README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -922,13 +922,13 @@ The following sets of tools are available:
922922
- `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)
923923

924924
- **list_issue_fields** - List issue fields
925-
- **Required OAuth Scopes (any of)**: `repo`, `read:org`
925+
- **Required OAuth Scopes**: `repo`, `read:org`
926926
- **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org`
927927
- `owner`: The account owner of the repository or organization. The name is not case sensitive. (string, required)
928928
- `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)
929929

930930
- **list_issue_types** - List available issue types
931-
- **Required OAuth Scopes (any of)**: `repo`, `read:org`
931+
- **Required OAuth Scopes**: `repo`, `read:org`
932932
- **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org`
933933
- `owner`: The account owner of the repository or organization. (string, required)
934934
- `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)

cmd/github-mcp-server/feature_flag_docs.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,14 @@ func generateFlaggedToolsDoc(flags []string, emptyMessage string) string {
6363
if !hasAny {
6464
return emptyMessage
6565
}
66+
// Clarify scope semantics for the rendered tools: every listed required
67+
// scope is needed (AND), and a higher scope in the hierarchy also satisfies
68+
// a required scope.
69+
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"
6670
// Leading/trailing newlines around the body produce blank lines between
6771
// our content and the surrounding marker comments, so the trailing comment
6872
// doesn't get absorbed into the final list item by markdown renderers.
69-
return "\n" + strings.TrimSuffix(buf.String(), "\n") + "\n"
73+
return "\n" + preamble + strings.TrimSuffix(buf.String(), "\n") + "\n"
7074
}
7175

7276
// flaggedToolDiff returns the tools whose definition (input schema or meta)

cmd/github-mcp-server/generate_docs.go

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -221,15 +221,11 @@ func writeToolDoc(buf *strings.Builder, tool inventory.ServerTool) {
221221

222222
// OAuth scopes if present
223223
if len(tool.RequiredScopes) > 0 {
224-
// Scope filtering uses "any of" semantics (see scopes.HasRequiredScopes),
225-
// so when multiple required scopes are listed, render them as alternatives
226-
// rather than implying all are required.
224+
// All listed required scopes are needed (AND). A higher scope in the
225+
// hierarchy also satisfies a required scope (see scopes.HasRequiredScopes).
226+
// AcceptedScopes below is non-authoritative display metadata.
227227
scopeList := "`" + strings.Join(tool.RequiredScopes, "`, `") + "`"
228-
if len(tool.RequiredScopes) > 1 {
229-
fmt.Fprintf(buf, " - **Required OAuth Scopes (any of)**: %s\n", scopeList)
230-
} else {
231-
fmt.Fprintf(buf, " - **Required OAuth Scopes**: %s\n", scopeList)
232-
}
228+
fmt.Fprintf(buf, " - **Required OAuth Scopes**: %s\n", scopeList)
233229

234230
// Only show accepted scopes if they differ from required scopes
235231
if len(tool.AcceptedScopes) > 0 && !scopesEqual(tool.RequiredScopes, tool.AcceptedScopes) {

docs/feature-flags.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ runtime behavior (such as output formatting) won't appear here.
3232

3333
<!-- START AUTOMATED FEATURE FLAG TOOLS -->
3434

35+
> **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.
36+
3537
### `remote_mcp_ui_apps`
3638

3739
- **create_pull_request** - Open new pull request
@@ -74,7 +76,7 @@ runtime behavior (such as output formatting) won't appear here.
7476
- `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)
7577

7678
- **ui_get** - Get UI data
77-
- **Required OAuth Scopes (any of)**: `repo`, `read:org`
79+
- **Required OAuth Scopes**: `repo`, `read:org`
7880
- **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org`
7981
- `method`: The type of data to fetch (string, required)
8082
- `owner`: Repository owner (required for all methods) (string, required)

docs/insiders-features.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ The list below is generated from the Go source. It covers tool **inventory and s
2626

2727
<!-- START AUTOMATED INSIDERS TOOLS -->
2828

29+
> **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.
30+
2931
### `remote_mcp_ui_apps`
3032

3133
- **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
6870
- `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)
6971

7072
- **ui_get** - Get UI data
71-
- **Required OAuth Scopes (any of)**: `repo`, `read:org`
73+
- **Required OAuth Scopes**: `repo`, `read:org`
7274
- **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org`
7375
- `method`: The type of data to fetch (string, required)
7476
- `owner`: Repository owner (required for all methods) (string, required)

pkg/github/scope_filter.go

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,14 @@ var repoScopesSet = map[string]bool{
1515
string(scopes.PublicRepo): true,
1616
}
1717

18-
// onlyRequiresRepoScopes returns true if all of the tool's accepted scopes
18+
// onlyRequiresRepoScopes returns true if all of the tool's required scopes
1919
// are repo-related scopes (repo, public_repo). Such tools work on public
2020
// repositories without needing any scope.
21-
func onlyRequiresRepoScopes(acceptedScopes []string) bool {
22-
if len(acceptedScopes) == 0 {
21+
func onlyRequiresRepoScopes(requiredScopes []string) bool {
22+
if len(requiredScopes) == 0 {
2323
return false
2424
}
25-
for _, scope := range acceptedScopes {
25+
for _, scope := range requiredScopes {
2626
if !repoScopesSet[scope] {
2727
return false
2828
}
@@ -41,9 +41,13 @@ func onlyRequiresRepoScopes(acceptedScopes []string) bool {
4141
// token is known at startup and won't change during the session.
4242
//
4343
// The filter returns true (include tool) if:
44-
// - The tool has no scope requirements (AcceptedScopes is empty)
44+
// - The tool has no scope requirements (RequiredScopes is empty)
4545
// - The tool is read-only and only requires repo/public_repo scopes (works on public repos)
46-
// - The token has at least one of the tool's accepted scopes
46+
// - The token satisfies ALL of the tool's required scopes (AND-of-ORs, where
47+
// each required scope may be met directly or by a higher scope)
48+
//
49+
// RequiredScopes is the single source of truth here; AcceptedScopes is
50+
// display-only metadata and is intentionally not consulted.
4751
//
4852
// Example usage:
4953
//
@@ -55,10 +59,11 @@ func onlyRequiresRepoScopes(acceptedScopes []string) bool {
5559
// inventory := github.NewInventory(t).WithFilter(filter).Build()
5660
func CreateToolScopeFilter(tokenScopes []string) inventory.ToolFilter {
5761
return func(_ context.Context, tool *inventory.ServerTool) (bool, error) {
58-
// Read-only tools requiring only repo/public_repo work on public repos without any scope
59-
if tool.Tool.Annotations != nil && tool.Tool.Annotations.ReadOnlyHint && onlyRequiresRepoScopes(tool.AcceptedScopes) {
62+
// Read-only tools requiring only repo/public_repo work on public repos without any scope.
63+
// Tools that also require a non-repo scope (e.g. {repo, read:org}) fall through to the AND check.
64+
if tool.Tool.Annotations != nil && tool.Tool.Annotations.ReadOnlyHint && onlyRequiresRepoScopes(tool.RequiredScopes) {
6065
return true, nil
6166
}
62-
return scopes.HasRequiredScopes(tokenScopes, tool.AcceptedScopes), nil
67+
return scopes.HasRequiredScopes(tokenScopes, tool.RequiredScopes), nil
6368
}
6469
}

pkg/github/scope_filter_test.go

Lines changed: 45 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,51 +11,56 @@ import (
1111
)
1212

1313
func TestCreateToolScopeFilter(t *testing.T) {
14-
// Create test tools with various scope requirements
14+
// Create test tools with various scope requirements.
15+
// RequiredScopes is the single source of truth for filtering.
1516
toolNoScopes := &inventory.ServerTool{
1617
Tool: mcp.Tool{Name: "no_scopes_tool"},
17-
AcceptedScopes: nil,
18+
RequiredScopes: nil,
1819
}
1920

2021
toolEmptyScopes := &inventory.ServerTool{
2122
Tool: mcp.Tool{Name: "empty_scopes_tool"},
22-
AcceptedScopes: []string{},
23+
RequiredScopes: []string{},
2324
}
2425

2526
toolRepoScope := &inventory.ServerTool{
2627
Tool: mcp.Tool{Name: "repo_tool"},
27-
AcceptedScopes: []string{"repo"},
28+
RequiredScopes: []string{"repo"},
2829
}
2930

3031
toolRepoScopeReadOnly := &inventory.ServerTool{
3132
Tool: mcp.Tool{
3233
Name: "repo_tool_readonly",
3334
Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true},
3435
},
35-
AcceptedScopes: []string{"repo"},
36+
RequiredScopes: []string{"repo"},
3637
}
3738

3839
toolPublicRepoScope := &inventory.ServerTool{
3940
Tool: mcp.Tool{Name: "public_repo_tool"},
40-
AcceptedScopes: []string{"public_repo", "repo"}, // repo is parent, also accepted
41+
RequiredScopes: []string{"public_repo"},
4142
}
4243

4344
toolPublicRepoScopeReadOnly := &inventory.ServerTool{
4445
Tool: mcp.Tool{
4546
Name: "public_repo_tool_readonly",
4647
Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true},
4748
},
48-
AcceptedScopes: []string{"public_repo", "repo"},
49+
RequiredScopes: []string{"public_repo"},
4950
}
5051

5152
toolGistScope := &inventory.ServerTool{
5253
Tool: mcp.Tool{Name: "gist_tool"},
53-
AcceptedScopes: []string{"gist"},
54+
RequiredScopes: []string{"gist"},
5455
}
5556

56-
toolMultiScope := &inventory.ServerTool{
57-
Tool: mcp.Tool{Name: "multi_scope_tool"},
58-
AcceptedScopes: []string{"repo", "admin:org"},
57+
// Models ui_get / list_issue_fields: read-only, but requires repo AND read:org.
58+
toolRepoAndReadOrg := &inventory.ServerTool{
59+
Tool: mcp.Tool{
60+
Name: "ui_get",
61+
Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true},
62+
},
63+
RequiredScopes: []string{"repo", "read:org"},
5964
}
6065

6166
tests := []struct {
@@ -100,12 +105,6 @@ func TestCreateToolScopeFilter(t *testing.T) {
100105
tool: toolGistScope,
101106
expected: false,
102107
},
103-
{
104-
name: "token with one of multiple accepted scopes can see tool",
105-
tokenScopes: []string{"admin:org"},
106-
tool: toolMultiScope,
107-
expected: true,
108-
},
109108
{
110109
name: "empty token scopes cannot see scoped tools",
111110
tokenScopes: []string{},
@@ -130,6 +129,24 @@ func TestCreateToolScopeFilter(t *testing.T) {
130129
tool: toolPublicRepoScope,
131130
expected: true,
132131
},
132+
{
133+
name: "AND: repo-only classic PAT hides {repo, read:org} tool",
134+
tokenScopes: []string{"repo"},
135+
tool: toolRepoAndReadOrg,
136+
expected: false,
137+
},
138+
{
139+
name: "AND: {repo, read:org} token shows {repo, read:org} tool",
140+
tokenScopes: []string{"repo", "read:org"},
141+
tool: toolRepoAndReadOrg,
142+
expected: true,
143+
},
144+
{
145+
name: "AND: {repo, admin:org} token shows {repo, read:org} tool via hierarchy",
146+
tokenScopes: []string{"repo", "admin:org"},
147+
tool: toolRepoAndReadOrg,
148+
expected: true,
149+
},
133150
}
134151

135152
for _, tt := range tests {
@@ -149,17 +166,23 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) {
149166
{
150167
Tool: mcp.Tool{Name: "public_tool"},
151168
Toolset: inventory.ToolsetMetadata{ID: "test"},
152-
AcceptedScopes: nil, // No scopes required
169+
RequiredScopes: nil, // No scopes required
153170
},
154171
{
155172
Tool: mcp.Tool{Name: "repo_tool"},
156173
Toolset: inventory.ToolsetMetadata{ID: "test"},
157-
AcceptedScopes: []string{"repo"},
174+
RequiredScopes: []string{"repo"},
158175
},
159176
{
160177
Tool: mcp.Tool{Name: "gist_tool"},
161178
Toolset: inventory.ToolsetMetadata{ID: "test"},
162-
AcceptedScopes: []string{"gist"},
179+
RequiredScopes: []string{"gist"},
180+
},
181+
{
182+
// Requires repo AND read:org; hidden for a {repo}-only token.
183+
Tool: mcp.Tool{Name: "list_issue_fields"},
184+
Toolset: inventory.ToolsetMetadata{ID: "test"},
185+
RequiredScopes: []string{"repo", "read:org"},
163186
},
164187
}
165188

@@ -177,7 +200,7 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) {
177200
// Get available tools
178201
availableTools := inv.AvailableTools(context.Background())
179202

180-
// Should see public_tool and repo_tool, but not gist_tool
203+
// Should see public_tool and repo_tool, but not gist_tool or list_issue_fields
181204
assert.Len(t, availableTools, 2)
182205

183206
toolNames := make([]string, len(availableTools))
@@ -188,4 +211,5 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) {
188211
assert.Contains(t, toolNames, "public_tool")
189212
assert.Contains(t, toolNames, "repo_tool")
190213
assert.NotContains(t, toolNames, "gist_tool")
214+
assert.NotContains(t, toolNames, "list_issue_fields")
191215
}

pkg/http/middleware/scope_challenge.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -109,31 +109,31 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter
109109
ctx = ghcontext.WithTokenScopes(ctx, activeScopes)
110110
r = r.WithContext(ctx)
111111

112-
// Check if user has the required scopes
113-
if toolScopeInfo.HasAcceptedScope(activeScopes...) {
112+
// Check if user satisfies all required scopes
113+
if toolScopeInfo.Satisfies(activeScopes...) {
114114
next.ServeHTTP(w, r)
115115
return
116116
}
117117

118-
// User lacks required scopes - get the scopes they need
119-
requiredScopes := toolScopeInfo.GetRequiredScopesSlice()
118+
// User lacks one or more required scopes - request only the missing ones
119+
missingScopes := toolScopeInfo.MissingScopes(activeScopes...)
120120

121121
// Build the resource metadata URL using the shared utility
122122
// GetEffectiveResourcePath returns the original path (e.g., /mcp or /mcp/x/all)
123123
// which is used to construct the well-known OAuth protected resource URL
124124
resourcePath := oauth.ResolveResourcePath(r, oauthCfg)
125125
resourceMetadataURL := oauth.BuildResourceMetadataURL(r, oauthCfg, resourcePath)
126126

127-
// Build recommended scopes: existing scopes + required scopes
128-
recommendedScopes := make([]string, 0, len(activeScopes)+len(requiredScopes))
127+
// Build recommended scopes: existing scopes + missing required scopes
128+
recommendedScopes := make([]string, 0, len(activeScopes)+len(missingScopes))
129129
recommendedScopes = append(recommendedScopes, activeScopes...)
130-
recommendedScopes = append(recommendedScopes, requiredScopes...)
130+
recommendedScopes = append(recommendedScopes, missingScopes...)
131131

132132
// Build the WWW-Authenticate header value
133133
wwwAuthenticateHeader := fmt.Sprintf(`Bearer error="insufficient_scope", scope=%q, resource_metadata=%q, error_description=%q`,
134134
strings.Join(recommendedScopes, " "),
135135
resourceMetadataURL,
136-
"Additional scopes required: "+strings.Join(requiredScopes, ", "),
136+
"Additional scopes required: "+strings.Join(missingScopes, ", "),
137137
)
138138

139139
// Send scope challenge response with the superset of existing and required scopes

0 commit comments

Comments
 (0)