diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 05c2c6e0b..8f2ae5852 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -105,6 +105,28 @@ var ( Short: "Start HTTP server", Long: `Start an HTTP server that listens for MCP requests over HTTP.`, RunE: func(_ *cobra.Command, _ []string) error { + // Parse toolsets (same approach as stdio — see comment there) + var enabledToolsets []string + if viper.IsSet("toolsets") { + if err := viper.UnmarshalKey("toolsets", &enabledToolsets); err != nil { + return fmt.Errorf("failed to unmarshal toolsets: %w", err) + } + } + + var enabledTools []string + if viper.IsSet("tools") { + if err := viper.UnmarshalKey("tools", &enabledTools); err != nil { + return fmt.Errorf("failed to unmarshal tools: %w", err) + } + } + + var excludeTools []string + if viper.IsSet("exclude_tools") { + if err := viper.UnmarshalKey("exclude_tools", &excludeTools); err != nil { + return fmt.Errorf("failed to unmarshal exclude-tools: %w", err) + } + } + ttl := viper.GetDuration("repo-access-cache-ttl") httpConfig := ghhttp.ServerConfig{ Version: version, @@ -119,6 +141,12 @@ var ( LockdownMode: viper.GetBool("lockdown-mode"), RepoAccessCacheTTL: &ttl, ScopeChallenge: viper.GetBool("scope-challenge"), + ReadOnly: viper.GetBool("read-only"), + EnabledToolsets: enabledToolsets, + EnabledTools: enabledTools, + DynamicToolsets: viper.GetBool("dynamic_toolsets"), + ExcludeTools: excludeTools, + InsidersMode: viper.GetBool("insiders"), } return ghhttp.RunHTTPServer(httpConfig) diff --git a/pkg/http/handler.go b/pkg/http/handler.go index 2e828211d..37906a03e 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -236,13 +236,56 @@ func DefaultGitHubMCPServerFactory(r *http.Request, deps github.ToolDependencies return github.NewMCPServer(r.Context(), cfg, deps, inventory) } -// DefaultInventoryFactory creates the default inventory factory for HTTP mode -func DefaultInventoryFactory(_ *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker, scopeFetcher scopes.FetcherInterface) InventoryFactoryFunc { +// DefaultInventoryFactory creates the default inventory factory for HTTP mode. +// When the ServerConfig includes static flags (--toolsets, --read-only, etc.), +// 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 { + // 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, featureChecker) + hasStaticFilters := hasStaticConfig(cfg) + + // Pre-compute valid tool names for filtering per-request tool headers. + // When a request asks for a tool by name that's been excluded from the + // static universe, we silently drop it rather than returning an error. + validToolNames := make(map[string]bool, len(staticTools)) + for i := range staticTools { + validToolNames[staticTools[i].Tool.Name] = true + } + return func(r *http.Request) (*inventory.Inventory, error) { - b := github.NewInventory(t). + b := inventory.NewBuilder(). + SetTools(staticTools). + SetResources(staticResources). + SetPrompts(staticPrompts). WithDeprecatedAliases(github.DeprecatedToolAliases). WithFeatureChecker(featureChecker) + // When static flags constrain the universe, default to showing + // everything within those bounds (per-request filters narrow further). + // When no static flags are set, preserve existing behavior where + // the default toolsets apply. + if hasStaticFilters { + b = b.WithToolsets([]string{"all"}) + } + + // Static read-only is an upper bound — enforce before request filters + if cfg.ReadOnly { + b = b.WithReadOnly(true) + } + + // Static insiders mode — enforce before request filters + if cfg.InsidersMode { + b = b.WithInsidersMode(true) + } + + // Filter request tool names to only those in the static universe, + // so requests for statically-excluded tools degrade gracefully. + if hasStaticFilters { + r = filterRequestTools(r, validToolNames) + } + b = InventoryFiltersForRequest(r, b) b = PATScopeFilter(b, r, scopeFetcher) @@ -252,6 +295,69 @@ func DefaultInventoryFactory(_ *ServerConfig, t translations.TranslationHelperFu } } +// filterRequestTools returns a shallow copy of the request with any per-request +// tool names (from X-MCP-Tools header) filtered to only include tools that exist +// in validNames. This ensures requests for statically-excluded tools are silently +// ignored rather than causing build errors. +func filterRequestTools(r *http.Request, validNames map[string]bool) *http.Request { + reqTools := ghcontext.GetTools(r.Context()) + if len(reqTools) == 0 { + return r + } + + filtered := make([]string, 0, len(reqTools)) + for _, name := range reqTools { + if validNames[name] { + filtered = append(filtered, name) + } + } + ctx := ghcontext.WithTools(r.Context(), filtered) + return r.WithContext(ctx) +} + +// hasStaticConfig returns true if any static filtering flags are set on the ServerConfig. +func hasStaticConfig(cfg *ServerConfig) bool { + return cfg.ReadOnly || + cfg.EnabledToolsets != nil || + cfg.EnabledTools != nil || + cfg.DynamicToolsets || + len(cfg.ExcludeTools) > 0 || + cfg.InsidersMode +} + +// buildStaticInventory pre-filters the full tool/resource/prompt universe using +// the static CLI flags (--toolsets, --read-only, --exclude-tools, etc.). +// The returned slices serve as the upper bound for per-request inventory builders. +func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { + if !hasStaticConfig(cfg) { + return github.AllTools(t), github.AllResources(t), github.AllPrompts(t) + } + + b := github.NewInventory(t). + WithFeatureChecker(featureChecker). + WithReadOnly(cfg.ReadOnly). + WithToolsets(github.ResolvedEnabledToolsets(cfg.DynamicToolsets, cfg.EnabledToolsets, cfg.EnabledTools)). + WithInsidersMode(cfg.InsidersMode) + + if len(cfg.EnabledTools) > 0 { + b = b.WithTools(github.CleanTools(cfg.EnabledTools)) + } + + if len(cfg.ExcludeTools) > 0 { + b = b.WithExcludeTools(cfg.ExcludeTools) + } + + inv, err := b.Build() + if err != nil { + // Fall back to all tools if there's an error (e.g. unknown tool names). + // The error will surface again at per-request time if relevant. + return github.AllTools(t), github.AllResources(t), github.AllPrompts(t) + } + + ctx := context.Background() + return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) +} + // InventoryFiltersForRequest applies filters to the inventory builder // based on the request context and headers func InventoryFiltersForRequest(r *http.Request, builder *inventory.Builder) *inventory.Builder { diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 2a19e0a23..ee465c174 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -23,6 +23,10 @@ import ( ) func mockTool(name, toolsetID string, readOnly bool) inventory.ServerTool { + return mockToolFull(name, toolsetID, readOnly, false) +} + +func mockToolFull(name, toolsetID string, readOnly bool, isDefault bool) inventory.ServerTool { return inventory.ServerTool{ Tool: mcp.Tool{ Name: name, @@ -31,6 +35,7 @@ func mockTool(name, toolsetID string, readOnly bool) inventory.ServerTool { Toolset: inventory.ToolsetMetadata{ ID: inventory.ToolsetID(toolsetID), Description: "Test: " + toolsetID, + Default: isDefault, }, } } @@ -409,3 +414,253 @@ func TestHTTPHandlerRoutes(t *testing.T) { }) } } + +func TestStaticConfigEnforcement(t *testing.T) { + // Use default toolsets to match real-world behavior where repos/issues/pull_requests are defaults + tools := []inventory.ServerTool{ + mockToolFull("get_file_contents", "repos", true, true), + mockToolFull("create_repository", "repos", false, true), + mockToolFull("list_issues", "issues", true, true), + mockToolFull("create_issue", "issues", false, true), + mockToolFull("list_pull_requests", "pull_requests", true, true), + mockToolFull("create_pull_request", "pull_requests", false, true), + mockToolWithFeatureFlag("hidden_by_holdback", "repos", true, "", "mcp_holdback_consolidated_projects"), + } + + tests := []struct { + name string + config *ServerConfig + path string + headers map[string]string + expectedTools []string + }{ + { + name: "no static config preserves existing behavior", + config: &ServerConfig{Version: "test"}, + path: "/", + expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, + }, + { + name: "static read-only filters write tools", + config: &ServerConfig{Version: "test", ReadOnly: true}, + path: "/", + expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "hidden_by_holdback"}, + }, + { + name: "static read-only cannot be overridden by header", + config: &ServerConfig{Version: "test", ReadOnly: true}, + path: "/", + headers: map[string]string{ + headers.MCPReadOnlyHeader: "false", + }, + expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "hidden_by_holdback"}, + }, + { + name: "static toolsets restricts available tools", + config: &ServerConfig{Version: "test", EnabledToolsets: []string{"repos"}}, + path: "/", + expectedTools: []string{"get_file_contents", "create_repository", "hidden_by_holdback"}, + }, + { + name: "static toolsets cannot be expanded by header", + config: &ServerConfig{Version: "test", EnabledToolsets: []string{"repos"}}, + path: "/", + headers: map[string]string{ + headers.MCPToolsetsHeader: "issues", + }, + // Header asks for "issues" but only "repos" tools exist in the static universe + expectedTools: []string{}, + }, + { + name: "per-request header can narrow within static toolset bounds", + config: &ServerConfig{Version: "test", EnabledToolsets: []string{"repos", "issues"}}, + path: "/", + headers: map[string]string{ + headers.MCPToolsetsHeader: "repos", + }, + expectedTools: []string{"get_file_contents", "create_repository", "hidden_by_holdback"}, + }, + { + name: "static exclude-tools removes tools", + config: &ServerConfig{Version: "test", ExcludeTools: []string{"create_repository", "create_issue"}}, + path: "/", + expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, + }, + { + name: "static exclude-tools cannot be re-included by header", + config: &ServerConfig{Version: "test", ExcludeTools: []string{"create_repository"}}, + path: "/", + headers: map[string]string{ + headers.MCPToolsHeader: "create_repository,list_issues", + }, + // create_repository was excluded at static level, only list_issues available + expectedTools: []string{"list_issues"}, + }, + { + name: "static read-only combined with per-request toolset", + config: &ServerConfig{Version: "test", ReadOnly: true}, + path: "/", + headers: map[string]string{ + headers.MCPToolsetsHeader: "repos", + }, + expectedTools: []string{"get_file_contents", "hidden_by_holdback"}, + }, + { + name: "static toolset with URL readonly", + config: &ServerConfig{Version: "test", EnabledToolsets: []string{"repos", "issues"}}, + path: "/readonly", + expectedTools: []string{"get_file_contents", "list_issues", "hidden_by_holdback"}, + }, + { + name: "static tools enables specific tools only", + config: &ServerConfig{Version: "test", EnabledTools: []string{"list_issues", "get_file_contents"}}, + path: "/", + expectedTools: []string{"list_issues", "get_file_contents"}, + }, + { + name: "static tools cannot be expanded by header", + config: &ServerConfig{Version: "test", EnabledTools: []string{"list_issues"}}, + path: "/", + headers: map[string]string{ + headers.MCPToolsHeader: "create_repository", + }, + // create_repository isn't in the static universe so it's silently dropped; + // the empty filter shows all tools within static bounds + expectedTools: []string{"list_issues"}, + }, + { + name: "static exclude-tools combined with per-request exclude", + config: &ServerConfig{Version: "test", ExcludeTools: []string{"create_repository"}}, + path: "/", + headers: map[string]string{ + headers.MCPExcludeToolsHeader: "create_issue", + }, + // Both static and per-request exclusions apply + expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var capturedInventory *inventory.Inventory + var capturedCtx context.Context + + featureChecker := func(ctx context.Context, flag string) (bool, error) { + return slices.Contains(ghcontext.GetHeaderFeatures(ctx), flag), nil + } + + apiHost, err := utils.NewAPIHost("https://api.github.com") + require.NoError(t, err) + + // Build static tools the same way the production code does + staticTools, staticResources, staticPrompts := buildStaticInventoryFromTools(tt.config, tools, featureChecker) + hasStatic := hasStaticConfig(tt.config) + + validToolNames := make(map[string]bool, len(staticTools)) + for _, tool := range staticTools { + validToolNames[tool.Tool.Name] = true + } + + inventoryFactory := func(r *http.Request) (*inventory.Inventory, error) { + capturedCtx = r.Context() + builder := inventory.NewBuilder(). + SetTools(staticTools). + SetResources(staticResources). + SetPrompts(staticPrompts). + WithDeprecatedAliases(github.DeprecatedToolAliases). + WithFeatureChecker(featureChecker) + + if hasStatic { + builder = builder.WithToolsets([]string{"all"}) + } + if tt.config.ReadOnly { + builder = builder.WithReadOnly(true) + } + if tt.config.InsidersMode { + builder = builder.WithInsidersMode(true) + } + + if hasStatic { + r = filterRequestTools(r, validToolNames) + } + + builder = InventoryFiltersForRequest(r, builder) + inv, buildErr := builder.Build() + if buildErr != nil { + return nil, buildErr + } + capturedInventory = inv + return inv, nil + } + + mcpServerFactory := func(_ *http.Request, _ github.ToolDependencies, _ *inventory.Inventory, _ *github.MCPServerConfig) (*mcp.Server, error) { + return mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.0.1"}, nil), nil + } + + handler := NewHTTPMcpHandler( + context.Background(), + tt.config, + nil, + translations.NullTranslationHelper, + slog.Default(), + apiHost, + WithInventoryFactory(inventoryFactory), + WithGitHubMCPServerFactory(mcpServerFactory), + WithScopeFetcher(allScopesFetcher{}), + ) + + r := chi.NewRouter() + handler.RegisterMiddleware(r) + handler.RegisterRoutes(r) + + req := httptest.NewRequest(http.MethodPost, tt.path, nil) + req.Header.Set(headers.AuthorizationHeader, "Bearer ghp_testtoken") + for k, v := range tt.headers { + req.Header.Set(k, v) + } + + rr := httptest.NewRecorder() + r.ServeHTTP(rr, req) + + require.NotNil(t, capturedInventory, "inventory should have been created") + + toolNames := extractToolNames(capturedCtx, capturedInventory) + expectedSorted := make([]string, len(tt.expectedTools)) + copy(expectedSorted, tt.expectedTools) + sort.Strings(expectedSorted) + + assert.Equal(t, expectedSorted, toolNames, "tools should match expected") + }) + } +} + +// 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, featureChecker inventory.FeatureFlagChecker) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { + if !hasStaticConfig(cfg) { + return tools, nil, nil + } + + b := inventory.NewBuilder(). + SetTools(tools). + WithFeatureChecker(featureChecker). + WithReadOnly(cfg.ReadOnly). + WithToolsets(github.ResolvedEnabledToolsets(cfg.DynamicToolsets, cfg.EnabledToolsets, cfg.EnabledTools)). + WithInsidersMode(cfg.InsidersMode) + + if len(cfg.EnabledTools) > 0 { + b = b.WithTools(github.CleanTools(cfg.EnabledTools)) + } + + if len(cfg.ExcludeTools) > 0 { + b = b.WithExcludeTools(cfg.ExcludeTools) + } + + inv, err := b.Build() + if err != nil { + return tools, nil, nil + } + + ctx := context.Background() + return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) +} diff --git a/pkg/http/server.go b/pkg/http/server.go index 872303940..c9fc21d33 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -67,6 +67,28 @@ type ServerConfig struct { // ScopeChallenge indicates if we should return OAuth scope challenges, and if we should perform // tool filtering based on token scopes. ScopeChallenge bool + + // ReadOnly indicates if we should only register read-only tools. + // When set via CLI flag, this acts as an upper bound — per-request headers + // cannot re-enable write tools. + ReadOnly bool + + // EnabledToolsets is a list of toolsets to enable. + // When set via CLI flag, per-request headers can only narrow within these toolsets. + EnabledToolsets []string + + // EnabledTools is a list of specific tools to enable (additive to toolsets). + EnabledTools []string + + // DynamicToolsets enables dynamic toolset discovery mode. + DynamicToolsets bool + + // ExcludeTools is a list of tool names to disable regardless of other settings. + // When set via CLI flag, per-request headers cannot re-include these tools. + ExcludeTools []string + + // InsidersMode indicates if we should enable experimental features. + InsidersMode bool } func RunHTTPServer(cfg ServerConfig) error { @@ -90,7 +112,7 @@ func RunHTTPServer(cfg ServerConfig) error { slogHandler = slog.NewTextHandler(logOutput, &slog.HandlerOptions{Level: slog.LevelInfo}) } logger := slog.New(slogHandler) - logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "lockdownEnabled", cfg.LockdownMode) + logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "lockdownEnabled", cfg.LockdownMode, "readOnly", cfg.ReadOnly, "insidersMode", cfg.InsidersMode) apiHost, err := utils.NewAPIHost(cfg.Host) if err != nil {