Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add MCP bridge tool allowlists#2139

Merged
senamakel merged 1 commit into
tinyhumansai:maintinyhumansai/openhuman:mainfrom
vaddisrinivas:codex/mcp-tool-allowlistsvaddisrinivas/openhuman:codex/mcp-tool-allowlistsCopy head branch name to clipboard
May 20, 2026
Merged

Add MCP bridge tool allowlists#2139
senamakel merged 1 commit into
tinyhumansai:maintinyhumansai/openhuman:mainfrom
vaddisrinivas:codex/mcp-tool-allowlistsvaddisrinivas/openhuman:codex/mcp-tool-allowlistsCopy head branch name to clipboard

Conversation

@vaddisrinivas

@vaddisrinivas vaddisrinivas commented May 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add per-server allowed_tools / disallowed_tools config for the generic MCP bridge.
  • Filter mcp_list_tools output through the configured allowlist.
  • Reject blocked mcp_call_tool requests before remote transport dispatch.
  • Include allowlist state in mcp_list_servers diagnostics.

Validation

  • cargo fmt --manifest-path Cargo.toml
  • cargo test --manifest-path Cargo.toml mcp

Refs #2134

Summary by CodeRabbit

Release Notes

  • New Features
    • Added tool-level access control for MCP servers through allow and deny lists. Disallowed tools are blocked from execution and take precedence over allowed tools.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Tool-level access control was added to MCP servers via configuration-driven allowlists and denylists. McpServerConfig and McpServerDefinition now track allowed and disallowed tool names. Registry operations (list_tools, call_tool) enforce these rules. Server listing and tests were updated accordingly.

Changes

Tool Access Control Implementation

Layer / File(s) Summary
Config schema and server definition
src/openhuman/config/schema/tools.rs, src/openhuman/mcp_client/registry.rs
McpServerConfig and McpServerDefinition now each define allowed_tools and disallowed_tools fields. McpServerDefinition exposes is_tool_allowed and filter_allowed_tools methods to enforce allowlist/denylist logic.
Tool filtering and normalization helpers
src/openhuman/mcp_client/registry.rs
Helper function normalize_tool_names trims whitespace and deduplicates tool names while preserving order.
Registry operation enforcement and integration
src/openhuman/mcp_client/registry.rs
list_tools post-filters allowed tools; call_tool validates the tool is allowed before invoking transport and errors if disallowed. Config registration normalizes tool names. Legacy gitbooks definition explicitly sets empty lists.
Server listing output updates
src/openhuman/tools/impl/network/mcp.rs
mcp_list_servers JSON payload and markdown rendering now include allowed_tools and disallowed_tools, conditionally rendering tool list sections when non-empty.
Test coverage for access control
src/openhuman/mcp_client/registry.rs
Unit test verifies filter_allowed_tools correctly filters by allowlist and excludes disallowed items. Async test validates call_tool rejects disallowed tools with appropriate error message.
Integration test updates
src/openhuman/tools/ops_tests.rs
MCP server config fixture updated with empty allowed_tools and disallowed_tools fields.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly Related Issues

Suggested Labels

working

Suggested Reviewers

  • senamakel

Poem

🐰 Hops into the tool garden with a careful eye,
Allowed and disallowed lists stand guard nearby,
MCP servers now choose which tools to share,
Access control blooms—a thoughtful, safe affair!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add MCP bridge tool allowlists' accurately and concisely describes the main change: adding allowlist/denylist configuration for MCP bridge tools across multiple components.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@vaddisrinivas
vaddisrinivas marked this pull request as ready for review May 20, 2026 00:40
@vaddisrinivas
vaddisrinivas requested a review from a team May 20, 2026 00:40
@coderabbitai coderabbitai Bot added the working A PR that is being worked on by the team. label May 20, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/openhuman/mcp_client/registry.rs (1)

135-141: 💤 Low value

Consider adding a debug log when blocking disallowed tools.

Per coding guidelines, error paths benefit from diagnostic logging. A tracing::debug! here would help operators trace tool-blocking decisions without needing to surface the error through call stacks.

🔧 Suggested enhancement
 let tool = tool.trim();
 if !server.is_tool_allowed(tool) {
+    tracing::debug!(
+        server = %server.name,
+        tool = %tool,
+        "[mcp_client] blocking disallowed tool call"
+    );
     anyhow::bail!(
         "MCP tool `{tool}` is not allowed for server `{}`",
         server.name
     );
 }

As per coding guidelines: "Use log / tracing at debug or trace level on ... error paths."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/mcp_client/registry.rs` around lines 135 - 141, Add a
debug-level tracing log when a tool is rejected so operators can trace blocking
decisions: inside the block where you call server.is_tool_allowed(tool) and
before calling anyhow::bail!, emit a tracing::debug! (or tracing::trace! per
verbosity) that includes the tool and server.name (and optionally caller context
if available) to record why the path is taken without changing the error
behavior in the is_tool_allowed / bail branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/openhuman/mcp_client/registry.rs`:
- Around line 135-141: Add a debug-level tracing log when a tool is rejected so
operators can trace blocking decisions: inside the block where you call
server.is_tool_allowed(tool) and before calling anyhow::bail!, emit a
tracing::debug! (or tracing::trace! per verbosity) that includes the tool and
server.name (and optionally caller context if available) to record why the path
is taken without changing the error behavior in the is_tool_allowed / bail
branch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2e47c4a6-9d11-4402-8377-9d36f229abe3

📥 Commits

Reviewing files that changed from the base of the PR and between 0b053c5 and a309fd0.

📒 Files selected for processing (4)
  • src/openhuman/config/schema/tools.rs
  • src/openhuman/mcp_client/registry.rs
  • src/openhuman/tools/impl/network/mcp.rs
  • src/openhuman/tools/ops_tests.rs

@senamakel
senamakel merged commit ba40be1 into tinyhumansai:main May 20, 2026
28 of 31 checks passed
mtkik pushed a commit to mtkik/openhuman-meet that referenced this pull request May 21, 2026
CodeGhost21 pushed a commit to CodeGhost21/openhuman that referenced this pull request May 22, 2026
AusAgentSmith pushed a commit to AusAgentSmith/openhuman that referenced this pull request May 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

working A PR that is being worked on by the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.