Skip to content

Commit d74fa87

Browse files
committed
fix(gateway): workspace-root routing & meta-tool review findings
Resolver / normalization: - Roots arriving empty (`Some([])`) now fall through to client grants instead of stranding a granted-but-folderless client on PendingRoots forever. - Propagate grant-lookup DB errors instead of silently denying. - normalize_workspace_root: case-fold the whole Windows/UNC path, only percent-decode when a `file://` scheme was present (idempotent), reconstruct drive-letter / UNC hosts, and collapse doubled leading slashes before a drive (`//d:/…` → `d:\…`). Fixes bindings silently never matching across clients that report the same folder differently. Trust model for client-asserted roots (HIGH-1, accepted) documented in the module. Notifier: - WorkspaceBindingChanged and SpaceDeleted fan out to ALL sessions (sessions moved out of the event's space were never reached by the space-filtered fanout); FeatureSetDeleted notifies its space. - Throttle now DEFERS to the window end instead of dropping forced notifications. Handlers / routing: - Bridge the init roots-probe race on call_tool / get_prompt / read_resource so the list==call invariant holds for a resumed roots-capable session. - FeatureSet composition cycles: visited-set in resolve_members + transitive cycle reject in add_feature_set_member (was a user-creatable tools/list DoS). - Redact secrets from server logs (stdio args, resolved env values, tool-call argument values). - PoolService per-(space,server) connect mutex closes a double-connect race; PrefixCache builds the table offline and swaps it in under one lock. Meta-tools: - Gate debug auto-approve behind `debug_assertions` so release builds can't disable the approval dialog. - bind_current_workspace rejects a FeatureSet from a different Space; create_feature_set rejects empty/whitespace names. Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com>
1 parent 3c7fea1 commit d74fa87

15 files changed

Lines changed: 694 additions & 200 deletions

File tree

apps/desktop/src-tauri/src/commands/feature_set.rs

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,25 @@ pub async fn add_feature_set_member(
364364
));
365365
}
366366
}
367+
368+
// Prevent INDIRECT composition cycles (A⊇B then B⊇A, or longer
369+
// chains). Direct self-reference is caught above; here we walk the
370+
// candidate child's member graph and reject if it can transitively
371+
// reach this feature set. Without this the resolver would loop on
372+
// every list/call (it now breaks cycles defensively, but persisting
373+
// one is still invalid state). Bounded by visited-set dedup.
374+
if reaches_feature_set(
375+
&state,
376+
&input.member_id,
377+
&feature_set_id,
378+
&mut std::collections::HashSet::new(),
379+
)
380+
.await
381+
{
382+
return Err(
383+
"Cannot add this feature set: it would create a composition cycle".to_string(),
384+
);
385+
}
367386
}
368387

