diff --git a/.mcp-sdk.lock b/.mcp-sdk.lock index 44c9e08..7d62f71 100644 --- a/.mcp-sdk.lock +++ b/.mcp-sdk.lock @@ -1,2 +1,2 @@ -version=v1.0.0 -sha256=8f683045f3ae724ce781fa40bcfe97bf2ee74f1b44aaf38afef38854e365ee90 +version=v2.0.0 +sha256=4b63025f6d9328436a8c0fd69e3e53c3205de642d0223481459ab9767f2309cd diff --git a/plugin-tests/github-mcp/tool_schemas.bats b/plugin-tests/github-mcp/tool_schemas.bats index d53a6fc..8f8b990 100644 --- a/plugin-tests/github-mcp/tool_schemas.bats +++ b/plugin-tests/github-mcp/tool_schemas.bats @@ -71,11 +71,14 @@ load_validator_for() { | select($prop.default != null) | select( ($prop.enum != null and ($prop.enum | index($prop.default)) == null) - or (($prop.type | type) == "string" and ( - if $prop.type == "integer" - then ($prop.default | type) != "number" - else ($prop.default | type) != $prop.type - end)) + or ($prop.type != null and ( + (if ($prop.type | type) == "array" then $prop.type else [$prop.type] end) as $names + | all($names[]; type == "string") + and (any($names[]; + if . == "integer" + then ($prop.default | type) == "number" + else ($prop.default | type) == . + end) | not))) ) | "\($tool).\($field) default \($prop.default | tojson) violates its own schema"' \ "$READ_TOOLS" "$WRITE_TOOLS" diff --git a/plugins/github-mcp/AGENTS.md b/plugins/github-mcp/AGENTS.md index 811813f..429aa69 100644 --- a/plugins/github-mcp/AGENTS.md +++ b/plugins/github-mcp/AGENTS.md @@ -198,5 +198,5 @@ Run tests: ## External References -- [Bash MCP SDK](https://github.com/muthuishere/mcp-server-bash-sdk) - SDK this server is based on +- [Bash MCP SDK](https://github.com/shopwareLabs/bash-mcp-sdk) - source of the vendored `shared/mcpserver_core.sh`; pinned in `.mcp-sdk.lock` - [MCP Protocol Specification](https://modelcontextprotocol.io/specification) - JSON-RPC 2.0 protocol details diff --git a/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index 2ee48eb..1fad637 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -8,8 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed -- `shared/mcpserver_core.sh` is now vendored from [shopwareLabs/bash-mcp-sdk](https://github.com/shopwareLabs/bash-mcp-sdk) `v1.0.0` instead of being maintained in this repository. The file is byte-identical to `lib/mcpserver_core.sh` at that tag; `.mcp-sdk.lock` records the release and `renovate.json` opens a PR when a new one is published. Protocol changes now go to the SDK repository and arrive here as a lock bump — a local edit is overwritten by the next update. -- Tool-call argument validation now enforces a declared `type`, a declared `pattern` on string values, `items.type` / `items.enum` on every element of an array, and `enum`. The `required` and `additionalProperties` checks were already applied. Diagnostics report the most fundamental defect first, in the order missing, unknown, type, pattern, items, enum. **Breaking for callers:** an argument of the wrong single type that was previously accepted and passed through to `gh` now returns an `isError` result naming the parameter, its expected type and the value received. This affects the integer-typed paging and output parameters — `limit`, `max_lines`, `tail_lines`, `grep_context_before`, `grep_context_after`, `line_start`, `line_end` — where a quoted number such as `"20"` is now refused. Identifier parameters declare `["integer", "string"]`; the pinned SDK `v1.0.0` does not enforce array-valued types, so it does not validate that their values belong to either member. +- `shared/mcpserver_core.sh` is now vendored from [shopwareLabs/bash-mcp-sdk](https://github.com/shopwareLabs/bash-mcp-sdk) `v2.0.0` instead of being maintained in this repository. The file is byte-identical to `lib/mcpserver_core.sh` at that tag; `.mcp-sdk.lock` records the release and `renovate.json` opens a PR when a new one is published. Protocol changes now go to the SDK repository and arrive here as a lock bump — a local edit is overwritten by the next update. +- Tool-call argument validation now enforces a declared `type`, a declared `pattern` on string values, `items.type` / `items.enum` on every element of an array, and `enum`. The `required` and `additionalProperties` checks were already applied. Diagnostics report the most fundamental defect first, in the order missing, unknown, type, pattern, items, enum. **Breaking for callers:** an argument of the wrong single type that was previously accepted and passed through to `gh` now returns an `isError` result naming the parameter, its expected type and the value received. This affects the integer-typed paging and output parameters — `limit`, `max_lines`, `tail_lines`, `grep_context_before`, `grep_context_after`, `line_start`, `line_end` — where a quoted number such as `"20"` is now refused. A `type` declared as a list of alternatives is enforced the same way: identifier parameters declare `["integer", "string"]` and accept `339` and `"339"` alike, while a value of neither type is refused naming both — `number expected integer or string, got boolean`. - `project_view.number`, and `issue_number` / `sub_issue_number` on `sub_issue_add` and `sub_issue_remove`, now declare `["integer", "string"]`. They were the only numeric identifiers still declared integer-only, so the string form a client may send would have been refused by the stricter validation above. The tool functions read both forms through `jq -r`; `sub_issue_add` and `sub_issue_remove` check the result with `_gh_validate_number`, while `project_view` checks that it is non-empty. Only the schemas were narrower. ### Fixed diff --git a/plugins/github-mcp/shared/mcpserver_core.sh b/plugins/github-mcp/shared/mcpserver_core.sh index e2a4742..913965a 100755 --- a/plugins/github-mcp/shared/mcpserver_core.sh +++ b/plugins/github-mcp/shared/mcpserver_core.sh @@ -118,15 +118,19 @@ handle_tools_list() { # Rejects arguments that are not a JSON object, enforces `required` (every # listed field must be present), when the schema sets # `additionalProperties: false` rejects any field not in `properties`, -# enforces a declared scalar `type` (string, integer, number, boolean, array, +# enforces a declared `type` (string, integer, number, boolean, array, # object) on any present field, enforces a declared `pattern` against any # present string-valued field, enforces a declared array `items.type` and # `items.enum` against every element of a present array-valued field, and # rejects any present field whose schema declares an `enum` when the supplied -# value is not one of the declared values. Diagnostics take precedence in -# that order — missing, unknown, type, pattern, items, enum — so a value that -# fails more than one constraint is reported with the most fundamental defect -# first (a type mismatch is reported before an unrelated enum mismatch). +# value is not one of the declared values. A declared `type` — on a property +# or on `items` — is either one name or a list of alternatives, and a value +# satisfies it by matching any member; a list that is empty or carries a +# non-string member is malformed and left unenforced. Diagnostics take +# precedence in that order — missing, unknown, type, pattern, items, enum — so +# a value that fails more than one constraint is reported with the most +# fundamental defect first (a type mismatch is reported before an unrelated +# enum mismatch). # A tool with no entry in the tools list, or whose entry declares no # inputSchema, is not validated. A jq failure is a rejection and never a skip: # a validator that could not evaluate its input has not validated it, and @@ -165,21 +169,37 @@ validate_tool_arguments() { --argjson schema "$schema" \ --argjson args "$arguments" \ ' - # `want == "integer"` treats a whole-valued JSON number as satisfying - # it (JSON has no distinct integer type); every other `want` is a - # plain jq `type` comparison. + # A declared `type` is one name or a list of alternatives, so it is + # normalized to a list and one comparison serves both forms. + # `"integer"` treats a whole-valued JSON number as satisfying it (JSON + # has no distinct integer type); every other name is a plain jq `type` + # comparison. + def type_names(want): + if (want | type) == "array" then want else [want] end; def type_ok(want; val): - if want == "integer" then - (val | type) == "number" and (val == (val | floor)) - else - (val | type) == want - end; + any(type_names(want)[]; + if . == "integer" then + (val | type) == "number" and (val == (val | floor)) + else + (val | type) == . + end); + # Only reached after `type_ok` failed, so a number here has already + # failed every declared alternative: with `integer` offered it is + # necessarily non-integer and reads "number (non-integer)". A list + # offering `number` accepts every number, so the `number` conjunct + # cannot fire at either call site — it keeps the label correct if the + # function is ever called somewhere `type_ok` did not gate. def type_label(want; val): - if want == "integer" and (val | type) == "number" then + (type_names(want)) as $w + | if (val | type) == "number" + and ($w | index("integer")) != null + and ($w | index("number")) == null then "number (non-integer)" - else + else (val | type) - end; + end; + def type_expected(want): + type_names(want) | join(" or "); if ($args | type) != "object" then "Invalid arguments: expected a JSON object, got " + ($args | type) + "." @@ -194,10 +214,14 @@ validate_tool_arguments() { else [] end ) as $unknown | [ $present[] | . as $p | ($props[$p].type // empty) as $t - | select($t != null and ($t | type) == "string") + | select($t != null) + | (type_names($t)) as $tn + # A malformed `type` — an empty list, or one carrying a non-string + # member — is left unenforced rather than rejecting every value. + | select(($tn | length) > 0 and all($tn[]; type == "string")) | ($args[$p]) as $v | select((type_ok($t; $v)) | not) - | {p: $p, expected: $t, actual: type_label($t; $v), v: $v} + | {p: $p, expected: type_expected($t), actual: type_label($t; $v), v: $v} ] as $invalid_type | [ $present[] | . as $p | ($props[$p].pattern // empty) as $pat @@ -226,8 +250,11 @@ validate_tool_arguments() { | . as $entry | ($entry.value) as $ev | ($entry.key) as $idx - | if ($it != null and ($it | type) == "string" and (type_ok($it; $ev) | not)) then - {p: $p, index: $idx, issue: "type", expected: $it, actual: type_label($it; $ev), v: $ev} + | if ($it != null + and ((type_names($it)) as $itn + | ($itn | length) > 0 and all($itn[]; type == "string")) + and (type_ok($it; $ev) | not)) then + {p: $p, index: $idx, issue: "type", expected: type_expected($it), actual: type_label($it; $ev), v: $ev} elif ($ie != null and ($ie | index($ev)) == null) then {p: $p, index: $idx, issue: "enum", enum: $ie, v: $ev} else empty end