Skip to content

Commit 014be24

Browse files
committed
fix(gateway): listed tools/prompts/resources are always callable
A tool could appear in tools/list yet be rejected on call ("not allowed by the current grants" — e.g. notion_notion-get-users). Root cause: the list path encodes names via ServerFeature::qualified_name() (alias-or-serverid + "_" + name), but the call path DECODED via the prefix-cache reverse lookup (split-on-"_" then get_server_for_prefix), which can be stale or fall back to the prefix string as server_id — so the (server_id, name) used to authorize didn''t match the listed feature. Authorize + route by matching the requested qualified name against the SAME resolved feature set the list path uses (identical encoding both ways), so "if it lists, it calls": - routing.call_tool: find by qualified_name() in resolve_feature_sets; take (server_id, tool_name) from the matched feature (no prefix-cache decode). - handler.get_prompt + read_resource: same treatment. Also refresh the workspace_binding module doc (exact-match, empty allowed). Regression test: mcp_flows::listed_tool_is_resolvable_by_qualified_name (hyphenated names + alias != server_id). Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com>
1 parent 1a5eb32 commit 014be24

4 files changed

Lines changed: 124 additions & 92 deletions

File tree

crates/mcpmux-core/src/domain/workspace_binding.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,15 @@
1111
//! all to `FeatureService::get_*_for_grants` which composes the union.
1212
//! This is what lets one folder layer e.g. `Read Only` + `Project-specific
1313
//! tools` without forcing the user to merge them into a single FS by hand.
14-
//! Empty `feature_set_ids` is rejected at validation time; storing one
15-
//! would be indistinguishable from "not bound" yet route via Tier 1.
14+
//! Empty `feature_set_ids` is allowed — a "no Space tools" mapping: the
15+
//! folder still routes to its Space (built-in servers apply per Space).
1616
//!
17-
//! Path handling is **platform-agnostic**. A binding written on Windows
18-
//! (`d:\work\proj`) has to match correctly on a Linux host that's just
19-
//! reading the DB (and vice versa). We detect the path style from the
20-
//! string itself — drive-letter prefix ⇒ Windows, leading `/` ⇒ POSIX —
21-
//! rather than from `cfg!(windows)`. Both separators are accepted for
22-
//! prefix matching regardless of the host OS.
17+
//! Resolution is an EXACT match on the normalized root — there is no
18+
//! ancestor/prefix inheritance (`d:\a\b` does not pick up a binding on
19+
//! `d:\a`). Path handling is still **platform-agnostic**: a binding written
20+
//! on Windows (`d:\work\proj`) must match on a Linux host reading the DB and
21+
//! vice versa, so we normalize from the string's own style (drive-letter ⇒
22+
//! Windows, leading `/` ⇒ POSIX) rather than from `cfg!(windows)`.
2323
2424
use chrono::{DateTime, Utc};
2525
use serde::{Deserialize, Serialize};

crates/mcpmux-gateway/src/mcp/handler.rs

Lines changed: 31 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -940,14 +940,11 @@ impl ServerHandler for McpMuxGatewayHandler {
940940
)
941941
.await?;
942942

943-
let (server_id, prompt_name) = self
944-
.services
945-
.pool_services
946-
.feature_service
947-
.parse_qualified_prompt_name(&space_id.to_string(), &params.name)
948-
.await
949-
.map_err(|e| McpError::invalid_params(format!("Invalid prompt name: {}", e), None))?;
950-
943+
// Authorize + route by matching the requested qualified name against
944+
// the resolved prompt set — the SAME encoding the list path uses
945+
// (ServerFeature::qualified_name). Guarantees "if it lists, it's
946+
// callable"; no dependency on the prefix-cache reverse lookup (which
947+
// could be stale and reject a listed prompt). Mirrors call_tool.
951948
let authorized_prompts = self
952949
.services
953950
.pool_services
@@ -958,16 +955,18 @@ impl ServerHandler for McpMuxGatewayHandler {
958955
McpError::internal_error(format!("Failed to verify authorization: {}", e), None)
959956
})?;
960957

961-
let is_authorized = authorized_prompts
958+
let (server_id, prompt_name) = match authorized_prompts
962959
.iter()
963-
.any(|p| p.server_id == server_id && p.feature_name == prompt_name && p.is_available);
964-
965-
if !is_authorized {
966-
return Err(McpError::invalid_params(
967-
format!("Prompt '{}' not authorized", params.name),
968-
None,
969-
));
970-
}
960+
.find(|p| p.is_available && p.qualified_name() == params.name)
961+
{
962+
Some(p) => (p.server_id.clone(), p.feature_name.clone()),
963+
None => {
964+
return Err(McpError::invalid_params(
965+
format!("Prompt '{}' not authorized", params.name),
966+
None,
967+
));
968+
}
969+
};
971970

