diff --git a/README.md b/README.md index 32f8eb82bc..5699aaf34f 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 (all required)**: `delete_repo`, `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/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.go b/cmd/github-mcp-server/main.go index 7671706b57..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) @@ -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/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/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/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/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 new file mode 100644 index 0000000000..cff3bfea7f --- /dev/null +++ b/internal/requeststate/sealer.go @@ -0,0 +1,81 @@ +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 +} + +// 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) + 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)) + } + 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) + } + 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..9f244c9116 --- /dev/null +++ b/internal/requeststate/sealer_test.go @@ -0,0 +1,68 @@ +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 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 + 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/__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/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/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..25be496457 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" @@ -704,6 +705,199 @@ func CreateRepository(t translations.TranslationHelperFunc) inventory.ServerTool ) } +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"` + RepositoryID int64 `json:"repository_id"` + ExpiresAt int64 `json:"expires_at"` +} + +// 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: 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"), + 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, 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 { + 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 + sealer := requestStateSealerFromDeps(deps) + 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 { + 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{ + 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}, + }, + }, + }, + RequestState: requestState, + }, nil, 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 + } + 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) + 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) + } + 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, + 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 + tool.RequiredElicitationMode = inventory.ElicitationModeForm + tool.RequiredScopeGroups = scopes.ExpandScopeGroups(scopes.DeleteRepo, scopes.Repo) + 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 332b212a17..1d488b9ced 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -11,8 +11,11 @@ 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" + "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 +2987,442 @@ 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, inventory.ElicitationModeForm, serverTool.RequiredElicitationMode) + 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) { + 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) + 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( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), + 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("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) + client := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), + 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 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) + + 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("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( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), + WithRequestMatchHandler( + DeleteReposByOwnerByRepo, + mockResponse(t, http.StatusNoContent, nil), + ), + ) + sealer, err := requeststate.NewRandom() + require.NoError(t, err) + deps := BaseDeps{ + Client: mustNewGHClient(t, httpClient), + StateSealer: sealer, + } + + 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) { + 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", + }, + }) + + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "did not match") + }) + + t.Run("refuses declined confirmation", func(t *testing.T) { + client := NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), + ) + result := invokeDeleteRepository(t, serverTool, client, &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( + GetReposByOwnerByRepo, + mockResponse(t, http.StatusOK, map[string]any{"id": 123}), + ), + 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() + + sealer, err := requeststate.NewRandom() + require.NoError(t, err) + deps := BaseDeps{ + Client: mustNewGHClient(t, httpClient), + StateSealer: sealer, + } + handler := tool.Handler(deps) + firstRequest := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &firstRequest) + require.NoError(t, err) + require.NotNil(t, result) + if confirmation == nil { + return result + } + + 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 +} + func Test_ListBranches(t *testing.T) { // Verify tool definition once serverTool := ListBranches(translations.NullTranslationHelper) 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/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..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" @@ -58,6 +59,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 +137,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 { @@ -189,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) + } +} 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.go b/pkg/http/handler.go index b8a063ea85..b9dfccf09f 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" @@ -358,12 +359,24 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun hostType = utils.HostTypeDotcom } opts := []github.ToolOption{github.WithHost(hostType)} + tools := github.AllTools(t, opts...) + 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 github.AllTools(t, opts...), github.AllResources(t), github.AllPrompts(t) + return filterUnavailable(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)) @@ -377,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 github.AllTools(t, opts...), 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 cc043f24c9..c1ee327952 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -48,6 +48,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), @@ -641,6 +642,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 +663,28 @@ 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) + + assert.Empty(t, tools, "an unavailable explicit allowlist must not widen to other tools") +} + // 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 @@ -908,6 +932,117 @@ 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 + elicitationCapabilities map[string]any + wantDeleteRepoTool bool + }{ + { + name: "current protocol with form elicitation includes delete repository", + protocolVersion: inventory.ProtocolVersionMultiRoundTrip, + elicitationCapabilities: map[string]any{"form": map[string]any{}}, + wantDeleteRepoTool: true, + }, + { + 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) { + 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.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") + } + }) + } +} + func TestSubscriptionsListenIsRejected(t *testing.T) { apiHost, err := utils.NewAPIHost("https://api.githubcopilot.com") require.NoError(t, err) diff --git a/pkg/http/oauth/oauth.go b/pkg/http/oauth/oauth.go index f7ffe67e6b..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,27 +20,14 @@ 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. -var SupportedScopes = []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 = 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 = scopes.DefaultOAuthScopes() // Config holds the OAuth configuration for the MCP server. type Config struct { diff --git a/pkg/http/oauth/oauth_test.go b/pkg/http/oauth/oauth_test.go index 1c2aa5c7c1..958b347076 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", @@ -585,6 +586,13 @@ func TestSupportedScopes(t *testing.T) { assert.Equal(t, expectedScopes, SupportedScopes) } +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") +} + func TestProtectedResourceResponseFormat(t *testing.T) { t.Parallel() 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() diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 915ed0aa1c..3483d448cb 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) + 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 44a062ba2e..d25458253f 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -81,6 +81,14 @@ 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 + + // 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 @@ -89,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. @@ -119,6 +131,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, + } +} 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 cb1b7681a7..05a7774cd0 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" @@ -58,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}, + {scope: Codespace}, +} + +// 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". @@ -150,6 +200,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", @@ -193,3 +254,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 b8e0d8e421..bf5269da17 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}, @@ -107,6 +112,27 @@ 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 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, 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) { tests := []struct { name string