From 1850911cc4f360247f3cc1a0e470cb5af2caaa16 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Fri, 21 Aug 2026 02:15:32 +0200 Subject: [PATCH] feat(auth): add per-call OAuth scope checks Let each tool define fixed-token visibility and return the exact scopes for an OAuth challenge based on the current call. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26e41558-43f9-42b2-8569-8489957c2b0a --- README.md | 185 +++++++------- cmd/github-mcp-server/generate_docs.go | 40 +-- cmd/github-mcp-server/list_scopes.go | 47 ++-- cmd/github-mcp-server/main_test.go | 38 +-- docs/feature-flags.md | 69 +++--- docs/insiders-features.md | 15 +- docs/scope-filtering.md | 11 +- pkg/github/actions.go | 8 +- pkg/github/code_quality.go | 2 +- pkg/github/code_scanning.go | 4 +- pkg/github/context_tools.go | 6 +- pkg/github/copilot.go | 6 +- pkg/github/dependabot.go | 4 +- pkg/github/dependencies.go | 18 +- pkg/github/discussions.go | 10 +- pkg/github/feature_flags_test.go | 2 +- pkg/github/find_duplicate.go | 2 +- pkg/github/find_duplicate_test.go | 4 +- pkg/github/gists.go | 8 +- pkg/github/git.go | 2 +- pkg/github/header_params_test.go | 4 + pkg/github/issue_dependencies.go | 4 +- pkg/github/issue_fields.go | 3 +- pkg/github/issue_fields_test.go | 5 +- pkg/github/issues.go | 17 +- pkg/github/issues_granular.go | 24 +- pkg/github/labels.go | 6 +- pkg/github/notifications.go | 12 +- pkg/github/projects.go | 6 +- pkg/github/pullrequests.go | 20 +- pkg/github/pullrequests_granular.go | 20 +- pkg/github/repositories.go | 125 +++++----- pkg/github/repositories_test.go | 6 +- pkg/github/repository_path.go | 82 +++++++ pkg/github/repository_path_test.go | 149 ++++++++++++ pkg/github/scope_filter.go | 37 +-- pkg/github/scope_filter_test.go | 72 +++--- pkg/github/search.go | 10 +- pkg/github/secret_scanning.go | 4 +- pkg/github/security_advisories.go | 8 +- pkg/github/tool_scopes.go | 60 +++++ pkg/github/tool_scopes_test.go | 79 ++++++ pkg/github/ui_tools.go | 3 +- pkg/http/middleware/scope_challenge.go | 34 +-- pkg/http/middleware/scope_challenge_test.go | 119 +++++++++ pkg/inventory/server_tool.go | 53 ++-- pkg/inventory/server_tool_test.go | 2 + pkg/scopes/map.go | 143 +---------- pkg/scopes/map_test.go | 239 +++--------------- pkg/scopes/scopes.go | 178 ++++---------- pkg/scopes/scopes_test.go | 256 +------------------- 51 files changed, 1028 insertions(+), 1233 deletions(-) create mode 100644 pkg/github/repository_path.go create mode 100644 pkg/github/repository_path_test.go create mode 100644 pkg/github/tool_scopes.go create mode 100644 pkg/github/tool_scopes_test.go diff --git a/README.md b/README.md index 9527478a0f..adf4adf34c 100644 --- a/README.md +++ b/README.md @@ -626,7 +626,7 @@ The following sets of tools are available: workflow Actions - **actions_get** - Get details of GitHub Actions resources (workflows, workflow runs, jobs, and artifacts) - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `method`: The method to execute (string, required) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) @@ -638,7 +638,7 @@ The following sets of tools are available: (string, required) - **actions_list** - List GitHub Actions workflows in a repository - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `method`: The action to perform (string, required) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (default: 1) (number, optional) @@ -653,7 +653,7 @@ The following sets of tools are available: - `workflow_runs_filter`: Filters for workflow runs. **ONLY** used when method is 'list_workflow_runs' (object, optional) - **actions_run_trigger** - Trigger GitHub Actions workflow actions - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `inputs`: Inputs the workflow accepts. Only used for 'run_workflow' method. (object, optional) - `method`: The method to execute (string, required) - `owner`: Repository owner (string, required) @@ -663,7 +663,7 @@ The following sets of tools are available: - `workflow_id`: The workflow ID (numeric) or workflow file name (e.g., main.yml, ci.yaml). Required for 'run_workflow' method. (string, optional) - **get_job_logs** - Get GitHub Actions workflow job logs - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `failed_only`: When true, gets logs for all failed jobs in the workflow run specified by run_id. Requires run_id to be provided. (boolean, optional) - `job_id`: The unique identifier of the workflow job. Required when getting logs for a single job. (number, optional) - `owner`: Repository owner (string, required) @@ -679,7 +679,7 @@ The following sets of tools are available: code-square Code Quality - **get_code_quality_finding** - Get code quality finding - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `findingNumber`: The number of the finding. (number, required) - `owner`: The owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) @@ -691,15 +691,13 @@ The following sets of tools are available: codescan Code Security - **get_code_scanning_alert** - Get code scanning alert - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `alertNumber`: The number of the alert. (number, required) - `owner`: The owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) - **list_code_scanning_alerts** - List code scanning alerts - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `owner`: The owner of the repository. (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) @@ -719,14 +717,12 @@ The following sets of tools are available: - No parameters required - **get_team_members** - Get team members - - **Required OAuth Scopes**: `read:org` - - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `write:org` + - **OAuth Challenge Scopes**: `read:org` - `org`: Organization login (owner) that contains the team. (string, required) - `team_slug`: Team slug (string, required) - **get_teams** - Get teams - - **Required OAuth Scopes**: `read:org` - - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `write:org` + - **OAuth Challenge Scopes**: `read:org` - `user`: Username to get teams for. If not provided, uses the authenticated user. (string, optional) @@ -736,7 +732,7 @@ The following sets of tools are available: copilot Copilot - **assign_copilot_to_issue** - Assign Copilot to issue - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `base_ref`: Git reference (e.g., branch) that the agent will start its work from. If not specified, defaults to the repository's default branch (string, optional) - `custom_instructions`: Optional custom instructions to guide the agent beyond the issue body. Use this to provide additional context, constraints, or guidance that is not captured in the issue description (string, optional) - `issue_number`: Issue number (number, required) @@ -744,7 +740,7 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **request_copilot_review** - Request Copilot review - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) - `pullNumber`: Pull request number (number, required) - `repo`: Repository name (string, required) @@ -756,7 +752,7 @@ The following sets of tools are available: copilot Copilot Issue Intents - **assign_copilot_to_issue_with_intent** - Assign Copilot to issue with intent - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `base_ref`: Git reference (e.g., branch) that the agent will start its work from. If not specified, defaults to the repository's default branch. Ignored when is_suggestion is true (string, optional) - `confidence`: How confident you are in this choice. 'HIGH' for clear signal or explicit user request, 'MEDIUM' for reasonable inference with some ambiguity, 'LOW' for best guess with limited signal. (string, required) - `custom_instructions`: Optional custom instructions to guide the agent beyond the issue body. Ignored when is_suggestion is true (string, optional) @@ -773,15 +769,13 @@ The following sets of tools are available: dependabot Dependabot - **get_dependabot_alert** - Get dependabot alert - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `alertNumber`: The number of the alert. (number, required) - `owner`: The owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) - **list_dependabot_alerts** - List dependabot alerts - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `owner`: The owner of the repository. (string, required) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) @@ -796,7 +790,7 @@ The following sets of tools are available: comment-discussion Discussions - **discussion_comment_write** - Manage discussion comments - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: Comment content (required for 'add', 'reply', and 'update' methods) (string, optional) - `commentNodeID`: The Node ID of the discussion comment (required for 'reply', 'update', 'delete', 'mark_answer', and 'unmark_answer' methods). For 'reply', this is the top-level comment to reply to; GitHub Discussions only support one level of nesting. (string, optional) - `discussionNumber`: Discussion number (required for 'add' and 'reply' methods) (number, optional) @@ -813,13 +807,13 @@ The following sets of tools are available: - `repo`: Repository name (required for 'add' and 'reply' methods) (string, optional) - **get_discussion** - Get discussion - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `discussionNumber`: Discussion Number (number, required) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - **get_discussion_comments** - Get discussion comments - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `discussionNumber`: Discussion Number (number, required) - `includeReplies`: When true, each top-level comment will include its replies nested within it (up to 100 replies per comment, which is the GitHub API maximum). Defaults to false. (boolean, optional) @@ -828,12 +822,12 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **list_discussion_categories** - List discussion categories - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) - `repo`: Repository name. If not provided, discussion categories will be queried at the organisation level. (string, optional) - **list_discussions** - List discussions - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `category`: Optional filter by discussion category ID. If provided, only discussions with this category are listed. (string, optional) - `direction`: Order direction. (string, optional) @@ -849,7 +843,7 @@ The following sets of tools are available: logo-gist Gists - **create_gist** - Create Gist - - **Required OAuth Scopes**: `gist` + - **OAuth Challenge Scopes**: `gist` - `content`: Content for simple single-file gist creation (string, required) - `description`: Description of the gist (string, optional) - `filename`: Filename for simple single-file gist creation (string, required) @@ -865,7 +859,7 @@ The following sets of tools are available: - `username`: GitHub username (omit for authenticated user's gists) (string, optional) - **update_gist** - Update Gist - - **Required OAuth Scopes**: `gist` + - **OAuth Challenge Scopes**: `gist` - `content`: Content for the file (string, required) - `description`: Updated description of the gist (string, optional) - `filename`: Filename to update or create (string, required) @@ -878,7 +872,7 @@ The following sets of tools are available: git-branch Git - **get_repository_tree** - Get repository tree - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (username or organization) (string, required) - `path_filter`: Optional path prefix to filter the tree results (e.g., 'src/' to only show files in the src directory) (string, optional) - `recursive`: Setting this parameter to true returns the objects or subtrees referenced by the tree. Default is false (boolean, optional) @@ -892,7 +886,7 @@ The following sets of tools are available: issue-opened Issues - **add_issue_comment** - Add comment to issue or pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: Comment content. Required unless reaction is provided. (string, optional) - `comment_id`: The numeric ID of the issue or pull request comment to react to. Use this for reactions to comments; omit it to react to the issue or pull request itself. Cannot be combined with body. (integer, optional) - `issue_number`: Issue or pull request number to comment on or react to. (number, required) @@ -901,13 +895,13 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **get_label** - Get a specific label from a repository - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `name`: Label name. (string, required) - `owner`: Repository owner (username or organization name) (string, required) - `repo`: Repository name (string, required) - **issue_read** - Get issue details - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The number of the issue (number, required) - `method`: The read operation to perform on a single issue. Options are: @@ -923,7 +917,7 @@ The following sets of tools are available: - `repo`: The name of the repository (string, required) - **issue_write** - Create or update issue/pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `assignees`: Usernames to assign to this issue (string[], optional) - `body`: Issue body content (string, optional) - `duplicate_of`: Issue number that this issue is a duplicate of. Required when state_reason is 'duplicate'. (number, optional) @@ -944,19 +938,17 @@ The following sets of tools are available: - `type`: Type of this issue. For updates, pass null to remove the current type. Only use if issue types are enabled for this repository. Use list_issue_types to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string | null, optional) - **list_issue_fields** - List issue fields - - **Required OAuth Scopes (any of)**: `repo`, `read:org` - - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` + - **OAuth Challenge Scopes**: `repo`, `read:org` - `owner`: The account owner of the repository or organization. The name is not case sensitive. (string, required) - `repo`: The name of the repository. When provided, returns fields for this specific repository (inherited from its organization). When omitted, returns org-level fields directly. (string, optional) - **list_issue_types** - List available issue types - - **Required OAuth Scopes (any of)**: `repo`, `read:org` - - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` + - **OAuth Challenge Scopes**: `repo`, `read:org` - `owner`: The account owner of the repository or organization. (string, required) - `repo`: The name of the repository. When provided, returns issue types for this specific repository. When omitted, returns org-level issue types directly. (string, optional) - **list_issues** - List issues - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `direction`: Order direction. If provided, the 'orderBy' also needs to be provided. (string, optional) - `field_filters`: Filter by custom issue field values. Each entry takes a field_name and a value; the server looks up the field and coerces the value to its type (single-select option name, text, number, or YYYY-MM-DD date). (object[], optional) @@ -970,7 +962,7 @@ The following sets of tools are available: - `state`: Filter by state, by default both open and closed issues are returned when not provided (string, optional) - **search_issues** - Search issues - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `fields`: Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) - `order`: Sort order (string, optional) - `owner`: Optional repository owner. If provided with repo, only issues for this repository are listed. (string, optional) @@ -981,7 +973,7 @@ The following sets of tools are available: - `sort`: Sort field by number of matches of categories, defaults to best match (string, optional) - **sub_issue_write** - Change sub-issue - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `after_id`: The ID of the sub-issue to be prioritized after (either after_id OR before_id should be specified) (number, optional) - `before_id`: The ID of the sub-issue to be prioritized before (either after_id OR before_id should be specified) (number, optional) - `issue_number`: The number of the parent issue (number, required) @@ -1004,13 +996,13 @@ The following sets of tools are available: tag Labels - **get_label** - Get a specific label from a repository - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `name`: Label name. (string, required) - `owner`: Repository owner (username or organization name) (string, required) - `repo`: Repository name (string, required) - **label_write** - Write operations on repository labels - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `color`: Label color as 6-character hex code without '#' prefix (e.g., 'f29513'). Required for 'create', optional for 'update'. (string, optional) - `description`: Label description text. Optional for 'create' and 'update'. (string, optional) - `method`: Operation to perform: 'create', 'update', or 'delete' (string, required) @@ -1020,7 +1012,7 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **list_label** - List labels from a repository - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (username or organization name) - required for all operations (string, required) - `repo`: Repository name - required for all operations (string, required) @@ -1031,16 +1023,16 @@ The following sets of tools are available: bell Notifications - **dismiss_notification** - Dismiss notification - - **Required OAuth Scopes**: `notifications` + - **OAuth Challenge Scopes**: `notifications` - `state`: The new state of the notification (read/done) (string, required) - `threadID`: The ID of the notification thread (string, required) - **get_notification_details** - Get notification details - - **Required OAuth Scopes**: `notifications` + - **OAuth Challenge Scopes**: `notifications` - `notificationID`: The ID of the notification (string, required) - **list_notifications** - List notifications - - **Required OAuth Scopes**: `notifications` + - **OAuth Challenge Scopes**: `notifications` - `before`: Only show notifications updated before the given time (ISO 8601 format) (string, optional) - `filter`: Filter notifications to, use default unless specified. Read notifications are ones that have already been acknowledged by the user. Participating notifications are those that the user is directly involved in, such as issues or pull requests they have commented on or created. (string, optional) - `owner`: Optional repository owner. If provided with repo, only notifications for this repository are listed. (string, optional) @@ -1050,18 +1042,18 @@ The following sets of tools are available: - `since`: Only show notifications updated after the given time (ISO 8601 format) (string, optional) - **manage_notification_subscription** - Manage notification subscription - - **Required OAuth Scopes**: `notifications` + - **OAuth Challenge Scopes**: `notifications` - `action`: Action to perform: ignore, watch, or delete the notification subscription. (string, required) - `notificationID`: The ID of the notification thread. (string, required) - **manage_repository_notification_subscription** - Manage repository notification subscription - - **Required OAuth Scopes**: `notifications` + - **OAuth Challenge Scopes**: `notifications` - `action`: Action to perform: ignore, watch, or delete the repository notification subscription. (string, required) - `owner`: The account owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) - **mark_all_notifications_read** - Mark all notifications as read - - **Required OAuth Scopes**: `notifications` + - **OAuth Challenge Scopes**: `notifications` - `lastReadAt`: Describes the last point that notifications were checked (optional). Default: Now (string, optional) - `owner`: Optional repository owner. If provided with repo, only notifications for this repository are marked as read. (string, optional) - `repo`: Optional repository name. If provided with owner, only notifications for this repository are marked as read. (string, optional) @@ -1073,8 +1065,7 @@ The following sets of tools are available: organization Organizations - **search_orgs** - Search organizations - - **Required OAuth Scopes**: `read:org` - - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `write:org` + - **OAuth Challenge Scopes**: `read:org` - `order`: Sort order (string, optional) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) @@ -1088,8 +1079,7 @@ The following sets of tools are available: project Projects - **projects_get** - Get details of GitHub Projects resources - - **Required OAuth Scopes**: `read:project` - - **Accepted OAuth Scopes**: `project`, `read:project` + - **OAuth Challenge Scopes**: `read:project` - `field_id`: The field's ID. Required for 'get_project_field' method. (number, optional) - `field_names`: Specific list of field names to include in the response when getting a project item (e.g. ["Status", "Priority"]). Resolved server-side to field IDs — pass this instead of 'fields' when you only know the human-readable names. Mutually exclusive with 'fields' — provide one, not both. Only used for 'get_project_item' method. (string[], optional) - `fields`: Specific list of field IDs to include in the response when getting a project item (e.g. ["102589", "985201", "169875"]). If neither 'fields' nor 'field_names' is provided, only the title field is included. Mutually exclusive with 'field_names' — provide one, not both. Only used for 'get_project_item' method. (string[], optional) @@ -1102,8 +1092,7 @@ The following sets of tools are available: - `view_id`: The node ID of the project view. Required for 'get_project_view' method. (string, optional) - **projects_list** - List GitHub Projects resources - - **Required OAuth Scopes**: `read:project` - - **Accepted OAuth Scopes**: `project`, `read:project` + - **OAuth Challenge Scopes**: `read:project` - `after`: Forward pagination cursor from previous pageInfo.nextCursor. (string, optional) - `before`: Backward pagination cursor from previous pageInfo.prevCursor (rare). (string, optional) - `field_names`: Field names to include when listing project items (e.g. ["Status", "Priority"]). Resolved server-side to field IDs — pass this instead of 'fields' when you only know the human-readable names. Names that fail to resolve return a structured error. Mutually exclusive with 'fields' — provide one, not both. Only used for 'list_project_items' method. (string[], optional) @@ -1116,7 +1105,7 @@ The following sets of tools are available: - `query`: Filter/query string. For list_projects: filter by title text and state (e.g. "roadmap is:open"). For list_project_items: advanced filtering using GitHub's project filtering syntax. (string, optional) - **projects_write** - Manage GitHub Projects - - **Required OAuth Scopes**: `project` + - **OAuth Challenge Scopes**: `project` - `body`: The body of the status update (markdown). Used for 'create_project_status_update' method. (string, optional) - `field_name`: The name of the iteration field (e.g. 'Sprint'). Required for 'create_iteration_field' method. (string, optional) - `filter`: Saved view filter; omit on update to preserve it, or pass null to clear it. (string | null, optional) @@ -1151,7 +1140,7 @@ The following sets of tools are available: git-pull-request Pull Requests - **add_comment_to_pending_review** - Add review comment to the requester's latest pending pull request review - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: The text of the review comment (string, required) - `line`: The line of the blob in the pull request diff that the comment applies to. For multi-line comments, the last line of the range (number, optional) - `owner`: Repository owner (string, required) @@ -1164,7 +1153,7 @@ The following sets of tools are available: - `subjectType`: The level at which the comment is targeted (string, required) - **add_reply_to_pull_request_comment** - Add reply to pull request comment - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: The text of the reply. Required unless reaction is provided. (string, optional) - `commentId`: The numeric ID of the pull request review comment to reply or react to. Use the number from a #discussion_r... anchor, not the GraphQL thread node ID (PRRT_...). (number, required) - `owner`: Repository owner (string, required) @@ -1173,7 +1162,7 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **create_pull_request** - Open new pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `base`: Branch to merge into (string, required) - `body`: PR description (string, optional) - `draft`: Create as draft PR (boolean, optional) @@ -1185,7 +1174,7 @@ The following sets of tools are available: - `title`: PR title (string, required) - **list_pull_requests** - List pull requests - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `base`: Filter by base branch (string, optional) - `direction`: Sort direction (string, optional) - `fields`: Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data. (string[], optional) @@ -1198,7 +1187,7 @@ The following sets of tools are available: - `state`: Filter by state (string, optional) - **merge_pull_request** - Merge pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `commit_message`: Extra detail for merge commit (string, optional) - `commit_title`: Title for merge commit (string, optional) - `merge_method`: Merge method (string, optional) @@ -1207,7 +1196,7 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **pull_request_read** - Get details for a single pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `after`: Cursor for pagination, used only by the get_review_comments method. Pass the endCursor from the previous page's PageInfo to fetch the next page. (string, optional) - `method`: Action to specify what pull request data needs to be retrieved from GitHub. Possible options: @@ -1228,7 +1217,7 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **pull_request_review_write** - Write operations (create, submit, delete) on pull request reviews - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: Review comment text (string, optional) - `commitID`: SHA of commit to review (string, optional) - `event`: Review action to perform. (string, optional) @@ -1239,7 +1228,7 @@ The following sets of tools are available: - `threadId`: The node ID of the review thread (e.g., PRRT_kwDOxxx). Required for resolve_thread and unresolve_thread methods. Get thread IDs from pull_request_read with method get_review_comments. (string, optional) - **search_pull_requests** - Search pull requests - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `fields`: Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) - `order`: Sort order (string, optional) - `owner`: Optional repository owner. If provided with repo, only pull requests for this repository are listed. (string, optional) @@ -1250,7 +1239,7 @@ The following sets of tools are available: - `sort`: Sort field by number of matches of categories, defaults to best match (string, optional) - **update_pull_request** - Edit pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `base`: New base branch name (string, optional) - `body`: New description (string, optional) - `draft`: Mark pull request as draft (true) or ready for review (false) (boolean, optional) @@ -1263,7 +1252,7 @@ The following sets of tools are available: - `title`: New title (string, optional) - **update_pull_request_branch** - Update pull request branch - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `expectedHeadSha`: The expected SHA of the pull request's HEAD ref (string, optional) - `owner`: Repository owner (string, required) - `pullNumber`: Pull request number (number, required) @@ -1276,14 +1265,14 @@ The following sets of tools are available: repo Repositories - **create_branch** - Create branch - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `branch`: Name for new branch (string, required) - `from_branch`: Source branch (defaults to repo default) (string, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - **create_or_update_file** - Create or update file - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo`, `workflow` - `allow_symlink_write`: Set true to update a symbolic link itself; content must be its new target path. (boolean, optional) - `branch`: Branch to create/update the file in (string, required) - `content`: Content of the file, exactly as it should appear once written. Do not base64-encode it; this server does that before calling the REST API. (string, required) @@ -1294,7 +1283,7 @@ The following sets of tools are available: - `sha`: The blob SHA of the file being replaced. Required if the file already exists. (string, optional) - **create_repository** - Create repository - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `autoInit`: Initialize with README (boolean, optional) - `description`: Repository description (string, optional) - `name`: Repository name (string, required) @@ -1302,7 +1291,7 @@ The following sets of tools are available: - `private`: Whether the repository should be private. Defaults to true (private) when omitted. (boolean, optional) - **delete_file** - Delete file - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo`, `workflow` - `branch`: Branch to delete the file from (string, required) - `message`: Commit message (string, required) - `owner`: Repository owner (username or organization) (string, required) @@ -1310,18 +1299,18 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **delete_repository** - Delete repository - - **Required OAuth Scopes (all required)**: `delete_repo`, `repo` + - **OAuth Challenge Scopes**: `delete_repo`, `repo` - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - **fork_repository** - Fork repository - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `organization`: Organization to fork to (string, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - **get_commit** - Get commit details - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `detail`: Level of detail to include for changed files. "none" omits stats and files entirely. "stats" (default) includes per-file metadata: filename, status, and lines-of-code counts (additions, deletions, changes), with no patch content. "full_patch" additionally includes the unified diff content for each file and can be very large. (string, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) @@ -1330,7 +1319,7 @@ The following sets of tools are available: - `sha`: Commit SHA, branch name, or tag name (string, required) - **get_file_contents** - Get file or directory contents - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `fields`: Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'. (string[], optional) - `owner`: Repository owner (username or organization) (string, required) - `path`: Path to file/directory (string, optional) @@ -1339,31 +1328,31 @@ The following sets of tools are available: - `sha`: Accepts optional commit SHA. If specified, it will be used instead of ref (string, optional) - **get_latest_release** - Get latest release - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - **get_release_by_tag** - Get a release by tag name - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - `tag`: Tag name (e.g., 'v1.0.0') (string, required) - **get_tag** - Get tag details - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - `tag`: Tag name (string, required) - **list_branches** - List branches - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - **list_commits** - List commits - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `author`: Author username or email address to filter commits by (string, optional) - `fields`: Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'. (string[], optional) - `owner`: Repository owner (string, required) @@ -1376,7 +1365,7 @@ The following sets of tools are available: - `until`: Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD) (string, optional) - **list_releases** - List releases - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `fields`: Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data. (string[], optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) @@ -1384,7 +1373,7 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **list_repository_collaborators** - List repository collaborators - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `affiliation`: Filter by affiliation. Can be one of: 'outside' (outside collaborators), 'direct' (all with permissions regardless of org membership), 'all' (all collaborators). Default: 'all' (string, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (default 1, min 1) (number, optional) @@ -1392,14 +1381,14 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **list_tags** - List tags - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - **push_files** - Push files to repository - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo`, `workflow` - `branch`: Branch to push to (string, required) - `files`: Array of file objects to push, each object with path (string) and content (string) (object[], required) - `message`: Commit message (string, required) @@ -1407,7 +1396,7 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - **search_code** - Search code - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `fields`: Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data. (string[], optional) - `order`: Sort order for results (string, optional) - `page`: Page number for pagination (min 1) (number, optional) @@ -1416,7 +1405,7 @@ The following sets of tools are available: - `sort`: Sort field ('indexed' only) (string, optional) - **search_commits** - Search commits - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `order`: Sort order (string, optional) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) @@ -1424,7 +1413,7 @@ The following sets of tools are available: - `sort`: Sort by author or committer date (defaults to best match) (string, optional) - **search_repositories** - Search repositories - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `minimal_output`: Return minimal repository information (default: true). When false, returns full GitHub API repository objects. (boolean, optional) - `order`: Sort order (string, optional) - `page`: Page number for pagination (min 1) (number, optional) @@ -1439,15 +1428,13 @@ The following sets of tools are available: shield-lock Secret Protection - **get_secret_scanning_alert** - Get secret scanning alert - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `alertNumber`: The number of the alert. (number, required) - `owner`: The owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) - **list_secret_scanning_alerts** - List secret scanning alerts - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `owner`: The owner of the repository. (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) @@ -1463,13 +1450,11 @@ The following sets of tools are available: shield Security Advisories - **get_global_security_advisory** - Get a global security advisory - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `ghsaId`: GitHub Security Advisory ID (format: GHSA-xxxx-xxxx-xxxx). (string, required) - **list_global_security_advisories** - List global security advisories - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `affects`: Filter advisories by affected package or version (e.g. "package1,package2@1.0.0"). (string, optional) - `cveId`: Filter by CVE ID. (string, optional) - `cwes`: Filter by Common Weakness Enumeration IDs (e.g. ["79", "284", "22"]). (string[], optional) @@ -1483,16 +1468,14 @@ The following sets of tools are available: - `updated`: Filter by update date or date range (ISO 8601 date or range). (string, optional) - **list_org_repository_security_advisories** - List org repository security advisories - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `direction`: Sort direction. (string, optional) - `org`: The organization login. (string, required) - `sort`: Sort field. (string, optional) - `state`: Filter by advisory state. (string, optional) - **list_repository_security_advisories** - List repository security advisories - - **Required OAuth Scopes**: `security_events` - - **Accepted OAuth Scopes**: `repo`, `security_events` + - **OAuth Challenge Scopes**: `security_events` - `direction`: Sort direction. (string, optional) - `owner`: The owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) @@ -1506,7 +1489,7 @@ The following sets of tools are available: star Stargazers - **list_starred_repositories** - List starred repositories - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `direction`: The direction to sort the results by. (string, optional) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) @@ -1514,12 +1497,12 @@ The following sets of tools are available: - `username`: Username to list starred repositories for. Defaults to the authenticated user. (string, optional) - **star_repository** - Star repository - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - **unstar_repository** - Unstar repository - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) @@ -1530,7 +1513,7 @@ The following sets of tools are available: people Users - **search_users** - Search users - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `order`: Sort order (string, optional) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 2eb1d35743..791a40b886 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -219,22 +219,8 @@ func writeToolDoc(buf *strings.Builder, tool inventory.ServerTool) { // Tool name (no icon - section header already has the toolset icon) fmt.Fprintf(buf, "- **%s** - %s\n", tool.Tool.Name, tool.Tool.Annotations.Title) - // OAuth scopes if present - if len(tool.RequiredScopes) > 0 { - scopeList := "`" + strings.Join(tool.RequiredScopes, "`, `") + "`" - 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) - default: - fmt.Fprintf(buf, " - **Required OAuth Scopes**: %s\n", scopeList) - } - - // Only show accepted scopes if they differ from required scopes - if len(tool.AcceptedScopes) > 0 && !scopesEqual(tool.RequiredScopes, tool.AcceptedScopes) { - fmt.Fprintf(buf, " - **Accepted OAuth Scopes**: `%s`\n", strings.Join(tool.AcceptedScopes, "`, `")) - } + if scopes := tool.ScopeAccess.Scopes; len(scopes) > 0 { + fmt.Fprintf(buf, " - **OAuth Challenge Scopes**: `%s`\n", strings.Join(scopes, "`, `")) } // MCP App UI metadata (only rendered when the remote_mcp_ui_apps flag @@ -322,28 +308,6 @@ func schemaTypeString(schema *jsonschema.Schema) string { return strings.Join(types, " | ") } -// scopesEqual checks if two scope slices contain the same elements (order-independent) -func scopesEqual(a, b []string) bool { - if len(a) != len(b) { - return false - } - - // Create a map for quick lookup - aMap := make(map[string]bool, len(a)) - for _, scope := range a { - aMap[scope] = true - } - - // Check if all elements in b are in a - for _, scope := range b { - if !aMap[scope] { - return false - } - } - - return true -} - // indentMultilineDescription adds the specified indent to all lines after the first line. // This ensures that multi-line descriptions maintain proper markdown list formatting. func indentMultilineDescription(description, indent string) string { diff --git a/cmd/github-mcp-server/list_scopes.go b/cmd/github-mcp-server/list_scopes.go index d8b8bf3922..8d6d038d1e 100644 --- a/cmd/github-mcp-server/list_scopes.go +++ b/cmd/github-mcp-server/list_scopes.go @@ -17,11 +17,10 @@ import ( // ToolScopeInfo contains scope information for a single tool. type ToolScopeInfo struct { - Name string `json:"name"` - Toolset string `json:"toolset"` - ReadOnly bool `json:"read_only"` - RequiredScopes []string `json:"required_scopes"` - AcceptedScopes []string `json:"accepted_scopes,omitempty"` + Name string `json:"name"` + Toolset string `json:"toolset"` + ReadOnly bool `json:"read_only"` + ChallengeScopes []string `json:"challenge_scopes,omitempty"` } // ScopesOutput is the full output structure for the list-scopes command. @@ -36,12 +35,11 @@ type ScopesOutput struct { var listScopesCmd = &cobra.Command{ Use: "list-scopes", - Short: "List required OAuth scopes for enabled tools", - Long: `List the required OAuth scopes for all enabled tools. + Short: "List OAuth scope policies for enabled tools", + Long: `List the OAuth challenge scopes for all enabled tools. This command creates an inventory based on the same flags as the stdio command -and outputs the required OAuth scopes for each enabled tool. This is useful for -determining what scopes a token needs to use specific tools. +and outputs the scopes each enabled tool may request in an OAuth challenge. The output format can be controlled with the --output flag: - text (default): Human-readable text output @@ -153,30 +151,27 @@ func collectToolScopes(inv *inventory.Inventory, readOnly bool) ScopesOutput { for _, serverTool := range availableTools { tool := serverTool.Tool - // Get scope information directly from ServerTool - requiredScopes := serverTool.RequiredScopes - acceptedScopes := serverTool.AcceptedScopes + challengeScopes := serverTool.ScopeAccess.Scopes // Determine if tool is read-only isReadOnly := serverTool.IsReadOnly() toolInfo := ToolScopeInfo{ - Name: tool.Name, - Toolset: string(serverTool.Toolset.ID), - ReadOnly: isReadOnly, - RequiredScopes: requiredScopes, - AcceptedScopes: acceptedScopes, + Name: tool.Name, + Toolset: string(serverTool.Toolset.ID), + ReadOnly: isReadOnly, + ChallengeScopes: challengeScopes, } tools = append(tools, toolInfo) // Track unique scopes - for _, s := range requiredScopes { + for _, s := range challengeScopes { scopeSet[s] = true toolsByScope[s] = append(toolsByScope[s], tool.Name) } // Track scopes by tool - scopesByTool[tool.Name] = requiredScopes + scopesByTool[tool.Name] = challengeScopes } // Sort tools by name @@ -225,7 +220,7 @@ func outputSummary(output ScopesOutput) error { return nil } - fmt.Println("Required OAuth scopes for enabled tools:") + fmt.Println("OAuth scope policies for enabled tools:") fmt.Println() for _, scope := range output.UniqueScopes { fmt.Printf(" %s\n", formatScopeDisplay(scope)) @@ -235,8 +230,8 @@ func outputSummary(output ScopesOutput) error { } func outputText(output ScopesOutput) error { - fmt.Printf("OAuth Scopes for Enabled Tools\n") - fmt.Printf("==============================\n\n") + fmt.Printf("OAuth Challenge Scopes for Enabled Tools\n") + fmt.Printf("========================================\n\n") fmt.Printf("Enabled Toolsets: %s\n", strings.Join(output.EnabledToolsets, ", ")) fmt.Printf("Read-Only Mode: %v\n\n", output.ReadOnly) @@ -265,8 +260,8 @@ func outputText(output ScopesOutput) error { } scopeStr := "(no scope required)" - if len(tool.RequiredScopes) > 0 { - scopeStr = strings.Join(tool.RequiredScopes, ", ") + if len(tool.ChallengeScopes) > 0 { + scopeStr = strings.Join(tool.ChallengeScopes, ", ") } fmt.Printf(" %s %s: %s\n", rwIndicator, tool.Name, scopeStr) @@ -278,9 +273,9 @@ func outputText(output ScopesOutput) error { fmt.Println("## Summary") fmt.Println() if len(output.UniqueScopes) == 0 { - fmt.Println("No OAuth scopes required for enabled tools.") + fmt.Println("No OAuth scopes are used by enabled tools.") } else { - fmt.Println("Unique scopes required:") + fmt.Println("Unique challenge scopes:") for _, scope := range output.UniqueScopes { fmt.Printf(" • %s\n", formatScopeDisplay(scope)) } diff --git a/cmd/github-mcp-server/main_test.go b/cmd/github-mcp-server/main_test.go index ad927701a2..a5b2b84967 100644 --- a/cmd/github-mcp-server/main_test.go +++ b/cmd/github-mcp-server/main_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/spf13/viper" @@ -56,38 +57,15 @@ func TestAuthorizationServerConfigurationIsHTTPOnly(t *testing.T) { assert.Equal(t, "https://oauth-proxy.example.com", viper.GetString("authorization-server")) } -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)**", - }, +func TestWriteToolDocScopes(t *testing.T) { + tool := inventory.ServerTool{ + Tool: mcp.Tool{Name: "delete", Annotations: &mcp.ToolAnnotations{Title: "Delete"}}, + ScopeAccess: scopes.RequireAll(scopes.DeleteRepo, scopes.Repo), } - 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) - }) - } + var buf strings.Builder + writeToolDoc(&buf, tool) + assert.Contains(t, buf.String(), "**OAuth Challenge Scopes**: `delete_repo`, `repo`") } func TestSchemaTypeString(t *testing.T) { diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0de5bdd722..a687671fa8 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -35,7 +35,7 @@ runtime behavior (such as output formatting) won't appear here. ### `remote_mcp_ui_apps` - **create_pull_request** - Open new pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - **MCP App UI**: `ui://github-mcp-server/pr-write` - `base`: Branch to merge into (string, required) - `body`: PR description (string, optional) @@ -52,7 +52,7 @@ runtime behavior (such as output formatting) won't appear here. - No parameters required - **issue_write** - Create or update issue/pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - **MCP App UI**: `ui://github-mcp-server/issue-write` - `assignees`: Usernames to assign to this issue (string[], optional) - `body`: Issue body content (string, optional) @@ -74,14 +74,13 @@ runtime behavior (such as output formatting) won't appear here. - `type`: Type of this issue. For updates, pass null to remove the current type. Only use if issue types are enabled for this repository. Use list_issue_types to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string | null, optional) - **ui_get** - Get UI data - - **Required OAuth Scopes (any of)**: `repo`, `read:org` - - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` + - **OAuth Challenge Scopes**: `repo`, `read:org` - `method`: The type of data to fetch (string, required) - `owner`: Repository owner (required for all methods) (string, required) - `repo`: Repository name (required for labels, assignees, milestones, branches, issue fields, reviewers) (string, optional) - **update_pull_request** - Edit pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - **MCP App UI**: `ui://github-mcp-server/pr-edit` - `base`: New base branch name (string, optional) - `body`: New description (string, optional) @@ -97,21 +96,21 @@ runtime behavior (such as output formatting) won't appear here. ### `issues_granular` - **add_issue_comment_reaction** - Add Reaction to Issue or Pull Request Comment - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `comment_id`: The issue or pull request comment ID (number, required) - `content`: The emoji reaction type (string, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - **add_issue_reaction** - Add Reaction to Issue or Pull Request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `content`: The emoji reaction type (string, required) - `issue_number`: The issue number (number, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - **add_sub_issue** - Add Sub-Issue - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The parent issue number (number, required) - `owner`: Repository owner (username or organization) (string, required) - `replace_parent`: If true, reparent the sub-issue if it already has a parent (boolean, optional) @@ -119,21 +118,21 @@ runtime behavior (such as output formatting) won't appear here. - `sub_issue_id`: The ID of the sub-issue to add. ID is not the same as issue number (number, required) - **create_issue** - Create Issue - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: Issue body content (optional) (string, optional) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - `title`: Issue title (string, required) - **remove_sub_issue** - Remove Sub-Issue - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The parent issue number (number, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - `sub_issue_id`: The ID of the sub-issue to remove. ID is not the same as issue number (number, required) - **reprioritize_sub_issue** - Reprioritize Sub-Issue - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `after_id`: The ID of the sub-issue to place this after (either after_id OR before_id should be specified) (number, optional) - `before_id`: The ID of the sub-issue to place this before (either after_id OR before_id should be specified) (number, optional) - `issue_number`: The parent issue number (number, required) @@ -142,42 +141,42 @@ runtime behavior (such as output formatting) won't appear here. - `sub_issue_id`: The ID of the sub-issue to reorder. ID is not the same as issue number (number, required) - **set_issue_fields** - Set Issue Fields - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `fields`: Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, or 'single_select_option_id' (the GraphQL node ID of the option) for single select fields. Set 'delete' to true to remove a field value. (object[], required) - `issue_number`: The issue number to update (number, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - **update_issue_assignees** - Update Issue Assignees - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `assignees`: GitHub usernames to assign to this issue. ([], required) - `issue_number`: The issue number to update (number, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - **update_issue_body** - Update Issue Body - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: The new body content for the issue (string, required) - `issue_number`: The issue number to update (number, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - **update_issue_labels** - Update Issue Labels - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The issue number to update (number, required) - `labels`: Labels to apply to this issue. ([], required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - **update_issue_milestone** - Update Issue Milestone - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The issue number to update (number, required) - `milestone`: The milestone number to set on the issue (integer, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - **update_issue_state** - Update Issue State - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `confidence`: How confident you are in this choice. Use 'HIGH' for clear signal or explicit user request, 'MEDIUM' for reasonable inference with some ambiguity, 'LOW' for best guess with limited signal. (string, optional) - `duplicate_of`: The issue number of the canonical issue this issue duplicates. Only valid when state_reason is 'duplicate'. Required when is_suggestion is true and state_reason is 'duplicate'. The issue number is resolved to a database ID before being sent to the API. (number, optional) - `is_suggestion`: If true, this state change is sent to the API as a suggestion (suggest:true) rather than an applied change. Whether the change is applied or recorded as a proposal is determined by the API. (boolean, optional) @@ -189,14 +188,14 @@ runtime behavior (such as output formatting) won't appear here. - `state_reason`: The reason for the state change (only for closed state) (string, optional) - **update_issue_title** - Update Issue Title - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The issue number to update (number, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - `title`: The new title for the issue (string, required) - **update_issue_type** - Update Issue Type - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `confidence`: How confident you are in this choice. Use 'HIGH' for clear signal or explicit user request, 'MEDIUM' for reasonable inference with some ambiguity, 'LOW' for best guess with limited signal. (string, optional) - `is_suggestion`: If true, this issue type change is sent to the API as a suggestion (suggest:true) rather than an applied value. Whether the type is applied or recorded as a proposal is determined by the API. (boolean, optional) - `issue_number`: The issue number to update (number, required) @@ -208,7 +207,7 @@ runtime behavior (such as output formatting) won't appear here. ### `pull_requests_granular` - **add_pull_request_review_comment** - Add Pull Request Review Comment - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: The comment body (string, required) - `line`: The line number in the diff to comment on (optional) (number, optional) - `owner`: Repository owner (username or organization) (string, required) @@ -221,14 +220,14 @@ runtime behavior (such as output formatting) won't appear here. - `subjectType`: The subject type of the comment (string, required) - **add_pull_request_review_comment_reaction** - Add Pull Request Review Comment Reaction - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `comment_id`: The numeric pull request review comment ID. Use the number from a #discussion_r... anchor, not the GraphQL thread node ID (PRRT_...). (number, required) - `content`: The emoji reaction type (string, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) - **create_pull_request_review** - Create Pull Request Review - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: The review body text (optional) (string, optional) - `commitID`: The SHA of the commit to review (optional, defaults to latest) (string, optional) - `event`: The review action to perform. If omitted, creates a pending review. (string, optional) @@ -237,24 +236,24 @@ runtime behavior (such as output formatting) won't appear here. - `repo`: Repository name (string, required) - **delete_pending_pull_request_review** - Delete Pending Pull Request Review - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (username or organization) (string, required) - `pullNumber`: The pull request number (number, required) - `repo`: Repository name (string, required) - **request_pull_request_reviewers** - Request Pull Request Reviewers - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (username or organization) (string, required) - `pullNumber`: The pull request number (number, required) - `repo`: Repository name (string, required) - `reviewers`: GitHub usernames or ORG/team-slug team reviewers to request reviews from (string[], required) - **resolve_review_thread** - Resolve Review Thread - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `threadID`: The node ID of the review thread to resolve (e.g., PRRT_kwDOxxx) (string, required) - **submit_pending_pull_request_review** - Submit Pending Pull Request Review - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: The review body text (optional) (string, optional) - `event`: The review action to perform (string, required) - `owner`: Repository owner (username or organization) (string, required) @@ -262,32 +261,32 @@ runtime behavior (such as output formatting) won't appear here. - `repo`: Repository name (string, required) - **unresolve_review_thread** - Unresolve Review Thread - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `threadID`: The node ID of the review thread to unresolve (e.g., PRRT_kwDOxxx) (string, required) - **update_pull_request_body** - Update Pull Request Body - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: The new body content for the pull request (string, required) - `owner`: Repository owner (username or organization) (string, required) - `pullNumber`: The pull request number (number, required) - `repo`: Repository name (string, required) - **update_pull_request_draft_state** - Update Pull Request Draft State - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `draft`: Set to true to convert to draft, false to mark as ready for review (boolean, required) - `owner`: Repository owner (username or organization) (string, required) - `pullNumber`: The pull request number (number, required) - `repo`: Repository name (string, required) - **update_pull_request_state** - Update Pull Request State - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (username or organization) (string, required) - `pullNumber`: The pull request number (number, required) - `repo`: Repository name (string, required) - `state`: The new state for the pull request (string, required) - **update_pull_request_title** - Update Pull Request Title - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (username or organization) (string, required) - `pullNumber`: The pull request number (number, required) - `repo`: Repository name (string, required) @@ -296,7 +295,7 @@ runtime behavior (such as output formatting) won't appear here. ### `file_blame` - **get_file_blame** - Get file blame information - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `end_line`: Optional 1-based ending line of the window of interest. Must be >= start_line when both are provided. (number, optional) - `owner`: Repository owner (username or organization) (string, required) @@ -309,7 +308,7 @@ runtime behavior (such as output formatting) won't appear here. ### `issue_dependencies` - **issue_dependency_read** - Read issue dependencies - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The number of the issue (number, required) - `method`: The read operation to perform on a single issue's dependencies. Options are: @@ -322,7 +321,7 @@ runtime behavior (such as output formatting) won't appear here. - `repo`: The name of the repository (string, required) - **issue_dependency_write** - Change issue dependency - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The number of the subject issue (number, required) - `method`: The action to perform. Options are: @@ -341,7 +340,7 @@ runtime behavior (such as output formatting) won't appear here. ### `duplicate_detection` - **find_duplicate** - Find duplicate issues - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `confidence_threshold`: Minimum similarity threshold a candidate must meet to be returned; higher values are stricter. When omitted, the API's high-precision default is used. The scale is defined by the API, so no client-side bounds are enforced. (number, optional) - `issue_number`: The number of the existing issue to find duplicates for (number, required) - `owner`: The owner of the repository (string, required) diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 350522bf5e..e07dbca37d 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -29,7 +29,7 @@ The list below is generated from the Go source. It covers tool **inventory and s ### `remote_mcp_ui_apps` - **create_pull_request** - Open new pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - **MCP App UI**: `ui://github-mcp-server/pr-write` - `base`: Branch to merge into (string, required) - `body`: PR description (string, optional) @@ -46,7 +46,7 @@ The list below is generated from the Go source. It covers tool **inventory and s - No parameters required - **issue_write** - Create or update issue/pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - **MCP App UI**: `ui://github-mcp-server/issue-write` - `assignees`: Usernames to assign to this issue (string[], optional) - `body`: Issue body content (string, optional) @@ -68,14 +68,13 @@ The list below is generated from the Go source. It covers tool **inventory and s - `type`: Type of this issue. For updates, pass null to remove the current type. Only use if issue types are enabled for this repository. Use list_issue_types to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string | null, optional) - **ui_get** - Get UI data - - **Required OAuth Scopes (any of)**: `repo`, `read:org` - - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` + - **OAuth Challenge Scopes**: `repo`, `read:org` - `method`: The type of data to fetch (string, required) - `owner`: Repository owner (required for all methods) (string, required) - `repo`: Repository name (required for labels, assignees, milestones, branches, issue fields, reviewers) (string, optional) - **update_pull_request** - Edit pull request - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - **MCP App UI**: `ui://github-mcp-server/pr-edit` - `base`: New base branch name (string, optional) - `body`: New description (string, optional) @@ -91,7 +90,7 @@ The list below is generated from the Go source. It covers tool **inventory and s ### `file_blame` - **get_file_blame** - Get file blame information - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `end_line`: Optional 1-based ending line of the window of interest. Must be >= start_line when both are provided. (number, optional) - `owner`: Repository owner (username or organization) (string, required) @@ -104,7 +103,7 @@ The list below is generated from the Go source. It covers tool **inventory and s ### `issue_dependencies` - **issue_dependency_read** - Read issue dependencies - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The number of the issue (number, required) - `method`: The read operation to perform on a single issue's dependencies. Options are: @@ -117,7 +116,7 @@ The list below is generated from the Go source. It covers tool **inventory and s - `repo`: The name of the repository (string, required) - **issue_dependency_write** - Change issue dependency - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `issue_number`: The number of the subject issue (number, required) - `method`: The action to perform. Options are: diff --git a/docs/scope-filtering.md b/docs/scope-filtering.md index f29d631ca1..2055db1483 100644 --- a/docs/scope-filtering.md +++ b/docs/scope-filtering.md @@ -22,6 +22,15 @@ When the server starts with a classic PAT, it makes a lightweight HTTP HEAD requ With OAuth, the remote server can dynamically request additional scopes as needed. With PATs, scopes are fixed at token creation, so the server proactively hides tools you can't use. +## Scope Checks + +Each tool defines two small checks: + +- A visibility check decides whether to show the tool for a classic PAT. +- A per-call check returns the exact scopes for an OAuth challenge, or no scopes when the call can continue. + +The per-call check receives the tool arguments, so it can make direct decisions. Listing issue fields uses `repo` for a repository request and `read:org` for an organization request. File writes request `workflow` in addition to `repo` only when the call changes a workflow file. + ## OAuth Scope Challenges (Remote Server) When using the [remote MCP server](./remote-server.md) with OAuth authentication, the server uses a different approach called **scope challenges**. Instead of hiding tools upfront, all tools are available, and the server requests additional scopes on-demand when you try to use a tool that requires them. @@ -58,7 +67,7 @@ Some scopes implicitly include others: This means if your token has `repo`, tools requiring `security_events` will also be available. -Each tool in the [README](../README.md#tools) lists its required and accepted OAuth scopes. +Each tool in the [README](../README.md#tools) lists the OAuth scopes it may challenge for. ## Public Repository Access diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 0a1db9d387..667901e623 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -323,7 +323,7 @@ Use this tool to list workflows in a repository, or list workflow runs, jobs, an Required: []string{"method", "owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -453,7 +453,7 @@ Use this tool to get details about individual workflows, workflow runs, jobs, an Required: []string{"method", "owner", "repo", "resource_id"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -581,7 +581,7 @@ func ActionsRunTrigger(t translations.TranslationHelperFunc) inventory.ServerToo Required: []string{"method", "owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -693,7 +693,7 @@ For single job logs, provide job_id. For all failed jobs in a run, provide run_i Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/code_quality.go b/pkg/github/code_quality.go index 41c791182b..78ba24d35d 100644 --- a/pkg/github/code_quality.go +++ b/pkg/github/code_quality.go @@ -46,7 +46,7 @@ func GetCodeQualityFinding(t translations.TranslationHelperFunc) inventory.Serve Required: []string{"owner", "repo", "findingNumber"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/code_scanning.go b/pkg/github/code_scanning.go index 74a1c7e0fa..12b0b3f1e2 100644 --- a/pkg/github/code_scanning.go +++ b/pkg/github/code_scanning.go @@ -46,7 +46,7 @@ func GetCodeScanningAlert(t translations.TranslationHelperFunc) inventory.Server Required: []string{"owner", "repo", "alertNumber"}, }, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -146,7 +146,7 @@ func ListCodeScanningAlerts(t translations.TranslationHelperFunc) inventory.Serv }, InputSchema: schema, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/context_tools.go b/pkg/github/context_tools.go index b4c7098c56..da1a42d694 100644 --- a/pkg/github/context_tools.go +++ b/pkg/github/context_tools.go @@ -62,7 +62,7 @@ func GetMe(t translations.TranslationHelperFunc) inventory.ServerTool { }, }, }, - nil, + scopes.NoScopes(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, _ map[string]any) (*mcp.CallToolResult, any, error) { client, err := deps.GetClient(ctx) if err != nil { @@ -143,7 +143,7 @@ func GetTeams(t translations.TranslationHelperFunc) inventory.ServerTool { }, }, }, - []scopes.Scope{scopes.ReadOrg}, + scopes.RequireAll(scopes.ReadOrg), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { user, err := OptionalParam[string](args, "user") if err != nil { @@ -251,7 +251,7 @@ func GetTeamMembers(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"org", "team_slug"}, }, }, - []scopes.Scope{scopes.ReadOrg}, + scopes.RequireAll(scopes.ReadOrg), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { org, err := RequiredParam[string](args, "org") if err != nil { diff --git a/pkg/github/copilot.go b/pkg/github/copilot.go index 4b3225e221..1064bef144 100644 --- a/pkg/github/copilot.go +++ b/pkg/github/copilot.go @@ -201,7 +201,7 @@ func AssignCopilotToIssue(t translations.TranslationHelperFunc) inventory.Server Required: []string{"owner", "repo", "issue_number"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, request *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { var params struct { Owner string `mapstructure:"owner"` @@ -565,7 +565,7 @@ func AssignCopilotToIssueWithIntent(t translations.TranslationHelperFunc) invent Required: []string{"owner", "repo", "issue_number", "rationale", "confidence", "is_suggestion"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, request *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { // Presence-check is_suggestion before decoding: mapstructure defaults a // missing bool to false, which would silently launch Copilot instead of @@ -867,7 +867,7 @@ func RequestCopilotReview(t translations.TranslationHelperFunc) inventory.Server }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/dependabot.go b/pkg/github/dependabot.go index f56a33134a..9cec4cb814 100644 --- a/pkg/github/dependabot.go +++ b/pkg/github/dependabot.go @@ -47,7 +47,7 @@ func GetDependabotAlert(t translations.TranslationHelperFunc) inventory.ServerTo Required: []string{"owner", "repo", "alertNumber"}, }, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -139,7 +139,7 @@ func ListDependabotAlerts(t translations.TranslationHelperFunc) inventory.Server }, InputSchema: schema, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index d52b5f4aeb..0de3e3b279 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -15,7 +15,6 @@ import ( "github.com/github/github-mcp-server/pkg/observability" "github.com/github/github-mcp-server/pkg/observability/metrics" "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" gogithub "github.com/google/go-github/v89/github" @@ -230,21 +229,18 @@ func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flagName string) bool { // The handler function receives deps extracted from context via MustDepsFromContext. // Ensure ContextWithDeps is called to inject deps before any tool handlers are invoked. // -// requiredScopes specifies the minimum OAuth scopes needed for this tool. -// AcceptedScopes are automatically derived using the scope hierarchy (e.g., if -// public_repo is required, repo is also accepted since repo grants public_repo). +// scopeAccess controls fixed-token visibility and per-call OAuth challenges. func NewTool[In, Out any]( toolset inventory.ToolsetMetadata, tool mcp.Tool, - requiredScopes []scopes.Scope, + scopeAccess inventory.ScopeAccess, handler func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args In) (*mcp.CallToolResult, Out, error), ) inventory.ServerTool { st := inventory.NewServerToolWithContextHandler(tool, toolset, func(ctx context.Context, req *mcp.CallToolRequest, args In) (*mcp.CallToolResult, Out, error) { deps := MustDepsFromContext(ctx) return handler(ctx, deps, req, args) }) - st.RequiredScopes = scopes.ToStringSlice(requiredScopes...) - st.AcceptedScopes = scopes.ExpandScopes(requiredScopes...) + st.ScopeAccess = scopeAccess return st } @@ -254,20 +250,18 @@ func NewTool[In, Out any]( // The handler function receives deps extracted from context via MustDepsFromContext. // Ensure ContextWithDeps is called to inject deps before any tool handlers are invoked. // -// requiredScopes specifies the minimum OAuth scopes needed for this tool. -// AcceptedScopes are automatically derived using the scope hierarchy. +// scopeAccess controls fixed-token visibility and per-call OAuth challenges. func NewToolFromHandler( toolset inventory.ToolsetMetadata, tool mcp.Tool, - requiredScopes []scopes.Scope, + scopeAccess inventory.ScopeAccess, handler func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest) (*mcp.CallToolResult, error), ) inventory.ServerTool { st := inventory.NewServerTool(tool, toolset, func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { deps := MustDepsFromContext(ctx) return handler(ctx, deps, req) }) - st.RequiredScopes = scopes.ToStringSlice(requiredScopes...) - st.AcceptedScopes = scopes.ExpandScopes(requiredScopes...) + st.ScopeAccess = scopeAccess return st } diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 9d9d02f82a..9ea31b2ebf 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -165,7 +165,7 @@ func ListDiscussions(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"owner"}, }), }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -312,7 +312,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "discussionNumber"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { // Decode params var params struct { @@ -424,7 +424,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve Required: []string{"owner", "repo", "discussionNumber"}, }), }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { // Decode params var params struct { @@ -639,7 +639,7 @@ Options are: Required: []string{"method"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { @@ -1015,7 +1015,7 @@ func ListDiscussionCategories(t translations.TranslationHelperFunc) inventory.Se Required: []string{"owner"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index 0b73ddeb3b..5a67b4bf21 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -41,7 +41,7 @@ func HelloWorldTool(t translations.TranslationHelperFunc) inventory.ServerTool { ReadOnlyHint: true, }, }, - []scopes.Scope{}, + scopes.NoScopes(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, _ map[string]any) (*mcp.CallToolResult, any, error) { // Check feature flag to determine greeting style diff --git a/pkg/github/find_duplicate.go b/pkg/github/find_duplicate.go index e32179ce8d..65cb6f2002 100644 --- a/pkg/github/find_duplicate.go +++ b/pkg/github/find_duplicate.go @@ -83,7 +83,7 @@ func FindDuplicate(t translations.TranslationHelperFunc) inventory.ServerTool { }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/find_duplicate_test.go b/pkg/github/find_duplicate_test.go index be5c3dda81..9e20d958b4 100644 --- a/pkg/github/find_duplicate_test.go +++ b/pkg/github/find_duplicate_test.go @@ -26,7 +26,9 @@ func Test_FindDuplicate(t *testing.T) { assert.Equal(t, "find_duplicate", tool.Name) assert.NotEmpty(t, tool.Description) assert.True(t, tool.Annotations.ReadOnlyHint) - assert.ElementsMatch(t, serverTool.RequiredScopes, []string{"repo"}) + assert.Equal(t, []string{"repo"}, serverTool.ScopeAccess.Scopes) + assert.NotNil(t, serverTool.ScopeAccess.Visible) + assert.NotNil(t, serverTool.ScopeAccess.Challenge) schema := tool.InputSchema.(*jsonschema.Schema) assert.Contains(t, schema.Properties, "owner") diff --git a/pkg/github/gists.go b/pkg/github/gists.go index 0ea883ec7d..441e59ea7c 100644 --- a/pkg/github/gists.go +++ b/pkg/github/gists.go @@ -43,7 +43,7 @@ func ListGists(t translations.TranslationHelperFunc) inventory.ServerTool { }, }), }, - nil, + scopes.NoScopes(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { username, err := OptionalParam[string](args, "username") if err != nil { @@ -129,7 +129,7 @@ func GetGist(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"gist_id"}, }, }, - nil, + scopes.NoScopes(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { gistID, err := RequiredParam[string](args, "gist_id") if err != nil { @@ -202,7 +202,7 @@ func CreateGist(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"filename", "content"}, }, }, - []scopes.Scope{scopes.Gist}, + scopes.RequireAll(scopes.Gist), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { description, err := OptionalParam[string](args, "description") if err != nil { @@ -303,7 +303,7 @@ func UpdateGist(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"gist_id", "filename", "content"}, }, }, - []scopes.Scope{scopes.Gist}, + scopes.RequireAll(scopes.Gist), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { gistID, err := RequiredParam[string](args, "gist_id") if err != nil { diff --git a/pkg/github/git.go b/pkg/github/git.go index 5b0c9b79b2..0f65b9324d 100644 --- a/pkg/github/git.go +++ b/pkg/github/git.go @@ -78,7 +78,7 @@ func GetRepositoryTree(t translations.TranslationHelperFunc) inventory.ServerToo Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { 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/issue_dependencies.go b/pkg/github/issue_dependencies.go index d0b596a66b..246192736e 100644 --- a/pkg/github/issue_dependencies.go +++ b/pkg/github/issue_dependencies.go @@ -62,7 +62,7 @@ Options are: }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { @@ -249,7 +249,7 @@ Options are: Required: []string{"method", "type", "owner", "repo", "issue_number", "related_issue_number"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { diff --git a/pkg/github/issue_fields.go b/pkg/github/issue_fields.go index e06bdbfcf9..325fb548e2 100644 --- a/pkg/github/issue_fields.go +++ b/pkg/github/issue_fields.go @@ -10,7 +10,6 @@ import ( ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/ifc" "github.com/github/github-mcp-server/pkg/inventory" - "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/jsonschema-go/jsonschema" @@ -129,7 +128,7 @@ func ListIssueFields(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"owner"}, }, }, - []scopes.Scope{scopes.Repo, scopes.ReadOrg}, + repositoryOrOrganizationScopeAccess(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/issue_fields_test.go b/pkg/github/issue_fields_test.go index 2c2b26ee2a..54ae2f97eb 100644 --- a/pkg/github/issue_fields_test.go +++ b/pkg/github/issue_fields_test.go @@ -26,8 +26,9 @@ func Test_ListIssueFields(t *testing.T) { assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "owner") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "repo") assert.ElementsMatch(t, tool.InputSchema.(*jsonschema.Schema).Required, []string{"owner"}) - assert.ElementsMatch(t, serverTool.RequiredScopes, []string{"repo", "read:org"}) - assert.ElementsMatch(t, serverTool.AcceptedScopes, []string{"repo", "read:org", "write:org", "admin:org"}) + assert.Equal(t, []string{"repo", "read:org"}, serverTool.ScopeAccess.Scopes) + assert.NotNil(t, serverTool.ScopeAccess.Visible) + assert.NotNil(t, serverTool.ScopeAccess.Challenge) queryStruct := issueFieldsRepoQuery{} defaultVars := map[string]any{ diff --git a/pkg/github/issues.go b/pkg/github/issues.go index d6cc55e1ed..5d44d6a0a4 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -831,7 +831,7 @@ func IssueRead(t translations.TranslationHelperFunc) inventory.ServerTool { }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { @@ -1256,7 +1256,7 @@ func GetIssueLabels(ctx context.Context, client *githubv4.Client, owner string, // ListIssueTypes creates a tool to list defined issue types for an organization or repository. // This can be used to understand supported issue type values for creating or updating issues. func ListIssueTypes(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( + st := NewTool( ToolsetMetadataIssues, mcp.Tool{ Name: "list_issue_types", @@ -1280,7 +1280,7 @@ func ListIssueTypes(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner"}, }, }, - []scopes.Scope{scopes.Repo, scopes.ReadOrg}, + repositoryOrOrganizationScopeAccess(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1354,6 +1354,7 @@ func ListIssueTypes(t translations.TranslationHelperFunc) inventory.ServerTool { result = attachStaticIFCLabel(ctx, deps, result, ifc.LabelRepoMetadata(true)) return result, nil, nil }) + return st } // AddIssueComment creates a tool to add a comment or reaction to an issue. @@ -1401,7 +1402,7 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"owner", "repo", "issue_number"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1620,7 +1621,7 @@ func SubIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"method", "owner", "repo", "issue_number", "sub_issue_id"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { @@ -1882,7 +1883,7 @@ func SearchIssues(t translations.TranslationHelperFunc, opts ...ToolOption) inve }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { options := []searchOption{ifcSearchPostProcessOption(ctx, deps)} fields, err := OptionalStringArrayParam(args, "fields") @@ -2511,7 +2512,7 @@ Options are: Required: []string{"method", "owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { @@ -3044,7 +3045,7 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index fb5ff32242..20d537d303 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -67,7 +67,7 @@ func issueUpdateTool( Required: required, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -148,7 +148,7 @@ func GranularCreateIssue(t translations.TranslationHelperFunc) inventory.ServerT Required: []string{"owner", "repo", "title"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -304,7 +304,7 @@ func GranularUpdateIssueAssignees(t translations.TranslationHelperFunc) inventor Required: []string{"owner", "repo", "issue_number", "assignees"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -522,7 +522,7 @@ func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.S Required: []string{"owner", "repo", "issue_number", "labels"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -735,7 +735,7 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser Required: []string{"owner", "repo", "issue_number", "issue_type"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -909,7 +909,7 @@ func GranularUpdateIssueState(t translations.TranslationHelperFunc) inventory.Se Required: []string{"owner", "repo", "issue_number", "state"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1067,7 +1067,7 @@ func GranularAddSubIssue(t translations.TranslationHelperFunc) inventory.ServerT Required: []string{"owner", "repo", "issue_number", "sub_issue_id"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1137,7 +1137,7 @@ func GranularRemoveSubIssue(t translations.TranslationHelperFunc) inventory.Serv Required: []string{"owner", "repo", "issue_number", "sub_issue_id"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1214,7 +1214,7 @@ func GranularReprioritizeSubIssue(t translations.TranslationHelperFunc) inventor Required: []string{"owner", "repo", "issue_number", "sub_issue_id"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1379,7 +1379,7 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv Required: []string{"owner", "repo", "issue_number", "fields"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1580,7 +1580,7 @@ func GranularAddIssueReaction(t translations.TranslationHelperFunc) inventory.Se Required: []string{"owner", "repo", "issue_number", "content"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1662,7 +1662,7 @@ func GranularAddIssueCommentReaction(t translations.TranslationHelperFunc) inven Required: []string{"owner", "repo", "comment_id", "content"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/labels.go b/pkg/github/labels.go index b8ea92f892..4be66a7c7d 100644 --- a/pkg/github/labels.go +++ b/pkg/github/labels.go @@ -52,7 +52,7 @@ func GetLabel(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "name"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -154,7 +154,7 @@ func ListLabels(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -274,7 +274,7 @@ func LabelWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"method", "owner", "repo", "name"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { // Get and validate required parameters method, err := RequiredParam[string](args, "method") diff --git a/pkg/github/notifications.go b/pkg/github/notifications.go index cd71a6c9d8..a637951f0a 100644 --- a/pkg/github/notifications.go +++ b/pkg/github/notifications.go @@ -63,7 +63,7 @@ func ListNotifications(t translations.TranslationHelperFunc) inventory.ServerToo }, }), }, - []scopes.Scope{scopes.Notifications}, + scopes.RequireAll(scopes.Notifications), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { client, err := deps.GetClient(ctx) if err != nil { @@ -189,7 +189,7 @@ func DismissNotification(t translations.TranslationHelperFunc) inventory.ServerT Required: []string{"threadID", "state"}, }, }, - []scopes.Scope{scopes.Notifications}, + scopes.RequireAll(scopes.Notifications), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { client, err := deps.GetClient(ctx) if err != nil { @@ -267,7 +267,7 @@ func MarkAllNotificationsRead(t translations.TranslationHelperFunc) inventory.Se }, }, }, - []scopes.Scope{scopes.Notifications}, + scopes.RequireAll(scopes.Notifications), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { client, err := deps.GetClient(ctx) if err != nil { @@ -352,7 +352,7 @@ func GetNotificationDetails(t translations.TranslationHelperFunc) inventory.Serv Required: []string{"notificationID"}, }, }, - []scopes.Scope{scopes.Notifications}, + scopes.RequireAll(scopes.Notifications), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { client, err := deps.GetClient(ctx) if err != nil { @@ -433,7 +433,7 @@ func ManageNotificationSubscription(t translations.TranslationHelperFunc) invent Required: []string{"notificationID", "action"}, }, }, - []scopes.Scope{scopes.Notifications}, + scopes.RequireAll(scopes.Notifications), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { client, err := deps.GetClient(ctx) if err != nil { @@ -534,7 +534,7 @@ func ManageRepositoryNotificationSubscription(t translations.TranslationHelperFu Required: []string{"owner", "repo", "action"}, }, }, - []scopes.Scope{scopes.Notifications}, + scopes.RequireAll(scopes.Notifications), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { client, err := deps.GetClient(ctx) if err != nil { diff --git a/pkg/github/projects.go b/pkg/github/projects.go index 57a5dc145b..df6d8ac190 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -357,7 +357,7 @@ Use this tool to list projects for a user or organization, or list project field Required: []string{"method", "owner"}, }, }, - []scopes.Scope{scopes.ReadProject}, + scopes.RequireAll(scopes.ReadProject), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { @@ -521,7 +521,7 @@ Use this tool to get details about individual projects, project fields, project Required: []string{"method"}, }, }, - []scopes.Scope{scopes.ReadProject}, + scopes.RequireAll(scopes.ReadProject), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { @@ -891,7 +891,7 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"method", "owner"}, }, }, - []scopes.Scope{scopes.Project}, + scopes.RequireAll(scopes.Project), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 8801ec2894..38e6e3d820 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -78,7 +78,7 @@ Possible options: }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { @@ -706,7 +706,7 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo Required: []string{"owner", "repo", "title", "head", "base"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(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 { @@ -923,7 +923,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(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 { @@ -1216,7 +1216,7 @@ func AddReplyToPullRequestComment(t translations.TranslationHelperFunc) inventor }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1377,7 +1377,7 @@ func ListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1529,7 +1529,7 @@ func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1651,7 +1651,7 @@ func SearchPullRequests(t translations.TranslationHelperFunc) inventory.ServerTo }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { options := []searchOption{ifcSearchPostProcessOption(ctx, deps)} fields, err := OptionalStringArrayParam(args, "fields") @@ -1700,7 +1700,7 @@ func UpdatePullRequestBranch(t translations.TranslationHelperFunc) inventory.Ser }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1834,7 +1834,7 @@ Available methods: }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { var params PullRequestReviewWriteParams if err := mapstructure.WeakDecode(args, ¶ms); err != nil { @@ -2327,7 +2327,7 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/pullrequests_granular.go b/pkg/github/pullrequests_granular.go index c727beb6e5..5da812f271 100644 --- a/pkg/github/pullrequests_granular.go +++ b/pkg/github/pullrequests_granular.go @@ -62,7 +62,7 @@ func prUpdateTool( Required: required, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -195,7 +195,7 @@ func GranularUpdatePullRequestDraftState(t translations.TranslationHelperFunc) i Required: []string{"owner", "repo", "pullNumber", "draft"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -304,7 +304,7 @@ func GranularRequestPullRequestReviewers(t translations.TranslationHelperFunc) i Required: []string{"owner", "repo", "pullNumber", "reviewers"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -397,7 +397,7 @@ func GranularCreatePullRequestReview(t translations.TranslationHelperFunc) inven Required: []string{"owner", "repo", "pullNumber"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -465,7 +465,7 @@ func GranularSubmitPendingPullRequestReview(t translations.TranslationHelperFunc Required: []string{"owner", "repo", "pullNumber", "event"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -527,7 +527,7 @@ func GranularDeletePendingPullRequestReview(t translations.TranslationHelperFunc Required: []string{"owner", "repo", "pullNumber"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -589,7 +589,7 @@ func GranularAddPullRequestReviewComment(t translations.TranslationHelperFunc) i Required: []string{"owner", "repo", "pullNumber", "path", "body", "subjectType"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -694,7 +694,7 @@ func GranularResolveReviewThread(t translations.TranslationHelperFunc) inventory Required: []string{"threadID"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { threadID, err := RequiredParam[string](args, "threadID") if err != nil { @@ -738,7 +738,7 @@ func GranularUnresolveReviewThread(t translations.TranslationHelperFunc) invento Required: []string{"threadID"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { threadID, err := RequiredParam[string](args, "threadID") if err != nil { @@ -796,7 +796,7 @@ func GranularAddPullRequestReviewCommentReaction(t translations.TranslationHelpe Required: []string{"owner", "repo", "comment_id", "content"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 446d7ca551..5fcb9d9181 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -61,7 +61,7 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "sha"}, }), }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -188,7 +188,7 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -337,7 +337,7 @@ func ListBranches(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo"}, }), }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -405,7 +405,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", @@ -461,7 +461,7 @@ SHA MUST be provided for existing file updates. Required: []string{"owner", "repo", "path", "content", "message", "branch"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -475,6 +475,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 @@ -518,8 +522,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} @@ -623,6 +625,9 @@ SHA MUST be provided for existing file updates. return MarshalledTextResult(minimalResponse), nil, nil }, ) + tool.ScopeAccess.Challenge = workflowScopeChallengeForPath + tool.ScopeAccess.Scopes = []string{string(scopes.Repo), string(scopes.Workflow)} + return tool } // CreateRepository creates a tool to create a new GitHub repository. @@ -664,7 +669,7 @@ func CreateRepository(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"name"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { name, err := RequiredParam[string](args, "name") if err != nil { @@ -774,7 +779,7 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.DeleteRepo, scopes.Repo}, + scopes.RequireAll(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 { @@ -906,7 +911,6 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool ) tool.MinimumProtocolVersion = inventory.ProtocolVersionMultiRoundTrip tool.RequiredElicitationMode = inventory.ElicitationModeForm - tool.RequiredScopeGroups = scopes.ExpandScopeGroups(scopes.DeleteRepo, scopes.Repo) return tool } @@ -985,7 +989,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1222,7 +1226,7 @@ func ForkRepository(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1292,7 +1296,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", @@ -1329,7 +1333,7 @@ func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "path", "message", "branch"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1343,6 +1347,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 @@ -1473,6 +1481,9 @@ func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultText(string(r)), nil, nil }, ) + tool.ScopeAccess.Challenge = workflowScopeChallengeForPath + tool.ScopeAccess.Scopes = []string{string(scopes.Repo), string(scopes.Workflow)} + return tool } // CreateBranch creates a tool to create a new branch. @@ -1509,7 +1520,7 @@ func CreateBranch(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "branch"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1590,7 +1601,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", @@ -1641,7 +1652,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "branch", "files", "message"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1666,6 +1677,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) @@ -1739,34 +1779,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 { @@ -1821,6 +1833,9 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultText(string(r)), nil, nil }, ) + tool.ScopeAccess.Challenge = workflowScopeChallengeForFiles + tool.ScopeAccess.Scopes = []string{string(scopes.Repo), string(scopes.Workflow)} + return tool } // ListTags creates a tool to list tags in a GitHub repository. @@ -1849,7 +1864,7 @@ func ListTags(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo"}, }), }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -1944,7 +1959,7 @@ func GetTag(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "tag"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -2060,7 +2075,7 @@ func ListReleases(t translations.TranslationHelperFunc) inventory.ServerTool { }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -2176,7 +2191,7 @@ func GetLatestRelease(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -2254,7 +2269,7 @@ func GetReleaseByTag(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"owner", "repo", "tag"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -2342,7 +2357,7 @@ func ListStarredRepositories(t translations.TranslationHelperFunc) inventory.Ser }, }), }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { username, err := OptionalParam[string](args, "username") if err != nil { @@ -2482,7 +2497,7 @@ func StarRepository(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -2547,7 +2562,7 @@ func UnstarRepository(t translations.TranslationHelperFunc) inventory.ServerTool Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.Repo}, + scopes.RequireAll(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -2754,7 +2769,7 @@ func GetFileBlame(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"owner", "repo", "path"}, }), }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -3043,7 +3058,7 @@ func ListRepositoryCollaborators(t translations.TranslationHelperFunc) inventory return schema }(), }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index e45c3c4f47..a1ea5ff340 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -15,7 +15,6 @@ import ( "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" @@ -3599,8 +3598,9 @@ func Test_DeleteRepository(t *testing.T) { assert.True(t, *tool.Annotations.DestructiveHint) assert.Equal(t, inventory.ProtocolVersionMultiRoundTrip, serverTool.MinimumProtocolVersion) assert.Equal(t, inventory.ElicitationModeForm, serverTool.RequiredElicitationMode) - assert.ElementsMatch(t, []string{string(scopes.DeleteRepo), string(scopes.Repo)}, serverTool.RequiredScopes) - assert.Len(t, serverTool.RequiredScopeGroups, 2) + assert.Equal(t, []string{"delete_repo", "repo"}, serverTool.ScopeAccess.Scopes) + assert.NotNil(t, serverTool.ScopeAccess.Visible) + assert.NotNil(t, serverTool.ScopeAccess.Challenge) t.Run("requests exact repository name through elicitation", func(t *testing.T) { client := NewMockedHTTPClient( diff --git a/pkg/github/repository_path.go b/pkg/github/repository_path.go new file mode 100644 index 0000000000..4d502fe1db --- /dev/null +++ b/pkg/github/repository_path.go @@ -0,0 +1,82 @@ +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) { + value = strings.TrimPrefix(value, "/") + 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 workflowScopeChallengeForPath(arguments map[string]any, activeScopes []string) []string { + value, ok := arguments["path"].(string) + if !ok { + return nil + } + cleaned, err := validateRelativePath(value) + if err != nil { + return nil + } + if !isWorkflowPath(cleaned) { + return scopes.ChallengeAll(activeScopes, scopes.Repo) + } + return scopes.ChallengeAll(activeScopes, scopes.Repo, scopes.Workflow) +} + +func workflowScopeChallengeForFiles(arguments map[string]any, activeScopes []string) []string { + files, ok := arguments["files"].([]any) + if !ok { + return nil + } + containsWorkflow := false + for _, file := range files { + fileMap, ok := file.(map[string]any) + if !ok { + return nil + } + value, ok := fileMap["path"].(string) + if !ok { + return nil + } + cleaned, err := validateRelativePath(value) + if err != nil { + return nil + } + if isWorkflowPath(cleaned) { + containsWorkflow = true + } + } + if containsWorkflow { + return scopes.ChallengeAll(activeScopes, scopes.Repo, scopes.Workflow) + } + return scopes.ChallengeAll(activeScopes, scopes.Repo) +} diff --git a/pkg/github/repository_path_test.go b/pkg/github/repository_path_test.go new file mode 100644 index 0000000000..d9178ae2af --- /dev/null +++ b/pkg/github/repository_path_test.go @@ -0,0 +1,149 @@ +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 leading slash", 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: "double leading slash", 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 TestFileWriteWorkflowScopeChallenges(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{"repo", "workflow"}, + }, + { + name: "delete normalized workflow", + tool: DeleteFile(translations.NullTranslationHelper), + args: map[string]any{"path": "./.github/workflows/ci.yml"}, + want: []string{"repo", "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{"repo", "workflow"}, + }, + { + name: "push validates entries after workflow", + tool: PushFiles(translations.NullTranslationHelper), + args: map[string]any{"files": []any{ + map[string]any{"path": ".github/workflows/ci.yml"}, + map[string]any{"path": "../unsafe.txt"}, + }}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.NotNil(t, tt.tool.ScopeAccess.Challenge) + assert.Equal(t, tt.want, tt.tool.ScopeAccess.Challenge(tt.args, []string{"repo"})) + }) + } +} + +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/github/scope_filter.go b/pkg/github/scope_filter.go index add0ee3b18..f5c1a46e38 100644 --- a/pkg/github/scope_filter.go +++ b/pkg/github/scope_filter.go @@ -4,32 +4,8 @@ import ( "context" "github.com/github/github-mcp-server/pkg/inventory" - "github.com/github/github-mcp-server/pkg/scopes" ) -// repoScopesSet contains scopes that grant access to repository content. -// Tools requiring only these scopes work on public repos without any token scope, -// so we don't filter them out even if the token lacks repo/public_repo. -var repoScopesSet = map[string]bool{ - string(scopes.Repo): true, - string(scopes.PublicRepo): true, -} - -// onlyRequiresRepoScopes returns true if all of the tool's accepted scopes -// are repo-related scopes (repo, public_repo). Such tools work on public -// repositories without needing any scope. -func onlyRequiresRepoScopes(acceptedScopes []string) bool { - if len(acceptedScopes) == 0 { - return false - } - for _, scope := range acceptedScopes { - if !repoScopesSet[scope] { - return false - } - } - return true -} - // CreateToolScopeFilter creates an inventory.ToolFilter that filters tools // based on the token's OAuth scopes. // @@ -41,9 +17,8 @@ func onlyRequiresRepoScopes(acceptedScopes []string) bool { // token is known at startup and won't change during the session. // // The filter returns true (include tool) if: -// - The tool has no scope requirements (AcceptedScopes is empty) -// - The tool is read-only and only requires repo/public_repo scopes (works on public repos) -// - The token has at least one of the tool's accepted scopes +// - The tool does not define a visibility check +// - The tool's visibility check accepts the token scopes // // Example usage: // @@ -55,13 +30,9 @@ func onlyRequiresRepoScopes(acceptedScopes []string) bool { // inventory := github.NewInventory(t).WithFilter(filter).Build() func CreateToolScopeFilter(tokenScopes []string) inventory.ToolFilter { return func(_ context.Context, tool *inventory.ServerTool) (bool, error) { - // Read-only tools requiring only repo/public_repo work on public repos without any scope - if tool.Tool.Annotations != nil && tool.Tool.Annotations.ReadOnlyHint && onlyRequiresRepoScopes(tool.AcceptedScopes) { + if tool.ScopeAccess.Visible == nil { return true, nil } - if len(tool.RequiredScopeGroups) > 0 { - return scopes.HasRequiredScopeGroups(tokenScopes, tool.RequiredScopeGroups), nil - } - return scopes.HasRequiredScopes(tokenScopes, tool.AcceptedScopes), nil + return tool.ScopeAccess.Visible(tokenScopes), nil } } diff --git a/pkg/github/scope_filter_test.go b/pkg/github/scope_filter_test.go index 934a848a2d..43ef6335b4 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/scopes" "github.com/github/github-mcp-server/pkg/translations" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" @@ -12,20 +13,18 @@ import ( ) func TestCreateToolScopeFilter(t *testing.T) { - // Create test tools with various scope requirements toolNoScopes := &inventory.ServerTool{ - Tool: mcp.Tool{Name: "no_scopes_tool"}, - AcceptedScopes: nil, + Tool: mcp.Tool{Name: "no_scopes_tool"}, } toolEmptyScopes := &inventory.ServerTool{ - Tool: mcp.Tool{Name: "empty_scopes_tool"}, - AcceptedScopes: []string{}, + Tool: mcp.Tool{Name: "empty_scopes_tool"}, + ScopeAccess: scopes.NoScopes(), } toolRepoScope := &inventory.ServerTool{ - Tool: mcp.Tool{Name: "repo_tool"}, - AcceptedScopes: []string{"repo"}, + Tool: mcp.Tool{Name: "repo_tool"}, + ScopeAccess: scopes.RequireAll(scopes.Repo), } toolRepoScopeReadOnly := &inventory.ServerTool{ @@ -33,12 +32,12 @@ func TestCreateToolScopeFilter(t *testing.T) { Name: "repo_tool_readonly", Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, }, - AcceptedScopes: []string{"repo"}, + ScopeAccess: publicReadAccess(scopes.Repo), } toolPublicRepoScope := &inventory.ServerTool{ - Tool: mcp.Tool{Name: "public_repo_tool"}, - AcceptedScopes: []string{"public_repo", "repo"}, // repo is parent, also accepted + Tool: mcp.Tool{Name: "public_repo_tool"}, + ScopeAccess: scopes.RequireAll(scopes.PublicRepo), } toolPublicRepoScopeReadOnly := &inventory.ServerTool{ @@ -46,23 +45,26 @@ func TestCreateToolScopeFilter(t *testing.T) { Name: "public_repo_tool_readonly", Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, }, - AcceptedScopes: []string{"public_repo", "repo"}, + ScopeAccess: publicReadAccess(scopes.PublicRepo), } toolGistScope := &inventory.ServerTool{ - Tool: mcp.Tool{Name: "gist_tool"}, - AcceptedScopes: []string{"gist"}, + Tool: mcp.Tool{Name: "gist_tool"}, + ScopeAccess: scopes.RequireAll(scopes.Gist), } toolMultiScope := &inventory.ServerTool{ - Tool: mcp.Tool{Name: "multi_scope_tool"}, - AcceptedScopes: []string{"repo", "admin:org"}, + Tool: mcp.Tool{Name: "multi_scope_tool"}, + ScopeAccess: inventory.ScopeAccess{ + Visible: func(activeScopes []string) bool { + return scopes.HasAll(activeScopes, scopes.Repo) || scopes.HasAll(activeScopes, scopes.AdminOrg) + }, + }, } toolConjunctiveScopes := &inventory.ServerTool{ - Tool: mcp.Tool{Name: "conjunctive_scope_tool"}, - AcceptedScopes: []string{"delete_repo", "repo"}, - RequiredScopeGroups: [][]string{{"delete_repo"}, {"repo"}}, + Tool: mcp.Tool{Name: "conjunctive_scope_tool"}, + ScopeAccess: scopes.RequireAll(scopes.DeleteRepo, scopes.Repo), } tests := []struct { @@ -166,19 +168,19 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) { // Test integration with inventory builder tools := []inventory.ServerTool{ { - Tool: mcp.Tool{Name: "public_tool"}, - Toolset: inventory.ToolsetMetadata{ID: "test"}, - AcceptedScopes: nil, // No scopes required + Tool: mcp.Tool{Name: "public_tool"}, + Toolset: inventory.ToolsetMetadata{ID: "test"}, + ScopeAccess: scopes.NoScopes(), }, { - Tool: mcp.Tool{Name: "repo_tool"}, - Toolset: inventory.ToolsetMetadata{ID: "test"}, - AcceptedScopes: []string{"repo"}, + Tool: mcp.Tool{Name: "repo_tool"}, + Toolset: inventory.ToolsetMetadata{ID: "test"}, + ScopeAccess: scopes.RequireAll(scopes.Repo), }, { - Tool: mcp.Tool{Name: "gist_tool"}, - Toolset: inventory.ToolsetMetadata{ID: "test"}, - AcceptedScopes: []string{"gist"}, + Tool: mcp.Tool{Name: "gist_tool"}, + Toolset: inventory.ToolsetMetadata{ID: "test"}, + ScopeAccess: scopes.RequireAll(scopes.Gist), }, } @@ -209,8 +211,9 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) { assert.NotContains(t, toolNames, "gist_tool") } -func TestCreateToolScopeFilterPreservesExistingMultiScopeSemantics(t *testing.T) { +func TestCreateToolScopeFilterSupportsAlternativePaths(t *testing.T) { filter := CreateToolScopeFilter([]string{"repo"}) + unscopedFilter := CreateToolScopeFilter(nil) tools := []inventory.ServerTool{ ListIssueFields(translations.NullTranslationHelper), ListIssueTypes(translations.NullTranslationHelper), @@ -220,7 +223,16 @@ func TestCreateToolScopeFilterPreservesExistingMultiScopeSemantics(t *testing.T) 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) + assert.True(t, allowed, "%s should be visible with a repo-only token", tools[i].Tool.Name) + + allowed, err = unscopedFilter(context.Background(), &tools[i]) + require.NoError(t, err) + assert.True(t, allowed, "%s should remain visible when a public-repository path is plausible", tools[i].Tool.Name) } } + +func publicReadAccess(required scopes.Scope) inventory.ScopeAccess { + access := scopes.RequireAll(required) + access.Visible = func([]string) bool { return true } + return access +} diff --git a/pkg/github/search.go b/pkg/github/search.go index 3160209318..715ca83aed 100644 --- a/pkg/github/search.go +++ b/pkg/github/search.go @@ -58,7 +58,7 @@ func SearchRepositories(t translations.TranslationHelperFunc) inventory.ServerTo }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { query, err := RequiredParam[string](args, "query") if err != nil { @@ -229,7 +229,7 @@ func SearchCode(t translations.TranslationHelperFunc) inventory.ServerTool { }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { query, err := RequiredParam[string](args, "query") if err != nil { @@ -474,7 +474,7 @@ func SearchUsers(t translations.TranslationHelperFunc) inventory.ServerTool { }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { return userOrOrgHandler(ctx, "user", deps, args) }, @@ -516,7 +516,7 @@ func SearchOrgs(t translations.TranslationHelperFunc) inventory.ServerTool { }, InputSchema: schema, }, - []scopes.Scope{scopes.ReadOrg}, + scopes.RequireAll(scopes.ReadOrg), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { return userOrOrgHandler(ctx, "org", deps, args) }, @@ -558,7 +558,7 @@ func SearchCommits(t translations.TranslationHelperFunc) inventory.ServerTool { }, InputSchema: schema, }, - []scopes.Scope{scopes.Repo}, + scopes.PublicRead(scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { query, err := RequiredParam[string](args, "query") if err != nil { diff --git a/pkg/github/secret_scanning.go b/pkg/github/secret_scanning.go index de444faea7..1991d5d2be 100644 --- a/pkg/github/secret_scanning.go +++ b/pkg/github/secret_scanning.go @@ -47,7 +47,7 @@ func GetSecretScanningAlert(t translations.TranslationHelperFunc) inventory.Serv Required: []string{"owner", "repo", "alertNumber"}, }, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -142,7 +142,7 @@ func ListSecretScanningAlerts(t translations.TranslationHelperFunc) inventory.Se }, InputSchema: schema, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/security_advisories.go b/pkg/github/security_advisories.go index 4f718de767..63a9a43b0e 100644 --- a/pkg/github/security_advisories.go +++ b/pkg/github/security_advisories.go @@ -85,7 +85,7 @@ func ListGlobalSecurityAdvisories(t translations.TranslationHelperFunc) inventor }, }, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { client, err := deps.GetClient(ctx) if err != nil { @@ -254,7 +254,7 @@ func ListRepositorySecurityAdvisories(t translations.TranslationHelperFunc) inve Required: []string{"owner", "repo"}, }, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -349,7 +349,7 @@ func GetGlobalSecurityAdvisory(t translations.TranslationHelperFunc) inventory.S Required: []string{"ghsaId"}, }, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { client, err := deps.GetClient(ctx) if err != nil { @@ -425,7 +425,7 @@ func ListOrgRepositorySecurityAdvisories(t translations.TranslationHelperFunc) i Required: []string{"org"}, }, }, - []scopes.Scope{scopes.SecurityEvents}, + scopes.RequireAll(scopes.SecurityEvents), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { org, err := RequiredParam[string](args, "org") if err != nil { diff --git a/pkg/github/tool_scopes.go b/pkg/github/tool_scopes.go new file mode 100644 index 0000000000..e56e6f7659 --- /dev/null +++ b/pkg/github/tool_scopes.go @@ -0,0 +1,60 @@ +package github + +import ( + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" +) + +func repositoryOrOrganizationScopeAccess() inventory.ScopeAccess { + return inventory.ScopeAccess{ + Scopes: []string{string(scopes.Repo), string(scopes.ReadOrg)}, + Visible: func([]string) bool { + // Repository reads may target public repositories. + return true + }, + Challenge: func(arguments map[string]any, activeScopes []string) []string { + if owner, ok := arguments["owner"].(string); !ok || owner == "" { + return nil + } + repoValue, hasRepo := arguments["repo"] + if !hasRepo || repoValue == nil || repoValue == "" { + return scopes.ChallengeAll(activeScopes, scopes.ReadOrg) + } + if _, ok := repoValue.(string); !ok { + return nil + } + return scopes.ChallengeAll(activeScopes, scopes.Repo) + }, + } +} + +func uiGetScopeAccess() inventory.ScopeAccess { + return inventory.ScopeAccess{ + Scopes: []string{string(scopes.Repo), string(scopes.ReadOrg)}, + Visible: func([]string) bool { + return true + }, + Challenge: func(arguments map[string]any, activeScopes []string) []string { + if owner, ok := arguments["owner"].(string); !ok || owner == "" { + return nil + } + method, ok := arguments["method"].(string) + if !ok { + return nil + } + if method == "issue_types" { + return scopes.ChallengeAll(activeScopes, scopes.ReadOrg) + } + if method == "reviewers" { + return scopes.ChallengeAll(activeScopes, scopes.Repo, scopes.ReadOrg) + } + switch method { + case "labels", "assignees", "milestones", "branches", "issue_fields": + if repo, ok := arguments["repo"].(string); ok && repo != "" { + return scopes.ChallengeAll(activeScopes, scopes.Repo) + } + } + return nil + }, + } +} diff --git a/pkg/github/tool_scopes_test.go b/pkg/github/tool_scopes_test.go new file mode 100644 index 0000000000..9700bf33eb --- /dev/null +++ b/pkg/github/tool_scopes_test.go @@ -0,0 +1,79 @@ +package github + +import ( + "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 TestConditionalToolScopeChecks(t *testing.T) { + tests := []struct { + name string + tool inventory.ServerTool + arguments map[string]any + allowed []string + disallowed []string + }{ + { + name: "issue fields repository route", + tool: ListIssueFields(translations.NullTranslationHelper), + arguments: map[string]any{"owner": "octo", "repo": "repo"}, + allowed: []string{"repo"}, + disallowed: []string{"read:org"}, + }, + { + name: "issue fields organization route", + tool: ListIssueFields(translations.NullTranslationHelper), + arguments: map[string]any{"owner": "octo"}, + allowed: []string{"read:org"}, + disallowed: []string{"repo"}, + }, + { + name: "issue types repository route", + tool: ListIssueTypes(translations.NullTranslationHelper), + arguments: map[string]any{"owner": "octo", "repo": "repo"}, + allowed: []string{"repo"}, + disallowed: []string{"read:org"}, + }, + { + name: "issue types organization route", + tool: ListIssueTypes(translations.NullTranslationHelper), + arguments: map[string]any{"owner": "octo"}, + allowed: []string{"admin:org"}, + disallowed: []string{"repo"}, + }, + { + name: "ui repository method", + tool: UIGet(translations.NullTranslationHelper), + arguments: map[string]any{"method": "labels", "owner": "octo", "repo": "repo"}, + allowed: []string{"repo"}, + disallowed: []string{"read:org"}, + }, + { + name: "ui organization method", + tool: UIGet(translations.NullTranslationHelper), + arguments: map[string]any{"method": "issue_types", "owner": "octo"}, + allowed: []string{"write:org"}, + disallowed: []string{"repo"}, + }, + { + name: "ui reviewers method", + tool: UIGet(translations.NullTranslationHelper), + arguments: map[string]any{"method": "reviewers", "owner": "octo", "repo": "repo"}, + allowed: []string{"repo", "read:org"}, + disallowed: []string{"repo"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.NotNil(t, tt.tool.ScopeAccess.Challenge) + assert.Empty(t, tt.tool.ScopeAccess.Challenge(tt.arguments, tt.allowed)) + assert.NotEmpty(t, tt.tool.ScopeAccess.Challenge(tt.arguments, tt.disallowed)) + assert.True(t, tt.tool.ScopeAccess.Visible(nil)) + }) + } +} diff --git a/pkg/github/ui_tools.go b/pkg/github/ui_tools.go index 3fa4afcb15..62bba06ef6 100644 --- a/pkg/github/ui_tools.go +++ b/pkg/github/ui_tools.go @@ -11,7 +11,6 @@ import ( ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/inventory" - "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" @@ -68,7 +67,7 @@ func UIGet(t translations.TranslationHelperFunc) inventory.ServerTool { Required: []string{"method", "owner"}, }, }, - []scopes.Scope{scopes.Repo, scopes.ReadOrg}, + uiGetScopeAccess(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { method, err := RequiredParam[string](args, "method") if err != nil { diff --git a/pkg/http/middleware/scope_challenge.go b/pkg/http/middleware/scope_challenge.go index 4ac3bab906..f4ae9aa4bf 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) @@ -85,15 +87,10 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter } toolName = mcpRequest.Params.Name + arguments = mcpRequest.Params.Arguments } - toolScopeInfo, err := scopes.GetToolScopeInfo(toolName) - if err != nil { - next.ServeHTTP(w, r) - return - } - - // If tool not found in scope map, allow the request - if toolScopeInfo == nil { + checkScopes := scopes.GetToolScopeChallenge(toolName) + if checkScopes == nil { next.ServeHTTP(w, r) return } @@ -102,45 +99,38 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter // This allows Remote Server to pass scope info to avoid redundant GitHub API calls. activeScopes, ok := ghcontext.GetTokenScopes(ctx) if !ok || (len(activeScopes) == 0 && tokenInfo.Token != "") { - activeScopes, err = scopeFetcher.FetchTokenScopes(ctx, tokenInfo.Token) + fetchedScopes, err := scopeFetcher.FetchTokenScopes(ctx, tokenInfo.Token) if err != nil { next.ServeHTTP(w, r) return } + activeScopes = fetchedScopes } // Store active scopes in context for downstream use ctx = ghcontext.WithTokenScopes(ctx, activeScopes) r = r.WithContext(ctx) - // Check if user has the required scopes - if toolScopeInfo.HasAcceptedScope(activeScopes...) { + challengeScopes := checkScopes(arguments, activeScopes) + if len(challengeScopes) == 0 { next.ServeHTTP(w, r) return } - // User lacks required scopes - get the scopes they need - requiredScopes := toolScopeInfo.GetRequiredScopesSlice() - // Build the resource metadata URL using the shared utility // GetEffectiveResourcePath returns the original path (e.g., /mcp or /mcp/x/all) // which is used to construct the well-known OAuth protected resource URL resourcePath := oauth.ResolveResourcePath(r, oauthCfg) resourceMetadataURL := oauth.BuildResourceMetadataURL(r, oauthCfg, resourcePath) - // Build recommended scopes: existing scopes + required scopes - recommendedScopes := make([]string, 0, len(activeScopes)+len(requiredScopes)) - recommendedScopes = append(recommendedScopes, activeScopes...) - recommendedScopes = append(recommendedScopes, requiredScopes...) - // Build the WWW-Authenticate header value wwwAuthenticateHeader := fmt.Sprintf(`Bearer error="insufficient_scope", scope=%q, resource_metadata=%q, error_description=%q`, - strings.Join(recommendedScopes, " "), + strings.Join(challengeScopes, " "), resourceMetadataURL, - "Additional scopes required: "+strings.Join(requiredScopes, ", "), + "Additional scopes required: "+strings.Join(challengeScopes, ", "), ) - // Send scope challenge response with the superset of existing and required scopes + // Send the exact scope set returned by the tool's scope check. w.Header().Set("WWW-Authenticate", wwwAuthenticateHeader) http.Error(w, "Forbidden: insufficient scopes", http.StatusForbidden) } diff --git a/pkg/http/middleware/scope_challenge_test.go b/pkg/http/middleware/scope_challenge_test.go index 9ce67bc7cd..0484e72105 100644 --- a/pkg/http/middleware/scope_challenge_test.go +++ b/pkg/http/middleware/scope_challenge_test.go @@ -1,6 +1,8 @@ package middleware import ( + "bytes" + "context" "io" "net/http" "net/http/httptest" @@ -9,6 +11,7 @@ import ( 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" "github.com/stretchr/testify/require" @@ -82,3 +85,119 @@ func TestWithScopeChallenge_MaxBodySize(t *testing.T) { assert.Equal(t, body, capturedBody, "body should be preserved for downstream handlers") }) } + +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: repo, 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: repo, workflow") +} + +func setDynamicScopeTestMap(t *testing.T) { + t.Helper() + scopes.SetGlobalToolScopeMap(scopes.ToolScopeMap{ + "write_file": func(arguments map[string]any, activeScopes []string) []string { + if path, _ := arguments["path"].(string); strings.HasPrefix(path, ".github/workflows/") { + return scopes.ChallengeAll(activeScopes, scopes.Repo, scopes.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 scopes.ChallengeAll(activeScopes, scopes.Repo, scopes.Workflow) + } + } + return scopes.ChallengeAll(activeScopes, scopes.Repo) + }, + }) + 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/inventory/server_tool.go b/pkg/inventory/server_tool.go index 1d8cbcf885..42d0136057 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -19,6 +19,28 @@ import ( // should define their own typed dependencies struct and type-assert as needed. type HandlerFunc func(deps any) mcp.ToolHandler +// ScopeVisibility reports whether a token can use any form of a tool. +type ScopeVisibility func(activeScopes []string) bool + +// ScopeChallenge returns the exact scopes to include in an OAuth challenge. +// An empty result means the call can continue. +type ScopeChallenge func(arguments map[string]any, activeScopes []string) []string + +// ScopeAccess contains scope metadata and the two checks used by the server. +type ScopeAccess struct { + // Scopes lists every scope this tool may request. It is used for + // documentation and the list-scopes command. + Scopes []string + + // Visible is used when filtering tools for a fixed-scope token. + // Nil means the tool remains visible. + Visible ScopeVisibility + + // Challenge evaluates one call before its handler runs. + // Nil means the call does not use OAuth scope challenges. + Challenge ScopeChallenge +} + // 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 @@ -90,18 +112,8 @@ type ServerTool struct { // 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 - - // AcceptedScopes specifies all OAuth scopes that can be used with this tool. - // 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 + // ScopeAccess controls fixed-token visibility and per-call OAuth challenges. + ScopeAccess ScopeAccess } // IsReadOnly returns true if this tool is marked as read-only via annotations. @@ -139,21 +151,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 9e32b5f30c..08bb8fe553 100644 --- a/pkg/inventory/server_tool_test.go +++ b/pkg/inventory/server_tool_test.go @@ -167,6 +167,7 @@ func TestAnnotateHeaderParams(t *testing.T) { Properties: map[string]*jsonschema.Schema{ "owner": {Type: "string"}, "repo": {Type: "string"}, + "path": {Type: "string"}, "detail": {Type: "string"}, }, }} @@ -174,6 +175,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..27c8fb1ac5 100644 --- a/pkg/scopes/map.go +++ b/pkg/scopes/map.go @@ -2,154 +2,33 @@ package scopes import "github.com/github/github-mcp-server/pkg/inventory" -// ToolScopeMap maps tool names to their scope requirements. -type ToolScopeMap map[string]*ToolScopeInfo +// ToolScopeMap maps tool names to their per-call scope checks. +type ToolScopeMap map[string]inventory.ScopeChallenge -// ToolScopeInfo contains scope information for a single tool. -type ToolScopeInfo struct { - // RequiredScopes contains the scopes that are directly required by this tool. - RequiredScopes []string - - // 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 var globalToolScopeMap ToolScopeMap -// SetToolScopeMapFromInventory builds and stores a tool scope map from an inventory. -// This should be called after building the inventory to make scopes available for middleware. +// SetToolScopeMapFromInventory builds and stores the scope checks from an inventory. func SetToolScopeMapFromInventory(inv *inventory.Inventory) { globalToolScopeMap = GetToolScopeMapFromInventory(inv) } -// SetGlobalToolScopeMap sets the global tool scope map directly. -// This is useful for testing when you don't have a full inventory. +// SetGlobalToolScopeMap sets the scope map directly. func SetGlobalToolScopeMap(m ToolScopeMap) { globalToolScopeMap = m } -// GetToolScopeMap returns the global tool scope map. -// Returns an empty map if SetToolScopeMapFromInventory hasn't been called yet. -func GetToolScopeMap() (ToolScopeMap, error) { - if globalToolScopeMap == nil { - return make(ToolScopeMap), nil - } - return globalToolScopeMap, nil +// GetToolScopeChallenge returns the scope check for a tool. +func GetToolScopeChallenge(toolName string) inventory.ScopeChallenge { + return globalToolScopeMap[toolName] } -// GetToolScopeInfo returns scope information for a specific tool from the global scope map. -func GetToolScopeInfo(toolName string) (*ToolScopeInfo, error) { - m, err := GetToolScopeMap() - if err != nil { - return nil, err - } - return m[toolName], nil -} - -// GetToolScopeMapFromInventory builds a tool scope map from an inventory. -// This extracts scope information from ServerTool.RequiredScopes and ServerTool.AcceptedScopes. +// GetToolScopeMapFromInventory builds a scope map from an inventory. func GetToolScopeMapFromInventory(inv *inventory.Inventory) ToolScopeMap { result := make(ToolScopeMap) - - // Get all tools from the inventory (both enabled and disabled) - // We need all tools for scope checking purposes - allTools := inv.AllTools() - for i := range allTools { - tool := &allTools[i] - if len(tool.RequiredScopes) > 0 || len(tool.AcceptedScopes) > 0 { - result[tool.Tool.Name] = &ToolScopeInfo{ - RequiredScopes: tool.RequiredScopes, - AcceptedScopes: tool.AcceptedScopes, - RequiredScopeGroups: tool.RequiredScopeGroups, - } + for _, tool := range inv.AllTools() { + if tool.ScopeAccess.Challenge != nil { + result[tool.Tool.Name] = tool.ScopeAccess.Challenge } } - return result } - -// 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 - } - - userScopeSet := make(map[string]bool) - for _, scope := range userScopes { - userScopeSet[scope] = true - } - - for _, scope := range t.AcceptedScopes { - if userScopeSet[scope] { - return true - } - } - return false -} - -// MissingScopes returns the required scopes that are not present in the user's scopes. -func (t *ToolScopeInfo) MissingScopes(userScopes ...string) []string { - if t == nil || len(t.RequiredScopes) == 0 { - return nil - } - - // Create a set of user scopes for O(1) lookup - userScopeSet := make(map[string]bool, len(userScopes)) - for _, s := range userScopes { - 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 { - if userScopeSet[scope] { - hasAccepted = true - break - } - } - - if hasAccepted { - return nil // User has sufficient scopes - } - - // Return required scopes as the minimum needed - missing := make([]string, len(t.RequiredScopes)) - copy(missing, t.RequiredScopes) - return missing -} - -// GetRequiredScopesSlice returns the required scopes as a slice of strings. -func (t *ToolScopeInfo) GetRequiredScopesSlice() []string { - if t == nil { - return nil - } - scopes := make([]string, len(t.RequiredScopes)) - copy(scopes, t.RequiredScopes) - return scopes -} diff --git a/pkg/scopes/map_test.go b/pkg/scopes/map_test.go index 3c5a7ede72..fc576ec37c 100644 --- a/pkg/scopes/map_test.go +++ b/pkg/scopes/map_test.go @@ -3,223 +3,42 @@ package scopes import ( "testing" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestGetToolScopeMap(t *testing.T) { - // Reset and set up a test map - SetGlobalToolScopeMap(ToolScopeMap{ - "test_tool": &ToolScopeInfo{ - RequiredScopes: []string{"read:org"}, - AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, - }, - }) - - m, err := GetToolScopeMap() - require.NoError(t, err) - require.NotNil(t, m) - require.Greater(t, len(m), 0, "expected at least one tool in the scope map") - - testTool, ok := m["test_tool"] - require.True(t, ok, "expected test_tool to be in the scope map") - assert.Contains(t, testTool.RequiredScopes, "read:org") - assert.Contains(t, testTool.AcceptedScopes, "read:org") - assert.Contains(t, testTool.AcceptedScopes, "admin:org") -} - -func TestGetToolScopeInfo(t *testing.T) { - // Set up test scope map - SetGlobalToolScopeMap(ToolScopeMap{ - "search_orgs": &ToolScopeInfo{ - RequiredScopes: []string{"read:org"}, - AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, - }, - }) - - info, err := GetToolScopeInfo("search_orgs") +func TestGetToolScopeMapFromInventory(t *testing.T) { + access := RequireAll(ReadOrg) + inv, err := inventory.NewBuilder(). + SetTools([]inventory.ServerTool{ + { + Tool: mcp.Tool{Name: "scoped"}, + Toolset: inventory.ToolsetMetadata{ID: "test"}, + ScopeAccess: access, + }, + { + Tool: mcp.Tool{Name: "unscoped"}, + Toolset: inventory.ToolsetMetadata{ID: "test"}, + }, + }). + WithToolsets([]string{"test"}). + Build() require.NoError(t, err) - require.NotNil(t, info) - // Non-existent tool should return nil - info, err = GetToolScopeInfo("nonexistent_tool") - require.NoError(t, err) - assert.Nil(t, info) + scopeMap := GetToolScopeMapFromInventory(inv) + require.Contains(t, scopeMap, "scoped") + assert.Empty(t, scopeMap["scoped"](nil, []string{"admin:org"})) + assert.Equal(t, []string{"read:org"}, scopeMap["scoped"](nil, []string{"repo"})) + assert.NotContains(t, scopeMap, "unscoped") } -func TestToolScopeInfo_HasAcceptedScope(t *testing.T) { - testCases := []struct { - name string - scopeInfo *ToolScopeInfo - userScopes []string - expected bool - }{ - { - name: "has exact required scope", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{"read:org"}, - AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, - }, - userScopes: []string{"read:org"}, - expected: true, - }, - { - name: "has parent scope (admin:org grants read:org)", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{"read:org"}, - AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, - }, - userScopes: []string{"admin:org"}, - expected: true, - }, - { - name: "has parent scope (write:org grants read:org)", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{"read:org"}, - AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, - }, - userScopes: []string{"write:org"}, - expected: true, - }, - { - name: "missing required scope", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{"read:org"}, - AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, - }, - userScopes: []string{"repo"}, - expected: false, - }, - { - name: "no scope required", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{}, - AcceptedScopes: []string{}, - }, - userScopes: []string{}, - expected: true, - }, - { - name: "nil scope info", - scopeInfo: nil, - userScopes: []string{}, - expected: true, - }, - { - name: "repo scope for tool requiring repo", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{"repo"}, - AcceptedScopes: []string{"repo"}, - }, - userScopes: []string{"repo"}, - expected: true, - }, - { - name: "missing repo scope", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{"repo"}, - AcceptedScopes: []string{"repo"}, - }, - 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 { - t.Run(tc.name, func(t *testing.T) { - result := tc.scopeInfo.HasAcceptedScope(tc.userScopes...) - assert.Equal(t, tc.expected, result) - }) - } -} - -func TestToolScopeInfo_MissingScopes(t *testing.T) { - testCases := []struct { - name string - scopeInfo *ToolScopeInfo - userScopes []string - expectedLen int - expectedScopes []string - }{ - { - name: "has required scope - no missing", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{"read:org"}, - AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, - }, - userScopes: []string{"read:org"}, - expectedLen: 0, - expectedScopes: nil, - }, - { - name: "missing scope", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{"read:org"}, - AcceptedScopes: []string{"read:org", "write:org", "admin:org"}, - }, - userScopes: []string{"repo"}, - expectedLen: 1, - expectedScopes: []string{"read:org"}, - }, - { - name: "no scope required - no missing", - scopeInfo: &ToolScopeInfo{ - RequiredScopes: []string{}, - AcceptedScopes: []string{}, - }, - userScopes: []string{}, - expectedLen: 0, - expectedScopes: nil, - }, - { - name: "nil scope info - no missing", - scopeInfo: nil, - userScopes: []string{}, - 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"}, - }, - } +func TestGlobalToolScopeMap(t *testing.T) { + check := RequireAll(Repo).Challenge + SetGlobalToolScopeMap(ToolScopeMap{"repo_tool": check}) + t.Cleanup(func() { SetGlobalToolScopeMap(nil) }) - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - missing := tc.scopeInfo.MissingScopes(tc.userScopes...) - assert.Len(t, missing, tc.expectedLen) - if tc.expectedScopes != nil { - for _, expected := range tc.expectedScopes { - assert.Contains(t, missing, expected) - } - } - }) - } + assert.Equal(t, []string{"repo"}, GetToolScopeChallenge("repo_tool")(nil, nil)) + assert.Nil(t, GetToolScopeChallenge("missing")) } diff --git a/pkg/scopes/scopes.go b/pkg/scopes/scopes.go index 05a7774cd0..2188a28aac 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -1,8 +1,7 @@ package scopes import ( - "slices" - "sort" + "github.com/github/github-mcp-server/pkg/inventory" ) // Scope represents a GitHub OAuth scope. @@ -122,93 +121,61 @@ var ScopeHierarchy = map[Scope][]Scope{ User: {ReadUser, UserEmail}, } -// ScopeSet represents a set of OAuth scopes. -type ScopeSet map[Scope]bool - -// NewScopeSet creates a new ScopeSet from the given scopes. -func NewScopeSet(scopes ...Scope) ScopeSet { - set := make(ScopeSet) - for _, scope := range scopes { - set[scope] = true - } - return set -} - -// ToSlice converts a ScopeSet to a slice of Scope values. -func (s ScopeSet) ToSlice() []Scope { - scopes := make([]Scope, 0, len(s)) - for scope := range s { - scopes = append(scopes, scope) +// RequireAll creates scope checks for a tool that always needs the given scopes. +func RequireAll(required ...Scope) inventory.ScopeAccess { + scopes := scopeStrings(required) + return inventory.ScopeAccess{ + Scopes: scopes, + Visible: func(activeScopes []string) bool { + return HasAll(activeScopes, required...) + }, + Challenge: func(_ map[string]any, activeScopes []string) []string { + if HasAll(activeScopes, required...) { + return nil + } + return append([]string(nil), scopes...) + }, } - // Sort for deterministic output - slices.Sort(scopes) - return scopes } -// ToStringSlice converts a ScopeSet to a slice of string values. -// The returned slice is sorted for deterministic output. -func (s ScopeSet) ToStringSlice() []string { - scopes := make([]string, 0, len(s)) - for scope := range s { - scopes = append(scopes, string(scope)) - } - sort.Strings(scopes) - return scopes +// PublicRead creates checks for a read-only operation that may target public data. +func PublicRead(required ...Scope) inventory.ScopeAccess { + access := RequireAll(required...) + access.Visible = func([]string) bool { return true } + return access } -// ToStringSlice converts a slice of Scopes to a slice of strings. -func ToStringSlice(scopes ...Scope) []string { - result := make([]string, len(scopes)) - for i, scope := range scopes { - result[i] = string(scope) - } - return result +// NoScopes creates scope checks for a tool that does not need OAuth scopes. +func NoScopes() inventory.ScopeAccess { + return inventory.ScopeAccess{} } -// ExpandScopes takes a list of required scopes and returns all accepted scopes -// including parent scopes from the hierarchy. -// For example, if "public_repo" is required, "repo" is also accepted since -// having the "repo" scope grants access to "public_repo". -// The returned slice is sorted for deterministic output. -func ExpandScopes(required ...Scope) []string { - if len(required) == 0 { - return nil - } - - accepted := make(map[string]bool) - - // Add required scopes +// HasAll reports whether a token grants every requested scope. +func HasAll(activeScopes []string, required ...Scope) bool { + granted := expandScopeSet(activeScopes) for _, scope := range required { - accepted[string(scope)] = true - } - - // Add parent scopes that grant access to required scopes - for parent, children := range ScopeHierarchy { - for _, child := range children { - if accepted[string(child)] { - accepted[string(parent)] = true - } + if !granted[string(scope)] { + return false } } + return true +} - // Convert to slice and sort for deterministic output - result := make([]string, 0, len(accepted)) - for scope := range accepted { - result = append(result, scope) +// ChallengeAll returns the complete scope set for an operation, or nil when +// the active token already grants every scope. +func ChallengeAll(activeScopes []string, required ...Scope) []string { + if HasAll(activeScopes, required...) { + return nil } - sort.Strings(result) - return result + return scopeStrings(required) } -// 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)) +func scopeStrings(scopes []Scope) []string { + result := make([]string, len(scopes)) + for i, scope := range scopes { + result[i] = string(scope) } - return groups + return result } // expandScopeSet returns a set of all scopes granted by the given scopes, @@ -217,62 +184,19 @@ func ExpandScopeGroups(required ...Scope) [][]string { // and "security_events" since "repo" grants access to those child scopes. func expandScopeSet(scopes []string) map[string]bool { expanded := make(map[string]bool, len(scopes)) - for _, scope := range scopes { + queue := append([]string(nil), scopes...) + for len(queue) > 0 { + scope := queue[0] + queue = queue[1:] + if expanded[scope] { + continue + } expanded[scope] = true - // Add child scopes granted by this scope - if children, ok := ScopeHierarchy[Scope(scope)]; ok { - for _, child := range children { - expanded[string(child)] = true + for _, child := range ScopeHierarchy[Scope(scope)] { + if !expanded[string(child)] { + queue = append(queue, string(child)) } } } return expanded } - -// HasRequiredScopes checks if tokenScopes satisfy the acceptedScopes requirement. -// A tool's acceptedScopes includes both the required scopes AND parent scopes -// that implicitly grant the required permissions (via ExpandScopes). -// -// For PAT filtering: if ANY of the acceptedScopes are granted by the token -// (directly or via scope hierarchy), the tool should be visible. -// -// Returns true if the tool should be visible to the token holder. -func HasRequiredScopes(tokenScopes []string, acceptedScopes []string) bool { - // No scopes required = always allowed - if len(acceptedScopes) == 0 { - return true - } - - // Expand token scopes to include child scopes they grant - grantedScopes := expandScopeSet(tokenScopes) - - // Check if any accepted scope is granted by the token - for _, accepted := range acceptedScopes { - if grantedScopes[accepted] { - return true - } - } - 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 bf5269da17..ec679d864c 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -1,125 +1,11 @@ package scopes import ( - "sort" "testing" "github.com/stretchr/testify/assert" ) -func TestExpandScopes(t *testing.T) { - tests := []struct { - name string - required []Scope - expected []string - }{ - { - name: "nil returns nil", - required: nil, - expected: nil, - }, - { - name: "empty returns nil", - required: []Scope{}, - expected: nil, - }, - { - name: "repo scope returns just repo", - required: []Scope{Repo}, - expected: []string{"repo"}, - }, - { - name: "public_repo also accepts repo (parent)", - 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}, - expected: []string{"repo", "security_events"}, - }, - { - name: "read:org also accepts write:org and admin:org (parents)", - required: []Scope{ReadOrg}, - expected: []string{"admin:org", "read:org", "write:org"}, - }, - { - name: "write:org also accepts admin:org (parent)", - required: []Scope{WriteOrg}, - expected: []string{"admin:org", "write:org"}, - }, - { - name: "admin:org returns just admin:org (no parent)", - required: []Scope{AdminOrg}, - expected: []string{"admin:org"}, - }, - { - name: "read:project also accepts project (parent)", - required: []Scope{ReadProject}, - expected: []string{"project", "read:project"}, - }, - { - name: "project returns just project (no parent)", - required: []Scope{Project}, - expected: []string{"project"}, - }, - { - name: "gist returns just gist (no parent)", - required: []Scope{Gist}, - expected: []string{"gist"}, - }, - { - name: "notifications returns just notifications (no parent)", - required: []Scope{Notifications}, - expected: []string{"notifications"}, - }, - { - name: "read:packages also accepts write:packages (parent)", - required: []Scope{ReadPackages}, - expected: []string{"read:packages", "write:packages"}, - }, - { - name: "read:user also accepts user (parent)", - required: []Scope{ReadUser}, - expected: []string{"read:user", "user"}, - }, - { - name: "multiple scopes combine correctly", - required: []Scope{PublicRepo, ReadOrg}, - expected: []string{"admin:org", "public_repo", "read:org", "repo", "write:org"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ExpandScopes(tt.required...) - - // Sort both for consistent comparison - if result != nil { - sort.Strings(result) - } - if tt.expected != nil { - sort.Strings(tt.expected) - } - - assert.Equal(t, tt.expected, result) - }) - } -} - -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() @@ -133,35 +19,12 @@ func TestOAuthScopeCatalog(t *testing.T) { assert.NotContains(t, defaults, string(Codespace)) } -func TestToStringSlice(t *testing.T) { - tests := []struct { - name string - scopes []Scope - expected []string - }{ - { - name: "empty returns empty", - scopes: []Scope{}, - expected: []string{}, - }, - { - name: "single scope", - scopes: []Scope{Repo}, - expected: []string{"repo"}, - }, - { - name: "multiple scopes", - scopes: []Scope{Repo, Gist, ReadOrg}, - expected: []string{"repo", "gist", "read:org"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ToStringSlice(tt.scopes...) - assert.Equal(t, tt.expected, result) - }) - } +func TestScopeChecks(t *testing.T) { + assert.True(t, HasAll([]string{"repo", "workflow"}, Repo, Workflow)) + assert.False(t, HasAll([]string{"repo"}, Repo, Workflow)) + assert.True(t, HasAll([]string{"admin:org"}, ReadOrg)) + assert.Nil(t, ChallengeAll([]string{"repo", "workflow"}, Repo, Workflow)) + assert.Equal(t, []string{"repo", "workflow"}, ChallengeAll([]string{"repo"}, Repo, Workflow)) } func TestScopeHierarchy(t *testing.T) { @@ -249,110 +112,3 @@ func TestExpandScopeSet(t *testing.T) { }) } } - -func TestHasRequiredScopes(t *testing.T) { - tests := []struct { - name string - tokenScopes []string - acceptedScopes []string - expected bool - }{ - { - name: "no accepted scopes - always allowed", - tokenScopes: []string{}, - acceptedScopes: []string{}, - expected: true, - }, - { - name: "nil accepted scopes - always allowed", - tokenScopes: []string{"repo"}, - acceptedScopes: nil, - expected: true, - }, - { - name: "token has exact required scope", - tokenScopes: []string{"repo"}, - acceptedScopes: []string{"repo"}, - expected: true, - }, - { - name: "token has parent scope that grants access", - tokenScopes: []string{"repo"}, - acceptedScopes: []string{"public_repo"}, - expected: true, - }, - { - name: "token has parent scope for security_events", - tokenScopes: []string{"repo"}, - acceptedScopes: []string{"security_events"}, - expected: true, - }, - { - name: "token has admin:org which grants read:org", - tokenScopes: []string{"admin:org"}, - acceptedScopes: []string{"read:org"}, - expected: true, - }, - { - name: "token has write:org which grants read:org", - tokenScopes: []string{"write:org"}, - acceptedScopes: []string{"read:org"}, - expected: true, - }, - { - name: "token missing required scope", - tokenScopes: []string{"gist"}, - acceptedScopes: []string{"repo"}, - expected: false, - }, - { - name: "token has child but not parent - fails", - tokenScopes: []string{"public_repo"}, - acceptedScopes: []string{"repo"}, - expected: false, - }, - { - name: "multiple token scopes - one matches", - tokenScopes: []string{"gist", "repo"}, - acceptedScopes: []string{"public_repo"}, - expected: true, - }, - { - name: "multiple accepted scopes - token has one", - tokenScopes: []string{"repo"}, - acceptedScopes: []string{"repo", "admin:org"}, - expected: true, - }, - { - name: "empty token scopes - fails when scopes required", - tokenScopes: []string{}, - acceptedScopes: []string{"repo"}, - expected: false, - }, - { - name: "user scope grants read:user", - tokenScopes: []string{"user"}, - acceptedScopes: []string{"read:user"}, - expected: true, - }, - { - name: "user scope grants user:email", - tokenScopes: []string{"user"}, - acceptedScopes: []string{"user:email"}, - expected: true, - }, - { - name: "write:packages grants read:packages", - tokenScopes: []string{"write:packages"}, - acceptedScopes: []string{"read:packages"}, - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := HasRequiredScopes(tt.tokenScopes, tt.acceptedScopes) - assert.Equal(t, tt.expected, result) - }) - } -}