Four comparisons evaluate tool-supplied values as arithmetic expressions and validate nothing themselves:
| Site |
Value |
mcp-server-gh/lib/common.sh:295 |
grep_before, from grep_context_before |
mcp-server-gh/lib/common.sh:296 |
grep_after, from grep_context_after |
mcp-server-gh/lib/common.sh:301 |
max_lines |
mcp-server-gh/lib/common.sh:305 |
tail_lines |
mcp-server-gh/lib/release.sh:76 |
per_page, from release_list's limit |
(The common.sh numbers move with any edit above them. The sites are the four -gt comparisons in _gh_post_process.)
[[ x -gt y ]] evaluates its operands as arithmetic expressions, and bash evaluates a command substitution written inside an array subscript in that context. A value such as PATH[$(command)0] reaching any of these lines runs command in the server process, which holds the user's gh credentials and filesystem access. A payload naming an unset variable (x[$(...)0]) aborts under the server's set -u before the substitution runs, so the payload names a variable that exists.
The other -gt comparisons in release.sh (lines 48, 63, 150) take jq-computed counts rather than tool input and are not affected.
What holds today
Two gates upstream of these sites, both in the protocol layer rather than in the code that does the comparing:
- Every dispatchable tool is declared.
_gh_unset_undeclared_tools (lib/common.sh) drops every sourced tool_* function the running server's tools list does not name, so nothing reaches dispatch without a schema.
- Every declared parameter that feeds these sites is integer-typed. 45 such declarations in
tools-read.json and 2 in tools-write.json; validate_tool_arguments enforces the declared type before dispatch.
A crafted call is refused before it reaches a tool function:
{"jsonrpc":"2.0","id":2,"result":{"content":[{"type":"text","text":"Invalid type(s): max_lines expected integer, got string (\"PATH[$(touch /tmp/marker)0]\")."}],"isError":true}}
There is no route through the MCP protocol that delivers a non-integer value to these comparisons.
Why this is still open
The gates are upstream, implicit, and in different files from the hazard. Nothing at the comparison sites depends on them, and nothing re-checks them.
- A schema edit that drops or misspells one
type restores command execution, silently. The 47 declarations are the whole guarantee, and no test asserts that they exist.
- A caller that does not go through
validate_tool_arguments is unguarded. _gh_post_process is an internal helper, and a new tool function calling it directly inherits no protection:
source plugins/github-mcp/mcp-server-gh/lib/common.sh
log() { :; }
_gh_post_process "data" "" "" 0 0 false false 'PATH[$(touch /tmp/marker)0]' ""
/tmp/marker exists afterwards.
This is a hardening item, not a live vulnerability. It is not reachable by any client today, and it does not gate a release.
Fix
Validate before comparing, at each of the five sites. _gh_validate_number in lib/common.sh:8 already has the right shape, or inline:
if [[ "${max_lines}" =~ ^[0-9]+$ ]] && [[ "${max_lines}" -gt 0 ]]; then
A regex match is not an arithmetic context, so the guard runs first and the comparison never sees an unvalidated value. Each site gets a test asserting that a non-numeric value is rejected rather than evaluated.
A test asserting that every parameter feeding these sites declares "type": "integer" is worth adding either way: it turns the second gate from a convention into something CI enforces, and it is useful even after the sites validate for themselves.
Four comparisons evaluate tool-supplied values as arithmetic expressions and validate nothing themselves:
mcp-server-gh/lib/common.sh:295grep_before, fromgrep_context_beforemcp-server-gh/lib/common.sh:296grep_after, fromgrep_context_aftermcp-server-gh/lib/common.sh:301max_linesmcp-server-gh/lib/common.sh:305tail_linesmcp-server-gh/lib/release.sh:76per_page, fromrelease_list'slimit(The
common.shnumbers move with any edit above them. The sites are the four-gtcomparisons in_gh_post_process.)[[ x -gt y ]]evaluates its operands as arithmetic expressions, and bash evaluates a command substitution written inside an array subscript in that context. A value such asPATH[$(command)0]reaching any of these lines runscommandin the server process, which holds the user'sghcredentials and filesystem access. A payload naming an unset variable (x[$(...)0]) aborts under the server'sset -ubefore the substitution runs, so the payload names a variable that exists.The other
-gtcomparisons inrelease.sh(lines 48, 63, 150) take jq-computed counts rather than tool input and are not affected.What holds today
Two gates upstream of these sites, both in the protocol layer rather than in the code that does the comparing:
_gh_unset_undeclared_tools(lib/common.sh) drops every sourcedtool_*function the running server's tools list does not name, so nothing reaches dispatch without a schema.tools-read.jsonand 2 intools-write.json;validate_tool_argumentsenforces the declaredtypebefore dispatch.A crafted call is refused before it reaches a tool function:
There is no route through the MCP protocol that delivers a non-integer value to these comparisons.
Why this is still open
The gates are upstream, implicit, and in different files from the hazard. Nothing at the comparison sites depends on them, and nothing re-checks them.
typerestores command execution, silently. The 47 declarations are the whole guarantee, and no test asserts that they exist.validate_tool_argumentsis unguarded._gh_post_processis an internal helper, and a new tool function calling it directly inherits no protection:/tmp/markerexists afterwards.This is a hardening item, not a live vulnerability. It is not reachable by any client today, and it does not gate a release.
Fix
Validate before comparing, at each of the five sites.
_gh_validate_numberinlib/common.sh:8already has the right shape, or inline:A regex match is not an arithmetic context, so the guard runs first and the comparison never sees an unvalidated value. Each site gets a test asserting that a non-numeric value is rejected rather than evaluated.
A test asserting that every parameter feeding these sites declares
"type": "integer"is worth adding either way: it turns the second gate from a convention into something CI enforces, and it is useful even after the sites validate for themselves.