Repository navigation
fix(mcp-bridge): refuse undeclared arguments on every tool - #359
Merged
Merged
Conversation
Every tool's inputSchema declares additionalProperties:false, but the bridge only enforced it for the 13 routed tools. The argument gate now checks every tool in the full list, plus the deprecated coord_promote_to_supervisor alias against coord_promote_to_master's schema. Unlisted tools are refused as unknown before dispatch. Arguments the handlers already read are now declared: sender_role on coord_send and coord_send_gated, and role and capabilities on coord_register (as in the local-coord-mcp manifest). Only top-level argument names are checked; types, required fields and enums are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j8She9eTFx54r6aL6sCHP Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j8She9eTFx54r6aL6sCHP Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (19)
|
| Layer / File(s) | Summary |
|---|---|
Coordinator argument contracts mcp-bridge/lib/tools.js |
coord_register declares optional role and capabilities arguments. coord_send and coord_send_gated declare optional sender_role arguments. |
Dispatcher argument validation mcp-bridge/lib/dispatcher.js, CHANGELOG.adoc |
The dispatcher resolves coord_promote_to_supervisor through coord_promote_to_master. It rejects unknown tools with -32601 and invalid arguments with -32602 before dispatch. The changelog records these behaviours and the added arguments. |
Validation tests and test command mcp-bridge/tests/declared_args_test.js, mcp-bridge/tests/routing_args_test.js, package.json |
Tests cover declared and undeclared arguments, unknown tools, and rejection before dispatch. The routing test checks the declared sender_role value. The test command includes the new validation test file. |
Priority: ⬇️ Low
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Merge Risk: ⚪ Minimal · up to f3b48
The argument-validation change is ready to merge after normal checks; no actionable issue remains.
Security Architecture Review
Security architecture risk: 🔵 Low · up to f3b48
The change tightens accepted argument names while preserving existing routing controls. No introduced privilege escalation was established, but downstream identity-to-role enforcement remains unverified.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
- inferred — The contract change affects callers of every known tool through the existing transports. Coordinator role metadata reaches the configured coordinator service, while routed tools retain their existing cartridge selection. The comparison establishes no new service route or privilege grant; tenant, asset, credential, and deployment scope cannot be quantified from the available source.
Security Findings and Attack Paths
- observed — Caller-controlled role metadata participates in envelope validation and is forwarded to the coordinator. This behavior predates the PR. Whether an attacker could use it to gain authority remains unresolved because the downstream binding between registration tokens, persisted roles, and gated-send authorization was not available; this is not a verified introduced vulnerability.
Trust Boundaries and Controls
- observed — Declared argument names and envelope-contract validation are not authenticated-role binding. The checked-in coordinator manifest describes registration roles excluding master and separate promotion, but that fixture does not prove production enforcement. Envelope validation can also be skipped when its dependencies are unavailable unless strict enforcement is configured.
Resilience and Maintainability Implications
- observed — The compared coordinator forwarding and bridge path-claim lifecycle are unchanged. Claim annotations remain post-success; progress refreshes claims and outcome reporting releases them. Envelope rejection precedes forwarding, and network or non-JSON failures bypass post-response bookkeeping. The new role fields do not enter these bookkeeping branches; downstream concurrency, interruption, repetition, and recovery guarantees remain unverified.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (2 skipped: 2 … |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Title check | ✅ Passed | The title clearly and concisely describes the main change: refusing undeclared arguments on every MCP tool. |
| Description check | ✅ Passed | The description follows the repository template. It explains the purpose, changes, testing, scope, checklist status, and deferred checks. Unconfigured formatting and lint checks are clearly identified… |
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
📝 Generate docstrings
- Commit to this branch
- Create a new PR
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each tool’s declared fare,
And sends no extra arguments there.
New roles and skills join the schema’s list,
Unknown calls meet an error they can’t resist.
Tests hop through each path with care.
Comment @coderabbitai help to get the list of available commands.
🏁 path-claims benchCommit NumbersHost-dependent — compare deltas across commits, not absolute values. |
🔍 Hypatia Security ScanFindings: 111 issues detected
View findings[
{
"reason": "Job `sonarqube` in build.yml has no `timeout-minutes:` declaration. Default is 6 hours — a stuck codeload fetch or runner hang can burn budget. Add `timeout-minutes: 10` (or proportional).",
"type": "missing_timeout_minutes",
"file": ".github/workflows/build.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"recipe_id": "recipe-add-workflow-timeout-minutes",
"job": "sonarqube"
},
{
"reason": "Job `triage` in label-triage.yml has no `timeout-minutes:` declaration. Default is 6 hours — a stuck codeload fetch or runner hang can burn budget. Add `timeout-minutes: 10` (or proportional).",
"type": "missing_timeout_minutes",
"file": ".github/workflows/label-triage.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"recipe_id": "recipe-add-workflow-timeout-minutes",
"job": "triage"
},
{
"reason": "Job `sync` in labels.yml has no `timeout-minutes:` declaration. Default is 6 hours — a stuck codeload fetch or runner hang can burn budget. Add `timeout-minutes: 10` (or proportional).",
"type": "missing_timeout_minutes",
"file": ".github/workflows/labels.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"recipe_id": "recipe-add-workflow-timeout-minutes",
"job": "sync"
},
{
"reason": "Job `deploy` in pages-deploy.yml has no `timeout-minutes:` declaration. Default is 6 hours — a stuck codeload fetch or runner hang can burn budget. Add `timeout-minutes: 10` (or proportional).",
"type": "missing_timeout_minutes",
"file": ".github/workflows/pages-deploy.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"recipe_id": "recipe-add-workflow-timeout-minutes",
"job": "deploy"
},
{
"reason": "Step uses `peter-evans/repository-dispatch` with `token: ${{ secrets.FARM_DISPATCH_TOKEN }}` but has no `if: secrets.FARM_DISPATCH_TOKEN != ''` gate. On repos where the secret hasn't been propagated the action fails on every push, red-maining the repo. Add the step-level gate (or env+if pattern) so the missing-secret path is a clean skip instead of a red.",
"type": "secret_action_without_presence_gate",
"file": ".github/workflows/instant-sync.yml",
"action": "peter-evans/repository-dispatch",
"rule_module": "workflow_audit",
"severity": "high",
"fix_recipe": "add_secret_presence_gate"
},
{
"reason": "codeql.yml does not list `language: actions` in its matrix, but the repo has workflow files. CodeQL's `actions` language scans workflow YAML for injection and other CI/CD-specific weaknesses — every repo with workflows benefits. Add an entry to `matrix.include` with `language: actions` + `build-mode: none`.",
"type": "codeql_missing_actions_language",
"file": ".github/workflows/codeql.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"fix_recipe": "add_codeql_actions_language"
},
{
"line": 39,
"reason": "job in .github/workflows/labels.yml references `secrets.*` but does not install `step-security/harden-runner` — review outbound-egress monitoring",
"type": "RE001",
"file": ".github/workflows/labels.yml",
"action": "report",
"rule_module": "research_extensions",
"severity": "medium"
},
{
"line": 46,
"reason": "job in .github/workflows/push-email-notify.yml references `secrets.*` but does not install `step-security/harden-runner` — review outbound-egress monitoring",
"type": "RE001",
"file": ".github/workflows/push-email-notify.yml",
"action": "report",
"rule_module": "research_extensions",
"severity": "medium"
},
{
"line": 32,
"reason": "job in .github/workflows/build.yml references `secrets.*` but does not install `step-security/harden-runner` — review outbound-egress monitoring",
"type": "RE001",
"file": ".github/workflows/build.yml",
"action": "report",
"rule_module": "research_extensions",
"severity": "medium"
},
{
"line": 44,
"reason": "job in .github/workflows/container-publish.yml references `secrets.*` but does not install `step-security/harden-runner` — review outbound-egress monitoring",
"type": "RE001",
"file": ".github/workflows/container-publish.yml",
"action": "report",
"rule_module": "research_extensions",
"severity": "medium"
}
]Powered by Hypatia Neurosymbolic CI/CD Intelligence |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Every MCP tool's
inputSchemadeclaresadditionalProperties: false, but the bridge enforced that only for the 13 routed tools (browser/cloud/comms/ml, since #349). This PR enforces it for every tool in the full list, so a call carrying an argument its schema does not declare is refused with JSON-RPC-32602before dispatch.📌 New pins
Head SHA:
f3b48f2b1d7c18f9f8f85a6db3312fd59b761d78. No action, lockfile or container pins added or changed.Changes
mcp-bridge/lib/dispatcher.js:validateRoutedArgsbecomesvalidateDeclaredArgsand applies to every tool. The routing-key refusal for routed tools is kept. The deprecatedcoord_promote_to_supervisoralias is checked againstcoord_promote_to_master's schema, and a tool missing from the full list is refused as-32601 Unknown toolin the gate.mcp-bridge/lib/tools.js: declares arguments the handlers already read.sender_role(optional enum) oncoord_sendandcoord_send_gated.roleandcapabilitiesoncoord_register, copied from the local-coord-mcpcartridge.json, which already accepts them.mcp-bridge/tests/declared_args_test.js(new):tools/callcases (non-routed, coord, unknown tool).mcp-bridge/tests/routing_args_test.js: the old test asserted thatcoord_sendaccepts an undeclaredsender_role. It now asserts thatcoord_sendaccepts the declared argument.package.jsontestscript runs the new file.CHANGELOG.adochas a line under Unreleased.Scope: this checks top-level argument names only. Types,
requiredand enums are unchanged.RSR Quality Checklist
Required
bun test mcp-bridge/tests/gives 210 pass, 0 fail, andnpm run test(the repo'snode --testscript) gives 173 pass, 0 fail.mcp-bridge/. I matched the surrounding style.mcp-bridge/. Not run..jsonly, no new TypeScript or Python.unsafeblocks without// SAFETY:comments: not applicable, no Rust or Zig touched..envfiles included.As Applicable
.machine_readable/*.a2ml: not applicable; A2ML is retired (D308).CHANGELOG.adoc.TOPOLOGY.md: not applicable, architecture unchanged.CHANGELOGupdated.src/abi/orffi/zig/change.Pre-existing red checks (deferred)
Both checks below are also red on
mainat7dd5897d. This PR does not touch the code either one covers.governance / UUID v7 conformanceis deferred to governance / UUID v7 conformance is red on main: v5 UUID literal in .machine_readable/CLADE.a2ml #347.SonarQubeis deferred to Three pre-existing red non-required checks on main: Allowlist Preflight (empty GH_TOKEN), Workflow security linter (duplicatewithin abi-drift.yml), SonarQube (403) #338.Testing
bun test mcp-bridge/tests/: 210 pass, 0 fail.npm run test: 173 pass, 0 fail.if (!routingKey) return null;, which checks routed tools only). With that in place,declared_args_test.jsfails exactly 58 cases: 55 non-routed tools, the alias, and the two end-to-end refusals. 84 cases still pass. Then I restored the fix.args.<field>read inapi-clients.js(60 reads) is declared. I confirmed the check catches a missing field by planting one undeclared read.coord_registerfields now added.validateDeclaredArgsdirectly, so the 60/min rate limiter cannot change which error a refusal returns.🤖 Generated with Claude Code
https://claude.ai/code/session_019j8She9eTFx54r6aL6sCHP