Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions internal/server/mcp_describe_direct_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 20 additions & 12 deletions internal/server/mcp_direct_callability.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand Down
47 changes: 47 additions & 0 deletions internal/server/mcp_direct_callability_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
27 changes: 14 additions & 13 deletions internal/server/mcp_direct_scope.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
}
}

Expand Down
21 changes: 21 additions & 0 deletions internal/server/mcp_prompt_scope_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"),
Expand Down
Loading