Skip to content

Commit b769a55

Browse files
committed
Address direct tool scope review feedback
1 parent 86664c8 commit b769a55

2 files changed

Lines changed: 108 additions & 7 deletions

File tree

internal/server/mcp_direct_scope.go

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,14 @@ import (
1111
)
1212

1313
func requiredPermissionForDirectTool(annotations *config.ToolAnnotations) string {
14-
requiredVariant := contracts.DeriveCallWith(annotations)
15-
requiredPerm := contracts.ToolVariantToOperationType[requiredVariant]
16-
if requiredPerm == "" {
17-
return contracts.OperationTypeRead
14+
switch contracts.DeriveCallWith(annotations) {
15+
case contracts.ToolVariantWrite:
16+
return auth.PermWrite
17+
case contracts.ToolVariantDestructive:
18+
return auth.PermDestructive
19+
default:
20+
return auth.PermRead
1821
}
19-
return requiredPerm
2022
}
2123

2224
func (p *MCPProxyServer) setDirectToolPermissions(perms map[string]string) {
@@ -28,7 +30,11 @@ func (p *MCPProxyServer) setDirectToolPermissions(perms map[string]string) {
2830
return
2931
}
3032

31-
p.directToolPerms = perms
33+
copied := make(map[string]string, len(perms))
34+
for name, perm := range perms {
35+
copied[name] = perm
36+
}
37+
p.directToolPerms = copied
3238
}
3339

3440
func (p *MCPProxyServer) lookupDirectToolPermission(directName string) (string, bool) {
@@ -56,7 +62,7 @@ func (p *MCPProxyServer) filterDirectModeToolsForAuth(ctx context.Context, tools
5662
return tools
5763
}
5864

59-
filtered := tools[:0]
65+
filtered := make([]mcp.Tool, 0, len(tools))
6066
for _, tool := range tools {
6167
serverName, _, ok := ParseDirectToolName(tool.Name)
6268
if !ok {

internal/server/mcp_routing_test.go

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,101 @@ func TestDirectModeHandler_DestructiveToolNeedsDestructivePermission(t *testing.
366366
assert.Contains(t, result.Content[0].(mcp.TextContent).Text, "destructive")
367367
}
368368

369+
func TestRequiredPermissionForDirectTool_MapsAnnotationsToAuthPermissions(t *testing.T) {
370+
readOnly := true
371+
write := false
372+
destructive := true
373+
374+
tests := []struct {
375+
name string
376+
annotations *config.ToolAnnotations
377+
want string
378+
}{
379+
{
380+
name: "nil annotations default to read",
381+
want: auth.PermRead,
382+
},
383+
{
384+
name: "read only hint maps to read",
385+
annotations: &config.ToolAnnotations{
386+
ReadOnlyHint: &readOnly,
387+
},
388+
want: auth.PermRead,
389+
},
390+
{
391+
name: "read only false maps to write",
392+
annotations: &config.ToolAnnotations{
393+
ReadOnlyHint: &write,
394+
},
395+
want: auth.PermWrite,
396+
},
397+
{
398+
name: "destructive hint maps to destructive",
399+
annotations: &config.ToolAnnotations{
400+
DestructiveHint: &destructive,
401+
},
402+
want: auth.PermDestructive,
403+
},
404+
{
405+
name: "destructive hint takes precedence over read only hint",
406+
annotations: &config.ToolAnnotations{
407+
ReadOnlyHint: &readOnly,
408+
DestructiveHint: &destructive,
409+
},
410+
want: auth.PermDestructive,
411+
},
412+
}
413+
414+
for _, tt := range tests {
415+
t.Run(tt.name, func(t *testing.T) {
416+
assert.Equal(t, tt.want, requiredPermissionForDirectTool(tt.annotations))
417+
})
418+
}
419+
}
420+
421+
func TestSetDirectToolPermissions_DefensivelyCopiesMap(t *testing.T) {
422+
proxy := &MCPProxyServer{}
423+
toolName := FormatDirectToolName("github", "get_issue")
424+
perms := map[string]string{
425+
toolName: auth.PermRead,
426+
}
427+
428+
proxy.setDirectToolPermissions(perms)
429+
perms[toolName] = auth.PermDestructive
430+
431+
got, ok := proxy.lookupDirectToolPermission(toolName)
432+
require.True(t, ok)
433+
assert.Equal(t, auth.PermRead, got)
434+
}
435+
436+
func TestFilterDirectModeToolsForAuth_DoesNotMutateInputSlice(t *testing.T) {
437+
proxy := &MCPProxyServer{}
438+
allowed := FormatDirectToolName("github", "get_issue")
439+
denied := FormatDirectToolName("gitlab", "get_issue")
440+
tools := []mcp.Tool{
441+
{Name: allowed},
442+
{Name: denied},
443+
}
444+
original := append([]mcp.Tool(nil), tools...)
445+
446+
proxy.setDirectToolPermissions(map[string]string{
447+
allowed: auth.PermRead,
448+
denied: auth.PermRead,
449+
})
450+
451+
ctx := auth.WithAuthContext(context.Background(), &auth.AuthContext{
452+
Type: auth.AuthTypeAgent,
453+
AgentName: "test-agent",
454+
AllowedServers: []string{"github"},
455+
Permissions: []string{auth.PermRead},
456+
})
457+
458+
filtered := proxy.filterDirectModeToolsForAuth(ctx, tools)
459+
460+
assert.Equal(t, []string{allowed}, directToolNamesForTest(filtered))
461+
assert.Equal(t, original, tools)
462+
}
463+
369464
func TestFilterDirectModeToolsForAuth_AgentServerAndPermissionScope(t *testing.T) {
370465
proxy := &MCPProxyServer{}
371466

0 commit comments

Comments
 (0)