From acc9d7c49ca206ff6652f346c1ed15b56256e531 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Sat, 3 Oct 2026 14:18:11 +0200 Subject: [PATCH 1/5] fix(github-mcp): block api_read for a single issue and for code search With block_api_tool_read enabled, check-api-tools.sh let a GET of repos/OWNER/REPO/issues/N and of search/code through, although issue_view and search_code cover both. Its GET patterns covered only issues/N/issue-field-values and no search endpoint. The issue pattern is anchored at the issue number, so sub-resources without a dedicated tool, such as issues/N/timeline, stay allowed. search/issues, search/repositories, and search/commits stay allowed as well; REFERENCE.md uses search/issues through api_read as an example. The issue_view hint names pr_view for a pull request number, since the issues endpoint also answers for pull requests. Co-Authored-By: Claude --- plugin-tests/github-mcp/check_api_tools.bats | 26 +++++++++++++++++++ plugins/github-mcp/CHANGELOG.md | 4 +++ .../hooks/scripts/check-api-tools.sh | 10 +++++++ 3 files changed, 40 insertions(+) diff --git a/plugin-tests/github-mcp/check_api_tools.bats b/plugin-tests/github-mcp/check_api_tools.bats index dcf5062..f5d6a45 100644 --- a/plugin-tests/github-mcp/check_api_tools.bats +++ b/plugin-tests/github-mcp/check_api_tools.bats @@ -73,6 +73,32 @@ 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" +} + +@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: blocks search/code with a query string → suggests search_code" { + setup_read_blocking + run_api_hook "$READ_TOOL" "search/code?q=typebox+repo:shopware/shopware" + assert_failure 2 + assert_output --partial "search_code" +} + +@test "read api: allows search/issues, a documented api_read use" { + setup_read_blocking + run_api_hook "$READ_TOOL" "search/issues?q=repo:shopware/shopware+is:open" + 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" diff --git a/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index a234d49..c8f640f 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -49,6 +49,10 @@ 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` is now blocked with a + pointer to `issue_view`, and a GET of `search/code` with a pointer to `search_code`. Before, both + went through. Sub-resources of an issue without their own tool, such as `issues/N/timeline`, are + still allowed, and so are `search/issues`, `search/repositories`, and `search/commits`. - `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/hooks/scripts/check-api-tools.sh b/plugins/github-mcp/hooks/scripts/check-api-tools.sh index cbd3a0e..9df8f30 100755 --- a/plugins/github-mcp/hooks/scripts/check-api-tools.sh +++ b/plugins/github-mcp/hooks/scripts/check-api-tools.sh @@ -107,6 +107,16 @@ if echo "$ENDPOINT" | grep -qE 'labels(\?|$)'; then block_tool "label_list" "Use label_list with optional repo and filter parameters." fi +# One issue +if echo "$ENDPOINT" | 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." +fi + +# Code search +if echo "$ENDPOINT" | grep -qE 'search/code(\?|$)'; then + block_tool "search_code" "Use search_code with search and optional repo, owner, language, extension, and filename." +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." From d589ee257ade5cfd8b730165cd18e722e71eb000 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Sat, 3 Oct 2026 14:18:12 +0200 Subject: [PATCH 2/5] fix(github-mcp): let an explicit owner beat the default repo in search search_code and search_commits used GH_DEFAULT_REPO whenever repo was empty, and passed owner to gh only when no default repo was configured. With a default repo set, an explicit owner was dropped, and neither tool could search outside that repository. With api_read blocked for search/code, no other tool could either. The scope now resolves an explicit repo first, then an explicit owner, then the default repo. The owner descriptions in tools-read.json and REFERENCE.md state the precedence. Co-Authored-By: Claude --- plugin-tests/github-mcp/mcp_tool_gh.bats | 32 +++++++++++++++++++ plugins/github-mcp/CHANGELOG.md | 3 ++ plugins/github-mcp/REFERENCE.md | 4 +-- .../github-mcp/mcp-server-gh/lib/search.sh | 12 +++---- .../github-mcp/mcp-server-gh/tools-read.json | 4 +-- 5 files changed, 45 insertions(+), 10 deletions(-) diff --git a/plugin-tests/github-mcp/mcp_tool_gh.bats b/plugin-tests/github-mcp/mcp_tool_gh.bats index 177dff2..891d561 100644 --- a/plugin-tests/github-mcp/mcp_tool_gh.bats +++ b/plugin-tests/github-mcp/mcp_tool_gh.bats @@ -837,6 +837,22 @@ bats_test_function --description "pr_checks: fails without repo outside git" -- } } +@test "search_code: explicit owner overrides GH_DEFAULT_REPO" { + GH_DEFAULT_REPO="default/repo" + gh() { + echo "$*" > "${BATS_TEST_TMPDIR}/captured_cmd" + echo '[]' + } + run tool_search_code '{"search":"test","owner":"myorg"}' + assert_success + local captured_cmd + captured_cmd=$(cat "${BATS_TEST_TMPDIR}/captured_cmd") + [[ "${captured_cmd}" == *"--owner myorg"* && "${captured_cmd}" != *"--repo"* ]] || { + echo "Expected --owner myorg and no --repo in command: ${captured_cmd}" + return 1 + } +} + @test "search_code: fails when search is missing" { run tool_search_code '{}' assert_failure @@ -910,6 +926,22 @@ bats_test_function --description "pr_checks: fails without repo outside git" -- } } +@test "search_commits: explicit owner overrides GH_DEFAULT_REPO" { + GH_DEFAULT_REPO="default/repo" + gh() { + echo "$*" > "${BATS_TEST_TMPDIR}/captured_cmd" + echo '[]' + } + run tool_search_commits '{"search":"test","owner":"myorg"}' + assert_success + local captured_cmd + captured_cmd=$(cat "${BATS_TEST_TMPDIR}/captured_cmd") + [[ "${captured_cmd}" == *"--owner myorg"* && "${captured_cmd}" != *"--repo"* ]] || { + echo "Expected --owner myorg and no --repo in command: ${captured_cmd}" + return 1 + } +} + @test "search_commits: fails when search is missing" { run tool_search_commits '{}' assert_failure diff --git a/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index c8f640f..4cada9b 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -53,6 +53,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 pointer to `issue_view`, and a GET of `search/code` with a pointer to `search_code`. Before, both went through. Sub-resources of an issue without their own tool, such as `issues/N/timeline`, are still allowed, and so are `search/issues`, `search/repositories`, and `search/commits`. +- `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. An explicit `repo` still takes precedence over `owner`. - `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/REFERENCE.md b/plugins/github-mcp/REFERENCE.md index 76bedc8..033c561 100644 --- a/plugins/github-mcp/REFERENCE.md +++ b/plugins/github-mcp/REFERENCE.md @@ -361,7 +361,7 @@ 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. +- `owner` (string, optional): Limit to repositories owned by this user/org. Used instead of the configured default repo; ignored when `repo` is set. - `repo` (string, optional): Limit to this repository in `owner/repo` format. - `language` (string, optional): Filter by language (e.g. `php`, `typescript`). - `extension` (string, optional): Filter by file extension (e.g. `php`, `ts`). @@ -403,7 +403,7 @@ 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. +- `owner` (string, optional): Limit to repositories owned by this user/org. Used instead of the configured default repo; ignored when `repo` is set. - `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/mcp-server-gh/lib/search.sh b/plugins/github-mcp/mcp-server-gh/lib/search.sh index 495062f..31f2fca 100644 --- a/plugins/github-mcp/mcp-server-gh/lib/search.sh +++ b/plugins/github-mcp/mcp-server-gh/lib/search.sh @@ -190,11 +190,11 @@ tool_search_code() { local -a cmd=("gh" "search" "code") - # Resolve repo: explicit param > GH_DEFAULT_REPO (consistent with tool_search) - local effective_repo + # Resolve scope: explicit repo > explicit owner > GH_DEFAULT_REPO + local effective_repo="" if [[ -n "${repo}" ]]; then effective_repo="${repo}" - else + elif [[ -z "${owner}" ]]; then effective_repo="${GH_DEFAULT_REPO:-}" fi @@ -363,11 +363,11 @@ tool_search_commits() { local -a cmd=("gh" "search" "commits") - # Resolve repo: explicit param > GH_DEFAULT_REPO (consistent with tool_search) - local effective_repo + # Resolve scope: explicit repo > explicit owner > GH_DEFAULT_REPO + local effective_repo="" if [[ -n "${repo}" ]]; then effective_repo="${repo}" - else + elif [[ -z "${owner}" ]]; then effective_repo="${GH_DEFAULT_REPO:-}" fi diff --git a/plugins/github-mcp/mcp-server-gh/tools-read.json b/plugins/github-mcp/mcp-server-gh/tools-read.json index 3ce3c78..bcadd2d 100644 --- a/plugins/github-mcp/mcp-server-gh/tools-read.json +++ b/plugins/github-mcp/mcp-server-gh/tools-read.json @@ -1108,7 +1108,7 @@ "properties": { "owner": { "type": "string", - "description": "Limit search to repositories owned by this user/organization." + "description": "Limit search to repositories owned by this user/organization. Used instead of the configured default repo; ignored when repo is set." }, "repo": { "type": "string", @@ -1290,7 +1290,7 @@ }, "owner": { "type": "string", - "description": "Limit search to repositories owned by this user/organization." + "description": "Limit search to repositories owned by this user/organization. Used instead of the configured default repo; ignored when repo is set." }, "author": { "type": "string", From 6719d9e3d121bdbd7ceebdc0bd5db4c641570526 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Sat, 3 Oct 2026 16:25:49 +0200 Subject: [PATCH 3/5] fix(github-mcp): allow api_read for search/code, anchor the issue rule The search/code block left queries without a working tool: search_code sends its search to gh as one exact phrase, so separate keywords, OR/NOT, and path: could only go through api_read. check-api-tools.sh no longer blocks search/code. The issues/N rule matched the whole endpoint, so a fragment, which gh drops before sending, got past it, and an issue path inside another endpoint's query string, such as search/issues?q=..., triggered it. It now matches the path without query and fragment, along whole segments, and also covers a trailing slash. The block message names the fields issue_view cannot return: closed_by, author_association, and the lock state. The CHANGELOG entry now also names block_api_tool_write, under which the write server's api tool applies the same rule. Tests cover a leading slash, a query string, a fragment, a trailing slash, gh's {owner}/{repo} placeholders, an issue path inside a search query, search/code, and the write tool. Co-Authored-By: Claude --- plugin-tests/github-mcp/check_api_tools.bats | 32 +++++++++++++++---- plugins/github-mcp/CHANGELOG.md | 13 +++++--- .../hooks/scripts/check-api-tools.sh | 16 +++++----- 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/plugin-tests/github-mcp/check_api_tools.bats b/plugin-tests/github-mcp/check_api_tools.bats index f5d6a45..792c399 100644 --- a/plugin-tests/github-mcp/check_api_tools.bats +++ b/plugin-tests/github-mcp/check_api_tools.bats @@ -80,22 +80,35 @@ setup_read_blocking() { 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: blocks search/code with a query string → suggests search_code" { +@test "read api: allows search/issues whose query names an issue path" { setup_read_blocking - run_api_hook "$READ_TOOL" "search/code?q=typebox+repo:shopware/shopware" - assert_failure 2 - assert_output --partial "search_code" + run_api_hook "$READ_TOOL" "search/issues?q=https://api.github.com/repos/shopware/shopware/issues/1" + assert_success } -@test "read api: allows search/issues, a documented api_read use" { +@test "read api: allows search/code, which search_code cannot fully replace" { setup_read_blocking - run_api_hook "$READ_TOOL" "search/issues?q=repo:shopware/shopware+is:open" + run_api_hook "$READ_TOOL" "search/code?q=addClass+removeClass+repo:shopware/shopware" assert_success } @@ -158,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/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index 4cada9b..3d33075 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -49,10 +49,15 @@ 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` is now blocked with a - pointer to `issue_view`, and a GET of `search/code` with a pointer to `search_code`. Before, both - went through. Sub-resources of an issue without their own tool, such as `issues/N/timeline`, are - still allowed, and so are `search/issues`, `search/repositories`, and `search/commits`. +- 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. An explicit `repo` still takes precedence over `owner`. diff --git a/plugins/github-mcp/hooks/scripts/check-api-tools.sh b/plugins/github-mcp/hooks/scripts/check-api-tools.sh index 9df8f30..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,14 +111,10 @@ if echo "$ENDPOINT" | grep -qE 'labels(\?|$)'; then block_tool "label_list" "Use label_list with optional repo and filter parameters." fi -# One issue -if echo "$ENDPOINT" | 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." -fi - -# Code search -if echo "$ENDPOINT" | grep -qE 'search/code(\?|$)'; then - block_tool "search_code" "Use search_code with search and optional repo, owner, language, extension, and filename." +# 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 From 22669f14f42ec45679a78f48c501396f44ea3435 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Sat, 3 Oct 2026 16:25:49 +0200 Subject: [PATCH 4/5] fix(github-mcp): accept owner plus a bare repo in search_code The SessionStart directive says the search tools accept owner with a bare repo name, but search_code and search_commits sent the bare name to gh as the repo and failed it as not owner/repo. _gh_resolve_search_scope now resolves both tools' scope in one place: an owner/repo value in repo first, then owner joined with a bare repo, then owner alone, then GH_DEFAULT_REPO. owner goes through _gh_validate_org, so a value such as "acme,other", which gh's --owner would read as two owners, or a whitespace-only owner fails before gh runs. The owner and repo descriptions in tools-read.json and REFERENCE.md describe the split form, and the README says when the default repo applies to these two tools. The tests now check the exact gh arguments for each order of precedence in both tools. Co-Authored-By: Claude --- plugin-tests/github-mcp/mcp_tool_gh.bats | 74 +++++++++++-------- plugins/github-mcp/CHANGELOG.md | 5 +- plugins/github-mcp/README.md | 2 +- plugins/github-mcp/REFERENCE.md | 8 +- .../github-mcp/mcp-server-gh/lib/search.sh | 73 +++++++++++------- .../github-mcp/mcp-server-gh/tools-read.json | 8 +- 6 files changed, 102 insertions(+), 68 deletions(-) diff --git a/plugin-tests/github-mcp/mcp_tool_gh.bats b/plugin-tests/github-mcp/mcp_tool_gh.bats index 891d561..81a815b 100644 --- a/plugin-tests/github-mcp/mcp_tool_gh.bats +++ b/plugin-tests/github-mcp/mcp_tool_gh.bats @@ -837,20 +837,50 @@ bats_test_function --description "pr_checks: fails without repo outside git" -- } } -@test "search_code: explicit owner overrides GH_DEFAULT_REPO" { - GH_DEFAULT_REPO="default/repo" - gh() { - echo "$*" > "${BATS_TEST_TMPDIR}/captured_cmd" - echo '[]' - } - run tool_search_code '{"search":"test","owner":"myorg"}' - assert_success - local captured_cmd - captured_cmd=$(cat "${BATS_TEST_TMPDIR}/captured_cmd") - [[ "${captured_cmd}" == *"--owner myorg"* && "${captured_cmd}" != *"--repo"* ]] || { - echo "Expected --owner myorg and no --repo in command: ${captured_cmd}" - return 1 - } +# 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_repo_beats_owner() { + GH_DEFAULT_REPO="shopware/shopware" + assert_search_argv "tool_$1" '{"search":"test","repo":"acme/app","owner":"other"}' "$("_scope_argv_${1#search_}" $'--repo\nacme/app')" +} +_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/repo value in repo beats owner" -- _test_scope_repo_beats_owner search_code +bats_test_function --description "search_commits: an owner/repo value in repo beats owner" -- _test_scope_repo_beats_owner search_commits +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" { @@ -926,22 +956,6 @@ bats_test_function --description "pr_checks: fails without repo outside git" -- } } -@test "search_commits: explicit owner overrides GH_DEFAULT_REPO" { - GH_DEFAULT_REPO="default/repo" - gh() { - echo "$*" > "${BATS_TEST_TMPDIR}/captured_cmd" - echo '[]' - } - run tool_search_commits '{"search":"test","owner":"myorg"}' - assert_success - local captured_cmd - captured_cmd=$(cat "${BATS_TEST_TMPDIR}/captured_cmd") - [[ "${captured_cmd}" == *"--owner myorg"* && "${captured_cmd}" != *"--repo"* ]] || { - echo "Expected --owner myorg and no --repo in command: ${captured_cmd}" - return 1 - } -} - @test "search_commits: fails when search is missing" { run tool_search_commits '{}' assert_failure diff --git a/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index 3d33075..ee6b009 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -60,7 +60,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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. An explicit `repo` still takes precedence over `owner`. + 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/repo` value in `repo` still takes precedence over `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 033c561..5e79788 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. Used instead of the configured default repo; ignored when `repo` is set. -- `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/repo` value makes `owner` unused. - `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. Used instead of the configured default repo; ignored when `repo` is set. +- `repo` (string, optional): Limit to this repository, as `owner/repo` or as a bare name together with `owner`. An `owner/repo` value makes `owner` unused. +- `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/mcp-server-gh/lib/search.sh b/plugins/github-mcp/mcp-server-gh/lib/search.sh index 31f2fca..48942fd 100644 --- a/plugins/github-mcp/mcp-server-gh/lib/search.sh +++ b/plugins/github-mcp/mcp-server-gh/lib/search.sh @@ -79,6 +79,47 @@ _gh_split_search_terms() { fi } +####################################### +# Resolve the scope of a search_code or search_commits call. An owner/repo value +# in repo comes first; 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. An owner/repo value in repo makes owner +# unused. +# 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 or an invalid owner or repo. +####################################### +_gh_resolve_search_scope() { + local owner="$1" repo="$2" tool_name="$3" + _GH_SEARCH_SCOPE=() + + 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_validate_org "${owner}" "${tool_name}" || return 1 + _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 +231,8 @@ tool_search_code() { local -a cmd=("gh" "search" "code") - # Resolve scope: explicit repo > explicit owner > GH_DEFAULT_REPO - local effective_repo="" - if [[ -n "${repo}" ]]; then - effective_repo="${repo}" - elif [[ -z "${owner}" ]]; then - 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 +392,8 @@ tool_search_commits() { local -a cmd=("gh" "search" "commits") - # Resolve scope: explicit repo > explicit owner > GH_DEFAULT_REPO - local effective_repo="" - if [[ -n "${repo}" ]]; then - effective_repo="${repo}" - elif [[ -z "${owner}" ]]; then - 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 bcadd2d..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. Used instead of the configured default repo; ignored when repo is set." + "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. Used instead of the configured default repo; ignored when repo is set." + "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", From 83f346293980646ffcdb4a8ab46998205a12ea58 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Sat, 3 Oct 2026 16:33:18 +0200 Subject: [PATCH 5/5] fix(github-mcp): accept user logins as owner, reject a conflicting owner _gh_resolve_search_scope checked a lone owner with _gh_validate_org, whose pattern allows only letters, digits, and hyphens. That is right for organizations, but a search owner can be a user, and an Enterprise Managed User login such as octocat_acme contains an underscore, so search_code and search_commits rejected a valid owner that gh had accepted before. The owner check now uses the characters _gh_validate_repo allows in an owner, so the lone owner and the owner in the split form follow the same rule, and the error names a user or organization login. An owner passed with an owner/repo value in repo was ignored, so repo acme/app with owner other searched acme/app without saying so. Such a call now fails before gh runs unless the two owners match, compared without regard to case. REFERENCE.md and the CHANGELOG state the rule. Co-Authored-By: Claude --- plugin-tests/github-mcp/mcp_tool_gh.bats | 22 +++++++++++++---- plugins/github-mcp/CHANGELOG.md | 4 ++-- plugins/github-mcp/REFERENCE.md | 4 ++-- .../github-mcp/mcp-server-gh/lib/search.sh | 24 ++++++++++++++----- 4 files changed, 40 insertions(+), 14 deletions(-) diff --git a/plugin-tests/github-mcp/mcp_tool_gh.bats b/plugin-tests/github-mcp/mcp_tool_gh.bats index 81a815b..af46894 100644 --- a/plugin-tests/github-mcp/mcp_tool_gh.bats +++ b/plugin-tests/github-mcp/mcp_tool_gh.bats @@ -846,9 +846,20 @@ _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_repo_beats_owner() { +_test_scope_matching_owner() { GH_DEFAULT_REPO="shopware/shopware" - assert_search_argv "tool_$1" '{"search":"test","repo":"acme/app","owner":"other"}' "$("_scope_argv_${1#search_}" $'--repo\nacme/app')" + 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" @@ -861,8 +872,11 @@ _test_scope_repo_beats_default() { 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/repo value in repo beats owner" -- _test_scope_repo_beats_owner search_code -bats_test_function --description "search_commits: an owner/repo value in repo beats owner" -- _test_scope_repo_beats_owner 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 diff --git a/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index ee6b009..c531f57 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -62,8 +62,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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/repo` value in `repo` still takes precedence over `owner`, and an `owner` that is not a - single user or organization login is rejected before `gh` runs. + `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/REFERENCE.md b/plugins/github-mcp/REFERENCE.md index 5e79788..8d97963 100644 --- a/plugins/github-mcp/REFERENCE.md +++ b/plugins/github-mcp/REFERENCE.md @@ -362,7 +362,7 @@ 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, 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/repo` value makes `owner` unused. +- `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,7 +402,7 @@ 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, as `owner/repo` or as a bare name together with `owner`. An `owner/repo` value makes `owner` unused. +- `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. diff --git a/plugins/github-mcp/mcp-server-gh/lib/search.sh b/plugins/github-mcp/mcp-server-gh/lib/search.sh index 48942fd..917bd29 100644 --- a/plugins/github-mcp/mcp-server-gh/lib/search.sh +++ b/plugins/github-mcp/mcp-server-gh/lib/search.sh @@ -81,10 +81,11 @@ _gh_split_search_terms() { ####################################### # Resolve the scope of a search_code or search_commits call. An owner/repo value -# in repo comes first; 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. An owner/repo value in repo makes owner -# unused. +# 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: @@ -94,12 +95,24 @@ _gh_split_search_terms() { # Outputs: # An error message on stdout on failure. # Returns: -# 0 on success, 1 on a bare repo without owner or an invalid owner or repo. +# 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'" @@ -112,7 +125,6 @@ _gh_resolve_search_scope() { _gh_validate_repo "${repo}" || return 1 _GH_SEARCH_SCOPE=("--repo" "${repo}") elif [[ -n "${owner}" ]]; then - _gh_validate_org "${owner}" "${tool_name}" || return 1 _GH_SEARCH_SCOPE=("--owner" "${owner}") elif [[ -n "${GH_DEFAULT_REPO:-}" ]]; then _gh_validate_repo "${GH_DEFAULT_REPO}" || return 1