Skip to content

Commit c285c6a

Browse files
fix(http): preserve tool and scope restrictions
Apply static allowlists before removing unavailable tools and fail closed on invalid configured tool names. Model independent OAuth requirements as conjunctive groups so repository deletion requires both delete_repo and repo. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
1 parent 8eba6fd commit c285c6a

13 files changed

Lines changed: 142 additions & 15 deletions

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1309,7 +1309,7 @@ The following sets of tools are available:
13091309
- `repo`: Repository name (string, required)
13101310

13111311
- **delete_repository** - Delete repository
1312-
- **Required OAuth Scopes**: `delete_repo`
1312+
- **Required OAuth Scopes (any of)**: `delete_repo`, `repo`
13131313
- `owner`: Repository owner (username or organization) (string, required)
13141314
- `repo`: Repository name (string, required)
13151315

pkg/github/dependencies.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,7 @@ func NewTool[In, Out any](
245245
})
246246
st.RequiredScopes = scopes.ToStringSlice(requiredScopes...)
247247
st.AcceptedScopes = scopes.ExpandScopes(requiredScopes...)
248+
st.RequiredScopeGroups = scopes.ExpandScopeGroups(requiredScopes...)
248249
return st
249250
}
250251

@@ -268,6 +269,7 @@ func NewToolFromHandler(
268269
})
269270
st.RequiredScopes = scopes.ToStringSlice(requiredScopes...)
270271
st.AcceptedScopes = scopes.ExpandScopes(requiredScopes...)
272+
st.RequiredScopeGroups = scopes.ExpandScopeGroups(requiredScopes...)
271273
return st
272274
}
273275

