diff --git a/plugin-tests/github-mcp/check_api_tools.bats b/plugin-tests/github-mcp/check_api_tools.bats index dcf5062..792c399 100644 --- a/plugin-tests/github-mcp/check_api_tools.bats +++ b/plugin-tests/github-mcp/check_api_tools.bats @@ -73,6 +73,45 @@ setup_read_blocking() { assert_output --partial "issue_view" } +@test "read api: blocks GET issues/N → suggests issue_view" { + setup_read_blocking + run_api_hook "$READ_TOOL" "repos/shopware/shopware/issues/123" + assert_failure 2 + assert_output --partial "issue_view" +} + +# Spellings that fetch the same issue: a leading slash, a query string, a +# fragment gh drops before sending, a trailing slash, and gh's placeholders. +_test_issue_spelling_blocked() { + setup_read_blocking + run_api_hook "$READ_TOOL" "$1" + assert_failure 2 + assert_output --partial "issue_view" +} +bats_test_function --description "read api: blocks /repos/O/R/issues/N" -- _test_issue_spelling_blocked "/repos/shopware/shopware/issues/123" +bats_test_function --description "read api: blocks issues/N with a query string" -- _test_issue_spelling_blocked "repos/shopware/shopware/issues/123?per_page=1" +bats_test_function --description "read api: blocks issues/N with a fragment" -- _test_issue_spelling_blocked "repos/shopware/shopware/issues/123#top" +bats_test_function --description "read api: blocks issues/N/ with a trailing slash" -- _test_issue_spelling_blocked "repos/shopware/shopware/issues/123/" +bats_test_function --description "read api: blocks issues/N with gh's placeholders" -- _test_issue_spelling_blocked "repos/{owner}/{repo}/issues/123" + +@test "read api: allows issues/N/timeline, which has no dedicated tool" { + setup_read_blocking + run_api_hook "$READ_TOOL" "repos/shopware/shopware/issues/123/timeline" + assert_success +} + +@test "read api: allows search/issues whose query names an issue path" { + setup_read_blocking + run_api_hook "$READ_TOOL" "search/issues?q=https://api.github.com/repos/shopware/shopware/issues/1" + assert_success +} + +@test "read api: allows search/code, which search_code cannot fully replace" { + setup_read_blocking + run_api_hook "$READ_TOOL" "search/code?q=addClass+removeClass+repo:shopware/shopware" + assert_success +} + @test "read api: blocks orgs/N/issue-types → suggests issue_schema" { setup_read_blocking run_api_hook "$READ_TOOL" "orgs/shopware/issue-types" @@ -132,6 +171,13 @@ setup_write_blocking() { setup_config "gh-tooling" '{"block_api_tool_write": true}' } +@test "write api: blocks GET issues/N → suggests issue_view" { + setup_write_blocking + run_api_hook "$WRITE_TOOL" "repos/shopware/shopware/issues/123" + assert_failure 2 + assert_output --partial "issue_view" +} + @test "write api: blocks POST pulls → suggests pr_create" { setup_write_blocking run_api_hook "$WRITE_TOOL" "repos/shopware/shopware/pulls" "POST" diff --git a/plugin-tests/github-mcp/mcp_tool_gh.bats b/plugin-tests/github-mcp/mcp_tool_gh.bats index 177dff2..af46894 100644 --- a/plugin-tests/github-mcp/mcp_tool_gh.bats +++ b/plugin-tests/github-mcp/mcp_tool_gh.bats @@ -837,6 +837,66 @@ bats_test_function --description "pr_checks: fails without repo outside git" -- } } +# Scope: an owner/repo value in repo, then owner + bare repo, then owner alone, +# then GH_DEFAULT_REPO. GH_DEFAULT_REPO is shopware/shopware in each test. +_scope_argv_code() { printf 'search\ncode\n%s\n--limit\n30\n--json\nrepository,path,textMatches\n--\ntest' "$1"; } +_scope_argv_commits() { printf 'search\ncommits\n%s\n--limit\n20\n--json\nsha,commit\n--\ntest' "$1"; } + +_test_scope_owner_beats_default() { + GH_DEFAULT_REPO="shopware/shopware" + assert_search_argv "tool_$1" '{"search":"test","owner":"acme"}' "$("_scope_argv_${1#search_}" $'--owner\nacme')" +} +_test_scope_matching_owner() { + GH_DEFAULT_REPO="shopware/shopware" + assert_search_argv "tool_$1" '{"search":"test","repo":"acme/app","owner":"ACME"}' "$("_scope_argv_${1#search_}" $'--repo\nacme/app')" +} +_test_scope_user_login_owner() { + GH_DEFAULT_REPO="shopware/shopware" + assert_search_argv "tool_$1" '{"search":"test","owner":"octocat_acme"}' "$("_scope_argv_${1#search_}" $'--owner\noctocat_acme')" +} +_test_scope_conflicting_owner() { + gh() { touch "${BATS_TEST_TMPDIR}/gh_called"; echo '[]'; } + run "tool_$1" '{"search":"test","repo":"acme/app","owner":"other"}' + assert_failure + assert_output --partial "does not match the owner in repo" + assert [ ! -e "${BATS_TEST_TMPDIR}/gh_called" ] +} +_test_scope_split_form() { + GH_DEFAULT_REPO="shopware/shopware" + assert_search_argv "tool_$1" '{"search":"test","owner":"acme","repo":"app"}' "$("_scope_argv_${1#search_}" $'--repo\nacme/app')" +} +_test_scope_repo_beats_default() { + GH_DEFAULT_REPO="shopware/shopware" + assert_search_argv "tool_$1" '{"search":"test","repo":"acme/app"}' "$("_scope_argv_${1#search_}" $'--repo\nacme/app')" +} + +bats_test_function --description "search_code: an explicit owner beats GH_DEFAULT_REPO" -- _test_scope_owner_beats_default search_code +bats_test_function --description "search_commits: an explicit owner beats GH_DEFAULT_REPO" -- _test_scope_owner_beats_default search_commits +bats_test_function --description "search_code: an owner matching the owner in repo is accepted, in any case" -- _test_scope_matching_owner search_code +bats_test_function --description "search_commits: an owner matching the owner in repo is accepted, in any case" -- _test_scope_matching_owner search_commits +bats_test_function --description "search_code: an owner differing from the owner in repo is rejected before gh runs" -- _test_scope_conflicting_owner search_code +bats_test_function --description "search_commits: an owner differing from the owner in repo is rejected before gh runs" -- _test_scope_conflicting_owner search_commits +bats_test_function --description "search_code: a user login with an underscore is a valid owner" -- _test_scope_user_login_owner search_code +bats_test_function --description "search_code: owner with a bare repo name searches owner/repo" -- _test_scope_split_form search_code +bats_test_function --description "search_commits: owner with a bare repo name searches owner/repo" -- _test_scope_split_form search_commits +bats_test_function --description "search_commits: an explicit repo beats GH_DEFAULT_REPO" -- _test_scope_repo_beats_default search_commits + +@test "search_code: a bare repo name without owner is rejected before gh runs" { + gh() { touch "${BATS_TEST_TMPDIR}/gh_called"; echo '[]'; } + run tool_search_code '{"search":"test","repo":"app"}' + assert_failure + assert_output --partial "needs owner" + assert [ ! -e "${BATS_TEST_TMPDIR}/gh_called" ] +} + +@test "search_code: an owner that is not a single login is rejected before gh runs" { + gh() { touch "${BATS_TEST_TMPDIR}/gh_called"; echo '[]'; } + run tool_search_code '{"search":"test","owner":"acme,other"}' + assert_failure + assert_output --partial "acme,other" + assert [ ! -e "${BATS_TEST_TMPDIR}/gh_called" ] +} + @test "search_code: fails when search is missing" { run tool_search_code '{}' assert_failure diff --git a/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index a234d49..c531f57 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -49,6 +49,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `issue_schema` called without `org`, `owner`, or a repository now reports why `gh repo view` could not supply the organization (for example an authentication failure), instead of only saying that `org` is required. `suppress_errors: true` leaves gh's message out. +- With `block_api_tool_read: true`, a GET of `repos/OWNER/REPO/issues/N` through `api_read` is + now blocked with a pointer to `issue_view`, or to `pr_view` for a pull request number; the same + applies to the write server's `api` tool under `block_api_tool_write: true`. Before, it went + through. The rule matches the path only, so a query string or a fragment does not get past it and + an issue path inside another endpoint's query does not trigger it. Sub-resources of an issue + without their own tool, such as `issues/N/timeline`, stay allowed. `issue_view` does not return + `closed_by`, `author_association`, or the lock state, so those are not available while the block + is on. `search/code` stays allowed, because `search_code` sends its search as one exact phrase and + has no form for separate keywords, `OR`/`NOT`, or `path:`. +- `search_code` and `search_commits` now search the repositories of an explicit `owner` when a + default repo is configured. Before, the default repo replaced `owner`, so these tools could not + search outside it. `owner` together with a bare repo name in `repo` now searches `owner/repo`, + the split form the other tools accept; before, the bare name failed as not `owner/repo`. An + `owner` passed with an `owner/repo` value in `repo` must name the same owner, and an `owner` that + is not a single user or organization login is rejected before `gh` runs. - `sub_issue_add`, `sub_issue_remove`, `project_item_add`, and `project_status_set` now reject a `repo` that is not in `owner/repo` form before any GitHub call. Before, `acme/app/extra` was read as owner `acme` and repository `extra`, or built a wrong item URL. diff --git a/plugins/github-mcp/README.md b/plugins/github-mcp/README.md index 4ca78ed..36f48d8 100644 --- a/plugins/github-mcp/README.md +++ b/plugins/github-mcp/README.md @@ -127,7 +127,7 @@ With enforcement disabled: | Field | Type | Default | Description | |------------------------|---------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `repo` | string | -- | Default repository in `owner/repo` format. Used when `repo` is not passed to a tool call. | +| `repo` | string | -- | Default repository in `owner/repo` format. Used when `repo` is not passed to a tool call, and for `search_code` and `search_commits` only when `owner` is not passed either. | | `enforce_mcp_tools` | boolean | `true` | Blocks high-level `gh` subcommands (`gh pr view`, `gh issue view`, `gh run view`, `gh search`, `gh pr create`, `gh label list`, `gh project view`, etc.) and redirects to MCP tools. Set to `false` to disable all gh hook enforcement. | | `block_api_commands` | boolean | `false` | When `true` (and `enforce_mcp_tools` is also `true`), additionally blocks `gh api` calls for endpoints that have a dedicated MCP tool: `pulls/N/comments`, `pulls/N/reviews`, `pulls/N/files`, `pulls/N/commits`, `actions/jobs/N/logs`, `actions/jobs/N`, `check-runs/N/annotations`, `commits/SHA`, `releases`. Other `gh api` calls remain unblocked. | | `enable_write_server` | boolean | `false` | When `true`, the write MCP server exposes write tools (PR creation, issue editing, reviews, etc.). When `false` (default), the write server returns an empty tools list. | diff --git a/plugins/github-mcp/REFERENCE.md b/plugins/github-mcp/REFERENCE.md index 76bedc8..8d97963 100644 --- a/plugins/github-mcp/REFERENCE.md +++ b/plugins/github-mcp/REFERENCE.md @@ -361,8 +361,8 @@ Use gh-tooling search_code with search "addClass" and repo "shopware/shopware" a **Parameters:** - `search` (string, required): Code search expression, matched as exact text (no regex). Unlike the other search tools, the whole value is one phrase. -- `owner` (string, optional): Limit to repositories owned by this user/org. -- `repo` (string, optional): Limit to this repository in `owner/repo` format. +- `owner` (string, optional): Limit to repositories owned by this user/org, instead of the configured default repo. With a bare repo name in `repo`, the two form `owner/repo`. +- `repo` (string, optional): Limit to this repository, as `owner/repo` or as a bare name together with `owner`. An `owner` passed with an `owner/repo` value must name the same owner. - `language` (string, optional): Filter by language (e.g. `php`, `typescript`). - `extension` (string, optional): Filter by file extension (e.g. `php`, `ts`). - `filename` (string, optional): Filter by filename (e.g. `composer.json`). @@ -402,8 +402,8 @@ Use gh-tooling search_commits with search "fix cart" and author "mitelg" and aut **Parameters:** - `search` (string, required): Commit message search expression; see [Search expressions](#search). -- `repo` (string, optional): Limit to this repository in `owner/repo` format. -- `owner` (string, optional): Limit to repositories owned by this user/org. +- `repo` (string, optional): Limit to this repository, as `owner/repo` or as a bare name together with `owner`. An `owner` passed with an `owner/repo` value must name the same owner. +- `owner` (string, optional): Limit to repositories owned by this user/org, instead of the configured default repo. With a bare repo name in `repo`, the two form `owner/repo`. - `author` (string, optional): Filter by commit author username. - `committer` (string, optional): Filter by committer username. - `author_date` (string, optional): Date range (e.g. `>2024-01-01`, `2024-01-01..2024-06-30`). diff --git a/plugins/github-mcp/hooks/scripts/check-api-tools.sh b/plugins/github-mcp/hooks/scripts/check-api-tools.sh index cbd3a0e..85a0b3a 100755 --- a/plugins/github-mcp/hooks/scripts/check-api-tools.sh +++ b/plugins/github-mcp/hooks/scripts/check-api-tools.sh @@ -48,6 +48,10 @@ fi [[ "$BLOCK" != "true" ]] && exit 0 COMMAND="api ${METHOD} ${ENDPOINT}" +# The endpoint without its query string or fragment, for rules that match the +# end of the path: text inside a query must not match one, and gh drops a +# fragment before sending, so a fragment must not escape one. +ENDPOINT_PATH="${ENDPOINT%%[?#]*}" # ============================================================================ # Read endpoint mapping (GET requests with dedicated read tools) @@ -107,6 +111,12 @@ if echo "$ENDPOINT" | grep -qE 'labels(\?|$)'; then block_tool "label_list" "Use label_list with optional repo and filter parameters." fi +# One issue. search/code stays allowed: search_code sends its search as one +# exact phrase, so separate keywords, OR/NOT, and path: have no other route. +if echo "$ENDPOINT_PATH" | grep -qE '(^|/)repos/[^/]+/[^/]+/issues/[0-9]+/?$'; then + block_tool "issue_view" "Use issue_view with number and optional repo, fields, and jq_filter. For a pull request number, use pr_view. issue_view does not return closed_by, author_association, or the lock state; those are not available while this block is on." +fi + # Issue field values on one issue if echo "$ENDPOINT" | grep -qE 'issues/[0-9]+/issue-field-values'; then block_tool "issue_view" "Use issue_view with number and with_field_values true. It returns the issue's type and field values keyed by field name." diff --git a/plugins/github-mcp/mcp-server-gh/lib/search.sh b/plugins/github-mcp/mcp-server-gh/lib/search.sh index 495062f..917bd29 100644 --- a/plugins/github-mcp/mcp-server-gh/lib/search.sh +++ b/plugins/github-mcp/mcp-server-gh/lib/search.sh @@ -79,6 +79,59 @@ _gh_split_search_terms() { fi } +####################################### +# Resolve the scope of a search_code or search_commits call. An owner/repo value +# in repo comes first, and an owner passed with it must name the same owner; a +# bare repo name joins owner (the split form); an owner alone searches that +# user's or organization's repositories; GH_DEFAULT_REPO applies only when +# neither is passed. owner takes the characters _gh_validate_repo allows in an +# owner, since a user login, unlike an organization's, can contain '_'. +# Globals: +# GH_DEFAULT_REPO (read); _GH_SEARCH_SCOPE (set) +# Arguments: +# $1 owner from the tool arguments, may be empty, +# $2 repo from the tool arguments, may be empty, +# $3 tool name, for error messages. +# Outputs: +# An error message on stdout on failure. +# Returns: +# 0 on success, 1 on a bare repo without owner, an owner that differs from +# the owner in repo, or an invalid owner or repo. +####################################### +_gh_resolve_search_scope() { + local owner="$1" repo="$2" tool_name="$3" + _GH_SEARCH_SCOPE=() + + if [[ -n "${owner}" && ! "${owner}" =~ ^[a-zA-Z0-9_.-]+$ ]]; then + echo "Error: owner for ${tool_name} must be a single GitHub user or organization login, got: '${owner}'" + return 1 + fi + # GitHub logins are case-insensitive. + local repo_owner="${repo%%/*}" + if [[ -n "${owner}" && "${repo}" == */* && "${owner,,}" != "${repo_owner,,}" ]]; then + echo "Error: owner '${owner}' for ${tool_name} does not match the owner in repo '${repo}'" + return 1 + fi + + if [[ -n "${repo}" && "${repo}" != */* ]]; then + if [[ -z "${owner}" ]]; then + echo "Error: repo '${repo}' for ${tool_name} needs owner, or pass repo as 'owner/repo'" + return 1 + fi + repo="${owner}/${repo}" + fi + + if [[ -n "${repo}" ]]; then + _gh_validate_repo "${repo}" || return 1 + _GH_SEARCH_SCOPE=("--repo" "${repo}") + elif [[ -n "${owner}" ]]; then + _GH_SEARCH_SCOPE=("--owner" "${owner}") + elif [[ -n "${GH_DEFAULT_REPO:-}" ]]; then + _gh_validate_repo "${GH_DEFAULT_REPO}" || return 1 + _GH_SEARCH_SCOPE=("--repo" "${GH_DEFAULT_REPO}") + fi +} + # Search for GitHub issues or pull requests using a search expression. # Maps to: gh search issues|prs [--repo] [--state] [--limit] [--json] -- tool_search() { @@ -190,20 +243,8 @@ tool_search_code() { local -a cmd=("gh" "search" "code") - # Resolve repo: explicit param > GH_DEFAULT_REPO (consistent with tool_search) - local effective_repo - if [[ -n "${repo}" ]]; then - effective_repo="${repo}" - else - effective_repo="${GH_DEFAULT_REPO:-}" - fi - - if [[ -n "${effective_repo}" ]]; then - _gh_validate_repo "${effective_repo}" || return 1 - cmd+=("--repo" "${effective_repo}") - elif [[ -n "${owner}" ]]; then - cmd+=("--owner" "${owner}") - fi + _gh_resolve_search_scope "${owner}" "${repo}" "search_code" || return 1 + cmd+=(${_GH_SEARCH_SCOPE[@]+"${_GH_SEARCH_SCOPE[@]}"}) [[ -n "${language}" ]] && cmd+=("--language" "${language}") [[ -n "${extension}" ]] && cmd+=("--extension" "${extension}") @@ -363,20 +404,8 @@ tool_search_commits() { local -a cmd=("gh" "search" "commits") - # Resolve repo: explicit param > GH_DEFAULT_REPO (consistent with tool_search) - local effective_repo - if [[ -n "${repo}" ]]; then - effective_repo="${repo}" - else - effective_repo="${GH_DEFAULT_REPO:-}" - fi - - if [[ -n "${effective_repo}" ]]; then - _gh_validate_repo "${effective_repo}" || return 1 - cmd+=("--repo" "${effective_repo}") - elif [[ -n "${owner}" ]]; then - cmd+=("--owner" "${owner}") - fi + _gh_resolve_search_scope "${owner}" "${repo}" "search_commits" || return 1 + cmd+=(${_GH_SEARCH_SCOPE[@]+"${_GH_SEARCH_SCOPE[@]}"}) [[ -n "${author}" ]] && cmd+=("--author" "${author}") [[ -n "${committer}" ]] && cmd+=("--committer" "${committer}") diff --git a/plugins/github-mcp/mcp-server-gh/tools-read.json b/plugins/github-mcp/mcp-server-gh/tools-read.json index 3ce3c78..32b3cfa 100644 --- a/plugins/github-mcp/mcp-server-gh/tools-read.json +++ b/plugins/github-mcp/mcp-server-gh/tools-read.json @@ -1108,11 +1108,11 @@ "properties": { "owner": { "type": "string", - "description": "Limit search to repositories owned by this user/organization." + "description": "Limit search to repositories owned by this user/organization, instead of the configured default repo. With a bare repo name in repo, the two form owner/repo." }, "repo": { "type": "string", - "description": "Limit search to this repository in 'owner/repo' format." + "description": "Limit search to this repository, as 'owner/repo' or as a bare name together with owner." }, "language": { "type": "string", @@ -1286,11 +1286,11 @@ "properties": { "repo": { "type": "string", - "description": "Limit search to this repository in 'owner/repo' format." + "description": "Limit search to this repository, as 'owner/repo' or as a bare name together with owner." }, "owner": { "type": "string", - "description": "Limit search to repositories owned by this user/organization." + "description": "Limit search to repositories owned by this user/organization, instead of the configured default repo. With a bare repo name in repo, the two form owner/repo." }, "author": { "type": "string",