972971
let result_value = self
973972
.services
@@ -1049,19 +1048,10 @@ impl ServerHandler for McpMuxGatewayHandler {
10491048
)
10501049
.await?;
10511050

1052-
let server_id = self
1053-
.services
1054-
.pool_services
1055-
.feature_service
1056-
.find_server_for_resource(&space_id.to_string(), &params.uri)
1057-
.await
1058-
.map_err(|e| {
1059-
McpError::internal_error(format!("Failed to resolve resource: {}", e), None)
1060-
})?
1061-
.ok_or_else(|| {
1062-
McpError::invalid_params(format!("Resource '{}' not found", params.uri), None)
1063-
})?;
1064-
1051+
// Authorize + route by matching the requested URI against the resolved
1052+
// resource set (resources are namespaced by URI, so qualified_name ==
1053+
// feature_name == uri). The server_id comes from the matched feature,
1054+
// so a listed resource is always readable. Mirrors call_tool / get_prompt.
10651055
let authorized_resources = self
10661056
.services
10671057
.pool_services
@@ -1072,16 +1062,18 @@ impl ServerHandler for McpMuxGatewayHandler {
10721062
McpError::internal_error(format!("Failed to verify authorization: {}", e), None)
10731063
})?;
10741064

1075-
let is_authorized = authorized_resources
1065+
let server_id = match authorized_resources
10761066
.iter()
1077-
.any(|r| r.server_id == server_id && r.feature_name == params.uri && r.is_available);
1078-
1079-
if !is_authorized {
1080-
return Err(McpError::invalid_params(
1081-
format!("Resource '{}' not authorized", params.uri),
1082-
None,
1083-
));
1084-
}
1067+
.find(|r| r.is_available && r.qualified_name() == params.uri)
1068+
{
1069+
Some(r) => r.server_id.clone(),
1070+
None => {
1071+
return Err(McpError::invalid_params(
1072+
format!("Resource '{}' not authorized", params.uri),
1073+
None,
1074+
));
1075+
}
1076+
};
10851077

10861078
let contents_values = self
10871079
.services

crates/mcpmux-gateway/src/pool/routing.rs