pkg/github/repositories.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -747,7 +747,7 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool
747747
Required: []string{"owner", "repo"},
748748
},
749749
},
750-
[]scopes.Scope{scopes.DeleteRepo},
750+
[]scopes.Scope{scopes.DeleteRepo, scopes.Repo},
751751
func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
752752
owner, err := RequiredParam[string](args, "owner")
753753
if err != nil {

pkg/github/repositories_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3002,7 +3002,8 @@ func Test_DeleteRepository(t *testing.T) {
30023002
assert.True(t, *tool.Annotations.DestructiveHint)
30033003
assert.Equal(t, inventory.ProtocolVersionMultiRoundTrip, serverTool.MinimumProtocolVersion)
30043004
assert.Equal(t, inventory.ElicitationModeForm, serverTool.RequiredElicitationMode)
3005-
assert.Equal(t, []string{string(scopes.DeleteRepo)}, serverTool.RequiredScopes)
3005+
assert.ElementsMatch(t, []string{string(scopes.DeleteRepo), string(scopes.Repo)}, serverTool.RequiredScopes)
3006+
assert.Len(t, serverTool.RequiredScopeGroups, 2)
30063007

30073008
t.Run("requests exact repository name through elicitation", func(t *testing.T) {
30083009
result := invokeDeleteRepository(t, serverTool, NewMockedHTTPClient(), nil)

pkg/github/scope_filter.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ func CreateToolScopeFilter(tokenScopes []string) inventory.ToolFilter {
5959
if tool.Tool.Annotations != nil && tool.Tool.Annotations.ReadOnlyHint && onlyRequiresRepoScopes(tool.AcceptedScopes) {
6060
return true, nil
6161
}
62+
if len(tool.RequiredScopeGroups) > 0 {
63+
return scopes.HasRequiredScopeGroups(tokenScopes, tool.RequiredScopeGroups), nil
64+
}
6265
return scopes.HasRequiredScopes(tokenScopes, tool.AcceptedScopes), nil
6366
}
6467
}

pkg/github/scope_filter_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,12 @@ func TestCreateToolScopeFilter(t *testing.T) {
5858
AcceptedScopes: []string{"repo", "admin:org"},
5959
}
6060

61+
toolConjunctiveScopes := &inventory.ServerTool{
62+
Tool: mcp.Tool{Name: "conjunctive_scope_tool"},
63+
AcceptedScopes: []string{"delete_repo", "repo"},
64+
RequiredScopeGroups: [][]string{{"delete_repo"}, {"repo"}},
65+
}
66+
6167
tests := []struct {
6268
name string
6369
tokenScopes []string
@@ -130,6 +136,18 @@ func TestCreateToolScopeFilter(t *testing.T) {
130136
tool: toolPublicRepoScope,
131137
expected: true,
132138
},
139+
{
140+
name: "token must satisfy every required scope group",
141+
tokenScopes: []string{"delete_repo"},
142+
tool: toolConjunctiveScopes,
143+
expected: false,
144+
},
145+
{
146+
name: "token satisfying every required scope group can see tool",
147+
tokenScopes: []string{"delete_repo", "repo"},
148+
tool: toolConjunctiveScopes,
149+
expected: true,
150+
},
133151
}
134152

135153
for _, tt := range tests {

pkg/http/handler.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -360,14 +360,17 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun
360360
}
361361
opts := []github.ToolOption{github.WithHost(hostType)}
362362
tools := github.AllTools(t, opts...)
363-
if cfg.disableDeleteRepository {
364-
tools = slices.DeleteFunc(tools, func(tool inventory.ServerTool) bool {
363+
filterUnavailable := func(tools []inventory.ServerTool) []inventory.ServerTool {
364+
if !cfg.disableDeleteRepository {
365+
return tools
366+
}
367+
return slices.DeleteFunc(tools, func(tool inventory.ServerTool) bool {
365368
return tool.Tool.Name == github.DeleteRepositoryToolName
366369
})
367370
}
368371

369372
if !hasStaticConfig(cfg) {
370-
return tools, github.AllResources(t), github.AllPrompts(t)
373+
return filterUnavailable(tools), github.AllResources(t), github.AllPrompts(t)
371374
}
372375

373376
b := inventory.NewBuilder().
@@ -387,13 +390,13 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun
387390

388391
inv, err := b.Build()
389392
if err != nil {
390-
// Fall back to all tools if there's an error (e.g. unknown tool names).
391-
// The error will surface again at per-request time if relevant.
392-
return tools, github.AllResources(t), github.AllPrompts(t)
393+
// Invalid static tool names must fail closed rather than widening an
394+
// explicit allowlist to every tool.
395+
return nil, github.AllResources(t), github.AllPrompts(t)
393396
}
394397

395398
ctx := context.Background()
396-
return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx)
399+
return filterUnavailable(inv.AvailableTools(ctx)), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx)
397400
}
398401

399402
// InventoryFiltersForRequest applies filters to the inventory builder

pkg/http/handler_test.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -682,9 +682,7 @@ func TestStaticInventoryFallbackKeepsDeleteRepositoryDisabled(t *testing.T) {
682682
}
683683
tools, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper)
684684

685-
for _, tool := range tools {
686-
assert.NotEqual(t, github.DeleteRepositoryToolName, tool.Tool.Name)
687-
}
685+
assert.Empty(t, tools, "an unavailable explicit allowlist must not widen to other tools")
688686
}
689687

690688
// TestContentTypeHandling verifies that the MCP StreamableHTTP handler

pkg/inventory/server_tool.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,10 @@ type ServerTool struct {
9797
// This includes the required scopes plus any higher-level scopes that provide
9898
// the necessary permissions due to scope hierarchy.
9999
AcceptedScopes []string
100+
101+
// RequiredScopeGroups contains one group of accepted alternatives for each
102+
// independently required OAuth scope. Every group must be satisfied.
103+
RequiredScopeGroups [][]string
100104
}
101105

102106
// IsReadOnly returns true if this tool is marked as read-only via annotations.

pkg/scopes/map.go

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ type ToolScopeInfo struct {
1212

1313
// AcceptedScopes contains all scopes that satisfy the requirements (including parent scopes).
1414
AcceptedScopes []string
15+
16+
// RequiredScopeGroups contains accepted alternatives for each independently
17+
// required scope. Every group must be satisfied.
18+
RequiredScopeGroups [][]string
1519
}
1620

1721
// globalToolScopeMap is populated from inventory when SetToolScopeMapFromInventory is called
@@ -59,8 +63,9 @@ func GetToolScopeMapFromInventory(inv *inventory.Inventory) ToolScopeMap {
5963
tool := &allTools[i]
6064
if len(tool.RequiredScopes) > 0 || len(tool.AcceptedScopes) > 0 {
6165
result[tool.Tool.Name] = &ToolScopeInfo{
62-
RequiredScopes: tool.RequiredScopes,
63-
AcceptedScopes: tool.AcceptedScopes,
66+
RequiredScopes: tool.RequiredScopes,
67+
AcceptedScopes: tool.AcceptedScopes,
68+
RequiredScopeGroups: tool.RequiredScopeGroups,
6469
}
6570
}
6671
}
@@ -70,6 +75,9 @@ func GetToolScopeMapFromInventory(inv *inventory.Inventory) ToolScopeMap {
7075

7176
// HasAcceptedScope checks if any of the provided user scopes satisfy the tool's requirements.
7277
func (t *ToolScopeInfo) HasAcceptedScope(userScopes ...string) bool {
78+
if t != nil && len(t.RequiredScopeGroups) > 0 {
79+
return HasRequiredScopeGroups(userScopes, t.RequiredScopeGroups)
80+
}
7381
if t == nil || len(t.AcceptedScopes) == 0 {
7482
return true // No scopes required
7583
}
@@ -99,6 +107,24 @@ func (t *ToolScopeInfo) MissingScopes(userScopes ...string) []string {
99107
userScopeSet[s] = true
100108
}
101109

110+
if len(t.RequiredScopeGroups) > 0 {
111+
userScopeSet := expandScopeSet(userScopes)
112+
var missing []string
113+
for i, group := range t.RequiredScopeGroups {
114+
satisfied := false
115+
for _, scope := range group {
116+
if userScopeSet[scope] {
117+
satisfied = true
118+
break
119+
}
120+
}
121+
if !satisfied && i < len(t.RequiredScopes) {
122+
missing = append(missing, t.RequiredScopes[i])
123+
}
124+
}
125+
return missing
126+
}
127+
102128
// Check if any accepted scope is present
103129
hasAccepted := false
104130
for _, scope := range t.AcceptedScopes {

0 commit comments

Comments
 (0)