From 00a795e0ee998415a732dd473ad5abc7df41cec5 Mon Sep 17 00:00:00 2001 From: root Date: Sat, 6 Jun 2026 17:16:40 +0000 Subject: [PATCH] fix: gate MCP App UI resources on backing tool availability Closes #2519 UI resources for write tools were registered unconditionally even when --read-only mode filtered out the backing tools. Register each UI resource only when its companion tool passes inventory filters. --- pkg/github/server.go | 15 ++++--- pkg/github/ui_resources.go | 38 +++++++++++++++-- pkg/github/ui_resources_test.go | 74 +++++++++++++++++++++++++++++---- 3 files changed, 107 insertions(+), 20 deletions(-) diff --git a/pkg/github/server.go b/pkg/github/server.go index f56ac7d3a8..f34f749e59 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -103,15 +103,14 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci inv.RegisterAll(ctx, ghServer, deps) // Register MCP App UI resources whenever the embedded UI assets are - // available. The resources are static HTML and are only referenced by - // tools when the remote_mcp_ui_apps feature flag is enabled for the - // request (the inventory strips the _meta.ui block otherwise via - // stripMCPAppsMetadata), so registering them unconditionally is safe. - // Registering here — rather than in the stdio bootstrap — ensures the - // remote/HTTP server also serves them, fixing the "-32002 Resource not - // found" error clients hit after the tool returns a ui:// URI. + // available. Each resource is registered only if its backing write/read + // tool is in the inventory (see RegisterUIResources), so read-only mode + // does not expose write-tool UI surfaces. Registering here — rather than + // in the stdio bootstrap — ensures the remote/HTTP server also serves + // them, fixing the "-32002 Resource not found" error clients hit after + // the tool returns a ui:// URI. if UIAssetsAvailable() { - RegisterUIResources(ghServer) + RegisterUIResources(ctx, ghServer, inv) } return ghServer, nil diff --git a/pkg/github/ui_resources.go b/pkg/github/ui_resources.go index ab3ebfd163..6f8cfce65a 100644 --- a/pkg/github/ui_resources.go +++ b/pkg/github/ui_resources.go @@ -3,18 +3,46 @@ package github import ( "context" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/modelcontextprotocol/go-sdk/mcp" ) +type uiResourceSpec struct { + toolName string + register func(s *mcp.Server) +} + // RegisterUIResources registers MCP App UI resources with the server. // These are static resources (not templates) that serve HTML content for // MCP App-enabled tools. The HTML is built from React/Primer components // in the ui/ directory using `script/build-ui`. // +// UI resources are registered only when their backing tool is present in +// the inventory's available tools (respecting read-only mode and other filters). +// // Resource metadata follows the stable 2026-01-26 MCP Apps spec: // https://github.com/modelcontextprotocol/ext-apps/blob/main/specification/2026-01-26/apps.mdx -func RegisterUIResources(s *mcp.Server) { - // Register the get_me UI resource +func RegisterUIResources(ctx context.Context, s *mcp.Server, inv *inventory.Inventory) { + tools := inv.AvailableTools(ctx) + available := make(map[string]struct{}, len(tools)) + for _, tool := range tools { + available[tool.Tool.Name] = struct{}{} + } + + specs := []uiResourceSpec{ + {toolName: "get_me", register: registerGetMeUIResource}, + {toolName: "issue_write", register: registerIssueWriteUIResource}, + {toolName: "create_pull_request", register: registerPullRequestWriteUIResource}, + } + + for _, spec := range specs { + if _, ok := available[spec.toolName]; ok { + spec.register(s) + } + } +} + +func registerGetMeUIResource(s *mcp.Server) { s.AddResource( &mcp.Resource{ URI: GetMeUIResourceURI, @@ -45,8 +73,9 @@ func RegisterUIResources(s *mcp.Server) { }, nil }, ) +} - // Register the issue_write UI resource +func registerIssueWriteUIResource(s *mcp.Server) { s.AddResource( &mcp.Resource{ URI: IssueWriteUIResourceURI, @@ -75,8 +104,9 @@ func RegisterUIResources(s *mcp.Server) { }, nil }, ) +} - // Register the create_pull_request UI resource +func registerPullRequestWriteUIResource(s *mcp.Server) { s.AddResource( &mcp.Resource{ URI: PullRequestWriteUIResourceURI, diff --git a/pkg/github/ui_resources_test.go b/pkg/github/ui_resources_test.go index 928950ac73..8dd37df636 100644 --- a/pkg/github/ui_resources_test.go +++ b/pkg/github/ui_resources_test.go @@ -25,8 +25,9 @@ func TestRegisterUIResources_ReadableViaClient(t *testing.T) { t.Skip("UI assets not built; run script/build-ui to enable this test") } + ctx := context.Background() srv := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.0.1"}, nil) - RegisterUIResources(srv) + RegisterUIResources(ctx, srv, mustInventoryWithUIAppTools(t)) // Connect an in-memory client/server pair and read each advertised URI. st, ct := mcp.NewInMemoryTransports() @@ -68,6 +69,52 @@ func TestRegisterUIResources_ReadableViaClient(t *testing.T) { } } +// TestRegisterUIResources_ReadOnlyExcludesWriteUIResources verifies that write +// tool UI resources are not registered when the server runs in read-only mode, +// while read-only tool UI (get_me) remains available. +func TestRegisterUIResources_ReadOnlyExcludesWriteUIResources(t *testing.T) { + t.Parallel() + + if !UIAssetsAvailable() { + t.Skip("UI assets not built; run script/build-ui to enable this test") + } + + ctx := context.Background() + srv := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.0.1"}, nil) + RegisterUIResources(ctx, srv, mustReadOnlyInventoryWithUIAppToolsets(t)) + + st, ct := mcp.NewInMemoryTransports() + + type clientResult struct { + session *mcp.ClientSession + err error + } + clientCh := make(chan clientResult, 1) + go func() { + client := mcp.NewClient(&mcp.Implementation{Name: "test-client"}, nil) + cs, err := client.Connect(context.Background(), ct, nil) + clientCh <- clientResult{session: cs, err: err} + }() + + ss, err := srv.Connect(context.Background(), st, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = ss.Close() }) + + got := <-clientCh + require.NoError(t, got.err) + t.Cleanup(func() { _ = got.session.Close() }) + + res, err := got.session.ReadResource(context.Background(), &mcp.ReadResourceParams{URI: GetMeUIResourceURI}) + require.NoError(t, err) + require.NotEmpty(t, res.Contents) + + _, err = got.session.ReadResource(context.Background(), &mcp.ReadResourceParams{URI: IssueWriteUIResourceURI}) + require.Error(t, err, "issue_write UI should not be registered in read-only mode") + + _, err = got.session.ReadResource(context.Background(), &mcp.ReadResourceParams{URI: PullRequestWriteUIResourceURI}) + require.Error(t, err, "pr_write UI should not be registered in read-only mode") +} + // TestNewMCPServer_RegistersUIResources verifies that NewMCPServer — the // shared constructor used by both the stdio and HTTP entry points — registers // the UI resources when UI assets are embedded. Previously this registration @@ -80,9 +127,10 @@ func TestNewMCPServer_RegistersUIResources(t *testing.T) { } srv, err := NewMCPServer(context.Background(), &MCPServerConfig{ - Version: "test", - Translator: stubTranslator, - }, stubDeps{t: stubTranslator}, mustEmptyInventory(t)) + Version: "test", + Translator: stubTranslator, + EnabledToolsets: []string{"issues"}, + }, stubDeps{t: stubTranslator}, mustInventoryWithUIAppTools(t)) require.NoError(t, err) st, ct := mcp.NewInMemoryTransports() @@ -113,11 +161,21 @@ func TestNewMCPServer_RegistersUIResources(t *testing.T) { assert.Equal(t, MCPAppMIMEType, res.Contents[0].MIMEType) } -// mustEmptyInventory builds an empty inventory for tests that only care about -// resources/prompts registered outside the inventory (such as the UI resources). -func mustEmptyInventory(t *testing.T) *inventory.Inventory { +func mustInventoryWithUIAppTools(t *testing.T) *inventory.Inventory { + t.Helper() + inv, err := NewInventory(stubTranslator). + WithToolsets([]string{"context", "issues", "pull_requests"}). + Build() + require.NoError(t, err) + return inv +} + +func mustReadOnlyInventoryWithUIAppToolsets(t *testing.T) *inventory.Inventory { t.Helper() - inv, err := NewInventory(stubTranslator).WithToolsets([]string{}).Build() + inv, err := NewInventory(stubTranslator). + WithToolsets([]string{"context", "issues", "pull_requests"}). + WithReadOnly(true). + Build() require.NoError(t, err) return inv }