Lines changed: 30 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -189,59 +189,44 @@ impl RoutingService {
189189
) -> Result<ToolCallResult> {
190190
let space_id_str = space_id.to_string();
191191

192-
// 1. Find the server that provides this tool
193-
let (server_id, actual_tool_name) = self
194-
.feature_service
195-
.find_server_for_qualified_tool(&space_id_str, tool_name)
196-
.await?
197-
.ok_or_else(|| anyhow!("Tool '{}' not found", tool_name))?;
198-
199-
// 2. Check if the tool is allowed by grants
192+
// Authorize AND route in one step by matching the requested qualified
193+
// name against the resolved feature set — using the SAME encoding the
194+
// list path uses (`ServerFeature::qualified_name`). This guarantees
195+
// "if it lists, it calls": the (server_id, tool_name) we route to come
196+
// straight from the matched feature, so there's no dependency on the
197+
// prefix-cache reverse lookup, which could be stale and surface a
198+
// listed tool as "not allowed by the current grants".
200199
let allowed_features = self
201200
.feature_service
202201
.resolve_feature_sets(&space_id_str, feature_set_ids)
203202
.await?;
204203

205-
info!(
206-
"[RoutingService] Checking authorization for tool '{}' (server: {}, actual_name: {})",
207-
tool_name, server_id, actual_tool_name
208-
);
209-
info!(
210-
"[RoutingService] Feature sets to check: {:?}",
211-
feature_set_ids
212-
);
213-
info!(
214-
"[RoutingService] Total allowed features: {}",
215-
allowed_features.len()
216-
);
217-
218-
// Log all tool features for debugging
219-
let tool_features: Vec<_> = allowed_features
220-
.iter()
221-
.filter(|f| f.feature_type == FeatureType::Tool)
222-
.map(|f| format!("{}::{}", f.server_id, f.feature_name))
223-
.collect();
224-
info!("[RoutingService] Allowed tools: {:?}", tool_features);
225-
226-
let is_allowed = allowed_features.iter().any(|f| {
227-
f.feature_type == FeatureType::Tool
228-
&& f.server_id == server_id
229-
&& f.feature_name == actual_tool_name
230-
&& f.is_available
204+
let feature = allowed_features.iter().find(|f| {
205+
f.feature_type == FeatureType::Tool && f.is_available && f.qualified_name() == tool_name
231206
});
232207

233-
if !is_allowed {
234-
warn!(
235-
"[RoutingService] Tool '{}' NOT allowed. Looking for server_id='{}', feature_name='{}', is_available=true",
236-
tool_name, server_id, actual_tool_name
237-
);
238-
return Err(anyhow!(
239-
"Tool '{}' is not allowed by the current grants",
240-
tool_name
241-
));
242-
}
208+
let (server_id, actual_tool_name) = match feature {
209+
Some(f) => (f.server_id.clone(), f.feature_name.clone()),
210+
None => {
211+
let available = allowed_features
212+
.iter()
213+
.filter(|f| f.feature_type == FeatureType::Tool && f.is_available)
214+
.count();
215+
warn!(
216+
"[RoutingService] Tool '{}' not in the resolved feature set ({} tools available)",
217+
tool_name, available
218+
);
219+
return Err(anyhow!(
220+
"Tool '{}' is not allowed by the current grants",
221+
tool_name
222+
));
223+
}
224+
};
243225

244-
info!("[RoutingService] Tool '{}' is ALLOWED", tool_name);
226+
info!(
227+
"[RoutingService] Tool '{}' ALLOWED → server={}, tool={}",
228+
tool_name, server_id, actual_tool_name
229+
);
245230

246231
info!(
247232
"[RoutingService] Calling tool {} on server {}",

tests/rust/tests/integration/mcp_flows.rs

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,61 @@ async fn test_call_tool_unauthorized() {
242242
assert_eq!(tools.len(), 0, "No tools should be authorized");
243243
}
244244

245+
/// Regression: every tool that LISTS must be callable. call_tool now authorizes
246+
/// + routes by matching `qualified_name()` against the SAME resolved feature
247+
/// set the list path uses, so a listed tool always maps back to exactly one
248+
/// callable feature — no prefix-cache reverse-lookup that could reject it.
249+
/// Covers hyphenated tool names under an alias prefix (e.g.
250+
/// `notion_notion-get-users`), the exact shape that surfaced the bug.
251+
#[tokio::test]
252+
async fn listed_tool_is_resolvable_by_qualified_name() {
253+
let ctx = TestContext::new();
254+
ctx.register_server("notion-mcp-http", Some("notion")).await;
255+
ctx.add_feature("notion-mcp-http", "notion-get-users", FeatureType::Tool)
256+
.await;
257+
ctx.add_feature("notion-mcp-http", "notion-search", FeatureType::Tool)
258+
.await;
259+
// A second server with a hyphenated alias, to exercise alias≠server_id too.
260+
ctx.register_server("idsearch", Some("instant-domain-search"))
261+
.await;
262+
ctx.add_feature("idsearch", "check_domain_availability", FeatureType::Tool)
263+
.await;
264+
265+
let all = ctx.new_grant_everything_set().await;
266+
let all_id = ctx.add_feature_set(all).await;
267+
268+
let listed = ctx
269+
.service
270+
.get_tools_for_grants(&ctx.space_id, &[all_id.clone()])
271+
.await
272+
.unwrap();
273+
let resolved = ctx
274+
.service
275+
.resolve_feature_sets(&ctx.space_id, &[all_id])
276+
.await
277+
.unwrap();
278+
279+
// The hyphen-under-alias tool lists with the exact qualified name.
280+
assert!(listed
281+
.iter()
282+
.any(|t| t.qualified_name() == "notion_notion-get-users"));
283+
284+
// Every listed tool maps back to EXACTLY ONE callable feature by qualified
285+
// name — the invariant call_tool relies on for authorization + routing.
286+
for t in &listed {
287+
let qn = t.qualified_name();
288+
let matches: Vec<_> = resolved
289+
.iter()
290+
.filter(|f| {
291+
f.feature_type == FeatureType::Tool && f.is_available && f.qualified_name() == qn
292+
})
293+
.collect();
294+
assert_eq!(matches.len(), 1, "listed tool {qn} must map to one feature");
295+
assert_eq!(matches[0].feature_name, t.feature_name);
296+
assert_eq!(matches[0].server_id, t.server_id);
297+
}
298+
}
299+
245300
// ============================================================================
246301
// RESOURCES FLOW TESTS
247302
// ============================================================================

0 commit comments

Comments
 (0)