Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions crates/tui/src/mcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1236,14 +1236,15 @@ pub struct McpResourceTemplate {
/// simple (`{id}`), and reserved (`{+path}`) expansions cover the common MCP
/// resource templates. More elaborate operators remain listable but are not
/// callable until their expansion semantics are implemented exactly.
fn resource_uri_matches_template(uri: &str, template: &str) -> bool {
///
/// `None` is the fail-closed answer: a template this subset cannot express
/// matches nothing.
fn resource_template_pattern(template: &str) -> Option<String> {
let mut pattern = String::from("^");
let mut rest = template;
while let Some(start) = rest.find('{') {
pattern.push_str(&regex::escape(&rest[..start]));
let Some(end) = rest[start + 1..].find('}') else {
return false;
};
let end = rest[start + 1..].find('}')?;
let expression = &rest[start + 1..start + 1 + end];
let (reserved, variables) = match expression.strip_prefix('+') {
Some(variables) => (true, variables),
Expand All @@ -1257,7 +1258,7 @@ fn resource_uri_matches_template(uri: &str, template: &str) -> bool {
.all(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '_' | '-' | '.'))
})
{
return false;
return None;
}
let atom = if reserved { ".+" } else { "[^/?#]+" };
for (index, _) in variables.split(',').enumerate() {
Expand All @@ -1269,11 +1270,38 @@ fn resource_uri_matches_template(uri: &str, template: &str) -> bool {
rest = &rest[start + end + 2..];
}
if rest.contains('}') {
return false;
return None;
}
pattern.push_str(&regex::escape(rest));
pattern.push('$');
regex::Regex::new(&pattern).is_ok_and(|regex| regex.is_match(uri))
Some(pattern)
}

/// `template`'s anchored pattern, compiled once and reused.
///
/// This runs per URI per advertised template, while the template itself is
/// fixed by the server's listing, so compiling it on every call was pure
/// repetition. `None` still means "matches nothing" (#6213 T7).
fn compiled_resource_template(template: &str) -> Option<Arc<regex::Regex>> {
static CACHE: std::sync::OnceLock<

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[WARNING] Global template-regex cache is never evicted (unbounded memory retention)

compiled_resource_template inserts every distinct template string into a static HashMap that is never pruned, keeping both the key String and either a compiled Regex or a negative None. The key is server-supplied (uriTemplate from resources/templates/list), and listings can be re-fetched or a server reconnected with different templates, so a long-running TUI session retains one entry per template string ever observed — including templates belonging to servers that are no longer connected — for the life of the process. Triggering path: repeated listing refresh where the server returns varying uriTemplate values, or a server that advertises many templates; the map grows monotonically and its Regex values are retained forever. Impact: memory growth in a long-lived interactive process, with no bound and no way to release it. Smallest useful fix: bound or scope the cache (an LRU/capacity-limited structure, invalidation when a server's template listing is replaced/disconnected), instead of a process-global static keyed by arbitrary input.

std::sync::Mutex<HashMap<String, Option<Arc<regex::Regex>>>>,
> = std::sync::OnceLock::new();
let cache = CACHE.get_or_init(|| std::sync::Mutex::new(HashMap::new()));
let mut cache = cache
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner());
cache
.entry(template.to_string())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[INFO] Every cache lookup allocates a String key and takes the global mutex

cache.entry(template.to_string()) builds an owned String on every call, including hits, and does so while holding the process-wide Mutex; first-time compilations of other templates also run inside or_insert_with with the lock held, serialising them behind each other. This matters only because the change is explicitly a per-call hot-path optimisation: a cache.get(template) fast path (the HashMap<String, _> key is reachable by &str through Borrow<str>) would avoid the per-call allocation, and compiling outside the lock would keep unrelated lookups from blocking. The impact is small relative to the Regex::new that this PR removes, so this is informational, not a correctness defect.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider a cache.get(template) fast path before the entry(...) insert so cache hits avoid allocating a fresh String key, and consider bounding or invalidating the cache (per-server scope, capacity limit, or clear on listing refresh) so that template strings from servers that are gone are not retained for the process lifetime. Both require structural judgement, so no literal replacement is given.

.or_insert_with(|| {
resource_template_pattern(template)
.and_then(|pattern| regex::Regex::new(&pattern).ok())
.map(Arc::new)
})
.clone()
}
Comment on lines +1285 to +1301

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.

CACHE here is a process-lifetime static keyed by the raw uriTemplate string, with no eviction and no size cap. template.uri_template (McpResourceTemplate::uri_template, populated in discover_resource_templates) is server-controlled content — a single discovery call is bounded to 4,096 items by McpCatalogBudget, but that budget resets every time a server (re)connects, and this cache never resets or shrinks. A misbehaving/malicious MCP server that varies its advertised templates across reconnects (or a long session that connects to many different servers) grows this map for the life of the process with no bound.

This crate already has the exact solved pattern for "small cache of things derived from untrusted/remote input" in crates/tui/src/tools/web/cache.rs: an lru::LruCache (already a workspace dependency, lru = "0.18" in Cargo.toml) behind a parking_lot::Mutex (already imported into this very file as RwLock), with a fixed entry cap. Reusing that shape here — a bounded LruCache<String, Option<Arc<Regex>>> — would cap worst-case memory instead of leaving it open-ended, and would also drop the std::sync::Mutex poisoning-recovery boilerplate (unwrap_or_else(|poisoned| poisoned.into_inner())) since parking_lot::Mutex doesn't poison.

Fix this →


fn resource_uri_matches_template(uri: &str, template: &str) -> bool {
compiled_resource_template(template).is_some_and(|regex| regex.is_match(uri))
}

/// Prompt discovered from an MCP server
Expand Down
40 changes: 40 additions & 0 deletions crates/tui/src/mcp/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8149,3 +8149,43 @@ async fn mcp_ceiling_preserves_ordinary_tool_result_tools_field() {
}
}
}

/// #6213 T7: the resource-URI template check is an authorization decision that
/// runs per URI per advertised template. Pin what it accepts, what it refuses,
/// and that the anchored pattern is compiled once rather than per call.
#[test]
fn resource_uri_template_matching_is_anchored_and_fail_closed() {
// Literal templates are anchored: no suffix may sneak past.
assert!(resource_uri_matches_template(
"file:///readme",
"file:///readme"
));
assert!(!resource_uri_matches_template(
"file:///readme/extra",
"file:///readme"
));

// `{id}` is a simple expansion, so it must not cross a path separator.
assert!(resource_uri_matches_template("file:///a", "file:///{id}"));
assert!(!resource_uri_matches_template(
"file:///a/b",
"file:///{id}"
));

// `{+path}` is a reserved expansion, so it may.
assert!(resource_uri_matches_template(
"file:///a/b/c",
"file:///{+path}"
));

// An operator this subset does not implement, and a template that never
// closes its expression, both stay uncallable rather than over-matching.
assert!(!resource_uri_matches_template("x", "x{?query}"));
assert!(!resource_uri_matches_template("x", "x{id"));

// The compile happens once per template and is reused.
let first = compiled_resource_template("file:///{path}").expect("template compiles");
let second = compiled_resource_template("file:///{path}").expect("template compiles");
assert!(Arc::ptr_eq(&first, &second));
assert!(compiled_resource_template("x{?query}").is_none());
}
Loading