diff --git a/pkg/http/handler.go b/pkg/http/handler.go index b9dfccf09f..935bca1c0e 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -265,9 +265,24 @@ func DefaultGitHubMCPServerFactory(r *http.Request, deps github.ToolDependencies // a static inventory is built once at factory creation to pre-filter the tool // universe. Per-request headers can only narrow within these bounds. func DefaultInventoryFactory(cfg *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker, scopeFetcher scopes.FetcherInterface) InventoryFactoryFunc { + factory, err := NewDefaultInventoryFactory(cfg, t, featureChecker, scopeFetcher) + if err != nil { + return func(_ *http.Request) (*inventory.Inventory, error) { + return nil, err + } + } + return factory +} + +// NewDefaultInventoryFactory creates the default HTTP inventory factory and +// validates static tool configuration before returning it. +func NewDefaultInventoryFactory(cfg *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker, scopeFetcher scopes.FetcherInterface) (InventoryFactoryFunc, error) { // Build the static tool/resource/prompt universe from CLI flags. // This is done once at startup and captured in the closure. - staticTools, staticResources, staticPrompts := buildStaticInventory(cfg, t) + staticTools, staticResources, staticPrompts, err := buildStaticInventory(cfg, t) + if err != nil { + return nil, err + } hasStaticFilters := hasStaticConfig(cfg) // Pre-compute valid tool names for filtering per-request tool headers. @@ -311,7 +326,7 @@ func DefaultInventoryFactory(cfg *ServerConfig, t translations.TranslationHelper b.WithServerInstructions() return b.Build() - } + }, nil } // filterRequestTools returns a shallow copy of the request with any per-request @@ -350,7 +365,7 @@ func hasStaticConfig(cfg *ServerConfig) bool { // non-granular siblings — must be carried through to the per-request // inventory, which then installs a checker and resolves the flag before // registering tools with the MCP server. -func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFunc) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { +func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFunc) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt, error) { // Tools with host-specific capabilities need to know the deployment they // will talk to. An unparseable host is not fatal here: NewAPIHost rejects // it later with a clearer error, so fall back to the dotcom default. @@ -370,7 +385,7 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun } if !hasStaticConfig(cfg) { - return filterUnavailable(tools), github.AllResources(t), github.AllPrompts(t) + return filterUnavailable(tools), github.AllResources(t), github.AllPrompts(t), nil } b := inventory.NewBuilder(). @@ -390,13 +405,11 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun inv, err := b.Build() if err != nil { - // Invalid static tool names must fail closed rather than widening an - // explicit allowlist to every tool. - return nil, github.AllResources(t), github.AllPrompts(t) + return nil, nil, nil, err } ctx := context.Background() - return filterUnavailable(inv.AvailableTools(ctx)), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) + return filterUnavailable(inv.AvailableTools(ctx)), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx), nil } // InventoryFiltersForRequest applies filters to the inventory builder diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index c1ee327952..13e87fc034 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -559,7 +559,8 @@ func TestStaticConfigEnforcement(t *testing.T) { require.NoError(t, err) // Build static tools the same way the production code does - staticTools, staticResources, staticPrompts := buildStaticInventoryFromTools(tt.config, tools) + staticTools, staticResources, staticPrompts, err := buildStaticInventoryFromTools(tt.config, tools) + require.NoError(t, err) hasStatic := hasStaticConfig(tt.config) validToolNames := make(map[string]bool, len(staticTools)) @@ -637,6 +638,79 @@ func TestStaticConfigEnforcement(t *testing.T) { } } +func TestDefaultInventoryFactoriesRejectInvalidEnabledTools(t *testing.T) { + tests := []struct { + name string + enabledTools []string + unknownTools []string + }{ + { + name: "mixed valid and invalid tools", + enabledTools: []string{"get_file_contents", "nonexistent_tool"}, + unknownTools: []string{"nonexistent_tool"}, + }, + { + name: "all invalid tools", + enabledTools: []string{"nonexistent_tool", "another_nonexistent_tool"}, + unknownTools: []string{"nonexistent_tool", "another_nonexistent_tool"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &ServerConfig{Version: "test", EnabledTools: tt.enabledTools} + factory, err := NewDefaultInventoryFactory( + cfg, + translations.NullTranslationHelper, + nil, + allScopesFetcher{}, + ) + require.ErrorIs(t, err, inventory.ErrUnknownTools) + for _, unknownTool := range tt.unknownTools { + assert.Contains(t, err.Error(), unknownTool) + } + assert.Nil(t, factory, "invalid static configuration must not produce a widened inventory factory") + + factory = DefaultInventoryFactory( + cfg, + translations.NullTranslationHelper, + nil, + allScopesFetcher{}, + ) + inv, err := factory(httptest.NewRequest(http.MethodPost, "/", nil)) + require.ErrorIs(t, err, inventory.ErrUnknownTools) + assert.Nil(t, inv, "the compatibility factory must preserve the validation error") + }) + } +} + +func TestStaticInventoryInvalidEnabledToolsReturnsBadRequest(t *testing.T) { + apiHost, err := utils.NewAPIHost("https://api.github.com") + require.NoError(t, err) + + handler := NewHTTPMcpHandler( + context.Background(), + &ServerConfig{Version: "test", EnabledTools: []string{"nonexistent_tool"}}, + nil, + translations.NullTranslationHelper, + slog.Default(), + apiHost, + ) + + r := chi.NewRouter() + handler.RegisterMiddleware(r) + handler.RegisterRoutes(r) + + req := httptest.NewRequest(http.MethodPost, "/", nil) + req.Header.Set(headers.AuthorizationHeader, "Bearer ghp_testtoken") + + rr := httptest.NewRecorder() + r.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusBadRequest, rr.Code) + assert.Contains(t, rr.Body.String(), "unknown tools specified") +} + func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) { tools := []inventory.ServerTool{ mockToolWithFeatureFlag("list_issues", "issues", true, "", github.FeatureFlagCSVOutput), @@ -646,7 +720,8 @@ func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) { cfg := &ServerConfig{Version: "test", EnabledToolsets: []string{"issues"}} featureChecker := createHTTPFeatureChecker(nil, false) - staticTools, _, _ := buildStaticInventoryFromTools(cfg, tools) + staticTools, _, _, err := buildStaticInventoryFromTools(cfg, tools) + require.NoError(t, err) require.Len(t, staticTools, 2, "static upper bounds should preserve both feature variants") inv, err := inventory.NewBuilder(). @@ -665,7 +740,8 @@ func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) { func TestStaticInventoryDisablesOnlyDeleteRepository(t *testing.T) { cfg := &ServerConfig{disableDeleteRepository: true} - tools, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper) + tools, _, _, err := buildStaticInventory(cfg, translations.NullTranslationHelper) + require.NoError(t, err) names := make([]string, 0, len(tools)) for _, tool := range tools { @@ -675,12 +751,13 @@ func TestStaticInventoryDisablesOnlyDeleteRepository(t *testing.T) { assert.Contains(t, names, "actions_list", "non-default toolsets must remain available for per-request selection") } -func TestStaticInventoryFallbackKeepsDeleteRepositoryDisabled(t *testing.T) { +func TestStaticInventoryKeepsExplicitDeleteRepositoryDisabled(t *testing.T) { cfg := &ServerConfig{ EnabledTools: []string{github.DeleteRepositoryToolName}, disableDeleteRepository: true, } - tools, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper) + tools, _, _, err := buildStaticInventory(cfg, translations.NullTranslationHelper) + require.NoError(t, err) assert.Empty(t, tools, "an unavailable explicit allowlist must not widen to other tools") } @@ -782,9 +859,9 @@ func TestContentTypeHandling(t *testing.T) { // buildStaticInventoryFromTools is a test helper that mirrors buildStaticInventory // but uses the provided mock tools instead of calling github.AllTools. -func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTool) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { +func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTool) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt, error) { if !hasStaticConfig(cfg) { - return tools, nil, nil + return tools, nil, nil, nil } b := inventory.NewBuilder(). @@ -802,11 +879,11 @@ func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTo inv, err := b.Build() if err != nil { - return tools, nil, nil + return nil, nil, nil, err } ctx := context.Background() - return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) + return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx), nil } // TestStaticInventoryAppliesHostCapabilities guards against HTTP deployments @@ -843,7 +920,7 @@ func TestStaticInventoryAppliesHostCapabilities(t *testing.T) { t.Parallel() cfg := &ServerConfig{Version: "test", Host: tt.host} - staticTools, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper) + staticTools, _, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper) var found bool for _, st := range staticTools { diff --git a/pkg/http/server.go b/pkg/http/server.go index 270c83772c..6bf48a07a7 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -156,6 +156,11 @@ func RunHTTPServer(cfg ServerConfig) error { } featureChecker := createHTTPFeatureChecker(cfg.EnabledFeatures, cfg.InsidersMode) + scopeFetcher := scopes.NewFetcher(apiHost, scopes.FetcherOptions{}) + inventoryFactory, err := NewDefaultInventoryFactory(&cfg, t, featureChecker, scopeFetcher) + if err != nil { + return fmt.Errorf("failed to build inventory: %w", err) + } obs, err := observability.NewExporters(logger, metrics.NewNoopMetrics()) if err != nil { @@ -187,10 +192,9 @@ func RunHTTPServer(cfg ServerConfig) error { TrustProxyHeaders: cfg.TrustProxyHeaders, } - serverOptions := []HandlerOption{} - if cfg.ScopeChallenge { - scopeFetcher := scopes.NewFetcher(apiHost, scopes.FetcherOptions{}) - serverOptions = append(serverOptions, WithScopeFetcher(scopeFetcher)) + serverOptions := []HandlerOption{ + WithInventoryFactory(inventoryFactory), + WithScopeFetcher(scopeFetcher), } r := chi.NewRouter() diff --git a/pkg/http/server_test.go b/pkg/http/server_test.go index 05703cf9b5..e7f9dd9da1 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -9,11 +9,41 @@ import ( ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestRunHTTPServerRejectsInvalidStaticTools(t *testing.T) { + tests := []struct { + name string + enabledTools []string + }{ + { + name: "mixed valid and invalid tools", + enabledTools: []string{"get_file_contents", "nonexistent_tool"}, + }, + { + name: "all invalid tools", + enabledTools: []string{"nonexistent_tool", "another_nonexistent_tool"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := RunHTTPServer(ServerConfig{ + Version: "test", + Host: "https://github.com", + EnabledTools: tt.enabledTools, + }) + + require.ErrorIs(t, err, inventory.ErrUnknownTools) + assert.ErrorContains(t, err, "failed to build inventory") + }) + } +} + func TestInitGlobalToolScopeMapUsesHost(t *testing.T) { tests := []struct { name string