From 98aef1fe64065c40813adcb5403ec43282794073 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Sat, 15 Aug 2026 00:43:51 +0200 Subject: [PATCH 01/11] feat(repos): add confirmed repository deletion Add a destructive delete_repository tool that requires an exact owner/repo confirmation through multi-round-trip elicitation. Gate the tool to MCP protocol 2026-07-28 and newer across local and remote transports. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- README.md | 5 + internal/ghmcp/oauth.go | 10 +- .../__toolsnaps__/delete_repository.snap | 27 +++ pkg/github/helper_test.go | 1 + pkg/github/repositories.go | 113 ++++++++++++ pkg/github/repositories_test.go | 167 ++++++++++++++++++ pkg/github/tools.go | 1 + pkg/http/handler_test.go | 91 ++++++++++ pkg/inventory/protocol_version.go | 74 ++++++++ pkg/inventory/protocol_version_test.go | 119 +++++++++++++ pkg/inventory/registry.go | 4 +- pkg/inventory/server_tool.go | 4 + pkg/scopes/scopes.go | 3 + pkg/scopes/scopes_test.go | 5 + 14 files changed, 614 insertions(+), 10 deletions(-) create mode 100644 pkg/github/__toolsnaps__/delete_repository.snap create mode 100644 pkg/inventory/protocol_version.go create mode 100644 pkg/inventory/protocol_version_test.go diff --git a/README.md b/README.md index 32f8eb82bc..efae2b8ac4 100644 --- a/README.md +++ b/README.md @@ -1308,6 +1308,11 @@ The following sets of tools are available: - `path`: Path to the file to delete (string, required) - `repo`: Repository name (string, required) +- **delete_repository** - Delete repository + - **Required OAuth Scopes**: `delete_repo` + - `owner`: Repository owner (username or organization) (string, required) + - `repo`: Repository name (string, required) + - **fork_repository** - Fork repository - **Required OAuth Scopes**: `repo` - `organization`: Organization to fork to (string, optional) diff --git a/internal/ghmcp/oauth.go b/internal/ghmcp/oauth.go index 35e48f5bbc..3648189217 100644 --- a/internal/ghmcp/oauth.go +++ b/internal/ghmcp/oauth.go @@ -103,14 +103,6 @@ type oauthAuthenticator interface { // delayed response from an older prompt from affecting a newer flow. const oauthElicitIDPrefix = "github_authorization:" -// protocolVersionNoServerElicitation is the first MCP protocol version that -// forbids server-initiated JSON-RPC requests (SEP-2322): from this version on -// the server may not send elicitation/create while serving a request and must -// instead return an InputRequests map from the tool call (multi round-trip -// requests). It mirrors the go-sdk's internal constant of the same value, which -// the SDK does not export. -const protocolVersionNoServerElicitation = "2026-07-28" - // serverMayInitiateElicitation reports whether the server is permitted to send // elicitation requests to the client itself, which the spec allows only before // protocol version 2026-07-28. A nil or un-negotiated session (only reached in @@ -120,7 +112,7 @@ func serverMayInitiateElicitation(ss *mcp.ServerSession) bool { return true } params := ss.InitializeParams() - return params == nil || params.ProtocolVersion < protocolVersionNoServerElicitation + return params == nil || params.ProtocolVersion < inventory.ProtocolVersionMultiRoundTrip } // createOAuthToolMiddleware returns tool-handler middleware that authorizes the diff --git a/pkg/github/__toolsnaps__/delete_repository.snap b/pkg/github/__toolsnaps__/delete_repository.snap new file mode 100644 index 0000000000..a75845b3f4 --- /dev/null +++ b/pkg/github/__toolsnaps__/delete_repository.snap @@ -0,0 +1,27 @@ +{ + "annotations": { + "destructiveHint": true, + "idempotentHint": false, + "readOnlyHint": false, + "title": "Delete repository" + }, + "description": "Delete a GitHub repository after the user confirms the exact owner/repository name", + "inputSchema": { + "properties": { + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo" + ], + "type": "object" + }, + "name": "delete_repository" +} \ No newline at end of file diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index c5a73d9667..f6737c5df0 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -40,6 +40,7 @@ const ( PostReposForksByOwnerByRepo = "POST /repos/{owner}/{repo}/forks" GetReposSubscriptionByOwnerByRepo = "GET /repos/{owner}/{repo}/subscription" PutReposSubscriptionByOwnerByRepo = "PUT /repos/{owner}/{repo}/subscription" + DeleteReposByOwnerByRepo = "DELETE /repos/{owner}/{repo}" DeleteReposSubscriptionByOwnerByRepo = "DELETE /repos/{owner}/{repo}/subscription" ListCollaborators = "GET /repos/{owner}/{repo}/collaborators" diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 560e8c1bac..1c4b23c75c 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -704,6 +704,119 @@ func CreateRepository(t translations.TranslationHelperFunc) inventory.ServerTool ) } +const ( + deleteRepositoryConfirmationID = "delete_repository_confirmation" + deleteRepositoryConfirmationField = "repository_name" +) + +// DeleteRepository creates a tool that deletes a GitHub repository after the +// user confirms its full name through elicitation. +func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool { + tool := NewTool( + ToolsetMetadataRepos, + mcp.Tool{ + Name: "delete_repository", + Description: t("TOOL_DELETE_REPOSITORY_DESCRIPTION", "Delete a GitHub repository after the user confirms the exact owner/repository name"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_DELETE_REPOSITORY_USER_TITLE", "Delete repository"), + ReadOnlyHint: false, + DestructiveHint: github.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner (username or organization)", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + }, + Required: []string{"owner", "repo"}, + }, + }, + []scopes.Scope{scopes.DeleteRepo}, + func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + fullName := owner + "/" + repo + var responses mcp.InputResponseMap + if req != nil && req.Params != nil { + responses = req.Params.InputResponses + } + response, ok := responses[deleteRepositoryConfirmationID] + if !ok { + return &mcp.CallToolResult{ + InputRequests: mcp.InputRequestMap{ + deleteRepositoryConfirmationID: &mcp.ElicitParams{ + Mode: "form", + Message: fmt.Sprintf("Type %q to confirm permanent deletion of this repository.", fullName), + RequestedSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + deleteRepositoryConfirmationField: { + Type: "string", + Title: "Repository name", + Description: fmt.Sprintf("Enter %s exactly to confirm deletion", fullName), + }, + }, + Required: []string{deleteRepositoryConfirmationField}, + }, + }, + }, + }, nil, nil + } + + confirmation, ok := response.(*mcp.ElicitResult) + if !ok { + return utils.NewToolResultError("Repository deletion confirmation was invalid. The repository was not deleted."), nil, nil + } + if confirmation.Action != "accept" { + return utils.NewToolResultError("Repository deletion was not confirmed. The repository was not deleted."), nil, nil + } + confirmedName, ok := confirmation.Content[deleteRepositoryConfirmationField].(string) + if !ok || confirmedName != fullName { + return utils.NewToolResultError(fmt.Sprintf("Repository name confirmation did not match %q. The repository was not deleted.", fullName)), nil, nil + } + + client, err := deps.GetClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + resp, err := client.Repositories.Delete(ctx, owner, repo) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + fmt.Sprintf("failed to delete repository: %s", fullName), + resp, + err, + ), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusNoContent { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to delete repository", resp, body), nil, nil + } + + return utils.NewToolResultText(fmt.Sprintf("Repository %s was deleted.", fullName)), nil, nil + }, + ) + tool.MinimumProtocolVersion = inventory.ProtocolVersionMultiRoundTrip + return tool +} + // FetchRepoIsPrivate returns whether a repository is private. It is a thin // wrapper around the GitHub Repositories.Get endpoint provided as a shared // helper for IFC label computation across tools. diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 332b212a17..432a0ea754 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -12,7 +12,9 @@ import ( "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/raw" + "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" "github.com/github/github-mcp-server/pkg/utils" "github.com/google/go-github/v89/github" @@ -2984,6 +2986,171 @@ func Test_PushFiles(t *testing.T) { } } +func Test_DeleteRepository(t *testing.T) { + serverTool := DeleteRepository(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.Equal(t, "delete_repository", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.ElementsMatch(t, []string{"owner", "repo"}, schema.Required) + require.NotNil(t, tool.Annotations) + require.NotNil(t, tool.Annotations.DestructiveHint) + assert.True(t, *tool.Annotations.DestructiveHint) + assert.Equal(t, inventory.ProtocolVersionMultiRoundTrip, serverTool.MinimumProtocolVersion) + assert.Equal(t, []string{string(scopes.DeleteRepo)}, serverTool.RequiredScopes) + + t.Run("requests exact repository name through elicitation", func(t *testing.T) { + result := invokeDeleteRepository(t, serverTool, NewMockedHTTPClient(), nil) + + require.False(t, result.IsError) + require.Len(t, result.InputRequests, 1) + inputRequest, ok := result.InputRequests[deleteRepositoryConfirmationID].(*mcp.ElicitParams) + require.True(t, ok) + assert.Equal(t, "form", inputRequest.Mode) + assert.Contains(t, inputRequest.Message, `"owner/repo"`) + + requestedSchema, ok := inputRequest.RequestedSchema.(*jsonschema.Schema) + require.True(t, ok) + assert.ElementsMatch(t, []string{deleteRepositoryConfirmationField}, requestedSchema.Required) + assert.Contains(t, requestedSchema.Properties, deleteRepositoryConfirmationField) + }) + + t.Run("deletes after exact confirmation", func(t *testing.T) { + client := NewMockedHTTPClient( + WithRequestMatchHandler( + DeleteReposByOwnerByRepo, + mockResponse(t, http.StatusNoContent, nil), + ), + ) + result := invokeDeleteRepository(t, serverTool, client, &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }) + + require.False(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, "owner/repo was deleted") + }) + + t.Run("completes multi-round-trip elicitation before deleting", func(t *testing.T) { + httpClient := NewMockedHTTPClient( + WithRequestMatchHandler( + DeleteReposByOwnerByRepo, + mockResponse(t, http.StatusNoContent, nil), + ), + ) + deps := BaseDeps{Client: mustNewGHClient(t, httpClient)} + + inv, err := inventory.NewBuilder(). + SetTools([]inventory.ServerTool{serverTool}). + WithToolsets([]string{"all"}). + Build() + require.NoError(t, err) + + server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) + server.AddReceivingMiddleware(func(next mcp.MethodHandler) mcp.MethodHandler { + return func(ctx context.Context, method string, request mcp.Request) (mcp.Result, error) { + return next(ContextWithDeps(ctx, deps), method, request) + } + }) + inv.RegisterTools(context.Background(), server, deps) + + serverTransport, clientTransport := mcp.NewInMemoryTransports() + serverSession, err := server.Connect(context.Background(), serverTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = serverSession.Close() }) + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, &mcp.ClientOptions{ + ElicitationHandler: func(_ context.Context, _ *mcp.ElicitRequest) (*mcp.ElicitResult, error) { + return &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }, nil + }, + }) + clientSession, err := client.Connect(context.Background(), clientTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = clientSession.Close() }) + + result, err := clientSession.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "delete_repository", + Arguments: map[string]any{ + "owner": "owner", + "repo": "repo", + }, + }) + require.NoError(t, err) + require.False(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, "owner/repo was deleted") + }) + + t.Run("refuses mismatched confirmation", func(t *testing.T) { + result := invokeDeleteRepository(t, serverTool, NewMockedHTTPClient(), &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/another-repo", + }, + }) + + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "did not match") + }) + + t.Run("refuses declined confirmation", func(t *testing.T) { + result := invokeDeleteRepository(t, serverTool, NewMockedHTTPClient(), &mcp.ElicitResult{ + Action: "decline", + }) + + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "was not confirmed") + }) + + t.Run("returns GitHub API errors", func(t *testing.T) { + client := NewMockedHTTPClient( + WithRequestMatchHandler( + DeleteReposByOwnerByRepo, + mockResponse(t, http.StatusForbidden, map[string]any{"message": "Requires admin permissions"}), + ), + ) + result := invokeDeleteRepository(t, serverTool, client, &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }) + + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "failed to delete repository") + }) +} + +func invokeDeleteRepository(t *testing.T, tool inventory.ServerTool, httpClient *http.Client, confirmation *mcp.ElicitResult) *mcp.CallToolResult { + t.Helper() + + deps := BaseDeps{Client: mustNewGHClient(t, httpClient)} + handler := tool.Handler(deps) + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + }) + if confirmation != nil { + request.Params.InputResponses = mcp.InputResponseMap{ + deleteRepositoryConfirmationID: confirmation, + } + } + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.NotNil(t, result) + return result +} + func Test_ListBranches(t *testing.T) { // Verify tool definition once serverTool := ListBranches(translations.NullTranslationHelper) diff --git a/pkg/github/tools.go b/pkg/github/tools.go index af571f8426..f9b51159b5 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -232,6 +232,7 @@ func AllTools(t translations.TranslationHelperFunc, opts ...ToolOption) []invent GetReleaseByTag(t), CreateOrUpdateFile(t), CreateRepository(t), + DeleteRepository(t), ForkRepository(t), CreateBranch(t), PushFiles(t), diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index b4d509c3e5..4e799c1b3f 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -2,6 +2,7 @@ package http import ( "context" + "encoding/json" "log/slog" "net/http" "net/http/httptest" @@ -46,6 +47,7 @@ type allScopesFetcher struct{} func (f allScopesFetcher) FetchTokenScopes(_ context.Context, _ string) ([]string, error) { return []string{ string(scopes.Repo), + string(scopes.DeleteRepo), string(scopes.WriteOrg), string(scopes.User), string(scopes.Gist), @@ -906,6 +908,95 @@ func TestCrossOriginProtection(t *testing.T) { } } +func TestHTTPToolMinimumProtocolVersion(t *testing.T) { + apiHost, err := utils.NewAPIHost("https://api.github.com") + require.NoError(t, err) + + inventoryFactory := func(_ *http.Request) (*inventory.Inventory, error) { + return inventory.NewBuilder(). + SetTools([]inventory.ServerTool{github.DeleteRepository(translations.NullTranslationHelper)}). + WithToolsets([]string{"all"}). + Build() + } + handler := NewHTTPMcpHandler( + context.Background(), + &ServerConfig{Version: "test"}, + github.BaseDeps{}, + translations.NullTranslationHelper, + slog.Default(), + apiHost, + WithInventoryFactory(inventoryFactory), + WithScopeFetcher(allScopesFetcher{}), + ) + + router := chi.NewRouter() + handler.RegisterMiddleware(router) + handler.RegisterRoutes(router) + + for _, tt := range []struct { + name string + protocolVersion string + wantDeleteRepoTool bool + }{ + { + name: "current protocol includes delete repository", + protocolVersion: inventory.ProtocolVersionMultiRoundTrip, + wantDeleteRepoTool: true, + }, + { + name: "legacy protocol hides delete repository", + protocolVersion: "2025-11-25", + wantDeleteRepoTool: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + body := strings.Replace( + `{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{"_meta":{"io.modelcontextprotocol/protocolVersion":"PROTOCOL_VERSION","io.modelcontextprotocol/clientCapabilities":{"elicitation":{"form":{}}},"io.modelcontextprotocol/clientInfo":{"name":"test","version":"v0.0.1"}}}}`, + "PROTOCOL_VERSION", + tt.protocolVersion, + 1, + ) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + req.Header.Set(headers.ContentTypeHeader, headers.ContentTypeJSON) + req.Header.Set(headers.AuthorizationHeader, "Bearer test-token") + req.Header.Set(headers.AcceptHeader, strings.Join([]string{headers.ContentTypeJSON, headers.ContentTypeEventStream}, ", ")) + req.Header.Set("Mcp-Protocol-Version", tt.protocolVersion) + req.Header.Set("Mcp-Method", "tools/list") + req.Header.Set(headers.AuthorizationHeader, strings.Join([]string{"ghs", "test-token"}, "_")) + + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, req) + require.Equal(t, http.StatusOK, recorder.Code, "response body: %s", recorder.Body.String()) + + var response struct { + Result struct { + Tools []struct { + Name string `json:"name"` + } `json:"tools"` + } `json:"result"` + } + responseBody := recorder.Body.String() + for line := range strings.SplitSeq(responseBody, "\n") { + if data, ok := strings.CutPrefix(line, "data: "); ok { + responseBody = data + break + } + } + require.NoError(t, json.Unmarshal([]byte(responseBody), &response)) + + toolNames := make([]string, 0, len(response.Result.Tools)) + for _, tool := range response.Result.Tools { + toolNames = append(toolNames, tool.Name) + } + if tt.wantDeleteRepoTool { + assert.Contains(t, toolNames, "delete_repository") + } else { + assert.NotContains(t, toolNames, "delete_repository") + } + }) + } +} + // TestInsidersRoutePreservesUIMeta is a regression test for the bug where // _meta.ui was stripped from tools/list responses on the HTTP /insiders route. // diff --git a/pkg/inventory/protocol_version.go b/pkg/inventory/protocol_version.go new file mode 100644 index 0000000000..1cb8ce0065 --- /dev/null +++ b/pkg/inventory/protocol_version.go @@ -0,0 +1,74 @@ +package inventory + +import ( + "context" + "fmt" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// ProtocolVersionMultiRoundTrip is the first MCP protocol version that supports +// multi-round-trip input requests. +const ProtocolVersionMultiRoundTrip = "2026-07-28" + +func addToolProtocolVersionMiddleware(server *mcp.Server, tools []ServerTool) { + minimumVersions := make(map[string]string) + for _, tool := range tools { + minimum := tool.MinimumProtocolVersion + if minimum == "" || minimum <= minimumVersions[tool.Tool.Name] { + continue + } + minimumVersions[tool.Tool.Name] = minimum + } + if len(minimumVersions) == 0 { + return + } + + server.AddReceivingMiddleware(toolProtocolVersionMiddleware(minimumVersions)) +} + +func toolProtocolVersionMiddleware(minimumVersions map[string]string) mcp.Middleware { + return func(next mcp.MethodHandler) mcp.MethodHandler { + return func(ctx context.Context, method string, request mcp.Request) (mcp.Result, error) { + switch req := request.(type) { + case *mcp.CallToolRequest: + if req.Params != nil { + if minimum := minimumVersions[req.Params.Name]; !protocolVersionAllowed(req.ProtocolVersion(), minimum) { + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf( + "Tool %q requires MCP protocol version %s or later.", + req.Params.Name, + minimum, + )}}, + IsError: true, + }, nil + } + } + case *mcp.ListToolsRequest: + result, err := next(ctx, method, request) + if err != nil { + return nil, err + } + list, ok := result.(*mcp.ListToolsResult) + if !ok { + return result, nil + } + + tools := make([]*mcp.Tool, 0, len(list.Tools)) + for _, tool := range list.Tools { + if protocolVersionAllowed(req.ProtocolVersion(), minimumVersions[tool.Name]) { + tools = append(tools, tool) + } + } + list.Tools = tools + return list, nil + } + + return next(ctx, method, request) + } + } +} + +func protocolVersionAllowed(protocolVersion, minimum string) bool { + return minimum == "" || protocolVersion >= minimum +} diff --git a/pkg/inventory/protocol_version_test.go b/pkg/inventory/protocol_version_test.go new file mode 100644 index 0000000000..4b3965ca2f --- /dev/null +++ b/pkg/inventory/protocol_version_test.go @@ -0,0 +1,119 @@ +package inventory + +import ( + "context" + "errors" + "testing" + + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestToolMinimumProtocolVersion(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + protocolVersion string + wantVersionedTool bool + }{ + { + name: "current protocol lists and calls versioned tool", + protocolVersion: ProtocolVersionMultiRoundTrip, + wantVersionedTool: true, + }, + { + name: "legacy protocol hides and refuses versioned tool", + protocolVersion: "2025-11-25", + wantVersionedTool: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var versionedToolCalls int + tools := []ServerTool{ + protocolTestTool("always_available", "", nil), + protocolTestTool("versioned", ProtocolVersionMultiRoundTrip, func() { + versionedToolCalls++ + }), + } + inv, err := NewBuilder(). + SetTools(tools). + WithToolsets([]string{"all"}). + Build() + require.NoError(t, err) + + server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) + inv.RegisterTools(context.Background(), server, nil) + if tt.protocolVersion < ProtocolVersionMultiRoundTrip { + server.AddReceivingMiddleware(func(next mcp.MethodHandler) mcp.MethodHandler { + return func(ctx context.Context, method string, request mcp.Request) (mcp.Result, error) { + if method == "server/discover" { + return nil, errors.New("legacy server does not support discovery") + } + return next(ctx, method, request) + } + }) + } + + serverTransport, clientTransport := mcp.NewInMemoryTransports() + serverSession, err := server.Connect(context.Background(), serverTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = serverSession.Close() }) + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, nil) + clientSession, err := client.Connect(context.Background(), clientTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = clientSession.Close() }) + + listResult, err := clientSession.ListTools(context.Background(), nil) + require.NoError(t, err) + toolNames := make([]string, 0, len(listResult.Tools)) + for _, tool := range listResult.Tools { + toolNames = append(toolNames, tool.Name) + } + assert.Contains(t, toolNames, "always_available") + if tt.wantVersionedTool { + assert.Contains(t, toolNames, "versioned") + } else { + assert.NotContains(t, toolNames, "versioned") + } + + callResult, err := clientSession.CallTool(context.Background(), &mcp.CallToolParams{Name: "versioned"}) + require.NoError(t, err) + if tt.wantVersionedTool { + assert.False(t, callResult.IsError) + assert.Equal(t, 1, versionedToolCalls) + } else { + assert.True(t, callResult.IsError) + assert.Zero(t, versionedToolCalls) + } + }) + } +} + +func protocolTestTool(name, minimumProtocolVersion string, onCall func()) ServerTool { + return ServerTool{ + Tool: mcp.Tool{ + Name: name, + InputSchema: &jsonschema.Schema{Type: "object"}, + }, + Toolset: ToolsetMetadata{ID: "test"}, + HandlerFunc: func(any) mcp.ToolHandler { + return func(context.Context, *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + if onCall != nil { + onCall() + } + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "called"}}, + }, nil + } + }, + MinimumProtocolVersion: minimumProtocolVersion, + } +} diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 915ed0aa1c..86ae978df9 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -220,7 +220,9 @@ func shouldStripMCPAppsMetadata(ctx context.Context, featureFlagEnabled bool) bo // falsely report the flag off, even when the actual request arrived on the // /insiders route. func (r *Inventory) RegisterTools(ctx context.Context, s *mcp.Server, deps any, middleware ...ToolHandlerMiddleware) { - for _, tool := range r.ToolsForRegistration(ctx) { + tools := r.ToolsForRegistration(ctx) + addToolProtocolVersionMiddleware(s, tools) + for _, tool := range tools { tool.RegisterFunc(s, deps, middleware...) } } diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 44a062ba2e..71f6db08a3 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -81,6 +81,10 @@ type ServerTool struct { // Returns (enabled, error). On error, the tool should be treated as disabled. Enabled func(ctx context.Context) (bool, error) + // MinimumProtocolVersion is the oldest MCP protocol version that may list or + // call this tool. Empty means the tool is available on every version. + MinimumProtocolVersion string + // RequiredScopes specifies the minimum OAuth scopes required for this tool. // These are the scopes that must be present for the tool to function. RequiredScopes []string diff --git a/pkg/scopes/scopes.go b/pkg/scopes/scopes.go index cb1b7681a7..084ecaf1c6 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -20,6 +20,9 @@ const ( // PublicRepo grants access to public repositories PublicRepo Scope = "public_repo" + // DeleteRepo grants permission to delete repositories + DeleteRepo Scope = "delete_repo" + // ReadOrg grants read-only access to organization membership, teams, and projects ReadOrg Scope = "read:org" diff --git a/pkg/scopes/scopes_test.go b/pkg/scopes/scopes_test.go index b8e0d8e421..94e21a9cd7 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -33,6 +33,11 @@ func TestExpandScopes(t *testing.T) { required: []Scope{PublicRepo}, expected: []string{"public_repo", "repo"}, }, + { + name: "delete_repo returns just delete_repo", + required: []Scope{DeleteRepo}, + expected: []string{"delete_repo"}, + }, { name: "security_events also accepts repo (parent)", required: []Scope{SecurityEvents}, From c34055ea780659ff27884c996701aa507d49282d Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Mon, 17 Aug 2026 11:23:51 +0200 Subject: [PATCH 02/11] refactor(inventory): generalize tool availability guards Gate protocol-restricted tools on required elicitation capabilities and enforce direct calls inside the registered handler so SDK result finalization remains intact. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- pkg/github/repositories.go | 1 + pkg/github/repositories_test.go | 1 + pkg/http/handler_test.go | 56 +++++--- pkg/inventory/protocol_version.go | 74 ----------- pkg/inventory/protocol_version_test.go | 119 ----------------- pkg/inventory/registry.go | 2 +- pkg/inventory/server_tool.go | 5 + pkg/inventory/tool_availability.go | 149 ++++++++++++++++++++++ pkg/inventory/tool_availability_test.go | 163 ++++++++++++++++++++++++ 9 files changed, 359 insertions(+), 211 deletions(-) delete mode 100644 pkg/inventory/protocol_version.go delete mode 100644 pkg/inventory/protocol_version_test.go create mode 100644 pkg/inventory/tool_availability.go create mode 100644 pkg/inventory/tool_availability_test.go diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 1c4b23c75c..1e0f79d7c2 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -814,6 +814,7 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool }, ) tool.MinimumProtocolVersion = inventory.ProtocolVersionMultiRoundTrip + tool.RequiredElicitationMode = inventory.ElicitationModeForm return tool } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 432a0ea754..0dbe521081 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -3000,6 +3000,7 @@ func Test_DeleteRepository(t *testing.T) { require.NotNil(t, tool.Annotations.DestructiveHint) assert.True(t, *tool.Annotations.DestructiveHint) assert.Equal(t, inventory.ProtocolVersionMultiRoundTrip, serverTool.MinimumProtocolVersion) + assert.Equal(t, inventory.ElicitationModeForm, serverTool.RequiredElicitationMode) assert.Equal(t, []string{string(scopes.DeleteRepo)}, serverTool.RequiredScopes) t.Run("requests exact repository name through elicitation", func(t *testing.T) { diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 4e799c1b3f..c3c7c4b752 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -934,31 +934,53 @@ func TestHTTPToolMinimumProtocolVersion(t *testing.T) { handler.RegisterRoutes(router) for _, tt := range []struct { - name string - protocolVersion string - wantDeleteRepoTool bool + name string + protocolVersion string + elicitationCapabilities map[string]any + wantDeleteRepoTool bool }{ { - name: "current protocol includes delete repository", - protocolVersion: inventory.ProtocolVersionMultiRoundTrip, - wantDeleteRepoTool: true, + name: "current protocol with form elicitation includes delete repository", + protocolVersion: inventory.ProtocolVersionMultiRoundTrip, + elicitationCapabilities: map[string]any{"form": map[string]any{}}, + wantDeleteRepoTool: true, }, { - name: "legacy protocol hides delete repository", - protocolVersion: "2025-11-25", - wantDeleteRepoTool: false, + name: "current protocol with URL-only elicitation hides delete repository", + protocolVersion: inventory.ProtocolVersionMultiRoundTrip, + elicitationCapabilities: map[string]any{"url": map[string]any{}}, + }, + { + name: "current protocol without elicitation hides delete repository", + protocolVersion: inventory.ProtocolVersionMultiRoundTrip, + }, + { + name: "legacy protocol hides delete repository", + protocolVersion: "2025-11-25", + elicitationCapabilities: map[string]any{"form": map[string]any{}}, }, } { t.Run(tt.name, func(t *testing.T) { - body := strings.Replace( - `{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{"_meta":{"io.modelcontextprotocol/protocolVersion":"PROTOCOL_VERSION","io.modelcontextprotocol/clientCapabilities":{"elicitation":{"form":{}}},"io.modelcontextprotocol/clientInfo":{"name":"test","version":"v0.0.1"}}}}`, - "PROTOCOL_VERSION", - tt.protocolVersion, - 1, - ) - req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + clientCapabilities := map[string]any{} + if tt.elicitationCapabilities != nil { + clientCapabilities["elicitation"] = tt.elicitationCapabilities + } + body, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/list", + "params": map[string]any{ + "_meta": map[string]any{ + mcp.MetaKeyProtocolVersion: tt.protocolVersion, + mcp.MetaKeyClientCapabilities: clientCapabilities, + mcp.MetaKeyClientInfo: map[string]any{"name": "test", "version": "v0.0.1"}, + }, + }, + }) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(string(body))) req.Header.Set(headers.ContentTypeHeader, headers.ContentTypeJSON) - req.Header.Set(headers.AuthorizationHeader, "Bearer test-token") req.Header.Set(headers.AcceptHeader, strings.Join([]string{headers.ContentTypeJSON, headers.ContentTypeEventStream}, ", ")) req.Header.Set("Mcp-Protocol-Version", tt.protocolVersion) req.Header.Set("Mcp-Method", "tools/list") diff --git a/pkg/inventory/protocol_version.go b/pkg/inventory/protocol_version.go deleted file mode 100644 index 1cb8ce0065..0000000000 --- a/pkg/inventory/protocol_version.go +++ /dev/null @@ -1,74 +0,0 @@ -package inventory - -import ( - "context" - "fmt" - - "github.com/modelcontextprotocol/go-sdk/mcp" -) - -// ProtocolVersionMultiRoundTrip is the first MCP protocol version that supports -// multi-round-trip input requests. -const ProtocolVersionMultiRoundTrip = "2026-07-28" - -func addToolProtocolVersionMiddleware(server *mcp.Server, tools []ServerTool) { - minimumVersions := make(map[string]string) - for _, tool := range tools { - minimum := tool.MinimumProtocolVersion - if minimum == "" || minimum <= minimumVersions[tool.Tool.Name] { - continue - } - minimumVersions[tool.Tool.Name] = minimum - } - if len(minimumVersions) == 0 { - return - } - - server.AddReceivingMiddleware(toolProtocolVersionMiddleware(minimumVersions)) -} - -func toolProtocolVersionMiddleware(minimumVersions map[string]string) mcp.Middleware { - return func(next mcp.MethodHandler) mcp.MethodHandler { - return func(ctx context.Context, method string, request mcp.Request) (mcp.Result, error) { - switch req := request.(type) { - case *mcp.CallToolRequest: - if req.Params != nil { - if minimum := minimumVersions[req.Params.Name]; !protocolVersionAllowed(req.ProtocolVersion(), minimum) { - return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf( - "Tool %q requires MCP protocol version %s or later.", - req.Params.Name, - minimum, - )}}, - IsError: true, - }, nil - } - } - case *mcp.ListToolsRequest: - result, err := next(ctx, method, request) - if err != nil { - return nil, err - } - list, ok := result.(*mcp.ListToolsResult) - if !ok { - return result, nil - } - - tools := make([]*mcp.Tool, 0, len(list.Tools)) - for _, tool := range list.Tools { - if protocolVersionAllowed(req.ProtocolVersion(), minimumVersions[tool.Name]) { - tools = append(tools, tool) - } - } - list.Tools = tools - return list, nil - } - - return next(ctx, method, request) - } - } -} - -func protocolVersionAllowed(protocolVersion, minimum string) bool { - return minimum == "" || protocolVersion >= minimum -} diff --git a/pkg/inventory/protocol_version_test.go b/pkg/inventory/protocol_version_test.go deleted file mode 100644 index 4b3965ca2f..0000000000 --- a/pkg/inventory/protocol_version_test.go +++ /dev/null @@ -1,119 +0,0 @@ -package inventory - -import ( - "context" - "errors" - "testing" - - "github.com/google/jsonschema-go/jsonschema" - "github.com/modelcontextprotocol/go-sdk/mcp" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestToolMinimumProtocolVersion(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - protocolVersion string - wantVersionedTool bool - }{ - { - name: "current protocol lists and calls versioned tool", - protocolVersion: ProtocolVersionMultiRoundTrip, - wantVersionedTool: true, - }, - { - name: "legacy protocol hides and refuses versioned tool", - protocolVersion: "2025-11-25", - wantVersionedTool: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - var versionedToolCalls int - tools := []ServerTool{ - protocolTestTool("always_available", "", nil), - protocolTestTool("versioned", ProtocolVersionMultiRoundTrip, func() { - versionedToolCalls++ - }), - } - inv, err := NewBuilder(). - SetTools(tools). - WithToolsets([]string{"all"}). - Build() - require.NoError(t, err) - - server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) - inv.RegisterTools(context.Background(), server, nil) - if tt.protocolVersion < ProtocolVersionMultiRoundTrip { - server.AddReceivingMiddleware(func(next mcp.MethodHandler) mcp.MethodHandler { - return func(ctx context.Context, method string, request mcp.Request) (mcp.Result, error) { - if method == "server/discover" { - return nil, errors.New("legacy server does not support discovery") - } - return next(ctx, method, request) - } - }) - } - - serverTransport, clientTransport := mcp.NewInMemoryTransports() - serverSession, err := server.Connect(context.Background(), serverTransport, nil) - require.NoError(t, err) - t.Cleanup(func() { _ = serverSession.Close() }) - - client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, nil) - clientSession, err := client.Connect(context.Background(), clientTransport, nil) - require.NoError(t, err) - t.Cleanup(func() { _ = clientSession.Close() }) - - listResult, err := clientSession.ListTools(context.Background(), nil) - require.NoError(t, err) - toolNames := make([]string, 0, len(listResult.Tools)) - for _, tool := range listResult.Tools { - toolNames = append(toolNames, tool.Name) - } - assert.Contains(t, toolNames, "always_available") - if tt.wantVersionedTool { - assert.Contains(t, toolNames, "versioned") - } else { - assert.NotContains(t, toolNames, "versioned") - } - - callResult, err := clientSession.CallTool(context.Background(), &mcp.CallToolParams{Name: "versioned"}) - require.NoError(t, err) - if tt.wantVersionedTool { - assert.False(t, callResult.IsError) - assert.Equal(t, 1, versionedToolCalls) - } else { - assert.True(t, callResult.IsError) - assert.Zero(t, versionedToolCalls) - } - }) - } -} - -func protocolTestTool(name, minimumProtocolVersion string, onCall func()) ServerTool { - return ServerTool{ - Tool: mcp.Tool{ - Name: name, - InputSchema: &jsonschema.Schema{Type: "object"}, - }, - Toolset: ToolsetMetadata{ID: "test"}, - HandlerFunc: func(any) mcp.ToolHandler { - return func(context.Context, *mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if onCall != nil { - onCall() - } - return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: "called"}}, - }, nil - } - }, - MinimumProtocolVersion: minimumProtocolVersion, - } -} diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 86ae978df9..3483d448cb 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -221,7 +221,7 @@ func shouldStripMCPAppsMetadata(ctx context.Context, featureFlagEnabled bool) bo // /insiders route. func (r *Inventory) RegisterTools(ctx context.Context, s *mcp.Server, deps any, middleware ...ToolHandlerMiddleware) { tools := r.ToolsForRegistration(ctx) - addToolProtocolVersionMiddleware(s, tools) + addToolAvailabilityMiddleware(s, tools) for _, tool := range tools { tool.RegisterFunc(s, deps, middleware...) } diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 71f6db08a3..39f64c716f 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -85,6 +85,10 @@ type ServerTool struct { // call this tool. Empty means the tool is available on every version. MinimumProtocolVersion string + // RequiredElicitationMode is the elicitation mode the client must support to + // list or call this tool. Empty means the tool does not require elicitation. + RequiredElicitationMode ElicitationMode + // RequiredScopes specifies the minimum OAuth scopes required for this tool. // These are the scopes that must be present for the tool to function. RequiredScopes []string @@ -123,6 +127,7 @@ func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any, middleware ...ToolHa for i := len(middleware) - 1; i >= 0; i-- { handler = middleware[i](handler) } + handler = st.wrapAvailabilityCheck(handler) // Make a shallow copy of the tool to avoid mutating the original toolCopy := st.Tool // Apply icons from toolset metadata if tool doesn't have icons set diff --git a/pkg/inventory/tool_availability.go b/pkg/inventory/tool_availability.go new file mode 100644 index 0000000000..937726a8c7 --- /dev/null +++ b/pkg/inventory/tool_availability.go @@ -0,0 +1,149 @@ +package inventory + +import ( + "context" + "fmt" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// ProtocolVersionMultiRoundTrip is the first MCP protocol version that supports +// multi-round-trip input requests. +const ProtocolVersionMultiRoundTrip = "2026-07-28" + +// ElicitationMode identifies a client-supported elicitation interaction mode. +type ElicitationMode string + +const ( + // ElicitationModeForm collects structured user input through the client. + ElicitationModeForm ElicitationMode = "form" + // ElicitationModeURL directs the user to an external URL. + ElicitationModeURL ElicitationMode = "url" +) + +type toolAvailability struct { + minimumProtocolVersion string + requiredElicitationMode ElicitationMode +} + +func (st *ServerTool) availability() toolAvailability { + return toolAvailability{ + minimumProtocolVersion: st.MinimumProtocolVersion, + requiredElicitationMode: st.RequiredElicitationMode, + } +} + +func (a toolAvailability) unrestricted() bool { + return a.minimumProtocolVersion == "" && a.requiredElicitationMode == "" +} + +func addToolAvailabilityMiddleware(server *mcp.Server, tools []ServerTool) { + availabilityByName := make(map[string]toolAvailability) + for _, tool := range tools { + availability := tool.availability() + if availability.unrestricted() { + delete(availabilityByName, tool.Tool.Name) + } else { + // AddTool replaces an existing tool with the same name, so preserve + // the availability metadata from the last registered definition too. + availabilityByName[tool.Tool.Name] = availability + } + } + if len(availabilityByName) == 0 { + return + } + + server.AddReceivingMiddleware(toolAvailabilityMiddleware(availabilityByName)) +} + +func toolAvailabilityMiddleware(availabilityByName map[string]toolAvailability) mcp.Middleware { + return func(next mcp.MethodHandler) mcp.MethodHandler { + return func(ctx context.Context, method string, request mcp.Request) (mcp.Result, error) { + req, ok := request.(*mcp.ListToolsRequest) + if !ok { + return next(ctx, method, request) + } + result, err := next(ctx, method, request) + if err != nil { + return nil, err + } + list, ok := result.(*mcp.ListToolsResult) + if !ok { + return result, nil + } + + tools := make([]*mcp.Tool, 0, len(list.Tools)) + for _, tool := range list.Tools { + if toolAvailable(req.ProtocolVersion(), req.ClientCapabilities(), availabilityByName[tool.Name]) { + tools = append(tools, tool) + } + } + list.Tools = tools + return list, nil + } + } +} + +func (st *ServerTool) wrapAvailabilityCheck(next mcp.ToolHandler) mcp.ToolHandler { + availability := st.availability() + if availability.unrestricted() { + return next + } + return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + if toolAvailable(req.ProtocolVersion(), req.ClientCapabilities(), availability) { + return next(ctx, req) + } + return toolUnavailableResult(st.Tool.Name, req, availability), nil + } +} + +func toolAvailable(protocolVersion string, capabilities *mcp.ClientCapabilities, availability toolAvailability) bool { + return protocolVersionAllowed(protocolVersion, availability.minimumProtocolVersion) && + elicitationModeSupported(capabilities, availability.requiredElicitationMode) +} + +func protocolVersionAllowed(protocolVersion, minimum string) bool { + // MCP protocol versions use ISO dates, so lexical ordering is chronological. + return minimum == "" || protocolVersion >= minimum +} + +func elicitationModeSupported(capabilities *mcp.ClientCapabilities, requiredMode ElicitationMode) bool { + if requiredMode == "" { + return true + } + if capabilities == nil || capabilities.Elicitation == nil { + return false + } + elicitation := capabilities.Elicitation + switch requiredMode { + case ElicitationModeForm: + // An empty elicitation capability means form-only for compatibility. + return elicitation.Form != nil || elicitation.URL == nil + case ElicitationModeURL: + return elicitation.URL != nil + default: + return false + } +} + +func toolUnavailableResult(name string, req *mcp.CallToolRequest, availability toolAvailability) *mcp.CallToolResult { + message := fmt.Sprintf("Tool %q is unavailable for this client.", name) + switch { + case !protocolVersionAllowed(req.ProtocolVersion(), availability.minimumProtocolVersion): + message = fmt.Sprintf( + "Tool %q requires MCP protocol version %s or later.", + name, + availability.minimumProtocolVersion, + ) + case !elicitationModeSupported(req.ClientCapabilities(), availability.requiredElicitationMode): + message = fmt.Sprintf( + "Tool %q requires client support for %s elicitation.", + name, + availability.requiredElicitationMode, + ) + } + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: message}}, + IsError: true, + } +} diff --git a/pkg/inventory/tool_availability_test.go b/pkg/inventory/tool_availability_test.go new file mode 100644 index 0000000000..1f76e3cb26 --- /dev/null +++ b/pkg/inventory/tool_availability_test.go @@ -0,0 +1,163 @@ +package inventory + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestToolAvailability(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + protocolVersion string + capabilities *mcp.ClientCapabilities + wantTool bool + wantError string + }{ + { + name: "current protocol and form elicitation list and call tool", + protocolVersion: ProtocolVersionMultiRoundTrip, + capabilities: &mcp.ClientCapabilities{ + Elicitation: &mcp.ElicitationCapabilities{Form: &mcp.FormElicitationCapabilities{}}, + }, + wantTool: true, + }, + { + name: "empty elicitation capability implies form support", + protocolVersion: ProtocolVersionMultiRoundTrip, + capabilities: &mcp.ClientCapabilities{ + Elicitation: &mcp.ElicitationCapabilities{}, + }, + wantTool: true, + }, + { + name: "URL-only elicitation hides and refuses form tool", + protocolVersion: ProtocolVersionMultiRoundTrip, + capabilities: &mcp.ClientCapabilities{ + Elicitation: &mcp.ElicitationCapabilities{URL: &mcp.URLElicitationCapabilities{}}, + }, + wantError: "requires client support for form elicitation", + }, + { + name: "missing elicitation capability hides and refuses form tool", + protocolVersion: ProtocolVersionMultiRoundTrip, + capabilities: &mcp.ClientCapabilities{}, + wantError: "requires client support for form elicitation", + }, + { + name: "legacy protocol hides and refuses versioned tool", + protocolVersion: "2025-11-25", + capabilities: &mcp.ClientCapabilities{ + Elicitation: &mcp.ElicitationCapabilities{Form: &mcp.FormElicitationCapabilities{}}, + }, + wantTool: false, + wantError: "requires MCP protocol version 2026-07-28 or later", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var restrictedToolCalls int + tools := []ServerTool{ + availabilityTestTool("always_available", "", "", nil), + availabilityTestTool("restricted", ProtocolVersionMultiRoundTrip, ElicitationModeForm, func() { + restrictedToolCalls++ + }), + } + inv, err := NewBuilder(). + SetTools(tools). + WithToolsets([]string{"all"}). + Build() + require.NoError(t, err) + + server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) + inv.RegisterTools(context.Background(), server, nil) + if tt.protocolVersion < ProtocolVersionMultiRoundTrip { + server.AddReceivingMiddleware(func(next mcp.MethodHandler) mcp.MethodHandler { + return func(ctx context.Context, method string, request mcp.Request) (mcp.Result, error) { + if method == "server/discover" { + return nil, errors.New("legacy server does not support discovery") + } + return next(ctx, method, request) + } + }) + } + + serverTransport, clientTransport := mcp.NewInMemoryTransports() + serverSession, err := server.Connect(context.Background(), serverTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = serverSession.Close() }) + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, &mcp.ClientOptions{ + Capabilities: tt.capabilities, + }) + clientSession, err := client.Connect(context.Background(), clientTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = clientSession.Close() }) + + listResult, err := clientSession.ListTools(context.Background(), nil) + require.NoError(t, err) + toolNames := make([]string, 0, len(listResult.Tools)) + for _, tool := range listResult.Tools { + toolNames = append(toolNames, tool.Name) + } + assert.Contains(t, toolNames, "always_available") + if tt.wantTool { + assert.Contains(t, toolNames, "restricted") + } else { + assert.NotContains(t, toolNames, "restricted") + } + + callResult, err := clientSession.CallTool(context.Background(), &mcp.CallToolParams{Name: "restricted"}) + require.NoError(t, err) + if tt.wantTool { + assert.False(t, callResult.IsError) + assert.Equal(t, 1, restrictedToolCalls) + } else { + assert.True(t, callResult.IsError) + assert.Zero(t, restrictedToolCalls) + require.Len(t, callResult.Content, 1) + text, ok := callResult.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.Contains(t, text.Text, tt.wantError) + if tt.protocolVersion >= ProtocolVersionMultiRoundTrip { + encoded, err := json.Marshal(callResult) + require.NoError(t, err) + assert.Contains(t, string(encoded), `"resultType":"complete"`) + } + } + }) + } +} + +func availabilityTestTool(name, minimumProtocolVersion string, requiredElicitationMode ElicitationMode, onCall func()) ServerTool { + return ServerTool{ + Tool: mcp.Tool{ + Name: name, + InputSchema: &jsonschema.Schema{Type: "object"}, + }, + Toolset: ToolsetMetadata{ID: "test"}, + HandlerFunc: func(any) mcp.ToolHandler { + return func(context.Context, *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + if onCall != nil { + onCall() + } + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "called"}}, + }, nil + } + }, + MinimumProtocolVersion: minimumProtocolVersion, + RequiredElicitationMode: requiredElicitationMode, + } +} From 14aea5234535afa51303fdac42ae0761f87a3802 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Mon, 17 Aug 2026 12:10:23 +0200 Subject: [PATCH 03/11] feat(http): protect MRTR request state Seal repository deletion targets for self-hosted HTTP with a stable AES-256-GCM key. Hide only delete_repository when no key is configured and expose an optional sealer interface for remote integrators. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- cmd/github-mcp-server/main.go | 1 + docs/streamable-http.md | 21 +++++++ internal/requeststate/sealer.go | 68 +++++++++++++++++++++ internal/requeststate/sealer_test.go | 58 ++++++++++++++++++ pkg/github/dependencies.go | 12 ++++ pkg/github/repositories.go | 38 +++++++++++- pkg/github/repositories_test.go | 91 ++++++++++++++++++++++++++++ pkg/github/request_state.go | 25 ++++++++ pkg/http/handler.go | 16 ++++- pkg/http/handler_test.go | 25 ++++++++ pkg/http/server.go | 28 +++++++++ pkg/http/server_test.go | 37 +++++++++++ 12 files changed, 416 insertions(+), 4 deletions(-) create mode 100644 internal/requeststate/sealer.go create mode 100644 internal/requeststate/sealer_test.go create mode 100644 pkg/github/request_state.go diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 7671706b57..fb61ed036c 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -217,6 +217,7 @@ var ( EnabledFeatures: enabledFeatures, InsidersMode: viper.GetBool("insiders"), TrustProxyHeaders: viper.GetBool("trust-proxy-headers"), + MRTRStateKey: os.Getenv(ghhttp.MRTRStateKeyEnv), } return ghhttp.RunHTTPServer(httpConfig) diff --git a/docs/streamable-http.md b/docs/streamable-http.md index 8f4a2bff84..bdcf149b8f 100644 --- a/docs/streamable-http.md +++ b/docs/streamable-http.md @@ -32,6 +32,27 @@ github-mcp-server http --scope-challenge When `--scope-challenge` is enabled, requests with insufficient scopes receive a `403 Forbidden` response with a `WWW-Authenticate` header indicating the required scopes. +### Repository deletion and request-state encryption + +The `delete_repository` tool uses multi-round-trip elicitation and carries its +confirmed target through client-held request state. To expose this tool in HTTP +mode, configure a stable 32-byte encryption key encoded with standard Base64: + +```bash +export GITHUB_MCP_SERVER_MRTR_STATE_KEY="$(openssl rand -base64 32 | tr -d '\n')" +github-mcp-server http +``` + +Use the same key on every replica that may handle a retry. Keep it secret and +stable during deployments; changing it invalidates confirmations already in +flight. If the variable is absent, `delete_repository` is not exposed by the +HTTP server. If it is present but malformed, the server refuses to start. + +This self-hosted key is independent of keys used by the hosted remote server. +Integrators can provide their own request-state sealer through the exported +`github.RequestStateSealer` interface and expose it from their tool dependencies +through `github.RequestStateSealerProvider` without changing their key format. + ### With OAuth Metadata Discovery For use behind reverse proxies or with custom domains, expose OAuth metadata endpoints: diff --git a/internal/requeststate/sealer.go b/internal/requeststate/sealer.go new file mode 100644 index 0000000000..25ec725b6b --- /dev/null +++ b/internal/requeststate/sealer.go @@ -0,0 +1,68 @@ +package requeststate + +import ( + "context" + "crypto/aes" + "crypto/cipher" + "crypto/rand" + "encoding/base64" + "errors" + "fmt" +) + +const keySize = 32 + +// Sealer protects request state with AES-256-GCM. +type Sealer struct { + aead cipher.AEAD +} + +// New constructs a sealer from a standard Base64-encoded 32-byte key. +func New(encodedKey string) (*Sealer, error) { + key, err := base64.StdEncoding.DecodeString(encodedKey) + if err != nil { + return nil, fmt.Errorf("decoding key: %w", err) + } + if len(key) != keySize { + return nil, fmt.Errorf("decoded key must be %d bytes, got %d", keySize, len(key)) + } + block, err := aes.NewCipher(key) + if err != nil { + return nil, fmt.Errorf("creating cipher: %w", err) + } + aead, err := cipher.NewGCM(block) + if err != nil { + return nil, fmt.Errorf("creating GCM: %w", err) + } + return &Sealer{aead: aead}, nil +} + +// Seal encrypts and authenticates plaintext into a URL-safe opaque token. +func (s *Sealer) Seal(_ context.Context, plaintext []byte) (string, error) { + nonce := make([]byte, s.aead.NonceSize()) + if _, err := rand.Read(nonce); err != nil { + return "", fmt.Errorf("generating nonce: %w", err) + } + sealed := s.aead.Seal(nonce, nonce, plaintext, nil) + return base64.RawURLEncoding.EncodeToString(sealed), nil +} + +// Open verifies and decrypts a token produced by Seal. +func (s *Sealer) Open(token string) ([]byte, error) { + if token == "" { + return nil, errors.New("empty token") + } + sealed, err := base64.RawURLEncoding.DecodeString(token) + if err != nil { + return nil, fmt.Errorf("decoding token: %w", err) + } + nonceSize := s.aead.NonceSize() + if len(sealed) < nonceSize { + return nil, errors.New("token is too short") + } + plaintext, err := s.aead.Open(nil, sealed[:nonceSize], sealed[nonceSize:], nil) + if err != nil { + return nil, fmt.Errorf("opening token: %w", err) + } + return plaintext, nil +} diff --git a/internal/requeststate/sealer_test.go b/internal/requeststate/sealer_test.go new file mode 100644 index 0000000000..b258beeb61 --- /dev/null +++ b/internal/requeststate/sealer_test.go @@ -0,0 +1,58 @@ +package requeststate + +import ( + "context" + "encoding/base64" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSealer(t *testing.T) { + key := base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef")) + sealer, err := New(key) + require.NoError(t, err) + + t.Run("round trip", func(t *testing.T) { + plaintext := []byte(`{"owner":"octo","repo":"repo"}`) + token, err := sealer.Seal(context.Background(), plaintext) + require.NoError(t, err) + assert.NotContains(t, token, string(plaintext)) + + opened, err := sealer.Open(token) + require.NoError(t, err) + assert.Equal(t, plaintext, opened) + }) + + t.Run("rejects tampering", func(t *testing.T) { + token, err := sealer.Seal(context.Background(), []byte("state")) + require.NoError(t, err) + replacement := "A" + if strings.HasSuffix(token, replacement) { + replacement = "B" + } + + _, err = sealer.Open(token[:len(token)-1] + replacement) + require.Error(t, err) + }) +} + +func TestNew(t *testing.T) { + tests := []struct { + name string + key string + }{ + {name: "empty key"}, + {name: "invalid Base64", key: "not-base64"}, + {name: "wrong decoded length", key: base64.StdEncoding.EncodeToString([]byte("too short"))}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := New(tt.key) + require.Error(t, err) + }) + } +} diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 49b6f6315a..4f3a9446cc 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -127,6 +127,9 @@ type BaseDeps struct { // Observability exporters (includes logger) Obsv observability.Exporters + + // StateSealer protects state sent through multi-round-trip requests. + StateSealer RequestStateSealer } // Compile-time assertion to verify that BaseDeps implements the ToolDependencies interface. @@ -199,6 +202,9 @@ func (d BaseDeps) Metrics(ctx context.Context) metrics.Metrics { return d.Obsv.Metrics(ctx) } +// GetRequestStateSealer implements RequestStateSealerProvider. +func (d BaseDeps) GetRequestStateSealer() RequestStateSealer { return d.StateSealer } + // IsFeatureEnabled checks if a feature flag is enabled. // Returns false if the feature checker is nil, flag name is empty, or an error occurs. // This allows tools to conditionally change behavior based on feature flags. @@ -279,6 +285,9 @@ type RequestDeps struct { // Observability exporters (includes logger) obsv observability.Exporters + + // StateSealer protects state sent through multi-round-trip requests. + StateSealer RequestStateSealer } // NewRequestDeps creates a RequestDeps with the provided clients and configuration. @@ -334,6 +343,9 @@ func (d *RequestDeps) GetClient(ctx context.Context) (*gogithub.Client, error) { return restClient, nil } +// GetRequestStateSealer implements RequestStateSealerProvider. +func (d *RequestDeps) GetRequestStateSealer() RequestStateSealer { return d.StateSealer } + // GetGQLClient implements ToolDependencies. func (d *RequestDeps) GetGQLClient(ctx context.Context) (*githubv4.Client, error) { // extract the token from the context diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 1e0f79d7c2..9d0b7b35ab 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -705,17 +705,23 @@ func CreateRepository(t translations.TranslationHelperFunc) inventory.ServerTool } const ( + DeleteRepositoryToolName = "delete_repository" deleteRepositoryConfirmationID = "delete_repository_confirmation" deleteRepositoryConfirmationField = "repository_name" ) +type deleteRepositoryState struct { + Owner string `json:"owner"` + Repo string `json:"repo"` +} + // DeleteRepository creates a tool that deletes a GitHub repository after the // user confirms its full name through elicitation. func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool { tool := NewTool( ToolsetMetadataRepos, mcp.Tool{ - Name: "delete_repository", + Name: DeleteRepositoryToolName, Description: t("TOOL_DELETE_REPOSITORY_DESCRIPTION", "Delete a GitHub repository after the user confirms the exact owner/repository name"), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_DELETE_REPOSITORY_USER_TITLE", "Delete repository"), @@ -749,12 +755,24 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool } fullName := owner + "/" + repo + sealer := requestStateSealerFromDeps(deps) var responses mcp.InputResponseMap if req != nil && req.Params != nil { responses = req.Params.InputResponses } response, ok := responses[deleteRepositoryConfirmationID] if !ok { + var requestState string + if sealer != nil { + state, err := json.Marshal(deleteRepositoryState{Owner: owner, Repo: repo}) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal repository deletion state: %w", err) + } + requestState, err = sealer.Seal(ctx, state) + if err != nil { + return nil, nil, fmt.Errorf("failed to seal repository deletion state: %w", err) + } + } return &mcp.CallToolResult{ InputRequests: mcp.InputRequestMap{ deleteRepositoryConfirmationID: &mcp.ElicitParams{ @@ -773,9 +791,27 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool }, }, }, + RequestState: requestState, }, nil, nil } + if sealer != nil { + if req.Params.RequestState == "" { + return utils.NewToolResultError("Repository deletion confirmation state was missing. The repository was not deleted."), nil, nil + } + stateJSON, err := sealer.Open(req.Params.RequestState) + if err != nil { + return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil + } + var state deleteRepositoryState + if err := json.Unmarshal(stateJSON, &state); err != nil { + return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil + } + if state.Owner != owner || state.Repo != repo { + return utils.NewToolResultError("Repository deletion target changed after confirmation was requested. The repository was not deleted."), nil, nil + } + } + confirmation, ok := response.(*mcp.ElicitResult) if !ok { return utils.NewToolResultError("Repository deletion confirmation was invalid. The repository was not deleted."), nil, nil diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 0dbe521081..0a7255aa3c 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/github/github-mcp-server/internal/requeststate" "github.com/github/github-mcp-server/internal/toolsnaps" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/raw" @@ -3037,6 +3038,96 @@ func Test_DeleteRepository(t *testing.T) { assert.Contains(t, getTextResult(t, result).Text, "owner/repo was deleted") }) + t.Run("seals and verifies the deletion target", func(t *testing.T) { + sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef"))) + require.NoError(t, err) + client := NewMockedHTTPClient( + WithRequestMatchHandler( + DeleteReposByOwnerByRepo, + mockResponse(t, http.StatusNoContent, nil), + ), + ) + deps := BaseDeps{ + Client: mustNewGHClient(t, client), + StateSealer: sealer, + } + handler := serverTool.Handler(deps) + + firstRequest := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) + firstResult, err := handler(ContextWithDeps(context.Background(), deps), &firstRequest) + require.NoError(t, err) + require.NotEmpty(t, firstResult.RequestState) + + retry := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) + retry.Params.RequestState = firstResult.RequestState + retry.Params.InputResponses = mcp.InputResponseMap{ + deleteRepositoryConfirmationID: &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }, + } + result, err := handler(ContextWithDeps(context.Background(), deps), &retry) + require.NoError(t, err) + require.False(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, "owner/repo was deleted") + }) + + t.Run("refuses tampered deletion state", func(t *testing.T) { + sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef"))) + require.NoError(t, err) + deps := BaseDeps{ + Client: mustNewGHClient(t, NewMockedHTTPClient()), + StateSealer: sealer, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) + request.Params.RequestState = "tampered" + request.Params.InputResponses = mcp.InputResponseMap{ + deleteRepositoryConfirmationID: &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }, + } + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "state was invalid") + }) + + t.Run("refuses a changed deletion target", func(t *testing.T) { + sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef"))) + require.NoError(t, err) + deps := BaseDeps{ + Client: mustNewGHClient(t, NewMockedHTTPClient()), + StateSealer: sealer, + } + handler := serverTool.Handler(deps) + + firstRequest := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) + firstResult, err := handler(ContextWithDeps(context.Background(), deps), &firstRequest) + require.NoError(t, err) + + retry := createMCPRequest(map[string]any{"owner": "owner", "repo": "another"}) + retry.Params.RequestState = firstResult.RequestState + retry.Params.InputResponses = mcp.InputResponseMap{ + deleteRepositoryConfirmationID: &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }, + } + result, err := handler(ContextWithDeps(context.Background(), deps), &retry) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "target changed") + }) + t.Run("completes multi-round-trip elicitation before deleting", func(t *testing.T) { httpClient := NewMockedHTTPClient( WithRequestMatchHandler( diff --git a/pkg/github/request_state.go b/pkg/github/request_state.go new file mode 100644 index 0000000000..fc38887fde --- /dev/null +++ b/pkg/github/request_state.go @@ -0,0 +1,25 @@ +package github + +import "context" + +// RequestStateSealer protects opaque state sent to clients during multi-round-trip requests. +type RequestStateSealer interface { + Seal(context.Context, []byte) (string, error) + Open(string) ([]byte, error) +} + +// RequestStateSealerProvider optionally supplies request-state protection to tools. +// Keeping this separate from ToolDependencies preserves compatibility for integrators +// that do not expose tools which use multi-round-trip request state. Stateless HTTP +// integrators should implement it or exclude tools that return request state. +type RequestStateSealerProvider interface { + GetRequestStateSealer() RequestStateSealer +} + +func requestStateSealerFromDeps(deps ToolDependencies) RequestStateSealer { + provider, ok := deps.(RequestStateSealerProvider) + if !ok { + return nil + } + return provider.GetRequestStateSealer() +} diff --git a/pkg/http/handler.go b/pkg/http/handler.go index ab229e55dc..4897846c4d 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -5,6 +5,7 @@ import ( "errors" "log/slog" "net/http" + "slices" ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/github/github-mcp-server/pkg/github" @@ -336,12 +337,21 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun hostType = utils.HostTypeDotcom } opts := []github.ToolOption{github.WithHost(hostType)} + tools := github.AllTools(t, opts...) + if cfg.disableDeleteRepository { + tools = slices.DeleteFunc(tools, func(tool inventory.ServerTool) bool { + return tool.Tool.Name == github.DeleteRepositoryToolName + }) + } if !hasStaticConfig(cfg) { - return github.AllTools(t, opts...), github.AllResources(t), github.AllPrompts(t) + return tools, github.AllResources(t), github.AllPrompts(t) } - b := github.NewInventory(t, opts...). + b := inventory.NewBuilder(). + SetTools(tools). + SetResources(github.AllResources(t)). + SetPrompts(github.AllPrompts(t)). WithReadOnly(cfg.ReadOnly). WithToolsets(github.ResolvedEnabledToolsets(cfg.EnabledToolsets, cfg.EnabledTools)) @@ -357,7 +367,7 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun 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, opts...), github.AllResources(t), github.AllPrompts(t) + return tools, github.AllResources(t), github.AllPrompts(t) } ctx := context.Background() diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index c3c7c4b752..fdb04584ee 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -641,6 +641,7 @@ func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) { mockToolWithFeatureFlag("list_issues", "issues", true, "", github.FeatureFlagCSVOutput), mockToolWithFeatureFlag("list_issues", "issues", true, github.FeatureFlagCSVOutput, ""), } + cfg := &ServerConfig{Version: "test", EnabledToolsets: []string{"issues"}} featureChecker := createHTTPFeatureChecker(nil, false) @@ -661,6 +662,30 @@ func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) { assert.Equal(t, github.FeatureFlagCSVOutput, available[0].FeatureFlagEnable) } +func TestStaticInventoryDisablesOnlyDeleteRepository(t *testing.T) { + cfg := &ServerConfig{disableDeleteRepository: true} + tools, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper) + + names := make([]string, 0, len(tools)) + for _, tool := range tools { + names = append(names, tool.Tool.Name) + } + assert.NotContains(t, names, github.DeleteRepositoryToolName) + assert.Contains(t, names, "actions_list", "non-default toolsets must remain available for per-request selection") +} + +func TestStaticInventoryFallbackKeepsDeleteRepositoryDisabled(t *testing.T) { + cfg := &ServerConfig{ + EnabledTools: []string{github.DeleteRepositoryToolName}, + disableDeleteRepository: true, + } + tools, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper) + + for _, tool := range tools { + assert.NotEqual(t, github.DeleteRepositoryToolName, tool.Tool.Name) + } +} + // TestContentTypeHandling verifies that the MCP StreamableHTTP handler // accepts Content-Type values with additional parameters like charset=utf-8. // This is a regression test for https://github.com/github/github-mcp-server/issues/2333 diff --git a/pkg/http/server.go b/pkg/http/server.go index 183116e5e7..270c83772c 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -13,6 +13,7 @@ import ( "syscall" "time" + "github.com/github/github-mcp-server/internal/requeststate" ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/github/github-mcp-server/pkg/github" "github.com/github/github-mcp-server/pkg/http/middleware" @@ -27,6 +28,9 @@ import ( "github.com/go-chi/chi/v5" ) +// MRTRStateKeyEnv is the environment variable used to configure HTTP request-state encryption. +const MRTRStateKeyEnv = "GITHUB_MCP_SERVER_MRTR_STATE_KEY" + type ServerConfig struct { // Version of the server Version string @@ -100,6 +104,11 @@ type ServerConfig struct { // InsidersMode expands to the curated set of feature flags enabled for insiders. InsidersMode bool + + // MRTRStateKey is a Base64-encoded 32-byte key used to protect multi-round-trip request state. + MRTRStateKey string + + disableDeleteRepository bool } func RunHTTPServer(cfg ServerConfig) error { @@ -125,6 +134,11 @@ func RunHTTPServer(cfg ServerConfig) error { logger := slog.New(slogHandler) logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "lockdownEnabled", cfg.LockdownMode, "readOnly", cfg.ReadOnly, "insidersMode", cfg.InsidersMode) + stateSealer, err := configureRequestState(&cfg, logger) + if err != nil { + return err + } + apiHost, err := utils.NewAPIHost(cfg.Host) if err != nil { return fmt.Errorf("failed to parse API host: %w", err) @@ -158,6 +172,7 @@ func RunHTTPServer(cfg ServerConfig) error { featureChecker, obs, ) + deps.StateSealer = stateSealer // Initialize the global tool scope map err = initGlobalToolScopeMap(t, hostType) @@ -243,6 +258,19 @@ func resolveListenAddress(host string, port int) string { return net.JoinHostPort(host, strconv.Itoa(port)) } +func configureRequestState(cfg *ServerConfig, logger *slog.Logger) (github.RequestStateSealer, error) { + if cfg.MRTRStateKey == "" { + cfg.disableDeleteRepository = true + logger.Warn("delete_repository disabled: request-state encryption key is not configured", "environmentVariable", MRTRStateKeyEnv) + return nil, nil + } + sealer, err := requeststate.New(cfg.MRTRStateKey) + if err != nil { + return nil, fmt.Errorf("invalid %s: %w", MRTRStateKeyEnv, err) + } + return sealer, nil +} + func initGlobalToolScopeMap(t translations.TranslationHelperFunc, hostType utils.HostType) error { // Build inventory with all tools to extract scope information inv, err := inventory.NewBuilder(). diff --git a/pkg/http/server_test.go b/pkg/http/server_test.go index ebf4e0e295..05703cf9b5 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -2,6 +2,9 @@ package http import ( "context" + "encoding/base64" + "io" + "log/slog" "testing" ghcontext "github.com/github/github-mcp-server/pkg/context" @@ -216,6 +219,40 @@ func TestResolveListenAddress(t *testing.T) { } } +func TestConfigureRequestState(t *testing.T) { + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + t.Run("missing key disables delete repository", func(t *testing.T) { + cfg := &ServerConfig{} + sealer, err := configureRequestState(cfg, logger) + require.NoError(t, err) + assert.Nil(t, sealer) + assert.True(t, cfg.disableDeleteRepository) + }) + + t.Run("valid key configures sealer", func(t *testing.T) { + cfg := &ServerConfig{ + MRTRStateKey: base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef")), + } + sealer, err := configureRequestState(cfg, logger) + require.NoError(t, err) + require.NotNil(t, sealer) + assert.False(t, cfg.disableDeleteRepository) + + token, err := sealer.Seal(context.Background(), []byte("state")) + require.NoError(t, err) + opened, err := sealer.Open(token) + require.NoError(t, err) + assert.Equal(t, []byte("state"), opened) + }) + + t.Run("malformed key fails", func(t *testing.T) { + cfg := &ServerConfig{MRTRStateKey: "invalid"} + _, err := configureRequestState(cfg, logger) + require.ErrorContains(t, err, "invalid "+MRTRStateKeyEnv) + }) +} + func TestHeaderAllowedFeatureFlagsMatchesAllowed(t *testing.T) { // Ensure HeaderAllowedFeatureFlags delegates to AllowedFeatureFlags allowed := github.HeaderAllowedFeatureFlags() From 676a5b254b30747207e5a90190a63bb16bb9f051 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Mon, 17 Aug 2026 13:46:00 +0200 Subject: [PATCH 04/11] fix(repos): expire deletion confirmations Bind sealed repository deletion state to the immutable repository ID and a ten-minute expiry. Re-check identity before deletion so replay cannot affect a recreated repository. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- pkg/github/repositories.go | 55 ++++++++++++++++++-- pkg/github/repositories_test.go | 89 ++++++++++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 4 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 9d0b7b35ab..015b0c2534 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -10,6 +10,7 @@ import ( "slices" "strconv" "strings" + "time" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/ifc" @@ -708,11 +709,14 @@ const ( DeleteRepositoryToolName = "delete_repository" deleteRepositoryConfirmationID = "delete_repository_confirmation" deleteRepositoryConfirmationField = "repository_name" + deleteRepositoryConfirmationTTL = 10 * time.Minute ) type deleteRepositoryState struct { - Owner string `json:"owner"` - Repo string `json:"repo"` + Owner string `json:"owner"` + Repo string `json:"repo"` + RepositoryID int64 `json:"repository_id"` + ExpiresAt int64 `json:"expires_at"` } // DeleteRepository creates a tool that deletes a GitHub repository after the @@ -756,6 +760,7 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool fullName := owner + "/" + repo sealer := requestStateSealerFromDeps(deps) + var deletionState *deleteRepositoryState var responses mcp.InputResponseMap if req != nil && req.Params != nil { responses = req.Params.InputResponses @@ -764,7 +769,20 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool if !ok { var requestState string if sealer != nil { - state, err := json.Marshal(deleteRepositoryState{Owner: owner, Repo: repo}) + client, err := deps.GetClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + repositoryID, result := repositoryIDForDeletion(ctx, client, owner, repo) + if result != nil { + return result, nil, nil + } + state, err := json.Marshal(deleteRepositoryState{ + Owner: owner, + Repo: repo, + RepositoryID: repositoryID, + ExpiresAt: time.Now().Add(deleteRepositoryConfirmationTTL).Unix(), + }) if err != nil { return nil, nil, fmt.Errorf("failed to marshal repository deletion state: %w", err) } @@ -810,6 +828,13 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool if state.Owner != owner || state.Repo != repo { return utils.NewToolResultError("Repository deletion target changed after confirmation was requested. The repository was not deleted."), nil, nil } + if state.ExpiresAt <= time.Now().Unix() { + return utils.NewToolResultError("Repository deletion confirmation expired. The repository was not deleted."), nil, nil + } + if state.RepositoryID == 0 { + return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil + } + deletionState = &state } confirmation, ok := response.(*mcp.ElicitResult) @@ -828,6 +853,15 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool if err != nil { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } + if deletionState != nil { + currentRepositoryID, result := repositoryIDForDeletion(ctx, client, owner, repo) + if result != nil { + return result, nil, nil + } + if currentRepositoryID != deletionState.RepositoryID { + return utils.NewToolResultError("Repository identity changed after confirmation was requested. The repository was not deleted."), nil, nil + } + } resp, err := client.Repositories.Delete(ctx, owner, repo) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, @@ -854,6 +888,21 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool return tool } +func repositoryIDForDeletion(ctx context.Context, client *github.Client, owner, repo string) (int64, *mcp.CallToolResult) { + repository, resp, err := client.Repositories.Get(ctx, owner, repo) + if err != nil { + return 0, ghErrors.NewGitHubAPIErrorResponse(ctx, + fmt.Sprintf("failed to get repository: %s/%s", owner, repo), + resp, + err, + ) + } + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + return repository.GetID(), nil +} + // FetchRepoIsPrivate returns whether a repository is private. It is a thin // wrapper around the GitHub Repositories.Get endpoint provided as a shared // helper for IFC label computation across tools. diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 0a7255aa3c..ce36a65d15 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -3042,6 +3042,10 @@ func Test_DeleteRepository(t *testing.T) { sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef"))) require.NoError(t, err) client := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), WithRequestMatchHandler( DeleteReposByOwnerByRepo, mockResponse(t, http.StatusNoContent, nil), @@ -3099,15 +3103,54 @@ func Test_DeleteRepository(t *testing.T) { assert.Contains(t, getErrorResult(t, result).Text, "state was invalid") }) - t.Run("refuses a changed deletion target", func(t *testing.T) { + t.Run("refuses expired deletion state", func(t *testing.T) { sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef"))) require.NoError(t, err) + stateJSON, err := json.Marshal(deleteRepositoryState{ + Owner: "owner", + Repo: "repo", + RepositoryID: 123, + ExpiresAt: time.Now().Add(-time.Minute).Unix(), + }) + require.NoError(t, err) + state, err := sealer.Seal(context.Background(), stateJSON) + require.NoError(t, err) deps := BaseDeps{ Client: mustNewGHClient(t, NewMockedHTTPClient()), StateSealer: sealer, } handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) + request.Params.RequestState = state + request.Params.InputResponses = mcp.InputResponseMap{ + deleteRepositoryConfirmationID: &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }, + } + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "confirmation expired") + }) + + t.Run("refuses a changed deletion target", func(t *testing.T) { + sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef"))) + require.NoError(t, err) + deps := BaseDeps{ + Client: mustNewGHClient(t, NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), + )), + StateSealer: sealer, + } + handler := serverTool.Handler(deps) + firstRequest := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) firstResult, err := handler(ContextWithDeps(context.Background(), deps), &firstRequest) require.NoError(t, err) @@ -3128,6 +3171,50 @@ func Test_DeleteRepository(t *testing.T) { assert.Contains(t, getErrorResult(t, result).Text, "target changed") }) + t.Run("refuses a recreated repository", func(t *testing.T) { + sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef"))) + require.NoError(t, err) + var repositoryLookups int + client := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + repositoryLookups++ + id := 123 + if repositoryLookups > 1 { + id = 456 + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"id": id})) + }), + ), + ) + deps := BaseDeps{ + Client: mustNewGHClient(t, client), + StateSealer: sealer, + } + handler := serverTool.Handler(deps) + + firstRequest := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) + firstResult, err := handler(ContextWithDeps(context.Background(), deps), &firstRequest) + require.NoError(t, err) + + retry := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) + retry.Params.RequestState = firstResult.RequestState + retry.Params.InputResponses = mcp.InputResponseMap{ + deleteRepositoryConfirmationID: &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }, + } + result, err := handler(ContextWithDeps(context.Background(), deps), &retry) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "identity changed") + }) + t.Run("completes multi-round-trip elicitation before deleting", func(t *testing.T) { httpClient := NewMockedHTTPClient( WithRequestMatchHandler( From c285c6af464e0bee3bf514d4a72686d428c746b1 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Mon, 17 Aug 2026 14:11:50 +0200 Subject: [PATCH 05/11] fix(http): preserve tool and scope restrictions Apply static allowlists before removing unavailable tools and fail closed on invalid configured tool names. Model independent OAuth requirements as conjunctive groups so repository deletion requires both delete_repo and repo. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- README.md | 2 +- pkg/github/dependencies.go | 2 ++ pkg/github/repositories.go | 2 +- pkg/github/repositories_test.go | 3 ++- pkg/github/scope_filter.go | 3 +++ pkg/github/scope_filter_test.go | 18 ++++++++++++++++++ pkg/http/handler.go | 17 ++++++++++------- pkg/http/handler_test.go | 4 +--- pkg/inventory/server_tool.go | 4 ++++ pkg/scopes/map.go | 30 ++++++++++++++++++++++++++++-- pkg/scopes/map_test.go | 31 +++++++++++++++++++++++++++++++ pkg/scopes/scopes.go | 33 +++++++++++++++++++++++++++++++++ pkg/scopes/scopes_test.go | 8 ++++++++ 13 files changed, 142 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index efae2b8ac4..a80801c753 100644 --- a/README.md +++ b/README.md @@ -1309,7 +1309,7 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **delete_repository** - Delete repository - - **Required OAuth Scopes**: `delete_repo` + - **Required OAuth Scopes (any of)**: `delete_repo`, `repo` - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 4f3a9446cc..70caa74faa 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -245,6 +245,7 @@ func NewTool[In, Out any]( }) st.RequiredScopes = scopes.ToStringSlice(requiredScopes...) st.AcceptedScopes = scopes.ExpandScopes(requiredScopes...) + st.RequiredScopeGroups = scopes.ExpandScopeGroups(requiredScopes...) return st } @@ -268,6 +269,7 @@ func NewToolFromHandler( }) st.RequiredScopes = scopes.ToStringSlice(requiredScopes...) st.AcceptedScopes = scopes.ExpandScopes(requiredScopes...) + st.RequiredScopeGroups = scopes.ExpandScopeGroups(requiredScopes...) return st } diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 015b0c2534..773a428fe1 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -747,7 +747,7 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.DeleteRepo}, + []scopes.Scope{scopes.DeleteRepo, scopes.Repo}, func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index ce36a65d15..88ae042b3b 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -3002,7 +3002,8 @@ func Test_DeleteRepository(t *testing.T) { assert.True(t, *tool.Annotations.DestructiveHint) assert.Equal(t, inventory.ProtocolVersionMultiRoundTrip, serverTool.MinimumProtocolVersion) assert.Equal(t, inventory.ElicitationModeForm, serverTool.RequiredElicitationMode) - assert.Equal(t, []string{string(scopes.DeleteRepo)}, serverTool.RequiredScopes) + assert.ElementsMatch(t, []string{string(scopes.DeleteRepo), string(scopes.Repo)}, serverTool.RequiredScopes) + assert.Len(t, serverTool.RequiredScopeGroups, 2) t.Run("requests exact repository name through elicitation", func(t *testing.T) { result := invokeDeleteRepository(t, serverTool, NewMockedHTTPClient(), nil) diff --git a/pkg/github/scope_filter.go b/pkg/github/scope_filter.go index 42f8e98b0c..add0ee3b18 100644 --- a/pkg/github/scope_filter.go +++ b/pkg/github/scope_filter.go @@ -59,6 +59,9 @@ func CreateToolScopeFilter(tokenScopes []string) inventory.ToolFilter { if tool.Tool.Annotations != nil && tool.Tool.Annotations.ReadOnlyHint && onlyRequiresRepoScopes(tool.AcceptedScopes) { return true, nil } + if len(tool.RequiredScopeGroups) > 0 { + return scopes.HasRequiredScopeGroups(tokenScopes, tool.RequiredScopeGroups), nil + } return scopes.HasRequiredScopes(tokenScopes, tool.AcceptedScopes), nil } } diff --git a/pkg/github/scope_filter_test.go b/pkg/github/scope_filter_test.go index 9cdd4db19b..669f1ce169 100644 --- a/pkg/github/scope_filter_test.go +++ b/pkg/github/scope_filter_test.go @@ -58,6 +58,12 @@ func TestCreateToolScopeFilter(t *testing.T) { AcceptedScopes: []string{"repo", "admin:org"}, } + toolConjunctiveScopes := &inventory.ServerTool{ + Tool: mcp.Tool{Name: "conjunctive_scope_tool"}, + AcceptedScopes: []string{"delete_repo", "repo"}, + RequiredScopeGroups: [][]string{{"delete_repo"}, {"repo"}}, + } + tests := []struct { name string tokenScopes []string @@ -130,6 +136,18 @@ func TestCreateToolScopeFilter(t *testing.T) { tool: toolPublicRepoScope, expected: true, }, + { + name: "token must satisfy every required scope group", + tokenScopes: []string{"delete_repo"}, + tool: toolConjunctiveScopes, + expected: false, + }, + { + name: "token satisfying every required scope group can see tool", + tokenScopes: []string{"delete_repo", "repo"}, + tool: toolConjunctiveScopes, + expected: true, + }, } for _, tt := range tests { diff --git a/pkg/http/handler.go b/pkg/http/handler.go index 73b07b28b8..b9dfccf09f 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -360,14 +360,17 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun } opts := []github.ToolOption{github.WithHost(hostType)} tools := github.AllTools(t, opts...) - if cfg.disableDeleteRepository { - tools = slices.DeleteFunc(tools, func(tool inventory.ServerTool) bool { + filterUnavailable := func(tools []inventory.ServerTool) []inventory.ServerTool { + if !cfg.disableDeleteRepository { + return tools + } + return slices.DeleteFunc(tools, func(tool inventory.ServerTool) bool { return tool.Tool.Name == github.DeleteRepositoryToolName }) } if !hasStaticConfig(cfg) { - return tools, github.AllResources(t), github.AllPrompts(t) + return filterUnavailable(tools), github.AllResources(t), github.AllPrompts(t) } b := inventory.NewBuilder(). @@ -387,13 +390,13 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun 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 tools, github.AllResources(t), github.AllPrompts(t) + // Invalid static tool names must fail closed rather than widening an + // explicit allowlist to every tool. + return nil, github.AllResources(t), github.AllPrompts(t) } ctx := context.Background() - return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) + return filterUnavailable(inv.AvailableTools(ctx)), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) } // InventoryFiltersForRequest applies filters to the inventory builder diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 33c81eb068..c1ee327952 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -682,9 +682,7 @@ func TestStaticInventoryFallbackKeepsDeleteRepositoryDisabled(t *testing.T) { } tools, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper) - for _, tool := range tools { - assert.NotEqual(t, github.DeleteRepositoryToolName, tool.Tool.Name) - } + assert.Empty(t, tools, "an unavailable explicit allowlist must not widen to other tools") } // TestContentTypeHandling verifies that the MCP StreamableHTTP handler diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 39f64c716f..d25458253f 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -97,6 +97,10 @@ type ServerTool struct { // This includes the required scopes plus any higher-level scopes that provide // the necessary permissions due to scope hierarchy. AcceptedScopes []string + + // RequiredScopeGroups contains one group of accepted alternatives for each + // independently required OAuth scope. Every group must be satisfied. + RequiredScopeGroups [][]string } // IsReadOnly returns true if this tool is marked as read-only via annotations. diff --git a/pkg/scopes/map.go b/pkg/scopes/map.go index 3c98338347..e13345f0f7 100644 --- a/pkg/scopes/map.go +++ b/pkg/scopes/map.go @@ -12,6 +12,10 @@ type ToolScopeInfo struct { // AcceptedScopes contains all scopes that satisfy the requirements (including parent scopes). AcceptedScopes []string + + // RequiredScopeGroups contains accepted alternatives for each independently + // required scope. Every group must be satisfied. + RequiredScopeGroups [][]string } // globalToolScopeMap is populated from inventory when SetToolScopeMapFromInventory is called @@ -59,8 +63,9 @@ func GetToolScopeMapFromInventory(inv *inventory.Inventory) ToolScopeMap { tool := &allTools[i] if len(tool.RequiredScopes) > 0 || len(tool.AcceptedScopes) > 0 { result[tool.Tool.Name] = &ToolScopeInfo{ - RequiredScopes: tool.RequiredScopes, - AcceptedScopes: tool.AcceptedScopes, + RequiredScopes: tool.RequiredScopes, + AcceptedScopes: tool.AcceptedScopes, + RequiredScopeGroups: tool.RequiredScopeGroups, } } } @@ -70,6 +75,9 @@ func GetToolScopeMapFromInventory(inv *inventory.Inventory) ToolScopeMap { // HasAcceptedScope checks if any of the provided user scopes satisfy the tool's requirements. func (t *ToolScopeInfo) HasAcceptedScope(userScopes ...string) bool { + if t != nil && len(t.RequiredScopeGroups) > 0 { + return HasRequiredScopeGroups(userScopes, t.RequiredScopeGroups) + } if t == nil || len(t.AcceptedScopes) == 0 { return true // No scopes required } @@ -99,6 +107,24 @@ func (t *ToolScopeInfo) MissingScopes(userScopes ...string) []string { userScopeSet[s] = true } + if len(t.RequiredScopeGroups) > 0 { + userScopeSet := expandScopeSet(userScopes) + var missing []string + for i, group := range t.RequiredScopeGroups { + satisfied := false + for _, scope := range group { + if userScopeSet[scope] { + satisfied = true + break + } + } + if !satisfied && i < len(t.RequiredScopes) { + missing = append(missing, t.RequiredScopes[i]) + } + } + return missing + } + // Check if any accepted scope is present hasAccepted := false for _, scope := range t.AcceptedScopes { diff --git a/pkg/scopes/map_test.go b/pkg/scopes/map_test.go index 5f33cdda2b..3c5a7ede72 100644 --- a/pkg/scopes/map_test.go +++ b/pkg/scopes/map_test.go @@ -123,6 +123,26 @@ func TestToolScopeInfo_HasAcceptedScope(t *testing.T) { userScopes: []string{"public_repo"}, expected: false, }, + { + name: "satisfies all required groups", + scopeInfo: &ToolScopeInfo{ + RequiredScopes: []string{"delete_repo", "repo"}, + AcceptedScopes: []string{"delete_repo", "repo"}, + RequiredScopeGroups: [][]string{{"delete_repo"}, {"repo"}}, + }, + userScopes: []string{"delete_repo", "repo"}, + expected: true, + }, + { + name: "missing one required group", + scopeInfo: &ToolScopeInfo{ + RequiredScopes: []string{"delete_repo", "repo"}, + AcceptedScopes: []string{"delete_repo", "repo"}, + RequiredScopeGroups: [][]string{{"delete_repo"}, {"repo"}}, + }, + userScopes: []string{"delete_repo"}, + expected: false, + }, } for _, tc := range testCases { @@ -178,6 +198,17 @@ func TestToolScopeInfo_MissingScopes(t *testing.T) { expectedLen: 0, expectedScopes: nil, }, + { + name: "reports only missing required groups", + scopeInfo: &ToolScopeInfo{ + RequiredScopes: []string{"delete_repo", "repo"}, + AcceptedScopes: []string{"delete_repo", "repo"}, + RequiredScopeGroups: [][]string{{"delete_repo"}, {"repo"}}, + }, + userScopes: []string{"delete_repo"}, + expectedLen: 1, + expectedScopes: []string{"repo"}, + }, } for _, tc := range testCases { diff --git a/pkg/scopes/scopes.go b/pkg/scopes/scopes.go index 084ecaf1c6..4864f4e474 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -153,6 +153,17 @@ func ExpandScopes(required ...Scope) []string { return result } +// ExpandScopeGroups returns one accepted-scope group for each independently +// required scope. A token must satisfy every group, while any scope within a +// group is sufficient because parent scopes grant the same permission. +func ExpandScopeGroups(required ...Scope) [][]string { + groups := make([][]string, 0, len(required)) + for _, scope := range required { + groups = append(groups, ExpandScopes(scope)) + } + return groups +} + // expandScopeSet returns a set of all scopes granted by the given scopes, // including child scopes from the hierarchy. // For example, if "repo" is provided, the result includes "repo", "public_repo", @@ -196,3 +207,25 @@ func HasRequiredScopes(tokenScopes []string, acceptedScopes []string) bool { } return false } + +// HasRequiredScopeGroups reports whether the token satisfies every independent +// required-scope group. +func HasRequiredScopeGroups(tokenScopes []string, groups [][]string) bool { + if len(groups) == 0 { + return true + } + grantedScopes := expandScopeSet(tokenScopes) + for _, group := range groups { + satisfied := false + for _, accepted := range group { + if grantedScopes[accepted] { + satisfied = true + break + } + } + if !satisfied { + return false + } + } + return true +} diff --git a/pkg/scopes/scopes_test.go b/pkg/scopes/scopes_test.go index 94e21a9cd7..d938504817 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -112,6 +112,14 @@ func TestExpandScopes(t *testing.T) { } } +func TestHasRequiredScopeGroups(t *testing.T) { + groups := ExpandScopeGroups(DeleteRepo, Repo) + + assert.True(t, HasRequiredScopeGroups([]string{"delete_repo", "repo"}, groups)) + assert.False(t, HasRequiredScopeGroups([]string{"delete_repo"}, groups)) + assert.False(t, HasRequiredScopeGroups([]string{"repo"}, groups)) +} + func TestToStringSlice(t *testing.T) { tests := []struct { name string From 2dce589e9d3dba148d6f21464db8a758e5d13087 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 18 Aug 2026 12:07:33 +0200 Subject: [PATCH 06/11] fix(repos): require protected confirmation state Give stdio a process-local request-state sealer and make deletion fail closed without one. Preserve legacy any-of OAuth behavior globally while documenting and enforcing delete_repository's conjunctive delete_repo and repo requirements. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- README.md | 2 +- cmd/github-mcp-server/generate_docs.go | 10 +-- cmd/github-mcp-server/main_test.go | 37 ++++++++ internal/ghmcp/server.go | 5 ++ internal/requeststate/sealer.go | 13 +++ internal/requeststate/sealer_test.go | 10 +++ pkg/github/dependencies.go | 2 - pkg/github/repositories.go | 103 +++++++++++----------- pkg/github/repositories_test.go | 114 ++++++++++++++++++++++--- pkg/github/scope_filter_test.go | 17 ++++ 10 files changed, 240 insertions(+), 73 deletions(-) diff --git a/README.md b/README.md index a80801c753..5699aaf34f 100644 --- a/README.md +++ b/README.md @@ -1309,7 +1309,7 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **delete_repository** - Delete repository - - **Required OAuth Scopes (any of)**: `delete_repo`, `repo` + - **Required OAuth Scopes (all required)**: `delete_repo`, `repo` - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index a2310e2106..2eb1d35743 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -221,13 +221,13 @@ func writeToolDoc(buf *strings.Builder, tool inventory.ServerTool) { // OAuth scopes if present if len(tool.RequiredScopes) > 0 { - // Scope filtering uses "any of" semantics (see scopes.HasRequiredScopes), - // so when multiple required scopes are listed, render them as alternatives - // rather than implying all are required. scopeList := "`" + strings.Join(tool.RequiredScopes, "`, `") + "`" - if len(tool.RequiredScopes) > 1 { + switch { + case len(tool.RequiredScopeGroups) > 1: + fmt.Fprintf(buf, " - **Required OAuth Scopes (all required)**: %s\n", scopeList) + case len(tool.RequiredScopes) > 1: fmt.Fprintf(buf, " - **Required OAuth Scopes (any of)**: %s\n", scopeList) - } else { + default: fmt.Fprintf(buf, " - **Required OAuth Scopes**: %s\n", scopeList) } diff --git a/cmd/github-mcp-server/main_test.go b/cmd/github-mcp-server/main_test.go index aa81c637dd..a1ed642a64 100644 --- a/cmd/github-mcp-server/main_test.go +++ b/cmd/github-mcp-server/main_test.go @@ -3,9 +3,12 @@ package main import ( "os" "path/filepath" + "strings" "testing" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -38,6 +41,40 @@ func TestGitHubAppFlagsAreStdioOnly(t *testing.T) { assert.Nil(t, httpCmd.Flags().Lookup("app-id")) } +func TestWriteToolDocScopeSemantics(t *testing.T) { + tests := []struct { + name string + tool inventory.ServerTool + want string + }{ + { + name: "legacy multi-scope tools use any-of", + tool: inventory.ServerTool{ + Tool: mcp.Tool{Name: "legacy", Annotations: &mcp.ToolAnnotations{Title: "Legacy"}}, + RequiredScopes: []string{"repo", "read:org"}, + }, + want: "**Required OAuth Scopes (any of)**", + }, + { + name: "conjunctive scope groups use all-required", + tool: inventory.ServerTool{ + Tool: mcp.Tool{Name: "conjunctive", Annotations: &mcp.ToolAnnotations{Title: "Conjunctive"}}, + RequiredScopes: []string{"delete_repo", "repo"}, + RequiredScopeGroups: [][]string{{"delete_repo"}, {"repo"}}, + }, + want: "**Required OAuth Scopes (all required)**", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf strings.Builder + writeToolDoc(&buf, tt.tool) + assert.Contains(t, buf.String(), tt.want) + }) + } +} + func TestSchemaTypeString(t *testing.T) { tests := []struct { name string diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 12306e6a23..b407f30057 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -13,6 +13,7 @@ import ( "time" "github.com/github/github-mcp-server/internal/oauth" + "github.com/github/github-mcp-server/internal/requeststate" "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/github" "github.com/github/github-mcp-server/pkg/http/transport" @@ -169,6 +170,10 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se featureChecker, obs, ) + deps.StateSealer, err = requeststate.NewRandom() + if err != nil { + return nil, fmt.Errorf("failed to configure request-state protection: %w", err) + } // Build and register the tool/resource/prompt inventory inventoryBuilder := github.NewInventory(cfg.Translator, github.WithHost(hostType)). WithDeprecatedAliases(github.DeprecatedToolAliases). diff --git a/internal/requeststate/sealer.go b/internal/requeststate/sealer.go index 25ec725b6b..cff3bfea7f 100644 --- a/internal/requeststate/sealer.go +++ b/internal/requeststate/sealer.go @@ -17,6 +17,15 @@ type Sealer struct { aead cipher.AEAD } +// NewRandom constructs a sealer with a process-local random key. +func NewRandom() (*Sealer, error) { + key := make([]byte, keySize) + if _, err := rand.Read(key); err != nil { + return nil, fmt.Errorf("generating key: %w", err) + } + return newFromKey(key) +} + // New constructs a sealer from a standard Base64-encoded 32-byte key. func New(encodedKey string) (*Sealer, error) { key, err := base64.StdEncoding.DecodeString(encodedKey) @@ -26,6 +35,10 @@ func New(encodedKey string) (*Sealer, error) { if len(key) != keySize { return nil, fmt.Errorf("decoded key must be %d bytes, got %d", keySize, len(key)) } + return newFromKey(key) +} + +func newFromKey(key []byte) (*Sealer, error) { block, err := aes.NewCipher(key) if err != nil { return nil, fmt.Errorf("creating cipher: %w", err) diff --git a/internal/requeststate/sealer_test.go b/internal/requeststate/sealer_test.go index b258beeb61..9f244c9116 100644 --- a/internal/requeststate/sealer_test.go +++ b/internal/requeststate/sealer_test.go @@ -39,6 +39,16 @@ func TestSealer(t *testing.T) { }) } +func TestNewRandom(t *testing.T) { + sealer, err := NewRandom() + require.NoError(t, err) + token, err := sealer.Seal(context.Background(), []byte("state")) + require.NoError(t, err) + opened, err := sealer.Open(token) + require.NoError(t, err) + assert.Equal(t, []byte("state"), opened) +} + func TestNew(t *testing.T) { tests := []struct { name string diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 70caa74faa..4f3a9446cc 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -245,7 +245,6 @@ func NewTool[In, Out any]( }) st.RequiredScopes = scopes.ToStringSlice(requiredScopes...) st.AcceptedScopes = scopes.ExpandScopes(requiredScopes...) - st.RequiredScopeGroups = scopes.ExpandScopeGroups(requiredScopes...) return st } @@ -269,7 +268,6 @@ func NewToolFromHandler( }) st.RequiredScopes = scopes.ToStringSlice(requiredScopes...) st.AcceptedScopes = scopes.ExpandScopes(requiredScopes...) - st.RequiredScopeGroups = scopes.ExpandScopeGroups(requiredScopes...) return st } diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 773a428fe1..25be496457 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -760,36 +760,36 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool fullName := owner + "/" + repo sealer := requestStateSealerFromDeps(deps) - var deletionState *deleteRepositoryState + if sealer == nil { + return utils.NewToolResultError("Repository deletion is unavailable because request-state protection is not configured."), nil, nil + } + var deletionState deleteRepositoryState var responses mcp.InputResponseMap if req != nil && req.Params != nil { responses = req.Params.InputResponses } response, ok := responses[deleteRepositoryConfirmationID] if !ok { - var requestState string - if sealer != nil { - client, err := deps.GetClient(ctx) - if err != nil { - return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) - } - repositoryID, result := repositoryIDForDeletion(ctx, client, owner, repo) - if result != nil { - return result, nil, nil - } - state, err := json.Marshal(deleteRepositoryState{ - Owner: owner, - Repo: repo, - RepositoryID: repositoryID, - ExpiresAt: time.Now().Add(deleteRepositoryConfirmationTTL).Unix(), - }) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal repository deletion state: %w", err) - } - requestState, err = sealer.Seal(ctx, state) - if err != nil { - return nil, nil, fmt.Errorf("failed to seal repository deletion state: %w", err) - } + client, err := deps.GetClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + repositoryID, result := repositoryIDForDeletion(ctx, client, owner, repo) + if result != nil { + return result, nil, nil + } + state, err := json.Marshal(deleteRepositoryState{ + Owner: owner, + Repo: repo, + RepositoryID: repositoryID, + ExpiresAt: time.Now().Add(deleteRepositoryConfirmationTTL).Unix(), + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal repository deletion state: %w", err) + } + requestState, err := sealer.Seal(ctx, state) + if err != nil { + return nil, nil, fmt.Errorf("failed to seal repository deletion state: %w", err) } return &mcp.CallToolResult{ InputRequests: mcp.InputRequestMap{ @@ -813,28 +813,24 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool }, nil, nil } - if sealer != nil { - if req.Params.RequestState == "" { - return utils.NewToolResultError("Repository deletion confirmation state was missing. The repository was not deleted."), nil, nil - } - stateJSON, err := sealer.Open(req.Params.RequestState) - if err != nil { - return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil - } - var state deleteRepositoryState - if err := json.Unmarshal(stateJSON, &state); err != nil { - return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil - } - if state.Owner != owner || state.Repo != repo { - return utils.NewToolResultError("Repository deletion target changed after confirmation was requested. The repository was not deleted."), nil, nil - } - if state.ExpiresAt <= time.Now().Unix() { - return utils.NewToolResultError("Repository deletion confirmation expired. The repository was not deleted."), nil, nil - } - if state.RepositoryID == 0 { - return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil - } - deletionState = &state + if req.Params.RequestState == "" { + return utils.NewToolResultError("Repository deletion confirmation state was missing. The repository was not deleted."), nil, nil + } + stateJSON, err := sealer.Open(req.Params.RequestState) + if err != nil { + return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil + } + if err := json.Unmarshal(stateJSON, &deletionState); err != nil { + return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil + } + if deletionState.Owner != owner || deletionState.Repo != repo { + return utils.NewToolResultError("Repository deletion target changed after confirmation was requested. The repository was not deleted."), nil, nil + } + if deletionState.ExpiresAt <= time.Now().Unix() { + return utils.NewToolResultError("Repository deletion confirmation expired. The repository was not deleted."), nil, nil + } + if deletionState.RepositoryID == 0 { + return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil } confirmation, ok := response.(*mcp.ElicitResult) @@ -853,14 +849,12 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool if err != nil { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } - if deletionState != nil { - currentRepositoryID, result := repositoryIDForDeletion(ctx, client, owner, repo) - if result != nil { - return result, nil, nil - } - if currentRepositoryID != deletionState.RepositoryID { - return utils.NewToolResultError("Repository identity changed after confirmation was requested. The repository was not deleted."), nil, nil - } + currentRepositoryID, result := repositoryIDForDeletion(ctx, client, owner, repo) + if result != nil { + return result, nil, nil + } + if currentRepositoryID != deletionState.RepositoryID { + return utils.NewToolResultError("Repository identity changed after confirmation was requested. The repository was not deleted."), nil, nil } resp, err := client.Repositories.Delete(ctx, owner, repo) if err != nil { @@ -885,6 +879,7 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool ) tool.MinimumProtocolVersion = inventory.ProtocolVersionMultiRoundTrip tool.RequiredElicitationMode = inventory.ElicitationModeForm + tool.RequiredScopeGroups = scopes.ExpandScopeGroups(scopes.DeleteRepo, scopes.Repo) return tool } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 88ae042b3b..1d488b9ced 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -3006,7 +3006,13 @@ func Test_DeleteRepository(t *testing.T) { assert.Len(t, serverTool.RequiredScopeGroups, 2) t.Run("requests exact repository name through elicitation", func(t *testing.T) { - result := invokeDeleteRepository(t, serverTool, NewMockedHTTPClient(), nil) + client := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), + ) + result := invokeDeleteRepository(t, serverTool, client, nil) require.False(t, result.IsError) require.Len(t, result.InputRequests, 1) @@ -3023,6 +3029,10 @@ func Test_DeleteRepository(t *testing.T) { t.Run("deletes after exact confirmation", func(t *testing.T) { client := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), WithRequestMatchHandler( DeleteReposByOwnerByRepo, mockResponse(t, http.StatusNoContent, nil), @@ -3039,6 +3049,49 @@ func Test_DeleteRepository(t *testing.T) { assert.Contains(t, getTextResult(t, result).Text, "owner/repo was deleted") }) + t.Run("refuses deletion without request-state protection", func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, NewMockedHTTPClient())} + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) + request.Params.InputResponses = mcp.InputResponseMap{ + deleteRepositoryConfirmationID: &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }, + } + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "request-state protection is not configured") + }) + + t.Run("refuses confirmation without request state", func(t *testing.T) { + sealer, err := requeststate.NewRandom() + require.NoError(t, err) + deps := BaseDeps{ + Client: mustNewGHClient(t, NewMockedHTTPClient()), + StateSealer: sealer, + } + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"}) + request.Params.InputResponses = mcp.InputResponseMap{ + deleteRepositoryConfirmationID: &mcp.ElicitResult{ + Action: "accept", + Content: map[string]any{ + deleteRepositoryConfirmationField: "owner/repo", + }, + }, + } + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "state was missing") + }) + t.Run("seals and verifies the deletion target", func(t *testing.T) { sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef"))) require.NoError(t, err) @@ -3218,12 +3271,21 @@ func Test_DeleteRepository(t *testing.T) { t.Run("completes multi-round-trip elicitation before deleting", func(t *testing.T) { httpClient := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), WithRequestMatchHandler( DeleteReposByOwnerByRepo, mockResponse(t, http.StatusNoContent, nil), ), ) - deps := BaseDeps{Client: mustNewGHClient(t, httpClient)} + sealer, err := requeststate.NewRandom() + require.NoError(t, err) + deps := BaseDeps{ + Client: mustNewGHClient(t, httpClient), + StateSealer: sealer, + } inv, err := inventory.NewBuilder(). SetTools([]inventory.ServerTool{serverTool}). @@ -3271,7 +3333,13 @@ func Test_DeleteRepository(t *testing.T) { }) t.Run("refuses mismatched confirmation", func(t *testing.T) { - result := invokeDeleteRepository(t, serverTool, NewMockedHTTPClient(), &mcp.ElicitResult{ + client := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), + ) + result := invokeDeleteRepository(t, serverTool, client, &mcp.ElicitResult{ Action: "accept", Content: map[string]any{ deleteRepositoryConfirmationField: "owner/another-repo", @@ -3283,7 +3351,13 @@ func Test_DeleteRepository(t *testing.T) { }) t.Run("refuses declined confirmation", func(t *testing.T) { - result := invokeDeleteRepository(t, serverTool, NewMockedHTTPClient(), &mcp.ElicitResult{ + client := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), + ) + result := invokeDeleteRepository(t, serverTool, client, &mcp.ElicitResult{ Action: "decline", }) @@ -3293,6 +3367,10 @@ func Test_DeleteRepository(t *testing.T) { t.Run("returns GitHub API errors", func(t *testing.T) { client := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), WithRequestMatchHandler( DeleteReposByOwnerByRepo, mockResponse(t, http.StatusForbidden, map[string]any{"message": "Requires admin permissions"}), @@ -3313,19 +3391,33 @@ func Test_DeleteRepository(t *testing.T) { func invokeDeleteRepository(t *testing.T, tool inventory.ServerTool, httpClient *http.Client, confirmation *mcp.ElicitResult) *mcp.CallToolResult { t.Helper() - deps := BaseDeps{Client: mustNewGHClient(t, httpClient)} + sealer, err := requeststate.NewRandom() + require.NoError(t, err) + deps := BaseDeps{ + Client: mustNewGHClient(t, httpClient), + StateSealer: sealer, + } handler := tool.Handler(deps) - request := createMCPRequest(map[string]any{ + firstRequest := createMCPRequest(map[string]any{ "owner": "owner", "repo": "repo", }) - if confirmation != nil { - request.Params.InputResponses = mcp.InputResponseMap{ - deleteRepositoryConfirmationID: confirmation, - } + result, err := handler(ContextWithDeps(context.Background(), deps), &firstRequest) + require.NoError(t, err) + require.NotNil(t, result) + if confirmation == nil { + return result } - result, err := handler(ContextWithDeps(context.Background(), deps), &request) + retry := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + }) + retry.Params.RequestState = result.RequestState + retry.Params.InputResponses = mcp.InputResponseMap{ + deleteRepositoryConfirmationID: confirmation, + } + result, err = handler(ContextWithDeps(context.Background(), deps), &retry) require.NoError(t, err) require.NotNil(t, result) return result diff --git a/pkg/github/scope_filter_test.go b/pkg/github/scope_filter_test.go index 669f1ce169..934a848a2d 100644 --- a/pkg/github/scope_filter_test.go +++ b/pkg/github/scope_filter_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -207,3 +208,19 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) { assert.Contains(t, toolNames, "repo_tool") assert.NotContains(t, toolNames, "gist_tool") } + +func TestCreateToolScopeFilterPreservesExistingMultiScopeSemantics(t *testing.T) { + filter := CreateToolScopeFilter([]string{"repo"}) + tools := []inventory.ServerTool{ + ListIssueFields(translations.NullTranslationHelper), + ListIssueTypes(translations.NullTranslationHelper), + UIGet(translations.NullTranslationHelper), + } + + for i := range tools { + allowed, err := filter(context.Background(), &tools[i]) + require.NoError(t, err) + assert.True(t, allowed, "%s should remain visible with a repo-only token", tools[i].Tool.Name) + assert.Empty(t, tools[i].RequiredScopeGroups, "%s should retain legacy any-of scope semantics", tools[i].Tool.Name) + } +} From cceb5cfbfd37088f1d38b689bfc704b7375e93f1 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 18 Aug 2026 13:51:41 +0200 Subject: [PATCH 07/11] fix(oauth): request repository deletion scope Include delete_repo in the supported OAuth scope set used by stdio login, HTTP protected-resource metadata, and tool filtering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- pkg/http/oauth/oauth.go | 1 + pkg/http/oauth/oauth_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/pkg/http/oauth/oauth.go b/pkg/http/oauth/oauth.go index f7ffe67e6b..606db14bdc 100644 --- a/pkg/http/oauth/oauth.go +++ b/pkg/http/oauth/oauth.go @@ -28,6 +28,7 @@ const ( // requirements when scopes change. var SupportedScopes = []string{ "repo", + "delete_repo", "read:org", "read:user", "user:email", diff --git a/pkg/http/oauth/oauth_test.go b/pkg/http/oauth/oauth_test.go index 1c2aa5c7c1..179566a9ba 100644 --- a/pkg/http/oauth/oauth_test.go +++ b/pkg/http/oauth/oauth_test.go @@ -569,6 +569,7 @@ func TestSupportedScopes(t *testing.T) { // Verify all expected scopes are present expectedScopes := []string{ "repo", + "delete_repo", "read:org", "read:user", "user:email", From ec7bd6ff24a7a4e503854cc0031846198a7eb619 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 18 Aug 2026 13:55:49 +0200 Subject: [PATCH 08/11] fix(oauth): require deletion scope opt-in Keep delete_repo in protected-resource discovery for step-up authorization while excluding it from the default stdio OAuth grant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- cmd/github-mcp-server/main.go | 8 ++++---- pkg/http/oauth/oauth.go | 27 ++++++++++++++++++++------- pkg/http/oauth/oauth_test.go | 6 ++++++ 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index fb61ed036c..3450aabe24 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -128,11 +128,11 @@ var ( } // When no static token is provided, log in via OAuth using the given - // client. The requested scopes default to the full supported set - // (which filters out no tools); an explicit, narrower --oauth-scopes - // both narrows the grant and hides tools needing other scopes. + // client. The requested scopes default to the standard set; high-risk + // scopes such as delete_repo require explicit --oauth-scopes opt-in. + // The requested set also filters tools needing other scopes. if token == "" && !appAuthRequested { - scopes := ghoauth.SupportedScopes + scopes := ghoauth.DefaultScopes if viper.IsSet("oauth-scopes") { if err := viper.UnmarshalKey("oauth-scopes", &scopes); err != nil { return fmt.Errorf("failed to unmarshal oauth-scopes: %w", err) diff --git a/pkg/http/oauth/oauth.go b/pkg/http/oauth/oauth.go index 606db14bdc..62ec45e41a 100644 --- a/pkg/http/oauth/oauth.go +++ b/pkg/http/oauth/oauth.go @@ -19,13 +19,26 @@ const ( OAuthProtectedResourcePrefix = "/.well-known/oauth-protected-resource" ) -// SupportedScopes lists every OAuth scope that an MCP tool may require. It is the -// source of truth in two places: HTTP mode advertises it as scopes_supported in -// the protected-resource metadata, and stdio OAuth login requests it by default -// and then filters the exposed tools to the granted scopes. A tool whose required -// scope is absent here is therefore hidden under default OAuth even though a PAT -// carrying that scope would expose it, so keep this list in sync with tool scope -// requirements when scopes change. +// DefaultScopes are requested by stdio OAuth unless the operator explicitly +// supplies --oauth-scopes. High-risk scopes such as delete_repo require opt-in. +var DefaultScopes = []string{ + "repo", + "read:org", + "read:user", + "user:email", + "read:packages", + "write:packages", + "read:project", + "project", + "gist", + "notifications", + "workflow", + "codespace", +} + +// SupportedScopes lists every OAuth scope that an MCP tool may require. HTTP +// protected-resource metadata advertises this full set so clients can step up +// authorization for tools excluded from the default grant. var SupportedScopes = []string{ "repo", "delete_repo", diff --git a/pkg/http/oauth/oauth_test.go b/pkg/http/oauth/oauth_test.go index 179566a9ba..a13f0d9cd2 100644 --- a/pkg/http/oauth/oauth_test.go +++ b/pkg/http/oauth/oauth_test.go @@ -586,6 +586,12 @@ func TestSupportedScopes(t *testing.T) { assert.Equal(t, expectedScopes, SupportedScopes) } +func TestDefaultScopesRequiresExplicitDeleteRepoOptIn(t *testing.T) { + assert.Contains(t, SupportedScopes, "delete_repo") + assert.NotContains(t, DefaultScopes, "delete_repo") + assert.Contains(t, DefaultScopes, "repo") +} + func TestProtectedResourceResponseFormat(t *testing.T) { t.Parallel() From f3a32d6f2e3000dc33a2bb275941a7c4ae7d9ee3 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 18 Aug 2026 14:07:13 +0200 Subject: [PATCH 09/11] refactor(oauth): derive scope sets from catalog Generate protected-resource supported scopes and the lower-risk default OAuth grant from one canonical scope definition list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- pkg/http/oauth/oauth.go | 62 +++++++++++++++++++----------------- pkg/http/oauth/oauth_test.go | 1 + 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/pkg/http/oauth/oauth.go b/pkg/http/oauth/oauth.go index 62ec45e41a..51cca87bbf 100644 --- a/pkg/http/oauth/oauth.go +++ b/pkg/http/oauth/oauth.go @@ -19,40 +19,44 @@ const ( OAuthProtectedResourcePrefix = "/.well-known/oauth-protected-resource" ) -// DefaultScopes are requested by stdio OAuth unless the operator explicitly -// supplies --oauth-scopes. High-risk scopes such as delete_repo require opt-in. -var DefaultScopes = []string{ - "repo", - "read:org", - "read:user", - "user:email", - "read:packages", - "write:packages", - "read:project", - "project", - "gist", - "notifications", - "workflow", - "codespace", +type scopeDefinition struct { + name string + byDefault bool +} + +var scopeDefinitions = []scopeDefinition{ + {name: "repo", byDefault: true}, + {name: "delete_repo"}, + {name: "read:org", byDefault: true}, + {name: "read:user", byDefault: true}, + {name: "user:email", byDefault: true}, + {name: "read:packages", byDefault: true}, + {name: "write:packages", byDefault: true}, + {name: "read:project", byDefault: true}, + {name: "project", byDefault: true}, + {name: "gist", byDefault: true}, + {name: "notifications", byDefault: true}, + {name: "workflow", byDefault: true}, + {name: "codespace", byDefault: true}, } // SupportedScopes lists every OAuth scope that an MCP tool may require. HTTP // protected-resource metadata advertises this full set so clients can step up // authorization for tools excluded from the default grant. -var SupportedScopes = []string{ - "repo", - "delete_repo", - "read:org", - "read:user", - "user:email", - "read:packages", - "write:packages", - "read:project", - "project", - "gist", - "notifications", - "workflow", - "codespace", +var SupportedScopes = scopesFromDefinitions(false) + +// DefaultScopes are requested by stdio OAuth unless the operator explicitly +// supplies --oauth-scopes. High-risk scopes such as delete_repo require opt-in. +var DefaultScopes = scopesFromDefinitions(true) + +func scopesFromDefinitions(defaultOnly bool) []string { + result := make([]string, 0, len(scopeDefinitions)) + for _, scope := range scopeDefinitions { + if !defaultOnly || scope.byDefault { + result = append(result, scope.name) + } + } + return result } // Config holds the OAuth configuration for the MCP server. diff --git a/pkg/http/oauth/oauth_test.go b/pkg/http/oauth/oauth_test.go index a13f0d9cd2..958b347076 100644 --- a/pkg/http/oauth/oauth_test.go +++ b/pkg/http/oauth/oauth_test.go @@ -587,6 +587,7 @@ func TestSupportedScopes(t *testing.T) { } func TestDefaultScopesRequiresExplicitDeleteRepoOptIn(t *testing.T) { + assert.Subset(t, SupportedScopes, DefaultScopes) assert.Contains(t, SupportedScopes, "delete_repo") assert.NotContains(t, DefaultScopes, "delete_repo") assert.Contains(t, DefaultScopes, "repo") From 1dd5f336e9d90d8de1f75bafdd2734c46602ed72 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 18 Aug 2026 14:13:09 +0200 Subject: [PATCH 10/11] refactor(scopes): own OAuth scope catalog Move supported and default OAuth scope policy into pkg/scopes so protected-resource metadata and stdio grants derive from the scope domain package. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- pkg/http/oauth/oauth.go | 36 +++--------------------------- pkg/scopes/scopes.go | 47 +++++++++++++++++++++++++++++++++++++++ pkg/scopes/scopes_test.go | 11 +++++++++ 3 files changed, 61 insertions(+), 33 deletions(-) diff --git a/pkg/http/oauth/oauth.go b/pkg/http/oauth/oauth.go index 51cca87bbf..e6a53ba80c 100644 --- a/pkg/http/oauth/oauth.go +++ b/pkg/http/oauth/oauth.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/github/github-mcp-server/pkg/http/headers" + "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/utils" "github.com/go-chi/chi/v5" "github.com/modelcontextprotocol/go-sdk/auth" @@ -19,45 +20,14 @@ const ( OAuthProtectedResourcePrefix = "/.well-known/oauth-protected-resource" ) -type scopeDefinition struct { - name string - byDefault bool -} - -var scopeDefinitions = []scopeDefinition{ - {name: "repo", byDefault: true}, - {name: "delete_repo"}, - {name: "read:org", byDefault: true}, - {name: "read:user", byDefault: true}, - {name: "user:email", byDefault: true}, - {name: "read:packages", byDefault: true}, - {name: "write:packages", byDefault: true}, - {name: "read:project", byDefault: true}, - {name: "project", byDefault: true}, - {name: "gist", byDefault: true}, - {name: "notifications", byDefault: true}, - {name: "workflow", byDefault: true}, - {name: "codespace", byDefault: true}, -} - // SupportedScopes lists every OAuth scope that an MCP tool may require. HTTP // protected-resource metadata advertises this full set so clients can step up // authorization for tools excluded from the default grant. -var SupportedScopes = scopesFromDefinitions(false) +var SupportedScopes = scopes.SupportedOAuthScopes() // DefaultScopes are requested by stdio OAuth unless the operator explicitly // supplies --oauth-scopes. High-risk scopes such as delete_repo require opt-in. -var DefaultScopes = scopesFromDefinitions(true) - -func scopesFromDefinitions(defaultOnly bool) []string { - result := make([]string, 0, len(scopeDefinitions)) - for _, scope := range scopeDefinitions { - if !defaultOnly || scope.byDefault { - result = append(result, scope.name) - } - } - return result -} +var DefaultScopes = scopes.DefaultOAuthScopes() // Config holds the OAuth configuration for the MCP server. type Config struct { diff --git a/pkg/scopes/scopes.go b/pkg/scopes/scopes.go index 4864f4e474..fc4b3fc20d 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -61,8 +61,55 @@ const ( // WritePackages grants write access to packages WritePackages Scope = "write:packages" + + // Workflow grants permission to update GitHub Actions workflow files + Workflow Scope = "workflow" + + // Codespace grants full control of codespaces + Codespace Scope = "codespace" ) +type oauthScopeDefinition struct { + scope Scope + byDefault bool +} + +var oauthScopeDefinitions = []oauthScopeDefinition{ + {scope: Repo, byDefault: true}, + {scope: DeleteRepo}, + {scope: ReadOrg, byDefault: true}, + {scope: ReadUser, byDefault: true}, + {scope: UserEmail, byDefault: true}, + {scope: ReadPackages, byDefault: true}, + {scope: WritePackages, byDefault: true}, + {scope: ReadProject, byDefault: true}, + {scope: Project, byDefault: true}, + {scope: Gist, byDefault: true}, + {scope: Notifications, byDefault: true}, + {scope: Workflow, byDefault: true}, + {scope: Codespace, byDefault: true}, +} + +// SupportedOAuthScopes returns every OAuth scope the server may request. +func SupportedOAuthScopes() []string { + return oauthScopes(false) +} + +// DefaultOAuthScopes returns the lower-risk scopes requested by default. +func DefaultOAuthScopes() []string { + return oauthScopes(true) +} + +func oauthScopes(defaultOnly bool) []string { + result := make([]string, 0, len(oauthScopeDefinitions)) + for _, definition := range oauthScopeDefinitions { + if !defaultOnly || definition.byDefault { + result = append(result, string(definition.scope)) + } + } + return result +} + // ScopeHierarchy defines parent-child relationships between scopes. // A parent scope implicitly grants access to all child scopes. // For example, "repo" grants access to "public_repo" and "security_events". diff --git a/pkg/scopes/scopes_test.go b/pkg/scopes/scopes_test.go index d938504817..2f4cdeb3b8 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -120,6 +120,17 @@ func TestHasRequiredScopeGroups(t *testing.T) { assert.False(t, HasRequiredScopeGroups([]string{"repo"}, groups)) } +func TestOAuthScopeCatalog(t *testing.T) { + supported := SupportedOAuthScopes() + defaults := DefaultOAuthScopes() + + assert.Subset(t, supported, defaults) + assert.Contains(t, supported, string(DeleteRepo)) + assert.NotContains(t, defaults, string(DeleteRepo)) + assert.Contains(t, defaults, string(Workflow)) + assert.Contains(t, defaults, string(Codespace)) +} + func TestToStringSlice(t *testing.T) { tests := []struct { name string From 213d53adf4048c2a10971f03d352827eea16edbf Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 18 Aug 2026 14:23:48 +0200 Subject: [PATCH 11/11] fix(scopes): require workflow scope opt-in Keep workflow and codespace in protected-resource discovery while excluding both from the default OAuth grant alongside delete_repo. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8 --- pkg/scopes/scopes.go | 4 ++-- pkg/scopes/scopes_test.go | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/scopes/scopes.go b/pkg/scopes/scopes.go index fc4b3fc20d..05a7774cd0 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -86,8 +86,8 @@ var oauthScopeDefinitions = []oauthScopeDefinition{ {scope: Project, byDefault: true}, {scope: Gist, byDefault: true}, {scope: Notifications, byDefault: true}, - {scope: Workflow, byDefault: true}, - {scope: Codespace, byDefault: true}, + {scope: Workflow}, + {scope: Codespace}, } // SupportedOAuthScopes returns every OAuth scope the server may request. diff --git a/pkg/scopes/scopes_test.go b/pkg/scopes/scopes_test.go index 2f4cdeb3b8..bf5269da17 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -127,8 +127,10 @@ func TestOAuthScopeCatalog(t *testing.T) { assert.Subset(t, supported, defaults) assert.Contains(t, supported, string(DeleteRepo)) assert.NotContains(t, defaults, string(DeleteRepo)) - assert.Contains(t, defaults, string(Workflow)) - assert.Contains(t, defaults, string(Codespace)) + assert.Contains(t, supported, string(Workflow)) + assert.NotContains(t, defaults, string(Workflow)) + assert.Contains(t, supported, string(Codespace)) + assert.NotContains(t, defaults, string(Codespace)) } func TestToStringSlice(t *testing.T) {