diff --git a/README.md b/README.md index 5b90f64a58..740e0691ee 100644 --- a/README.md +++ b/README.md @@ -1650,6 +1650,27 @@ docker run -i --rm \ ghcr.io/github/github-mcp-server ``` +### Per-toolset read-only mode + +Use `--read-only-toolsets` or `GITHUB_READ_ONLY_TOOLSETS` to keep selected toolsets read-only while allowing writes in other enabled toolsets. This works with both `stdio` and self-hosted `http` servers: + +```bash +./github-mcp-server stdio --toolsets=repos,issues,pull_requests --read-only-toolsets=issues,pull_requests +``` + +Environment equivalent: `GITHUB_READ_ONLY_TOOLSETS=issues,pull_requests`. + +Read tools in `issues` and `pull_requests` remain available; their write tools are omitted. Repository write tools remain available. The policy follows each tool's declared toolset and read-only annotation. + +- Global `--read-only` still blocks all write tools. +- Explicit `--tools` selections and HTTP request configuration cannot bypass the restriction. +- Listing a toolset here does not enable it. Normal toolset selection still applies. +- In HTTP mode, this policy alone preserves default tool selection and allows request headers to select other toolsets. Use `--toolsets` to limit which toolsets requests may select. +- Names are trimmed and deduplicated. `all` restricts every toolset; `default` restricts the default toolsets. Unknown names cause startup to fail. +- `list-scopes` honors this setting when reporting available tools and required scopes. + +This is a static policy; it does not prompt for approval or grant temporary write access. + ## Lockdown Mode Lockdown mode limits the content that the server will surface from public repositories. When enabled, the server checks whether the author of each item has push access to the repository. Private repositories are unaffected, and collaborators keep full access to their own content. diff --git a/cmd/github-mcp-server/list_scopes.go b/cmd/github-mcp-server/list_scopes.go index 8d6d038d1e..2c9c3f568f 100644 --- a/cmd/github-mcp-server/list_scopes.go +++ b/cmd/github-mcp-server/list_scopes.go @@ -99,6 +99,11 @@ func runListScopes() error { } } + var readOnlyToolsets []string + if err := viper.UnmarshalKey("read-only-toolsets", &readOnlyToolsets); err != nil { + return fmt.Errorf("failed to unmarshal read-only-toolsets: %w", err) + } + readOnly := viper.GetBool("read-only") outputFormat := viper.GetString("list-scopes-output") @@ -107,7 +112,8 @@ func runListScopes() error { // Build inventory using the same logic as the stdio server inventoryBuilder := github.NewInventory(t). - WithReadOnly(readOnly) + WithReadOnly(readOnly). + WithReadOnlyToolsets(readOnlyToolsets) // Configure toolsets (same as stdio) if enabledToolsets != nil { diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index c0cadbbc63..eceebd0b0d 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -108,6 +108,11 @@ var ( } } + var readOnlyToolsets []string + if err := viper.UnmarshalKey("read-only-toolsets", &readOnlyToolsets); err != nil { + return fmt.Errorf("failed to unmarshal read-only-toolsets: %w", err) + } + ttl := viper.GetDuration("repo-access-cache-ttl") stdioServerConfig := ghmcp.StdioServerConfig{ Version: version, @@ -117,6 +122,7 @@ var ( EnabledTools: enabledTools, EnabledFeatures: enabledFeatures, ReadOnly: viper.GetBool("read-only"), + ReadOnlyToolsets: readOnlyToolsets, ExportTranslations: viper.GetBool("export-translations"), EnableCommandLogging: viper.GetBool("enable-command-logging"), LogFilePath: viper.GetString("log-file"), @@ -195,6 +201,11 @@ var ( } } + var readOnlyToolsets []string + if err := viper.UnmarshalKey("read-only-toolsets", &readOnlyToolsets); err != nil { + return fmt.Errorf("failed to unmarshal read-only-toolsets: %w", err) + } + ttl := viper.GetDuration("repo-access-cache-ttl") httpConfig := ghhttp.ServerConfig{ Version: version, @@ -212,6 +223,7 @@ var ( RepoAccessCacheTTL: &ttl, ScopeChallenge: viper.GetBool("scope-challenge"), ReadOnly: viper.GetBool("read-only"), + ReadOnlyToolsets: readOnlyToolsets, EnabledToolsets: enabledToolsets, EnabledTools: enabledTools, ExcludeTools: excludeTools, @@ -237,6 +249,7 @@ func init() { rootCmd.PersistentFlags().StringSlice("tools", nil, "Comma-separated list of specific tools to enable") rootCmd.PersistentFlags().StringSlice("exclude-tools", nil, "Comma-separated list of tool names to disable regardless of other settings") rootCmd.PersistentFlags().StringSlice("features", nil, "Comma-separated list of feature flags to enable") + rootCmd.PersistentFlags().StringSlice("read-only-toolsets", nil, "Comma-separated list of toolsets to restrict to read-only operations (supports all and default)") rootCmd.PersistentFlags().Bool("read-only", false, "Restrict the server to read-only operations") rootCmd.PersistentFlags().String("log-file", "", "Path to log file") rootCmd.PersistentFlags().Bool("enable-command-logging", false, "When enabled, the server will log all command requests and responses to the log file") @@ -274,6 +287,7 @@ func init() { _ = viper.BindPFlag("tools", rootCmd.PersistentFlags().Lookup("tools")) _ = viper.BindPFlag("exclude_tools", rootCmd.PersistentFlags().Lookup("exclude-tools")) _ = viper.BindPFlag("features", rootCmd.PersistentFlags().Lookup("features")) + _ = viper.BindPFlag("read-only-toolsets", rootCmd.PersistentFlags().Lookup("read-only-toolsets")) _ = viper.BindPFlag("read-only", rootCmd.PersistentFlags().Lookup("read-only")) _ = viper.BindPFlag("log-file", rootCmd.PersistentFlags().Lookup("log-file")) _ = viper.BindPFlag("enable-command-logging", rootCmd.PersistentFlags().Lookup("enable-command-logging")) diff --git a/cmd/github-mcp-server/main_test.go b/cmd/github-mcp-server/main_test.go index a5b2b84967..8c872951d7 100644 --- a/cmd/github-mcp-server/main_test.go +++ b/cmd/github-mcp-server/main_test.go @@ -10,6 +10,7 @@ import ( "github.com/github/github-mcp-server/pkg/scopes" "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/spf13/pflag" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -93,3 +94,24 @@ func TestSchemaTypeString(t *testing.T) { }) } } + +func TestReadOnlyToolsetsConfiguration(t *testing.T) { + initConfig() + flag := rootCmd.PersistentFlags().Lookup("read-only-toolsets") + require.NotNil(t, flag) + originalValue, err := rootCmd.PersistentFlags().GetStringSlice("read-only-toolsets") + require.NoError(t, err) + originalChanged := flag.Changed + t.Cleanup(func() { + require.NoError(t, flag.Value.(pflag.SliceValue).Replace(originalValue)) + flag.Changed = originalChanged + }) + t.Setenv("GITHUB_READ_ONLY_TOOLSETS", "issues,pull_requests") + var toolsets []string + require.NoError(t, viper.UnmarshalKey("read-only-toolsets", &toolsets)) + require.Equal(t, []string{"issues", "pull_requests"}, toolsets) + + require.NoError(t, rootCmd.PersistentFlags().Set("read-only-toolsets", "repos")) + require.NoError(t, viper.UnmarshalKey("read-only-toolsets", &toolsets)) + require.Equal(t, []string{"repos"}, toolsets, "CLI flag takes precedence over environment") +} diff --git a/docs/server-configuration.md b/docs/server-configuration.md index 42584362d9..ca7e12847e 100644 --- a/docs/server-configuration.md +++ b/docs/server-configuration.md @@ -10,6 +10,7 @@ We currently support the following ways in which the GitHub MCP Server can be co | Toolsets | `X-MCP-Toolsets` header or `/x/{toolset}` URL | `--toolsets` flag or `GITHUB_TOOLSETS` env var | | Individual Tools | `X-MCP-Tools` header | `--tools` flag or `GITHUB_TOOLS` env var | | Exclude Tools | `X-MCP-Exclude-Tools` header | `--exclude-tools` flag or `GITHUB_EXCLUDE_TOOLS` env var | +| Per-toolset read-only | Not available on the hosted server | `--read-only-toolsets` or `GITHUB_READ_ONLY_TOOLSETS` (stdio and self-hosted HTTP) | | Read-Only Mode | `X-MCP-Readonly` header or `/readonly` URL | `--read-only` flag or `GITHUB_READ_ONLY` env var | | Lockdown Mode | `X-MCP-Lockdown` header | `--lockdown-mode` flag or `GITHUB_LOCKDOWN_MODE` env var | | Insiders Mode | `X-MCP-Insiders` header or `/insiders` URL | `--insiders` flag or `GITHUB_INSIDERS` env var | @@ -233,6 +234,8 @@ Listed tools are removed regardless of any other configuration — even if their When active, this mode will disable all tools that are not read-only even if they were requested. +For a mixed policy on a local or self-hosted server, use `--read-only-toolsets=issues,pull_requests` or `GITHUB_READ_ONLY_TOOLSETS=issues,pull_requests`. Reads in those toolsets remain available, while their write tools are removed. Other enabled toolsets remain writable. Global read-only takes precedence, and explicit tools or HTTP request headers cannot restore blocked tools. This setting does not enable toolsets. Names are trimmed and deduplicated; `all` and `default` are supported, and unknown names fail startup. See [per-toolset read-only mode](../README.md#per-toolset-read-only-mode). + **Example:** diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index f713a44026..1c10aacc44 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -211,6 +211,7 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se inventoryBuilder := github.NewInventory(cfg.Translator, github.WithHost(hostType)). WithDeprecatedAliases(github.DeprecatedToolAliases). WithReadOnly(cfg.ReadOnly). + WithReadOnlyToolsets(cfg.ReadOnlyToolsets). WithToolsets(github.ResolvedEnabledToolsets(cfg.EnabledToolsets, cfg.EnabledTools)). WithTools(github.CleanTools(cfg.EnabledTools)). WithExcludeTools(cfg.ExcludeTools). @@ -262,6 +263,9 @@ type StdioServerConfig struct { // ReadOnly indicates if we should only register read-only tools ReadOnly bool + // ReadOnlyToolsets restricts write tools in the listed toolsets. + ReadOnlyToolsets []string + // ExportTranslations indicates if we should export translations // See: https://github.com/github/github-mcp-server?tab=readme-ov-file#i18n--overriding-descriptions ExportTranslations bool @@ -375,6 +379,7 @@ func RunStdioServer(cfg StdioServerConfig) error { EnabledTools: cfg.EnabledTools, EnabledFeatures: cfg.EnabledFeatures, ReadOnly: cfg.ReadOnly, + ReadOnlyToolsets: cfg.ReadOnlyToolsets, Translator: t, ContentWindowSize: cfg.ContentWindowSize, LockdownMode: cfg.LockdownMode, diff --git a/pkg/github/server.go b/pkg/github/server.go index 6e9b5b7566..30c99173d1 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -41,6 +41,9 @@ type MCPServerConfig struct { // ReadOnly indicates if we should only offer read-only tools ReadOnly bool + // ReadOnlyToolsets restricts write tools in the listed toolsets. + ReadOnlyToolsets []string + // Translator provides translated text for the server tooling Translator translations.TranslationHelperFunc diff --git a/pkg/http/handler.go b/pkg/http/handler.go index 1aad3ed01d..3b45a4bdfe 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -339,7 +339,7 @@ func NewDefaultInventoryFactory(cfg *ServerConfig, t translations.TranslationHel // Filter request tool names to only those in the static universe, // so requests for statically-excluded tools degrade gracefully. - if hasStaticFilters { + if hasStaticFilters || len(cfg.ReadOnlyToolsets) > 0 { r = filterRequestTools(r, validToolNames) } @@ -372,7 +372,8 @@ func filterRequestTools(r *http.Request, validNames map[string]bool) *http.Reque return r.WithContext(ctx) } -// hasStaticConfig returns true if any static filtering flags are set on the ServerConfig. +// hasStaticConfig reports whether static configuration sets the toolset bounds. +// Per-toolset read-only policy alone preserves per-request toolset selection. func hasStaticConfig(cfg *ServerConfig) bool { return cfg.ReadOnly || cfg.EnabledToolsets != nil || @@ -407,7 +408,7 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun }) } - if !hasStaticConfig(cfg) { + if !hasStaticConfig(cfg) && len(cfg.ReadOnlyToolsets) == 0 { return filterUnavailable(tools), github.AllResources(t), github.AllPrompts(t), nil } @@ -416,7 +417,13 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun SetResources(github.AllResources(t)). SetPrompts(github.AllPrompts(t)). WithReadOnly(cfg.ReadOnly). + WithReadOnlyToolsets(cfg.ReadOnlyToolsets). WithToolsets(github.ResolvedEnabledToolsets(cfg.EnabledToolsets, cfg.EnabledTools)) + if !hasStaticConfig(cfg) { + // Apply the policy to the full catalogue; request defaults and headers + // still determine which toolsets are selected. + b = b.WithToolsets([]string{"all"}) + } if len(cfg.EnabledTools) > 0 { b = b.WithTools(github.CleanTools(cfg.EnabledTools)) diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index ea37ec2a6a..75d81d05d6 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -487,6 +487,12 @@ func TestStaticConfigEnforcement(t *testing.T) { path: "/", expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "hidden_by_holdback"}, }, + { + name: "per-toolset policy filters only its writes", + config: &ServerConfig{Version: "test", ReadOnlyToolsets: []string{"issues"}}, + path: "/", + expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, + }, { name: "static read-only cannot be overridden by header", config: &ServerConfig{Version: "test", ReadOnly: true}, @@ -619,7 +625,7 @@ func TestStaticConfigEnforcement(t *testing.T) { builder = builder.WithReadOnly(true) } - if hasStatic { + if hasStatic || len(tt.config.ReadOnlyToolsets) > 0 { r = filterRequestTools(r, validToolNames) } @@ -897,14 +903,18 @@ 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, error) { - if !hasStaticConfig(cfg) { + if !hasStaticConfig(cfg) && len(cfg.ReadOnlyToolsets) == 0 { return tools, nil, nil, nil } b := inventory.NewBuilder(). SetTools(tools). WithReadOnly(cfg.ReadOnly). + WithReadOnlyToolsets(cfg.ReadOnlyToolsets). WithToolsets(github.ResolvedEnabledToolsets(cfg.EnabledToolsets, cfg.EnabledTools)) + if !hasStaticConfig(cfg) { + b = b.WithToolsets([]string{"all"}) + } if len(cfg.EnabledTools) > 0 { b = b.WithTools(github.CleanTools(cfg.EnabledTools)) @@ -1525,3 +1535,94 @@ func TestMaxRequestBodySizeEnforcement(t *testing.T) { assert.True(t, mcpServerFactoryCalled, "the MCP server should be constructed for an allowed request") }) } + +func TestDefaultInventoryFactoryReadOnlyToolsets(t *testing.T) { + cfg := &ServerConfig{ + Version: "test", EnabledToolsets: []string{"issues", "repos", "pull_requests"}, + ReadOnlyToolsets: []string{"issues", "pull_requests"}, + EnabledTools: []string{"add_issue_comment"}, + } + factory, err := NewDefaultInventoryFactory(cfg, translations.NullTranslationHelper, nil, allScopesFetcher{}) + require.NoError(t, err) + for _, explicit := range []bool{false, true} { + t.Run(fmt.Sprintf("explicit_request_%v", explicit), func(t *testing.T) { + ctx := ghcontext.WithToolsets(context.Background(), []string{"all"}) + if explicit { + ctx = ghcontext.WithTools(ctx, []string{"add_issue_comment", "merge_pull_request", "push_files", "get_file_contents"}) + } + req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) + req.Header.Set(headers.MCPReadOnlyHeader, "false") + inv, err := factory(req) + require.NoError(t, err) + names := extractToolNames(ctx, inv) + require.Contains(t, names, "push_files") + require.Contains(t, names, "get_file_contents") + require.NotContains(t, names, "add_issue_comment") + require.NotContains(t, names, "merge_pull_request") + require.Empty(t, inv.ForMCPRequest(inventory.MCPMethodToolsCall, "add_issue_comment").AvailableTools(ctx)) + }) + } + // A policy alone must trigger static filtering, even without tool selection. + factory, err = NewDefaultInventoryFactory(&ServerConfig{ + ReadOnlyToolsets: []string{"issues"}, + }, translations.NullTranslationHelper, nil, allScopesFetcher{}) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodPost, "/", nil) + inv, err := factory(req) + require.NoError(t, err) + names := extractToolNames(req.Context(), inv) + require.NotContains(t, names, "add_issue_comment") + require.Contains(t, names, "issue_read") + require.Contains(t, names, "push_files") + + cfg.ReadOnlyToolsets = []string{"pull-request"} + factory, err = NewDefaultInventoryFactory(cfg, translations.NullTranslationHelper, nil, allScopesFetcher{}) + require.ErrorIs(t, err, inventory.ErrUnknownReadOnlyToolsets) + require.Nil(t, factory) +} + +func TestReadOnlyToolsetsRequestSelection(t *testing.T) { + tests := []struct { + name string + policy []string + toolsets string + tools string + readonly string + want []string + absent []string + }{ + {name: "defaults remain defaults", policy: []string{"issues"}, want: []string{"issue_read", "push_files"}, absent: []string{"add_issue_comment", "actions_list"}}, + {name: "non-default toolset", policy: []string{"issues"}, toolsets: "actions", want: []string{"actions_list", "actions_run_trigger"}, absent: []string{"issue_read"}}, + {name: "restricted non-default toolset", policy: []string{"actions"}, toolsets: "actions", want: []string{"actions_list"}, absent: []string{"actions_run_trigger"}}, + {name: "explicit tools respect policy", policy: []string{"issues"}, tools: "add_issue_comment,actions_run_trigger", readonly: "false", want: []string{"actions_run_trigger"}, absent: []string{"add_issue_comment", "issue_read"}}, + {name: "request can restrict further", policy: []string{"issues"}, toolsets: "actions", readonly: "true", want: []string{"actions_list"}, absent: []string{"actions_run_trigger"}}, + {name: "empty policy", policy: []string{}, toolsets: "actions,issues", want: []string{"actions_run_trigger", "add_issue_comment"}}, + {name: "blank policy", policy: []string{" ", ""}, toolsets: "actions,issues", want: []string{"actions_run_trigger", "add_issue_comment"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + factory, err := NewDefaultInventoryFactory(&ServerConfig{ReadOnlyToolsets: tt.policy}, translations.NullTranslationHelper, createHTTPFeatureChecker(nil, false), allScopesFetcher{}) + require.NoError(t, err) + called := false + handler := middleware.WithRequestConfig(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + called = true + inv, err := factory(r) + require.NoError(t, err) + names := extractToolNames(r.Context(), inv) + for _, name := range tt.want { + assert.Contains(t, names, name) + } + for _, name := range tt.absent { + assert.NotContains(t, names, name) + assert.Empty(t, inv.ForMCPRequest(inventory.MCPMethodToolsCall, name).AvailableTools(r.Context())) + } + })) + req := httptest.NewRequest(http.MethodPost, "/", nil) + req.Header.Set(headers.MCPToolsetsHeader, tt.toolsets) + req.Header.Set(headers.MCPToolsHeader, tt.tools) + req.Header.Set(headers.MCPReadOnlyHeader, tt.readonly) + handler.ServeHTTP(httptest.NewRecorder(), req) + require.True(t, called) + }) + } +} diff --git a/pkg/http/server.go b/pkg/http/server.go index 8a3a305e49..41ce696659 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -95,6 +95,9 @@ type ServerConfig struct { // cannot re-enable write tools. ReadOnly bool + // ReadOnlyToolsets restricts write tools in the listed toolsets. + ReadOnlyToolsets []string + // EnabledToolsets is a list of toolsets to enable. // When set via CLI flag, per-request headers can only narrow within these toolsets. EnabledToolsets []string diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 60bda764b6..1352399e23 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -12,6 +12,9 @@ import ( var ( // ErrUnknownTools is returned when tools specified via WithTools() are not recognized. ErrUnknownTools = errors.New("unknown tools specified in WithTools") + + // ErrUnknownReadOnlyToolsets is returned for invalid read-only policy toolsets. + ErrUnknownReadOnlyToolsets = errors.New("unknown toolsets specified in WithReadOnlyToolsets") ) // mcpAppsFeatureFlag is the feature flag name that controls MCP Apps UI metadata. @@ -47,6 +50,7 @@ type Builder struct { // Configuration options (processed at Build time) readOnly bool + readOnlyToolsets []string toolsetIDs []string // raw input, processed at Build() toolsetIDsIsNil bool // tracks if nil was passed (nil = defaults) additionalTools []string // raw input, processed at Build() @@ -95,6 +99,15 @@ func (b *Builder) WithReadOnly(readOnly bool) *Builder { return b } +// WithReadOnlyToolsets restricts write tools in the specified toolsets. +// It does not enable toolsets or override global read-only mode. Names are +// trimmed and deduplicated at Build time; "all" and "default" expand to the +// corresponding toolsets. Unknown names cause Build to fail. +func (b *Builder) WithReadOnlyToolsets(toolsetIDs []string) *Builder { + b.readOnlyToolsets = slices.Clone(toolsetIDs) + return b +} + func (b *Builder) WithServerInstructions() *Builder { b.generateInstructions = true return b @@ -202,7 +215,8 @@ func cleanTools(tools []string) []string { // AvailableTools(), RegisterAll(), etc. // // Build returns an error if any tools specified via WithTools() are not recognized -// (i.e., they don't exist in the tool set and are not deprecated aliases). +// (i.e., they don't exist in the tool set and are not deprecated aliases), or +// WithReadOnlyToolsets() contains an unknown toolset. // This ensures invalid tool configurations fail fast at build time. func (b *Builder) Build() (*Inventory, error) { tools := b.tools @@ -222,6 +236,28 @@ func (b *Builder) Build() (*Inventory, error) { // Process toolsets and pre-compute metadata in a single pass r.enabledToolsets, r.unrecognizedToolsets, r.toolsetIDs, r.toolsetIDSet, r.defaultToolsetIDs, r.toolsetDescriptions = b.processToolsets() + r.readOnlyToolsets = make(map[ToolsetID]bool) + var unknownReadOnlyToolsets []string + for _, id := range cleanTools(b.readOnlyToolsets) { + switch id { + case "all": + maps.Copy(r.readOnlyToolsets, r.toolsetIDSet) + case "default": + for _, defaultID := range r.defaultToolsetIDs { + r.readOnlyToolsets[defaultID] = true + } + default: + if !r.toolsetIDSet[ToolsetID(id)] { + unknownReadOnlyToolsets = append(unknownReadOnlyToolsets, id) + } else { + r.readOnlyToolsets[ToolsetID(id)] = true + } + } + } + if len(unknownReadOnlyToolsets) > 0 { + return nil, fmt.Errorf("%w: %s", ErrUnknownReadOnlyToolsets, strings.Join(unknownReadOnlyToolsets, ", ")) + } + // Build set of valid tool names for validation validToolNames := make(map[string]bool, len(tools)) for i := range tools { diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index b3dfbeb912..1c669a5472 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -41,7 +41,7 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool, feature } } // 2. Apply static inventory filters. - if r.readOnly && !tool.IsReadOnly() { + if (r.readOnly || r.readOnlyToolsets[tool.Toolset.ID]) && !tool.IsReadOnly() { return false } for _, filter := range r.filters { diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 9439e6f201..571ce6b0a2 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -43,6 +43,8 @@ type Inventory struct { // Filters - these control what's returned by Available* methods // readOnly when true filters out write tools readOnly bool + // readOnlyToolsets filters write tools by their owning toolset. + readOnlyToolsets map[ToolsetID]bool // enabledToolsets when non-nil, only include tools/resources/prompts from these toolsets // when nil, all toolsets are enabled enabledToolsets map[ToolsetID]bool @@ -110,8 +112,9 @@ func (r *Inventory) ForMCPRequest(method string, itemName string) *Inventory { prompts: r.prompts, deprecatedAliases: r.deprecatedAliases, readOnly: r.readOnly, - enabledToolsets: r.enabledToolsets, // shared, not modified - additionalTools: r.additionalTools, // shared, not modified + readOnlyToolsets: r.readOnlyToolsets, // shared, not modified + enabledToolsets: r.enabledToolsets, // shared, not modified + additionalTools: r.additionalTools, // shared, not modified featureChecker: r.featureChecker, filters: r.filters, // shared, not modified unrecognizedToolsets: r.unrecognizedToolsets, diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 2e21fc632c..995c1b5245 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -2547,3 +2547,68 @@ func TestForMCPRequest_PreservesInstructions(t *testing.T) { "instructions must be preserved for %s (server identity)", m) } } + +func TestWithReadOnlyToolsets(t *testing.T) { + tools := []ServerTool{ + mockToolWithDefault("issue_read", "issues", true, true), + mockToolWithDefault("issue_write", "issues", false, true), + mockTool("repo_read", "repos", true), + mockTool("repo_write", "repos", false), + } + unannotated := mockTool("unannotated", "issues", false) + unannotated.Tool.Annotations = nil + tools = append(tools, unannotated) + tests := []struct { + name string + policy []string + enabled []string + additional []string + global bool + want []string + }{ + {name: "unset preserves writes", enabled: []string{"all"}, want: []string{"issue_read", "issue_write", "unannotated", "repo_read", "repo_write"}}, + {name: "mixed policy", policy: []string{" issues ", "issues", ""}, enabled: []string{"all"}, want: []string{"issue_read", "repo_read", "repo_write"}}, + {name: "global wins", policy: []string{"issues"}, enabled: []string{"all"}, global: true, want: []string{"issue_read", "repo_read"}}, + {name: "explicit tools and aliases cannot bypass", policy: []string{"issues"}, enabled: []string{"repos"}, additional: []string{"old_issue_write", "unannotated", "issue_read"}, want: []string{"issue_read", "repo_read", "repo_write"}}, + {name: "does not enable toolset", policy: []string{"issues"}, enabled: []string{"repos"}, want: []string{"repo_read", "repo_write"}}, + {name: "all", policy: []string{"all"}, enabled: []string{"all"}, want: []string{"issue_read", "repo_read"}}, + {name: "default", policy: []string{"default"}, enabled: []string{"all"}, want: []string{"issue_read", "repo_read", "repo_write"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + inv := mustBuild(t, NewBuilder().SetTools(tools). + WithDeprecatedAliases(map[string]string{"old_issue_write": "issue_write"}). + WithToolsets(tt.enabled).WithTools(tt.additional). + WithReadOnly(tt.global).WithReadOnlyToolsets(tt.policy)) + names := []string{} + for _, tool := range inv.AvailableTools(context.Background()) { + names = append(names, tool.Tool.Name) + } + require.ElementsMatch(t, tt.want, names) + // Request-specific inventories must preserve the same policy for direct calls. + for _, tool := range tools { + allowed := false + for _, name := range tt.want { + if name == tool.Tool.Name { + allowed = true + } + } + called := inv.ForMCPRequest(MCPMethodToolsCall, tool.Tool.Name).AvailableTools(context.Background()) + require.Equal(t, allowed, len(called) == 1, tool.Tool.Name) + } + if len(tt.policy) > 0 { + require.Empty(t, inv.ForMCPRequest(MCPMethodToolsCall, "old_issue_write").AvailableTools(context.Background())) + } + }) + } +} + +func TestWithReadOnlyToolsetsRejectsUnknownNames(t *testing.T) { + for _, policy := range [][]string{{"issues", "pull-request"}, {"all", "pull-request"}, {"default", "pull-request"}} { + inv, err := NewBuilder().SetTools([]ServerTool{mockTool("issue_write", "issues", false)}). + WithReadOnlyToolsets(policy).Build() + require.ErrorIs(t, err, ErrUnknownReadOnlyToolsets) + require.ErrorContains(t, err, "pull-request") + require.Nil(t, inv) + } +}
Remote ServerLocal Server