diff --git a/internal/server/mcp_describe_direct_test.go b/internal/server/mcp_describe_direct_test.go index 267611969..96eddcb5b 100644 --- a/internal/server/mcp_describe_direct_test.go +++ b/internal/server/mcp_describe_direct_test.go @@ -302,6 +302,69 @@ func TestDescribeDirect_ServerScopeGate(t *testing.T) { assert.Equal(t, describeErrNotFound, byID["github__read_file"]["error"]) } +// TestDescribeDirect_ServerScopeGate_UserType is TestDescribeDirect_ServerScopeGate's +// Spec 105 PR G regression: describe's isScopeRestrictedCaller gate must +// scope-check a server-edition OAuth "user" exactly like an agent token, not +// wave it through as an unrestricted (admin-like) caller — see +// directEntryVisibleToSession, which must stay in parity with the listing +// gate this same fix landed in (mcp_direct_scope.go). +func TestDescribeDirect_ServerScopeGate_UserType(t *testing.T) { + p := newDirectDescribeProxy(t) + + elsewhere := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeUser, + UserID: "u-other", + AllowedServers: []string{"gitlab"}, + }) + + resp := callDescribeDirect(t, p, elsewhere, []interface{}{"github__read_file"}) + assert.Empty(t, resp.Definitions) + byID := describeErrorsByID(resp) + require.Contains(t, byID, "github__read_file") + assert.Equal(t, describeErrNotFound, byID["github__read_file"]["error"]) +} + +// TestDescribeDirect_CallabilityGate_UserType is the describe-time half of +// TestFilterDirectToolsForAgentCallability_UserTypeIsScopeRestrictedToo +// (mcp_direct_callability_test.go), which only proved the listing side of +// the fix. directEntryVisibleToSession runs directEntryInScope BEFORE +// directEntryCallable, so a caller scoped OUT of the server (as +// TestDescribeDirect_ServerScopeGate_UserType uses) never reaches the +// callability check at all — it is not a regression test for +// directEntryCallable's own isScopeRestrictedCaller fix. This one scopes the +// OAuth user INTO "github" so the pending tool must be refused by +// callability specifically, with an approved sibling on the same server as +// the positive control (codex gpt-5.6-sol cross-review, round 1: the +// describe-side gate had no non-vacuous AuthTypeUser coverage). +func TestDescribeDirect_CallabilityGate_UserType(t *testing.T) { + p := newDirectDescribeProxy(t) + require.NoError(t, p.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "github", + ToolName: "read_file", + Status: storage.ToolApprovalStatusPending, + })) + + scopedUser := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeUser, + UserID: "u1", + AllowedServers: []string{"github"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + + resp := callDescribeDirect(t, p, scopedUser, []interface{}{"github__read_file", "github__create_issue"}) + byID := describeErrorsByID(resp) + require.Contains(t, byID, "github__read_file", + "a pending tool must be refused by callability for a scoped OAuth user, exactly like an agent token") + assert.Equal(t, describeErrNotFound, byID["github__read_file"]["error"]) + + defsByName := map[string]bool{} + for _, def := range resp.Definitions { + defsByName[def["name"].(string)] = true + } + assert.True(t, defsByName["github__create_issue"], + "positive control: an approved sibling on the same in-scope server must still describe") +} + // T044: catalog divergence. A tool that is pending approval is still LISTED for // a non-agent session, so it must still describe — from the catalog snapshot. // An index-backed resolver answers not_found here, which would make deferral diff --git a/internal/server/mcp_direct_callability.go b/internal/server/mcp_direct_callability.go index 888802b60..42e042f1c 100644 --- a/internal/server/mcp_direct_callability.go +++ b/internal/server/mcp_direct_callability.go @@ -42,17 +42,22 @@ func newDirectCallabilityEvaluator(proxy *MCPProxyServer) *directCallabilityEval } } -// filterDirectToolsForAgentCallability hides direct-mode tools that an agent -// token cannot actually invoke because they are disabled, quarantined, pending -// approval, or changed since approval. Non-agent contexts keep the existing -// operator-visible discovery behavior. +// filterDirectToolsForAgentCallability hides direct-mode tools that a +// scope-restricted caller (an agent token or an OAuth-authenticated user — +// see isScopeRestrictedCaller) cannot actually invoke because they are +// disabled, quarantined, pending approval, or changed since approval. +// Administrator contexts keep the existing operator-visible discovery +// behavior: server-edition-multiuser-auth.md documents the admin role as the +// one that "sees all activity, manages users" and reviews pending/quarantined +// tools, which a plain "user" account is not (it is caller-bounded exactly +// like an agent token — see cache_authz.go's CallerKindUser case). func (p *MCPProxyServer) filterDirectToolsForAgentCallability(ctx context.Context, tools []mcp.Tool) []mcp.Tool { if len(tools) == 0 { return tools } authCtx := auth.AuthContextFromContext(ctx) - if authCtx == nil || authCtx.Type != auth.AuthTypeAgent { + if !isScopeRestrictedCaller(authCtx) { return tools } @@ -124,19 +129,22 @@ func (p *MCPProxyServer) filterDirectToolsForAgentCallability(ctx context.Contex return filtered } -// directEntryCallable is the agent-callability half of the direct listing gate, +// directEntryCallable is the callability half of the direct listing gate, // for callers that already hold a resolved catalog entry (Spec 102 US2). // -// Non-agent sessions are unfiltered here, exactly as the loop above leaves them: -// the direct listing deliberately RETAINS tool-level pending/changed/disabled -// states for an operator, and describe_tool must therefore keep describing them -// — a listed tool is never undescribable (SC-007). Only agent tokens, which -// cannot see those tools in their own listing, are gated. +// Administrator sessions are unfiltered here, exactly as the loop above +// leaves them: the direct listing deliberately RETAINS tool-level +// pending/changed/disabled states for an operator, and describe_tool must +// therefore keep describing them — a listed tool is never undescribable +// (SC-007). Only scope-restricted callers (agent tokens and OAuth +// users — see isScopeRestrictedCaller), which cannot see those tools in +// their own listing, are gated. This MUST stay in parity with +// filterDirectToolsForAgentCallability above — the same SC-007 invariant. func (p *MCPProxyServer) directEntryCallable(authCtx *auth.AuthContext, entry *directCatalogEntry) bool { if entry == nil { return false } - if authCtx == nil || authCtx.Type != auth.AuthTypeAgent { + if !isScopeRestrictedCaller(authCtx) { return true } return newDirectCallabilityEvaluator(p).evaluate(entry.ServerName, entry.ToolName).callable diff --git a/internal/server/mcp_direct_callability_test.go b/internal/server/mcp_direct_callability_test.go index 993d79d3c..572184d6f 100644 --- a/internal/server/mcp_direct_callability_test.go +++ b/internal/server/mcp_direct_callability_test.go @@ -224,6 +224,53 @@ func TestFilterDirectToolsForAgentCallability_AgentOnly(t *testing.T) { assert.Equal(t, tools, proxy.filterDirectToolsForAgentCallability(context.Background(), tools)) } +// TestFilterDirectToolsForAgentCallability_UserTypeIsScopeRestrictedToo is the +// Spec 105 PR G regression for this gate: it used to key on +// `authCtx.Type == auth.AuthTypeAgent`, which let a server-edition OAuth +// "user" context fall through to the operator-visible branch (unfiltered, +// same as admin) and see pending/disabled tools it cannot actually call. A +// "user" is not an administrator — server-edition-multiuser-auth.md reserves +// "sees all activity, manages users" for the admin role — so it must be +// gated exactly like an agent token here too (isScopeRestrictedCaller). +func TestFilterDirectToolsForAgentCallability_UserTypeIsScopeRestrictedToo(t *testing.T) { + proxy := createTestMCPProxyServer(t) + require.NoError(t, proxy.storage.SaveUpstreamServer(&config.ServerConfig{Name: "github", Enabled: true})) + require.NoError(t, proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "github", + ToolName: "allowed", + Status: storage.ToolApprovalStatusApproved, + })) + require.NoError(t, proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "github", + ToolName: "pending", + Status: storage.ToolApprovalStatusPending, + })) + + tools := []mcp.Tool{ + {Name: FormatDirectToolName("github", "allowed")}, + {Name: FormatDirectToolName("github", "pending")}, + } + publishPermsCatalog(proxy, map[string]string{ + FormatDirectToolName("github", "allowed"): auth.PermRead, + FormatDirectToolName("github", "pending"): auth.PermRead, + }) + + userCtx := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeUser, + UserID: "u1", + AllowedServers: []string{"github"}, + }) + + filtered := proxy.filterDirectToolsForAgentCallability(userCtx, tools) + assert.Equal(t, []string{FormatDirectToolName("github", "allowed")}, directCallabilityToolNamesForTest(filtered), + "a scoped OAuth user must not see a tool pending approval, same as an equivalently-scoped agent token") + + // Positive control: an OAuth admin_user keeps the operator-visible view. + adminUserCtx := auth.WithAuthContext(context.Background(), auth.AdminUserContext("a1", "admin@example.com", "Admin", "google")) + assert.Equal(t, tools, proxy.filterDirectToolsForAgentCallability(adminUserCtx, tools), + "an OAuth admin user keeps the operator-visible discovery behavior, like api-key admin") +} + func directCallabilityToolNamesForTest(tools []mcp.Tool) []string { names := make([]string, 0, len(tools)) for _, tool := range tools { diff --git a/internal/server/mcp_direct_scope.go b/internal/server/mcp_direct_scope.go index 952c0baf2..c4a63265f 100644 --- a/internal/server/mcp_direct_scope.go +++ b/internal/server/mcp_direct_scope.go @@ -496,13 +496,14 @@ var builtinPromptNames = map[string]struct{}{ troubleshootServerPrompt().Name: {}, } -// filterAggregatedPromptsForAuth filters prompts/list AND prompts/get for scoped -// agent tokens and for any request with an active profile. It is the prompt -// analogue of filterDirectModeToolsForAuth and the list-side half of the -// aggregated-prompt gate (the handler-side half is -// authorizeAggregatedPromptServer): without it a scoped agent token could -// discover any upstream server's prompt even when the tool filters hid that -// server (PR #973 review, finding F1). mcp-go enforces this on both list and +// filterAggregatedPromptsForAuth filters prompts/list AND prompts/get for +// scope-restricted callers (agent tokens and OAuth-authenticated users — see +// isScopeRestrictedCaller) and for any request with an active profile. It is +// the prompt analogue of filterDirectModeToolsForAuth and the list-side half +// of the aggregated-prompt gate (the handler-side half is +// authorizeAggregatedPromptServer): without it a scope-restricted caller +// could discover any upstream server's prompt even when the tool filters hid +// that server (PR #973 review, finding F1). mcp-go enforces this on both list and // get (server.go filteredPrompts / passesPromptFilters, v1.0.0), so a prompt // dropped here is neither discoverable nor retrievable. // @@ -531,8 +532,8 @@ func (p *MCPProxyServer) filterAggregatedPromptsForAuth(ctx context.Context, pro authCtx := auth.AuthContextFromContext(ctx) _, profileScope := p.resolveActiveProfile(ctx) - isScopedAgent := authCtx != nil && authCtx.Type == auth.AuthTypeAgent - enforce := isScopedAgent || profileScope != nil + isScopeRestricted := isScopeRestrictedCaller(authCtx) + enforce := isScopeRestricted || profileScope != nil allowed := promptServerAllowed(authCtx, profileScope) filtered := make([]mcp.Prompt, 0, len(prompts)) @@ -566,18 +567,18 @@ func (p *MCPProxyServer) filterAggregatedPromptsForAuth(ctx context.Context, pro } // promptServerAllowed returns the per-server access predicate for one caller: -// profile scope (Allows) plus, for scoped agent tokens, server scope +// profile scope (Allows) plus, for scope-restricted callers, server scope // (CanAccessServer). profileScope.Allows tolerates a nil receiver (returns -// true), so the scoped-agent-without-profile case falls through correctly. +// true), so the scope-restricted-without-profile case falls through correctly. // It is the ONE definition of "may this caller touch prompts on server X", // shared by the list/get filter and by every aggregated prompt handler. func promptServerAllowed(authCtx *auth.AuthContext, profileScope *profile.ProfileScope) func(serverName string) bool { - isScopedAgent := authCtx != nil && authCtx.Type == auth.AuthTypeAgent + isScopeRestricted := isScopeRestrictedCaller(authCtx) return func(serverName string) bool { if !profileScope.Allows(serverName) { return false } - return !isScopedAgent || authCtx.CanAccessServer(serverName) + return !isScopeRestricted || authCtx.CanAccessServer(serverName) } } diff --git a/internal/server/mcp_prompt_scope_test.go b/internal/server/mcp_prompt_scope_test.go index 1f72e0893..61a7f8e47 100644 --- a/internal/server/mcp_prompt_scope_test.go +++ b/internal/server/mcp_prompt_scope_test.go @@ -68,6 +68,13 @@ func TestFilterAggregatedPromptsForAuth(t *testing.T) { profile.NewProfileScope("dev", servers), ) } + userCtx := func(servers ...string) context.Context { + return auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeUser, + UserID: "u1", + AllowedServers: servers, + }) + } tests := []struct { name string @@ -94,6 +101,20 @@ func TestFilterAggregatedPromptsForAuth(t *testing.T) { ctx: agentCtx("*"), want: []string{builtinSetup, builtinTrbl, githubPrompt, gitlabPrompt}, }, + { + // Regression: Spec 105 PR G. isScopeRestrictedCaller must key off + // IsAdmin(), not Type == AuthTypeAgent — a server-edition OAuth + // "user" is scope-restricted too (cache_authz.go's CallerKindUser), + // and a Type-only check let it see every server's prompts. + name: "scoped OAuth user sees only its server's prompts plus built-ins", + ctx: userCtx("github"), + want: []string{builtinSetup, builtinTrbl, githubPrompt}, + }, + { + name: "OAuth admin user sees everything", + ctx: auth.WithAuthContext(context.Background(), auth.AdminUserContext("a1", "admin@example.com", "Admin", "google")), + want: []string{builtinSetup, builtinTrbl, githubPrompt, gitlabPrompt}, + }, { name: "profile-scoped session sees only in-profile prompts plus built-ins", ctx: profileCtx("gitlab"),