diff --git a/pkg/github/header_params_test.go b/pkg/github/header_params_test.go index ed08f27a79..cbd0771ca4 100644 --- a/pkg/github/header_params_test.go +++ b/pkg/github/header_params_test.go @@ -29,6 +29,10 @@ func TestAllToolsRoutingParamsGetHeaders(t *testing.T) { if !ok || schema == nil { continue } + if pathSchema := schema.Properties["path"]; pathSchema != nil { + require.NotContainsf(t, pathSchema.Extra, "x-mcp-header", + "tool %q path must remain in MCP arguments", tool.Name) + } for prop, header := range inventory.HeaderParams { ps, ok := schema.Properties[prop] if !ok || ps == nil { diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 25be496457..af928672bc 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -404,7 +404,7 @@ func ListBranches(t translations.TranslationHelperFunc) inventory.ServerTool { // CreateOrUpdateFile creates a tool to create or update a file in a GitHub repository. func CreateOrUpdateFile(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( + tool := NewTool( ToolsetMetadataRepos, mcp.Tool{ Name: "create_or_update_file", @@ -469,6 +469,10 @@ SHA MUST be provided for existing file updates. if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + path, err = validateRelativePath(path) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("invalid path: %s", err)), nil, nil + } content, err := RequiredParam[string](args, "content") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -507,8 +511,6 @@ SHA MUST be provided for existing file updates. return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } - path = strings.TrimPrefix(path, "/") - // SHA validation using Contents API to fetch current file metadata (blob SHA) getOpts := &github.RepositoryContentGetOptions{Ref: branch} @@ -596,6 +598,8 @@ SHA MUST be provided for existing file updates. return MarshalledTextResult(minimalResponse), nil, nil }, ) + tool.ScopeResolver = workflowScopeForPath + return tool } // CreateRepository creates a tool to create a new GitHub repository. @@ -1244,7 +1248,7 @@ func ForkRepository(t translations.TranslationHelperFunc) inventory.ServerTool { // The approach implemented here gets automatic commit signing when used with either the github-actions user or as an app, // both of which suit an LLM well. func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( + tool := NewTool( ToolsetMetadataRepos, mcp.Tool{ Name: "delete_file", @@ -1295,6 +1299,10 @@ func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + path, err = validateRelativePath(path) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("invalid path: %s", err)), nil, nil + } message, err := RequiredParam[string](args, "message") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -1425,6 +1433,8 @@ func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultText(string(r)), nil, nil }, ) + tool.ScopeResolver = workflowScopeForPath + return tool } // CreateBranch creates a tool to create a new branch. @@ -1542,7 +1552,7 @@ func CreateBranch(t translations.TranslationHelperFunc) inventory.ServerTool { // PushFiles creates a tool to push multiple files in a single commit to a GitHub repository. func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( + tool := NewTool( ToolsetMetadataRepos, mcp.Tool{ Name: "push_files", @@ -1618,6 +1628,35 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError("files parameter must be an array of objects with path and content"), nil, nil } + entries := make([]*github.TreeEntry, 0, len(filesObj)) + for _, file := range filesObj { + fileMap, ok := file.(map[string]any) + if !ok { + return utils.NewToolResultError("each file must be an object with path and content"), nil, nil + } + + filePath, ok := fileMap["path"].(string) + if !ok || filePath == "" { + return utils.NewToolResultError("each file must have a path"), nil, nil + } + filePath, err = validateRelativePath(filePath) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("invalid file path: %s", err)), nil, nil + } + + content, ok := fileMap["content"].(string) + if !ok { + return utils.NewToolResultError("each file must have content"), nil, nil + } + + entries = append(entries, &github.TreeEntry{ + Path: github.Ptr(filePath), + Mode: github.Ptr("100644"), + Type: github.Ptr("blob"), + Content: github.Ptr(content), + }) + } + client, err := deps.GetClient(ctx) if err != nil { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) @@ -1691,34 +1730,6 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { baseCommit = base } - // Create tree entries for all files (or remaining files if empty repo) - var entries []*github.TreeEntry - - for _, file := range filesObj { - fileMap, ok := file.(map[string]any) - if !ok { - return utils.NewToolResultError("each file must be an object with path and content"), nil, nil - } - - path, ok := fileMap["path"].(string) - if !ok || path == "" { - return utils.NewToolResultError("each file must have a path"), nil, nil - } - - content, ok := fileMap["content"].(string) - if !ok { - return utils.NewToolResultError("each file must have content"), nil, nil - } - - // Create a tree entry for the file - entries = append(entries, &github.TreeEntry{ - Path: github.Ptr(path), - Mode: github.Ptr("100644"), // Regular file mode - Type: github.Ptr("blob"), - Content: github.Ptr(content), - }) - } - // Create a new tree with the file entries (baseCommit is now guaranteed to exist) newTree, resp, err := client.Git.CreateTree(ctx, owner, repo, *baseCommit.Tree.SHA, entries) if err != nil { @@ -1773,6 +1784,8 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultText(string(r)), nil, nil }, ) + tool.ScopeResolver = workflowScopeForFiles + return tool } // ListTags creates a tool to list tags in a GitHub repository. diff --git a/pkg/github/repository_path.go b/pkg/github/repository_path.go new file mode 100644 index 0000000000..1d0aaff659 --- /dev/null +++ b/pkg/github/repository_path.go @@ -0,0 +1,66 @@ +package github + +import ( + "fmt" + "path" + "slices" + "strings" + + "github.com/github/github-mcp-server/pkg/scopes" +) + +const workflowPathPrefix = ".github/workflows/" + +func validateRelativePath(value string) (string, error) { + if value == "" { + return "", fmt.Errorf("path must not be empty") + } + if path.IsAbs(value) { + return "", fmt.Errorf("path must be relative") + } + if strings.Contains(value, `\`) { + return "", fmt.Errorf("path must use forward slashes") + } + if slices.Contains(strings.Split(value, "/"), "..") { + return "", fmt.Errorf("path must not contain parent directory traversal") + } + + cleaned := path.Clean(value) + if cleaned == "." { + return "", fmt.Errorf("path must identify a file") + } + return cleaned, nil +} + +func isWorkflowPath(value string) bool { + return strings.HasPrefix(value, workflowPathPrefix) && len(value) > len(workflowPathPrefix) +} + +func workflowScopeForPath(arguments map[string]any) []string { + value, ok := arguments["path"].(string) + if !ok { + return nil + } + cleaned, err := validateRelativePath(value) + if err != nil || !isWorkflowPath(cleaned) { + return nil + } + return []string{string(scopes.Workflow)} +} + +func workflowScopeForFiles(arguments map[string]any) []string { + files, ok := arguments["files"].([]any) + if !ok { + return nil + } + for _, file := range files { + fileMap, ok := file.(map[string]any) + if !ok { + continue + } + if len(workflowScopeForPath(fileMap)) > 0 { + return []string{string(scopes.Workflow)} + } + } + return nil +} diff --git a/pkg/github/repository_path_test.go b/pkg/github/repository_path_test.go new file mode 100644 index 0000000000..64fc9d3005 --- /dev/null +++ b/pkg/github/repository_path_test.go @@ -0,0 +1,140 @@ +package github + +import ( + "context" + "testing" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateRelativePath(t *testing.T) { + tests := []struct { + name string + value string + want string + wantErr string + }{ + {name: "file", value: "docs/readme.md", want: "docs/readme.md"}, + {name: "normalizes dot segment", value: "./.github/workflows/ci.yml", want: ".github/workflows/ci.yml"}, + {name: "normalizes duplicate separator", value: ".github//workflows/ci.yml", want: ".github/workflows/ci.yml"}, + {name: "empty", value: "", wantErr: "must not be empty"}, + {name: "current directory", value: ".", wantErr: "must identify a file"}, + {name: "absolute", value: "/.github/workflows/ci.yml", wantErr: "must be relative"}, + {name: "parent traversal", value: "docs/../.github/workflows/ci.yml", wantErr: "parent directory traversal"}, + {name: "leading traversal", value: "../.github/workflows/ci.yml", wantErr: "parent directory traversal"}, + {name: "backslash traversal", value: `docs\..\.github\workflows\ci.yml`, wantErr: "forward slashes"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := validateRelativePath(tt.value) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestFileWriteWorkflowScopeResolvers(t *testing.T) { + tests := []struct { + name string + tool inventory.ServerTool + args map[string]any + want []string + }{ + { + name: "create regular file", + tool: CreateOrUpdateFile(translations.NullTranslationHelper), + args: map[string]any{"path": "docs/readme.md"}, + }, + { + name: "create workflow", + tool: CreateOrUpdateFile(translations.NullTranslationHelper), + args: map[string]any{"path": ".github/workflows/ci.yml"}, + want: []string{"workflow"}, + }, + { + name: "delete normalized workflow", + tool: DeleteFile(translations.NullTranslationHelper), + args: map[string]any{"path": "./.github/workflows/ci.yml"}, + want: []string{"workflow"}, + }, + { + name: "reject traversal instead of resolving it", + tool: DeleteFile(translations.NullTranslationHelper), + args: map[string]any{"path": "docs/../.github/workflows/ci.yml"}, + }, + { + name: "push regular files", + tool: PushFiles(translations.NullTranslationHelper), + args: map[string]any{"files": []any{map[string]any{"path": "README.md"}}}, + }, + { + name: "push includes workflow", + tool: PushFiles(translations.NullTranslationHelper), + args: map[string]any{"files": []any{ + map[string]any{"path": "README.md"}, + map[string]any{"path": ".github/workflows/ci.yml"}, + }}, + want: []string{"workflow"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.NotNil(t, tt.tool.ScopeResolver) + assert.Equal(t, tt.want, tt.tool.ScopeResolver(tt.args)) + }) + } +} + +func TestFileWriteToolsRejectUnsafePathsBeforeAPICalls(t *testing.T) { + tests := []struct { + name string + tool inventory.ServerTool + args map[string]any + }{ + { + name: "create or update", + tool: CreateOrUpdateFile(translations.NullTranslationHelper), + args: map[string]any{ + "owner": "owner", "repo": "repo", "path": "../workflow.yml", + "content": "content", "message": "message", "branch": "main", + }, + }, + { + name: "delete", + tool: DeleteFile(translations.NullTranslationHelper), + args: map[string]any{ + "owner": "owner", "repo": "repo", "path": "/.github/workflows/ci.yml", + "message": "message", "branch": "main", + }, + }, + { + name: "push", + tool: PushFiles(translations.NullTranslationHelper), + args: map[string]any{ + "owner": "owner", "repo": "repo", "branch": "main", "message": "message", + "files": []any{map[string]any{"path": `..\workflow.yml`, "content": "content"}}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + deps := BaseDeps{} + request := createMCPRequest(tt.args) + result, err := tt.tool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "path") + }) + } +} diff --git a/pkg/http/middleware/scope_challenge.go b/pkg/http/middleware/scope_challenge.go index 1a86bf93ce..6f3c32ee2a 100644 --- a/pkg/http/middleware/scope_challenge.go +++ b/pkg/http/middleware/scope_challenge.go @@ -43,6 +43,7 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter // Try to use pre-parsed MCP method info first (performance optimization) // This avoids re-parsing the JSON body if WithMCPParse middleware ran earlier var toolName string + var arguments map[string]any if methodInfo, ok := ghcontext.MCPMethod(ctx); ok && methodInfo != nil { // Only check tools/call requests if methodInfo.Method != "tools/call" { @@ -50,6 +51,7 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter return } toolName = methodInfo.ItemName + arguments = methodInfo.Arguments } else { // Fallback: parse the request body directly body, err := io.ReadAll(r.Body) @@ -81,6 +83,7 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter } toolName = mcpRequest.Params.Name + arguments = mcpRequest.Params.Arguments } toolScopeInfo, err := scopes.GetToolScopeInfo(toolName) if err != nil { @@ -93,6 +96,7 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter next.ServeHTTP(w, r) return } + toolScopeInfo = toolScopeInfo.Resolve(arguments) // Get OAuth scopes for Token. First check if scopes are already in context, then fetch from GitHub if not present. // This allows Remote Server to pass scope info to avoid redundant GitHub API calls. @@ -116,7 +120,7 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter } // User lacks required scopes - get the scopes they need - requiredScopes := toolScopeInfo.GetRequiredScopesSlice() + requiredScopes := toolScopeInfo.MissingScopes(activeScopes...) // Build the resource metadata URL using the shared utility // GetEffectiveResourcePath returns the original path (e.g., /mcp or /mcp/x/all) diff --git a/pkg/http/middleware/scope_challenge_test.go b/pkg/http/middleware/scope_challenge_test.go new file mode 100644 index 0000000000..598a18a9c2 --- /dev/null +++ b/pkg/http/middleware/scope_challenge_test.go @@ -0,0 +1,136 @@ +package middleware + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + ghcontext "github.com/github/github-mcp-server/pkg/context" + "github.com/github/github-mcp-server/pkg/http/oauth" + "github.com/github/github-mcp-server/pkg/scopes" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/stretchr/testify/assert" +) + +func TestWithScopeChallengeResolvesScopesFromParsedArguments(t *testing.T) { + setDynamicScopeTestMap(t) + + tests := []struct { + name string + arguments map[string]any + wantStatus int + wantNext bool + }{ + { + name: "regular file only requires repo", + arguments: map[string]any{"path": "README.md"}, + wantStatus: http.StatusNoContent, + wantNext: true, + }, + { + name: "non-ASCII workflow path without header requires workflow", + arguments: map[string]any{"path": ".github/workflows/构建.yml"}, + wantStatus: http.StatusForbidden, + }, + { + name: "workflow in array requires workflow", + arguments: map[string]any{"files": []any{ + map[string]any{"path": "README.md"}, + map[string]any{"path": ".github/workflows/ci.yml"}, + }}, + wantStatus: http.StatusForbidden, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + nextCalled := false + next := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + nextCalled = true + w.WriteHeader(http.StatusNoContent) + }) + handler := WithScopeChallenge(&oauth.Config{}, &mockScopeFetcher{})(next) + + request := httptest.NewRequest(http.MethodPost, "/mcp", nil) + assert.Empty(t, request.Header.Get("Mcp-Param-path")) + ctx := scopeChallengeContext(request.Context()) + ctx = ghcontext.WithMCPMethodInfo(ctx, &ghcontext.MCPMethodInfo{ + Method: "tools/call", + ItemName: "write_file", + Arguments: tt.arguments, + }) + request = request.WithContext(ctx) + + response := httptest.NewRecorder() + handler.ServeHTTP(response, request) + + assert.Equal(t, tt.wantStatus, response.Code) + assert.Equal(t, tt.wantNext, nextCalled) + if tt.wantStatus == http.StatusForbidden { + challenge := response.Header().Get("WWW-Authenticate") + assert.Contains(t, challenge, `scope="repo workflow"`) + assert.Contains(t, challenge, "Additional scopes required: workflow") + } + }) + } +} + +func TestWithScopeChallengeResolvesScopesFromFallbackBody(t *testing.T) { + setDynamicScopeTestMap(t) + + nextCalled := false + next := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + nextCalled = true + w.WriteHeader(http.StatusNoContent) + }) + handler := WithScopeChallenge(&oauth.Config{}, &mockScopeFetcher{})(next) + + body := []byte(`{"jsonrpc":"2.0","method":"tools/call","params":{"name":"write_file","arguments":{"path":".github/workflows/ci.yml"}}}`) + request := httptest.NewRequest(http.MethodPost, "/mcp", bytes.NewReader(body)) + request = request.WithContext(scopeChallengeContext(request.Context())) + + response := httptest.NewRecorder() + handler.ServeHTTP(response, request) + + assert.Equal(t, http.StatusForbidden, response.Code) + assert.False(t, nextCalled) + assert.Contains(t, response.Header().Get("WWW-Authenticate"), "Additional scopes required: workflow") +} + +func setDynamicScopeTestMap(t *testing.T) { + t.Helper() + scopes.SetGlobalToolScopeMap(scopes.ToolScopeMap{ + "write_file": { + RequiredScopes: []string{"repo"}, + AcceptedScopes: []string{"repo"}, + ScopeResolver: func(arguments map[string]any) []string { + if path, _ := arguments["path"].(string); strings.HasPrefix(path, ".github/workflows/") { + return []string{"workflow"} + } + files, _ := arguments["files"].([]any) + for _, file := range files { + fileMap, _ := file.(map[string]any) + if path, _ := fileMap["path"].(string); strings.HasPrefix(path, ".github/workflows/") { + return []string{"workflow"} + } + } + return nil + }, + }, + }) + t.Cleanup(func() { + scopes.SetGlobalToolScopeMap(nil) + }) +} + +func scopeChallengeContext(ctx context.Context) context.Context { + ctx = ghcontext.WithTokenInfo(ctx, &ghcontext.TokenInfo{ + Token: "oauth-token", + TokenType: utils.TokenTypeOAuthAccessToken, + }) + ctx = ghcontext.WithTokenScopes(ctx, []string{"repo"}) + return ctx +} diff --git a/pkg/http/oauth/oauth_test.go b/pkg/http/oauth/oauth_test.go index 958b347076..8dd1c6e146 100644 --- a/pkg/http/oauth/oauth_test.go +++ b/pkg/http/oauth/oauth_test.go @@ -580,7 +580,6 @@ func TestSupportedScopes(t *testing.T) { "gist", "notifications", "workflow", - "codespace", } assert.Equal(t, expectedScopes, SupportedScopes) diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index d25458253f..51f3c176fb 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -18,6 +18,9 @@ import ( // should define their own typed dependencies struct and type-assert as needed. type HandlerFunc func(deps any) mcp.ToolHandler +// ScopeResolver returns additional OAuth scopes required for a specific call. +type ScopeResolver func(arguments map[string]any) []string + // ToolHandlerMiddleware wraps an MCP tool handler. Middleware is applied from // right to left, so the first middleware passed to RegisterFunc executes first. type ToolHandlerMiddleware func(next mcp.ToolHandler) mcp.ToolHandler @@ -101,6 +104,10 @@ type ServerTool struct { // RequiredScopeGroups contains one group of accepted alternatives for each // independently required OAuth scope. Every group must be satisfied. RequiredScopeGroups [][]string + + // ScopeResolver returns scopes that are conditionally required based on the + // tool call arguments. + ScopeResolver ScopeResolver } // IsReadOnly returns true if this tool is marked as read-only via annotations. @@ -138,21 +145,20 @@ func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any, middleware ...ToolHa if len(toolCopy.Icons) == 0 { toolCopy.Icons = st.Toolset.Icons() } - // Project routing-relevant params to standard MCP-Param-* headers (SEP-2243) - // so a remote proxy can read owner/repo from headers instead of re-parsing the - // JSON-RPC body. No-op for tools without these params. + // Project owner/repo routing params to standard MCP-Param-* headers (SEP-2243) + // so a remote proxy can route requests without re-parsing the JSON-RPC body. + // No-op for tools without these params. AnnotateHeaderParams(&toolCopy) s.AddTool(&toolCopy, handler) } -// HeaderParams maps tool input properties to the MCP-Param-* header name a -// header-aware proxy reads, avoiding a second parse of the request body. New -// routing-relevant params should be added here so projection stays automatic -// for every tool; the enforcement test in pkg/github guards full coverage. +// HeaderParams maps owner/repo input properties to the MCP-Param-* headers a +// header-aware proxy reads for repository routing. The enforcement test in +// pkg/github guards full coverage. var HeaderParams = map[string]string{"owner": "owner", "repo": "repo"} -// AnnotateHeaderParams returns a copy of tool whose routing-relevant input -// properties (per HeaderParams) carry an "x-mcp-header" annotation, which the +// AnnotateHeaderParams returns a copy of tool whose owner/repo input properties +// carry an "x-mcp-header" annotation, which the // SDK projects onto Mcp-Param-{name} request headers. It never mutates the // input tool's schema or any map shared with the original tool definition: // callers shallow-copy ServerTool.Tool, so the *jsonschema.Schema (and its diff --git a/pkg/inventory/server_tool_test.go b/pkg/inventory/server_tool_test.go index c6d2a6fdd8..8a865bc6d1 100644 --- a/pkg/inventory/server_tool_test.go +++ b/pkg/inventory/server_tool_test.go @@ -132,6 +132,7 @@ func TestAnnotateHeaderParams(t *testing.T) { Properties: map[string]*jsonschema.Schema{ "owner": {Type: "string"}, "repo": {Type: "string"}, + "path": {Type: "string"}, "detail": {Type: "string"}, }, }} @@ -139,6 +140,7 @@ func TestAnnotateHeaderParams(t *testing.T) { schema := tool.InputSchema.(*jsonschema.Schema) assert.Equal(t, "owner", schema.Properties["owner"].Extra["x-mcp-header"]) assert.Equal(t, "repo", schema.Properties["repo"].Extra["x-mcp-header"]) + assert.Nil(t, schema.Properties["path"].Extra) assert.Nil(t, schema.Properties["detail"].Extra) // No-op for tools without owner/repo and when InputSchema is not a *jsonschema.Schema diff --git a/pkg/scopes/map.go b/pkg/scopes/map.go index e13345f0f7..ea649a33d1 100644 --- a/pkg/scopes/map.go +++ b/pkg/scopes/map.go @@ -1,6 +1,10 @@ package scopes -import "github.com/github/github-mcp-server/pkg/inventory" +import ( + "slices" + + "github.com/github/github-mcp-server/pkg/inventory" +) // ToolScopeMap maps tool names to their scope requirements. type ToolScopeMap map[string]*ToolScopeInfo @@ -16,6 +20,9 @@ type ToolScopeInfo struct { // RequiredScopeGroups contains accepted alternatives for each independently // required scope. Every group must be satisfied. RequiredScopeGroups [][]string + + // ScopeResolver returns scopes that are conditionally required for a call. + ScopeResolver inventory.ScopeResolver } // globalToolScopeMap is populated from inventory when SetToolScopeMapFromInventory is called @@ -61,11 +68,12 @@ func GetToolScopeMapFromInventory(inv *inventory.Inventory) ToolScopeMap { allTools := inv.AllTools() for i := range allTools { tool := &allTools[i] - if len(tool.RequiredScopes) > 0 || len(tool.AcceptedScopes) > 0 { + if len(tool.RequiredScopes) > 0 || len(tool.AcceptedScopes) > 0 || tool.ScopeResolver != nil { result[tool.Tool.Name] = &ToolScopeInfo{ RequiredScopes: tool.RequiredScopes, AcceptedScopes: tool.AcceptedScopes, RequiredScopeGroups: tool.RequiredScopeGroups, + ScopeResolver: tool.ScopeResolver, } } } @@ -73,6 +81,39 @@ func GetToolScopeMapFromInventory(inv *inventory.Inventory) ToolScopeMap { return result } +// Resolve returns the scope requirements for a specific call. +func (t *ToolScopeInfo) Resolve(arguments map[string]any) *ToolScopeInfo { + if t == nil || t.ScopeResolver == nil { + return t + } + additionalScopes := t.ScopeResolver(arguments) + if len(additionalScopes) == 0 { + return t + } + + resolved := &ToolScopeInfo{ + RequiredScopes: append([]string(nil), t.RequiredScopes...), + AcceptedScopes: append([]string(nil), t.AcceptedScopes...), + RequiredScopeGroups: append([][]string(nil), t.RequiredScopeGroups...), + ScopeResolver: t.ScopeResolver, + } + if len(resolved.RequiredScopeGroups) == 0 { + for _, required := range resolved.RequiredScopes { + resolved.RequiredScopeGroups = append(resolved.RequiredScopeGroups, ExpandScopes(Scope(required))) + } + } + for _, required := range additionalScopes { + if slices.Contains(resolved.RequiredScopes, required) { + continue + } + resolved.RequiredScopes = append(resolved.RequiredScopes, required) + accepted := ExpandScopes(Scope(required)) + resolved.AcceptedScopes = append(resolved.AcceptedScopes, accepted...) + resolved.RequiredScopeGroups = append(resolved.RequiredScopeGroups, accepted) + } + return resolved +} + // 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 { diff --git a/pkg/scopes/map_test.go b/pkg/scopes/map_test.go index 3c5a7ede72..23a562d2ff 100644 --- a/pkg/scopes/map_test.go +++ b/pkg/scopes/map_test.go @@ -223,3 +223,29 @@ func TestToolScopeInfo_MissingScopes(t *testing.T) { }) } } + +func TestToolScopeInfo_Resolve(t *testing.T) { + base := &ToolScopeInfo{ + RequiredScopes: []string{"repo"}, + AcceptedScopes: []string{"repo"}, + ScopeResolver: func(arguments map[string]any) []string { + if arguments["workflow"] == true { + return []string{"workflow"} + } + return nil + }, + } + + resolved := base.Resolve(map[string]any{"workflow": true}) + require.NotSame(t, base, resolved) + assert.Equal(t, []string{"repo", "workflow"}, resolved.RequiredScopes) + assert.Equal(t, [][]string{{"repo"}, {"workflow"}}, resolved.RequiredScopeGroups) + assert.True(t, resolved.HasAcceptedScope("repo", "workflow")) + assert.False(t, resolved.HasAcceptedScope("repo")) + assert.False(t, resolved.HasAcceptedScope("workflow")) + assert.Equal(t, []string{"workflow"}, resolved.MissingScopes("repo")) + + assert.Equal(t, []string{"repo"}, base.RequiredScopes) + assert.Empty(t, base.RequiredScopeGroups) + assert.Same(t, base, base.Resolve(map[string]any{"workflow": false})) +} diff --git a/pkg/scopes/scopes.go b/pkg/scopes/scopes.go index 05a7774cd0..5e9c7813b6 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -64,9 +64,6 @@ const ( // Workflow grants permission to update GitHub Actions workflow files Workflow Scope = "workflow" - - // Codespace grants full control of codespaces - Codespace Scope = "codespace" ) type oauthScopeDefinition struct { @@ -87,7 +84,6 @@ var oauthScopeDefinitions = []oauthScopeDefinition{ {scope: Gist, byDefault: true}, {scope: Notifications, 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 bf5269da17..4cdf916f2b 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -129,8 +129,6 @@ func TestOAuthScopeCatalog(t *testing.T) { 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) {