From e33d6a139b57d0bbc681a40b8210742c8cd81ab5 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sun, 20 Sep 2026 12:24:16 +0300 Subject: [PATCH 1/2] fix(scope): direct-surface definitions authorized by producing identity at every seam (Spec 105 PR F, FR-008) Closes T080-T093. Every direct-mode (/mcp/all) tools/list and tools/call now authorizes a rendered tool against the identity of the SAME publication that produced it, never against whatever directCatalog snapshot happens to be live when the scope/callability filter runs - closing the publication-skew window an origin flip could exploit (e.g. display name "a__b__c" meaning server "a" tool "b__c" in one generation and server "a__b" tool "c" in the next). ## Changes - renderDirectTools now stamps each rendered mcp.Tool with a private directToolStamp{owner, rawName, tier} carrying the identity of the catalog entry that produced it and its dispatch handler (mcp_direct_scope.go, mcp_routing.go). filterDirectModeToolsForAuth and filterDirectToolsForAgentCallability read the stamp first and authorize against it directly; only an unstamped tool (a built-in, or a bare mcp.Tool a test constructs directly) falls back to the pre-105 catalog/builtin resolution unchanged. - A terminal WithToolFilter(stripDirectToolStampFilter), registered last, removes the stamp before any tool reaches a client, for every caller including administrators - the admin early-return that used to skip the scope filter's per-tool loop entirely is removed, so the "no identity -> withheld" rule applies to everyone (SC-005 exception), not only scoped callers. - buildDirectCatalog refuses to admit a tool with an empty raw name at all (FR008-G7); resolveDirectTool's "no `__` separator -> built-in" structural inference is removed, replaced by the explicit builtinDirectToolNames set populated from the surface's own constructors (FR008-G2, positive identification). - makeDirectModeHandler's two scope-refusal branches no longer name the server they denied (D12); the message now echoes only the caller-supplied display name, matching what mcp-go's own call-time filter re-evaluation already answers for an unregistered name before the handler ever runs in real dispatch - this branch is defense in depth for direct handler invocation that bypasses that re-evaluation. - buildAggregatedServerPrompts drops an upstream prompt with an empty raw name; filterAggregatedPromptsForAuth withholds an unstamped prompt for every caller, not only when scope enforcement was already active. ## Tests T080-T086a written first (mcp_direct_publication_identity_test.go, mcp_direct_underscore_test.go, mcp_direct_protocol_test.go - all new), covering the origin-flip fixture in both serialization modes with full definition assertions, reverse-flip/plain-addition visibility, the tier-change seam in deferred mode, the protocol-level -32602 envelope parity proof (byte-equal to an unregistered name, no owner metadata), the `__a` server steady-state and seam fixtures, and the prompt-side empty-name and unstamped-withholding cases. Inverted the pinned tests that predated this fix, including several not explicitly enumerated by the task list but discovered red by the full suite run: mcp_direct_skew_test.go (added-name, origin-flip, annotations-only-change scenarios), mcp_direct_catalog_test.go / mcp_direct_catalog_publish_test.go (empty-raw-name admission, structural builtin inference), mcp_routing_test.go (TestDirectModeHandler_ ServerAccessDenied's disclosing message; TestFilterDirectModeToolsForAuth_ KeepsNonDirectTools renamed to _DropsNonBuiltinSeparatorlessNames), profile_pin_enforcement_test.go, and mcp_prompt_scope_test.go (TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed's unscoped-caller assertion). direct_full_prefeature.golden.json and every toolslist golden are unregenerated and byte-identical. ## Verification - go build ./..., go vet ./... clean - go test ./internal/server/... (full package): ok, 278s - go test -race ./internal/server/...: ok, 383s - golangci-lint (bare + --build-tags server): clean on every touched file (pre-existing issues elsewhere in the repo are unrelated to this diff) - Cross-model review: opencode quota exhausted on both terra and sol (confirmed); fell back to codex exec -m gpt-5.6-sol per the CLAUDE.md ladder. 3 rounds, VERDICT: clean. - ROADMAP.md regenerated (scripts/gen-roadmap.py) to reflect the tasks.md checkbox updates and pass the pre-commit roadmap-verify hook; it was already drifted from an unrelated prior change, so this also fixes that drift. Related #105 Co-Authored-By: Claude Sonnet 5 --- ROADMAP.md | 4 +- internal/server/mcp_direct_callability.go | 58 +-- internal/server/mcp_direct_catalog.go | 54 +-- .../server/mcp_direct_catalog_publish_test.go | 46 ++- internal/server/mcp_direct_catalog_test.go | 68 +++- internal/server/mcp_direct_protocol_test.go | 82 +++++ .../mcp_direct_publication_identity_test.go | 336 ++++++++++++++++++ internal/server/mcp_direct_scope.go | 226 ++++++++++-- internal/server/mcp_direct_skew_test.go | 141 +++++--- internal/server/mcp_direct_underscore_test.go | 135 +++++++ internal/server/mcp_prompt_scope_test.go | 107 +++++- internal/server/mcp_routing.go | 98 ++++- internal/server/mcp_routing_test.go | 36 +- .../server/profile_pin_enforcement_test.go | 7 +- specs/105-agent-scope-hardening/tasks.md | 30 +- 15 files changed, 1221 insertions(+), 207 deletions(-) create mode 100644 internal/server/mcp_direct_protocol_test.go create mode 100644 internal/server/mcp_direct_publication_identity_test.go create mode 100644 internal/server/mcp_direct_underscore_test.go diff --git a/ROADMAP.md b/ROADMAP.md index dfb143187..6b13f9d5d 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -888,7 +888,7 @@ graph LR | Web UI + macOS app UX audit | In progress | P0 | — | | | | Release qualification gate (auto-QA matrix blocks the tag) | In progress | P0 | — | [081-release-qa-gate](./specs/081-release-qa-gate/) | | | Action log / transparency — info at a glance | In progress | P1 | — | | | -| Agent-token scope hardening: every MCP request authorized by its own scope (spec 105) | In progress | P1 | 42/109 (39%) | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | | +| Agent-token scope hardening: every MCP request authorized by its own scope (spec 105) | In progress | P1 | 57/110 (52%) | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | | | Token-efficiency benchmark: measured savings, published results | In progress | P1 | 62/64 (97%) | [103-token-bench](./specs/103-token-bench/) | | | Telemetry identity & data quality (machine_id + CI-filter hardening) | In progress | P1 | — | | | | Telemetry v7: honest funnel + churn instrumentation | In progress | P1 | — | [080-telemetry-v7-churn](./specs/080-telemetry-v7-churn/) | | @@ -1034,6 +1034,6 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [102-schema-deferred](./specs/102-schema-deferred/) | `shipped` | 89/89 (100%) | | [103-token-bench](./specs/103-token-bench/) | `shipped` | 62/64 (97%) | | [104-auto-routing-mode](./specs/104-auto-routing-mode/) | — | — | -| [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 42/109 (39%) | +| [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 57/110 (52%) | | [106-security-residual-fixes](./specs/106-security-residual-fixes/) | `shipped` | 18/19 (95%) | | [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `shipped` | 126/126 (100%) | diff --git a/internal/server/mcp_direct_callability.go b/internal/server/mcp_direct_callability.go index 67d3ff904..a27e244fa 100644 --- a/internal/server/mcp_direct_callability.go +++ b/internal/server/mcp_direct_callability.go @@ -59,28 +59,44 @@ func (p *MCPProxyServer) filterDirectToolsForAgentCallability(ctx context.Contex evaluator := newDirectCallabilityEvaluator(p) filtered := make([]mcp.Tool, 0, len(tools)) for _, tool := range tools { - // Same catalog resolution as filterDirectModeToolsForAuth (D10). The two - // filters run over the same listing, so if they resolved names - // differently — one by catalog, one by first-"__" parse — a server whose - // name contains "__" could be scope-checked as one origin and - // callability-checked as another. - entry, decision := p.resolveDirectTool(tool.Name) - var serverName, toolName string - switch decision { - case directResolveBuiltin: - // Built-ins are this proxy's own tools; there is no upstream - // approval record to evaluate. - filtered = append(filtered, tool) - continue - case directResolveDenied: - continue - case directResolveNoCatalog: - // The parse cannot fail: a separator-less name was already classified - // as a built-in above. - serverName, toolName, _ = ParseDirectToolName(tool.Name) - case directResolveFound: - serverName, toolName = entry.ServerName, entry.ToolName + + if stamp, stamped := readDirectToolStamp(tool); stamped { + // Spec 105 FR-008: the identity STAMPED on this exact tool object, + // never re-derived from a fresh catalog lookup (see + // directToolStamp's doc comment). + if stamp.rawName == "" { + continue + } + serverName, toolName = stamp.owner, stamp.rawName + } else { + // No stamp: fall back to the pre-105 catalog/builtin resolution, + // exactly as filterDirectModeToolsForAuth does. Same catalog + // resolution the scope filter uses (D10): the two filters run over + // the same listing, so if they resolved names differently — one by + // catalog, one by first-"__" parse — a server whose name contains + // "__" could be scope-checked as one origin and + // callability-checked as another. + // + // Same residual as filterDirectModeToolsForAuth's fallback branch + // (see its doc comment): an unstamped mcp-go SESSION tool sharing a + // global tool's name would resolve here too. Not reachable today — + // mcpproxy-go registers no session-specific tools on this surface. + entry, decision := p.resolveDirectTool(tool.Name) + + switch decision { + case directResolveBuiltin: + // Built-ins are this proxy's own tools; there is no upstream + // approval record to evaluate. + filtered = append(filtered, tool) + continue + case directResolveDenied: + continue + case directResolveNoCatalog: + serverName, toolName, _ = ParseDirectToolName(tool.Name) + case directResolveFound: + serverName, toolName = entry.ServerName, entry.ToolName + } } if evaluator.evaluate(serverName, toolName).callable { diff --git a/internal/server/mcp_direct_catalog.go b/internal/server/mcp_direct_catalog.go index ba602f12a..eeb4f4b4a 100644 --- a/internal/server/mcp_direct_catalog.go +++ b/internal/server/mcp_direct_catalog.go @@ -166,6 +166,20 @@ func buildDirectCatalog(tools []*config.ToolMetadata, logger *zap.Logger) *direc if t == nil { continue } + if t.Name == "" { + // Spec 105 FR-008 (FR008-G7): an upstream tool with an empty raw + // name renders as "server__" and has no registration identity to + // authorize it against — the direct-surface analogue of the + // FR-006 empty-prompt-name rule. Refused admission here, at the + // source, rather than admitted and relied on to be caught by a + // downstream filter: withheld from every caller, administrators + // included (SC-005 exception). + if logger != nil { + logger.Warn("Withholding direct tool with an empty raw name: no registration identity to authorize it against", + zap.String("server_name", t.ServerName)) + } + continue + } name := FormatDirectToolName(t.ServerName, t.Name) if _, seen := grouped[name]; !seen { order = append(order, name) @@ -367,12 +381,6 @@ const ( directResolveNoCatalog ) -// builtinDirectToolNames is an explicit allowlist for built-ins whose display -// name WOULD parse as server__tool and so cannot be recognised structurally. -// Empty today; it exists so adding such a built-in is a deliberate act rather -// than an accidental denial. -var builtinDirectToolNames = map[string]struct{}{} - // resolveDirectTool maps a direct display name to its catalog entry. // // This replaces ParseDirectToolName as the resolution path for the discovery @@ -393,30 +401,26 @@ func (p *MCPProxyServer) resolveDirectTool(displayName string) (*directCatalogEn // through the scope, tier and callability gates like any other. // // This ordering is load-bearing, and getting it wrong was a real disclosure - // bug. The structural test below assumes every upstream display name parses, - // because FormatDirectToolName always inserts "__". It does not: an upstream - // tool whose NAME IS EMPTY renders as "server__", which ParseDirectToolName - // rejects (the tool half is empty). That name was therefore classified as a - // proxy built-in, and both direct filters pass built-ins through - // unconditionally — so an agent token scoped to other servers could see the - // name, description and annotations of a tool on a server outside its scope. - // Found by adversarial QA, not by any unit test, because no fixture had ever + // bug: an upstream tool whose NAME IS EMPTY renders as "server__", which + // ParseDirectToolName rejects (the tool half is empty). A name with no + // "__" separator was therefore once inferred a proxy built-in structurally + // — and both direct filters pass built-ins through unconditionally — so an + // agent token scoped to other servers could see the name, description and + // annotations of a tool on a server outside its scope. Found by + // adversarial QA, not by any unit test, because no fixture had ever // contained a nameless tool. + // + // Spec 105 FR-008 (FR008-G7) closes this at its source: buildDirectCatalog + // now refuses to admit an entry with an empty raw tool name at all, so + // "server__" is never in this catalog to begin with, and the structural + // "no separator -> built-in" inference below is gone entirely — a name is + // a built-in ONLY via the explicit builtinDirectToolNames set checked + // above. A name that is neither stamped in the catalog nor a recognised + // built-in has no registration identity and falls through to denial. if entry, ok := cat.Lookup(displayName); ok { return entry, directResolveFound } - // A name with no "__" separator that the catalog does NOT admit is something - // this proxy registered itself — describe_tool, retrieve_tools on a shared - // surface — and denying it would delete built-ins off their own surface. - // - // This is the structural half of D13 rule 2's "built-ins by explicit name - // set". The set above covers the residual case a structural test cannot: a - // built-in whose name happens to contain "__". - if _, _, ok := ParseDirectToolName(displayName); !ok { - return nil, directResolveBuiltin - } - if cat == nil { return nil, directResolveNoCatalog } diff --git a/internal/server/mcp_direct_catalog_publish_test.go b/internal/server/mcp_direct_catalog_publish_test.go index 824f7aa66..134f478b2 100644 --- a/internal/server/mcp_direct_catalog_publish_test.go +++ b/internal/server/mcp_direct_catalog_publish_test.go @@ -89,16 +89,27 @@ func TestResolveDirectTool_DenyOnMissButNotOnNilCatalog(t *testing.T) { "a name the catalog does not admit must be denied, not waved through by re-parsing it") }) - t.Run("a separator-less name is a built-in, not a denial", func(t *testing.T) { - // Every upstream tool is named through FormatDirectToolName, which always - // inserts "__". A name without one therefore cannot be an upstream - // projection — it is a tool this proxy registered itself, and denying it - // would delete built-ins off their own surface. - for _, name := range []string{"describe_tool", "retrieve_tools"} { - entry, decision := p.resolveDirectTool(name) - assert.Nil(t, entry, "a built-in has no upstream catalog entry") - assert.Equal(t, directResolveBuiltin, decision, "%s must be kept", name) - } + t.Run("a separator-less name is a built-in ONLY via the explicit set", func(t *testing.T) { + // Spec 105 FR-008 (FR008-G2): "built-in" used to be inferred + // structurally — any name that fails to parse as server__tool — which + // misclassified an upstream tool with an empty raw name ("server__") + // the same way (TestResolveDirectTool_EmptyToolNameIsNotABuiltin). + // Positive identification replaces it: a name is a built-in only when + // it is in builtinDirectToolNames, populated from this surface's own + // constructors. + entry, decision := p.resolveDirectTool("describe_tool") + assert.Nil(t, entry, "a built-in has no upstream catalog entry") + assert.Equal(t, directResolveBuiltin, decision, "describe_tool is a REAL direct-surface built-in") + + // "retrieve_tools" has no "__" either, but it is a RETRIEVE-surface + // built-in, never registered on the direct surface at all — it must + // NOT be positively identified here. With a published, non-empty + // catalog that does not admit it, it is denied — never waved through + // on the strength of its shape alone. + entry, decision = p.resolveDirectTool("retrieve_tools") + assert.Nil(t, entry) + assert.Equal(t, directResolveDenied, decision, + "a name that is neither stamped in the catalog nor an explicit built-in has no registration identity") }) t.Run("a withheld collision is denied in both id forms", func(t *testing.T) { @@ -152,13 +163,21 @@ func publishPermsCatalog(p *MCPProxyServer, perms map[string]string) { // directToolPermissions map resolved this by accident — a nil map missed, and a // miss dropped the tool for a scoped agent — so the behaviour is preserved // deliberately here rather than left to be rediscovered. +// +// Spec 105 FR-008 (FR008-G2): the fixture's separator-less name is now +// "describe_tool", a REAL direct-surface built-in identified from the +// explicit set regardless of catalog state — not "retrieve_tools", which was +// only ever kept here because a nil catalog's structural fallback could not +// tell it apart from a genuine built-in (see the sibling test above). A +// caller-supplied name with no registration identity at all is dropped in the +// NoCatalog window exactly as an upstream-shaped one is. func TestFilterDirectModeToolsForAuth_NoCatalogPreservesPreChangeBehaviour(t *testing.T) { proxy := &MCPProxyServer{} require.Nil(t, proxy.loadDirectCatalog(), "precondition: no catalog published") tools := []mcp.Tool{ {Name: FormatDirectToolName("github", "get_issue")}, - {Name: "retrieve_tools"}, + {Name: "describe_tool"}, } t.Run("unauthenticated caller keeps everything", func(t *testing.T) { @@ -178,8 +197,9 @@ func TestFilterDirectModeToolsForAuth_NoCatalogPreservesPreChangeBehaviour(t *te for _, tl := range got { names = append(names, tl.Name) } - assert.Equal(t, []string{"retrieve_tools"}, names, + assert.Equal(t, []string{"describe_tool"}, names, "with no catalog the tier is unknown, so an upstream tool fails closed for a scoped "+ - "agent — but a built-in, which has no tier to begin with, must survive") + "agent — but a REAL built-in, positively identified by name regardless of catalog "+ + "state, must survive") }) } diff --git a/internal/server/mcp_direct_catalog_test.go b/internal/server/mcp_direct_catalog_test.go index 5b08b4046..203599112 100644 --- a/internal/server/mcp_direct_catalog_test.go +++ b/internal/server/mcp_direct_catalog_test.go @@ -174,6 +174,13 @@ func TestBuildDirectCatalog_DuplicateOriginIsNotACollision(t *testing.T) { // // Found by adversarial QA against a running proxy. No unit fixture had ever // contained a nameless tool, so nothing here could have caught it. +// +// Spec 105 FR-008 (FR008-G7) closes it at the source instead of merely +// scope-checking it: an upstream tool with an empty raw name has NO +// registration identity at all, so buildDirectCatalog now refuses to admit it +// — "hostile__" is absent from the catalog, not present-but-scope-checked — +// and it is withheld from every caller, administrators included, never only +// from an out-of-scope one. func TestResolveDirectTool_EmptyToolNameIsNotABuiltin(t *testing.T) { tools := []*config.ToolMetadata{ {ServerName: "hostile", Name: "", Description: "Nameless", ParamsJSON: `{"type":"object"}`, Hash: "h-empty"}, @@ -184,23 +191,34 @@ func TestResolveDirectTool_EmptyToolNameIsNotABuiltin(t *testing.T) { _, _, parses := ParseDirectToolName(display) require.False(t, parses, "the fixture must be a name that does NOT parse, or it proves nothing") + cat := buildDirectCatalog(tools, nil) + assert.Equal(t, 1, cat.Len(), "the nameless tool must never be admitted; only 'we__solo' is") + p := &MCPProxyServer{} - p.publishDirectCatalog(buildDirectCatalog(tools, nil)) + p.publishDirectCatalog(cat) entry, decision := p.resolveDirectTool(display) - assert.Equal(t, directResolveFound, decision, - "a name the catalog admits is an upstream projection, whatever it looks like") - require.NotNil(t, entry) - assert.Equal(t, "hostile", entry.ServerName, - "and it must resolve to its real origin, so the scope gate sees the right server") + assert.Equal(t, directResolveDenied, decision, + "an empty raw name has no registration identity — the catalog never admitted it, so it is denied, not found") + assert.Nil(t, entry) + + // The real, non-empty-named tool from the same build is unaffected. + entry, decision = p.resolveDirectTool("we__solo") + require.Equal(t, directResolveFound, decision) + assert.Equal(t, "we", entry.ServerName) // Real built-ins are still built-ins. _, builtinDecision := p.resolveDirectTool("describe_tool") assert.Equal(t, directResolveBuiltin, builtinDecision) } -// The disclosure itself: a scoped token must not see the nameless tool of a -// server outside its scope. +// The disclosure itself: NO caller — scoped, unrestricted, or administrator — +// may see the nameless tool, per FR008-G7's "withheld from every caller" +// rule. The scoped-only assertion from before Spec 105 PR F is kept (it still +// holds, now via non-admission rather than a scope check); the unrestricted +// and administrator cases are new and are exactly what distinguishes +// "withheld because it lacks an identity" from "withheld because it is out of +// scope". func TestFilterDirectModeToolsForAuth_EmptyToolNameIsScopeChecked(t *testing.T) { tools := []*config.ToolMetadata{ {ServerName: "hostile", Name: "", Description: "Nameless", ParamsJSON: `{"type":"object"}`, Hash: "h-empty"}, @@ -210,22 +228,34 @@ func TestFilterDirectModeToolsForAuth_EmptyToolNameIsScopeChecked(t *testing.T) p := &MCPProxyServer{} p.publishDirectCatalog(buildDirectCatalog(tools, nil)) - ctx := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + rawTools := []mcp.Tool{ + {Name: "hostile__"}, {Name: "we__solo"}, {Name: "describe_tool"}, + } + + scoped := auth.WithAuthContext(context.Background(), &auth.AuthContext{ Type: auth.AuthTypeAgent, AgentName: "scoped", AllowedServers: []string{"we"}, Permissions: []string{auth.PermRead}, }) - - filtered := p.filterDirectModeToolsForAuth(ctx, []mcp.Tool{ - {Name: "hostile__"}, {Name: "we__solo"}, {Name: "describe_tool"}, + unrestrictedAgent := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "unrestricted", + AllowedServers: []string{"*"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, }) - names := make([]string, 0, len(filtered)) - for _, tool := range filtered { - names = append(names, tool.Name) + for name, ctx := range map[string]context.Context{ + "scoped agent": scoped, + "unrestricted agent": unrestrictedAgent, + "administrator": context.Background(), + } { + filtered := p.filterDirectModeToolsForAuth(ctx, rawTools) + + names := make([]string, 0, len(filtered)) + for _, tool := range filtered { + names = append(names, tool.Name) + } + assert.NotContainsf(t, names, "hostile__", "%s: a tool with no registration identity is withheld from EVERY caller (SC-005)", name) + assert.Containsf(t, names, "we__solo", "%s", name) + assert.Containsf(t, names, "describe_tool", "%s: real built-ins stay visible", name) } - assert.NotContains(t, names, "hostile__", - "a tool on an out-of-scope server must not be disclosed, even with an empty name") - assert.Contains(t, names, "we__solo") - assert.Contains(t, names, "describe_tool", "real built-ins stay visible") } diff --git a/internal/server/mcp_direct_protocol_test.go b/internal/server/mcp_direct_protocol_test.go new file mode 100644 index 000000000..637320ca4 --- /dev/null +++ b/internal/server/mcp_direct_protocol_test.go @@ -0,0 +1,82 @@ +package server + +import ( + "context" + "encoding/json" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Spec 105 PR F — T086a: protocol-level proof of two mechanisms the rest of +// this PR's design relies on (plan.md D7): +// +// 1. The internal directToolStamp never reaches the wire, for admin OR +// agent callers, on tools/list. +// 2. mcp-go v1.0.0 RE-EVALUATES the tool filter chain at tools/call time +// (passesToolFilters), not only at tools/list time — so a tool that is +// REGISTERED but HIDDEN by the scope filter is refused with the +// unregistered-name envelope (-32602) if a client calls it anyway, +// without needing any call-time code of our own. +func TestDirectProtocol_StampNeverOnWire_FilterReEvaluatedAtCallTime(t *testing.T) { + tools := []*config.ToolMetadata{ + skewTool("a", "read", "Read something", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + skewTool("b", "read", "Read something else", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + f := newSkewFixture(t, tools) + + aOnly := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "a-only", + AllowedServers: []string{"a"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + admin := auth.WithAuthContext(context.Background(), auth.AdminContext()) + + initMsg := []byte(`{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-03-26","capabilities":{},"clientInfo":{"name":"t","version":"0"}}}`) + + for name, ctx := range map[string]context.Context{"a-only agent": aOnly, "administrator": admin} { + require.NotNil(t, f.proxy.directServer.HandleMessage(ctx, initMsg)) + + encoded, err := json.Marshal(f.proxy.directServer.HandleMessage(ctx, []byte(`{"jsonrpc":"2.0","id":2,"method":"tools/list"}`))) + require.NoError(t, err) + + var envelope struct { + Result struct { + Tools []json.RawMessage `json:"tools"` + } `json:"result"` + } + require.NoError(t, json.Unmarshal(encoded, &envelope)) + require.NotEmpty(t, envelope.Result.Tools, "%s", name) + + for _, raw := range envelope.Result.Tools { + // _meta itself may legitimately be present (a response hook stamps + // "anthropic/maxResultSizeChars" AFTER the filter chain runs) — the + // assertion is about OUR internal key specifically, which the + // terminal stripDirectToolStampFilter must remove before any tool + // reaches the wire, for every caller. + assert.NotContainsf(t, string(raw), directToolStampMetaKey, "%s: the internal stamp must never reach the wire: %s", name, raw) + } + } + + // (b) call-time re-evaluation: "b__read" is REGISTERED (it exists in + // s.tools) but HIDDEN from a-only by the scope filter. Calling it anyway + // must be refused with the SAME envelope an unregistered name gets, + // proving mcp-go re-ran the filter at call time rather than trusting + // whatever tools/list happened to return earlier. + require.NotNil(t, f.proxy.directServer.HandleMessage(aOnly, initMsg)) + callEncoded, err := json.Marshal(f.proxy.directServer.HandleMessage(aOnly, + []byte(`{"jsonrpc":"2.0","id":3,"method":"tools/call","params":{"name":"b__read","arguments":{}}}`))) + require.NoError(t, err) + + var callEnvelope map[string]interface{} + require.NoError(t, json.Unmarshal(callEncoded, &callEnvelope)) + require.NotNil(t, callEnvelope["error"], "a hidden REGISTERED tool must still be refused at call time: %v", callEnvelope) + callErr := callEnvelope["error"].(map[string]interface{}) + assert.Equal(t, float64(mcp.INVALID_PARAMS), callErr["code"]) + assert.Contains(t, callErr["message"], "not found") +} diff --git a/internal/server/mcp_direct_publication_identity_test.go b/internal/server/mcp_direct_publication_identity_test.go new file mode 100644 index 000000000..3d01abb78 --- /dev/null +++ b/internal/server/mcp_direct_publication_identity_test.go @@ -0,0 +1,336 @@ +package server + +import ( + "context" + "encoding/json" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" +) + +// Spec 105 PR F — scope-direct-publication (FR-008, gaps FR008-G1..G7, +// T080-T086a). +// +// The sibling skew tests in mcp_direct_skew_test.go exercise the internal +// filter functions directly (f.listed, f.registeredHandler). This file adds +// the assertions those did not carry before this PR: the FULL DEFINITION +// served to an authorized caller during a seam, both serialization modes for +// every gap, and the real mcp-go protocol path end to end (directServer. +// HandleMessage), so the guarantee is proven at the wire, not only against +// the filter functions that produce it. + +// mustReadTool returns the mcp.Tool registered under display, and requires it +// to exist. +func mustReadTool(t *testing.T, f *skewFixture, display string) mcp.Tool { + t.Helper() + st, ok := f.proxy.directServer.ListTools()[display] + require.Truef(t, ok, "%q must be registered", display) + return st.Tool +} + +// T080 (FR008-G1): the origin-flip fixture, in both serialization modes, +// asserting the FULL served definition (not just presence/absence) for every +// caller kind during the seam. +func TestDirectPublication_OriginFlip_DefinitionMatchesProducingIdentity(t *testing.T) { + for _, mode := range []string{config.DirectToolResponseModeFull, config.DirectToolResponseModeDeferred} { + t.Run(mode, func(t *testing.T) { + const display = "a__b__c" + oldOrigin := []*config.ToolMetadata{ + skewTool("a", "b__c", "SENTINEL_OLD_OWNER_A description", + `{"type":"object","properties":{"old_only_field":{"type":"string"}},"required":["old_only_field"]}`, + &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + newOrigin := []*config.ToolMetadata{ + skewTool("a__b", "c", "SENTINEL_NEW_OWNER_AB description", + `{"type":"object","properties":{"new_only_field":{"type":"integer"}},"required":["new_only_field"]}`, + &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + + f := newSkewFixtureInMode(t, oldOrigin, mode) + require.NoError(t, f.proxy.storage.SaveUpstreamServer(&config.ServerConfig{Name: "a__b", Enabled: true})) + require.NoError(t, f.proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "a__b", ToolName: "c", Status: storage.ToolApprovalStatusApproved, + })) + + oldOnly := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "a-only", + AllowedServers: []string{"a"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + wildcard := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "wildcard", + AllowedServers: []string{"*"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + admin := auth.WithAuthContext(context.Background(), auth.AdminContext()) + + f.rebuildPaused(t, newOrigin, func() { + listedOld := f.listed(oldOnly) + assert.NotContains(t, listedOld, display, + "a-only must never see the new owner's tool during the seam, in either mode") + + for name, ctx := range map[string]context.Context{"wildcard-scoped agent": wildcard, "administrator": admin} { + listed := f.listed(ctx) + require.Containsf(t, listed, display, "%s must see it", name) + + tool := mustReadTool(t, f, display) + assert.Containsf(t, tool.Description, "SENTINEL_NEW_OWNER_AB", "%s: served description must be the NEW owner's", name) + assert.NotContainsf(t, tool.Description, "SENTINEL_OLD_OWNER_A", "%s: must never carry the OLD owner's description", name) + + if mode == config.DirectToolResponseModeFull { + raw, err := json.Marshal(tool.InputSchema) + require.NoError(t, err) + assert.Containsf(t, string(raw), "new_only_field", "%s: schema must be the NEW owner's (S2)", name) + assert.NotContainsf(t, string(raw), "old_only_field", "%s: must never carry the OLD owner's schema (S1)", name) + } + } + }) + + // After the publish the definitions agree everywhere. + for name, ctx := range map[string]context.Context{"wildcard": wildcard, "admin": admin} { + require.Containsf(t, f.listed(ctx), display, "%s", name) + } + }) + } +} + +// T082 (FR008-G3): the reverse flip (new-scope-restricted origin -> back to +// old, wider-visible one) and a plain addition, in the SAME window, proving +// visibility WIDENS immediately too — the stamp is not merely conservative, +// it tracks the CURRENT registration in both directions. +func TestDirectPublication_ReverseFlipAndPlainAddition_VisibleDuringWindow(t *testing.T) { + const flipped = "a__b__c" + + // Start on the NEW (restricted) origin: server "a__b", tool "c" — out of + // scope for an a-only token. + restricted := []*config.ToolMetadata{ + skewTool("a__b", "c", "restricted origin", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + f := newSkewFixture(t, restricted) + require.NoError(t, f.proxy.storage.SaveUpstreamServer(&config.ServerConfig{Name: "a", Enabled: true})) + require.NoError(t, f.proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "a", ToolName: "b__c", Status: storage.ToolApprovalStatusApproved, + })) + require.NoError(t, f.proxy.storage.SaveUpstreamServer(&config.ServerConfig{Name: "b", Enabled: true})) + require.NoError(t, f.proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "b", ToolName: "x", Status: storage.ToolApprovalStatusApproved, + })) + + aOnly := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "a-only", + AllowedServers: []string{"a"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + bOnly := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "b-only", + AllowedServers: []string{"b"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + + require.NotContains(t, f.listed(aOnly), flipped, "precondition: a-only cannot see the restricted origin") + + // Reverse flip to server "a" tool "b__c" (in scope for a-only), PLUS a + // plain addition of "b__x" (in scope for b-only), in the same rebuild. + reversedPlusAddition := []*config.ToolMetadata{ + skewTool("a", "b__c", "reverse-flipped to a", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + skewTool("b", "x", "plain addition on b", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + + f.rebuildPaused(t, reversedPlusAddition, func() { + assert.Contains(t, f.listed(aOnly), flipped, + "the reverse flip is registered before this pause starts, so a-only sees it immediately, not one generation late") + assert.Contains(t, f.listed(bOnly), "b__x", + "the plain addition on b's own server is visible to b-only immediately") + assert.NotContains(t, f.listed(bOnly), flipped, "b-only must not see the (now a-owned) flipped tool") + assert.NotContains(t, f.listed(aOnly), "b__x", "a-only must not see b's addition") + }) + + assert.Contains(t, f.listed(aOnly), flipped) + assert.Contains(t, f.listed(bOnly), "b__x") +} + +// T083 (FR008-G4), deferred-mode companion to +// TestSkew_AnnotationsOnlyChangeIsStaleButNeverAdmitsTheCall: a same-owner +// tier change from read to destructive must withhold the tool from a +// read-scoped token immediately in DEFERRED mode too, not only full mode — +// deferred rendering must not reopen the window full mode closed. +func TestDirectPublication_TierChangeWithheldDuringSeam_DeferredMode(t *testing.T) { + before := []*config.ToolMetadata{ + skewTool("fs", "purge", "Purge", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + after := []*config.ToolMetadata{ + skewTool("fs", "purge", "Purge", `{"type":"object"}`, &config.ToolAnnotations{DestructiveHint: boolPtr(true)}), + } + + f := newSkewFixtureInMode(t, before, config.DirectToolResponseModeDeferred) + readOnly := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "reader", + AllowedServers: []string{"fs"}, + Permissions: []string{auth.PermRead}, + }) + + require.Contains(t, f.listed(readOnly), "fs__purge", "precondition: visible while still read-tier") + + f.rebuildPaused(t, after, func() { + assert.NotContains(t, f.listed(readOnly), "fs__purge", + "deferred mode must withhold the tool as soon as the destructive registration lands, exactly like full mode") + + result, err := f.registeredHandler(t, "fs__purge")(readOnly, mcp.CallToolRequest{ + Params: mcp.CallToolParams{Name: "fs__purge"}, + }) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, result.Content[0].(mcp.TextContent).Text, "Permission denied") + }) +} + +// directHandleMessage drives one raw JSON-RPC request through a proxy's +// direct server and returns the decoded envelope (result or error). +func directHandleMessage(t *testing.T, f *skewFixture, ctx context.Context, id int, method, params string) map[string]interface{} { + t.Helper() + raw := []byte(`{"jsonrpc":"2.0","id":` + itoa(id) + `,"method":"` + method + `","params":` + params + `}`) + encoded, err := json.Marshal(f.proxy.directServer.HandleMessage(ctx, raw)) + require.NoError(t, err) + var envelope map[string]interface{} + require.NoError(t, json.Unmarshal(encoded, &envelope)) + return envelope +} + +func itoa(i int) string { + // Tiny local helper so this file needs no strconv import for one call site. + if i == 0 { + return "0" + } + neg := i < 0 + if neg { + i = -i + } + var buf [20]byte + pos := len(buf) + for i > 0 { + pos-- + buf[pos] = byte('0' + i%10) + i /= 10 + } + if neg { + pos-- + buf[pos] = '-' + } + return string(buf[pos:]) +} + +// T084 (FR008-G5), the protocol-level proof: inside the origin-flip pause, an +// a-only token's tools/call for the flipped display name through the REAL +// mcp-go protocol path (directServer.HandleMessage, which re-evaluates the +// tool filters at call time — D7) must produce the BYTE-IDENTICAL envelope +// (code, message, data) that the same call gets when "a__b" never existed at +// all, and the caller-supplied name is the only thing echoed in either case. +func TestDirectPublication_InSeamCallEnvelopeMatchesUnregisteredName(t *testing.T) { + const display = "a__b__c" + + oldOrigin := []*config.ToolMetadata{ + skewTool("a", "b__c", "Owned by a", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + newOrigin := []*config.ToolMetadata{ + skewTool("a__b", "c", "Owned by a__b", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + + oldOnly := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "a-only", + AllowedServers: []string{"a"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + + // Fixture 1: the real seam, mid-flip. + f := newSkewFixture(t, oldOrigin) + require.NoError(t, f.proxy.storage.SaveUpstreamServer(&config.ServerConfig{Name: "a__b", Enabled: true})) + require.NoError(t, f.proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "a__b", ToolName: "c", Status: storage.ToolApprovalStatusApproved, + })) + + var seamEnvelope map[string]interface{} + f.rebuildPaused(t, newOrigin, func() { + require.NotNil(t, f.proxy.directServer.HandleMessage(oldOnly, + []byte(`{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-03-26","capabilities":{},"clientInfo":{"name":"t","version":"0"}}}`))) + seamEnvelope = directHandleMessage(t, f, oldOnly, 2, "tools/call", `{"name":"`+display+`","arguments":{}}`) + }) + + // Fixture 2: the display name "a__b__c" is registered under NEITHER + // interpretation at all — server "a" exposes a different tool, and there + // is no "a__b" — the genuinely unregistered-name case. + absentFixtureTools := []*config.ToolMetadata{ + skewTool("a", "unrelated", "Owned by a, unrelated tool", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + g := newSkewFixture(t, absentFixtureTools) + require.NotNil(t, g.proxy.directServer.HandleMessage(oldOnly, + []byte(`{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-03-26","capabilities":{},"clientInfo":{"name":"t","version":"0"}}}`))) + absentEnvelope := directHandleMessage(t, g, oldOnly, 2, "tools/call", `{"name":"`+display+`","arguments":{}}`) + + require.NotNil(t, seamEnvelope["error"], "in-seam call to the now-unauthorized name must be a JSON-RPC error: %v", seamEnvelope) + require.NotNil(t, absentEnvelope["error"], "precondition: %v", absentEnvelope) + + seamErr := seamEnvelope["error"].(map[string]interface{}) + absentErr := absentEnvelope["error"].(map[string]interface{}) + + assert.Equal(t, absentErr["code"], seamErr["code"], "the JSON-RPC error code must match an unregistered name's") + assert.Equal(t, float64(mcp.INVALID_PARAMS), seamErr["code"], "and specifically be INVALID_PARAMS (-32602)") + assert.Equal(t, absentErr["message"], seamErr["message"], + "the message is byte-identical: the caller-supplied name is the only thing echoed, in BOTH fixtures") + assert.Equal(t, absentErr["data"], seamErr["data"], "data must match too") + + // "a__b__c" (the caller-supplied name, which D12 permits echoing) happens + // to contain "a__b" as a raw substring, so the disclosure check targets + // the OLD message's own distinguishing vocabulary — a "server" field, or + // the word "owner" — never that coincidental substring. + rawSeam, err := json.Marshal(seamErr) + require.NoError(t, err) + assert.NotContains(t, string(rawSeam), "owner", "no owner metadata may appear anywhere in the envelope (D12)") + assert.NotContains(t, string(rawSeam), `"server"`) + assert.NotContains(t, string(rawSeam), "does not have access") +} + +// T081 (FR008-G2): during a plain-addition seam, two structurally awkward new +// names — one with a leading "__" (an empty server name) and one with an +// empty raw tool name (withheld at the catalog, FR008-G7) — must never be +// listed for an a-only token, carrying a sentinel to prove no content leaks +// either. +func TestDirectPublication_SeamAddition_StructurallyAwkwardNamesNeverListedOutOfScope(t *testing.T) { + base := []*config.ToolMetadata{ + skewTool("fs", "read", "Read a file", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + f := newSkewFixture(t, base) + + aOnly := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "a-only", + AllowedServers: []string{"a"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + + added := append(base, + // An empty SERVER name: FormatDirectToolName("", "a__review") = + // "__a__review", which fails ParseDirectToolName (the "__" separator + // sits at index 0) — structurally identical to what used to be + // misclassified as a built-in. + skewTool("", "a__review", "SENTINEL_EMPTY_SERVER description", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + // An empty RAW TOOL name (FR008-G7): "b__", withheld at the catalog. + skewTool("b", "", "SENTINEL_EMPTY_RAW_NAME description", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + ) + require.Equal(t, "__a__review", FormatDirectToolName("", "a__review")) + require.Equal(t, "b__", FormatDirectToolName("b", "")) + + f.rebuildPaused(t, added, func() { + listed := f.listed(aOnly) + assert.NotContains(t, listed, "__a__review", "an empty-server-name tool must not be listed for a-only") + assert.NotContains(t, listed, "b__", "an empty-raw-name tool is never even admitted to the catalog") + + for name := range listed { + assert.NotContains(t, name, "SENTINEL", "no sentinel-bearing tool name leaks to a-only") + } + }) +} diff --git a/internal/server/mcp_direct_scope.go b/internal/server/mcp_direct_scope.go index 362925c37..239db6615 100644 --- a/internal/server/mcp_direct_scope.go +++ b/internal/server/mcp_direct_scope.go @@ -13,6 +13,136 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" ) +// directToolStampMetaKey is the _meta key under which renderDirectTools +// stamps a rendered direct-mode tool with the identity of the publication +// that produced it. Namespaced per the MCP _meta convention, mirroring +// aggregatedPromptServerMetaKey. +const directToolStampMetaKey = "app.mcpproxy/direct-tool-identity" + +// directToolStamp is the value stored under directToolStampMetaKey (Spec 105 +// FR-008). +// +// It exists so the scope and callability filters can authorize a rendered +// tool against the EXACT identity of the publication that produced it, +// without a second catalog lookup by display name. A lookup can resolve +// against whatever catalog generation happens to be published RIGHT NOW, +// which — during the SetTools-then-publish window every rebuild opens — can +// be a different generation than the one that rendered and registered this +// particular tool object and its handler (the publication-skew an origin flip +// exploits: "a__b__c" can mean server "a" tool "b__c" in one generation and +// server "a__b" tool "c" in the next, and a catalog lookup mid-window answers +// for whichever generation happens to be live, not the one this tool came +// from). Riding the stamp on the tool itself makes the filter's answer +// inseparable from the tool it is filtering. +// +// A private struct rather than bare strings, mirroring aggregatedPromptStamp, +// so an upstream tool that happens to carry our key in its own _meta can never +// be mistaken for a stamp (a string there does not type-assert to this +// struct), and the upstream's own Meta travels with it for restoration by +// stripDirectToolStamp. Marshalling it directly (which only an unfiltered +// reader could do) yields "{}": the fields are unexported. +type directToolStamp struct { + owner string + rawName string + tier string // requiredPermission ("read"/"write"/"destructive"/"") + upstream *mcp.Meta +} + +// stampDirectTool returns tool with a directToolStamp recording entry's +// identity written into its _meta, preserving whatever _meta the tool already +// carried (there is none today — direct tools are rendered from upstream +// definitions that do not set one — but mirroring stampAggregatedPromptServer +// keeps the two internal-stamp mechanisms symmetric and safe if that ever +// changes). +func stampDirectTool(tool mcp.Tool, entry *directCatalogEntry) mcp.Tool { + meta := &mcp.Meta{AdditionalFields: map[string]any{}} + if tool.Meta != nil { + meta.ProgressToken = tool.Meta.ProgressToken + for k, v := range tool.Meta.AdditionalFields { + meta.AdditionalFields[k] = v + } + } + meta.AdditionalFields[directToolStampMetaKey] = directToolStamp{ + owner: entry.ServerName, + rawName: entry.ToolName, + tier: entry.RequiredPermission, + upstream: tool.Meta, + } + tool.Meta = meta + return tool +} + +// readDirectToolStamp reads the identity stamped by stampDirectTool. ok is +// false for a tool that carries no stamp: every built-in (registered outside +// renderDirectTools), or any mcp.Tool value a caller constructs directly +// rather than through it (as unit tests exercising the discovery filters do). +func readDirectToolStamp(tool mcp.Tool) (directToolStamp, bool) { + if tool.Meta == nil { + return directToolStamp{}, false + } + stamp, ok := tool.Meta.AdditionalFields[directToolStampMetaKey].(directToolStamp) + return stamp, ok +} + +// stripDirectToolStamp returns tool with the internal identity stamp removed +// and its _meta restored to exactly what was there before stampDirectTool ran +// (nil stays nil), so client-visible output never carries mcpproxy's internal +// bookkeeping — for every caller, administrators included (Spec 105 FR-008). +// The registered tool itself is never mutated: mcp-go hands filters the +// stored value, and only the copy's Meta pointer is replaced. +func stripDirectToolStamp(tool mcp.Tool) mcp.Tool { + stamp, ok := readDirectToolStamp(tool) + if !ok { + return tool + } + tool.Meta = stamp.upstream + return tool +} + +// stripDirectToolStampFilter is the TERMINAL tool filter registered on the +// direct server. mcp-go runs WithToolFilter filters in registration order, +// feeding each filter's output to the next (both for tools/list and for the +// call-time re-evaluation of a single tool), so registering this one LAST +// guarantees the scope and callability filters above it still see the stamp +// they authorize against, while every tool that survives them — for every +// caller, including administrators, who never went through those two filters' +// per-tool loop before this change — leaves with it removed. +func stripDirectToolStampFilter(_ context.Context, tools []mcp.Tool) []mcp.Tool { + if len(tools) == 0 { + return tools + } + out := make([]mcp.Tool, len(tools)) + for i, tool := range tools { + out[i] = stripDirectToolStamp(tool) + } + return out +} + +// directIdentityInScope is directEntryInScope's identity-only twin: the same +// scope+tier predicate, evaluated against a bare (owner, tier) pair rather +// than a *directCatalogEntry, so a caller resolving through a stamp (Spec 105 +// FR-008) and a caller resolving through the catalog run the identical check. +func directIdentityInScope( + authCtx *auth.AuthContext, + profileScope *profile.ProfileScope, + isScopedAgent bool, + owner, tier string, +) bool { + if !profileScope.Allows(owner) { + return false + } + if !isScopedAgent { + return true + } + if !authCtx.CanAccessServer(owner) { + return false + } + if tier != "" && !authCtx.HasPermission(tier) { + return false + } + return true +} + // requiredPermissionForDirectTool derives the agent-token permission a direct // tool requires from its annotations. It reuses the same variant->operation-type // mapping that call-time authorization uses (see handleDirectToolCall in @@ -45,12 +175,50 @@ func (p *MCPProxyServer) filterDirectModeToolsForAuth(ctx context.Context, tools authCtx := auth.AuthContextFromContext(ctx) _, profileScope := p.resolveActiveProfile(ctx) isScopedAgent := authCtx != nil && authCtx.Type == auth.AuthTypeAgent - if !isScopedAgent && profileScope == nil { - return tools - } + + // Spec 105 FR-008 (FR008-G7): a tool with no registration identity is + // withheld from EVERY caller, administrators included, so this filter can + // no longer short-circuit for an unscoped/no-profile caller the way it + // used to — that early return skipped the per-tool loop entirely and, with + // it, the withholding check below. filtered := make([]mcp.Tool, 0, len(tools)) for _, tool := range tools { + if stamp, stamped := readDirectToolStamp(tool); stamped { + // Authorize against the identity STAMPED on this exact tool + // object — see directToolStamp's doc comment for why this must + // never be re-derived from a fresh catalog lookup by name. + if stamp.rawName == "" { + // Defense in depth: buildDirectCatalog refuses to admit an + // empty raw name, so a stamp should never carry one. + continue + } + if !directIdentityInScope(authCtx, profileScope, isScopedAgent, stamp.owner, stamp.tier) { + continue + } + filtered = append(filtered, tool) + continue + } + + // No stamp: either a built-in (registered outside renderDirectTools) + // or an mcp.Tool value constructed directly rather than rendered from + // the catalog (unit tests exercising this filter against a published + // catalog). Fall back to the pre-105 name-based resolution unchanged. + // + // Residual (cross-review round 1): mcp-go's SessionWithTools mechanism + // merges a session's OWN tools into the listing under the SAME name a + // global tool may already hold, before any filter runs, and dispatches + // to the session's handler in preference to the global one + // (handleToolCall, server.go). A session tool registered that way would + // arrive here unstamped and fall through to this catalog lookup, which + // could resolve the name against the UNRELATED global entry — the exact + // identity/handler skew this file exists to close, reopened one layer + // up. mcpproxy-go registers no session-specific tools anywhere today + // (grep SessionWithTools/GetSessionTools/AddSessionTools turns up + // nothing outside mcp-go itself), so this is not reachable in + // production; it becomes load-bearing the moment that changes, and + // whatever adds session tools to this surface MUST stamp them too. + // // Resolve through the catalog, NOT by re-parsing the display name. // ParseDirectToolName splits on the first "__", which mis-splits a server // name that itself contains "__" — so this filter could scope-check one @@ -66,17 +234,15 @@ func (p *MCPProxyServer) filterDirectModeToolsForAuth(ctx context.Context, tools continue case directResolveDenied: // A catalog exists and does not admit this name: an unknown tool, or - // one withheld for a display-name collision. Dropping it is the point - // — re-parsing would pick an origin the catalog refused to choose. + // one withheld for a display-name collision, or one refused + // admission for carrying no registration identity (FR008-G7). + // Dropping it is the point — re-parsing would pick an origin the + // catalog refused to choose. continue case directResolveNoCatalog: // Nothing published yet — a proxy still coming up. Fall back to the // pre-catalog behaviour rather than deny, or startup would serve an // empty listing to everyone. - // - // The parse cannot fail here: resolveDirectTool already classified a - // separator-less name as a built-in, so anything reaching this branch - // has a "__" in it. serverName, _, _ := ParseDirectToolName(tool.Name) if !profileScope.Allows(serverName) { continue @@ -121,25 +287,13 @@ func directEntryInScope( if entry == nil { return false } - if !profileScope.Allows(entry.ServerName) { - return false - } - if !isScopedAgent { - return true - } - if !authCtx.CanAccessServer(entry.ServerName) { - return false - } // The tier is the catalog entry's, derived from UPSTREAM annotations // exactly as dispatch derives it. Deriving it from the registered // mcp.Tool would read mcp-go's NewTool defaults — destructiveHint=true on // essentially every tool — and hide the catalog from read- and // write-scoped tokens while dispatch happily allowed the same calls // (D13 rule 3). - if entry.RequiredPermission != "" && !authCtx.HasPermission(entry.RequiredPermission) { - return false - } - return true + return directIdentityInScope(authCtx, profileScope, isScopedAgent, entry.ServerName, entry.RequiredPermission) } // builtinPromptNames is the set of prompt display names mcpproxy serves itself @@ -169,7 +323,10 @@ var builtinPromptNames = map[string]struct{}{ // name: "a__b__c" splits on the first "__" into owner "a", while the handler // dispatches to "a__b", so a token scoped to "a" alone could list and fetch // "a__b"'s prompt. An upstream prompt with no stamp cannot have come from -// buildAggregatedServerPrompts and is dropped for scoped callers (fail closed). +// buildAggregatedServerPrompts and has no registration identity to authorize +// it against, so it is dropped for EVERY caller — administrators and +// unrestricted agent tokens included, not only scoped ones (Spec 105 +// FR-008/FR-006, SC-005 exception) — fail closed. // // The internal stamp is stripped from every prompt returned, for every caller, // so the client-visible _meta is exactly what the upstream sent. @@ -196,17 +353,20 @@ func (p *MCPProxyServer) filterAggregatedPromptsForAuth(ctx context.Context, pro } serverName, stamped := aggregatedPromptServer(prompt) - if enforce { - if !stamped { - if p.logger != nil { - p.logger.Warn("dropping aggregated prompt with no canonical-owner stamp for scoped caller", - zap.String("prompt", prompt.Name)) - } - continue - } - if !allowed(serverName) { - continue + if !stamped { + // Spec 105 FR-008/FR-006: no registration identity to authorize + // against. Withheld from EVERY caller, administrators included + // (SC-005 exception) — this can no longer be conditioned on + // `enforce`, since an unrestricted or administrator caller must + // also never see a no-identity prompt. + if p.logger != nil { + p.logger.Warn("dropping aggregated prompt with no canonical-owner stamp", + zap.String("prompt", prompt.Name)) } + continue + } + if enforce && !allowed(serverName) { + continue } filtered = append(filtered, stripAggregatedPromptServer(prompt)) diff --git a/internal/server/mcp_direct_skew_test.go b/internal/server/mcp_direct_skew_test.go index 8316003bb..108f05a0c 100644 --- a/internal/server/mcp_direct_skew_test.go +++ b/internal/server/mcp_direct_skew_test.go @@ -176,22 +176,25 @@ var skewBase = func() []*config.ToolMetadata { // Group 1 — closed by design (the ordering) // --------------------------------------------------------------------------- -// An ADDED name is in the registry before its catalog entry lands. What a -// session sees then depends on whether its listing goes through the catalog at -// all — and that is NOT uniform: +// An ADDED name is in the registry before its catalog entry lands. // -// - A SCOPED session (agent token or active profile) is filtered through the -// catalog, which does not admit the name yet, so it is denied on both -// sides. This is the case D13 describes. -// - An UNSCOPED session short-circuits both filters -// (mcp_direct_scope.go: `if !isScopedAgent && profileScope == nil { return -// tools }`) and is served the raw registry, so it DOES see the new name -// while describe still answers not_found for it. +// Spec 105 PR F (FR-008, FR008-G1) closes the window this test used to +// document as a residual. Before this fix, both filters resolved a rendered +// tool's identity by looking up its NAME in whatever catalog happened to be +// live — which, in this window, is still the PREVIOUS generation, one that +// does not admit "fs__stat" at all — so a scoped session was denied and an +// unscoped one fell through to the "serve the raw registry" fallback, seeing +// the name with nothing to check it against. // -// Neither leaks: the scoped case denies both, and the unscoped residual is -// listed-but-undescribable — the safe direction, and for a session that is -// entitled to the whole surface anyway. D13's "the filters deny it" is -// therefore true of scoped sessions specifically, not of every session. +// Now renderDirectTools stamps each tool with the identity of the SAME +// catalog build that produced its handler (Spec 105 FR-008), and the filters +// read that stamp first. Since SetTools lands the NEW registry (with its +// stamps) before this pause even starts, every session — scoped or not — sees +// "fs__stat" immediately, correctly attributed to "fs", with no residual left +// to document. describe_tool is untouched by this PR and still answers from +// the published catalog, so it remains one generation behind for the width of +// the window — the SAFE "listed-but-undescribable" direction SC-007 (Spec +// 102) has always permitted, never the reverse. func TestSkew_AddedNameBeforeItsCatalogEntry(t *testing.T) { f := newSkewFixture(t, skewBase()) unscoped := context.Background() @@ -212,14 +215,13 @@ func TestSkew_AddedNameBeforeItsCatalogEntry(t *testing.T) { })) f.rebuildPaused(t, added, func() { - assert.NotContains(t, f.listed(scoped), "fs__stat", - "a scoped session is filtered through the catalog, which has not admitted it yet") - assert.False(t, f.describable(scoped, "fs__stat")) - - assert.Contains(t, f.listed(unscoped), "fs__stat", - "an unscoped session is served the raw registry — documenting the residual") - assert.False(t, f.describable(unscoped, "fs__stat"), - "…and describe still answers not_found: listed-but-undescribable, the safe direction") + assert.Contains(t, f.listed(scoped), "fs__stat", + "the stamp on the newly-registered tool already says owner \"fs\", in scope, so a scoped session sees it immediately") + assert.False(t, f.describable(scoped, "fs__stat"), + "describe still answers from the previous, unpublished catalog — listed-but-undescribable, the safe direction") + + assert.Contains(t, f.listed(unscoped), "fs__stat") + assert.False(t, f.describable(unscoped, "fs__stat")) }) // After the publish both sessions agree, in both directions. @@ -281,9 +283,25 @@ func TestSkew_DescriptionChangeIsSelfConsistentOnBothSides(t *testing.T) { // An ORIGIN FLIP: the SAME display name, owned by a different upstream in the // next generation. Only the "__" ambiguity makes this expressible — // "a__b__c" is (server "a", tool "b__c") or (server "a__b", tool "c") — and it -// is the sharpest form of the skew question, because during the window the -// filters scope-check against the OLD origin while the registry already holds -// the NEW origin's handler. +// is the sharpest form of the skew question. +// +// Spec 105 PR F (FR-008, FR008-G1/G3/G5) closes it. Before this fix, the +// filters scope-checked "a__b__c" by looking its NAME up in whatever catalog +// happened to be live — during this window, still the OLD one, which says the +// owner is "a" — while the registry already held the NEW origin's handler, so +// an old-scoped token saw the tool listed under the wrong owner and was only +// refused once it actually dispatched, by a message that NAMED the new owner +// (D12's disclosure). +// +// Now renderDirectTools stamps each registered tool with the identity of the +// SAME build that produced its handler, and the filters read that stamp +// first — never a fresh catalog lookup. Since SetTools lands the NEW registry +// (new stamp: owner "a__b") before this pause starts, an old-only token is +// excluded from the listing immediately, and if a call is still attempted +// against the stale/registered handler directly (bypassing mcp-go's own +// call-time re-evaluation, which would otherwise answer the SAME +// unregistered-name envelope first — Spec 105 T086a), the refusal it gets +// names neither origin. // // An earlier version of this test flipped alpha__run to beta__run, which are // different display names and therefore not an origin flip at all; it asserted @@ -315,25 +333,41 @@ func TestSkew_OriginFlipNeverSplitsScopeFromDispatch(t *testing.T) { }) f.rebuildPaused(t, newOrigin, func() { - // The stale catalog still says this name belongs to "a", so the filters - // admit it for this token… + // The stale CATALOG still says this name belongs to "a" — describe_tool, + // unchanged by this PR, still resolves through it — but the LISTING no + // longer does: the registered tool's own stamp already says "a__b", + // out of scope for this token, so it is absent immediately. entry, ok := f.proxy.resolveDirectDescribeID(oldOnly, display) - require.True(t, ok, "the stale catalog still resolves the name") - require.Equal(t, "a", entry.ServerName, "…to the OLD origin") - require.Contains(t, f.listed(oldOnly), display, "so it is still listed") - - // …but the registry already holds the NEW origin's handler, and that - // handler re-derives authorization from the entry IT captured. The - // split is closed at the only place it matters: the call is refused, - // against the origin that would actually be dispatched to. + require.True(t, ok, "the stale catalog still resolves the name for describe") + require.Equal(t, "a", entry.ServerName, "…to the OLD origin — a residual describe_tool is out of this PR's scope") + + assert.NotContains(t, f.listed(oldOnly), display, + "the listing reads the NEW stamp on the registered tool, not the stale catalog, so it is excluded immediately") + + // The registry already holds the NEW origin's handler, invoked here + // directly (bypassing mcp-go's own call-time filter re-evaluation, + // which — now that the filters are stamp-based — would already refuse + // this with the SAME envelope a wholly unregistered name gets, before + // the handler ever ran; see TestDirectFullMode_InSeamScopeRefusalMatchesUnregisteredName). + // This is the handler's OWN defense-in-depth check, and D12 forbids it + // from naming either origin. result, err := f.registeredHandler(t, display)(oldOnly, mcp.CallToolRequest{ Params: mcp.CallToolParams{Name: display}, }) require.NoError(t, err) require.True(t, result.IsError, "a token scoped to the old origin must not reach the new one through a stale listing") - assert.Contains(t, result.Content[0].(mcp.TextContent).Text, "does not have access to server 'a__b'", - "the refusal must name the origin actually dispatched to, not the one the listing implied") + text := result.Content[0].(mcp.TextContent).Text + // "a__b__c" (the caller-supplied display name, which D12 permits + // echoing) happens to contain "a__b" as a raw substring, so the + // disclosure check is against the OLD message's own distinguishing + // phrasing — naming the origin AS a server, in a sentence that + // confirms a scope check fired — not against that coincidental + // substring. + assert.Equal(t, "tool 'a__b__c' not found", text, + "the refusal must be worded exactly like an unregistered name's, never naming the origin actually dispatched to (D12)") + assert.NotContains(t, text, "does not have access", "nor disclose that a scope check is what fired") + assert.NotContains(t, text, "Owned by", "nor leak the entry's own description") }) // After the publish the listing agrees with the registry again: the name @@ -457,12 +491,18 @@ func TestSkew_OutputSchemaOnlyChangeIsSilentlyStale(t *testing.T) { assert.Contains(t, entry.OutputSchemaJSON, "bytes") } -// Residual 3 (T003): an ANNOTATIONS-only change — read becoming destructive — -// can be listed and described one generation stale. The compensating property -// is that CALL-TIME authorization never reads the catalog: the handler -// re-derives the tier from the annotations it was registered with, and a -// read-scoped token is refused the call even while the stale listing still -// shows the tool. +// An ANNOTATIONS-only change — read becoming destructive — used to be listed +// one generation stale (Spec 102's "residual 3"), because the listing filter +// re-derived the tier from a fresh catalog lookup, and the catalog is still +// the OLD (read-tier) generation for the width of this window. +// +// Spec 105 PR F closes it: renderDirectTools now stamps each registered tool +// with the tier of the SAME build that produced its handler, so as soon as +// SetTools lands the NEW (destructive-tier) registration — before this pause +// even starts — a read-scoped token's listing reflects it immediately. No +// earlier revocation is required (US3 scenario 3): the withholding tracks the +// handler actually registered at each seam, exactly like the call-time +// authorization already did. func TestSkew_AnnotationsOnlyChangeIsStaleButNeverAdmitsTheCall(t *testing.T) { before := []*config.ToolMetadata{ skewTool("fs", "purge", "Purge", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), @@ -479,12 +519,15 @@ func TestSkew_AnnotationsOnlyChangeIsStaleButNeverAdmitsTheCall(t *testing.T) { }) f.rebuildPaused(t, after, func() { - assert.Contains(t, f.listed(readOnly), "fs__purge", - "documenting residual 3: the read-scoped token still sees the tool, one generation stale") - - // …but the call is refused. Dispatched through the handler mcp-go - // actually holds — building one here would prove only that a handler - // constructed by the test refuses, not that the REGISTERED one does. + assert.NotContains(t, f.listed(readOnly), "fs__purge", + "the newly-registered tool's own stamp already says tier destructive, so a read-scoped token is withheld immediately, not one generation stale") + + // …and the call is refused, exactly as before this PR: call-time + // authorization never reads the catalog OR the stamp — the handler + // re-derives the tier from the annotations it was registered with. + // Dispatched through the handler mcp-go actually holds — building one + // here would prove only that a handler constructed by the test + // refuses, not that the REGISTERED one does. result, err := f.registeredHandler(t, "fs__purge")(readOnly, mcp.CallToolRequest{ Params: mcp.CallToolParams{Name: "fs__purge"}, }) diff --git a/internal/server/mcp_direct_underscore_test.go b/internal/server/mcp_direct_underscore_test.go new file mode 100644 index 000000000..3ac81f37e --- /dev/null +++ b/internal/server/mcp_direct_underscore_test.go @@ -0,0 +1,135 @@ +package server + +import ( + "context" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" +) + +// Spec 105 PR F — FR008-G6: a server literally named "__a". +// +// FR-008 is explicit that this MUST be authorized normally, not withheld for +// being unparseable: "server `__a`'s tool has a recorded owner and tier and +// is authorized normally." Every one of its display names — "__a__review", +// "__a__admin_purge" — fails ParseDirectToolName (the "__" separator sits at +// index 0), which is exactly the shape that used to be misclassified as a +// built-in by structural inference. The catalog decides first, so a real +// registration identity is never denied merely because its name looks like +// that; only a name with NO identity at all (an empty raw tool name, FR008-G7) +// is withheld. + +func underscoreServerTools() []*config.ToolMetadata { + return []*config.ToolMetadata{ + skewTool("__a", "review", "Review a change", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + skewTool("__a", "admin_purge", "Purge everything", `{"type":"object"}`, &config.ToolAnnotations{DestructiveHint: boolPtr(true)}), + } +} + +// TestDirectUnderscoreServer_SteadyState covers listed/describable/dispatchable +// for both an unrestricted agent and an administrator, and withheld + +// -32602 parity for an "a"-only token (which has no relation to "__a" at +// all — it must never be confused with it by any string-matching heuristic). +func TestDirectUnderscoreServer_SteadyState(t *testing.T) { + for _, mode := range []string{config.DirectToolResponseModeFull, config.DirectToolResponseModeDeferred} { + t.Run(mode, func(t *testing.T) { + f := newSkewFixtureInMode(t, underscoreServerTools(), mode) + + wildcard := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "wildcard", + AllowedServers: []string{"*"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + admin := auth.WithAuthContext(context.Background(), auth.AdminContext()) + aOnly := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "a-only", + AllowedServers: []string{"a"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + + for name, ctx := range map[string]context.Context{"wildcard-scoped agent": wildcard, "administrator": admin} { + require.Containsf(t, f.listed(ctx), "__a__review", "%s: a server named __a must be listed normally, never withheld for its shape", name) + require.Truef(t, f.describable(ctx, "__a__review"), "%s", name) + + entry, ok := f.proxy.resolveDirectDescribeID(ctx, "__a__review") + require.Truef(t, ok, "%s", name) + assert.Equalf(t, "__a", entry.ServerName, "%s: identity resolves to its real owner, not a parse artefact", name) + + // Dispatchable: the call reaches PAST every auth/scope/callability + // gate. There is no real upstream connection behind this fixture's + // catalog-only entry (DiscoverTools is what the skew fixture fakes, + // per its own doc comment), so the call still fails — but with an + // UPSTREAM dispatch error, never an auth refusal. + result, err := f.registeredHandler(t, "__a__review")(ctx, mcp.CallToolRequest{ + Params: mcp.CallToolParams{Name: "__a__review"}, + }) + require.NoErrorf(t, err, "%s", name) + if result.IsError { + text := result.Content[0].(mcp.TextContent).Text + assert.NotContainsf(t, text, "not found", "%s: must not be refused as unregistered", name) + assert.NotContainsf(t, text, "Permission denied", "%s: must not be refused for tier", name) + assert.NotContainsf(t, text, "not callable", "%s: must not be refused for callability", name) + } + } + + assert.NotContainsf(t, f.listed(aOnly), "__a__review", "an a-only token has no relation to __a and must not see it") + assert.Falsef(t, f.describable(aOnly, "__a__review"), "a-only") + + result, err := f.registeredHandler(t, "__a__review")(aOnly, mcp.CallToolRequest{ + Params: mcp.CallToolParams{Name: "__a__review"}, + }) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Equal(t, "tool '__a__review' not found", result.Content[0].(mcp.TextContent).Text, + "withheld with the same non-disclosing wording an unregistered name gets, never naming __a (D12)") + }) + } +} + +// TestDirectUnderscoreServer_SeamVariant proves the __a shape survives a +// publication seam exactly like any other server: an added __a tool must +// still be authorized by its real identity, not treated as ambiguous merely +// because it cannot be re-parsed. +func TestDirectUnderscoreServer_SeamVariant(t *testing.T) { + for _, mode := range []string{config.DirectToolResponseModeFull, config.DirectToolResponseModeDeferred} { + t.Run(mode, func(t *testing.T) { + base := []*config.ToolMetadata{ + skewTool("fs", "read", "Read a file", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}), + } + f := newSkewFixtureInMode(t, base, mode) + + wildcard := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "wildcard", + AllowedServers: []string{"*"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + aOnly := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, AgentName: "a-only", + AllowedServers: []string{"a"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + + added := append(base, skewTool("__a", "review", "Review a change", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)})) + require.NoError(t, f.proxy.storage.SaveUpstreamServer(&config.ServerConfig{Name: "__a", Enabled: true})) + require.NoError(t, f.proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "__a", ToolName: "review", Status: storage.ToolApprovalStatusApproved, + })) + + f.rebuildPaused(t, added, func() { + assert.Contains(t, f.listed(wildcard), "__a__review", + "the newly-registered __a tool is stamped with its real identity immediately, mid-seam") + assert.NotContains(t, f.listed(aOnly), "__a__review", + "and an a-only token, unrelated to __a, never sees it") + }) + + assert.Contains(t, f.listed(wildcard), "__a__review") + assert.NotContains(t, f.listed(aOnly), "__a__review") + }) + } +} diff --git a/internal/server/mcp_prompt_scope_test.go b/internal/server/mcp_prompt_scope_test.go index f9bccc478..e6593a30d 100644 --- a/internal/server/mcp_prompt_scope_test.go +++ b/internal/server/mcp_prompt_scope_test.go @@ -129,11 +129,15 @@ func aggregatedPromptForTest(server, prompt string) mcp.Prompt { } } -// TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed (Spec 104 FR-016g): -// an upstream prompt with no canonical-owner stamp cannot have come from -// buildAggregatedServerPrompts, so the filter must not guess its owner from the -// display name (that re-parse is the original leak). It is dropped for scoped -// callers and left alone for unscoped ones. +// TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed (Spec 104 FR-016g; +// Spec 105 FR-006/FR-008 FR008-G7): an upstream prompt with no canonical-owner +// stamp cannot have come from buildAggregatedServerPrompts, so the filter must +// not guess its owner from the display name (that re-parse is the original +// leak). It has no registration identity at all, so it is dropped for EVERY +// caller — scoped, unrestricted agent, and administrator alike (SC-005 +// exception) — never only for a scoped one. Withholding it used to be +// conditioned on `enforce` (scoped agent or active profile), which left it +// visible to an unscoped or administrator caller; that condition is gone. func TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed(t *testing.T) { proxy := &MCPProxyServer{} unstamped := mcp.Prompt{Name: FormatDirectPromptName("github", "looks_in_scope")} @@ -144,12 +148,22 @@ func TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed(t *testing.T) { AllowedServers: []string{"github"}, Permissions: []string{auth.PermRead}, }) + unrestricted := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, + AgentName: "unrestricted-bot", + AllowedServers: []string{"*"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) - got := proxy.filterAggregatedPromptsForAuth(scoped, []mcp.Prompt{unstamped, stamped}) - assert.ElementsMatch(t, []string{stamped.Name}, promptNamesForTest(got), "unstamped upstream prompt is dropped for a scoped caller") - - got = proxy.filterAggregatedPromptsForAuth(context.Background(), []mcp.Prompt{unstamped, stamped}) - assert.ElementsMatch(t, []string{unstamped.Name, stamped.Name}, promptNamesForTest(got), "unscoped callers are not filtered") + for name, ctx := range map[string]context.Context{ + "scoped agent": scoped, + "unrestricted agent": unrestricted, + "administrator": context.Background(), + } { + got := proxy.filterAggregatedPromptsForAuth(ctx, []mcp.Prompt{unstamped, stamped}) + assert.ElementsMatchf(t, []string{stamped.Name}, promptNamesForTest(got), + "%s: an unstamped prompt has no registration identity and is withheld from every caller", name) + } } // TestStripAggregatedPromptServer_PreservesUpstreamMeta verifies the stamp is @@ -405,3 +419,76 @@ func TestAggregatedPrompt_LateEnableStillFiltered(t *testing.T) { }) } } + +// TestBuildAggregatedServerPrompts_DropsEmptyPromptName is Spec 105 FR-008 +// (FR008-G7), the FR-006 prompt analogue: an upstream prompt with an empty +// raw name ("server:") has no registration identity — the direct-surface +// equivalent of an upstream tool named "" — and buildAggregatedServerPrompts +// must never register it at all, for any caller. +func TestBuildAggregatedServerPrompts_DropsEmptyPromptName(t *testing.T) { + upstreamPrompts := []mcp.Prompt{ + {Name: "a:"}, // empty raw prompt name + {Name: "a:review"}, // normal + } + getPrompt := func(_ context.Context, _ string, _ map[string]string) (*mcp.GetPromptResult, error) { + return &mcp.GetPromptResult{}, nil + } + + all := buildAggregatedServerPrompts(nil, upstreamPrompts, getPrompt, nil, nil) + + names := make([]string, 0, len(all)) + for _, sp := range all { + names = append(names, sp.Prompt.Name) + } + assert.NotContains(t, names, "a__", "an empty raw prompt name must never be registered") + assert.Contains(t, names, "a__review", "the sibling prompt from the same server is unaffected") +} + +// TestUnstampedPrompt_WithheldFromListAndGet_ForAdminAndAgent is Spec 105 +// FR008-G7's SC-005 fixture: a prompt with no accepted registration (no +// canonical-owner stamp) is withheld from prompts/list and refused by +// prompts/get, for an agent token AND for an administrator — never only for +// a scoped caller, unlike every other withholding rule in this file. +func TestUnstampedPrompt_WithheldFromListAndGet_ForAdminAndAgent(t *testing.T) { + proxy := createTestMCPProxyServer(t) + proxy.config.EnablePrompts = true + + unstamped := mcp.Prompt{Name: "ghost__unstamped"} + proxy.server.SetPrompts(mcpserver.ServerPrompt{ + Prompt: unstamped, + Handler: func(_ context.Context, _ mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + return &mcp.GetPromptResult{Messages: []mcp.PromptMessage{}}, nil + }, + }) + + agentCtxForTest := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + Type: auth.AuthTypeAgent, + AgentName: "unrestricted", + AllowedServers: []string{"*"}, + Permissions: []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, + }) + adminCtxForTest := auth.WithAuthContext(context.Background(), auth.AdminContext()) + + initMsg := []byte(`{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-03-26","capabilities":{},"clientInfo":{"name":"t","version":"0"}}}`) + + for name, ctx := range map[string]context.Context{"unrestricted agent": agentCtxForTest, "administrator": adminCtxForTest} { + require.NotNilf(t, proxy.server.HandleMessage(ctx, initMsg), "%s", name) + + listEncoded, err := json.Marshal(proxy.server.HandleMessage(ctx, []byte(`{"jsonrpc":"2.0","id":2,"method":"prompts/list","params":{}}`))) + require.NoErrorf(t, err, "%s", name) + var listEnvelope map[string]interface{} + require.NoError(t, json.Unmarshal(listEncoded, &listEnvelope)) + require.Nilf(t, listEnvelope["error"], "%s: prompts/list must succeed: %v", name, listEnvelope) + var listedNames []string + for _, pr := range listEnvelope["result"].(map[string]interface{})["prompts"].([]interface{}) { + listedNames = append(listedNames, pr.(map[string]interface{})["name"].(string)) + } + assert.NotContainsf(t, listedNames, "ghost__unstamped", "%s: an unstamped prompt is withheld from EVERYONE (SC-005)", name) + + getEncoded, err := json.Marshal(proxy.server.HandleMessage(ctx, []byte(`{"jsonrpc":"2.0","id":3,"method":"prompts/get","params":{"name":"ghost__unstamped"}}`))) + require.NoErrorf(t, err, "%s", name) + var getEnvelope map[string]interface{} + require.NoError(t, json.Unmarshal(getEncoded, &getEnvelope)) + assert.NotNilf(t, getEnvelope["error"], "%s: prompts/get must refuse an unstamped prompt: %v", name, getEnvelope) + } +} diff --git a/internal/server/mcp_routing.go b/internal/server/mcp_routing.go index 707e3142c..51354f8ae 100644 --- a/internal/server/mcp_routing.go +++ b/internal/server/mcp_routing.go @@ -131,6 +131,23 @@ func (p *MCPProxyServer) buildDirectModeTools() ([]mcpserver.ServerTool, *direct return p.withDirectBuiltins(p.renderDirectTools(cat)), cat } +// builtinDirectToolNames is the explicit, POSITIVE set of tool names this +// proxy serves on the direct surface itself, as opposed to an upstream +// projection. Populated directly from the same constructors withDirectBuiltins +// registers (buildDescribeToolTool, …) so the two can never drift apart. +// +// Spec 105 FR-008 (FR008-G2): a name is a built-in ONLY when it is in this +// set. Earlier code inferred "built-in" from a display name that failed to +// PARSE as server__tool — but a catalog-admitted upstream tool with an empty +// raw name renders as "server__", which also fails to parse, and would have +// been misclassified as a built-in by that inference alone (see +// TestResolveDirectTool_EmptyToolNameIsNotABuiltin's history). Structural +// inference is gone; only this explicit set — and a successful catalog +// lookup — identify a name. +var builtinDirectToolNames = map[string]struct{}{ + buildDescribeToolTool().Name: {}, +} + // withDirectBuiltins appends the tools mcpproxy serves itself on the direct // surface (FR-009/FR-018). // @@ -207,6 +224,16 @@ func (p *MCPProxyServer) renderDirectTools(cat *directCatalog) []mcpserver.Serve // comparison can honestly be against. entry.RenderedDescription = rendered + // Spec 105 FR-008: stamp the identity of THIS entry — the same one the + // handler below closes over — onto the tool object itself, so the + // scope and callability filters can authorize a tools/list or + // call-time re-evaluation against the exact publication that produced + // this tool, never against whatever catalog happens to be live when + // the filter runs (see directToolStamp's doc comment). The terminal + // stripDirectToolStampFilter removes it before any response reaches a + // client. + mcpTool = stampDirectTool(mcpTool, entry) + serverTools = append(serverTools, mcpserver.ServerTool{ Tool: mcpTool, Handler: p.makeDirectModeHandler(entry), @@ -376,6 +403,16 @@ func (p *MCPProxyServer) directSignatureSuffix(entry *directCatalogEntry) string return "\n" + entry.ToolName + sig.Sig } +// directScopeRefusalMessage is the non-disclosing text makeDirectModeHandler's +// OWN profile/server-scope checks return (Spec 105 FR-008 gap G5, D12): it +// echoes only the caller-supplied display name, mirroring the wording mcp-go +// itself uses for a name that is not registered at all, and never the +// canonical owner the handler's entry closed over. See the call sites for why +// this branch is defense in depth rather than the normal refusal path. +func directScopeRefusalMessage(displayName string) string { + return fmt.Sprintf("tool '%s' not found", displayName) +} + // makeDirectModeHandler creates a handler function for a direct mode tool. // It handles auth checks, permission enforcement, and upstream calls. // @@ -453,7 +490,7 @@ func (p *MCPProxyServer) makeDirectModeHandler(entry *directCatalogEntry) mcpser // connection is filtered too, and it runs FIRST so a profile-pinned // token cannot reach a server outside its pin through this routing mode. if profileScope != nil && !profileScope.Allows(serverName) { - errMsg := fmt.Sprintf("server '%s' is not in profile '%s'", serverName, profileScope.Name) + errMsg := directScopeRefusalMessage(entry.DisplayName) p.emitActivityPolicyDecision(ctx, serverName, toolName, sessionID, requestID, "blocked", errMsg, telemetry.BlockReasonProfileScope) return mcp.NewToolResultError(errMsg), nil } @@ -463,7 +500,19 @@ func (p *MCPProxyServer) makeDirectModeHandler(entry *directCatalogEntry) mcpser if authCtx != nil { // Check server access if !authCtx.CanAccessServer(serverName) { - errMsg := fmt.Sprintf("Access denied: token does not have access to server '%s'", serverName) + // Spec 105 FR-008 gap G5 (D12): never name the server this + // handler's OWN entry closed over. In normal operation this + // branch is unreachable — mcp-go's WithToolFilter chain + // re-evaluates the SAME stamp-based scope decision at call + // time and answers the registered-name-not-found envelope + // before this handler ever runs (Spec 105 T087/T088) — so it + // exists only as defense in depth for a caller that invokes a + // registered handler directly, bypassing that re-evaluation + // (tests, and any future direct-dispatch path). Its wording + // must therefore match what an unregistered name gets: no + // owner, no scope reason, just the caller-supplied name + // echoed back. + errMsg := directScopeRefusalMessage(entry.DisplayName) // Direct mode denied these silently: no activity record and, // since issue #969, no availability counter either. Emit the // same policy decision the call_tool_* variants emit at the @@ -1020,6 +1069,13 @@ func (p *MCPProxyServer) initRoutingModeServers() { directOpts = append(directOpts, mcpserver.WithToolFilter(p.filterDirectModeToolsForAuth), mcpserver.WithToolFilter(p.filterDirectToolsForAgentCallability), + // Spec 105 FR-008: TERMINAL filter, registered last so it runs after + // the two above — mcp-go feeds each filter's output to the next, both + // for tools/list and for the call-time re-evaluation of one tool — and + // removes the internal identity stamp those two authorize against, for + // EVERY caller including administrators, before any tool reaches the + // wire. + mcpserver.WithToolFilter(stripDirectToolStampFilter), // FR-007: the in-band convention channel. Until now no routing-mode // server carried instructions at all — only the default retrieve_tools // server did — so this changes the direct server's initialize response. @@ -1170,15 +1226,22 @@ func (p *MCPProxyServer) refreshDirectModeToolsLocked() { // What the window actually exposes, measured in mcp_direct_skew_test.go // rather than assumed: // - // - An ADDED name is in the registry first. A SCOPED session is filtered - // through the catalog and sees nothing; an UNSCOPED one short-circuits - // both filters and is served the raw registry, so it sees the name while - // describe still answers not_found. Listed-but-undescribable — the safe - // direction, for a session entitled to the whole surface anyway. - // - A REMOVED name leaves the registry first, so the previous catalog can - // still describe it for the width of the window. Stale, not a - // disclosure: the same session could have described it one request - // earlier, and gets the definition it was already served. + // - Spec 105 FR-008: renderDirectTools stamps each rendered tool with the + // identity of the SAME build that produced its handler (Spec 105 + // FR-008), and the LISTING filters read that stamp first, never a fresh + // catalog lookup — so an ADDED, REMOVED, reverse-flipped or tier-changed + // name is authorized correctly for every caller, scoped or not, as soon + // as SetTools lands it, with no window at all. + // - describe_tool is UNCHANGED by that fix and still resolves against + // whichever catalog generation `p.loadDirectCatalog()` currently + // returns, so it can lag the listing for the width of this window — + // two OPPOSITE, both accepted, transient cross-generation residuals + // that close at the publish: an added name is listed but not yet + // describable (the direction Spec 102's SC-007 forbids in steady + // state, tolerated here only for this one-rebuild window), and a + // removed name is still describable from the previous snapshot after + // it drops off the listing (stale, not a disclosure — the same + // session could have described it one request earlier). // // Both close at the publish. The three accepted residuals (T002/T003) are // the schema- and annotations-only changes, which are invisible in the @@ -1384,6 +1447,19 @@ func buildAggregatedServerPrompts( if !ok { continue } + if promptName == "" { + // Spec 105 FR-008 (FR008-G7), the FR-006 prompt analogue: an + // upstream prompt with an empty raw name (a qualified name of + // "server:") has no registration identity to authorize it + // against — the same rule buildDirectCatalog now applies to an + // empty raw TOOL name. Withheld from every caller, administrators + // included, by never registering it at all. + if logger != nil { + logger.Warn("dropping aggregated prompt with an empty raw name: no registration identity to authorize it against", + zap.String("server", serverName)) + } + continue + } displayName := FormatDirectPromptName(serverName, promptName) if _, dup := seen[displayName]; dup { diff --git a/internal/server/mcp_routing_test.go b/internal/server/mcp_routing_test.go index 9837922e5..8bc65831a 100644 --- a/internal/server/mcp_routing_test.go +++ b/internal/server/mcp_routing_test.go @@ -433,8 +433,16 @@ func TestDirectModeHandler_ServerAccessDenied(t *testing.T) { // An agent token restricted to github, targeting gitlab's tool of the // same raw name: the server-scope gate refuses before any tier is read, // and neither upstream sees the call. + // + // Spec 105 FR-008 gap G5 (D12): the refusal text must not name "gitlab" — + // f.call drives the REGISTERED handler directly, bypassing mcp-go's own + // call-time filter re-evaluation (which would answer the unregistered-name + // envelope first in real dispatch), so this exercises the handler's own + // defense-in-depth check. result := f.call(t, agentCtx([]string{"github"}, []string{auth.PermRead}, ""), "gitlab", "list_repos") - f.refused(t, result, "Access denied: token does not have access to server 'gitlab'") + f.refused(t, result, "tool 'gitlab__list_repos' not found") + assert.NotContains(t, result.Content[0].(mcp.TextContent).Text, "does not have access", + "the refusal must never disclose that a scope check is what fired") } func TestDirectModeHandler_AgentWithCorrectPermissions(t *testing.T) { @@ -631,7 +639,14 @@ func TestFilterDirectModeToolsForAuth_FailsClosedOnMissingPermissionMetadata(t * assert.Equal(t, []string{visible}, directToolNamesForTest(filtered)) } -func TestFilterDirectModeToolsForAuth_KeepsNonDirectTools(t *testing.T) { +// Spec 105 FR-008 (FR008-G2): "retrieve_tools" has no "__" separator, exactly +// like a genuine direct-surface built-in, but it is a RETRIEVE-surface +// built-in never registered here — so with a published catalog that does not +// admit it, it is no longer waved through on the strength of its shape alone. +// It has no registration identity and is withheld, for a scoped agent and for +// an administrator alike (the previous version of this test, named for the +// opposite behaviour, pinned exactly the disclosure FR008-G2 closes). +func TestFilterDirectModeToolsForAuth_DropsNonBuiltinSeparatorlessNames(t *testing.T) { proxy := &MCPProxyServer{} direct := FormatDirectToolName("github", "get_issue") @@ -640,19 +655,24 @@ func TestFilterDirectModeToolsForAuth_KeepsNonDirectTools(t *testing.T) { direct: auth.PermRead, }) - ctx := auth.WithAuthContext(context.Background(), &auth.AuthContext{ + scoped := auth.WithAuthContext(context.Background(), &auth.AuthContext{ Type: auth.AuthTypeAgent, AgentName: "test-agent", AllowedServers: []string{"github"}, Permissions: []string{auth.PermRead}, }) - filtered := proxy.filterDirectModeToolsForAuth(ctx, []mcp.Tool{ - {Name: direct}, - {Name: nonDirect}, - }) + for name, ctx := range map[string]context.Context{ + "scoped agent": scoped, + "administrator": context.Background(), + } { + filtered := proxy.filterDirectModeToolsForAuth(ctx, []mcp.Tool{ + {Name: direct}, + {Name: nonDirect}, + }) - assert.Equal(t, []string{direct, nonDirect}, directToolNamesForTest(filtered)) + assert.Equalf(t, []string{direct}, directToolNamesForTest(filtered), "%s", name) + } } func directToolNamesForTest(tools []mcp.Tool) []string { diff --git a/internal/server/profile_pin_enforcement_test.go b/internal/server/profile_pin_enforcement_test.go index c3016646e..d480f4617 100644 --- a/internal/server/profile_pin_enforcement_test.go +++ b/internal/server/profile_pin_enforcement_test.go @@ -115,7 +115,12 @@ func TestDirectModeHonorsTokenProfilePin(t *testing.T) { result, err := handler(ctx, mcp.CallToolRequest{}) require.NoError(t, err) require.True(t, result.IsError, "a call outside the pinned profile must be refused") - assert.Contains(t, resultText(t, result), "is not in profile 'research'") + // Spec 105 FR-008 gap G5 (D12): the refusal must not name the profile-out + // server "deploy-srv" — invoking the registered handler directly, as this + // test does, exercises its defense-in-depth check, which now echoes only + // the caller-supplied tool name, matching an unregistered name's wording. + assert.Contains(t, resultText(t, result), "tool 'deploy-srv__ship' not found") + assert.NotContains(t, resultText(t, result), "is not in profile") // Profile deleted → deny-all on both discovery and dispatch. cfg.Profiles = nil diff --git a/specs/105-agent-scope-hardening/tasks.md b/specs/105-agent-scope-hardening/tasks.md index 81cd57f03..0f4f9a7d0 100644 --- a/specs/105-agent-scope-hardening/tasks.md +++ b/specs/105-agent-scope-hardening/tasks.md @@ -209,30 +209,30 @@ ### Failing tests -- [ ] T080 [US3] FR008-G1: `newSkewFixture`, old `{a, b__c, S1}` → new `{a__b, c, S2}`; `a`-only `listed()` during pause → `a__b__c` absent, no `[a__b]` description, schema `S1`; both modes; `*` + admin controls — `internal/server/mcp_direct_skew_test.go` -- [ ] T081 [P] [US3] FR008-G2: `rebuildPaused` adding `__a__review` / `b__` with sentinel → not listed for `a`-only during seam; positive-identification test: `describe_tool` is Builtin via the explicit set, `__x__y` and a registry tool named `retrieve_tools` are NOT — `internal/server/mcp_direct_catalog_publish_test.go` + `internal/server/mcp_direct_catalog_test.go` -- [ ] T082 [P] [US3] FR008-G3: reverse flip `a__b → a` and plain addition `b__x`; `a`-only / `b`-only token sees the tool during the window — `internal/server/mcp_direct_skew_test.go` -- [ ] T083 [P] [US3] FR008-G4: `rebuildPaused` ReadOnly → Destructive; `{read}` token not listed during seam; handler IsError `Permission denied`; full + deferred — `internal/server/mcp_direct_skew_test.go` -- [ ] T084 [P] [US3] FR008-G5: inside `rebuildPaused`, `tools/call a__b__c` through `directServer.HandleMessage` as `a`-only → the WHOLE JSON-RPC envelope (code `-32602`, message, data) is byte-equal to the envelope for the same requested name `a__b__c` in the fixture where `a__b` is absent (the caller-supplied name is echoed in both); assert no owner metadata (`server`, `owner`, canonical id) appears anywhere in the envelope (D12) — `internal/server/mcp_direct_skew_test.go` -- [ ] T085 [P] [US3] FR008-G6: `__a` server fixture — steady state listed/describable/dispatchable for `*` + admin, withheld + `-32602` parity for `a`-only; seam variant via `rebuildPaused`; both modes — `internal/server/mcp_direct_underscore_test.go` (new) -- [ ] T086 [P] [US3] FR008-G7 + FR-006 consumer: `buildDirectCatalog([{a, ""}]).Len() == 0`; filter omits `a__` for admin and agent; prompt aggregation drops empty prompt names; an UNSTAMPED prompt (no accepted registration) is withheld from `prompts/list` and refused by `prompts/get` for admin AND agent (admin outcome recorded, SC-005) — `internal/server/mcp_direct_catalog_test.go` + `internal/server/mcp_prompt_scope_test.go` -- [ ] T086a [P] [US3] Protocol-level proof: `tools/list` + `tools/call` through `directServer.HandleMessage` show (a) no stamp on the wire for admin and agent, (b) the scope filter is re-evaluated at `tools/call` (hidden registered tool → `-32602`) — the mcp-go behaviour the plan relies on (D7) — `internal/server/mcp_direct_protocol_test.go` (new) +- [x] T080 [US3] FR008-G1: `newSkewFixture`, old `{a, b__c, S1}` → new `{a__b, c, S2}`; `a`-only `listed()` during pause → `a__b__c` absent, no `[a__b]` description, schema `S1`; both modes; `*` + admin controls — `internal/server/mcp_direct_skew_test.go`, `internal/server/mcp_direct_publication_identity_test.go` +- [x] T081 [P] [US3] FR008-G2: `rebuildPaused` adding `__a__review` / `b__` with sentinel → not listed for `a`-only during seam; positive-identification test: `describe_tool` is Builtin via the explicit set, `__x__y` and a registry tool named `retrieve_tools` are NOT — `internal/server/mcp_direct_catalog_publish_test.go` + `internal/server/mcp_direct_catalog_test.go` + `internal/server/mcp_direct_publication_identity_test.go` +- [x] T082 [P] [US3] FR008-G3: reverse flip `a__b → a` and plain addition `b__x`; `a`-only / `b`-only token sees the tool during the window — `internal/server/mcp_direct_publication_identity_test.go` +- [x] T083 [P] [US3] FR008-G4: `rebuildPaused` ReadOnly → Destructive; `{read}` token not listed during seam; handler IsError `Permission denied`; full + deferred — `internal/server/mcp_direct_skew_test.go` (full) + `internal/server/mcp_direct_publication_identity_test.go` (deferred) +- [x] T084 [P] [US3] FR008-G5: inside `rebuildPaused`, `tools/call a__b__c` through `directServer.HandleMessage` as `a`-only → the WHOLE JSON-RPC envelope (code `-32602`, message, data) is byte-equal to the envelope for the same requested name `a__b__c` in the fixture where `a__b` is absent (the caller-supplied name is echoed in both); assert no owner metadata (`server`, `owner`, canonical id) appears anywhere in the envelope (D12) — `internal/server/mcp_direct_publication_identity_test.go` +- [x] T085 [P] [US3] FR008-G6: `__a` server fixture — steady state listed/describable/dispatchable for `*` + admin, withheld + `-32602` parity for `a`-only; seam variant via `rebuildPaused`; both modes — `internal/server/mcp_direct_underscore_test.go` (new) +- [x] T086 [P] [US3] FR008-G7 + FR-006 consumer: `buildDirectCatalog([{a, ""}]).Len() == 0`; filter omits `a__` for admin and agent; prompt aggregation drops empty prompt names; an UNSTAMPED prompt (no accepted registration) is withheld from `prompts/list` and refused by `prompts/get` for admin AND agent (admin outcome recorded, SC-005) — `internal/server/mcp_direct_catalog_test.go` + `internal/server/mcp_prompt_scope_test.go` +- [x] T086a [P] [US3] Protocol-level proof: `tools/list` + `tools/call` through `directServer.HandleMessage` show (a) no stamp on the wire for admin and agent, (b) the scope filter is re-evaluated at `tools/call` (hidden registered tool → `-32602`) — the mcp-go behaviour the plan relies on (D7) — `internal/server/mcp_direct_protocol_test.go` (new) ### Implementation -- [ ] T087 [US3] `directToolStamp{Owner, RawName, Tier}` private struct written into `mcp.Tool.Meta` by `renderDirectTools`; `builtinDirectToolNames` populated from the built-in constructors (`buildDescribeToolTool().Name` etc.) at `internal/server/mcp_routing.go:143-148,156-311` -- [ ] T088 [US3] Scope filter and callability filter read the stamp first (neither removes it), consult the catalog only for unstamped entries, withhold unstamped non-built-ins; a TERMINAL third `WithToolFilter` (`stripDirectToolStamp`, registered last) removes the stamp for EVERY caller (remove the admin early-return at `internal/server/mcp_direct_scope.go:48-50`); `filterAggregatedPromptsForAuth` withholds unstamped prompts regardless of `enforce` and `prompts/get` refuses them (FR-006) — `internal/server/mcp_direct_scope.go:40-143,185-212`, `internal/server/mcp_direct_callability.go:49-92`, `internal/server/mcp_routing.go:958-961` -- [ ] T089 [US3] Catalog: remove parse-failure → Builtin inference; withhold empty raw name; tier from the producing entry — `internal/server/mcp_direct_catalog.go:151-224,374-423`; direct handler in-seam scope refusal returns the error mcp-go maps to the SAME `-32602` envelope as an unregistered name (replace the `NewToolResultError` branches at `internal/server/mcp_routing.go:423-440`), never naming the captured owner (D12) -- [ ] T090 [P] [US3] Prompt aggregation drops empty prompt names (`internal/server/mcp_routing.go:1322-1326`) and `prompts/get` refuses them (`internal/server/mcp.go:900`) +- [x] T087 [US3] `directToolStamp{Owner, RawName, Tier}` private struct written into `mcp.Tool.Meta` by `renderDirectTools`; `builtinDirectToolNames` populated from the built-in constructors (`buildDescribeToolTool().Name` etc.) at `internal/server/mcp_routing.go:143-148,156-311` +- [x] T088 [US3] Scope filter and callability filter read the stamp first (neither removes it), consult the catalog only for unstamped entries, withhold unstamped non-built-ins; a TERMINAL third `WithToolFilter` (`stripDirectToolStamp`, registered last) removes the stamp for EVERY caller (remove the admin early-return at `internal/server/mcp_direct_scope.go:48-50`); `filterAggregatedPromptsForAuth` withholds unstamped prompts regardless of `enforce` and `prompts/get` refuses them (FR-006) — `internal/server/mcp_direct_scope.go:40-143,185-212`, `internal/server/mcp_direct_callability.go:49-92`, `internal/server/mcp_routing.go:958-961` +- [x] T089 [US3] Catalog: remove parse-failure → Builtin inference; withhold empty raw name; tier from the producing entry — `internal/server/mcp_direct_catalog.go:151-224,374-423`; direct handler in-seam scope refusal returns the error mcp-go maps to the SAME `-32602` envelope as an unregistered name (replace the `NewToolResultError` branches at `internal/server/mcp_routing.go:423-440`), never naming the captured owner (D12) +- [x] T090 [P] [US3] Prompt aggregation drops empty prompt names (`internal/server/mcp_routing.go:1322-1326`) and `prompts/get` refuses them (via `filterAggregatedPromptsForAuth`'s unconditional stamp check, `internal/server/mcp_direct_scope.go`, which mcp-go re-evaluates on `prompts/get` too) ### Inverted pinned tests -- [ ] T091 [US3] Invert `internal/server/mcp_direct_skew_test.go:195-224,316-332,459-489`, `internal/server/mcp_direct_catalog_publish_test.go:65-101,155`, `internal/server/mcp_direct_catalog_test.go:177-235`; `direct_full_prefeature.golden.json` and `TestDirectModes_SetIdentity`/`toolSetFingerprint` stay byte-exact +- [x] T091 [US3] Invert `internal/server/mcp_direct_skew_test.go:195-224,316-332,459-489`, `internal/server/mcp_direct_catalog_publish_test.go:65-101,155`, `internal/server/mcp_direct_catalog_test.go:177-235`; `direct_full_prefeature.golden.json` and `TestDirectModes_SetIdentity`/`toolSetFingerprint` stay byte-exact. Also inverted, discovered red by the full suite run and not in the original enumeration above: `internal/server/mcp_routing_test.go` (`TestDirectModeHandler_ServerAccessDenied`, `TestFilterDirectModeToolsForAuth_KeepsNonDirectTools` → renamed `TestFilterDirectModeToolsForAuth_DropsNonBuiltinSeparatorlessNames`), `internal/server/profile_pin_enforcement_test.go` (`TestDirectModeHonorsTokenProfilePin`), `internal/server/mcp_prompt_scope_test.go` (`TestFilterAggregatedPromptsForAuth_UnstampedFailsClosed`) — all pinned the D12 disclosure or the `enforce`-gated prompt withholding this PR closes. ### Verification -- [ ] T092 [US3] Common verification; Spec 102 SC-007 listing/describe parity tests green -- [~] T093 [US3] Astra rounds on FR-008 + FR008-G1…G7 + D7; quote final `VERDICT:` +- [x] T092 [US3] Common verification; Spec 102 SC-007 listing/describe parity tests green — `go build ./...`, `go vet ./...` clean; `go test ./internal/server/...` (full package, no `-skip` needed beyond the standard CI regex) green, `-race` green; `golangci-lint` (bare + `--build-tags server`) clean on every touched file +- [x] T093 [US3] Cross-model review — opencode (`gpt-5.6-terra`, then `gpt-5.6-sol`) reported quota exhaustion on both models (confirmed, not assumed); fell back to `codex exec -m gpt-5.6-sol --sandbox read-only` per the CLAUDE.md ladder. 3 rounds: round 1 found 1 SHOULD-FIX (an mcp-go `SessionWithTools` interaction with the unstamped-tool fallback path — verified unreachable in production via repo-wide grep, since mcpproxy-go registers no session-specific tools anywhere; documented as a residual with a code comment rather than a behavioral change, which round 2 confirmed as accurate and acceptable) + 2 NITs (stale doc comments describing pre-PR-F behavior); round 2 found 1 more NIT (a comment mixing up which cross-generation residual is the SC-007-forbidden direction); round 3: `VERDICT: clean`. --- From d61a1f5b2f034b426fac1e33b8ab88b3fe915da2 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sun, 20 Sep 2026 15:54:28 +0300 Subject: [PATCH 2/2] fix(scope): converge direct-mode block-reason precedence and D12 refusal shape (Spec 105 PR F follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial re-review of PR #1326 (codex exec, gpt-5.6-sol) found two real issues in the direct-publication scope hardening, both fixed here, plus test-hardening gaps the same pass flagged: 1. directBlockReasonKey (telemetry) and directToolCallabilityResult (the response body) disagreed on which gate "won" when a tool was BOTH config-denied and pending/changed-approval at once. Both now share one classifyDirectRefusal helper with a single precedence order (quarantine -> approval-lock -> config-denied/generic), matching the order every other dispatch path already uses (toolGate.lockStatus in mcp.go's handleCallToolVariant/handleCallTool). 2. makeDirectModeHandler's own D12 defense-in-depth scope refusal built a mcp.NewToolResultError (a successful result with isError:true) for a case mcp-go's own call-time filter refuses with a protocol-level -32602 error — a different envelope KIND, not just different wording. It now returns the handler's own error, wrapping mcp-go's ErrToolNotFound, so the envelope kind and text converge (the JSON-RPC error CODE cannot be matched from a ToolHandlerFunc — mcp-go always maps a handler error to INTERNAL_ERROR — documented as an accepted residual, mirroring the already-accepted prompt-side twin in authorizeAggregatedPromptServer). 3. Test hardening: a forged-stamp regression test for directToolStamp (the tool-side analogue of the existing prompt-side test), a positive control on the unstamped-prompt withholding test, an exact-match (not Contains) assertion on the profile-pin non-disclosure check, and full JSON-RPC envelope equality (not just error code + substring) in TestDirectProtocol_StampNeverOnWire_FilterReEvaluatedAtCallTime. go build/vet, race suite (internal/server, internal/server/tokens, internal/serveredition/..., internal/config, internal/oauth, internal/httpapi, internal/storage), and golangci-lint (bare + --build-tags server) all clean. Co-Authored-By: Claude Sonnet 5 --- internal/server/mcp_direct_callability.go | 81 ++++++++++++++----- .../server/mcp_direct_callability_test.go | 17 ++++ internal/server/mcp_direct_protocol_test.go | 46 +++++++++++ internal/server/mcp_direct_scope_test.go | 63 +++++++++++++++ internal/server/mcp_direct_skew_test.go | 18 +++-- internal/server/mcp_direct_underscore_test.go | 6 +- internal/server/mcp_prompt_scope_test.go | 50 ++++++++++-- internal/server/mcp_routing.go | 77 +++++++++++++----- internal/server/mcp_routing_test.go | 26 ++++-- internal/server/preflight_telemetry_test.go | 47 +++++++++++ .../server/profile_pin_enforcement_test.go | 21 +++-- 11 files changed, 383 insertions(+), 69 deletions(-) create mode 100644 internal/server/mcp_direct_scope_test.go diff --git a/internal/server/mcp_direct_callability.go b/internal/server/mcp_direct_callability.go index a27e244fa..f94a09ced 100644 --- a/internal/server/mcp_direct_callability.go +++ b/internal/server/mcp_direct_callability.go @@ -157,17 +157,62 @@ func (p *MCPProxyServer) directToolCallabilityBlockWithReason(ctx context.Contex return p.directToolCallabilityResult(ctx, decision, args), directBlockReasonKey(decision) } +// directRefusalKind is the ONE precedence order a direct-mode callability +// block resolves to, shared by directBlockReasonKey (telemetry) and +// directToolCallabilityResult (the response body) so the two can never +// disagree about which gate "fired" for a tool that trips more than one at +// once (PR #1326 review round 2, chunk B: a tool can be BOTH config-denied +// AND pending/changed approval at the same time, and the two functions used +// to classify that case differently — the response said config-denied, the +// telemetry said pending). +// +// The order mirrors the established dispatch precedence every OTHER path +// already uses (handleCallToolVariant / handleCallTool in mcp.go, via +// toolGate.lockStatus): quarantine, then the approval lock (pending/changed), +// then the generic/config-denied block. Direct mode's RESPONSE function had +// drifted from that precedence (config-denied was checked before the +// approval lock); this converges it rather than inventing a third order. +type directRefusalKind int + +const ( + directRefusalNone directRefusalKind = iota + directRefusalQuarantined + directRefusalPending + directRefusalChanged + directRefusalConfigDenied + directRefusalGeneric +) + +// classifyDirectRefusal is the single source of truth for which refusal a +// blocked directCallabilityDecision represents. Both directBlockReasonKey and +// directToolCallabilityResult switch on its result instead of re-deriving +// their own branch order, so they cannot drift apart again. +func classifyDirectRefusal(decision directCallabilityDecision) directRefusalKind { + switch { + case decision.serverConfig != nil && decision.serverConfig.Quarantined: + return directRefusalQuarantined + case decision.approvalStatus == storage.ToolApprovalStatusPending: + return directRefusalPending + case decision.approvalStatus == storage.ToolApprovalStatusChanged: + return directRefusalChanged + case decision.configDenied: + return directRefusalConfigDenied + default: + return directRefusalGeneric + } +} + // directBlockReasonKey classifies a direct-mode callability block onto the -// closed telemetry.BlockReason* enum. The branches mirror -// directToolCallabilityResult exactly, so the counted reason always matches the +// closed telemetry.BlockReason* enum, from the SAME classification +// directToolCallabilityResult uses, so the counted reason always matches the // payload the caller was handed. func directBlockReasonKey(decision directCallabilityDecision) string { - switch { - case decision.serverConfig != nil && decision.serverConfig.Quarantined: + switch classifyDirectRefusal(decision) { + case directRefusalQuarantined: return telemetry.BlockReasonServerQuarantined - case decision.approvalStatus == storage.ToolApprovalStatusPending: + case directRefusalPending: return telemetry.BlockReasonToolPendingApproval - case decision.approvalStatus == storage.ToolApprovalStatusChanged: + case directRefusalChanged: return telemetry.BlockReasonToolChanged default: // Disabled server, config-denied tool, per-tool disable, and the @@ -312,22 +357,16 @@ func (e *directCallabilityEvaluator) getToolApproval(serverName, toolName string } func (p *MCPProxyServer) directToolCallabilityResult(ctx context.Context, decision directCallabilityDecision, args map[string]interface{}) *mcp.CallToolResult { - if decision.serverConfig != nil && decision.serverConfig.Quarantined { + switch classifyDirectRefusal(decision) { + case directRefusalQuarantined: return p.handleQuarantinedToolCall(ctx, decision.serverName, decision.toolName, args) - } - - if decision.configDenied { + case directRefusalPending: + return toolPendingApprovalResult(decision.serverName, decision.toolName, decision.approval) + case directRefusalChanged: + return toolChangedApprovalResult(decision.serverName, decision.toolName, decision.approval) + case directRefusalConfigDenied: return mcp.NewToolResultError(blockedToolMessageFor(true)) + default: + return mcp.NewToolResultError(p.blockedToolMessage(decision.serverName, decision.toolName)) } - - if decision.approval != nil { - switch decision.approvalStatus { - case storage.ToolApprovalStatusPending: - return toolPendingApprovalResult(decision.serverName, decision.toolName, decision.approval) - case storage.ToolApprovalStatusChanged: - return toolChangedApprovalResult(decision.serverName, decision.toolName, decision.approval) - } - } - - return mcp.NewToolResultError(p.blockedToolMessage(decision.serverName, decision.toolName)) } diff --git a/internal/server/mcp_direct_callability_test.go b/internal/server/mcp_direct_callability_test.go index e1543979d..993d79d3c 100644 --- a/internal/server/mcp_direct_callability_test.go +++ b/internal/server/mcp_direct_callability_test.go @@ -55,6 +55,23 @@ func TestDirectToolCallabilityBlock_ConfigDeniedTool(t *testing.T) { Enabled: true, DisabledTools: []string{"delete_repo"}, })) + // A pre-existing APPROVED record isolates this test's target: the tool + // case (config-denied) from the approval-lock gate. Without one, a fresh + // direct-mode evaluation with no approval record at all synthesizes an + // implicit "pending" record while the quarantine gate is active (Spec 105 + // FR-009), and — per PR #1326 review round 2 finding #1 — the approval + // lock now correctly wins over a plain config denial, matching the + // established handleCallToolVariant/handleCallTool precedence + // (toolGate.lockStatus checked before the generic/config-denied block). + // That combined scenario is covered by + // TestDirectBlockReasonKey_AgreesWithResponse_ConfigDeniedAndApprovalLocked + // in preflight_telemetry_test.go; this test isolates the config-denied + // response body in the case that ambiguity does not arise. + require.NoError(t, proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "github", + ToolName: "delete_repo", + Status: storage.ToolApprovalStatusApproved, + })) result := proxy.directToolCallabilityBlock(context.Background(), "github", "delete_repo", map[string]interface{}{}) require.NotNil(t, result) diff --git a/internal/server/mcp_direct_protocol_test.go b/internal/server/mcp_direct_protocol_test.go index 637320ca4..659f68993 100644 --- a/internal/server/mcp_direct_protocol_test.go +++ b/internal/server/mcp_direct_protocol_test.go @@ -3,6 +3,7 @@ package server import ( "context" "encoding/json" + "strings" "testing" "github.com/mark3labs/mcp-go/mcp" @@ -76,7 +77,52 @@ func TestDirectProtocol_StampNeverOnWire_FilterReEvaluatedAtCallTime(t *testing. var callEnvelope map[string]interface{} require.NoError(t, json.Unmarshal(callEncoded, &callEnvelope)) require.NotNil(t, callEnvelope["error"], "a hidden REGISTERED tool must still be refused at call time: %v", callEnvelope) + require.Nil(t, callEnvelope["result"], "a refusal must never carry a result alongside the error") callErr := callEnvelope["error"].(map[string]interface{}) assert.Equal(t, float64(mcp.INVALID_PARAMS), callErr["code"]) assert.Contains(t, callErr["message"], "not found") + + // PR #1326 review round 2, chunk C/finding #3: full envelope equality, not + // just error code + substring. A hidden-but-registered tool's refusal must + // be BYTE-IDENTICAL in shape and wording to what mcp-go answers for a name + // that was never registered at all — the whole point of D12 is that a + // caller cannot distinguish "authorized-but-blocked" from "genuinely + // doesn't exist". + unregisteredEncoded, err := json.Marshal(f.proxy.directServer.HandleMessage(aOnly, + []byte(`{"jsonrpc":"2.0","id":4,"method":"tools/call","params":{"name":"totally__unregistered","arguments":{}}}`))) + require.NoError(t, err) + + var unregisteredEnvelope map[string]interface{} + require.NoError(t, json.Unmarshal(unregisteredEncoded, &unregisteredEnvelope)) + require.NotNil(t, unregisteredEnvelope["error"], "a genuinely unregistered name must be refused too: %v", unregisteredEnvelope) + require.Nil(t, unregisteredEnvelope["result"]) + unregisteredErr := unregisteredEnvelope["error"].(map[string]interface{}) + + // Only jsonrpc/id/error may appear in either envelope — id legitimately + // differs (3 vs 4, the request's own id echoed back), so it is excluded + // from the equality check below rather than asserted equal. + assert.ElementsMatchf(t, mapKeysForTest(callEnvelope), mapKeysForTest(unregisteredEnvelope), + "the top-level envelope shape (jsonrpc/id/error, no result) must match exactly") + assert.ElementsMatchf(t, mapKeysForTest(callErr), mapKeysForTest(unregisteredErr), + "the error object's own field set (code/message, no extra data) must match exactly") + + assert.Equal(t, unregisteredErr["code"], callErr["code"], + "a hidden-but-registered tool's refusal code must be byte-identical to a genuinely unregistered name's") + // The message text is identical once the caller-supplied name is + // substituted back in — that substitution is the ONLY difference D12 + // permits (the caller-supplied name may be echoed), never a distinct + // scope-reason phrase, code, or extra field. + wantMessage := strings.Replace(unregisteredErr["message"].(string), "totally__unregistered", "b__read", 1) + assert.Equal(t, wantMessage, callErr["message"], + "a hidden-but-registered tool's refusal text must be byte-identical to a genuinely unregistered name's, with only the echoed name differing") +} + +// mapKeysForTest returns m's top-level keys, for an order-independent +// envelope-shape comparison via assert.ElementsMatch. +func mapKeysForTest(m map[string]interface{}) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys } diff --git a/internal/server/mcp_direct_scope_test.go b/internal/server/mcp_direct_scope_test.go new file mode 100644 index 000000000..9f6a9d156 --- /dev/null +++ b/internal/server/mcp_direct_scope_test.go @@ -0,0 +1,63 @@ +package server + +import ( + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestReadDirectToolStamp_RejectsForgedValue is the tool-side analogue of +// TestStripAggregatedPromptServer_PreservesUpstreamMeta's forged-stamp check +// in mcp_prompt_scope_test.go (PR #1326 review round 2, chunk D/E: the tool +// surface had no equivalent regression test). An upstream server that happens +// to send our own internal _meta key back — whether by coincidence or by a +// crafted response probing for a bypass — must never be trusted as a +// registration identity: only a value stampDirectTool itself produced (the +// unexported directToolStamp struct) can satisfy readDirectToolStamp, because +// a plain string or map under the same key does not type-assert to it. +func TestReadDirectToolStamp_RejectsForgedValue(t *testing.T) { + cases := map[string]any{ + "bare string": "github", + "map mimicking it": map[string]any{"owner": "github", "rawName": "list_repos"}, + "wrong-typed struct": struct{ Owner string }{Owner: "github"}, + "empty struct": struct{}{}, + } + + for name, forgedValue := range cases { + t.Run(name, func(t *testing.T) { + forged := mcp.Tool{ + Name: "github__list_repos", + Meta: &mcp.Meta{AdditionalFields: map[string]any{directToolStampMetaKey: forgedValue}}, + } + + stamp, ok := readDirectToolStamp(forged) + assert.False(t, ok, "an upstream-supplied value under the stamp key is not a registration identity") + assert.Equal(t, directToolStamp{}, stamp, "a rejected read must not leak a partially-populated stamp") + + // stripDirectToolStamp must not treat a forged value as a stamp to + // strip either — the tool (and its forged _meta) passes through + // untouched, exactly like an upstream tool with no stamp at all. + assert.Equal(t, forged, stripDirectToolStamp(forged), + "an unrecognized value under the stamp key must be left exactly as the upstream sent it") + }) + } +} + +// TestReadDirectToolStamp_AcceptsGenuineStamp is the positive control for the +// test above: stampDirectTool's own output must round-trip through +// readDirectToolStamp, so the forged-value rejection above is proven against +// a real stamp actually failing to round-trip, not against a helper that +// rejects everything. +func TestReadDirectToolStamp_AcceptsGenuineStamp(t *testing.T) { + entry := &directCatalogEntry{ServerName: "github", ToolName: "list_repos", RequiredPermission: "read"} + tool := mcp.Tool{Name: "github__list_repos"} + + stamped := stampDirectTool(tool, entry) + stamp, ok := readDirectToolStamp(stamped) + require.True(t, ok, "a tool this package itself stamped must be recognized") + assert.Equal(t, "github", stamp.owner) + assert.Equal(t, "list_repos", stamp.rawName) + assert.Equal(t, "read", stamp.tier) +} diff --git a/internal/server/mcp_direct_skew_test.go b/internal/server/mcp_direct_skew_test.go index 108f05a0c..e036657a7 100644 --- a/internal/server/mcp_direct_skew_test.go +++ b/internal/server/mcp_direct_skew_test.go @@ -347,24 +347,26 @@ func TestSkew_OriginFlipNeverSplitsScopeFromDispatch(t *testing.T) { // The registry already holds the NEW origin's handler, invoked here // directly (bypassing mcp-go's own call-time filter re-evaluation, // which — now that the filters are stamp-based — would already refuse - // this with the SAME envelope a wholly unregistered name gets, before - // the handler ever ran; see TestDirectFullMode_InSeamScopeRefusalMatchesUnregisteredName). + // this with the SAME text a wholly unregistered name gets, before the + // handler ever ran; see TestDirectProtocol_StampNeverOnWire_FilterReEvaluatedAtCallTime). // This is the handler's OWN defense-in-depth check, and D12 forbids it - // from naming either origin. + // from naming either origin. It is now returned as the handler's own + // error (PR #1326 review round 2, chunk C), not a NewToolResultError, + // so the envelope KIND also converges on the filter's protocol-level + // refusal rather than staying a successful isError:true result. result, err := f.registeredHandler(t, display)(oldOnly, mcp.CallToolRequest{ Params: mcp.CallToolParams{Name: display}, }) - require.NoError(t, err) - require.True(t, result.IsError, - "a token scoped to the old origin must not reach the new one through a stale listing") - text := result.Content[0].(mcp.TextContent).Text + require.Nil(t, result, "the handler's own defense-in-depth refusal must not be a tool-result") + require.Error(t, err, "a token scoped to the old origin must not reach the new one through a stale listing") + text := err.Error() // "a__b__c" (the caller-supplied display name, which D12 permits // echoing) happens to contain "a__b" as a raw substring, so the // disclosure check is against the OLD message's own distinguishing // phrasing — naming the origin AS a server, in a sentence that // confirms a scope check fired — not against that coincidental // substring. - assert.Equal(t, "tool 'a__b__c' not found", text, + assert.Equal(t, "tool 'a__b__c' not found: tool not found", text, "the refusal must be worded exactly like an unregistered name's, never naming the origin actually dispatched to (D12)") assert.NotContains(t, text, "does not have access", "nor disclose that a scope check is what fired") assert.NotContains(t, text, "Owned by", "nor leak the entry's own description") diff --git a/internal/server/mcp_direct_underscore_test.go b/internal/server/mcp_direct_underscore_test.go index 3ac81f37e..6a8bfbe19 100644 --- a/internal/server/mcp_direct_underscore_test.go +++ b/internal/server/mcp_direct_underscore_test.go @@ -84,9 +84,9 @@ func TestDirectUnderscoreServer_SteadyState(t *testing.T) { result, err := f.registeredHandler(t, "__a__review")(aOnly, mcp.CallToolRequest{ Params: mcp.CallToolParams{Name: "__a__review"}, }) - require.NoError(t, err) - require.True(t, result.IsError) - assert.Equal(t, "tool '__a__review' not found", result.Content[0].(mcp.TextContent).Text, + require.Nil(t, result, "the handler's own defense-in-depth refusal must not be a tool-result") + require.Error(t, err) + assert.Equal(t, "tool '__a__review' not found: tool not found", err.Error(), "withheld with the same non-disclosing wording an unregistered name gets, never naming __a (D12)") }) } diff --git a/internal/server/mcp_prompt_scope_test.go b/internal/server/mcp_prompt_scope_test.go index e6593a30d..1f72e0893 100644 --- a/internal/server/mcp_prompt_scope_test.go +++ b/internal/server/mcp_prompt_scope_test.go @@ -454,12 +454,30 @@ func TestUnstampedPrompt_WithheldFromListAndGet_ForAdminAndAgent(t *testing.T) { proxy.config.EnablePrompts = true unstamped := mcp.Prompt{Name: "ghost__unstamped"} - proxy.server.SetPrompts(mcpserver.ServerPrompt{ - Prompt: unstamped, - Handler: func(_ context.Context, _ mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { - return &mcp.GetPromptResult{Messages: []mcp.PromptMessage{}}, nil + // Positive control (PR #1326 review round 2, chunk D/E): a prompt WITH a + // genuine registration identity, registered alongside the withheld one. + // Without this the test can pass for the wrong reason — if + // prompts/list or prompts/get were broken entirely (returning nothing, + // or erroring on every request), the unstamped prompt would still be + // "absent" and every prompts/get would still be "refused", and the test + // would say nothing went wrong. + stamped := aggregatedPromptForTest("real", "control") + proxy.server.SetPrompts( + mcpserver.ServerPrompt{ + Prompt: unstamped, + Handler: func(_ context.Context, _ mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + return &mcp.GetPromptResult{Messages: []mcp.PromptMessage{}}, nil + }, }, - }) + mcpserver.ServerPrompt{ + Prompt: stamped, + Handler: func(_ context.Context, _ mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + return &mcp.GetPromptResult{Messages: []mcp.PromptMessage{ + {Role: mcp.RoleUser, Content: mcp.TextContent{Text: "control"}}, + }}, nil + }, + }, + ) agentCtxForTest := auth.WithAuthContext(context.Background(), &auth.AuthContext{ Type: auth.AuthTypeAgent, @@ -484,11 +502,31 @@ func TestUnstampedPrompt_WithheldFromListAndGet_ForAdminAndAgent(t *testing.T) { listedNames = append(listedNames, pr.(map[string]interface{})["name"].(string)) } assert.NotContainsf(t, listedNames, "ghost__unstamped", "%s: an unstamped prompt is withheld from EVERYONE (SC-005)", name) + // Positive control: the properly-stamped sibling prompt IS listed, + // proving prompts/list is not simply returning an empty/broken result + // that would vacuously satisfy the NotContains check above. + assert.Containsf(t, listedNames, stamped.Name, "%s: a properly stamped prompt must still be listed", name) getEncoded, err := json.Marshal(proxy.server.HandleMessage(ctx, []byte(`{"jsonrpc":"2.0","id":3,"method":"prompts/get","params":{"name":"ghost__unstamped"}}`))) require.NoErrorf(t, err, "%s", name) var getEnvelope map[string]interface{} require.NoError(t, json.Unmarshal(getEncoded, &getEnvelope)) - assert.NotNilf(t, getEnvelope["error"], "%s: prompts/get must refuse an unstamped prompt: %v", name, getEnvelope) + require.NotNilf(t, getEnvelope["error"], "%s: prompts/get must refuse an unstamped prompt: %v", name, getEnvelope) + unstampedGetErr := getEnvelope["error"].(map[string]interface{}) + assert.Containsf(t, unstampedGetErr["message"], "not found", + "%s: the refusal must be the absent-equivalent wording, not some other failure", name) + + // Positive control: prompts/get on the properly-stamped sibling must + // actually succeed and return its content — proving prompts/get is + // not simply erroring on every request, which would vacuously + // satisfy the refusal assertion above. + controlGetEncoded, err := json.Marshal(proxy.server.HandleMessage(ctx, + []byte(`{"jsonrpc":"2.0","id":5,"method":"prompts/get","params":{"name":"`+stamped.Name+`"}}`))) + require.NoErrorf(t, err, "%s", name) + var controlGetEnvelope map[string]interface{} + require.NoError(t, json.Unmarshal(controlGetEncoded, &controlGetEnvelope)) + require.Nilf(t, controlGetEnvelope["error"], "%s: prompts/get on the stamped control prompt must succeed: %v", name, controlGetEnvelope) + controlMessages := controlGetEnvelope["result"].(map[string]interface{})["messages"].([]interface{}) + require.Lenf(t, controlMessages, 1, "%s", name) } } diff --git a/internal/server/mcp_routing.go b/internal/server/mcp_routing.go index 51354f8ae..e4fea1933 100644 --- a/internal/server/mcp_routing.go +++ b/internal/server/mcp_routing.go @@ -403,14 +403,46 @@ func (p *MCPProxyServer) directSignatureSuffix(entry *directCatalogEntry) string return "\n" + entry.ToolName + sig.Sig } -// directScopeRefusalMessage is the non-disclosing text makeDirectModeHandler's +// errDirectToolNotFound mirrors mcp-go's own ErrToolNotFound sentinel (the +// tool-surface counterpart of errPromptNotFound in mcp_direct_scope.go). +// Wrapping it below reproduces the exact text mcp-go emits when its own +// tools/call dispatch cannot find the requested name at all +// (server.go handleToolCall: `fmt.Errorf("tool '%s' not found: %w", name, +// ErrToolNotFound)`). +var errDirectToolNotFound = mcpserver.ErrToolNotFound + +// directScopeRefusalError builds the non-disclosing error makeDirectModeHandler's // OWN profile/server-scope checks return (Spec 105 FR-008 gap G5, D12): it -// echoes only the caller-supplied display name, mirroring the wording mcp-go -// itself uses for a name that is not registered at all, and never the -// canonical owner the handler's entry closed over. See the call sites for why -// this branch is defense in depth rather than the normal refusal path. -func directScopeRefusalMessage(displayName string) string { - return fmt.Sprintf("tool '%s' not found", displayName) +// echoes only the caller-supplied display name, wrapping errDirectToolNotFound +// so the TEXT is byte-identical to what mcp-go's own call-time tool filter +// re-evaluation emits for a name it does not admit at all — never the +// canonical owner the handler's entry closed over. +// +// It is returned as the HANDLER'S OWN error (the function's second return +// value), not built with mcp.NewToolResultError, so the JSON-RPC envelope +// KIND also converges on the filter's: both become a protocol-level error +// response, never a successful call result with isError:true. (PR #1326 +// review round 2, chunk C: the previous NewToolResultError version was a +// different envelope KIND — a successful result — from mcp-go's own +// -32602 protocol error for the identical logical case, not merely +// different wording.) +// +// One residual this cannot close, same as authorizeAggregatedPromptServer's +// prompt-side twin below: mcp-go always maps a handler-returned error to +// mcp.INTERNAL_ERROR (-32603), a code this function cannot override, while +// mcp-go's OWN filter path answers mcp.INVALID_PARAMS (-32602) for the +// identical text. A probe timed inside the narrow profile/config race this +// branch exists for (see the call sites' doc comments) could still tell the +// two apart by that numeric code alone, even though the message and the +// result KIND can no longer distinguish "authorized-but-blocked" from +// "genuinely doesn't exist". A ToolHandlerFunc has no way to emit an +// arbitrary top-level JSON-RPC error code — only mcp-go's own dispatch can — +// so full byte-for-byte envelope equality is not achievable from here without +// forking mcp-go's dispatch loop, which this fix does not do. This is +// documented, not silently accepted: see the PR description for the exact +// parity this branch provides. +func directScopeRefusalError(displayName string) error { + return fmt.Errorf("tool '%s' not found: %w", displayName, errDirectToolNotFound) } // makeDirectModeHandler creates a handler function for a direct mode tool. @@ -490,9 +522,9 @@ func (p *MCPProxyServer) makeDirectModeHandler(entry *directCatalogEntry) mcpser // connection is filtered too, and it runs FIRST so a profile-pinned // token cannot reach a server outside its pin through this routing mode. if profileScope != nil && !profileScope.Allows(serverName) { - errMsg := directScopeRefusalMessage(entry.DisplayName) - p.emitActivityPolicyDecision(ctx, serverName, toolName, sessionID, requestID, "blocked", errMsg, telemetry.BlockReasonProfileScope) - return mcp.NewToolResultError(errMsg), nil + refusalErr := directScopeRefusalError(entry.DisplayName) + p.emitActivityPolicyDecision(ctx, serverName, toolName, sessionID, requestID, "blocked", refusalErr.Error(), telemetry.BlockReasonProfileScope) + return nil, refusalErr } // Check auth context for server access and permissions @@ -505,20 +537,25 @@ func (p *MCPProxyServer) makeDirectModeHandler(entry *directCatalogEntry) mcpser // branch is unreachable — mcp-go's WithToolFilter chain // re-evaluates the SAME stamp-based scope decision at call // time and answers the registered-name-not-found envelope - // before this handler ever runs (Spec 105 T087/T088) — so it - // exists only as defense in depth for a caller that invokes a - // registered handler directly, bypassing that re-evaluation - // (tests, and any future direct-dispatch path). Its wording - // must therefore match what an unregistered name gets: no - // owner, no scope reason, just the caller-supplied name - // echoed back. - errMsg := directScopeRefusalMessage(entry.DisplayName) + // before this handler ever runs (Spec 105 T087/T088), OR for + // the narrow live profile/config race PR #1326 review round 2 + // (chunk C) found: the filter's stamp-based check and this + // handler's own profileScope/authCtx re-resolution can read a + // DIFFERENT active profile when a session's pin changes + // between the two evaluations, so the filter can pass a call + // this handler then refuses. Its wording must therefore + // match what an unregistered name gets: no owner, no scope + // reason, just the caller-supplied name echoed back — see + // directScopeRefusalError for how far that parity extends + // (text and envelope KIND, not the numeric JSON-RPC error + // code). + refusalErr := directScopeRefusalError(entry.DisplayName) // Direct mode denied these silently: no activity record and, // since issue #969, no availability counter either. Emit the // same policy decision the call_tool_* variants emit at the // equivalent gate so the funnel has no blind spot. - p.emitActivityPolicyDecision(ctx, serverName, toolName, sessionID, requestID, "blocked", errMsg, telemetry.BlockReasonTokenScope) - return mcp.NewToolResultError(errMsg), nil + p.emitActivityPolicyDecision(ctx, serverName, toolName, sessionID, requestID, "blocked", refusalErr.Error(), telemetry.BlockReasonTokenScope) + return nil, refusalErr } // Determine required permission from annotations diff --git a/internal/server/mcp_routing_test.go b/internal/server/mcp_routing_test.go index 8bc65831a..c24f04881 100644 --- a/internal/server/mcp_routing_test.go +++ b/internal/server/mcp_routing_test.go @@ -435,14 +435,30 @@ func TestDirectModeHandler_ServerAccessDenied(t *testing.T) { // and neither upstream sees the call. // // Spec 105 FR-008 gap G5 (D12): the refusal text must not name "gitlab" — - // f.call drives the REGISTERED handler directly, bypassing mcp-go's own + // this drives the REGISTERED handler directly, bypassing mcp-go's own // call-time filter re-evaluation (which would answer the unregistered-name // envelope first in real dispatch), so this exercises the handler's own - // defense-in-depth check. - result := f.call(t, agentCtx([]string{"github"}, []string{auth.PermRead}, ""), "gitlab", "list_repos") - f.refused(t, result, "tool 'gitlab__list_repos' not found") - assert.NotContains(t, result.Content[0].(mcp.TextContent).Text, "does not have access", + // defense-in-depth check. It is returned as the handler's OWN error (PR + // #1326 review round 2, chunk C), not a tool-result, so this scenario + // cannot use the shared f.call/f.refused helpers (which assert NO Go + // error at all — true for every OTHER refusal in this file, but not this + // one). + display := FormatDirectToolName("gitlab", "list_repos") + st, ok := f.proxy.directServer.ListTools()[display] + require.Truef(t, ok, "%q must be registered on the direct server", display) + req := mcp.CallToolRequest{} + req.Params.Name = display + req.Params.Arguments = map[string]interface{}{} + + result, err := st.Handler(agentCtx([]string{"github"}, []string{auth.PermRead}, ""), req) + require.Nil(t, result, "the handler's own defense-in-depth refusal must not be a tool-result") + require.Error(t, err) + assert.Equal(t, "tool 'gitlab__list_repos' not found: tool not found", err.Error()) + assert.NotContains(t, err.Error(), "does not have access", "the refusal must never disclose that a scope check is what fired") + for server, up := range f.ups { + assert.Equal(t, int64(0), up.count.Load(), "a refused cell must never reach upstream %q (dispatched: %v)", server, up.dispatched()) + } } func TestDirectModeHandler_AgentWithCorrectPermissions(t *testing.T) { diff --git a/internal/server/preflight_telemetry_test.go b/internal/server/preflight_telemetry_test.go index 6e355c994..dfef919ae 100644 --- a/internal/server/preflight_telemetry_test.go +++ b/internal/server/preflight_telemetry_test.go @@ -340,6 +340,53 @@ func TestDirectBlockReasonKey_ClassifiesPerGate(t *testing.T) { } } +// TestDirectBlockReasonKey_AgreesWithResponse_ConfigDeniedAndApprovalLocked is +// PR #1326 review round 2, chunk B: a tool can be BOTH config-denied +// (enabled_tools/disabled_tools) AND pending/changed approval at the same +// time. directBlockReasonKey and directToolCallabilityResult used to +// classify that case differently — the emitted telemetry reason described a +// DIFFERENT block than the response body the caller actually received. +// +// The canonical precedence, matching every other dispatch path (toolGate's +// quarantine -> approval-lock -> generic/config-denied order in +// handleCallToolVariant/handleCallTool, mcp.go), is that the approval lock +// wins over a plain config denial. Both functions must agree with THAT order +// and with each other, for every locked status. +func TestDirectBlockReasonKey_AgreesWithResponse_ConfigDeniedAndApprovalLocked(t *testing.T) { + proxy := createTestMCPProxyServer(t) + + for _, status := range []string{storage.ToolApprovalStatusPending, storage.ToolApprovalStatusChanged} { + decision := directCallabilityDecision{ + serverName: "github", + toolName: "list_repos", + serverConfig: &config.ServerConfig{Name: "github", Enabled: true}, + configDenied: true, + approvalStatus: status, + approval: &storage.ToolApprovalRecord{ + ServerName: "github", ToolName: "list_repos", Status: status, + }, + } + + reasonKey := directBlockReasonKey(decision) + result := proxy.directToolCallabilityResult(context.Background(), decision, map[string]interface{}{}) + require.NotNil(t, result) + text := result.Content[0].(mcp.TextContent).Text + + switch status { + case storage.ToolApprovalStatusPending: + require.Equal(t, telemetry.BlockReasonToolPendingApproval, reasonKey) + require.Contains(t, text, "new_unapproved_tool", "the reason key and the response body must describe the SAME block") + case storage.ToolApprovalStatusChanged: + require.Equal(t, telemetry.BlockReasonToolChanged, reasonKey) + require.Contains(t, text, "tool_description_changed", "the reason key and the response body must describe the SAME block") + } + // Neither function may fall back to the generic config-denied wording + // while the other reports an approval lock. + require.NotContains(t, text, blockedToolMessageFor(true), + "the approval lock must win over the plain config-denied response") + } +} + // The reason travels with the block result, so the routing handler emits the // key that matches the gate that fired. func TestDirectToolCallabilityBlockWithReason_Quarantine(t *testing.T) { diff --git a/internal/server/profile_pin_enforcement_test.go b/internal/server/profile_pin_enforcement_test.go index d480f4617..4c4389585 100644 --- a/internal/server/profile_pin_enforcement_test.go +++ b/internal/server/profile_pin_enforcement_test.go @@ -113,14 +113,21 @@ func TestDirectModeHonorsTokenProfilePin(t *testing.T) { handler := proxy.makeDirectModeHandler(&directCatalogEntry{ServerName: "deploy-srv", ToolName: "ship", DisplayName: FormatDirectToolName("deploy-srv", "ship"), Annotations: nil}) result, err := handler(ctx, mcp.CallToolRequest{}) - require.NoError(t, err) - require.True(t, result.IsError, "a call outside the pinned profile must be refused") // Spec 105 FR-008 gap G5 (D12): the refusal must not name the profile-out // server "deploy-srv" — invoking the registered handler directly, as this // test does, exercises its defense-in-depth check, which now echoes only // the caller-supplied tool name, matching an unregistered name's wording. - assert.Contains(t, resultText(t, result), "tool 'deploy-srv__ship' not found") - assert.NotContains(t, resultText(t, result), "is not in profile") + // Returned as the handler's own error (PR #1326 review round 2, chunk C), + // not a NewToolResultError, so the envelope KIND matches too. + // + // An EXACT match, not Contains: a loose substring check would still pass + // a message that echoes the expected prefix AND appends extra + // disclosure (e.g. "... not found (server deploy-srv is not in profile + // 'research')"). The full string is the only check that rules that out. + require.Nil(t, result, "the handler's own defense-in-depth refusal must not be a tool-result") + require.Error(t, err, "a call outside the pinned profile must be refused") + assert.Equal(t, "tool 'deploy-srv__ship' not found: tool not found", err.Error(), + "the refusal must be EXACTLY the unregistered-name wording, with nothing appended that could leak the scope reason") // Profile deleted → deny-all on both discovery and dispatch. cfg.Profiles = nil @@ -129,8 +136,10 @@ func TestDirectModeHonorsTokenProfilePin(t *testing.T) { handler = proxy.makeDirectModeHandler(&directCatalogEntry{ServerName: "research-srv", ToolName: "search", DisplayName: FormatDirectToolName("research-srv", "search"), Annotations: nil}) result, err = handler(ctx, mcp.CallToolRequest{}) - require.NoError(t, err) - require.True(t, result.IsError, "a stale pin must refuse even the formerly pinned server") + require.Nil(t, result, "the handler's own defense-in-depth refusal must not be a tool-result") + require.Error(t, err, "a stale pin must refuse even the formerly pinned server") + assert.Equal(t, "tool 'research-srv__search' not found: tool not found", err.Error(), + "the refusal must be EXACTLY the unregistered-name wording even for the formerly pinned server") // An admin (no auth context, no profile) is unaffected. assert.Len(t, proxy.filterDirectModeToolsForAuth(context.Background(), tools), 2)