369388
let member = FeatureSetMember {
@@ -523,3 +542,41 @@ pub async fn set_feature_set_members(
523542

524543
Ok(feature_set.into())
525544
}
545+
546+
/// Does `start_fs_id` transitively compose `target_fs_id` (i.e. would adding
547+
/// `start_fs_id` as a member of `target_fs_id` close a cycle)?
548+
///
549+
/// Walks the composition graph via `FeatureSet` members of type
550+
/// `FeatureSet`, depth-first, deduping with `visited`. Repository read
551+
/// errors and missing sets are treated as "no path" — they can't form a
552+
/// cycle, and the resolver breaks any residual cycle defensively.
553+
fn reaches_feature_set<'a>(
554+
state: &'a AppState,
555+
start_fs_id: &'a str,
556+
target_fs_id: &'a str,
557+
visited: &'a mut std::collections::HashSet<String>,
558+
) -> std::pin::Pin<Box<dyn std::future::Future<Output = bool> + Send + 'a>> {
559+
Box::pin(async move {
560+
if start_fs_id == target_fs_id {
561+
return true;
562+
}
563+
if !visited.insert(start_fs_id.to_string()) {
564+
return false;
565+
}
566+
let Ok(Some(fs)) = state
567+
.feature_set_repository
568+
.get_with_members(start_fs_id)
569+
.await
570+
else {
571+
return false;
572+
};
573+
for member in &fs.members {
574+
if member.member_type == MemberType::FeatureSet
575+
&& reaches_feature_set(state, &member.member_id, target_fs_id, visited).await
576+
{
577+
return true;
578+
}
579+
}
580+
false
581+
})
582+
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,12 @@ impl DomainEvent {
476476
// A Space's built-in-server config changes the tool list every
477477
// session resolving to that Space sees.
478478
Self::BuiltinServerConfigChanged { .. } => true,
479+
// Deleting a Space cascade-removes its bindings; deleting a
480+
// FeatureSet strips its tools from every binding referencing it.
481+
// Both leave live sessions holding stale tool lists unless we push
482+
// list_changed.
483+
Self::SpaceDeleted { .. } => true,
484+
Self::FeatureSetDeleted { .. } => true,
479485
// WorkspaceNeedsBinding is a UI prompt — doesn't itself change what
480486
// tools a client sees, just invites the user to configure.
481487
// All other events don't affect MCP capabilities

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

Lines changed: 192 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -114,13 +114,16 @@ fn detect_style(path: &str) -> Option<PathStyle> {
114114
/// on the host OS. Same input always yields the same output.
115115
///
116116
/// Rules:
117-
/// * Strip `file://` / `file:///` scheme (tolerating an optional host).
118-
/// * URL-decode percent escapes.
117+
/// * Strip `file://` / `file:///` scheme (case-insensitive, tolerating an
118+
/// optional host) and percent-decode — but ONLY when a scheme was
119+
/// actually present, so the function is idempotent on plain paths that
120+
/// contain a literal `%xx` (e.g. a folder named `proj%20demo`).
119121
/// * On Windows-style paths:
120-
/// - Lowercase the drive letter (`D:` → `d:`).
122+
/// - Case-fold the WHOLE path (Windows filesystems are
123+
/// case-insensitive, so `D:\Foo` and `d:\foo` are the same folder).
121124
/// - Use `\` as the separator throughout (`d:/foo` → `d:\foo`).
122125
/// - Strip trailing separators but keep `c:\` as the root form.
123-
/// * On POSIX paths: strip trailing `/` but keep `/` alone.
126+
/// * On POSIX paths: strip trailing `/` but keep `/` alone (case-sensitive).
124127
/// * On empty input: return empty string (callers filter).
125128
pub fn normalize_workspace_root(input: &str) -> String {
126129
if input.is_empty() {
@@ -148,32 +151,79 @@ pub fn normalize_workspace_root(input: &str) -> String {
148151
}
149152

150153
fn strip_scheme_and_decode(input: &str) -> String {
151-
let without_scheme = if let Some(rest) = input.strip_prefix("file://") {
152-
// Triple-slash form `file:///abs` → `rest` = `/abs`. Host form
153-
// `file://localhost/abs` → drop up to the first `/`.
154-
match rest.find('/') {
155-
Some(0) => rest.to_string(),
156-
Some(n) => rest[n..].to_string(),
157-
None => rest.to_string(),
158-
}
159-
} else {
160-
input.to_string()
154+
// Match `file://` case-insensitively (RFC 3986 schemes are
155+
// case-insensitive) without allocating unless it actually matches.
156+
let scheme_len = input
157+
.get(..7)
158+
.filter(|p| p.eq_ignore_ascii_case("file://"))
159+
.map(|_| 7);
160+
161+
let Some(scheme_len) = scheme_len else {
162+
// No scheme: NOT a URI — return verbatim. Crucially we do NOT
163+
// percent-decode here, so re-normalizing an already-normalized plain
164+
// path (or one whose folder name legitimately contains `%xx`) is a
165+
// no-op. (Idempotency: normalize(normalize(x)) == normalize(x).)
166+
return input.to_string();
161167
};
162168

169+
let rest = &input[scheme_len..];
170+
let without_scheme = reconstruct_uri_path(rest);
171+
172+
// Scheme WAS present, so percent escapes are URI encoding — decode them.
163173
urlencoding::decode(&without_scheme)
164174
.map(|s| s.into_owned())
165175
.unwrap_or(without_scheme)
166176
}
167177

168-
fn strip_leading_slash_before_drive(path: &str) -> String {
169-
let rest = match path.strip_prefix('/') {
170-
Some(r) => r,
171-
None => return path.to_string(),
178+
/// Turn the part of a `file://` URI after the scheme into a filesystem path,
179+
/// preserving drive letters and UNC hosts that the naive "drop everything
180+
/// before the first slash" approach used to discard.
181+
fn reconstruct_uri_path(rest: &str) -> String {
182+
// Triple-slash form `file:///abs` → rest = `/abs`: no host component.
183+
if rest.starts_with('/') {
184+
return rest.to_string();
185+
}
186+
187+
// Authority form `file://<host>[/<path>]`. Split off the host.
188+
let (host, path) = match rest.find('/') {
189+
Some(n) => (&rest[..n], &rest[n..]), // path keeps its leading '/'
190+
None => (rest, ""),
172191
};
173-
let bytes = rest.as_bytes();
192+
193+
// `file://C:/Users/x` — the "host" is really a drive letter. Keep it.
194+
let host_bytes = host.as_bytes();
195+
let host_is_drive =
196+
host_bytes.len() == 2 && host_bytes[0].is_ascii_alphabetic() && host_bytes[1] == b':';
197+
if host_is_drive {
198+
return format!("{host}{path}");
199+
}
200+
201+
// Empty or local host → ordinary local path (`file:///abs` equivalent).
202+
if host.is_empty() || host.eq_ignore_ascii_case("localhost") {
203+
return if path.is_empty() {
204+
"/".to_string()
205+
} else {
206+
path.to_string()
207+
};
208+
}
209+
210+
// A real remote host → UNC path `\\host\share\...`. Emit the `\\host`
211+
// prefix; normalize_windows_unc converts the remaining separators.
212+
format!("\\\\{host}{path}")
213+
}
214+
215+
fn strip_leading_slash_before_drive(path: &str) -> String {
216+
// Strip ALL leading separators before a drive letter, not just one: a
217+
// `file://` URI for a Windows path can arrive with a doubled slash
218+
// (`file:////D:/x` → `//D:/x`), which `detect_style` would otherwise read
219+
// as a UNC path and mangle to `\\d:\x`. A genuine UNC path
220+
// (`//server/share`) has a non-drive first component, so trimming then
221+
// checking for `X:` leaves it untouched.
222+
let trimmed = path.trim_start_matches(['/', '\\']);
223+
let bytes = trimmed.as_bytes();
174224
let looks_like_drive = bytes.len() >= 2 && bytes[0].is_ascii_alphabetic() && bytes[1] == b':';
175225
if looks_like_drive {
176-
rest.to_string()
226+
trimmed.to_string()
177227
} else {
178228
path.to_string()
179229
}
@@ -189,12 +239,11 @@ fn normalize_posix(path: &str) -> String {
189239
}
190240

191241
fn normalize_windows_drive(path: &str) -> String {
192-
// Lowercase the drive letter.
193-
let mut chars: Vec<char> = path.chars().collect();
194-
if !chars.is_empty() && chars[0].is_ascii_alphabetic() {
195-
chars[0] = chars[0].to_ascii_lowercase();
196-
}
197-
let mut s: String = chars.into_iter().collect();
242+
// Case-fold the WHOLE path, not just the drive letter: Windows
243+
// filesystems are case-insensitive, and binding lookup is exact string
244+
// equality, so `D:\Projects\Foo` and `d:\projects\foo` must collapse to
245+
// one key or the binding silently never matches the session root.
246+
let mut s = path.to_lowercase();
198247

199248
// Convert every `/` to `\` for canonical Windows form.
200249
s = s.replace('/', "\\");
@@ -213,8 +262,10 @@ fn normalize_windows_drive(path: &str) -> String {
213262
}
214263

215264
fn normalize_windows_unc(path: &str) -> String {
216-
// `\\server\share\path` — normalize separators to `\` and strip trailing `\`.
217-
let s = path.replace('/', "\\");
265+
// `\\server\share\path` — case-fold (UNC server/share names are
266+
// case-insensitive, same rationale as drive paths), normalize separators
267+
// to `\`, and strip the trailing `\`.
268+
let s = path.to_lowercase().replace('/', "\\");
218269
let trimmed = s.trim_end_matches('\\');
219270
// Preserve the leading `\\` prefix.
220271
if trimmed.len() < 2 {
@@ -366,26 +417,134 @@ mod tests {
366417
#[test]
367418
fn normalize_windows_plain_on_any_host() {
368419
// Normalization runs the same everywhere — cfg(windows) isn't involved.
420+
// The WHOLE path is case-folded (Windows is case-insensitive), not
421+
// just the drive letter, so bindings match regardless of casing.
369422
assert_eq!(
370423
normalize_workspace_root("D:\\Projects\\Foo"),
371-
"d:\\Projects\\Foo"
424+
"d:\\projects\\foo"
372425
);
373-
assert_eq!(normalize_workspace_root("C:/work/proj"), "c:\\work\\proj");
426+
assert_eq!(normalize_workspace_root("C:/Work/Proj"), "c:\\work\\proj");
374427
}
375428

376429
#[test]
377430
fn normalize_windows_file_uri_on_any_host() {
378431
assert_eq!(
379432
normalize_workspace_root("file:///D:/Projects/Foo"),
380-
"d:\\Projects\\Foo"
433+
"d:\\projects\\foo"
381434
);
382435
}
383436

384437
#[test]
385-
fn normalize_windows_drive_letter_case_insensitive() {
438+
fn normalize_windows_case_insensitive_full_path() {
439+
// Same folder, different casing anywhere in the path → one key.
386440
assert_eq!(
387441
normalize_workspace_root("D:\\Projects\\Foo"),
388-
normalize_workspace_root("d:\\Projects\\Foo")
442+
normalize_workspace_root("d:\\PROJECTS\\foo")
443+
);
444+
// UNC server/share names are case-insensitive too.
445+
assert_eq!(
446+
normalize_workspace_root("\\\\SERVER\\Share\\Dir"),
447+
normalize_workspace_root("\\\\server\\share\\dir")
448+
);
449+
}
450+
451+
#[test]
452+
fn normalize_is_idempotent() {
453+
// normalize(normalize(x)) == normalize(x) for every shape, including
454+
// plain paths whose folder name legitimately contains a `%xx` (must
455+
// NOT be percent-decoded when there was no file:// scheme).
456+
for input in [
457+
"/home/user/proj",
458+
"/home/user/my%20proj",
459+
"D:\\Projects\\Foo",
460+
"C:/work/proj/",
461+
"\\\\server\\share\\dir",
462+
"file:///D:/Projects/My%20App",
463+
"file:///home/user/my%20project",
464+
] {
465+
let once = normalize_workspace_root(input);
466+
let twice = normalize_workspace_root(&once);
467+
assert_eq!(once, twice, "not idempotent for {input:?}");
468+
}
469+
}
470+
471+
#[test]
472+
fn normalize_plain_percent_is_not_decoded() {
473+
// A real folder named `proj%20demo` (no scheme) keeps its literal %.
474+
assert_eq!(
475+
normalize_workspace_root("d:\\proj%20demo"),
476+
"d:\\proj%20demo"
477+
);
478+
assert_eq!(
479+
normalize_workspace_root("/home/user/proj%20demo"),
480+
"/home/user/proj%20demo"
481+
);
482+
}
483+
484+
#[test]
485+
fn normalize_file_uri_scheme_case_insensitive() {
486+
assert_eq!(
487+
normalize_workspace_root("FILE:///home/user/proj"),
488+
"/home/user/proj"
489+
);
490+
}
491+
492+
#[test]
493+
fn normalize_file_uri_drive_letter_host() {
494+
// Nonstandard `file://C:/...` — the "host" is really a drive letter.
495+
assert_eq!(
496+
normalize_workspace_root("file://C:/Users/x"),
497+
"c:\\users\\x"
498+
);
499+
}
500+
501+
#[test]
502+
fn normalize_file_uri_localhost_host() {
503+
assert_eq!(
504+
normalize_workspace_root("file://localhost/home/user/proj"),
505+
"/home/user/proj"
506+
);
507+
assert_eq!(
508+
normalize_workspace_root("file://localhost/D:/work"),
509+
"d:\\work"
510+
);
511+
}
512+
513+
#[test]
514+
fn normalize_file_uri_unc_host_reconstructed() {
515+
// Standard UNC file URI keeps the host as the UNC server.
516+
assert_eq!(
517+
normalize_workspace_root("file://server/share/dir"),
518+
"\\\\server\\share\\dir"
519+
);
520+
}
521+
522+
#[test]
523+
fn normalize_doubled_slash_before_drive_is_not_unc() {
524+
// Regression (live manual test): a doubled leading slash before a
525+
// drive letter — from a `file:////D:/x` URI or a `//D:/x` path — must
526+
// collapse to a drive path, NOT be misread as a UNC path `\\d:\x`
527+
// (which then never matches the `d:\x` other clients report).
528+
let expected = "d:\\mcpmux\\mcp-mux";
529+
assert_eq!(normalize_workspace_root("//d:/mcpmux/mcp-mux"), expected);
530+
assert_eq!(
531+
normalize_workspace_root("\\\\d:\\mcpmux\\mcp-mux"),
532+
expected
533+
);
534+
assert_eq!(
535+
normalize_workspace_root("file:////D:/mcpmux/mcp-mux"),
536+
expected
537+
);
538+
// All collapse to the SAME key as the canonical drive forms.
539+
assert_eq!(normalize_workspace_root("D:\\mcpmux\\mcp-mux"), expected);
540+
assert_eq!(
541+
normalize_workspace_root("file:///D:/mcpmux/mcp-mux"),
542+
expected
543+
);
544+
// A genuine UNC path (non-drive first component) is left intact.
545+
assert_eq!(
546+
normalize_workspace_root("//server/share"),
547+
"\\\\server\\share"
389548
);
390549
}
391550

crates/mcpmux-core/src/repository/mod.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -216,8 +216,9 @@ pub trait InboundMcpClientRepository: Send + Sync {
216216
/// Workspace binding repository trait
217217
///
218218
/// Bindings map normalized filesystem paths to FeatureSets on a per-Space basis.
219-
/// Matching is longest-prefix-wins; callers are expected to pass
220-
/// already-normalized paths (see [`crate::domain::normalize_workspace_root`]).
219+
/// Matching is EXACT (no ancestor/prefix inheritance — see `find_exact_for_roots`);
220+
/// callers are expected to pass already-normalized paths (see
221+
/// [`crate::domain::normalize_workspace_root`]).
221222
#[async_trait]
222223
pub trait WorkspaceBindingRepository: Send + Sync {
223224
/// List every binding across all Spaces.

0 commit comments

Comments
 (0)